[PATCH] tpm: clean up error checking in setup_ring()

[PATCH] tpm: clean up error checking in setup_ring()

[Dan Carpenter]

The bind_evtchn_to_irqhandler() function can never return 0.
Historically, IRQ functions used to return zero on error, but then we
added an architecture where that was a valid IRQ so it went from being
an error to being success (but in a limitted way, just enough for that
arch to boot, I guess).  Eventually we removed it and we will never
allow zero to be a valid IRQ again.

The problem here is that if zero were an invalid IRQ then we should
return a negative error code but the code returns zero/success.

Change the code to check for negative error codes so it's not question
of how the zero should be handled.

Signed-off-by: Dan Carpenter <error27@gmail.com>
---
From static analysis.  Untested.

 drivers/char/tpm/xen-tpmfront.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/char/tpm/xen-tpmfront.c b/drivers/char/tpm/xen-tpmfront.c
index 3c0cce57c4ae..731444eac5f2 100644
--- a/drivers/char/tpm/xen-tpmfront.c
+++ b/drivers/char/tpm/xen-tpmfront.c
@@ -266,7 +266,7 @@ static int setup_ring(struct xenbus_device *dev, struct tpm_private *priv)
 
 	rv = bind_evtchn_to_irqhandler(priv->evtchn, tpmif_interrupt, 0,
 				       "tpmif", priv);
