linux-kernel - RE: [EXTERNAL] Re: [PATCH net-next] net: mana: Support HW link state events

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:
 <CY5PR21MB3447B6D69542EA4532547A5FCAEAA@CY5PR21MB3447.namprd21.prod.outlook.com>
Date: Mon, 13 Oct 2025 21:47:27 +0000
From: Haiyang Zhang <haiyangz@...rosoft.com>
To: Andrew Lunn <andrew@...n.ch>, Haiyang Zhang <haiyangz@...ux.microsoft.com>
CC: "linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>, Paul Rosswurm
	<paulros@...rosoft.com>, Dexuan Cui <decui@...rosoft.com>, KY Srinivasan
	<kys@...rosoft.com>, "wei.liu@...nel.org" <wei.liu@...nel.org>,
	"edumazet@...gle.com" <edumazet@...gle.com>, "davem@...emloft.net"
	<davem@...emloft.net>, "kuba@...nel.org" <kuba@...nel.org>,
	"pabeni@...hat.com" <pabeni@...hat.com>, Long Li <longli@...rosoft.com>,
	"ssengar@...ux.microsoft.com" <ssengar@...ux.microsoft.com>,
	"ernis@...ux.microsoft.com" <ernis@...ux.microsoft.com>,
	"dipayanroy@...ux.microsoft.com" <dipayanroy@...ux.microsoft.com>, Konstantin
 Taranov <kotaranov@...rosoft.com>, "horms@...nel.org" <horms@...nel.org>,
	"shradhagupta@...ux.microsoft.com" <shradhagupta@...ux.microsoft.com>,
	"leon@...nel.org" <leon@...nel.org>, "mlevitsk@...hat.com"
	<mlevitsk@...hat.com>, "yury.norov@...il.com" <yury.norov@...il.com>, Shiraz
 Saleem <shirazsaleem@...rosoft.com>, "andrew+netdev@...n.ch"
	<andrew+netdev@...n.ch>, "linux-rdma@...r.kernel.org"
	<linux-rdma@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
Subject: RE: [EXTERNAL] Re: [PATCH net-next] net: mana: Support HW link state
 events



> -----Original Message-----
> From: Andrew Lunn <andrew@...n.ch>
> Sent: Monday, October 13, 2025 5:13 PM
> To: Haiyang Zhang <haiyangz@...ux.microsoft.com>
> Cc: linux-hyperv@...r.kernel.org; netdev@...r.kernel.org; Haiyang Zhang
> <haiyangz@...rosoft.com>; Paul Rosswurm <paulros@...rosoft.com>; Dexuan
> Cui <decui@...rosoft.com>; KY Srinivasan <kys@...rosoft.com>;
> wei.liu@...nel.org; edumazet@...gle.com; davem@...emloft.net;
> kuba@...nel.org; pabeni@...hat.com; Long Li <longli@...rosoft.com>;
> ssengar@...ux.microsoft.com; ernis@...ux.microsoft.com;
> dipayanroy@...ux.microsoft.com; Konstantin Taranov
> <kotaranov@...rosoft.com>; horms@...nel.org;
> shradhagupta@...ux.microsoft.com; leon@...nel.org; mlevitsk@...hat.com;
> yury.norov@...il.com; Shiraz Saleem <shirazsaleem@...rosoft.com>;
> andrew+netdev@...n.ch; linux-rdma@...r.kernel.org; linux-
> kernel@...r.kernel.org
> Subject: [EXTERNAL] Re: [PATCH net-next] net: mana: Support HW link state
> events
> 
> > +static void mana_link_state_handle(struct work_struct *w)
> > +{
> > +	struct mana_context *ac =
> > +		container_of(w, struct mana_context, link_change_work.work);
> > +	struct mana_port_context *apc;
> > +	struct net_device *ndev;
> > +	bool link_up;
> > +	int i;
> 
> Since you don't need ac here, i would postpone the assignment into the
> body of the function, so keeping with reverse christmass tree.
Will do.

> 
> > +
> > +	if (!rtnl_trylock()) {
> > +		schedule_delayed_work(&ac->link_change_work, 1);
> > +		return;
> > +	}
> 
> Is there a deadlock you are trying to avoid here? Why not wait for the
> lock?
I think it's probably not needed, will double check...

> 
> > +
> > +	if (ac->link_event == HWC_DATA_HW_LINK_CONNECT)
> > +		link_up = true;
> > +	else if (ac->link_event == HWC_DATA_HW_LINK_DISCONNECT)
> > +		link_up = false;
> > +	else
> > +		goto out;
> > +
> > +	/* Process all ports */
> > +	for (i = 0; i < ac->num_ports; i++) {
> > +		ndev = ac->ports[i];
> > +		if (!ndev)
> > +			continue;
> > +
> > +		apc = netdev_priv(ndev);
> > +
> > +		if (link_up) {
> > +			netif_carrier_on(ndev);
> > +
> > +			if (apc->port_is_up)
> > +				netif_tx_wake_all_queues(ndev);
> > +
> > +			__netdev_notify_peers(ndev);
> > +		} else {
> > +			if (netif_carrier_ok(ndev)) {
> > +				netif_tx_disable(ndev);
> > +				netif_carrier_off(ndev);
> > +			}
> > +		}
> 
> It is odd this is asymmetric. Up and down should really be opposites.
For the up event, we need to delay the wake up queues if the 
mana_close() is called, or mana_open() isn't called yet.

Also, we notify peers only when link up.

> 
> > @@ -3500,6 +3548,8 @@ void mana_remove(struct gdma_dev *gd, bool
> suspending)
> >  	int err;
> >  	int i;
> >
> > +	cancel_delayed_work_sync(&ac->link_change_work);
> 
> I don't know delayed work too well. Is this sufficient when the work
> requeues itself because it cannot get RTNL?

cancel_work_sync()'s doc says "This function can be used
even if the work re-queues itself".
cancel_delayed_work_sync() calls the same underlying function but 
with WORK_CANCEL_DELAYED flag. So it should be OK.

Thanks,
- Haiyang