[PATCH 00/14] scsi: Remove in_interrupt() usage.

[PATCH 00/14] scsi: Remove in_interrupt() usage.

[Sebastian Andrzej Siewior]

Folks,

in the discussion about preempt count consistency across kernel
configurations:

 https://lore.kernel.org/r/20200914204209.256266093@linutronix.de/

it was concluded that the usage of in_interrupt() and related context
checks should be removed from non-core code.

This includes allocation mode (GFP_*) decisions and avoidance of code paths
which might sleep.
In the long run, usage of 'preemptible, in_*irq etc.' should be banned from
driver code completely.

This series addresses most of the SCSI subsystem.
The first three patches have Fixes tags and address bugs were noticed during
review.

Sebastian

[PATCH 01/14] scsi: pm80xx: Do not sleep in atomic context.

[Sebastian Andrzej Siewior]

From: "Ahmed S. Darwish" <a.darwish@linutronix.de>

hw_event_sas_phy_up() is used in hardirq/softirq context:

 pm8001_interrupt_handler_msix() || pm8001_interrupt_handler_intx() || pm8001_tasklet
   => PM8001_CHIP_DISP->isr() = pm80xx_chip_isr()
     => process_oq() [spin_lock_irqsave(&pm8001_ha->lock,)]
       => process_one_iomb()
         => mpi_hw_event()
           => hw_event_sas_phy_up()
             => msleep(200)

Revert the msleep() back to an mdelay() to avoid sleeping in atomic
context.

Fixes: 4daf1ef3c681 ("scsi: pm80xx: Convert 'long' mdelay to msleep")
Signed-off-by: Ahmed S. Darwish <a.darwish@linutronix.de>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Vikram Auradkar <auradkar@google.com>
Cc: Jack Wang <jinpu.wang@cloud.ionos.com>
---
 drivers/scsi/pm8001/pm80xx_hwi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/pm8001/pm80xx_hwi.c b/drivers/scsi/pm8001/pm80xx_hwi.c
index 69f8244539e04..7d838e316657c 100644
--- a/drivers/scsi/pm8001/pm80xx_hwi.c
+++ b/drivers/scsi/pm8001/pm80xx_hwi.c
@@ -3296,7 +3296,7 @@ hw_event_sas_phy_up(struct pm8001_hba_info *pm8001_ha, void *piomb)
 	pm8001_get_attached_sas_addr(phy, phy->sas_phy.attached_sas_addr);
 	spin_unlock_irqrestore(&phy->sas_phy.frame_rcvd_lock, flags);
 	if (pm8001_ha->flags == PM8001F_RUN_TIME)
-		msleep(200);/*delay a moment to wait disk to spinup*/
+		mdelay(200);/*delay a moment to wait disk to spinup*/
 	pm8001_bytes_dmaed(pm8001_ha, phy_id);
 }
 
-- 
2.29.2

Re: [PATCH 01/14] scsi: pm80xx: Do not sleep in atomic context.

[Jinpu Wang]

On Thu, Nov 26, 2020 at 2:30 PM Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
>
> From: "Ahmed S. Darwish" <a.darwish@linutronix.de>
>
> hw_event_sas_phy_up() is used in hardirq/softirq context:
>
>  pm8001_interrupt_handler_msix() || pm8001_interrupt_handler_intx() || pm8001_tasklet
>    => PM8001_CHIP_DISP->isr() = pm80xx_chip_isr()
>      => process_oq() [spin_lock_irqsave(&pm8001_ha->lock,)]
>        => process_one_iomb()
>          => mpi_hw_event()
>            => hw_event_sas_phy_up()
>              => msleep(200)
>
> Revert the msleep() back to an mdelay() to avoid sleeping in atomic
> context.
>
> Fixes: 4daf1ef3c681 ("scsi: pm80xx: Convert 'long' mdelay to msleep")
> Signed-off-by: Ahmed S. Darwish <a.darwish@linutronix.de>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Cc: Vikram Auradkar <auradkar@google.com>
> Cc: Jack Wang <jinpu.wang@cloud.ionos.com>
Indeed, thx for the fix.
Acked-by: Jack Wang <jinpu.wang@cloud.ionos.com>> ---
>  drivers/scsi/pm8001/pm80xx_hwi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/pm8001/pm80xx_hwi.c b/drivers/scsi/pm8001/pm80xx_hwi.c
> index 69f8244539e04..7d838e316657c 100644
> --- a/drivers/scsi/pm8001/pm80xx_hwi.c
> +++ b/drivers/scsi/pm8001/pm80xx_hwi.c
> @@ -3296,7 +3296,7 @@ hw_event_sas_phy_up(struct pm8001_hba_info *pm8001_ha, void *piomb)
>         pm8001_get_attached_sas_addr(phy, phy->sas_phy.attached_sas_addr);
>         spin_unlock_irqrestore(&phy->sas_phy.frame_rcvd_lock, flags);
>         if (pm8001_ha->flags == PM8001F_RUN_TIME)
> -               msleep(200);/*delay a moment to wait disk to spinup*/
> +               mdelay(200);/*delay a moment to wait disk to spinup*/
>         pm8001_bytes_dmaed(pm8001_ha, phy_id);
>  }
>
> --
> 2.29.2
>

[PATCH 02/14] scsi: hisi_sas: Remove preemptible().

[Sebastian Andrzej Siewior]

From: "Ahmed S. Darwish" <a.darwish@linutronix.de>

hisi_sas_task_exec() uses preemptible() to see if it's safe to block.
This does not work for CONFIG_PREEMPT_COUNT=n kernels in which
preemptible() always returns 0.

The problem is masked when enabling some of the common Kconfig.debug
options (like CONFIG_DEBUG_ATOMIC_SLEEP), as they implicitly enable the
preemption counter.

In general, driver leaf functions should not make logic decisions based
on the context they're called from. The caller should be the entity
responsible for explicitly indicating context.

Since hisi_sas_task_exec() already has a gfp_t flags parameter, use it
as the explicit context marker.

Fixes: 214e702d4b70 ("scsi: hisi_sas: Adjust task reject period during host reset")
Fixes: 550c0d89d52d ("scsi: hisi_sas: Replace in_softirq() check in hisi_sas_task_exec()")
Signed-off-by: Ahmed S. Darwish <a.darwish@linutronix.de>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Xiaofei Tan <tanxiaofei@huawei.com>
Cc: Xiang Chen <chenxiang66@hisilicon.com>
Cc: John Garry <john.garry@huawei.com>
---
 drivers/scsi/hisi_sas/hisi_sas_main.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c b/drivers/scsi/hisi_sas/hisi_sas_main.c
