RE: [PATCH v2 1/6] Add ancillary bus support

["Ertman, David M" <david.m.ertman@intel.com>]

> -----Original Message-----
> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Sent: Friday, October 9, 2020 12:39 PM
> To: Williams, Dan J <dan.j.williams@intel.com>
> Cc: Ertman, David M <david.m.ertman@intel.com>; alsa-devel@alsa-
> project.org; parav@mellanox.com; Leon Romanovsky <leon@kernel.org>;
> tiwai@suse.de; netdev@vger.kernel.org; ranjani.sridharan@linux.intel.com;
> fred.oh@linux.intel.com; linux-rdma@vger.kernel.org;
> dledford@redhat.com; broonie@kernel.org; jgg@nvidia.com;
> gregkh@linuxfoundation.org; kuba@kernel.org; Saleem, Shiraz
> <shiraz.saleem@intel.com>; davem@davemloft.net; Patil, Kiran
> <kiran.patil@intel.com>
> Subject: Re: [PATCH v2 1/6] Add ancillary bus support
> 
> 
> >>>>>> +
> >>>>>> +   ancildrv->driver.owner = owner;
> >>>>>> +   ancildrv->driver.bus = &ancillary_bus_type;
> >>>>>> +   ancildrv->driver.probe = ancillary_probe_driver;
> >>>>>> +   ancildrv->driver.remove = ancillary_remove_driver;
> >>>>>> +   ancildrv->driver.shutdown = ancillary_shutdown_driver;
> >>>>>> +
> >>>>>
> >>>>> I think that this part is wrong, probe/remove/shutdown functions
> should
> >>>>> come from ancillary_bus_type.
> >>>>
> >>>>   From checking other usage cases, this is the model that is used for
> probe, remove,
> >>>> and shutdown in drivers.  Here is the example from Greybus.
> >>>>
> >>>> int greybus_register_driver(struct greybus_driver *driver, struct
> module *owner,
> >>>>                               const char *mod_name)
> >>>> {
> >>>>           int retval;
> >>>>
> >>>>           if (greybus_disabled())
> >>>>                   return -ENODEV;
> >>>>
> >>>>           driver->driver.bus = &greybus_bus_type;
> >>>>           driver->driver.name = driver->name;
> >>>>           driver->driver.probe = greybus_probe;
> >>>>           driver->driver.remove = greybus_remove;
> >>>>           driver->driver.owner = owner;
> >>>>           driver->driver.mod_name = mod_name;
> >>>>
> >>>>
> >>>>> You are overwriting private device_driver
> >>>>> callbacks that makes impossible to make container_of of
> ancillary_driver
> >>>>> to chain operations.
> >>>>>
> >>>>
> >>>> I am sorry, you lost me here.  you cannot perform container_of on the
> callbacks
> >>>> because they are pointers, but if you are referring to going from
> device_driver
> >>>> to the auxiliary_driver, that is what happens in auxiliary_probe_driver
> in the
> >>>> very beginning.
> >>>>
> >>>> static int auxiliary_probe_driver(struct device *dev)
> >>>> 145 {
> >>>> 146         struct auxiliary_driver *auxdrv = to_auxiliary_drv(dev->driver);
> >>>> 147         struct auxiliary_device *auxdev = to_auxiliary_dev(dev);
> >>>>
> >>>> Did I miss your meaning?
> >>>
> >>> I think you're misunderstanding the cases when the
> >>> bus_type.{probe,remove} is used vs the driver.{probe,remove}
> >>> callbacks. The bus_type callbacks are to implement a pattern where the
> >>> 'probe' and 'remove' method are typed to the bus device type. For
> >>> example 'struct pci_dev *' instead of raw 'struct device *'. See this
> >>> conversion of dax bus as an example of going from raw 'struct device
> >>> *' typed probe/remove to dax-device typed probe/remove:
> >>>
> >>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-
> next.git/commit/?id=75797273189d
> >>
> >> Thanks Dan for the reference, very useful. This doesn't look like a a
> >> big change to implement, just wondering about the benefits and
> >> drawbacks, if any? I am a bit confused here.
> >>
> >> First, was the initial pattern wrong as Leon asserted it? Such code
> >> exists in multiple examples in the kernel and there's nothing preventing
> >> the use of container_of that I can think of. Put differently, if this
> >> code was wrong then there are other existing buses that need to be
> updated.
> >>
> >> Second, what additional functionality does this move from driver to
> >> bus_type provide? The commit reference just states 'In preparation for
> >> introducing seed devices the dax-bus core needs to be able to intercept
> >> ->probe() and ->remove() operations", but that doesn't really help me
> >> figure out what 'intercept' means. Would you mind elaborating?
> >>
> >> And last, the existing probe function does calls dev_pm_domain_attach():
> >>
> >> static int ancillary_probe_driver(struct device *dev)
> >> {
> >>          struct ancillary_driver *ancildrv = to_ancillary_drv(dev->driver);
> >>          struct ancillary_device *ancildev = to_ancillary_dev(dev);
> >>          int ret;
> >>
> >>          ret = dev_pm_domain_attach(dev, true);
> >>
> >> So the need to access the raw device still exists. Is this still legit
> >> if the probe() is moved to the bus_type structure?
> >
> > Sure, of course.
> >
> >>
> >> I have no objection to this change if it preserves the same
> >> functionality and possibly extends it, just wanted to better understand
> >> the reasons for the change and in which cases the bus probe() makes
> more
> >> sense than a driver probe().
> >>
> >> Thanks for enlightening the rest of us!
> >
> > tl;dr: The ops set by the device driver should never be overwritten by
> > the bus, the bus can only wrap them in its own ops.
> >
> > The reason to use the bus_type is because the bus type is the only
> > agent that knows both how to convert a raw 'struct device *' to the
> > bus's native type, and how to convert a raw 'struct device_driver *'
> > to the bus's native driver type. The driver core does:
> >
> >          if (dev->bus->probe) {
> >                  ret = dev->bus->probe(dev);
> >          } else if (drv->probe) {
> >                  ret = drv->probe(dev);
> >          }
> >
> > ...so that the bus has the first priority for probing a device /
> > wrapping the native driver ops. The bus ->probe, in addition to
> > optionally performing some bus specific pre-work, lets the bus upcast
> > the device to bus-native type.
> >
> > The bus also knows the types of drivers that will be registered to it,
> > so the bus can upcast the dev->driver to the native type.
> >
> > So with bus_type based driver ops driver authors can do:
> >
> > struct auxiliary_device_driver auxdrv {
> >      .probe = fn(struct auxiliary_device *, <any aux bus custom probe
> arguments>)
> > };
> >
> > auxiliary_driver_register(&auxdrv); <-- the core code can hide bus details
> >
> > Without bus_type the driver author would need to do:
> >
> > struct auxiliary_device_driver auxdrv {
> >      .drv = {
> >          .probe = fn(struct device *), <-- no opportunity for bus
> > specific probe args
> >          .bus = &auxilary_bus_type, <-- unnecessary export to device drivers
> >      },
> > };
> >
> > driver_register(&auxdrv.drv)
> 
> Thanks Dan, I appreciate the explanation.
> 
> I guess the misunderstanding on my side was that in practice the drivers
> only declare a probe at the auxiliary level:
> 
> struct auxiliary_device_driver auxdrv {
>      .drv = {
>          .name = "my driver"
>          <<< .probe not set here.
>      }
>      .probe =  fn(struct auxiliary_device *, int id),
> }
> 
> It looks indeed cleaner with your suggestion. DaveE and I were talking
> about this moments ago and made the change, will be testing later today.
> 
> Again thanks for the write-up and have a nice week-end.
> 

Like Pierre said, I have already changed the probe, remove, and shutdown callbacks
into the bus_type.

But it should be noted that you are not supposed to have these callbacks in both the
auxdrv->drv->* and in the bus->*.

in drivers/base/driver.c line 158 it checks for this:

if ((drv->bus->probe && drv->probe) ||
             (drv->bus->remove && drv->remove) ||
             (drv->bus->shutdown && drv->shutdown))
                 pr_warn("Driver '%s' needs updating - please use "
                         "bus_type methods\n", drv->name);

So, changing to the bus_type for these is the right thing to do, but driver writers need to
make sure that auxdrv->drv->[probe|remove|shutdown] are NULL.

-DaveE