[PATCH v3 2/2] libata-scsi: better style in ata_msense_caching()

[PATCH v3 2/2] libata-scsi: better style in ata_msense_caching()

[tom.ty89]

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 */
+	}
 	return sizeof(def_cache_mpage);
 }
 
-- 
2.9.0

Re: [PATCH v3 2/2] libata-scsi: better style in ata_msense_caching()

[Tejun Heo]

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

Re: [PATCH v3 2/2] libata-scsi: better style in ata_msense_caching()

[Tom Yan]

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

Re: [PATCH v3 2/2] libata-scsi: better style in ata_msense_caching()

[Tejun Heo]

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

Re: [PATCH v3 2/2] libata-scsi: better style in ata_msense_caching()

[Tom Yan]

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

[PATCH v4] libata-scsi: better style in ata_msense_*()

[tom.ty89]

From: Tom Yan <tom.ty89@gmail.com>

`changeable` is the "version" of mode page requested by the user.
It will be less confusing/misleading if we do not check it
"together" with the setting bits of the drive.

Not to mention that we currently have ata_mselect_*() implemented
in a way that each of them will serve exclusively a particular bit
on each page. The old style will hence make the condition look even
more unnecessarily arcane if the ata_msense_*() is reflecting more
than one bit.

Signed-off-by: Tom Yan <tom.ty89@gmail.com>

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index bf4cb21..b00521b 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);
 }
 
@@ -2446,8 +2448,13 @@ static unsigned int ata_msense_control(struct ata_device *dev, u8 *buf,
 					bool changeable)
 {
 	modecpy(buf, def_control_mpage, sizeof(def_control_mpage), changeable);
-	if (changeable || (dev->flags & ATA_DFLAG_D_SENSE))
-		buf[2] |= (1 << 2);	/* Descriptor sense requested */
+	if (changeable) {
+		buf[2] |= (1 << 2);	/* ata_mselect_control() */
+	} else {
+		bool d_sense = (dev->flags & ATA_DFLAG_D_SENSE);
+
+		buf[2] |= (d_sense << 2);	/* descriptor format sense data */
+	}
 	return sizeof(def_control_mpage);
 }
 
-- 
2.9.0

Re: [PATCH v4] libata-scsi: better style in ata_msense_*()

[Tom Yan]

FWIW, I end up using `bool d_sense = (dev->flags &
ATA_DFLAG_D_SENSE)`, which was introduced to ata_scsi_set_sense() in
commit 06dbde5f3a44 ("libata: Implement control mode page to select
sense format"), in the polished ata_msense_control().

@Sergei, I kept the parentheses for the bit shifting/masking results
because it appears that the rest of the code in libata-scsi.c does
that as well. So I am not going to partially clean up that to avoid
making it look inconsistent.

On 20 July 2016 at 04:39,  <tom.ty89@gmail.com> wrote:
> From: Tom Yan <tom.ty89@gmail.com>
>
> `changeable` is the "version" of mode page requested by the user.
> It will be less confusing/misleading if we do not check it
> "together" with the setting bits of the drive.
>
> Not to mention that we currently have ata_mselect_*() implemented
> in a way that each of them will serve exclusively a particular bit
> on each page. The old style will hence make the condition look even
> more unnecessarily arcane if the ata_msense_*() is reflecting more
> than one bit.
>
> Signed-off-by: Tom Yan <tom.ty89@gmail.com>
>
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index bf4cb21..b00521b 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);
>  }
>
> @@ -2446,8 +2448,13 @@ static unsigned int ata_msense_control(struct ata_device *dev, u8 *buf,
>                                         bool changeable)
>  {
>         modecpy(buf, def_control_mpage, sizeof(def_control_mpage), changeable);
> -       if (changeable || (dev->flags & ATA_DFLAG_D_SENSE))
> -               buf[2] |= (1 << 2);     /* Descriptor sense requested */
> +       if (changeable) {
> +               buf[2] |= (1 << 2);     /* ata_mselect_control() */
> +       } else {
> +               bool d_sense = (dev->flags & ATA_DFLAG_D_SENSE);
> +
> +               buf[2] |= (d_sense << 2);       /* descriptor format sense data */
> +       }
>         return sizeof(def_control_mpage);
>  }
>
> --
> 2.9.0
>

