mbox series

[v3,00/10] gpiolib: Handle immutable irq_chip structures

Message ID 20220419141846.598305-1-maz@kernel.org
Headers show
Series gpiolib: Handle immutable irq_chip structures | expand

Message

Marc Zyngier April 19, 2022, 2:18 p.m. UTC
This is a followup from [2].

I recently realised that the gpiolib play ugly tricks on the
unsuspecting irq_chip structures by patching the callbacks.

Not only this breaks when an irq_chip structure is made const (which
really should be the default case), but it also forces this structure
to be copied at nauseam for each instance of the GPIO block, which is
a waste of memory.

My current approach is to add a new irq_chip flag (IRQCHIP_IMMUTABLE)
which does what it says on the tin: don't you dare writing to them.
Gpiolib is further updated not to install its own callbacks, and it
becomes the responsibility of the driver to call into the gpiolib when
required. This is similar to what we do for other subsystems such as
PCI-MSI.

5 drivers are updated to this new model: M1, QC, Tegra, pl061 and AMD
(as I actively use them) keeping a single irq_chip structure, marking
it const, and exposing the new flag.

Nothing breaks, the volume of change is small, the memory usage goes
down and we have fewer callbacks that can be used as attack vectors.
What's not to love?

Since there wasn't any objection in the previous round of review, I'm
going to take this series into -next to see if anything breaks at
scale.

Thanks,

	M.

* From v2 [2]:
  - Fixed documentation
  - Collected RBs, with thanks

* From v1 [1]:
  - pl061 and AMD drivers converted
  - New helpers to keep the changes small
  - New warning for non-converted drivers
  - Documentation and TODO updates

[1] https://lore.kernel.org/r/20220223154405.54912-1-maz@kernel.org
[2] https://lore.kernel.org/r/20220405135444.199295-1-maz@kernel.org

Marc Zyngier (10):
  gpio: Don't fiddle with irqchips marked as immutable
  gpio: Expose the gpiochip_irq_re[ql]res helpers
  gpio: Add helpers to ease the transition towards immutable irq_chip
  gpio: tegra186: Make the irqchip immutable
  gpio: pl061: Make the irqchip immutable
  pinctrl: apple-gpio: Make the irqchip immutable
  pinctrl: msmgpio: Make the irqchip immutable
  pinctrl: amd: Make the irqchip immutable
  gpio: Update TODO to mention immutable irq_chip structures
  Documentation: Update the recommended pattern for GPIO irqchips

 Documentation/driver-api/gpio/driver.rst | 175 ++++++++++++++++++-----
 drivers/gpio/TODO                        |  19 +++
 drivers/gpio/gpio-pl061.c                |  32 +++--
 drivers/gpio/gpio-tegra186.c             |  32 +++--
 drivers/gpio/gpiolib.c                   |  13 +-
 drivers/pinctrl/pinctrl-amd.c            |  11 +-
 drivers/pinctrl/pinctrl-apple-gpio.c     |  29 ++--
 drivers/pinctrl/qcom/pinctrl-msm.c       |  53 ++++---
 include/linux/gpio/driver.h              |  16 +++
 include/linux/irq.h                      |   2 +
 kernel/irq/debugfs.c                     |   1 +
 11 files changed, 293 insertions(+), 90 deletions(-)

Comments

Linus Walleij April 22, 2022, 9:24 p.m. UTC | #1
On Tue, Apr 19, 2022 at 4:19 PM Marc Zyngier <maz@kernel.org> wrote:

> This is a followup from [2].
>
> I recently realised that the gpiolib play ugly tricks on the
> unsuspecting irq_chip structures by patching the callbacks.
>
> Not only this breaks when an irq_chip structure is made const (which
> really should be the default case), but it also forces this structure
> to be copied at nauseam for each instance of the GPIO block, which is
> a waste of memory.
>
> My current approach is to add a new irq_chip flag (IRQCHIP_IMMUTABLE)
> which does what it says on the tin: don't you dare writing to them.
> Gpiolib is further updated not to install its own callbacks, and it
> becomes the responsibility of the driver to call into the gpiolib when
> required. This is similar to what we do for other subsystems such as
> PCI-MSI.
>
> 5 drivers are updated to this new model: M1, QC, Tegra, pl061 and AMD
> (as I actively use them) keeping a single irq_chip structure, marking
> it const, and exposing the new flag.
>
> Nothing breaks, the volume of change is small, the memory usage goes
> down and we have fewer callbacks that can be used as attack vectors.
> What's not to love?
>
> Since there wasn't any objection in the previous round of review, I'm
> going to take this series into -next to see if anything breaks at
> scale.

