Message-ID: <20250226185022.GM53094@unreal>
Date: Wed, 26 Feb 2025 20:50:22 +0200
From: Leon Romanovsky <leon@...nel.org>
To: "Ertman, David M" <david.m.ertman@...el.com>
Cc: "Nikolova, Tatyana E" <tatyana.e.nikolova@...el.com>,
	"jgg@...dia.com" <jgg@...dia.com>,
	"intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
	"linux-rdma@...r.kernel.org" <linux-rdma@...r.kernel.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
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@...nel.org>
> > Sent: Monday, February 24, 2025 11:56 PM
> > To: Nikolova, Tatyana E <tatyana.e.nikolova@...el.com>
> > Cc: jgg@...dia.com; intel-wired-lan@...ts.osuosl.org; linux-
> > rdma@...r.kernel.org; netdev@...r.kernel.org; Ertman, David M
> > <david.m.ertman@...el.com>
> > 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@...el.com>
> > >
> > > 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.

> 
> > 
> > <...>
> > 
> > > +/* Following APIs are implemented by core PCI driver */
> > > +struct idc_rdma_core_ops {
> > > +	int (*vc_send_sync)(struct idc_rdma_core_dev_info *cdev_info, u8
> > *msg,
> > > +			    u16 len, u8 *recv_msg, u16 *recv_len);
> > > +	int (*vc_queue_vec_map_unmap)(struct idc_rdma_core_dev_info
> > *cdev_info,
> > > +				      struct idc_rdma_qvlist_info *qvl_info,
> > > +				      bool map);
> > > +	/* vport_dev_ctrl is for RDMA CORE driver to indicate it is either
> > ready
> > > +	 * for individual vport aux devices, or it is leaving the state where it
> > > +	 * can support vports and they need to be downed
> > > +	 */
> > > +	int (*vport_dev_ctrl)(struct idc_rdma_core_dev_info *cdev_info,
> > > +			      bool up);
> > > +	int (*request_reset)(struct idc_rdma_core_dev_info *cdev_info,
> > > +			     enum idc_rdma_reset_type reset_type);
> > > +};
> > 
> > Core driver can call to callbacks in irdma, like you already have for
> > irdma_iidc_event_handler(), but all calls from irdma to core driver must
> > be through exported symbols. It gives us race-free world in whole driver
> > except one very specific place (irdma_iidc_event_handler).
> 
> I am confused here as well.  Calling a function through an exported symbol,
> or calling the same function from a function pointer should not affect the
> generation of a race condition, as the same function is being called.
> What is inherently better about an exported symbol versus a function
> pointer when considering race conditions?

Exported symbol guarantees that function exists in core module. Module
subsystem will ensure that core module is impossible to unload until all
users are gone. Function pointer has no such guarantees.

> 
> Also, why is calling a function pointer from the irdma module ok, but calling
> one from the core module not?

Because we need to make sure that core module doesn't disappear while
irdma executes its flow. The opposite is not true because core module
controls irdma devices and aware than irdma module is loaded/unloaded.

Thanks

> 
> Again - Thank you for the review, and if I completely missed your points, please let me know!
> 
> Thanks
> DaveE
> 
> > 
> > Thanks
> 
> 

Powered by blists - more mailing lists