Re: [PATCH 3/4] libata: make ata_set_mode() responsible for failure handling

[Jeff Garzik <jgarzik@pobox.com>]

Tejun Heo wrote:
> Make ata_set_mode() responsible for determining whether to take port
> or device offline on failure.  ata_dev_set_xfermode() and
> ata_dev_set_mode() indicate error to the caller instead of disabling
> port directly on failure.  Also, for consistency, ata_dev_present()
> check is done in ata_set_mode() instead of ata_dev_set_mode().
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> 
> ---
> 
>  drivers/scsi/libata-core.c |   56 ++++++++++++++++++++++++++++----------------
>  1 files changed, 36 insertions(+), 20 deletions(-)
> 
> 07934616ecb83d7bac2adfe9eb32f5173feb32ff
> diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
> index b7595bf..6826181 100644
> --- a/drivers/scsi/libata-core.c
> +++ b/drivers/scsi/libata-core.c
> @@ -64,7 +64,8 @@
>  static unsigned int ata_dev_init_params(struct ata_port *ap,
>  					struct ata_device *dev);
>  static void ata_set_mode(struct ata_port *ap);
> -static void ata_dev_set_xfermode(struct ata_port *ap, struct ata_device *dev);
> +static unsigned int ata_dev_set_xfermode(struct ata_port *ap,
> +					 struct ata_device *dev);
>  static void ata_dev_xfermask(struct ata_port *ap, struct ata_device *dev);
>  
>  static unsigned int ata_unique_id = 1;
> @@ -1754,20 +1755,28 @@ int ata_timing_compute(struct ata_device
>  	return 0;
>  }

ACK 1-4 in this patchset, but dropping due to dropped patches in 
previous patchset.

I have the following concern with this patch (#3) however:

> -static void ata_dev_set_mode(struct ata_port *ap, struct ata_device *dev)
> +static int ata_dev_set_mode(struct ata_port *ap, struct ata_device *dev)
>  {
> -	if (!ata_dev_present(dev) || (ap->flags & ATA_FLAG_PORT_DISABLED))
> -		return;

I think you drop too many ATA_FLAG_PORT_DISABLED tests in this patch, 
leading the code to potentially miss a previously-flagged PORT_DISABLED 
(perhaps by an LLDD).

	Jeff