[PATCH net v2 0/9] net: ethernet: mediatek: a couple of fixes

STALE3642d

9 messages, 3 authors, 2016-08-29 · open the first message on its own page

[PATCH net v2 0/9] net: ethernet: mediatek: a couple of fixes

From: <hidden>
Date: 2016-08-29 05:03:14

From: Sean Wang <sean.wang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>

a couple of fixes come out from integrating with linux-4.8 rc1
they all are verified and workable on linux-4.8 rc1

changes since v1:
- usage of loops to work out if all required clock are ready instead
of tedious coding
- remove redundant pinctrl setup that is already done by core driver
thanks for careful and patient reviewing by Andrew Lunn
- splitting distinct changes into the separate patches
- change variable naming from err to ret for readable coding

Sean Wang (9):
  net: ethernet: mediatek: fix fails from TX housekeeping due to
    incorrect port setup
  net: ethernet: mediatek: fix incorrect return value of devm_clk_get
    with EPROBE_DEFER
  net: ethernet: mediatek: fix API usage with skb_free_frag
  net: ethernet: mediatek: remove redundant free_irq for
    devm_request_irq allocated irq
  net: ethernet: mediatek: fix logic unbalance between probe and remove
  net: ethernet: mediatek: fix issue of driver removal with interface is
    up
  net: ethernet: mediatek: fix the missing of_node_put() after node is
    used done inside mtk_mdio_init
  net: ethernet: mediatek: use devm_mdiobus_alloc instead of
    mdiobus_alloc inside mtk_mdio_init
  net: ethernet: mediatek: fix error handling inside mtk_mdio_init

 drivers/net/ethernet/mediatek/mtk_eth_soc.c | 82 +++++++++++++++--------------
 drivers/net/ethernet/mediatek/mtk_eth_soc.h | 22 +++++---
 2 files changed, 56 insertions(+), 48 deletions(-)

-- 
1.9.1

[PATCH net v2 7/9] net: ethernet: mediatek: fix the missing of_node_put() after node is used done inside mtk_mdio_init

From: <sean.wang@mediatek.com>
Date: 2016-08-29 05:04:46

From: Sean Wang <sean.wang@mediatek.com>

This patch adds the missing of_node_put() after finishing the usage
of of_get_child_by_name.

Signed-off-by: Sean Wang <sean.wang@mediatek.com>
Acked-by: John Crispin <john@phrozen.org>
---
 drivers/net/ethernet/mediatek/mtk_eth_soc.c | 1 +
 1 file changed, 1 insertion(+)
diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
index 1001317..85a527a 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
@@ -324,6 +324,7 @@ static int mtk_mdio_init(struct mtk_eth *eth)
 	err = of_mdiobus_register(eth->mii_bus, mii_np);
 	if (err)
 		goto err_free_bus;
+	of_node_put(mii_np);
 
 	return 0;
 
-- 
1.9.1

[PATCH net v2 3/9] net: ethernet: mediatek: fix API usage with skb_free_frag

From: <sean.wang@mediatek.com>
Date: 2016-08-29 05:04:46

From: Sean Wang <sean.wang@mediatek.com>

use skb_free_frag() instead of legacy put_page()

Signed-off-by: Sean Wang <sean.wang@mediatek.com>
Acked-by: John Crispin <john@phrozen.org>
---
 drivers/net/ethernet/mediatek/mtk_eth_soc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
index ad4865c..518d987 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
@@ -868,7 +868,7 @@ static int mtk_poll_rx(struct napi_struct *napi, int budget,
 		/* receive data */
 		skb = build_skb(data, ring->frag_size);
 		if (unlikely(!skb)) {
-			put_page(virt_to_head_page(new_data));
+			skb_free_frag(new_data);
 			netdev->stats.rx_dropped++;
 			goto release_desc;
 		}
-- 
1.9.1

[PATCH net v2 9/9] net: ethernet: mediatek: fix error handling inside mtk_mdio_init

From: <sean.wang@mediatek.com>
Date: 2016-08-29 05:04:47

From: Sean Wang <sean.wang@mediatek.com>

return -ENODEV if no child is found in MDIO bus.

Signed-off-by: Sean Wang <sean.wang@mediatek.com>
Acked-by: John Crispin <john@phrozen.org>
---
 drivers/net/ethernet/mediatek/mtk_eth_soc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
index f741c6a..e48b2a4 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
@@ -304,7 +304,7 @@ static int mtk_mdio_init(struct mtk_eth *eth)
 	}
 
 	if (!of_device_is_available(mii_np)) {
-		ret = 0;
+		ret = -ENODEV;
 		goto err_put_node;
 	}
 
-- 
1.9.1

