* [bug report] net: macb: convert to phylink
@ 2023-01-17 13:58 Dan Carpenter
2023-01-17 15:31 ` Antoine Tenart
0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2023-01-17 13:58 UTC (permalink / raw)
To: atenart; +Cc: linux-clk
Hello Antoine Tenart,
The patch 7897b071ac3b: "net: macb: convert to phylink" from Nov 13,
2019, leads to the following Smatch static checker warning:
drivers/clk/clk.c:133 clk_prepare_lock()
warn: sleeping in atomic context
drivers/clk/clk.c
126 static void clk_prepare_lock(void)
127 {
128 if (!mutex_trylock(&prepare_lock)) {
129 if (prepare_owner == current) {
130 prepare_refcnt++;
131 return;
132 }
--> 133 mutex_lock(&prepare_lock);
134 }
135 WARN_ON_ONCE(prepare_owner != NULL);
136 WARN_ON_ONCE(prepare_refcnt != 0);
137 prepare_owner = current;
138 prepare_refcnt = 1;
139 }
The problem is that cadence/macb_main calls clk_set_rate() while holding
a spinlock.
The call tree is:
owl_uart_set_termios() <- disables preempt
-> owl_uart_change_baudrate()
rda_uart_set_termios() <- disables preempt
-> rda_uart_change_baudrate()
atmel_set_termios() <- disables preempt
macb_mac_link_up() <- disables preempt (THIS ONE)
-> macb_set_tx_clk()
-> clk_set_rate()
-> clk_prepare_lock()
drivers/net/ethernet/cadence/macb_main.c
666 static void macb_mac_link_up(struct phylink_config *config,
667 struct phy_device *phy,
668 unsigned int mode, phy_interface_t interface,
669 int speed, int duplex,
670 bool tx_pause, bool rx_pause)
671 {
672 struct net_device *ndev = to_net_dev(config->dev);
673 struct macb *bp = netdev_priv(ndev);
674 struct macb_queue *queue;
675 unsigned long flags;
676 unsigned int q;
677 u32 ctrl;
678
679 spin_lock_irqsave(&bp->lock, flags);
^^^^^^^^^^^^^^^^^
Spin lock.
680
681 ctrl = macb_or_gem_readl(bp, NCFGR);
682
683 ctrl &= ~(MACB_BIT(SPD) | MACB_BIT(FD));
684
685 if (speed == SPEED_100)
686 ctrl |= MACB_BIT(SPD);
687
688 if (duplex)
689 ctrl |= MACB_BIT(FD);
690
691 if (!(bp->caps & MACB_CAPS_MACB_IS_EMAC)) {
692 ctrl &= ~MACB_BIT(PAE);
693 if (macb_is_gem(bp)) {
694 ctrl &= ~GEM_BIT(GBE);
695
696 if (speed == SPEED_1000)
697 ctrl |= GEM_BIT(GBE);
698 }
699
700 if (rx_pause)
701 ctrl |= MACB_BIT(PAE);
702
703 macb_set_tx_clk(bp, speed);
^^^^^^^^^^^^^^^^^^^^^^^^^^^
Takes a mutex.
704
705 /* Initialize rings & buffers as clearing MACB_BIT(TE) in link down
706 * cleared the pipeline and control registers.
707 */
708 bp->macbgem_ops.mog_init_rings(bp);
709 macb_init_buffers(bp);
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [bug report] net: macb: convert to phylink
2023-01-17 13:58 [bug report] net: macb: convert to phylink Dan Carpenter
@ 2023-01-17 15:31 ` Antoine Tenart
2023-01-18 8:36 ` Dan Carpenter
0 siblings, 1 reply; 3+ messages in thread
From: Antoine Tenart @ 2023-01-17 15:31 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-clk
Quoting Dan Carpenter (2023-01-17 14:58:12)
>
> The patch 7897b071ac3b: "net: macb: convert to phylink" from Nov 13,
> 2019, leads to the following Smatch static checker warning:
I don't think the above commit is the right one, macb_set_tx_clk wasn't
called while holding a spinlock at the time. This behaviour seemed to
have been introduced by commit 633e98a711ac ("net: macb: use resolved
link config in mac_link_up()").
> drivers/clk/clk.c:133 clk_prepare_lock()
> warn: sleeping in atomic context
>
> drivers/clk/clk.c
> 126 static void clk_prepare_lock(void)
> 127 {
> 128 if (!mutex_trylock(&prepare_lock)) {
> 129 if (prepare_owner == current) {
> 130 prepare_refcnt++;
> 131 return;
> 132 }
> --> 133 mutex_lock(&prepare_lock);
> 134 }
> 135 WARN_ON_ONCE(prepare_owner != NULL);
> 136 WARN_ON_ONCE(prepare_refcnt != 0);
> 137 prepare_owner = current;
> 138 prepare_refcnt = 1;
> 139 }
>
> The problem is that cadence/macb_main calls clk_set_rate() while holding
> a spinlock.
>
> The call tree is:
>
> owl_uart_set_termios() <- disables preempt
> -> owl_uart_change_baudrate()
> rda_uart_set_termios() <- disables preempt
> -> rda_uart_change_baudrate()
> atmel_set_termios() <- disables preempt
> macb_mac_link_up() <- disables preempt (THIS ONE)
> -> macb_set_tx_clk()
> -> clk_set_rate()
> -> clk_prepare_lock()
>
> drivers/net/ethernet/cadence/macb_main.c
> 666 static void macb_mac_link_up(struct phylink_config *config,
> 667 struct phy_device *phy,
> 668 unsigned int mode, phy_interface_t interface,
> 669 int speed, int duplex,
> 670 bool tx_pause, bool rx_pause)
> 671 {
> 672 struct net_device *ndev = to_net_dev(config->dev);
> 673 struct macb *bp = netdev_priv(ndev);
> 674 struct macb_queue *queue;
> 675 unsigned long flags;
> 676 unsigned int q;
> 677 u32 ctrl;
> 678
> 679 spin_lock_irqsave(&bp->lock, flags);
> ^^^^^^^^^^^^^^^^^
> Spin lock.
>
> 680
> 681 ctrl = macb_or_gem_readl(bp, NCFGR);
> 682
> 683 ctrl &= ~(MACB_BIT(SPD) | MACB_BIT(FD));
> 684
> 685 if (speed == SPEED_100)
> 686 ctrl |= MACB_BIT(SPD);
> 687
> 688 if (duplex)
> 689 ctrl |= MACB_BIT(FD);
> 690
> 691 if (!(bp->caps & MACB_CAPS_MACB_IS_EMAC)) {
> 692 ctrl &= ~MACB_BIT(PAE);
> 693 if (macb_is_gem(bp)) {
> 694 ctrl &= ~GEM_BIT(GBE);
> 695
> 696 if (speed == SPEED_1000)
> 697 ctrl |= GEM_BIT(GBE);
> 698 }
> 699
> 700 if (rx_pause)
> 701 ctrl |= MACB_BIT(PAE);
> 702
> 703 macb_set_tx_clk(bp, speed);
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Takes a mutex.
>
> 704
> 705 /* Initialize rings & buffers as clearing MACB_BIT(TE) in link down
> 706 * cleared the pipeline and control registers.
> 707 */
> 708 bp->macbgem_ops.mog_init_rings(bp);
> 709 macb_init_buffers(bp);
>
> regards,
> dan carpenter
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [bug report] net: macb: convert to phylink
2023-01-17 15:31 ` Antoine Tenart
@ 2023-01-18 8:36 ` Dan Carpenter
0 siblings, 0 replies; 3+ messages in thread
From: Dan Carpenter @ 2023-01-18 8:36 UTC (permalink / raw)
To: Antoine Tenart; +Cc: linux-clk
On Tue, Jan 17, 2023 at 04:31:29PM +0100, Antoine Tenart wrote:
> Quoting Dan Carpenter (2023-01-17 14:58:12)
> >
> > The patch 7897b071ac3b: "net: macb: convert to phylink" from Nov 13,
> > 2019, leads to the following Smatch static checker warning:
>
> I don't think the above commit is the right one, macb_set_tx_clk wasn't
> called while holding a spinlock at the time. This behaviour seemed to
> have been introduced by commit 633e98a711ac ("net: macb: use resolved
> link config in mac_link_up()").
>
Oh, yeah. You're right. Let me send this to Russell.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-01-18 9:26 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-01-17 13:58 [bug report] net: macb: convert to phylink Dan Carpenter
2023-01-17 15:31 ` Antoine Tenart
2023-01-18 8:36 ` Dan Carpenter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).