Re: [PATCH] gpio: sim: don't fiddle with GPIOLIB private members

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>]

On Tue, Feb 20, 2024 at 05:46:27PM -0800, Saravana Kannan wrote:
> On Mon, Sep 4, 2023 at 3:29 AM Andy Shevchenko
> <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
> > On Mon, Sep 04, 2023 at 12:12:44PM +0200, Bartosz Golaszewski wrote:
> > > On Mon, Sep 4, 2023 at 12:05 PM Andy Shevchenko
> > > <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
> > > > On Mon, Sep 04, 2023 at 11:47:54AM +0200, Bartosz Golaszewski wrote:
> > > > > On Mon, Sep 4, 2023 at 11:40 AM Andy Shevchenko
> > > > > <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
> > > > > > On Mon, Sep 04, 2023 at 11:22:32AM +0200, Bartosz Golaszewski wrote:
> > > > > > > On Mon, Sep 4, 2023 at 10:59 AM Andy Shevchenko
> > > > > > > <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
> > > > > > > > On Sat, Sep 02, 2023 at 04:40:05PM +0200, Bartosz Golaszewski wrote:
> > > > > > > > > On Fri, Sep 1, 2023 at 11:10 PM Andy Shevchenko
> > > > > > > > > <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
> > > > > > > > > > On Fri, Sep 01, 2023 at 08:32:40PM +0200, Bartosz Golaszewski wrote:

...

> > > > > > > > > > > -     /* Used by sysfs and configfs callbacks. */
> > > > > > > > > > > -     dev_set_drvdata(&gc->gpiodev->dev, chip);
> > > > > > > > > > > +     /* Used by sysfs callbacks. */
> > > > > > > > > > > +     dev_set_drvdata(swnode->dev, chip);
> > > > > > > > > >
> > > > > > > > > > dev pointer of firmware node is solely for dev links. Is it the case here?
> > > > > > > > > > Seems to me you luckily abuse it.
> > > > > > > > >
> > > > > > > > > I don't think so. If anything we have a helper in the form of
> > > > > > > > > get_dev_from_fwnode() but it takes reference to the device while we
> > > > > > > > > don't need it - we know it'll be there because we created it.
> > > > > > > > >
> > > > > > > > > This information (struct device of the GPIO device) can also be
> > > > > > > > > retrieved by iterating over the device children of the top platform
> > > > > > > > > device and comparing their fwnodes against the one we got passed down
> > > > > > > > > from probe() but it's just so many extra steps.
> > > > > > > > >
> > > > > > > > > Or we can have a getter in gpio/driver.h for that but I don't want to
> > > > > > > > > expose another interface is we can simply use the fwnode.
> > > > > > > >
> 
> Sorry for being late to the party.

You decided to make a blast from the past due to the last patches from me? :-)

> > > > > > > > dev pointer in the fwnode strictly speaking is optional. No-one, except
> > > > > > > > its solely user, should rely on it (its presence and lifetime).
> > > > > > >
> > > > > > > Where is this documented? Because just by a quick glance into
> > > > > > > drivers/base/core.c I can tell that if a device has an fwnode then
> > > > > > > fwnode->dev gets assigned when the device is created and cleared when
> > > > > > > it's removed (note: note even attached to driver, just
> > > > > > > created/removed). Seems like pretty reliable behavior to me.
> > > > > >
> > > > > > Yes, and even that member in fwnode is a hack in my opinion. We should not mix
> > > > > > layers and the idea in the future to get rid of the fwnode_handle to be
> > > > > > _embedded_ into struct device. It should be separate entity, and device
> > > > > > instance may use it as a linked list. Currently we have a few problems because
> > > > > > of the this design mistake.
> > > > >
> > > > > I don't see how this would work if fwnodes can exist before struct
> > > > > device is even created.
> > > >
> > > > That's whole idea behind swnodes. They (ideally) should be created _before_
> > > > any other object they are being used with. This is how it works today.
> > >
> > > Yes, this is what I meant: if fwnodes can be created before struct
> > > device (as it is now) and their life-time is separated then how could
> > > you possibly make the fwnode part of struct device?
> > >
> > > > And doing swnode->dev = ... contradicts a lot: layering, lifetime objects, etc.
> 
> I understand what you are trying to say about layering, but there are
> no lifetime violations here.

There is. Software node is not firmware node, their lifetime is the same or
wider than the respective device (often, they are statically defined without
any device in mind).

> > > No it doesn't. We have the software node - the template for the
> > > device. It can only be populated with a single device entry.
> >
> > Which is wrong assumption. Software nodes (and firmware nodes) in general
> > can be shared. Which device pointer you want to add there?
> 
> I don't think this is any harder to handle than how a device's
> secondary fwnode is handled in set_primary_fwnode(). For secondary
> fwnodes, you just WARN and overwrite it and move on.

The whole concept of a single linked list with limitation to up to two
nodes and being the part of the struct fwnode_handle itself appears to
be problematic. We have a lot of tricks here and there instead of properly
having a list head in the struct device without any limitations in number
of nodes with a priority based on the appearance in the list.

For the details you may ask USB DWC3 developers and related to that.

> > Which one should be next when one of the devices is gone?
> 
> Similar to how set_primary_fwnode() handles deletion (NULL), you can
> handle the same for when a device is removed. You can check the parent
> or the bus for another device with the same fwnode and set it.

> > No, simply no. Do not use it!
> 
> Using fwnode_handle->dev is no different than searching a bus for a
> device which has dev->fwnode match the fwnode you are looking for.
> 
> In both cases, you are just going to get the first device that was
> added. It's completely pointless to force searching a bus to find the
> device with a specific fwnode.
> 
> In the special cases where one fwnode has multiple devices, no generic
> code is going to always handle the device search correctly. The
> framework adding those devices probably knows what's the right thing
> to do based on which of the N devices with the same fwnode they are
> trying to find.
> 
> I understand it's not great, but blindly saying "search the bus" isn't
> really improving anything here and just makes things unnecessarily
> inefficient.

Is there any _good_ documentation for devlinks and all that fields in the
struct fwnode? Why should we use that without any understanding of the
purposes of that field. We, as device property developers, hadn't introduced
that field and never required it. It's an alien to device properties APIs.

-- 
With Best Regards,
Andy Shevchenko