Re: [PATCH 6/6] net: ravb: Keep reverse order of operations in ravb_remove()

[Sergey Shtylyov <s.shtylyov@omp.ru>]

On 11/27/23 12:04 PM, Claudiu wrote:

> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> On RZ/G3S SMARC Carrier II board having RGMII connections b/w Ethernet
> MACs and PHYs it has been discovered that doing unbind/bind for ravb
> driver in a loop leads to wrong speed and duplex for Ethernet links and
> broken connectivity (the connectivity cannot be restored even with
> bringing interface down/up). Before doing unbind/bind the Ethernet
> interfaces were configured though systemd. The sh instructions used to
> do unbind/bind were:
> 
> $ cd /sys/bus/platform/drivers/ravb/
> $ while :; do echo 11c30000.ethernet > unbind ; \
>   echo 11c30000.ethernet > bind; done
> 
> It has been discovered that there is a race b/w IOCTLs initialized by
> systemd at the response of success binding and the
> "ravb_write(ndev, CCC_OPC_RESET, CCC)" instruction in ravb_remove() as

   s/instruction/call/, perhaps?

> follows:
> 
> 1/ as a result of bind success the user space open/configures the
>    interfaces tough an IOCTL; the following stack trace has been
>    identified on RZ/G3S:
> 
> Call trace:
> dump_backtrace+0x9c/0x100
> show_stack+0x20/0x38
> dump_stack_lvl+0x48/0x60
> dump_stack+0x18/0x28
> ravb_open+0x70/0xa58
> __dev_open+0xf4/0x1e8
> __dev_change_flags+0x198/0x218
> dev_change_flags+0x2c/0x80
> devinet_ioctl+0x640/0x708
> inet_ioctl+0x1e4/0x200
> sock_do_ioctl+0x50/0x108
> sock_ioctl+0x240/0x358
> __arm64_sys_ioctl+0xb0/0x100
> invoke_syscall+0x50/0x128
> el0_svc_common.constprop.0+0xc8/0xf0
> do_el0_svc+0x24/0x38
> el0_svc+0x34/0xb8
> el0t_64_sync_handler+0xc0/0xc8
> el0t_64_sync+0x190/0x198
> 
> 2/ this call may execute concurrently with ravb_remove() as the
>    unbind/bind operation was executed in a loop
> 3/ if the operation mode is changed to RESET (though

   Through?

>    ravb_write(ndev, CCC_OPC_RESET, CCC) instruction in ravb_remove())

   s/instruction/call/, perhaps?

>    while the above ravb_open() is in progress it may lead to MAC
>    (or PHY, or MAC-PHY connection, the right point hasn't been identified
>    at the moment) to be broken, thus the Ethernet connectivity fails to
>    restore.
> 
> The simple fix for this is to move ravb_write(ndev, CCC_OPC_RESET, CCC))
> after unregister_netdev() to avoid resetting the controller while the
> netdev interface is still registered.
> 
> To avoid future issues in ravb_remove(), the patch follows the proper order
> of operations in ravb_remove(): reverse order compared with ravb_probe().
> This avoids described races as the IOCTLs as well as unregister_netdev()
> (called now at the beginning of ravb_remove()) calls rtnl_lock() before
> continuing and IOCTLs check (though devinet_ioctl()) if device is still
> registered just after taking the lock:
> 
> int devinet_ioctl(struct net *net, unsigned int cmd, struct ifreq *ifr)
> {
> 	// ...
> 
>         rtnl_lock();
> 
>         ret = -ENODEV;
>         dev = __dev_get_by_name(net, ifr->ifr_name);
>         if (!dev)
>                 goto done;
> 
> 	// ...
> done:
>         rtnl_unlock();
> out:
>         return ret;
> }
> 
> Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper")
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>

[...]

   Sorry for overlooking this race (and other bugs) when prepping
the driver for upstream!

MBR, Sergey