Re: [PATCH] vmw_pvscsi: return SUCCESS for successful command aborts

[Martin K. Petersen]

> "Jim" == Jim Gill  writes:

Jim> VMware prefers to update the module's version number as the driver
Jim> is updated. Would you please update the
Jim> PVSCSI_DRIVER_VERSION_STRING in vmx_pvscsi.h from 1.0.6.0-k to
Jim> 1.0.7.0-k?  Otherwise the patch looks fine to us.

Applied to 4.9/scsi-fixes with a bumped version number.

-- 
Martin K. Petersen  Oracle Linux Engineering
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] vmw_pvscsi: return SUCCESS for successful command aborts

[Jim Gill]

On Fri, 2016-10-28 at 12:27 -0400, David Jeffery wrote:
> The vmw_pvscsi driver reports most successful aborts as FAILED to the scsi
> error handler.  This is do to a misunderstanding of how completion_done() 
> works
> and its interaction with a successful wait using 
> wait_for_completion_timeout().
> The vmw_pvscsi driver is expecting completion_done() to always return true if
> complete() has been called on the completion structure.  But completion_done()
> returns true after complete() has been called only if no function like
> wait_for_completion_timeout() has seen the completion and cleared it as part 
> of
> successfully waiting for the completion.
> 
> Instead of using completion_done(), vmw_pvscsi should just use the return
> value from wait_for_completion_timeout() to know if the wait timed out or not.
> 
> Signed-off-by: David Jeffery 
> 
> ---
>  vmw_pvscsi.c |5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> --- a/drivers/scsi/vmw_pvscsi.c
> +++ b/drivers/scsi/vmw_pvscsi.c
> @@ -793,6 +793,7 @@ static int pvscsi_abort(struct scsi_cmnd
>   unsigned long flags;
>   int result = SUCCESS;
>   DECLARE_COMPLETION_ONSTACK(abort_cmp);
> + int done;
>  
>   scmd_printk(KERN_DEBUG, cmd, "task abort on host %u, %p\n",
>   adapter->host->host_no, cmd);
> @@ -824,10 +825,10 @@ static int pvscsi_abort(struct scsi_cmnd
>   pvscsi_abort_cmd(adapter, ctx);
>   spin_unlock_irqrestore(>hw_lock, flags);
>   /* Wait for 2 secs for the completion. */
> - wait_for_completion_timeout(_cmp, msecs_to_jiffies(2000));
> + done = wait_for_completion_timeout(_cmp, msecs_to_jiffies(2000));
>   spin_lock_irqsave(>hw_lock, flags);
>  
> - if (!completion_done(_cmp)) {
> + if (!done) {
>   /*
>* Failed to abort the command, unmark the fact that it
>* was requested to be aborted.
> --

David, VMware prefers to update the module's version number as the driver is 
updated. Would you
please update the PVSCSI_DRIVER_VERSION_STRING in vmx_pvscsi.h from 1.0.6.0-k 
to 1.0.7.0-k?
Otherwise the patch looks fine to us.--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] vmw_pvscsi: return SUCCESS for successful command aborts

[Ewan D. Milne]

On Fri, 2016-10-28 at 12:27 -0400, David Jeffery wrote:
> The vmw_pvscsi driver reports most successful aborts as FAILED to the scsi
> error handler.  This is do to a misunderstanding of how completion_done() 
> works
> and its interaction with a successful wait using 
> wait_for_completion_timeout().
> The vmw_pvscsi driver is expecting completion_done() to always return true if
> complete() has been called on the completion structure.  But completion_done()
> returns true after complete() has been called only if no function like
> wait_for_completion_timeout() has seen the completion and cleared it as part 
> of
> successfully waiting for the completion.
> 
> Instead of using completion_done(), vmw_pvscsi should just use the return
> value from wait_for_completion_timeout() to know if the wait timed out or not.
> 
> Signed-off-by: David Jeffery 
> 
> ---
>  vmw_pvscsi.c |5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> --- a/drivers/scsi/vmw_pvscsi.c
> +++ b/drivers/scsi/vmw_pvscsi.c
> @@ -793,6 +793,7 @@ static int pvscsi_abort(struct scsi_cmnd
>   unsigned long flags;
>   int result = SUCCESS;
>   DECLARE_COMPLETION_ONSTACK(abort_cmp);
> + int done;
>  
>   scmd_printk(KERN_DEBUG, cmd, "task abort on host %u, %p\n",
>   adapter->host->host_no, cmd);
> @@ -824,10 +825,10 @@ static int pvscsi_abort(struct scsi_cmnd
>   pvscsi_abort_cmd(adapter, ctx);
>   spin_unlock_irqrestore(>hw_lock, flags);
>   /* Wait for 2 secs for the completion. */
> - wait_for_completion_timeout(_cmp, msecs_to_jiffies(2000));
> + done = wait_for_completion_timeout(_cmp, msecs_to_jiffies(2000));
>   spin_lock_irqsave(>hw_lock, flags);
>  
> - if (!completion_done(_cmp)) {
> + if (!done) {
>   /*
>* Failed to abort the command, unmark the fact that it
>* was requested to be aborted.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reviewed-by: Ewan D. Milne 


--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] vmw_pvscsi: return SUCCESS for successful command aborts

[Laurence Oberman]

- Original Message -
> From: "David Jeffery" <djeff...@redhat.com>
> To: linux-scsi@vger.kernel.org
> Sent: Friday, October 28, 2016 12:27:26 PM
> Subject: [PATCH] vmw_pvscsi: return SUCCESS for successful command aborts
> 
> 
> The vmw_pvscsi driver reports most successful aborts as FAILED to the scsi
> error handler.  This is do to a misunderstanding of how completion_done()
> works
> and its interaction with a successful wait using
> wait_for_completion_timeout().
> The vmw_pvscsi driver is expecting completion_done() to always return true if
> complete() has been called on the completion structure.  But
> completion_done()
> returns true after complete() has been called only if no function like
> wait_for_completion_timeout() has seen the completion and cleared it as part
> of
> successfully waiting for the completion.
> 
> Instead of using completion_done(), vmw_pvscsi should just use the return
> value from wait_for_completion_timeout() to know if the wait timed out or
> not.
> 
> Signed-off-by: David Jeffery <djeff...@redhat.com>
> 
> ---
>  vmw_pvscsi.c |5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> --- a/drivers/scsi/vmw_pvscsi.c
> +++ b/drivers/scsi/vmw_pvscsi.c
> @@ -793,6 +793,7 @@ static int pvscsi_abort(struct scsi_cmnd
>   unsigned long flags;
>   int result = SUCCESS;
>   DECLARE_COMPLETION_ONSTACK(abort_cmp);
> + int done;
>  
>   scmd_printk(KERN_DEBUG, cmd, "task abort on host %u, %p\n",
>   adapter->host->host_no, cmd);
> @@ -824,10 +825,10 @@ static int pvscsi_abort(struct scsi_cmnd
>   pvscsi_abort_cmd(adapter, ctx);
>   spin_unlock_irqrestore(>hw_lock, flags);
>   /* Wait for 2 secs for the completion. */
> - wait_for_completion_timeout(_cmp, msecs_to_jiffies(2000));
> + done = wait_for_completion_timeout(_cmp, msecs_to_jiffies(2000));
>   spin_lock_irqsave(>hw_lock, flags);
>  
> - if (!completion_done(_cmp)) {
> + if (!done) {
>   /*
>* Failed to abort the command, unmark the fact that it
>* was requested to be aborted.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
Looks correct to me.
Reviewed-by: Laurence Oberman <lober...@redhat.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] vmw_pvscsi: return SUCCESS for successful command aborts

[David Jeffery]

The vmw_pvscsi driver reports most successful aborts as FAILED to the scsi
error handler.  This is do to a misunderstanding of how completion_done() works
and its interaction with a successful wait using wait_for_completion_timeout().
The vmw_pvscsi driver is expecting completion_done() to always return true if
complete() has been called on the completion structure.  But completion_done()
returns true after complete() has been called only if no function like
wait_for_completion_timeout() has seen the completion and cleared it as part of
successfully waiting for the completion.

Instead of using completion_done(), vmw_pvscsi should just use the return
value from wait_for_completion_timeout() to know if the wait timed out or not.

Signed-off-by: David Jeffery 

---
 vmw_pvscsi.c |5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

--- a/drivers/scsi/vmw_pvscsi.c
+++ b/drivers/scsi/vmw_pvscsi.c
@@ -793,6 +793,7 @@ static int pvscsi_abort(struct scsi_cmnd
unsigned long flags;
int result = SUCCESS;
DECLARE_COMPLETION_ONSTACK(abort_cmp);
+   int done;
 
scmd_printk(KERN_DEBUG, cmd, "task abort on host %u, %p\n",
adapter->host->host_no, cmd);
@@ -824,10 +825,10 @@ static int pvscsi_abort(struct scsi_cmnd
pvscsi_abort_cmd(adapter, ctx);
spin_unlock_irqrestore(>hw_lock, flags);
/* Wait for 2 secs for the completion. */
-   wait_for_completion_timeout(_cmp, msecs_to_jiffies(2000));
+   done = wait_for_completion_timeout(_cmp, msecs_to_jiffies(2000));
spin_lock_irqsave(>hw_lock, flags);
 
-   if (!completion_done(_cmp)) {
+   if (!done) {
/*
 * Failed to abort the command, unmark the fact that it
 * was requested to be aborted.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html