The series:
Acked-by: Linus Walleij <linus.walleij@linaro.org>

Bartosz: if you're happy with this can you apply it to an immutable branch
from v5.18-rc1 and merge that into the GPIO for-next and then I can also
pull that into pinctrl?

Yours,
Linus Walleij
Marc Zyngier April 23, 2022, 10:30 a.m. UTC | #2
On Fri, 22 Apr 2022 22:24:22 +0100,
Linus Walleij <linus.walleij@linaro.org> wrote:
> 
> On Tue, Apr 19, 2022 at 4:19 PM Marc Zyngier <maz@kernel.org> wrote:
> 
> > This is a followup from [2].
> >
> > I recently realised that the gpiolib play ugly tricks on the
> > unsuspecting irq_chip structures by patching the callbacks.
> >
> > Not only this breaks when an irq_chip structure is made const (which
> > really should be the default case), but it also forces this structure
> > to be copied at nauseam for each instance of the GPIO block, which is
> > a waste of memory.
> >
> > My current approach is to add a new irq_chip flag (IRQCHIP_IMMUTABLE)
> > which does what it says on the tin: don't you dare writing to them.
> > Gpiolib is further updated not to install its own callbacks, and it
> > becomes the responsibility of the driver to call into the gpiolib when
> > required. This is similar to what we do for other subsystems such as
> > PCI-MSI.
> >
> > 5 drivers are updated to this new model: M1, QC, Tegra, pl061 and AMD
> > (as I actively use them) keeping a single irq_chip structure, marking
> > it const, and exposing the new flag.
> >
> > Nothing breaks, the volume of change is small, the memory usage goes
> > down and we have fewer callbacks that can be used as attack vectors.
> > What's not to love?
> >
> > Since there wasn't any objection in the previous round of review, I'm
> > going to take this series into -next to see if anything breaks at
> > scale.
> 
> The series:
> Acked-by: Linus Walleij <linus.walleij@linaro.org>
> 
> Bartosz: if you're happy with this can you apply it to an immutable branch
> from v5.18-rc1 and merge that into the GPIO for-next and then I can also
> pull that into pinctrl?

For what it is worth, I've pushed this branch into irqchip-next.

You can pick it up from:

https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=irq/gpio-immutable

but I can also drop it from the irqchip tree.

Just let me know.

	M.
Andy Shevchenko April 26, 2022, 10:24 a.m. UTC | #3
On Sat, Apr 23, 2022 at 12:30 PM Marc Zyngier <maz@kernel.org> wrote:
>
> On Fri, 22 Apr 2022 22:24:22 +0100,
> Linus Walleij <linus.walleij@linaro.org> wrote:
> >
> > On Tue, Apr 19, 2022 at 4:19 PM Marc Zyngier <maz@kernel.org> wrote:
> >
> > > This is a followup from [2].
> > >
> > > I recently realised that the gpiolib play ugly tricks on the
> > > unsuspecting irq_chip structures by patching the callbacks.
> > >
> > > Not only this breaks when an irq_chip structure is made const (which
> > > really should be the default case), but it also forces this structure
> > > to be copied at nauseam for each instance of the GPIO block, which is
> > > a waste of memory.
> > >
> > > My current approach is to add a new irq_chip flag (IRQCHIP_IMMUTABLE)
> > > which does what it says on the tin: don't you dare writing to them.
> > > Gpiolib is further updated not to install its own callbacks, and it
> > > becomes the responsibility of the driver to call into the gpiolib when
> > > required. This is similar to what we do for other subsystems such as
> > > PCI-MSI.
> > >
> > > 5 drivers are updated to this new model: M1, QC, Tegra, pl061 and AMD
> > > (as I actively use them) keeping a single irq_chip structure, marking
> > > it const, and exposing the new flag.
> > >
> > > Nothing breaks, the volume of change is small, the memory usage goes
> > > down and we have fewer callbacks that can be used as attack vectors.
> > > What's not to love?
> > >
> > > Since there wasn't any objection in the previous round of review, I'm
> > > going to take this series into -next to see if anything breaks at
> > > scale.
> >
> > The series:
> > Acked-by: Linus Walleij <linus.walleij@linaro.org>
> >
> > Bartosz: if you're happy with this can you apply it to an immutable branch
> > from v5.18-rc1 and merge that into the GPIO for-next and then I can also
> > pull that into pinctrl?
>
> For what it is worth, I've pushed this branch into irqchip-next.
>
> You can pick it up from:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=irq/gpio-immutable
>
> but I can also drop it from the irqchip tree.
>
> Just let me know.

