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

[Andrew Lunn <andrew@lunn.ch>]

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