linux-kernel - Re: [PATCH 11/16] OPP: Add dev_pm_opp_add

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]

Date:   Thu, 8 Jun 2023 10:59:51 +0200
From:   Ulf Hansson <ulf.hansson@...aro.org>
To:     Viresh Kumar <viresh.kumar@...aro.org>
Cc:     Sudeep Holla <sudeep.holla@....com>,
        Cristian Marussi <cristian.marussi@....com>,
        Viresh Kumar <vireshk@...nel.org>, Nishanth Menon <nm@...com>,
        Stephen Boyd <sboyd@...nel.org>,
        Nikunj Kela <nkela@...cinc.com>,
        Prasad Sodagudi <psodagud@...cinc.com>,
        Alexandre Torgue <alexandre.torgue@...s.st.com>,
        linux-pm@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 11/16] OPP: Add dev_pm_opp_add_dynamic() to allow more flexibility

On Thu, 8 Jun 2023 at 07:29, Viresh Kumar <viresh.kumar@...aro.org> wrote:
>
> On 07-06-23, 14:46, Ulf Hansson wrote:
> > diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> > index 954c94865cf5..0e6ee2980f88 100644
> > --- a/drivers/opp/core.c
> > +++ b/drivers/opp/core.c
> > @@ -1921,8 +1921,7 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp,
> >   * _opp_add_v1() - Allocate a OPP based on v1 bindings.
> >   * @opp_table:       OPP table
> >   * @dev:     device for which we do this operation
> > - * @freq:    Frequency in Hz for this OPP
> > - * @u_volt:  Voltage in uVolts for this OPP
> > + * @opp:     The OPP to add
> >   * @dynamic: Dynamically added OPPs.
> >   *
> >   * This function adds an opp definition to the opp table and returns status.
> > @@ -1940,10 +1939,10 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp,
> >   * -ENOMEM   Memory allocation failure
> >   */
> >  int _opp_add_v1(struct opp_table *opp_table, struct device *dev,
> > -             unsigned long freq, long u_volt, bool dynamic)
> > +             struct dev_pm_opp_data *opp, bool dynamic)
>
> The name `opp` is mostly used for instances of `struct dev_pm_opp`. Can we use a
> different name here please for the data ?

Certainly, what do you suggest?

>
> > +/**
> > + * dev_pm_opp_add()  - Add an OPP table from a table definitions
> > + * @dev:     device for which we do this operation
> > + * @freq:    Frequency in Hz for this OPP
> > + * @u_volt:  Voltage in uVolts for this OPP
> > + *
> > + * This function adds an opp definition to the opp table and returns status.
> > + * The opp is made available by default and it can be controlled using
> > + * dev_pm_opp_enable/disable functions.
> > + *
> > + * Return:
> > + * 0         On success OR
> > + *           Duplicate OPPs (both freq and volt are same) and opp->available
> > + * -EEXIST   Freq are same and volt are different OR
> > + *           Duplicate OPPs (both freq and volt are same) and !opp->available
> > + * -ENOMEM   Memory allocation failure
> > + */
> > +int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt)
>
> Maybe move this to include/linux/pm_opp.h and mark it static inline and get rid
> of documentation too.

Good idea!

>
> > +{
> > +     struct dev_pm_opp_data opp;
> > +
> > +     memset(&opp, 0, sizeof(opp));
>
> What about
>         struct dev_pm_opp_data data = {0};
>
> I think it is guaranteed that all the fields will be 0 now, not the padding of
> course, but we don't care about that here.
>
> > +     opp.freq = freq;
> > +     opp.u_volt = u_volt;
>
> Or maybe just
>
>         struct dev_pm_opp_data data = {
>                 .freq = freq,
>                 .u_volt = u_volt,
>         };
>
> Rest must be 0.

Good suggestions both, I will change to whatever is best suitable!

Thanks for reviewing!

Kind regards
Uffe