diff mbox series

[v3,3/5] gpio: gpiolib: Allow free() callback to be overridden

Message ID 20220511183210.5248-4-prabhakar.mahadev-lad.rj@bp.renesas.com (mailing list archive)
State Superseded
Delegated to: Geert Uytterhoeven
Headers show
Series Renesas RZ/G2L IRQC support | expand

Commit Message

Lad Prabhakar May 11, 2022, 6:32 p.m. UTC
Allow free() callback to be overridden from irq_domain_ops for
hierarchical chips.

This allows drivers to free any resources which are allocated during
populate_parent_alloc_arg().

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/gpio/gpiolib.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Comments

Geert Uytterhoeven May 12, 2022, 7:06 a.m. UTC | #1
On Wed, May 11, 2022 at 8:32 PM Lad Prabhakar
<prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> Allow free() callback to be overridden from irq_domain_ops for
> hierarchical chips.
>
> This allows drivers to free any resources which are allocated during
> populate_parent_alloc_arg().
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Marc Zyngier May 12, 2022, 11:19 a.m. UTC | #2
On Wed, 11 May 2022 19:32:08 +0100,
Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> 
> Allow free() callback to be overridden from irq_domain_ops for
> hierarchical chips.
> 
> This allows drivers to free any resources which are allocated during
> populate_parent_alloc_arg().

Do you mean more than the fwspec? I don't see this being used.

There is also the question of why we need to have dynamic allocation
for the fwspec itself. Why isn't that a simple stack allocation in the
context of gpiochip_hierarchy_irq_domain_alloc()?

	M.

> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
>  drivers/gpio/gpiolib.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index b7694171655c..d36c4a965efc 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1187,15 +1187,18 @@ static void gpiochip_hierarchy_setup_domain_ops(struct irq_domain_ops *ops)
>  	ops->activate = gpiochip_irq_domain_activate;
>  	ops->deactivate = gpiochip_irq_domain_deactivate;
>  	ops->alloc = gpiochip_hierarchy_irq_domain_alloc;
> -	ops->free = irq_domain_free_irqs_common;
>  
>  	/*
> -	 * We only allow overriding the translate() function for
> +	 * We only allow overriding the translate() and free() functions for
>  	 * hierarchical chips, and this should only be done if the user
> -	 * really need something other than 1:1 translation.
> +	 * really need something other than 1:1 translation for translate()
> +	 * callback and free if user wants to free up any resources which
> +	 * were allocated during callbacks, for example populate_parent_alloc_arg.
>  	 */
>  	if (!ops->translate)
>  		ops->translate = gpiochip_hierarchy_irq_domain_translate;
> +	if (!ops->free)
> +		ops->free = irq_domain_free_irqs_common;
>  }
>  
>  static int gpiochip_hierarchy_add_domain(struct gpio_chip *gc)
> -- 
> 2.25.1
> 
>
Lad, Prabhakar May 12, 2022, 12:48 p.m. UTC | #3
Hi Marc,

Thank you for the review.

On Thu, May 12, 2022 at 12:19 PM Marc Zyngier <maz@kernel.org> wrote:
>
> On Wed, 11 May 2022 19:32:08 +0100,
> Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> >
> > Allow free() callback to be overridden from irq_domain_ops for
> > hierarchical chips.
> >
> > This allows drivers to free any resources which are allocated during
> > populate_parent_alloc_arg().
>
> Do you mean more than the fwspec? I don't see this being used.
>
The free callback is used in patch 5/5 where free is overridden by
rzg2l_gpio_irq_domain_free. I just gave an example there as an
populate_parent_alloc_arg()  In actual in the child_to_parent_hwirq
callback I am using a bitmap [0] to get a free tint slot, this bitmap
needs freeing up when the GPIO interrupt is released from the driver
that as when overridden free callback frees the allocated tint slot so
that its available for re-use.

> There is also the question of why we need to have dynamic allocation
> for the fwspec itself. Why isn't that a simple stack allocation in the
> context of gpiochip_hierarchy_irq_domain_alloc()?
>
you mean gpio core itself should handle the fwspec allocation/freeing?

[0] https://patchwork.kernel.org/project/linux-renesas-soc/patch/20220511183210.5248-6-prabhakar.mahadev-lad.rj@bp.renesas.com/

Cheers,
Prabhakar

