netdev - RE: [iwl-next v4 1/1] iidc/ice/irdma: Update IDC to support multiple consumers

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <IA1PR11MB61944C74491DECA111E84021DDC22@IA1PR11MB6194.namprd11.prod.outlook.com>
Date: Wed, 26 Feb 2025 17:36:44 +0000
From: "Ertman, David M" <david.m.ertman@...el.com>
To: Leon Romanovsky <leon@...nel.org>, "Nikolova, Tatyana E"
	<tatyana.e.nikolova@...el.com>
CC: "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

> -----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?

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.

> 
> <...>
> 
> > +/* 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?

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

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

Thanks
DaveE

> 
> Thanks