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 <[email protected]>
---
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;
+
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
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 <[email protected]>
> ---
> 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
Hi Niklas,
Thank you for the review.
On Wed, Jan 5, 2022 at 7:13 PM Niklas Söderlund
<[email protected]> wrote:
>
> 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 <[email protected]>
> > ---
> > 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.
>
There are no non-DT users for this driver. Do you see this driver
being used in a non-DT environment in near future?
> 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.
>
No other reason, It was suggested by Gerrt too to use a break instead
of continue in v1.
Cheers,
Prabhakar
On Tue, Jan 4, 2022 at 4:52 PM Lad Prabhakar
<[email protected]> 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().
...
> 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;
Wouldn't be better to assign to ret
ret = platform_get_irq_optional(pdev, i);
if (ret < 0 && ret != -ENXIO)
goto error_unregister;
if (ret > 0)
irq = ret;
else
break;
?
--
With Best Regards,
Andy Shevchenko
Hi Lad,
On 2022-01-05 19:25:25 +0000, Lad, Prabhakar wrote:
> Hi Niklas,
>
> Thank you for the review.
>
> On Wed, Jan 5, 2022 at 7:13 PM Niklas S?derlund
> <[email protected]> wrote:
> >
> > 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 <[email protected]>
> > > ---
> > > 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.
> >
> There are no non-DT users for this driver. Do you see this driver
> being used in a non-DT environment in near future?
No, maybe I was unclear sorry about that. What I intended to say was
that this change will break platforms that that make use of this driver
but do not describe interrupts in its DT description. As with this
change not describing interrupts is consider an error.
For example checkout thermal@ffc48000 in arch/arm/boot/dts/r8a7779.dtsi.
>
> > 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.
> >
> No other reason, It was suggested by Gerrt too to use a break instead
> of continue in v1.
I think we need to keep the original behavior.
>
> Cheers,
> Prabhakar
--
Kind Regards,
Niklas S?derlund
Hi Niklas,
On Thu, Jan 6, 2022 at 3:29 PM Niklas Söderlund
<[email protected]> wrote:
>
> Hi Lad,
>
> On 2022-01-05 19:25:25 +0000, Lad, Prabhakar wrote:
> > Hi Niklas,
> >
> > Thank you for the review.
> >
> > On Wed, Jan 5, 2022 at 7:13 PM Niklas Söderlund
> > <[email protected]> wrote:
> > >
> > > 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 <[email protected]>
> > > > ---
> > > > 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.
> > >
> > There are no non-DT users for this driver. Do you see this driver
> > being used in a non-DT environment in near future?
>
> No, maybe I was unclear sorry about that. What I intended to say was
> that this change will break platforms that that make use of this driver
> but do not describe interrupts in its DT description. As with this
> change not describing interrupts is consider an error.
>
> For example checkout thermal@ffc48000 in arch/arm/boot/dts/r8a7779.dtsi.
>
If the interrupts are missing in DT (for example in [1])
platform_get_irq_optional() will return -ENXIO with this patch this
error code is handled gracefully i.e. it doesn't return error and
breaks instead keeping the orignal behavior of the driver.
[1] arch/arm/boot/dts/r8a7779.dtsi
Cheers,
Prabhakar
On Thu, Jan 6, 2022 at 5:29 PM Niklas Söderlund
<[email protected]> wrote:
> On 2022-01-05 19:25:25 +0000, Lad, Prabhakar wrote:
> > On Wed, Jan 5, 2022 at 7:13 PM Niklas Söderlund
> > <[email protected]> wrote:
> > > On 2022-01-04 14:52:11 +0000, Lad Prabhakar wrote:
...
> > > > + 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.
> > >
> > There are no non-DT users for this driver. Do you see this driver
> > being used in a non-DT environment in near future?
>
> No, maybe I was unclear sorry about that. What I intended to say was
> that this change will break platforms that that make use of this driver
> but do not describe interrupts in its DT description. As with this
> change not describing interrupts is consider an error.
>
> For example checkout thermal@ffc48000 in arch/arm/boot/dts/r8a7779.dtsi.
> > > 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.
> > >
> > No other reason, It was suggested by Gerrt too to use a break instead
> > of continue in v1.
>
> I think we need to keep the original behavior.
I don't see how this can break those. Or are you stating that some of
them are using board files with 0 as a valid (v)IRQ?
--
With Best Regards,
Andy Shevchenko