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

[James Bottomley <James.Bottomley-d9PhHud1JfjCXq6kfMZ53/egYHeGw8Jk@public.gmane.org>]

On Wed, 2015-04-22 at 17:46 +0200, Greg Kroah-Hartman wrote:
> On Wed, Apr 22, 2015 at 08:35:54AM -0700, James Bottomley wrote:
> > On Wed, 2015-04-15 at 15:19 +0200, Greg Kroah-Hartman wrote:
> > > 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:
> > > > > > + */
> > > > > > +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.
> > 
> > Coming back to this, am I the only one confused here?  What is a
> > 'platform device' then?  Because if it doesn't fit a direct channel to
> > the platform firmware, which seems to be one of the definitions covered
> > in driver-model/platform.txt under devices with minimal infrastructure
> > then perhaps the documentation needs updating.
> 
> I don't remember the original code here at all, sorry.  I'm guessing
> that they were using a class, and a platform device together, which is
> not a good idea.  Just make a "virtual" device, as you don't need/want
> any of the platform device infrastructure here, you just wanted a device
> node and/or a way to show up in sysfs somewhere.

It was a platform device called efi_platform_loader and a single
attribute file in that device called  capsule_load.  I agree that if
we're going to use this for other things, we should probably have a uefi
directory somewhere (under firmware?) to collect everything together
rather than spraying random devices around.

> If you have some kind of "platform resource", then you can be a platform
> device, otherwise please don't use that api just because it seems simple
> to use.  Use the ones the driver core provides for you that really are
> just as simple (i.e. device_create()).

OK, so this is what I'm trying to understand.  Why isn't a pipe to
firmware for something a "platform resource"?  I think UEFI is in the
same class as ACPI which uses platform devices all over.

James