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 [off-list ref] wrote:

On Tue, Jul 12, 2016 at 09:37:03PM +0800, tom.ty89@gmail.com wrote:

quoted

From: Tom Yan <redacted>

Signed-off-by: Tom Yan <redacted>

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