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.
>
> Signed-off-by: Niklas Cassel <niklas.cassel@xxxxxxx>
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
