Hello, Jeff.
Jeff Garzik wrote:
>> switch (class) {
>> + case ATA_DEV_SEMB:
>> + class = ATA_DEV_ATA; /* some hard drives report SEMB sig */
>> case ATA_DEV_ATA:
>> tf.command = ATA_CMD_ID_ATA;
>> break;
>
> Why assign class now, as opposed to after IDENTIFY DEVICE succeeds / fails?
The class variable indicates which class the code is gonna try not the
one which is discovered, so it's more consistent to set it before
trying it out.
>
>> @@ -2119,6 +2125,7 @@ retry:
>> else
>> err_mask = ata_do_dev_read_id(dev, &tf, id);
>>
>> + err_mask |= AC_ERR_DEV;
>> if (err_mask) {
>> if (err_mask & AC_ERR_NODEV_HINT) {
>> ata_dev_printk(dev, KERN_DEBUG,
>
> Why is err_mask being unconditionally set?
Oh.. c**p, that's debug code I forgot to remove. Thanks for spotting
it. :-)
> I would think this would make more sense:
>
> if (!err_mask && class == ATA_DEV_SEMB)
> class = ATA_DEV_ATA;
> else if (err_mask) {
> if (err_mask & AC_ERR_NODEV_HINT) {
> ...
I made the code a fast exit path because I didn't know how an actual
SEMB device would respond to it. If the device timeouts IDENTIFYs,
retrying can be quite painful. ATA drives w/ SEMB signature being
extrmemely rare, I think it's better to trade their slight chance of
detection failure but then again it's not like we have a lot of SEMB
devices. What do you think?
Thanks.
--
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
