Re: [PATCH] net: davinci_emac: Replace devm_request_irq with request_irq
Hi,
--On March 06, 2014 16:57 -0500 David Miller da...@davemloft.net wrote:
From: Christian Riesch christian.rie...@omicron.at
Date: Wed, 5 Mar 2014 11:25:28 +0100
@@ -1641,7 +1640,15 @@ static int emac_dev_open(struct net_device *ndev)
rollback:
- dev_err(emac_dev, DaVinci EMAC: devm_request_irq() failed);
+ dev_err(emac_dev, DaVinci EMAC: request_irq() failed);
+
+ for (q = k; k = 0; k--) {
+ for (m = i; m = res-start; m--)
+ free_irq(m, ndev);
+ res = platform_get_resource(priv-pdev, IORESOURCE_IRQ, k-1);
+ m = res-end;
So this should definitely read i = res-end;.
+ }
This final assignment in the loop of 'm' is unused?
The inner for() loop always started with m = i;
But it is more than just that. The entire rollback mechanism of
emac_dev_open is a mess. It just won't work.
1) Freeing the interrupts. Currently, the code tries to do a
platform_get_resource(priv-pdev, IORESOURCE_IRQ, -1) in its last
iteration. To make this work, the code should be something like
for (q = k; q = 0; q--) {
res = platform_get_resource(priv-pdev, IORESOURCE_IRQ, k-1);
if (q != k)
i = res-end;
for (m = i; m = res-start; m--)
free_irq(m, ndev);
}
2) Wrong order of err: and rollback: labels. Currently the function does
something like this:
pm_runtime_get
request irq
if requesting irqs fails, goto rollback
setup phy
if phy setup fails, goto err
return 0
rollback:
free irqs
err:
pm_runtime_put
So if PHY initialization fails, the IRQs are never freed. So rollback and
err must be exchanged, and some additonal code must be added to keep the
correct values of k and i for rollback.
3) RX DMA descriptors are not freed in case of an error: Right before
requesting the irqs, the function creates DMA descriptors for the RX
channel. These RX descriptors are never freed when we jump to either
rollback or err. And I think there is also no way to free them as long as
cpdma_ctlr_start() has not been called (which is done between requesting
irqs and phy setup in the code).
Problems 1+2 are easy to solve. Prabhakar, Mugunthan, any ideas for 3? I
think cpsw_ndo_start() in cpsw.c has the same problem. It will also try to
call cpdma_ctlr_stop in error handling before cpdma_ctlr_start was called
in some cases.
Regards, Christian
___
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH] net: davinci_emac: Replace devm_request_irq with request_irq
On Wednesday 05 March 2014 03:55 PM, Christian Riesch wrote: In commit 6892b41d9701283085b655c6086fb57a5d63fa47 Author: Lad, Prabhakar prabhakar.cse...@gmail.com Date: Tue Jun 25 21:24:51 2013 +0530 net: davinci: emac: Convert to devm_* api the call of request_irq is replaced by devm_request_irq and the call of free_irq is removed. But since interrupts are requested in emac_dev_open, doing ifconfig up/down on the board requests the interrupts again each time, causing devm_request_irq to fail. This patch reverts said commit partially: It changes the driver back to use request_irq instead of devm_request_irq, puts free_irq back in place, but keeps the remaining changes of the original patch. Reported-by: Jon Ringle j...@ringle.org Signed-off-by: Christian Riesch christian.rie...@omicron.at Cc: Lad, Prabhakar prabhakar.cse...@gmail.com Cc: sta...@vger.kernel.org Instead of moving back to request irq. can you move the devm interrupt request code from open to probe so that the issue is fixed, IRQ will be requested on module insert and IRQ will be free in module remove and there will not be any failures in devm request irq while repeating ifup/ifdown and also during module insert/remove. Regards Mugunthan V N ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH] net: davinci_emac: Replace devm_request_irq with request_irq
[Sent again, sorry for the HTML mail before.]
--On March 06, 2014 16:57 -0500 David Miller da...@davemloft.net wrote:
From: Christian Riesch christian.rie...@omicron.at
Date: Wed, 5 Mar 2014 11:25:28 +0100
@@ -1641,7 +1640,15 @@ static int emac_dev_open(struct net_device *ndev)
rollback:
- dev_err(emac_dev, DaVinci EMAC: devm_request_irq() failed);
+ dev_err(emac_dev, DaVinci EMAC: request_irq() failed);
+
+ for (q = k; k = 0; k--) {
+ for (m = i; m = res-start; m--)
+ free_irq(m, ndev);
+ res = platform_get_resource(priv-pdev, IORESOURCE_IRQ, k-1);
+ m = res-end;
+ }
This final assignment in the loop of 'm' is unused?
The inner for() loop always started with m = i;
This should probably read
i = res-end;
I just took the old code without thinking about it. Thanks for catching it.
Christian
___
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH] net: davinci_emac: Replace devm_request_irq with request_irq
Hi Christian, Thanks for the patch. On Wed, Mar 5, 2014 at 3:55 PM, Christian Riesch christian.rie...@omicron.at wrote: In commit 6892b41d9701283085b655c6086fb57a5d63fa47 Author: Lad, Prabhakar prabhakar.cse...@gmail.com Date: Tue Jun 25 21:24:51 2013 +0530 net: davinci: emac: Convert to devm_* api the call of request_irq is replaced by devm_request_irq and the call of free_irq is removed. But since interrupts are requested in emac_dev_open, doing ifconfig up/down on the board requests the interrupts again each time, causing devm_request_irq to fail. This patch reverts said commit partially: It changes the driver back to use request_irq instead of devm_request_irq, puts free_irq back in place, but keeps the remaining changes of the original patch. Reported-by: Jon Ringle j...@ringle.org Signed-off-by: Christian Riesch christian.rie...@omicron.at Cc: Lad, Prabhakar prabhakar.cse...@gmail.com Cc: sta...@vger.kernel.org Acked-by: Lad, Prabhakar prabhakar.cse...@gmail.com Regards, --Prabhakar lad ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
