| Message ID | 20220405135444.199295-1-maz@kernel.org |
|---|---|
| Headers | show |
| Series | gpiolib: Handle immutable irq_chip structures | expand |
On Wed, Apr 6, 2022 at 2:06 PM Marc Zyngier <maz@kernel.org> wrote: > > This is a followup from [1]. > > 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? Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> for the patches 1-3,9 and 10 after clarifying one example case. > * 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 > > 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(-) > > -- > 2.34.1 >
On 2022-04-08 12:33, Andy Shevchenko wrote: > On Wed, Apr 6, 2022 at 1:57 PM Marc Zyngier <maz@kernel.org> wrote: >> >> Update the documentation to get rid of the per-gpio_irq_chip >> irq_chip structure, and give examples of the new pattern. > > ... > >> + static void my_gpio_mask_irq(struct irq_data *d) >> + { >> + struct gpio_chip *gc = irq_desc_get_handler_data(d); >> + >> + /* >> + * Perform any necessary action to mask the interrupt, >> + * and then call into the core code to synchronise the >> + * state. >> + */ >> + >> + gpiochip_disable_irq(gc, d->hwirq); > > (1) > >> + } >> + >> + static void my_gpio_unmask_irq(struct irq_data *d) >> + { >> + struct gpio_chip *gc = irq_desc_get_handler_data(d); > >> + gpiochip_disable_irq(gc, d->hwirq); > > (2) > >> + /* >> + * Perform any necessary action to unmask the interrupt, >> + * after having called into the core code to synchronise >> + * the state. >> + */ >> + } > > If (1) and (2) being the same is not a mistake (typo) it probably > needs additional explanation. Well spotted, this is definitely a copy-paste issue. I'll fix this locally. Thanks, M.
On Tue, Apr 5, 2022 at 3:55 PM Marc Zyngier <maz@kernel.org> wrote: > > This is a followup from [1]. > > 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? > > * 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 > > 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(-) > > -- > 2.34.1 > This may be coming too late but for the GPIO part: Reviewed-by: Bartosz Golaszewski <brgl@bgdev.pl>
On Tue, Apr 5, 2022 at 3:55 PM Marc Zyngier <maz@kernel.org> wrote: > 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? > > * 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 > > 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 Acked-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij