[PATCH] net: mscc: ocelot: replace readx_poll_timeout with readx_poll_timeout_atomic

[PATCH] net: mscc: ocelot: replace readx_poll_timeout with readx_poll_timeout_atomic

[Xulin Sun]

This fixes call trace like below to use atomic safe API:

BUG: sleeping function called from invalid context at drivers/net/ethernet/mscc/ocelot.c:59
in_atomic(): 1, irqs_disabled(): 0, pid: 3778, name: ifconfig
INFO: lockdep is turned off.
Preemption disabled at:
[<ffff2b163c83b78c>] dev_set_rx_mode+0x24/0x40
Hardware name: LS1028A RDB Board (DT)
Call trace:
dump_backtrace+0x0/0x140
show_stack+0x24/0x30
dump_stack+0xc4/0x10c
___might_sleep+0x194/0x230
__might_sleep+0x58/0x90
ocelot_mact_forget+0x74/0xf8
ocelot_mc_unsync+0x2c/0x38
__hw_addr_sync_dev+0x6c/0x130
ocelot_set_rx_mode+0x8c/0xa0
__dev_set_rx_mode+0x58/0xa0
dev_set_rx_mode+0x2c/0x40
__dev_open+0x120/0x190
__dev_change_flags+0x168/0x1c0
dev_change_flags+0x3c/0x78
devinet_ioctl+0x6c4/0x7c8
inet_ioctl+0x2b8/0x2f8
sock_do_ioctl+0x54/0x260
sock_ioctl+0x21c/0x4d0
do_vfs_ioctl+0x6d4/0x968
ksys_ioctl+0x84/0xb8
__arm64_sys_ioctl+0x28/0x38
el0_svc_common.constprop.0+0x78/0x190
el0_svc_handler+0x70/0x90
el0_svc+0x8/0xc

Signed-off-by: Xulin Sun <xulin.sun@windriver.com>
---
 drivers/net/ethernet/mscc/ocelot.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
