* 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