Sure. I was checking whether other MODE SENSE functions have the same problem. It turns out the recently rewritten ata_msense_ctl_mode() is actually bugged, seemingly because of the original arcane style. Sent the patch for that, fixing it in the original style. So I am wondering if I should further fix the style of ata_msense_ctl_mode() when I resend this patch. If so, how should I make `dev->flags & ATA_DFLAG_D_SENSE` a boolean, so that it can be bit shifted? Double negation? Type casting? Or should I just use an `else if` clause? On 13 July 2016 at 01:20, Tejun Heo <tj@xxxxxxxxxx> wrote: > On Wed, Jul 13, 2016 at 12:20:40AM +0800, Tom Yan wrote: >> First of all "changeable" is the "version" of mode page requested by >> the user, while ata_id_*_enabled(id) are the value of setting reported >> by the device. I think it's ugly and confusing to check values of >> totally different nature "together". >> >> The worse thing is, since we have implemented MODE SELECT / >> ata_mselect_caching() to serve exclusively the write cache feature, >> the difference in the two if-conditions made the code look even more >> arcane. >> >> In my version, it is clear that when the user request the changeable >> mode page, the value for the WCE bit will always be "1" / "y", since >> we've made (only) it changeable in ata_mselect_caching(); and when the >> user request the current / default page, the values reflect the >> settings from the drive. > > Can you please put the rationale in the patch description and repost? > > Thanks. > > -- > tejun -- 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
