* issue patch in next net/eth: fix link handling @ 2012-09-28 2:28 Jean-Christophe PLAGNIOL-VILLARD 2012-09-28 7:50 ` Sascha Hauer 0 siblings, 1 reply; 5+ messages in thread From: Jean-Christophe PLAGNIOL-VILLARD @ 2012-09-28 2:28 UTC (permalink / raw) To: barebox HI, The patch is next net/eth: fix link handling was NEVER send to the ML IIRC I was the author of the first version and this disapear Uwe and I just get this discussion on the kernel ML about patch update Best Regards, J. _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: issue patch in next net/eth: fix link handling 2012-09-28 2:28 issue patch in next net/eth: fix link handling Jean-Christophe PLAGNIOL-VILLARD @ 2012-09-28 7:50 ` Sascha Hauer 2012-09-28 8:27 ` Jean-Christophe PLAGNIOL-VILLARD 0 siblings, 1 reply; 5+ messages in thread From: Sascha Hauer @ 2012-09-28 7:50 UTC (permalink / raw) To: Jean-Christophe PLAGNIOL-VILLARD; +Cc: barebox On Fri, Sep 28, 2012 at 04:28:21AM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote: > HI, > > The patch is next > net/eth: fix link handling > > was NEVER send to the ML > > IIRC I was the author of the first version and this disapear > > Uwe and I just get this discussion on the kernel ML about patch update > I was basically pissed off because I got the strong feeling that I spent more time reviewing and testing the patch than you initially spent writing it in the first place. The second version still stored apples in edev->phydev->link and bananas in edev->carrier, but still did a edev->carrier = dev->link. Sorry for not posting it on the ML, doing that now. Sascha 8<-------------------------------------------------- From 0dc9de2efd7ea6fb613afec822b52b4bfe5a7b2e Mon Sep 17 00:00:00 2001 From: Sascha Hauer <s.hauer@pengutronix.de> Date: Wed, 26 Sep 2012 17:53:29 +0200 Subject: [PATCH] net/eth: fix link handling Check link status on eth device open time and then periodically every 5 seconds. Based on work from from Jean-Christophe, but this version does not duplicate the link status information in two different variables. Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> --- drivers/net/phy/phy.c | 35 ++++++++++++++++------- include/linux/phy.h | 3 ++ net/eth.c | 75 +++++++++++++++++++++++++++++++++++++------------ 3 files changed, 85 insertions(+), 28 deletions(-) diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c index e1a24fa..88c3ff7 100644 --- a/drivers/net/phy/phy.c +++ b/drivers/net/phy/phy.c @@ -29,6 +29,30 @@ static int genphy_config_init(struct phy_device *phydev); +int phy_update_status(struct phy_device *dev) +{ + struct phy_driver *drv = to_phy_driver(dev->dev.driver); + struct eth_device *edev = dev->attached_dev; + int ret; + int oldspeed = dev->speed, oldduplex = dev->duplex; + + ret = drv->read_status(dev); + if (ret) + return ret; + + if (dev->speed == oldspeed && dev->duplex == oldduplex) + return 0; + + if (dev->adjust_link) + dev->adjust_link(edev); + + if (dev->link) + printf("%dMbps %s duplex link detected\n", dev->speed, + dev->duplex ? "full" : "half"); + + return 0; +} + struct phy_device *phy_device_create(struct mii_bus *bus, int addr, int phy_id) { struct phy_device *dev; @@ -172,16 +196,7 @@ int phy_device_connect(struct eth_device *edev, struct mii_bus *bus, int addr, drv->config_aneg(dev); - ret = drv->read_status(dev); - if (ret < 0) - return ret; - - if (dev->link) - printf("%dMbps %s duplex link detected\n", dev->speed, - dev->duplex ? "full" : "half"); - - if (adjust_link) - adjust_link(edev); + dev->adjust_link = adjust_link; return 0; diff --git a/include/linux/phy.h b/include/linux/phy.h index 8fe6b86..76f9edb 100644 --- a/include/linux/phy.h +++ b/include/linux/phy.h @@ -164,6 +164,7 @@ struct phy_device { void *priv; struct eth_device *attached_dev; + void (*adjust_link)(struct eth_device *dev); struct cdev cdev; }; @@ -253,6 +254,8 @@ int phy_device_connect(struct eth_device *dev, struct mii_bus *bus, int addr, void (*adjust_link) (struct eth_device *edev), u32 flags, phy_interface_t interface); +int phy_update_status(struct phy_device *phydev); + /* Generic PHY support and helper functions */ int genphy_restart_aneg(struct phy_device *phydev); int genphy_config_aneg(struct phy_device *phydev); diff --git a/net/eth.c b/net/eth.c index 6cfe4f2..9f52870 100644 --- a/net/eth.c +++ b/net/eth.c @@ -32,6 +32,7 @@ #include <malloc.h> static struct eth_device *eth_current; +static uint64_t last_link_check; static LIST_HEAD(netdev_list); @@ -128,24 +129,64 @@ int eth_complete(struct string_list *sl, char *instr) } #endif -int eth_send(void *packet, int length) +/* + * Check for link if we haven't done so for longer. + */ +static int eth_carrier_check(int force) { int ret; - if (!eth_current) - return -ENODEV; + if (!IS_ENABLED(CONFIG_PHYLIB)) + return 0; - if (!eth_current->active) { - ret = eth_current->open(eth_current); + if (!eth_current->phydev) + return 0; + + if (force || is_timeout(last_link_check, 5 * SECOND)) { + ret = phy_update_status(eth_current->phydev); if (ret) return ret; - - if (eth_current->phydev) - eth_current->active = eth_current->phydev->link; - else - eth_current->active = 1; + last_link_check = get_time_ns(); } + return eth_current->phydev->link ? 0 : -ENETDOWN; +} + +/* + * Check if we have a current ethernet device and + * eventually open it if we have to. + */ +static int eth_check_open(void) +{ + int ret; + + if (!eth_current) + return -ENODEV; + + if (eth_current->active) + return 0; + + ret = eth_current->open(eth_current); + if (ret) + return ret; + + eth_current->active = 1; + + return eth_carrier_check(1); +} + +int eth_send(void *packet, int length) +{ + int ret; + + ret = eth_check_open(); + if (ret) + return ret; + + ret = eth_carrier_check(0); + if (ret) + return ret; + led_trigger_network(LED_TRIGGER_NET_TX); return eth_current->send(eth_current, packet, length); @@ -155,15 +196,13 @@ int eth_rx(void) { int ret; - if (!eth_current) - return -ENODEV; + ret = eth_check_open(); + if (ret) + return ret; - if (!eth_current->active) { - ret = eth_current->open(eth_current); - if (ret) - return ret; - eth_current->active = 1; - } + ret = eth_carrier_check(0); + if (ret) + return ret; return eth_current->recv(eth_current); } -- 1.7.10.4 -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: issue patch in next net/eth: fix link handling 2012-09-28 7:50 ` Sascha Hauer @ 2012-09-28 8:27 ` Jean-Christophe PLAGNIOL-VILLARD 2012-09-28 8:55 ` Sascha Hauer 0 siblings, 1 reply; 5+ messages in thread From: Jean-Christophe PLAGNIOL-VILLARD @ 2012-09-28 8:27 UTC (permalink / raw) To: Sascha Hauer; +Cc: barebox On 09:50 Fri 28 Sep , Sascha Hauer wrote: > On Fri, Sep 28, 2012 at 04:28:21AM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote: > > HI, > > > > The patch is next > > net/eth: fix link handling > > > > was NEVER send to the ML > > > > IIRC I was the author of the first version and this disapear > > > > Uwe and I just get this discussion on the kernel ML about patch update > > > > I was basically pissed off because I got the strong feeling that I spent > more time reviewing and testing the patch than you initially spent > writing it in the first place. The second version still stored apples > in edev->phydev->link and bananas in edev->carrier, but still did a > edev->carrier = dev->link. I did this on purpose as I do want to store the link and later export it via env as I get a patch here for 2 wifi driver where I'll not use the phylib so store the carrier is the correct way we could use a net_carrier(xx) as in the kernel to update the field but I do need the carrier to not be manage only for the phylib my work on thiose wifi driver is not finished but on the way maybe next release > > Sorry for not posting it on the ML, doing that now. I was pissed off that this is based on my work and I lost the author of the patch and no mention about my work Best Regards, J. _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: issue patch in next net/eth: fix link handling 2012-09-28 8:27 ` Jean-Christophe PLAGNIOL-VILLARD @ 2012-09-28 8:55 ` Sascha Hauer 2012-09-28 9:14 ` Jean-Christophe PLAGNIOL-VILLARD 0 siblings, 1 reply; 5+ messages in thread From: Sascha Hauer @ 2012-09-28 8:55 UTC (permalink / raw) To: Jean-Christophe PLAGNIOL-VILLARD; +Cc: barebox On Fri, Sep 28, 2012 at 10:27:16AM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote: > On 09:50 Fri 28 Sep , Sascha Hauer wrote: > > On Fri, Sep 28, 2012 at 04:28:21AM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote: > > > HI, > > > > > > The patch is next > > > net/eth: fix link handling > > > > > > was NEVER send to the ML > > > > > > IIRC I was the author of the first version and this disapear > > > > > > Uwe and I just get this discussion on the kernel ML about patch update > > > > > > > I was basically pissed off because I got the strong feeling that I spent > > more time reviewing and testing the patch than you initially spent > > writing it in the first place. The second version still stored apples > > in edev->phydev->link and bananas in edev->carrier, but still did a > > edev->carrier = dev->link. > I did this on purpose as I do want to store the link and later export it via > env as I get a patch here for 2 wifi driver where I'll not use the phylib > > so store the carrier is the correct way Whatever it is, adding a variable to an ethernet device and then manipulating it in both the phylib and the ethernet code is desastrous. It must be clear everytime who owns a variable. Doing a eth_current->carrier = CARRIER_UNKNOW; in the ethernet code, and then: edev->carrier = dev->link; in the phy code is a recipe for spaghetti code. Sascha (Who loves spaghetti - on his plate) -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: issue patch in next net/eth: fix link handling 2012-09-28 8:55 ` Sascha Hauer @ 2012-09-28 9:14 ` Jean-Christophe PLAGNIOL-VILLARD 0 siblings, 0 replies; 5+ messages in thread From: Jean-Christophe PLAGNIOL-VILLARD @ 2012-09-28 9:14 UTC (permalink / raw) To: Sascha Hauer; +Cc: barebox On 10:55 Fri 28 Sep , Sascha Hauer wrote: > On Fri, Sep 28, 2012 at 10:27:16AM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote: > > On 09:50 Fri 28 Sep , Sascha Hauer wrote: > > > On Fri, Sep 28, 2012 at 04:28:21AM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote: > > > > HI, > > > > > > > > The patch is next > > > > net/eth: fix link handling > > > > > > > > was NEVER send to the ML > > > > > > > > IIRC I was the author of the first version and this disapear > > > > > > > > Uwe and I just get this discussion on the kernel ML about patch update > > > > > > > > > > I was basically pissed off because I got the strong feeling that I spent > > > more time reviewing and testing the patch than you initially spent > > > writing it in the first place. The second version still stored apples > > > in edev->phydev->link and bananas in edev->carrier, but still did a > > > edev->carrier = dev->link. > > I did this on purpose as I do want to store the link and later export it via > > env as I get a patch here for 2 wifi driver where I'll not use the phylib > > > > so store the carrier is the correct way > > Whatever it is, adding a variable to an ethernet device and then > manipulating it in both the phylib and the ethernet code is desastrous. > It must be clear everytime who owns a variable. Doing a > > eth_current->carrier = CARRIER_UNKNOW; > > in the ethernet code, and then: > > edev->carrier = dev->link; > > in the phy code is a recipe for spaghetti code. > so we need to do call net_carrier in phylib as done in the kernel the net framework mamange the carrier by itself as example in wifi phy up does not mean carrier on Best Regards, J. _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2012-09-28 9:16 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-09-28 2:28 issue patch in next net/eth: fix link handling Jean-Christophe PLAGNIOL-VILLARD 2012-09-28 7:50 ` Sascha Hauer 2012-09-28 8:27 ` Jean-Christophe PLAGNIOL-VILLARD 2012-09-28 8:55 ` Sascha Hauer 2012-09-28 9:14 ` Jean-Christophe PLAGNIOL-VILLARD
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox