linux-kernel - Re: [PATCH v3] thermal: rcar_thermal: Use platform_get_irq

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 5 Jan 2022 20:13:30 +0100
From:   Niklas Söderlund 
        <niklas.soderlund@...natech.se>
To:     Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
Cc:     Geert Uytterhoeven <geert+renesas@...der.be>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Daniel Lezcano <daniel.lezcano@...aro.org>,
        Amit Kucheria <amitk@...nel.org>,
        Zhang Rui <rui.zhang@...el.com>,
        Rob Herring <robh+dt@...nel.org>,
        Andy Shevchenko <andy.shevchenko@...il.com>,
        Prabhakar <prabhakar.csengg@...il.com>,
        linux-renesas-soc@...r.kernel.org, linux-pm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] thermal: rcar_thermal: Use
 platform_get_irq_optional() to get the interrupt

Hi Lad,

Thanks for your work.

On 2022-01-04 14:52:11 +0000, Lad Prabhakar wrote:
> platform_get_resource(pdev, IORESOURCE_IRQ, ..) relies on static
> allocation of IRQ resources in DT core code, this causes an issue
> when using hierarchical interrupt domains using "interrupts" property
> in the node as this bypasses the hierarchical setup and messes up the
> irq chaining.
> 
> In preparation for removal of static setup of IRQ resource from DT core
> code use platform_get_irq_optional().
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
> ---
> v2-v3:
> * Fixed review comment pointed by Andy
> 
> v1->v2
> * Simplified checking error code
> * Break loop earlier if no interrupts are seen
> 
> v1: https://lkml.org/lkml/2021/12/18/163
> ---
>  drivers/thermal/rcar_thermal.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/thermal/rcar_thermal.c b/drivers/thermal/rcar_thermal.c
> index b49f04daaf47..e480f7290ccf 100644
> --- a/drivers/thermal/rcar_thermal.c
> +++ b/drivers/thermal/rcar_thermal.c
> @@ -445,7 +445,7 @@ static int rcar_thermal_probe(struct platform_device *pdev)
>  	struct rcar_thermal_common *common;
>  	struct rcar_thermal_priv *priv;
>  	struct device *dev = &pdev->dev;
> -	struct resource *res, *irq;
> +	struct resource *res;
>  	const struct rcar_thermal_chip *chip = of_device_get_match_data(dev);
>  	int mres = 0;
>  	int i;
> @@ -467,9 +467,16 @@ static int rcar_thermal_probe(struct platform_device *pdev)
>  	pm_runtime_get_sync(dev);
>  
>  	for (i = 0; i < chip->nirqs; i++) {
> -		irq = platform_get_resource(pdev, IORESOURCE_IRQ, i);
> -		if (!irq)
> -			continue;
> +		int irq;
> +
> +		irq = platform_get_irq_optional(pdev, i);
> +		if (irq < 0 && irq != -ENXIO) {
> +			ret = irq;
> +			goto error_unregister;
> +		}
> +		if (!irq || irq == -ENXIO)
> +			break;

This do not look correct and differs form v1.

In the old code if we can't get an IRQ the loop is continued. This is 
used to detect if interrupts are supported or not on the platform.  This 
change will fail on all systems that don't describes interrupts in DT 
while the driver can function without interrupts.

Is there a reason you wish to do this change in addition to the switch 
to platform_get_irq_optional()? If so I think that should be done in a 
separate patch.

> +
>  		if (!common->base) {
>  			/*
>  			 * platform has IRQ support.
> @@ -487,7 +494,7 @@ static int rcar_thermal_probe(struct platform_device *pdev)
>  			idle = 0; /* polling delay is not needed */
>  		}
>  
> -		ret = devm_request_irq(dev, irq->start, rcar_thermal_irq,
> +		ret = devm_request_irq(dev, irq, rcar_thermal_irq,
>  				       IRQF_SHARED, dev_name(dev), common);
>  		if (ret) {
>  			dev_err(dev, "irq request failed\n ");
> -- 
> 2.17.1
> 

-- 
Kind Regards,
Niklas Söderlund