diff mbox series

[2/8] net: pxa168_eth: Use platform_get_irq() to get the interrupt

Message ID 20211224192626.15843-3-prabhakar.mahadev-lad.rj@bp.renesas.com (mailing list archive)
State Accepted
Delegated to: Netdev Maintainers
Headers show
Series net: Use platform_get_irq*() variants to fetch IRQ's | expand

Checks

Context Check Description
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers warning 1 maintainers not CCed: michael@walle.cc
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch warning WARNING: Avoid crashing the kernel - try using WARN_ON & recovery code rather than BUG() or BUG_ON()
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/tree_selection success Guessing tree name failed - patch did not apply

Commit Message

Prabhakar Dec. 24, 2021, 7:26 p.m. UTC
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().

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/net/ethernet/marvell/pxa168_eth.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

Comments

Lad, Prabhakar Dec. 25, 2021, 12:19 p.m. UTC | #1
Hi Andy,

Thank you for the review.

On Sat, Dec 25, 2021 at 11:24 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
>
>
> On Friday, December 24, 2021, Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> 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().
>>
>> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>> ---
>>  drivers/net/ethernet/marvell/pxa168_eth.c | 9 +++++----
>>  1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/marvell/pxa168_eth.c b/drivers/net/ethernet/marvell/pxa168_eth.c
>> index 1d607bc6b59e..52bef50f5a0d 100644
>> --- a/drivers/net/ethernet/marvell/pxa168_eth.c
>> +++ b/drivers/net/ethernet/marvell/pxa168_eth.c
>> @@ -1388,7 +1388,6 @@ static int pxa168_eth_probe(struct platform_device *pdev)
>>  {
>>         struct pxa168_eth_private *pep = NULL;
>>         struct net_device *dev = NULL;
>> -       struct resource *res;
>>         struct clk *clk;
>>         struct device_node *np;
>>         int err;
>> @@ -1419,9 +1418,11 @@ static int pxa168_eth_probe(struct platform_device *pdev)
>>                 goto err_netdev;
>>         }
>>
>> -       res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>> -       BUG_ON(!res);
>> -       dev->irq = res->start;
>> +       err = platform_get_irq(pdev, 0);
>> +       if (err == -EPROBE_DEFER)
>
>
>  What about other errors?
>
Ouch I missed it...
>
>>
>> +               goto err_netdev;
>> +       BUG_ON(dev->irq < 0);
>
>
> ??? What is this and how it supposed to work?
>
.. should have been BUG_ON(dev->irq < 0);

Cheers,
Prabhakar
>>
>> +       dev->irq = err;
>>         dev->netdev_ops = &pxa168_eth_netdev_ops;
>>         dev->watchdog_timeo = 2 * HZ;
>>         dev->base_addr = 0;
>> --
>> 2.17.1
>>
>
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
Andrew Lunn Dec. 25, 2021, 3:16 p.m. UTC | #2
> >> +               goto err_netdev;
> >> +       BUG_ON(dev->irq < 0);
> >
> >
> > ??? What is this and how it supposed to work?
> >
> .. should have been BUG_ON(dev->irq < 0);

Is this fatal to the machine as a whole, now way to recover, all that
can be done is to limit the damage while it explodes?

If not, please use WARN_ON(), not BUG_ON(). There is an email from
Linus about this, not using BUG_ON() in general.

   Andrew
Heiner Kallweit Dec. 26, 2021, 11:14 a.m. UTC | #3
On 25.12.2021 13:19, Lad, Prabhakar wrote:
> Hi Andy,
> 
> Thank you for the review.
> 
> On Sat, Dec 25, 2021 at 11:24 AM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>>
>>
>>
>> On Friday, December 24, 2021, Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> 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().
>>>
>>> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>>> ---
>>>  drivers/net/ethernet/marvell/pxa168_eth.c | 9 +++++----
>>>  1 file changed, 5 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/marvell/pxa168_eth.c b/drivers/net/ethernet/marvell/pxa168_eth.c
>>> index 1d607bc6b59e..52bef50f5a0d 100644
>>> --- a/drivers/net/ethernet/marvell/pxa168_eth.c
>>> +++ b/drivers/net/ethernet/marvell/pxa168_eth.c
>>> @@ -1388,7 +1388,6 @@ static int pxa168_eth_probe(struct platform_device *pdev)
>>>  {
>>>         struct pxa168_eth_private *pep = NULL;
>>>         struct net_device *dev = NULL;
>>> -       struct resource *res;
>>>         struct clk *clk;
>>>         struct device_node *np;
>>>         int err;
>>> @@ -1419,9 +1418,11 @@ static int pxa168_eth_probe(struct platform_device *pdev)
>>>                 goto err_netdev;
>>>         }
>>>
>>> -       res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>>> -       BUG_ON(!res);
>>> -       dev->irq = res->start;
>>> +       err = platform_get_irq(pdev, 0);
>>> +       if (err == -EPROBE_DEFER)
>>
>>
>>  What about other errors?
>>
> Ouch I missed it...
>>
>>>
>>> +               goto err_netdev;
>>> +       BUG_ON(dev->irq < 0);
>>
>>
>> ??? What is this and how it supposed to work?
>>
> 
.. should have been BUG_ON(dev->irq < 0);