I would prefer it if it goes as is now and every stakeholder can just
pull it. As far as my drivers are concerned I also want to convert
them sooner than later, meaning I want to pull this into my little
tree as well. Bart, Linus, would it be also preferable for you?
Linus Walleij April 26, 2022, 9:59 p.m. UTC | #4
On Tue, Apr 26, 2022 at 12:25 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Sat, Apr 23, 2022 at 12:30 PM Marc Zyngier <maz@kernel.org> wrote:

> > You can pick it up from:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=irq/gpio-immutable
> >
> > but I can also drop it from the irqchip tree.
> >
> > Just let me know.

Pulling that looks good to me.

If Bartosz pulls this to the GPIO tree I will also pull it to pin control,
I just want him to decide, because the impact is biggest on GPIO.

> I would prefer it if it goes as is now and every stakeholder can just
> pull it. As far as my drivers are concerned I also want to convert
> them sooner than later, meaning I want to pull this into my little
> tree as well. Bart, Linus, would it be also preferable for you?

I'd say let's all three pull this branch, Bart?

Yours,
Linus Walleij
Andy Shevchenko May 12, 2022, 5:08 p.m. UTC | #5
On Tue, Apr 19, 2022 at 03:18:36PM +0100, Marc Zyngier wrote:
> This is a followup from [2].
> 
> I recently realised that the gpiolib play ugly tricks on the
> unsuspecting irq_chip structures by patching the callbacks.
> 
> Not only this breaks when an irq_chip structure is made const (which
> really should be the default case), but it also forces this structure
> to be copied at nauseam for each instance of the GPIO block, which is
> a waste of memory.

Is this brings us to the issue with IRQ chip name?

The use case in my mind is the following:
1) we have two or more GPIO chips that supports IRQ;
2) the user registers two IRQs of the same (by number) pin on different chips;
3) cat /proc/interrupt will show 'my_gpio_chip XX', where XX is the number.

So, do I understand correct current state of affairs?

If so, we have to fix this to have any kind of ID added to the chip name that
we can map /proc/interrupts output correctly.

> My current approach is to add a new irq_chip flag (IRQCHIP_IMMUTABLE)
> which does what it says on the tin: don't you dare writing to them.
> Gpiolib is further updated not to install its own callbacks, and it
> becomes the responsibility of the driver to call into the gpiolib when
> required. This is similar to what we do for other subsystems such as
> PCI-MSI.
> 
> 5 drivers are updated to this new model: M1, QC, Tegra, pl061 and AMD
> (as I actively use them) keeping a single irq_chip structure, marking
> it const, and exposing the new flag.
> 
> Nothing breaks, the volume of change is small, the memory usage goes
> down and we have fewer callbacks that can be used as attack vectors.
> What's not to love?
> 
> Since there wasn't any objection in the previous round of review, I'm
> going to take this series into -next to see if anything breaks at
> scale.
Andy Shevchenko May 12, 2022, 5:35 p.m. UTC | #6
On Thu, May 12, 2022 at 08:08:28PM +0300, Andy Shevchenko wrote:
> On Tue, Apr 19, 2022 at 03:18:36PM +0100, Marc Zyngier wrote:
> > This is a followup from [2].
> > 
> > I recently realised that the gpiolib play ugly tricks on the
> > unsuspecting irq_chip structures by patching the callbacks.
> > 
> > Not only this breaks when an irq_chip structure is made const (which
> > really should be the default case), but it also forces this structure
> > to be copied at nauseam for each instance of the GPIO block, which is
> > a waste of memory.
> 
> Is this brings us to the issue with IRQ chip name?
> 
> The use case in my mind is the following:
> 1) we have two or more GPIO chips that supports IRQ;
> 2) the user registers two IRQs of the same (by number) pin on different chips;
> 3) cat /proc/interrupt will show 'my_gpio_chip XX', where XX is the number.
> 
> So, do I understand correct current state of affairs?
> 
> If so, we have to fix this to have any kind of ID added to the chip name that
> we can map /proc/interrupts output correctly.

