Re: ipr crashes due to NULL dma_need_drain since cc97923a5bcc ("block: move dma drain handling to scsi")

2020-06-09 Thread Michael Ellerman 
Christoph Hellwig  writes:
> Can you try this patch?
>
> ---
> From 1c9913360a0494375c5655b133899cb4323bceb4 Mon Sep 17 00:00:00 2001
> From: Christoph Hellwig 
> Date: Tue, 9 Jun 2020 14:07:31 +0200
> Subject: scsi: wire up ata_scsi_dma_need_drain for SAS HBA drivers
>
> We need ata_scsi_dma_need_drain for all drivers wired up to drive ATAPI
> devices through libata.  That also includes the SAS HBA drivers in
> addition to native libata HBA drivers.
>
> Fixes: cc97923a5bcc ("block: move dma drain handling to scsi")
> Reported-by: Michael Ellerman 
> Signed-off-by: Christoph Hellwig 

Yep that works for me here with ipr.

Tested-by: Michael Ellerman 

cheers

Re: ipr crashes due to NULL dma_need_drain since cc97923a5bcc ("block: move dma drain handling to scsi")

2020-06-09 Thread Christoph Hellwig 
Can you try this patch?

---
>From 1c9913360a0494375c5655b133899cb4323bceb4 Mon Sep 17 00:00:00 2001
From: Christoph Hellwig 
Date: Tue, 9 Jun 2020 14:07:31 +0200
Subject: scsi: wire up ata_scsi_dma_need_drain for SAS HBA drivers

We need ata_scsi_dma_need_drain for all drivers wired up to drive ATAPI
devices through libata.  That also includes the SAS HBA drivers in
addition to native libata HBA drivers.

Fixes: cc97923a5bcc ("block: move dma drain handling to scsi")
Reported-by: Michael Ellerman 
Signed-off-by: Christoph Hellwig 
---
 drivers/scsi/aic94xx/aic94xx_init.c| 1 +
 drivers/scsi/hisi_sas/hisi_sas_v1_hw.c | 1 +
 drivers/scsi/hisi_sas/hisi_sas_v2_hw.c | 1 +
 drivers/scsi/hisi_sas/hisi_sas_v3_hw.c | 1 +
 drivers/scsi/ipr.c | 1 +
 drivers/scsi/isci/init.c   | 1 +
 drivers/scsi/mvsas/mv_init.c   | 1 +
 drivers/scsi/pm8001/pm8001_init.c  | 1 +
 8 files changed, 8 insertions(+)

