* [PATCH] net: lantiq_etop: check the result of request_irq() @ 2020-12-21 5:43 Masahiro Yamada 2020-12-21 15:26 ` Andrew Lunn 0 siblings, 1 reply; 7+ messages in thread From: Masahiro Yamada @ 2020-12-21 5:43 UTC (permalink / raw) To: David S . Miller, Jakub Kicinski, netdev Cc: linux-kernel, Masahiro Yamada, Miguel Ojeda, Andrew Lunn, Arnd Bergmann, Heiner Kallweit 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] net: lantiq_etop: check the result of request_irq() 2020-12-21 5:43 [PATCH] net: lantiq_etop: check the result of request_irq() Masahiro Yamada @ 2020-12-21 15:26 ` Andrew Lunn 2020-12-21 15:59 ` Masahiro Yamada 0 siblings, 1 reply; 7+ messages in thread From: Andrew Lunn @ 2020-12-21 15:26 UTC (permalink / raw) To: Masahiro Yamada Cc: David S . Miller, Jakub Kicinski, netdev, linux-kernel, Miguel Ojeda, Arnd Bergmann, Heiner Kallweit 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] net: lantiq_etop: check the result of request_irq() 2020-12-21 15:26 ` Andrew Lunn @ 2020-12-21 15:59 ` Masahiro Yamada 2020-12-21 17:01 ` Andrew Lunn 2020-12-21 18:04 ` Andrew Lunn 0 siblings, 2 replies; 7+ messages in thread From: Masahiro Yamada @ 2020-12-21 15:59 UTC (permalink / raw) To: Andrew Lunn Cc: David S . Miller, Jakub Kicinski, Networking, Linux Kernel Mailing List, Miguel Ojeda, Arnd Bergmann, Heiner Kallweit 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] net: lantiq_etop: check the result of request_irq() 2020-12-21 15:59 ` Masahiro Yamada @ 2020-12-21 17:01 ` Andrew Lunn 2020-12-21 17:38 ` Miguel Ojeda 2020-12-21 18:04 ` Andrew Lunn 1 sibling, 1 reply; 7+ messages in thread From: Andrew Lunn @ 2020-12-21 17:01 UTC (permalink / raw) To: Masahiro Yamada Cc: David S . Miller, Jakub Kicinski, Networking, Linux Kernel Mailing List, Miguel Ojeda, Arnd Bergmann, Heiner Kallweit 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] net: lantiq_etop: check the result of request_irq() 2020-12-21 17:01 ` Andrew Lunn @ 2020-12-21 17:38 ` Miguel Ojeda 0 siblings, 0 replies; 7+ messages in thread From: Miguel Ojeda @ 2020-12-21 17:38 UTC (permalink / raw) To: Andrew Lunn Cc: Masahiro Yamada, David S . Miller, Jakub Kicinski, Networking, Linux Kernel Mailing List, Miguel Ojeda, Arnd Bergmann, Heiner Kallweit 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] net: lantiq_etop: check the result of request_irq() 2020-12-21 15:59 ` Masahiro Yamada 2020-12-21 17:01 ` Andrew Lunn @ 2020-12-21 18:04 ` Andrew Lunn 2020-12-21 23:41 ` Hauke Mehrtens 1 sibling, 1 reply; 7+ messages in thread From: Andrew Lunn @ 2020-12-21 18:04 UTC (permalink / raw) To: Masahiro Yamada, Hauke Mehrtens Cc: David S . Miller, Jakub Kicinski, Networking, Linux Kernel Mailing List, Miguel Ojeda, Arnd Bergmann, Heiner Kallweit 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] net: lantiq_etop: check the result of request_irq() 2020-12-21 18:04 ` Andrew Lunn @ 2020-12-21 23:41 ` Hauke Mehrtens 0 siblings, 0 replies; 7+ messages in thread From: Hauke Mehrtens @ 2020-12-21 23:41 UTC (permalink / raw) To: Andrew Lunn, Masahiro Yamada Cc: David S . Miller, Jakub Kicinski, Networking, Linux Kernel Mailing List, Miguel Ojeda, Arnd Bergmann, Heiner Kallweit 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-12-21 23:42 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-12-21 5:43 [PATCH] net: lantiq_etop: check the result of request_irq() Masahiro Yamada 2020-12-21 15:26 ` Andrew Lunn 2020-12-21 15:59 ` Masahiro Yamada 2020-12-21 17:01 ` Andrew Lunn 2020-12-21 17:38 ` Miguel Ojeda 2020-12-21 18:04 ` Andrew Lunn 2020-12-21 23:41 ` Hauke Mehrtens
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).