Message-ID: <a6eddd81-9746-aee7-3403-971c2b6286ef@linux.intel.com>
Date:   Fri, 9 Oct 2020 14:39:02 -0500
From:   Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
To:     Dan Williams <dan.j.williams@...el.com>
Cc:     "Ertman, David M" <david.m.ertman@...el.com>,
        "alsa-devel@...a-project.org" <alsa-devel@...a-project.org>,
        "parav@...lanox.com" <parav@...lanox.com>,
        Leon Romanovsky <leon@...nel.org>,
        "tiwai@...e.de" <tiwai@...e.de>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "ranjani.sridharan@...ux.intel.com" 
        <ranjani.sridharan@...ux.intel.com>,
        "fred.oh@...ux.intel.com" <fred.oh@...ux.intel.com>,
        "linux-rdma@...r.kernel.org" <linux-rdma@...r.kernel.org>,
        "dledford@...hat.com" <dledford@...hat.com>,
        "broonie@...nel.org" <broonie@...nel.org>,
        "jgg@...dia.com" <jgg@...dia.com>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "Saleem, Shiraz" <shiraz.saleem@...el.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "Patil, Kiran" <kiran.patil@...el.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.

Powered by blists - more mailing lists