Re: [PATCH v6 05/11] bus: introduce hotplug functionality

[Jan Blunck <jblunck@infradead.org>]

On Wed, Jun 28, 2017 at 2:11 PM, Thomas Monjalon <thomas@monjalon.net> wrote:
> 28/06/2017 13:58, Jan Blunck:
>> On Wed, Jun 28, 2017 at 1:44 PM, Thomas Monjalon <thomas@monjalon.net> wrote:
>> > 27/06/2017 21:03, Jan Blunck:
>> >> On Tue, Jun 27, 2017 at 6:11 PM, Gaetan Rivet <gaetan.rivet@6wind.com> wrote:
>> >> > --- a/lib/librte_eal/common/include/rte_bus.h
>> >> > +++ b/lib/librte_eal/common/include/rte_bus.h
>> >> >  /**
>> >> > + * Implementation specific probe function which is responsible for linking
>> >> > + * devices on that bus with applicable drivers.
>> >> > + * The plugged device might already have been used previously by the bus,
>> >> > + * in which case some buses might prefer to detect and re-use the relevant
>> >> > + * information pertaining to this device.
>> >> > + *
>> >> > + * @param da
>> >> > + *     Device declaration.
>> >> > + *
>> >> > + * @return
>> >> > + *     The pointer to a valid rte_device usable by the bus on success.
>> >> > + *     NULL on error. rte_errno is then set.
>> >> > + */
>> >> > +typedef struct rte_device * (*rte_bus_plug_t)(struct rte_devargs *da);
>> >>
>> >> Shouldn't this be orthogonal to unplug() and take a rte_device. You
>> >> should only be able to plug devices that have been found by scan
>> >> before.
>> >
>> > Plugging a device that has been scanned before is a special case.
>> > In a true hotplug scenario, we could use this plug function passing
>> > a devargs.
>> > I don't see any issue passing rte_devargs to plug and rte_device to unplug.
>>
>> What do you mean by "true hotplug"?
>
> I mean a kernel notification of a new device.
>

Does a "false hotplug" exist, too? :)

>> The problem with this is that passing just rte_devargs to plug()
>> requires the bus to parse and enrich the rte_devargs with bus
>> specifics. From there it gets folded into the to-be-created bus
>> specific rte_XXX_device. This makes it unnecessarily complicated and
>> even worse it adds a second code path how devices come alive.
>
> Just after the notification, the rte_device does not exist yet.
> So the plug function could share the same code as the scan function
> to get the metadata and create the device instance.
>

Exactly this is what I want to avoid. The plug() function would become
a "scan-one and probe". From my point of view plug() and unplug()
should be orthogonal. The plug() and unplug() should only be
responsible for adding drivers with optional arguments. The EAL should
allow the drivers to get unplugged/re-plugged at run-time. I want to
be able to change arguments ... or even drivers :)

>> When we get notified about a hotplug event we already know which bus
>> this event belongs to:
>>
>> 1. scan the bus for incoming devices
>
> No need to scan every devices here.
>

This is a readdir followed by open+read+close for any new device. This
code belongs here anyway. Its lightweight if nothing changed. The scan
itself should be idempotent anyway.

>> 2. plug single device with devargs and probe for drivers
>>
>> Makes sense?
>
> I want to make sure there is no misunderstanding first :)

Which makes sense. That is probably my fault due to being too
distracted with other things and not communicating well enough while
Gaetan consumed my code.