>         M.
>
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> >  drivers/gpio/gpiolib.c | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > index b7694171655c..d36c4a965efc 100644
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -1187,15 +1187,18 @@ static void gpiochip_hierarchy_setup_domain_ops(struct irq_domain_ops *ops)
> >       ops->activate = gpiochip_irq_domain_activate;
> >       ops->deactivate = gpiochip_irq_domain_deactivate;
> >       ops->alloc = gpiochip_hierarchy_irq_domain_alloc;
> > -     ops->free = irq_domain_free_irqs_common;
> >
> >       /*
> > -      * We only allow overriding the translate() function for
> > +      * We only allow overriding the translate() and free() functions for
> >        * hierarchical chips, and this should only be done if the user
> > -      * really need something other than 1:1 translation.
> > +      * really need something other than 1:1 translation for translate()
> > +      * callback and free if user wants to free up any resources which
> > +      * were allocated during callbacks, for example populate_parent_alloc_arg.
> >        */
> >       if (!ops->translate)
> >               ops->translate = gpiochip_hierarchy_irq_domain_translate;
> > +     if (!ops->free)
> > +             ops->free = irq_domain_free_irqs_common;
> >  }
> >
> >  static int gpiochip_hierarchy_add_domain(struct gpio_chip *gc)
> > --
> > 2.25.1
> >
> >
>
> --
> Without deviation from the norm, progress is not possible.
Marc Zyngier May 12, 2022, 1:24 p.m. UTC | #4
On Thu, 12 May 2022 13:48:53 +0100,
"Lad, Prabhakar" <prabhakar.csengg@gmail.com> wrote:
> 
> Hi Marc,
> 
> Thank you for the review.
> 
> On Thu, May 12, 2022 at 12:19 PM Marc Zyngier <maz@kernel.org> wrote:
> >
> > On Wed, 11 May 2022 19:32:08 +0100,
> > Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> > >
> > > Allow free() callback to be overridden from irq_domain_ops for
> > > hierarchical chips.
> > >
> > > This allows drivers to free any resources which are allocated during
> > > populate_parent_alloc_arg().
> >
> > Do you mean more than the fwspec? I don't see this being used.
> >
> The free callback is used in patch 5/5 where free is overridden by
> rzg2l_gpio_irq_domain_free. I just gave an example there as an
> populate_parent_alloc_arg()  In actual in the child_to_parent_hwirq
> callback I am using a bitmap [0] to get a free tint slot, this bitmap
> needs freeing up when the GPIO interrupt is released from the driver
> that as when overridden free callback frees the allocated tint slot so
> that its available for re-use.

Right, so that's actually a different life-cycle, and the whole
populate_parent_alloc_arg() is a red herring. What you want is to free
resources that have been allocated via some other paths. It'd be good
if your commit message actually reflected this instead of using an
example that doesn't actually exist.

> 
> > There is also the question of why we need to have dynamic allocation
> > for the fwspec itself. Why isn't that a simple stack allocation in the
> > context of gpiochip_hierarchy_irq_domain_alloc()?
> >
> you mean gpio core itself should handle the fwspec
> allocation/freeing?

Yes. The only reason we resort to dynamic allocation is because
ThunderX is using MSI-based GPIOs, and thus doesn't use a fwspec (no
firmware is involved here).

If we had a union of the two types, we could just have a stack
variable, and pass that along, completely sidestepping the whole
dynamic allocation/freeing business.

	M.
Lad, Prabhakar May 12, 2022, 1:50 p.m. UTC | #5
Hi Marc,

On Thu, May 12, 2022 at 2:24 PM Marc Zyngier <maz@kernel.org> wrote:
>
> On Thu, 12 May 2022 13:48:53 +0100,
> "Lad, Prabhakar" <prabhakar.csengg@gmail.com> wrote:
> >
> > Hi Marc,
> >
> > Thank you for the review.
> >
> > On Thu, May 12, 2022 at 12:19 PM Marc Zyngier <maz@kernel.org> wrote:
> > >
> > > On Wed, 11 May 2022 19:32:08 +0100,
> > > Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> > > >
> > > > Allow free() callback to be overridden from irq_domain_ops for
> > > > hierarchical chips.
> > > >
> > > > This allows drivers to free any resources which are allocated during
> > > > populate_parent_alloc_arg().
> > >
> > > Do you mean more than the fwspec? I don't see this being used.
> > >
> > The free callback is used in patch 5/5 where free is overridden by
> > rzg2l_gpio_irq_domain_free. I just gave an example there as an
> > populate_parent_alloc_arg()  In actual in the child_to_parent_hwirq
> > callback I am using a bitmap [0] to get a free tint slot, this bitmap
> > needs freeing up when the GPIO interrupt is released from the driver
> > that as when overridden free callback frees the allocated tint slot so
> > that its available for re-use.
>
> Right, so that's actually a different life-cycle, and the whole
> populate_parent_alloc_arg() is a red herring. What you want is to free
> resources that have been allocated via some other paths. It'd be good
Is there any other path which I have missed where I can free up resources?

