[PATCH] net: lantiq_etop: check the result of request_irq()

[PATCH] net: lantiq_etop: check the result of request_irq()

[Masahiro Yamada]

The declaration of request_irq() in <linux/interrupt.h> is marked as
__must_check.

Without the return value check, I see the following warnings:

drivers/net/ethernet/lantiq_etop.c: In function 'ltq_etop_hw_init':
drivers/net/ethernet/lantiq_etop.c:273:4: warning: ignoring return value of 'request_irq', declared with attribute warn_unused_result [-Wunused-result]
  273 |    request_irq(irq, ltq_etop_dma_irq, 0, "etop_tx", priv);
      |    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/net/ethernet/lantiq_etop.c:281:4: warning: ignoring return value of 'request_irq', declared with attribute warn_unused_result [-Wunused-result]
  281 |    request_irq(irq, ltq_etop_dma_irq, 0, "etop_rx", priv);
      |    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Reported-by: Miguel Ojeda <ojeda@kernel.org>
Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---

 drivers/net/ethernet/lantiq_etop.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/lantiq_etop.c b/drivers/net/ethernet/lantiq_etop.c
index 2d0c52f7106b..960494f9752b 100644
--- a/drivers/net/ethernet/lantiq_etop.c
+++ b/drivers/net/ethernet/lantiq_etop.c
@@ -264,13 +264,18 @@ ltq_etop_hw_init(struct net_device *dev)
 	for (i = 0; i < MAX_DMA_CHAN; i++) {
 		int irq = LTQ_DMA_CH0_INT + i;
 		struct ltq_etop_chan *ch = &priv->ch[i];
+		int ret;
 
 		ch->idx = ch->dma.nr = i;
 		ch->dma.dev = &priv->pdev->dev;
 
 		if (IS_TX(i)) {
 			ltq_dma_alloc_tx(&ch->dma);
-			request_irq(irq, ltq_etop_dma_irq, 0, "etop_tx", priv);
+			ret = request_irq(irq, ltq_etop_dma_irq, 0, "etop_tx", priv);
+			if (ret) {
+				netdev_err(dev, "failed to request irq\n");
+				return ret;
+			}
 		} else if (IS_RX(i)) {
 			ltq_dma_alloc_rx(&ch->dma);
 			for (ch->dma.desc = 0; ch->dma.desc < LTQ_DESC_NUM;
@@ -278,7 +283,11 @@ ltq_etop_hw_init(struct net_device *dev)
 				if (ltq_etop_alloc_skb(ch))
 					return -ENOMEM;
 			ch->dma.desc = 0;
-			request_irq(irq, ltq_etop_dma_irq, 0, "etop_rx", priv);
+			ret = request_irq(irq, ltq_etop_dma_irq, 0, "etop_rx", priv);
+			if (ret) {
+				netdev_err(dev, "failed to request irq\n");
+				return ret;
+			}
 		}
 		ch->dma.irq = irq;
 	}
-- 
2.27.0

Re: [PATCH] net: lantiq_etop: check the result of request_irq()

[Andrew Lunn]

On Mon, Dec 21, 2020 at 02:43:23PM +0900, Masahiro Yamada wrote:
> The declaration of request_irq() in <linux/interrupt.h> is marked as
> __must_check.
> 
> Without the return value check, I see the following warnings:
> 
> drivers/net/ethernet/lantiq_etop.c: In function 'ltq_etop_hw_init':
> drivers/net/ethernet/lantiq_etop.c:273:4: warning: ignoring return value of 'request_irq', declared with attribute warn_unused_result [-Wunused-result]
>   273 |    request_irq(irq, ltq_etop_dma_irq, 0, "etop_tx", priv);
>       |    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/net/ethernet/lantiq_etop.c:281:4: warning: ignoring return value of 'request_irq', declared with attribute warn_unused_result [-Wunused-result]
>   281 |    request_irq(irq, ltq_etop_dma_irq, 0, "etop_rx", priv);
>       |    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> Reported-by: Miguel Ojeda <ojeda@kernel.org>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---
> 
>  drivers/net/ethernet/lantiq_etop.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/lantiq_etop.c b/drivers/net/ethernet/lantiq_etop.c
> index 2d0c52f7106b..960494f9752b 100644
> --- a/drivers/net/ethernet/lantiq_etop.c
> +++ b/drivers/net/ethernet/lantiq_etop.c
> @@ -264,13 +264,18 @@ ltq_etop_hw_init(struct net_device *dev)
>  	for (i = 0; i < MAX_DMA_CHAN; i++) {
>  		int irq = LTQ_DMA_CH0_INT + i;
>  		struct ltq_etop_chan *ch = &priv->ch[i];
> +		int ret;
>  
>  		ch->idx = ch->dma.nr = i;
>  		ch->dma.dev = &priv->pdev->dev;
>  
>  		if (IS_TX(i)) {
>  			ltq_dma_alloc_tx(&ch->dma);
> -			request_irq(irq, ltq_etop_dma_irq, 0, "etop_tx", priv);
> +			ret = request_irq(irq, ltq_etop_dma_irq, 0, "etop_tx", priv);
> +			if (ret) {
> +				netdev_err(dev, "failed to request irq\n");
> +				return ret;

You need to cleanup what ltq_dma_alloc_tx() did.

> +			}
>  		} else if (IS_RX(i)) {
>  			ltq_dma_alloc_rx(&ch->dma);
>  			for (ch->dma.desc = 0; ch->dma.desc < LTQ_DESC_NUM;
> @@ -278,7 +283,11 @@ ltq_etop_hw_init(struct net_device *dev)
>  				if (ltq_etop_alloc_skb(ch))
>  					return -ENOMEM;
>  			ch->dma.desc = 0;
> -			request_irq(irq, ltq_etop_dma_irq, 0, "etop_rx", priv);
> +			ret = request_irq(irq, ltq_etop_dma_irq, 0, "etop_rx", priv);
> +			if (ret) {
> +				netdev_err(dev, "failed to request irq\n");
> +				return ret;

And here you need to cleanup ltq_dma_alloc_rx().

    Andrew

Re: [PATCH] net: lantiq_etop: check the result of request_irq()

[Masahiro Yamada]

On Tue, Dec 22, 2020 at 12:26 AM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Mon, Dec 21, 2020 at 02:43:23PM +0900, Masahiro Yamada wrote:
> > The declaration of request_irq() in <linux/interrupt.h> is marked as
> > __must_check.
> >
> > Without the return value check, I see the following warnings:
> >
> > drivers/net/ethernet/lantiq_etop.c: In function 'ltq_etop_hw_init':
> > drivers/net/ethernet/lantiq_etop.c:273:4: warning: ignoring return value of 'request_irq', declared with attribute warn_unused_result [-Wunused-result]
> >   273 |    request_irq(irq, ltq_etop_dma_irq, 0, "etop_tx", priv);
> >       |    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > drivers/net/ethernet/lantiq_etop.c:281:4: warning: ignoring return value of 'request_irq', declared with attribute warn_unused_result [-Wunused-result]
> >   281 |    request_irq(irq, ltq_etop_dma_irq, 0, "etop_rx", priv);
> >       |    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >
> > Reported-by: Miguel Ojeda <ojeda@kernel.org>
> > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> > ---
> >
> >  drivers/net/ethernet/lantiq_etop.c | 13 +++++++++++--
> >  1 file changed, 11 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/lantiq_etop.c b/drivers/net/ethernet/lantiq_etop.c
> > index 2d0c52f7106b..960494f9752b 100644
> > --- a/drivers/net/ethernet/lantiq_etop.c
> > +++ b/drivers/net/ethernet/lantiq_etop.c
> > @@ -264,13 +264,18 @@ ltq_etop_hw_init(struct net_device *dev)
> >       for (i = 0; i < MAX_DMA_CHAN; i++) {
> >               int irq = LTQ_DMA_CH0_INT + i;
> >               struct ltq_etop_chan *ch = &priv->ch[i];
> > +             int ret;
> >
> >               ch->idx = ch->dma.nr = i;
> >               ch->dma.dev = &priv->pdev->dev;
> >
> >               if (IS_TX(i)) {
> >                       ltq_dma_alloc_tx(&ch->dma);
> > -                     request_irq(irq, ltq_etop_dma_irq, 0, "etop_tx", priv);
> > +                     ret = request_irq(irq, ltq_etop_dma_irq, 0, "etop_tx", priv);
> > +                     if (ret) {
> > +                             netdev_err(dev, "failed to request irq\n");
> > +                             return ret;
>
> You need to cleanup what ltq_dma_alloc_tx() did.


Any failure from this function will roll back
in the following paths:

  ltq_etop_hw_exit()
     -> ltq_etop_free_channel()
          -> ltq_dma_free()


So, dma is freed anyway.

One problem I see is,
ltq_etop_hw_exit() frees all DMA channels,
some of which may not have been allocated yet.

If it is a bug, it is an existing bug.


>
> > +                     }
> >               } else if (IS_RX(i)) {
> >                       ltq_dma_alloc_rx(&ch->dma);
> >                       for (ch->dma.desc = 0; ch->dma.desc < LTQ_DESC_NUM;
> > @@ -278,7 +283,11 @@ ltq_etop_hw_init(struct net_device *dev)
> >                               if (ltq_etop_alloc_skb(ch))
> >                                       return -ENOMEM;


This -ENOMEM does not roll back anything here.

As stated above, dma_free_coherent() is called.
The problem is, ltq_etop_hw_exit() rolls back too much.

If your requirement is "this driver is completely wrong. Please rewrite it",
sorry, I cannot (unless I am paid to do so).

I am just following this driver's roll-back model.

Please do not expect more to a person who
volunteers to eliminate build warnings.

Of course, if somebody volunteers to rewrite this driver correctly,
that is appreciated.



> >                       ch->dma.desc = 0;
> > -                     request_irq(irq, ltq_etop_dma_irq, 0, "etop_rx", priv);
> > +                     ret = request_irq(irq, ltq_etop_dma_irq, 0, "etop_rx", priv);
> > +                     if (ret) {
> > +                             netdev_err(dev, "failed to request irq\n");
> > +                             return ret;
>
> And here you need to cleanup ltq_dma_alloc_rx().
>
>     Andrew



-- 
Best Regards
Masahiro Yamada

Re: [PATCH] net: lantiq_etop: check the result of request_irq()

[Andrew Lunn]

On Tue, Dec 22, 2020 at 12:59:08AM +0900, Masahiro Yamada wrote:
> On Tue, Dec 22, 2020 at 12:26 AM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > On Mon, Dec 21, 2020 at 02:43:23PM +0900, Masahiro Yamada wrote:
> > > The declaration of request_irq() in <linux/interrupt.h> is marked as
> > > __must_check.
> > >
> > > Without the return value check, I see the following warnings:
> > >
> > > drivers/net/ethernet/lantiq_etop.c: In function 'ltq_etop_hw_init':
> > > drivers/net/ethernet/lantiq_etop.c:273:4: warning: ignoring return value of 'request_irq', declared with attribute warn_unused_result [-Wunused-result]
> > >   273 |    request_irq(irq, ltq_etop_dma_irq, 0, "etop_tx", priv);
> > >       |    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > drivers/net/ethernet/lantiq_etop.c:281:4: warning: ignoring return value of 'request_irq', declared with attribute warn_unused_result [-Wunused-result]
> > >   281 |    request_irq(irq, ltq_etop_dma_irq, 0, "etop_rx", priv);
> > >       |    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > >
> > > Reported-by: Miguel Ojeda <ojeda@kernel.org>
> > > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> > > ---
> > >
> > >  drivers/net/ethernet/lantiq_etop.c | 13 +++++++++++--
> > >  1 file changed, 11 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/lantiq_etop.c b/drivers/net/ethernet/lantiq_etop.c
> > > index 2d0c52f7106b..960494f9752b 100644
> > > --- a/drivers/net/ethernet/lantiq_etop.c
> > > +++ b/drivers/net/ethernet/lantiq_etop.c
> > > @@ -264,13 +264,18 @@ ltq_etop_hw_init(struct net_device *dev)
> > >       for (i = 0; i < MAX_DMA_CHAN; i++) {
> > >               int irq = LTQ_DMA_CH0_INT + i;
> > >               struct ltq_etop_chan *ch = &priv->ch[i];
> > > +             int ret;
> > >
> > >               ch->idx = ch->dma.nr = i;
> > >               ch->dma.dev = &priv->pdev->dev;
> > >
> > >               if (IS_TX(i)) {
> > >                       ltq_dma_alloc_tx(&ch->dma);
> > > -                     request_irq(irq, ltq_etop_dma_irq, 0, "etop_tx", priv);
> > > +                     ret = request_irq(irq, ltq_etop_dma_irq, 0, "etop_tx", priv);
> > > +                     if (ret) {
> > > +                             netdev_err(dev, "failed to request irq\n");
> > > +                             return ret;
> >
> > You need to cleanup what ltq_dma_alloc_tx() did.
> 
> 
> Any failure from this function will roll back
> in the following paths:
> 
>   ltq_etop_hw_exit()
>      -> ltq_etop_free_channel()
>           -> ltq_dma_free()
> 
> 
> So, dma is freed anyway.

O.K, thanks for the information. 

> One problem I see is,
> ltq_etop_hw_exit() frees all DMA channels,
> some of which may not have been allocated yet.
> 
> If it is a bug, it is an existing bug.
> 
> 
> >
> > > +                     }
> > >               } else if (IS_RX(i)) {
> > >                       ltq_dma_alloc_rx(&ch->dma);
> > >                       for (ch->dma.desc = 0; ch->dma.desc < LTQ_DESC_NUM;
> > > @@ -278,7 +283,11 @@ ltq_etop_hw_init(struct net_device *dev)
> > >                               if (ltq_etop_alloc_skb(ch))
> > >                                       return -ENOMEM;
> 
> 
> This -ENOMEM does not roll back anything here.
> 
> As stated above, dma_free_coherent() is called.
> The problem is, ltq_etop_hw_exit() rolls back too much.
> 
> If your requirement is "this driver is completely wrong. Please rewrite it",
> sorry, I cannot (unless I am paid to do so).
> 
> I am just following this driver's roll-back model.
> 
> Please do not expect more to a person who
> volunteers to eliminate build warnings.

There is a balance here. We should not remove a warning unless we
properly fix the warning. Otherwise having the warning is pointless.

So please leave the warning in place, and maybe somebody else will
fully fix it.

      Andrew

Re: [PATCH] net: lantiq_etop: check the result of request_irq()

[Miguel Ojeda]

On Mon, Dec 21, 2020 at 6:01 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> So please leave the warning in place, and maybe somebody else will
> fully fix it.

For context: the plan is to enable the warning unconditionally
starting with 5.11. After that, the idea is making it an error as soon
as reasonable (e.g. 5.12 if no warnings remain by then).

However, if there is nobody planning to fix a given warning, then I'd
say documenting the problem with a `FIXME` comment (plus a change like
this or simply ignoring the return value) would be the best approach.

Cheers,
Miguel

Re: [PATCH] net: lantiq_etop: check the result of request_irq()

[Andrew Lunn]

On Tue, Dec 22, 2020 at 12:59:08AM +0900, Masahiro Yamada wrote:
> On Tue, Dec 22, 2020 at 12:26 AM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > On Mon, Dec 21, 2020 at 02:43:23PM +0900, Masahiro Yamada wrote:
> > > The declaration of request_irq() in <linux/interrupt.h> is marked as
> > > __must_check.
> > >
> > > Without the return value check, I see the following warnings:
> > >
> > > drivers/net/ethernet/lantiq_etop.c: In function 'ltq_etop_hw_init':
> > > drivers/net/ethernet/lantiq_etop.c:273:4: warning: ignoring return value of 'request_irq', declared with attribute warn_unused_result [-Wunused-result]
> > >   273 |    request_irq(irq, ltq_etop_dma_irq, 0, "etop_tx", priv);
> > >       |    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > drivers/net/ethernet/lantiq_etop.c:281:4: warning: ignoring return value of 'request_irq', declared with attribute warn_unused_result [-Wunused-result]
> > >   281 |    request_irq(irq, ltq_etop_dma_irq, 0, "etop_rx", priv);
> > >       |    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > >
> > > Reported-by: Miguel Ojeda <ojeda@kernel.org>
> > > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> > > ---
> > >
> > >  drivers/net/ethernet/lantiq_etop.c | 13 +++++++++++--
> > >  1 file changed, 11 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/lantiq_etop.c b/drivers/net/ethernet/lantiq_etop.c
> > > index 2d0c52f7106b..960494f9752b 100644
> > > --- a/drivers/net/ethernet/lantiq_etop.c
> > > +++ b/drivers/net/ethernet/lantiq_etop.c
> > > @@ -264,13 +264,18 @@ ltq_etop_hw_init(struct net_device *dev)
> > >       for (i = 0; i < MAX_DMA_CHAN; i++) {
> > >               int irq = LTQ_DMA_CH0_INT + i;
> > >               struct ltq_etop_chan *ch = &priv->ch[i];
> > > +             int ret;
> > >
> > >               ch->idx = ch->dma.nr = i;
> > >               ch->dma.dev = &priv->pdev->dev;
> > >
> > >               if (IS_TX(i)) {
> > >                       ltq_dma_alloc_tx(&ch->dma);
> > > -                     request_irq(irq, ltq_etop_dma_irq, 0, "etop_tx", priv);
> > > +                     ret = request_irq(irq, ltq_etop_dma_irq, 0, "etop_tx", priv);
> > > +                     if (ret) {
> > > +                             netdev_err(dev, "failed to request irq\n");
> > > +                             return ret;
> >
> > You need to cleanup what ltq_dma_alloc_tx() did.
> 
> 
> Any failure from this function will roll back
> in the following paths:
> 
>   ltq_etop_hw_exit()
>      -> ltq_etop_free_channel()
>           -> ltq_dma_free()
> 
> 
> So, dma is freed anyway.
> 
> One problem I see is,
> ltq_etop_hw_exit() frees all DMA channels,
> some of which may not have been allocated yet.
> 
> If it is a bug, it is an existing bug.
> 
> 
> >
> > > +                     }
> > >               } else if (IS_RX(i)) {
> > >                       ltq_dma_alloc_rx(&ch->dma);
> > >                       for (ch->dma.desc = 0; ch->dma.desc < LTQ_DESC_NUM;
> > > @@ -278,7 +283,11 @@ ltq_etop_hw_init(struct net_device *dev)
> > >                               if (ltq_etop_alloc_skb(ch))
> > >                                       return -ENOMEM;
> 
> 
> This -ENOMEM does not roll back anything here.
> 
> As stated above, dma_free_coherent() is called.
> The problem is, ltq_etop_hw_exit() rolls back too much.
> 
> If your requirement is "this driver is completely wrong. Please rewrite it",
> sorry, I cannot (unless I am paid to do so).
> 
> I am just following this driver's roll-back model.
> 
> Please do not expect more to a person who
> volunteers to eliminate build warnings.
> 
> Of course, if somebody volunteers to rewrite this driver correctly,
> that is appreciated.

Hi Hauke

Do you still have this hardware? Do you have time to take a look at
the cleanup code?

Thanks
	Andrew

Re: [PATCH] net: lantiq_etop: check the result of request_irq()

[Hauke Mehrtens]

On 12/21/20 7:04 PM, Andrew Lunn wrote:
> On Tue, Dec 22, 2020 at 12:59:08AM +0900, Masahiro Yamada wrote:
>> On Tue, Dec 22, 2020 at 12:26 AM Andrew Lunn <andrew@lunn.ch> wrote:
>>>
>>> On Mon, Dec 21, 2020 at 02:43:23PM +0900, Masahiro Yamada wrote:
>>>> The declaration of request_irq() in <linux/interrupt.h> is marked as
>>>> __must_check.
>>>>
>>>> Without the return value check, I see the following warnings:
>>>>
>>>> drivers/net/ethernet/lantiq_etop.c: In function 'ltq_etop_hw_init':
>>>> drivers/net/ethernet/lantiq_etop.c:273:4: warning: ignoring return value of 'request_irq', declared with attribute warn_unused_result [-Wunused-result]
>>>>    273 |    request_irq(irq, ltq_etop_dma_irq, 0, "etop_tx", priv);
>>>>        |    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>> drivers/net/ethernet/lantiq_etop.c:281:4: warning: ignoring return value of 'request_irq', declared with attribute warn_unused_result [-Wunused-result]
>>>>    281 |    request_irq(irq, ltq_etop_dma_irq, 0, "etop_rx", priv);
>>>>        |    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>
>>>> Reported-by: Miguel Ojeda <ojeda@kernel.org>
>>>> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
>>>> ---
>>>>
>>>>   drivers/net/ethernet/lantiq_etop.c | 13 +++++++++++--
>>>>   1 file changed, 11 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/lantiq_etop.c b/drivers/net/ethernet/lantiq_etop.c
>>>> index 2d0c52f7106b..960494f9752b 100644
>>>> --- a/drivers/net/ethernet/lantiq_etop.c
>>>> +++ b/drivers/net/ethernet/lantiq_etop.c
>>>> @@ -264,13 +264,18 @@ ltq_etop_hw_init(struct net_device *dev)
>>>>        for (i = 0; i < MAX_DMA_CHAN; i++) {
>>>>                int irq = LTQ_DMA_CH0_INT + i;
>>>>                struct ltq_etop_chan *ch = &priv->ch[i];
>>>> +             int ret;
>>>>
>>>>                ch->idx = ch->dma.nr = i;
>>>>                ch->dma.dev = &priv->pdev->dev;
>>>>
>>>>                if (IS_TX(i)) {
>>>>                        ltq_dma_alloc_tx(&ch->dma);
>>>> -                     request_irq(irq, ltq_etop_dma_irq, 0, "etop_tx", priv);
>>>> +                     ret = request_irq(irq, ltq_etop_dma_irq, 0, "etop_tx", priv);
>>>> +                     if (ret) {
>>>> +                             netdev_err(dev, "failed to request irq\n");
>>>> +                             return ret;
>>>
>>> You need to cleanup what ltq_dma_alloc_tx() did.
>>
>>
>> Any failure from this function will roll back
>> in the following paths:
>>
>>    ltq_etop_hw_exit()
>>       -> ltq_etop_free_channel()
>>            -> ltq_dma_free()
>>
>>
>> So, dma is freed anyway.
>>
>> One problem I see is,
>> ltq_etop_hw_exit() frees all DMA channels,
>> some of which may not have been allocated yet.
>>
>> If it is a bug, it is an existing bug.
>>
>>
>>>
>>>> +                     }
>>>>                } else if (IS_RX(i)) {
>>>>                        ltq_dma_alloc_rx(&ch->dma);
>>>>                        for (ch->dma.desc = 0; ch->dma.desc < LTQ_DESC_NUM;
>>>> @@ -278,7 +283,11 @@ ltq_etop_hw_init(struct net_device *dev)
>>>>                                if (ltq_etop_alloc_skb(ch))
>>>>                                        return -ENOMEM;
>>
>>
>> This -ENOMEM does not roll back anything here.
>>
>> As stated above, dma_free_coherent() is called.
>> The problem is, ltq_etop_hw_exit() rolls back too much.
>>
>> If your requirement is "this driver is completely wrong. Please rewrite it",
>> sorry, I cannot (unless I am paid to do so).
>>
>> I am just following this driver's roll-back model.
>>
>> Please do not expect more to a person who
>> volunteers to eliminate build warnings.
>>
>> Of course, if somebody volunteers to rewrite this driver correctly,
>> that is appreciated.
> 
> Hi Hauke
> 
> Do you still have this hardware? Do you have time to take a look at
> the cleanup code?
> 
> Thanks
> 	Andrew
> 

Hi Andrew,

I have this hardware somewhere at home, but I never made it work.
If I find some time I can have a loom at this problem in the next few weeks.

Hauke