[PATCH net v2 8/9] net: ethernet: mediatek: use devm_mdiobus_alloc instead of mdiobus_alloc inside mtk_mdio_init

From: <sean.wang@mediatek.com>
Date: 2016-08-29 05:04:47

From: Sean Wang <sean.wang@mediatek.com>

a lot of parts in the driver uses devm_* APIs to gain benefits from the
device resource management, so devm_mdiobus_alloc is also used instead
of mdiobus_alloc to have more elegant code flow.

Using common code provided by the devm_* helps to
1) have simplified the code flow as [1] says
2) decrease the risk of incorrect error handling by human
3) only a few drivers used it since it ware proposed on linux 3.16,
so just hope to promote for this.

Signed-off-by: Sean Wang <sean.wang@mediatek.com>

---
ref.
[1] https://patchwork.ozlabs.org/patch/344093/
---
 drivers/net/ethernet/mediatek/mtk_eth_soc.c | 23 ++++++-----------------
 1 file changed, 6 insertions(+), 17 deletions(-)
diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
index 85a527a..f741c6a 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
@@ -295,7 +295,7 @@ err_phy:
 static int mtk_mdio_init(struct mtk_eth *eth)
 {
 	struct device_node *mii_np;
-	int err;
+	int ret;
 
 	mii_np = of_get_child_by_name(eth->dev->of_node, "mdio-bus");
 	if (!mii_np) {
@@ -304,13 +304,13 @@ static int mtk_mdio_init(struct mtk_eth *eth)
 	}
 
 	if (!of_device_is_available(mii_np)) {
-		err = 0;
+		ret = 0;
 		goto err_put_node;
 	}
 
-	eth->mii_bus = mdiobus_alloc();
+	eth->mii_bus = devm_mdiobus_alloc(eth->dev);
 	if (!eth->mii_bus) {
-		err = -ENOMEM;
+		ret = -ENOMEM;
 		goto err_put_node;
 	}
 
@@ -321,20 +321,11 @@ static int mtk_mdio_init(struct mtk_eth *eth)
 	eth->mii_bus->parent = eth->dev;
 
 	snprintf(eth->mii_bus->id, MII_BUS_ID_SIZE, "%s", mii_np->name);
-	err = of_mdiobus_register(eth->mii_bus, mii_np);
-	if (err)
-		goto err_free_bus;
-	of_node_put(mii_np);
-
-	return 0;
-
-err_free_bus:
-	mdiobus_free(eth->mii_bus);
+	ret = of_mdiobus_register(eth->mii_bus, mii_np);
 
 err_put_node:
 	of_node_put(mii_np);
-	eth->mii_bus = NULL;
-	return err;
+	return ret;
 }
 
 static void mtk_mdio_cleanup(struct mtk_eth *eth)
@@ -343,8 +334,6 @@ static void mtk_mdio_cleanup(struct mtk_eth *eth)
 		return;
 
 	mdiobus_unregister(eth->mii_bus);
-	of_node_put(eth->mii_bus->dev.of_node);
-	mdiobus_free(eth->mii_bus);
 }
 
 static inline void mtk_irq_disable(struct mtk_eth *eth, u32 mask)
-- 
1.9.1

Re: [PATCH net v2 8/9] net: ethernet: mediatek: use devm_mdiobus_alloc instead of mdiobus_alloc inside mtk_mdio_init

From: Andrew Lunn <andrew@lunn.ch>
Date: 2016-08-29 13:06:39

On Mon, Aug 29, 2016 at 01:03:22PM +0800, sean.wang@mediatek.com wrote:
From: Sean Wang <sean.wang@mediatek.com>

a lot of parts in the driver uses devm_* APIs to gain benefits from the
device resource management, so devm_mdiobus_alloc is also used instead
of mdiobus_alloc to have more elegant code flow.

Using common code provided by the devm_* helps to
1) have simplified the code flow as [1] says
2) decrease the risk of incorrect error handling by human
3) only a few drivers used it since it ware proposed on linux 3.16,
so just hope to promote for this.

Signed-off-by: Sean Wang <sean.wang@mediatek.com>

---
ref.
[1] https://patchwork.ozlabs.org/patch/344093/
---
Hi Sean

Your footnote is below the ---, so won't make it into the change log. 

Apart from that,

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net v2 9/9] net: ethernet: mediatek: fix error handling inside mtk_mdio_init

From: Andrew Lunn <andrew@lunn.ch>
Date: 2016-08-29 13:16:05

On Mon, Aug 29, 2016 at 01:03:23PM +0800, sean.wang@mediatek.com wrote:
From: Sean Wang <sean.wang@mediatek.com>

return -ENODEV if no child is found in MDIO bus.
Hi Sean

