linux-kernel - Re: [PATCH v3 3/3] pinctrl: pinctrl-single: fix pcs_pin_dbg_show() when bits_per

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: <cd589749-7f37-9f7f-9d36-42032c724506@amazon.com>
Date:   Fri, 19 Mar 2021 09:52:55 +0200
From:   "Hawa, Hanna" <hhhawa@...zon.com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
CC:     Tony Lindgren <tony@...mide.com>,
        Haojian Zhuang <haojian.zhuang@...aro.org>,
        Linus Walleij <linus.walleij@...aro.org>,
        "David Woodhouse" <dwmw@...zon.co.uk>, <benh@...zon.com>,
        <ronenk@...zon.com>, <talel@...zon.com>, <jonnyc@...zon.com>,
        <hanochu@...zon.com>, <tgershi@...zon.com>,
        linux-arm Mailing List <linux-arm-kernel@...ts.infradead.org>,
        Linux OMAP Mailing List <linux-omap@...r.kernel.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 3/3] pinctrl: pinctrl-single: fix pcs_pin_dbg_show() when
 bits_per_mux is not zero



On 3/18/2021 2:15 PM, Andy Shevchenko wrote:
> 
> 
> On Wed, Mar 17, 2021 at 11:42 PM Hanna Hawa<hhhawa@...zon.com>  wrote:
>> An SError was detected when trying to print the supported pins in a
> What is SError? Yes, I have read a discussion, but here is the hint:
> if a person sees this as a first text due to, for example, bisecting
> an issue, what she/he can get from this cryptic name?

What you suggest?
s/An SError/A kernel-panic/?
Or remove the sentence and keep the below:
"
This change fixes the pcs_pin_dbg_show() in pinctrl-single driver when 
bits_per_mux is not zero. In addition move offset calculation and pin 
offset in register to common function.
"

> 
>> pinctrl device which supports multiple pins per register. This change
>> fixes the pcs_pin_dbg_show() in pinctrl-single driver when bits_per_mux
>> is not zero. In addition move offset calculation and pin offset in
>> register to common function.
> Reviewed-by: Andy Shevchenko<andy.shevchenko@...il.com>

Thanks

> 
>> Fixes: 4e7e8017a80e ("pinctrl: pinctrl-single: enhance to configure multiple pins of different modules")
>> Signed-off-by: Hanna Hawa<hhhawa@...zon.com>
>> ---
>>   drivers/pinctrl/pinctrl-single.c | 57 +++++++++++++++++++++-----------
>>   1 file changed, 37 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
>> index f3394517cb2e..4595acf6545e 100644
>> --- a/drivers/pinctrl/pinctrl-single.c
>> +++ b/drivers/pinctrl/pinctrl-single.c
>> @@ -270,20 +270,46 @@ static void __maybe_unused pcs_writel(unsigned val, void __iomem *reg)
>>          writel(val, reg);
>>   }
>>
>> +static unsigned int pcs_pin_reg_offset_get(struct pcs_device *pcs,
>> +                                          unsigned int pin)
>> +{
>> +       unsigned int mux_bytes;
>> +
>> +       mux_bytes = pcs->width / BITS_PER_BYTE;
> Can be folded to one line.

Ack

> 
>> +       if (pcs->bits_per_mux) {
>> +               unsigned int pin_offset_bytes;
>> +
>> +               pin_offset_bytes = (pcs->bits_per_pin * pin) / BITS_PER_BYTE;
>> +               return (pin_offset_bytes / mux_bytes) * mux_bytes;
> Side note for the further improvements (in a separate change, because
> I see that you just copied an original code, and after all this is
> just a fix patch): this can be replaced by round down APIs (one which
> works for arbitrary divisors).

Agree, didn't want to change the formula as it's fix patch. (here and 
below), this can be taken for further improvements.

> 
>> +       }
>> +
>> +       return pin * mux_bytes;
>> +}
>> +
>> +static unsigned int pcs_pin_shift_reg_get(struct pcs_device *pcs,
>> +                                         unsigned int pin)
>> +{
>> +       return (pin % (pcs->width / pcs->bits_per_pin)) * pcs->bits_per_pin;
> Also a side note: I'm wondering if this can be optimized to have less divisions.
> 
>> +}
>> +
>>   static void pcs_pin_dbg_show(struct pinctrl_dev *pctldev,
>>                                          struct seq_file *s,
>>                                          unsigned pin)
>>   {

Thanks,
Hanna