[U-Boot] [U-Boot PATCH MX31:] smc911x MII made available

[U-Boot] [U-Boot PATCH MX31:] smc911x MII made available

[Helmut.Raiger at hale.at]

From: Helmut Raiger <helmut.raiger@hale.at>

The driver already had the MII functions, but they have not been
registered using miiphy_register().

Signed-off-by: Helmut Raiger <helmut.raiger@hale.at>
---
 drivers/net/smc911x.c |   24 ++++++++++++++++++++++++
 1 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/drivers/net/smc911x.c b/drivers/net/smc911x.c
index aeafeba..9378a63 100644
--- a/drivers/net/smc911x.c
+++ b/drivers/net/smc911x.c
@@ -235,6 +235,25 @@ static int smc911x_rx(struct eth_device *dev)
 	return 0;
 }
 
+#if defined(CONFIG_MII) || defined(CONFIG_CMD_MII)
+/* wrapper for smc911x_miiphy_read */
+static int _phy_read(char *devname, u8 phy, u8 reg, u16 *val)
+{
+	struct eth_device *dev = eth_get_dev_by_name(devname);
+	if (dev)
+		return smc911x_miiphy_read(dev, phy, reg, val);
+	return -1;
+}
+/* wrapper for smc911x_miiphy_write */
+static int _phy_write(char *devname, u8 phy, u8 reg, u16 val)
+{
+	struct eth_device *dev = eth_get_dev_by_name(devname);
+	if (dev)
+		return smc911x_miiphy_write(dev, phy, reg, val);
+	return -1;
+}
+#endif
+
 int smc911x_initialize(u8 dev_num, int base_addr)
 {
 	unsigned long addrl, addrh;
@@ -273,5 +292,10 @@ int smc911x_initialize(u8 dev_num, int base_addr)
 	sprintf(dev->name, "%s-%hu", DRIVERNAME, dev_num);
 
 	eth_register(dev);
+
+#if defined(CONFIG_MII) || defined(CONFIG_CMD_MII)
+	miiphy_register(dev->name, _phy_read, _phy_write);
+#endif
+
 	return 1;
 }
-- 
1.7.4.4



--
Scanned by MailScanner.

[U-Boot] [U-Boot PATCH MX31:] smc911x MII made available

[Stefano Babic]

On 06/20/2011 08:10 AM, Helmut.Raiger at hale.at wrote:
> From: Helmut Raiger <helmut.raiger@hale.at>

Hi Helmut,

> 
> The driver already had the MII functions, but they have not been
> registered using miiphy_register().
> 
> Signed-off-by: Helmut Raiger <helmut.raiger@hale.at>

Not noted before, thanks for fixing it. Only to remark the issue, on
boards with SMC911x (at least the one I tested your patch) and
CONFIG_CMD_MII set, a simple "mii info" returns "Read MDIO failed.."

You should always put in CC the maintainer for your patches. Because
this patch is related to network, you should send your changes to
Wolfgang Denk (Network Maintaner), too. I have already set his name in CC.

> +#if defined(CONFIG_MII) || defined(CONFIG_CMD_MII)
> +/* wrapper for smc911x_miiphy_read */
> +static int _phy_read(char *devname, u8 phy, u8 reg, u16 *val)

Is there some reason to use name starting with _ ? They have special
meaning, and there is no need here.

I have tested your patch on the mx35pdk board.

Tested-by: Stefano Babic <sbabic@denx.de>

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================

[U-Boot] [U-Boot PATCH MX31:] smc911x MII made available

[Helmut Raiger]

On 06/20/2011 02:30 PM, Stefano Babic wrote:
>
> Not noted before, thanks for fixing it. Only to remark the issue, on
> boards with SMC911x (at least the one I tested your patch) and
> CONFIG_CMD_MII set, a simple "mii info" returns "Read MDIO failed.."
>
Our board holds a SMSC LAN9211-ABZJ , 'mii info' returns:

PHY 0x00: OUI = 0x0000, Model = 0x00, Rev = 0x00,  10baseT, HDX
PHY 0x01: OUI = 0x01F0, Model = 0x0C, Rev = 0x03, 100baseT, FDX
PHY 0x02: OUI = 0x0000, Model = 0x00, Rev = 0x00,  10baseT, HDX
PHY 0x03: OUI = 0x0000, Model = 0x00, Rev = 0x00,  10baseT, HDX
PHY 0x04: OUI = 0x0000, Model = 0x00, Rev = 0x00,  10baseT, HDX
PHY 0x05: OUI = 0x0000, Model = 0x00, Rev = 0x00,  10baseT, HDX
PHY 0x06: OUI = 0x0000, Model = 0x00, Rev = 0x00,  10baseT, HDX
PHY 0x07: OUI = 0x0000, Model = 0x00, Rev = 0x00,  10baseT, HDX
PHY 0x08: OUI = 0x0000, Model = 0x00, Rev = 0x00,  10baseT, HDX
PHY 0x09: OUI = 0x0000, Model = 0x00, Rev = 0x00,  10baseT, HDX
PHY 0x0A: OUI = 0x0000, Model = 0x00, Rev = 0x00,  10baseT, HDX
PHY 0x0B: OUI = 0x0000, Model = 0x00, Rev = 0x00,  10baseT, HDX
PHY 0x0C: OUI = 0x0000, Model = 0x00, Rev = 0x00,  10baseT, HDX
PHY 0x0D: OUI = 0x0000, Model = 0x00, Rev = 0x00,  10baseT, HDX
PHY 0x0E: OUI = 0x0000, Model = 0x00, Rev = 0x00,  10baseT, HDX
PHY 0x0F: OUI = 0x0000, Model = 0x00, Rev = 0x00,  10baseT, HDX
PHY 0x10: OUI = 0x0000, Model = 0x00, Rev = 0x00,  10baseT, HDX
PHY 0x11: OUI = 0x0000, Model = 0x00, Rev = 0x00,  10baseT, HDX
PHY 0x12: OUI = 0x0000, Model = 0x00, Rev = 0x00,  10baseT, HDX
PHY 0x13: OUI = 0x0000, Model = 0x00, Rev = 0x00,  10baseT, HDX
PHY 0x14: OUI = 0x0000, Model = 0x00, Rev = 0x00,  10baseT, HDX
PHY 0x15: OUI = 0x0000, Model = 0x00, Rev = 0x00,  10baseT, HDX
PHY 0x16: OUI = 0x0000, Model = 0x00, Rev = 0x00,  10baseT, HDX
PHY 0x17: OUI = 0x0000, Model = 0x00, Rev = 0x00,  10baseT, HDX
PHY 0x18: OUI = 0x0000, Model = 0x00, Rev = 0x00,  10baseT, HDX
PHY 0x19: OUI = 0x0000, Model = 0x00, Rev = 0x00,  10baseT, HDX
PHY 0x1A: OUI = 0x0000, Model = 0x00, Rev = 0x00,  10baseT, HDX
PHY 0x1B: OUI = 0x0000, Model = 0x00, Rev = 0x00,  10baseT, HDX
PHY 0x1C: OUI = 0x0000, Model = 0x00, Rev = 0x00,  10baseT, HDX
PHY 0x1D: OUI = 0x0000, Model = 0x00, Rev = 0x00,  10baseT, HDX
PHY 0x1E: OUI = 0x0000, Model = 0x00, Rev = 0x00,  10baseT, HDX
PHY 0x1F: OUI = 0x0000, Model = 0x00, Rev = 0x00,  10baseT, HDX

'mii device' says:

MII devices: 'smc911x-0'
Current device: 'smc911x-0'
> You should always put in CC the maintainer for your patches. Because
> this patch is related to network, you should send your changes to
> Wolfgang Denk (Network Maintaner), too. I have already set his name in CC.
>
Ok, I did search the MAINTAINER file, but could not attach a name to my 
fix, neither smc... nor network, nor something else.
>> +#if defined(CONFIG_MII) || defined(CONFIG_CMD_MII)
>> +/* wrapper for smc911x_miiphy_read */
>> +static int _phy_read(char *devname, u8 phy, u8 reg, u16 *val)
> Is there some reason to use name starting with _ ? They have special
> meaning, and there is no need here.
>
No, I wasn't aware of the _ meaning. I usually name wrappers that way, 
but only personal preference. I'll fix the naming along with other 
things that might come up.

