[PATCH] ata: libata-eh: Clarify ata_eh_qc_retry() behavior at call site

[PATCH] ata: libata-eh: Clarify ata_eh_qc_retry() behavior at call site

[Niklas Cassel]

From: Niklas Cassel <niklas.cassel@wdc.com>

While the function documentation for ata_eh_qc_retry() is clear,
from simply reading the single function that calls ata_eh_qc_retry(),
it is not clear that ata_eh_qc_retry() might not retry the command.

Add a comment in the single function that calls ata_eh_qc_retry() to
clarify the behavior.

Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
---
 drivers/ata/libata-eh.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index a6c901811802..170326dc1073 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -3814,6 +3814,12 @@ void ata_eh_finish(struct ata_port *ap)
 			 * considering both err_mask and tf.
 			 */
 			if (qc->flags & ATA_QCFLAG_RETRY)
+				/*
+				 * Since qc->err_mask is set, ata_eh_qc_retry()
+				 * will not increment scmd->allowed, so upper
+				 * layer will only retry the command if it has
+				 * not already been retried too many times.
+				 */
 				ata_eh_qc_retry(qc);
 			else
 				ata_eh_qc_complete(qc);
@@ -3823,6 +3829,12 @@ void ata_eh_finish(struct ata_port *ap)
 			} else {
 				/* feed zero TF to sense generation */
 				memset(&qc->result_tf, 0, sizeof(qc->result_tf));
+				/*
+				 * Since qc->err_mask is not set,
+				 * ata_eh_qc_retry() will increment
+				 * scmd->allowed, so upper layer is guaranteed
+				 * to retry the command.
+				 */
 				ata_eh_qc_retry(qc);
 			}
 		}
-- 
2.40.1

Re: [PATCH] ata: libata-eh: Clarify ata_eh_qc_retry() behavior at call site

[Damien Le Moal]

On 5/19/23 19:40, Niklas Cassel wrote:
> From: Niklas Cassel <niklas.cassel@wdc.com>
> 
> While the function documentation for ata_eh_qc_retry() is clear,
> from simply reading the single function that calls ata_eh_qc_retry(),
> it is not clear that ata_eh_qc_retry() might not retry the command.
> 
> Add a comment in the single function that calls ata_eh_qc_retry() to
> clarify the behavior.

Looks good. But may be resend this rebased on top of Hannes v2 of the error
handler cleanup once he sends it ?

> 
> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> ---
>  drivers/ata/libata-eh.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index a6c901811802..170326dc1073 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
> @@ -3814,6 +3814,12 @@ void ata_eh_finish(struct ata_port *ap)
>  			 * considering both err_mask and tf.
>  			 */
>  			if (qc->flags & ATA_QCFLAG_RETRY)
> +				/*
> +				 * Since qc->err_mask is set, ata_eh_qc_retry()
> +				 * will not increment scmd->allowed, so upper
> +				 * layer will only retry the command if it has
> +				 * not already been retried too many times.
> +				 */
>  				ata_eh_qc_retry(qc);
>  			else
>  				ata_eh_qc_complete(qc);
> @@ -3823,6 +3829,12 @@ void ata_eh_finish(struct ata_port *ap)
>  			} else {
>  				/* feed zero TF to sense generation */
>  				memset(&qc->result_tf, 0, sizeof(qc->result_tf));
> +				/*
> +				 * Since qc->err_mask is not set,
> +				 * ata_eh_qc_retry() will increment
> +				 * scmd->allowed, so upper layer is guaranteed
> +				 * to retry the command.
> +				 */
>  				ata_eh_qc_retry(qc);
>  			}
>  		}

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH] ata: libata-eh: Clarify ata_eh_qc_retry() behavior at call site

[Niklas Cassel]

