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

From: Przemek Kitszel <[email-masked]>
Date: Tue, 5 Dec 2023 06:19:15 +0100

On 12/4/23 20:07, Johannes Berg wrote:
> From: Johannes Berg <[email-masked]>
> 
> As reported by Marc MERLIN in [1], at least one driver (igc)

perhaps Reported-by tag? (I know this is RFC as of now)

> wants/needs to acquire the RTNL inside suspend/resume ops,
> which can be called from here in ethtool if runtime PM is
> enabled.
> 
> [1] https://lore.kernel.org/r/[email-masked]
> 
> 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.
> 
> Signed-off-by: Johannes Berg <[email-masked]>
> ---
>   net/ethtool/ioctl.c   | 71 ++++++++++++++++++++++++++-----------------
>   net/ethtool/netlink.c | 32 ++++++++-----------
>   2 files changed, 56 insertions(+), 47 deletions(-)
> 
Thank you for the patch,

I like the idea of split into validate + do for dev_ethtool(),
what minimizes unneeded PM touching. Moving pm_runtime_get_sync() out of 
RTNL is also a great improvement per se. Also from the pure coding 
perspective I see no obvious flaws in the patch. I think that igc code
was just accidental to the issue, in a way that it was not deliberate to
hold RTNL for extended periods. With your patch fixing the bug, there is
no point with waiting IMO, so

Reviewed-by: Przemek Kitszel <[email-masked]>