index b4731df186f4..6c82ab1b3fa6 100644
--- a/drivers/net/ethernet/mscc/ocelot.c
+++ b/drivers/net/ethernet/mscc/ocelot.c
@@ -56,7 +56,7 @@ static inline int ocelot_mact_wait_for_completion(struct ocelot *ocelot)
 {
 	u32 val;
 
-	return readx_poll_timeout(ocelot_mact_read_macaccess,
+	return readx_poll_timeout_atomic(ocelot_mact_read_macaccess,
 		ocelot, val,
 		(val & ANA_TABLES_MACACCESS_MAC_TABLE_CMD_M) ==
 		MACACCESS_CMD_IDLE,
-- 
2.17.1

Re: [PATCH] net: mscc: ocelot: replace readx_poll_timeout with readx_poll_timeout_atomic

[Andrew Lunn]

On Fri, May 15, 2020 at 11:18:13AM +0800, Xulin Sun wrote:
> This fixes call trace like below to use atomic safe API:
> 
> BUG: sleeping function called from invalid context at drivers/net/ethernet/mscc/ocelot.c:59
> in_atomic(): 1, irqs_disabled(): 0, pid: 3778, name: ifconfig
> INFO: lockdep is turned off.
> Preemption disabled at:
> [<ffff2b163c83b78c>] dev_set_rx_mode+0x24/0x40
> Hardware name: LS1028A RDB Board (DT)
> Call trace:
> dump_backtrace+0x0/0x140
> show_stack+0x24/0x30
> dump_stack+0xc4/0x10c
> ___might_sleep+0x194/0x230
> __might_sleep+0x58/0x90
> ocelot_mact_forget+0x74/0xf8
> ocelot_mc_unsync+0x2c/0x38
> __hw_addr_sync_dev+0x6c/0x130
> ocelot_set_rx_mode+0x8c/0xa0
> __dev_set_rx_mode+0x58/0xa0
> dev_set_rx_mode+0x2c/0x40
> __dev_open+0x120/0x190
> __dev_change_flags+0x168/0x1c0
> dev_change_flags+0x3c/0x78
> devinet_ioctl+0x6c4/0x7c8
> inet_ioctl+0x2b8/0x2f8
> sock_do_ioctl+0x54/0x260
> sock_ioctl+0x21c/0x4d0
> do_vfs_ioctl+0x6d4/0x968
> ksys_ioctl+0x84/0xb8
> __arm64_sys_ioctl+0x28/0x38
> el0_svc_common.constprop.0+0x78/0x190
> el0_svc_handler+0x70/0x90
> el0_svc+0x8/0xc
> 
> Signed-off-by: Xulin Sun <xulin.sun@windriver.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH] net: mscc: ocelot: replace readx_poll_timeout with readx_poll_timeout_atomic

[Vladimir Oltean]

Hi Xulin,

On Fri, 15 May 2020 at 06:23, Xulin Sun <xulin.sun@windriver.com> wrote:
>
> This fixes call trace like below to use atomic safe API:
>
> BUG: sleeping function called from invalid context at drivers/net/ethernet/mscc/ocelot.c:59
> in_atomic(): 1, irqs_disabled(): 0, pid: 3778, name: ifconfig
> INFO: lockdep is turned off.
> Preemption disabled at:
> [<ffff2b163c83b78c>] dev_set_rx_mode+0x24/0x40
> Hardware name: LS1028A RDB Board (DT)
> Call trace:
> dump_backtrace+0x0/0x140
> show_stack+0x24/0x30
> dump_stack+0xc4/0x10c
> ___might_sleep+0x194/0x230
> __might_sleep+0x58/0x90
> ocelot_mact_forget+0x74/0xf8
> ocelot_mc_unsync+0x2c/0x38
> __hw_addr_sync_dev+0x6c/0x130
> ocelot_set_rx_mode+0x8c/0xa0
> __dev_set_rx_mode+0x58/0xa0
> dev_set_rx_mode+0x2c/0x40
> __dev_open+0x120/0x190
> __dev_change_flags+0x168/0x1c0
> dev_change_flags+0x3c/0x78
> devinet_ioctl+0x6c4/0x7c8
> inet_ioctl+0x2b8/0x2f8
> sock_do_ioctl+0x54/0x260
> sock_ioctl+0x21c/0x4d0
> do_vfs_ioctl+0x6d4/0x968
> ksys_ioctl+0x84/0xb8
> __arm64_sys_ioctl+0x28/0x38
> el0_svc_common.constprop.0+0x78/0x190
> el0_svc_handler+0x70/0x90
> el0_svc+0x8/0xc
>
> Signed-off-by: Xulin Sun <xulin.sun@windriver.com>
> ---

The code path which you presented in your stack trace is impossible
using code available currently in mainline.

>  drivers/net/ethernet/mscc/ocelot.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
> index b4731df186f4..6c82ab1b3fa6 100644
> --- a/drivers/net/ethernet/mscc/ocelot.c
> +++ b/drivers/net/ethernet/mscc/ocelot.c
> @@ -56,7 +56,7 @@ static inline int ocelot_mact_wait_for_completion(struct ocelot *ocelot)
>  {
>         u32 val;
>
> -       return readx_poll_timeout(ocelot_mact_read_macaccess,
> +       return readx_poll_timeout_atomic(ocelot_mact_read_macaccess,
>                 ocelot, val,
>                 (val & ANA_TABLES_MACACCESS_MAC_TABLE_CMD_M) ==
>                 MACACCESS_CMD_IDLE,
> --
> 2.17.1
>

Regards,
-Vladimir

Re: [PATCH] net: mscc: ocelot: replace readx_poll_timeout with readx_poll_timeout_atomic

[David Miller]

From: Xulin Sun <xulin.sun@windriver.com>
Date: Fri, 15 May 2020 11:18:13 +0800

> BUG: sleeping function called from invalid context at drivers/net/ethernet/mscc/ocelot.c:59
> in_atomic(): 1, irqs_disabled(): 0, pid: 3778, name: ifconfig
> INFO: lockdep is turned off.
> Preemption disabled at:
> [<ffff2b163c83b78c>] dev_set_rx_mode+0x24/0x40
> Hardware name: LS1028A RDB Board (DT)
> Call trace:
> dump_backtrace+0x0/0x140
> show_stack+0x24/0x30
> dump_stack+0xc4/0x10c
> ___might_sleep+0x194/0x230
> __might_sleep+0x58/0x90
> ocelot_mact_forget+0x74/0xf8
> ocelot_mc_unsync+0x2c/0x38
> __hw_addr_sync_dev+0x6c/0x130
> ocelot_set_rx_mode+0x8c/0xa0

Vladimir states that this call chain is not possible in mainline.

I'm not applying this.

Re: [PATCH] net: mscc: ocelot: replace readx_poll_timeout with readx_poll_timeout_atomic

[Vladimir Oltean]

On Sat, 16 May 2020 at 23:54, David Miller <davem@davemloft.net> wrote:
>
> From: Xulin Sun <xulin.sun@windriver.com>
> Date: Fri, 15 May 2020 11:18:13 +0800
>
> > BUG: sleeping function called from invalid context at drivers/net/ethernet/mscc/ocelot.c:59
> > in_atomic(): 1, irqs_disabled(): 0, pid: 3778, name: ifconfig
> > INFO: lockdep is turned off.
> > Preemption disabled at:
> > [<ffff2b163c83b78c>] dev_set_rx_mode+0x24/0x40
> > Hardware name: LS1028A RDB Board (DT)
> > Call trace:
> > dump_backtrace+0x0/0x140
> > show_stack+0x24/0x30
> > dump_stack+0xc4/0x10c
> > ___might_sleep+0x194/0x230
> > __might_sleep+0x58/0x90
> > ocelot_mact_forget+0x74/0xf8
> > ocelot_mc_unsync+0x2c/0x38
> > __hw_addr_sync_dev+0x6c/0x130
> > ocelot_set_rx_mode+0x8c/0xa0
>
> Vladimir states that this call chain is not possible in mainline.
>
> I'm not applying this.

(but the essence of the problem is legitimate though)

There are 2 specific things I don't like:
- The problem is claimed to reproduce on "LS1028A RDB Board (DT)"
which does not call ocelot_set_rx_mode. So it claims to fix a problem
for which only Xulin has the ability to decide whether it is the right
solution or not.
- On ocelot, it _looks_ like it is indeed a problem which was
introduced in 639c1b2625af ("net: mscc: ocelot: Register poll timeout
should be wall time not attempts"). But there was no attempt to bring
it up with the author of that patch, who very clearly expressed that
he is working on hardware where the polling timeout is in the order of
milliseconds, and the timeout for the driver is currently set at 100
ms. I'm not very sure that it is desirable to spin in atomic context
for 100 ms.

Thanks,
-Vladimir