index c8dd8588f800e..06e65c461f027 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_main.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
@@ -585,13 +585,7 @@ static int hisi_sas_task_exec(struct sas_task *task, gfp_t gfp_flags,
 	dev = hisi_hba->dev;
 
 	if (unlikely(test_bit(HISI_SAS_REJECT_CMD_BIT, &hisi_hba->flags))) {
-		/*
-		 * For IOs from upper layer, it may already disable preempt
-		 * in the IO path, if disable preempt again in down(),
-		 * function schedule() will report schedule_bug(), so check
-		 * preemptible() before goto down().
-		 */
-		if (!preemptible())
+		if (!gfpflags_allow_blocking(gfp_flags))
 			return -EINVAL;
 
 		down(&hisi_hba->sem);
-- 
2.29.2

Re: [PATCH 02/14] scsi: hisi_sas: Remove preemptible().

[John Garry]

On 26/11/2020 13:29, Sebastian Andrzej Siewior wrote:
> From: "Ahmed S. Darwish"<a.darwish@linutronix.de>
> 
> hisi_sas_task_exec() uses preemptible() to see if it's safe to block.
> This does not work for CONFIG_PREEMPT_COUNT=n kernels in which
> preemptible() always returns 0.
> 
> The problem is masked when enabling some of the common Kconfig.debug
> options (like CONFIG_DEBUG_ATOMIC_SLEEP), as they implicitly enable the
> preemption counter.
> 
> In general, driver leaf functions should not make logic decisions based
> on the context they're called from. The caller should be the entity
> responsible for explicitly indicating context.
> 
> Since hisi_sas_task_exec() already has a gfp_t flags parameter, use it
> as the explicit context marker.
> 
> Fixes: 214e702d4b70 ("scsi: hisi_sas: Adjust task reject period during host reset")
> Fixes: 550c0d89d52d ("scsi: hisi_sas: Replace in_softirq() check in hisi_sas_task_exec()")
> Signed-off-by: Ahmed S. Darwish<a.darwish@linutronix.de>
> Signed-off-by: Sebastian Andrzej Siewior<bigeasy@linutronix.de>
> Cc: Xiaofei Tan<tanxiaofei@huawei.com>
> Cc: Xiang Chen<chenxiang66@hisilicon.com>
> Cc: John Garry<john.garry@huawei.com>

Acked-by: John Garry <john.garry@huawei.com>

[PATCH 03/14] scsi: qla4xxx: qla4_82xx_crb_win_lock(): Remove in_interrupt().

[Sebastian Andrzej Siewior]

From: "Ahmed S. Darwish" <a.darwish@linutronix.de>

qla4_82xx_crb_win_lock() spins on a certain hardware state until it's
updated. At the end of each spin, if in_interrupt() is true, it does 20
loops of cpu_relax(). Otherwise, it yields the CPU.

The in_interrupt() macro is ill-defined as it does not provide what the
name suggests, and it does not catch the intended use-case here.

qla4_82xx_crb_win_lock() is always invoked with scsi_qla_host::hw_lock
acquired, with disabled interrupts. If the caller is in process context,
as in qla4_82xx_need_reset_handler(), then in_interrupt() will return
false even though it is not allowed to call schedule().

Remove the in_interrupt() check.

Change qla4_82xx_crb_win_lock() specification to a purely atomic
function. Mark it as static, remove its forward declaration, and move it
above its callers. To avoid hammering the PCI bus while spinning, use a
10 micro-second delay instead of cpu_relax().

Fixes: f4f5df23bf72 ("[SCSI] qla4xxx: Added support for ISP82XX")
Signed-off-by: Ahmed S. Darwish <a.darwish@linutronix.de>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Nilesh Javali <njavali@marvell.com>
Cc: Manish Rangankar <mrangankar@marvell.com>
Cc: <GR-QLogic-Storage-Upstream@marvell.com>
---
 drivers/scsi/qla4xxx/ql4_glbl.h |  1 -
 drivers/scsi/qla4xxx/ql4_nx.c   | 63 +++++++++++++++------------------
 2 files changed, 29 insertions(+), 35 deletions(-)

diff --git a/drivers/scsi/qla4xxx/ql4_glbl.h b/drivers/scsi/qla4xxx/ql4_glbl.h
index b8f02210aeb03..ea60057b2e20a 100644
--- a/drivers/scsi/qla4xxx/ql4_glbl.h
+++ b/drivers/scsi/qla4xxx/ql4_glbl.h
@@ -114,7 +114,6 @@ irqreturn_t qla4_82xx_intr_handler(int irq, void *dev_id);
 void qla4_82xx_queue_iocb(struct scsi_qla_host *ha);
 void qla4_82xx_complete_iocb(struct scsi_qla_host *ha);
 
-int qla4_82xx_crb_win_lock(struct scsi_qla_host *);
 void qla4_82xx_crb_win_unlock(struct scsi_qla_host *);
 int qla4_82xx_pci_get_crb_addr_2M(struct scsi_qla_host *, ulong *);
 void qla4_82xx_wr_32(struct scsi_qla_host *, ulong, u32);
diff --git a/drivers/scsi/qla4xxx/ql4_nx.c b/drivers/scsi/qla4xxx/ql4_nx.c
index f1767b21076f7..da903a545b2c0 100644
--- a/drivers/scsi/qla4xxx/ql4_nx.c
+++ b/drivers/scsi/qla4xxx/ql4_nx.c
@@ -375,6 +375,35 @@ qla4_82xx_pci_set_crbwindow_2M(struct scsi_qla_host *ha, ulong *off)
 	*off = (*off & MASK(16)) + CRB_INDIRECT_2M + ha->nx_pcibase;
 }
 
+#define CRB_WIN_LOCK_TIMEOUT 100000000
+
+/*
+ * Context: atomic
+ */
+static int qla4_82xx_crb_win_lock(struct scsi_qla_host *ha)
+{
+	int done = 0, timeout = 0;
+
+	while (!done) {
+		/* acquire semaphore3 from PCI HW block */
+		done = qla4_82xx_rd_32(ha, QLA82XX_PCIE_REG(PCIE_SEM7_LOCK));
+		if (done == 1)
+			break;
+		if (timeout >= CRB_WIN_LOCK_TIMEOUT)
+			return -1;
+
+		timeout++;
+		udelay(10);
+	}
+	qla4_82xx_wr_32(ha, QLA82XX_CRB_WIN_LOCK_ID, ha->func_num);
+	return 0;
+}
+
+void qla4_82xx_crb_win_unlock(struct scsi_qla_host *ha)
+{
+	qla4_82xx_rd_32(ha, QLA82XX_PCIE_REG(PCIE_SEM7_UNLOCK));
+}
+
 void
 qla4_82xx_wr_32(struct scsi_qla_host *ha, ulong off, u32 data)
 {
@@ -475,40 +504,6 @@ int qla4_82xx_md_wr_32(struct scsi_qla_host *ha, uint32_t off, uint32_t data)
 	return rval;
 }
 
-#define CRB_WIN_LOCK_TIMEOUT 100000000
-
-int qla4_82xx_crb_win_lock(struct scsi_qla_host *ha)
-{
-	int i;
-	int done = 0, timeout = 0;
-
-	while (!done) {
-		/* acquire semaphore3 from PCI HW block */
-		done = qla4_82xx_rd_32(ha, QLA82XX_PCIE_REG(PCIE_SEM7_LOCK));
-		if (done == 1)
-			break;
-		if (timeout >= CRB_WIN_LOCK_TIMEOUT)
-			return -1;
-
-		timeout++;
-
-		/* Yield CPU */
-		if (!in_interrupt())
-			schedule();
-		else {
-			for (i = 0; i < 20; i++)
-				cpu_relax();    /*This a nop instr on i386*/
-		}
-	}
-	qla4_82xx_wr_32(ha, QLA82XX_CRB_WIN_LOCK_ID, ha->func_num);
-	return 0;
-}
-
-void qla4_82xx_crb_win_unlock(struct scsi_qla_host *ha)
-{
-	qla4_82xx_rd_32(ha, QLA82XX_PCIE_REG(PCIE_SEM7_UNLOCK));
-}
-
 #define IDC_LOCK_TIMEOUT 100000000
 
 /**
-- 
2.29.2

Re: [PATCH 03/14] scsi: qla4xxx: qla4_82xx_crb_win_lock(): Remove in_interrupt().

[Daniel Wagner]

On Thu, Nov 26, 2020 at 02:29:41PM +0100, Sebastian Andrzej Siewior wrote:
> From: "Ahmed S. Darwish" <a.darwish@linutronix.de>
> 
> qla4_82xx_crb_win_lock() spins on a certain hardware state until it's
> updated. At the end of each spin, if in_interrupt() is true, it does 20
> loops of cpu_relax(). Otherwise, it yields the CPU.
> 
> The in_interrupt() macro is ill-defined as it does not provide what the
> name suggests, and it does not catch the intended use-case here.
> 
> qla4_82xx_crb_win_lock() is always invoked with scsi_qla_host::hw_lock
> acquired, with disabled interrupts. If the caller is in process context,
> as in qla4_82xx_need_reset_handler(), then in_interrupt() will return
> false even though it is not allowed to call schedule().
> 
> Remove the in_interrupt() check.
> 
> Change qla4_82xx_crb_win_lock() specification to a purely atomic
> function. Mark it as static, remove its forward declaration, and move it
> above its callers. To avoid hammering the PCI bus while spinning, use a
> 10 micro-second delay instead of cpu_relax().
> 
> Fixes: f4f5df23bf72 ("[SCSI] qla4xxx: Added support for ISP82XX")
> Signed-off-by: Ahmed S. Darwish <a.darwish@linutronix.de>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Cc: Nilesh Javali <njavali@marvell.com>
> Cc: Manish Rangankar <mrangankar@marvell.com>
> Cc: <GR-QLogic-Storage-Upstream@marvell.com>

Reviewed-by: Daniel Wagner <dwagner@suse.de>

[PATCH 04/14] scsi: qla2xxx: qla82xx: Remove in_interrupt().

[Sebastian Andrzej Siewior]

From: "Ahmed S. Darwish" <a.darwish@linutronix.de>

qla82xx_idc_lock() spins on a certain hardware state until it's updated. At
the end of each spin, if in_interrupt() is true, it does 20 loops of
cpu_relax(). Otherwise, it yields the CPU.

While in_interrupt() is ill-defined and does not provide what the name
suggests, it is not needed here: qla82xx_idc_lock() is always called
from process context. Below is an analysis of its callers, in order of
appearance:

  - qla_nx.c: qla82xx_device_bootstrap(), only called by
    qla82xx_device_state_handler(), has multiple msleep()s.

  - qla_nx.c: qla82xx_need_qsnt_handler(), has one second msleep()

  - qla_nx.c: qla82xx_wait_for_state_change(), one second msleep()

  - qla_nx.c: qla82xx_need_reset_handler(), can sleep up to 10 seconds

  - qla_nx.c: qla82xx_device_state_handler(), has multiple msleep()s

  - qla_nx.c: qla82xx_abort_isp(), if it's a qla82xx controller, calls
    qla82xx_device_state_handler(), which sleeps. It's also bound to
    isp_operations ->abort_isp() hook, where all the callers are in
    process context.

  - qla_nx.c: qla82xx_beacon_on(), bound to isp_operations ->beacon_on()
    hook.  That hook is only called once, in a mutex locked context,
    from qla2x00_beacon_store().

  - qla_nx.c: qla82xx_beacon_off(), bound to isp_operations
    ->beacon_off() hook.  Like ->beacon_on(), it's only called once, in
    a mutex locked context, from qla2x00_beacon_store().

  - qla_nx.c: qla82xx_fw_dump(), calls qla2x00_wait_for_chip_reset(),
    which has msleep() in a loop. It is bound to isp_operations
    ->fw_dump() hook. That hook *is* called from atomic context at
    qla_isr.c by multiple interrupt handlers. Nonetheless, it's other
    controllers interrupt handlers, and not the qla82xx.

  - qla_attr.c: qla2x00_sysfs_write_fw_dump(), and
    qla2x00_sysfs_write_reset(), process-context sysfs ->write() hooks.

  - qla_os.c: qla2x00_probe_one(). PCI ->probe(), process context.

  - qla_os.c: qla2x00_clear_drv_active(), called solely from
    qla2x00_remove_one(), which is PCI ->remove() hook, process context.

  - qla_os.c: qla2x00_do_dpc(), kthread function, process context.

Remove the in_interrupt() check. Change qla82xx_idc_lock() specification
to a purely process-context function. Mark it with "Context: task, might
sleep".

Change qla82xx_idc_lock() implementation to sleep 100ms, instead of a
schedule(), for each spin. This is more deterministic, and it matches
the other qla models idc_lock() functions.

Signed-off-by: Ahmed S. Darwish <a.darwish@linutronix.de>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Nilesh Javali <njavali@marvell.com>
Cc: <GR-QLogic-Storage-Upstream@marvell.com>
---
 drivers/scsi/qla2xxx/qla_def.h |  5 +++++
 drivers/scsi/qla2xxx/qla_nx.c  | 25 +++++++++++--------------
 2 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_def.h b/drivers/scsi/qla2xxx/qla_def.h
index ed9b10f8537d6..fe3c0e2f1ce88 100644
--- a/drivers/scsi/qla2xxx/qla_def.h
+++ b/drivers/scsi/qla2xxx/qla_def.h
@@ -3296,8 +3296,10 @@ struct isp_operations {
 	void (*fw_dump)(struct scsi_qla_host *vha);
 	void (*mpi_fw_dump)(struct scsi_qla_host *, int);
 
+	/* Context: task, might sleep */
 	int (*beacon_on) (struct scsi_qla_host *);
 	int (*beacon_off) (struct scsi_qla_host *);
+
 	void (*beacon_blink) (struct scsi_qla_host *);
 
 	void *(*read_optrom)(struct scsi_qla_host *, void *,
@@ -3308,7 +3310,10 @@ struct isp_operations {
 	int (*get_flash_version) (struct scsi_qla_host *, void *);
 	int (*start_scsi) (srb_t *);
 	int (*start_scsi_mq) (srb_t *);
+
+	/* Context: task, might sleep */
 	int (*abort_isp) (struct scsi_qla_host *);
+
 	int (*iospace_config)(struct qla_hw_data *);
 	int (*initialize_adapter)(struct scsi_qla_host *);
 };
diff --git a/drivers/scsi/qla2xxx/qla_nx.c b/drivers/scsi/qla2xxx/qla_nx.c
index b3ba0de5d4fb8..b2017f1c35044 100644
--- a/drivers/scsi/qla2xxx/qla_nx.c
+++ b/drivers/scsi/qla2xxx/qla_nx.c
@@ -489,29 +489,26 @@ qla82xx_rd_32(struct qla_hw_data *ha, ulong off_in)
 	return data;
 }
 
-#define IDC_LOCK_TIMEOUT 100000000
+/*
+ * Context: task, might sleep
+ */
 int qla82xx_idc_lock(struct qla_hw_data *ha)
 {
-	int i;
-	int done = 0, timeout = 0;
+	const int delay_ms = 100, timeout_ms = 2000;
+	int done, total = 0;
 
-	while (!done) {
+	might_sleep();
+
+	while (true) {
 		/* acquire semaphore5 from PCI HW block */
 		done = qla82xx_rd_32(ha, QLA82XX_PCIE_REG(PCIE_SEM5_LOCK));
 		if (done == 1)
 			break;
-		if (timeout >= IDC_LOCK_TIMEOUT)
+		if (WARN_ON_ONCE(total >= timeout_ms))
 			return -1;
 
-		timeout++;
-
-		/* Yield CPU */
-		if (!in_interrupt())
-			schedule();
-		else {
-			for (i = 0; i < 20; i++)
-				cpu_relax();
-		}
+		total += delay_ms;
+		msleep(delay_ms);
 	}
 
 	return 0;
-- 
2.29.2

Re: [PATCH 04/14] scsi: qla2xxx: qla82xx: Remove in_interrupt().

[Daniel Wagner]

On Thu, Nov 26, 2020 at 02:29:42PM +0100, Sebastian Andrzej Siewior wrote:
> From: "Ahmed S. Darwish" <a.darwish@linutronix.de>
> 
> qla82xx_idc_lock() spins on a certain hardware state until it's updated. At
> the end of each spin, if in_interrupt() is true, it does 20 loops of
> cpu_relax(). Otherwise, it yields the CPU.
> 
> While in_interrupt() is ill-defined and does not provide what the name
> suggests, it is not needed here: qla82xx_idc_lock() is always called
> from process context. Below is an analysis of its callers, in order of
> appearance:
> 
>   - qla_nx.c: qla82xx_device_bootstrap(), only called by
>     qla82xx_device_state_handler(), has multiple msleep()s.
> 
>   - qla_nx.c: qla82xx_need_qsnt_handler(), has one second msleep()
> 
>   - qla_nx.c: qla82xx_wait_for_state_change(), one second msleep()
> 
>   - qla_nx.c: qla82xx_need_reset_handler(), can sleep up to 10 seconds
> 
>   - qla_nx.c: qla82xx_device_state_handler(), has multiple msleep()s
> 
>   - qla_nx.c: qla82xx_abort_isp(), if it's a qla82xx controller, calls
>     qla82xx_device_state_handler(), which sleeps. It's also bound to
>     isp_operations ->abort_isp() hook, where all the callers are in
>     process context.
> 
>   - qla_nx.c: qla82xx_beacon_on(), bound to isp_operations ->beacon_on()
>     hook.  That hook is only called once, in a mutex locked context,
>     from qla2x00_beacon_store().
> 
>   - qla_nx.c: qla82xx_beacon_off(), bound to isp_operations
>     ->beacon_off() hook.  Like ->beacon_on(), it's only called once, in
>     a mutex locked context, from qla2x00_beacon_store().
> 
>   - qla_nx.c: qla82xx_fw_dump(), calls qla2x00_wait_for_chip_reset(),
>     which has msleep() in a loop. It is bound to isp_operations
>     ->fw_dump() hook. That hook *is* called from atomic context at
>     qla_isr.c by multiple interrupt handlers. Nonetheless, it's other
>     controllers interrupt handlers, and not the qla82xx.

qla82xx_msix_default() and qla82xx_msix_rsp_q() call
qla24xx_process_response_queue() which doesn't implement the firmware
dumping.

>   - qla_attr.c: qla2x00_sysfs_write_fw_dump(), and
>     qla2x00_sysfs_write_reset(), process-context sysfs ->write() hooks.
> 
>   - qla_os.c: qla2x00_probe_one(). PCI ->probe(), process context.
> 
>   - qla_os.c: qla2x00_clear_drv_active(), called solely from
>     qla2x00_remove_one(), which is PCI ->remove() hook, process context.
> 
>   - qla_os.c: qla2x00_do_dpc(), kthread function, process context.
> 
> Remove the in_interrupt() check. Change qla82xx_idc_lock() specification
> to a purely process-context function. Mark it with "Context: task, might
> sleep".
> 
> Change qla82xx_idc_lock() implementation to sleep 100ms, instead of a
> schedule(), for each spin. This is more deterministic, and it matches
> the other qla models idc_lock() functions.
> 
> Signed-off-by: Ahmed S. Darwish <a.darwish@linutronix.de>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Cc: Nilesh Javali <njavali@marvell.com>
> Cc: <GR-QLogic-Storage-Upstream@marvell.com>

Reviewed-by: Daniel Wagner <dwagner@suse.de>

Re: [PATCH 04/14] scsi: qla2xxx: qla82xx: Remove in_interrupt().

[Himanshu Madhani]

> On Nov 26, 2020, at 7:29 AM, Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
> 
> From: "Ahmed S. Darwish" <a.darwish@linutronix.de>
> 
> qla82xx_idc_lock() spins on a certain hardware state until it's updated. At
> the end of each spin, if in_interrupt() is true, it does 20 loops of
> cpu_relax(). Otherwise, it yields the CPU.
> 
> While in_interrupt() is ill-defined and does not provide what the name
> suggests, it is not needed here: qla82xx_idc_lock() is always called
> from process context. Below is an analysis of its callers, in order of
> appearance:
> 
>  - qla_nx.c: qla82xx_device_bootstrap(), only called by
>    qla82xx_device_state_handler(), has multiple msleep()s.
> 
>  - qla_nx.c: qla82xx_need_qsnt_handler(), has one second msleep()
> 
>  - qla_nx.c: qla82xx_wait_for_state_change(), one second msleep()
> 
>  - qla_nx.c: qla82xx_need_reset_handler(), can sleep up to 10 seconds
> 
>  - qla_nx.c: qla82xx_device_state_handler(), has multiple msleep()s
> 
>  - qla_nx.c: qla82xx_abort_isp(), if it's a qla82xx controller, calls
>    qla82xx_device_state_handler(), which sleeps. It's also bound to
>    isp_operations ->abort_isp() hook, where all the callers are in
>    process context.
> 
>  - qla_nx.c: qla82xx_beacon_on(), bound to isp_operations ->beacon_on()
>    hook.  That hook is only called once, in a mutex locked context,
>    from qla2x00_beacon_store().
> 
>  - qla_nx.c: qla82xx_beacon_off(), bound to isp_operations
>    ->beacon_off() hook.  Like ->beacon_on(), it's only called once, in
>    a mutex locked context, from qla2x00_beacon_store().
> 
>  - qla_nx.c: qla82xx_fw_dump(), calls qla2x00_wait_for_chip_reset(),
>    which has msleep() in a loop. It is bound to isp_operations
>    ->fw_dump() hook. That hook *is* called from atomic context at
>    qla_isr.c by multiple interrupt handlers. Nonetheless, it's other
>    controllers interrupt handlers, and not the qla82xx.
> 
>  - qla_attr.c: qla2x00_sysfs_write_fw_dump(), and
>    qla2x00_sysfs_write_reset(), process-context sysfs ->write() hooks.
> 
>  - qla_os.c: qla2x00_probe_one(). PCI ->probe(), process context.
> 
>  - qla_os.c: qla2x00_clear_drv_active(), called solely from
>    qla2x00_remove_one(), which is PCI ->remove() hook, process context.
> 
>  - qla_os.c: qla2x00_do_dpc(), kthread function, process context.
> 
> Remove the in_interrupt() check. Change qla82xx_idc_lock() specification
> to a purely process-context function. Mark it with "Context: task, might
> sleep".
> 
> Change qla82xx_idc_lock() implementation to sleep 100ms, instead of a
> schedule(), for each spin. This is more deterministic, and it matches
> the other qla models idc_lock() functions.
> 
> Signed-off-by: Ahmed S. Darwish <a.darwish@linutronix.de>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Cc: Nilesh Javali <njavali@marvell.com>
> Cc: <GR-QLogic-Storage-Upstream@marvell.com>
> ---
> drivers/scsi/qla2xxx/qla_def.h |  5 +++++
> drivers/scsi/qla2xxx/qla_nx.c  | 25 +++++++++++--------------
> 2 files changed, 16 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/scsi/qla2xxx/qla_def.h b/drivers/scsi/qla2xxx/qla_def.h
> index ed9b10f8537d6..fe3c0e2f1ce88 100644
> --- a/drivers/scsi/qla2xxx/qla_def.h
> +++ b/drivers/scsi/qla2xxx/qla_def.h
> @@ -3296,8 +3296,10 @@ struct isp_operations {
> 	void (*fw_dump)(struct scsi_qla_host *vha);
> 	void (*mpi_fw_dump)(struct scsi_qla_host *, int);
> 
> +	/* Context: task, might sleep */
> 	int (*beacon_on) (struct scsi_qla_host *);
> 	int (*beacon_off) (struct scsi_qla_host *);
> +
> 	void (*beacon_blink) (struct scsi_qla_host *);
> 
> 	void *(*read_optrom)(struct scsi_qla_host *, void *,
> @@ -3308,7 +3310,10 @@ struct isp_operations {
> 	int (*get_flash_version) (struct scsi_qla_host *, void *);
> 	int (*start_scsi) (srb_t *);
> 	int (*start_scsi_mq) (srb_t *);
> +
> +	/* Context: task, might sleep */
> 	int (*abort_isp) (struct scsi_qla_host *);
> +
> 	int (*iospace_config)(struct qla_hw_data *);
> 	int (*initialize_adapter)(struct scsi_qla_host *);
> };
> diff --git a/drivers/scsi/qla2xxx/qla_nx.c b/drivers/scsi/qla2xxx/qla_nx.c
> index b3ba0de5d4fb8..b2017f1c35044 100644
> --- a/drivers/scsi/qla2xxx/qla_nx.c
> +++ b/drivers/scsi/qla2xxx/qla_nx.c
> @@ -489,29 +489,26 @@ qla82xx_rd_32(struct qla_hw_data *ha, ulong off_in)
> 	return data;
> }
> 
> -#define IDC_LOCK_TIMEOUT 100000000
> +/*
> + * Context: task, might sleep
> + */
> int qla82xx_idc_lock(struct qla_hw_data *ha)
> {
> -	int i;
> -	int done = 0, timeout = 0;
> +	const int delay_ms = 100, timeout_ms = 2000;
> +	int done, total = 0;
> 
> -	while (!done) {
> +	might_sleep();
> +
> +	while (true) {
> 		/* acquire semaphore5 from PCI HW block */
> 		done = qla82xx_rd_32(ha, QLA82XX_PCIE_REG(PCIE_SEM5_LOCK));
> 		if (done == 1)
> 			break;
> -		if (timeout >= IDC_LOCK_TIMEOUT)
> +		if (WARN_ON_ONCE(total >= timeout_ms))
> 			return -1;
> 
> -		timeout++;
> -
> -		/* Yield CPU */
> -		if (!in_interrupt())
> -			schedule();
> -		else {
> -			for (i = 0; i < 20; i++)
> -				cpu_relax();
> -		}
> +		total += delay_ms;
> +		msleep(delay_ms);
> 	}
> 
> 	return 0;
> -- 
> 2.29.2
> 

Looks Good. 

Reviewed-by: Himanshu Madhani <himanshu.madhani@oracle.com>

--
Himanshu Madhani	 Oracle Linux Engineering

[PATCH 05/14] scsi: qla2xxx: tcm_qla2xxx: Remove BUG_ON(in_interrupt()).

[Sebastian Andrzej Siewior]

From: "Ahmed S. Darwish" <a.darwish@linutronix.de>

tcm_qla2xxx_free_session() has a BUG_ON(in_interrupt()).

While in_interrupt() is ill-defined and does not provide what the name
suggests, it is not needed here: the function is always invoked from
workqueue context through "struct qla_tgt_func_tmpl" ->free_session()
hook it is bound to.

The function also calls wait_event_timeout() down the chain, which
already has a might_sleep().

Remove the in_interrupt() check.

Signed-off-by: Ahmed S. Darwish <a.darwish@linutronix.de>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Nilesh Javali <njavali@marvell.com>
Cc: <GR-QLogic-Storage-Upstream@marvell.com>
---
 drivers/scsi/qla2xxx/tcm_qla2xxx.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
index 784b43f181818..b55fc768a2a7a 100644
--- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
+++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
@@ -1400,8 +1400,6 @@ static void tcm_qla2xxx_free_session(struct fc_port *sess)
 	struct se_session *se_sess;
 	struct tcm_qla2xxx_lport *lport;
 
-	BUG_ON(in_interrupt());
-
 	se_sess = sess->se_sess;
 	if (!se_sess) {
 		pr_err("struct fc_port->se_sess is NULL\n");
-- 
2.29.2

Re: [PATCH 05/14] scsi: qla2xxx: tcm_qla2xxx: Remove BUG_ON(in_interrupt()).

[Daniel Wagner]

On Thu, Nov 26, 2020 at 02:29:43PM +0100, Sebastian Andrzej Siewior wrote:
> From: "Ahmed S. Darwish" <a.darwish@linutronix.de>
> 
> tcm_qla2xxx_free_session() has a BUG_ON(in_interrupt()).
> 
> While in_interrupt() is ill-defined and does not provide what the name
> suggests, it is not needed here: the function is always invoked from
> workqueue context through "struct qla_tgt_func_tmpl" ->free_session()
> hook it is bound to.
> 
> The function also calls wait_event_timeout() down the chain, which
> already has a might_sleep().
> 
> Remove the in_interrupt() check.
> 
> Signed-off-by: Ahmed S. Darwish <a.darwish@linutronix.de>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Cc: Nilesh Javali <njavali@marvell.com>
> Cc: <GR-QLogic-Storage-Upstream@marvell.com>

Reviewed-by: Daniel Wagner <dwagner@suse.de>

Re: [PATCH 05/14] scsi: qla2xxx: tcm_qla2xxx: Remove BUG_ON(in_interrupt()).

[Himanshu Madhani]

> On Nov 26, 2020, at 7:29 AM, Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
> 
> From: "Ahmed S. Darwish" <a.darwish@linutronix.de>
> 
> tcm_qla2xxx_free_session() has a BUG_ON(in_interrupt()).
> 
> While in_interrupt() is ill-defined and does not provide what the name
> suggests, it is not needed here: the function is always invoked from
> workqueue context through "struct qla_tgt_func_tmpl" ->free_session()
> hook it is bound to.
> 
> The function also calls wait_event_timeout() down the chain, which
> already has a might_sleep().
> 
> Remove the in_interrupt() check.
> 
> Signed-off-by: Ahmed S. Darwish <a.darwish@linutronix.de>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Cc: Nilesh Javali <njavali@marvell.com>
> Cc: <GR-QLogic-Storage-Upstream@marvell.com>
> ---
> drivers/scsi/qla2xxx/tcm_qla2xxx.c | 2 --
> 1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
> index 784b43f181818..b55fc768a2a7a 100644
> --- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
> +++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
> @@ -1400,8 +1400,6 @@ static void tcm_qla2xxx_free_session(struct fc_port *sess)
> 	struct se_session *se_sess;
> 	struct tcm_qla2xxx_lport *lport;
> 
> -	BUG_ON(in_interrupt());
> -
> 	se_sess = sess->se_sess;
> 	if (!se_sess) {
> 		pr_err("struct fc_port->se_sess is NULL\n");
> -- 
> 2.29.2
> 

Reviewed-by: Himanshu Madhani <himanshu.madhani@oracle.com>

--
Himanshu Madhani	 Oracle Linux Engineering

[PATCH 06/14] scsi: qla2xxx: init/os: Remove in_interrupt().

[Sebastian Andrzej Siewior]

From: "Ahmed S. Darwish" <a.darwish@linutronix.de>

qla83xx_wait_logic() is used to control the frequency of device IDC lock
retries. If in_interrupt() is true, it does 20 loops of cpu_relax().
Otherwise, it sleeps for 100ms and yields the CPU.

While in_interrupt() is ill-defined and does not provide what the name
suggests, it is not needed here: that qla83xx_wait_logic() is exclusively
called by qla83xx_idc_lock() / unlock(), and they always run from
process context. Below is an analysis of all the idc lock/unlock callers,
in order of appearance:

  - qla_os.c:
      qla83xx_nic_core_unrecoverable_work(),
      qla83xx_idc_state_handler_work(),
      qla83xx_nic_core_reset_work(),
      qla83xx_service_idc_aen(), all workqueue context

  - qla_os.c: qla83xx_check_nic_core_fw_alive(), has msleep()

  - qla_os.c: qla83xx_set_drv_presence(), called once from
    qla2x00_abort_isp(), which is bound to process-context ->abort_isp()
    hook. It also invokes wait_for_completion_timeout() through the
    chain qla2x00_configure_hba() => qla24xx_link_initialize() =>
    qla2x00_mailbox_command().

  - qla_os.c: qla83xx_clear_drv_presence(), which is called from
    qla2x00_abort_isp() discussed above, and from qla2x00_remove_one()
    which is PCI process-context ->remove() hook.

  - qla_os.c: qla83xx_need_reset_handler(), has a one second msleep() in
    a loop.

  - qla_os.c: qla83xx_device_bootstrap(), called only by
    qla83xx_idc_state_handler(), which has multiple msleep()
    invocations.

  - qla_os.c: qla83xx_idc_state_handler(), multiple msleep()
    invocations.

  - qla_attr.c: qla2x00_sysfs_write_reset(), sysfs bin_attribute
    ->write() hook, process context

  - qla_init.c: qla83xx_nic_core_fw_load()
      => qla_init.c: qla2x00_initialize_adapter()
        => bound to isp_operations ->initialize_adapter() hook
        ** => qla_os.c: qla2x00_probe_one(), PCI ->probe() process ctx

  - qla_init.c: qla83xx_initiating_reset(), msleep() in a loop.

  - qla_init.c: qla83xx_nic_core_reset(), called by
    qla83xx_nic_core_reset_work(), workqueue context.

Remove the in_interrupt() check, and thus replace the entirety of
qla83xx_wait_logic() with an msleep(QLA83XX_WAIT_LOGIC_MS).

Mark qla83xx_idc_lock() / unlock() with "Context: task, can sleep".

Signed-off-by: Ahmed S. Darwish <a.darwish@linutronix.de>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Nilesh Javali <njavali@marvell.com>
Cc: GR-QLogic-Storage-Upstream@marvell.com
---
 drivers/scsi/qla2xxx/qla_os.c | 43 ++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 24 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
index f9c8ae9d669ef..2a8e065b192c3 100644
--- a/drivers/scsi/qla2xxx/qla_os.c
+++ b/drivers/scsi/qla2xxx/qla_os.c
@@ -5619,25 +5619,10 @@ qla83xx_service_idc_aen(struct work_struct *work)
 	}
 }
 
-static void
-qla83xx_wait_logic(void)
-{
-	int i;
-
-	/* Yield CPU */
-	if (!in_interrupt()) {
-		/*
-		 * Wait about 200ms before retrying again.
-		 * This controls the number of retries for single
-		 * lock operation.
-		 */
-		msleep(100);
-		schedule();
-	} else {
-		for (i = 0; i < 20; i++)
-			cpu_relax(); /* This a nop instr on i386 */
-	}
-}
+/*
+ * Control the frequency of IDC lock retries
+ */
+#define QLA83XX_WAIT_LOGIC_MS	100
 
 static int
 qla83xx_force_lock_recovery(scsi_qla_host_t *base_vha)
@@ -5727,7 +5712,7 @@ qla83xx_idc_lock_recovery(scsi_qla_host_t *base_vha)
 		goto exit;
 
 	if (o_drv_lockid == n_drv_lockid) {
-		qla83xx_wait_logic();
+		msleep(QLA83XX_WAIT_LOGIC_MS);
 		goto retry_lockid;
 	} else
 		return QLA_SUCCESS;
@@ -5736,6 +5721,9 @@ qla83xx_idc_lock_recovery(scsi_qla_host_t *base_vha)
 	return rval;
 }
 
+/*
+ * Context: task, can sleep
+ */
 void
 qla83xx_idc_lock(scsi_qla_host_t *base_vha, uint16_t requester_id)
 {
@@ -5743,6 +5731,8 @@ qla83xx_idc_lock(scsi_qla_host_t *base_vha, uint16_t requester_id)
 	uint32_t lock_owner;
 	struct qla_hw_data *ha = base_vha->hw;
 
+	might_sleep();
+
 	/* IDC-lock implementation using driver-lock/lock-id remote registers */
 retry_lock:
 	if (qla83xx_rd_reg(base_vha, QLA83XX_DRIVER_LOCK, &data)
@@ -5761,7 +5751,7 @@ qla83xx_idc_lock(scsi_qla_host_t *base_vha, uint16_t requester_id)
 			/* Retry/Perform IDC-Lock recovery */
 			if (qla83xx_idc_lock_recovery(base_vha)
 			    == QLA_SUCCESS) {
-				qla83xx_wait_logic();
+				msleep(QLA83XX_WAIT_LOGIC_MS);
 				goto retry_lock;
 			} else
 				ql_log(ql_log_warn, base_vha, 0xb075,
@@ -6259,6 +6249,9 @@ void qla24xx_process_purex_list(struct purex_list *list)
 	}
 }
 
+/*
+ * Context: task, can sleep
+ */
 void
 qla83xx_idc_unlock(scsi_qla_host_t *base_vha, uint16_t requester_id)
 {
@@ -6269,6 +6262,8 @@ qla83xx_idc_unlock(scsi_qla_host_t *base_vha, uint16_t requester_id)
 	uint32_t data;
 	struct qla_hw_data *ha = base_vha->hw;
 
+	might_sleep();
+
 	/* IDC-unlock implementation using driver-unlock/lock-id
 	 * remote registers
 	 */
@@ -6284,7 +6279,7 @@ qla83xx_idc_unlock(scsi_qla_host_t *base_vha, uint16_t requester_id)
 			/* SV: XXX: IDC unlock retrying needed here? */
 
 			/* Retry for IDC-unlock */
-			qla83xx_wait_logic();
+			msleep(QLA83XX_WAIT_LOGIC_MS);
 			retry++;
 			ql_dbg(ql_dbg_p3p, base_vha, 0xb064,
 			    "Failed to release IDC lock, retrying=%d\n", retry);
@@ -6292,7 +6287,7 @@ qla83xx_idc_unlock(scsi_qla_host_t *base_vha, uint16_t requester_id)
 		}
 	} else if (retry < 10) {
 		/* Retry for IDC-unlock */
-		qla83xx_wait_logic();
+		msleep(QLA83XX_WAIT_LOGIC_MS);
 		retry++;
 		ql_dbg(ql_dbg_p3p, base_vha, 0xb065,
 		    "Failed to read drv-lockid, retrying=%d\n", retry);
@@ -6308,7 +6303,7 @@ qla83xx_idc_unlock(scsi_qla_host_t *base_vha, uint16_t requester_id)
 	if (qla83xx_access_control(base_vha, options, 0, 0, NULL)) {
 		if (retry < 10) {
 			/* Retry for IDC-unlock */
-			qla83xx_wait_logic();
+			msleep(QLA83XX_WAIT_LOGIC_MS);
 			retry++;
 			ql_dbg(ql_dbg_p3p, base_vha, 0xb066,
 			    "Failed to release IDC lock, retrying=%d\n", retry);
-- 
2.29.2

Re: [PATCH 06/14] scsi: qla2xxx: init/os: Remove in_interrupt().

[Daniel Wagner]

On Thu, Nov 26, 2020 at 02:29:44PM +0100, Sebastian Andrzej Siewior wrote:
> From: "Ahmed S. Darwish" <a.darwish@linutronix.de>
> 
> qla83xx_wait_logic() is used to control the frequency of device IDC lock
> retries. If in_interrupt() is true, it does 20 loops of cpu_relax().
> Otherwise, it sleeps for 100ms and yields the CPU.
> 
> While in_interrupt() is ill-defined and does not provide what the name
> suggests, it is not needed here: that qla83xx_wait_logic() is exclusively
> called by qla83xx_idc_lock() / unlock(), and they always run from
> process context. Below is an analysis of all the idc lock/unlock callers,
> in order of appearance:
> 
>   - qla_os.c:
>       qla83xx_nic_core_unrecoverable_work(),
>       qla83xx_idc_state_handler_work(),
>       qla83xx_nic_core_reset_work(),
>       qla83xx_service_idc_aen(), all workqueue context
> 
>   - qla_os.c: qla83xx_check_nic_core_fw_alive(), has msleep()
> 
>   - qla_os.c: qla83xx_set_drv_presence(), called once from
>     qla2x00_abort_isp(), which is bound to process-context ->abort_isp()
>     hook. It also invokes wait_for_completion_timeout() through the
>     chain qla2x00_configure_hba() => qla24xx_link_initialize() =>
>     qla2x00_mailbox_command().
> 
>   - qla_os.c: qla83xx_clear_drv_presence(), which is called from
>     qla2x00_abort_isp() discussed above, and from qla2x00_remove_one()
>     which is PCI process-context ->remove() hook.
> 
>   - qla_os.c: qla83xx_need_reset_handler(), has a one second msleep() in
>     a loop.
> 
>   - qla_os.c: qla83xx_device_bootstrap(), called only by
>     qla83xx_idc_state_handler(), which has multiple msleep()
>     invocations.
> 
>   - qla_os.c: qla83xx_idc_state_handler(), multiple msleep()
>     invocations.
> 
>   - qla_attr.c: qla2x00_sysfs_write_reset(), sysfs bin_attribute
>     ->write() hook, process context
> 
>   - qla_init.c: qla83xx_nic_core_fw_load()
>       => qla_init.c: qla2x00_initialize_adapter()
>         => bound to isp_operations ->initialize_adapter() hook
>         ** => qla_os.c: qla2x00_probe_one(), PCI ->probe() process ctx
> 
>   - qla_init.c: qla83xx_initiating_reset(), msleep() in a loop.
> 
>   - qla_init.c: qla83xx_nic_core_reset(), called by
>     qla83xx_nic_core_reset_work(), workqueue context.
> 
> Remove the in_interrupt() check, and thus replace the entirety of
> qla83xx_wait_logic() with an msleep(QLA83XX_WAIT_LOGIC_MS).
> 
> Mark qla83xx_idc_lock() / unlock() with "Context: task, can sleep".
> 
> Signed-off-by: Ahmed S. Darwish <a.darwish@linutronix.de>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Cc: Nilesh Javali <njavali@marvell.com>
> Cc: GR-QLogic-Storage-Upstream@marvell.com

Reviewed-by: Daniel Wagner <dwagner@suse.de>

Re: [PATCH 06/14] scsi: qla2xxx: init/os: Remove in_interrupt().

[Himanshu Madhani]

> On Nov 26, 2020, at 7:29 AM, Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
> 
> From: "Ahmed S. Darwish" <a.darwish@linutronix.de>
> 
> qla83xx_wait_logic() is used to control the frequency of device IDC lock
> retries. If in_interrupt() is true, it does 20 loops of cpu_relax().
> Otherwise, it sleeps for 100ms and yields the CPU.
> 
> While in_interrupt() is ill-defined and does not provide what the name
> suggests, it is not needed here: that qla83xx_wait_logic() is exclusively
> called by qla83xx_idc_lock() / unlock(), and they always run from
> process context. Below is an analysis of all the idc lock/unlock callers,
> in order of appearance:
> 
>  - qla_os.c:
>      qla83xx_nic_core_unrecoverable_work(),
>      qla83xx_idc_state_handler_work(),
>      qla83xx_nic_core_reset_work(),
>      qla83xx_service_idc_aen(), all workqueue context
> 
>  - qla_os.c: qla83xx_check_nic_core_fw_alive(), has msleep()
> 
>  - qla_os.c: qla83xx_set_drv_presence(), called once from
>    qla2x00_abort_isp(), which is bound to process-context ->abort_isp()
>    hook. It also invokes wait_for_completion_timeout() through the
>    chain qla2x00_configure_hba() => qla24xx_link_initialize() =>
>    qla2x00_mailbox_command().
> 
>  - qla_os.c: qla83xx_clear_drv_presence(), which is called from
>    qla2x00_abort_isp() discussed above, and from qla2x00_remove_one()
>    which is PCI process-context ->remove() hook.
> 
>  - qla_os.c: qla83xx_need_reset_handler(), has a one second msleep() in
>    a loop.
> 
>  - qla_os.c: qla83xx_device_bootstrap(), called only by
>    qla83xx_idc_state_handler(), which has multiple msleep()
>    invocations.
> 
>  - qla_os.c: qla83xx_idc_state_handler(), multiple msleep()
>    invocations.
> 
>  - qla_attr.c: qla2x00_sysfs_write_reset(), sysfs bin_attribute
>    ->write() hook, process context
> 
>  - qla_init.c: qla83xx_nic_core_fw_load()
>      => qla_init.c: qla2x00_initialize_adapter()
>        => bound to isp_operations ->initialize_adapter() hook
>        ** => qla_os.c: qla2x00_probe_one(), PCI ->probe() process ctx
> 
>  - qla_init.c: qla83xx_initiating_reset(), msleep() in a loop.
> 
>  - qla_init.c: qla83xx_nic_core_reset(), called by
>    qla83xx_nic_core_reset_work(), workqueue context.
> 
> Remove the in_interrupt() check, and thus replace the entirety of
> qla83xx_wait_logic() with an msleep(QLA83XX_WAIT_LOGIC_MS).
> 
> Mark qla83xx_idc_lock() / unlock() with "Context: task, can sleep".
> 
> Signed-off-by: Ahmed S. Darwish <a.darwish@linutronix.de>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Cc: Nilesh Javali <njavali@marvell.com>
> Cc: GR-QLogic-Storage-Upstream@marvell.com
> ---
> drivers/scsi/qla2xxx/qla_os.c | 43 ++++++++++++++++-------------------
> 1 file changed, 19 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
> index f9c8ae9d669ef..2a8e065b192c3 100644
> --- a/drivers/scsi/qla2xxx/qla_os.c
> +++ b/drivers/scsi/qla2xxx/qla_os.c
> @@ -5619,25 +5619,10 @@ qla83xx_service_idc_aen(struct work_struct *work)
> 	}
> }
> 
> -static void
> -qla83xx_wait_logic(void)
> -{
> -	int i;
> -
> -	/* Yield CPU */
> -	if (!in_interrupt()) {
> -		/*
> -		 * Wait about 200ms before retrying again.
> -		 * This controls the number of retries for single
> -		 * lock operation.
> -		 */
> -		msleep(100);
> -		schedule();
> -	} else {
> -		for (i = 0; i < 20; i++)
> -			cpu_relax(); /* This a nop instr on i386 */
> -	}
> -}
> +/*
> + * Control the frequency of IDC lock retries
> + */
> +#define QLA83XX_WAIT_LOGIC_MS	100
> 
> static int
> qla83xx_force_lock_recovery(scsi_qla_host_t *base_vha)
> @@ -5727,7 +5712,7 @@ qla83xx_idc_lock_recovery(scsi_qla_host_t *base_vha)
> 		goto exit;
> 
> 	if (o_drv_lockid == n_drv_lockid) {
> -		qla83xx_wait_logic();
> +		msleep(QLA83XX_WAIT_LOGIC_MS);
> 		goto retry_lockid;
> 	} else
> 		return QLA_SUCCESS;
> @@ -5736,6 +5721,9 @@ qla83xx_idc_lock_recovery(scsi_qla_host_t *base_vha)
> 	return rval;
> }
> 
> +/*
> + * Context: task, can sleep
> + */
> void
> qla83xx_idc_lock(scsi_qla_host_t *base_vha, uint16_t requester_id)
> {
> @@ -5743,6 +5731,8 @@ qla83xx_idc_lock(scsi_qla_host_t *base_vha, uint16_t requester_id)
> 	uint32_t lock_owner;
> 	struct qla_hw_data *ha = base_vha->hw;
> 
> +	might_sleep();
> +
> 	/* IDC-lock implementation using driver-lock/lock-id remote registers */
> retry_lock:
> 	if (qla83xx_rd_reg(base_vha, QLA83XX_DRIVER_LOCK, &data)
> @@ -5761,7 +5751,7 @@ qla83xx_idc_lock(scsi_qla_host_t *base_vha, uint16_t requester_id)
> 			/* Retry/Perform IDC-Lock recovery */
> 			if (qla83xx_idc_lock_recovery(base_vha)
> 			    == QLA_SUCCESS) {
> -				qla83xx_wait_logic();
> +				msleep(QLA83XX_WAIT_LOGIC_MS);
> 				goto retry_lock;
> 			} else
> 				ql_log(ql_log_warn, base_vha, 0xb075,
> @@ -6259,6 +6249,9 @@ void qla24xx_process_purex_list(struct purex_list *list)
> 	}
> }
> 
> +/*
> + * Context: task, can sleep
> + */
> void
> qla83xx_idc_unlock(scsi_qla_host_t *base_vha, uint16_t requester_id)
> {
> @@ -6269,6 +6262,8 @@ qla83xx_idc_unlock(scsi_qla_host_t *base_vha, uint16_t requester_id)
> 	uint32_t data;
> 	struct qla_hw_data *ha = base_vha->hw;
> 
> +	might_sleep();
> +
> 	/* IDC-unlock implementation using driver-unlock/lock-id
> 	 * remote registers
> 	 */
> @@ -6284,7 +6279,7 @@ qla83xx_idc_unlock(scsi_qla_host_t *base_vha, uint16_t requester_id)
> 			/* SV: XXX: IDC unlock retrying needed here? */
> 
> 			/* Retry for IDC-unlock */
> -			qla83xx_wait_logic();
> +			msleep(QLA83XX_WAIT_LOGIC_MS);
> 			retry++;
> 			ql_dbg(ql_dbg_p3p, base_vha, 0xb064,
> 			    "Failed to release IDC lock, retrying=%d\n", retry);
> @@ -6292,7 +6287,7 @@ qla83xx_idc_unlock(scsi_qla_host_t *base_vha, uint16_t requester_id)
> 		}
> 	} else if (retry < 10) {
> 		/* Retry for IDC-unlock */
> -		qla83xx_wait_logic();
> +		msleep(QLA83XX_WAIT_LOGIC_MS);
> 		retry++;
> 		ql_dbg(ql_dbg_p3p, base_vha, 0xb065,
> 		    "Failed to read drv-lockid, retrying=%d\n", retry);
> @@ -6308,7 +6303,7 @@ qla83xx_idc_unlock(scsi_qla_host_t *base_vha, uint16_t requester_id)
> 	if (qla83xx_access_control(base_vha, options, 0, 0, NULL)) {
> 		if (retry < 10) {
> 			/* Retry for IDC-unlock */
> -			qla83xx_wait_logic();
> +			msleep(QLA83XX_WAIT_LOGIC_MS);
> 			retry++;
> 			ql_dbg(ql_dbg_p3p, base_vha, 0xb066,
> 			    "Failed to release IDC lock, retrying=%d\n", retry);
> 
> 2.29.2
> 

Looks Good. 

Reviewed-by: Himanshu Madhani <himanshu.madhani@oracle.com>

--
Himanshu Madhani	 Oracle Linux Engineering

[PATCH 07/14] scsi: qla4xxx: qla4_82xx_idc_lock(): Remove in_interrupt().

[Sebastian Andrzej Siewior]

From: "Ahmed S. Darwish" <a.darwish@linutronix.de>

qla4_82xx_idc_lock() spins on a certain hardware state until it is
updated. At the end of each spin, if in_interrupt() is true, it does 20
loops of cpu_relax(). Otherwise, it yields the CPU.

While in_interrupt() is ill-defined and does not provide what the name
suggests, it is not needed here: qla4_82xx_idc_lock() is always called
from process context. Below is an analysis of its callers:

  - ql4_nx.c: qla4_82xx_need_reset_handler(), 1-second msleep() in a
    loop.

  - ql4_nx.c: qla4_82xx_isp_reset(), calls
    qla4_8xxx_device_state_handler(), which has multiple msleep()s.

Beside direct calls, qla4_82xx_idc_lock() is also bound to
isp_operations ->idc_lock() hook. Other functions which are bound to the
same hook, e.g. qla4_83xx_drv_lock(), also have an msleep(). For
completeness, below is an analysis of all callers of that hook:

  - ql4_83xx.c: qla4_83xx_need_reset_handler(), has an msleep()

  - ql4_83xx.c: qla4_83xx_isp_reset(), calls
    qla4_8xxx_device_state_handler(), which has multiple msleep()s.

  - ql4_83xx.c: qla4_83xx_disable_pause(), all process context callers:
    => ql4_mbx.c: qla4xxx_mailbox_command(), msleep(), mutex_lock()
    => ql4_os.c: qla4xxx_recover_adapter(), schedule_timeout() in loop
    => ql4_os.c: qla4xxx_do_dpc(), workqueue context

  - ql4_attr.c: qla4_8xxx_sysfs_write_fw_dump(), sysfs bin_attribute
    ->write() hook, process context

  - ql4_mbx.c: qla4xxx_mailbox_command(), earlier discussed

  - ql4_nx.c: qla4_8xxx_device_bootstrap(), callers:
    => ql4_83xx.c: qla4_83xx_need_reset_handler(), process, msleep()
    => ql4_nx.c: qla4_8xxx_device_state_handler(), earlier discussed

  - ql4_nx.c: qla4_8xxx_need_qsnt_handler(), callers:
    => ql4_nx.c: qla4_8xxx_device_state_handler(), multipe msleep()s
    => ql4_os.c: qla4xxx_do_dpc(), workqueue context

  - ql4_nx.c: qla4_8xxx_update_idc_reg(), callers:
    => ql4_nx.c: qla4_8xxx_device_state_handler(), earlier discussed
    => ql4_os.c: qla4_8xxx_error_recovery(), only called by
    qla4xxx_pci_slot_reset(), which is bound to PCI ->slot_reset()
    process-context hook

  - ql4_nx.c: qla4_8xxx_device_state_handler(), earlier discussed

  - ql4_os.c: qla4xxx_recover_adapter(), earlier discussed

  - ql4_os.c: qla4xxx_do_dpc(), earlier discussed

Remove the in_interrupt() check. Mark, qla4_82xx_idc_lock(), and the
->idc_lock() hook itself, with "Context: task, can sleep".

Change qla4_82xx_idc_lock() implementation to sleep 100ms, instead of a
schedule(), for each spin. This is more deterministic, and it matches
other PCI HW locking functions in the driver.

Signed-off-by: Ahmed S. Darwish <a.darwish@linutronix.de>
Cc: Nilesh Javali <njavali@marvell.com>
Cc: Manish Rangankar <mrangankar@marvell.com>
Cc: <GR-QLogic-Storage-Upstream@marvell.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/scsi/qla4xxx/ql4_def.h |  2 +-
 drivers/scsi/qla4xxx/ql4_nx.c  | 14 +++++---------
 2 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/scsi/qla4xxx/ql4_def.h b/drivers/scsi/qla4xxx/ql4_def.h
index f5b382ed0a1b2..9210547483886 100644
--- a/drivers/scsi/qla4xxx/ql4_def.h
+++ b/drivers/scsi/qla4xxx/ql4_def.h
@@ -435,7 +435,7 @@ struct isp_operations {
 	void (*wr_reg_direct) (struct scsi_qla_host *, ulong, uint32_t);
 	int (*rd_reg_indirect) (struct scsi_qla_host *, uint32_t, uint32_t *);
 	int (*wr_reg_indirect) (struct scsi_qla_host *, uint32_t, uint32_t);
-	int (*idc_lock) (struct scsi_qla_host *);
+	int (*idc_lock) (struct scsi_qla_host *); /* Context: task, can sleep */
 	void (*idc_unlock) (struct scsi_qla_host *);
 	void (*rom_lock_recovery) (struct scsi_qla_host *);
 	void (*queue_mailbox_command) (struct scsi_qla_host *, uint32_t *, int);
diff --git a/drivers/scsi/qla4xxx/ql4_nx.c b/drivers/scsi/qla4xxx/ql4_nx.c
index da903a545b2c0..4362d0ebe0e15 100644
--- a/drivers/scsi/qla4xxx/ql4_nx.c
+++ b/drivers/scsi/qla4xxx/ql4_nx.c
@@ -512,12 +512,15 @@ int qla4_82xx_md_wr_32(struct scsi_qla_host *ha, uint32_t off, uint32_t data)
  *
  * General purpose lock used to synchronize access to
  * CRB_DEV_STATE, CRB_DEV_REF_COUNT, etc.
+ *
+ * Context: task, can sleep
  **/
 int qla4_82xx_idc_lock(struct scsi_qla_host *ha)
 {
-	int i;
 	int done = 0, timeout = 0;
 
+	might_sleep();
+
 	while (!done) {
 		/* acquire semaphore5 from PCI HW block */
 		done = qla4_82xx_rd_32(ha, QLA82XX_PCIE_REG(PCIE_SEM5_LOCK));
@@ -527,14 +530,7 @@ int qla4_82xx_idc_lock(struct scsi_qla_host *ha)
 			return -1;
 
 		timeout++;
-
-		/* Yield CPU */
-		if (!in_interrupt())
-			schedule();
-		else {
-			for (i = 0; i < 20; i++)
-				cpu_relax();    /*This a nop instr on i386*/
-		}
+		msleep(100);
 	}
 	return 0;
 }
-- 
2.29.2

Re: [PATCH 07/14] scsi: qla4xxx: qla4_82xx_idc_lock(): Remove in_interrupt().

[Daniel Wagner]

On Thu, Nov 26, 2020 at 02:29:45PM +0100, Sebastian Andrzej Siewior wrote:
> From: "Ahmed S. Darwish" <a.darwish@linutronix.de>
> 
> qla4_82xx_idc_lock() spins on a certain hardware state until it is
> updated. At the end of each spin, if in_interrupt() is true, it does 20
> loops of cpu_relax(). Otherwise, it yields the CPU.
> 
> While in_interrupt() is ill-defined and does not provide what the name
> suggests, it is not needed here: qla4_82xx_idc_lock() is always called
> from process context. Below is an analysis of its callers:
> 
>   - ql4_nx.c: qla4_82xx_need_reset_handler(), 1-second msleep() in a
>     loop.
> 
>   - ql4_nx.c: qla4_82xx_isp_reset(), calls
>     qla4_8xxx_device_state_handler(), which has multiple msleep()s.
> 
> Beside direct calls, qla4_82xx_idc_lock() is also bound to
> isp_operations ->idc_lock() hook. Other functions which are bound to the
> same hook, e.g. qla4_83xx_drv_lock(), also have an msleep(). For
> completeness, below is an analysis of all callers of that hook:
> 
>   - ql4_83xx.c: qla4_83xx_need_reset_handler(), has an msleep()
> 
>   - ql4_83xx.c: qla4_83xx_isp_reset(), calls
>     qla4_8xxx_device_state_handler(), which has multiple msleep()s.
> 
>   - ql4_83xx.c: qla4_83xx_disable_pause(), all process context callers:
>     => ql4_mbx.c: qla4xxx_mailbox_command(), msleep(), mutex_lock()
>     => ql4_os.c: qla4xxx_recover_adapter(), schedule_timeout() in loop
>     => ql4_os.c: qla4xxx_do_dpc(), workqueue context
> 
>   - ql4_attr.c: qla4_8xxx_sysfs_write_fw_dump(), sysfs bin_attribute
>     ->write() hook, process context
> 
>   - ql4_mbx.c: qla4xxx_mailbox_command(), earlier discussed
> 
>   - ql4_nx.c: qla4_8xxx_device_bootstrap(), callers:
>     => ql4_83xx.c: qla4_83xx_need_reset_handler(), process, msleep()
>     => ql4_nx.c: qla4_8xxx_device_state_handler(), earlier discussed
> 
>   - ql4_nx.c: qla4_8xxx_need_qsnt_handler(), callers:
>     => ql4_nx.c: qla4_8xxx_device_state_handler(), multipe msleep()s
>     => ql4_os.c: qla4xxx_do_dpc(), workqueue context
> 
>   - ql4_nx.c: qla4_8xxx_update_idc_reg(), callers:
>     => ql4_nx.c: qla4_8xxx_device_state_handler(), earlier discussed
>     => ql4_os.c: qla4_8xxx_error_recovery(), only called by
>     qla4xxx_pci_slot_reset(), which is bound to PCI ->slot_reset()
>     process-context hook
> 
>   - ql4_nx.c: qla4_8xxx_device_state_handler(), earlier discussed
> 
>   - ql4_os.c: qla4xxx_recover_adapter(), earlier discussed
> 
>   - ql4_os.c: qla4xxx_do_dpc(), earlier discussed
> 
> Remove the in_interrupt() check. Mark, qla4_82xx_idc_lock(), and the
> ->idc_lock() hook itself, with "Context: task, can sleep".
> 
> Change qla4_82xx_idc_lock() implementation to sleep 100ms, instead of a
> schedule(), for each spin. This is more deterministic, and it matches
> other PCI HW locking functions in the driver.
> 
> Signed-off-by: Ahmed S. Darwish <a.darwish@linutronix.de>
> Cc: Nilesh Javali <njavali@marvell.com>
> Cc: Manish Rangankar <mrangankar@marvell.com>
> Cc: <GR-QLogic-Storage-Upstream@marvell.com>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Reviewed-by: Daniel Wagner <dwagner@suse.de>

[PATCH 08/14] scsi: qla4xxx: qla4_82xx_rom_lock(): Remove in_interrupt().

[Sebastian Andrzej Siewior]

From: "Ahmed S. Darwish" <a.darwish@linutronix.de>

qla4_82xx_rom_lock() spins on a certain hardware state until it is
updated. At the end of each spin, if in_interrupt() is true, it does 20
loops of cpu_relax(). Otherwise, it yields the CPU.

While in_interrupt() is ill-defined and does not provide what the name
suggests, it is not needed here: qla4_82xx_rom_lock() is always called
from process context. Below is an analysis of its callers:

  - ql4_nx.c: qla4_82xx_rom_fast_read(), all process context callers:
    => ql4_nx.c: qla4_82xx_pinit_from_rom(), GFP_KERNEL allocation
    => ql4_nx.c: qla4_82xx_load_from_flash(), msleep() in a loop

  - ql4_nx.c: qla4_82xx_pinit_from_rom(), earlier discussed

  - ql4_nx.c: qla4_82xx_rom_lock_recovery(), bound to "isp_operations"
    ->rom_lock_recovery() hook, which has one process context caller,
    qla4_8xxx_device_bootstrap(), with callers:
      => ql4_83xx.c: qla4_83xx_need_reset_handler(), process, msleep()
      => ql4_nx.c: qla4_8xxx_device_state_handler(), multiple msleep()s

  - ql4_nx.c: qla4_82xx_read_flash_data(), has cond_resched()

Remove the in_interrupt() check. Mark, qla4_82xx_rom_lock(), and the
->rom_lock_recovery() hook, with "Context: task, can sleep".

Change qla4_82xx_rom_lock() implementation to sleep 20ms, instead of a
schedule(), for each spin. This is more deterministic, and it matches
the other implementations bound to ->rom_lock_recovery().

Signed-off-by: Ahmed S. Darwish <a.darwish@linutronix.de>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Nilesh Javali <njavali@marvell.com>
Cc: Manish Rangankar <mrangankar@marvell.com>
Cc: <GR-QLogic-Storage-Upstream@marvell.com>
---
 drivers/scsi/qla4xxx/ql4_def.h |  2 +-
 drivers/scsi/qla4xxx/ql4_nx.c  | 16 ++++++----------
 2 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/drivers/scsi/qla4xxx/ql4_def.h b/drivers/scsi/qla4xxx/ql4_def.h
index 9210547483886..031569c496e57 100644
--- a/drivers/scsi/qla4xxx/ql4_def.h
+++ b/drivers/scsi/qla4xxx/ql4_def.h
@@ -437,7 +437,7 @@ struct isp_operations {
 	int (*wr_reg_indirect) (struct scsi_qla_host *, uint32_t, uint32_t);
 	int (*idc_lock) (struct scsi_qla_host *); /* Context: task, can sleep */
 	void (*idc_unlock) (struct scsi_qla_host *);
-	void (*rom_lock_recovery) (struct scsi_qla_host *);
+	void (*rom_lock_recovery) (struct scsi_qla_host *); /* Context: task, can sleep */
 	void (*queue_mailbox_command) (struct scsi_qla_host *, uint32_t *, int);
 	void (*process_mailbox_interrupt) (struct scsi_qla_host *, int);
 };
diff --git a/drivers/scsi/qla4xxx/ql4_nx.c b/drivers/scsi/qla4xxx/ql4_nx.c
index 4362d0ebe0e15..fd30fbd0d33cb 100644
--- a/drivers/scsi/qla4xxx/ql4_nx.c
+++ b/drivers/scsi/qla4xxx/ql4_nx.c
@@ -871,15 +871,18 @@ qla4_82xx_decode_crb_addr(unsigned long addr)
 static long rom_max_timeout = 100;
 static long qla4_82xx_rom_lock_timeout = 100;
 
+/*
+ * Context: task, can_sleep
+ */
 static int
 qla4_82xx_rom_lock(struct scsi_qla_host *ha)
 {
-	int i;
 	int done = 0, timeout = 0;
 
+	might_sleep();
+
 	while (!done) {
 		/* acquire semaphore2 from PCI HW block */
-
 		done = qla4_82xx_rd_32(ha, QLA82XX_PCIE_REG(PCIE_SEM2_LOCK));
 		if (done == 1)
 			break;
@@ -887,14 +890,7 @@ qla4_82xx_rom_lock(struct scsi_qla_host *ha)
 			return -1;
 
 		timeout++;
-
-		/* Yield CPU */
-		if (!in_interrupt())
-			schedule();
-		else {
-			for (i = 0; i < 20; i++)
-				cpu_relax();    /*This a nop instr on i386*/
-		}
+		msleep(20);
 	}
 	qla4_82xx_wr_32(ha, QLA82XX_ROM_LOCK_ID, ROM_LOCK_DRIVER);
 	return 0;
-- 
2.29.2

Re: [PATCH 08/14] scsi: qla4xxx: qla4_82xx_rom_lock(): Remove in_interrupt().

[Daniel Wagner]

On Thu, Nov 26, 2020 at 02:29:46PM +0100, Sebastian Andrzej Siewior wrote:
> From: "Ahmed S. Darwish" <a.darwish@linutronix.de>
> 
> qla4_82xx_rom_lock() spins on a certain hardware state until it is
> updated. At the end of each spin, if in_interrupt() is true, it does 20
> loops of cpu_relax(). Otherwise, it yields the CPU.
> 
> While in_interrupt() is ill-defined and does not provide what the name
> suggests, it is not needed here: qla4_82xx_rom_lock() is always called
> from process context. Below is an analysis of its callers:
> 
>   - ql4_nx.c: qla4_82xx_rom_fast_read(), all process context callers:
>     => ql4_nx.c: qla4_82xx_pinit_from_rom(), GFP_KERNEL allocation
>     => ql4_nx.c: qla4_82xx_load_from_flash(), msleep() in a loop
> 
>   - ql4_nx.c: qla4_82xx_pinit_from_rom(), earlier discussed
> 
>   - ql4_nx.c: qla4_82xx_rom_lock_recovery(), bound to "isp_operations"
>     ->rom_lock_recovery() hook, which has one process context caller,
>     qla4_8xxx_device_bootstrap(), with callers:
>       => ql4_83xx.c: qla4_83xx_need_reset_handler(), process, msleep()
>       => ql4_nx.c: qla4_8xxx_device_state_handler(), multiple msleep()s
> 
>   - ql4_nx.c: qla4_82xx_read_flash_data(), has cond_resched()
> 
> Remove the in_interrupt() check. Mark, qla4_82xx_rom_lock(), and the
> ->rom_lock_recovery() hook, with "Context: task, can sleep".
> 
> Change qla4_82xx_rom_lock() implementation to sleep 20ms, instead of a
> schedule(), for each spin. This is more deterministic, and it matches
> the other implementations bound to ->rom_lock_recovery().
> 
> Signed-off-by: Ahmed S. Darwish <a.darwish@linutronix.de>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Cc: Nilesh Javali <njavali@marvell.com>
> Cc: Manish Rangankar <mrangankar@marvell.com>
> Cc: <GR-QLogic-Storage-Upstream@marvell.com>

Reviewed-by: Daniel Wagner <dwagner@suse.de>

[PATCH 09/14] scsi: mpt3sas: Remove in_interrupt().

[Sebastian Andrzej Siewior]

From: "Ahmed S. Darwish" <a.darwish@linutronix.de>

_scsih_fw_event_cleanup_queue() waits for all outstanding firmware
events wokrqueue handlers to finish. If in_interrupt() is true, it
cancels itself and return early.

That in_interrupt() check is ill-defined and does not provide what the
name suggests: it does not cover all states in which it is safe to block
and call functions like cancel_work_sync().

That check is also not needed: _scsih_fw_event_cleanup_queue() is always
invoked from process context. Below is an analysis of its callers:

  - scsih_remove(), bound to PCI ->remove(), process context

  - scsih_shutdown(), bound to PCI ->shutdown(), process context

  - mpt3sas_scsih_clear_outstanding_scsi_tm_commands(), called by
      => _base_clear_outstanding_commands(), called by
        =>_base_fault_reset_work(), workqueue
        => mpt3sas_base_hard_reset_handler(), locks mutex

Remove the in_interrupt() check. Change _scsih_fw_event_cleanup_queue()
specification to a purely process-context function and mark it with
"Context: task, can sleep".

Signed-off-by: Ahmed S. Darwish <a.darwish@linutronix.de>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Sathya Prakash <sathya.prakash@broadcom.com>
Cc: Sreekanth Reddy <sreekanth.reddy@broadcom.com>
Cc: Suganath Prabu Subramani <suganath-prabu.subramani@broadcom.com>
Cc: <MPT-FusionLinux.pdl@broadcom.com>
---
 drivers/scsi/mpt3sas/mpt3sas_scsih.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
index f081adb85addc..d91f45abe6b86 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
@@ -3673,6 +3673,8 @@ static struct fw_event_work *dequeue_next_fw_event(struct MPT3SAS_ADAPTER *ioc)
  *
  * Walk the firmware event queue, either killing timers, or waiting
  * for outstanding events to complete
+ *
+ * Context: task, can sleep
  */
 static void
 _scsih_fw_event_cleanup_queue(struct MPT3SAS_ADAPTER *ioc)
@@ -3680,7 +3682,7 @@ _scsih_fw_event_cleanup_queue(struct MPT3SAS_ADAPTER *ioc)
 	struct fw_event_work *fw_event;
 
 	if ((list_empty(&ioc->fw_event_list) && !ioc->current_event) ||
-	     !ioc->firmware_event_thread || in_interrupt())
+	    !ioc->firmware_event_thread)
 		return;
 
 	ioc->fw_events_cleanup = 1;
-- 
2.29.2

Re: [PATCH 09/14] scsi: mpt3sas: Remove in_interrupt().

[Daniel Wagner]

On Thu, Nov 26, 2020 at 02:29:47PM +0100, Sebastian Andrzej Siewior wrote:
> From: "Ahmed S. Darwish" <a.darwish@linutronix.de>
> 
> _scsih_fw_event_cleanup_queue() waits for all outstanding firmware
> events wokrqueue handlers to finish. If in_interrupt() is true, it
> cancels itself and return early.
> 
> That in_interrupt() check is ill-defined and does not provide what the
> name suggests: it does not cover all states in which it is safe to block
> and call functions like cancel_work_sync().
> 
> That check is also not needed: _scsih_fw_event_cleanup_queue() is always
> invoked from process context. Below is an analysis of its callers:
> 
>   - scsih_remove(), bound to PCI ->remove(), process context
> 
>   - scsih_shutdown(), bound to PCI ->shutdown(), process context
> 
>   - mpt3sas_scsih_clear_outstanding_scsi_tm_commands(), called by
>       => _base_clear_outstanding_commands(), called by
>         =>_base_fault_reset_work(), workqueue
>         => mpt3sas_base_hard_reset_handler(), locks mutex
> 
> Remove the in_interrupt() check. Change _scsih_fw_event_cleanup_queue()
> specification to a purely process-context function and mark it with
> "Context: task, can sleep".
> 
> Signed-off-by: Ahmed S. Darwish <a.darwish@linutronix.de>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Cc: Sathya Prakash <sathya.prakash@broadcom.com>
> Cc: Sreekanth Reddy <sreekanth.reddy@broadcom.com>
> Cc: Suganath Prabu Subramani <suganath-prabu.subramani@broadcom.com>
> Cc: <MPT-FusionLinux.pdl@broadcom.com>

Reviewed-by: Daniel Wagner <dwagner@suse.de>

[PATCH 10/14] scsi: myrb: Remove WARN_ON(in_interrupt()).

[Sebastian Andrzej Siewior]

From: "Ahmed S. Darwish" <a.darwish@linutronix.de>

The in_interrupt() macro is ill-defined and does not provide what the
name suggests. The usage especially in driver code is deprecated and a
tree-wide effort to clean up and consolidate the (ab)usage of
in_interrupt() and related checks is happening.

In this case the check covers only parts of the contexts in which these
functions cannot be called. It fails to detect preemption or interrupt
disabled invocations.

As wait_for_completion() already contains a broad variety of checks
(always enabled or debug option dependent) which cover all invalid
conditions already, there is no point in having extra inconsistent
warnings in drivers.

Just remove it.

Signed-off-by: Ahmed S. Darwish <a.darwish@linutronix.de>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Hannes Reinecke <hare@kernel.org>
---
 drivers/scsi/myrb.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/scsi/myrb.c b/drivers/scsi/myrb.c
index 5fa0f4ed6565f..3d8e91c07dc77 100644
--- a/drivers/scsi/myrb.c
+++ b/drivers/scsi/myrb.c
@@ -194,7 +194,6 @@ static unsigned short myrb_exec_cmd(struct myrb_hba *cb,
 	cb->qcmd(cb, cmd_blk);
 	spin_unlock_irqrestore(&cb->queue_lock, flags);
 
-	WARN_ON(in_interrupt());
 	wait_for_completion(&cmpl);
 	return cmd_blk->status;
 }
-- 
2.29.2

Re: [PATCH 10/14] scsi: myrb: Remove WARN_ON(in_interrupt()).

[Daniel Wagner]

On Thu, Nov 26, 2020 at 02:29:48PM +0100, Sebastian Andrzej Siewior wrote:
> From: "Ahmed S. Darwish" <a.darwish@linutronix.de>
> 
> The in_interrupt() macro is ill-defined and does not provide what the
> name suggests. The usage especially in driver code is deprecated and a
> tree-wide effort to clean up and consolidate the (ab)usage of
> in_interrupt() and related checks is happening.
> 
> In this case the check covers only parts of the contexts in which these
> functions cannot be called. It fails to detect preemption or interrupt
> disabled invocations.
> 
> As wait_for_completion() already contains a broad variety of checks
> (always enabled or debug option dependent) which cover all invalid
> conditions already, there is no point in having extra inconsistent
> warnings in drivers.
> 
> Just remove it.
> 
> Signed-off-by: Ahmed S. Darwish <a.darwish@linutronix.de>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Cc: Hannes Reinecke <hare@kernel.org>

Reviewed-by: Daniel Wagner <dwagner@suse.de>

[PATCH 11/14] scsi: myrs: Remove WARN_ON(in_interrupt()).

[Sebastian Andrzej Siewior]

From: "Ahmed S. Darwish" <a.darwish@linutronix.de>

The in_interrupt() macro is ill-defined and does not provide what the
name suggests. The usage especially in driver code is deprecated and a
tree-wide effort to clean up and consolidate the (ab)usage of
in_interrupt() and related checks is happening.

In this case the check covers only parts of the contexts in which these
functions cannot be called. It fails to detect preemption or interrupt
disabled invocations.

As wait_for_completion() already contains a broad variety of checks
(always enabled or debug option dependent) which cover all invalid
conditions already, there is no point in having extra inconsistent
warnings in drivers.

Just remove it.

Signed-off-by: Ahmed S. Darwish <a.darwish@linutronix.de>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Hannes Reinecke <hare@kernel.org>
---
 drivers/scsi/myrs.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/scsi/myrs.c b/drivers/scsi/myrs.c
index 7a3ade765ce3b..4adf9ded296aa 100644
--- a/drivers/scsi/myrs.c
+++ b/drivers/scsi/myrs.c
@@ -136,7 +136,6 @@ static void myrs_exec_cmd(struct myrs_hba *cs,
 	myrs_qcmd(cs, cmd_blk);
 	spin_unlock_irqrestore(&cs->queue_lock, flags);
 
-	WARN_ON(in_interrupt());
 	wait_for_completion(&complete);
 }
 
-- 
2.29.2

Re: [PATCH 11/14] scsi: myrs: Remove WARN_ON(in_interrupt()).

[Daniel Wagner]

On Thu, Nov 26, 2020 at 02:29:49PM +0100, Sebastian Andrzej Siewior wrote:
> From: "Ahmed S. Darwish" <a.darwish@linutronix.de>
> 
> The in_interrupt() macro is ill-defined and does not provide what the
> name suggests. The usage especially in driver code is deprecated and a
> tree-wide effort to clean up and consolidate the (ab)usage of
> in_interrupt() and related checks is happening.
> 
> In this case the check covers only parts of the contexts in which these
> functions cannot be called. It fails to detect preemption or interrupt
> disabled invocations.
> 
> As wait_for_completion() already contains a broad variety of checks
> (always enabled or debug option dependent) which cover all invalid
> conditions already, there is no point in having extra inconsistent
> warnings in drivers.
> 
> Just remove it.
> 
> Signed-off-by: Ahmed S. Darwish <a.darwish@linutronix.de>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Cc: Hannes Reinecke <hare@kernel.org>

Reviewed-by: Daniel Wagner <dwagner@suse.de>

[PATCH 12/14] scsi: NCR5380: Remove in_interrupt().

[Sebastian Andrzej Siewior]

From: "Ahmed S. Darwish" <a.darwish@linutronix.de>

NCR5380_poll_politely2() uses in_interrupt() to check if it is safe to
sleep.

The usage of in_interrupt() in drivers is phased out and Linus clearly
requested that code which changes behaviour depending on context should
either be separated, or the context be explicitly conveyed in an
argument passed by the caller.

Below is a context analysis of NCR5380_poll_politely2() uppermost
callers:

  - NCR5380_maybe_reset_bus(), task, invoked during device probe.
    -> NCR5380_poll_politely()
    -> do_abort()

  - NCR5380_select(), task, but can only sleep in the "release, then
    re-acquire" regions of the spinlock held by its caller.
    Sleeping invocations (lock released):
    -> NCR5380_poll_politely2()

    Atomic invocations (lock acquired):
    -> NCR5380_reselect()
       -> NCR5380_poll_politely()
       -> do_abort()
       -> NCR5380_transfer_pio()

  - NCR5380_intr(), interrupt handler
    -> NCR5380_dma_complete()
       -> NCR5380_transfer_pio()
	  -> NCR5380_poll_politely()
    -> NCR5380_reselect() (see above)

  - NCR5380_information_transfer(), task, but can only sleep in the
    "release, then re-acquire" regions of the caller-held spinlock.
    Sleeping invocations (lock released):
      - NCR5380_transfer_pio() -> NCR5380_poll_politely()
      - NCR5380_poll_politely()

    Atomic invocations (lock acquired):
      - NCR5380_transfer_dma()
	-> NCR5380_dma_recv_setup()
           => generic_NCR5380_precv() -> NCR5380_poll_politely()
	   => macscsi_pread() -> NCR5380_poll_politely()

	-> NCR5380_dma_send_setup()
 	   => generic_NCR5380_psend -> NCR5380_poll_politely2()
	   => macscsi_pwrite() -> NCR5380_poll_politely()

	-> NCR5380_poll_politely2()
        -> NCR5380_dma_complete()
           -> NCR5380_transfer_pio()
	      -> NCR5380_poll_politely()
      - NCR5380_transfer_pio() -> NCR5380_poll_politely

  - NCR5380_reselect(), atomic, always called with hostdata spinlock
    held.

If direct callers are purely atomic, or purely task context, change
their specifications accordingly and mark them with "Context: " tags.

For the mixed ones, trickle-down context from upper layers.

Signed-off-by: Ahmed S. Darwish <a.darwish@linutronix.de>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Finn Thain <fthain@telegraphics.com.au>
Cc: Michael Schmitz <schmitzmic@gmail.com>
Cc: <linux-m68k@lists.linux-m68k.org>
---
 drivers/scsi/NCR5380.c   | 115 ++++++++++++++++++++++-----------------
 drivers/scsi/NCR5380.h   |   9 +--
 drivers/scsi/g_NCR5380.c |  26 ++++++---
 drivers/scsi/mac_scsi.c  |  10 ++--
 4 files changed, 93 insertions(+), 67 deletions(-)

diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c
index d597d7493a627..51a80f3330faa 100644
--- a/drivers/scsi/NCR5380.c
+++ b/drivers/scsi/NCR5380.c
@@ -132,7 +132,7 @@
 static unsigned int disconnect_mask = ~0;
 module_param(disconnect_mask, int, 0444);
 
-static int do_abort(struct Scsi_Host *);
+static int do_abort(struct Scsi_Host *, bool);
 static void do_reset(struct Scsi_Host *);
 static void bus_reset_cleanup(struct Scsi_Host *);
 
@@ -198,6 +198,7 @@ static inline void set_resid_from_SCp(struct scsi_cmnd *cmd)
  * @bit2: Second bitmask to check
  * @val2: Second expected value
  * @wait: Time-out in jiffies
+ * @can_sleep: True if the function can sleep
  *
  * Polls the chip in a reasonably efficient manner waiting for an
  * event to occur. After a short quick poll we begin to yield the CPU
@@ -210,7 +211,7 @@ static inline void set_resid_from_SCp(struct scsi_cmnd *cmd)
 static int NCR5380_poll_politely2(struct NCR5380_hostdata *hostdata,
                                   unsigned int reg1, u8 bit1, u8 val1,
                                   unsigned int reg2, u8 bit2, u8 val2,
-                                  unsigned long wait)
+                                  unsigned long wait, bool can_sleep)
 {
 	unsigned long n = hostdata->poll_loops;
 	unsigned long deadline = jiffies + wait;
@@ -223,7 +224,7 @@ static int NCR5380_poll_politely2(struct NCR5380_hostdata *hostdata,
 		cpu_relax();
 	} while (n--);
 
-	if (irqs_disabled() || in_interrupt())
+	if (!can_sleep)
 		return -ETIMEDOUT;
 
 	/* Repeatedly sleep for 1 ms until deadline */
@@ -467,9 +468,10 @@ static int NCR5380_init(struct Scsi_Host *instance, int flags)
  *
  * Note that a bus reset will cause the chip to assert IRQ.
  *
+ * Context: task, can sleep
+ *
  * Returns 0 if successful, otherwise -ENXIO.
  */
-
 static int NCR5380_maybe_reset_bus(struct Scsi_Host *instance)
 {
 	struct NCR5380_hostdata *hostdata = shost_priv(instance);
@@ -482,11 +484,11 @@ static int NCR5380_maybe_reset_bus(struct Scsi_Host *instance)
 		case 5:
 			shost_printk(KERN_ERR, instance, "SCSI bus busy, waiting up to five seconds\n");
 			NCR5380_poll_politely(hostdata,
-			                      STATUS_REG, SR_BSY, 0, 5 * HZ);
+			                      STATUS_REG, SR_BSY, 0, 5 * HZ, true);
 			break;
 		case 2:
 			shost_printk(KERN_ERR, instance, "bus busy, attempting abort\n");
-			do_abort(instance);
+			do_abort(instance, true);
 			break;
 		case 4:
 			shost_printk(KERN_ERR, instance, "bus busy, attempting reset\n");
@@ -690,7 +692,6 @@ static void requeue_cmd(struct Scsi_Host *instance, struct scsi_cmnd *cmd)
  * NCR5380_queue_command() and NCR5380_intr() will try to start it
  * in case it is not running.
  */
-
 static void NCR5380_main(struct work_struct *work)
 {
 	struct NCR5380_hostdata *hostdata =
@@ -750,10 +751,9 @@ static void NCR5380_main(struct work_struct *work)
  * NCR5380_dma_complete - finish DMA transfer
  * @instance: the scsi host instance
  *
- * Called by the interrupt handler when DMA finishes or a phase
- * mismatch occurs (which would end the DMA transfer).
+ * Context: atomic. Called by the interrupt handler when DMA finishes
+ * or a phase mismatch occurs (which would end the DMA transfer).
  */
-
 static void NCR5380_dma_complete(struct Scsi_Host *instance)
 {
 	struct NCR5380_hostdata *hostdata = shost_priv(instance);
@@ -822,7 +822,7 @@ static void NCR5380_dma_complete(struct Scsi_Host *instance)
 			if (toPIO > 0) {
 				dsprintk(NDEBUG_DMA, instance,
 				         "Doing %d byte PIO to 0x%p\n", cnt, *data);
-				NCR5380_transfer_pio(instance, &p, &cnt, data);
+				NCR5380_transfer_pio(instance, &p, &cnt, data, false);
 				*count -= toPIO - cnt;
 			}
 		}
@@ -962,8 +962,11 @@ static irqreturn_t __maybe_unused NCR5380_intr(int irq, void *dev_id)
  *
  * If failed (no target) : cmd->scsi_done() will be called, and the
  * cmd->result host byte set to DID_BAD_TARGET.
+ *
+ * Context: task context, with @instance hostdata spinlock held by
+ * caller.  That is, this function can sleep in the areas between
+ * releasing and re-acquring that spinlock.
  */
-
 static bool NCR5380_select(struct Scsi_Host *instance, struct scsi_cmnd *cmd)
 	__releases(&hostdata->lock) __acquires(&hostdata->lock)
 {
@@ -1010,8 +1013,10 @@ static bool NCR5380_select(struct Scsi_Host *instance, struct scsi_cmnd *cmd)
 
 	spin_unlock_irq(&hostdata->lock);
 	err = NCR5380_poll_politely2(hostdata, MODE_REG, MR_ARBITRATE, 0,
-	                INITIATOR_COMMAND_REG, ICR_ARBITRATION_PROGRESS,
-	                                       ICR_ARBITRATION_PROGRESS, HZ);
+				     INITIATOR_COMMAND_REG,
+				     ICR_ARBITRATION_PROGRESS,
+				     ICR_ARBITRATION_PROGRESS,
+				     HZ, true);
 	spin_lock_irq(&hostdata->lock);
 	if (!(NCR5380_read(MODE_REG) & MR_ARBITRATE)) {
 		/* Reselection interrupt */
@@ -1136,7 +1141,7 @@ static bool NCR5380_select(struct Scsi_Host *instance, struct scsi_cmnd *cmd)
 	 */
 
 	err = NCR5380_poll_politely(hostdata, STATUS_REG, SR_BSY, SR_BSY,
-	                            msecs_to_jiffies(250));
+	                            msecs_to_jiffies(250), true);
 
 	if ((NCR5380_read(STATUS_REG) & (SR_SEL | SR_IO)) == (SR_SEL | SR_IO)) {
 		spin_lock_irq(&hostdata->lock);
@@ -1181,7 +1186,7 @@ static bool NCR5380_select(struct Scsi_Host *instance, struct scsi_cmnd *cmd)
 
 	/* Wait for start of REQ/ACK handshake */
 
-	err = NCR5380_poll_politely(hostdata, STATUS_REG, SR_REQ, SR_REQ, HZ);
+	err = NCR5380_poll_politely(hostdata, STATUS_REG, SR_REQ, SR_REQ, HZ, true);
 	spin_lock_irq(&hostdata->lock);
 	if (err < 0) {
 		shost_printk(KERN_ERR, instance, "select: REQ timeout\n");
@@ -1189,7 +1194,7 @@ static bool NCR5380_select(struct Scsi_Host *instance, struct scsi_cmnd *cmd)
 		goto out;
 	}
 	if (!hostdata->selecting) {
-		do_abort(instance);
+		do_abort(instance, false);
 		return false;
 	}
 
@@ -1200,7 +1205,7 @@ static bool NCR5380_select(struct Scsi_Host *instance, struct scsi_cmnd *cmd)
 	len = 1;
 	data = tmp;
 	phase = PHASE_MSGOUT;
-	NCR5380_transfer_pio(instance, &phase, &len, &data);
+	NCR5380_transfer_pio(instance, &phase, &len, &data, false);
 	if (len) {
 		NCR5380_write(INITIATOR_COMMAND_REG, ICR_BASE);
 		cmd->result = DID_ERROR << 16;
@@ -1238,7 +1243,8 @@ static bool NCR5380_select(struct Scsi_Host *instance, struct scsi_cmnd *cmd)
  *
  * Inputs : instance - instance of driver, *phase - pointer to
  * what phase is expected, *count - pointer to number of
- * bytes to transfer, **data - pointer to data pointer.
+ * bytes to transfer, **data - pointer to data pointer,
+ * can_sleep - whether it is safe to sleep.
  *
  * Returns : -1 when different phase is entered without transferring
  * maximum number of bytes, 0 if all bytes are transferred or exit
@@ -1257,7 +1263,7 @@ static bool NCR5380_select(struct Scsi_Host *instance, struct scsi_cmnd *cmd)
 
 static int NCR5380_transfer_pio(struct Scsi_Host *instance,
 				unsigned char *phase, int *count,
-				unsigned char **data)
+				unsigned char **data, bool can_sleep)
 {
 	struct NCR5380_hostdata *hostdata = shost_priv(instance);
 	unsigned char p = *phase, tmp;
@@ -1278,7 +1284,8 @@ static int NCR5380_transfer_pio(struct Scsi_Host *instance,
 		 * valid
 		 */
 
-		if (NCR5380_poll_politely(hostdata, STATUS_REG, SR_REQ, SR_REQ, HZ) < 0)
+		if (NCR5380_poll_politely(hostdata, STATUS_REG, SR_REQ, SR_REQ,
+					  HZ, can_sleep) < 0)
 			break;
 
 		dsprintk(NDEBUG_HANDSHAKE, instance, "REQ asserted\n");
@@ -1324,7 +1331,7 @@ static int NCR5380_transfer_pio(struct Scsi_Host *instance,
 		}
 
 		if (NCR5380_poll_politely(hostdata,
-		                          STATUS_REG, SR_REQ, 0, 5 * HZ) < 0)
+		                          STATUS_REG, SR_REQ, 0, 5 * HZ, can_sleep) < 0)
 			break;
 
 		dsprintk(NDEBUG_HANDSHAKE, instance, "REQ negated, handshake complete\n");
@@ -1399,11 +1406,12 @@ static void do_reset(struct Scsi_Host *instance)
  * do_abort - abort the currently established nexus by going to
  * MESSAGE OUT phase and sending an ABORT message.
  * @instance: relevant scsi host instance
+ * @can_sleep: true if the function can sleep
  *
  * Returns 0 on success, negative error code on failure.
  */
 
-static int do_abort(struct Scsi_Host *instance)
+static int do_abort(struct Scsi_Host *instance, bool can_sleep)
 {
 	struct NCR5380_hostdata *hostdata = shost_priv(instance);
 	unsigned char *msgptr, phase, tmp;
@@ -1423,7 +1431,8 @@ static int do_abort(struct Scsi_Host *instance)
 	 * the target sees, so we just handshake.
 	 */
 
-	rc = NCR5380_poll_politely(hostdata, STATUS_REG, SR_REQ, SR_REQ, 10 * HZ);
+	rc = NCR5380_poll_politely(hostdata, STATUS_REG, SR_REQ, SR_REQ,
+				   10 * HZ, can_sleep);
 	if (rc < 0)
 		goto out;
 
@@ -1434,7 +1443,8 @@ static int do_abort(struct Scsi_Host *instance)
 	if (tmp != PHASE_MSGOUT) {
 		NCR5380_write(INITIATOR_COMMAND_REG,
 		              ICR_BASE | ICR_ASSERT_ATN | ICR_ASSERT_ACK);
-		rc = NCR5380_poll_politely(hostdata, STATUS_REG, SR_REQ, 0, 3 * HZ);
+		rc = NCR5380_poll_politely(hostdata, STATUS_REG, SR_REQ, 0,
+					   3 * HZ, can_sleep);
 		if (rc < 0)
 			goto out;
 		NCR5380_write(INITIATOR_COMMAND_REG, ICR_BASE | ICR_ASSERT_ATN);
@@ -1444,7 +1454,7 @@ static int do_abort(struct Scsi_Host *instance)
 	msgptr = &tmp;
 	len = 1;
 	phase = PHASE_MSGOUT;
-	NCR5380_transfer_pio(instance, &phase, &len, &msgptr);
+	NCR5380_transfer_pio(instance, &phase, &len, &msgptr, can_sleep);
 	if (len)
 		rc = -ENXIO;
 
@@ -1474,9 +1484,9 @@ static int do_abort(struct Scsi_Host *instance)
  * is in same phase.
  *
  * Also, *phase, *count, *data are modified in place.
+ *
+ * Context: atomic, @instance hostdata spinlock held
  */
-
-
 static int NCR5380_transfer_dma(struct Scsi_Host *instance,
 				unsigned char *phase, int *count,
 				unsigned char **data)
@@ -1623,12 +1633,12 @@ static int NCR5380_transfer_dma(struct Scsi_Host *instance,
 			 */
 
 			if (NCR5380_poll_politely(hostdata, BUS_AND_STATUS_REG,
-			                          BASR_DRQ, BASR_DRQ, HZ) < 0) {
+			                          BASR_DRQ, BASR_DRQ, HZ, false) < 0) {
 				result = -1;
 				shost_printk(KERN_ERR, instance, "PDMA read: DRQ timeout\n");
 			}
 			if (NCR5380_poll_politely(hostdata, STATUS_REG,
-			                          SR_REQ, 0, HZ) < 0) {
+			                          SR_REQ, 0, HZ, false) < 0) {
 				result = -1;
 				shost_printk(KERN_ERR, instance, "PDMA read: !REQ timeout\n");
 			}
@@ -1640,7 +1650,7 @@ static int NCR5380_transfer_dma(struct Scsi_Host *instance,
 			 */
 			if (NCR5380_poll_politely2(hostdata,
 			     BUS_AND_STATUS_REG, BASR_DRQ, BASR_DRQ,
-			     BUS_AND_STATUS_REG, BASR_PHASE_MATCH, 0, HZ) < 0) {
+			     BUS_AND_STATUS_REG, BASR_PHASE_MATCH, 0, HZ, false) < 0) {
 				result = -1;
 				shost_printk(KERN_ERR, instance, "PDMA write: DRQ and phase timeout\n");
 			}
@@ -1666,8 +1676,11 @@ static int NCR5380_transfer_dma(struct Scsi_Host *instance,
  *
  * XXX Note : we need to watch for bus free or a reset condition here
  * to recover from an unexpected bus free condition.
+ *
+ * Context: task context, with @instance hostdata spinlock held by
+ * caller.  That is, this function can sleep in the areas between
+ * releasing and re-acquring that spinlock.
  */
-
 static void NCR5380_information_transfer(struct Scsi_Host *instance)
 	__releases(&hostdata->lock) __acquires(&hostdata->lock)
 {
@@ -1737,7 +1750,7 @@ static void NCR5380_information_transfer(struct Scsi_Host *instance)
 #if (NDEBUG & NDEBUG_NO_DATAOUT)
 				shost_printk(KERN_DEBUG, instance, "NDEBUG_NO_DATAOUT set, attempted DATAOUT aborted\n");
 				sink = 1;
-				do_abort(instance);
+				do_abort(instance, false);
 				cmd->result = DID_ERROR << 16;
 				complete_cmd(instance, cmd);
 				hostdata->connected = NULL;
@@ -1793,7 +1806,8 @@ static void NCR5380_information_transfer(struct Scsi_Host *instance)
 							   NCR5380_PIO_CHUNK_SIZE);
 					len = transfersize;
 					NCR5380_transfer_pio(instance, &phase, &len,
-					                     (unsigned char **)&cmd->SCp.ptr);
+					                     (unsigned char **)&cmd->SCp.ptr,
+							     false);
 					cmd->SCp.this_residual -= transfersize - len;
 				}
 #ifdef CONFIG_SUN3
@@ -1804,7 +1818,7 @@ static void NCR5380_information_transfer(struct Scsi_Host *instance)
 			case PHASE_MSGIN:
 				len = 1;
 				data = &tmp;
-				NCR5380_transfer_pio(instance, &phase, &len, &data);
+				NCR5380_transfer_pio(instance, &phase, &len, &data, false);
 				cmd->SCp.Message = tmp;
 
 				switch (tmp) {
@@ -1910,7 +1924,7 @@ static void NCR5380_information_transfer(struct Scsi_Host *instance)
 					len = 2;
 					data = extended_msg + 1;
 					phase = PHASE_MSGIN;
-					NCR5380_transfer_pio(instance, &phase, &len, &data);
+					NCR5380_transfer_pio(instance, &phase, &len, &data, true);
 					dsprintk(NDEBUG_EXTENDED, instance, "length %d, code 0x%02x\n",
 					         (int)extended_msg[1],
 					         (int)extended_msg[2]);
@@ -1923,7 +1937,7 @@ static void NCR5380_information_transfer(struct Scsi_Host *instance)
 						data = extended_msg + 3;
 						phase = PHASE_MSGIN;
 
-						NCR5380_transfer_pio(instance, &phase, &len, &data);
+						NCR5380_transfer_pio(instance, &phase, &len, &data, true);
 						dsprintk(NDEBUG_EXTENDED, instance, "message received, residual %d\n",
 						         len);
 
@@ -1970,7 +1984,7 @@ static void NCR5380_information_transfer(struct Scsi_Host *instance)
 				len = 1;
 				data = &msgout;
 				hostdata->last_message = msgout;
-				NCR5380_transfer_pio(instance, &phase, &len, &data);
+				NCR5380_transfer_pio(instance, &phase, &len, &data, false);
 				if (msgout == ABORT) {
 					hostdata->connected = NULL;
 					hostdata->busy[scmd_id(cmd)] &= ~(1 << cmd->device->lun);
@@ -1988,12 +2002,12 @@ static void NCR5380_information_transfer(struct Scsi_Host *instance)
 				 * PSEUDO-DMA architecture we should probably
 				 * use the dma transfer function.
 				 */
-				NCR5380_transfer_pio(instance, &phase, &len, &data);
+				NCR5380_transfer_pio(instance, &phase, &len, &data, false);
 				break;
 			case PHASE_STATIN:
 				len = 1;
 				data = &tmp;
-				NCR5380_transfer_pio(instance, &phase, &len, &data);
+				NCR5380_transfer_pio(instance, &phase, &len, &data, false);
 				cmd->SCp.Status = tmp;
 				break;
 			default:
@@ -2002,7 +2016,7 @@ static void NCR5380_information_transfer(struct Scsi_Host *instance)
 			} /* switch(phase) */
 		} else {
 			spin_unlock_irq(&hostdata->lock);
-			NCR5380_poll_politely(hostdata, STATUS_REG, SR_REQ, SR_REQ, HZ);
+			NCR5380_poll_politely(hostdata, STATUS_REG, SR_REQ, SR_REQ, HZ, true);
 			spin_lock_irq(&hostdata->lock);
 		}
 	}
@@ -2016,8 +2030,9 @@ static void NCR5380_information_transfer(struct Scsi_Host *instance)
  * nexus has been reestablished,
  *
  * Inputs : instance - this instance of the NCR5380.
+ *
+ * Context: atomic, with @instance hostdata spinlock held
  */
-
 static void NCR5380_reselect(struct Scsi_Host *instance)
 {
 	struct NCR5380_hostdata *hostdata = shost_priv(instance);
@@ -2052,7 +2067,7 @@ static void NCR5380_reselect(struct Scsi_Host *instance)
 
 	NCR5380_write(INITIATOR_COMMAND_REG, ICR_BASE | ICR_ASSERT_BSY);
 	if (NCR5380_poll_politely(hostdata,
-	                          STATUS_REG, SR_SEL, 0, 2 * HZ) < 0) {
+	                          STATUS_REG, SR_SEL, 0, 2 * HZ, false) < 0) {
 		shost_printk(KERN_ERR, instance, "reselect: !SEL timeout\n");
 		NCR5380_write(INITIATOR_COMMAND_REG, ICR_BASE);
 		return;
@@ -2064,12 +2079,12 @@ static void NCR5380_reselect(struct Scsi_Host *instance)
 	 */
 
 	if (NCR5380_poll_politely(hostdata,
-	                          STATUS_REG, SR_REQ, SR_REQ, 2 * HZ) < 0) {
+	                          STATUS_REG, SR_REQ, SR_REQ, 2 * HZ, false) < 0) {
 		if ((NCR5380_read(STATUS_REG) & (SR_BSY | SR_SEL)) == 0)
 			/* BUS FREE phase */
 			return;
 		shost_printk(KERN_ERR, instance, "reselect: REQ timeout\n");
-		do_abort(instance);
+		do_abort(instance, false);
 		return;
 	}
 
@@ -2085,10 +2100,10 @@ static void NCR5380_reselect(struct Scsi_Host *instance)
 		unsigned char *data = msg;
 		unsigned char phase = PHASE_MSGIN;
 
-		NCR5380_transfer_pio(instance, &phase, &len, &data);
+		NCR5380_transfer_pio(instance, &phase, &len, &data, false);
 
 		if (len) {
-			do_abort(instance);
+			do_abort(instance, false);
 			return;
 		}
 	}
@@ -2098,7 +2113,7 @@ static void NCR5380_reselect(struct Scsi_Host *instance)
 		shost_printk(KERN_ERR, instance, "expecting IDENTIFY message, got ");
 		spi_print_msg(msg);
 		printk("\n");
-		do_abort(instance);
+		do_abort(instance, false);
 		return;
 	}
 	lun = msg[0] & 0x07;
@@ -2138,7 +2153,7 @@ static void NCR5380_reselect(struct Scsi_Host *instance)
 		 * Since we have an established nexus that we can't do anything
 		 * with, we must abort it.
 		 */
-		if (do_abort(instance) == 0)
+		if (do_abort(instance, false) == 0)
 			hostdata->busy[target] &= ~(1 << lun);
 		return;
 	}
@@ -2285,7 +2300,7 @@ static int NCR5380_abort(struct scsi_cmnd *cmd)
 		dsprintk(NDEBUG_ABORT, instance, "abort: cmd %p is connected\n", cmd);
 		hostdata->connected = NULL;
 		hostdata->dma_len = 0;
-		if (do_abort(instance) < 0) {
+		if (do_abort(instance, false) < 0) {
 			set_host_byte(cmd, DID_ERROR);
 			complete_cmd(instance, cmd);
 			result = FAILED;
diff --git a/drivers/scsi/NCR5380.h b/drivers/scsi/NCR5380.h
index 5935fd6d1a058..7c569e92ca58e 100644
--- a/drivers/scsi/NCR5380.h
+++ b/drivers/scsi/NCR5380.h
@@ -277,20 +277,21 @@ static const char *NCR5380_info(struct Scsi_Host *instance);
 static void NCR5380_reselect(struct Scsi_Host *instance);
 static bool NCR5380_select(struct Scsi_Host *, struct scsi_cmnd *);
 static int NCR5380_transfer_dma(struct Scsi_Host *instance, unsigned char *phase, int *count, unsigned char **data);
-static int NCR5380_transfer_pio(struct Scsi_Host *instance, unsigned char *phase, int *count, unsigned char **data);
+static int NCR5380_transfer_pio(struct Scsi_Host *instance, unsigned char *phase,
+				int *count, unsigned char **data, bool can_sleep);
 static int NCR5380_poll_politely2(struct NCR5380_hostdata *,
                                   unsigned int, u8, u8,
-                                  unsigned int, u8, u8, unsigned long);
+                                  unsigned int, u8, u8, unsigned long, bool);
 
 static inline int NCR5380_poll_politely(struct NCR5380_hostdata *hostdata,
                                         unsigned int reg, u8 bit, u8 val,
-                                        unsigned long wait)
+                                        unsigned long wait, bool can_sleep)
 {
 	if ((NCR5380_read(reg) & bit) == val)
 		return 0;
 
 	return NCR5380_poll_politely2(hostdata, reg, bit, val,
-						reg, bit, val, wait);
+				      reg, bit, val, wait, can_sleep);
 }
 
 static int NCR5380_dma_xfer_len(struct NCR5380_hostdata *,
diff --git a/drivers/scsi/g_NCR5380.c b/drivers/scsi/g_NCR5380.c
index 29e4cdcade720..06b1fcfd33cc9 100644
--- a/drivers/scsi/g_NCR5380.c
+++ b/drivers/scsi/g_NCR5380.c
@@ -513,9 +513,11 @@ static void wait_for_53c80_access(struct NCR5380_hostdata *hostdata)
  * @dst: buffer to write into
  * @len: transfer size
  *
+ * Context: atomic. This implements NCR5380.c NCR5380_dma_recv_setup(),
+ * which is always called with @hostdata spinlock held.
+ *
  * Perform a pseudo DMA mode receive from a 53C400 or equivalent device.
  */
-
 static inline int generic_NCR5380_precv(struct NCR5380_hostdata *hostdata,
                                         unsigned char *dst, int len)
 {
@@ -529,14 +531,16 @@ static inline int generic_NCR5380_precv(struct NCR5380_hostdata *hostdata,
 		if (start == len - 128) {
 			/* Ignore End of DMA interrupt for the final buffer */
 			if (NCR5380_poll_politely(hostdata, hostdata->c400_ctl_status,
-			                          CSR_HOST_BUF_NOT_RDY, 0, HZ / 64) < 0)
+			                          CSR_HOST_BUF_NOT_RDY, 0, HZ / 64,
+						  false) < 0)
 				break;
 		} else {
 			if (NCR5380_poll_politely2(hostdata, hostdata->c400_ctl_status,
 			                           CSR_HOST_BUF_NOT_RDY, 0,
 			                           hostdata->c400_ctl_status,
 			                           CSR_GATED_53C80_IRQ,
-			                           CSR_GATED_53C80_IRQ, HZ / 64) < 0 ||
+			                           CSR_GATED_53C80_IRQ, HZ / 64,
+						   false) < 0 ||
 			    NCR5380_read(hostdata->c400_ctl_status) & CSR_HOST_BUF_NOT_RDY)
 				break;
 		}
@@ -565,7 +569,8 @@ static inline int generic_NCR5380_precv(struct NCR5380_hostdata *hostdata,
 	if (residual == 0 && NCR5380_poll_politely(hostdata, BUS_AND_STATUS_REG,
 	                                           BASR_END_DMA_TRANSFER,
 	                                           BASR_END_DMA_TRANSFER,
-	                                           HZ / 64) < 0)
+	                                           HZ / 64,
+						   false) < 0)
 		scmd_printk(KERN_ERR, hostdata->connected, "%s: End of DMA timeout\n",
 		            __func__);
 
@@ -580,9 +585,11 @@ static inline int generic_NCR5380_precv(struct NCR5380_hostdata *hostdata,
  * @src: buffer to read from
  * @len: transfer size
  *
+ * Context: atomic. This implements NCR5380.c NCR5380_dma_send_setup(),
+ * which is always called with @hostdata spinlock held.
+ *
  * Perform a pseudo DMA mode send to a 53C400 or equivalent device.
  */
-
 static inline int generic_NCR5380_psend(struct NCR5380_hostdata *hostdata,
                                         unsigned char *src, int len)
 {
@@ -597,7 +604,8 @@ static inline int generic_NCR5380_psend(struct NCR5380_hostdata *hostdata,
 		                           CSR_HOST_BUF_NOT_RDY, 0,
 		                           hostdata->c400_ctl_status,
 		                           CSR_GATED_53C80_IRQ,
-		                           CSR_GATED_53C80_IRQ, HZ / 64) < 0 ||
+		                           CSR_GATED_53C80_IRQ, HZ / 64,
+					   false) < 0 ||
 		    NCR5380_read(hostdata->c400_ctl_status) & CSR_HOST_BUF_NOT_RDY) {
 			/* Both 128 B buffers are in use */
 			if (start >= 128)
@@ -644,13 +652,15 @@ static inline int generic_NCR5380_psend(struct NCR5380_hostdata *hostdata,
 	if (residual == 0) {
 		if (NCR5380_poll_politely(hostdata, TARGET_COMMAND_REG,
 		                          TCR_LAST_BYTE_SENT, TCR_LAST_BYTE_SENT,
-		                          HZ / 64) < 0)
+		                          HZ / 64,
+					  false) < 0)
 			scmd_printk(KERN_ERR, hostdata->connected,
 			            "%s: Last Byte Sent timeout\n", __func__);
 
 		if (NCR5380_poll_politely(hostdata, BUS_AND_STATUS_REG,
 		                          BASR_END_DMA_TRANSFER, BASR_END_DMA_TRANSFER,
-		                          HZ / 64) < 0)
+		                          HZ / 64,
+					  false) < 0)
 			scmd_printk(KERN_ERR, hostdata->connected, "%s: End of DMA timeout\n",
 			            __func__);
 	}
diff --git a/drivers/scsi/mac_scsi.c b/drivers/scsi/mac_scsi.c
index b5dde9d0d0545..3c39db74fd847 100644
--- a/drivers/scsi/mac_scsi.c
+++ b/drivers/scsi/mac_scsi.c
@@ -285,7 +285,7 @@ static inline int macscsi_pread(struct NCR5380_hostdata *hostdata,
 
 	while (!NCR5380_poll_politely(hostdata, BUS_AND_STATUS_REG,
 	                              BASR_DRQ | BASR_PHASE_MATCH,
-	                              BASR_DRQ | BASR_PHASE_MATCH, HZ / 64)) {
+	                              BASR_DRQ | BASR_PHASE_MATCH, HZ / 64, false)) {
 		int bytes;
 
 		if (macintosh_config->ident == MAC_MODEL_IIFX)
@@ -304,7 +304,7 @@ static inline int macscsi_pread(struct NCR5380_hostdata *hostdata,
 
 		if (NCR5380_poll_politely2(hostdata, STATUS_REG, SR_REQ, SR_REQ,
 		                           BUS_AND_STATUS_REG, BASR_ACK,
-		                           BASR_ACK, HZ / 64) < 0)
+		                           BASR_ACK, HZ / 64, false) < 0)
 			scmd_printk(KERN_DEBUG, hostdata->connected,
 			            "%s: !REQ and !ACK\n", __func__);
 		if (!(NCR5380_read(BUS_AND_STATUS_REG) & BASR_PHASE_MATCH))
@@ -344,7 +344,7 @@ static inline int macscsi_pwrite(struct NCR5380_hostdata *hostdata,
 
 	while (!NCR5380_poll_politely(hostdata, BUS_AND_STATUS_REG,
 	                              BASR_DRQ | BASR_PHASE_MATCH,
-	                              BASR_DRQ | BASR_PHASE_MATCH, HZ / 64)) {
+	                              BASR_DRQ | BASR_PHASE_MATCH, HZ / 64, false)) {
 		int bytes;
 
 		if (macintosh_config->ident == MAC_MODEL_IIFX)
@@ -362,7 +362,7 @@ static inline int macscsi_pwrite(struct NCR5380_hostdata *hostdata,
 			if (NCR5380_poll_politely(hostdata, TARGET_COMMAND_REG,
 			                          TCR_LAST_BYTE_SENT,
 			                          TCR_LAST_BYTE_SENT,
-			                          HZ / 64) < 0) {
+			                          HZ / 64, false) < 0) {
 				scmd_printk(KERN_ERR, hostdata->connected,
 				            "%s: Last Byte Sent timeout\n", __func__);
 				result = -1;
@@ -372,7 +372,7 @@ static inline int macscsi_pwrite(struct NCR5380_hostdata *hostdata,
 
 		if (NCR5380_poll_politely2(hostdata, STATUS_REG, SR_REQ, SR_REQ,
 		                           BUS_AND_STATUS_REG, BASR_ACK,
-		                           BASR_ACK, HZ / 64) < 0)
+		                           BASR_ACK, HZ / 64, false) < 0)
 			scmd_printk(KERN_DEBUG, hostdata->connected,
 			            "%s: !REQ and !ACK\n", __func__);
 		if (!(NCR5380_read(BUS_AND_STATUS_REG) & BASR_PHASE_MATCH))
-- 
2.29.2

Re: [PATCH 12/14] scsi: NCR5380: Remove in_interrupt().

[Finn Thain]

On Thu, 26 Nov 2020, Sebastian Andrzej Siewior wrote:

> From: "Ahmed S. Darwish" <a.darwish@linutronix.de>
> 
> NCR5380_poll_politely2() uses in_interrupt() to check if it is safe to
> sleep.
> 
> The usage of in_interrupt() in drivers is phased out and Linus clearly
> requested that code which changes behaviour depending on context should
> either be separated, or the context be explicitly conveyed in an
> argument passed by the caller.
> 
> Below is a context analysis of NCR5380_poll_politely2() uppermost
> callers:
> 
>   - NCR5380_maybe_reset_bus(), task, invoked during device probe.
>     -> NCR5380_poll_politely()
>     -> do_abort()
> 
>   - NCR5380_select(), task, but can only sleep in the "release, then
>     re-acquire" regions of the spinlock held by its caller.
>     Sleeping invocations (lock released):
>     -> NCR5380_poll_politely2()
> 
>     Atomic invocations (lock acquired):
>     -> NCR5380_reselect()
>        -> NCR5380_poll_politely()
>        -> do_abort()
>        -> NCR5380_transfer_pio()
> 
>   - NCR5380_intr(), interrupt handler
>     -> NCR5380_dma_complete()
>        -> NCR5380_transfer_pio()
> 	  -> NCR5380_poll_politely()
>     -> NCR5380_reselect() (see above)
> 
>   - NCR5380_information_transfer(), task, but can only sleep in the
>     "release, then re-acquire" regions of the caller-held spinlock.
>     Sleeping invocations (lock released):
>       - NCR5380_transfer_pio() -> NCR5380_poll_politely()
>       - NCR5380_poll_politely()
> 
>     Atomic invocations (lock acquired):
>       - NCR5380_transfer_dma()
> 	-> NCR5380_dma_recv_setup()
>            => generic_NCR5380_precv() -> NCR5380_poll_politely()
> 	   => macscsi_pread() -> NCR5380_poll_politely()
> 
> 	-> NCR5380_dma_send_setup()
>  	   => generic_NCR5380_psend -> NCR5380_poll_politely2()
> 	   => macscsi_pwrite() -> NCR5380_poll_politely()
> 
> 	-> NCR5380_poll_politely2()
>         -> NCR5380_dma_complete()
>            -> NCR5380_transfer_pio()
> 	      -> NCR5380_poll_politely()
>       - NCR5380_transfer_pio() -> NCR5380_poll_politely
> 
>   - NCR5380_reselect(), atomic, always called with hostdata spinlock
>     held.
> 
> If direct callers are purely atomic, or purely task context, change
> their specifications accordingly and mark them with "Context: " tags.
> 
> For the mixed ones, trickle-down context from upper layers.
> 
> Signed-off-by: Ahmed S. Darwish <a.darwish@linutronix.de>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Acked-by: Finn Thain <fthain@telegraphics.com.au>

> @@ -513,9 +513,11 @@ static void wait_for_53c80_access(struct NCR5380_hostdata *hostdata)
>   * @dst: buffer to write into
>   * @len: transfer size
>   *
> + * Context: atomic. This implements NCR5380.c NCR5380_dma_recv_setup(),
> + * which is always called with @hostdata spinlock held.
> + *
>   * Perform a pseudo DMA mode receive from a 53C400 or equivalent device.
>   */
> -
>  static inline int generic_NCR5380_precv(struct NCR5380_hostdata *hostdata,
>                                          unsigned char *dst, int len)
>  {

BTW, if I was doing this, I'd omit all of the many gratuitous whitespace 
changes and I'd avoid copying so much program logic into the comments 
where it is redundant -- here I'd have written only "Context: atomic, 
spinlock held". However, no need to revise. Looks fine otherwise.

Re: [PATCH 12/14] scsi: NCR5380: Remove in_interrupt().

[Finn Thain]

On Fri, 27 Nov 2020, Finn Thain wrote:

> 
> On Thu, 26 Nov 2020, Sebastian Andrzej Siewior wrote:
> 
> > From: "Ahmed S. Darwish" <a.darwish@linutronix.de>
> > 
> > NCR5380_poll_politely2() uses in_interrupt() to check if it is safe to
> > sleep.
> > 
> > The usage of in_interrupt() in drivers is phased out and Linus clearly
> > requested that code which changes behaviour depending on context should
> > either be separated, or the context be explicitly conveyed in an
> > argument passed by the caller.
> > 
> > Below is a context analysis of NCR5380_poll_politely2() uppermost
> > callers:
> > 
> >   - NCR5380_maybe_reset_bus(), task, invoked during device probe.
> >     -> NCR5380_poll_politely()
> >     -> do_abort()
> > 
> >   - NCR5380_select(), task, but can only sleep in the "release, then
> >     re-acquire" regions of the spinlock held by its caller.
> >     Sleeping invocations (lock released):
> >     -> NCR5380_poll_politely2()
> > 
> >     Atomic invocations (lock acquired):
> >     -> NCR5380_reselect()
> >        -> NCR5380_poll_politely()
> >        -> do_abort()
> >        -> NCR5380_transfer_pio()
> > 
> >   - NCR5380_intr(), interrupt handler
> >     -> NCR5380_dma_complete()
> >        -> NCR5380_transfer_pio()
> > 	  -> NCR5380_poll_politely()
> >     -> NCR5380_reselect() (see above)
> > 
> >   - NCR5380_information_transfer(), task, but can only sleep in the
> >     "release, then re-acquire" regions of the caller-held spinlock.
> >     Sleeping invocations (lock released):
> >       - NCR5380_transfer_pio() -> NCR5380_poll_politely()
> >       - NCR5380_poll_politely()
> > 
> >     Atomic invocations (lock acquired):
> >       - NCR5380_transfer_dma()
> > 	-> NCR5380_dma_recv_setup()
> >            => generic_NCR5380_precv() -> NCR5380_poll_politely()
> > 	   => macscsi_pread() -> NCR5380_poll_politely()
> > 
> > 	-> NCR5380_dma_send_setup()
> >  	   => generic_NCR5380_psend -> NCR5380_poll_politely2()
> > 	   => macscsi_pwrite() -> NCR5380_poll_politely()
> > 
> > 	-> NCR5380_poll_politely2()
> >         -> NCR5380_dma_complete()
> >            -> NCR5380_transfer_pio()
> > 	      -> NCR5380_poll_politely()
> >       - NCR5380_transfer_pio() -> NCR5380_poll_politely
> > 
> >   - NCR5380_reselect(), atomic, always called with hostdata spinlock
> >     held.
> > 
> > If direct callers are purely atomic, or purely task context, change
> > their specifications accordingly and mark them with "Context: " tags.
> > 
> > For the mixed ones, trickle-down context from upper layers.
> > 
> > Signed-off-by: Ahmed S. Darwish <a.darwish@linutronix.de>
> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> 
> Acked-by: Finn Thain <fthain@telegraphics.com.au>
> 

On second thoughts, have you considered this patch instead?

diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c
index d654a6cc4162..739def70cffb 100644
--- a/drivers/scsi/NCR5380.c
+++ b/drivers/scsi/NCR5380.c
@@ -223,7 +223,10 @@ static int NCR5380_poll_politely2(struct NCR5380_hostdata *hostdata,
 		cpu_relax();
 	} while (n--);
 
-	if (irqs_disabled() || in_interrupt())
+	/* We can't sleep when local irqs are disabled and callers ensure
+	 * that local irqs are disabled whenever we can't sleep.
+	 */
+	if (irqs_disabled())
 		return -ETIMEDOUT;
 
 	/* Repeatedly sleep for 1 ms until deadline */

> > @@ -513,9 +513,11 @@ static void wait_for_53c80_access(struct NCR5380_hostdata *hostdata)
> >   * @dst: buffer to write into
> >   * @len: transfer size
> >   *
> > + * Context: atomic. This implements NCR5380.c NCR5380_dma_recv_setup(),
> > + * which is always called with @hostdata spinlock held.
> > + *
> >   * Perform a pseudo DMA mode receive from a 53C400 or equivalent device.
> >   */
> > -
> >  static inline int generic_NCR5380_precv(struct NCR5380_hostdata *hostdata,
> >                                          unsigned char *dst, int len)
> >  {
> 
> BTW, if I was doing this, I'd omit all of the many gratuitous whitespace 
> changes and I'd avoid copying so much program logic into the comments 
> where it is redundant -- here I'd have written only "Context: atomic, 
> spinlock held". However, no need to revise. Looks fine otherwise.
> 

Also BTW, the phrase "may sleep" expresses that the caller permits the 
callee to sleep. Whereas, "can sleep" expresses that the callee is put on 
notice by the caller that sleeping is a possibility. And since the caller 
determines the actual true or false value, and the callee determines 
whether sleeping actually takes place, only the former phrase makes sense.

Re: [PATCH 12/14] scsi: NCR5380: Remove in_interrupt().

[Finn Thain]

On Sat, 28 Nov 2020, Finn Thain wrote:

> 
> diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c
> index d654a6cc4162..739def70cffb 100644
> --- a/drivers/scsi/NCR5380.c
> +++ b/drivers/scsi/NCR5380.c
> @@ -223,7 +223,10 @@ static int NCR5380_poll_politely2(struct NCR5380_hostdata *hostdata,
>  		cpu_relax();
>  	} while (n--);
>  
> -	if (irqs_disabled() || in_interrupt())
> +	/* We can't sleep when local irqs are disabled and callers ensure
> +	 * that local irqs are disabled whenever we can't sleep.
> +	 */
> +	if (irqs_disabled())
>  		return -ETIMEDOUT;
>  
>  	/* Repeatedly sleep for 1 ms until deadline */
> 

Michael, Andreas, would you please confirm that this is workable on Atari? 
The driver could sleep when IPL == 2 because arch_irqs_disabled_flags() 
would return false (on Atari). I'm wondering whether that would deadlock.

Re: [PATCH 12/14] scsi: NCR5380: Remove in_interrupt().

[Ahmed S. Darwish]

On Sat, Nov 28, 2020 at 08:48:00AM +1100, Finn Thain wrote:
>
> On Sat, 28 Nov 2020, Finn Thain wrote:
>
> >
> > diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c
> > index d654a6cc4162..739def70cffb 100644
> > --- a/drivers/scsi/NCR5380.c
> > +++ b/drivers/scsi/NCR5380.c
> > @@ -223,7 +223,10 @@ static int NCR5380_poll_politely2(struct NCR5380_hostdata *hostdata,
> >  		cpu_relax();
> >  	} while (n--);
> >
> > -	if (irqs_disabled() || in_interrupt())
> > +	/* We can't sleep when local irqs are disabled and callers ensure
> > +	 * that local irqs are disabled whenever we can't sleep.
> > +	 */
> > +	if (irqs_disabled())
> >  		return -ETIMEDOUT;
> >
> >  	/* Repeatedly sleep for 1 ms until deadline */
> >
>
> Michael, Andreas, would you please confirm that this is workable on Atari?
> The driver could sleep when IPL == 2 because arch_irqs_disabled_flags()
> would return false (on Atari). I'm wondering whether that would deadlock.

Please re-check the commit log:

  "Linus clearly requested that code which changes behaviour depending
   on context should either be separated, or the context be explicitly
   conveyed in an argument passed by the caller."

So, sorry, drivers shouldn't do context-dependent dances anymore.

For more context (no pun intended), please check the thread mentioned in
the cover letter, and also below message:

  https://lkml.kernel.org/r/CAKMK7uHAk9-Vy2cof0ws=DrcD52GHiCDiyHbjLd19CgpBU2rKQ@mail.gmail.com

Kind regards,

--
Ahmed S. Darwish
Linutronix GmbH

Re: [PATCH 12/14] scsi: NCR5380: Remove in_interrupt().

[Finn Thain]

On Sat, 28 Nov 2020, Ahmed S. Darwish wrote:

> On Sat, Nov 28, 2020 at 08:48:00AM +1100, Finn Thain wrote:
> >
> > On Sat, 28 Nov 2020, Finn Thain wrote:
> >
> > >
> > > diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c
> > > index d654a6cc4162..739def70cffb 100644
> > > --- a/drivers/scsi/NCR5380.c
> > > +++ b/drivers/scsi/NCR5380.c
> > > @@ -223,7 +223,10 @@ static int NCR5380_poll_politely2(struct NCR5380_hostdata *hostdata,
> > >  		cpu_relax();
> > >  	} while (n--);
> > >
> > > -	if (irqs_disabled() || in_interrupt())
> > > +	/* We can't sleep when local irqs are disabled and callers ensure
> > > +	 * that local irqs are disabled whenever we can't sleep.
> > > +	 */
> > > +	if (irqs_disabled())
> > >  		return -ETIMEDOUT;
> > >
> > >  	/* Repeatedly sleep for 1 ms until deadline */
> > >
> >
> > Michael, Andreas, would you please confirm that this is workable on Atari?
> > The driver could sleep when IPL == 2 because arch_irqs_disabled_flags()
> > would return false (on Atari). I'm wondering whether that would deadlock.
> 
> Please re-check the commit log:
> 
>   "Linus clearly requested that code which changes behaviour depending
>    on context should either be separated, or the context be explicitly
>    conveyed in an argument passed by the caller."
> 

Yes, I knew about the discussion around the issues with preempt_count() 
and CONFIG_PREEMPT. And I don't have any problem with removing 
in_interrupt(), as you can see from my patch.

> So, sorry, drivers shouldn't do context-dependent dances anymore.
> 

I don't know what is meant by 'context-dependent'. I suspect that it's 
left ill-defined because there are many cases where global state is 
needed, such as those mentioned in the thread you cited, like the 
memalloc_no*() calls. See also, in_compat_syscall().

> For more context (no pun intended), please check the thread mentioned in
> the cover letter, and also below message:
> 
>   https://lkml.kernel.org/r/CAKMK7uHAk9-Vy2cof0ws=DrcD52GHiCDiyHbjLd19CgpBU2rKQ@mail.gmail.com
> 

Are you also planning to remove spin_lock_irqsave/restore() and replace 
these with spin_lock_irq/unlock_irq()? And if not, why do you object to 
irqs_disabled()?

Please also compare your patch and mine with regard to stack usage, 
readability and code size. Also consider that adding a new argument to all 
those functions creates new opportunities for mistakes in new callers.

> Kind regards,
> 
> --
> Ahmed S. Darwish
> Linutronix GmbH
> 

Re: [PATCH 12/14] scsi: NCR5380: Remove in_interrupt().

[Michael Schmitz]

Hi Finn,

Am 28.11.20 um 10:48 schrieb Finn Thain:
> On Sat, 28 Nov 2020, Finn Thain wrote:
>
>> diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c
>> index d654a6cc4162..739def70cffb 100644
>> --- a/drivers/scsi/NCR5380.c
>> +++ b/drivers/scsi/NCR5380.c
>> @@ -223,7 +223,10 @@ static int NCR5380_poll_politely2(struct NCR5380_hostdata *hostdata,
>>  		cpu_relax();
>>  	} while (n--);
>>  
>> -	if (irqs_disabled() || in_interrupt())
>> +	/* We can't sleep when local irqs are disabled and callers ensure
>> +	 * that local irqs are disabled whenever we can't sleep.
>> +	 */
>> +	if (irqs_disabled())
>>  		return -ETIMEDOUT;
>>  
>>  	/* Repeatedly sleep for 1 ms until deadline */
>>
> Michael, Andreas, would you please confirm that this is workable on Atari? 
> The driver could sleep when IPL == 2 because arch_irqs_disabled_flags() 
> would return false (on Atari). I'm wondering whether that would deadlock.

Pretty sure this would deadlock when in interrupt context here.
Otherwise, IPL 2 is perfectly OK (which is why
arch_irqs_disabled_flags() returns false in that case).

If you want to be 100% certain, I can give this one a spin.

Cheers,

    Michael

Re: [PATCH 12/14] scsi: NCR5380: Remove in_interrupt().

[Finn Thain]

[-- Attachment #1: Type: text/plain, Size: 1571 bytes --]

On Sun, 29 Nov 2020, Michael Schmitz wrote:

> Am 28.11.20 um 10:48 schrieb Finn Thain:
> > On Sat, 28 Nov 2020, Finn Thain wrote:
> >
> >> diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c
> >> index d654a6cc4162..739def70cffb 100644
> >> --- a/drivers/scsi/NCR5380.c
> >> +++ b/drivers/scsi/NCR5380.c
> >> @@ -223,7 +223,10 @@ static int NCR5380_poll_politely2(struct NCR5380_hostdata *hostdata,
> >>  		cpu_relax();
> >>  	} while (n--);
> >>  
> >> -	if (irqs_disabled() || in_interrupt())
> >> +	/* We can't sleep when local irqs are disabled and callers ensure
> >> +	 * that local irqs are disabled whenever we can't sleep.
> >> +	 */
> >> +	if (irqs_disabled())
> >>  		return -ETIMEDOUT;
> >>  
> >>  	/* Repeatedly sleep for 1 ms until deadline */
> >>
> > Michael, Andreas, would you please confirm that this is workable on 
> > Atari? The driver could sleep when IPL == 2 because 
> > arch_irqs_disabled_flags() would return false (on Atari). I'm 
> > wondering whether that would deadlock.
> 
> Pretty sure this would deadlock when in interrupt context here.

When in interrupt context, irqs_disabled() is true due to 
spinlock_irqsave/restore() in NCR5380_intr().

My question was really about what would happen if we sleep with IPL == 2.

> Otherwise, IPL 2 is perfectly OK (which is why 
> arch_irqs_disabled_flags() returns false in that case).
> 
> If you want to be 100% certain, I can give this one a spin.
> 

Please only test it if you think it will work.

> Cheers,
> 
>     Michael
> 
> 

Re: [PATCH 12/14] scsi: NCR5380: Remove in_interrupt().

[Michael Schmitz]

Hi Finn,

Am 30.11.2020 um 13:15 schrieb Finn Thain:
> On Sun, 29 Nov 2020, Michael Schmitz wrote:
>
>> Am 28.11.20 um 10:48 schrieb Finn Thain:
>>> On Sat, 28 Nov 2020, Finn Thain wrote:
>>>
>>>> diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c
>>>> index d654a6cc4162..739def70cffb 100644
>>>> --- a/drivers/scsi/NCR5380.c
>>>> +++ b/drivers/scsi/NCR5380.c
>>>> @@ -223,7 +223,10 @@ static int NCR5380_poll_politely2(struct NCR5380_hostdata *hostdata,
>>>>  		cpu_relax();
>>>>  	} while (n--);
>>>>
>>>> -	if (irqs_disabled() || in_interrupt())
>>>> +	/* We can't sleep when local irqs are disabled and callers ensure
>>>> +	 * that local irqs are disabled whenever we can't sleep.
>>>> +	 */
>>>> +	if (irqs_disabled())
>>>>  		return -ETIMEDOUT;
>>>>
>>>>  	/* Repeatedly sleep for 1 ms until deadline */
>>>>
>>> Michael, Andreas, would you please confirm that this is workable on
>>> Atari? The driver could sleep when IPL == 2 because
>>> arch_irqs_disabled_flags() would return false (on Atari). I'm
>>> wondering whether that would deadlock.
>>
>> Pretty sure this would deadlock when in interrupt context here.
>
> When in interrupt context, irqs_disabled() is true due to
> spinlock_irqsave/restore() in NCR5380_intr().

OK.

> My question was really about what would happen if we sleep with IPL == 2.

All relevant system interrupts are at higher priority (5 or 6). Both 
timer and SCSI / DMA completion interrupt in particular are at IPL 6 and 
won't be blocked when sleeping with IPL == 2. That's what I meant by 
'IPL 2 is perfectly OK' below.

>> Otherwise, IPL 2 is perfectly OK (which is why
>> arch_irqs_disabled_flags() returns false in that case).
>>
>> If you want to be 100% certain, I can give this one a spin.
>>
>
> Please only test it if you think it will work.

With your explanation above, I'm now quite certain your patch will work. 
I've not seen deadlocks in softirq context since you rewrote the driver 
so it will no more sleep waiting for the ST-DMA lock.

Cheers,

	Michael

[PATCH 13/14] scsi: message: fusion: Remove in_interrupt() usage in mpt_config().

[Sebastian Andrzej Siewior]

From: Thomas Gleixner <tglx@linutronix.de>

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().

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Sathya Prakash <sathya.prakash@broadcom.com>
Cc: Sreekanth Reddy <sreekanth.reddy@broadcom.com>
Cc: Suganath Prabu Subramani <suganath-prabu.subramani@broadcom.com>
Cc: MPT-FusionLinux.pdl@broadcom.com
---
 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(-)

diff --git a/drivers/message/fusion/mptbase.c b/drivers/message/fusion/mptbase.c
index 3078fac34e51f..549797d0301d8 100644
--- a/drivers/message/fusion/mptbase.c
+++ b/drivers/message/fusion/mptbase.c
@@ -57,7 +57,7 @@
 #include <linux/kdev_t.h>
 #include <linux/blkdev.h>
 #include <linux/delay.h>
-#include <linux/interrupt.h>		/* needed for in_interrupt() proto */
+#include <linux/interrupt.h>
 #include <linux/dma-mapping.h>
 #include <linux/kthread.h>
 #include <scsi/scsi_host.h>
@@ -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 f92b0433f599f..0484e9c15c097 100644
--- a/drivers/message/fusion/mptfc.c
+++ b/drivers/message/fusion/mptfc.c
@@ -50,7 +50,7 @@
 #include <linux/kdev_t.h>
 #include <linux/blkdev.h>
 #include <linux/delay.h>	/* for mdelay */
-#include <linux/interrupt.h>	/* needed for in_interrupt() proto */
+#include <linux/interrupt.h>
 #include <linux/reboot.h>	/* notifier code */
 #include <linux/workqueue.h>
 #include <linux/sort.h>
diff --git a/drivers/message/fusion/mptscsih.c b/drivers/message/fusion/mptscsih.c
index e7f0d4ae0f960..ce2e5b21978e2 100644
--- a/drivers/message/fusion/mptscsih.c
+++ b/drivers/message/fusion/mptscsih.c
@@ -52,7 +52,7 @@
 #include <linux/kdev_t.h>
 #include <linux/blkdev.h>
 #include <linux/delay.h>	/* for mdelay */
-#include <linux/interrupt.h>	/* needed for in_interrupt() proto */
+#include <linux/interrupt.h>
 #include <linux/reboot.h>	/* notifier code */
 #include <linux/workqueue.h>
 
diff --git a/drivers/message/fusion/mptspi.c b/drivers/message/fusion/mptspi.c
index eabc4de5816cb..af0ce5611e4ac 100644
--- a/drivers/message/fusion/mptspi.c
+++ b/drivers/message/fusion/mptspi.c
@@ -52,7 +52,7 @@
 #include <linux/kdev_t.h>
 #include <linux/blkdev.h>
 #include <linux/delay.h>	/* for mdelay */
-#include <linux/interrupt.h>	/* needed for in_interrupt() proto */
+#include <linux/interrupt.h>
 #include <linux/reboot.h>	/* notifier code */
 #include <linux/workqueue.h>
 #include <linux/raid_class.h>
-- 
2.29.2

Re: [PATCH 13/14] scsi: message: fusion: Remove in_interrupt() usage in mpt_config().

[Daniel Wagner]

On Thu, Nov 26, 2020 at 02:29:51PM +0100, Sebastian Andrzej Siewior wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> 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().
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Cc: Sathya Prakash <sathya.prakash@broadcom.com>
> Cc: Sreekanth Reddy <sreekanth.reddy@broadcom.com>
> Cc: Suganath Prabu Subramani <suganath-prabu.subramani@broadcom.com>
> Cc: MPT-FusionLinux.pdl@broadcom.com

Reviewed-by: Daniel Wagner <dwagner@suse.de>

[PATCH 14/14] scsi: message: fusion: Remove in_interrupt() usage in mptsas_cleanup_fw_event_q().

[Sebastian Andrzej Siewior]

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.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Sathya Prakash <sathya.prakash@broadcom.com>
Cc: Sreekanth Reddy <sreekanth.reddy@broadcom.com>
Cc: Suganath Prabu Subramani <suganath-prabu.subramani@broadcom.com>
Cc: MPT-FusionLinux.pdl@broadcom.com
---
 drivers/message/fusion/mptsas.c | 45 +++++++++++++++++++++++++--------
 drivers/message/fusion/mptsas.h |  1 +
 2 files changed, 36 insertions(+), 10 deletions(-)

diff --git a/drivers/message/fusion/mptsas.c b/drivers/message/fusion/mptsas.c
index 18b91ea1a353f..5eb0b3361e4e0 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 e35b13891fe42..71abf3477495e 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;
-- 
2.29.2

Re: [PATCH 14/14] scsi: message: fusion: Remove in_interrupt() usage in mptsas_cleanup_fw_event_q().

[Daniel Wagner]

On Thu, Nov 26, 2020 at 02:29:52PM +0100, Sebastian Andrzej Siewior wrote:
> 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().

mpt_diag_reset(), iterates over all reset handler...

> All these functions have a `sleepFlag'

...and also uses the same flag.

> 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.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Cc: Sathya Prakash <sathya.prakash@broadcom.com>
> Cc: Sreekanth Reddy <sreekanth.reddy@broadcom.com>
> Cc: Suganath Prabu Subramani <suganath-prabu.subramani@broadcom.com>
> Cc: MPT-FusionLinux.pdl@broadcom.com

Reviewed-by: Daniel Wagner <dwagner@suse.de>

Re: [PATCH 00/14] scsi: Remove in_interrupt() usage.

[Martin K. Petersen]

Sebastian,

> This series addresses most of the SCSI subsystem.  The first three
> patches have Fixes tags and address bugs were noticed during review.

Applied series to 5.11/scsi-staging except for the NCR patch. Would like
to see where that lands first.

Thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH 00/14] scsi: Remove in_interrupt() usage.

[Sebastian Andrzej Siewior]

On 2020-12-01 00:06:55 [-0500], Martin K. Petersen wrote:
>
> Applied series to 5.11/scsi-staging except for the NCR patch. Would like
> to see where that lands first.

Thank you Martin.

> Thanks!

Sebastian

Re: [PATCH 00/14] scsi: Remove in_interrupt() usage.

[Martin K. Petersen]

On Thu, 26 Nov 2020 14:29:38 +0100, Sebastian Andrzej Siewior wrote:

> Folks,
> 
> in the discussion about preempt count consistency across kernel
> configurations:
> 
>  https://lore.kernel.org/r/20200914204209.256266093@linutronix.de/
> 
> [...]

Applied to 5.11/scsi-queue, thanks!

[01/14] scsi: pm80xx: Do not sleep in atomic context.
        https://git.kernel.org/mkp/scsi/c/4ba9e516573e
[02/14] scsi: hisi_sas: Remove preemptible().
        https://git.kernel.org/mkp/scsi/c/18577cdcaeeb
[03/14] scsi: qla4xxx: qla4_82xx_crb_win_lock(): Remove in_interrupt().
        https://git.kernel.org/mkp/scsi/c/a93c38353198
[04/14] scsi: qla2xxx: qla82xx: Remove in_interrupt().
        https://git.kernel.org/mkp/scsi/c/8ac246bdd07a
[05/14] scsi: qla2xxx: tcm_qla2xxx: Remove BUG_ON(in_interrupt()).
        https://git.kernel.org/mkp/scsi/c/9fef41f25d60
[06/14] scsi: qla2xxx: init/os: Remove in_interrupt().
        https://git.kernel.org/mkp/scsi/c/4f6a57c23b1e
[07/14] scsi: qla4xxx: qla4_82xx_idc_lock(): Remove in_interrupt().
        https://git.kernel.org/mkp/scsi/c/3627668c2e2c
[08/14] scsi: qla4xxx: qla4_82xx_rom_lock(): Remove in_interrupt().
        https://git.kernel.org/mkp/scsi/c/014aced18aff
[09/14] scsi: mpt3sas: Remove in_interrupt().
        https://git.kernel.org/mkp/scsi/c/547c0d1aeb76
[10/14] scsi: myrb: Remove WARN_ON(in_interrupt()).
        https://git.kernel.org/mkp/scsi/c/3bc08b9545da
[11/14] scsi: myrs: Remove WARN_ON(in_interrupt()).
        https://git.kernel.org/mkp/scsi/c/ca6853693cbd
[12/14] scsi: NCR5380: Remove in_interrupt().
        (no commit info)
[13/14] scsi: message: fusion: Remove in_interrupt() usage in mpt_config().
        https://git.kernel.org/mkp/scsi/c/b8a5144370bc
[14/14] scsi: message: fusion: Remove in_interrupt() usage in mptsas_cleanup_fw_event_q().
        https://git.kernel.org/mkp/scsi/c/817a7c996786

-- 
Martin K. Petersen	Oracle Linux Engineering