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
--
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
