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

[Greg Kroah-Hartman]

On Wed, Apr 22, 2015 at 09:11:17AM -0700, James Bottomley wrote:
> 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@xxxxxxxxxxxxxxxxxxx]
> > > > > > 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.

And I hate the fact that ACPI did that, but that ship has sailed a long
time ago.  It "should" have been it's own bus and device type, but oh
well.

For a "simple" bus-less device, that has no platform resources needed
(i.e from acpi or device tree), so you don't need the infrastructure
from the platform core, just use a simple device_create() call, that's
what it is there for.

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/