> if your commit message actually reflected this instead of using an
> example that doesn't actually exist.
>
My bad, I will update the commit message.

> >
> > > There is also the question of why we need to have dynamic allocation
> > > for the fwspec itself. Why isn't that a simple stack allocation in the
> > > context of gpiochip_hierarchy_irq_domain_alloc()?
> > >
> > you mean gpio core itself should handle the fwspec
> > allocation/freeing?
>
> Yes. The only reason we resort to dynamic allocation is because
> ThunderX is using MSI-based GPIOs, and thus doesn't use a fwspec (no
> firmware is involved here).
>
I see..

> If we had a union of the two types, we could just have a stack
> variable, and pass that along, completely sidestepping the whole
> dynamic allocation/freeing business.
>
Right agreed.

Cheers,
Prabhakar
Marc Zyngier May 12, 2022, 4:26 p.m. UTC | #6
On Thu, 12 May 2022 14:50:05 +0100,
"Lad, Prabhakar" <prabhakar.csengg@gmail.com> wrote:
> 
> Hi Marc,
> 
> On Thu, May 12, 2022 at 2:24 PM Marc Zyngier <maz@kernel.org> wrote:
> >
> > On Thu, 12 May 2022 13:48:53 +0100,
> > "Lad, Prabhakar" <prabhakar.csengg@gmail.com> wrote:
> > >
> > > Hi Marc,
> > >
> > > Thank you for the review.
> > >
> > > On Thu, May 12, 2022 at 12:19 PM Marc Zyngier <maz@kernel.org> wrote:
> > > >
> > > > On Wed, 11 May 2022 19:32:08 +0100,
> > > > Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> > > > >
> > > > > Allow free() callback to be overridden from irq_domain_ops for
> > > > > hierarchical chips.
> > > > >
> > > > > This allows drivers to free any resources which are allocated during
> > > > > populate_parent_alloc_arg().
> > > >
> > > > Do you mean more than the fwspec? I don't see this being used.
> > > >
> > > The free callback is used in patch 5/5 where free is overridden by
> > > rzg2l_gpio_irq_domain_free. I just gave an example there as an
> > > populate_parent_alloc_arg()  In actual in the child_to_parent_hwirq
> > > callback I am using a bitmap [0] to get a free tint slot, this bitmap
> > > needs freeing up when the GPIO interrupt is released from the driver
> > > that as when overridden free callback frees the allocated tint slot so
> > > that its available for re-use.
> >
> > Right, so that's actually a different life-cycle, and the whole
> > populate_parent_alloc_arg() is a red herring. What you want is to free
> > resources that have been allocated via some other paths. It'd be good
> Is there any other path which I have missed where I can free up resources?

No, that's the only one. It is just that usually, the alloc()
callback is where you are supposed to perform... allocations.

It'd be good if you could move your allocation there, as I would
expect calls to child_to_parent_hwirq() to be idempotent.

>
> > if your commit message actually reflected this instead of using an
> > example that doesn't actually exist.
> >
> My bad, I will update the commit message.
> 
> > >
> > > > There is also the question of why we need to have dynamic allocation
> > > > for the fwspec itself. Why isn't that a simple stack allocation in the
> > > > context of gpiochip_hierarchy_irq_domain_alloc()?
> > > >
> > > you mean gpio core itself should handle the fwspec
> > > allocation/freeing?
> >
> > Yes. The only reason we resort to dynamic allocation is because
> > ThunderX is using MSI-based GPIOs, and thus doesn't use a fwspec (no
> > firmware is involved here).
> >
> I see..
> 
> > If we had a union of the two types, we could just have a stack
> > variable, and pass that along, completely sidestepping the whole
> > dynamic allocation/freeing business.
> >
> Right agreed.

FWIW, I've just posted a PoC patch[1].

Thanks,

	M.

