Re: [PATCH v2 1/8] ata: remove reference to non-existing error_handler()

[Niklas Cassel <Niklas.Cassel@wdc.com>]

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@suse.de>
> > 
> > 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@suse.de>
> > [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@wdc.com>

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)
>