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

[Dan Williams <dan.j.williams@intel.com>]

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