[1] https://lore.kernel.org/r/20220512162320.2213488-1-maz@kernel.org
Lad, Prabhakar May 12, 2022, 5:55 p.m. UTC | #7
Hi Marc,

On Thu, May 12, 2022 at 5:26 PM Marc Zyngier <maz@kernel.org> wrote:
>
> On Thu, 12 May 2022 14:50:05 +0100,
> "Lad, Prabhakar" <prabhakar.csengg@gmail.com> wrote:
> >
> > Hi Marc,
> >
> > On Thu, May 12, 2022 at 2:24 PM Marc Zyngier <maz@kernel.org> wrote:
> > >
> > > On Thu, 12 May 2022 13:48:53 +0100,
> > > "Lad, Prabhakar" <prabhakar.csengg@gmail.com> wrote:
> > > >
> > > > Hi Marc,
> > > >
> > > > Thank you for the review.
> > > >
> > > > On Thu, May 12, 2022 at 12:19 PM Marc Zyngier <maz@kernel.org> wrote:
> > > > >
> > > > > On Wed, 11 May 2022 19:32:08 +0100,
> > > > > Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> > > > > >
> > > > > > Allow free() callback to be overridden from irq_domain_ops for
> > > > > > hierarchical chips.
> > > > > >
> > > > > > This allows drivers to free any resources which are allocated during
> > > > > > populate_parent_alloc_arg().
> > > > >
> > > > > Do you mean more than the fwspec? I don't see this being used.
> > > > >
> > > > The free callback is used in patch 5/5 where free is overridden by
> > > > rzg2l_gpio_irq_domain_free. I just gave an example there as an
> > > > populate_parent_alloc_arg()  In actual in the child_to_parent_hwirq
> > > > callback I am using a bitmap [0] to get a free tint slot, this bitmap
> > > > needs freeing up when the GPIO interrupt is released from the driver
> > > > that as when overridden free callback frees the allocated tint slot so
> > > > that its available for re-use.
> > >
> > > Right, so that's actually a different life-cycle, and the whole
> > > populate_parent_alloc_arg() is a red herring. What you want is to free
> > > resources that have been allocated via some other paths. It'd be good
> > Is there any other path which I have missed where I can free up resources?
>
> No, that's the only one. It is just that usually, the alloc()
> callback is where you are supposed to perform... allocations.
>
OK.

> It'd be good if you could move your allocation there, as I would
> expect calls to child_to_parent_hwirq() to be idempotent.
>
For now I'll go with the current implementation, as currently a an
array is maintained which is tied with the tint slot and child (which
is obtained from child_to_parent_hwirq)

> >
> > > if your commit message actually reflected this instead of using an
> > > example that doesn't actually exist.
> > >
> > My bad, I will update the commit message.
> >
> > > >
> > > > > There is also the question of why we need to have dynamic allocation
> > > > > for the fwspec itself. Why isn't that a simple stack allocation in the
> > > > > context of gpiochip_hierarchy_irq_domain_alloc()?
> > > > >
> > > > you mean gpio core itself should handle the fwspec
> > > > allocation/freeing?
> > >
> > > Yes. The only reason we resort to dynamic allocation is because
> > > ThunderX is using MSI-based GPIOs, and thus doesn't use a fwspec (no
> > > firmware is involved here).
> > >
> > I see..
> >
> > > If we had a union of the two types, we could just have a stack
> > > variable, and pass that along, completely sidestepping the whole
> > > dynamic allocation/freeing business.
> > >
> > Right agreed.
>
> FWIW, I've just posted a PoC patch[1].
>
I guess I'll have to rebase my changes on top of it now ;)

