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

[Haiyang Zhang]

> -----Original Message-----
> From: Andrew Lunn <andrew@xxxxxxx>
> Sent: Monday, October 13, 2025 5:13 PM
> To: Haiyang Zhang <haiyangz@xxxxxxxxxxxxxxxxxxx>
> Cc: linux-hyperv@xxxxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; Haiyang Zhang
> <haiyangz@xxxxxxxxxxxxx>; Paul Rosswurm <paulros@xxxxxxxxxxxxx>; Dexuan
> Cui <decui@xxxxxxxxxxxxx>; KY Srinivasan <kys@xxxxxxxxxxxxx>;
> wei.liu@xxxxxxxxxx; edumazet@xxxxxxxxxx; davem@xxxxxxxxxxxxx;
> kuba@xxxxxxxxxx; pabeni@xxxxxxxxxx; Long Li <longli@xxxxxxxxxxxxx>;
> ssengar@xxxxxxxxxxxxxxxxxxx; ernis@xxxxxxxxxxxxxxxxxxx;
> dipayanroy@xxxxxxxxxxxxxxxxxxx; Konstantin Taranov
> <kotaranov@xxxxxxxxxxxxx>; horms@xxxxxxxxxx;
> shradhagupta@xxxxxxxxxxxxxxxxxxx; leon@xxxxxxxxxx; mlevitsk@xxxxxxxxxx;
> yury.norov@xxxxxxxxx; Shiraz Saleem <shirazsaleem@xxxxxxxxxxxxx>;
> andrew+netdev@xxxxxxx; linux-rdma@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx
> 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