Why is it an error not to have any children on the bus?

Say i have a fibre optical module connected to the MAC. It is unlikely
to have an MII interface, so i would not list it on the bus. With this
change, if i have the mdio-bus node in my device tree, i don't get a
working system. Without this change, it simply does not instantiate
the MDIO device, and returns without an error.

I think this patch should be dropped, or maybe a comment adding, why
the current code returns 0 at this point.

    Andrew
quoted hunk
Signed-off-by: Sean Wang <sean.wang@mediatek.com>
Acked-by: John Crispin <john@phrozen.org>
---
 drivers/net/ethernet/mediatek/mtk_eth_soc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
index f741c6a..e48b2a4 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
@@ -304,7 +304,7 @@ static int mtk_mdio_init(struct mtk_eth *eth)
 	}
 
 	if (!of_device_is_available(mii_np)) {
-		ret = 0;
+		ret = -ENODEV;
 		goto err_put_node;
 	}
 
-- 
1.9.1

Re: [PATCH net v2 9/9] net: ethernet: mediatek: fix error handling inside mtk_mdio_init

From: Sean Wang <sean.wang@mediatek.com>
Date: 2016-08-29 16:10:09

Date: Mon, 29 Aug 2016 15:15:58 +0200,Andrew Lunn wrote:
On Mon, Aug 29, 2016 at 01:03:23PM +0800, sean.wang@mediatek.com wrote:
quoted
From: Sean Wang <sean.wang@mediatek.com>

return -ENODEV if no child is found in MDIO bus.
Hi Sean

Why is it an error not to have any children on the bus?

Say i have a fibre optical module connected to the MAC. It is unlikely
to have an MII interface, so i would not list it on the bus. With this
change, if i have the mdio-bus node in my device tree, i don't get a
working system. Without this change, it simply does not instantiate
the MDIO device, and returns without an error.

I think this patch should be dropped, or maybe a comment adding, why
the current code returns 0 at this point.

   Andrew
Hi Andrew,

Sorry, i didn't add the comment enough and well on the patch for let you can't see 
what i am done

the patch I just want to let driver know if device tree is defined well at the earlier time

although original driver still works without this patch 

the original logic on the driver is 

1) returning -ENODEV if no mdio_bus defined in the device tree. 

2) returning -ENODEV inside ndo_init callback if no phy_dev detected on the bus

--
after with the patch, the logic becomes

1) returning -ENODEV if no mdio_bus defined in the device tree

2) returning -ENODEV if no phy_dev defined in the device tree 
a. that is used to check if the device tree about phy_dev is defined well
b. and it could be aligned with the above 1) is doing

3) returning -ENODEV inside ndo_init callback if no phy device detected on the bus 
(that is used to test if phy_device is workable or encounters real phy problems)

so add 2) help to make distinguish if it is a device tree definition 
problem or a real phy problem at the earlier time

this is my whole thought

thanks,
Sean
quoted
Signed-off-by: Sean Wang <sean.wang@mediatek.com>
Acked-by: John Crispin <john@phrozen.org>
---
 drivers/net/ethernet/mediatek/mtk_eth_soc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
index f741c6a..e48b2a4 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
@@ -304,7 +304,7 @@ static int mtk_mdio_init(struct mtk_eth *eth)
 	}
 
 	if (!of_device_is_available(mii_np)) {
-		ret = 0;
+		ret = -ENODEV;
 		goto err_put_node;
 	}
 
-- 
1.9.1

Re: [PATCH net v2 9/9] net: ethernet: mediatek: fix error handling inside mtk_mdio_init

From: Andrew Lunn <andrew@lunn.ch>
Date: 2016-08-29 16:36:37

On Mon, Aug 29, 2016 at 01:03:23PM +0800, sean.wang@mediatek.com wrote:
From: Sean Wang <sean.wang@mediatek.com>

return -ENODEV if no child is found in MDIO bus.
The "no child" is wrong here, and got me confused. What the code is
actually doing is of_device_is_available() which is looking to see if
there is a status = "okay". So what the change log should say is:

Return -ENODEV if the MDIO bus is disabled in the device tree.

Once that has been corrected:

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

quoted hunk
Signed-off-by: Sean Wang <sean.wang@mediatek.com>
Acked-by: John Crispin <john@phrozen.org>
---
 drivers/net/ethernet/mediatek/mtk_eth_soc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
index f741c6a..e48b2a4 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
@@ -304,7 +304,7 @@ static int mtk_mdio_init(struct mtk_eth *eth)
 	}
 
 	if (!of_device_is_available(mii_np)) {
-		ret = 0;
+		ret = -ENODEV;
 		goto err_put_node;
 	}
 
-- 
1.9.1
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help