[patch 13/17] scsi: remove private implementation of get_unaligned_be32

[patch 13/17] scsi: remove private implementation of get_unaligned_be32

[akpm]

From: Harvey Harrison <harvey.harrison@gmail.com>

Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 drivers/scsi/libsas/sas_expander.c |    9 +++++----
 include/scsi/scsi.h                |    6 ------
 2 files changed, 5 insertions(+), 10 deletions(-)

diff -puN drivers/scsi/libsas/sas_expander.c~scsi-remove-private-implementation-of-get_unaligned_be32 drivers/scsi/libsas/sas_expander.c
--- a/drivers/scsi/libsas/sas_expander.c~scsi-remove-private-implementation-of-get_unaligned_be32
+++ a/drivers/scsi/libsas/sas_expander.c
@@ -24,6 +24,7 @@
 
 #include <linux/scatterlist.h>
 #include <linux/blkdev.h>
+#include <asm/unaligned.h>
 
 #include "sas_internal.h"
 
@@ -541,10 +542,10 @@ int sas_smp_get_phy_events(struct sas_ph
 	if (!res)
 		goto out;
 
-	phy->invalid_dword_count = scsi_to_u32(&resp[12]);
-	phy->running_disparity_error_count = scsi_to_u32(&resp[16]);
-	phy->loss_of_dword_sync_count = scsi_to_u32(&resp[20]);
-	phy->phy_reset_problem_count = scsi_to_u32(&resp[24]);
+	phy->invalid_dword_count = get_unaligned_be32(&resp[12]);
+	phy->running_disparity_error_count = get_unaligned_be32(&resp[16]);
+	phy->loss_of_dword_sync_count = get_unaligned_be32(&resp[20]);
+	phy->phy_reset_problem_count = get_unaligned_be32(&resp[24]);
 
  out:
 	kfree(resp);
diff -puN include/scsi/scsi.h~scsi-remove-private-implementation-of-get_unaligned_be32 include/scsi/scsi.h
--- a/include/scsi/scsi.h~scsi-remove-private-implementation-of-get_unaligned_be32
+++ a/include/scsi/scsi.h
@@ -527,10 +527,4 @@ static inline void set_driver_byte(struc
 /* Used to obtain the PCI location of a device */
 #define SCSI_IOCTL_GET_PCI		0x5387
 
-/* Pull a u32 out of a SCSI message (using BE SCSI conventions) */
-static inline __u32 scsi_to_u32(__u8 *ptr)
-{
-	return (ptr[0]<<24) + (ptr[1]<<16) + (ptr[2]<<8) + ptr[3];
-}
-
 #endif /* _SCSI_SCSI_H */
_

Re: [patch 13/17] scsi: remove private implementation of get_unaligned_be32

[James Bottomley]

On Wed, 2008-10-29 at 14:24 -0700, akpm@linux-foundation.org wrote:
> From: Harvey Harrison <harvey.harrison@gmail.com>
> 
> Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
> Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
> 
>  drivers/scsi/libsas/sas_expander.c |    9 +++++----
>  include/scsi/scsi.h                |    6 ------
>  2 files changed, 5 insertions(+), 10 deletions(-)
> 
> diff -puN drivers/scsi/libsas/sas_expander.c~scsi-remove-private-implementation-of-get_unaligned_be32 drivers/scsi/libsas/sas_expander.c
> --- a/drivers/scsi/libsas/sas_expander.c~scsi-remove-private-implementation-of-get_unaligned_be32
> +++ a/drivers/scsi/libsas/sas_expander.c
> @@ -24,6 +24,7 @@
>  
>  #include <linux/scatterlist.h>
>  #include <linux/blkdev.h>
> +#include <asm/unaligned.h>
>  
>  #include "sas_internal.h"
>  
> @@ -541,10 +542,10 @@ int sas_smp_get_phy_events(struct sas_ph
>  	if (!res)
>  		goto out;
>  
> -	phy->invalid_dword_count = scsi_to_u32(&resp[12]);
> -	phy->running_disparity_error_count = scsi_to_u32(&resp[16]);
> -	phy->loss_of_dword_sync_count = scsi_to_u32(&resp[20]);
> -	phy->phy_reset_problem_count = scsi_to_u32(&resp[24]);
> +	phy->invalid_dword_count = get_unaligned_be32(&resp[12]);
> +	phy->running_disparity_error_count = get_unaligned_be32(&resp[16]);
> +	phy->loss_of_dword_sync_count = get_unaligned_be32(&resp[20]);
> +	phy->phy_reset_problem_count = get_unaligned_be32(&resp[24]);
>  
>   out:
>  	kfree(resp);
> diff -puN include/scsi/scsi.h~scsi-remove-private-implementation-of-get_unaligned_be32 include/scsi/scsi.h
> --- a/include/scsi/scsi.h~scsi-remove-private-implementation-of-get_unaligned_be32
> +++ a/include/scsi/scsi.h
> @@ -527,10 +527,4 @@ static inline void set_driver_byte(struc
>  /* Used to obtain the PCI location of a device */
>  #define SCSI_IOCTL_GET_PCI		0x5387
>  
> -/* Pull a u32 out of a SCSI message (using BE SCSI conventions) */
> -static inline __u32 scsi_to_u32(__u8 *ptr)
> -{
> -	return (ptr[0]<<24) + (ptr[1]<<16) + (ptr[2]<<8) + ptr[3];
> -}
> -
>  #endif /* _SCSI_SCSI_H */

No ... as I've said several times now, there's a debate going on about
what we're supposed to be doing with all of this (either putting it in
SCSI, pulling it out or just using the inline notation.  I'm not putting
a patch like this in until we at least get some consensus.

James

Re: [patch 13/17] scsi: remove private implementation of get_unaligned_be32

[Andrew Morton]

On Wed, 29 Oct 2008 16:48:14 -0500
James Bottomley <James.Bottomley@HansenPartnership.com> wrote:

> On Wed, 2008-10-29 at 14:24 -0700, akpm@linux-foundation.org wrote:
> > From: Harvey Harrison <harvey.harrison@gmail.com>
> > 
> > Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
> > Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
> > Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> > ---
> > 
> >  drivers/scsi/libsas/sas_expander.c |    9 +++++----
> >  include/scsi/scsi.h                |    6 ------
> >  2 files changed, 5 insertions(+), 10 deletions(-)
> > 
> > diff -puN drivers/scsi/libsas/sas_expander.c~scsi-remove-private-implementation-of-get_unaligned_be32 drivers/scsi/libsas/sas_expander.c
> > --- a/drivers/scsi/libsas/sas_expander.c~scsi-remove-private-implementation-of-get_unaligned_be32
> > +++ a/drivers/scsi/libsas/sas_expander.c
> > @@ -24,6 +24,7 @@
> >  
> >  #include <linux/scatterlist.h>
> >  #include <linux/blkdev.h>
> > +#include <asm/unaligned.h>
> >  
> >  #include "sas_internal.h"
> >  
> > @@ -541,10 +542,10 @@ int sas_smp_get_phy_events(struct sas_ph
> >  	if (!res)
> >  		goto out;
> >  
> > -	phy->invalid_dword_count = scsi_to_u32(&resp[12]);
> > -	phy->running_disparity_error_count = scsi_to_u32(&resp[16]);
> > -	phy->loss_of_dword_sync_count = scsi_to_u32(&resp[20]);
> > -	phy->phy_reset_problem_count = scsi_to_u32(&resp[24]);
> > +	phy->invalid_dword_count = get_unaligned_be32(&resp[12]);
> > +	phy->running_disparity_error_count = get_unaligned_be32(&resp[16]);
> > +	phy->loss_of_dword_sync_count = get_unaligned_be32(&resp[20]);
> > +	phy->phy_reset_problem_count = get_unaligned_be32(&resp[24]);
> >  
> >   out:
> >  	kfree(resp);
> > diff -puN include/scsi/scsi.h~scsi-remove-private-implementation-of-get_unaligned_be32 include/scsi/scsi.h
> > --- a/include/scsi/scsi.h~scsi-remove-private-implementation-of-get_unaligned_be32
> > +++ a/include/scsi/scsi.h
> > @@ -527,10 +527,4 @@ static inline void set_driver_byte(struc
> >  /* Used to obtain the PCI location of a device */
> >  #define SCSI_IOCTL_GET_PCI		0x5387
> >  
> > -/* Pull a u32 out of a SCSI message (using BE SCSI conventions) */
> > -static inline __u32 scsi_to_u32(__u8 *ptr)
> > -{
> > -	return (ptr[0]<<24) + (ptr[1]<<16) + (ptr[2]<<8) + ptr[3];
> > -}
> > -
> >  #endif /* _SCSI_SCSI_H */
> 
> No ... as I've said several times now, there's a debate going on about
> what we're supposed to be doing with all of this (either putting it in
> SCSI, pulling it out or just using the inline notation.

If I knew what the terrible word "it" is replacing, I'd know what the
above sentence means.

I'm kinda struggling to imagine why there's controversy, really.  scsi
has a private implementation of something which core kernel provides. 
Zap!

>  I'm not putting
> a patch like this in until we at least get some consensus.

I'll hang onto it, so there's nowhere to hide...

Re: [patch 13/17] scsi: remove private implementation of get_unaligned_be32

[James Bottomley]

On Wed, 2008-10-29 at 15:07 -0700, Andrew Morton wrote:
> On Wed, 29 Oct 2008 16:48:14 -0500
> James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> 
> > On Wed, 2008-10-29 at 14:24 -0700, akpm@linux-foundation.org wrote:
> > > From: Harvey Harrison <harvey.harrison@gmail.com>
> > > 
> > > Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
> > > Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
> > > Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> > > ---
> > > 
> > >  drivers/scsi/libsas/sas_expander.c |    9 +++++----
> > >  include/scsi/scsi.h                |    6 ------
> > >  2 files changed, 5 insertions(+), 10 deletions(-)
> > > 
> > > diff -puN drivers/scsi/libsas/sas_expander.c~scsi-remove-private-implementation-of-get_unaligned_be32 drivers/scsi/libsas/sas_expander.c
> > > --- a/drivers/scsi/libsas/sas_expander.c~scsi-remove-private-implementation-of-get_unaligned_be32
> > > +++ a/drivers/scsi/libsas/sas_expander.c
> > > @@ -24,6 +24,7 @@
> > >  
> > >  #include <linux/scatterlist.h>
> > >  #include <linux/blkdev.h>
> > > +#include <asm/unaligned.h>
> > >  
> > >  #include "sas_internal.h"
> > >  
> > > @@ -541,10 +542,10 @@ int sas_smp_get_phy_events(struct sas_ph
> > >  	if (!res)
> > >  		goto out;
> > >  
> > > -	phy->invalid_dword_count = scsi_to_u32(&resp[12]);
> > > -	phy->running_disparity_error_count = scsi_to_u32(&resp[16]);
> > > -	phy->loss_of_dword_sync_count = scsi_to_u32(&resp[20]);
> > > -	phy->phy_reset_problem_count = scsi_to_u32(&resp[24]);
> > > +	phy->invalid_dword_count = get_unaligned_be32(&resp[12]);
> > > +	phy->running_disparity_error_count = get_unaligned_be32(&resp[16]);
> > > +	phy->loss_of_dword_sync_count = get_unaligned_be32(&resp[20]);
> > > +	phy->phy_reset_problem_count = get_unaligned_be32(&resp[24]);
> > >  
> > >   out:
> > >  	kfree(resp);
> > > diff -puN include/scsi/scsi.h~scsi-remove-private-implementation-of-get_unaligned_be32 include/scsi/scsi.h
> > > --- a/include/scsi/scsi.h~scsi-remove-private-implementation-of-get_unaligned_be32
> > > +++ a/include/scsi/scsi.h
> > > @@ -527,10 +527,4 @@ static inline void set_driver_byte(struc
> > >  /* Used to obtain the PCI location of a device */
> > >  #define SCSI_IOCTL_GET_PCI		0x5387
> > >  
> > > -/* Pull a u32 out of a SCSI message (using BE SCSI conventions) */
> > > -static inline __u32 scsi_to_u32(__u8 *ptr)
> > > -{
> > > -	return (ptr[0]<<24) + (ptr[1]<<16) + (ptr[2]<<8) + ptr[3];
> > > -}
> > > -
> > >  #endif /* _SCSI_SCSI_H */
> > 
> > No ... as I've said several times now, there's a debate going on about
> > what we're supposed to be doing with all of this (either putting it in
> > SCSI, pulling it out or just using the inline notation.
> 
> If I knew what the terrible word "it" is replacing, I'd know what the
> above sentence means.

It being the correct way to handle multibyte SCSI problems.

> I'm kinda struggling to imagine why there's controversy, really.  scsi
> has a private implementation of something which core kernel provides. 
> Zap!

The main problems with the above are that a lot of people don't
necessarily think Big Endian when they see SCSI (even though it's a BE
bus), so the get_unaligned_beXX looks strange, and secondly most of the
arrays we operate on are simple u8 ones, so sparse gets annoyed about
sending a non BE quantity through a BE conversion ... I bet it even does
this for the above patch.

> >  I'm not putting
> > a patch like this in until we at least get some consensus.
> 
> I'll hang onto it, so there's nowhere to hide...

OK.

James

Re: [patch 13/17] scsi: remove private implementation of get_unaligned_be32

[Harvey Harrison]

On Wed, 2008-10-29 at 17:13 -0500, James Bottomley wrote:
> > > No ... as I've said several times now, there's a debate going on about
> > > what we're supposed to be doing with all of this (either putting it in
> > > SCSI, pulling it out or just using the inline notation.
> > 
> > If I knew what the terrible word "it" is replacing, I'd know what the
> > above sentence means.
> 
> It being the correct way to handle multibyte SCSI problems.
> 
> > I'm kinda struggling to imagine why there's controversy, really.  scsi
> > has a private implementation of something which core kernel provides. 
> > Zap!
> 
> The main problems with the above are that a lot of people don't
> necessarily think Big Endian when they see SCSI (even though it's a BE
> bus), so the get_unaligned_beXX looks strange, and secondly most of the
> arrays we operate on are simple u8 ones, so sparse gets annoyed about
> sending a non BE quantity through a BE conversion ... I bet it even does
> this for the above patch.

It does not, the get_unaligned_* family is not typesafe (takes a void *)
as lots of people have u8 arrays or similar.  Even though it probably
should be.

> 
> > >  I'm not putting
> > > a patch like this in until we at least get some consensus.
> > 
> > I'll hang onto it, so there's nowhere to hide...
> 
> OK.
> 

Did you ever have some time to think about my suggestion regarding
defining some common packed structs for the different command formats
taht could be endian-annotated, then use the unaligned helpers against
pointers to these structs...so sparse would actually spot endian
mixups in scsi thereafter?

Harvey

Re: [patch 13/17] scsi: remove private implementation of get_unaligned_be32

[Harvey Harrison]

On Wed, 2008-10-29 at 15:29 -0700, Harvey Harrison wrote:
> Did you ever have some time to think about my suggestion regarding
> defining some common packed structs for the different command formats
> taht could be endian-annotated, then use the unaligned helpers against
> pointers to these structs...so sparse would actually spot endian
> mixups in scsi thereafter?

Here's a small patch to illustrate the kind of thing I'm suggesting in
a less hand-wavy way.

diff --git a/drivers/scsi/megaraid/megaraid_sas.c b/drivers/scsi/megaraid/megaraid_sas.c
index afe1de9..451f097 100644
--- a/drivers/scsi/megaraid/megaraid_sas.c
+++ b/drivers/scsi/megaraid/megaraid_sas.c
@@ -40,6 +40,7 @@
 #include <linux/compat.h>
 #include <linux/blkdev.h>
 #include <linux/mutex.h>
+#include <asm/unaligned.h>
 
 #include <scsi/scsi.h>
 #include <scsi/scsi_cmnd.h>
@@ -753,57 +754,41 @@ megasas_build_ldio(struct megasas_instance *instance, struct scsi_cmnd *scp,
 	ldio->start_lba_hi = 0;
 	ldio->access_byte = (scp->cmd_len != 6) ? scp->cmnd[1] : 0;
 
-	/*
-	 * 6-byte READ(0x08) or WRITE(0x0A) cdb
-	 */
 	if (scp->cmd_len == 6) {
+		/*
+		 * 6-byte READ(0x08) or WRITE(0x0A) cdb
+		 */
 		ldio->lba_count = (u32) scp->cmnd[4];
 		ldio->start_lba_lo = ((u32) scp->cmnd[1] << 16) |
 		    ((u32) scp->cmnd[2] << 8) | (u32) scp->cmnd[3];
 
 		ldio->start_lba_lo &= 0x1FFFFF;
-	}
-
-	/*
-	 * 10-byte READ(0x28) or WRITE(0x2A) cdb
-	 */
-	else if (scp->cmd_len == 10) {
-		ldio->lba_count = (u32) scp->cmnd[8] |
-		    ((u32) scp->cmnd[7] << 8);
-		ldio->start_lba_lo = ((u32) scp->cmnd[2] << 24) |
-		    ((u32) scp->cmnd[3] << 16) |
-		    ((u32) scp->cmnd[4] << 8) | (u32) scp->cmnd[5];
-	}
-
-	/*
-	 * 12-byte READ(0xA8) or WRITE(0xAA) cdb
-	 */
-	else if (scp->cmd_len == 12) {
-		ldio->lba_count = ((u32) scp->cmnd[6] << 24) |
-		    ((u32) scp->cmnd[7] << 16) |
-		    ((u32) scp->cmnd[8] << 8) | (u32) scp->cmnd[9];
-
-		ldio->start_lba_lo = ((u32) scp->cmnd[2] << 24) |
-		    ((u32) scp->cmnd[3] << 16) |
-		    ((u32) scp->cmnd[4] << 8) | (u32) scp->cmnd[5];
-	}
-
-	/*
-	 * 16-byte READ(0x88) or WRITE(0x8A) cdb
-	 */
-	else if (scp->cmd_len == 16) {
-		ldio->lba_count = ((u32) scp->cmnd[10] << 24) |
-		    ((u32) scp->cmnd[11] << 16) |
-		    ((u32) scp->cmnd[12] << 8) | (u32) scp->cmnd[13];
+	} else if (scp->cmd_len == 10) {
+		/*
+		 * 10-byte READ(0x28) or WRITE(0x2A) cdb
+		 */
+		struct scsi_read10 *cmd = (struct scsi_read10 *)scp->cmnd;
 
-		ldio->start_lba_lo = ((u32) scp->cmnd[6] << 24) |
-		    ((u32) scp->cmnd[7] << 16) |
-		    ((u32) scp->cmnd[8] << 8) | (u32) scp->cmnd[9];
+		ldio->lba_count = get_unaligned_be16(&cmd->length);
+		ldio->start_lba_lo = get_unaligned_be32(&cmd->lba);
+	} else if (scp->cmd_len == 12) {
+		/*
+		 * 12-byte READ(0xA8) or WRITE(0xAA) cdb
+		 */
+		struct scsi_read12 *cmd = (struct scsi_read12 *)scp->cmnd;
 
-		ldio->start_lba_hi = ((u32) scp->cmnd[2] << 24) |
-		    ((u32) scp->cmnd[3] << 16) |
-		    ((u32) scp->cmnd[4] << 8) | (u32) scp->cmnd[5];
+		ldio->lba_count = get_unaligned_be32(&cmd->length);
+		ldio->start_lba_lo = get_unaligned_be32(&cmd->lba);
+	} else if (scp->cmd_len == 16) {
+		/*
+		 * 16-byte READ(0x88) or WRITE(0x8A) cdb
+		 */
+		struct scsi_read16 *cmd = (struct scsi_read16 *)scp->cmnd;
+		u64 lba = get_unaligned_be64(&cmd->lba);
 
+		ldio->lba_count = get_unaligned_be32(&cmd->length);
+		ldio->start_lba_lo = (u32)lba;
+		ldio->start_lba_hi = (u32)(lba >> 32);
 	}
 
 	/*
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index 855bf95..184d83d 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -131,6 +131,42 @@ struct scsi_cmnd {
 	unsigned char tag;	/* SCSI-II queued command tag */
 };
 
+/*
+ * Opcode 0x28
+ */
+struct scsi_read10 {
+	u8 op;
+	u8 flags;
+	__be32 lba;
+	u8 pad; /* reserved */
+	__be16 length;
+	u8 control;
+} __packed;
+
+/*
+ * Opcode 0xa8
+ */
+struct scsi_read12 {
+	u8 op;
+	u8 flags;
+	__be32 lba;
+	__be32 length;
+	u8 pad; /* reserved */
+	u8 control;
+} __packed;
+
+/*
+ * Opcode 0x88
+ */
+struct scsi_read16 {
+	u8 op;
+	u8 flags;
+	__be64 lba;
+	__be32 length;
+	u8 pad; /* MMC4, reserved, group number */
+	u8 control;
+} __packed;
+
 extern struct scsi_cmnd *scsi_get_command(struct scsi_device *, gfp_t);
 extern struct scsi_cmnd *__scsi_get_command(struct Scsi_Host *, gfp_t);
 extern void scsi_put_command(struct scsi_cmnd *);

Re: [patch 13/17] scsi: remove private implementation of get_unaligned_be32

[James Bottomley]

On Wed, 2008-10-29 at 18:03 -0700, Harvey Harrison wrote: 
> +/*
> + * Opcode 0x28
> + */
> +struct scsi_read10 {
> +	u8 op;
> +	u8 flags;
> +	__be32 lba;
> +	u8 pad; /* reserved */
> +	__be16 length;
> +	u8 control;
> +} __packed;
> +
> +/*
> + * Opcode 0xa8
> + */
> +struct scsi_read12 {
> +	u8 op;
> +	u8 flags;
> +	__be32 lba;
> +	__be32 length;
> +	u8 pad; /* reserved */
> +	u8 control;
> +} __packed;
> +
> +/*
> + * Opcode 0x88
> + */
> +struct scsi_read16 {
> +	u8 op;
> +	u8 flags;
> +	__be64 lba;
> +	__be32 length;
> +	u8 pad; /* MMC4, reserved, group number */
> +	u8 control;
> +} __packed;

OK, so this is wrong: the commands, unfortunately, have several meanings
for the bits depending on the version of the spec.  Also, it's not
really the way the mid-layer thinks about commands.  We tend to use the
generic groupings there.  Finally, this looks pretty ugly and it would
get really horrible if we had to do unions for the bits as they changed
meanings through the standards.

So all in all, to try to solve a problem which doesn't really exist, I
don't really think its a good idea.  There's no evidence of any
confusion caused by the SCSI bus being BE in any of the drivers.

James