Hmm... Some drivers are using static names, some -- dynamically prepared (one
way or another). Either way I think the ID is good to have if we still miss it.
Marc Zyngier May 12, 2022, 10:15 p.m. UTC | #7
On Thu, 12 May 2022 18:08:28 +0100,
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> 
> On Tue, Apr 19, 2022 at 03:18:36PM +0100, Marc Zyngier wrote:
> > This is a followup from [2].
> > 
> > I recently realised that the gpiolib play ugly tricks on the
> > unsuspecting irq_chip structures by patching the callbacks.
> > 
> > Not only this breaks when an irq_chip structure is made const (which
> > really should be the default case), but it also forces this structure
> > to be copied at nauseam for each instance of the GPIO block, which is
> > a waste of memory.
> 
> Is this brings us to the issue with IRQ chip name?
> 
> The use case in my mind is the following:
> 1) we have two or more GPIO chips that supports IRQ;
> 2) the user registers two IRQs of the same (by number) pin on different chips;
> 3) cat /proc/interrupt will show 'my_gpio_chip XX', where XX is the number.

/proc/interrupts isn't a dumping ground for debug information. Yes,
some irqchips do that, and they have been fixed by providing the
irq_print_chip callback, thus ensuring that the irq_chip structure is
never written to. I would have loved to simply get rid of the variable
string, but this is obviously ABI, and we can't break that.

> So, do I understand correct current state of affairs?
> 
> If so, we have to fix this to have any kind of ID added to the chip name that
> we can map /proc/interrupts output correctly.

This is already done.

That's not an excuse to add more of those though. We already have the
required infrastructure in debugfs, and that's where this sort of
thing should happen.

	M.
Marc Zyngier May 12, 2022, 10:18 p.m. UTC | #8
On Thu, 12 May 2022 18:35:55 +0100,
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> 
> On Thu, May 12, 2022 at 08:08:28PM +0300, Andy Shevchenko wrote:
> > On Tue, Apr 19, 2022 at 03:18:36PM +0100, Marc Zyngier wrote:
> > > This is a followup from [2].
> > > 
> > > I recently realised that the gpiolib play ugly tricks on the
> > > unsuspecting irq_chip structures by patching the callbacks.
> > > 
> > > Not only this breaks when an irq_chip structure is made const (which
> > > really should be the default case), but it also forces this structure
> > > to be copied at nauseam for each instance of the GPIO block, which is
> > > a waste of memory.
> > 
> > Is this brings us to the issue with IRQ chip name?
> > 
> > The use case in my mind is the following:
> > 1) we have two or more GPIO chips that supports IRQ;
> > 2) the user registers two IRQs of the same (by number) pin on different chips;
> > 3) cat /proc/interrupt will show 'my_gpio_chip XX', where XX is the number.
> > 
> > So, do I understand correct current state of affairs?
> > 
> > If so, we have to fix this to have any kind of ID added to the chip name that
> > we can map /proc/interrupts output correctly.
> 
> Hmm... Some drivers are using static names, some -- dynamically
> prepared (one way or another). Either way I think the ID is good to
> have if we still miss it.

No, this is a terrible idea. /proc/interrupts gives you a hint of
which driver/subsystem deals with the interrupt. This isn't a source
of topological information. /sys/kernel/debug/irq has all the
information you can dream of, and much more. Just make use of it.

	M.
