[PATCH 0/7] Provide and use generic_handle_irq_safe() where appropriate.

[PATCH 0/7] Provide and use generic_handle_irq_safe() where appropriate.

[Sebastian Andrzej Siewior]

generic_handle_irq() must be used from primary IRQ-handler (chain
handler/ interrupt controller entry). It is low level code and the
function expects that interrupts are disabled at entry point.

This isn't the case for invocations from tasklets, workqueues or the
primary interrupt handler on PREEMPT_RT. Once this gets noticed a
"local_irq_disable|safe()" is added. To avoid further confusion this
series adds generic_handle_irq_safe() which can be used from any context
and adds a few user.

Sebastian

[PATCH 1/7] genirq: Provide generic_handle_irq_safe().

[Sebastian Andrzej Siewior]

Provide generic_handle_irq_safe() which can be used can used from any
context.

Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 include/linux/irqdesc.h |  1 +
 kernel/irq/irqdesc.c    | 21 +++++++++++++++++++++
 2 files changed, 22 insertions(+)

diff --git a/include/linux/irqdesc.h b/include/linux/irqdesc.h
index 93d270ca0c567..a77584593f7d1 100644
--- a/include/linux/irqdesc.h
+++ b/include/linux/irqdesc.h
@@ -160,6 +160,7 @@ static inline void generic_handle_irq_desc(struct irq_desc *desc)
 
 int handle_irq_desc(struct irq_desc *desc);
 int generic_handle_irq(unsigned int irq);
+int generic_handle_irq_safe(unsigned int irq);
 
 #ifdef CONFIG_IRQ_DOMAIN
 /*
diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
index 2267e6527db3c..97223df2f460e 100644
--- a/kernel/irq/irqdesc.c
+++ b/kernel/irq/irqdesc.c
@@ -662,6 +662,27 @@ int generic_handle_irq(unsigned int irq)
 }
 EXPORT_SYMBOL_GPL(generic_handle_irq);
 
+/**
+ * generic_handle_irq_safe - Invoke the handler for a particular irq
+ * @irq:	The irq number to handle
+ *
+ * Returns:	0 on success, or -EINVAL if conversion has failed
+ *
+ * This function must be called either from an IRQ context with irq regs
+ * initialized or with care from any context.
+ */
+int generic_handle_irq_safe(unsigned int irq)
+{
+	unsigned long flags;
+	int ret;
+
+	local_irq_save(flags);
+	ret = handle_irq_desc(irq_to_desc(irq));
+	local_irq_restore(flags);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(generic_handle_irq_safe);
+
 #ifdef CONFIG_IRQ_DOMAIN
 /**
  * generic_handle_domain_irq - Invoke the handler for a HW irq belonging
-- 
2.34.1

[PATCH 2/7] i2c: core: Use generic_handle_irq_safe() in i2c_handle_smbus_host_notify().

[Sebastian Andrzej Siewior]

The i2c-i801 driver invokes i2c_handle_smbus_host_notify() from his
interrupt service routine. On PREEMPT_RT i2c-i801's handler is forced
threaded with enabled interrupts which leads to a warning by
handle_irq_event_percpu() assuming that irq_default_primary_handler()
enabled interrupts.

i2c-i801's interrupt handler can't be made non-threaded because the
interrupt line is shared with other devices.

Use generic_handle_irq_safe() which can invoked with disabled and enabled
interrupts.

Reported-by: Michael Below <below@judiz.de>
Link: https://bugs.debian.org/1002537
Cc: Salvatore Bonaccorso <carnil@debian.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/i2c/i2c-core-base.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 2c59dd748a49f..3f9e5303b6163 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -1424,7 +1424,7 @@ int i2c_handle_smbus_host_notify(struct i2c_adapter *adap, unsigned short addr)
 	if (irq <= 0)
 		return -ENXIO;
 
-	generic_handle_irq(irq);
+	generic_handle_irq_safe(irq);
 
 	return 0;
 }
-- 
2.34.1

[PATCH 3/7] i2c: cht-wc: Use generic_handle_irq_safe().

[Sebastian Andrzej Siewior]

Instead of manually disabling interrupts before invoking use
generic_handle_irq() which can be invoked with enabled and disabled
interrupts.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/i2c/busses/i2c-cht-wc.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/i2c/busses/i2c-cht-wc.c b/drivers/i2c/busses/i2c-cht-wc.c
index 1cf68f85b2e11..8ccf0c928bb44 100644
--- a/drivers/i2c/busses/i2c-cht-wc.c
+++ b/drivers/i2c/busses/i2c-cht-wc.c
@@ -99,15 +99,8 @@ static irqreturn_t cht_wc_i2c_adap_thread_handler(int id, void *data)
 	 * interrupt handler as well, so running the client irq handler from
 	 * this thread will cause things to lock up.
 	 */
-	if (reg & CHT_WC_EXTCHGRIRQ_CLIENT_IRQ) {
-		/*
-		 * generic_handle_irq expects local IRQs to be disabled
-		 * as normally it is called from interrupt context.
-		 */
-		local_irq_disable();
-		generic_handle_irq(adap->client_irq);
-		local_irq_enable();
-	}
+	if (reg & CHT_WC_EXTCHGRIRQ_CLIENT_IRQ)
+		generic_handle_irq_safe(adap->client_irq);
 
 	return IRQ_HANDLED;
 }
