Patchwork [1/6] libata: Signal that our SATL supports WRITE SAME(16) with UNMAP

login
register
mail settings
Submitter Martin K. Petersen
Date Aug. 19, 2010, 3:48 p.m.
Message ID <1282232941-9910-2-git-send-email-martin.petersen@oracle.com>
Download mbox | patch
Permalink /patch/62181/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Martin K. Petersen - Aug. 19, 2010, 3:48 p.m.
Until now identifying that a device supports WRITE SAME(16) with the
UNMAP bit set has been black magic.  Implement support for the new (SBC3
r24) Thin Provisioning VPD page and the TPWS bit.

Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
---
 drivers/ata/libata-scsi.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)
James Bottomley - Aug. 19, 2010, 4:15 p.m.
On Thu, 2010-08-19 at 11:48 -0400, Martin K. Petersen wrote:
> Until now identifying that a device supports WRITE SAME(16) with the
> UNMAP bit set has been black magic.  Implement support for the new (SBC3
> r24) Thin Provisioning VPD page and the TPWS bit.
> 
> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
> ---
>  drivers/ata/libata-scsi.c |   13 +++++++++++++
>  1 files changed, 13 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index a54273d..e280ae6 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -2001,6 +2001,7 @@ static unsigned int ata_scsiop_inq_00(struct ata_scsi_args *args, u8 *rbuf)
>  		0x89,	/* page 0x89, ata info page */
>  		0xb0,	/* page 0xb0, block limits page */
>  		0xb1,	/* page 0xb1, block device characteristics page */
> +		0xb2,	/* page 0xb2, thin provisioning page */

Should this be unconditional?  Shouldn't it be conditioned on the
current supported trim indicator (which is word 169 being non-zero
unless they've changed it yet again)?

James


--
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
Martin K. Petersen - Aug. 19, 2010, 4:20 p.m.
>>>>> "James" == James Bottomley <James.Bottomley@suse.de> writes:

James> Should this be unconditional?  Shouldn't it be conditioned on the
James> current supported trim indicator (which is word 169 being
James> non-zero unless they've changed it yet again)?

I thought about it.  But the VPD page isn't gated on thin provisioning
being enabled.

With the SCSI hat on we won't enable discard unless TPE=1 in READ
CAPACITY(16).  Regardless of whether the device reports TPU=1 or TPWS=1
in the TP VPD.
Tejun Heo - Aug. 23, 2010, 8:32 a.m.
On 08/19/2010 05:48 PM, Martin K. Petersen wrote:
> Until now identifying that a device supports WRITE SAME(16) with the
> UNMAP bit set has been black magic.  Implement support for the new (SBC3
> r24) Thin Provisioning VPD page and the TPWS bit.
> 
> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
> ---
>  drivers/ata/libata-scsi.c |   13 +++++++++++++
>  1 files changed, 13 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index a54273d..e280ae6 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -2001,6 +2001,7 @@ static unsigned int ata_scsiop_inq_00(struct ata_scsi_args *args, u8 *rbuf)
>  		0x89,	/* page 0x89, ata info page */
>  		0xb0,	/* page 0xb0, block limits page */
>  		0xb1,	/* page 0xb1, block device characteristics page */
> +		0xb2,	/* page 0xb2, thin provisioning page */
>  	};
>  
>  	rbuf[3] = sizeof(pages);	/* number of supported VPD pages */
> @@ -2172,6 +2173,15 @@ static unsigned int ata_scsiop_inq_b1(struct ata_scsi_args *args, u8 *rbuf)
>  	return 0;
>  }
>  
> +static unsigned int ata_scsiop_inq_b2(struct ata_scsi_args *args, u8 *rbuf)
> +{
> +	rbuf[1] = 0xb1;
> +	rbuf[3] = 0x3c;
> +	rbuf[5] = 1 << 6;	/* TPWS */

I would love a bit more documentation here.

Thanks.
Douglas Gilbert - Aug. 23, 2010, 6:22 p.m.
On 10-08-23 04:32 AM, Tejun Heo wrote:
> On 08/19/2010 05:48 PM, Martin K. Petersen wrote:
>> Until now identifying that a device supports WRITE SAME(16) with the
>> UNMAP bit set has been black magic.  Implement support for the new (SBC3
>> r24) Thin Provisioning VPD page and the TPWS bit.
>>
>> Signed-off-by: Martin K. Petersen<martin.petersen@oracle.com>
>> ---
>>   drivers/ata/libata-scsi.c |   13 +++++++++++++
>>   1 files changed, 13 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
>> index a54273d..e280ae6 100644
>> --- a/drivers/ata/libata-scsi.c
>> +++ b/drivers/ata/libata-scsi.c
>> @@ -2001,6 +2001,7 @@ static unsigned int ata_scsiop_inq_00(struct ata_scsi_args *args, u8 *rbuf)
>>   		0x89,	/* page 0x89, ata info page */
>>   		0xb0,	/* page 0xb0, block limits page */
>>   		0xb1,	/* page 0xb1, block device characteristics page */
>> +		0xb2,	/* page 0xb2, thin provisioning page */
>>   	};
>>
>>   	rbuf[3] = sizeof(pages);	/* number of supported VPD pages */
>> @@ -2172,6 +2173,15 @@ static unsigned int ata_scsiop_inq_b1(struct ata_scsi_args *args, u8 *rbuf)
>>   	return 0;
>>   }
>>
>> +static unsigned int ata_scsiop_inq_b2(struct ata_scsi_args *args, u8 *rbuf)
>> +{
>> +	rbuf[1] = 0xb1;
>> +	rbuf[3] = 0x3c;
>> +	rbuf[5] = 1<<  6;	/* TPWS */
>
> I would love a bit more documentation here.

Yes, that style comes from my code in scsi_debug and
some of my utilities. Recently I added some more
documentation, just prior the the function:

/* SCSI Thin Provisioning VPD page: SBC-3 rev 22 or later */

I see no point in commenting the individual code lines.
Just follow that reference ...

And when I do then I see that rbuf[3] should be 4 (not 0x3c)
unless the DP bit is set.

Doug Gilbert


--
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
Douglas Gilbert - Aug. 23, 2010, 6:29 p.m.
On 10-08-23 02:22 PM, Douglas Gilbert wrote:
> On 10-08-23 04:32 AM, Tejun Heo wrote:
>> On 08/19/2010 05:48 PM, Martin K. Petersen wrote:
>>> Until now identifying that a device supports WRITE SAME(16) with the
>>> UNMAP bit set has been black magic. Implement support for the new (SBC3
>>> r24) Thin Provisioning VPD page and the TPWS bit.
>>>
>>> Signed-off-by: Martin K. Petersen<martin.petersen@oracle.com>
>>> ---
>>> drivers/ata/libata-scsi.c | 13 +++++++++++++
>>> 1 files changed, 13 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
>>> index a54273d..e280ae6 100644
>>> --- a/drivers/ata/libata-scsi.c
>>> +++ b/drivers/ata/libata-scsi.c
>>> @@ -2001,6 +2001,7 @@ static unsigned int ata_scsiop_inq_00(struct
>>> ata_scsi_args *args, u8 *rbuf)
>>> 0x89, /* page 0x89, ata info page */
>>> 0xb0, /* page 0xb0, block limits page */
>>> 0xb1, /* page 0xb1, block device characteristics page */
>>> + 0xb2, /* page 0xb2, thin provisioning page */
>>> };
>>>
>>> rbuf[3] = sizeof(pages); /* number of supported VPD pages */
>>> @@ -2172,6 +2173,15 @@ static unsigned int ata_scsiop_inq_b1(struct
>>> ata_scsi_args *args, u8 *rbuf)
>>> return 0;
>>> }
>>>
>>> +static unsigned int ata_scsiop_inq_b2(struct ata_scsi_args *args, u8
>>> *rbuf)
>>> +{
>>> + rbuf[1] = 0xb1;
>>> + rbuf[3] = 0x3c;
>>> + rbuf[5] = 1<< 6; /* TPWS */
>>
>> I would love a bit more documentation here.
>
> Yes, that style comes from my code in scsi_debug and
> some of my utilities. Recently I added some more
> documentation, just prior the the function:
>
> /* SCSI Thin Provisioning VPD page: SBC-3 rev 22 or later */
>
> I see no point in commenting the individual code lines.
> Just follow that reference ...
>
> And when I do then I see that rbuf[3] should be 4 (not 0x3c)
> unless the DP bit is set.

... and of course rbuf[1] should be 0xb2 . Martin ????
The rbuf[5] line is correct :-)

