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

[John Garry <john.g.garry@oracle.com>]

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>

ata_qc_from_tag() still has a ap->ops->error_handler check, right?

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

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

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