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

[Saravana Kannan <saravanak@xxxxxxxxxx>]

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.

> > > > > > > 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.

> >
> > 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.

> 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.

-Saravana

>
> > Once it's done, I don't see why you wouldn't want to assign this device to
> > its corresponding software node. Provided locking is in place etc.
> >
> > > > They - after all - represent the actual
> > > > physical device hierarchy which may or may not be populated at
> > > > run-time depending on many factors.
> > >
> > > No. This is a mistaken assumption.
> >
> > How so?
>
> See above.
>
> > > > Once populated, being able to retrieve the software representation of
> > > > the device (struct device) from the node from which it was populated
> > > > sounds like a reasonable thing to do. What are those problems and are
> > > > they even linked to this issue?
> > > >
> > > > > The get_dev_from_fwnode() is used only in devlink and I want to keep it that way.
> > > > > Nobody else should use it, really.
> > > >
> > > > I don't care all that much, I can get the device from the children of
> > > > the platform device. Still comparing fwnodes, though this time the
> > > > other way around.
> > >
> > > Fine, but do not use dev pointer from fwnode, esp. software node.
> >
> > I will do it but I'd like to clarify the above at some point.
>
> The relationship between device instance(s) and firmware node instance(s)
> is m:n, where each of them can be from 0 to ... x or y.
>
> There is no unique mapping between two.