[PATCH v4] libata-scsi: minor cleanup in ata_mselect_*()

[tom.ty89]

From: Tom Yan <tom.ty89@gmail.com>

1. Removed a repeated bit masking in ata_mselect_control()
2. Moved `wce`/`d_sense` assignment below the page validity checks
3. Added/Removed empty lines where appropriate

Signed-off-by: Tom Yan <tom.ty89@gmail.com>

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index b00521b..06afe63 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3604,7 +3604,6 @@ static int ata_mselect_caching(struct ata_queued_cmd *qc,
 	 * The first two bytes of def_cache_mpage are a header, so offsets
 	 * in mpage are off by 2 compared to buf.  Same for len.
 	 */
-
 	if (len != CACHE_MPAGE_LEN - 2) {
 		if (len < CACHE_MPAGE_LEN - 2)
 			*fp = len;
@@ -3613,8 +3612,6 @@ static int ata_mselect_caching(struct ata_queued_cmd *qc,
 		return -EINVAL;
 	}
 
-	wce = buf[0] & (1 << 2);
-
 	/*
 	 * Check that read-only bits are not modified.
 	 */
@@ -3628,6 +3625,8 @@ static int ata_mselect_caching(struct ata_queued_cmd *qc,
 		}
 	}
 
+	wce = buf[0] & (1 << 2);
+
 	tf->flags |= ATA_TFLAG_DEVICE | ATA_TFLAG_ISADDR;
 	tf->protocol = ATA_PROT_NODATA;
 	tf->nsect = 0;
@@ -3660,7 +3659,6 @@ static int ata_mselect_control(struct ata_queued_cmd *qc,
 	 * The first two bytes of def_control_mpage are a header, so offsets
 	 * in mpage are off by 2 compared to buf.  Same for len.
 	 */
-
 	if (len != CONTROL_MPAGE_LEN - 2) {
 		if (len < CONTROL_MPAGE_LEN - 2)
 			*fp = len;
@@ -3669,8 +3667,6 @@ static int ata_mselect_control(struct ata_queued_cmd *qc,
 		return -EINVAL;
 	}
 
-	d_sense = buf[0] & (1 << 2);
-
 	/*
 	 * Check that read-only bits are not modified.
 	 */
@@ -3683,7 +3679,10 @@ static int ata_mselect_control(struct ata_queued_cmd *qc,
 			return -EINVAL;
 		}
 	}
-	if (d_sense & (1 << 2))
+
+	d_sense = buf[0] & (1 << 2);
+
+	if (d_sense)
 		dev->flags |= ATA_DFLAG_D_SENSE;
 	else
 		dev->flags &= ~ATA_DFLAG_D_SENSE;
-- 
2.9.0

[PATCH] libata-scsi: fix read-only bits checking in ata_mselect_*()

[tom.ty89]

From: Tom Yan <tom.ty89@gmail.com>

Commit 7780081c1f04 ("libata-scsi: Set information sense field for
invalid parameter") changed how ata_mselect_*() make sure read-only
bits are not modified. The new implementation introduced a bug that
the read-only bits in the byte that has a changeable bit will not
be checked.

Added the necessary check, with comments explaining the heuristic.

Signed-off-by: Tom Yan <tom.ty89@gmail.com>

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 06afe63..005d186 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3617,8 +3617,18 @@ static int ata_mselect_caching(struct ata_queued_cmd *qc,
 	 */
 	ata_msense_caching(dev->id, mpage, false);
 	for (i = 0; i < CACHE_MPAGE_LEN - 2; i++) {
-		if (i == 0)
-			continue;
+		/* Check the first byte */
+		if (i == 0) {
+			/* except the WCE bit */
+			if (mpage[i + 2] & 0xfb != buf[i] & 0xfb) {
+				continue;
+			} else {
+				*fp = i;
+				return -EINVAL;
+			}
+		}
+
+		/* Check the remaining bytes */
 		if (mpage[i + 2] != buf[i]) {
 			*fp = i;
 			return -EINVAL;
@@ -3672,8 +3682,18 @@ static int ata_mselect_control(struct ata_queued_cmd *qc,
 	 */
 	ata_msense_control(dev, mpage, false);
 	for (i = 0; i < CONTROL_MPAGE_LEN - 2; i++) {
-		if (i == 0)
-			continue;
+		/* Check the first byte */
+		if (i == 0) {
+			/* except the D_SENSE bit */
+			if (mpage[i + 2] & 0xfb != buf[i] & 0xfb) {
+				continue;
+			} else {
+				*fp = i;
+				return -EINVAL;
+			}
+		}
+
+		/* Check the remaining bytes */
 		if (mpage[2 + i] != buf[i]) {
 			*fp = i;
 			return -EINVAL;
-- 
2.9.0

Re: [PATCH] libata-scsi: fix read-only bits checking in ata_mselect_*()

[Tom Yan]

Oops I put the wrong action under the opposite condition. Sending a v2.

On 20 July 2016 at 06:50,  <tom.ty89@gmail.com> wrote:
> From: Tom Yan <tom.ty89@gmail.com>
>
> Commit 7780081c1f04 ("libata-scsi: Set information sense field for
> invalid parameter") changed how ata_mselect_*() make sure read-only
> bits are not modified. The new implementation introduced a bug that
> the read-only bits in the byte that has a changeable bit will not
> be checked.
>
> Added the necessary check, with comments explaining the heuristic.
>
> Signed-off-by: Tom Yan <tom.ty89@gmail.com>
>
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 06afe63..005d186 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -3617,8 +3617,18 @@ static int ata_mselect_caching(struct ata_queued_cmd *qc,
>          */
>         ata_msense_caching(dev->id, mpage, false);
>         for (i = 0; i < CACHE_MPAGE_LEN - 2; i++) {
> -               if (i == 0)
> -                       continue;
> +               /* Check the first byte */
> +               if (i == 0) {
> +                       /* except the WCE bit */
> +                       if (mpage[i + 2] & 0xfb != buf[i] & 0xfb) {
> +                               continue;
> +                       } else {
> +                               *fp = i;
> +                               return -EINVAL;
> +                       }
> +               }
> +
> +               /* Check the remaining bytes */
>                 if (mpage[i + 2] != buf[i]) {
>                         *fp = i;
>                         return -EINVAL;
> @@ -3672,8 +3682,18 @@ static int ata_mselect_control(struct ata_queued_cmd *qc,
>          */
>         ata_msense_control(dev, mpage, false);
>         for (i = 0; i < CONTROL_MPAGE_LEN - 2; i++) {
> -               if (i == 0)
> -                       continue;
> +               /* Check the first byte */
> +               if (i == 0) {
> +                       /* except the D_SENSE bit */
> +                       if (mpage[i + 2] & 0xfb != buf[i] & 0xfb) {
> +                               continue;
> +                       } else {
> +                               *fp = i;
> +                               return -EINVAL;
> +                       }
> +               }
> +
> +               /* Check the remaining bytes */
>                 if (mpage[2 + i] != buf[i]) {
>                         *fp = i;
>                         return -EINVAL;
> --
> 2.9.0
>

[PATCH v2] libata-scsi: fix read-only bits checking in ata_mselect_*()

[tom.ty89]

From: Tom Yan <tom.ty89@gmail.com>

Commit 7780081c1f04 ("libata-scsi: Set information sense field for
invalid parameter") changed how ata_mselect_*() make sure read-only
bits are not modified. The new implementation introduced a bug that
the read-only bits in the byte that has a changeable bit will not
be checked.

Added the necessary check, with comments explaining the heuristic.

Signed-off-by: Tom Yan <tom.ty89@gmail.com>

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 06afe63..b47c3ce 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3617,8 +3617,18 @@ static int ata_mselect_caching(struct ata_queued_cmd *qc,
 	 */
 	ata_msense_caching(dev->id, mpage, false);
 	for (i = 0; i < CACHE_MPAGE_LEN - 2; i++) {
-		if (i == 0)
-			continue;
+		/* Check the first byte */
+		if (i == 0) {
+			/* except the WCE bit */
+			if (mpage[i + 2] & 0xfb != buf[i] & 0xfb) {
+				*fp = i;
+				return -EINVAL;
+			} else {
+				continue;
+			}
+		}
+
+		/* Check the remaining bytes */
 		if (mpage[i + 2] != buf[i]) {
 			*fp = i;
 			return -EINVAL;
@@ -3672,8 +3682,18 @@ static int ata_mselect_control(struct ata_queued_cmd *qc,
 	 */
 	ata_msense_control(dev, mpage, false);
 	for (i = 0; i < CONTROL_MPAGE_LEN - 2; i++) {
-		if (i == 0)
-			continue;
+		/* Check the first byte */
+		if (i == 0) {
+			/* except the D_SENSE bit */
+			if (mpage[i + 2] & 0xfb != buf[i] & 0xfb) {
+				*fp = i;
+				return -EINVAL;
+			} else {
+				continue;
+			}
+		}
+
+		/* Check the remaining bytes */
 		if (mpage[2 + i] != buf[i]) {
 			*fp = i;
 			return -EINVAL;
-- 
2.9.0

Re: [PATCH v4] libata-scsi: minor cleanup in ata_mselect_*()

[Tom Yan]

Wrongly marked this as a "v4". It's actually a "v1".

On 20 July 2016 at 05:11,  <tom.ty89@gmail.com> wrote:
> From: Tom Yan <tom.ty89@gmail.com>
>
> 1. Removed a repeated bit masking in ata_mselect_control()
> 2. Moved `wce`/`d_sense` assignment below the page validity checks
> 3. Added/Removed empty lines where appropriate
>
> Signed-off-by: Tom Yan <tom.ty89@gmail.com>
>
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index b00521b..06afe63 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -3604,7 +3604,6 @@ static int ata_mselect_caching(struct ata_queued_cmd *qc,
>          * The first two bytes of def_cache_mpage are a header, so offsets
>          * in mpage are off by 2 compared to buf.  Same for len.
>          */
> -
>         if (len != CACHE_MPAGE_LEN - 2) {
>                 if (len < CACHE_MPAGE_LEN - 2)
>                         *fp = len;
> @@ -3613,8 +3612,6 @@ static int ata_mselect_caching(struct ata_queued_cmd *qc,
>                 return -EINVAL;
>         }
>
> -       wce = buf[0] & (1 << 2);
> -
>         /*
>          * Check that read-only bits are not modified.
>          */
> @@ -3628,6 +3625,8 @@ static int ata_mselect_caching(struct ata_queued_cmd *qc,
>                 }
>         }
>
> +       wce = buf[0] & (1 << 2);
> +
>         tf->flags |= ATA_TFLAG_DEVICE | ATA_TFLAG_ISADDR;
>         tf->protocol = ATA_PROT_NODATA;
>         tf->nsect = 0;
> @@ -3660,7 +3659,6 @@ static int ata_mselect_control(struct ata_queued_cmd *qc,
>          * The first two bytes of def_control_mpage are a header, so offsets
>          * in mpage are off by 2 compared to buf.  Same for len.
>          */
> -
>         if (len != CONTROL_MPAGE_LEN - 2) {
>                 if (len < CONTROL_MPAGE_LEN - 2)
>                         *fp = len;
> @@ -3669,8 +3667,6 @@ static int ata_mselect_control(struct ata_queued_cmd *qc,
>                 return -EINVAL;
>         }
>
> -       d_sense = buf[0] & (1 << 2);
> -
>         /*
>          * Check that read-only bits are not modified.
>          */
> @@ -3683,7 +3679,10 @@ static int ata_mselect_control(struct ata_queued_cmd *qc,
>                         return -EINVAL;
>                 }
>         }
> -       if (d_sense & (1 << 2))
> +
> +       d_sense = buf[0] & (1 << 2);
> +
> +       if (d_sense)
>                 dev->flags |= ATA_DFLAG_D_SENSE;
>         else
>                 dev->flags &= ~ATA_DFLAG_D_SENSE;
> --
> 2.9.0
>

Re: [PATCH v4] libata-scsi: better style in ata_msense_*()

[Tejun Heo]

On Wed, Jul 20, 2016 at 04:39:28AM +0800, tom.ty89@gmail.com wrote:
> From: Tom Yan <tom.ty89@gmail.com>
> 
> `changeable` is the "version" of mode page requested by the user.
> It will be less confusing/misleading if we do not check it
> "together" with the setting bits of the drive.
> 
> Not to mention that we currently have ata_mselect_*() implemented
> in a way that each of them will serve exclusively a particular bit
> on each page. The old style will hence make the condition look even
> more unnecessarily arcane if the ata_msense_*() is reflecting more
> than one bit.
> 
> Signed-off-by: Tom Yan <tom.ty89@gmail.com>

Applied to libata/for-4.8.

Thanks.

-- 
tejun

Re: [PATCH v4] libata-scsi: minor cleanup in ata_mselect_*()

[Tejun Heo]

On Wed, Jul 20, 2016 at 05:11:50AM +0800, tom.ty89@gmail.com wrote:
> From: Tom Yan <tom.ty89@gmail.com>
> 
> 1. Removed a repeated bit masking in ata_mselect_control()
> 2. Moved `wce`/`d_sense` assignment below the page validity checks
> 3. Added/Removed empty lines where appropriate
> 
> Signed-off-by: Tom Yan <tom.ty89@gmail.com>

Applied to libata/for-4.8.

Thanks.

-- 
tejun

Re: [PATCH v2] libata-scsi: fix read-only bits checking in ata_mselect_*()

[Tejun Heo]

On Wed, Jul 20, 2016 at 06:59:23AM +0800, tom.ty89@gmail.com wrote:
> From: Tom Yan <tom.ty89@gmail.com>
> 
> Commit 7780081c1f04 ("libata-scsi: Set information sense field for
> invalid parameter") changed how ata_mselect_*() make sure read-only
> bits are not modified. The new implementation introduced a bug that
> the read-only bits in the byte that has a changeable bit will not
> be checked.
> 
> Added the necessary check, with comments explaining the heuristic.
> 
> Signed-off-by: Tom Yan <tom.ty89@gmail.com>

Applied to libata/for-4.8.

This thread was confusing to look at.  Please use the "n/N" sequence
tagging for the patches (the -n option of git-format-patch).

Thanks.

-- 
tejun

Re: [PATCH v2] libata-scsi: fix read-only bits checking in ata_mselect_*()

[Tejun Heo]

So, just reverted this patch.

On Wed, Jul 20, 2016 at 06:59:23AM +0800, tom.ty89@gmail.com wrote:
> From: Tom Yan <tom.ty89@gmail.com>
> 
> Commit 7780081c1f04 ("libata-scsi: Set information sense field for
> invalid parameter") changed how ata_mselect_*() make sure read-only
> bits are not modified. The new implementation introduced a bug that
> the read-only bits in the byte that has a changeable bit will not
> be checked.
> 
> Added the necessary check, with comments explaining the heuristic.
> 
> Signed-off-by: Tom Yan <tom.ty89@gmail.com>
> 
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 06afe63..b47c3ce 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -3617,8 +3617,18 @@ static int ata_mselect_caching(struct ata_queued_cmd *qc,
>  	 */
>  	ata_msense_caching(dev->id, mpage, false);
>  	for (i = 0; i < CACHE_MPAGE_LEN - 2; i++) {
> -		if (i == 0)
> -			continue;
> +		/* Check the first byte */
> +		if (i == 0) {
> +			/* except the WCE bit */
> +			if (mpage[i + 2] & 0xfb != buf[i] & 0xfb) {

This not only triggered compiler warning but is actually wrong.  The
above is

	mpage[i + 1] & (0xfb != buf[i]) & 0xfb

which is non-sensical.  So, this patch wasn't tested at all.  Not even
compile test.  Please don't do this.

Thanks.

-- 
tejun