Re: [PATCH #upstream-fixes] libata: handle SEMB signature better

[Tejun Heo <tj@kernel.org>]

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