gpiolib: Handle immutable irq_chip structures
Subsystems (8)
cover gpiolib: Handle immutable irq_chip structures
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 there. 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.
3 drivers are updated to this new model: M1, QC and Tegra, as I actively use them (though Tegra is hosed at the moment), 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.
Another approach was to let gpiolib provide its own irq_chip structure and stack it, but:
- only a few drivers are hierarchy aware
- the diversity of interrupt flows makes it impractical
I'd welcome comments on the approach. If deemed acceptable, there are another 300+ drivers to update! Not to mention the documentation. I appreciate that this is a lot of potential changes, but the current situation is messy.
Note that these patches are on top of irqchip-next, which contains more constifying work.
M.
Marc Zyngier (5):
gpio: Don't fiddle with irqchips marked as immutable
gpio: Expose the gpiochip_irq_re[ql]res helpers
pinctrl: apple-gpio: Make the irqchip immutable
pinctrl: msmgpio: Make the irqchip immutable
gpio: tegra186: Make the irqchip immutable
drivers/gpio/gpio-tegra186.c | 33 ++++++++++++----- drivers/gpio/gpiolib.c | 13 +++++-- drivers/pinctrl/pinctrl-apple-gpio.c | 30 +++++++++------- drivers/pinctrl/qcom/pinctrl-msm.c | 53 +++++++++++++++++----------- include/linux/gpio/driver.h | 4 +++ include/linux/irq.h | 2 ++ kernel/irq/debugfs.c | 1 + 7 files changed, 91 insertions(+), 45 deletions(-)
Signature · 1 line
2.30.2
PATCH 1/5 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 wueh required.
--- 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 ) { +chip_info (gc,"Immutable chip, not updating\n" ); +return ; + } +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 · 1 line
2.30.2
Re: [PATCH 1/5] gpio: Don't fiddle with irqchips marked as immutable
Quoted 5 lines · Marc Zyngier
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 wueh required.
"wueh"? Should that be "when"?
Re: [PATCH 1/5] gpio: Don't fiddle with irqchips marked as immutable
Quoted 8 lines · Jeffrey Hugo
Quoted 5 lines · Marc Zyngier
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 wueh required.
"wueh"? Should that be "when"?
Absolutely. There are more typos in this cover letter, and probably even more in the individual patches!
M.
Signature · 1 line
Jazz is not dead. It just smells funny...
Re: [PATCH 1/5] gpio: Don't fiddle with irqchips marked as immutable
In reply to Marc ZyngierQuoted 11 lines · Marc Zyngier
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 wueh required.
Signed-off-by Marc Zyngier <[email protected]>--- drivers/gpio/gpiolib.c | 7 ++++++- include/linux/irq.h | 2 ++ kernel/irq/debugfs.c | 1 + 3 files changed, 9 insertions(+), 1 deletion(-)
I kind of like this. The bit where the const cast is essentially guarded by an "immutable" flag is a bit funky, but it doesn't look like there is a good way to do it by making all references const without doing a huge all-at-once conversion.
I've always found it a bit irritating that irq_chip was somewhere between a container for chip-specific data and an "ops" structure. I think it'd be even nicer if this was split into an extra struct irq_chip_ops, which could then always be const and a struct irq_chip that contained primarily chip-specific data as well as a pointer to struct irq_chip_ops.
But again, this seems fairly tricky to pull off given all the interdependencies and we can iterate on this in the future, so this seems like a good enough compromise:
Re: [PATCH 1/5] gpio: Don't fiddle with irqchips marked as immutable
Quoted 14 lines · Thierry Reding
Quoted 11 lines · Marc Zyngier
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 wueh required.
Signed-off-by Marc Zyngier <[email protected]>--- drivers/gpio/gpiolib.c | 7 ++++++- include/linux/irq.h | 2 ++ kernel/irq/debugfs.c | 1 + 3 files changed, 9 insertions(+), 1 deletion(-)
Quoted 4 lines
I kind of like this. The bit where the const cast is essentially guarded by an "immutable" flag is a bit funky, but it doesn't look like there is a good way to do it by making all references const without doing a huge all-at-once conversion.
Exactly. Somehow, we need to advertise it to the gpiolib code, and this does the job. I hope to be able to simply drop it once everthing is converted. One day.
Quoted 6 lines
I've always found it a bit irritating that
irq_chipwas somewhere between a container for chip-specific data and an "ops" structure. I think it'd be even nicer if this was split into an extra structirq_chip_ops, which could then always be const and a structirq_chipthat contained primarily chip-specific data as well as a pointer to structirq_chip_ops.
But that's the thing: it almost is a pure 'ops' structure. Only two things are getting in the way of it:
- the 'parent_dev' field: this is now sorted, as I moved it to the irq_domain structure, and updated all the relevant drivers (see what is currently in -next).
- the .name field: it really should never be something that changes from one instance of the chip to another. Which is why we have the .irq_print_chip() method to handle that (and ideally we'd stick to pure const names). I'm addressing this as I go, but everything in drivers/irqchip/ should be fixed in -next.
The "context" part really lives in irq_domain.
Quoted 5 lines
But again, this seems fairly tricky to pull off given all the interdependencies and we can iterate on this in the future, so this seems like a good enough compromise:
Acked-by Thierry Reding <[email protected]>
Thanks,
M.
Signature · 1 line
Without deviation from the norm, progress is not possible.
PATCH 2/5 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
@@ -576,6 +576,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 · 1 line
2.30.2
PATCH 3/5 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 | 30 ++++++++++++++++------------ 1 file changed, 17 insertions(+), 13 deletions(-)
drivers/pinctrl/pinctrl-apple-gpio.c +17 -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,16 @@ 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_request_resources = gpiochip_irq_reqres, + .irq_release_resources = gpiochip_irq_relres, + .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 , };/* Probe & register */
@@ -360,8 +366,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 +381,7 @@ static int apple_gpio_register(struct apple_gpio_pinctrl *pctl)if (girq->num_parents) {int i; - girq->chip = &pctl->irq_chip; + girq->chip = (struct irq_chip *)&apple_gpio_irqchip; girq->parent_handler = apple_gpio_irq_handler; girq->parents = kmalloc_array(girq->num_parents,
Signature · 1 line
2.30.2
PATCH 4/5 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;
@@ -859,6 +857,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);
@@ -876,6 +876,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); }/**
@@ -949,6 +951,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) {
@@ -1246,6 +1256,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;
@@ -1267,22 +1297,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,
@@ -1291,7 +1305,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.
@@ -1304,7 +1317,7 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl) } girq = &chip->irq; - girq->chip = &pctrl->irq_chip; + girq->chip = (struct irq_chip *)&msm_gpio_irq_chip; girq->parent_handler = msm_gpio_irq_handler; girq->fwnode = pctrl->dev->fwnode; girq->num_parents =1 ;
Signature · 1 line
2.30.2
PATCH 5/5 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 | 33 ++++++++++++++++++++++++--------- 1 file changed, 24 insertions(+), 9 deletions(-)
drivers/gpio/gpio-tegra186.c +24 -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,25 @@ 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_request_resources = gpiochip_irq_reqres, + .irq_release_resources = gpiochip_irq_relres, + .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 , +}; +static void tegra186_gpio_irq (struct irq_desc *desc) {struct tegra_gpio *gpio =irq_desc_get_handler_data (desc);
@@ -760,15 +782,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; + irq->chip = (struct irq_chip *)&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 · 1 line
2.30.2
Quoted 15 lines · Marc Zyngier
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 there. 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 2 lines
3 drivers are updated to this new model: M1, QC and Tegra, as I actively use them (though Tegra is hosed at the moment), keeping a
Hosed in what way? Anything I can help with?
Thierry
Quoted 16 lines · Thierry Reding
Quoted 15 lines · Marc Zyngier
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 there. 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 4 lines
Quoted 2 lines
3 drivers are updated to this new model: M1, QC and Tegra, as I actively use them (though Tegra is hosed at the moment), keeping a
Hosed in what way? Anything I can help with?
Tegra186 doesn't boot as host1x is been broken since -rc1.
I have been carrying this[1] patch which has been in -next for some time, but still not merged AFAICS.
M.
Signature · 1 line
Jazz is not dead. It just smells funny...
Quoted 21 lines · Marc Zyngier
Quoted 19 lines · Thierry Reding
Quoted 17 lines · Marc Zyngier
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 there. 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.3 drivers are updated to this new model: M1, QC and Tegra, as I actively use them (though Tegra is hosed at the moment), keeping a
Quoted 6 lines
Quoted 1 line
Hosed in what way? Anything I can help with?
Tegra186 doesn't boot as host1x is been broken since -rc1.
I have been carrying this[1] patch which has been in -next for some time, but still not merged AFAICS.
Looks like it finally made it -rc6.
Thanks,
M.
Signature · 1 line
Without deviation from the norm, progress is not possible.
Quoted 2 lines · Marc Zyngier
I recently realised that the gpiolib play ugly tricks on the unsuspecting
irq_chipstructures by patching the callbacks.
Sorry about that...
Quoted 6 lines
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 there. 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.
OK if there is a precedent it is usually wise to follow.
Quoted 4 lines
I'd welcome comments on the approach. If deemed acceptable, there are another 300+ drivers to update! Not to mention the documentation. I appreciate that this is a lot of potential changes, but the current situation is messy.
I'm happy with this approach as long as the 300+ drivers get fixed and the old way of doing it gets deleted.
Yours,
Linus Walleij
Quoted 6 lines · Linus Walleij
Quoted 2 lines · Marc Zyngier
I recently realised that the gpiolib play ugly tricks on the unsuspecting
irq_chipstructures by patching the callbacks.Sorry about that...
No worries. It probably did seem like a good idea at the time, and I have the benefit of hindsight here...
Quoted 10 lines
Quoted 7 lines
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 there. 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.OK if there is a precedent it is usually wise to follow.
Quoted 7 lines
Quoted 4 lines
I'd welcome comments on the approach. If deemed acceptable, there are another 300+ drivers to update! Not to mention the documentation. I appreciate that this is a lot of potential changes, but the current situation is messy.
I'm happy with this approach as long as the 300+ drivers get fixed and the old way of doing it gets deleted.
Of course. Note that it will take some time before it actually happens. My current plan is to stick in a pr_warn() each time a driver following the old scheme gets registered, as a nudge for people to update their driver if they care about it.
Regarding documentation, are you OK with me simply replacing the current code samples with the new approach? It will at least avoid giving people the wrong idea. I also want to write a brief migration guide that people willing to bump up their patch count can follow.
I'll repost something once -rc1 is out.
Thanks,
M.
Signature · 1 line
Jazz is not dead. It just smells funny...
Quoted 7 lines · Marc Zyngier
Quoted 2 lines
I'm happy with this approach as long as the 300+ drivers get fixed and the old way of doing it gets deleted.
Of course. Note that it will take some time before it actually happens. My current plan is to stick in a
pr_warn()each time a driver following the old scheme gets registered, as a nudge for people to update their driver if they care about it.
Fair enough, we have a bunch of those WIP churn tasks in GPIO anyways.
What about adding a blurb to:drivers/gpio/TODO
so we have it on the radar?
Quoted 4 lines
Regarding documentation, are you OK with me simply replacing the current code samples with the new approach? It will at least avoid giving people the wrong idea. I also want to write a brief migration guide that people willing to bump up their patch count can follow.
Yup
Quoted 1 line
I'll repost something once -rc1 is out.
Sounds good!
Yours,
Linus Walleij