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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date: Mon, 8 Jan 2024 12:18:29 +0100
From: Stanislaw Gruszka <stanislaw.gruszka@...ux.intel.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: Heiner Kallweit <hkallweit1@...il.com>,
	Johannes Berg <johannes@...solutions.net>, netdev@...r.kernel.org,
	Johannes Berg <johannes.berg@...el.com>,
	Marc MERLIN <marc@...lins.org>,
	Przemek Kitszel <przemyslaw.kitszel@...el.com>
Subject: Re: [PATCH net v3] net: ethtool: do runtime PM outside RTNL

On Fri, Jan 05, 2024 at 07:02:18PM -0800, Jakub Kicinski wrote:
> On Fri, 5 Jan 2024 17:29:16 +0100 Stanislaw Gruszka wrote:
> > > Removing the rpm calls from the core is just going to lead to a
> > > whack-a-mole of bugs in the drivers themselves.
> > >
> > > IOW I look at the RPM calls in the core as a canary for people
> > > doing the wrong thing :(  
> > 
> > Hmm, this one I don't understand, what other bugs could pop up
> > after reverting bd869245a3dcc and others that added rpm calls
> > for the net core?
> 
> IDK what igc powers down,

>From what I can tell basically everything, it's full shutdown.

> but if there's any ndo or ethtool
> callback which needs to access a register that requires power 
> to be resumed - it will deadlock on rtnl exactly the same.
> 
> Looking at igc_ethtool I see:
> 
> static int igc_ethtool_begin(struct net_device *netdev)
> {
> 	struct igc_adapter *adapter = netdev_priv(netdev);
> 
> 	pm_runtime_get_sync(&adapter->pdev->dev);
> 	return 0;
> }
> 
> static void igc_ethtool_complete(struct net_device *netdev)
> {
> 	struct igc_adapter *adapter = netdev_priv(netdev);
> 
> 	pm_runtime_put(&adapter->pdev->dev);
> }
> 
> so unless we think that returning -ENODEV from all ethtool calls
> when cable is not plugged in is okay - removing the PM resume
> from the core doesn't buy us much :(

It would address the regression in simple fix that can be send
to -stable. Event if -ENODEV for all ethtool ops and open is
not good, it's still better than deadlocking whole system.

I agree RPM for igc is not perfect and has issues that need
to be fix. People are working on it inspired by e1000e
implementation. Is should address the main requirement:
no rtnl_lock on resume path - waking up device when needed
on ndo/ethtool.

But that would not be simple fix AFICT, more likely it
will be reimplementation of the whole thing.

Additionally, in context of ethtool, previously each driver 
that implement RPM, woke up the device for actual HW access,
and don't when only memory was used. For example e1000e has
fine tuned ethtool ops. Some others like cadence/macb or 
renesas/sh_eth went event further and have their 
pm_runtime_resume_get_sync() in register access functions.

Now a hardware is powered up on every ethtool op regardless
of actual need.

So I think that the calls are only needed for some drivers, but
for others are detrimental. Would adding new netdev->priv_flags
for calling them be acceptable ?

Regards
Stanislaw