Hello!
On 10/17/23 11:53 AM, Yoshihiro Shimoda wrote:
> Fix races between ravb_tx_timeout_work() and functions of net_device_ops
> and ethtool_ops by using rtnl_trylock() and rtnl_unlock(). Note that
> since ravb_close() is under the rtnl lock and calls cancel_work_sync(),
> ravb_tx_timeout_work() calls rtnl_trylock() to avoid a deadlock.
I don't quite follow... how calling cancel_work_sync() is a problem?
I thought the problem was that unregister_netdev() can be called with
the TX timeout work still pending? And, more generally, shouldn't we
protect against the TX timeout work being executed on a different CPU
than the {net_device|ethtool}_ops methods are being executed (is that
possible?)?
I also had a suspicion that we still miss the spinlock calls in
ravb_tx_timeout_work() but nothing in particular jumped at me...
mind looking into that? :-)
> Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper")
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@xxxxxxxxxxx>
> ---
> drivers/net/ethernet/renesas/ravb_main.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 0ef0b88b7145..b53533ab4599 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
[...]
> @@ -1907,6 +1910,7 @@ static void ravb_tx_timeout_work(struct work_struct *work)
> */
> netdev_err(ndev, "%s: ravb_dmac_init() failed, error %d\n",
> __func__, error);
> + rtnl_unlock();
> return;
Perhaps *goto* instead here?
> }
> ravb_emac_init(ndev);
> @@ -1917,6 +1921,7 @@ static void ravb_tx_timeout_work(struct work_struct *work)
> ravb_ptp_init(ndev, priv->pdev);
>
> netif_tx_start_all_queues(ndev);
... and add label here?
> + rtnl_unlock();
> }
MBR, Sergey
