linux-kernel - 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: <74490632-68da-401d-89a7-3d937d63cbe3@lunn.ch>
Date: Mon, 13 Oct 2025 23:13:20 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Haiyang Zhang <haiyangz@...ux.microsoft.com>
Cc: linux-hyperv@...r.kernel.org, netdev@...r.kernel.org,
	haiyangz@...rosoft.com, paulros@...rosoft.com, decui@...rosoft.com,
	kys@...rosoft.com, wei.liu@...nel.org, edumazet@...gle.com,
	davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com,
	longli@...rosoft.com, ssengar@...ux.microsoft.com,
	ernis@...ux.microsoft.com, dipayanroy@...ux.microsoft.com,
	kotaranov@...rosoft.com, horms@...nel.org,
	shradhagupta@...ux.microsoft.com, leon@...nel.org,
	mlevitsk@...hat.com, yury.norov@...il.com,
	shirazsaleem@...rosoft.com, andrew+netdev@...n.ch,
	linux-rdma@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: 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.

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

> +
> +	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.

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

	Andrew