* [PATCH v2 net-next] net: ethernet: ti: cpdma: correct error handling for chan create
@ 2017-12-12 16:35 Ivan Khoronzhuk
2017-12-12 17:08 ` Grygorii Strashko
0 siblings, 1 reply; 4+ messages in thread
From: Ivan Khoronzhuk @ 2017-12-12 16:35 UTC (permalink / raw)
To: grygorii.strashko
Cc: netdev, davem, linux-omap, linux-kernel, Ivan Khoronzhuk
It's not correct to return NULL when that is actually an error and
function returns errors in any other wrong case. In the same time,
the cpsw driver and davinci emac doesn't check error case while
creating channel and it can miss actual error. Also remove WARNs
duplicated dev_err msgs.
Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
---
drivers/net/ethernet/ti/cpsw.c | 12 +++++++++---
drivers/net/ethernet/ti/davinci_cpdma.c | 2 +-
drivers/net/ethernet/ti/davinci_emac.c | 9 +++++++--
3 files changed, 17 insertions(+), 6 deletions(-)
diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index a60a378..3c85a08 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -3065,10 +3065,16 @@ static int cpsw_probe(struct platform_device *pdev)
}
cpsw->txv[0].ch = cpdma_chan_create(cpsw->dma, 0, cpsw_tx_handler, 0);
+ if (IS_ERR(cpsw->txv[0].ch)) {
+ dev_err(priv->dev, "error initializing tx dma channel\n");
+ ret = PTR_ERR(cpsw->txv[0].ch);
+ goto clean_dma_ret;
+ }
+
cpsw->rxv[0].ch = cpdma_chan_create(cpsw->dma, 0, cpsw_rx_handler, 1);
- if (WARN_ON(!cpsw->rxv[0].ch || !cpsw->txv[0].ch)) {
- dev_err(priv->dev, "error initializing dma channels\n");
- ret = -ENOMEM;
+ if (IS_ERR(cpsw->rxv[0].ch)) {
+ dev_err(priv->dev, "error initializing rx dma channel\n");
+ ret = PTR_ERR(cpsw->rxv[0].ch);
goto clean_dma_ret;
}
diff --git a/drivers/net/ethernet/ti/davinci_cpdma.c b/drivers/net/ethernet/ti/davinci_cpdma.c
index e4d6edf..6f9173f 100644
--- a/drivers/net/ethernet/ti/davinci_cpdma.c
+++ b/drivers/net/ethernet/ti/davinci_cpdma.c
@@ -893,7 +893,7 @@ struct cpdma_chan *cpdma_chan_create(struct cpdma_ctlr *ctlr, int chan_num,
chan_num = rx_type ? rx_chan_num(chan_num) : tx_chan_num(chan_num);
if (__chan_linear(chan_num) >= ctlr->num_chan)
- return NULL;
+ return ERR_PTR(-EINVAL);
chan = devm_kzalloc(ctlr->dev, sizeof(*chan), GFP_KERNEL);
if (!chan)
diff --git a/drivers/net/ethernet/ti/davinci_emac.c b/drivers/net/ethernet/ti/davinci_emac.c
index f58c0c6..3d4af64 100644
--- a/drivers/net/ethernet/ti/davinci_emac.c
+++ b/drivers/net/ethernet/ti/davinci_emac.c
@@ -1870,10 +1870,15 @@ static int davinci_emac_probe(struct platform_device *pdev)
priv->txchan = cpdma_chan_create(priv->dma, EMAC_DEF_TX_CH,
emac_tx_handler, 0);
+ if (WARN_ON(IS_ERR(priv->txchan))) {
+ rc = PTR_ERR(priv->txchan);
+ goto no_cpdma_chan;
+ }
+
priv->rxchan = cpdma_chan_create(priv->dma, EMAC_DEF_RX_CH,
emac_rx_handler, 1);
- if (WARN_ON(!priv->txchan || !priv->rxchan)) {
- rc = -ENOMEM;
+ if (WARN_ON(IS_ERR(priv->rxchan))) {
+ rc = PTR_ERR(priv->rxchan);
goto no_cpdma_chan;
}
--
2.7.4
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH v2 net-next] net: ethernet: ti: cpdma: correct error handling for chan create
2017-12-12 16:35 [PATCH v2 net-next] net: ethernet: ti: cpdma: correct error handling for chan create Ivan Khoronzhuk
@ 2017-12-12 17:08 ` Grygorii Strashko
2017-12-12 17:50 ` Ivan Khoronzhuk
0 siblings, 1 reply; 4+ messages in thread
From: Grygorii Strashko @ 2017-12-12 17:08 UTC (permalink / raw)
To: Ivan Khoronzhuk; +Cc: netdev, davem, linux-omap, linux-kernel
On 12/12/2017 10:35 AM, Ivan Khoronzhuk wrote:
> It's not correct to return NULL when that is actually an error and
> function returns errors in any other wrong case. In the same time,
> the cpsw driver and davinci emac doesn't check error case while
> creating channel and it can miss actual error. Also remove WARNs
> duplicated dev_err msgs.
>
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> ---
> drivers/net/ethernet/ti/cpsw.c | 12 +++++++++---
> drivers/net/ethernet/ti/davinci_cpdma.c | 2 +-
> drivers/net/ethernet/ti/davinci_emac.c | 9 +++++++--
> 3 files changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
> index a60a378..3c85a08 100644
> --- a/drivers/net/ethernet/ti/cpsw.c
> +++ b/drivers/net/ethernet/ti/cpsw.c
> @@ -3065,10 +3065,16 @@ static int cpsw_probe(struct platform_device *pdev)
> }
>
> cpsw->txv[0].ch = cpdma_chan_create(cpsw->dma, 0, cpsw_tx_handler, 0);
> + if (IS_ERR(cpsw->txv[0].ch)) {
> + dev_err(priv->dev, "error initializing tx dma channel\n");
> + ret = PTR_ERR(cpsw->txv[0].ch);
> + goto clean_dma_ret;
> + }
> +
> cpsw->rxv[0].ch = cpdma_chan_create(cpsw->dma, 0, cpsw_rx_handler, 1);
> - if (WARN_ON(!cpsw->rxv[0].ch || !cpsw->txv[0].ch)) {
> - dev_err(priv->dev, "error initializing dma channels\n");
> - ret = -ENOMEM;
> + if (IS_ERR(cpsw->rxv[0].ch)) {
> + dev_err(priv->dev, "error initializing rx dma channel\n");
> + ret = PTR_ERR(cpsw->rxv[0].ch);
> goto clean_dma_ret;
> }
>
> diff --git a/drivers/net/ethernet/ti/davinci_cpdma.c b/drivers/net/ethernet/ti/davinci_cpdma.c
> index e4d6edf..6f9173f 100644
> --- a/drivers/net/ethernet/ti/davinci_cpdma.c
> +++ b/drivers/net/ethernet/ti/davinci_cpdma.c
> @@ -893,7 +893,7 @@ struct cpdma_chan *cpdma_chan_create(struct cpdma_ctlr *ctlr, int chan_num,
> chan_num = rx_type ? rx_chan_num(chan_num) : tx_chan_num(chan_num);
>
> if (__chan_linear(chan_num) >= ctlr->num_chan)
> - return NULL;
> + return ERR_PTR(-EINVAL);
>
> chan = devm_kzalloc(ctlr->dev, sizeof(*chan), GFP_KERNEL);
> if (!chan)
> diff --git a/drivers/net/ethernet/ti/davinci_emac.c b/drivers/net/ethernet/ti/davinci_emac.c
> index f58c0c6..3d4af64 100644
> --- a/drivers/net/ethernet/ti/davinci_emac.c
> +++ b/drivers/net/ethernet/ti/davinci_emac.c
> @@ -1870,10 +1870,15 @@ static int davinci_emac_probe(struct platform_device *pdev)
>
> priv->txchan = cpdma_chan_create(priv->dma, EMAC_DEF_TX_CH,
> emac_tx_handler, 0);
> + if (WARN_ON(IS_ERR(priv->txchan))) {
So, logically WARN_ON() should be removed in davinci_emac.c also. Right?
> + rc = PTR_ERR(priv->txchan);
> + goto no_cpdma_chan;
> + }
> +
> priv->rxchan = cpdma_chan_create(priv->dma, EMAC_DEF_RX_CH,
> emac_rx_handler, 1);
> - if (WARN_ON(!priv->txchan || !priv->rxchan)) {
> - rc = -ENOMEM;
> + if (WARN_ON(IS_ERR(priv->rxchan))) {
> + rc = PTR_ERR(priv->rxchan);
> goto no_cpdma_chan;
> }
>
>
--
regards,
-grygorii
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH v2 net-next] net: ethernet: ti: cpdma: correct error handling for chan create
2017-12-12 17:08 ` Grygorii Strashko
@ 2017-12-12 17:50 ` Ivan Khoronzhuk
2017-12-12 18:16 ` Grygorii Strashko
0 siblings, 1 reply; 4+ messages in thread
From: Ivan Khoronzhuk @ 2017-12-12 17:50 UTC (permalink / raw)
To: Grygorii Strashko; +Cc: netdev, davem, linux-omap, linux-kernel
On Tue, Dec 12, 2017 at 11:08:51AM -0600, Grygorii Strashko wrote:
>
>
> On 12/12/2017 10:35 AM, Ivan Khoronzhuk wrote:
> > It's not correct to return NULL when that is actually an error and
> > function returns errors in any other wrong case. In the same time,
> > the cpsw driver and davinci emac doesn't check error case while
> > creating channel and it can miss actual error. Also remove WARNs
> > duplicated dev_err msgs.
> >
> > Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> > ---
> > drivers/net/ethernet/ti/cpsw.c | 12 +++++++++---
> > drivers/net/ethernet/ti/davinci_cpdma.c | 2 +-
> > drivers/net/ethernet/ti/davinci_emac.c | 9 +++++++--
> > 3 files changed, 17 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
> > index a60a378..3c85a08 100644
> > --- a/drivers/net/ethernet/ti/cpsw.c
> > +++ b/drivers/net/ethernet/ti/cpsw.c
> > @@ -3065,10 +3065,16 @@ static int cpsw_probe(struct platform_device *pdev)
> > }
> >
> > cpsw->txv[0].ch = cpdma_chan_create(cpsw->dma, 0, cpsw_tx_handler, 0);
> > + if (IS_ERR(cpsw->txv[0].ch)) {
> > + dev_err(priv->dev, "error initializing tx dma channel\n");
> > + ret = PTR_ERR(cpsw->txv[0].ch);
> > + goto clean_dma_ret;
> > + }
> > +
> > cpsw->rxv[0].ch = cpdma_chan_create(cpsw->dma, 0, cpsw_rx_handler, 1);
> > - if (WARN_ON(!cpsw->rxv[0].ch || !cpsw->txv[0].ch)) {
> > - dev_err(priv->dev, "error initializing dma channels\n");
> > - ret = -ENOMEM;
> > + if (IS_ERR(cpsw->rxv[0].ch)) {
> > + dev_err(priv->dev, "error initializing rx dma channel\n");
> > + ret = PTR_ERR(cpsw->rxv[0].ch);
> > goto clean_dma_ret;
> > }
> >
> > diff --git a/drivers/net/ethernet/ti/davinci_cpdma.c b/drivers/net/ethernet/ti/davinci_cpdma.c
> > index e4d6edf..6f9173f 100644
> > --- a/drivers/net/ethernet/ti/davinci_cpdma.c
> > +++ b/drivers/net/ethernet/ti/davinci_cpdma.c
> > @@ -893,7 +893,7 @@ struct cpdma_chan *cpdma_chan_create(struct cpdma_ctlr *ctlr, int chan_num,
> > chan_num = rx_type ? rx_chan_num(chan_num) : tx_chan_num(chan_num);
> >
> > if (__chan_linear(chan_num) >= ctlr->num_chan)
> > - return NULL;
> > + return ERR_PTR(-EINVAL);
> >
> > chan = devm_kzalloc(ctlr->dev, sizeof(*chan), GFP_KERNEL);
> > if (!chan)
> > diff --git a/drivers/net/ethernet/ti/davinci_emac.c b/drivers/net/ethernet/ti/davinci_emac.c
> > index f58c0c6..3d4af64 100644
> > --- a/drivers/net/ethernet/ti/davinci_emac.c
> > +++ b/drivers/net/ethernet/ti/davinci_emac.c
> > @@ -1870,10 +1870,15 @@ static int davinci_emac_probe(struct platform_device *pdev)
> >
> > priv->txchan = cpdma_chan_create(priv->dma, EMAC_DEF_TX_CH,
> > emac_tx_handler, 0);
> > + if (WARN_ON(IS_ERR(priv->txchan))) {
>
> So, logically WARN_ON() should be removed in davinci_emac.c also. Right?
It doesn't have dev_err() duplicate, so not very.
But would be better to replace them on dev_err() if no objection.
>
> > + rc = PTR_ERR(priv->txchan);
> > + goto no_cpdma_chan;
> > + }
> > +
> > priv->rxchan = cpdma_chan_create(priv->dma, EMAC_DEF_RX_CH,
> > emac_rx_handler, 1);
> > - if (WARN_ON(!priv->txchan || !priv->rxchan)) {
> > - rc = -ENOMEM;
> > + if (WARN_ON(IS_ERR(priv->rxchan))) {
> > + rc = PTR_ERR(priv->rxchan);
> > goto no_cpdma_chan;
> > }
> >
> >
>
> --
> regards,
> -grygorii
--
Regards,
Ivan Khoronzhuk
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH v2 net-next] net: ethernet: ti: cpdma: correct error handling for chan create
2017-12-12 17:50 ` Ivan Khoronzhuk
@ 2017-12-12 18:16 ` Grygorii Strashko
0 siblings, 0 replies; 4+ messages in thread
From: Grygorii Strashko @ 2017-12-12 18:16 UTC (permalink / raw)
To: netdev, davem, linux-omap, linux-kernel
On 12/12/2017 11:50 AM, Ivan Khoronzhuk wrote:
> On Tue, Dec 12, 2017 at 11:08:51AM -0600, Grygorii Strashko wrote:
>>
>>
>> On 12/12/2017 10:35 AM, Ivan Khoronzhuk wrote:
>>> It's not correct to return NULL when that is actually an error and
>>> function returns errors in any other wrong case. In the same time,
>>> the cpsw driver and davinci emac doesn't check error case while
>>> creating channel and it can miss actual error. Also remove WARNs
>>> duplicated dev_err msgs.
>>>
>>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
>>> ---
>>> drivers/net/ethernet/ti/cpsw.c | 12 +++++++++---
>>> drivers/net/ethernet/ti/davinci_cpdma.c | 2 +-
>>> drivers/net/ethernet/ti/davinci_emac.c | 9 +++++++--
>>> 3 files changed, 17 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
>>> index a60a378..3c85a08 100644
>>> --- a/drivers/net/ethernet/ti/cpsw.c
>>> +++ b/drivers/net/ethernet/ti/cpsw.c
>>> @@ -3065,10 +3065,16 @@ static int cpsw_probe(struct platform_device *pdev)
>>> }
>>>
>>> cpsw->txv[0].ch = cpdma_chan_create(cpsw->dma, 0, cpsw_tx_handler, 0);
>>> + if (IS_ERR(cpsw->txv[0].ch)) {
>>> + dev_err(priv->dev, "error initializing tx dma channel\n");
>>> + ret = PTR_ERR(cpsw->txv[0].ch);
>>> + goto clean_dma_ret;
>>> + }
>>> +
>>> cpsw->rxv[0].ch = cpdma_chan_create(cpsw->dma, 0, cpsw_rx_handler, 1);
>>> - if (WARN_ON(!cpsw->rxv[0].ch || !cpsw->txv[0].ch)) {
>>> - dev_err(priv->dev, "error initializing dma channels\n");
>>> - ret = -ENOMEM;
>>> + if (IS_ERR(cpsw->rxv[0].ch)) {
>>> + dev_err(priv->dev, "error initializing rx dma channel\n");
>>> + ret = PTR_ERR(cpsw->rxv[0].ch);
>>> goto clean_dma_ret;
>>> }
>>>
>>> diff --git a/drivers/net/ethernet/ti/davinci_cpdma.c b/drivers/net/ethernet/ti/davinci_cpdma.c
>>> index e4d6edf..6f9173f 100644
>>> --- a/drivers/net/ethernet/ti/davinci_cpdma.c
>>> +++ b/drivers/net/ethernet/ti/davinci_cpdma.c
>>> @@ -893,7 +893,7 @@ struct cpdma_chan *cpdma_chan_create(struct cpdma_ctlr *ctlr, int chan_num,
>>> chan_num = rx_type ? rx_chan_num(chan_num) : tx_chan_num(chan_num);
>>>
>>> if (__chan_linear(chan_num) >= ctlr->num_chan)
>>> - return NULL;
>>> + return ERR_PTR(-EINVAL);
>>>
>>> chan = devm_kzalloc(ctlr->dev, sizeof(*chan), GFP_KERNEL);
>>> if (!chan)
>>> diff --git a/drivers/net/ethernet/ti/davinci_emac.c b/drivers/net/ethernet/ti/davinci_emac.c
>>> index f58c0c6..3d4af64 100644
>>> --- a/drivers/net/ethernet/ti/davinci_emac.c
>>> +++ b/drivers/net/ethernet/ti/davinci_emac.c
>>> @@ -1870,10 +1870,15 @@ static int davinci_emac_probe(struct platform_device *pdev)
>>>
>>> priv->txchan = cpdma_chan_create(priv->dma, EMAC_DEF_TX_CH,
>>> emac_tx_handler, 0);
>>> + if (WARN_ON(IS_ERR(priv->txchan))) {
>>
>> So, logically WARN_ON() should be removed in davinci_emac.c also. Right?
> It doesn't have dev_err() duplicate, so not very.
> But would be better to replace them on dev_err() if no objection.
>
right.
>
>>
>>> + rc = PTR_ERR(priv->txchan);
>>> + goto no_cpdma_chan;
>>> + }
>>> +
>>> priv->rxchan = cpdma_chan_create(priv->dma, EMAC_DEF_RX_CH,
>>> emac_rx_handler, 1);
>>> - if (WARN_ON(!priv->txchan || !priv->rxchan)) {
>>> - rc = -ENOMEM;
>>> + if (WARN_ON(IS_ERR(priv->rxchan))) {
>>> + rc = PTR_ERR(priv->rxchan);
>>> goto no_cpdma_chan;
>>> }
--
regards,
-grygorii
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-12-12 18:16 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-12-12 16:35 [PATCH v2 net-next] net: ethernet: ti: cpdma: correct error handling for chan create Ivan Khoronzhuk
2017-12-12 17:08 ` Grygorii Strashko
2017-12-12 17:50 ` Ivan Khoronzhuk
2017-12-12 18:16 ` Grygorii Strashko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).