LinuxLists.cc - [PATCH 0/8] net: Use platform_get

2021-12-24 19:26:37

Subject: [PATCH 0/8] net: Use platform_get_irq*() variants to fetch IRQ's

Hi All,

This patch series aims to drop using platform_get_resource() for IRQ types
in preparation for removal of static setup of IRQ resource from DT core
code.

Dropping usage of platform_get_resource() was agreed based on
the discussion [0].

[0] https://patchwork.kernel.org/project/linux-renesas-soc/
patch/[email protected]/

Cheers,
Prabhakar

Lad Prabhakar (8):
ethernet: netsec: Use platform_get_irq() to get the interrupt
net: pxa168_eth: Use platform_get_irq() to get the interrupt
fsl/fman: Use platform_get_irq() to get the interrupt
net: ethoc: Use platform_get_irq() to get the interrupt
net: xilinx: emaclite: Use platform_get_irq() to get the interrupt
wcn36xx: Use platform_get_irq_byname() to get the interrupt
ath10k: Use platform_get_irq() to get the interrupt
net: ethernet: ti: davinci_emac: Use platform_get_irq() to get the
interrupt

drivers/net/ethernet/ethoc.c | 9 +--
drivers/net/ethernet/freescale/fman/fman.c | 32 ++++-----
drivers/net/ethernet/marvell/pxa168_eth.c | 9 +--
drivers/net/ethernet/socionext/netsec.c | 13 ++--
drivers/net/ethernet/ti/davinci_emac.c | 69 +++++++++++--------
drivers/net/ethernet/xilinx/xilinx_emaclite.c | 9 +--
drivers/net/wireless/ath/ath10k/snoc.c | 15 ++--
drivers/net/wireless/ath/wcn36xx/main.c | 21 +++---
8 files changed, 88 insertions(+), 89 deletions(-)

--
2.17.1

2021-12-24 19:26:47

by Prabhakar Mahadev Lad

[permalink] [raw]

Subject: [PATCH 1/8] ethernet: netsec: Use platform_get_irq() to get the interrupt

2021-12-24 19:26:47

by Prabhakar Mahadev Lad

[permalink] [raw]

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

2021-12-24 19:26:47

by Prabhakar Mahadev Lad

[permalink] [raw]

Subject: [PATCH 3/8] fsl/fman: Use platform_get_irq() to get the interrupt

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(). While doing so return error pointer
from read_dts_node() as platform_get_irq() may return -EPROBE_DEFER.

Signed-off-by: Lad Prabhakar <[email protected]>
---
drivers/net/ethernet/freescale/fman/fman.c | 32 +++++++++++-----------
1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fman/fman.c b/drivers/net/ethernet/freescale/fman/fman.c
index ce0a121580f6..8f0db61cb1f6 100644
--- a/drivers/net/ethernet/freescale/fman/fman.c
+++ b/drivers/net/ethernet/freescale/fman/fman.c
@@ -2727,7 +2727,7 @@ static struct fman *read_dts_node(struct platform_device *of_dev)

fman = kzalloc(sizeof(*fman), GFP_KERNEL);
if (!fman)
- return NULL;
+ return ERR_PTR(-ENOMEM);

fm_node = of_node_get(of_dev->dev.of_node);

@@ -2740,26 +2740,21 @@ static struct fman *read_dts_node(struct platform_device *of_dev)
fman->dts_params.id = (u8)val;

/* Get the FM interrupt */
- res = platform_get_resource(of_dev, IORESOURCE_IRQ, 0);
- if (!res) {
- dev_err(&of_dev->dev, "%s: Can't get FMan IRQ resource\n",
- __func__);
+ err = platform_get_irq(of_dev, 0);
+ if (err < 0)
goto fman_node_put;
- }
- irq = res->start;
+ irq = err;

/* Get the FM error interrupt */
- res = platform_get_resource(of_dev, IORESOURCE_IRQ, 1);
- if (!res) {
- dev_err(&of_dev->dev, "%s: Can't get FMan Error IRQ resource\n",
- __func__);
+ err = platform_get_irq(of_dev, 1);
+ if (err < 0)
goto fman_node_put;
- }
- fman->dts_params.err_irq = res->start;
+ fman->dts_params.err_irq = err;

