Re: [PATCH v11] firmware: google: Implement cbmem in sysfs driver

[Greg KH]

On Fri, Sep 30, 2022 at 04:14:41PM -0600, Jack Rosenthal wrote:
> On 2022-09-30 at 08:32 +0200, Greg KH wrote:
> > symlink?  Ick, no, do not do that at all please.
> > 
> > As these are device attributes, just stick with them.  Don't do a crazy
> > symlink into a non-device-attribute portion of the sysfs tree, by doing
> > that you break all userspace tools and stuff like libudev will never
> > even see these attributes.
> 
> I guess I can fill in some info here about the use case needed:
> userspace tools (in this case, a tool called "crossystem") needs to look
> up a CBMEM entry by ID and read it.  So, being able to find a fixed path
> like /sys/firmware/cbmem/<id>/mem is significantly easier than scanning
> all /sys/bus/coreboot/devices/coreboot*/id to find the right device
> first.

No, this is a device, make these device attributes, don't polute sysfs
with random symlinks from a device into the firmware location, that's
not ok.

And again, your current code means that tools like udev and libraries
will not see these attributes at all.  Stick with what every other
device in the kernel does please, consistancy is good.

> What exactly do we break here by adding symlinks?  udev won't look into
> /sys/firmware, right?

Exactly.  You want that to work.

> Or, is there another good alternative that we could use to find a CBMEM
> entry by its id without needing to scan thru all coreboot bus type
> devices?  Setting the device name to something more predictable (e.g.,
> "cbmem-<id>") would require the coreboot bus type to "look ahead" and
> notice it's a CBMEM entry before registering the device, which wouldn't
> exactly be all that clean.

All devices on a bus can call themselves whatever they want, of course
they should be naming themselves based on their device type, why
wouldn't they?

Or put an attribute of the type in the directory of the coreboot device
and iterate on that.  It's a simple udev rule for matching on an
attribute value on a bus.

> > > +What:		/sys/firmware/cbmem/
> > > +Date:		August 2022
> > > +Contact:	Jack Rosenthal <jrosenth@xxxxxxxxxxxx>
> > > +Description:
> > > +		Coreboot provides a variety of data structures in CBMEM.  This
> > > +		directory contains each CBMEM entry, which can be found via
> > > +		Coreboot tables.
> > 
> > What happened to the coreboot name?
> 
> I removed it as it seemed like from your last message you didn't want
> it?

I do not recall saying that, I was just confused that it had not been
used before.

Please do not use cbmem.

thanks,

greg k-h