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@xxxxxxx>
> >
> > 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@xxxxxxx>
> > ---
> > 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
>