> I have tested your patch on the mx35pdk board.
>
> Tested-by: Stefano Babic<sbabic@denx.de>
>
> Best regards,
> Stefano Babic
>
Thanks for testing,
Helmut



--
Scanned by MailScanner.

[U-Boot] [PATCH] smc911x MII made available

[helmut.raiger at hale.at]

From: Helmut Raiger <helmut.raiger@hale.at>

The driver already had the MII functions, but they have not been
registered using miiphy_register().
---
 drivers/net/smc911x.c |   24 ++++++++++++++++++++++++
 1 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/drivers/net/smc911x.c b/drivers/net/smc911x.c
index aeafeba..8753588 100644
--- a/drivers/net/smc911x.c
+++ b/drivers/net/smc911x.c
@@ -235,6 +235,25 @@ static int smc911x_rx(struct eth_device *dev)
 	return 0;
 }
 
+#if defined(CONFIG_MII) || defined(CONFIG_CMD_MII)
+/* wrapper for smc911x_miiphy_read */
+static int mii_phy_read(char *devname, u8 phy, u8 reg, u16 *val)
+{
+	struct eth_device *dev = eth_get_dev_by_name(devname);
+	if (dev)
+		return smc911x_miiphy_read(dev, phy, reg, val);
+	return -1;
+}
+/* wrapper for smc911x_miiphy_write */
+static int mii_phy_write(char *devname, u8 phy, u8 reg, u16 val)
+{
+	struct eth_device *dev = eth_get_dev_by_name(devname);
+	if (dev)
+		return smc911x_miiphy_write(dev, phy, reg, val);
+	return -1;
+}
+#endif
+
 int smc911x_initialize(u8 dev_num, int base_addr)
 {
 	unsigned long addrl, addrh;
@@ -273,5 +292,10 @@ int smc911x_initialize(u8 dev_num, int base_addr)
 	sprintf(dev->name, "%s-%hu", DRIVERNAME, dev_num);
 
 	eth_register(dev);
+
+#if defined(CONFIG_MII) || defined(CONFIG_CMD_MII)
+	miiphy_register(dev->name, mii_phy_read, mii_phy_write);
+#endif
+
 	return 1;
 }
-- 
1.7.4.4



--
Scanned by MailScanner.

[U-Boot] [PATCH] smc911x MII made available

[Mike Frysinger]

On Monday, June 27, 2011 03:22:03 helmut.raiger at hale.at wrote:
> From: Helmut Raiger <helmut.raiger@hale.at>
> 
> The driver already had the MII functions, but they have not been
> registered using miiphy_register().

missing signed-off-by tag

> +static int mii_phy_read(char *devname, u8 phy, u8 reg, u16 *val)

this name is already in common name space in mii_phy.h.  all driver funcs 
really should be prefixed anyways.  so perhaps:
	smc911x_mii_phy_read
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20110627/1aad3fb5/attachment.pgp 

[U-Boot] [PATCH] smc911x MII made available

[helmut.raiger at hale.at]

From: Helmut Raiger <helmut.raiger@hale.at>

The driver already had the MII functions, but they have not been
registered using miiphy_register().

Signed-off-by: Helmut Raiger <helmut.raiger@hale.at>
---
 drivers/net/smc911x.c |   36 ++++++++++++++++++++++++++++++------
 1 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/drivers/net/smc911x.c b/drivers/net/smc911x.c
index aeafeba..6cc236c 100644
--- a/drivers/net/smc911x.c
+++ b/drivers/net/smc911x.c
@@ -50,7 +50,7 @@ static void smc911x_handle_mac_address(struct eth_device *dev)
 	printf(DRIVERNAME ": MAC %pM\n", m);
 }
 
