[PATCH 1/1] ibmvscsi: correct command abort logic

[PATCH 1/1] ibmvscsi: correct command abort logic

[Robert Jennings]

The logic is swapped when we determine if a command that we're trying
to abort has already completed.  This will keep us from freeing event
structures at the correct time, fail to unmap command data properly 
and throw off our request_limit count.

Signed-off-by: Robert Jennings <rcjenn@us.ibm.com>

---

 ibmvscsi.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
index 6cad175..2d9db9b 100644
--- a/drivers/scsi/ibmvscsi/ibmvscsi.c
+++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
@@ -1099,7 +1099,7 @@ static int ibmvscsi_eh_abort_handler(struct scsi_cmnd *cmd)
 		}
 	}
 
-	if (found_evt == NULL) {
+	if (found_evt != NULL) {
 		spin_unlock_irqrestore(hostdata->host->host_lock, flags);
 		sdev_printk(KERN_INFO, cmd->device, "aborted task tag 0x%lx completed\n",
 			    tsk_mgmt->task_tag);

Re: [PATCH 1/1] ibmvscsi: correct command abort logic

[Brian King]

I don't think this is right. This will inject an oops into the
found_evt == NULL path. Additionally, if the event is still on the
sent list, with this change, we leave it on the sent list. The
way I thought this was supposed to work was if we got back a successful
response to the abort command that implied that everything was cleaned
up for the command we were aborting, which is why we clean up after it
in the found_evt != NULL case.

-Brian

Robert Jennings wrote:
> The logic is swapped when we determine if a command that we're trying
> to abort has already completed.  This will keep us from freeing event
> structures at the correct time, fail to unmap command data properly 
> and throw off our request_limit count.
> 
> Signed-off-by: Robert Jennings <rcjenn@us.ibm.com>
> 
> ---
> 
>  ibmvscsi.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
> index 6cad175..2d9db9b 100644
> --- a/drivers/scsi/ibmvscsi/ibmvscsi.c
> +++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
> @@ -1099,7 +1099,7 @@ static int ibmvscsi_eh_abort_handler(struct scsi_cmnd *cmd)
>  		}
>  	}
> 
> -	if (found_evt == NULL) {
> +	if (found_evt != NULL) {
>  		spin_unlock_irqrestore(hostdata->host->host_lock, flags);
>  		sdev_printk(KERN_INFO, cmd->device, "aborted task tag 0x%lx completed\n",
>  			    tsk_mgmt->task_tag);


-- 
Brian King
Linux on Power Virtualization
IBM Linux Technology Center

Re: [PATCH 1/1] ibmvscsi: correct command abort logic

[Robert Jennings]

My patch is wrong and the current code is correct, I wasn't following
the logic correctly.

--Rob

* Brian King (brking@linux.vnet.ibm.com) wrote:
> I don't think this is right. This will inject an oops into the
> found_evt == NULL path. Additionally, if the event is still on the
> sent list, with this change, we leave it on the sent list. The
> way I thought this was supposed to work was if we got back a successful
> response to the abort command that implied that everything was cleaned
> up for the command we were aborting, which is why we clean up after it
> in the found_evt != NULL case.
> 
> -Brian
> 
> Robert Jennings wrote:
> > The logic is swapped when we determine if a command that we're trying
> > to abort has already completed.  This will keep us from freeing event
> > structures at the correct time, fail to unmap command data properly 
> > and throw off our request_limit count.
> > 
> > Signed-off-by: Robert Jennings <rcjenn@us.ibm.com>
> > 
> > ---
> > 
> >  ibmvscsi.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
> > index 6cad175..2d9db9b 100644
> > --- a/drivers/scsi/ibmvscsi/ibmvscsi.c
> > +++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
> > @@ -1099,7 +1099,7 @@ static int ibmvscsi_eh_abort_handler(struct scsi_cmnd *cmd)
> >  		}
> >  	}
> > 
> > -	if (found_evt == NULL) {
> > +	if (found_evt != NULL) {
> >  		spin_unlock_irqrestore(hostdata->host->host_lock, flags);
> >  		sdev_printk(KERN_INFO, cmd->device, "aborted task tag 0x%lx completed\n",
> >  			    tsk_mgmt->task_tag);
> 
> 
> -- 
> Brian King
> Linux on Power Virtualization
> IBM Linux Technology Center
> 
>