Cheers,
Prabhakar
Marc Zyngier May 12, 2022, 10:24 p.m. UTC | #8
On Thu, 12 May 2022 18:55:38 +0100,
"Lad, Prabhakar" <prabhakar.csengg@gmail.com> wrote:
> 
> Hi Marc,
> 
> On Thu, May 12, 2022 at 5:26 PM Marc Zyngier <maz@kernel.org> wrote:
> >
> > On Thu, 12 May 2022 14:50:05 +0100,
> > "Lad, Prabhakar" <prabhakar.csengg@gmail.com> wrote:
> > >
> > > Hi Marc,
> > >
> > > On Thu, May 12, 2022 at 2:24 PM Marc Zyngier <maz@kernel.org> wrote:
> > > >
> > > > On Thu, 12 May 2022 13:48:53 +0100,
> > > > "Lad, Prabhakar" <prabhakar.csengg@gmail.com> wrote:
> > > > >
> > > > > Hi Marc,
> > > > >
> > > > > Thank you for the review.
> > > > >
> > > > > On Thu, May 12, 2022 at 12:19 PM Marc Zyngier <maz@kernel.org> wrote:
> > > > > >
> > > > > > On Wed, 11 May 2022 19:32:08 +0100,
> > > > > > Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> > > > > > >
> > > > > > > Allow free() callback to be overridden from irq_domain_ops for
> > > > > > > hierarchical chips.
> > > > > > >
> > > > > > > This allows drivers to free any resources which are allocated during
> > > > > > > populate_parent_alloc_arg().
> > > > > >
> > > > > > Do you mean more than the fwspec? I don't see this being used.
> > > > > >
> > > > > The free callback is used in patch 5/5 where free is overridden by
> > > > > rzg2l_gpio_irq_domain_free. I just gave an example there as an
> > > > > populate_parent_alloc_arg()  In actual in the child_to_parent_hwirq
> > > > > callback I am using a bitmap [0] to get a free tint slot, this bitmap
> > > > > needs freeing up when the GPIO interrupt is released from the driver
> > > > > that as when overridden free callback frees the allocated tint slot so
> > > > > that its available for re-use.
> > > >
> > > > Right, so that's actually a different life-cycle, and the whole
> > > > populate_parent_alloc_arg() is a red herring. What you want is to free
> > > > resources that have been allocated via some other paths. It'd be good
> > > Is there any other path which I have missed where I can free up resources?
> >
> > No, that's the only one. It is just that usually, the alloc()
> > callback is where you are supposed to perform... allocations.
> >
> OK.
> 
> > It'd be good if you could move your allocation there, as I would
> > expect calls to child_to_parent_hwirq() to be idempotent.
> >
> For now I'll go with the current implementation, as currently a an
> array is maintained which is tied with the tint slot and child (which
> is obtained from child_to_parent_hwirq)
> 
> > >
> > > > if your commit message actually reflected this instead of using an
> > > > example that doesn't actually exist.
> > > >
> > > My bad, I will update the commit message.
> > >
> > > > >
> > > > > > There is also the question of why we need to have dynamic allocation
> > > > > > for the fwspec itself. Why isn't that a simple stack allocation in the
> > > > > > context of gpiochip_hierarchy_irq_domain_alloc()?
> > > > > >
> > > > > you mean gpio core itself should handle the fwspec
> > > > > allocation/freeing?
> > > >
> > > > Yes. The only reason we resort to dynamic allocation is because
> > > > ThunderX is using MSI-based GPIOs, and thus doesn't use a fwspec (no
> > > > firmware is involved here).
> > > >
> > > I see..
> > >
> > > > If we had a union of the two types, we could just have a stack
> > > > variable, and pass that along, completely sidestepping the whole
> > > > dynamic allocation/freeing business.
> > > >
> > > Right agreed.
> >
> > FWIW, I've just posted a PoC patch[1].
> >
> I guess I'll have to rebase my changes on top of it now ;)

Not yet. Let's see what people say about it.

	M.
diff mbox series

Patch

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index b7694171655c..d36c4a965efc 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1187,15 +1187,18 @@  static void gpiochip_hierarchy_setup_domain_ops(struct irq_domain_ops *ops)
 	ops->activate = gpiochip_irq_domain_activate;
 	ops->deactivate = gpiochip_irq_domain_deactivate;
 	ops->alloc = gpiochip_hierarchy_irq_domain_alloc;
-	ops->free = irq_domain_free_irqs_common;
 
 	/*
-	 * We only allow overriding the translate() function for
+	 * We only allow overriding the translate() and free() functions for
 	 * hierarchical chips, and this should only be done if the user
-	 * really need something other than 1:1 translation.
+	 * really need something other than 1:1 translation for translate()
+	 * callback and free if user wants to free up any resources which
+	 * were allocated during callbacks, for example populate_parent_alloc_arg.
 	 */
 	if (!ops->translate)
 		ops->translate = gpiochip_hierarchy_irq_domain_translate;
+	if (!ops->free)
+		ops->free = irq_domain_free_irqs_common;
 }
 
 static int gpiochip_hierarchy_add_domain(struct gpio_chip *gc)