From mboxrd@z Thu Jan 1 00:00:00 1970 From: Bruce Richardson Subject: Re: [PATCH v6 05/11] bus: introduce hotplug functionality Date: Wed, 28 Jun 2017 16:33:20 +0100 Message-ID: <20170628153320.GA14672@bricha3-MOBL3.ger.corp.intel.com> References: <14287370.n4WKZa6dWl@xps> <1765263.t0XI3ojoND@xps> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Thomas Monjalon , dev , Gaetan Rivet To: Jan Blunck Return-path: Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by dpdk.org (Postfix) with ESMTP id 705381F5 for ; Wed, 28 Jun 2017 17:33:24 +0200 (CEST) Content-Disposition: inline In-Reply-To: List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Wed, Jun 28, 2017 at 05:11:46PM +0200, Jan Blunck wrote: > On Wed, Jun 28, 2017 at 3:30 PM, Thomas Monjalon wrote: > > 28/06/2017 15:09, Jan Blunck: > >> On Wed, Jun 28, 2017 at 2:11 PM, Thomas Monjalon wrote: > >> > 28/06/2017 13:58, Jan Blunck: > >> >> On Wed, Jun 28, 2017 at 1:44 PM, Thomas Monjalon wrote: > >> >> > 27/06/2017 21:03, Jan Blunck: > >> >> >> On Tue, Jun 27, 2017 at 6:11 PM, Gaetan Rivet 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 false hotplug was the original attach function which was just > > adding a new ethdev interface. > > > >> >> 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. > > > > Why do you want to avoid that? > > I think you mean it is not what you had in mind. > > > >> The plug() function would become a "scan-one and probe". > > > > Yes > > > >> 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 :) > > > > It is a totally different thing. > > We are talking about hotplug of a device, > > and you are talking about changing drivers dynamically. > > > > So I still don't understand what is the issue with the plug/unplug > > functions proposed here. > > > > I don't agree with the notion that plug() means "scan-one and probe". What do you see as plug doing then? What do you see as the use-case and then logic flow for hotplug?