[PATCH 0/2] mailbox: qcom-ipcc: fix error reporting paths

[PATCH 0/2] mailbox: qcom-ipcc: fix error reporting paths

[Gabriele Paoloni]

This patchset implements a couple of fixes to ensure errors being
properly reported in the qcom-ipcc driver.

Thanks
Gab

Gabriele Paoloni (2):
  mailbox: qcom-ipcc: amend qcom_ipcc_domain_map() to report errors
  mailbox: qcom-ipcc: amend qcom_ipcc_irq_fn() to report errors

 drivers/mailbox/qcom-ipcc.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

-- 
2.48.1

[PATCH 1/2] mailbox: qcom-ipcc: amend qcom_ipcc_domain_map() to report errors

[Gabriele Paoloni]

Currently qcom_ipcc_domain_map() ignores errors returned by
irq_set_chip_data() and invokes irq_set_chip_and_handler()
that in turn ignores errors returned by irq_set_chip().
This patch fixes both issues; no other functional changes
are implemented.

Signed-off-by: Gabriele Paoloni <gpaoloni@redhat.com>
---
 drivers/mailbox/qcom-ipcc.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/mailbox/qcom-ipcc.c b/drivers/mailbox/qcom-ipcc.c
index d957d989c0ce..c23efaaa64a1 100644
--- a/drivers/mailbox/qcom-ipcc.c
+++ b/drivers/mailbox/qcom-ipcc.c
@@ -116,12 +116,20 @@ static struct irq_chip qcom_ipcc_irq_chip = {
 static int qcom_ipcc_domain_map(struct irq_domain *d, unsigned int irq,
 				irq_hw_number_t hw)
 {
+	int ret;
 	struct qcom_ipcc *ipcc = d->host_data;
 
-	irq_set_chip_and_handler(irq, &qcom_ipcc_irq_chip, handle_level_irq);
-	irq_set_chip_data(irq, ipcc);
-	irq_set_noprobe(irq);
+	ret = irq_set_chip(irq, &qcom_ipcc_irq_chip);
+	if (ret)
+		return ret;
+
+	irq_set_handler(irq, handle_level_irq);
 
+	ret = irq_set_chip_data(irq, ipcc);
+	if (ret)
+		return ret;
+
+	irq_set_noprobe(irq);
 	return 0;
 }
 
-- 
2.48.1

Re: [PATCH 1/2] mailbox: qcom-ipcc: amend qcom_ipcc_domain_map() to report errors

[Brian Masney]

On Mon, Mar 16, 2026 at 11:26:17AM +0100, Gabriele Paoloni wrote:
> Currently qcom_ipcc_domain_map() ignores errors returned by
> irq_set_chip_data() and invokes irq_set_chip_and_handler()
> that in turn ignores errors returned by irq_set_chip().
> This patch fixes both issues; no other functional changes
> are implemented.
> 
> Signed-off-by: Gabriele Paoloni <gpaoloni@redhat.com>
> ---
>  drivers/mailbox/qcom-ipcc.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mailbox/qcom-ipcc.c b/drivers/mailbox/qcom-ipcc.c
> index d957d989c0ce..c23efaaa64a1 100644
> --- a/drivers/mailbox/qcom-ipcc.c
> +++ b/drivers/mailbox/qcom-ipcc.c
> @@ -116,12 +116,20 @@ static struct irq_chip qcom_ipcc_irq_chip = {
>  static int qcom_ipcc_domain_map(struct irq_domain *d, unsigned int irq,
>  				irq_hw_number_t hw)
>  {
> +	int ret;
>  	struct qcom_ipcc *ipcc = d->host_data;

Put variables in reverse Christmas tree order.

>  
> -	irq_set_chip_and_handler(irq, &qcom_ipcc_irq_chip, handle_level_irq);

Should irq_set_chip_and_handler() and irq_set_chip_and_handler_name() be
updated to return an int to reduce boiler plate code?

> -	irq_set_chip_data(irq, ipcc);
> -	irq_set_noprobe(irq);
> +	ret = irq_set_chip(irq, &qcom_ipcc_irq_chip);
> +	if (ret)
> +		return ret;
> +
> +	irq_set_handler(irq, handle_level_irq);
>  
> +	ret = irq_set_chip_data(irq, ipcc);
> +	if (ret)
> +		return ret;
> +
> +	irq_set_noprobe(irq);
>  	return 0;

The newline before the return 0 is removed. That should also remove the
irq_set_noprobe() change from the diffstat.

Brian

Re: [PATCH 1/2] mailbox: qcom-ipcc: amend qcom_ipcc_domain_map() to report errors

[Gabriele Paoloni]

On Mon, Mar 16, 2026 at 12:05 PM Brian Masney <bmasney@redhat.com> wrote:
>
> On Mon, Mar 16, 2026 at 11:26:17AM +0100, Gabriele Paoloni wrote:
> > Currently qcom_ipcc_domain_map() ignores errors returned by
> > irq_set_chip_data() and invokes irq_set_chip_and_handler()
> > that in turn ignores errors returned by irq_set_chip().
> > This patch fixes both issues; no other functional changes
> > are implemented.
> >
> > Signed-off-by: Gabriele Paoloni <gpaoloni@redhat.com>
> > ---
> >  drivers/mailbox/qcom-ipcc.c | 14 +++++++++++---
> >  1 file changed, 11 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/mailbox/qcom-ipcc.c b/drivers/mailbox/qcom-ipcc.c
> > index d957d989c0ce..c23efaaa64a1 100644
> > --- a/drivers/mailbox/qcom-ipcc.c
> > +++ b/drivers/mailbox/qcom-ipcc.c
> > @@ -116,12 +116,20 @@ static struct irq_chip qcom_ipcc_irq_chip = {
> >  static int qcom_ipcc_domain_map(struct irq_domain *d, unsigned int irq,
> >                               irq_hw_number_t hw)
> >  {
> > +     int ret;
> >       struct qcom_ipcc *ipcc = d->host_data;
>
> Put variables in reverse Christmas tree order.

Noted, I'll fix in v2.

>
> >
> > -     irq_set_chip_and_handler(irq, &qcom_ipcc_irq_chip, handle_level_irq);
>
> Should irq_set_chip_and_handler() and irq_set_chip_and_handler_name() be
> updated to return an int to reduce boiler plate code?

+TO Thomas Gleixner

It may be a possibility. @Thomas do you have any preference in this regard?

Thanks
Gab

>
> > -     irq_set_chip_data(irq, ipcc);
> > -     irq_set_noprobe(irq);
> > +     ret = irq_set_chip(irq, &qcom_ipcc_irq_chip);
> > +     if (ret)
> > +             return ret;
> > +
> > +     irq_set_handler(irq, handle_level_irq);
> >
> > +     ret = irq_set_chip_data(irq, ipcc);
> > +     if (ret)
> > +             return ret;
> > +
> > +     irq_set_noprobe(irq);
> >       return 0;
>
> The newline before the return 0 is removed. That should also remove the
> irq_set_noprobe() change from the diffstat.
>
> Brian
>

[PATCH 2/2] mailbox: qcom-ipcc: amend qcom_ipcc_irq_fn() to report errors

[Gabriele Paoloni]

check the virq value returned by irq_find_mapping(), also
check the return value of generic_handle_irq(); return IRQ_NONE
if either of the checks fails.

Signed-off-by: Gabriele Paoloni <gpaoloni@redhat.com>
---
 drivers/mailbox/qcom-ipcc.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/mailbox/qcom-ipcc.c b/drivers/mailbox/qcom-ipcc.c
index c23efaaa64a1..0795184591f0 100644
--- a/drivers/mailbox/qcom-ipcc.c
+++ b/drivers/mailbox/qcom-ipcc.c
@@ -75,7 +75,7 @@ static irqreturn_t qcom_ipcc_irq_fn(int irq, void *data)
 {
 	struct qcom_ipcc *ipcc = data;
 	u32 hwirq;
-	int virq;
+	int virq, ret;
 
 	for (;;) {
 		hwirq = readl(ipcc->base + IPCC_REG_RECV_ID);
@@ -83,8 +83,14 @@ static irqreturn_t qcom_ipcc_irq_fn(int irq, void *data)
 			break;
 
 		virq = irq_find_mapping(ipcc->irq_domain, hwirq);
+		if (unlikely(!virq))
+			return IRQ_NONE;
+
 		writel(hwirq, ipcc->base + IPCC_REG_RECV_SIGNAL_CLEAR);
-		generic_handle_irq(virq);
+
+		ret = generic_handle_irq(virq);
+		if (unlikely(ret))
+			return IRQ_NONE;
 	}
 
 	return IRQ_HANDLED;
-- 
2.48.1

Re: [PATCH 2/2] mailbox: qcom-ipcc: amend qcom_ipcc_irq_fn() to report errors

[Brian Masney]

On Mon, Mar 16, 2026 at 11:26:18AM +0100, Gabriele Paoloni wrote:
> check the virq value returned by irq_find_mapping(), also
> check the return value of generic_handle_irq(); return IRQ_NONE
> if either of the checks fails.
> 
> Signed-off-by: Gabriele Paoloni <gpaoloni@redhat.com>
> ---
>  drivers/mailbox/qcom-ipcc.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mailbox/qcom-ipcc.c b/drivers/mailbox/qcom-ipcc.c
> index c23efaaa64a1..0795184591f0 100644
> --- a/drivers/mailbox/qcom-ipcc.c
> +++ b/drivers/mailbox/qcom-ipcc.c
> @@ -75,7 +75,7 @@ static irqreturn_t qcom_ipcc_irq_fn(int irq, void *data)
>  {
>  	struct qcom_ipcc *ipcc = data;
>  	u32 hwirq;
> -	int virq;
> +	int virq, ret;

Put variables in reverse Christmas tree order.

Brian


>  
>  	for (;;) {
>  		hwirq = readl(ipcc->base + IPCC_REG_RECV_ID);
> @@ -83,8 +83,14 @@ static irqreturn_t qcom_ipcc_irq_fn(int irq, void *data)
>  			break;
>  
>  		virq = irq_find_mapping(ipcc->irq_domain, hwirq);
> +		if (unlikely(!virq))
> +			return IRQ_NONE;
> +
>  		writel(hwirq, ipcc->base + IPCC_REG_RECV_SIGNAL_CLEAR);
> -		generic_handle_irq(virq);
> +
> +		ret = generic_handle_irq(virq);
> +		if (unlikely(ret))
> +			return IRQ_NONE;
>  	}
>  
>  	return IRQ_HANDLED;
> -- 
> 2.48.1
> 

Re: [PATCH 2/2] mailbox: qcom-ipcc: amend qcom_ipcc_irq_fn() to report errors

[Gabriele Paoloni]

On Mon, Mar 16, 2026 at 12:05 PM Brian Masney <bmasney@redhat.com> wrote:
>
> On Mon, Mar 16, 2026 at 11:26:18AM +0100, Gabriele Paoloni wrote:
> > check the virq value returned by irq_find_mapping(), also
> > check the return value of generic_handle_irq(); return IRQ_NONE
> > if either of the checks fails.
> >
> > Signed-off-by: Gabriele Paoloni <gpaoloni@redhat.com>
> > ---
> >  drivers/mailbox/qcom-ipcc.c | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/mailbox/qcom-ipcc.c b/drivers/mailbox/qcom-ipcc.c
> > index c23efaaa64a1..0795184591f0 100644
> > --- a/drivers/mailbox/qcom-ipcc.c
> > +++ b/drivers/mailbox/qcom-ipcc.c
> > @@ -75,7 +75,7 @@ static irqreturn_t qcom_ipcc_irq_fn(int irq, void *data)
> >  {
> >       struct qcom_ipcc *ipcc = data;
> >       u32 hwirq;
> > -     int virq;
> > +     int virq, ret;
>
> Put variables in reverse Christmas tree order.

Note, many thanks. I will fix this in v2

Gab

>
> Brian
>
>
> >
> >       for (;;) {
> >               hwirq = readl(ipcc->base + IPCC_REG_RECV_ID);
> > @@ -83,8 +83,14 @@ static irqreturn_t qcom_ipcc_irq_fn(int irq, void *data)
> >                       break;
> >
> >               virq = irq_find_mapping(ipcc->irq_domain, hwirq);
> > +             if (unlikely(!virq))
> > +                     return IRQ_NONE;
> > +
> >               writel(hwirq, ipcc->base + IPCC_REG_RECV_SIGNAL_CLEAR);
> > -             generic_handle_irq(virq);
> > +
> > +             ret = generic_handle_irq(virq);
> > +             if (unlikely(ret))
> > +                     return IRQ_NONE;
> >       }
> >
> >       return IRQ_HANDLED;
> > --
> > 2.48.1
> >
>

Re: [PATCH 2/2] mailbox: qcom-ipcc: amend qcom_ipcc_irq_fn() to report errors

[Konrad Dybcio]

On 3/16/26 11:26 AM, Gabriele Paoloni wrote:
> check the virq value returned by irq_find_mapping(), also
> check the return value of generic_handle_irq(); return IRQ_NONE
> if either of the checks fails.
> 
> Signed-off-by: Gabriele Paoloni <gpaoloni@redhat.com>
> ---

It certainly seems useful to inspect a function's return value, however I
noticed that out of 47 in-tree callers of generic_handle_irq() within
drivers/, only one other driver (intel_lpe_audio within drm/i915) checks
that..

Similarly, many other drivers never check if irq_find_mapping() actually
succeeded. This makes me wonder how you discovered there's an issue in
the first place. Could you please add some details in the commit message?

Konrad

Re: [PATCH 2/2] mailbox: qcom-ipcc: amend qcom_ipcc_irq_fn() to report errors

[Gabriele Paoloni]

Hi Konrad

On Tue, Mar 17, 2026 at 10:13 AM Konrad Dybcio
<konrad.dybcio@oss.qualcomm.com> wrote:
>
> On 3/16/26 11:26 AM, Gabriele Paoloni wrote:
> > check the virq value returned by irq_find_mapping(), also
> > check the return value of generic_handle_irq(); return IRQ_NONE
> > if either of the checks fails.
> >
> > Signed-off-by: Gabriele Paoloni <gpaoloni@redhat.com>
> > ---
>
> It certainly seems useful to inspect a function's return value, however I
> noticed that out of 47 in-tree callers of generic_handle_irq() within
> drivers/, only one other driver (intel_lpe_audio within drm/i915) checks
> that..
>
> Similarly, many other drivers never check if irq_find_mapping() actually
> succeeded. This makes me wonder how you discovered there's an issue in
> the first place. Could you please add some details in the commit message?

Code inspection spotted the issue, and I could not find a justification for
irq_find_mapping() and generic_handle_irq() to always succeed; hence
I proposed this patch to follow good coding practices.
However if there is a justification for these functions to always succeed
in the context of use of the Qualcomm IPCC driver, I can drop this patch.

Thanks
Gab

>
> Konrad
>

Re: [PATCH 2/2] mailbox: qcom-ipcc: amend qcom_ipcc_irq_fn() to report errors

[Konrad Dybcio]

On 3/20/26 12:24 PM, Gabriele Paoloni wrote:
> Hi Konrad
> 
> On Tue, Mar 17, 2026 at 10:13 AM Konrad Dybcio
> <konrad.dybcio@oss.qualcomm.com> wrote:
>>
>> On 3/16/26 11:26 AM, Gabriele Paoloni wrote:
>>> check the virq value returned by irq_find_mapping(), also
>>> check the return value of generic_handle_irq(); return IRQ_NONE
>>> if either of the checks fails.
>>>
>>> Signed-off-by: Gabriele Paoloni <gpaoloni@redhat.com>
>>> ---
>>
>> It certainly seems useful to inspect a function's return value, however I
>> noticed that out of 47 in-tree callers of generic_handle_irq() within
>> drivers/, only one other driver (intel_lpe_audio within drm/i915) checks
>> that..
>>
>> Similarly, many other drivers never check if irq_find_mapping() actually
>> succeeded. This makes me wonder how you discovered there's an issue in
>> the first place. Could you please add some details in the commit message?
> 
> Code inspection spotted the issue, and I could not find a justification for
> irq_find_mapping() and generic_handle_irq() to always succeed; hence
> I proposed this patch to follow good coding practices.

That's a good enough reason

Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

Konrad