Message-ID: <CAMRc=Meznt=5m_4OnSRf04xHsUy39hH7S7_8ftZaHq6GD-taEw@mail.gmail.com>
Date:   Fri, 5 Mar 2021 11:58:18 +0100
From:   Bartosz Golaszewski <brgl@...ev.pl>
To:     Greg KH <gregkh@...uxfoundation.org>
Cc:     Geert Uytterhoeven <geert@...ux-m68k.org>,
        Joel Becker <jlbec@...lplan.org>,
        Christoph Hellwig <hch@....de>, Shuah Khan <shuah@...nel.org>,
        Linus Walleij <linus.walleij@...aro.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>,
        Kent Gibson <warthog618@...il.com>,
        Jonathan Corbet <corbet@....net>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "open list:DOCUMENTATION" <linux-doc@...r.kernel.org>,
        Bartosz Golaszewski <bgolaszewski@...libre.com>
Subject: Re: [PATCH v2 08/12] drivers: export device_is_bound()

On Fri, Mar 5, 2021 at 11:24 AM Greg KH <gregkh@...uxfoundation.org> wrote:
>
> On Fri, Mar 05, 2021 at 10:16:10AM +0100, Bartosz Golaszewski wrote:
> > On Fri, Mar 5, 2021 at 9:55 AM Greg KH <gregkh@...uxfoundation.org> wrote:
> > >
> > > On Fri, Mar 05, 2021 at 09:45:41AM +0100, Bartosz Golaszewski wrote:
> > > > On Fri, Mar 5, 2021 at 9:34 AM Greg KH <gregkh@...uxfoundation.org> wrote:
> > > > >
> > > > > On Fri, Mar 05, 2021 at 09:18:30AM +0100, Geert Uytterhoeven wrote:
> > > > > > CC Greg
> > > > > >
> > > > > > On Thu, Mar 4, 2021 at 11:30 AM Bartosz Golaszewski <brgl@...ev.pl> wrote:
> > > > > > >
> > > > > > > From: Bartosz Golaszewski <bgolaszewski@...libre.com>
> > > > > > >
> > > > > > > Export the symbol for device_is_bound() so that we can use it in gpio-sim
> > > > > > > to check if the simulated GPIO chip is bound before fetching its driver
> > > > > > > data from configfs callbacks in order to retrieve the name of the GPIO
> > > > > > > chip device.
> > > > > > >
> > > > > > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@...libre.com>
> > > > > > > ---
> > > > > > >  drivers/base/dd.c | 1 +
> > > > > > >  1 file changed, 1 insertion(+)
> > > > > > >
> > > > > > > diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> > > > > > > index 9179825ff646..c62c02e3490a 100644
> > > > > > > --- a/drivers/base/dd.c
> > > > > > > +++ b/drivers/base/dd.c
> > > > > > > @@ -353,6 +353,7 @@ bool device_is_bound(struct device *dev)
> > > > > > >  {
> > > > > > >         return dev->p && klist_node_attached(&dev->p->knode_driver);
> > > > > > >  }
> > > > > > > +EXPORT_SYMBOL_GPL(device_is_bound);
> > > > >
> > > > > No.  Please no.  Why is this needed?  Feels like someone is doing
> > > > > something really wrong...
> > > > >
> > > > > NACK.
> > > > >
> > > >
> > > > I should have Cc'ed you the entire series, my bad.
> > > >
> > > > This is the patch that uses this change - it's a new, improved testing
> > > > module for GPIO using configfs & sysfs as you (I think) suggested a
> > > > while ago:
> > > >
> > > > https://lkml.org/lkml/2021/3/4/355
> > > >
> > > > The story goes like this: committing the configfs item registers a
> > > > platform device.
> > >
> > > Ick, no, stop there, that's not a "real" device, please do not abuse
> > > platform devices like that, you all know I hate this :(
> > >
> > > Use the virtbus code instead perhaps?
> > >
> >
> > I have no idea what virtbus is and grepping for it only returns three
> > hits in: ./drivers/pci/iov.c and it's a function argument.
> >
> > If it stands for virtual bus then for sure it sounds like the right
> > thing but I need to find more info on this.
>
> Sorry, wrong name, see Documentation/driver-api/auxiliary_bus.rst for
> the details.  "virtbus" was what I think about it as that was my
> original name for it, but it eventually got merged with a different
> name.
>
> > > > As far as I understand - there's no guarantee that
> > > > the device will be bound to a driver before the commit callback (or
> > > > more specifically platform_device_register_full() in this case)
> > > > returns so the user may try to retrieve the name of the device
> > > > immediately (normally user-space should wait for the associated uevent
> > > > but nobody can force that) by doing:
> > > >
> > > > mv /sys/kernel/config/gpio-sim/pending/foo /sys/kernel/config/gpio-sim/live/
> > > > cat /sys/kernel/config/gpio-sim/live/foo/dev_name
> > > >
> > > > If the device is not bound at this point, we'll have a crash in the
> > > > kernel as opposed to just returning -ENODEV.
> > >
> > > How will the kernel crash?  What has created the dev_name sysfs file
> > > before it is possible to be read from?  That feels like the root
> > > problem.
> > >
> >
> > It's not sysfs - it's in configfs. Each chip has a read-only configfs
> > attribute that returns the name of the device - I don't really have a
> > better idea to map the configfs items to devices that committing
> > creates.
>
> Same question, why are you exporting a configfs attribute that can not
> be read from?  Only export it when your driver is bound to the device.
>

The device doesn't know anything about configfs. Why would it? The
configuration of a GPIO chip can't be changed after it's instantiated,
this is why we have committable items.

We export a directory in configfs: gpio-sim -> user creates a new
directory (item) in gpio-sim/pending/foo and it's not tied to any
device yet but exports attributes which we use to configure the device
(label, number of lines, line names etc.), then we mv
gpio-sim/pending/foo gpio-sim/live and this is when the device gets
created and registered with the subsystem. We take all the configured
attributes and put them into device properties for both the driver and
gpiolib core (for standard properties) to read - just like we would
with a regular GPIO driver because this is the goal: test the core
code.

Configfs doesn't even allow to dynamically export and unexport attributes.

Bart

Powered by blists - more mailing lists