-static int smc911x_miiphy_read(struct eth_device *dev,
+static int smc911x_eth_phy_read(struct eth_device *dev,
 				u8 phy, u8 reg, u16 *val)
 {
 	while (smc911x_get_mac_csr(dev, MII_ACC) & MII_ACC_MII_BUSY)
@@ -67,7 +67,7 @@ static int smc911x_miiphy_read(struct eth_device *dev,
 	return 0;
 }
 
-static int smc911x_miiphy_write(struct eth_device *dev,
+static int smc911x_eth_phy_write(struct eth_device *dev,
 				u8 phy, u8 reg, u16  val)
 {
 	while (smc911x_get_mac_csr(dev, MII_ACC) & MII_ACC_MII_BUSY)
@@ -103,10 +103,10 @@ static void smc911x_phy_configure(struct eth_device *dev)
 
 	smc911x_phy_reset(dev);
 
-	smc911x_miiphy_write(dev, 1, MII_BMCR, BMCR_RESET);
+	smc911x_eth_phy_write(dev, 1, MII_BMCR, BMCR_RESET);
 	mdelay(1);
-	smc911x_miiphy_write(dev, 1, MII_ADVERTISE, 0x01e1);
-	smc911x_miiphy_write(dev, 1, MII_BMCR, BMCR_ANENABLE |
+	smc911x_eth_phy_write(dev, 1, MII_ADVERTISE, 0x01e1);
+	smc911x_eth_phy_write(dev, 1, MII_BMCR, BMCR_ANENABLE |
 				BMCR_ANRESTART);
 
 	timeout = 5000;
@@ -115,7 +115,7 @@ static void smc911x_phy_configure(struct eth_device *dev)
 		if ((timeout--) == 0)
 			goto err_out;
 
-		if (smc911x_miiphy_read(dev, 1, MII_BMSR, &status) != 0)
+		if (smc911x_eth_phy_read(dev, 1, MII_BMSR, &status) != 0)
 			goto err_out;
 	} while (!(status & BMSR_LSTATUS));
 
@@ -235,6 +235,25 @@ static int smc911x_rx(struct eth_device *dev)
 	return 0;
 }
 
+#if defined(CONFIG_MII) || defined(CONFIG_CMD_MII)
+/* wrapper for smc911x_eth_phy_read */
+static int smc911x_miiphy_read(char *devname, u8 phy, u8 reg, u16 *val)
+{
+	struct eth_device *dev = eth_get_dev_by_name(devname);
+	if (dev)
+		return smc911x_eth_phy_read(dev, phy, reg, val);
+	return -1;
+}
+/* wrapper for smc911x_eth_phy_write */
+static int smc911x_miiphy_write(char *devname, u8 phy, u8 reg, u16 val)
+{
+	struct eth_device *dev = eth_get_dev_by_name(devname);
+	if (dev)
+		return smc911x_eth_phy_write(dev, phy, reg, val);
+	return -1;
+}
+#endif
+
 int smc911x_initialize(u8 dev_num, int base_addr)
 {
 	unsigned long addrl, addrh;
@@ -273,5 +292,10 @@ int smc911x_initialize(u8 dev_num, int base_addr)
 	sprintf(dev->name, "%s-%hu", DRIVERNAME, dev_num);
 
 	eth_register(dev);
+
+#if defined(CONFIG_MII) || defined(CONFIG_CMD_MII)
+	miiphy_register(dev->name, smc911x_miiphy_read, smc911x_miiphy_write);
+#endif
+
 	return 1;
 }
-- 
1.7.4.4



--
Scanned by MailScanner.

[U-Boot] [PATCH] smc911x MII made available

[Luca Ceresoli]

Hi Helmut,


helmut.raiger at hale.at wrote:
> From: Helmut Raiger<helmut.raiger@hale.at>
>
> The driver already had the MII functions, but they have not been
> registered using miiphy_register().
>
> Signed-off-by: Helmut Raiger<helmut.raiger@hale.at>
[...]

I implemented in the past, but never found the time to polish it for
submission. It looks very similar to yours, and the actual code is the
same.

Your version has more complete #ifdefs, mine has a more complete error
checking.

I'm sending my patch in reply to this e-mail. Feel free to have a look
and integrate my changes with yours.

Luca



--------------------------------------------------------------------------
Luca Ceresoli
Comelit R&D - Progettazione Software

Phone
Fax
Mail
Web
YouTube 

luca.ceresoli at comelit.it
http://www.comelitgroup.com/
http://www.youtube.com/comelitgroup
--------------------------------------------------------------------------
Prima di stampare pensa all'ambiente / Think about environment before printing
AVVISO DI CONFIDENZIALIT?
Questo messaggio ed i suoi eventuali allegati pu? contenere informazioni confidenziali, di propriet?, legalmente protette.? destinato all'utilizzo esclusivo del destinatario sopra indicato; privatezza e confidenzialit? non sono modificati da eventuali errori di trasmissione. Se il messaggio non ? a lei destinato, non deve utilizzarlo, diffonderlo, copiarlo con qualunque mezzo, o intraprendere azioni basandosi su di esso. Se ha ricevuto questo messaggio per errore, la preghiamo di volerlo distruggere (unitamente ad eventuali copie dello stesso) e di volerci cortesemente informare del fatto scrivendo al mittente.
CONFIDENTIALITY NOTICE
This message and its attachments (if any) may contain confidential, proprietary or legally privileged information and it is intended only for the use of the addressee named above. No confidentiality or privilege is waived or lost by any erroneous transmission. If you are not the intended recipient of this message you are hereby notified that you must not use, disseminate, copy it in any form or take any action in reliance on it. If you have received this message in error, please, delete it (and any copies of it) and kindly inform the sender.

[U-Boot] [PATCH RFC] smc911x: enable mii commands

[Luca Ceresoli]

Signed-off-by: Luca Ceresoli <luca.ceresoli@comelit.it>
---
This patch is my implementation of the same functionality that Helmut's patch
implements. I'm sending it as a reference for him to look at the error
checking code and possibly merge it into his own patch.

 drivers/net/smc911x.c |   41 +++++++++++++++++++++++++++++++++++++++++
 1 files changed, 41 insertions(+), 0 deletions(-)

diff --git a/drivers/net/smc911x.c b/drivers/net/smc911x.c
index aeafeba..d946fd4 100644
--- a/drivers/net/smc911x.c
+++ b/drivers/net/smc911x.c
@@ -82,6 +82,42 @@ static int smc911x_miiphy_write(struct eth_device *dev,
 	return 0;
 }
 
+static int smc911x_miiphy_read_byname(char *devname, unsigned char addr,
+				      unsigned char reg, unsigned short *value)
+{
+	struct eth_device *dev;
+
+	if (devname == NULL)
+		return -1;
+
+	dev = eth_get_dev_by_name(devname);
+
+	if (dev == NULL) {
+		printf(DRIVERNAME ": device %s not found\n", devname);
+		return -1;
+	}
+
+	return smc911x_miiphy_read(dev, addr, reg, value);
+}
+
+static int smc911x_miiphy_write_byname(char *devname, unsigned char addr,
+				       unsigned char reg, unsigned short value)
+{
+	struct eth_device *dev;
+
+	if (devname == NULL)
+		return -1;
+
+	dev = eth_get_dev_by_name(devname);
+
+	if (dev == NULL) {
+		printf(DRIVERNAME ": device %s not found\n", devname);
+		return -1;
+	}
+
+	return smc911x_miiphy_write(dev, addr, reg, value);
+}
+
 static int smc911x_phy_reset(struct eth_device *dev)
 {
 	u32 reg;
@@ -273,5 +309,10 @@ int smc911x_initialize(u8 dev_num, int base_addr)
 	sprintf(dev->name, "%s-%hu", DRIVERNAME, dev_num);
 
 	eth_register(dev);
+
+#if defined(CONFIG_MII) || defined(CONFIG_CMD_MII)
+	miiphy_register(dev->name, smc911x_miiphy_read_byname, smc911x_miiphy_write_byname);
+#endif
+
 	return 1;
 }
-- 
1.7.4.1




--------------------------------------------------------------------------
Luca Ceresoli
Comelit R&D - Progettazione Software

Phone
Fax
Mail
Web
YouTube 

luca.ceresoli at comelit.it
http://www.comelitgroup.com/
http://www.youtube.com/comelitgroup
--------------------------------------------------------------------------
Prima di stampare pensa all'ambiente / Think about environment before printing
AVVISO DI CONFIDENZIALIT?
Questo messaggio ed i suoi eventuali allegati pu? contenere informazioni confidenziali, di propriet?, legalmente protette.? destinato all'utilizzo esclusivo del destinatario sopra indicato; privatezza e confidenzialit? non sono modificati da eventuali errori di trasmissione. Se il messaggio non ? a lei destinato, non deve utilizzarlo, diffonderlo, copiarlo con qualunque mezzo, o intraprendere azioni basandosi su di esso. Se ha ricevuto questo messaggio per errore, la preghiamo di volerlo distruggere (unitamente ad eventuali copie dello stesso) e di volerci cortesemente informare del fatto scrivendo al mittente.
CONFIDENTIALITY NOTICE
This message and its attachments (if any) may contain confidential, proprietary or legally privileged information and it is intended only for the use of the addressee named above. No confidentiality or privilege is waived or lost by any erroneous transmission. If you are not the intended recipient of this message you are hereby notified that you must not use, disseminate, copy it in any form or take any action in reliance on it. If you have received this message in error, please, delete it (and any copies of it) and kindly inform the sender.

[U-Boot] [PATCH RFC] smc911x: enable mii commands

[Helmut Raiger]

On 06/30/2011 04:02 PM, Luca Ceresoli wrote:
> +static int smc911x_miiphy_read_byname(char *devname, unsigned char addr,
> +				      unsigned char reg, unsigned short *value)
> +{
> +	struct eth_device *dev;
> +
> +	if (devname == NULL)
> +		return -1;
> +
> +	dev = eth_get_dev_by_name(devname);
You're right. eth_get_dev_by_name() is not safe for devname == NULL as 
it uses strcmp().
Best would be to fix this there, I'll adjust my patch accordingly.

> +
> +	if (dev == NULL) {
> +		printf(DRIVERNAME ": device %s not found\n", devname);
> +		return -1;
> +	}

None of the other drivers in drivers/net add this kind of verbosity, so 
I tend to leave it at that.

Helmut





--
Scanned by MailScanner.

[U-Boot] [PATCH 1/2] net/eth.c: make eth_get_dev_by_name(NULL) safe

[helmut.raiger at hale.at]

From: Helmut Raiger <helmut.raiger@hale.at>

eth_get_dev_by_name() is not safe to use for devname being NULL
as it uses strcmp. This patch makes it return NULL if devname NULL
is passed.

Signed-off-by: Helmut Raiger <helmut.raiger@hale.at>
---
 net/eth.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/eth.c b/net/eth.c
index 6523834..c75b7c9 100644
--- a/net/eth.c
+++ b/net/eth.c
@@ -107,7 +107,7 @@ struct eth_device *eth_get_dev_by_name(const char *devname)
 {
 	struct eth_device *dev, *target_dev;
 
-	if (!eth_devices)
+	if (!eth_devices || !devname)
 		return NULL;
 
 	dev = eth_devices;
-- 
1.7.4.4



--
Scanned by MailScanner.

[U-Boot] [PATCH 2/2] smc911x MII made available

[helmut.raiger at hale.at]

From: Helmut Raiger <helmut.raiger@hale.at>

The driver already had the MII functions, but they have not been
registered using miiphy_register().

Signed-off-by: Helmut Raiger <helmut.raiger@hale.at>
---
 drivers/net/smc911x.c |   36 ++++++++++++++++++++++++++++++------
 1 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/drivers/net/smc911x.c b/drivers/net/smc911x.c
index aeafeba..6cc236c 100644
--- a/drivers/net/smc911x.c
+++ b/drivers/net/smc911x.c
@@ -50,7 +50,7 @@ static void smc911x_handle_mac_address(struct eth_device *dev)
 	printf(DRIVERNAME ": MAC %pM\n", m);
 }
 
-static int smc911x_miiphy_read(struct eth_device *dev,
+static int smc911x_eth_phy_read(struct eth_device *dev,
 				u8 phy, u8 reg, u16 *val)
 {
 	while (smc911x_get_mac_csr(dev, MII_ACC) & MII_ACC_MII_BUSY)
@@ -67,7 +67,7 @@ static int smc911x_miiphy_read(struct eth_device *dev,
 	return 0;
 }
 
-static int smc911x_miiphy_write(struct eth_device *dev,
+static int smc911x_eth_phy_write(struct eth_device *dev,
 				u8 phy, u8 reg, u16  val)
 {
 	while (smc911x_get_mac_csr(dev, MII_ACC) & MII_ACC_MII_BUSY)
@@ -103,10 +103,10 @@ static void smc911x_phy_configure(struct eth_device *dev)
 
 	smc911x_phy_reset(dev);
 
-	smc911x_miiphy_write(dev, 1, MII_BMCR, BMCR_RESET);
+	smc911x_eth_phy_write(dev, 1, MII_BMCR, BMCR_RESET);
 	mdelay(1);
-	smc911x_miiphy_write(dev, 1, MII_ADVERTISE, 0x01e1);
-	smc911x_miiphy_write(dev, 1, MII_BMCR, BMCR_ANENABLE |
+	smc911x_eth_phy_write(dev, 1, MII_ADVERTISE, 0x01e1);
+	smc911x_eth_phy_write(dev, 1, MII_BMCR, BMCR_ANENABLE |
 				BMCR_ANRESTART);
 
 	timeout = 5000;
@@ -115,7 +115,7 @@ static void smc911x_phy_configure(struct eth_device *dev)
 		if ((timeout--) == 0)
 			goto err_out;
 
-		if (smc911x_miiphy_read(dev, 1, MII_BMSR, &status) != 0)
+		if (smc911x_eth_phy_read(dev, 1, MII_BMSR, &status) != 0)
 			goto err_out;
 	} while (!(status & BMSR_LSTATUS));
 
@@ -235,6 +235,25 @@ static int smc911x_rx(struct eth_device *dev)
 	return 0;
 }
 
+#if defined(CONFIG_MII) || defined(CONFIG_CMD_MII)
+/* wrapper for smc911x_eth_phy_read */
+static int smc911x_miiphy_read(char *devname, u8 phy, u8 reg, u16 *val)
+{
+	struct eth_device *dev = eth_get_dev_by_name(devname);
+	if (dev)
+		return smc911x_eth_phy_read(dev, phy, reg, val);
+	return -1;
+}
+/* wrapper for smc911x_eth_phy_write */
+static int smc911x_miiphy_write(char *devname, u8 phy, u8 reg, u16 val)
+{
+	struct eth_device *dev = eth_get_dev_by_name(devname);
+	if (dev)
+		return smc911x_eth_phy_write(dev, phy, reg, val);
+	return -1;
+}
+#endif
+
 int smc911x_initialize(u8 dev_num, int base_addr)
 {
 	unsigned long addrl, addrh;
@@ -273,5 +292,10 @@ int smc911x_initialize(u8 dev_num, int base_addr)
 	sprintf(dev->name, "%s-%hu", DRIVERNAME, dev_num);
 
 	eth_register(dev);
+
+#if defined(CONFIG_MII) || defined(CONFIG_CMD_MII)
+	miiphy_register(dev->name, smc911x_miiphy_read, smc911x_miiphy_write);
+#endif
+
 	return 1;
 }
-- 
1.7.4.4



--
Scanned by MailScanner.

[U-Boot] [PATCH 1/2] net/eth.c: make eth_get_dev_by_name(NULL) safe

[Mike Frysinger]

On Monday, July 04, 2011 06:29:51 helmut.raiger at hale.at wrote:
> eth_get_dev_by_name() is not safe to use for devname being NULL
> as it uses strcmp. This patch makes it return NULL if devname NULL
> is passed.

i'm not sure about this.  passing NULL is wrong, and the caller should catch 
that shouldnt it ?
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20110704/83be5d2f/attachment.pgp 

[U-Boot] [PATCH 1/2] net/eth.c: make eth_get_dev_by_name(NULL) safe

[Helmut Raiger]

On 07/05/2011 05:44 AM, Mike Frysinger wrote:
> On Monday, July 04, 2011 06:29:51 helmut.raiger at hale.at wrote:
>> eth_get_dev_by_name() is not safe to use for devname being NULL
>> as it uses strcmp. This patch makes it return NULL if devname NULL
>> is passed.
> i'm not sure about this.  passing NULL is wrong, and the caller should catch
> that shouldnt it ?
> -mike
So what is your suggestion how to deal with it?

It returns:
"There is no ethernet device with name NULL"

This is pretty much the only thing it can return. The user of the 
function may handle this situation individually like:

printf("ethernet device '%s' not found\n, devname);
      --> "ethernet device '(NULL)' not found".

A panic on a NULL pointer de-reference is probably not helpful either.

Helmut


--
Scanned by MailScanner.

[U-Boot] [PATCH 1/2] net/eth.c: make eth_get_dev_by_name(NULL) safe

[Mike Frysinger]

On Wednesday, July 06, 2011 03:15:08 Helmut Raiger wrote:
> On 07/05/2011 05:44 AM, Mike Frysinger wrote:
> > On Monday, July 04, 2011 06:29:51 helmut.raiger at hale.at wrote:
> >> eth_get_dev_by_name() is not safe to use for devname being NULL
> >> as it uses strcmp. This patch makes it return NULL if devname NULL
> >> is passed.
> > 
> > i'm not sure about this.  passing NULL is wrong, and the caller should
> > catch that shouldnt it ?
> 
> So what is your suggestion how to deal with it?

in what situation is eth_get_dev_by_name(NULL) being called ?  my suggestion 
would be to fix that call point since it's doing something wrong.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20110706/1e9ee21b/attachment.pgp 

[U-Boot] [PATCH 1/2] net/eth.c: make eth_get_dev_by_name(NULL) safe

[Helmut Raiger]

On 07/06/2011 09:38 PM, Mike Frysinger wrote:
> On Wednesday, July 06, 2011 03:15:08 Helmut Raiger wrote:
>> On 07/05/2011 05:44 AM, Mike Frysinger wrote:
>>> On Monday, July 04, 2011 06:29:51 helmut.raiger at hale.at wrote:
>>>> eth_get_dev_by_name() is not safe to use for devname being NULL
>>>> as it uses strcmp. This patch makes it return NULL if devname NULL
>>>> is passed.
>>> i'm not sure about this.  passing NULL is wrong, and the caller should
>>> catch that shouldnt it ?
>> So what is your suggestion how to deal with it?
> in what situation is eth_get_dev_by_name(NULL) being called ?  my suggestion
> would be to fix that call point since it's doing something wrong.
> -mike
I couldn't find a situation where this might be the case. But as Luca 
Ceresoli pointed out in his e-mail, somewhere up the thread, that he 
tested for devname being NULL in his miiphy_read and write routines, I 
checked eth_get_dev_by_name() and found that it is vulnerable to passing 
a NULL pointer, hence the fix.

Is there something missing for the patch to be acknowledged?
It's hanging there quite a while now?

Helmut


--
Scanned by MailScanner.

[U-Boot] [PATCH 1/2] net/eth.c: make eth_get_dev_by_name(NULL) safe

[Detlev Zundel]

Hi Helmut,

> On 07/06/2011 09:38 PM, Mike Frysinger wrote:
>> On Wednesday, July 06, 2011 03:15:08 Helmut Raiger wrote:
>>> On 07/05/2011 05:44 AM, Mike Frysinger wrote:
>>>> On Monday, July 04, 2011 06:29:51 helmut.raiger at hale.at wrote:
>>>>> eth_get_dev_by_name() is not safe to use for devname being NULL
>>>>> as it uses strcmp. This patch makes it return NULL if devname NULL
>>>>> is passed.
>>>> i'm not sure about this.  passing NULL is wrong, and the caller should
>>>> catch that shouldnt it ?
>>> So what is your suggestion how to deal with it?
>> in what situation is eth_get_dev_by_name(NULL) being called ?  my suggestion
>> would be to fix that call point since it's doing something wrong.
>> -mike
> I couldn't find a situation where this might be the case. But as Luca 
> Ceresoli pointed out in his e-mail, somewhere up the thread, that he 
> tested for devname being NULL in his miiphy_read and write routines, I 
> checked eth_get_dev_by_name() and found that it is vulnerable to passing 
> a NULL pointer, hence the fix.
>
> Is there something missing for the patch to be acknowledged?
> It's hanging there quite a while now?

Actually I want to ack your patch.  eth_get_dev_by_name is a project
wide utility function and as such should be tolerant to being called
with wrong parameters.

Mike's argument of course also has merit to it, but because of the
"library function" argument, I'd give it less priority.  So

Acked-by: Detlev Zundel <dzu@denx.de>

-- 
We have a live-manual.  It's called emacs-devel at gnu.org.
You can stick to just reading it, but you can skip to a specific chapter
by simply sending an email asking for it ;-)
                                    -- Stefan Monnier
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de

[U-Boot] [PATCH 1/2] net/eth.c: make eth_get_dev_by_name(NULL) safe

[Albert ARIBAUD]

Hi Helmut,

Le 04/07/2011 12:29, helmut.raiger at hale.at a ?crit :
> From: Helmut Raiger<helmut.raiger@hale.at>

Seems like your git send-email config does not have your name along with 
your e-mail address, causing this From: to appear in the patch body (and 
the mail itself to lack your name) -- git can handle this, I think, but 
I'd be grateful if you fixed your config.

Amicalement,
-- 
Albert.

[U-Boot] [PATCH 1/2] net/eth.c: make eth_get_dev_by_name(NULL) safe

[Mike Frysinger]

On Thu, Jul 7, 2011 at 02:12, Helmut Raiger wrote:
> On 07/06/2011 09:38 PM, Mike Frysinger wrote:
>> On Wednesday, July 06, 2011 03:15:08 Helmut Raiger wrote:
>>> On 07/05/2011 05:44 AM, Mike Frysinger wrote:
>>>> On Monday, July 04, 2011 06:29:51 helmut.raiger at hale.at wrote:
>>>>> eth_get_dev_by_name() is not safe to use for devname being NULL
>>>>> as it uses strcmp. This patch makes it return NULL if devname NULL
>>>>> is passed.
>>>>
>>>> i'm not sure about this. ?passing NULL is wrong, and the caller should
>>>> catch that shouldnt it ?
>>>
>>> So what is your suggestion how to deal with it?
>>
>> in what situation is eth_get_dev_by_name(NULL) being called ? ?my
>> suggestion
>> would be to fix that call point since it's doing something wrong.
>
> I couldn't find a situation where this might be the case. But as Luca
> Ceresoli pointed out in his e-mail, somewhere up the thread, that he tested
> for devname being NULL in his miiphy_read and write routines, I checked
> eth_get_dev_by_name() and found that it is vulnerable to passing a NULL
> pointer, hence the fix.

those NULL checks should not be necessary either.  a correctly written
networking driver should only register itself with the miiphy layer
when it has successfully registered itself with the eth layer.  thus
any of the miiphy callbacks should always come in with a name that is
found via eth_get_dev_by_name().

checking for NULLs here and gracefully returning is unnecessary
overhead imo as you're only catering to broken code.  fix the broken
drivers instead.

by your logic, why put the NULL check in eth_get_dev_by_name() ?  why
not handle strcmp(NULL, NULL) too ?  then eth_get_dev_by_name() would
automatically get "fixed" as would all other call points.
-mike

[U-Boot] [PATCH 1/2] net/eth.c: make eth_get_dev_by_name(NULL) safe

[Helmut Raiger]

On 07/07/2011 07:46 PM, Mike Frysinger wrote:
>
> those NULL checks should not be necessary either.  a correctly written
> networking driver should only register itself with the miiphy layer
> when it has successfully registered itself with the eth layer.  thus
> any of the miiphy callbacks should always come in with a name that is
> found via eth_get_dev_by_name().
>
You are right, in a perfect world nobody ever falters.

> checking for NULLs here and gracefully returning is unnecessary
> overhead imo as you're only catering to broken code.  fix the broken
> drivers instead.

I could not find drivers that have the potential of delivering NULLs to 
the miiphy_read and write functions, I simply refused to test at  this 
level. Either there is a potential of having NULL passed then the test 
should be in eth_get_dev_by_name() or there is none then we should 
simply leave it away.
I'm rather indifferent to either solution.
And about 'catering to broken code': It must be broken in 2 ways, first 
it must pass a NULL to the function and second it must ignore the return 
code.

> by your logic, why put the NULL check in eth_get_dev_by_name() ?  why
> not handle strcmp(NULL, NULL) too ?  then eth_get_dev_by_name() would
> automatically get "fixed" as would all other call points.
>
For strcmp() you have several issues at hand: Compatibility, performance 
and even a logical problem. What should be the result in case one of the 
arguments is NULL (or both). The logic for eth_get_dev_by_name() is 
completely sane "There is no ethernet device named (NULL)", performance 
and compatibility don't matter. Your comparison is flawed.

And finally we are discussing a few _chararacters_ of code and a 
probably a single assembly instruction.

Helmut


--
Scanned by MailScanner.

[U-Boot] [PATCH 1/2] net/eth.c: make eth_get_dev_by_name(NULL) safe

[Helmut Raiger]

On 07/07/2011 06:46 PM, Albert ARIBAUD wrote:
> Hi Helmut,
>
> Le 04/07/2011 12:29, helmut.raiger at hale.at a ?crit :
>> From: Helmut Raiger<helmut.raiger@hale.at>
>
> Seems like your git send-email config does not have your name along 
> with your e-mail address, causing this From: to appear in the patch 
> body (and the mail itself to lack your name) -- git can handle this, I 
> think, but I'd be grateful if you fixed your config.
>
> Amicalement,
Hi Albert,

     I just checked my config, user and e-mail were fine, but I had the 
from configured in the sendemail section which obviously generates the 
'From:' line. Do you like me to resend the patches?

Helmut



--
Scanned by MailScanner.

[U-Boot] [PATCH 1/2] net/eth.c: make eth_get_dev_by_name(NULL) safe

[Mike Frysinger]

On Monday, July 11, 2011 05:53:49 Helmut Raiger wrote:
> On 07/07/2011 07:46 PM, Mike Frysinger wrote:
> > those NULL checks should not be necessary either.  a correctly written
> > networking driver should only register itself with the miiphy layer
> > when it has successfully registered itself with the eth layer.  thus
> > any of the miiphy callbacks should always come in with a name that is
> > found via eth_get_dev_by_name().
> 
> You are right, in a perfect world nobody ever falters.

you missed the point.  either the driver works, or it doesnt.  this func is 
used in such a way that the behavior is fairly deterministic (as i clearly 
laid out).  it isnt relying on allocated memory that could fall to be 
allocated for example.

> > checking for NULLs here and gracefully returning is unnecessary
> > overhead imo as you're only catering to broken code.  fix the broken
> > drivers instead.
> 
> I could not find drivers that have the potential of delivering NULLs to
> the miiphy_read and write functions, I simply refused to test at  this
> level. Either there is a potential of having NULL passed then the test
> should be in eth_get_dev_by_name() or there is none then we should
> simply leave it away.

i did go through the level of detail and showed the call graphs ... none of 
which should allow a driver tested as working to even once hit the NULL path.

> I'm rather indifferent to either solution.
> And about 'catering to broken code': It must be broken in 2 ways, first
> it must pass a NULL to the function and second it must ignore the return
> code.

not necessarily.  many platforms will abort on NULL pointer accesses.

> > by your logic, why put the NULL check in eth_get_dev_by_name() ?  why
> > not handle strcmp(NULL, NULL) too ?  then eth_get_dev_by_name() would
> > automatically get "fixed" as would all other call points.
> 
> For strcmp() you have several issues at hand: Compatibility, performance
> and even a logical problem. What should be the result in case one of the
> arguments is NULL (or both).

fair enough on the API, but your performance aspect is kind of hard to swallow 
when you turn around and say that NULL pointer checking elsewhere is 
minuscule.

> And finally we are discussing a few _chararacters_ of code and a
> probably a single assembly instruction.

it's not a single assembly insn.  try generating it.  it adds like 8 to my 
platform, mostly because of the increased register pressure.

but the point isnt the impact of this single check.  it sets the precedence 
that every function in u-boot that takes a pointer should start over 
protecting itself against poorly written code originating elsewhere.  now your 
"few characters" is quite a bit more.

what i wouldnt mind is annotating the prototype with gcc attributes saying 
that the argument is nonnull.
...
#define __nonnull(x) __attribute__((__nonnull__ x))
...
extern struct eth_device *eth_get_dev_by_name(const char *devname) 
__nonnull(1);
...
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20110712/11806c4f/attachment.pgp 

[U-Boot] [PATCH 1/2] net/eth.c: make eth_get_dev_by_name(NULL) safe

[Detlev Zundel]

Hi Mike,

> On Monday, July 11, 2011 05:53:49 Helmut Raiger wrote:
>> On 07/07/2011 07:46 PM, Mike Frysinger wrote:
>> > those NULL checks should not be necessary either.  a correctly written
>> > networking driver should only register itself with the miiphy layer
>> > when it has successfully registered itself with the eth layer.  thus
>> > any of the miiphy callbacks should always come in with a name that is
>> > found via eth_get_dev_by_name().
>> 
>> You are right, in a perfect world nobody ever falters.
>
> you missed the point.  either the driver works, or it doesnt.  this func is 
> used in such a way that the behavior is fairly deterministic (as i clearly 
> laid out).  it isnt relying on allocated memory that could fall to be 
> allocated for example.

I for myself am also thinking of code that will be written in the
future, i.e. probably new uses of eth_get_dev_by_name.  Saving time in
this development because errors are caught early is a good thing IMHO.

>> > checking for NULLs here and gracefully returning is unnecessary
>> > overhead imo as you're only catering to broken code.  fix the broken
>> > drivers instead.
>> 
>> I could not find drivers that have the potential of delivering NULLs to
>> the miiphy_read and write functions, I simply refused to test at  this
>> level. Either there is a potential of having NULL passed then the test
>> should be in eth_get_dev_by_name() or there is none then we should
>> simply leave it away.
>
> i did go through the level of detail and showed the call graphs ... none of 
> which should allow a driver tested as working to even once hit the NULL path.

As I said, these are tha call graphs currently existing...

>> I'm rather indifferent to either solution.
>> And about 'catering to broken code': It must be broken in 2 ways, first
>> it must pass a NULL to the function and second it must ignore the return
>> code.
>
> not necessarily.  many platforms will abort on NULL pointer accesses.
>
>> > by your logic, why put the NULL check in eth_get_dev_by_name() ?  why
>> > not handle strcmp(NULL, NULL) too ?  then eth_get_dev_by_name() would
>> > automatically get "fixed" as would all other call points.
>> 
>> For strcmp() you have several issues at hand: Compatibility, performance
>> and even a logical problem. What should be the result in case one of the
>> arguments is NULL (or both).
>
> fair enough on the API, but your performance aspect is kind of hard to swallow 
> when you turn around and say that NULL pointer checking elsewhere is 
> minuscule.
>
>> And finally we are discussing a few _chararacters_ of code and a
>> probably a single assembly instruction.
>
> it's not a single assembly insn.  try generating it.  it adds like 8 to my 
> platform, mostly because of the increased register pressure.

On PowerPC with ELDK 4.2 this is the difference:

before:

00000004 g     F .text.eth_get_dev_by_name      00000080 eth_get_dev_by_name

after:

00000004 g     F .text.eth_get_dev_by_name      00000084 eth_get_dev_by_name


So at least for this arch it is indeed one word difference.

> but the point isnt the impact of this single check.  it sets the
> precedence that every function in u-boot that takes a pointer should
> start over protecting itself against poorly written code originating
> elsewhere.  now your "few characters" is quite a bit more.

I still stand by what I said that if we have functions that can be
called from many places (i.e. "library"-like), then the functions should
be conservative in what they expect.  Tightly coupled code can be looser
in this respect.  Maybe our disagreement stems from the fact that you
consider this function to be "tightly coupled" and not really library
like?

> what i wouldnt mind is annotating the prototype with gcc attributes saying 
> that the argument is nonnull.
> ...
> #define __nonnull(x) __attribute__((__nonnull__ x))
> ...
> extern struct eth_device *eth_get_dev_by_name(const char *devname) 
> __nonnull(1);
> ...

This can only catch calls the compiler can statically derive, but still
I think it is a good thing.

Cheers
  Detlev

-- 
Don't trust everything you read, and don't assume every poster in
a thread is actually relevant to the problem.
        -- Stefan Monnier <jwvlj1gk44h.fsf-monnier+emacs@gnu.org>
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de

[U-Boot] [PATCH 1/2] net/eth.c: make eth_get_dev_by_name(NULL) safe

[Mike Frysinger]

On Tue, Jul 12, 2011 at 05:22, Detlev Zundel wrote:
> Mike Frysinger wrote:
>> but the point isnt the impact of this single check. ?it sets the
>> precedence that every function in u-boot that takes a pointer should
>> start over protecting itself against poorly written code originating
>> elsewhere. ?now your "few characters" is quite a bit more.
>
> I still stand by what I said that if we have functions that can be
> called from many places (i.e. "library"-like), then the functions should
> be conservative in what they expect. ?Tightly coupled code can be looser
> in this respect. ?Maybe our disagreement stems from the fact that you
> consider this function to be "tightly coupled" and not really library
> like?

not really.  i consider this to be "garbage-in garbage-out".  imo,
u-boot isnt a C library that should be padded with garbage checking
all over.  the result only helps broken systems (edge cases) while
hindering the rest.

i wouldnt have a problem with adopting an NDEBUG system, or perhaps
adding assert()'s to this code.  then people can easily opt-out of it
all and for the people doing development, can easily turn things on.
    assert(name != NULL);

the current miiphy system needs to be replaced (this runtime string
based approach is crazy), but that's a completely different topic :).
-mike

[U-Boot] [PATCH 1/2] net/eth.c: make eth_get_dev_by_name(NULL) safe

[Helmut Raiger]

On 07/12/2011 11:22 AM, Detlev Zundel wrote:

> > i did go through the level of detail and showed the call graphs ...
> > none of
> > which should allow a driver tested as working to even once hit the
> > NULL path.
>
>  As I said, these are the call graphs currently existing...

This was also my trail.

> > what i wouldnt mind is annotating the prototype with gcc attributes
> > saying that the argument is nonnull. ... #define __nonnull(x)
> > __attribute__((__nonnull__ x)) ... extern struct eth_device
> > *eth_get_dev_by_name(const char *devname) __nonnull(1); ...
>
>  This can only catch calls the compiler can statically derive, but
>  still I think it is a good thing.
>

     __nonnull__ is actually a optimization attribute, gcc removes tests 
for NULL in the function body, warnings are only generated if one 
literally writes: eth_get_dev_by_name(NULL), so 'statically derive'
is already exageration.
This really is no help at all. It would indeed establish a precendence 
to using an IMHO quite flawed attribute in gcc. If I had a vote, I'd be 
against it.

The NDEBUG approach however, as Mike suggested,  was what I was looking 
for in the first place.

Helmut


--
Scanned by MailScanner.

[U-Boot] [PATCH 1/2] net/eth.c: make eth_get_dev_by_name(NULL) safe

[Detlev Zundel]

Hi Mike,

[...]

> not really.  i consider this to be "garbage-in garbage-out".  imo,
> u-boot isnt a C library that should be padded with garbage checking
> all over.  the result only helps broken systems (edge cases) while
> hindering the rest.
>
> i wouldnt have a problem with adopting an NDEBUG system, or perhaps
> adding assert()'s to this code.  then people can easily opt-out of it
> all and for the people doing development, can easily turn things on.
>     assert(name != NULL);

While developing, I certainly appreciate 'garbage-in error-out' and as
your approach allows this, I believe we have reached a consensus.

> the current miiphy system needs to be replaced (this runtime string
> based approach is crazy), but that's a completely different topic :).

I'm looking forward to your changes :)

Thanks!
  Detlev

-- 
By all means let's be open-minded, but not so open-minded that our
brains drop out.
                                                -- Richard Dawkins
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de

[U-Boot] [PATCH 1/2] net/eth.c: make eth_get_dev_by_name(NULL) safe

[Detlev Zundel]

Hi Helmut,

> On 07/12/2011 11:22 AM, Detlev Zundel wrote:
>
>> > i did go through the level of detail and showed the call graphs ...
>> > none of
>> > which should allow a driver tested as working to even once hit the
>> > NULL path.
>>
>>  As I said, these are the call graphs currently existing...
>
> This was also my trail.
>
>> > what i wouldnt mind is annotating the prototype with gcc attributes
>> > saying that the argument is nonnull. ... #define __nonnull(x)
>> > __attribute__((__nonnull__ x)) ... extern struct eth_device
>> > *eth_get_dev_by_name(const char *devname) __nonnull(1); ...
>>
>>  This can only catch calls the compiler can statically derive, but
>>  still I think it is a good thing.
>>
>
>     __nonnull__ is actually a optimization attribute, gcc removes
> tests for NULL in the function body, warnings are only generated if
> one literally writes: eth_get_dev_by_name(NULL), so 'statically
> derive'
> is already exageration.

I just checked and can confirm that currently gcc does not do any static
analysis of char* arguments - however in theory it could.

> This really is no help at all. It would indeed establish a precendence
> to using an IMHO quite flawed attribute in gcc. If I had a vote, I'd
> be against it.

I agree that how this is implemented in gcc is no big help.  Rather than
believing documentation I should have checked how this works before
lobbying for it.

> The NDEBUG approach however, as Mike suggested,  was what I was
> looking for in the first place.

Great!
  Detlev

-- 
<ESC>:!emacs %
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de

[U-Boot] [PATCH 1/2] net/eth.c: make eth_get_dev_by_name(NULL) safe

[Helmut Raiger]

On 07/13/2011 01:46 PM, Detlev Zundel wrote:
>
>> The NDEBUG approach however, as Mike suggested,  was what I was
>> looking for in the first place.
> Great!
>    Detlev
>

Again, not so great. U-boot uses all kinds of assert(), BUG_ON(), 
ASSERT() all over the place.
This probably needs a project wide effort, which is why I simply threw 
in my NULL pointer check.

Helmut


--
Scanned by MailScanner.

[U-Boot] [PATCH 1/2] net/eth.c: make eth_get_dev_by_name(NULL) safe

[Albert ARIBAUD]

Hi Helmut,

Le 11/07/2011 12:10, Helmut Raiger a ?crit :
> On 07/07/2011 06:46 PM, Albert ARIBAUD wrote:
>> Hi Helmut,
>>
>> Le 04/07/2011 12:29, helmut.raiger at hale.at a ?crit :
>>> From: Helmut Raiger<helmut.raiger@hale.at>
>>
>> Seems like your git send-email config does not have your name along
>> with your e-mail address, causing this From: to appear in the patch
>> body (and the mail itself to lack your name) -- git can handle this, I
>> think, but I'd be grateful if you fixed your config.
>>
>> Amicalement,
> Hi Albert,
>
> I just checked my config, user and e-mail were fine, but I had the from
> configured in the sendemail section which obviously generates the
> 'From:' line. Do you like me to resend the patches?

Not needed for this patch (if that From: is ok with patchwork and Git) 
but if you send a V2 patch or for future patches, please do avoid the 
added From.

Amicalement,
-- 
Albert.

[U-Boot] [PATCH 1/2] net/eth.c: make eth_get_dev_by_name(NULL) safe

[Mike Frysinger]

On Thursday, July 14, 2011 05:14:07 Helmut Raiger wrote:
> On 07/13/2011 01:46 PM, Detlev Zundel wrote:
> >> The NDEBUG approach however, as Mike suggested,  was what I was
> >> looking for in the first place.
> 
> Again, not so great. U-boot uses all kinds of assert(), BUG_ON(),
> ASSERT() all over the place.
> This probably needs a project wide effort, which is why I simply threw
> in my NULL pointer check.

we tend to look at the long term picture with the best/correct solution rather 
than one-offs which get thrown away in the end
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20110714/b9de94e6/attachment.pgp 

[U-Boot] [PATCH 1/2] net/eth.c: make eth_get_dev_by_name(NULL) safe

[Mike Frysinger]

On Wednesday, July 13, 2011 02:32:37 Helmut Raiger wrote:

for future reference, please dont send html e-mails
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20110714/c9780fb3/attachment.pgp 

[U-Boot] [PATCH V2 1/2] net/eth.c: throw BUG for eth_get_dev_by_name(NULL)

[Helmut Raiger]

eth_get_dev_by_name() is not safe to use for devname being NULL
as it uses strcmp. This patch makes it fail with a BUG().

Signed-off-by: Helmut Raiger <helmut.raiger@hale.at>
---
V2: use BUG_ON() instead of gracefully returning 0

 net/eth.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/net/eth.c b/net/eth.c
index 6523834..25c147c 100644
--- a/net/eth.c
+++ b/net/eth.c
@@ -107,6 +107,8 @@ struct eth_device *eth_get_dev_by_name(const char *devname)
 {
 	struct eth_device *dev, *target_dev;
 
+	BUG_ON(devname == 0);
+
 	if (!eth_devices)
 		return NULL;
 
-- 
1.7.4.4



--
Scanned by MailScanner.

[U-Boot] [PATCH V2 1/2] net/eth.c: throw BUG for eth_get_dev_by_name(NULL)

[Sergei Shtylyov]

Hello.

On 22-08-2011 12:45, Helmut Raiger wrote:

> eth_get_dev_by_name() is not safe to use for devname being NULL
> as it uses strcmp. This patch makes it fail with a BUG().

> Signed-off-by: Helmut Raiger<helmut.raiger@hale.at>
> ---
> V2: use BUG_ON() instead of gracefully returning 0

>   net/eth.c |    2 ++
>   1 files changed, 2 insertions(+), 0 deletions(-)

> diff --git a/net/eth.c b/net/eth.c
> index 6523834..25c147c 100644
> --- a/net/eth.c
> +++ b/net/eth.c
> @@ -107,6 +107,8 @@ struct eth_device *eth_get_dev_by_name(const char *devname)
>   {
>   	struct eth_device *dev, *target_dev;
>
> +	BUG_ON(devname == 0);

    Do not use 0 instead of NULL.

WBR, Sergei

[U-Boot] [PATCH 1/2] net/eth.c: throw BUG for eth_get_dev_by_name(NULL)

[Helmut Raiger]

eth_get_dev_by_name() is not safe to use for devname being NULL
as it uses strcmp. This patch makes it fail with a BUG().

Signed-off-by: Helmut Raiger <helmut.raiger@hale.at>
---
V2: use BUG_ON() instead of gracefully returning 0

 net/eth.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/net/eth.c b/net/eth.c
index 6523834..deb41ba 100644
--- a/net/eth.c
+++ b/net/eth.c
@@ -107,6 +107,8 @@ struct eth_device *eth_get_dev_by_name(const char *devname)
 {
 	struct eth_device *dev, *target_dev;
 
+	BUG_ON(devname == NULL);
+
 	if (!eth_devices)
 		return NULL;
 
-- 
1.7.4.4



--
Scanned by MailScanner.

[U-Boot] [PATCH 1/2] net/eth.c: throw BUG for eth_get_dev_by_name(NULL)

[Mike Frysinger]

On Monday, August 22, 2011 06:17:17 Helmut Raiger wrote:
> eth_get_dev_by_name() is not safe to use for devname being NULL
> as it uses strcmp. This patch makes it fail with a BUG().

Acked-by: Mike Frysinger <vapier@gentoo.org>
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20110822/d1a3f838/attachment.pgp 

[U-Boot] Anything missing?

[Helmut Raiger]

Is there anything missing for this patch to be accepted?

Helmut



--
Scanned by MailScanner.

[U-Boot] Anything missing?

[Wolfgang Denk]

Dear Helmut Raiger,

In message <4E5DE5A2.70603@hale.at> you wrote:
> Is there anything missing for this patch to be accepted?

Yes, definitely.

for example, you fail to mention which patch you are talking about.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Quote from the Boss... "I didn't say it was your fault.  I said I was
going to blame it on you."

[U-Boot] [U-Boot PATCH MX31:] smc911x MII made available, ping?

[Helmut Raiger]

This is sitting here for more than 2 months, could someone please ACK 
and/or apply.

Helmut



--
Scanned by MailScanner.

[U-Boot] [U-Boot,2/2] smc911x MII made available

[Stefano Babic]

On 01/-10/-28163 08:59 PM, Helmut Raiger wrote:
> From: Helmut Raiger <helmut.raiger@hale.at>
> 
> The driver already had the MII functions, but they have not been
> registered using miiphy_register().
> 
> Signed-off-by: Helmut Raiger <helmut.raiger@hale.at>
> 

As for V1, tested on a MX35.

Tested-by: Stefano Babic <sbabic@denx.de>

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================

[U-Boot] [U-Boot PATCH MX31:] smc911x MII made available, ping?

[Stefano Babic]

On 09/07/2011 07:40 AM, Helmut Raiger wrote:

Hi Helmut,

> This is sitting here for more than 2 months, could someone please ACK 
> and/or apply.

I think that one reason for the delay is that you do not add the network
maintainer (Wolfgang) as CC in V2 of your patch - I missed also this
patch, and I send now after testing my tested-by as I did for V1.

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================

[U-Boot] [U-Boot,2/2] smc911x MII made available

[Helmut Raiger]

On 09/07/2011 02:33 PM, Stefano Babic wrote:
> On 01/-10/-28163 08:59 PM, Helmut Raiger wrote:
>> From: Helmut Raiger<helmut.raiger@hale.at>
>>
>> The driver already had the MII functions, but they have not been
>> registered using miiphy_register().
>>
>> Signed-off-by: Helmut Raiger<helmut.raiger@hale.at>
>>
> As for V1, tested on a MX35.
>
> Tested-by: Stefano Babic<sbabic@denx.de>
>
> Best regards,
> Stefano Babic
>
Thx Stefano, I do not know how to handle Tested-by: tags, however.
Helmut



--
Scanned by MailScanner.

[U-Boot] [U-Boot,2/2] smc911x MII made available

[Stefano Babic]

On 09/07/2011 03:06 PM, Helmut Raiger wrote:

>>
> Thx Stefano, I do not know how to handle Tested-by: tags, however.

Do not worry: you have not to handle. These tags are automatically
inserted by patchworks in a patch by downloading and this helps the
maintainer who does not need to insert them manually before applying the
patch to the tree.

As submitter, nothing is requested to you to handle them ;-).

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================

[U-Boot] [U-Boot PATCH MX31:] smc911x MII made available, ping?

[Wolfgang Denk]

Dear Stefano Babic,

In message <4E676571.5090003@denx.de> you wrote:
> On 09/07/2011 07:40 AM, Helmut Raiger wrote:
> 
> Hi Helmut,
> 
> > This is sitting here for more than 2 months, could someone please ACK 
> > and/or apply.
> 
> I think that one reason for the delay is that you do not add the network
> maintainer (Wolfgang) as CC in V2 of your patch - I missed also this
> patch, and I send now after testing my tested-by as I did for V1.

Even more so: the patch is flagged as for MX31, so I don;t even look
at it.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
He'd been wrong, there _was_ a light at the end of the tunnel, and it
was a flamethrower.                         - Terry Pratchett, _Mort_

[U-Boot] [PATCH 2/2] smc911x MII made available

[Wolfgang Denk]

Dear helmut.raiger at hale.at,

In message <1309775392-8282-2-git-send-email-helmut.raiger@hale.at> you wrote:
> From: Helmut Raiger <helmut.raiger@hale.at>
> 
> The driver already had the MII functions, but they have not been
> registered using miiphy_register().
> 
> Signed-off-by: Helmut Raiger <helmut.raiger@hale.at>
> ---
>  drivers/net/smc911x.c |   36 ++++++++++++++++++++++++++++++------
>  1 files changed, 30 insertions(+), 6 deletions(-)

Applied, thanks.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Respect is a rational process
	-- McCoy, "The Galileo Seven", stardate 2822.3

[U-Boot] [U-Boot PATCH MX31:] smc911x MII made available, ping?

[Helmut Raiger]

On 09/07/2011 11:47 PM, Wolfgang Denk wrote:
> Dear Stefano Babic,
>
> In message<4E676571.5090003@denx.de>  you wrote:
>> On 09/07/2011 07:40 AM, Helmut Raiger wrote:
>>
>> Hi Helmut,
>>
>>> This is sitting here for more than 2 months, could someone please ACK
>>> and/or apply.
>> I think that one reason for the delay is that you do not add the network
>> maintainer (Wolfgang) as CC in V2 of your patch - I missed also this
>> patch, and I send now after testing my tested-by as I did for V1.
> Even more so: the patch is flagged as for MX31, so I don;t even look
> at it.
>
> Best regards,
>
> Wolfgang Denk
>
Damn, yes, my fault. In the heat of the action ...
Thx, Helmut



--
Scanned by MailScanner.

[U-Boot] [PATCH] smc911x: Fix build warnings

[Wolfgang Denk]

Commit 6af1d41 "smc911x MII made available" was missing a few "const"
qualifiers.  Fix the resulting in build warnings:

smc911x.c: In function 'smc911x_initialize':
smc911x.c:297: warning: passing argument 2 of 'miiphy_register' from incompatible pointer type
smc911x.c:297: warning: passing argument 3 of 'miiphy_register' from incompatible pointer type

Signed-off-by: Wolfgang Denk <wd@denx.de>
Cc: Helmut Raiger <helmut.raiger@hale.at>
---
 drivers/net/smc911x.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/smc911x.c b/drivers/net/smc911x.c
index 6cc236c..a677fd4 100644
--- a/drivers/net/smc911x.c
+++ b/drivers/net/smc911x.c
@@ -237,7 +237,7 @@ static int smc911x_rx(struct eth_device *dev)
 
 #if defined(CONFIG_MII) || defined(CONFIG_CMD_MII)
 /* wrapper for smc911x_eth_phy_read */
-static int smc911x_miiphy_read(char *devname, u8 phy, u8 reg, u16 *val)
+static int smc911x_miiphy_read(const char *devname, u8 phy, u8 reg, u16 *val)
 {
 	struct eth_device *dev = eth_get_dev_by_name(devname);
 	if (dev)
@@ -245,7 +245,7 @@ static int smc911x_miiphy_read(char *devname, u8 phy, u8 reg, u16 *val)
 	return -1;
 }
 /* wrapper for smc911x_eth_phy_write */
-static int smc911x_miiphy_write(char *devname, u8 phy, u8 reg, u16 val)
+static int smc911x_miiphy_write(const char *devname, u8 phy, u8 reg, u16 val)
 {
 	struct eth_device *dev = eth_get_dev_by_name(devname);
 	if (dev)
-- 
1.7.6

[U-Boot] [PATCH 1/2] net/eth.c: throw BUG for eth_get_dev_by_name(NULL)

[Wolfgang Denk]

Dear Helmut Raiger,

In message <1314008237-24180-1-git-send-email-helmut.raiger@hale.at> you wrote:
> eth_get_dev_by_name() is not safe to use for devname being NULL
> as it uses strcmp. This patch makes it fail with a BUG().
> 
> Signed-off-by: Helmut Raiger <helmut.raiger@hale.at>
> ---
> V2: use BUG_ON() instead of gracefully returning 0
> 
>  net/eth.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)

Applied, thanks.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"...all the  good  computer  designs  are  bootlegged;  the  formally
planned  products,  if  they  are built at all, are dogs!" - David E.
Lundstrom, "A Few Good Men From Univac", MIT Press, 1987

[U-Boot] [PATCH] smc911x: Fix build warnings

[Wolfgang Denk]

Dear Wolfgang Denk,

In message <1315479867-23506-1-git-send-email-wd@denx.de> you wrote:
> Commit 6af1d41 "smc911x MII made available" was missing a few "const"
> qualifiers.  Fix the resulting in build warnings:
> 
> smc911x.c: In function 'smc911x_initialize':
> smc911x.c:297: warning: passing argument 2 of 'miiphy_register' from incompatible pointer type
> smc911x.c:297: warning: passing argument 3 of 'miiphy_register' from incompatible pointer type
> 
> Signed-off-by: Wolfgang Denk <wd@denx.de>
> Cc: Helmut Raiger <helmut.raiger@hale.at>
> ---
>  drivers/net/smc911x.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)

Applied, thanks.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
You!  What PLANET is this!
	-- McCoy, "The City on the Edge of Forever", stardate 3134.0