Re: [PATCH 4/4] firewire: sbp2: remove overzealous alignment constraints

Re: [PATCH 4/4] firewire: sbp2: remove overzealous alignment constraints

[Stefan Richter]

On May 18 Clemens Ladisch wrote at linux1394-devel:
> The SBP-2/3 specifications do not require any alignment of data
> buffers; only their own data structures need to be quadlet-aligned.

SBP-2 clause 3.1.2.24 requires that system memory accepts quadlet r/w
access.  Memory which is not aligned at quadlet boundaries is not
accessible by quadlet accesses per IEEE 1394 clause 6.2.5.2.2.

> This patch is perfectly safe because there is no actual change:
> the SCSI framework uses a default block queue DMA alignment of
> 32 bits anyway.

This code was added after recommendation to set it explicitly in the
driver:
http://marc.info/?l=linux-scsi&m=120137366708017
http://thread.gmane.org/gmane.linux.kernel.firewire.devel/11424

It is probably not going to happen that somebody decreases the SCSI
default.  But perhaps we should still keep this explicit...?

> Signed-off-by: Clemens Ladisch <clemens@ladisch.de>
> ---
>  drivers/firewire/sbp2.c |    8 ++------
>  1 files changed, 2 insertions(+), 6 deletions(-)
> 
> --- a/drivers/firewire/sbp2.c
> +++ b/drivers/firewire/sbp2.c
> @@ -207,9 +207,8 @@ static const struct device *lu_dev(const struct sbp2_logical_unit *lu)
>  #define SBP2_MAX_CDB_SIZE		16
> 
>  /*
> - * The default maximum s/g segment size of a FireWire controller is
> - * usually 0x10000, but SBP-2 only allows 0xffff. Since buffers have to
> - * be quadlet-aligned, we set the length limit to 0xffff & ~3.
> + * The maximum SBP-2 data buffer size is 0xffff.  We quadlet-align this
> + * for compatibility with earlier versions of this driver.
>   */
>  #define SBP2_MAX_SEG_SIZE		0xfffc
> 
> @@ -1530,9 +1529,6 @@ static int sbp2_scsi_slave_alloc(struct scsi_device *sdev)
> 
>  	sdev->allow_restart = 1;
> 
> -	/* SBP-2 requires quadlet alignment of the data buffers. */
> -	blk_queue_update_dma_alignment(sdev->request_queue, 4 - 1);
> -
>  	if (lu->tgt->workarounds & SBP2_WORKAROUND_INQUIRY_36)
>  		sdev->inquiry_len = 36;
> 

-- 
Stefan Richter
-=====-===-- -=-= =--=-
http://arcgraph.de/sr/

------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and 
threat landscape has changed and how IT managers can respond. Discussions 
will include endpoint security, mobile security and the latest in malware 
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/

Re: [PATCH 4/4] firewire: sbp2: remove overzealous alignment constraints

[Clemens Ladisch]

Stefan Richter wrote:
> On May 18 Clemens Ladisch wrote at linux1394-devel:
>> The SBP-2/3 specifications do not require any alignment of data
>> buffers; only their own data structures need to be quadlet-aligned.
>
> SBP-2 clause 3.1.2.24 requires that system memory accepts quadlet r/w
> access.

Most of these definitions are just copied from IEC 13213; system
memory is allowed to accept block R/W requests, and SBP in fact
requires them.

> Memory which is not aligned at quadlet boundaries is not
> accessible by quadlet accesses per IEEE 1394 clause 6.2.5.2.2.

SBP-2 explicitly mentions unaligned accesses in 3.2.2, and 9.2 says:
| When page_table_present is zero, a page_size value of zero indicates
| that there are no alignment requirements.

>> This patch is perfectly safe because there is no actual change:
>> the SCSI framework uses a default block queue DMA alignment of
>> 32 bits anyway.
>
> This code was added after recommendation to set it explicitly in the
> driver:
> http://marc.info/?l=linux-scsi&m=120137366708017
> http://thread.gmane.org/gmane.linux.kernel.firewire.devel/11424

That recommendation was based on the assumption that quadlet alignment
would be actually needed.

> It is probably not going to happen that somebody decreases the SCSI
> default.  But perhaps we should still keep this explicit...?

Keeping this would be nothing more than paranoia.  But this a perfectly
fine sentiment for a maintainer dealing with real-world firmware, so
you might use the patch below instead.  (I'm not budging from my
interpretation of SBP-2.  :-)

--8<---------------------------------------------------------------->8--
firewire: sbp2: document the absence of alignment requirements

The SBP-2/3 specifications do not require any alignment of data
buffers; only their own data structures need to be quadlet-aligned.

Fix the comments to reflect this, but leave the actual alignment at
32 bits to avoid theoretical problems with target implementations
that might handle this incorrectly.

Signed-off-by: Clemens Ladisch <clemens@ladisch.de>
---
 drivers/firewire/sbp2.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

--- a/drivers/firewire/sbp2.c
+++ b/drivers/firewire/sbp2.c
@@ -207,9 +207,8 @@ static const struct device *lu_dev(const struct sbp2_logical_unit *lu)
 #define SBP2_MAX_CDB_SIZE		16

 /*
- * The default maximum s/g segment size of a FireWire controller is
- * usually 0x10000, but SBP-2 only allows 0xffff. Since buffers have to
- * be quadlet-aligned, we set the length limit to 0xffff & ~3.
+ * The maximum SBP-2 data buffer size is 0xffff.  We quadlet-align this
+ * for compatibility with earlier versions of this driver.
  */
 #define SBP2_MAX_SEG_SIZE		0xfffc

@@ -1530,7 +1529,10 @@ static int sbp2_scsi_slave_alloc(struct scsi_device *sdev)

 	sdev->allow_restart = 1;

-	/* SBP-2 requires quadlet alignment of the data buffers. */
+	/*
+	 * SBP-2 does not require any alignment, but we set it anyway
+	 * for compatibility with earlier versions of this driver.
+	 */
 	blk_queue_update_dma_alignment(sdev->request_queue, 4 - 1);

 	if (lu->tgt->workarounds & SBP2_WORKAROUND_INQUIRY_36)

Re: [PATCH 4/4] firewire: sbp2: remove overzealous alignment constraints

[Stefan Richter]

On May 18 Clemens Ladisch wrote:
> firewire: sbp2: document the absence of alignment requirements

This and patches 1/3...3/4 have been pushed out to linux1394.git.
-- 
Stefan Richter
-=====-===-- -=-= =-=-=
http://arcgraph.de/sr/