[PATCH 0/5] gpiolib: Handle immutable irq_chip structures

[PATCH 0/5] gpiolib: Handle immutable irq_chip structures

[Marc Zyngier]

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(-)

-- 
2.30.2

[PATCH 1/5] gpio: Don't fiddle with irqchips marked as immutable

[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 <maz@kernel.org>
---
 drivers/gpio/gpiolib.c | 7 ++++++-
 include/linux/irq.h    | 2 ++
 kernel/irq/debugfs.c   | 1 +
 3 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 3859911b61e9..515838708b00 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -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;
diff --git a/include/linux/irq.h b/include/linux/irq.h
index f92788ccdba2..505308253d23 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -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>
diff --git a/kernel/irq/debugfs.c b/kernel/irq/debugfs.c
index 2b43f5f5033d..bc8e40cf2b65 100644
--- a/kernel/irq/debugfs.c
+++ b/kernel/irq/debugfs.c
@@ -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
-- 
2.30.2

[PATCH 2/5] gpio: Expose the gpiochip_irq_re[ql]res helpers

[Marc Zyngier]

The 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.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 drivers/gpio/gpiolib.c      | 6 ++++--
 include/linux/gpio/driver.h | 4 ++++
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 515838708b00..408ff8f262d3 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -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)
 {
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index b0728c8ad90c..89291c02854d 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -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);
-- 
2.30.2

[PATCH 3/5] pinctrl: apple-gpio: Make the irqchip immutable

[Marc Zyngier]

Prevent 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.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 drivers/pinctrl/pinctrl-apple-gpio.c | 30 ++++++++++++++++------------
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-apple-gpio.c b/drivers/pinctrl/pinctrl-apple-gpio.c
index 72f4dd2466e1..0ab5cc10f885 100644
--- a/drivers/pinctrl/pinctrl-apple-gpio.c
+++ b/drivers/pinctrl/pinctrl-apple-gpio.c
@@ -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,
-- 
2.30.2

[PATCH 4/5] pinctrl: msmgpio: Make the irqchip immutable

[Marc Zyngier]

Prevent 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.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 drivers/pinctrl/qcom/pinctrl-msm.c | 53 +++++++++++++++++++-----------
 1 file changed, 33 insertions(+), 20 deletions(-)

diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index 780878dede9e..33ce5aa4e562 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -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;
-- 
2.30.2

[PATCH 5/5] gpio: tegra186: Make the irqchip immutable

[Marc Zyngier]

Prevent 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.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 drivers/gpio/gpio-tegra186.c | 33 ++++++++++++++++++++++++---------
 1 file changed, 24 insertions(+), 9 deletions(-)

diff --git a/drivers/gpio/gpio-tegra186.c b/drivers/gpio/gpio-tegra186.c
index 8d298beffd86..2455e1a2c451 100644
--- a/drivers/gpio/gpio-tegra186.c
+++ b/drivers/gpio/gpio-tegra186.c
@@ -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;
-- 
2.30.2

Re: [PATCH 1/5] gpio: Don't fiddle with irqchips marked as immutable

[Jeffrey Hugo]

On Wed, Feb 23, 2022 at 10:40 AM Marc Zyngier <maz@kernel.org> wrote:
>
> 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

[Marc Zyngier]

On 2022-02-23 17:48, Jeffrey Hugo wrote:
> On Wed, Feb 23, 2022 at 10:40 AM Marc Zyngier <maz@kernel.org> wrote:
>> 
>> 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.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 0/5] gpiolib: Handle immutable irq_chip structures

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1009 bytes --]

On Wed, Feb 23, 2022 at 03:44:00PM +0000, Marc Zyngier wrote:
> 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

Hosed in what way? Anything I can help with?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/5] gpio: Don't fiddle with irqchips marked as immutable

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1377 bytes --]

On Wed, Feb 23, 2022 at 03:44:01PM +0000, Marc Zyngier wrote:
> 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 <maz@kernel.org>
> ---
>  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:

Acked-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/5] gpiolib: Handle immutable irq_chip structures

[Marc Zyngier]

On 2022-02-24 16:40, Thierry Reding wrote:
> On Wed, Feb 23, 2022 at 03:44:00PM +0000, Marc Zyngier wrote:
>> 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
> 
> 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.

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/commit/?h=irq/gpio-immutable&id=58d1d925f05485020306e5141336cfd2989843dc
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 1/5] gpio: Don't fiddle with irqchips marked as immutable

[Marc Zyngier]

On Thu, 24 Feb 2022 16:51:41 +0000,
Thierry Reding <thierry.reding@gmail.com> wrote:
> 
> On Wed, Feb 23, 2022 at 03:44:01PM +0000, Marc Zyngier wrote:
> > 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 <maz@kernel.org>
> > ---
> >  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.

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.

> 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 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.

> 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 <treding@nvidia.com>

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH 0/5] gpiolib: Handle immutable irq_chip structures

[Marc Zyngier]

On Thu, 24 Feb 2022 17:42:02 +0000,
Marc Zyngier <maz@kernel.org> wrote:
> 
> On 2022-02-24 16:40, Thierry Reding wrote:
> > On Wed, Feb 23, 2022 at 03:44:00PM +0000, Marc Zyngier wrote:
> >> 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
> > 
> > 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.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH 0/5] gpiolib: Handle immutable irq_chip structures

[Linus Walleij]

On Wed, Feb 23, 2022 at 4:44 PM Marc Zyngier <maz@kernel.org> wrote:

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

Sorry about that...

> 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.

OK if there is a precedent it is usually wise to follow.

> 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

Re: [PATCH 0/5] gpiolib: Handle immutable irq_chip structures

[Marc Zyngier]

On 2022-03-15 00:44, Linus Walleij wrote:
> On Wed, Feb 23, 2022 at 4:44 PM Marc Zyngier <maz@kernel.org> wrote:
> 
>> I recently realised that the gpiolib play ugly tricks on the
>> unsuspecting irq_chip structures 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...

> 
>> 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.
> 
> OK if there is a precedent it is usually wise to follow.
> 
>> 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.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 0/5] gpiolib: Handle immutable irq_chip structures

[Linus Walleij]

On Tue, Mar 15, 2022 at 10:35 AM Marc Zyngier <maz@kernel.org> wrote:

> > 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?

> 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

> I'll repost something once -rc1 is out.

Sounds good!

Yours,
Linus Walleij