Usage of BUG_ON() is discouraged. Better handle the error w/o stopping
the whole system.

> 
> Cheers,
> Prabhakar
>>>
>>> +       dev->irq = err;
>>>         dev->netdev_ops = &pxa168_eth_netdev_ops;
>>>         dev->watchdog_timeo = 2 * HZ;
>>>         dev->base_addr = 0;
>>> --
>>> 2.17.1
>>>
>>
>>
>> --
>> With Best Regards,
>> Andy Shevchenko
>>
>>
Lad, Prabhakar Dec. 26, 2021, 2:24 p.m. UTC | #4
Hi Heiner,

Thank you for the review.

On Sun, Dec 26, 2021 at 11:15 AM Heiner Kallweit <hkallweit1@gmail.com> wrote:
>
> On 25.12.2021 13:19, Lad, Prabhakar wrote:
> > Hi Andy,
> >
> > Thank you for the review.
> >
> > On Sat, Dec 25, 2021 at 11:24 AM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> >>
> >>
> >>
> >> On Friday, December 24, 2021, Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> 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().
> >>>
> >>> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >>> ---
> >>>  drivers/net/ethernet/marvell/pxa168_eth.c | 9 +++++----
> >>>  1 file changed, 5 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/drivers/net/ethernet/marvell/pxa168_eth.c b/drivers/net/ethernet/marvell/pxa168_eth.c
> >>> index 1d607bc6b59e..52bef50f5a0d 100644
> >>> --- a/drivers/net/ethernet/marvell/pxa168_eth.c
> >>> +++ b/drivers/net/ethernet/marvell/pxa168_eth.c
> >>> @@ -1388,7 +1388,6 @@ static int pxa168_eth_probe(struct platform_device *pdev)
> >>>  {
> >>>         struct pxa168_eth_private *pep = NULL;
> >>>         struct net_device *dev = NULL;
> >>> -       struct resource *res;
> >>>         struct clk *clk;
> >>>         struct device_node *np;
> >>>         int err;
> >>> @@ -1419,9 +1418,11 @@ static int pxa168_eth_probe(struct platform_device *pdev)
> >>>                 goto err_netdev;
> >>>         }
> >>>
> >>> -       res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> >>> -       BUG_ON(!res);
> >>> -       dev->irq = res->start;
> >>> +       err = platform_get_irq(pdev, 0);
> >>> +       if (err == -EPROBE_DEFER)
> >>
> >>
> >>  What about other errors?
> >>
> > Ouch I missed it...
> >>
> >>>
> >>> +               goto err_netdev;
> >>> +       BUG_ON(dev->irq < 0);
> >>
> >>
> >> ??? What is this and how it supposed to work?
> >>
> >
> .. should have been BUG_ON(dev->irq < 0);
>
> Usage of BUG_ON() is discouraged. Better handle the error w/o stopping
> the whole system.
>
Agreed, if everyone is OK i'll create a separate patch for removal of
BUG_ON() and the later patch for using platform_get_irq().

Cheers,
Prabhakar
diff mbox series

Patch

diff --git a/drivers/net/ethernet/marvell/pxa168_eth.c b/drivers/net/ethernet/marvell/pxa168_eth.c
index 1d607bc6b59e..52bef50f5a0d 100644
--- a/drivers/net/ethernet/marvell/pxa168_eth.c
+++ b/drivers/net/ethernet/marvell/pxa168_eth.c
@@ -1388,7 +1388,6 @@  static int pxa168_eth_probe(struct platform_device *pdev)
 {
 	struct pxa168_eth_private *pep = NULL;
 	struct net_device *dev = NULL;
-	struct resource *res;
 	struct clk *clk;
 	struct device_node *np;
 	int err;
@@ -1419,9 +1418,11 @@  static int pxa168_eth_probe(struct platform_device *pdev)
 		goto err_netdev;
 	}
 
-	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
-	BUG_ON(!res);
-	dev->irq = res->start;
+	err = platform_get_irq(pdev, 0);
+	if (err == -EPROBE_DEFER)
+		goto err_netdev;
+	BUG_ON(dev->irq < 0);
+	dev->irq = err;
 	dev->netdev_ops = &pxa168_eth_netdev_ops;
 	dev->watchdog_timeo = 2 * HZ;
 	dev->base_addr = 0;