/* Get the FM address */
res = platform_get_resource(of_dev, IORESOURCE_MEM, 0);
if (!res) {
+ err = -EINVAL;
dev_err(&of_dev->dev, "%s: Can't get FMan memory resource\n",
__func__);
goto fman_node_put;
@@ -2770,6 +2765,7 @@ static struct fman *read_dts_node(struct platform_device *of_dev)

clk = of_clk_get(fm_node, 0);
if (IS_ERR(clk)) {
+ err = PTR_ERR(clk);
dev_err(&of_dev->dev, "%s: Failed to get FM%d clock structure\n",
__func__, fman->dts_params.id);
goto fman_node_put;
@@ -2777,6 +2773,7 @@ static struct fman *read_dts_node(struct platform_device *of_dev)

clk_rate = clk_get_rate(clk);
if (!clk_rate) {
+ err = -EINVAL;
dev_err(&of_dev->dev, "%s: Failed to determine FM%d clock rate\n",
__func__, fman->dts_params.id);
goto fman_node_put;
@@ -2797,6 +2794,7 @@ static struct fman *read_dts_node(struct platform_device *of_dev)
/* Get the MURAM base address and size */
muram_node = of_find_matching_node(fm_node, fman_muram_match);
if (!muram_node) {
+ err = -EINVAL;
dev_err(&of_dev->dev, "%s: could not find MURAM node\n",
__func__);
goto fman_free;
@@ -2836,6 +2834,7 @@ static struct fman *read_dts_node(struct platform_device *of_dev)
devm_request_mem_region(&of_dev->dev, phys_base_addr,
mem_size, "fman");
if (!fman->dts_params.res) {
+ err = -EBUSY;
dev_err(&of_dev->dev, "%s: request_mem_region() failed\n",
__func__);
goto fman_free;
@@ -2844,6 +2843,7 @@ static struct fman *read_dts_node(struct platform_device *of_dev)
fman->dts_params.base_addr =
devm_ioremap(&of_dev->dev, phys_base_addr, mem_size);
if (!fman->dts_params.base_addr) {
+ err = -ENOMEM;
dev_err(&of_dev->dev, "%s: devm_ioremap() failed\n", __func__);
goto fman_free;
}
@@ -2868,7 +2868,7 @@ static struct fman *read_dts_node(struct platform_device *of_dev)
of_node_put(fm_node);
fman_free:
kfree(fman);
- return NULL;
+ return ERR_PTR(err);
}

static int fman_probe(struct platform_device *of_dev)
@@ -2880,8 +2880,8 @@ static int fman_probe(struct platform_device *of_dev)
dev = &of_dev->dev;

fman = read_dts_node(of_dev);
- if (!fman)
- return -EIO;
+ if (IS_ERR(fman))
+ return PTR_ERR(fman);

err = fman_config(fman);
if (err) {
--
2.17.1

2021-12-24 19:27:00

by Prabhakar Mahadev Lad

[permalink] [raw]

Subject: [PATCH 5/8] net: xilinx: emaclite: Use platform_get_irq() to get the interrupt

2021-12-24 19:27:08

by Prabhakar Mahadev Lad

[permalink] [raw]

Subject: [PATCH 4/8] net: ethoc: Use platform_get_irq() to get the interrupt

2021-12-24 19:27:11

by Prabhakar Mahadev Lad

[permalink] [raw]

Subject: [PATCH 6/8] wcn36xx: Use platform_get_irq_byname() to get the interrupt

platform_get_resource_byname(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_byname().

Signed-off-by: Lad Prabhakar <[email protected]>
---
drivers/net/wireless/ath/wcn36xx/main.c | 21 ++++++++-------------
1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/drivers/net/wireless/ath/wcn36xx/main.c b/drivers/net/wireless/ath/wcn36xx/main.c
index 9575d7373bf2..bd334a302057 100644
--- a/drivers/net/wireless/ath/wcn36xx/main.c
+++ b/drivers/net/wireless/ath/wcn36xx/main.c
@@ -1446,25 +1446,20 @@ static int wcn36xx_platform_get_resources(struct wcn36xx *wcn,
{
struct device_node *mmio_node;
struct device_node *iris_node;
- struct resource *res;
int index;
int ret;

/* Set TX IRQ */
- res = platform_get_resource_byname(pdev, IORESOURCE_IRQ, "tx");
- if (!res) {
- wcn36xx_err("failed to get tx_irq\n");
- return -ENOENT;
- }
- wcn->tx_irq = res->start;
+ ret = platform_get_irq_byname(pdev, "tx");
+ if (ret < 0)
+ return ret;
+ wcn->tx_irq = ret;

/* Set RX IRQ */
- res = platform_get_resource_byname(pdev, IORESOURCE_IRQ, "rx");
- if (!res) {
- wcn36xx_err("failed to get rx_irq\n");
- return -ENOENT;
- }
- wcn->rx_irq = res->start;
+ ret = platform_get_irq_byname(pdev, "rx");
+ if (ret < 0)
+ return ret;
+ wcn->rx_irq = ret;

/* Acquire SMSM tx enable handle */
wcn->tx_enable_state = qcom_smem_state_get(&pdev->dev,
--
2.17.1

2021-12-24 19:27:14

by Prabhakar Mahadev Lad

[permalink] [raw]

Subject: [PATCH 7/8] ath10k: Use platform_get_irq() to get the interrupt

2021-12-24 19:27:19

by Prabhakar Mahadev Lad

[permalink] [raw]

Subject: [PATCH 8/8] net: ethernet: ti: davinci_emac: Use platform_get_irq() to get the interrupt

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() for DT users only.

While at it propagate error code in case request_irq() fails instead of
returning -EBUSY.

Signed-off-by: Lad Prabhakar <[email protected]>
---
drivers/net/ethernet/ti/davinci_emac.c | 69 ++++++++++++++++----------
1 file changed, 42 insertions(+), 27 deletions(-)

diff --git a/drivers/net/ethernet/ti/davinci_emac.c b/drivers/net/ethernet/ti/davinci_emac.c
index d55f06120ce7..31df3267a01a 100644
--- a/drivers/net/ethernet/ti/davinci_emac.c
+++ b/drivers/net/ethernet/ti/davinci_emac.c
@@ -1454,23 +1454,33 @@ static int emac_dev_open(struct net_device *ndev)
}

/* Request IRQ */
- while ((res = platform_get_resource(priv->pdev, IORESOURCE_IRQ,
- res_num))) {
- for (irq_num = res->start; irq_num <= res->end; irq_num++) {
- if (request_irq(irq_num, emac_irq, 0, ndev->name,
- ndev)) {
- dev_err(emac_dev,
- "DaVinci EMAC: request_irq() failed\n");
- ret = -EBUSY;
+ if (dev_of_node(&priv->pdev->dev)) {
+ while ((ret = platform_get_irq_optional(priv->pdev, res_num)) != -ENXIO) {
+ if (ret < 0)
+ goto rollback;

+ ret = request_irq(ret, emac_irq, 0, ndev->name, ndev);
+ if (ret) {
+ dev_err(emac_dev, "DaVinci EMAC: request_irq() failed\n");
goto rollback;
}
+ res_num++;
}
- res_num++;
+ } else {
+ while ((res = platform_get_resource(priv->pdev, IORESOURCE_IRQ, res_num))) {
+ for (irq_num = res->start; irq_num <= res->end; irq_num++) {
+ ret = request_irq(irq_num, emac_irq, 0, ndev->name, ndev);
+ if (ret) {
+ dev_err(emac_dev, "DaVinci EMAC: request_irq() failed\n");
+ goto rollback;
+ }
+ }
+ res_num++;
+ }
+ /* prepare counters for rollback in case of an error */
+ res_num--;
+ irq_num--;
}
- /* prepare counters for rollback in case of an error */
- res_num--;
- irq_num--;

/* Start/Enable EMAC hardware */
emac_hw_enable(priv);
@@ -1554,16 +1564,24 @@ static int emac_dev_open(struct net_device *ndev)
napi_disable(&priv->napi);

rollback:
- for (q = res_num; q >= 0; q--) {
- res = platform_get_resource(priv->pdev, IORESOURCE_IRQ, q);
- /* at the first iteration, irq_num is already set to the
- * right value
- */
- if (q != res_num)
- irq_num = res->end;
+ if (dev_of_node(&priv->pdev->dev)) {
+ for (q = res_num - 1; q >= 0; q--) {
+ irq_num = platform_get_irq(priv->pdev, q);
+ if (irq_num > 0)
+ free_irq(irq_num, ndev);
+ }
+ } else {
+ for (q = res_num; q >= 0; q--) {
+ res = platform_get_resource(priv->pdev, IORESOURCE_IRQ, q);
+ /* at the first iteration, irq_num is already set to the
+ * right value
+ */
+ if (q != res_num)
+ irq_num = res->end;

- for (m = irq_num; m >= res->start; m--)
- free_irq(m, ndev);
+ for (m = irq_num; m >= res->start; m--)
+ free_irq(m, ndev);
+ }
}
cpdma_ctlr_stop(priv->dma);
pm_runtime_put(&priv->pdev->dev);
@@ -1899,13 +1917,10 @@ static int davinci_emac_probe(struct platform_device *pdev)
goto err_free_txchan;
}

- res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
- if (!res) {
- dev_err(&pdev->dev, "error getting irq res\n");
- rc = -ENOENT;
+ rc = platform_get_irq(pdev, 0);
+ if (rc < 0)
goto err_free_rxchan;
- }
- ndev->irq = res->start;
+ ndev->irq = rc;

rc = davinci_emac_try_get_mac(pdev, res_ctrl ? 0 : 1, priv->mac_addr);
if (!rc)
--
2.17.1

2021-12-25 12:19:39

by Lad, Prabhakar

[permalink] [raw]

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

Hi Andy,

Thank you for the review.

On Sat, Dec 25, 2021 at 11:24 AM Andy Shevchenko
<[email protected]> wrote:
>
>
>
> On Friday, December 24, 2021, 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().
>>
>> Signed-off-by: Lad Prabhakar <[email protected]>
>> ---
>> 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
>
>

2021-12-25 15:16:17

by Andrew Lunn

[permalink] [raw]

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

> >> + 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

2021-12-25 16:59:26

by Andy Shevchenko

[permalink] [raw]

Subject: Re: [PATCH 8/8] net: ethernet: ti: davinci_emac: Use platform_get_irq() to get the interrupt

On Sat, Dec 25, 2021 at 4:06 AM 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() for DT users only.
>
> While at it propagate error code in case request_irq() fails instead of
> returning -EBUSY.

> + if (dev_of_node(&priv->pdev->dev)) {

Why?! What's wrong with using the same approach in all cases?

> + while ((ret = platform_get_irq_optional(priv->pdev, res_num)) != -ENXIO) {

This is wrong.

You need to check better as I pointed several times against your patches.

> + if (ret < 0)
> + goto rollback;
>
> + ret = request_irq(ret, emac_irq, 0, ndev->name, ndev);
> + if (ret) {
> + dev_err(emac_dev, "DaVinci EMAC: request_irq() failed\n");
> goto rollback;
> }
> + res_num++;
> }
> - res_num++;
> + } else {
> + while ((res = platform_get_resource(priv->pdev, IORESOURCE_IRQ, res_num))) {
> + for (irq_num = res->start; irq_num <= res->end; irq_num++) {
> + ret = request_irq(irq_num, emac_irq, 0, ndev->name, ndev);
> + if (ret) {
> + dev_err(emac_dev, "DaVinci EMAC: request_irq() failed\n");
> + goto rollback;
> + }
> + }
> + res_num++;
> + }
> + /* prepare counters for rollback in case of an error */
> + res_num--;
> + irq_num--;
> }

--
With Best Regards,
Andy Shevchenko

2021-12-25 17:18:19

by Lad, Prabhakar

[permalink] [raw]

Subject: Re: [PATCH 8/8] net: ethernet: ti: davinci_emac: Use platform_get_irq() to get the interrupt

Hi Andy,

Thank you for the review.

On Sat, Dec 25, 2021 at 4:59 PM Andy Shevchenko
<[email protected]> wrote:
>
> On Sat, Dec 25, 2021 at 4:06 AM 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() for DT users only.
> >
> > While at it propagate error code in case request_irq() fails instead of
> > returning -EBUSY.
>
>
> > + if (dev_of_node(&priv->pdev->dev)) {
>
> Why?! What's wrong with using the same approach in all cases?
>
The earlier code looped the resource from start to end and then
requested the resources. So I believed the the IORESOURCE_IRQ was
sending the range of interrupts. But now looking at the mach-davinci
folder I can confirm its just send an single IRQ resource. So this
check can be dropped and same approach can be used for OF and non OF
users.

> > + while ((ret = platform_get_irq_optional(priv->pdev, res_num)) != -ENXIO) {
>
> This is wrong.
>
> You need to check better as I pointed several times against your patches.
>
Right, Ill have a look.

Cheers,
Prabhakar
> > + if (ret < 0)
> > + goto rollback;
> >
> > + ret = request_irq(ret, emac_irq, 0, ndev->name, ndev);
> > + if (ret) {
> > + dev_err(emac_dev, "DaVinci EMAC: request_irq() failed\n");
> > goto rollback;
> > }
> > + res_num++;
> > }
> > - res_num++;
> > + } else {
> > + while ((res = platform_get_resource(priv->pdev, IORESOURCE_IRQ, res_num))) {
> > + for (irq_num = res->start; irq_num <= res->end; irq_num++) {
> > + ret = request_irq(irq_num, emac_irq, 0, ndev->name, ndev);
> > + if (ret) {
> > + dev_err(emac_dev, "DaVinci EMAC: request_irq() failed\n");
> > + goto rollback;
> > + }
> > + }
> > + res_num++;
> > + }
> > + /* prepare counters for rollback in case of an error */
> > + res_num--;
> > + irq_num--;
> > }
>
> --
> With Best Regards,
> Andy Shevchenko

2021-12-26 11:16:01

by Heiner Kallweit

[permalink] [raw]

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

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
> <[email protected]> wrote:
>>
>>
>>
>> On Friday, December 24, 2021, 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().
>>>
>>> Signed-off-by: Lad Prabhakar <[email protected]>
>>> ---
>>> 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
>>
>>

2021-12-26 14:25:05

by Lad, Prabhakar

[permalink] [raw]

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

Hi Heiner,

Thank you for the review.

On Sun, Dec 26, 2021 at 11:15 AM Heiner Kallweit <[email protected]> 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
> > <[email protected]> wrote:
> >>
> >>
> >>
> >> On Friday, December 24, 2021, 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().
> >>>
> >>> Signed-off-by: Lad Prabhakar <[email protected]>
> >>> ---
> >>> 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