linux-kernel - Re: [PATCH 2/2] gpio: mvebu: fix gpio bank registration when pwm is used

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: <CACQ1gAhy_S16YfxQ1eOJUX45rdnjrsPFckYJm_1OHGW0CThg0g@mail.gmail.com>
Date:   Tue, 30 May 2017 16:45:24 +0200
From:   Richard Genoud <richard.genoud@...il.com>
To:     Gregory CLEMENT <gregory.clement@...e-electrons.com>
Cc:     Linus Walleij <linus.walleij@...aro.org>,
        Alexandre Courbot <gnurou@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        Jason Cooper <jason@...edaemon.net>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        linux-pwm@...r.kernel.org, Mark Rutland <mark.rutland@....com>,
        Ralph Sennhauser <ralph.sennhauser@...il.com>,
        Rob Herring <robh+dt@...nel.org>,
        Russell King <linux@...linux.org.uk>,
        Sebastian Hesselbarth <sebastian.hesselbarth@...il.com>,
        Thierry Reding <thierry.reding@...il.com>
Subject: Re: [PATCH 2/2] gpio: mvebu: fix gpio bank registration when pwm is used

2017-05-30 15:16 GMT+02:00 Gregory CLEMENT <gregory.clement@...e-electrons.com>:
> Hi Richard,
Hi Greg !

>
>  On mar., mai 30 2017, Richard Genoud <richard.genoud@...il.com> wrote:
>
>> If more than one gpio bank has the "pwm" property, only one will be
>> registered successfully, all the others will fail with:
>> mvebu-gpio: probe of f1018140.gpio failed with error -17
>>
>> That's because in alloc_pwms(), the chip->base (aka "int pwm"), was not
>> set (thus, ==0) ; and 0 is a meaningful start value in alloc_pwm().
>> What was intended is chip->base = -1.
>> Like that, the numbering will be done auto-magically
>>
>> Tested on clearfog-pro (Marvell 88F6828)
>>
>> Signed-off-by: Richard Genoud <richard.genoud@...il.com>
>> ---
>>  drivers/gpio/gpio-mvebu.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c
>> index cdef2c78cb3b..4734923e11fd 100644
>> --- a/drivers/gpio/gpio-mvebu.c
>> +++ b/drivers/gpio/gpio-mvebu.c
>> @@ -768,6 +768,7 @@ static int mvebu_pwm_probe(struct platform_device *pdev,
>>       mvpwm->chip.dev = dev;
>>       mvpwm->chip.ops = &mvebu_pwm_ops;
>>       mvpwm->chip.npwm = mvchip->chip.ngpio;
>> +     mvpwm->chip.base = -1;
>
> Why not using
> mvpwm->chip.base = id * MVEBU_MAX_GPIO_PER_BANK;
> as it is done in the mvebu_gpio_probe() function?
Yes, that was my first move:
mvpwm->chip.base = mvchip->chip.base;

But after some reflexion, mvpwm->chip.base is not the GPIO base, it's
the PWM base,
(mvpwm->chip is a struct pwm_chip), so it would we weird to have
"holes" in the declared PWMs.
I'm not clear, so here's an example:
If, in the DTS, we have:
            gpio0: gpio@...00 {
                compatible = "marvell,armada-370-xp-gpio",
                         "marvell,orion-gpio";
                reg = <0x18100 0x40>, <0x181c0 0x08>;
                reg-names = "gpio";  /* "pwm" missing */
[...]
            gpio1: gpio@...40 {
                compatible = "marvell,armada-370-xp-gpio",
                         "marvell,orion-gpio";
                reg = <0x18140 0x40>, <0x181c8 0x08>;
                reg-names = "gpio", "pwm";
In this case, if gpio0 is not declared as PWM capable, the PWM
numbering will start at 32 if we have
mvpwm->chip.base = mvchip->chip.base;
but it will start at 0 if we have mvpwm->chip.base = -1;

The pros for having mvpwm->chip.base = mvchip->chip.base; is mainly
the stable numbering:
if we add the "pwm" feature to gpio0 afterwards, the pwm numbering in
sysfs will stay the same.
And if we have mvpwm->chip.base = -1; the pwm numbering will be shifted.

Looking back at the V5 of this patch
https://www.spinics.net/lists/kernel/msg2484889.html
There was the line:
mvpwm->chip.base = mvchip->chip.base;
I guess it got lost in the v6 rebase.

So I could change it back, but I'm not sure which one is better.

>
> I think that if you use base = -1, then the number start from (512 -
> number of pin already use). So starting from a low number for one
> compatible and a high number for an other compatible could be confusing.
>
> Besides that I agree that mvpwm->chip.base must be initialized and here
> again for adding mor context to this patch, we could add:
>
> Fixes: 757642f9a584 ("gpio: mvebu: Add limited PWM support")
yes, definitely !
should I resend the patch with it or the maintainer will add it ?

> Gregory
>
>>
>>       spin_lock_init(&mvpwm->lock);
>>
>
> --
> Gregory Clement, Free Electrons
> Kernel, drivers, real-time and embedded Linux
> development, consulting, training and support.
> http://free-electrons.com