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

[Jeff Garzik <jeff@garzik.org>]

Tejun Heo wrote:
> 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?

That's fine -- it is mainly the unconditional err_mask assignment that 
confused me.  I was trying to figure out the rest of the changes, in the 
context of that variable assignment :)

	Jeff