Re: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware

[Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>]

On Wed, Apr 15, 2015 at 11:32:29AM +0000, Kweh, Hock Leong wrote:
> > -----Original Message-----
> > From: Greg Kroah-Hartman [mailto:gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org]
> > Sent: Tuesday, April 14, 2015 10:09 PM
> > 
> > On Tue, Apr 14, 2015 at 05:44:56PM +0800, Kweh, Hock Leong wrote:
> > > From: "Kweh, Hock Leong" <hock.leong.kweh-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > >
> > > Introducing a kernel module to expose capsule loader interface
> > > for user to upload capsule binaries. This module leverage the
> > > request_firmware_direct_full_path() to obtain the binary at a
> > > specific path input by user.
> > >
> > > Example method to load the capsule binary:
> > > echo -n "/path/to/capsule/binary" >
> > /sys/devices/platform/efi_capsule_loader/capsule_loader
> > 
> > Ick, why not just have the firmware file location present, and copy it
> > to the sysfs file directly from userspace, instead of this two-step
> > process?
> 
> Err .... I may not catch your meaning correctly. Are you trying to say
> that you would prefer the user to perform:
> 
> cat file.bin > /sys/.../capsule_loader
> 
> instead of
> 
> echo -n "/path/to/binary" > /sys/..../capsule_laoder

Yes.  What's the namespace of your /path/to/binary/ and how do you know
the kernel has the same one when it does the firmware load call?  By
just copying the data with 'cat', you don't have to worry about
namespace issues at all.

> The reason we stick with the firmware_class is because we don't
> want to replicate a code which already mature and has open API
> for developer to use.

That's fine, but adding a new api to the firmware interface seems odd to
me, just because you don't like using /lib/ or any of the other
"standard" locations for firmware blobs.  And note, that path is
configurable.

> > > + */
> > > +static void __exit efi_capsule_loader_exit(void)
> > > +{
> > > +	platform_device_unregister(efi_capsule_pdev);
> > 
> > This is not a platform device, don't abuse that interface please.
> > 
> > greg k-h
> 
> Okay, so you would recommend to use device_register() for this case?
> Or you would think that this is more suitable to use class_register()?

A class isn't needed, you just want a device right?  So just use a
device, but not a platform device, as that isn't what you have here.

thanks,

greg k-h