-- 
2.34.1

[PATCH 4/7] mfd: hi6421-spmi-pmic: Use generic_handle_irq_safe().

[Sebastian Andrzej Siewior]

generic_handle_irq() is invoked from a regular interrupt service
routing. This handler will become a forced-threaded handler on
PREEMPT_RT and will be invoked with enabled interrupts. The
generic_handle_irq() must be invoked with disabled interrupts in order
to avoid deadlocks.

Instead of manually disabling interrupts before invoking use
generic_handle_irq() which can be invoked with enabled and disabled
interrupts.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/misc/hi6421v600-irq.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/misc/hi6421v600-irq.c b/drivers/misc/hi6421v600-irq.c
index 1c763796cf1fa..caa3de37698b0 100644
--- a/drivers/misc/hi6421v600-irq.c
+++ b/drivers/misc/hi6421v600-irq.c
@@ -117,8 +117,8 @@ static irqreturn_t hi6421v600_irq_handler(int irq, void *__priv)
 			 * If both powerkey down and up IRQs are received,
 			 * handle them at the right order
 			 */
-			generic_handle_irq(priv->irqs[POWERKEY_DOWN]);
-			generic_handle_irq(priv->irqs[POWERKEY_UP]);
+			generic_handle_irq_safe(priv->irqs[POWERKEY_DOWN]);
+			generic_handle_irq_safe(priv->irqs[POWERKEY_UP]);
 			pending &= ~HISI_IRQ_POWERKEY_UP_DOWN;
 		}
 
@@ -126,7 +126,7 @@ static irqreturn_t hi6421v600_irq_handler(int irq, void *__priv)
 			continue;
 
 		for_each_set_bit(offset, &pending, BITS_PER_BYTE) {
-			generic_handle_irq(priv->irqs[offset + i * BITS_PER_BYTE]);
+			generic_handle_irq_safe(priv->irqs[offset + i * BITS_PER_BYTE]);
 		}
 	}
 
-- 
2.34.1

[PATCH 5/7] mfd: ezx-pcap: Use generic_handle_irq_safe().

[Sebastian Andrzej Siewior]

Instead of manually disabling interrupts before invoking use
generic_handle_irq() which can be invoked with enabled and disabled
interrupts.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/mfd/ezx-pcap.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/mfd/ezx-pcap.c b/drivers/mfd/ezx-pcap.c
index 70fa18b04ad2b..b14d3f98e1ebd 100644
--- a/drivers/mfd/ezx-pcap.c
+++ b/drivers/mfd/ezx-pcap.c
@@ -193,13 +193,11 @@ static void pcap_isr_work(struct work_struct *work)
 		ezx_pcap_write(pcap, PCAP_REG_MSR, isr | msr);
 		ezx_pcap_write(pcap, PCAP_REG_ISR, isr);
 
