[PATCH] qla2xxx: Fix to allow to reset devices using sg interface (sg_reset)

[PATCH] qla2xxx: Fix to allow to reset devices using sg interface (sg_reset)

[Vladislav Bolkhovitin]

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

Currently it is impossible to reset provided by Qlogic QLA2xxx driver 
SCSI devices externally using corresponding sg devices, particularly via 
sg_reset utility, because qla2xxx driver in qla2xxx_eh_device_reset() 
function checks if the input scsi_cmnd has its private data (CMD_SP()) 
attached. Then the found pointer isn't used anywhere inside of 
qla2xxx_eh_device_reset(). If the RESET request comes from sg device, it 
doesn't have such private data.

The attached patch removes check for non-NULL CMD_SP() from 
qla2xxx_eh_device_reset(), hence allows to reset QLA2xxx's devices using 
corresponding sg devices.

Against 2.6.18-rc2.

Signed-off-by: Vladislav Bolkhovitin <vst@vlnb.net>

Vlad

P.S. Sorry for the attachment format, I hope there will be no problems 
with it for such a small patch.


[-- Attachment #2: qla_reset_fix.diff --]
[-- Type: text/x-patch, Size: 621 bytes --]

--- linux-2.6.18-rc2/drivers/scsi/qla2xxx/qla_os.c	2006-07-21 18:05:55.000000000 +0400
+++ linux-2.6.18-rc2/drivers/scsi/qla2xxx/qla_os.c	2006-07-21 18:07:26.000000000 +0400
@@ -744,7 +744,6 @@ qla2xxx_eh_device_reset(struct scsi_cmnd
 {
 	scsi_qla_host_t *ha = to_qla_host(cmd->device->host);
 	fc_port_t *fcport = (struct fc_port *) cmd->device->hostdata;
-	srb_t *sp;
 	int ret;
 	unsigned int id, lun;
 	unsigned long serial;
@@ -755,8 +754,7 @@
 	lun = cmd->device->lun;
 	serial = cmd->serial_number;
 
-	sp = (srb_t *) CMD_SP(cmd);
-	if (!sp || !fcport)
+	if (!fcport)
 		return ret;
 
 	qla_printk(KERN_INFO, ha,

Re: [PATCH] qla2xxx: Fix to allow to reset devices using sg interface (sg_reset)

[Vladislav Bolkhovitin]

Also I've found that without this patch qla2xxx_eh_device_reset() 
refuses to reset the device during normal error recovery carrying out by 
SCSI mid-layer as well. I didn't do deeper investigations though, with 
this patch the error recovery works as expected.

Vlad

Vladislav Bolkhovitin wrote:
> Currently it is impossible to reset provided by Qlogic QLA2xxx driver 
> SCSI devices externally using corresponding sg devices, particularly via 
> sg_reset utility, because qla2xxx driver in qla2xxx_eh_device_reset() 
> function checks if the input scsi_cmnd has its private data (CMD_SP()) 
> attached. Then the found pointer isn't used anywhere inside of 
> qla2xxx_eh_device_reset(). If the RESET request comes from sg device, it 
> doesn't have such private data.
> 
> The attached patch removes check for non-NULL CMD_SP() from 
> qla2xxx_eh_device_reset(), hence allows to reset QLA2xxx's devices using 
> corresponding sg devices.
> 
> Against 2.6.18-rc2.
> 
> Signed-off-by: Vladislav Bolkhovitin <vst@vlnb.net>
> 
> Vlad
> 
> P.S. Sorry for the attachment format, I hope there will be no problems 
> with it for such a small patch.
> 
> 
> ------------------------------------------------------------------------
> 
> --- linux-2.6.18-rc2/drivers/scsi/qla2xxx/qla_os.c	2006-07-21 18:05:55.000000000 +0400
> +++ linux-2.6.18-rc2/drivers/scsi/qla2xxx/qla_os.c	2006-07-21 18:07:26.000000000 +0400
> @@ -744,7 +744,6 @@ qla2xxx_eh_device_reset(struct scsi_cmnd
>  {
>  	scsi_qla_host_t *ha = to_qla_host(cmd->device->host);
>  	fc_port_t *fcport = (struct fc_port *) cmd->device->hostdata;
> -	srb_t *sp;
>  	int ret;
>  	unsigned int id, lun;
>  	unsigned long serial;
> @@ -755,8 +754,7 @@
>  	lun = cmd->device->lun;
>  	serial = cmd->serial_number;
>  
> -	sp = (srb_t *) CMD_SP(cmd);
> -	if (!sp || !fcport)
> +	if (!fcport)
>  		return ret;
>  
>  	qla_printk(KERN_INFO, ha,

Re: [PATCH] qla2xxx: Fix to allow to reset devices using sg interface (sg_reset)

[Andrew Vasquez]

On Tue, 25 Jul 2006, Vladislav Bolkhovitin wrote:

> Currently it is impossible to reset provided by Qlogic QLA2xxx driver 
> SCSI devices externally using corresponding sg devices, particularly via 
> sg_reset utility, because qla2xxx driver in qla2xxx_eh_device_reset() 
> function checks if the input scsi_cmnd has its private data (CMD_SP()) 
> attached. Then the found pointer isn't used anywhere inside of 
> qla2xxx_eh_device_reset(). If the RESET request comes from sg device, it 
> doesn't have such private data.
> 
> The attached patch removes check for non-NULL CMD_SP() from 
> qla2xxx_eh_device_reset(), hence allows to reset QLA2xxx's devices using 
> corresponding sg devices.

hmm... actually this change should propegate to the bus and host reset
handlers as well.  I have a series of small fixes queued for
submission, I'll add you patch (with the bus/host deltas) to the
queue.

Thanks,
Andrew Vasquez

Re: [Suspected Spam:#] Re: [PATCH] qla2xxx: Fix to allow to reset devices using sg interface (sg_reset)

[Andrew Vasquez]

On Thu, 27 Jul 2006, Vladislav Bolkhovitin wrote:

> Also I've found that without this patch qla2xxx_eh_device_reset() 
> refuses to reset the device during normal error recovery carrying out by 
> SCSI mid-layer as well. I didn't do deeper investigations though, with 
> this patch the error recovery works as expected.

The only way I see this happening is if the command completed after
the abort handler FAILED and before the device reset handler executed.
Possibly.

During normal EH routines the driver still hold a reference to the
command, so the sp should be valid.

In any case, those codes (referencing SP) are residual left-overs from
our 'standard' non-upstream driver, and should be clensed.

Thanks again,
Andrew Vasquez

Re: [Suspected Spam:#] Re: [PATCH] qla2xxx: Fix to allow to reset devices using sg interface (sg_reset)

[Christoph Hellwig]

On Thu, Jul 27, 2006 at 03:53:10PM -0700, Andrew Vasquez wrote:
> On Thu, 27 Jul 2006, Vladislav Bolkhovitin wrote:
> 
> > Also I've found that without this patch qla2xxx_eh_device_reset() 
> > refuses to reset the device during normal error recovery carrying out by 
> > SCSI mid-layer as well. I didn't do deeper investigations though, with 
> > this patch the error recovery works as expected.
> 
> The only way I see this happening is if the command completed after
> the abort handler FAILED and before the device reset handler executed.
> Possibly.
> 
> During normal EH routines the driver still hold a reference to the
> command, so the sp should be valid.
> 
> In any case, those codes (referencing SP) are residual left-overs from
> our 'standard' non-upstream driver, and should be clensed.

But it shows a bad problem in the design of those external reset ioctls.
Unlike normal EH commands they never went through ->queuecommand so don't
have private data allocated or other setup done.  This can have pretty
serious consequences.  We probably can't fix it short from redoing the
whole EH entry points, but it at least needs to be documented very carefully.

Anyone volunteering to write up the documentation?