Refactor module parsing function. #368

d0u9 · 2021-06-09T02:33:01Z

The logic of parsing a module macro is a little bit cumbersome and
error-prone. Refactor this part to make it clear, concise, structural
and extensible.

Signed-off-by: Douglas Su [email protected]

d0u9 · 2021-06-09T02:37:37Z

Rerun request :)

nbdd0121 · 2021-06-09T02:39:55Z

The CI failed legitimately. Clippy complains that the code is too complex (which I agree, I think the current approach is easier to follow).

d0u9 · 2021-06-09T02:45:34Z

REQUIRED_KEYS are subset of EXPECTED_KEYS, I think we have to do something on this. It works but very redundant. I will think this twice.

d0u9 · 2021-06-09T03:40:27Z

Make some updates. Merge REQUIRED_KEYS and EXPECTED_KEYS into one and tag required keys by a bool variable.

rust/macros/module.rs

nbdd0121 · 2021-06-09T04:17:55Z

rust/macros/module.rs

+                    ALL_KEYS
+                        .iter()
+                        .map(|e| e.1)
+                        .collect::<Vec<&'static str>>()


nit: you don't have to spell out the full type here. Just write Vec<_> and compiler will infer that it's a Vec<&'static str>.

ojeda · 2021-06-09T05:10:23Z

I also think the changes do not particularly reduce complexity, but since I wrote the current ones, I am probably biased :) So I let others decide what looks best.

ojeda · 2021-06-09T05:12:48Z

By the way, the kernel cannot accept anonymous contributions -- please see Documentation/process (e.g. Documentation/process/1.Intro.rst).

So please use your real name in the Signed-off-by and your Git info.

ojeda · 2021-06-09T05:20:17Z

I am adding a check to @ksquirrel to detect single words in the author, another one to detect numbers too since those are likely to be a nickname and another one to detect all lowercase and all uppercase names, since those are also likely a nickname.

d0u9 · 2021-06-09T06:30:41Z

So please use your real name in the Signed-off-by and your Git info.

Have updated commit info.

So I let others decide what looks best.

:) Sounds good.

ojeda · 2021-06-09T06:32:26Z

Thanks! The commit looks fine now :)

ojeda · 2021-06-09T06:36:48Z

Network failure... re-trying.

The logic of parsing a module macro is a little bit cumbersome and error-prone. Refactor this part to make it clear, concise, structural and extensible. Signed-off-by: Douglas Su <[email protected]>

ksquirrel · 2021-06-10T10:30:46Z

Review of 7ee4769e1ea9:

✔️ Commit 7ee4769: Looks fine!

PeterWrighten

I think it's a pretty good refactor, and expect to see it would be merged.

macb_set_tx_clk() is called under a spinlock but itself calls clk_set_rate() which can sleep. This results in: | BUG: sleeping function called from invalid context at kernel/locking/mutex.c:580 | pps pps1: new PPS source ptp1 | in_atomic(): 1, irqs_disabled(): 1, non_block: 0, pid: 40, name: kworker/u4:3 | preempt_count: 1, expected: 0 | RCU nest depth: 0, expected: 0 | 4 locks held by kworker/u4:3/40: | #0: ffff000003409148 | macb ff0c000.ethernet: gem-ptp-timer ptp clock registered. | ((wq_completion)events_power_efficient){+.+.}-{0:0}, at: process_one_work+0x14c/0x51c | #1: ffff8000833cbdd8 ((work_completion)(&pl->resolve)){+.+.}-{0:0}, at: process_one_work+0x14c/0x51c | #2: ffff000004f01578 (&pl->state_mutex){+.+.}-{4:4}, at: phylink_resolve+0x44/0x4e8 | #3: ffff000004f06f50 (&bp->lock){....}-{3:3}, at: macb_mac_link_up+0x40/0x2ac | irq event stamp: 113998 | hardirqs last enabled at (113997): [<ffff800080e8503c>] _raw_spin_unlock_irq+0x30/0x64 | hardirqs last disabled at (113998): [<ffff800080e84478>] _raw_spin_lock_irqsave+0xac/0xc8 | softirqs last enabled at (113608): [<ffff800080010630>] __do_softirq+0x430/0x4e4 | softirqs last disabled at (113597): [<ffff80008001614c>] ____do_softirq+0x10/0x1c | CPU: 0 PID: 40 Comm: kworker/u4:3 Not tainted 6.5.0-11717-g9355ce8b2f50-dirty #368 | Hardware name: ... ZynqMP ... (DT) | Workqueue: events_power_efficient phylink_resolve | Call trace: | dump_backtrace+0x98/0xf0 | show_stack+0x18/0x24 | dump_stack_lvl+0x60/0xac | dump_stack+0x18/0x24 | __might_resched+0x144/0x24c | __might_sleep+0x48/0x98 | __mutex_lock+0x58/0x7b0 | mutex_lock_nested+0x24/0x30 | clk_prepare_lock+0x4c/0xa8 | clk_set_rate+0x24/0x8c | macb_mac_link_up+0x25c/0x2ac | phylink_resolve+0x178/0x4e8 | process_one_work+0x1ec/0x51c | worker_thread+0x1ec/0x3e4 | kthread+0x120/0x124 | ret_from_fork+0x10/0x20 The obvious fix is to move the call to macb_set_tx_clk() out of the protected area. This seems safe as rx and tx are both disabled anyway at this point. It is however not entirely clear what the spinlock shall protect. It could be the read-modify-write access to the NCFGR register, but this is accessed in macb_set_rx_mode() and macb_set_rxcsum_feature() as well without holding the spinlock. It could also be the register accesses done in mog_init_rings() or macb_init_buffers(), but again these functions are called without holding the spinlock in macb_hresp_error_task(). The locking seems fishy in this driver and it might deserve another look before this patch is applied. Fixes: 633e98a ("net: macb: use resolved link config in mac_link_up()") Signed-off-by: Sascha Hauer <[email protected]> Link: https://lore.kernel.org/r/[email protected] Signed-off-by: Paolo Abeni <[email protected]>