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

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>]

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.
> > > > > >
> > > > > > 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.
> 
> 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? Which one
should be next when one of the devices is gone?

No, simply no. Do not use it!

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

> > > > We can discuss with Saravana, but I don't believe he can convince me otherwise.

-- 
With Best Regards,
Andy Shevchenko