Re: [PATCH v1 3/6] driver: Google EFI SMI

[Greg KH]

On Tue, Jan 25, 2011 at 03:12:52PM -0800, Mike Waychison wrote:
> On Mon, Jan 24, 2011 at 7:17 PM, Greg KH <greg@xxxxxxxxx> wrote:
> > On Mon, Jan 24, 2011 at 04:24:49PM -0800, Mike Waychison wrote:
> >> +struct gsmi_ioctl {
> >> +     uint16_t        length;         /* total length including data */
> >> +     int             version;        /* structure version */
> >> +     int             command;        /* ioctl command */
> >
> > Ick.  Use proper data types if you are going to create a new ioctl.
> > Same goes for the structures above (hint, use __u32 and friends, not the
> > unit??_t crap.
> >
> > I'd strongly suggest NOT creating a new ioctl though, that' just going
> > to be a pain in the long run.
> 
> Ok.  I'll change these.  Even if the __u32 vs u32 vs uint32_t
> differentiation is non-sense :(

Well, it really isn't nonsense.  __u32 and u32 can be different, and who
really knows what uint32_t means in the end (hint the _t things are for
userspace, not kernelspace, remember the issues of running 64bit kernels
with 32bit userspace programs.)

Actually, that reminds me, what are you going to do about 32/64bit
issues?  This is one reason why ioctls really really suck.  Not to
mention the fact that you really are just adding special syscalls to the
system here, which is another reason people hate them.

So, let me ask, what specifically are you wanting to import/export
to/from the kernel here?  Have you thought about other kernel/user apis
instead of ioctls?  What is forcing you to use ioctls?

> >> +     union {
> >> +             struct gsmi_get_nvram_var       get_nvram_var;
> >> +             struct gsmi_get_next_var        get_next_var;
> >> +             struct gsmi_set_nvram_var       set_nvram_var;
> >> +             struct gsmi_set_event_log       set_event_log;
> >> +             struct gsmi_clear_event_log     clear_event_log;
> >> +     } gsmi_cmd;
> >> +} __packed;
> >> +
> >> +#define GSMI_BASE                    'G'
> >> +#define GSMI_IOCTL_VERSION           1
> >> +#define GSMI_IOCTL                   _IOWR(GSMI_BASE, GSMI_IOCTL_VERSION, \
> >> +                                         struct gsmi_ioctl)
> >> +
> >> +#endif /* _LINUX_GSMI_H */
> >> diff --git a/include/linux/miscdevice.h b/include/linux/miscdevice.h
> >> index 18fd130..34f5dfa 100644
> >> --- a/include/linux/miscdevice.h
> >> +++ b/include/linux/miscdevice.h
> >> @@ -40,6 +40,7 @@
> >>  #define BTRFS_MINOR          234
> >>  #define AUTOFS_MINOR         235
> >>  #define MAPPER_CTRL_MINOR    236
> >> +#define GOOGLE_SMI_MINOR     242
> >>  #define MISC_DYNAMIC_MINOR   255
> >
> > Why make this a static number and not just use a dynamic one?
> 
> Well, we use static device numbers, which is why it has historically
> been static as well.  Yes, that puts us squarely in the 1990s :)
> 
> I can change this guy too though.

Yes please, we don't need to reserve any more numbers, as the 1990s was
a long time ago :)

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/