| Message ID | 20230519104003.37843-1-nks@flawful.org |
|---|---|
| State | New |
| Headers | show |
| Series | ata: libata-eh: Clarify ata_eh_qc_retry() behavior at call site | expand |
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); > } > }
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 >
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); > } > }
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); } }