| Message ID | 5784f28c.c4d0620a.69372.3df2@mx.google.com |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
On Tue, Jul 12, 2016 at 09:37:03PM +0800, tom.ty89@gmail.com wrote: > From: Tom Yan <tom.ty89@gmail.com> > > Signed-off-by: Tom Yan <tom.ty89@gmail.com> > > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c > index bfec66f..48ea887 100644 > --- a/drivers/ata/libata-scsi.c > +++ b/drivers/ata/libata-scsi.c > @@ -2424,10 +2424,12 @@ static void modecpy(u8 *dest, const u8 *src, int n, bool changeable) > static unsigned int ata_msense_caching(u16 *id, u8 *buf, bool changeable) > { > modecpy(buf, def_cache_mpage, sizeof(def_cache_mpage), changeable); > - if (changeable || ata_id_wcache_enabled(id)) > - buf[2] |= (1 << 2); /* write cache enable */ > - if (!changeable && !ata_id_rahead_enabled(id)) > - buf[12] |= (1 << 5); /* disable read ahead */ > + if (changeable) { > + buf[2] |= 1 << 2; /* ata_mselect_caching() */ > + } else { > + buf[2] |= ata_id_wcache_enabled(id) << 2; /* write cache enable */ > + buf[12] |= !ata_id_rahead_enabled(id) << 5; /* disable read ahead */ > + } It's different but I'm not sure this is better. Thanks.
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. On 12 July 2016 at 22:48, Tejun Heo <tj@kernel.org> wrote: > On Tue, Jul 12, 2016 at 09:37:03PM +0800, tom.ty89@gmail.com wrote: >> From: Tom Yan <tom.ty89@gmail.com> >> >> Signed-off-by: Tom Yan <tom.ty89@gmail.com> >> >> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c >> index bfec66f..48ea887 100644 >> --- a/drivers/ata/libata-scsi.c >> +++ b/drivers/ata/libata-scsi.c >> @@ -2424,10 +2424,12 @@ static void modecpy(u8 *dest, const u8 *src, int n, bool changeable) >> static unsigned int ata_msense_caching(u16 *id, u8 *buf, bool changeable) >> { >> modecpy(buf, def_cache_mpage, sizeof(def_cache_mpage), changeable); >> - if (changeable || ata_id_wcache_enabled(id)) >> - buf[2] |= (1 << 2); /* write cache enable */ >> - if (!changeable && !ata_id_rahead_enabled(id)) >> - buf[12] |= (1 << 5); /* disable read ahead */ >> + if (changeable) { >> + buf[2] |= 1 << 2; /* ata_mselect_caching() */ >> + } else { >> + buf[2] |= ata_id_wcache_enabled(id) << 2; /* write cache enable */ >> + buf[12] |= !ata_id_rahead_enabled(id) << 5; /* disable read ahead */ >> + } > > It's different but I'm not sure this is better. > > Thanks. > > -- > tejun -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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@kernel.org> 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-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c index bfec66f..48ea887 100644 --- a/drivers/ata/libata-scsi.c +++ b/drivers/ata/libata-scsi.c @@ -2424,10 +2424,12 @@ static void modecpy(u8 *dest, const u8 *src, int n, bool changeable) static unsigned int ata_msense_caching(u16 *id, u8 *buf, bool changeable) { modecpy(buf, def_cache_mpage, sizeof(def_cache_mpage), changeable); - if (changeable || ata_id_wcache_enabled(id)) - buf[2] |= (1 << 2); /* write cache enable */ - if (!changeable && !ata_id_rahead_enabled(id)) - buf[12] |= (1 << 5); /* disable read ahead */ + if (changeable) { + buf[2] |= 1 << 2; /* ata_mselect_caching() */ + } else { + buf[2] |= ata_id_wcache_enabled(id) << 2; /* write cache enable */ + buf[12] |= !ata_id_rahead_enabled(id) << 5; /* disable read ahead */ + } return sizeof(def_cache_mpage); }