From b9dd44fd79a1ed7ee8e7c7644ddbd803b6acfd9c Mon Sep 17 00:00:00 2001 From: Tom Rix Date: Mon, 19 Oct 2020 12:19:50 -0700 Subject: scsi: message: fusion: Remove unneeded break A break is not needed if it is preceded by a return. Link: https://lore.kernel.org/r/20201019191950.10244-1-trix@redhat.com Signed-off-by: Tom Rix Signed-off-by: Martin K. Petersen --- drivers/message/fusion/mptbase.c | 1 - 1 file changed, 1 deletion(-) (limited to 'drivers/message') diff --git a/drivers/message/fusion/mptbase.c b/drivers/message/fusion/mptbase.c index 9903e9660a38..3078fac34e51 100644 --- a/drivers/message/fusion/mptbase.c +++ b/drivers/message/fusion/mptbase.c @@ -473,7 +473,6 @@ mpt_turbo_reply(MPT_ADAPTER *ioc, u32 pa) mpt_free_msg_frame(ioc, mf); mb(); return; - break; } mr = (MPT_FRAME_HDR *) CAST_U32_TO_PTR(pa); break; -- cgit v1.2.3 From b8a5144370bc59dbb192b8f29298920ceadc3d1e Mon Sep 17 00:00:00 2001 From: Thomas Gleixner Date: Thu, 26 Nov 2020 14:29:51 +0100 Subject: scsi: message: fusion: Remove in_interrupt() usage in mpt_config() in_interrupt() is referenced all over the place in these drivers. Most of these references are comments which are outdated and wrong. Aside of that in_interrupt() is deprecated as it does not provide what the name suggests. It covers more than hard/soft interrupt servicing context and is semantically ill defined. >From reading the mpt_config() code and the history this is clearly a debug mechanism and should probably be replaced by might_sleep() or completely removed because such checks are already in the subsequent functions. Remove the in_interrupt() references and replace the usage in mpt_config() with might_sleep(). Link: https://lore.kernel.org/r/20201126132952.2287996-14-bigeasy@linutronix.de Cc: Sathya Prakash Cc: Sreekanth Reddy Cc: Suganath Prabu Subramani Cc: MPT-FusionLinux.pdl@broadcom.com Reviewed-by: Daniel Wagner Signed-off-by: Thomas Gleixner Signed-off-by: Sebastian Andrzej Siewior Signed-off-by: Martin K. Petersen --- drivers/message/fusion/mptbase.c | 14 ++------------ drivers/message/fusion/mptfc.c | 2 +- drivers/message/fusion/mptscsih.c | 2 +- drivers/message/fusion/mptspi.c | 2 +- 4 files changed, 5 insertions(+), 15 deletions(-) (limited to 'drivers/message') diff --git a/drivers/message/fusion/mptbase.c b/drivers/message/fusion/mptbase.c index 3078fac34e51..549797d0301d 100644 --- a/drivers/message/fusion/mptbase.c +++ b/drivers/message/fusion/mptbase.c @@ -57,7 +57,7 @@ #include #include #include -#include /* needed for in_interrupt() proto */ +#include #include #include #include @@ -6335,7 +6335,6 @@ SendEventAck(MPT_ADAPTER *ioc, EventNotificationReply_t *evnp) * Page header is updated. * * Returns 0 for success - * -EPERM if not allowed due to ISR context * -EAGAIN if no msg frames currently available * -EFAULT for non-successful reply or no reply (timeout) */ @@ -6353,19 +6352,10 @@ mpt_config(MPT_ADAPTER *ioc, CONFIGPARMS *pCfg) u8 page_type = 0, extend_page; unsigned long timeleft; unsigned long flags; - int in_isr; u8 issue_hard_reset = 0; u8 retry_count = 0; - /* Prevent calling wait_event() (below), if caller happens - * to be in ISR context, because that is fatal! - */ - in_isr = in_interrupt(); - if (in_isr) { - dcprintk(ioc, printk(MYIOC_s_WARN_FMT "Config request not allowed in ISR context!\n", - ioc->name)); - return -EPERM; - } + might_sleep(); /* don't send a config page during diag reset */ spin_lock_irqsave(&ioc->taskmgmt_lock, flags); diff --git a/drivers/message/fusion/mptfc.c b/drivers/message/fusion/mptfc.c index f92b0433f599..0484e9c15c09 100644 --- a/drivers/message/fusion/mptfc.c +++ b/drivers/message/fusion/mptfc.c @@ -50,7 +50,7 @@ #include #include #include /* for mdelay */ -#include /* needed for in_interrupt() proto */ +#include #include /* notifier code */ #include #include diff --git a/drivers/message/fusion/mptscsih.c b/drivers/message/fusion/mptscsih.c index a5ef9faf71c7..f441f433ad4d 100644 --- a/drivers/message/fusion/mptscsih.c +++ b/drivers/message/fusion/mptscsih.c @@ -52,7 +52,7 @@ #include #include #include /* for mdelay */ -#include /* needed for in_interrupt() proto */ +#include #include /* notifier code */ #include diff --git a/drivers/message/fusion/mptspi.c b/drivers/message/fusion/mptspi.c index eabc4de5816c..af0ce5611e4a 100644 --- a/drivers/message/fusion/mptspi.c +++ b/drivers/message/fusion/mptspi.c @@ -52,7 +52,7 @@ #include #include #include /* for mdelay */ -#include /* needed for in_interrupt() proto */ +#include #include /* notifier code */ #include #include -- cgit v1.2.3 From 817a7c996786f803a8b5528ca11a842eed88e01f Mon Sep 17 00:00:00 2001 From: Sebastian Andrzej Siewior Date: Thu, 26 Nov 2020 14:29:52 +0100 Subject: scsi: message: fusion: Remove in_interrupt() usage in mptsas_cleanup_fw_event_q() mptsas_cleanup_fw_event_q() uses in_interrupt() to determine if it is safe to cancel a worker item. Aside of that in_interrupt() is deprecated as it does not provide what the name suggests. It covers more than hard/soft interrupt servicing context and is semantically ill defined. Looking closer there are a few problems with the current construct: - It could be invoked from an interrupt handler / non-blocking context because cancel_delayed_work() has no such restriction. Also, mptsas_free_fw_event() has no such restriction. - The list is accessed unlocked. It may dequeue a valid work-item but at the time of invoking cancel_delayed_work() the memory may be released or reused because the worker has already run. mptsas_cleanup_fw_event_q() is invoked via mptsas_shutdown() which is always invoked from preemtible context on device shutdown. It is also invoked via mptsas_ioc_reset(, MPT_IOC_POST_RESET) which is a MptResetHandlers callback. The only caller here are mpt_SoftResetHandler(), mpt_HardResetHandler() and mpt_Soft_Hard_ResetHandler(). All these functions have a `sleepFlag' argument and each caller uses caller uses `CAN_SLEEP' here and according to current documentation: | @sleepFlag: Indicates if sleep or schedule must be called So it is safe to sleep. Add mptsas_hotplug_event::users member. Initialize it to one by default so mptsas_free_fw_event() will free the memory. mptsas_cleanup_fw_event_q() will increment its value for items it dequeues and then it may keep a pointer after dropping the lock. Invoke cancel_delayed_work_sync() to cancel the work item and wait if the worker is currently busy. Free the memory afterwards since it owns the last reference to it. Link: https://lore.kernel.org/r/20201126132952.2287996-15-bigeasy@linutronix.de Cc: Sathya Prakash Cc: Sreekanth Reddy Cc: Suganath Prabu Subramani Cc: MPT-FusionLinux.pdl@broadcom.com Reviewed-by: Daniel Wagner Signed-off-by: Sebastian Andrzej Siewior Signed-off-by: Martin K. Petersen --- drivers/message/fusion/mptsas.c | 45 ++++++++++++++++++++++++++++++++--------- drivers/message/fusion/mptsas.h | 1 + 2 files changed, 36 insertions(+), 10 deletions(-) (limited to 'drivers/message') diff --git a/drivers/message/fusion/mptsas.c b/drivers/message/fusion/mptsas.c index 18b91ea1a353..5eb0b3361e4e 100644 --- a/drivers/message/fusion/mptsas.c +++ b/drivers/message/fusion/mptsas.c @@ -289,6 +289,7 @@ mptsas_add_fw_event(MPT_ADAPTER *ioc, struct fw_event_work *fw_event, spin_lock_irqsave(&ioc->fw_event_lock, flags); list_add_tail(&fw_event->list, &ioc->fw_event_list); + fw_event->users = 1; INIT_DELAYED_WORK(&fw_event->work, mptsas_firmware_event_work); devtprintk(ioc, printk(MYIOC_s_DEBUG_FMT "%s: add (fw_event=0x%p)" "on cpuid %d\n", ioc->name, __func__, @@ -314,6 +315,15 @@ mptsas_requeue_fw_event(MPT_ADAPTER *ioc, struct fw_event_work *fw_event, spin_unlock_irqrestore(&ioc->fw_event_lock, flags); } +static void __mptsas_free_fw_event(MPT_ADAPTER *ioc, + struct fw_event_work *fw_event) +{ + devtprintk(ioc, printk(MYIOC_s_DEBUG_FMT "%s: kfree (fw_event=0x%p)\n", + ioc->name, __func__, fw_event)); + list_del(&fw_event->list); + kfree(fw_event); +} + /* free memory associated to a sas firmware event */ static void mptsas_free_fw_event(MPT_ADAPTER *ioc, struct fw_event_work *fw_event) @@ -321,10 +331,9 @@ mptsas_free_fw_event(MPT_ADAPTER *ioc, struct fw_event_work *fw_event) unsigned long flags; spin_lock_irqsave(&ioc->fw_event_lock, flags); - devtprintk(ioc, printk(MYIOC_s_DEBUG_FMT "%s: kfree (fw_event=0x%p)\n", - ioc->name, __func__, fw_event)); - list_del(&fw_event->list); - kfree(fw_event); + fw_event->users--; + if (!fw_event->users) + __mptsas_free_fw_event(ioc, fw_event); spin_unlock_irqrestore(&ioc->fw_event_lock, flags); } @@ -333,9 +342,10 @@ mptsas_free_fw_event(MPT_ADAPTER *ioc, struct fw_event_work *fw_event) static void mptsas_cleanup_fw_event_q(MPT_ADAPTER *ioc) { - struct fw_event_work *fw_event, *next; + struct fw_event_work *fw_event; struct mptsas_target_reset_event *target_reset_list, *n; MPT_SCSI_HOST *hd = shost_priv(ioc->sh); + unsigned long flags; /* flush the target_reset_list */ if (!list_empty(&hd->target_reset_list)) { @@ -350,14 +360,29 @@ mptsas_cleanup_fw_event_q(MPT_ADAPTER *ioc) } } - if (list_empty(&ioc->fw_event_list) || - !ioc->fw_event_q || in_interrupt()) + if (list_empty(&ioc->fw_event_list) || !ioc->fw_event_q) return; - list_for_each_entry_safe(fw_event, next, &ioc->fw_event_list, list) { - if (cancel_delayed_work(&fw_event->work)) - mptsas_free_fw_event(ioc, fw_event); + spin_lock_irqsave(&ioc->fw_event_lock, flags); + + while (!list_empty(&ioc->fw_event_list)) { + bool canceled = false; + + fw_event = list_first_entry(&ioc->fw_event_list, + struct fw_event_work, list); + fw_event->users++; + spin_unlock_irqrestore(&ioc->fw_event_lock, flags); + if (cancel_delayed_work_sync(&fw_event->work)) + canceled = true; + + spin_lock_irqsave(&ioc->fw_event_lock, flags); + if (canceled) + fw_event->users--; + fw_event->users--; + WARN_ON_ONCE(fw_event->users); + __mptsas_free_fw_event(ioc, fw_event); } + spin_unlock_irqrestore(&ioc->fw_event_lock, flags); } diff --git a/drivers/message/fusion/mptsas.h b/drivers/message/fusion/mptsas.h index e35b13891fe4..71abf3477495 100644 --- a/drivers/message/fusion/mptsas.h +++ b/drivers/message/fusion/mptsas.h @@ -107,6 +107,7 @@ struct mptsas_hotplug_event { struct fw_event_work { struct list_head list; struct delayed_work work; + int users; MPT_ADAPTER *ioc; u32 event; u8 retries; -- cgit v1.2.3