Re: [PATCH 1/5] libata: make ata_dev_knobble() per-device

[Jeff Garzik <jgarzik@pobox.com>]

Tejun Heo wrote:
> ata_dev_knobble() unconditionally used the first device of the port to
> determine whether a device is bridged or not.  This causes bridge
> limit to be incorrectly applied or unapplied for hosts with slave
> devices (e.g. ata_piix).
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> 
> ---
> 
>  drivers/scsi/libata-core.c |    9 +++++----
>  1 files changed, 5 insertions(+), 4 deletions(-)
> 
> 315dd661a46e94e44c6f9481465bac93f8ea84ef
> diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
> index f6511bd..c92f96f 100644
> --- a/drivers/scsi/libata-core.c
> +++ b/drivers/scsi/libata-core.c
> @@ -1483,9 +1483,10 @@ err_out:
>  }
>  
>  
> -static inline u8 ata_dev_knobble(const struct ata_port *ap)
> +static inline u8 ata_dev_knobble(const struct ata_port *ap,
> +				 struct ata_device *dev)
>  {
> -	return ((ap->cbl == ATA_CBL_SATA) && (!ata_id_is_sata(ap->device->id)));
> +	return ((ap->cbl == ATA_CBL_SATA) && (!ata_id_is_sata(dev->id)));
>  }
>  
>  /**
> @@ -1500,9 +1501,9 @@ static inline u8 ata_dev_knobble(const s
>  void ata_dev_config(struct ata_port *ap, unsigned int i)
>  {
>  	/* limit bridge transfers to udma5, 200 sectors */
> -	if (ata_dev_knobble(ap)) {
> +	if (ata_dev_knobble(ap, &ap->device[i])) {
>  		printk(KERN_INFO "ata%u(%u): applying bridge limits\n",
> -			ap->id, ap->device->devno);
> +		       ap->id, i);

patch is OK, but at the same time you should

1) remove the inline, its out of fashion

2) make ata_dev_knobble() return [machine] int rather than u8.  It's 
really just a bool.

BTW, if you were bored one day, it would be nice to figure out how to 
use gcc's [and C99's] bool type for stuff like this.