On Thu, May 11, 2023 at 12:52:06AM +0200, Hannes Reinecke wrote:
> With commit 65a15d6560df ("scsi: ipr: Remove SATA support") all
> libata drivers now have the error_handler() callback provided,
> so we can stop checking for non-existing error_handler callback.
>
> Signed-off-by: Hannes Reinecke <hare@xxxxxxx>
> ---
Hello Hannes,
I think that you forgot to update the comments in a few places:
$ git grep -i "new eh" drivers/ata
>
> - /* XXX: New EH and old EH use different mechanisms to
> - * synchronize EH with regular execution path.
> - *
> - * In new EH, a qc owned by EH is marked with ATA_QCFLAG_EH.
> - * Normal execution path is responsible for not accessing a
> - * qc owned by EH. libata core enforces the rule by returning NULL
> - * from ata_qc_from_tag() for qcs owned by EH.
I think we should keep this part, but rephrase it to not mention "new EH",
as it does provide valuable information related to ata_qc_complete().
> @@ -1680,9 +1674,8 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
> /* Keep the SCSI ML and status byte, clear host byte. */
> cmd->result &= 0x0000ffff;
>
> - if (need_sense && !ap->ops->error_handler)
> - ata_dump_status(ap, &qc->result_tf);
> -
> + if (!(qc->flags & ATA_QCFLAG_QUIET))
> + ata_dump_status(qc->ap, &qc->result_tf);
> ata_qc_done(qc);
Why remove the newline before the call to ata_qc_done() ?
With your change here, you will start to print all QCs that doesn't have
quiet set. That is a functional change and should be in a separate patch
if we wanted that behavior, but I don't see why we would want that.
I think that you should simply drop the call to ata_dump_status().
Kind regards,
Niklas
