| Message ID | ded9d793-83f8-4f11-87d9-a218d10c2981@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] r8169: fix LED-related deadlock on module removal | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net |
| netdev/apply | success | Patch already applied to net-0 |
On Mon, 15 Apr 2024 13:57:17 +0200 Heiner Kallweit wrote: > Binding devm_led_classdev_register() to the netdev is problematic > because on module removal we get a RTNL-related deadlock. Fix this > by avoiding the device-managed LED functions. > > Note: We can safely call led_classdev_unregister() for a LED even > if registering it failed, because led_classdev_unregister() detects > this and is a no-op in this case. > > Fixes: 18764b883e15 ("r8169: add support for LED's on RTL8168/RTL8101") > Cc: <stable@vger.kernel.org> # 6.8.x > Reported-by: Lukas Wunner <lukas@wunner.de> > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> Looks like I already applied one chunk of this as commit 97e176fcbbf3 ("r8169: add missing conditional compiling for call to r8169_remove_leds") Is it worth throwing that in as a Fixes tag?
On 17.04.2024 04:34, Jakub Kicinski wrote: > On Mon, 15 Apr 2024 13:57:17 +0200 Heiner Kallweit wrote: >> Binding devm_led_classdev_register() to the netdev is problematic >> because on module removal we get a RTNL-related deadlock. Fix this >> by avoiding the device-managed LED functions. >> >> Note: We can safely call led_classdev_unregister() for a LED even >> if registering it failed, because led_classdev_unregister() detects >> this and is a no-op in this case. >> >> Fixes: 18764b883e15 ("r8169: add support for LED's on RTL8168/RTL8101") >> Cc: <stable@vger.kernel.org> # 6.8.x >> Reported-by: Lukas Wunner <lukas@wunner.de> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > > Looks like I already applied one chunk of this as commit 97e176fcbbf3 > ("r8169: add missing conditional compiling for call to r8169_remove_leds") > Is it worth throwing that in as a Fixes tag? This is a version of the fix modified to apply on 6.8. It's not supposed to be applied on net / net-next. Should I have sent it to stable@vger.kernel.org only?
On Wed, Apr 17, 2024 at 08:02:31AM +0200, Heiner Kallweit wrote: > On 17.04.2024 04:34, Jakub Kicinski wrote: > > On Mon, 15 Apr 2024 13:57:17 +0200 Heiner Kallweit wrote: > >> Binding devm_led_classdev_register() to the netdev is problematic > >> because on module removal we get a RTNL-related deadlock. Fix this > >> by avoiding the device-managed LED functions. > >> > >> Note: We can safely call led_classdev_unregister() for a LED even > >> if registering it failed, because led_classdev_unregister() detects > >> this and is a no-op in this case. > >> > >> Fixes: 18764b883e15 ("r8169: add support for LED's on RTL8168/RTL8101") > >> Cc: <stable@vger.kernel.org> # 6.8.x > >> Reported-by: Lukas Wunner <lukas@wunner.de> > >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > > > > Looks like I already applied one chunk of this as commit 97e176fcbbf3 > > ("r8169: add missing conditional compiling for call to r8169_remove_leds") > > Is it worth throwing that in as a Fixes tag? > > This is a version of the fix modified to apply on 6.8. That was not obvious at all :( > It's not supposed to be applied on net / net-next. > Should I have sent it to stable@vger.kernel.org only? Why woudlu a commit only be relevent for older kernels and not the latest one? thanks, greg k-h
On 17.04.2024 09:04, Greg KH wrote: > On Wed, Apr 17, 2024 at 08:02:31AM +0200, Heiner Kallweit wrote: >> On 17.04.2024 04:34, Jakub Kicinski wrote: >>> On Mon, 15 Apr 2024 13:57:17 +0200 Heiner Kallweit wrote: >>>> Binding devm_led_classdev_register() to the netdev is problematic >>>> because on module removal we get a RTNL-related deadlock. Fix this >>>> by avoiding the device-managed LED functions. >>>> >>>> Note: We can safely call led_classdev_unregister() for a LED even >>>> if registering it failed, because led_classdev_unregister() detects >>>> this and is a no-op in this case. >>>> >>>> Fixes: 18764b883e15 ("r8169: add support for LED's on RTL8168/RTL8101") >>>> Cc: <stable@vger.kernel.org> # 6.8.x >>>> Reported-by: Lukas Wunner <lukas@wunner.de> >>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >>> >>> Looks like I already applied one chunk of this as commit 97e176fcbbf3 >>> ("r8169: add missing conditional compiling for call to r8169_remove_leds") >>> Is it worth throwing that in as a Fixes tag? >> >> This is a version of the fix modified to apply on 6.8. > > That was not obvious at all :( > Stating "Cc: <stable@vger.kernel.org> # 6.8.x" isn't sufficient? >> It's not supposed to be applied on net / net-next. >> Should I have sent it to stable@vger.kernel.org only? > > Why woudlu a commit only be relevent for older kernels and not the > latest one? > The fix version for 6.9-rc and next has been applied already. > thanks, > > greg k-h
On Wed, Apr 17, 2024 at 09:16:04AM +0200, Heiner Kallweit wrote: > On 17.04.2024 09:04, Greg KH wrote: > > On Wed, Apr 17, 2024 at 08:02:31AM +0200, Heiner Kallweit wrote: > >> On 17.04.2024 04:34, Jakub Kicinski wrote: > >>> On Mon, 15 Apr 2024 13:57:17 +0200 Heiner Kallweit wrote: > >>>> Binding devm_led_classdev_register() to the netdev is problematic > >>>> because on module removal we get a RTNL-related deadlock. Fix this > >>>> by avoiding the device-managed LED functions. > >>>> > >>>> Note: We can safely call led_classdev_unregister() for a LED even > >>>> if registering it failed, because led_classdev_unregister() detects > >>>> this and is a no-op in this case. > >>>> > >>>> Fixes: 18764b883e15 ("r8169: add support for LED's on RTL8168/RTL8101") > >>>> Cc: <stable@vger.kernel.org> # 6.8.x > >>>> Reported-by: Lukas Wunner <lukas@wunner.de> > >>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > >>> > >>> Looks like I already applied one chunk of this as commit 97e176fcbbf3 > >>> ("r8169: add missing conditional compiling for call to r8169_remove_leds") > >>> Is it worth throwing that in as a Fixes tag? > >> > >> This is a version of the fix modified to apply on 6.8. > > > > That was not obvious at all :( > > > Stating "Cc: <stable@vger.kernel.org> # 6.8.x" isn't sufficient? Without showing what commit id this is in Linus's tree, no. > >> It's not supposed to be applied on net / net-next. > >> Should I have sent it to stable@vger.kernel.org only? > > > > Why woudlu a commit only be relevent for older kernels and not the > > latest one? > > > The fix version for 6.9-rc and next has been applied already. Then a hint as to what the git id of that commit is would help out a lot here. Thanks, greg k-h
On Wed, 17 Apr 2024 08:02:31 +0200 Heiner Kallweit wrote: > > Looks like I already applied one chunk of this as commit 97e176fcbbf3 > > ("r8169: add missing conditional compiling for call to r8169_remove_leds") > > Is it worth throwing that in as a Fixes tag? > > This is a version of the fix modified to apply on 6.8. > It's not supposed to be applied on net / net-next. > Should I have sent it to stable@vger.kernel.org only? Ah! I'm not sure what the right way is, either, TBH, but I'd have put [PATCH 6.8] rather than [PATCH net] in that case.
On Wed, Apr 17, 2024 at 09:43:27AM +0200, Greg KH wrote: > On Wed, Apr 17, 2024 at 09:16:04AM +0200, Heiner Kallweit wrote: > > On 17.04.2024 09:04, Greg KH wrote: > > > On Wed, Apr 17, 2024 at 08:02:31AM +0200, Heiner Kallweit wrote: > > >> On 17.04.2024 04:34, Jakub Kicinski wrote: > > >>> On Mon, 15 Apr 2024 13:57:17 +0200 Heiner Kallweit wrote: > > >>>> Binding devm_led_classdev_register() to the netdev is problematic > > >>>> because on module removal we get a RTNL-related deadlock. Fix this > > >>>> by avoiding the device-managed LED functions. > > >>>> > > >>>> Note: We can safely call led_classdev_unregister() for a LED even > > >>>> if registering it failed, because led_classdev_unregister() detects > > >>>> this and is a no-op in this case. > > >>>> > > >>>> Fixes: 18764b883e15 ("r8169: add support for LED's on RTL8168/RTL8101") > > >>>> Cc: <stable@vger.kernel.org> # 6.8.x > > >>>> Reported-by: Lukas Wunner <lukas@wunner.de> > > >>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > > >> > > >> This is a version of the fix modified to apply on 6.8. > > > > > > That was not obvious at all :( > > > > > Stating "Cc: <stable@vger.kernel.org> # 6.8.x" isn't sufficient? > > Without showing what commit id this is in Linus's tree, no. The upstream commit id *is* called out in the patch, but it's buried below the three dashes: The original change was introduced with 6.8, 6.9 added support for LEDs on RTL8125. Therefore the first version of the fix applied on 6.9-rc only. This is the modified version for 6.8. Upstream commit: 19fa4f2a85d7 ^^^^^^^^^^^^ The proper way to do this is to prominently add ... commit 19fa4f2a85d777a8052e869c1b892a2f7556569d upstream. ... or ... [ Upstream commit 19fa4f2a85d777a8052e869c1b892a2f7556569d ] ... as the first line of the commit message, as per Documentation/process/stable-kernel-rules.rst
On Thu, Apr 18, 2024 at 12:33:00AM +0200, Lukas Wunner wrote: > On Wed, Apr 17, 2024 at 09:43:27AM +0200, Greg KH wrote: > > On Wed, Apr 17, 2024 at 09:16:04AM +0200, Heiner Kallweit wrote: > > > On 17.04.2024 09:04, Greg KH wrote: > > > > On Wed, Apr 17, 2024 at 08:02:31AM +0200, Heiner Kallweit wrote: > > > >> On 17.04.2024 04:34, Jakub Kicinski wrote: > > > >>> On Mon, 15 Apr 2024 13:57:17 +0200 Heiner Kallweit wrote: > > > >>>> Binding devm_led_classdev_register() to the netdev is problematic > > > >>>> because on module removal we get a RTNL-related deadlock. Fix this > > > >>>> by avoiding the device-managed LED functions. > > > >>>> > > > >>>> Note: We can safely call led_classdev_unregister() for a LED even > > > >>>> if registering it failed, because led_classdev_unregister() detects > > > >>>> this and is a no-op in this case. > > > >>>> > > > >>>> Fixes: 18764b883e15 ("r8169: add support for LED's on RTL8168/RTL8101") > > > >>>> Cc: <stable@vger.kernel.org> # 6.8.x > > > >>>> Reported-by: Lukas Wunner <lukas@wunner.de> > > > >>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > > > >> > > > >> This is a version of the fix modified to apply on 6.8. > > > > > > > > That was not obvious at all :( > > > > > > > Stating "Cc: <stable@vger.kernel.org> # 6.8.x" isn't sufficient? > > > > Without showing what commit id this is in Linus's tree, no. > > The upstream commit id *is* called out in the patch, but it's buried > below the three dashes: > > The original change was introduced with 6.8, 6.9 added support for > LEDs on RTL8125. Therefore the first version of the fix applied on > 6.9-rc only. This is the modified version for 6.8. > Upstream commit: 19fa4f2a85d7 > ^^^^^^^^^^^^ > > The proper way to do this is to prominently add ... > > commit 19fa4f2a85d777a8052e869c1b892a2f7556569d upstream. > > ... or ... > > [ Upstream commit 19fa4f2a85d777a8052e869c1b892a2f7556569d ] > > ... as the first line of the commit message, as per > Documentation/process/stable-kernel-rules.rst > Yes, Heiner, please resubmit this, AND submit the fix-for-this-fix as well, so that if we take this patch, it is not broken. thanks, greg k-hj
On Thu, Apr 18, 2024 at 11:55 AM Greg KH <gregkh@linuxfoundation.org> wrote: > > On Thu, Apr 18, 2024 at 12:33:00AM +0200, Lukas Wunner wrote: > > On Wed, Apr 17, 2024 at 09:43:27AM +0200, Greg KH wrote: > > > On Wed, Apr 17, 2024 at 09:16:04AM +0200, Heiner Kallweit wrote: > > > > On 17.04.2024 09:04, Greg KH wrote: > > > > > On Wed, Apr 17, 2024 at 08:02:31AM +0200, Heiner Kallweit wrote: > > > > >> On 17.04.2024 04:34, Jakub Kicinski wrote: > > > > >>> On Mon, 15 Apr 2024 13:57:17 +0200 Heiner Kallweit wrote: > > > > >>>> Binding devm_led_classdev_register() to the netdev is problematic > > > > >>>> because on module removal we get a RTNL-related deadlock. Fix this > > > > >>>> by avoiding the device-managed LED functions. > > > > >>>> > > > > >>>> Note: We can safely call led_classdev_unregister() for a LED even > > > > >>>> if registering it failed, because led_classdev_unregister() detects > > > > >>>> this and is a no-op in this case. > > > > >>>> > > > > >>>> Fixes: 18764b883e15 ("r8169: add support for LED's on RTL8168/RTL8101") > > > > >>>> Cc: <stable@vger.kernel.org> # 6.8.x > > > > >>>> Reported-by: Lukas Wunner <lukas@wunner.de> > > > > >>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > > > > >> > > > > >> This is a version of the fix modified to apply on 6.8. > > > > > > > > > > That was not obvious at all :( > > > > > > > > > Stating "Cc: <stable@vger.kernel.org> # 6.8.x" isn't sufficient? > > > > > > Without showing what commit id this is in Linus's tree, no. > > > > The upstream commit id *is* called out in the patch, but it's buried > > below the three dashes: > > > > The original change was introduced with 6.8, 6.9 added support for > > LEDs on RTL8125. Therefore the first version of the fix applied on > > 6.9-rc only. This is the modified version for 6.8. > > Upstream commit: 19fa4f2a85d7 > > ^^^^^^^^^^^^ > > > > The proper way to do this is to prominently add ... > > > > commit 19fa4f2a85d777a8052e869c1b892a2f7556569d upstream. > > > > ... or ... > > > > [ Upstream commit 19fa4f2a85d777a8052e869c1b892a2f7556569d ] > > > > ... as the first line of the commit message, as per > > Documentation/process/stable-kernel-rules.rst > > > > Yes, Heiner, please resubmit this, AND submit the fix-for-this-fix as > well, so that if we take this patch, it is not broken. > OK. The fix-for-the-fix was included already. It's trivial and IMO submitting it separately would just create overhead. > thanks, > > greg k-hj
On Thu, Apr 18, 2024 at 04:33:37PM +0200, Heiner Kallweit wrote: > On Thu, Apr 18, 2024 at 11:55 AM Greg KH <gregkh@linuxfoundation.org> wrote: > > > > On Thu, Apr 18, 2024 at 12:33:00AM +0200, Lukas Wunner wrote: > > > On Wed, Apr 17, 2024 at 09:43:27AM +0200, Greg KH wrote: > > > > On Wed, Apr 17, 2024 at 09:16:04AM +0200, Heiner Kallweit wrote: > > > > > On 17.04.2024 09:04, Greg KH wrote: > > > > > > On Wed, Apr 17, 2024 at 08:02:31AM +0200, Heiner Kallweit wrote: > > > > > >> On 17.04.2024 04:34, Jakub Kicinski wrote: > > > > > >>> On Mon, 15 Apr 2024 13:57:17 +0200 Heiner Kallweit wrote: > > > > > >>>> Binding devm_led_classdev_register() to the netdev is problematic > > > > > >>>> because on module removal we get a RTNL-related deadlock. Fix this > > > > > >>>> by avoiding the device-managed LED functions. > > > > > >>>> > > > > > >>>> Note: We can safely call led_classdev_unregister() for a LED even > > > > > >>>> if registering it failed, because led_classdev_unregister() detects > > > > > >>>> this and is a no-op in this case. > > > > > >>>> > > > > > >>>> Fixes: 18764b883e15 ("r8169: add support for LED's on RTL8168/RTL8101") > > > > > >>>> Cc: <stable@vger.kernel.org> # 6.8.x > > > > > >>>> Reported-by: Lukas Wunner <lukas@wunner.de> > > > > > >>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > > > > > >> > > > > > >> This is a version of the fix modified to apply on 6.8. > > > > > > > > > > > > That was not obvious at all :( > > > > > > > > > > > Stating "Cc: <stable@vger.kernel.org> # 6.8.x" isn't sufficient? > > > > > > > > Without showing what commit id this is in Linus's tree, no. > > > > > > The upstream commit id *is* called out in the patch, but it's buried > > > below the three dashes: > > > > > > The original change was introduced with 6.8, 6.9 added support for > > > LEDs on RTL8125. Therefore the first version of the fix applied on > > > 6.9-rc only. This is the modified version for 6.8. > > > Upstream commit: 19fa4f2a85d7 > > > ^^^^^^^^^^^^ > > > > > > The proper way to do this is to prominently add ... > > > > > > commit 19fa4f2a85d777a8052e869c1b892a2f7556569d upstream. > > > > > > ... or ... > > > > > > [ Upstream commit 19fa4f2a85d777a8052e869c1b892a2f7556569d ] > > > > > > ... as the first line of the commit message, as per > > > Documentation/process/stable-kernel-rules.rst > > > > > > > Yes, Heiner, please resubmit this, AND submit the fix-for-this-fix as > > well, so that if we take this patch, it is not broken. > > > OK. The fix-for-the-fix was included already. Included where? In this change? Please do not do that. > It's trivial and IMO submitting it > separately would just create overhead. Not submitting it would cause problems when people look and see that the "fix" is not also applied and then you would get automated emails complaining about it. Mirror what is in Linus's tree whenever possible please, it's simpler and saves EVERYONE extra work. thanks, greg k-h
diff --git a/drivers/net/ethernet/realtek/r8169.h b/drivers/net/ethernet/realtek/r8169.h index 81567fcf3..1ef399287 100644 --- a/drivers/net/ethernet/realtek/r8169.h +++ b/drivers/net/ethernet/realtek/r8169.h @@ -72,6 +72,7 @@ enum mac_version { }; struct rtl8169_private; +struct r8169_led_classdev; void r8169_apply_firmware(struct rtl8169_private *tp); u16 rtl8168h_2_get_adc_bias_ioffset(struct rtl8169_private *tp); @@ -83,4 +84,5 @@ void r8169_get_led_name(struct rtl8169_private *tp, int idx, char *buf, int buf_len); int rtl8168_get_led_mode(struct rtl8169_private *tp); int rtl8168_led_mod_ctrl(struct rtl8169_private *tp, u16 mask, u16 val); -void rtl8168_init_leds(struct net_device *ndev); +struct r8169_led_classdev *rtl8168_init_leds(struct net_device *ndev); +void r8169_remove_leds(struct r8169_led_classdev *leds); diff --git a/drivers/net/ethernet/realtek/r8169_leds.c b/drivers/net/ethernet/realtek/r8169_leds.c index 007d077ed..1c97f3cca 100644 --- a/drivers/net/ethernet/realtek/r8169_leds.c +++ b/drivers/net/ethernet/realtek/r8169_leds.c @@ -138,20 +138,31 @@ static void rtl8168_setup_ldev(struct r8169_led_classdev *ldev, led_cdev->hw_control_get_device = r8169_led_hw_control_get_device; /* ignore errors */ - devm_led_classdev_register(&ndev->dev, led_cdev); + led_classdev_register(&ndev->dev, led_cdev); } -void rtl8168_init_leds(struct net_device *ndev) +struct r8169_led_classdev *rtl8168_init_leds(struct net_device *ndev) { - /* bind resource mgmt to netdev */ - struct device *dev = &ndev->dev; struct r8169_led_classdev *leds; int i; - leds = devm_kcalloc(dev, RTL8168_NUM_LEDS, sizeof(*leds), GFP_KERNEL); + leds = kcalloc(RTL8168_NUM_LEDS + 1, sizeof(*leds), GFP_KERNEL); if (!leds) - return; + return NULL; for (i = 0; i < RTL8168_NUM_LEDS; i++) rtl8168_setup_ldev(leds + i, ndev, i); + + return leds; +} + +void r8169_remove_leds(struct r8169_led_classdev *leds) +{ + if (!leds) + return; + + for (struct r8169_led_classdev *l = leds; l->ndev; l++) + led_classdev_unregister(&l->led); + + kfree(leds); } diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c index 4b6c28576..32b73f398 100644 --- a/drivers/net/ethernet/realtek/r8169_main.c +++ b/drivers/net/ethernet/realtek/r8169_main.c @@ -634,6 +634,8 @@ struct rtl8169_private { const char *fw_name; struct rtl_fw *rtl_fw; + struct r8169_led_classdev *leds; + u32 ocp_base; }; @@ -4930,6 +4932,9 @@ static void rtl_remove_one(struct pci_dev *pdev) cancel_work_sync(&tp->wk.work); + if (IS_ENABLED(CONFIG_R8169_LEDS)) + r8169_remove_leds(tp->leds); + unregister_netdev(tp->dev); if (tp->dash_type != RTL_DASH_NONE) @@ -5391,7 +5396,7 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) if (IS_ENABLED(CONFIG_R8169_LEDS) && tp->mac_version > RTL_GIGA_MAC_VER_06 && tp->mac_version < RTL_GIGA_MAC_VER_61) - rtl8168_init_leds(dev); + tp->leds = rtl8168_init_leds(dev); netdev_info(dev, "%s, %pM, XID %03x, IRQ %d\n", rtl_chip_infos[chipset].name, dev->dev_addr, xid, tp->irq);
Binding devm_led_classdev_register() to the netdev is problematic because on module removal we get a RTNL-related deadlock. Fix this by avoiding the device-managed LED functions. Note: We can safely call led_classdev_unregister() for a LED even if registering it failed, because led_classdev_unregister() detects this and is a no-op in this case. Fixes: 18764b883e15 ("r8169: add support for LED's on RTL8168/RTL8101") Cc: <stable@vger.kernel.org> # 6.8.x Reported-by: Lukas Wunner <lukas@wunner.de> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- The original change was introduced with 6.8, 6.9 added support for LEDs on RTL8125. Therefore the first version of the fix applied on 6.9-rc only. This is the modified version for 6.8. Upstream commit: 19fa4f2a85d7 --- drivers/net/ethernet/realtek/r8169.h | 4 +++- drivers/net/ethernet/realtek/r8169_leds.c | 23 +++++++++++++++++------ drivers/net/ethernet/realtek/r8169_main.c | 7 ++++++- 3 files changed, 26 insertions(+), 8 deletions(-)