Re: [cdi-devel] Proposed Dynamic Driver Loading System

[Matthew Iselin <matthew@xxxxxxxxxxxxxx>]

On Sun, Jan 3, 2010 at 3:13 AM, Rich Edelman <redelman@xxxxxxxxx> wrote:
> On Sat, Jan 2, 2010 at 5:46 AM, Kevin Wolf <kevin@xxxxxxxxxx> wrote:
>> On Sat, Jan 02, 2010 at 04:58:39PM +1000, Matthew Iselin wrote:
>>> The idea is that the OS stores a map of device identifiers (at this stage the
>>> proposal considers the use of PCI vendor and device codes) which detail the
>>> name of a driver which is to be loaded if the given device is detected. This
>>> list may be OS-specific, or there may be a generic list provided as part of
>>> CDI. Either way, the OS is open to interpret the driver name as it sees fit:
>>> for example, appending ".o" in the case of Pedigree.
>>>
>>> This map might look something like this:
>>>
>>> /** OS-specific map of PCI identifiers -> driver names */
>>>
>>> struct cdi_os_pci_driver_map = {
>>>     0x8086, 0x1230, "piix", // Vendor, Device, Driver name
>>>     ...
>>> };
>>
>> This would be an additional top-level C file, right? I don't think it's
>> a good idea to manually create it and keep it in sync (especially
>> considering that many OSes don't use all drivers), but generating such a
>> file looks fine to me.
>
> I also agree that such a file should be generated. It'd not be too
> hard to generate this list just from the drivers that were compiled,
> rather than containing all the drivers.

A simple <your scripting language here> script could be added and
called to update the list during the build of CDI.

>>> I would like for this map to be implemented as part of CDI, in order to ensure
>>> that the list is kept up-to-date with drivers as they are written for CDI.
>>> Keeping the list in CDI also decreases the amount of work required to port CDI
>>> to a new OS and take advantage of these kind of features.
>>>
>>> In this early draft of the concept, there is no support for non-PCI devices.
>>> Things such as USB with each device having a class code (eg, mass storage, or
>>> human interface device) may need their own list. This would be logical as the
>>> method for loading drivers dynamically upon a hotplug or similar event could
>>> be quite different to that of detecting static PCI devices at boot-time. Hot
>>> plug PCI devices are considered to be out of the scope of this proposal.
>>
>> No, we need to do it right. And this definitely means considering more
>> than coldplugged PCI devices. So what I would like too see in drivers is
>> something like this:
>>
>> static struct cdi_bus_data* supported_devices[] = {
>>    CDI_PCI_VENDOR_DEVICE(0x8086, 0x1230),
>> };
>>
>> static struct cdi_driver piix_driver = {
>>    .name = "piix",
>>    .devices = &supported_devices,
>>    ...
>> }
>>
>> Accordingly, I would change the generated file to contain cdi_bus_data
>> structs instead of a fixed vendor ID/device ID/name schema.
>
> I'm in agreement here as well. We need to consider PCI, USB, etc all
> from the start.
> Also, using a separate struct like this would make it easier to scan
> through the compiled drivers
> and generate the mapping file as described above.

I agree - simplifying the generation script is very important.

>>> At a time defined by the OS implementing CDI, the list of PCI devices will be
>>> iterated, and each device will be checked against the known PCI->driver list.
>>> Should a driver be found in this list, the driver will be loaded (if not
>>> already) and used to initialise the detected device.
>>>
>>> Now, this brings up a small tangent: it is my belief that driver loading could
>>> be added to the CDI interface, as something like:
>>>
>>> /* Returns -1 if the driver could not be loaded, zero otherwise */
>>> int cdi_load_driver(const char *);
>
> I think this prototype is all wrong. Whatever it is that wants to load
> another driver needs to pass a structure that describes the device a
> driver is needed for. Driver's shouldn't be expected to know the name
> of the driver they need for another device. Sure, they can get this
> from the mapping file discussed previously, but I think that's wrong
> as well. Such a file needs to really be internal to the CDI core, and
> not something used by the drivers (only contributed to by the
> drivers).

Yes, this specifically is one thing I was very unsure about when I
thought out the initial proposal. The main reason I brought it up was
for a situation like this:
1. Plug in a USB device
2. The USB controller driver tells the  OS a new device is plugged in.
Now the OS needs to figure out what driver to load for the device.
The `cdi_load_driver' function is certainly incorrect here, but some
sort of hook in CDI to inform the OS of a new device that has been
added would help here.

> Of course, all this relies on bus drivers. It also means we need to be
> able to say "stop the driver for device X" and "unregister device X".
> Hotplugging isn't only about plugging in new devices, it's also about
> correctly handling when a device suddenly disappears.

Unregistering the device, yes, but perhaps not stopping the driver
(which is significantly less complex). Again, hooks in CDI could
provide an interface for drivers which facilitate hotplugging to
notify the OS of new devices _and_ removed devices. What the OS does
next is not important; what is important is that the controller
notifies the OS of these events.

>> Personally, I tend to consider coldplugging some kind of hotplugging
>> that incidentally happens at driver startup.
>
> +1. coldplugging is really nothing other than a device that was
> hotplugged at boot time. :)

I have never seen it this way, but that's a very good way of looking at it.

Thanks for the feedback!

Matt