[PATCH resend 1/5] libata-scsi: minor cleanup in ata_mselect_*()

[PATCH resend 1/5] 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 2bdb5da..eb5e8ff 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3618,7 +3618,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;
@@ -3627,8 +3626,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.
 	 */
@@ -3642,6 +3639,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;
@@ -3674,7 +3673,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;
@@ -3683,8 +3681,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.
 	 */
@@ -3697,7 +3693,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 resend 2/5] 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 eb5e8ff..ac90676 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3631,8 +3631,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;
@@ -3686,8 +3696,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

[PATCH resend 3/5] libata-scsi: fix overflow in mode page copy

[tom.ty89]

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

ata_mselect_*() would initialize a char array for storing a copy of
the current mode page. However, if char was actually signed char,
overflow could occur.

For example, `0xff` from def_control_mpage[] would be "truncated"
to `-1`. This prevented ata_mselect_control() from working at all,
since when it did the read-only bits check, there would always be
a mismatch.

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

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index ac90676..3c93341 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3610,7 +3610,7 @@ static int ata_mselect_caching(struct ata_queued_cmd *qc,
 {
 	struct ata_taskfile *tf = &qc->tf;
 	struct ata_device *dev = qc->dev;
-	char mpage[CACHE_MPAGE_LEN];
+	u8 mpage[CACHE_MPAGE_LEN];
 	u8 wce;
 	int i;
 
@@ -3675,7 +3675,7 @@ static int ata_mselect_control(struct ata_queued_cmd *qc,
 			       const u8 *buf, int len, u16 *fp)
 {
 	struct ata_device *dev = qc->dev;
-	char mpage[CONTROL_MPAGE_LEN];
+	u8 mpage[CONTROL_MPAGE_LEN];
 	u8 d_sense;
 	int i;
 
-- 
2.9.0

[PATCH resend 4/5] libata-scsi: have all checks done before calling ata_mselect_*()

[tom.ty89]

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

The one-page-at-a-time check in ata_scsi_mode_select_xlat() should
be done before either of the ata_mselect_*() is called.

Also updated the comment. We have more than one mode page that has
changeable bit since commit 06dbde5f3a44 ("libata: Implement
control mode page to select sense format").

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

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 3c93341..6c424c5 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3837,6 +3837,12 @@ static unsigned int ata_scsi_mode_select_xlat(struct ata_queued_cmd *qc)
 	if (pg_len > len)
 		goto invalid_param_len;
 
+	/*
+	 * Currently we only support setting one page at a time.
+	 */
+	if (len > pg_len)
+		goto invalid_param;
+
 	switch (pg) {
 	case CACHE_MPAGE:
 		if (ata_mselect_caching(qc, p, pg_len, &fp) < 0) {
@@ -3855,13 +3861,6 @@ static unsigned int ata_scsi_mode_select_xlat(struct ata_queued_cmd *qc)
 		goto invalid_param;
 	}
 
-	/*
-	 * Only one page has changeable data, so we only support setting one
-	 * page at a time.
-	 */
-	if (len > pg_len)
-		goto invalid_param;
-
 	return 0;
 
  invalid_fld:
-- 
2.9.0

[PATCH resend 5/5] libata-scsi: fix MODE SELECT translation for Control mode page

[tom.ty89]

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

scsi_done() was called repeatedly and apparently because of that,
the kernel would call trace when we touch the Control mode page:

Call Trace:
 [<ffffffff812ea0d2>] dump_stack+0x63/0x81
 [<ffffffff81079cfb>] __warn+0xcb/0xf0
 [<ffffffff81079e2d>] warn_slowpath_null+0x1d/0x20
 [<ffffffffa00f51b0>] ata_eh_finish+0xe0/0xf0 [libata]
 [<ffffffffa00fb830>] sata_pmp_error_handler+0x640/0xa50 [libata]
 [<ffffffffa00470ed>] ahci_error_handler+0x1d/0x70 [libahci]
 [<ffffffffa00f55f0>] ata_scsi_port_error_handler+0x430/0x770 [libata]
 [<ffffffffa00eff8d>] ? ata_scsi_cmd_error_handler+0xdd/0x160 [libata]
 [<ffffffffa00f59d7>] ata_scsi_error+0xa7/0xf0 [libata]
 [<ffffffffa00913ba>] scsi_error_handler+0xaa/0x560 [scsi_mod]
 [<ffffffffa0091310>] ? scsi_eh_get_sense+0x180/0x180 [scsi_mod]
 [<ffffffff81098eb8>] kthread+0xd8/0xf0
 [<ffffffff815d913f>] ret_from_fork+0x1f/0x40
 [<ffffffff81098de0>] ? kthread_worker_fn+0x170/0x170
---[ end trace 8b7501047e928a17 ]---

Removed the unnecessary code and let ata_scsi_translate() do the job.

Also, since ata_mselect_control() has no ATA command to send to the
device, ata_scsi_mode_select_xlat() should return 1 for it, so that
ata_scsi_translate() will finish early to avoid ata_qc_issue().

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

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 6c424c5..f5c4200 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3720,8 +3720,6 @@ static int ata_mselect_control(struct ata_queued_cmd *qc,
 		dev->flags |= ATA_DFLAG_D_SENSE;
 	else
 		dev->flags &= ~ATA_DFLAG_D_SENSE;
-	qc->scsicmd->result = SAM_STAT_GOOD;
-	qc->scsicmd->scsi_done(qc->scsicmd);
 	return 0;
 }
 
@@ -3854,6 +3852,8 @@ static unsigned int ata_scsi_mode_select_xlat(struct ata_queued_cmd *qc)
 		if (ata_mselect_control(qc, p, pg_len, &fp) < 0) {
 			fp += hdr_len + bd_len;
 			goto invalid_param;
+		} else {
+			goto skip; /* No ATA command to send */
 		}
 		break;
 	default:		/* invalid page code */
-- 
2.9.0

Re: [PATCH resend 5/5] libata-scsi: fix MODE SELECT translation for Control mode page

[Tejun Heo]

On Fri, Jul 22, 2016 at 02:41:54AM +0800, tom.ty89@gmail.com wrote:
> @@ -3854,6 +3852,8 @@ static unsigned int ata_scsi_mode_select_xlat(struct ata_queued_cmd *qc)
>  		if (ata_mselect_control(qc, p, pg_len, &fp) < 0) {
>  			fp += hdr_len + bd_len;
>  			goto invalid_param;
> +		} else {
> +			goto skip; /* No ATA command to send */

Hmmm... I'm a bit confused.  Why is mselect_control path different
from mselect_caching in terms of qc handling?

Thanks.

-- 
tejun

Re: [PATCH resend 5/5] libata-scsi: fix MODE SELECT translation for Control mode page

[Tom Yan]

As I've mentioned in the comment/message, there is no ATA command
needed to be sent to the device, since it only toggles a bit in
dev->flags. See that there is no ata_taskfile constructed in
ata_mselect_control().

On 22 July 2016 at 05:26, Tejun Heo <tj@kernel.org> wrote:
> On Fri, Jul 22, 2016 at 02:41:54AM +0800, tom.ty89@gmail.com wrote:
>> @@ -3854,6 +3852,8 @@ static unsigned int ata_scsi_mode_select_xlat(struct ata_queued_cmd *qc)
>>               if (ata_mselect_control(qc, p, pg_len, &fp) < 0) {
>>                       fp += hdr_len + bd_len;
>>                       goto invalid_param;
>> +             } else {
>> +                     goto skip; /* No ATA command to send */
>
> Hmmm... I'm a bit confused.  Why is mselect_control path different
> from mselect_caching in terms of qc handling?
>
> Thanks.
>
> --
> tejun

Re: [PATCH resend 5/5] libata-scsi: fix MODE SELECT translation for Control mode page

[Tejun Heo]

On Fri, Jul 22, 2016 at 05:50:18AM +0800, Tom Yan wrote:
> As I've mentioned in the comment/message, there is no ATA command
> needed to be sent to the device, since it only toggles a bit in
> dev->flags. See that there is no ata_taskfile constructed in
> ata_mselect_control().

But ata_mselect_caching() contructs a tf?

-- 
tejun

Re: [PATCH resend 5/5] libata-scsi: fix MODE SELECT translation for Control mode page

[Tom Yan]

Yes, because it touches an actual ATA drive setting, while D_SENSE is
merely a "software setting" stored in dev->flags to make the kernel
response differently when build sense data.

On 26 July 2016 at 02:30, Tejun Heo <tj@kernel.org> wrote:
> On Fri, Jul 22, 2016 at 05:50:18AM +0800, Tom Yan wrote:
>> As I've mentioned in the comment/message, there is no ATA command
>> needed to be sent to the device, since it only toggles a bit in
>> dev->flags. See that there is no ata_taskfile constructed in
>> ata_mselect_control().
>
> But ata_mselect_caching() contructs a tf?
>
> --
> tejun

Re: [PATCH resend 5/5] libata-scsi: fix MODE SELECT translation for Control mode page

[Tejun Heo]

On Fri, Jul 22, 2016 at 02:41:54AM +0800, tom.ty89@gmail.com wrote:
> From: Tom Yan <tom.ty89@gmail.com>
> 
> scsi_done() was called repeatedly and apparently because of that,
> the kernel would call trace when we touch the Control mode page:
> 
> Call Trace:
>  [<ffffffff812ea0d2>] dump_stack+0x63/0x81
>  [<ffffffff81079cfb>] __warn+0xcb/0xf0
>  [<ffffffff81079e2d>] warn_slowpath_null+0x1d/0x20
>  [<ffffffffa00f51b0>] ata_eh_finish+0xe0/0xf0 [libata]
>  [<ffffffffa00fb830>] sata_pmp_error_handler+0x640/0xa50 [libata]
>  [<ffffffffa00470ed>] ahci_error_handler+0x1d/0x70 [libahci]
>  [<ffffffffa00f55f0>] ata_scsi_port_error_handler+0x430/0x770 [libata]
>  [<ffffffffa00eff8d>] ? ata_scsi_cmd_error_handler+0xdd/0x160 [libata]
>  [<ffffffffa00f59d7>] ata_scsi_error+0xa7/0xf0 [libata]
>  [<ffffffffa00913ba>] scsi_error_handler+0xaa/0x560 [scsi_mod]
>  [<ffffffffa0091310>] ? scsi_eh_get_sense+0x180/0x180 [scsi_mod]
>  [<ffffffff81098eb8>] kthread+0xd8/0xf0
>  [<ffffffff815d913f>] ret_from_fork+0x1f/0x40
>  [<ffffffff81098de0>] ? kthread_worker_fn+0x170/0x170
> ---[ end trace 8b7501047e928a17 ]---
> 
> Removed the unnecessary code and let ata_scsi_translate() do the job.
> 
> Also, since ata_mselect_control() has no ATA command to send to the
> device, ata_scsi_mode_select_xlat() should return 1 for it, so that
> ata_scsi_translate() will finish early to avoid ata_qc_issue().
> 
> Signed-off-by: Tom Yan <tom.ty89@gmail.com>

Applied to libata/for-4.9.

Thanks.

-- 
tejun

Re: [PATCH resend 3/5] libata-scsi: fix overflow in mode page copy

[Tejun Heo]

Hello,

On Fri, Jul 22, 2016 at 02:41:52AM +0800, tom.ty89@gmail.com wrote:
> From: Tom Yan <tom.ty89@gmail.com>
> 
> ata_mselect_*() would initialize a char array for storing a copy of
> the current mode page. However, if char was actually signed char,
> overflow could occur.

Do you mean sign extension?

> For example, `0xff` from def_control_mpage[] would be "truncated"
> to `-1`. This prevented ata_mselect_control() from working at all,
> since when it did the read-only bits check, there would always be
> a mismatch.

Heh, the description doesn't really make sense.  Are you talking about
something like the following?

	char ar[N];
	int i;

	i = ar[x];
	if (i == 0xff)
		asdf;

If so, the description isn't quite right.

Thanks.

-- 
tejun

Re: [PATCH resend 3/5] libata-scsi: fix overflow in mode page copy

[Tom Yan]

Well, I mean this is happening when ata_mselect_*() calls ata_msense_*():

[tom@localhost ~]$ cat test.c
#include <stdio.h>
#include <string.h>

typedef unsigned char u8;

int main() {
  u8 a[2] = { 0xff, 0xff };
  char b[2];
  memcpy(b, a, 2);

  for (int i=0; i<2; i++) {
    printf("%d\n", a[i]);
  }

  for (int i=0; i<2; i++) {
    printf("%d\n", b[i]);
  }
}

[tom@localhost ~]$ cc test.c

[tom@localhost ~]$ ./a.out
255
255
-1
-1

Let me know how I should polish the description for this.

On 22 July 2016 at 05:17, Tejun Heo <tj@kernel.org> wrote:
> Hello,
>
> On Fri, Jul 22, 2016 at 02:41:52AM +0800, tom.ty89@gmail.com wrote:
>> From: Tom Yan <tom.ty89@gmail.com>
>>
>> ata_mselect_*() would initialize a char array for storing a copy of
>> the current mode page. However, if char was actually signed char,
>> overflow could occur.
>
> Do you mean sign extension?
>
>> For example, `0xff` from def_control_mpage[] would be "truncated"
>> to `-1`. This prevented ata_mselect_control() from working at all,
>> since when it did the read-only bits check, there would always be
>> a mismatch.
>
> Heh, the description doesn't really make sense.  Are you talking about
> something like the following?
>
>         char ar[N];
>         int i;
>
>         i = ar[x];
>         if (i == 0xff)
>                 asdf;
>
> If so, the description isn't quite right.
>
> Thanks.
>
> --
> tejun

Re: [PATCH resend 3/5] libata-scsi: fix overflow in mode page copy

[Tejun Heo]

On Fri, Jul 22, 2016 at 05:39:27AM +0800, Tom Yan wrote:
> Let me know how I should polish the description for this.

The above is because the signed ones are getting sign-extended making
them different from the unsigned ones which don't get padded in the
high bits.  Converting to u8 is the right thing to do but nothing here
is getting truncated.

Thanks.

-- 
tejun

[PATCH resend v2 3/5] libata-scsi: use u8 array to store mode page copy

[tom.ty89]

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

ata_mselect_*() would initialize a char array for storing a copy of
the current mode page. However, char could be signed char. In that
case, bytes larger than 127 would be converted to negative number.

For example, 0xff from def_control_mpage[] would become -1. This
prevented ata_mselect_control() from working at all, since when it
did the read-only bits check, there would always be a mismatch.

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

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 4a4e6f1..a28e2ea94 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3610,7 +3610,7 @@ static int ata_mselect_caching(struct ata_queued_cmd *qc,
 {
 	struct ata_taskfile *tf = &qc->tf;
 	struct ata_device *dev = qc->dev;
-	char mpage[CACHE_MPAGE_LEN];
+	u8 mpage[CACHE_MPAGE_LEN];
 	u8 wce, mask;
 	int i;
 
@@ -3668,7 +3668,7 @@ static int ata_mselect_control(struct ata_queued_cmd *qc,
 			       const u8 *buf, int len, u16 *fp)
 {
 	struct ata_device *dev = qc->dev;
-	char mpage[CONTROL_MPAGE_LEN];
+        u8 mpage[CONTROL_MPAGE_LEN];
 	u8 d_sense, mask;
 	int i;
 
-- 
2.9.0

Re: [PATCH resend v2 3/5] libata-scsi: use u8 array to store mode page copy

[Sergei Shtylyov]

Hello.

On 7/22/2016 2:29 AM, tom.ty89@gmail.com wrote:

> From: Tom Yan <tom.ty89@gmail.com>
>
> ata_mselect_*() would initialize a char array for storing a copy of
> the current mode page. However, char could be signed char. In that
> case, bytes larger than 127 would be converted to negative number.
>
> For example, 0xff from def_control_mpage[] would become -1. This
> prevented ata_mselect_control() from working at all, since when it
> did the read-only bits check, there would always be a mismatch.
>
> Signed-off-by: Tom Yan <tom.ty89@gmail.com>
>
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 4a4e6f1..a28e2ea94 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -3610,7 +3610,7 @@ static int ata_mselect_caching(struct ata_queued_cmd *qc,
>  {
>  	struct ata_taskfile *tf = &qc->tf;
>  	struct ata_device *dev = qc->dev;
> -	char mpage[CACHE_MPAGE_LEN];
> +	u8 mpage[CACHE_MPAGE_LEN];
>  	u8 wce, mask;
>  	int i;
>
> @@ -3668,7 +3668,7 @@ static int ata_mselect_control(struct ata_queued_cmd *qc,
>  			       const u8 *buf, int len, u16 *fp)
>  {
>  	struct ata_device *dev = qc->dev;
> -	char mpage[CONTROL_MPAGE_LEN];
> +        u8 mpage[CONTROL_MPAGE_LEN];

    Indent with tabs please, as above.

[...]

MBR, Sergei

Re: [PATCH resend v2 3/5] libata-scsi: use u8 array to store mode page copy

[Tom Yan]

Strange. I merely changed the two "char" to "u8". I wonder how the tab
became spaces. Anyway, sorry about that, resending soon.

On 22 July 2016 at 17:59, Sergei Shtylyov
<sergei.shtylyov@cogentembedded.com> wrote:
> Hello.
>
>
> On 7/22/2016 2:29 AM, tom.ty89@gmail.com wrote:
>
>> From: Tom Yan <tom.ty89@gmail.com>
>>
>> ata_mselect_*() would initialize a char array for storing a copy of
>> the current mode page. However, char could be signed char. In that
>> case, bytes larger than 127 would be converted to negative number.
>>
>> For example, 0xff from def_control_mpage[] would become -1. This
>> prevented ata_mselect_control() from working at all, since when it
>> did the read-only bits check, there would always be a mismatch.
>>
>> Signed-off-by: Tom Yan <tom.ty89@gmail.com>
>>
>> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
>> index 4a4e6f1..a28e2ea94 100644
>> --- a/drivers/ata/libata-scsi.c
>> +++ b/drivers/ata/libata-scsi.c
>> @@ -3610,7 +3610,7 @@ static int ata_mselect_caching(struct ata_queued_cmd
>> *qc,
>>  {
>>         struct ata_taskfile *tf = &qc->tf;
>>         struct ata_device *dev = qc->dev;
>> -       char mpage[CACHE_MPAGE_LEN];
>> +       u8 mpage[CACHE_MPAGE_LEN];
>>         u8 wce, mask;
>>         int i;
>>
>> @@ -3668,7 +3668,7 @@ static int ata_mselect_control(struct ata_queued_cmd
>> *qc,
>>                                const u8 *buf, int len, u16 *fp)
>>  {
>>         struct ata_device *dev = qc->dev;
>> -       char mpage[CONTROL_MPAGE_LEN];
>> +        u8 mpage[CONTROL_MPAGE_LEN];
>
>
>    Indent with tabs please, as above.
>
> [...]
>
> MBR, Sergei
>

[PATCH resend v3 3/5] libata-scsi: use u8 array to store mode page copy

[tom.ty89]

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

ata_mselect_*() would initialize a char array for storing a copy of
the current mode page. However, char could be signed char. In that
case, bytes larger than 127 would be converted to negative number.

For example, 0xff from def_control_mpage[] would become -1. This
prevented ata_mselect_control() from working at all, since when it
did the read-only bits check, there would always be a mismatch.

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

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 4a4e6f1..5cfcb4b 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3610,7 +3610,7 @@ static int ata_mselect_caching(struct ata_queued_cmd *qc,
 {
 	struct ata_taskfile *tf = &qc->tf;
 	struct ata_device *dev = qc->dev;
-	char mpage[CACHE_MPAGE_LEN];
+	u8 mpage[CACHE_MPAGE_LEN];
 	u8 wce, mask;
 	int i;
 
@@ -3668,7 +3668,7 @@ static int ata_mselect_control(struct ata_queued_cmd *qc,
 			       const u8 *buf, int len, u16 *fp)
 {
 	struct ata_device *dev = qc->dev;
-	char mpage[CONTROL_MPAGE_LEN];
+	u8 mpage[CONTROL_MPAGE_LEN];
 	u8 d_sense, mask;
 	int i;
 
-- 
2.9.0

Re: [PATCH resend v3 3/5] libata-scsi: use u8 array to store mode page copy

[Tejun Heo]

On Sat, Jul 23, 2016 at 02:34:08AM +0800, tom.ty89@gmail.com wrote:
> From: Tom Yan <tom.ty89@gmail.com>
> 
> ata_mselect_*() would initialize a char array for storing a copy of
> the current mode page. However, char could be signed char. In that
> case, bytes larger than 127 would be converted to negative number.
> 
> For example, 0xff from def_control_mpage[] would become -1. This
> prevented ata_mselect_control() from working at all, since when it
> did the read-only bits check, there would always be a mismatch.
> 
> Signed-off-by: Tom Yan <tom.ty89@gmail.com>

Applied to libata/for-4.9.

Thanks.

-- 
tejun

[PATCH resend v2 3/5] libata-scsi: use u8 array to store mode page copy

[tom.ty89]

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

ata_mselect_*() would initialize a char array for storing a copy of
the current mode page. However, char could be signed char. In that
case, bytes larger than 127 would be converted to negative number.

For example, 0xff from def_control_mpage[] would become -1. This
prevented ata_mselect_control() from working at all, since when it
did the read-only bits check, there would always be a mismatch.

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

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 4a4e6f1..a28e2ea94 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3610,7 +3610,7 @@ static int ata_mselect_caching(struct ata_queued_cmd *qc,
 {
 	struct ata_taskfile *tf = &qc->tf;
 	struct ata_device *dev = qc->dev;
-	char mpage[CACHE_MPAGE_LEN];
+	u8 mpage[CACHE_MPAGE_LEN];
 	u8 wce, mask;
 	int i;
 
@@ -3668,7 +3668,7 @@ static int ata_mselect_control(struct ata_queued_cmd *qc,
 			       const u8 *buf, int len, u16 *fp)
 {
 	struct ata_device *dev = qc->dev;
-	char mpage[CONTROL_MPAGE_LEN];
+        u8 mpage[CONTROL_MPAGE_LEN];
 	u8 d_sense, mask;
 	int i;
 
-- 
2.9.0

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

[Tom Yan]

This is actually a bit clumsy. Sending a rewritten version.

On 22 July 2016 at 02:41,  <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 eb5e8ff..ac90676 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -3631,8 +3631,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;
> @@ -3686,8 +3696,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
>

[PATCH resend v2 2/5] 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.

Made it check every byte but mask the changeable 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 eb5e8ff..4a4e6f1 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3611,7 +3611,7 @@ static int ata_mselect_caching(struct ata_queued_cmd *qc,
 	struct ata_taskfile *tf = &qc->tf;
 	struct ata_device *dev = qc->dev;
 	char mpage[CACHE_MPAGE_LEN];
-	u8 wce;
+	u8 wce, mask;
 	int i;
 
 	/*
@@ -3632,8 +3632,11 @@ 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;
-		if (mpage[i + 2] != buf[i]) {
+			mask = 0xfb;
+		else
+			mask = 0xff;
+
+		if ((mpage[i + 2] & mask) != (buf[i] & mask)) {
 			*fp = i;
 			return -EINVAL;
 		}
@@ -3666,7 +3669,7 @@ static int ata_mselect_control(struct ata_queued_cmd *qc,
 {
 	struct ata_device *dev = qc->dev;
 	char mpage[CONTROL_MPAGE_LEN];
-	u8 d_sense;
+	u8 d_sense, mask;
 	int i;
 
 	/*
@@ -3687,8 +3690,11 @@ 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;
-		if (mpage[2 + i] != buf[i]) {
+			mask = 0xfb;
+		else
+			mask = 0xff;
+
+		if ((mpage[2 + i] & mask) != (buf[i] & mask)) {
 			*fp = i;
 			return -EINVAL;
 		}
-- 
2.9.0

[PATCH resend v2 2/5] 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.

Made it check every byte but mask the changeable 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 eb5e8ff..4a4e6f1 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3611,7 +3611,7 @@ static int ata_mselect_caching(struct ata_queued_cmd *qc,
 	struct ata_taskfile *tf = &qc->tf;
 	struct ata_device *dev = qc->dev;
 	char mpage[CACHE_MPAGE_LEN];
-	u8 wce;
+	u8 wce, mask;
 	int i;
 
 	/*
@@ -3632,8 +3632,11 @@ 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;
-		if (mpage[i + 2] != buf[i]) {
+			mask = 0xfb;
+		else
+			mask = 0xff;
+
+		if ((mpage[i + 2] & mask) != (buf[i] & mask)) {
 			*fp = i;
 			return -EINVAL;
 		}
@@ -3666,7 +3669,7 @@ static int ata_mselect_control(struct ata_queued_cmd *qc,
 {
 	struct ata_device *dev = qc->dev;
 	char mpage[CONTROL_MPAGE_LEN];
-	u8 d_sense;
+	u8 d_sense, mask;
 	int i;
 
 	/*
@@ -3687,8 +3690,11 @@ 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;
-		if (mpage[2 + i] != buf[i]) {
+			mask = 0xfb;
+		else
+			mask = 0xff;
+
+		if ((mpage[2 + i] & mask) != (buf[i] & mask)) {
 			*fp = i;
 			return -EINVAL;
 		}
-- 
2.9.0

Re: [PATCH resend 1/5] libata-scsi: minor cleanup in ata_mselect_*()

[Sergei Shtylyov]

Hello.

On 07/21/2016 09:41 PM, 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>

    Perhaps Tejun is OK with that, but if you ask me doing 1 task per patch is 
a better practice.

[...]

MBR, Sergei

Re: [PATCH resend 1/5] libata-scsi: minor cleanup in ata_mselect_*()

[Tom Yan]

This patch should cause no behavioral change. Even the (removal of)
the redundant bit mask should be a nop. So it seems like a bit of an
overkill to split them.

On 22 July 2016 at 02:45, Sergei Shtylyov
<sergei.shtylyov@cogentembedded.com> wrote:
> Hello.
>
> On 07/21/2016 09:41 PM, 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>
>
>
>    Perhaps Tejun is OK with that, but if you ask me doing 1 task per patch
> is a better practice.
>
> [...]
>
> MBR, Sergei
>