[PATCH 2/2] sd: disable write same for SAT as per the comment

[PATCH 2/2] sd: disable write same for SAT as per the comment

[tom.ty89]

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

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

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 1179ec1..9eeee51 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2786,7 +2786,7 @@ static void sd_read_write_same(struct scsi_disk *sdkp, unsigned char *buffer)
 		 * CODES is unsupported and the device has an ATA
 		 * Information VPD page (SAT).
 		 */
-		if (!scsi_get_vpd_page(sdev, 0x89, buffer, vpd_buf_len))
+		if (scsi_get_vpd_page(sdev, 0x89, buffer, vpd_buf_len))
 			sdev->no_write_same = 1;
 	}
 
-- 
2.7.1

Re: [PATCH 2/2] sd: disable write same for SAT as per the comment

[James Bottomley]

On Sat, 2016-02-27 at 04:07 +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/scsi/sd.c b/drivers/scsi/sd.c
> index 1179ec1..9eeee51 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -2786,7 +2786,7 @@ static void sd_read_write_same(struct scsi_disk
> *sdkp, unsigned char *buffer)
>  		 * CODES is unsupported and the device has an ATA
>  		 * Information VPD page (SAT).
>  		 */
> -		if (!scsi_get_vpd_page(sdev, 0x89, buffer,
> vpd_buf_len))
> +		if (scsi_get_vpd_page(sdev, 0x89, buffer,
> vpd_buf_len))
>  			sdev->no_write_same = 1;
>  	}


If you're inverting the condition, you'd need to invert the comment as
well.  scsi_get_vpd_page returns 0 on success so !scsi_get_vpd_page is
true if it got the page (which is what the comment says).

James

Re: [PATCH 2/2] sd: disable write same for SAT as per the comment

[Tom Yan]

Oh I made a mistake on this one then.

Since I send it with another patch, should I resend that alone?

On 27 February 2016 at 04:16, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
> On Sat, 2016-02-27 at 04:07 +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/scsi/sd.c b/drivers/scsi/sd.c
>> index 1179ec1..9eeee51 100644
>> --- a/drivers/scsi/sd.c
>> +++ b/drivers/scsi/sd.c
>> @@ -2786,7 +2786,7 @@ static void sd_read_write_same(struct scsi_disk
>> *sdkp, unsigned char *buffer)
>>                * CODES is unsupported and the device has an ATA
>>                * Information VPD page (SAT).
>>                */
>> -             if (!scsi_get_vpd_page(sdev, 0x89, buffer,
>> vpd_buf_len))
>> +             if (scsi_get_vpd_page(sdev, 0x89, buffer,
>> vpd_buf_len))
>>                       sdev->no_write_same = 1;
>>       }
>
>
> If you're inverting the condition, you'd need to invert the comment as
> well.  scsi_get_vpd_page returns 0 on success so !scsi_get_vpd_page is
> true if it got the page (which is what the comment says).
>
> James
>

Re: [PATCH 2/2] sd: disable write same for SAT as per the comment

[James Bottomley]

On Sat, 2016-02-27 at 04:32 +0800, Tom Yan wrote:
> Oh I made a mistake on this one then.
> 
> Since I send it with another patch, should I resend that alone?

Yes, that's fine. but you need to explain as part of the changelog why
this condition needs inverting because your Subject just says "as per
the comment" which makes no sense if you're changing the comment.

James

Re: [PATCH 2/2] sd: disable write same for SAT as per the comment

[Tom Yan]

No I mean I no longer thinks that this condition needs to be inverted.
I just THOUGHT that !scsi_get_vpd_page is true if it DIDN'T get the
page.

On 27 February 2016 at 04:57, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
> On Sat, 2016-02-27 at 04:32 +0800, Tom Yan wrote:
>> Oh I made a mistake on this one then.
>>
>> Since I send it with another patch, should I resend that alone?
>
> Yes, that's fine. but you need to explain as part of the changelog why
> this condition needs inverting because your Subject just says "as per
> the comment" which makes no sense if you're changing the comment.
>
> James
>
>