Re: [PATCH] ata: libata: Add helper ata_eh_decide_disposition()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 8/28/24 01:48, Niklas Cassel wrote:
> Every time I see libata code calling scsi_check_sense(), I get confused
> why the code path that is working fine for SCSI code, is not sufficient
> for libata code.
> 
> The reason is that SCSI usually gets the sense data as part of the
> completion, and will thus automatically call scsi_check_sense(), which
> will set the SCSI ML byte (if any).
> 
> However, for libata queued commands, we always need to fetch the sense
> data via SCSI EH, and thus do not get the luxury of having
> scsi_check_sense() called automatically.
> 
> Add a new helper, ata_eh_decide_disposition(), that has a ata_eh_ prefix
> to more clearly highlight that this is only needed for code called by EH,
> while also having a similar name to scsi_decide_disposition(), such that
> it is easier to compare the libata code with the equivalent SCSI code.
> 
> Also add a big kdoc comment explaining why this helper is called/needed in
> the first place.
> 
> Signed-off-by: Niklas Cassel <cassel@xxxxxxxxxx>
> ---
>  drivers/ata/libata-eh.c   | 46 ++++++++++++++++++++++++++++++++++-----
>  drivers/ata/libata-sata.c |  8 +++----
>  drivers/ata/libata.h      |  1 +
>  3 files changed, 45 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index 214b935c2ced..173769999c13 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
> @@ -1401,6 +1401,43 @@ unsigned int atapi_eh_tur(struct ata_device *dev, u8 *r_sense_key)
>  	return err_mask;
>  }
>  
> +/**
> + *	ata_eh_decide_disposition - Disposition a qc based on sense data
> + *	@qc: qc to examine
> + *
> + *	For a regular SCSI command, the SCSI completion callback (scsi_done())
> + *	will call scsi_complete(), which will call scsi_decide_disposition(),
> + *	which will call scsi_check_sense(). scsi_complete() finally calls
> + *	scsi_finish_command(). This is fine for SCSI, since any eventual sense
> + *	data is usually returned in the completion itself (without invoking SCSI
> + *	EH). However, for a QC, we always need to fetch the sense data
> + *	explicitly using SCSI EH.
> + *
> + *	A command that is completed via SCSI EH will instead be completed using
> + *	scsi_eh_flush_done_q(), which will call scsi_finish_command() directly
> + *	(without ever calling scsi_check_sense()).
> + *
> + *	For a command that went through SCSI EH, it is the responsibility of the
> + *	SCSI EH strategy handler to call scsi_decide_disposition(), see e.g. how
> + *	scsi_eh_get_sense() calls scsi_decide_disposition() for SCSI LLDDs that
> + *	do not get the sense data as part of the completion.
> + *
> + *	Thus, for QC commands that went via SCSI EH, we need to call
> + *	scsi_check_sense() ourselves, similar to how scsi_eh_get_sense() calls
> + *	scsi_decide_disposition(), which calls scsi_check_sense(), in order to
> + *	set the correct SCSI ML byte (if any).
> + *
> + *	LOCKING:
> + *	EH context.
> + *
> + *	RETURNS:
> + *	SUCCESS or FAILED or NEEDS_RETRY or ADD_TO_MLQUEUE
> + */
> +enum scsi_disposition ata_eh_decide_disposition(struct ata_queued_cmd *qc)
> +{
> +	return scsi_check_sense(qc->scsicmd);
> +}
> +
>  /**
>   *	ata_eh_request_sense - perform REQUEST_SENSE_DATA_EXT
>   *	@qc: qc to perform REQUEST_SENSE_SENSE_DATA_EXT to
> @@ -1627,7 +1664,7 @@ static unsigned int ata_eh_analyze_tf(struct ata_queued_cmd *qc)
>  	}
>  
>  	if (qc->flags & ATA_QCFLAG_SENSE_VALID) {
> -		enum scsi_disposition ret = scsi_check_sense(qc->scsicmd);
> +		enum scsi_disposition ret = ata_eh_decide_disposition(qc);

While at it, please add the missing blank line after this declaration.
Other than this, looks fine to me.

>  		/*
>  		 * SUCCESS here means that the sense code could be
>  		 * evaluated and should be passed to the upper layers
> @@ -1942,11 +1979,10 @@ static int ata_eh_read_sense_success_non_ncq(struct ata_link *link)
>  		return -EIO;
>  
>  	/*
> -	 * If we have sense data, call scsi_check_sense() in order to set the
> -	 * correct SCSI ML byte (if any). No point in checking the return value,
> -	 * since the command has already completed successfully.
> +	 * No point in checking the return value, since the command has already
> +	 * completed successfully.
>  	 */
> -	scsi_check_sense(qc->scsicmd);
> +	ata_eh_decide_disposition(qc);
>  
>  	return 0;
>  }
> diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
> index 48660d445602..76904dce017e 100644
> --- a/drivers/ata/libata-sata.c
> +++ b/drivers/ata/libata-sata.c
> @@ -1455,12 +1455,10 @@ int ata_eh_read_sense_success_ncq_log(struct ata_link *link)
>  		qc->flags |= ATA_QCFLAG_SENSE_VALID;
>  
>  		/*
> -		 * If we have sense data, call scsi_check_sense() in order to
> -		 * set the correct SCSI ML byte (if any). No point in checking
> -		 * the return value, since the command has already completed
> -		 * successfully.
> +		 * No point in checking the return value, since the command has
> +		 * already completed successfully.
>  		 */
> -		scsi_check_sense(qc->scsicmd);
> +		ata_eh_decide_disposition(qc);
>  	}
>  
>  	return ret;
> diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
> index 6abf265f626e..e4029626641d 100644
> --- a/drivers/ata/libata.h
> +++ b/drivers/ata/libata.h
> @@ -162,6 +162,7 @@ extern void ata_eh_finish(struct ata_port *ap);
>  extern int ata_ering_map(struct ata_ering *ering,
>  			 int (*map_fn)(struct ata_ering_entry *, void *),
>  			 void *arg);
> +enum scsi_disposition ata_eh_decide_disposition(struct ata_queued_cmd *qc);
>  extern unsigned int atapi_eh_tur(struct ata_device *dev, u8 *r_sense_key);
>  extern unsigned int atapi_eh_request_sense(struct ata_device *dev,
>  					   u8 *sense_buf, u8 dfl_sense_key);

-- 
Damien Le Moal
Western Digital Research





[Index of Archives]     [Linux Filesystems]     [Linux SCSI]     [Linux RAID]     [Git]     [Kernel Newbies]     [Linux Newbie]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Samba]     [Device Mapper]

  Powered by Linux