-		local_irq_disable();
 		service = isr & ~msr;
 		for (irq = pcap->irq_base; service; service >>= 1, irq++) {
 			if (service & 1)
-				generic_handle_irq(irq);
+				generic_handle_irq_safe(irq);
 		}
-		local_irq_enable();
 		ezx_pcap_write(pcap, PCAP_REG_MSR, pcap->msr);
 	} while (gpio_get_value(pdata->gpio));
 }
-- 
2.34.1

[PATCH 6/7] net: usb: lan78xx: Use generic_handle_irq_safe().

[Sebastian Andrzej Siewior]

Instead of manually disabling interrupts before invoking use
generic_handle_irq() which can be invoked with enabled and disabled
interrupts.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/net/usb/lan78xx.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index b8e20a3f2b84e..415f16662f88e 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -1537,11 +1537,8 @@ static void lan78xx_status(struct lan78xx_net *dev, struct urb *urb)
 		netif_dbg(dev, link, dev->net, "PHY INTR: 0x%08x\n", intdata);
 		lan78xx_defer_kevent(dev, EVENT_LINK_RESET);
 
-		if (dev->domain_data.phyirq > 0) {
-			local_irq_disable();
-			generic_handle_irq(dev->domain_data.phyirq);
-			local_irq_enable();
-		}
+		if (dev->domain_data.phyirq > 0)
+			generic_handle_irq_safe(dev->domain_data.phyirq);
 	} else {
 		netdev_warn(dev->net,
 			    "unexpected interrupt: 0x%08x\n", intdata);
-- 
2.34.1

[PATCH 7/7] staging: greybus: gpio: Use generic_handle_irq_safe().

[Sebastian Andrzej Siewior]

Instead of manually disabling interrupts before invoking use
generic_handle_irq() which can be invoked with enabled and disabled
interrupts.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/staging/greybus/gpio.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/staging/greybus/gpio.c b/drivers/staging/greybus/gpio.c
index 7e6347fe93f99..8a7cf1d0e9688 100644
--- a/drivers/staging/greybus/gpio.c
+++ b/drivers/staging/greybus/gpio.c
@@ -391,10 +391,7 @@ static int gb_gpio_request_handler(struct gb_operation *op)
 		return -EINVAL;
 	}
 
-	local_irq_disable();
-	ret = generic_handle_irq(irq);
-	local_irq_enable();
-
+	ret = generic_handle_irq_safe(irq);
 	if (ret)
 		dev_err(dev, "failed to invoke irq handler\n");
 
-- 
2.34.1

Re: [PATCH 1/7] genirq: Provide generic_handle_irq_safe().

[Hans de Goede]

Hi,

On 1/27/22 12:32, Sebastian Andrzej Siewior wrote:
> Provide generic_handle_irq_safe() which can be used can used from any
> context.
> 
> Suggested-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans


