On Fri, Oct 9, 2020 at 7:27 AM Pierre-Louis Bossart
<pierre-louis.bossart@xxxxxxxxxxxxxxx> wrote:
>
>
>
> >>>> +
> >>>> + 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)
