Re: [PATCH] libata ATA vs SATA detection and workaround.

[Jeff Garzik]

Brad Campbell wrote:
> +			/* limit bridge transfers to udma5, 200 sectors */
> +			if ((ap->cbl == ATA_CBL_SATA) && (!ata_id_is_sata(ap->device))) {
> +				printk(KERN_INFO "ata%u(%u): applying bridge limits\n",
> +					ap->id, ap->device->devno);
> +				ap->udma_mask &= ATA_UDMA5;
> +				ap->host->max_sectors = ATA_MAX_SECTORS;
> +				ap->host->hostt->max_sectors = ATA_MAX_SECTORS;
> +				ap->device->flags |= ATA_DFLAG_LOCK_SECTORS;
> +			}
>  			if (ap->ops->dev_config)
>  				ap->ops->dev_config(ap, &ap->device[i]);


Close!  Please move the entire quoted section, including the two lines 
of code calling ->dev_config(), into a new function 'ata_dev_config'. 
Export it (bottom of libata-core.c) and prototype it (libata.h) as well.

I'm still pondering what Alan was hinting at, a bit.  You (Brad) are 
correct in pointing out that this code should only trigger for the 
correct situations (lba48, etc.) which are only present on modern 
drives, but...  there is still a chance that word 93 will be zero on 
some weird (probably non-compliant) device.

However, Alan's comment is actually more relevant for unrelated sections 
of libata.  Whenever we test a feature bit in words 82-87, we should 
check for "word != 0 && word != 0xffff" which is how one knows the word 
is implemented.  There are no feature bits indicating that feature bits 
exist :)

	Jeff


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/