linux-kernel - Re: [PATCH v3 3/3] serial: 8250_of: manage bus clock in suspend/resume

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHp75VchEcpP67oPC8xD+tYrY_A0BGSJqK=1au939-W60_qQoQ@mail.gmail.com>
Date: Fri, 11 Apr 2025 22:44:06 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Alex Elder <elder@...cstar.com>
Cc: gregkh@...uxfoundation.org, jirislaby@...nel.org, robh@...nel.org, 
	krzk+dt@...nel.org, conor+dt@...nel.org, dlan@...too.org, 
	benjamin.larsson@...exis.eu, bastien.curutchet@...tlin.com, 
	andriy.shevchenko@...ux.intel.com, u.kleine-koenig@...libre.com, 
	lkundrak@...sk, devicetree@...r.kernel.org, linux-serial@...r.kernel.org, 
	spacemit@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 3/3] serial: 8250_of: manage bus clock in suspend/resume

On Fri, Apr 11, 2025 at 10:36 PM Alex Elder <elder@...cstar.com> wrote:
> On 4/11/25 2:30 PM, Andy Shevchenko wrote:
> > Fri, Apr 11, 2025 at 10:44:18AM -0500, Alex Elder kirjoitti:
> >> Save the bus clock pointer in the of_serial_info structure, and use
> >> that to disable the bus clock on suspend and re-enable it on resume.

...

> >>      if (!port->uartclk) {
> >> -            struct clk *bus_clk;
> >> -
> >> -            bus_clk = devm_clk_get_optional_enabled(dev, "bus");
> >> -            if (IS_ERR(bus_clk)) {
> >> -                    ret = dev_err_probe(dev, PTR_ERR(bus_clk), "failed to get bus clock\n");
> >> +            info->bus_clk = devm_clk_get_optional_enabled(dev, "bus");
> >> +            if (IS_ERR(info->bus_clk)) {
> >> +                    ret = dev_err_probe(dev, PTR_ERR(info->bus_clk),
> >> +                                        "failed to get bus clock\n");
> >>                      goto err_pmruntime;
> >>              }
> >>
> >>              /* If the bus clock is required, core clock must be named */
> >> -            info->clk = devm_clk_get_enabled(dev, bus_clk ? "core" : NULL);
> >> +            info->clk = devm_clk_get_enabled(dev, info->bus_clk ? "core" : NULL);
> >>              if (IS_ERR(info->clk)) {
> >>                      ret = dev_err_probe(dev, PTR_ERR(info->clk), "failed to get clock\n");
> >
> > While the first patch against this file looks okay now, this one inherits the
> > same problem (seems like not enought thinking about the code representation).
> >
> > Instead of rewritting half of the lines you just introduced (which is also a
> > bad practice), add a one-liner that assigns a field to the local variable.
>
> So you want me to re-spin this again so that I use the local variable?

Yes.

> I understand what you're saying based on ease of review,

No, not only review. Here the main issue is ping-pong between patches.
If you know ahead that these lines should be changed, do it from the
start. But I understand the need to have separate changes for
logically different pieces. That's why

> but this
> is a simple patch

It doesn't matter how simple it is.

> and the change is very understandable, and the
> code is no more or less clear when using the local variable.

See above.

> >>                      goto err_pmruntime;


-- 
With Best Regards,
Andy Shevchenko