| Message ID | 1498638316-44420-20-git-send-email-hare@suse.de (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On Wed, Jun 28, 2017 at 10:25:07AM +0200, Hannes Reinecke wrote: > qedf has a host reset handler, but as the bus reset handler is > a stub always returning SUCCESS the host reset is never invoked. > So drop the bus reset handler. Given that comment this needs a lot more explanation, as the current behavior seems to be an odd/buggy workaround. I hope this patch won't block the rest of the series, which all seems nice and simple material for this late merge window :(
On Wed, 28 Jun 2017, 9:58am, Christoph Hellwig wrote: > On Wed, Jun 28, 2017 at 10:25:07AM +0200, Hannes Reinecke wrote: > > qedf has a host reset handler, but as the bus reset handler is > > a stub always returning SUCCESS the host reset is never invoked. > > So drop the bus reset handler. > > Given that comment this needs a lot more explanation, as the current > behavior seems to be an odd/buggy workaround. > > I hope this patch won't block the rest of the series, which all seems > nice and simple material for this late merge window :( > The test case this was meant to fix as using the sg_reset utility with the -b option which forces a bus reset with NPIV ports attached. What we were finding is that some of the devices attached to the vport were being brought offline after 5 seconds as escalating to the host reset handler in this case was taking too long. For this specific test case, returning SUCCESS allowed us to bypass the host reset escalation and force the SCSI error handler to send a TUR to the devices which of course would succeed since the device weren't physically offline keeping them from going offline.
On Wed, Jun 28, 2017 at 10:07:50AM -0400, Chad Dupuis wrote: > The test case this was meant to fix as using the sg_reset utility with the > -b option which forces a bus reset with NPIV ports attached. What we were > finding is that some of the devices attached to the vport were being > brought offline after 5 seconds as escalating to the host reset handler in > this case was taking too long. sg_reset / SG_SCSI_RESET are designed to escalatae to to the bigger hammers if the method isn't implemented or doesn't succeed. Use the -N / --no-esc option to disable that. I wish that behavior had been the default from the beginning, but we can't fix that anymore.
On Wed, 28 Jun 2017, 10:37am, Christoph Hellwig wrote: > > sg_reset / SG_SCSI_RESET are designed to escalatae to to the bigger > hammers if the method isn't implemented or doesn't succeed. > > Use the -N / --no-esc option to disable that. I wish that behavior had > been the default from the beginning, but we can't fix that anymore. > Yeah, for a sg_reset prespective that would be the better behavior. I removed the bus_reset handler and used the --no-esc parameter which causes sg_reset to behave more reasonably: # sg_reset -b --no-esc /dev/sdj sg_reset: reset (for value=0x102) may not be available Given this I don't have an objection to this patch. Tested-by: Chad Dupuis <chad.dupuis@cavium.com>
diff --git a/drivers/scsi/qedf/qedf_main.c b/drivers/scsi/qedf/qedf_main.c index 6149ea0..d48a13a 100644 --- a/drivers/scsi/qedf/qedf_main.c +++ b/drivers/scsi/qedf/qedf_main.c @@ -629,16 +629,6 @@ static int qedf_eh_device_reset(struct scsi_cmnd *sc_cmd) return qedf_initiate_tmf(sc_cmd, FCP_TMF_LUN_RESET); } -static int qedf_eh_bus_reset(struct scsi_cmnd *sc_cmd) -{ - QEDF_ERR(NULL, "BUS RESET Issued...\n"); - /* - * Essentially a no-op but return SUCCESS to prevent - * unnecessary escalation to the host reset handler. - */ - return SUCCESS; -} - void qedf_wait_for_upload(struct qedf_ctx *qedf) { while (1) { @@ -716,7 +706,6 @@ static int qedf_slave_configure(struct scsi_device *sdev) .eh_abort_handler = qedf_eh_abort, .eh_device_reset_handler = qedf_eh_device_reset, /* lun reset */ .eh_target_reset_handler = qedf_eh_target_reset, /* target reset */ - .eh_bus_reset_handler = qedf_eh_bus_reset, .eh_host_reset_handler = qedf_eh_host_reset, .slave_configure = qedf_slave_configure, .dma_boundary = QED_HW_DMA_BOUNDARY,
qedf has a host reset handler, but as the bus reset handler is a stub always returning SUCCESS the host reset is never invoked. So drop the bus reset handler. Signed-off-by: Hannes Reinecke <hare@suse.com> Cc: Chad Dupuis <chad.dupuis@cavium.com> --- drivers/scsi/qedf/qedf_main.c | 11 ----------- 1 file changed, 11 deletions(-)