> ---
>  include/linux/irqdesc.h |  1 +
>  kernel/irq/irqdesc.c    | 21 +++++++++++++++++++++
>  2 files changed, 22 insertions(+)
> 
> diff --git a/include/linux/irqdesc.h b/include/linux/irqdesc.h
> index 93d270ca0c567..a77584593f7d1 100644
> --- a/include/linux/irqdesc.h
> +++ b/include/linux/irqdesc.h
> @@ -160,6 +160,7 @@ static inline void generic_handle_irq_desc(struct irq_desc *desc)
>  
>  int handle_irq_desc(struct irq_desc *desc);
>  int generic_handle_irq(unsigned int irq);
> +int generic_handle_irq_safe(unsigned int irq);
>  
>  #ifdef CONFIG_IRQ_DOMAIN
>  /*
> diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
> index 2267e6527db3c..97223df2f460e 100644
> --- a/kernel/irq/irqdesc.c
> +++ b/kernel/irq/irqdesc.c
> @@ -662,6 +662,27 @@ int generic_handle_irq(unsigned int irq)
>  }
>  EXPORT_SYMBOL_GPL(generic_handle_irq);
>  
> +/**
> + * generic_handle_irq_safe - Invoke the handler for a particular irq
> + * @irq:	The irq number to handle
> + *
> + * Returns:	0 on success, or -EINVAL if conversion has failed
> + *
> + * This function must be called either from an IRQ context with irq regs
> + * initialized or with care from any context.
> + */
> +int generic_handle_irq_safe(unsigned int irq)
> +{
> +	unsigned long flags;
> +	int ret;
> +
> +	local_irq_save(flags);
> +	ret = handle_irq_desc(irq_to_desc(irq));
> +	local_irq_restore(flags);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(generic_handle_irq_safe);
> +
>  #ifdef CONFIG_IRQ_DOMAIN
>  /**
>   * generic_handle_domain_irq - Invoke the handler for a HW irq belonging
> 

Re: [PATCH 3/7] i2c: cht-wc: Use generic_handle_irq_safe().

[Hans de Goede]

Hi,

On 1/27/22 12:32, Sebastian Andrzej Siewior wrote:
> Instead of manually disabling interrupts before invoking use
> generic_handle_irq() which can be invoked with enabled and disabled
> interrupts.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans


> ---
>  drivers/i2c/busses/i2c-cht-wc.c | 11 ++---------
>  1 file changed, 2 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-cht-wc.c b/drivers/i2c/busses/i2c-cht-wc.c
> index 1cf68f85b2e11..8ccf0c928bb44 100644
> --- a/drivers/i2c/busses/i2c-cht-wc.c
> +++ b/drivers/i2c/busses/i2c-cht-wc.c
> @@ -99,15 +99,8 @@ static irqreturn_t cht_wc_i2c_adap_thread_handler(int id, void *data)
>  	 * interrupt handler as well, so running the client irq handler from
>  	 * this thread will cause things to lock up.
>  	 */
> -	if (reg & CHT_WC_EXTCHGRIRQ_CLIENT_IRQ) {
> -		/*
> -		 * generic_handle_irq expects local IRQs to be disabled
> -		 * as normally it is called from interrupt context.
> -		 */
> -		local_irq_disable();
> -		generic_handle_irq(adap->client_irq);
> -		local_irq_enable();
> -	}
> +	if (reg & CHT_WC_EXTCHGRIRQ_CLIENT_IRQ)
> +		generic_handle_irq_safe(adap->client_irq);
>  
>  	return IRQ_HANDLED;
>  }
> 

Re: [PATCH 1/7] genirq: Provide generic_handle_irq_safe().

[Oleksandr Natalenko]

Hello.

