linux-kernel - Re: [PATCH v6 1/6] tty: serial: qcom_geni_serial: Use OPP API to set clk/perf state

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: <159532101373.3847286.9695594340556014384@swboyd.mtv.corp.google.com>
Date:   Tue, 21 Jul 2020 01:43:33 -0700
From:   Stephen Boyd <swboyd@...omium.org>
To:     Rajendra Nayak <rnayak@...eaurora.org>,
        Viresh Kumar <viresh.kumar@...aro.org>
Cc:     agross@...nel.org, bjorn.andersson@...aro.org,
        robdclark@...omium.org, robdclark@...il.com,
        stanimir.varbanov@...aro.org, mka@...omium.org,
        linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Akash Asthana <akashast@...eaurora.org>,
        linux-serial@...r.kernel.org
Subject: Re: [PATCH v6 1/6] tty: serial: qcom_geni_serial: Use OPP API to set clk/perf state

Quoting Viresh Kumar (2020-06-29 20:05:52)
> On 30-06-20, 08:31, Rajendra Nayak wrote:
> > 
> > 
> > On 6/30/2020 4:47 AM, Stephen Boyd wrote:
> > > Quoting Rajendra Nayak (2020-06-15 05:02:39)
> > > > diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> > > > index 457c0bf..a90f8ec 100644
> > > > --- a/drivers/tty/serial/qcom_geni_serial.c
> > > > +++ b/drivers/tty/serial/qcom_geni_serial.c
> > > > @@ -9,6 +9,7 @@
> > > >   #include <linux/module.h>
> > > >   #include <linux/of.h>
> > > >   #include <linux/of_device.h>
> > > > +#include <linux/pm_opp.h>
> > > >   #include <linux/platform_device.h>
> > > >   #include <linux/pm_runtime.h>
> > > >   #include <linux/pm_wakeirq.h>
> > > > @@ -962,7 +963,7 @@ static void qcom_geni_serial_set_termios(struct uart_port *uport,
> > > >                  goto out_restart_rx;
> > > >          uport->uartclk = clk_rate;
> > > > -       clk_set_rate(port->se.clk, clk_rate);
> > > > +       dev_pm_opp_set_rate(uport->dev, clk_rate);
> > > 
> > > If there isn't an OPP table for the device because it is optional then
> > > how can we unconditionally call dev_pm_opp_set_rate()?
> 
> Looks like some *Maintainers* aren't paying enough attention lately ;)
> 
> Just kidding.
> 

It seems that dev_pm_opp_set_rate() calls _find_opp_table() and finds
something that isn't an error pointer but then dev_pm_opp_of_add_table()
returns an error value because there isn't an operating-points property
in DT. We're getting saved because this driver also happens to call
dev_pm_opp_set_clkname() which allocates the OPP table a second time
(because the first time it got freed when dev_pm_opp_of_add_table()
return -ENODEV because the property was missing).

Why do we need 'has_opp_table' logic? It seems that we have to keep
track of the fact that dev_pm_opp_of_add_table() failed so that we don't
put the table again, but then dev_pm_opp_set_clkname() can be called
to allocate the table regardless.

This maintainer is paying very close attention to super confusing code like
this:

	if (drv->has_opp_table)
		dev_pm_opp_of_remove_table(dev);
	dev_pm_opp_put_clkname(drv->opp_table);

which reads as "if I have an opp table remove it and oh by the way
remove the clk name for this opp table pointer I also happen to always
have".

Maybe I would be happier if dev_pm_opp_of_table() went away and we just
had dev_pm_opp_add_table(const struct opp_config *config) that did all
the things for us like set a clk name, set the supported hw, set the
prop name, etc. based on the single config struct pointer and also
parsed out the OPP table from DT or just ignored that if there isn't any
operating-points property. Then the caller wouldn't need to keep track
of 'if has_opp_table' because it doesn't seem to actually care and the
core is happy to allocate a table for the device anyway so long as it
sets a clk name.