> -----Original Message-----
> From: Andrew Lunn <[email protected]>
> Sent: Monday, October 13, 2025 5:13 PM
> To: Haiyang Zhang <[email protected]>
> Cc: [email protected]; [email protected]; Haiyang Zhang
> <[email protected]>; Paul Rosswurm <[email protected]>; Dexuan
> Cui <[email protected]>; KY Srinivasan <[email protected]>;
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; Long Li <[email protected]>;
> [email protected]; [email protected];
> [email protected]; Konstantin Taranov
> <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; Shiraz Saleem <[email protected]>;
> [email protected]; [email protected]; linux-
> [email protected]
> 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
