Re: [PATCH net v3] net: ethtool: do runtime PM outside RTNL

[Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>]

On Wed, Jan 03, 2024 at 12:24:18PM +0100, Heiner Kallweit wrote:
> On 03.01.2024 11:30, Stanislaw Gruszka wrote:
> > On Wed, Dec 06, 2023 at 08:44:48AM -0800, Jakub Kicinski wrote:
> >> On Wed,  6 Dec 2023 11:39:32 +0100 Johannes Berg wrote:
> >>> As reported by Marc MERLIN, at least one driver (igc) wants or
> >>> needs to acquire the RTNL inside suspend/resume ops, which can
> >>> be called from here in ethtool if runtime PM is enabled.
> >>>
> >>> Allow this by doing runtime PM transitions without the RTNL
> >>> held. For the ioctl to have the same operations order, this
> >>> required reworking the code to separately check validity and
> >>> do the operation. For the netlink code, this now has to do
> >>> the runtime_pm_put a bit later.
> >>
> >> I was really, really hoping that this would serve as a motivation
> >> for Intel to sort out the igb/igc implementation. The flow AFAICT
> >> is ndo_open() starts the NIC, the calls pm_sus, which shuts the NIC
> >> back down immediately (!?) then it schedules a link check from a work
> > 
> > It's not like that. pm_runtime_put() in igc_open() does not disable device.
> > It calls runtime_idle callback which check if there is link and if is
> > not, schedule device suspend in 5 second, otherwise device stays running.
> > 
> > Work watchdog_task runs periodically and also check for link changes.
> > 
> >> queue, which opens it again (!?). It's a source of never ending bugs.
> > 
> > Maybe there are issues there and igc pm runtime implementation needs
> > improvements, with lockings or otherwise. Some folks are looking at this. 
> > But I think for this particular deadlock problem reverting of below commits
> > should be considered:
> > 
> > bd869245a3dc net: core: try to runtime-resume detached device in __dev_open
> > f32a21376573 ethtool: runtime-resume netdev parent before ethtool ioctl ops
> > Reverting bd869245a3dc would break existing stuff.
> 
> > First, the deadlock should be addressed also in older kernels and
> > refactoring is not really backportable fix.
> > 
> You could simply disable igc runtime pm on older kernel versions
> if backporting a proper fix would be too cumbersome.

It would be better to have pm working on older kernels as it use to.

> > Second, I don't think network stack should do any calls to pm_runtime* .
> 
> It's not unusual that subsystem core code deals with runtime pm.
> E.g. see all the runtime pm calls in drivers/pci/pci.c
> IMO it's exactly the purpose of the RPM API to encapsulate the
> device-specific (r)pm features.

PCI is bus layer that control device probe/remove, suspend/resume, etc,
it has to do this. To make proper companion non-bus subsystem should be
used i.e. sound, drm,  bluetooth ...  all of those do not pm_runtime 
in core layer and leave that to drivers. One exception is block layer
with it's blk-pm.c , but that it's also more like library that is used
by the drivers.

Regards
Stanislaw