On Tue, 24 Apr 2007 17:03:29 +0900
Tejun Heo <htejun@xxxxxxxxx> wrote:
> Hello,
>
> Kristen Carlson Accardi wrote:
> > static unsigned int ata_print_id = 1;
> > @@ -1744,6 +1745,23 @@ int ata_dev_configure(struct ata_device
> > }
> > dev->cdb_len = (unsigned int) rc;
> >
> > + /*
> > + * check to see if this ATAPI device supports
> > + * Asynchronous Notification
> > + */
> > + if ((ap->flags & ATA_FLAG_AN) && ata_id_has_AN(id))
> > + {
> > + /* issue SET feature command to turn this on */
> > + rc = ata_dev_set_AN(dev);
>
> Please don't store err_mask into int rc. Please store it to a separate
> err_mask variable and report it when printing error message.
>
> > + if (rc) {
> > + ata_dev_printk(dev, KERN_ERR,
> > + "unable to set AN\n");
> > + rc = -EINVAL;
>
> Wouldn't -EIO be more appropriate?
I think Alan is right - and being unable to turn on AN should not be fatal.
I'll just change all this code to just print the err and keep going.
>
> > + goto err_out_nosup;
> > + }
> > + dev->flags |= ATA_DFLAG_AN;
> > + }
> > +
>
> Not NACKing. Just notes for future improvements. We need to be more
> careful here. ATA/ATAPI world is filled with braindamaged devices and I
> bet there are devices which advertises it can do AN but chokes when AN
> is enabled.
>
> This should be handled similarly to ACPI failure. Currently ACPI does
> the following.
>
> 1. try once, if fail, record that ACPI failed. return error to trigger
> retry.
> 2. try again, if fail again, ignore error if possible (!FROZEN) and turn
> off ACPI.
>
> This fallback mechanism for optional features can probably be
> generalized and used for both ACPI and AN.
Ok - meanwhile I think it's appropriate here to just do try-once-fail-give-up.
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
