smc911x MII made available

From: Helmut Raiger <[email protected]>

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(-)

index aeafeba..9378a63 100644
@@ -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.

Hi Helmut,

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.

=====================================================================
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
=====================================================================

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'

Ok, I did search the MAINTAINER file, but could not attach a name to my fix, neither smc... nor network, nor something else.

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.

Thanks for testing,
Helmut

--
Scanned by MailScanner.

From: Helmut Raiger <[email protected]>

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(-)

index aeafeba..8753588 100644
@@ -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.

missing signed-off-by tag

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

From: Helmut Raiger <[email protected]>

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

---
 drivers/net/smc911x.c |   36 ++++++++++++++++++++++++++++++------
 1 files changed, 30 insertions(+), 6 deletions(-)

index aeafeba..6cc236c 100644
@@ -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.

Hi Helmut,

[...]

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.

---
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(-)

index aeafeba..d946fd4 100644
@@ -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.

From: Helmut Raiger <[email protected]>

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.

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

index 6523834..c75b7c9 100644
@@ -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.

From: Helmut Raiger <[email protected]>

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

---
 drivers/net/smc911x.c |   36 ++++++++++++++++++++++++++++++------
 1 files changed, 30 insertions(+), 6 deletions(-)

index aeafeba..6cc236c 100644
@@ -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.

As for V1, tested on a MX35.

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
=====================================================================

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

--
Scanned by MailScanner.

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
=====================================================================

Dear helmut.raiger at hale.at,

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

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

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

index 6cc236c..a677fd4 100644
@@ -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

Dear Wolfgang Denk,

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

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

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.

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

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.

Hi Helmut,

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

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

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

You are right, in a perfect world nobody ever falters.

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.

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.

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 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.

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

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.

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

Hi Mike,

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.

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

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.

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?

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 <[email protected]>
--
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

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

Hi Mike,

[...]

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

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

This was also my trail.

__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.

Hi Helmut,

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

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.

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

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.

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

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().

---
V2: use BUG_ON() instead of gracefully returning 0

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

index 6523834..25c147c 100644
@@ -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.

Hello.

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().

---
V2: use BUG_ON() instead of gracefully returning 0

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

index 6523834..deb41ba 100644
@@ -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.

-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

Dear Helmut Raiger,

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

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

Hi Helmut,

Le 04/07/2011 12:29, helmut.raiger at hale.at a ?crit :

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.

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.

Hi Helmut,

Le 11/07/2011 12:10, Helmut Raiger a ?crit :

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.

Is there anything missing for this patch to be accepted?

Helmut

--
Scanned by MailScanner.

Dear Helmut Raiger,

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."

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

Helmut

--
Scanned by MailScanner.

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

Hi Helmut,

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
=====================================================================

Dear Stefano Babic,

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_

Damn, yes, my fault. In the heat of the action ... Thx, Helmut

--
Scanned by MailScanner.