| Message ID | 20230426220338.430638-1-andreas@kemnade.info (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | gpiolib: fix allocation of mixed dynamic/static GPIOs | expand |
Le 27/04/2023 à 00:03, Andreas Kemnade a écrit : > [Vous ne recevez pas souvent de courriers de andreas@kemnade.info. Découvrez pourquoi ceci est important à https://aka.ms/LearnAboutSenderIdentification ] > > If static allocation and dynamic allocation GPIOs are present, > dynamic allocation pollutes the numberspace for static allocation, > causing static allocation to fail. > Enfore dynamic allocation above GPIO_DYNAMIC_BASE. Hum .... Commit 7b61212f2a07 ("gpiolib: Get rid of ARCH_NR_GPIOS") was supposed to enforce dynamic allocation above GPIO_DYNAMIC_BASE already. Can you describe what is going wrong exactly with the above commit ? Thanks Christophe > > Seen on a GTA04 when omap-gpio (static) and twl-gpio (dynamic) > raced. > On that device it is fixed invasively by > commit 92bf78b33b0b4 ("gpio: omap: use dynamic allocation of base") > but lets also fix that for devices where there is still > a mixture of static and dynamic allocation. > > Fixes: 7b61212f2a07 ("gpiolib: Get rid of ARCH_NR_GPIOS") > Suggested-by: andy.shevchenko@gmail.com > Signed-off-by: Andreas Kemnade <andreas@kemnade.info> > --- > drivers/gpio/gpiolib.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > index 19bd23044b017..18b68d0aec7db 100644 > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -188,6 +188,10 @@ static int gpiochip_find_base(int ngpio) > int base = GPIO_DYNAMIC_BASE; > > list_for_each_entry(gdev, &gpio_devices, list) { > + /* do not pollute area for static allocation */ > + if (gdev->base < GPIO_DYNAMIC_BASE) > + continue; > + > /* found a free space? */ > if (gdev->base >= base + ngpio) > break; > -- > 2.39.2 >
On Thu, Apr 27, 2023 at 8:40 AM Christophe Leroy <christophe.leroy@csgroup.eu> wrote: > > > > Le 27/04/2023 à 00:03, Andreas Kemnade a écrit : > > [Vous ne recevez pas souvent de courriers de andreas@kemnade.info. Découvrez pourquoi ceci est important à https://aka.ms/LearnAboutSenderIdentification ] > > > > If static allocation and dynamic allocation GPIOs are present, > > dynamic allocation pollutes the numberspace for static allocation, > > causing static allocation to fail. > > Enfore dynamic allocation above GPIO_DYNAMIC_BASE. > > Hum .... > > Commit 7b61212f2a07 ("gpiolib: Get rid of ARCH_NR_GPIOS") was supposed > to enforce dynamic allocation above GPIO_DYNAMIC_BASE already. > > Can you describe what is going wrong exactly with the above commit ? Above commit only works to the first dynamic allocation, if you need more than one with static ones present it mistakenly will give you a base _below_ DYNAMIC_BASE. However, this change is just PoC I proposed, the conditional and action should be slightly different to cover a corner case, when statically allocated chip overlaps the DYNAMIC_BASE, i.e. gdev->base < DYNAMIC_BASE, while gdev->base + gdev->ngpio >= DYNAMIC_BASE.
Le 27/04/2023 à 08:00, Andy Shevchenko a écrit : > On Thu, Apr 27, 2023 at 8:40 AM Christophe Leroy > <christophe.leroy@csgroup.eu> wrote: >> >> >> >> Le 27/04/2023 à 00:03, Andreas Kemnade a écrit : >>> [Vous ne recevez pas souvent de courriers de andreas@kemnade.info. Découvrez pourquoi ceci est important à https://aka.ms/LearnAboutSenderIdentification ] >>> >>> If static allocation and dynamic allocation GPIOs are present, >>> dynamic allocation pollutes the numberspace for static allocation, >>> causing static allocation to fail. >>> Enfore dynamic allocation above GPIO_DYNAMIC_BASE. >> >> Hum .... >> >> Commit 7b61212f2a07 ("gpiolib: Get rid of ARCH_NR_GPIOS") was supposed >> to enforce dynamic allocation above GPIO_DYNAMIC_BASE already. >> >> Can you describe what is going wrong exactly with the above commit ? > > Above commit only works to the first dynamic allocation, if you need > more than one with static ones present it mistakenly will give you a > base _below_ DYNAMIC_BASE. Ah right, that needs to be fixed. > > However, this change is just PoC I proposed, the conditional and > action should be slightly different to cover a corner case, when > statically allocated chip overlaps the DYNAMIC_BASE, i.e. gdev->base < > DYNAMIC_BASE, while gdev->base + gdev->ngpio >= DYNAMIC_BASE. > Yes you are right, that's gdev->base + gdev->ngpio that should be checked. Christophe
On Thu, 27 Apr 2023 06:20:34 +0000 Christophe Leroy <christophe.leroy@csgroup.eu> wrote: > Le 27/04/2023 à 08:00, Andy Shevchenko a écrit : > > On Thu, Apr 27, 2023 at 8:40 AM Christophe Leroy > > <christophe.leroy@csgroup.eu> wrote: > >> > >> > >> > >> Le 27/04/2023 à 00:03, Andreas Kemnade a écrit : > >>> [Vous ne recevez pas souvent de courriers de andreas@kemnade.info. Découvrez pourquoi ceci est important à https://aka.ms/LearnAboutSenderIdentification ] > >>> > >>> If static allocation and dynamic allocation GPIOs are present, > >>> dynamic allocation pollutes the numberspace for static allocation, > >>> causing static allocation to fail. > >>> Enfore dynamic allocation above GPIO_DYNAMIC_BASE. > >> > >> Hum .... > >> > >> Commit 7b61212f2a07 ("gpiolib: Get rid of ARCH_NR_GPIOS") was supposed > >> to enforce dynamic allocation above GPIO_DYNAMIC_BASE already. > >> > >> Can you describe what is going wrong exactly with the above commit ? > > > > Above commit only works to the first dynamic allocation, if you need > > more than one with static ones present it mistakenly will give you a > > base _below_ DYNAMIC_BASE. > > Ah right, that needs to be fixed. > > > > > However, this change is just PoC I proposed, the conditional and > > action should be slightly different to cover a corner case, when > > statically allocated chip overlaps the DYNAMIC_BASE, i.e. gdev->base < > > DYNAMIC_BASE, while gdev->base + gdev->ngpio >= DYNAMIC_BASE. > > > > Yes you are right, that's gdev->base + gdev->ngpio that should be checked. > and that not with simple continue or base might simply stay at DYNAMIC_BASE. I will send a v2 of this patch with refined logic. Regards, Andreas
On Thu, Apr 27, 2023 at 1:37 PM Andreas Kemnade <andreas@kemnade.info> wrote: > > On Thu, 27 Apr 2023 06:20:34 +0000 > Christophe Leroy <christophe.leroy@csgroup.eu> wrote: > > > Le 27/04/2023 à 08:00, Andy Shevchenko a écrit : > > > On Thu, Apr 27, 2023 at 8:40 AM Christophe Leroy > > > <christophe.leroy@csgroup.eu> wrote: > > >> > > >> > > >> > > >> Le 27/04/2023 à 00:03, Andreas Kemnade a écrit : > > >>> [Vous ne recevez pas souvent de courriers de andreas@kemnade.info. Découvrez pourquoi ceci est important à https://aka.ms/LearnAboutSenderIdentification ] > > >>> > > >>> If static allocation and dynamic allocation GPIOs are present, > > >>> dynamic allocation pollutes the numberspace for static allocation, > > >>> causing static allocation to fail. > > >>> Enfore dynamic allocation above GPIO_DYNAMIC_BASE. > > >> > > >> Hum .... > > >> > > >> Commit 7b61212f2a07 ("gpiolib: Get rid of ARCH_NR_GPIOS") was supposed > > >> to enforce dynamic allocation above GPIO_DYNAMIC_BASE already. > > >> > > >> Can you describe what is going wrong exactly with the above commit ? > > > > > > Above commit only works to the first dynamic allocation, if you need > > > more than one with static ones present it mistakenly will give you a > > > base _below_ DYNAMIC_BASE. > > > > Ah right, that needs to be fixed. > > > > > > > > However, this change is just PoC I proposed, the conditional and > > > action should be slightly different to cover a corner case, when > > > statically allocated chip overlaps the DYNAMIC_BASE, i.e. gdev->base < > > > DYNAMIC_BASE, while gdev->base + gdev->ngpio >= DYNAMIC_BASE. > > > > > > > Yes you are right, that's gdev->base + gdev->ngpio that should be checked. > > > and that not with simple continue or base might simply stay at DYNAMIC_BASE. > > I will send a v2 of this patch with refined logic. Actually it would be nice to integrate a warning (if we don't have it yet) when adding a GPIO chip with a static allocation and which will overlap the dynamic base. Can you add that into your v2?
Le 27/04/2023 à 12:46, Andy Shevchenko a écrit : > On Thu, Apr 27, 2023 at 1:37 PM Andreas Kemnade <andreas@kemnade.info> wrote: >> >> On Thu, 27 Apr 2023 06:20:34 +0000 >> Christophe Leroy <christophe.leroy@csgroup.eu> wrote: >> >>> Le 27/04/2023 à 08:00, Andy Shevchenko a écrit : >>>> On Thu, Apr 27, 2023 at 8:40 AM Christophe Leroy >>>> <christophe.leroy@csgroup.eu> wrote: >>>>> >>>>> >>>>> >>>>> Le 27/04/2023 à 00:03, Andreas Kemnade a écrit : >>>>>> [Vous ne recevez pas souvent de courriers de andreas@kemnade.info. Découvrez pourquoi ceci est important à https://aka.ms/LearnAboutSenderIdentification ] >>>>>> >>>>>> If static allocation and dynamic allocation GPIOs are present, >>>>>> dynamic allocation pollutes the numberspace for static allocation, >>>>>> causing static allocation to fail. >>>>>> Enfore dynamic allocation above GPIO_DYNAMIC_BASE. >>>>> >>>>> Hum .... >>>>> >>>>> Commit 7b61212f2a07 ("gpiolib: Get rid of ARCH_NR_GPIOS") was supposed >>>>> to enforce dynamic allocation above GPIO_DYNAMIC_BASE already. >>>>> >>>>> Can you describe what is going wrong exactly with the above commit ? >>>> >>>> Above commit only works to the first dynamic allocation, if you need >>>> more than one with static ones present it mistakenly will give you a >>>> base _below_ DYNAMIC_BASE. >>> >>> Ah right, that needs to be fixed. >>> >>>> >>>> However, this change is just PoC I proposed, the conditional and >>>> action should be slightly different to cover a corner case, when >>>> statically allocated chip overlaps the DYNAMIC_BASE, i.e. gdev->base < >>>> DYNAMIC_BASE, while gdev->base + gdev->ngpio >= DYNAMIC_BASE. >>>> >>> >>> Yes you are right, that's gdev->base + gdev->ngpio that should be checked. >>> >> and that not with simple continue or base might simply stay at DYNAMIC_BASE. >> >> I will send a v2 of this patch with refined logic. > > Actually it would be nice to integrate a warning (if we don't have it > yet) when adding a GPIO chip with a static allocation and which will > overlap the dynamic base. Can you add that into your v2? > At the time being we have a warning for all static allocations, allthough their has been some discussion about reverting it, see commit 502df79b8605 ("gpiolib: Warn on drivers still using static gpiobase allocation") Christophe
On Thu, Apr 27, 2023 at 1:55 PM Christophe Leroy <christophe.leroy@csgroup.eu> wrote: > Le 27/04/2023 à 12:46, Andy Shevchenko a écrit : > > On Thu, Apr 27, 2023 at 1:37 PM Andreas Kemnade <andreas@kemnade.info> wrote: > >> On Thu, 27 Apr 2023 06:20:34 +0000 > >> Christophe Leroy <christophe.leroy@csgroup.eu> wrote: ... > >> I will send a v2 of this patch with refined logic. > > > > Actually it would be nice to integrate a warning (if we don't have it > > yet) when adding a GPIO chip with a static allocation and which will > > overlap the dynamic base. Can you add that into your v2? > > > > At the time being we have a warning for all static allocations, > allthough their has been some discussion about reverting it, see commit > 502df79b8605 ("gpiolib: Warn on drivers still using static gpiobase > allocation") Ah, even better! Then no need to have a specific one, thanks!
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 19bd23044b017..18b68d0aec7db 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -188,6 +188,10 @@ static int gpiochip_find_base(int ngpio) int base = GPIO_DYNAMIC_BASE; list_for_each_entry(gdev, &gpio_devices, list) { + /* do not pollute area for static allocation */ + if (gdev->base < GPIO_DYNAMIC_BASE) + continue; + /* found a free space? */ if (gdev->base >= base + ngpio) break;
If static allocation and dynamic allocation GPIOs are present, dynamic allocation pollutes the numberspace for static allocation, causing static allocation to fail. Enfore dynamic allocation above GPIO_DYNAMIC_BASE. Seen on a GTA04 when omap-gpio (static) and twl-gpio (dynamic) raced. On that device it is fixed invasively by commit 92bf78b33b0b4 ("gpio: omap: use dynamic allocation of base") but lets also fix that for devices where there is still a mixture of static and dynamic allocation. Fixes: 7b61212f2a07 ("gpiolib: Get rid of ARCH_NR_GPIOS") Suggested-by: andy.shevchenko@gmail.com Signed-off-by: Andreas Kemnade <andreas@kemnade.info> --- drivers/gpio/gpiolib.c | 4 ++++ 1 file changed, 4 insertions(+)