On čtvrtek 27. ledna 2022 12:32:57 CET Sebastian Andrzej Siewior wrote:
> Provide generic_handle_irq_safe() which can be used can used from any
> context.
> 
> Suggested-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  include/linux/irqdesc.h |  1 +
>  kernel/irq/irqdesc.c    | 21 +++++++++++++++++++++
>  2 files changed, 22 insertions(+)
> 
> diff --git a/include/linux/irqdesc.h b/include/linux/irqdesc.h
> index 93d270ca0c567..a77584593f7d1 100644
> --- a/include/linux/irqdesc.h
> +++ b/include/linux/irqdesc.h
> @@ -160,6 +160,7 @@ static inline void generic_handle_irq_desc(struct irq_desc *desc)
>  
>  int handle_irq_desc(struct irq_desc *desc);
>  int generic_handle_irq(unsigned int irq);
> +int generic_handle_irq_safe(unsigned int irq);
>  
>  #ifdef CONFIG_IRQ_DOMAIN
>  /*
> diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
> index 2267e6527db3c..97223df2f460e 100644
> --- a/kernel/irq/irqdesc.c
> +++ b/kernel/irq/irqdesc.c
> @@ -662,6 +662,27 @@ int generic_handle_irq(unsigned int irq)
>  }
>  EXPORT_SYMBOL_GPL(generic_handle_irq);
>  
> +/**
> + * generic_handle_irq_safe - Invoke the handler for a particular irq
> + * @irq:	The irq number to handle
> + *
> + * Returns:	0 on success, or -EINVAL if conversion has failed
> + *
> + * This function must be called either from an IRQ context with irq regs
> + * initialized or with care from any context.
> + */
> +int generic_handle_irq_safe(unsigned int irq)
> +{
> +	unsigned long flags;
> +	int ret;
> +
> +	local_irq_save(flags);
> +	ret = handle_irq_desc(irq_to_desc(irq));
> +	local_irq_restore(flags);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(generic_handle_irq_safe);
> +
>  #ifdef CONFIG_IRQ_DOMAIN
>  /**
>   * generic_handle_domain_irq - Invoke the handler for a HW irq belonging
> 

Reviewed-by: Oleksandr Natalenko <oleksandr@natalenko.name>

Thank you.

-- 
Oleksandr Natalenko (post-factum)

Re: [PATCH 2/7] i2c: core: Use generic_handle_irq_safe() in i2c_handle_smbus_host_notify().

[Oleksandr Natalenko]

Hello.

On čtvrtek 27. ledna 2022 12:32:58 CET Sebastian Andrzej Siewior wrote:
> The i2c-i801 driver invokes i2c_handle_smbus_host_notify() from his
> interrupt service routine. On PREEMPT_RT i2c-i801's handler is forced
> threaded with enabled interrupts which leads to a warning by
> handle_irq_event_percpu() assuming that irq_default_primary_handler()
> enabled interrupts.
> 
> i2c-i801's interrupt handler can't be made non-threaded because the
> interrupt line is shared with other devices.
> 
> Use generic_handle_irq_safe() which can invoked with disabled and enabled
> interrupts.
> 
> Reported-by: Michael Below <below@judiz.de>
> Link: https://bugs.debian.org/1002537
> Cc: Salvatore Bonaccorso <carnil@debian.org>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  drivers/i2c/i2c-core-base.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 2c59dd748a49f..3f9e5303b6163 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -1424,7 +1424,7 @@ int i2c_handle_smbus_host_notify(struct i2c_adapter *adap, unsigned short addr)
>  	if (irq <= 0)
>  		return -ENXIO;
>  
> -	generic_handle_irq(irq);
> +	generic_handle_irq_safe(irq);
>  
>  	return 0;
>  }
> 

Reviewed-by: Oleksandr Natalenko <oleksandr@natalenko.name>

Worth linking [1] [2] and [3] as well maybe?

[1] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1873673
[2] https://bugzilla.kernel.org/show_bug.cgi?id=202453
[3] https://lore.kernel.org/lkml/20201204201930.vtvitsq6xcftjj3o@spock.localdomain/

-- 
Oleksandr Natalenko (post-factum)

Re: [PATCH 1/7] genirq: Provide generic_handle_irq_safe().

[Wolfram Sang]

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

Hi Sebastian,

> +/**
> + * generic_handle_irq_safe - Invoke the handler for a particular irq

This is the same desc as for generic_handle_irq(). I suggest to add
something like "from any context" to have some distinction.

> + * This function must be called either from an IRQ context with irq regs
> + * initialized or with care from any context.

I think "with care" is not obvious enough. Can you describe it a little?

Thanks for this work,

   Wolfram

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

Re: [PATCH 2/7] i2c: core: Use generic_handle_irq_safe() in i2c_handle_smbus_host_notify().

[Wolfram Sang]

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

On Thu, Jan 27, 2022 at 12:32:58PM +0100, Sebastian Andrzej Siewior wrote:
> The i2c-i801 driver invokes i2c_handle_smbus_host_notify() from his
> interrupt service routine. On PREEMPT_RT i2c-i801's handler is forced
> threaded with enabled interrupts which leads to a warning by
> handle_irq_event_percpu() assuming that irq_default_primary_handler()
> enabled interrupts.
> 
> i2c-i801's interrupt handler can't be made non-threaded because the
> interrupt line is shared with other devices.
> 
> Use generic_handle_irq_safe() which can invoked with disabled and enabled
> interrupts.
> 
> Reported-by: Michael Below <below@judiz.de>
> Link: https://bugs.debian.org/1002537
> Cc: Salvatore Bonaccorso <carnil@debian.org>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

I guess you want this to go together with patch 1, so:

Acked-by: Wolfram Sang <wsa@kernel.org>

I agree with adding the kernel bugzilla entry at least:

https://bugzilla.kernel.org/show_bug.cgi?id=202453

Probably the others which Oleksandr metioned, too.

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

Re: [PATCH 3/7] i2c: cht-wc: Use generic_handle_irq_safe().

[Wolfram Sang]

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

On Thu, Jan 27, 2022 at 12:32:59PM +0100, Sebastian Andrzej Siewior wrote:
> Instead of manually disabling interrupts before invoking use
> generic_handle_irq() which can be invoked with enabled and disabled

generic_handle_irq_safe()

> interrupts.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Besides that:

Acked-by: Wolfram Sang <wsa@kernel.org>


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

Re: [PATCH 1/7] genirq: Provide generic_handle_irq_safe().

[Sergei Shtylyov]

On 1/27/22 2:32 PM, Sebastian Andrzej Siewior wrote:

> Provide generic_handle_irq_safe() which can be used can used from any
                                          ^^^^^^^^^^^^^^^^^^^^
   You're repeating yourself. :-)

> context.
> 
> Suggested-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
[...]

MBR, Sergey

Re: [PATCH 4/7] mfd: hi6421-spmi-pmic: Use generic_handle_irq_safe().

[Sergei Shtylyov]

On 1/27/22 2:33 PM, Sebastian Andrzej Siewior wrote:

> generic_handle_irq() is invoked from a regular interrupt service
> routing. This handler will become a forced-threaded handler on

   s/routing/routine/?

> PREEMPT_RT and will be invoked with enabled interrupts. The
> generic_handle_irq() must be invoked with disabled interrupts in order
> to avoid deadlocks.
> 
> Instead of manually disabling interrupts before invoking use
> generic_handle_irq() which can be invoked with enabled and disabled
> interrupts.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
[...]

MBR, Sergey

Re: [PATCH 4/7] mfd: hi6421-spmi-pmic: Use generic_handle_irq_safe().

[Lee Jones]

On Fri, 28 Jan 2022, Sergei Shtylyov wrote:

> On 1/27/22 2:33 PM, Sebastian Andrzej Siewior wrote:
> 
> > generic_handle_irq() is invoked from a regular interrupt service
> > routing. This handler will become a forced-threaded handler on
> 
>    s/routing/routine/?
> 
> > PREEMPT_RT and will be invoked with enabled interrupts. The
> > generic_handle_irq() must be invoked with disabled interrupts in order
> > to avoid deadlocks.
> > 
> > Instead of manually disabling interrupts before invoking use
> > generic_handle_irq() which can be invoked with enabled and disabled
> > interrupts.
> > 
> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> [...]
> 
> MBR, Sergey

What does that mean?

-- 
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 4/7] mfd: hi6421-spmi-pmic: Use generic_handle_irq_safe().

[Sergei Shtylyov]

On 1/28/22 4:33 PM, Lee Jones wrote:

>>> generic_handle_irq() is invoked from a regular interrupt service
>>> routing. This handler will become a forced-threaded handler on
>>
>>    s/routing/routine/?
>>
>>> PREEMPT_RT and will be invoked with enabled interrupts. The
>>> generic_handle_irq() must be invoked with disabled interrupts in order
>>> to avoid deadlocks.
>>>
>>> Instead of manually disabling interrupts before invoking use
>>> generic_handle_irq() which can be invoked with enabled and disabled
>>> interrupts.
>>>
>>> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
>> [...]
>>
>> MBR, Sergey
> 
> What does that mean?

   That means that I think you had a typo in the word "routing".
The s/// comes from vim, I think --where it means search and replace.

MBR, Sergey

Re: [PATCH 4/7] mfd: hi6421-spmi-pmic: Use generic_handle_irq_safe().

[Sergei Shtylyov]

On 1/28/22 4:33 PM, Lee Jones wrote:

>>> generic_handle_irq() is invoked from a regular interrupt service
>>> routing. This handler will become a forced-threaded handler on
>>
>>    s/routing/routine/?
>>
>>> PREEMPT_RT and will be invoked with enabled interrupts. The
>>> generic_handle_irq() must be invoked with disabled interrupts in order
>>> to avoid deadlocks.
>>>
>>> Instead of manually disabling interrupts before invoking use
>>> generic_handle_irq() which can be invoked with enabled and disabled
>>> interrupts.
>>>
>>> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
>> [...]
>>
>> MBR, Sergey
> 
> What does that mean?

   Ah, you were asking about MBR! My best regards then. :-)

MBR, Sergey

Re: [PATCH 4/7] mfd: hi6421-spmi-pmic: Use generic_handle_irq_safe().

[Lee Jones]

On Fri, 28 Jan 2022, Sergei Shtylyov wrote:

> On 1/28/22 4:33 PM, Lee Jones wrote:
> 
> >>> generic_handle_irq() is invoked from a regular interrupt service
> >>> routing. This handler will become a forced-threaded handler on
> >>
> >>    s/routing/routine/?
> >>
> >>> PREEMPT_RT and will be invoked with enabled interrupts. The
> >>> generic_handle_irq() must be invoked with disabled interrupts in order
> >>> to avoid deadlocks.
> >>>
> >>> Instead of manually disabling interrupts before invoking use
> >>> generic_handle_irq() which can be invoked with enabled and disabled
> >>> interrupts.
> >>>
> >>> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> >> [...]
> >>
> >> MBR, Sergey
> > 
> > What does that mean?
> 
>    Ah, you were asking about MBR! My best regards then. :-)

Yes this.  It's okay, Dan was kind enough to enlighten me.

Every day is a school day on the list! :)

-- 
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 4/7] mfd: hi6421-spmi-pmic: Use generic_handle_irq_safe().

[Sergei Shtylyov]

On 1/28/22 7:50 PM, Lee Jones wrote:

[...]
>>>>> generic_handle_irq() is invoked from a regular interrupt service
>>>>> routing. This handler will become a forced-threaded handler on
>>>>
>>>>    s/routing/routine/?
>>>>
>>>>> PREEMPT_RT and will be invoked with enabled interrupts. The
>>>>> generic_handle_irq() must be invoked with disabled interrupts in order
>>>>> to avoid deadlocks.
>>>>>
>>>>> Instead of manually disabling interrupts before invoking use
>>>>> generic_handle_irq() which can be invoked with enabled and disabled
>>>>> interrupts.
>>>>>
>>>>> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
>>>> [...]
>>>>
>>>> MBR, Sergey
>>>
>>> What does that mean?
>>
>>    Ah, you were asking about MBR! My best regards then. :-)
> 
> Yes this.  It's okay, Dan was kind enough to enlighten me.
> 
> Every day is a school day on the list! :)

   It's not exactly a well known phrase, I like it mainly because it also stands
for the Master Boot Record. :-)

MBR, Sergey

Re: [PATCH 1/7] genirq: Provide generic_handle_irq_safe().

[Sebastian Andrzej Siewior]

On 2022-01-27 18:03:29 [+0100], Wolfram Sang wrote:
> Hi Sebastian,
Hi Wolfram,

> > +/**
> > + * generic_handle_irq_safe - Invoke the handler for a particular irq
> 
> This is the same desc as for generic_handle_irq(). I suggest to add
> something like "from any context" to have some distinction.

There is something later but let me reword that.

> > + * This function must be called either from an IRQ context with irq regs
> > + * initialized or with care from any context.
> 
> I think "with care" is not obvious enough. Can you describe it a little?

Yeah, will do.

> Thanks for this work,
> 
>    Wolfram

Sebastian

Re: [PATCH 1/7] genirq: Provide generic_handle_irq_safe().

[Sebastian Andrzej Siewior]

On 2022-01-28 13:18:14 [+0300], Sergei Shtylyov wrote:
> On 1/27/22 2:32 PM, Sebastian Andrzej Siewior wrote:
> 
> > Provide generic_handle_irq_safe() which can be used can used from any
>                                           ^^^^^^^^^^^^^^^^^^^^
>    You're repeating yourself. :-)

corrected, thank you.

Sebastian

Re: [PATCH 2/7] i2c: core: Use generic_handle_irq_safe() in i2c_handle_smbus_host_notify().

[Sebastian Andrzej Siewior]

On 2022-01-27 18:11:16 [+0100], Wolfram Sang wrote:
> 
> I guess you want this to go together with patch 1, so:
> 
> Acked-by: Wolfram Sang <wsa@kernel.org>
> 
> I agree with adding the kernel bugzilla entry at least:
> 
> https://bugzilla.kernel.org/show_bug.cgi?id=202453
> 
> Probably the others which Oleksandr metioned, too.

No, they are not relevant because none of them can be reproduced on a
v5.12+ kernel or any of <v5.12 stable maintained trees.

They triggered in the past only with the `threadirqs' option on the
commandline and this has been fixed by commit
   81e2073c175b8 ("genirq: Disable interrupts for force threaded handlers")

Sebastian

Re: [PATCH 2/7] i2c: core: Use generic_handle_irq_safe() in i2c_handle_smbus_host_notify().

[Sebastian Andrzej Siewior]

On 2022-01-27 15:41:24 [+0100], Oleksandr Natalenko wrote:
> Hello.
Hi,

> Reviewed-by: Oleksandr Natalenko <oleksandr@natalenko.name>
> 
> Worth linking [1] [2] and [3] as well maybe?

no, they are fixed since commit
   81e2073c175b8 ("genirq: Disable interrupts for force threaded handlers")

> [1] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1873673
> [2] https://bugzilla.kernel.org/show_bug.cgi?id=202453
> [3] https://lore.kernel.org/lkml/20201204201930.vtvitsq6xcftjj3o@spock.localdomain/

Sebastian

Re: [PATCH 3/7] i2c: cht-wc: Use generic_handle_irq_safe().

[Sebastian Andrzej Siewior]

On 2022-01-27 18:16:53 [+0100], Wolfram Sang wrote:
> On Thu, Jan 27, 2022 at 12:32:59PM +0100, Sebastian Andrzej Siewior wrote:
> > Instead of manually disabling interrupts before invoking use
> > generic_handle_irq() which can be invoked with enabled and disabled
> 
> generic_handle_irq_safe()

Yes, thank you.

Sebastian

Re: [PATCH 4/7] mfd: hi6421-spmi-pmic: Use generic_handle_irq_safe().

[Sebastian Andrzej Siewior]

On 2022-01-28 13:23:08 [+0300], Sergei Shtylyov wrote:
> On 1/27/22 2:33 PM, Sebastian Andrzej Siewior wrote:
> 
> > generic_handle_irq() is invoked from a regular interrupt service
> > routing. This handler will become a forced-threaded handler on
> 
>    s/routing/routine/?

Yes, thank you.

Sebastian