netdev - Re: [net-next,v2] net: ethernet: rtsn: Add support for Renesas Ethernet-TSN

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 for Android: free password hash cracker in your pocket

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

Date: Wed, 8 May 2024 17:29:15 +0200
From: Geert Uytterhoeven <geert@...ux-m68k.org>
To: Niklas Söderlund <niklas.soderlund+renesas@...natech.se>
Cc: Andrew Lunn <andrew@...n.ch>, Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>, 
	"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org, 
	linux-renesas-soc@...r.kernel.org
Subject: Re: [net-next,v2] net: ethernet: rtsn: Add support for Renesas Ethernet-TSN

Hi Niklas,

On Wed, May 8, 2024 at 5:11 PM Niklas Söderlund
<niklas.soderlund+renesas@...natech.se> wrote:
> On 2024-05-08 16:00:21 +0200, Andrew Lunn wrote:
> > > I agree it's odd and I will try to find out.
> > >
> > > If I remove all pm_ calls and the include of pm_runtime.h register reads
> > > from the device do no longer works, so operating the device fails. Even
> > > if I dig out the root cause for this, is there any harm in keeping the
> > > pm_ operations in the initial entablement?
> >
> > It suggests something is broken. Do we want to merge broken code?
>
> Of course I do not want broken code merged. I was curious if you knew of
> any harmful side effect of of using pm_ functions I was unaware of.
>
> > Once we understand the root cause maybe then we can decide it is O.K.
>
> The root cause is that the module clock is not enabled without some
> action. If I remove all pm_ calls as well as the inclusion of
> linux/pm_runtime.h. The tsn module clock is left disabled after probe
> completes.

> As the clock is disabled trying to operate the device is not possible.

Which is perfectly normal...

> The clock can either be enabled by the pm_ calls as show or be replaced
> by an explicit clk_enable(), like this (the other pm_ related
> calls/includes are of course also removed).
>
>     -       pm_runtime_enable(&pdev->dev);
>     -       pm_runtime_get_sync(&pdev->dev);
>     +       clk_enable(priv->clk);
>
> Either of the two methods leaves the module clock running and the driver
> can operate the device.

> I would prefer to keep the pm_ operations, but if you prefer I can
> switch to using clk_enable().

While explicit clock management works, please use pm_runtime_*()
instead, as the device is part of a Clock Domain.  Also, the TSN
block may be reused on a different SoC, where it is part of a real
power domain, and manual clock management won't be sufficient.

BTW, if you need to debug more fine-grained Runtime PM handling, you
can add a volatile variable that tracks the module clock state
(update it in cpg_mstp_clock_endisable()), and WARN() in rtsn_read()
and friends if the clock is disabled.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds