Re: [patch] powerpc/ps3: Use hard coded values for LV1 device type

Re: [patch] powerpc/ps3: Use hard coded values for LV1 device type

[James Bottomley]

On Mon, 2009-02-09 at 11:11 +1100, Benjamin Herrenschmidt wrote:
> On Sun, 2009-02-08 at 22:29 +1100, Michael Ellerman wrote:
> > On Fri, 2009-02-06 at 18:42 -0800, Geoff Levand wrote:
> > > Change the PS3 platform code to use hard coded numbers for its
> > > LV1 device types.
> > > 
> > > The PS3 platform code was incorrectly using some scsi block
> > > constants for the device type returned from the LV1 hypervisor.
> > > 
> > > Fixes build errors like these when CONFIG_BLOCK=n:
> > > 
> > >   In file included from include/scsi/scsi.h:12,
> > >                    from arch/powerpc/platforms/ps3/platform.h:25,
> > >                    from arch/powerpc/platforms/ps3/setup.c:36:
> > >   include/scsi/scsi_cmnd.h:27:25: warning: "BLK_MAX_CDB" is not defined
> > >   include/scsi/scsi_cmnd.h:28:3: error: #error MAX_COMMAND_SIZE can not be bigger than BLK_MAX_CDB
> 
> Adding Jens and James on CC since I think a proper fix lies in blkdev.h
> or scsi*.h

And cc'd linux-scsi

> So basically, the whole of blkdev.h is inside a big ifdef
> CONFIG_BLOCK... which means that scsi_cmnd.h can't build which in turn
> makes scsi.h fail.

Well, look at it from our point of view; it's impossible to build SCSI
without block, so a little interdependence is easy to get.

> The PS3 platform code wants to use some of the standard SCSI types from
> there though, as they are part of the hypervisor ABI. (And in fact it
> can be argued that non-block devices using SCSI do exist, such as
> scanners, no ?)
> 
> Any reason other than pre-historical to have blkdev.h shielded like
> that ?

Actually, I think the fix lies in scsi.h ... we can make that into a
nicely independent protocol header file.  Your current woes come because
it pulls in scsi_cmnd.h ... perhaps just getting rid of this will fix
it.

Can the rest of linux-scsi verify that the fix below doesn't break
something else?

I found one cockup: block/cmd-filter.c is apparently not including
linuc/blkdev.h directly but via scsi/scsi.h ... I fixed this up.

