gpiolib: Handle immutable irq_chip structures
Subsystems (10)
v2 cover gpiolib: Handle immutable irq_chip structures
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/[email protected]
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(-)
Signature · 7 lines
2.34.1 _______________________________________________ linux-arm-kernel mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
PATCH v2 1/10 gpio: Don't fiddle with irqchips marked as immutable
In order to move away from gpiolib messing with the internals of unsuspecting irqchips, add a flag by which irqchips advertise that they are not to be messed with, and do solemnly swear that they correctly call into the gpiolib helpers when required.
Also nudge the users into converting their drivers to the new model.
--- drivers/gpio/gpiolib.c | 7 ++++++- include/linux/irq.h | 2 ++ kernel/irq/debugfs.c | 1 + 3 files changed, 9 insertions(+), 1 deletion(-)
drivers/gpio/gpiolib.c +6 -1
@@ -1475,6 +1475,11 @@ static void gpiochip_set_irq_hooks(struct gpio_chip *gc) {struct irq_chip *irqchip = gc->irq.chip; +if (irqchip->flags &IRQCHIP_IMMUTABLE ) +return ; + +chip_warn (gc,"not an immutable chip, please consider fixing it!\n" ); +if (!irqchip->irq_request_resources && !irqchip->irq_release_resources) { irqchip->irq_request_resources = gpiochip_irq_reqres;
@@ -1633,7 +1638,7 @@ static void gpiochip_irqchip_remove(struct gpio_chip *gc)irq_domain_remove (gc->irq.domain); } -if (irqchip) { +if (irqchip && !(irqchip->flags &IRQCHIP_IMMUTABLE )) {if (irqchip->irq_request_resources == gpiochip_irq_reqres) { irqchip->irq_request_resources =NULL ; irqchip->irq_release_resources =NULL ;
include/linux/irq.h +2 -0
@@ -569,6 +569,7 @@ struct irq_chip {* IRQCHIP_ENABLE_WAKEUP_ON_SUSPEND: Invokes __enable_irq()/__disable_irq() for wake irqs * in the suspend path if they are in disabled state * IRQCHIP_AFFINITY_PRE_STARTUP: Default affinity update before startup +* IRQCHIP_IMMUTABLE: Don't ever change anything in this chip */ enum {IRQCHIP_SET_TYPE_MASKED = (1 <<0 ),
@@ -582,6 +583,7 @@ enum {IRQCHIP_SUPPORTS_NMI = (1 <<8 ),IRQCHIP_ENABLE_WAKEUP_ON_SUSPEND = (1 <<9 ),IRQCHIP_AFFINITY_PRE_STARTUP = (1 <<10 ), + IRQCHIP_IMMUTABLE = (1 << 11), };#include <linux/irqdesc.h>
kernel/irq/debugfs.c +1 -0
@@ -58,6 +58,7 @@ static const struct irq_bit_descr irqchip_flags[] = {BIT_MASK_DESCR (IRQCHIP_SUPPORTS_LEVEL_MSI ),BIT_MASK_DESCR (IRQCHIP_SUPPORTS_NMI ),BIT_MASK_DESCR (IRQCHIP_ENABLE_WAKEUP_ON_SUSPEND ), + BIT_MASK_DESCR(IRQCHIP_IMMUTABLE), }; static void
Signature · 7 lines
2.34.1 _______________________________________________ linux-arm-kernel mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
PATCH v2 2/10 gpio: Expose the gpiochip_irq_re[ql]res helpers
In reply to Marc ZyngierThe GPIO subsystem has a couple of internal helpers to manage resources on behalf of the irqchip. Expose them so that GPIO drivers can use them directly.
--- drivers/gpio/gpiolib.c | 6 ++++-- include/linux/gpio/driver.h | 4 ++++ 2 files changed, 8 insertions(+), 2 deletions(-)
drivers/gpio/gpiolib.c +4 -2
@@ -1423,19 +1423,21 @@ static int gpiochip_to_irq(struct gpio_chip *gc, unsigned int offset)return irq_create_mapping (domain, offset); } -static int gpiochip_irq_reqres (struct irq_data *d) +int gpiochip_irq_reqres (struct irq_data *d) {struct gpio_chip *gc =irq_data_get_irq_chip_data (d);return gpiochip_reqres_irq (gc, d->hwirq); } +EXPORT_SYMBOL (gpiochip_irq_reqres); -static void gpiochip_irq_relres (struct irq_data *d) +void gpiochip_irq_relres (struct irq_data *d) {struct gpio_chip *gc =irq_data_get_irq_chip_data (d);gpiochip_relres_irq (gc, d->hwirq); } +EXPORT_SYMBOL (gpiochip_irq_relres);static void gpiochip_irq_mask (struct irq_data *d) {
include/linux/gpio/driver.h +4 -0
@@ -579,6 +579,10 @@ void gpiochip_relres_irq(struct gpio_chip *gc, unsigned int offset);void gpiochip_disable_irq (struct gpio_chip *gc,unsigned int offset);void gpiochip_enable_irq (struct gpio_chip *gc,unsigned int offset); +/* irq_data versions of the above */ +int gpiochip_irq_reqres (struct irq_data *data); +void gpiochip_irq_relres (struct irq_data *data); +/* Line status inquiry for drivers */ bool gpiochip_line_is_open_drain (struct gpio_chip *gc,unsigned int offset);bool gpiochip_line_is_open_source (struct gpio_chip *gc,unsigned int offset);
Signature · 7 lines
2.34.1 _______________________________________________ linux-arm-kernel mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
PATCH v2 3/10 gpio: Add helpers to ease the transition towards immutable irq_chip
In reply to Marc ZyngierAdd a couple of new helpers to make it slightly simpler to convert drivers to immutable irq_chip structures:
- GPIOCHIP_IRQ_RESOURCE_HELPERS populates the irq_chip structure with the resource management callbacks
- gpio_irq_chip_set_chip() populates the gpio_irq_chip.chip structure, avoiding the proliferation of ugly casts
--- include/linux/gpio/driver.h | 12 ++++++++++++ 1 file changed, 12 insertions(+)
include/linux/gpio/driver.h +12 -0
@@ -583,6 +583,18 @@ void gpiochip_enable_irq(struct gpio_chip *gc, unsigned int offset);int gpiochip_irq_reqres (struct irq_data *data);void gpiochip_irq_relres (struct irq_data *data); +/* Paste this in your irq_chip structure */ +#define GPIOCHIP_IRQ_RESOURCE_HELPERS \ + .irq_request_resources = gpiochip_irq_reqres, \ + .irq_release_resources = gpiochip_irq_relres + +static inline void gpio_irq_chip_set_chip (struct gpio_irq_chip *girq, +const struct irq_chip *chip) +{ +/* Yes, dropping const is ugly, but it isn't like we have a choice */ + girq->chip = (struct irq_chip *)chip; +} +/* Line status inquiry for drivers */ bool gpiochip_line_is_open_drain (struct gpio_chip *gc,unsigned int offset);bool gpiochip_line_is_open_source (struct gpio_chip *gc,unsigned int offset);
Signature · 7 lines
2.34.1 _______________________________________________ linux-arm-kernel mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
PATCH v2 4/10 gpio: tegra186: Make the irqchip immutable
In reply to Marc ZyngierPrevent gpiolib from messing with the irqchip by advertising the irq_chip structure as immutable, making it const, and adding the various calls that gpiolib relies upon.
--- drivers/gpio/gpio-tegra186.c | 32 +++++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 9 deletions(-)
drivers/gpio/gpio-tegra186.c +23 -9
@@ -80,7 +80,6 @@ struct tegra_gpio_soc {struct tegra_gpio {struct gpio_chip gpio; -struct irq_chip intc;unsigned int num_irq;unsigned int *irq;
@@ -372,6 +371,8 @@ static void tegra186_irq_mask(struct irq_data *data) value =readl (base +TEGRA186_GPIO_ENABLE_CONFIG ); value &= ~TEGRA186_GPIO_ENABLE_CONFIG_INTERRUPT ;writel (value, base +TEGRA186_GPIO_ENABLE_CONFIG ); + +gpiochip_disable_irq (&gpio->gpio, data->hwirq); }static void tegra186_irq_unmask (struct irq_data *data)
@@ -385,6 +386,8 @@ static void tegra186_irq_unmask(struct irq_data *data)if (WARN_ON (base ==NULL ))return ; +gpiochip_enable_irq (&gpio->gpio, data->hwirq); + value =readl (base +TEGRA186_GPIO_ENABLE_CONFIG ); value |=TEGRA186_GPIO_ENABLE_CONFIG_INTERRUPT ;writel (value, base +TEGRA186_GPIO_ENABLE_CONFIG );
@@ -456,6 +459,24 @@ static int tegra186_irq_set_wake(struct irq_data *data, unsigned int on)return 0 ; } +static void tegra186_irq_print_chip (struct irq_data *data,struct seq_file *p) +{ +struct gpio_chip *gc =irq_data_get_irq_chip_data (data); + +seq_printf (p,dev_name (gc->parent)); +} + +static const struct irq_chip tegra186_gpio_irq_chip = { + .irq_ack = tegra186_irq_ack, + .irq_mask = tegra186_irq_mask, + .irq_unmask = tegra186_irq_unmask, + .irq_set_type = tegra186_irq_set_type, + .irq_set_wake = tegra186_irq_set_wake, + .irq_print_chip = tegra186_irq_print_chip, + .flags =IRQCHIP_IMMUTABLE , +GPIOCHIP_IRQ_RESOURCE_HELPERS , +}; +static void tegra186_gpio_irq (struct irq_desc *desc) {struct tegra_gpio *gpio =irq_desc_get_handler_data (desc);
@@ -760,15 +781,8 @@ static int tegra186_gpio_probe(struct platform_device *pdev) gpio->gpio.of_xlate = tegra186_gpio_of_xlate;#endif /* CONFIG_OF_GPIO */ - gpio->intc.name =dev_name (&pdev->dev); - gpio->intc.irq_ack = tegra186_irq_ack; - gpio->intc.irq_mask = tegra186_irq_mask; - gpio->intc.irq_unmask = tegra186_irq_unmask; - gpio->intc.irq_set_type = tegra186_irq_set_type; - gpio->intc.irq_set_wake = tegra186_irq_set_wake; - irq = &gpio->gpio.irq; - irq->chip = &gpio->intc; +gpio_irq_chip_set_chip (irq, &tegra186_gpio_irq_chip); irq->fwnode =of_node_to_fwnode (pdev->dev.of_node); irq->child_to_parent_hwirq = tegra186_gpio_child_to_parent_hwirq; irq->populate_parent_alloc_arg = tegra186_gpio_populate_parent_fwspec;
Signature · 7 lines
2.34.1 _______________________________________________ linux-arm-kernel mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
PATCH v2 5/10 gpio: pl061: Make the irqchip immutable
In reply to Marc ZyngierPrevent gpiolib from messing with the irqchip by advertising the irq_chip structure as immutable, making it const, and adding the various calls that gpiolib relies upon.
--- drivers/gpio/gpio-pl061.c | 32 +++++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 9 deletions(-)
drivers/gpio/gpio-pl061.c +23 -9
@@ -52,7 +52,6 @@ struct pl061 {void __iomem *base;struct gpio_chip gc; -struct irq_chip irq_chip;int parent_irq;#ifdef CONFIG_PM
@@ -241,6 +240,8 @@ static void pl061_irq_mask(struct irq_data *d) gpioie =readb (pl061->base +GPIOIE ) & ~mask;writeb (gpioie, pl061->base +GPIOIE );raw_spin_unlock (&pl061->lock); + +gpiochip_disable_irq (gc, d->hwirq); }static void pl061_irq_unmask (struct irq_data *d)
@@ -250,6 +251,8 @@ static void pl061_irq_unmask(struct irq_data *d)u8 mask =BIT (irqd_to_hwirq (d) %PL061_GPIO_NR );u8 gpioie; +gpiochip_enable_irq (gc, d->hwirq); +raw_spin_lock (&pl061->lock); gpioie =readb (pl061->base +GPIOIE ) | mask;writeb (gpioie, pl061->base +GPIOIE );
@@ -283,6 +286,24 @@ static int pl061_irq_set_wake(struct irq_data *d, unsigned int state)return irq_set_irq_wake (pl061->parent_irq, state); } +static void pl061_irq_print_chip (struct irq_data *data,struct seq_file *p) +{ +struct gpio_chip *gc =irq_data_get_irq_chip_data (data); + +seq_printf (p,dev_name (gc->parent)); +} + +static const struct irq_chip pl061_irq_chip = { + .irq_ack = pl061_irq_ack, + .irq_mask = pl061_irq_mask, + .irq_unmask = pl061_irq_unmask, + .irq_set_type = pl061_irq_type, + .irq_set_wake = pl061_irq_set_wake, + .irq_print_chip = pl061_irq_print_chip, + .flags =IRQCHIP_IMMUTABLE , +GPIOCHIP_IRQ_RESOURCE_HELPERS , +}; +static int pl061_probe (struct amba_device *adev,const struct amba_id *id) {struct device *dev = &adev->dev;
@@ -315,13 +336,6 @@ static int pl061_probe(struct amba_device *adev, const struct amba_id *id)/* * irq_chip support */ - pl061->irq_chip.name =dev_name (dev); - pl061->irq_chip.irq_ack = pl061_irq_ack; - pl061->irq_chip.irq_mask = pl061_irq_mask; - pl061->irq_chip.irq_unmask = pl061_irq_unmask; - pl061->irq_chip.irq_set_type = pl061_irq_type; - pl061->irq_chip.irq_set_wake = pl061_irq_set_wake; -writeb (0 , pl061->base +GPIOIE );/* disable irqs */ irq = adev->irq[0 ];if (!irq)
@@ -329,7 +343,7 @@ static int pl061_probe(struct amba_device *adev, const struct amba_id *id) pl061->parent_irq = irq; girq = &pl061->gc.irq; - girq->chip = &pl061->irq_chip; +gpio_irq_chip_set_chip (girq, &pl061_irq_chip); girq->parent_handler = pl061_irq_handler; girq->num_parents =1 ; girq->parents = devm_kcalloc(dev, 1, sizeof(*girq->parents),
Signature · 7 lines
2.34.1 _______________________________________________ linux-arm-kernel mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
PATCH v2 6/10 pinctrl: apple-gpio: Make the irqchip immutable
In reply to Marc ZyngierPrevent gpiolib from messing with the irqchip by advertising the irq_chip structure as immutable, making it const, and adding the various calls that gpiolib relies upon.
--- drivers/pinctrl/pinctrl-apple-gpio.c | 29 +++++++++++++++------------- 1 file changed, 16 insertions(+), 13 deletions(-)
drivers/pinctrl/pinctrl-apple-gpio.c +16 -13
@@ -36,7 +36,6 @@ struct apple_gpio_pinctrl {struct pinctrl_desc pinctrl_desc;struct gpio_chip gpio_chip; -struct irq_chip irq_chip;u8 irqgrps[]; };
@@ -275,17 +274,21 @@ static unsigned int apple_gpio_irq_type(unsigned int type)static void apple_gpio_irq_mask (struct irq_data *data) { -struct apple_gpio_pinctrl *pctl =gpiochip_get_data (irq_data_get_irq_chip_data (data)); +struct gpio_chip *gc =irq_data_get_irq_chip_data (data); +struct apple_gpio_pinctrl *pctl =gpiochip_get_data (gc);apple_gpio_set_reg (pctl, data->hwirq, REG_GPIOx_MODE,FIELD_PREP (REG_GPIOx_MODE, REG_GPIOx_IN_IRQ_OFF)); +gpiochip_disable_irq (gc, data->hwirq); }static void apple_gpio_irq_unmask (struct irq_data *data) { -struct apple_gpio_pinctrl *pctl =gpiochip_get_data (irq_data_get_irq_chip_data (data)); +struct gpio_chip *gc =irq_data_get_irq_chip_data (data); +struct apple_gpio_pinctrl *pctl =gpiochip_get_data (gc);unsigned int irqtype =apple_gpio_irq_type (irqd_get_trigger_type (data)); +gpiochip_enable_irq (gc, data->hwirq);apple_gpio_set_reg (pctl, data->hwirq, REG_GPIOx_MODE,FIELD_PREP (REG_GPIOx_MODE, irqtype)); }
@@ -343,13 +346,15 @@ static void apple_gpio_irq_handler(struct irq_desc *desc)chained_irq_exit (chip, desc); } -static struct irq_chip apple_gpio_irqchip = { - .name ="Apple-GPIO" , - .irq_startup = apple_gpio_irq_startup, - .irq_ack = apple_gpio_irq_ack, - .irq_mask = apple_gpio_irq_mask, - .irq_unmask = apple_gpio_irq_unmask, - .irq_set_type = apple_gpio_irq_set_type, +static const struct irq_chip apple_gpio_irqchip = { + .name ="Apple-GPIO" , + .irq_startup = apple_gpio_irq_startup, + .irq_ack = apple_gpio_irq_ack, + .irq_mask = apple_gpio_irq_mask, + .irq_unmask = apple_gpio_irq_unmask, + .irq_set_type = apple_gpio_irq_set_type, + .flags =IRQCHIP_IMMUTABLE , +GPIOCHIP_IRQ_RESOURCE_HELPERS , };/* Probe & register */
@@ -360,8 +365,6 @@ static int apple_gpio_register(struct apple_gpio_pinctrl *pctl)void **irq_data =NULL ;int ret; - pctl->irq_chip = apple_gpio_irqchip; - pctl->gpio_chip.label =dev_name (pctl->dev); pctl->gpio_chip.request = gpiochip_generic_request; pctl->gpio_chip.free = gpiochip_generic_free;
@@ -377,7 +380,7 @@ static int apple_gpio_register(struct apple_gpio_pinctrl *pctl)if (girq->num_parents) {int i; - girq->chip = &pctl->irq_chip; +gpio_irq_chip_set_chip (girq, &apple_gpio_irqchip); girq->parent_handler = apple_gpio_irq_handler; girq->parents = kmalloc_array(girq->num_parents,
Signature · 7 lines
2.34.1 _______________________________________________ linux-arm-kernel mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
PATCH v2 7/10 pinctrl: msmgpio: Make the irqchip immutable
In reply to Marc ZyngierPrevent gpiolib from messing with the irqchip by advertising the irq_chip structure as immutable, making it const, and adding the various calls that gpiolib relies upon.
--- drivers/pinctrl/qcom/pinctrl-msm.c | 53 +++++++++++++++++++----------- 1 file changed, 33 insertions(+), 20 deletions(-)
drivers/pinctrl/qcom/pinctrl-msm.c +33 -20
@@ -42,7 +42,6 @@* @chip: gpiochip handle. * @desc: pin controller descriptor * @restart_nb: restart notifier block. -* @irq_chip: irq chip information * @irq: parent irq for the TLMM irq_chip. * @intr_target_use_scm: route irq to application cpu using scm calls * @lock: Spinlock to protect register resources as well
@@ -63,7 +62,6 @@ struct msm_pinctrl {struct pinctrl_desc desc;struct notifier_block restart_nb; -struct irq_chip irq_chip;int irq;bool intr_target_use_scm;
@@ -868,6 +866,8 @@ static void msm_gpio_irq_enable(struct irq_data *d)struct gpio_chip *gc =irq_data_get_irq_chip_data (d);struct msm_pinctrl *pctrl =gpiochip_get_data (gc); +gpiochip_enable_irq (gc, d->hwirq); +if (d->parent_data)irq_chip_enable_parent (d);
@@ -885,6 +885,8 @@ static void msm_gpio_irq_disable(struct irq_data *d)if (!test_bit (d->hwirq, pctrl->skip_wake_irqs))msm_gpio_irq_mask (d); + +gpiochip_disable_irq (gc, d->hwirq); }/**
@@ -958,6 +960,14 @@ static void msm_gpio_irq_ack(struct irq_data *d)raw_spin_unlock_irqrestore (&pctrl->lock, flags); } +static void msm_gpio_irq_eoi (struct irq_data *d) +{ + d = d->parent_data; + +if (d) + d->chip->irq_eoi (d); +} +static bool msm_gpio_needs_dual_edge_parent_workaround (struct irq_data *d,unsigned int type) {
@@ -1255,6 +1265,26 @@ static bool msm_gpio_needs_valid_mask(struct msm_pinctrl *pctrl)return device_property_count_u16 (pctrl->dev,"gpios" ) >0 ; } +static const struct irq_chip msm_gpio_irq_chip = { + .name ="msmgpio" , + .irq_enable = msm_gpio_irq_enable, + .irq_disable = msm_gpio_irq_disable, + .irq_mask = msm_gpio_irq_mask, + .irq_unmask = msm_gpio_irq_unmask, + .irq_ack = msm_gpio_irq_ack, + .irq_eoi = msm_gpio_irq_eoi, + .irq_set_type = msm_gpio_irq_set_type, + .irq_set_wake = msm_gpio_irq_set_wake, + .irq_request_resources = msm_gpio_irq_reqres, + .irq_release_resources = msm_gpio_irq_relres, + .irq_set_affinity = msm_gpio_irq_set_affinity, + .irq_set_vcpu_affinity = msm_gpio_irq_set_vcpu_affinity, + .flags = (IRQCHIP_MASK_ON_SUSPEND | +IRQCHIP_SET_TYPE_MASKED | +IRQCHIP_ENABLE_WAKEUP_ON_SUSPEND | +IRQCHIP_IMMUTABLE ), +}; +static int msm_gpio_init (struct msm_pinctrl *pctrl) {struct gpio_chip *chip;
@@ -1276,22 +1306,6 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)if (msm_gpio_needs_valid_mask (pctrl)) chip->init_valid_mask = msm_gpio_init_valid_mask; - pctrl->irq_chip.name ="msmgpio" ; - pctrl->irq_chip.irq_enable = msm_gpio_irq_enable; - pctrl->irq_chip.irq_disable = msm_gpio_irq_disable; - pctrl->irq_chip.irq_mask = msm_gpio_irq_mask; - pctrl->irq_chip.irq_unmask = msm_gpio_irq_unmask; - pctrl->irq_chip.irq_ack = msm_gpio_irq_ack; - pctrl->irq_chip.irq_set_type = msm_gpio_irq_set_type; - pctrl->irq_chip.irq_set_wake = msm_gpio_irq_set_wake; - pctrl->irq_chip.irq_request_resources = msm_gpio_irq_reqres; - pctrl->irq_chip.irq_release_resources = msm_gpio_irq_relres; - pctrl->irq_chip.irq_set_affinity = msm_gpio_irq_set_affinity; - pctrl->irq_chip.irq_set_vcpu_affinity = msm_gpio_irq_set_vcpu_affinity; - pctrl->irq_chip.flags =IRQCHIP_MASK_ON_SUSPEND | -IRQCHIP_SET_TYPE_MASKED | -IRQCHIP_ENABLE_WAKEUP_ON_SUSPEND ; - np =of_parse_phandle (pctrl->dev->of_node,"wakeup-parent" ,0 );if (np) { chip->irq.parent_domain = irq_find_matching_host(np,
@@ -1300,7 +1314,6 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)if (!chip->irq.parent_domain)return -EPROBE_DEFER ; chip->irq.child_to_parent_hwirq = msm_gpio_wakeirq; - pctrl->irq_chip.irq_eoi = irq_chip_eoi_parent;/* * Let's skip handling the GPIOs, if the parent irqchip * is handling the direct connect IRQ of the GPIO.
@@ -1313,7 +1326,7 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl) } girq = &chip->irq; - girq->chip = &pctrl->irq_chip; +gpio_irq_chip_set_chip (girq, &msm_gpio_irq_chip); girq->parent_handler = msm_gpio_irq_handler; girq->fwnode = pctrl->dev->fwnode; girq->num_parents =1 ;
Signature · 7 lines
2.34.1 _______________________________________________ linux-arm-kernel mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
PATCH v2 8/10 pinctrl: amd: Make the irqchip immutable
In reply to Marc ZyngierPrevent gpiolib from messing with the irqchip by advertising the irq_chip structure as immutable, making it const, and adding the various calls that gpiolib relies upon.
--- drivers/pinctrl/pinctrl-amd.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
drivers/pinctrl/pinctrl-amd.c +8 -3
@@ -387,6 +387,8 @@ static void amd_gpio_irq_enable(struct irq_data *d)struct gpio_chip *gc =irq_data_get_irq_chip_data (d);struct amd_gpio *gpio_dev =gpiochip_get_data (gc); +gpiochip_enable_irq (gc, d->hwirq); +raw_spin_lock_irqsave (&gpio_dev->lock, flags); pin_reg =readl (gpio_dev->base + (d->hwirq)*4 ); pin_reg |=BIT (INTERRUPT_ENABLE_OFF );
@@ -408,6 +410,8 @@ static void amd_gpio_irq_disable(struct irq_data *d) pin_reg &= ~BIT (INTERRUPT_MASK_OFF );writel (pin_reg, gpio_dev->base + (d->hwirq)*4 );raw_spin_unlock_irqrestore (&gpio_dev->lock, flags); + +gpiochip_disable_irq (gc, d->hwirq); }static void amd_gpio_irq_mask (struct irq_data *d)
@@ -577,7 +581,7 @@ static void amd_irq_ack(struct irq_data *d)*/ } -static struct irq_chip amd_gpio_irqchip = { +static const struct irq_chip amd_gpio_irqchip = { .name ="amd_gpio" , .irq_ack = amd_irq_ack, .irq_enable = amd_gpio_irq_enable,
@@ -593,7 +597,8 @@ static struct irq_chip amd_gpio_irqchip = {* the wake event. Otherwise the wake event will never clear and * prevent the system from suspending. */ - .flags = IRQCHIP_ENABLE_WAKEUP_ON_SUSPEND, + .flags = IRQCHIP_ENABLE_WAKEUP_ON_SUSPEND | IRQCHIP_IMMUTABLE, + GPIOCHIP_IRQ_RESOURCE_HELPERS, };#define PIN_IRQ_PENDING (BIT(INTERRUPT_STS_OFF) | BIT(WAKE_STS_OFF))
@@ -1026,7 +1031,7 @@ static int amd_gpio_probe(struct platform_device *pdev)amd_gpio_irq_init (gpio_dev); girq = &gpio_dev->gc.irq; - girq->chip = &amd_gpio_irqchip; +gpio_irq_chip_set_chip (girq, &amd_gpio_irqchip);/* This will let us handle the parent IRQ in the driver */ girq->parent_handler =NULL ; girq->num_parents =0 ;
Signature · 7 lines
2.34.1 _______________________________________________ linux-arm-kernel mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
PATCH v2 9/10 gpio: Update TODO to mention immutable irq_chip structures
In reply to Marc Zyngier5 drivers are converted, a few hundred to go. Definitely worth of a TODO entry, in the hope that someone will notice it and do a bulk update.
--- drivers/gpio/TODO | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)
drivers/gpio/TODO +19 -0
@@ -178,3 +178,22 @@ discussed but the idea is to provide a low-level access point for debugging and hacking and to expose all lines without the need of any exporting. Also provide ample ammunition to shoot oneself in the foot, because this is debugfs after all. + + +Moving over to immutable irq_chip structures + +Most of the gpio chips implementing interrupt support rely on gpiolib +intercepting some of the irq_chip callbacks, preventing the structures +from being made read-only and forcing duplication of structures that +should otherwise be unique. + +The solution is to call into the gpiolib code when needed (resource +management, enable/disable or unmask/mask callbacks), and to let the +core code know about that by exposing a flag (IRQCHIP_IMMUTABLE) in +the irq_chip structure. The irq_chip structure can then be made unique +and const. + +A small number of drivers have been converted (pl061, tegra186, msm, +amd, apple), and can be used as examples of how to proceed with this +conversion. Note that drivers using the generic irqchip framework +cannot be converted yet, but watch this space!
Signature · 7 lines
2.34.1 _______________________________________________ linux-arm-kernel mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
PATCH v2 10/10 Documentation: Update the recommended pattern for GPIO irqchips
In reply to Marc ZyngierUpdate the documentation to get rid of the per-gpio_irq_chip irq_chip structure, and give examples of the new pattern.
--- Documentation/driver-api/gpio/driver.rst | 175 ++++++++++++++++++----- 1 file changed, 142 insertions(+), 33 deletions(-)
Documentation/driver-api/gpio/driver.rst +142 -33
@@ -417,30 +417,66 @@ struct gpio_irq_chip inside struct gpio_chip before adding the gpio_chip. If you do this, the additional irq_chip will be set up by gpiolib at the same time as setting up the rest of the GPIO functionality. The following is a typical example of a chained cascaded interrupt handler using -the gpio_irq_chip: +the gpio_irq_chip. Note how the mask/unmask (or disable/enable) functions +call into the core gpiolib code: .. code-block:: c - /* Typical state container with dynamic irqchip */ + /* Typical state container */ struct my_gpio { struct gpio_chip gc; - struct irq_chip irq; + }; + + 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); + } + + 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); + + /* + * Perform any necessary action to unmask the interrupt, + * after having called into the core code to synchronise + * the state. + */ + } + + /* + * Statically populate the irqchip. Note that it is made const + * (further indicated by the IRQCHIP_IMMUTABLE flag), and that + * the GPIOCHIP_IRQ_RESOURCE_HELPER macro adds some extra + * callbacks to the structure. + */ + static const struct irq_chip my_gpio_irq_chip = { + .name = "my_gpio_irq", + .irq_ack = my_gpio_ack_irq, + .irq_mask = my_gpio_mask_irq, + .irq_unmask = my_gpio_unmask_irq, + .irq_set_type = my_gpio_set_irq_type, + .flags = IRQCHIP_IMMUTABLE, + /* Provide the gpio resource callbacks */ + GPIOCHIP_IRQ_RESOURCE_HELPERS, }; int irq; /* from platform etc */ struct my_gpio *g; struct gpio_irq_chip *girq; - /* Set up the irqchip dynamically */ - g->irq.name = "my_gpio_irq"; - g->irq.irq_ack = my_gpio_ack_irq; - g->irq.irq_mask = my_gpio_mask_irq; - g->irq.irq_unmask = my_gpio_unmask_irq; - g->irq.irq_set_type = my_gpio_set_irq_type; - /* Get a pointer to the gpio_irq_chip */ girq = &g->gc.irq; - girq->chip = &g->irq; + gpio_irq_chip_set_chip(girq, &my_gpio_irq_chip); girq->parent_handler = ftgpio_gpio_irq_handler; girq->num_parents = 1; girq->parents = devm_kcalloc(dev, 1, sizeof(*girq->parents),
@@ -458,23 +494,58 @@ the interrupt separately and go with it: .. code-block:: c - /* Typical state container with dynamic irqchip */ + /* Typical state container */ struct my_gpio { struct gpio_chip gc; - struct irq_chip irq; + }; + + 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); + } + + 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); + + /* + * Perform any necessary action to unmask the interrupt, + * after having called into the core code to synchronise + * the state. + */ + } + + /* + * Statically populate the irqchip. Note that it is made const + * (further indicated by the IRQCHIP_IMMUTABLE flag), and that + * the GPIOCHIP_IRQ_RESOURCE_HELPER macro adds some extra + * callbacks to the structure. + */ + static const struct irq_chip my_gpio_irq_chip = { + .name = "my_gpio_irq", + .irq_ack = my_gpio_ack_irq, + .irq_mask = my_gpio_mask_irq, + .irq_unmask = my_gpio_unmask_irq, + .irq_set_type = my_gpio_set_irq_type, + .flags = IRQCHIP_IMMUTABLE, + /* Provide the gpio resource callbacks */ + GPIOCHIP_IRQ_RESOURCE_HELPERS, }; int irq; /* from platform etc */ struct my_gpio *g; struct gpio_irq_chip *girq; - /* Set up the irqchip dynamically */ - g->irq.name = "my_gpio_irq"; - g->irq.irq_ack = my_gpio_ack_irq; - g->irq.irq_mask = my_gpio_mask_irq; - g->irq.irq_unmask = my_gpio_unmask_irq; - g->irq.irq_set_type = my_gpio_set_irq_type; - ret = devm_request_threaded_irq(dev, irq, NULL, irq_thread_fn, IRQF_ONESHOT, "my-chip", g); if (ret < 0)
@@ -482,7 +553,7 @@ the interrupt separately and go with it: /* Get a pointer to the gpio_irq_chip */ girq = &g->gc.irq; - girq->chip = &g->irq; + gpio_irq_chip_set_chip(girq, &my_gpio_irq_chip); /* This will let us handle the parent IRQ in the driver */ girq->parent_handler = NULL; girq->num_parents = 0;
@@ -500,24 +571,61 @@ In this case the typical set-up will look like this: /* Typical state container with dynamic irqchip */ struct my_gpio { struct gpio_chip gc; - struct irq_chip irq; struct fwnode_handle *fwnode; }; - int irq; /* from platform etc */ + 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); + irq_mask_mask_parent(d); + } + + 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); + + /* + * Perform any necessary action to unmask the interrupt, + * after having called into the core code to synchronise + * the state. + */ + + irq_mask_unmask_parent(d); + } + + /* + * Statically populate the irqchip. Note that it is made const + * (further indicated by the IRQCHIP_IMMUTABLE flag), and that + * the GPIOCHIP_IRQ_RESOURCE_HELPER macro adds some extra + * callbacks to the structure. + */ + static const struct irq_chip my_gpio_irq_chip = { + .name = "my_gpio_irq", + .irq_ack = my_gpio_ack_irq, + .irq_mask = my_gpio_mask_irq, + .irq_unmask = my_gpio_unmask_irq, + .irq_set_type = my_gpio_set_irq_type, + .flags = IRQCHIP_IMMUTABLE, + /* Provide the gpio resource callbacks */ + GPIOCHIP_IRQ_RESOURCE_HELPERS, + }; + struct my_gpio *g; struct gpio_irq_chip *girq; - /* Set up the irqchip dynamically */ - g->irq.name = "my_gpio_irq"; - g->irq.irq_ack = my_gpio_ack_irq; - g->irq.irq_mask = my_gpio_mask_irq; - g->irq.irq_unmask = my_gpio_unmask_irq; - g->irq.irq_set_type = my_gpio_set_irq_type; - /* Get a pointer to the gpio_irq_chip */ girq = &g->gc.irq; - girq->chip = &g->irq; + gpio_irq_chip_set_chip(girq, &my_gpio_irq_chip); girq->default_type = IRQ_TYPE_NONE; girq->handler = handle_bad_irq; girq->fwnode = g->fwnode;
@@ -605,8 +713,9 @@ When implementing an irqchip inside a GPIO driver, these two functions should typically be called in the .irq_disable() and .irq_enable() callbacks from the irqchip. -When using the gpiolib irqchip helpers, these callbacks are automatically -assigned. +When IRQCHIP_IMMUTABLE is not advertised by the irqchip, these callbacks +are automatically assigned. This behaviour is deprecated and on its way +to be removed from the kernel. Real-Time compliance for GPIO IRQ chips
Signature · 7 lines
2.34.1 _______________________________________________ linux-arm-kernel mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Re: [PATCH v2 10/10] Documentation: Update the recommended pattern for GPIO irqchips
Quoted 3 lines · Marc Zyngier
Update the documentation to get rid of the per-gpio_irq_chip
irq_chipstructure, and give examples of the new pattern.
...
Documentation/driver-api/gpio/driver.rst struct gpio_irq_chip inside struct gpio_chip before adding the gpio_chip. · line 440
Quoted 11 lines
Documentation/driver-api/gpio/driver.rst +11
-0+ 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)
Documentation/driver-api/gpio/driver.rst struct gpio_irq_chip inside struct gpio_chip before adding the gpio_chip.
Quoted 7 lines
Documentation/driver-api/gpio/driver.rst +6
-0+ } + + 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)
Documentation/driver-api/gpio/driver.rst struct gpio_irq_chip inside struct gpio_chip before adding the gpio_chip. · line 454
Quoted 6 lines
Documentation/driver-api/gpio/driver.rst +6
-0+ /* + * 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.
...
Documentation/driver-api/gpio/driver.rst struct gpio_irq_chip inside struct gpio_chip before adding the gpio_chip.
Quoted 11 lines
Documentation/driver-api/gpio/driver.rst +10
-0+ 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);
(3)
Documentation/driver-api/gpio/driver.rst struct gpio_irq_chip inside struct gpio_chip before adding the gpio_chip.
Quoted 7 lines
Documentation/driver-api/gpio/driver.rst +6
-0+ } + + 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);
(4)
Documentation/driver-api/gpio/driver.rst struct gpio_irq_chip inside struct gpio_chip before adding the gpio_chip. · line 454
Quoted 6 lines
Documentation/driver-api/gpio/driver.rst +6
-0+ /* + * Perform any necessary action to unmask the interrupt, + * after having called into the core code to synchronise + * the state. + */ + }
Ditto for (3) & (4).
...
Documentation/driver-api/gpio/driver.rst struct gpio_irq_chip inside struct gpio_chip before adding the gpio_chip. · line 440
Quoted 11 lines
Documentation/driver-api/gpio/driver.rst +11
-0+ 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);
(5)
Documentation/driver-api/gpio/driver.rst In this case the typical set-up will look like this:
Quoted 8 lines
Documentation/driver-api/gpio/driver.rst +7
-0+ irq_mask_mask_parent(d); + } + + 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);
(6)
Documentation/driver-api/gpio/driver.rst In this case the typical set-up will look like this: · line 604
Quoted 8 lines
Documentation/driver-api/gpio/driver.rst +8
-0+ /* + * Perform any necessary action to unmask the interrupt, + * after having called into the core code to synchronise + * the state. + */ + + irq_mask_unmask_parent(d); + }
Ditto for (5) & (6).
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v2 10/10] Documentation: Update the recommended pattern for GPIO irqchips
Quoted 38 lines · Andy Shevchenko
Quoted 3 lines · Marc Zyngier
Update the documentation to get rid of the per-gpio_irq_chip
irq_chipstructure, and give examples of the new pattern....
Documentation/driver-api/gpio/driver.rst struct gpio_irq_chip inside struct gpio_chip before adding the gpio_chip. · line 440
Quoted 11 lines
Documentation/driver-api/gpio/driver.rst +11
-0+ 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)
Documentation/driver-api/gpio/driver.rst struct gpio_irq_chip inside struct gpio_chip before adding the gpio_chip.
Quoted 7 lines
Documentation/driver-api/gpio/driver.rst +6
-0+ } + + 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)
Documentation/driver-api/gpio/driver.rst struct gpio_irq_chip inside struct gpio_chip before adding the gpio_chip. · line 454
Quoted 6 lines
Documentation/driver-api/gpio/driver.rst +6
-0+ /* + * Perform any necessary action to unmask the interrupt, + * after having called into the core code to synchronise + * the state. + */ + }
Quoted 2 lines
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.
Signature · 1 line
Jazz is not dead. It just smells funny...
Quoted 18 lines · Marc Zyngier
This is a followup from [1].
I recently realised that the gpiolib play ugly tricks on the unsuspecting
irq_chipstructures by patching the callbacks.Not only this breaks when an
irq_chipstructure 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_chipflag (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.
Quoted 7 lines
5 drivers are updated to this new model: M1, QC, Tegra, pl061 and AMD (as I actively use them) keeping a single
irq_chipstructure, 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?
for the patches 1-3,9 and 10 after clarifying one example case.
Quoted 36 lines · * 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/[email protected]
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 immutableirq_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 immutableirq_chipstructures
Documentation: Update the recommended pattern for GPIO irqchipsDocumentation/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
Signature · 2 lines
With Best Regards, Andy Shevchenko
Quoted 59 lines · Marc Zyngier
This is a followup from [1].
I recently realised that the gpiolib play ugly tricks on the unsuspecting
irq_chipstructures by patching the callbacks.Not only this breaks when an
irq_chipstructure 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_chipflag (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_chipstructure, 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/[email protected]
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 immutableirq_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 immutableirq_chipstructures
Documentation: Update the recommended pattern for GPIO irqchipsDocumentation/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(-)
Quoted 3 lines
--
2.34.1
This may be coming too late but for the GPIO part:
Quoted 28 lines · Marc Zyngier
Not only this breaks when an
irq_chipstructure 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_chipflag (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_chipstructure, 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
Quoted 11 lines
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 immutableirq_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 immutableirq_chipstructures
Documentation: Update the recommended pattern for GPIO irqchips
Yours,
Linus Walleij