| Message ID | 20210128213851.2499012-2-anthony.l.nguyen@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Intel Wired LAN Driver Updates 2021-01-28 | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 3 maintainers not CCed: jesse.brandeburg@intel.com jeffrey.t.kirsher@intel.com intel-wired-lan@lists.osuosl.org |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 1 this patch: 1 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 9 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 1 this patch: 1 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On Thu, 28 Jan 2021 13:38:48 -0800 Tony Nguyen wrote: > From: Kai-Heng Feng <kai.heng.feng@canonical.com> > > Similar to commit 165ae7a8feb5 ("igb: Report speed and duplex as unknown > when device is runtime suspended"), if we try to read speed and duplex > sysfs while the device is runtime suspended, igc will complain and > stops working: > The more generic approach will be wrap get_link_ksettings() with begin() > and complete() callbacks, and calls runtime resume and runtime suspend > routine respectively. However, igc is like igb, runtime resume routine > uses rtnl_lock() which upper ethtool layer also uses. > > So to prevent a deadlock on rtnl, take a different approach, use > pm_runtime_suspended() to avoid reading register while device is runtime > suspended. Is someone working on the full fix to how PM operates? There is another rd32(IGC_STATUS) in this file which I don't think is protected either.
On 1/30/2021 08:22, Jakub Kicinski wrote: > On Thu, 28 Jan 2021 13:38:48 -0800 Tony Nguyen wrote: >> From: Kai-Heng Feng <kai.heng.feng@canonical.com> >> >> Similar to commit 165ae7a8feb5 ("igb: Report speed and duplex as unknown >> when device is runtime suspended"), if we try to read speed and duplex >> sysfs while the device is runtime suspended, igc will complain and >> stops working: > >> The more generic approach will be wrap get_link_ksettings() with begin() >> and complete() callbacks, and calls runtime resume and runtime suspend >> routine respectively. However, igc is like igb, runtime resume routine >> uses rtnl_lock() which upper ethtool layer also uses. >> >> So to prevent a deadlock on rtnl, take a different approach, use >> pm_runtime_suspended() to avoid reading register while device is runtime >> suspended. > > Is someone working on the full fix to how PM operates? > > There is another rd32(IGC_STATUS) in this file which I don't think > is protected either. Hello Jakub, What is another rd32(IGC_STATUS) you meant? in igc_ethtool_get_regs? While the device in D3 state there is no configuration space registers access. sasha >
On Sat, 30 Jan 2021 16:00:06 +0200 Neftin, Sasha wrote: > On 1/30/2021 08:22, Jakub Kicinski wrote: > > On Thu, 28 Jan 2021 13:38:48 -0800 Tony Nguyen wrote: > >> From: Kai-Heng Feng <kai.heng.feng@canonical.com> > >> > >> Similar to commit 165ae7a8feb5 ("igb: Report speed and duplex as unknown > >> when device is runtime suspended"), if we try to read speed and duplex > >> sysfs while the device is runtime suspended, igc will complain and > >> stops working: > > > >> The more generic approach will be wrap get_link_ksettings() with begin() > >> and complete() callbacks, and calls runtime resume and runtime suspend > >> routine respectively. However, igc is like igb, runtime resume routine > >> uses rtnl_lock() which upper ethtool layer also uses. > >> > >> So to prevent a deadlock on rtnl, take a different approach, use > >> pm_runtime_suspended() to avoid reading register while device is runtime > >> suspended. > > > > Is someone working on the full fix to how PM operates? > > > > There is another rd32(IGC_STATUS) in this file which I don't think > > is protected either. > > What is another rd32(IGC_STATUS) you meant? in igc_ethtool_get_regs? Yes. > While the device in D3 state there is no configuration space registers > access. That's to say similar stack trace will be generated to the one fixed here, if someone runs ethtool -d, correct? I don't see anything checking runtime there either. To be clear I'm not asking for this to be addressed in this series. Rather for a strong commitment that PM handling will be restructured. It seems to me you should depend on refcounting / locking that the PM subsystem does more rather than involving rtnl_lock.
On 1/30/2021 20:12, Jakub Kicinski wrote: > On Sat, 30 Jan 2021 16:00:06 +0200 Neftin, Sasha wrote: >> On 1/30/2021 08:22, Jakub Kicinski wrote: >>> On Thu, 28 Jan 2021 13:38:48 -0800 Tony Nguyen wrote: >>>> From: Kai-Heng Feng <kai.heng.feng@canonical.com> >>>> >>>> Similar to commit 165ae7a8feb5 ("igb: Report speed and duplex as unknown >>>> when device is runtime suspended"), if we try to read speed and duplex >>>> sysfs while the device is runtime suspended, igc will complain and >>>> stops working: >>> >>>> The more generic approach will be wrap get_link_ksettings() with begin() >>>> and complete() callbacks, and calls runtime resume and runtime suspend >>>> routine respectively. However, igc is like igb, runtime resume routine >>>> uses rtnl_lock() which upper ethtool layer also uses. >>>> >>>> So to prevent a deadlock on rtnl, take a different approach, use >>>> pm_runtime_suspended() to avoid reading register while device is runtime >>>> suspended. >>> >>> Is someone working on the full fix to how PM operates? >>> >>> There is another rd32(IGC_STATUS) in this file which I don't think >>> is protected either. >> >> What is another rd32(IGC_STATUS) you meant? in igc_ethtool_get_regs? > > Yes. > >> While the device in D3 state there is no configuration space registers >> access. > > That's to say similar stack trace will be generated to the one fixed > here, if someone runs ethtool -d, correct? I don't see anything > checking runtime there either. > yes. This problem crosses many drivers. (not only igb, igc,...) specific to this one (igc), can we check 'netif_running at begin of the _get_regs method: if (!netif_running(netdev)) return; what do you think? (only OS can put device to the D3) > To be clear I'm not asking for this to be addressed in this series. > Rather for a strong commitment that PM handling will be restructured. > It seems to me you should depend on refcounting / locking that the PM > subsystem does more rather than involving rtnl_lock. >
On Sun, 31 Jan 2021 12:22:25 +0200 Neftin, Sasha wrote: > On 1/30/2021 20:12, Jakub Kicinski wrote: > > On Sat, 30 Jan 2021 16:00:06 +0200 Neftin, Sasha wrote: > >> What is another rd32(IGC_STATUS) you meant? in igc_ethtool_get_regs? > > > > Yes. > > > >> While the device in D3 state there is no configuration space registers > >> access. > > > > That's to say similar stack trace will be generated to the one fixed > > here, if someone runs ethtool -d, correct? I don't see anything > > checking runtime there either. > > > yes. > This problem crosses many drivers. (not only igb, igc,...) > > specific to this one (igc), can we check 'netif_running at begin of the > _get_regs method: > if (!netif_running(netdev)) > return; > what do you think? (only OS can put device to the D3) That'd address the particular issue we noticed in the 5min review of this patch, but similar, less obvious problems may still be lurking? I wish I knew more about PM so I could suggest a solution. It'd be ideal to avoid the rtnl_lock calls in resume, so that the driver can just wake up the device from within the callbacks. Maybe embedded experts can chime in and suggest how it's usually handled..
diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c b/drivers/net/ethernet/intel/igc/igc_ethtool.c index 831f2f09de5f..ec8cd69d4992 100644 --- a/drivers/net/ethernet/intel/igc/igc_ethtool.c +++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c @@ -1714,7 +1714,8 @@ static int igc_ethtool_get_link_ksettings(struct net_device *netdev, Asym_Pause); } - status = rd32(IGC_STATUS); + status = pm_runtime_suspended(&adapter->pdev->dev) ? + 0 : rd32(IGC_STATUS); if (status & IGC_STATUS_LU) { if (status & IGC_STATUS_SPEED_1000) {