-	if (rv <= 0) {
+	if (rv < 0) {
 		xenbus_dev_fatal(dev, rv, "allocating TPM irq");
 		return rv;
 	}
-- 
2.53.0

Re: [PATCH] tpm: clean up error checking in setup_ring()

[Jarkko Sakkinen]

On Fri, Oct 02, 2026 at 05:44:34PM +0300, Dan Carpenter wrote:
> The bind_evtchn_to_irqhandler() function can never return 0.
> Historically, IRQ functions used to return zero on error, but then we
> added an architecture where that was a valid IRQ so it went from being
> an error to being success (but in a limitted way, just enough for that
> arch to boot, I guess).  Eventually we removed it and we will never
> allow zero to be a valid IRQ again.
> 
> The problem here is that if zero were an invalid IRQ then we should
> return a negative error code but the code returns zero/success.
> 
> Change the code to check for negative error codes so it's not question
> of how the zero should be handled.
> 
> Signed-off-by: Dan Carpenter <error27@gmail.com>
> ---
> From static analysis.  Untested.
> 
>  drivers/char/tpm/xen-tpmfront.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/char/tpm/xen-tpmfront.c b/drivers/char/tpm/xen-tpmfront.c
> index 3c0cce57c4ae..731444eac5f2 100644
> --- a/drivers/char/tpm/xen-tpmfront.c
> +++ b/drivers/char/tpm/xen-tpmfront.c
> @@ -266,7 +266,7 @@ static int setup_ring(struct xenbus_device *dev, struct tpm_private *priv)
>  
>  	rv = bind_evtchn_to_irqhandler(priv->evtchn, tpmif_interrupt, 0,
>  				       "tpmif", priv);
> -	if (rv <= 0) {
> +	if (rv < 0) {
>  		xenbus_dev_fatal(dev, rv, "allocating TPM irq");
>  		return rv;
>  	}
> -- 
> 2.53.0
> 

Looks good to me. Isn't this a bug by definition? 

Br, Jarkko

Re: [PATCH] tpm: clean up error checking in setup_ring()

[Jarkko Sakkinen]

On Fri, Oct 02, 2026 at 05:44:34PM +0300, Dan Carpenter wrote:
> The bind_evtchn_to_irqhandler() function can never return 0.
> Historically, IRQ functions used to return zero on error, but then we
> added an architecture where that was a valid IRQ so it went from being
> an error to being success (but in a limitted way, just enough for that
> arch to boot, I guess).  Eventually we removed it and we will never
> allow zero to be a valid IRQ again.
> 
> The problem here is that if zero were an invalid IRQ then we should
> return a negative error code but the code returns zero/success.
> 
> Change the code to check for negative error codes so it's not question
> of how the zero should be handled.
> 
> Signed-off-by: Dan Carpenter <error27@gmail.com>
> ---
> From static analysis.  Untested.
> 
>  drivers/char/tpm/xen-tpmfront.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/char/tpm/xen-tpmfront.c b/drivers/char/tpm/xen-tpmfront.c
> index 3c0cce57c4ae..731444eac5f2 100644
> --- a/drivers/char/tpm/xen-tpmfront.c
> +++ b/drivers/char/tpm/xen-tpmfront.c
> @@ -266,7 +266,7 @@ static int setup_ring(struct xenbus_device *dev, struct tpm_private *priv)
>  
>  	rv = bind_evtchn_to_irqhandler(priv->evtchn, tpmif_interrupt, 0,
>  				       "tpmif", priv);
> -	if (rv <= 0) {
> +	if (rv < 0) {
>  		xenbus_dev_fatal(dev, rv, "allocating TPM irq");
>  		return rv;
>  	}
> -- 
> 2.53.0
> 

Sorry if responded to this already. Mutt shows this responded but
cannot find it from lore.

Anyway,


Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

Br, Jarkko

Re: [PATCH] tpm: clean up error checking in setup_ring()

[Markus Elfring]

> The bind_evtchn_to_irqhandler() function can never return 0.
…

I suggest to reconsider information once more also according to the probability
of return values from such a function.
https://elixir.bootlin.com/linux/v7.3-rc6/source/drivers/xen/events/events_base.c#L1453-L1461

bind_evtchn_to_irqhandler_chip()
https://elixir.bootlin.com/linux/v7.3-rc6/source/drivers/xen/events/events_base.c#L1432-L1451


…
> ---
> From static analysis.  Untested.
> 
>  drivers/char/tpm/xen-tpmfront.c | 2 +-
…

Would you be looking for further collateral evolution?
https://elixir.bootlin.com/linux/v7.3-rc6/source/drivers/char/tpm/xen-tpmfront.c#L267-L272

Regards,
Markus

Re: [PATCH] tpm: clean up error checking in setup_ring()

[Jarkko Sakkinen]

On Sun, Oct 11, 2026 at 11:30:13AM +0200, Markus Elfring wrote:
> > The bind_evtchn_to_irqhandler() function can never return 0.
> …
> 
> I suggest to reconsider information once more also according to the probability
> of return values from such a function.
> https://elixir.bootlin.com/linux/v7.3-rc6/source/drivers/xen/events/events_base.c#L1453-L1461
> 
> bind_evtchn_to_irqhandler_chip()
> https://elixir.bootlin.com/linux/v7.3-rc6/source/drivers/xen/events/events_base.c#L1432-L1451


I'm not sure what you are trying to exactly or have hard time following
this response but if there is a problem it can be investigated. All I'm
seeing now is a collection of links and abstract statements.

I guess this is the function that picks the IRQ number:

static struct irq_info *xen_allocate_irq_dynamic(void)
{
	int irq = irq_alloc_desc_from(0, -1);
	struct irq_info *info = NULL;

	if (irq >= 0) {
		info = xen_irq_init(irq);
		if (!info)
			xen_irq_free_desc(irq);
	}

	return info;
}

And if I did not get loss while scavenging the call hierachy it ends
up to:

static int irq_find_free_area(unsigned int from, unsigned int cnt)
{
	MA_STATE(mas, &sparse_irqs, 0, 0);

	if (mas_empty_area(&mas, from, MAX_SPARSE_IRQS, cnt))
		return -ENOSPC;
	return mas.index;
}

And if I looked it up right @from == -1 and @cnt == 1 when
irq_find_free_area is reached.

I'm not IRQ code expert per se but in order to this never return zero,
sparse_irqs should not have it available?

I do agree that something in this patch does not appear right, or 
at least not very well explained so it is a good catch anyhow.

My decision on this is that:

1. I'll drop the current patch as it actually does not make a case
   why zero could not possibly happen. I spent 45 minutes browsing
   because commit message did not have any technical details.
2. Even if it was legit it is hard to see that the change is high
   priority for anyone.
3. However, I'm open for v2 with better tecnical backing if there
   is any.

Br, Jarkko