Doug Gilbert
--
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
Martin K. Petersen - Aug. 23, 2010, 7:11 p.m.
>>>>> "Doug" == Douglas Gilbert <dgilbert@interlog.com> writes:

>> And when I do then I see that rbuf[3] should be 4 (not 0x3c) unless
>> the DP bit is set.

I totally missed the DP bit when I read the spec.


Doug> ... and of course rbuf[1] should be 0xb2 . Martin ????  The
Doug> rbuf[5] line is correct :-)

Copy and paste error.

I'll get these fixed up right away...

Patch

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index a54273d..e280ae6 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -2001,6 +2001,7 @@  static unsigned int ata_scsiop_inq_00(struct ata_scsi_args *args, u8 *rbuf)
 		0x89,	/* page 0x89, ata info page */
 		0xb0,	/* page 0xb0, block limits page */
 		0xb1,	/* page 0xb1, block device characteristics page */
+		0xb2,	/* page 0xb2, thin provisioning page */
 	};
 
 	rbuf[3] = sizeof(pages);	/* number of supported VPD pages */
@@ -2172,6 +2173,15 @@  static unsigned int ata_scsiop_inq_b1(struct ata_scsi_args *args, u8 *rbuf)
 	return 0;
 }
 
+static unsigned int ata_scsiop_inq_b2(struct ata_scsi_args *args, u8 *rbuf)
+{
+	rbuf[1] = 0xb1;
+	rbuf[3] = 0x3c;
+	rbuf[5] = 1 << 6;	/* TPWS */
+
+	return 0;
+}
+
 /**
  *	ata_scsiop_noop - Command handler that simply returns success.
  *	@args: device IDENTIFY data / SCSI command of interest.
@@ -3250,6 +3260,9 @@  void ata_scsi_simulate(struct ata_device *dev, struct scsi_cmnd *cmd,
 		case 0xb1:
 			ata_scsi_rbuf_fill(&args, ata_scsiop_inq_b1);
 			break;
+		case 0xb2:
+			ata_scsi_rbuf_fill(&args, ata_scsiop_inq_b2);
+			break;
 		default:
 			ata_scsi_invalid_field(cmd, done);
 			break;