[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <716b433e-5e0c-4353-ea39-12cb4f3d50c4@omp.ru>
Date: Mon, 27 Nov 2023 20:16:03 +0300
From: Sergey Shtylyov <s.shtylyov@....ru>
To: Claudiu <claudiu.beznea@...on.dev>, <davem@...emloft.net>,
<edumazet@...gle.com>, <kuba@...nel.org>, <pabeni@...hat.com>,
<richardcochran@...il.com>, <p.zabel@...gutronix.de>,
<yoshihiro.shimoda.uh@...esas.com>, <geert+renesas@...der.be>,
<wsa+renesas@...g-engineering.com>, <robh@...nel.org>,
<biju.das.jz@...renesas.com>, <prabhakar.mahadev-lad.rj@...renesas.com>,
<mitsuhiro.kimura.kc@...esas.com>, <masaru.nagai.vx@...esas.com>
CC: <netdev@...r.kernel.org>, <linux-renesas-soc@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, Claudiu Beznea
<claudiu.beznea.uj@...renesas.com>
Subject: Re: [PATCH 6/6] net: ravb: Keep reverse order of operations in
ravb_remove()
On 11/27/23 12:04 PM, Claudiu wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@...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@...renesas.com>
Reviewed-by: Sergey Shtylyov <s.shtylyov@....ru>
[...]
Sorry for overlooking this race (and other bugs) when prepping
the driver for upstream!
MBR, Sergey
Powered by blists - more mailing lists