> Cheers,
> Ben.
> 
> > > Signed-off-by: Geoff Levand <geoffrey.levand@am.sony.com>
> > > ---
> > > Ben,
> > > 
> > > Please send upstream for 2.6.29.
> > > 
> > > -Geoff
> > > 
> > >  arch/powerpc/platforms/ps3/platform.h |    8 +++-----
> > >  1 file changed, 3 insertions(+), 5 deletions(-)
> > > 
> > > --- a/arch/powerpc/platforms/ps3/platform.h
> > > +++ b/arch/powerpc/platforms/ps3/platform.h
> > > @@ -22,8 +22,6 @@
> > >  #define _PS3_PLATFORM_H
> > >  
> > >  #include <linux/rtc.h>
> > > -#include <scsi/scsi.h>
> > > -
> > >  #include <asm/ps3.h>
> > >  
> > >  /* htab */
> > > @@ -83,12 +81,12 @@ enum ps3_bus_type {
> > >  };
> > >  
> > >  enum ps3_dev_type {
> > > -	PS3_DEV_TYPE_STOR_DISK = TYPE_DISK,	/* 0 */
> > > +	PS3_DEV_TYPE_STOR_DISK = 0, /* TYPE_DISK */
> > >  	PS3_DEV_TYPE_SB_GELIC = 3,
> > >  	PS3_DEV_TYPE_SB_USB = 4,
> > > -	PS3_DEV_TYPE_STOR_ROM = TYPE_ROM,	/* 5 */
> > > +	PS3_DEV_TYPE_STOR_ROM = 5, /* TYPE_ROM */
> > >  	PS3_DEV_TYPE_SB_GPIO = 6,
> > > -	PS3_DEV_TYPE_STOR_FLASH = TYPE_RBC,	/* 14 */
> > > +	PS3_DEV_TYPE_STOR_FLASH = 14, /* TYPE_RBC */
> > 
> > This looks like you're just papering over the bug, by hardcoding the
> > same values that are in the scsi header. Or are they really independent,
> > in which case I'd say the comments are confusing.
> > 
> > cheers

James

---

diff --git a/block/cmd-filter.c b/block/cmd-filter.c
index 504b275..572bbc2 100644
--- a/block/cmd-filter.c
+++ b/block/cmd-filter.c
@@ -22,6 +22,7 @@
 #include <linux/spinlock.h>
 #include <linux/capability.h>
 #include <linux/bitops.h>
+#include <linux/blkdev.h>
 
 #include <scsi/scsi.h>
 #include <linux/cdrom.h>
diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h
index 80d7f60..084478e 100644
--- a/include/scsi/scsi.h
+++ b/include/scsi/scsi.h
@@ -9,7 +9,8 @@
 #define _SCSI_SCSI_H
 
 #include <linux/types.h>
-#include <scsi/scsi_cmnd.h>
+
+struct scsi_cmnd;
 
 /*
  * The maximum number of SG segments that we will put inside a
@@ -439,22 +440,6 @@ static inline int scsi_is_wlun(unsigned int lun)
 #define host_byte(result)   (((result) >> 16) & 0xff)
 #define driver_byte(result) (((result) >> 24) & 0xff)
 
-static inline void set_msg_byte(struct scsi_cmnd *cmd, char status)
-{
-	cmd->result |= status << 8;
-}
-
-static inline void set_host_byte(struct scsi_cmnd *cmd, char status)
-{
-	cmd->result |= status << 16;
-}
-
-static inline void set_driver_byte(struct scsi_cmnd *cmd, char status)
-{
-	cmd->result |= status << 24;
-}
-
-
 #define sense_class(sense)  (((sense) >> 4) & 0x7)
 #define sense_error(sense)  ((sense) & 0xf)
 #define sense_valid(sense)  ((sense) & 0x80);
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index 855bf95..43b50d3 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -291,4 +291,19 @@ static inline struct scsi_data_buffer *scsi_prot(struct scsi_cmnd *cmd)
 #define scsi_for_each_prot_sg(cmd, sg, nseg, __i)		\
 	for_each_sg(scsi_prot_sglist(cmd), sg, nseg, __i)
 
+static inline void set_msg_byte(struct scsi_cmnd *cmd, char status)
+{
+	cmd->result |= status << 8;
+}
+
+static inline void set_host_byte(struct scsi_cmnd *cmd, char status)
+{
+	cmd->result |= status << 16;
+}
+
+static inline void set_driver_byte(struct scsi_cmnd *cmd, char status)
+{
+	cmd->result |= status << 24;
+}
+
 #endif /* _SCSI_SCSI_CMND_H */

Re: [patch] powerpc/ps3: Use hard coded values for LV1 device type

[Geoff Levand]

James Bottomley wrote:
> On Mon, 2009-02-09 at 11:11 +1100, Benjamin Herrenschmidt wrote:
>> The PS3 platform code wants to use some of the standard SCSI types from
>> there though, as they are part of the hypervisor ABI. (And in fact it
>> can be argued that non-block devices using SCSI do exist, such as
>> scanners, no ?)
>> 
>> Any reason other than pre-historical to have blkdev.h shielded like
>> that ?
> 
> Actually, I think the fix lies in scsi.h ... we can make that into a
> nicely independent protocol header file.  Your current woes come because
> it pulls in scsi_cmnd.h ... perhaps just getting rid of this will fix
> it.
> 
> Can the rest of linux-scsi verify that the fix below doesn't break
> something else?
> 
> I found one cockup: block/cmd-filter.c is apparently not including
> linuc/blkdev.h directly but via scsi/scsi.h ... I fixed this up.
...
> diff --git a/block/cmd-filter.c b/block/cmd-filter.c
> diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h

I tested this scsi header fix on PS3 and it fixes the BLK_MAX_CDB
not defined build error when CONFIG_BLOCK=n.

Acked-by: Geoff Levand <geoffrey.levand@am.sony.com>

Re: [patch] powerpc/ps3: Use hard coded values for LV1 device type

[Sachin P. Sant]

James Bottomley wrote:
> Actually, I think the fix lies in scsi.h ... we can make that into a
> nicely independent protocol header file.  Your current woes come because
> it pulls in scsi_cmnd.h ... perhaps just getting rid of this will fix
> it.
>
> Can the rest of linux-scsi verify that the fix below doesn't break
> something else?
>
> I found one cockup: block/cmd-filter.c is apparently not including
> linuc/blkdev.h directly but via scsi/scsi.h ... I fixed this up.
>   
Tested the patch and it fixes the issue.

Thanks
-Sachin

-- 

---------------------------------
Sachin Sant
IBM Linux Technology Center
India Systems and Technology Labs
Bangalore, India
---------------------------------