On Thu, Jul 20, 2023 at 09:47:08AM +0100, John Garry wrote:
> On 20/07/2023 01:42, Niklas Cassel wrote:
> > From: Hannes Reinecke <hare@xxxxxxx>
> >
> > 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>
> > [niklas: fixed review comments, rebased, solved conflicts during rebase,
> > fixed bug that unconditionally dumped all QCs, removed the now unused
> > function ata_dump_status(), removed the now unreachable failure paths in
> > atapi_qc_complete(), removed the non-EH function to request ATAPI sense]
> > Signed-off-by: Niklas Cassel <niklas.cassel@xxxxxxx>
Hello John,
thank you for your review!
>
> ata_qc_from_tag() still has a ap->ops->error_handler check, right?
Correct, I will remove the check from ata_qc_from_tag() as well.
>
> > ---
> > drivers/ata/libata-core.c | 209 +++++++++++++++-----------------------
> > drivers/ata/libata-eh.c | 150 ++++++++++++---------------
> > drivers/ata/libata-sata.c | 7 +-
> > drivers/ata/libata-scsi.c | 142 ++------------------------
> > drivers/ata/libata-sff.c | 30 ++----
> > 5 files changed, 166 insertions(+), 372 deletions(-)
> >
> ...
>
>
> > /*
> > - * For new EH, all qcs are finished in one of three ways -
> > + * For EH, all qcs are finished in one of three ways -
> > * normal completion, error completion, and SCSI timeout.
> > * Both completions can race against SCSI timeout. When normal
> > * completion wins, the qc never reaches EH. When error
> > @@ -659,94 +656,89 @@ EXPORT_SYMBOL(ata_scsi_cmd_error_handler);
> > void ata_scsi_port_error_handler(struct Scsi_Host *host, struct ata_port *ap)
> > {
> > unsigned long flags;
> > + struct ata_link *link;
> > /* invoke error handler */
>
> Is this comment only really relevant when we may not previously invoked the
> error handler?
I'm not sure what you mean. I will simply drop this comment.
Further down in this same function, we have:
/* invoke EH, skip if unloading or suspended */
if (!(ap->pflags & (ATA_PFLAG_UNLOADING | ATA_PFLAG_SUSPENDED)))
ap->ops->error_handler(ap);
So a comment at the start of the function as well feels redundant.
>
> > - if (ap->ops->error_handler) {
> > - struct ata_link *link;
> > - /* acquire EH ownership */
> > - ata_eh_acquire(ap);
> > + /* acquire EH ownership */
> > + ata_eh_acquire(ap);
> > repeat:
> > - /* kill fast drain timer */
> > - del_timer_sync(&ap->fastdrain_timer);
> > + /* kill fast drain timer */
> > + del_timer_sync(&ap->fastdrain_timer);
> > - /* process port resume request */
> > - ata_eh_handle_port_resume(ap);
> > + /* process port resume request */
> > + ata_eh_handle_port_resume(ap);
> > - /* fetch & clear EH info */
> > - spin_lock_irqsave(ap->lock, flags);
> > + /* fetch & clear EH info */
> > + spin_lock_irqsave(ap->lock, flags);
>
> ...
>
> > * ata_to_sense_error - convert ATA error to SCSI error
> > * @id: ATA device number
> > @@ -904,7 +863,6 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
> > struct ata_taskfile *tf = &qc->result_tf;
> > unsigned char *sb = cmd->sense_buffer;
> > unsigned char *desc = sb + 8;
> > - int verbose = qc->ap->ops->error_handler == NULL;
> > u8 sense_key, asc, ascq;
> > memset(sb, 0, SCSI_SENSE_BUFFERSIZE);
> > @@ -916,7 +874,7 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
> > if (qc->err_mask ||
> > tf->status & (ATA_BUSY | ATA_DF | ATA_ERR | ATA_DRQ)) {
> > ata_to_sense_error(qc->ap->print_id, tf->status, tf->error,
> > - &sense_key, &asc, &ascq, verbose);
> > + &sense_key, &asc, &ascq, false);
> > ata_scsi_set_sense(qc->dev, cmd, sense_key, asc, ascq);
> > } else {
> > /*
> > @@ -999,7 +957,6 @@ static void ata_gen_ata_sense(struct ata_queued_cmd *qc)
> > struct scsi_cmnd *cmd = qc->scsicmd;
> > struct ata_taskfile *tf = &qc->result_tf;
> > unsigned char *sb = cmd->sense_buffer;
> > - int verbose = qc->ap->ops->error_handler == NULL;
> > u64 block;
> > u8 sense_key, asc, ascq;
> > @@ -1017,7 +974,7 @@ static void ata_gen_ata_sense(struct ata_queued_cmd *qc)
> > if (qc->err_mask ||
> > tf->status & (ATA_BUSY | ATA_DF | ATA_ERR | ATA_DRQ)) {
> > ata_to_sense_error(qc->ap->print_id, tf->status, tf->error,
> > - &sense_key, &asc, &ascq, verbose);
> > + &sense_key, &asc, &ascq, false);
>
> Please check this - AFAICS, we only ever pass false for @verbose arg now (so
> it would not be needed, and ata_to_sense_error() may be simplified)
You are correct, I will remove the parameter.
Kind regards,
Niklas
>
> > ata_scsi_set_sense(dev, cmd, sense_key, asc, ascq);
> > } else {
> > /* Could not decode error */
> > @@ -1179,9 +1136,6 @@ void ata_scsi_slave_destroy(struct scsi_device *sdev)
> > unsigned long flags;
> > struct ata_device *dev;
> > - if (!ap->ops->error_handler)
>
