Re: [SCSI] aic94xx: new driver

Re: [SCSI] aic94xx: new driver

[David Woodhouse]

On Sun, 2006-09-24 at 04:00 +0000, Linux Kernel Mailing List wrote:
> --- a/include/scsi/scsi.h
> +++ b/include/scsi/scsi.h
> @@ -429,4 +429,10 @@ #define SCSI_IOCTL_GET_BUS_NUMBER  0x5386
>  /* 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 */ 

Please explain why it's necessary to export this to userspace.

The files in /usr/include/scsi are actually shipped by glibc, and most
distributions use glibc's version instead of the one from the kernel --
so this additional userspace interface is automatically incompatible
with most people's installations.

It would perhaps make sense to stop glibc providing these files, and let
distributions use the version from the kernel -- but that's a separate
issue. And still doesn't seem to justify the addition of the above
function.

-- 
dwmw2

Re: [SCSI] aic94xx: new driver

[James Bottomley]

On Fri, 2007-08-10 at 23:09 +0800, David Woodhouse wrote:
> On Sun, 2006-09-24 at 04:00 +0000, Linux Kernel Mailing List wrote:
> > --- a/include/scsi/scsi.h
> > +++ b/include/scsi/scsi.h
> > @@ -429,4 +429,10 @@ #define SCSI_IOCTL_GET_BUS_NUMBER  0x5386
> >  /* 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 */ 
> 
> Please explain why it's necessary to export this to userspace.

Er it's not ... but then it's not necessary to export this entire file,
either.

> The files in /usr/include/scsi are actually shipped by glibc, and most
> distributions use glibc's version instead of the one from the kernel --
> so this additional userspace interface is automatically incompatible
> with most people's installations.
> 
> It would perhaps make sense to stop glibc providing these files, and let
> distributions use the version from the kernel -- but that's a separate
> issue. And still doesn't seem to justify the addition of the above
> function.

>From the SCSI point of view, the function definitely belongs in that
file because it's an accessor to facilitate the processing of commands
and their replies, which is what that file contains ... in fact it
contains a lot of the internal mechanics of the SCSI layer that the user
shouldn't necessarily be seeing.

James

Re: [SCSI] aic94xx: new driver

[David Woodhouse]

On Fri, 2007-08-10 at 08:52 -0700, James Bottomley wrote:
> On Fri, 2007-08-10 at 23:09 +0800, David Woodhouse wrote:
> > On Sun, 2006-09-24 at 04:00 +0000, Linux Kernel Mailing List wrote:
> > > --- a/include/scsi/scsi.h
> > > +++ b/include/scsi/scsi.h
> > > @@ -429,4 +429,10 @@ #define SCSI_IOCTL_GET_BUS_NUMBER  0x5386
> > >  /* 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 */ 
> > 
> > Please explain why it's necessary to export this to userspace.
> 
> Er it's not ... but then it's not necessary to export this entire file,
> either.

That's an acceptable answer, I suppose, given that glibc ships its own
copy of the file in question.

But it does contain ioctl definitions, and if we ever added any new
ioctls presumably they'd be added there too? It does seem to be the kind
of thing we _do_ want to export from the kernel, surely?


> From the SCSI point of view, the function definitely belongs in that
> file because it's an accessor to facilitate the processing of commands
> and their replies, which is what that file contains ... in fact it
> contains a lot of the internal mechanics of the SCSI layer that the user
> shouldn't necessarily be seeing.

All the more reason for us to export it for ourselves and exert some
editorial control, perhaps?

-- 
dwmw2

Re: [SCSI] aic94xx: new driver

[James Bottomley]

On Sat, 2007-08-11 at 00:06 +0800, David Woodhouse wrote:
> On Fri, 2007-08-10 at 08:52 -0700, James Bottomley wrote:
> > On Fri, 2007-08-10 at 23:09 +0800, David Woodhouse wrote:
> > > On Sun, 2006-09-24 at 04:00 +0000, Linux Kernel Mailing List wrote:
> > > > --- a/include/scsi/scsi.h
> > > > +++ b/include/scsi/scsi.h
> > > > @@ -429,4 +429,10 @@ #define SCSI_IOCTL_GET_BUS_NUMBER  0x5386
> > > >  /* 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 */ 
> > > 
> > > Please explain why it's necessary to export this to userspace.
> > 
> > Er it's not ... but then it's not necessary to export this entire file,
> > either.
> 
> That's an acceptable answer, I suppose, given that glibc ships its own
> copy of the file in question.
> 
> But it does contain ioctl definitions, and if we ever added any new
> ioctls presumably they'd be added there too? It does seem to be the kind
> of thing we _do_ want to export from the kernel, surely?

Oh .. I thought we'd already got around that one by duplicating the
ioctls?  Those five are all deprecated, but if that's what you want
exporting, we could separate them into a new file.

> > From the SCSI point of view, the function definitely belongs in that
> > file because it's an accessor to facilitate the processing of commands
> > and their replies, which is what that file contains ... in fact it
> > contains a lot of the internal mechanics of the SCSI layer that the user
> > shouldn't necessarily be seeing.
> 
> All the more reason for us to export it for ourselves and exert some
> editorial control, perhaps?

Even if I'd known it was exported, I'm still not sure I'd have noticed
why or what the issues are.

James

Re: [SCSI] aic94xx: new driver

[Christoph Hellwig]

On Fri, Aug 10, 2007 at 11:09:22PM +0800, David Woodhouse wrote:
> The files in /usr/include/scsi are actually shipped by glibc, and most
> distributions use glibc's version instead of the one from the kernel --
> so this additional userspace interface is automatically incompatible
> with most people's installations.

Stop here right now.  You just noticed the real bug, and that's exporting
scsi.h at all.  I think Olaf sent a patch to fix this already.

Re: [SCSI] aic94xx: new driver

[David Woodhouse]

On Sat, 2007-08-11 at 04:49 +0100, Christoph Hellwig wrote:
> On Fri, Aug 10, 2007 at 11:09:22PM +0800, David Woodhouse wrote:
> > The files in /usr/include/scsi are actually shipped by glibc, and most
> > distributions use glibc's version instead of the one from the kernel --
> > so this additional userspace interface is automatically incompatible
> > with most people's installations.
> 
> Stop here right now.  You just noticed the real bug, and that's exporting
> scsi.h at all.  I think Olaf sent a patch to fix this already. 

That's a good enough answer for me, certainly.

-- 
dwmw2