On Mon, May 22, 2023 at 09:48:24AM +0900, Damien Le Moal wrote:
> On 5/19/23 19:40, Niklas Cassel wrote:
> > From: Niklas Cassel <niklas.cassel@wdc.com>
> > 
> > While the function documentation for ata_eh_qc_retry() is clear,
> > from simply reading the single function that calls ata_eh_qc_retry(),
> > it is not clear that ata_eh_qc_retry() might not retry the command.
> > 
> > Add a comment in the single function that calls ata_eh_qc_retry() to
> > clarify the behavior.
> 
> Looks good. But may be resend this rebased on top of Hannes v2 of the error
> handler cleanup once he sends it ?

Still no v2 from Hannes yet, and considering that Hannes' series does not
touch this function (ata_eh_finish()) at all, perhaps this can be queued?


Kind regards,
Niklas

> 
> > 
> > Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> > ---
> >  drivers/ata/libata-eh.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> > index a6c901811802..170326dc1073 100644
> > --- a/drivers/ata/libata-eh.c
> > +++ b/drivers/ata/libata-eh.c
> > @@ -3814,6 +3814,12 @@ void ata_eh_finish(struct ata_port *ap)
> >  			 * considering both err_mask and tf.
> >  			 */
> >  			if (qc->flags & ATA_QCFLAG_RETRY)
> > +				/*
> > +				 * Since qc->err_mask is set, ata_eh_qc_retry()
> > +				 * will not increment scmd->allowed, so upper
> > +				 * layer will only retry the command if it has
> > +				 * not already been retried too many times.
> > +				 */
> >  				ata_eh_qc_retry(qc);
> >  			else
> >  				ata_eh_qc_complete(qc);
> > @@ -3823,6 +3829,12 @@ void ata_eh_finish(struct ata_port *ap)
> >  			} else {
> >  				/* feed zero TF to sense generation */
> >  				memset(&qc->result_tf, 0, sizeof(qc->result_tf));
> > +				/*
> > +				 * Since qc->err_mask is not set,
> > +				 * ata_eh_qc_retry() will increment
> > +				 * scmd->allowed, so upper layer is guaranteed
> > +				 * to retry the command.
> > +				 */
> >  				ata_eh_qc_retry(qc);
> >  			}
> >  		}
> 
> -- 
> Damien Le Moal
> Western Digital Research
> 

Re: [PATCH] ata: libata-eh: Clarify ata_eh_qc_retry() behavior at call site

[Damien Le Moal]

On 5/19/23 19:40, Niklas Cassel wrote:
> From: Niklas Cassel <niklas.cassel@wdc.com>
> 
> While the function documentation for ata_eh_qc_retry() is clear,
> from simply reading the single function that calls ata_eh_qc_retry(),
> it is not clear that ata_eh_qc_retry() might not retry the command.
> 
> Add a comment in the single function that calls ata_eh_qc_retry() to
> clarify the behavior.
> 
> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>

Applied to for-6.5 with one nit (see below). Thanks !

> ---
>  drivers/ata/libata-eh.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index a6c901811802..170326dc1073 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
> @@ -3814,6 +3814,12 @@ void ata_eh_finish(struct ata_port *ap)
>  			 * considering both err_mask and tf.
>  			 */
>  			if (qc->flags & ATA_QCFLAG_RETRY)
> +				/*
> +				 * Since qc->err_mask is set, ata_eh_qc_retry()
> +				 * will not increment scmd->allowed, so upper
> +				 * layer will only retry the command if it has
> +				 * not already been retried too many times.
> +				 */
>  				ata_eh_qc_retry(qc);

I added curly braces here (while not strictly needed, this if is multi-line with
the comment added, so I prefer having the braces around it).

>  			else
>  				ata_eh_qc_complete(qc);

And here as well to match the if side.

> @@ -3823,6 +3829,12 @@ void ata_eh_finish(struct ata_port *ap)
>  			} else {
>  				/* feed zero TF to sense generation */
>  				memset(&qc->result_tf, 0, sizeof(qc->result_tf));
> +				/*
> +				 * Since qc->err_mask is not set,
> +				 * ata_eh_qc_retry() will increment
> +				 * scmd->allowed, so upper layer is guaranteed
> +				 * to retry the command.
> +				 */
>  				ata_eh_qc_retry(qc);
>  			}
>  		}

-- 
Damien Le Moal
Western Digital Research