> -----Original Message-----
> From: Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>
> Sent: Friday, October 9, 2020 12:39 PM
> To: Williams, Dan J <dan.j.williams@xxxxxxxxx>
> Cc: Ertman, David M <david.m.ertman@xxxxxxxxx>; alsa-devel@alsa-
> project.org; parav@xxxxxxxxxxxx; Leon Romanovsky <leon@xxxxxxxxxx>;
> tiwai@xxxxxxx; netdev@xxxxxxxxxxxxxxx; ranjani.sridharan@xxxxxxxxxxxxxxx;
> fred.oh@xxxxxxxxxxxxxxx; linux-rdma@xxxxxxxxxxxxxxx;
> dledford@xxxxxxxxxx; broonie@xxxxxxxxxx; jgg@xxxxxxxxxx;
> gregkh@xxxxxxxxxxxxxxxxxxx; kuba@xxxxxxxxxx; Saleem, Shiraz
> <shiraz.saleem@xxxxxxxxx>; davem@xxxxxxxxxxxxx; Patil, Kiran
> <kiran.patil@xxxxxxxxx>
> 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
