On Thu, Mar 13, 2025 at 04:38:39PM -0700, Samudrala, Sridhar wrote: > > > On 3/2/2025 12:26 AM, Leon Romanovsky wrote: > > On Wed, Feb 26, 2025 at 11:01:52PM +0000, Ertman, David M wrote: > > > > > > > > > > -----Original Message----- > > > > From: Leon Romanovsky <leon@xxxxxxxxxx> > > > > Sent: Wednesday, February 26, 2025 10:50 AM > > > > To: Ertman, David M <david.m.ertman@xxxxxxxxx> > > > > Cc: Nikolova, Tatyana E <tatyana.e.nikolova@xxxxxxxxx>; jgg@xxxxxxxxxx; > > > > intel-wired-lan@xxxxxxxxxxxxxxxx; linux-rdma@xxxxxxxxxxxxxxx; > > > > netdev@xxxxxxxxxxxxxxx > > > > Subject: Re: [iwl-next v4 1/1] iidc/ice/irdma: Update IDC to support multiple > > > > consumers > > > > > > > > On Wed, Feb 26, 2025 at 05:36:44PM +0000, Ertman, David M wrote: > > > > > > -----Original Message----- > > > > > > From: Leon Romanovsky <leon@xxxxxxxxxx> > > > > > > Sent: Monday, February 24, 2025 11:56 PM > > > > > > To: Nikolova, Tatyana E <tatyana.e.nikolova@xxxxxxxxx> > > > > > > Cc: jgg@xxxxxxxxxx; intel-wired-lan@xxxxxxxxxxxxxxxx; linux- > > > > > > rdma@xxxxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; Ertman, David M > > > > > > <david.m.ertman@xxxxxxxxx> > > > > > > Subject: Re: [iwl-next v4 1/1] iidc/ice/irdma: Update IDC to support > > > > multiple > > > > > > consumers > > > > > > > > > > > > On Mon, Feb 24, 2025 at 11:04:28PM -0600, Tatyana Nikolova wrote: > > > > > > > From: Dave Ertman <david.m.ertman@xxxxxxxxx> > > > > > > > > > > > > > > To support RDMA for E2000 product, the idpf driver will use the IDC > > > > > > > interface with the irdma auxiliary driver, thus becoming a second > > > > > > > consumer of it. This requires the IDC be updated to support multiple > > > > > > > consumers. The use of exported symbols no longer makes sense > > > > because it > > > > > > > will require all core drivers (ice/idpf) that can interface with irdma > > > > > > > auxiliary driver to be loaded even if hardware is not present for those > > > > > > > drivers. > > > > > > > > > > > > In auxiliary bus world, the code drivers (ice/idpf) need to created > > > > > > auxiliary devices only if specific device present. That auxiliary device > > > > > > will trigger the load of specific module (irdma in our case). > > > > > > > > > > > > EXPORT_SYMBOL won't trigger load of irdma driver, but the opposite is > > > > > > true, load of irdma will trigger load of ice/idpf drivers (depends on > > > > > > their exported symbol). > > > > > > > > > > > > > > > > > > > > To address this, implement an ops struct that will be universal set of > > > > > > > naked function pointers that will be populated by each core driver for > > > > > > > the irdma auxiliary driver to call. > > > > > > > > > > > > No, we worked very hard to make proper HW discovery and driver > > > > autoload, > > > > > > let's not return back. For now, it is no-go. > > > > > > > > > > Hi Leon, > > > > > > > > > > I am a little confused about what the problem here is. The main issue I pull > > > > > from your response is: Removing exported symbols will stop ice/idpf from > > > > > autoloading when irdma loads. Is this correct or did I miss your point? > > > > > > > > It is one of the main points. > > > > > > > > > > > > > > But, if there is an ice or idpf supported device present in the system, the > > > > > appropriate driver will have already been loaded anyway (and gone > > > > through its > > > > > probe flow to create auxiliary devices). If it is not loaded, then the system > > > > owner > > > > > has either unloaded it manually or blacklisted it. This would not cause an > > > > issue > > > > > anyway, since irdma and ice/idpf can load in any order. > > > > > > > > There are two assumptions above, which both not true. > > > > 1. Users never issue "modprobe irdma" command alone and always will call > > > > to whole chain "modprobe ice ..." before. > > > > 2. You open-code module subsystem properly with reference counters, > > > > ownership and locks to protect from function pointers to be set/clear > > > > dynamically. > > > > > > Ah, I see your reasoning now. Our goal was to make the two modules independent, > > > with no prescribed load order mandated, and utilize the auxiliary bus and device subsystem > > > to handle load order and unload of one or the other module. The auxiliary device only has > > > the lifespan of the core PCI driver, so if the core driver unloads, then the auxiliary device gets > > > destroyed, and the associated link based off it will be gone. We wanted to be able to unload > > > and reload either of the modules (core or irdma) and have the interaction be able to restart with a > > > new probe. All our inter-driver function calls are protected by device lock on the auxiliary > > > device for the duration of the call. > > > > Yes, you are trying to return to pre-aux era. > > > The main motivation to go with callbacks to the parent driver instead of > using exported symbols is to allow loading only the parent driver required > for a particular deployment. irdma is a common rdma auxilary driver that > supports multiple parent pci drivers(ice, idpf, i40e, iavf). If we use > exported symbols, all these modules will get loaded even on a system with > only idpf device. It is not how kernel works. IRDMA doesn't call to all exported symbols of all these modules. It will call to only one exported symbol and that module will be loaded. > > The documentation for auxiliary bus > https://docs.kernel.org/driver-api/auxiliary_bus.html > shows an example on how shared data/callbacks can be used to establish > connection with the parent. I'm aware of this documentation, it is incorrect. You can find the explanation why this documentation exists in habanalabs discussion. > > Auxiliary devices are created and registered by a subsystem-level core > device that needs to break up its functionality into smaller fragments. One > way to extend the scope of an auxiliary_device is to encapsulate it within a > domain-specific structure defined by the parent device. This structure > contains the auxiliary_device and any associated shared data/callbacks > needed to establish the connection with the parent. > > An example is: > > struct foo { > struct auxiliary_device auxdev; > void (*connect)(struct auxiliary_device *auxdev); > void (*disconnect)(struct auxiliary_device *auxdev); > void *data; > }; > > This example clearly shows that it is OK to use callbacks from the aux > driver. The aux driver is dependent on the parent driver and the parent > driver will guarantee that it will not get unloaded until all the auxiliary > devices are destroyed. > > Hopefully you will understand our motivation of going with this design and > not force us to go with a solution that is not optimal. Feel free to fix documentation. Thanks > > Thanks > Sridhar >