Andy Shevchenko May 13, 2022, 8:43 a.m. UTC | #9
On Fri, May 13, 2022 at 12:18 AM Marc Zyngier <maz@kernel.org> wrote:
> On Thu, 12 May 2022 18:35:55 +0100,
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> >
> > On Thu, May 12, 2022 at 08:08:28PM +0300, Andy Shevchenko wrote:
> > > On Tue, Apr 19, 2022 at 03:18:36PM +0100, Marc Zyngier wrote:
> > > > This is a followup from [2].
> > > >
> > > > I recently realised that the gpiolib play ugly tricks on the
> > > > unsuspecting irq_chip structures by patching the callbacks.
> > > >
> > > > Not only this breaks when an irq_chip structure is made const (which
> > > > really should be the default case), but it also forces this structure
> > > > to be copied at nauseam for each instance of the GPIO block, which is
> > > > a waste of memory.
> > >
> > > Is this brings us to the issue with IRQ chip name?
> > >
> > > The use case in my mind is the following:
> > > 1) we have two or more GPIO chips that supports IRQ;
> > > 2) the user registers two IRQs of the same (by number) pin on different chips;
> > > 3) cat /proc/interrupt will show 'my_gpio_chip XX', where XX is the number.
> > >
> > > So, do I understand correct current state of affairs?
> > >
> > > If so, we have to fix this to have any kind of ID added to the chip name that
> > > we can map /proc/interrupts output correctly.
> >
> > Hmm... Some drivers are using static names, some -- dynamically
> > prepared (one way or another). Either way I think the ID is good to
> > have if we still miss it.
>
> No, this is a terrible idea. /proc/interrupts gives you a hint of
> which driver/subsystem deals with the interrupt. This isn't a source
> of topological information. /sys/kernel/debug/irq has all the
> information you can dream of, and much more. Just make use of it.

Okay, so IIUC the mapping is that: I got a vIRQ number from
/proc/interrupts, but then I have to mount debugfs and look into it
for a detailed answer of which chip/domain this vIRQ belongs to. Also
/sys/kernel/irq rings a bell, but not sure if it's related.
Marc Zyngier May 13, 2022, 8:51 a.m. UTC | #10
On Fri, 13 May 2022 09:43:29 +0100,
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> 
> On Fri, May 13, 2022 at 12:18 AM Marc Zyngier <maz@kernel.org> wrote:
> > On Thu, 12 May 2022 18:35:55 +0100,
> > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > >
> > > On Thu, May 12, 2022 at 08:08:28PM +0300, Andy Shevchenko wrote:
> > > > On Tue, Apr 19, 2022 at 03:18:36PM +0100, Marc Zyngier wrote:
> > > > > This is a followup from [2].
> > > > >
> > > > > I recently realised that the gpiolib play ugly tricks on the
> > > > > unsuspecting irq_chip structures by patching the callbacks.
> > > > >
> > > > > Not only this breaks when an irq_chip structure is made const (which
> > > > > really should be the default case), but it also forces this structure
> > > > > to be copied at nauseam for each instance of the GPIO block, which is
> > > > > a waste of memory.
> > > >
> > > > Is this brings us to the issue with IRQ chip name?
> > > >
> > > > The use case in my mind is the following:
> > > > 1) we have two or more GPIO chips that supports IRQ;
> > > > 2) the user registers two IRQs of the same (by number) pin on different chips;
> > > > 3) cat /proc/interrupt will show 'my_gpio_chip XX', where XX is the number.
> > > >
> > > > So, do I understand correct current state of affairs?
> > > >
> > > > If so, we have to fix this to have any kind of ID added to the chip name that
> > > > we can map /proc/interrupts output correctly.
> > >
> > > Hmm... Some drivers are using static names, some -- dynamically
> > > prepared (one way or another). Either way I think the ID is good to
> > > have if we still miss it.
> >
> > No, this is a terrible idea. /proc/interrupts gives you a hint of
> > which driver/subsystem deals with the interrupt. This isn't a source
> > of topological information. /sys/kernel/debug/irq has all the
> > information you can dream of, and much more. Just make use of it.
> 
> Okay, so IIUC the mapping is that: I got a vIRQ number from
> /proc/interrupts, but then I have to mount debugfs and look into it
> for a detailed answer of which chip/domain this vIRQ belongs to.

Normal users shouldn't care about irqdomains. If you are developing,
you already have debugfs enabled and mounted on your system.

> Also /sys/kernel/irq rings a bell, but not sure if it's related.

This is the same thing as /proc/interrupt, just dumped with a
different format.

	M.