diff --git a/drivers/scsi/aic94xx/aic94xx_init.c 
b/drivers/scsi/aic94xx/aic94xx_init.c
index d022407e5645c7..bef47f38dd0dbc 100644
--- a/drivers/scsi/aic94xx/aic94xx_init.c
+++ b/drivers/scsi/aic94xx/aic94xx_init.c
@@ -40,6 +40,7 @@ static struct scsi_host_template aic94xx_sht = {
/* .name is initialized */
.name   = "aic94xx",
.queuecommand   = sas_queuecommand,
+   .dma_need_drain = ata_scsi_dma_need_drain,
.target_alloc   = sas_target_alloc,
.slave_configure= sas_slave_configure,
.scan_finished  = asd_scan_finished,
diff --git a/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c 
b/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c
index 2e1718f9ade218..09a7669dad4c67 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c
@@ -1756,6 +1756,7 @@ static struct scsi_host_template sht_v1_hw = {
.proc_name  = DRV_NAME,
.module = THIS_MODULE,
.queuecommand   = sas_queuecommand,
+   .dma_need_drain = ata_scsi_dma_need_drain,
.target_alloc   = sas_target_alloc,
.slave_configure= hisi_sas_slave_configure,
.scan_finished  = hisi_sas_scan_finished,
diff --git a/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c 
b/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
index e7e7849a4c14e2..968d3870235359 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
@@ -3532,6 +3532,7 @@ static struct scsi_host_template sht_v2_hw = {
.proc_name  = DRV_NAME,
.module = THIS_MODULE,
.queuecommand   = sas_queuecommand,
+   .dma_need_drain = ata_scsi_dma_need_drain,
.target_alloc   = sas_target_alloc,
.slave_configure= hisi_sas_slave_configure,
.scan_finished  = hisi_sas_scan_finished,
diff --git a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c 
b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
index 3e6b78a1f993b9..55e2321a65bc5f 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
@@ -3075,6 +3075,7 @@ static struct scsi_host_template sht_v3_hw = {
.proc_name  = DRV_NAME,
.module = THIS_MODULE,
.queuecommand   = sas_queuecommand,
+   .dma_need_drain = ata_scsi_dma_need_drain,
.target_alloc   = sas_target_alloc,
.slave_configure= hisi_sas_slave_configure,
.scan_finished  = hisi_sas_scan_finished,
diff --git a/drivers/scsi/ipr.c b/drivers/scsi/ipr.c
index 7d77997d26d457..7d86f4ca266c86 100644
--- a/drivers/scsi/ipr.c
+++ b/drivers/scsi/ipr.c
@@ -6731,6 +6731,7 @@ static struct scsi_host_template driver_template = {
.compat_ioctl = ipr_ioctl,
 #endif
.queuecommand = ipr_queuecommand,
+   .dma_need_drain = ata_scsi_dma_need_drain,
.eh_abort_handler = ipr_eh_abort,
.eh_device_reset_handler = ipr_eh_dev_reset,
.eh_host_reset_handler = ipr_eh_host_reset,
diff --git a/drivers/scsi/isci/init.c b/drivers/scsi/isci/init.c
index 974c3b9116d5ba..085e285f427d93 100644
--- a/drivers/scsi/isci/init.c
+++ b/drivers/scsi/isci/init.c
@@ -153,6 +153,7 @@ static struct scsi_host_template isci_sht = {
.name   = DRV_NAME,
.proc_name  = DRV_NAME,
.queuecommand   = sas_queuecommand,
+   .dma_need_drain = ata_scsi_dma_need_drain,
.target_alloc   = sas_target_alloc,
.slave_configure= sas_slave_configure,
.scan_finished  = isci_host_scan_finished,
diff --git a/drivers/scsi/mvsas/mv_init.c b/drivers/scsi/mvsas/mv_init.c
index 5973eed9493820..b0de3bdb01db06 100644
--- a/drivers/scsi/mvsas/mv_init.c
+++ b/drivers/scsi/mvsas/mv_init.c
@@ -33,6 +33,7 @@ static struct scsi_host_template mvs_sht = {

Re: ipr crashes due to NULL dma_need_drain since cc97923a5bcc ("block: move dma drain handling to scsi")

2020-06-09 Thread Christoph Hellwig 
On Tue, Jun 09, 2020 at 08:00:35PM +1000, Michael Ellerman wrote:
> Hi all,
> 
> I'm seeing crashes on powerpc with the ipr driver, which I'm fairly sure
> are due to dma_need_drain being NULL.

Ooops, my changes completely forgot about SAS attached ATAPI devices.
I'll cook up a fix in a bit.

ipr crashes due to NULL dma_need_drain since cc97923a5bcc ("block: move dma drain handling to scsi")

2020-06-09 Thread Michael Ellerman 
Hi all,

I'm seeing crashes on powerpc with the ipr driver, which I'm fairly sure
are due to dma_need_drain being NULL.

The backtrace is:

  scsi_init_io+0x1d8/0x350
  scsi_queue_rq+0x7a4/0xc30
  blk_mq_dispatch_rq_list+0x1b0/0x910
  blk_mq_sched_dispatch_requests+0x154/0x270
  __blk_mq_run_hw_queue+0xa0/0x160
  __blk_mq_delay_run_hw_queue+0x244/0x250
  blk_mq_sched_insert_request+0x13c/0x250
  blk_execute_rq_nowait+0x88/0xb0
  blk_execute_rq+0x5c/0xf0
  __scsi_execute+0x10c/0x270
  scsi_mode_sense+0x144/0x440
  sr_probe+0x2e8/0x810
  really_probe+0x12c/0x580
  driver_probe_device+0x88/0x170
  device_driver_attach+0x11c/0x130
  __driver_attach+0xac/0x190
  bus_for_each_dev+0xa8/0x130
  driver_attach+0x34/0x50
  bus_add_driver+0x170/0x2b0
  driver_register+0xb4/0x1c0
  scsi_register_driver+0x2c/0x40
  init_sr+0x4c/0x80
  do_one_initcall+0x60/0x2b0
  kernel_init_freeable+0x2e0/0x3a0
  kernel_init+0x2c/0x148
  ret_from_kernel_thread+0x5c/0x74

And looking at the disassembly I think it's coming from:

static inline bool scsi_cmd_needs_dma_drain(struct scsi_device *sdev,
struct request *rq)
{
return sdev->dma_drain_len && blk_rq_is_passthrough(rq) &&
   !op_is_write(req_op(rq)) &&
   sdev->host->hostt->dma_need_drain(rq);
  ^^
}

Bisect agrees:

# first bad commit: [cc97923a5bccc776851c242b61015faf288d5c22] block: move dma 
drain handling to scsi


And looking at ipr.c, it constructs its scsi_host_template manually,
without using any of the macros that end up calling __ATA_BASE_SHT,
which populates dma_need_drain.

The obvious fix below works, the system boots and seems to be operating
normally, but I don't know enough (anything) about SCSI to say if it's
actually the correct fix.

cheers


diff --git a/drivers/scsi/ipr.c b/drivers/scsi/ipr.c
index 7d77997d26d4..7d86f4ca266c 100644
--- a/drivers/scsi/ipr.c
+++ b/drivers/scsi/ipr.c
@@ -6731,6 +6731,7 @@ static struct scsi_host_template driver_template = {
.compat_ioctl = ipr_ioctl,
 #endif
.queuecommand = ipr_queuecommand,
+   .dma_need_drain = ata_scsi_dma_need_drain,
.eh_abort_handler = ipr_eh_abort,
.eh_device_reset_handler = ipr_eh_dev_reset,
.eh_host_reset_handler = ipr_eh_host_reset,