* [U-Boot] [U-Boot PATCH MX31:] smc911x MII made available
@ 2011-06-20 6:10 Helmut.Raiger at hale.at
2011-06-20 12:30 ` Stefano Babic
` (2 more replies)
0 siblings, 3 replies; 47+ messages in thread
From: Helmut.Raiger at hale.at @ 2011-06-20 6:10 UTC (permalink / raw)
To: u-boot
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.
^ permalink raw reply related [flat|nested] 47+ messages in thread* [U-Boot] [U-Boot PATCH MX31:] smc911x MII made available 2011-06-20 6:10 [U-Boot] [U-Boot PATCH MX31:] smc911x MII made available Helmut.Raiger at hale.at @ 2011-06-20 12:30 ` Stefano Babic 2011-06-20 14:36 ` Helmut Raiger 2011-06-27 7:22 ` [U-Boot] [PATCH] " helmut.raiger at hale.at 2011-08-31 7:41 ` [U-Boot] Anything missing? Helmut Raiger 2011-09-07 5:40 ` [U-Boot] [U-Boot PATCH MX31:] smc911x MII made available, ping? Helmut Raiger 2 siblings, 2 replies; 47+ messages in thread From: Stefano Babic @ 2011-06-20 12:30 UTC (permalink / raw) To: u-boot 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 ===================================================================== ^ permalink raw reply [flat|nested] 47+ messages in thread
* [U-Boot] [U-Boot PATCH MX31:] smc911x MII made available 2011-06-20 12:30 ` Stefano Babic @ 2011-06-20 14:36 ` Helmut Raiger 2011-06-27 7:22 ` [U-Boot] [PATCH] " helmut.raiger at hale.at 1 sibling, 0 replies; 47+ messages in thread From: Helmut Raiger @ 2011-06-20 14:36 UTC (permalink / raw) To: u-boot 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. ^ permalink raw reply [flat|nested] 47+ messages in thread
* [U-Boot] [PATCH] smc911x MII made available 2011-06-20 12:30 ` Stefano Babic 2011-06-20 14:36 ` Helmut Raiger @ 2011-06-27 7:22 ` helmut.raiger at hale.at 2011-06-27 19:29 ` Mike Frysinger 1 sibling, 1 reply; 47+ messages in thread From: helmut.raiger at hale.at @ 2011-06-27 7:22 UTC (permalink / raw) To: u-boot 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. ^ permalink raw reply related [flat|nested] 47+ messages in thread
* [U-Boot] [PATCH] smc911x MII made available 2011-06-27 7:22 ` [U-Boot] [PATCH] " helmut.raiger at hale.at @ 2011-06-27 19:29 ` Mike Frysinger 2011-06-29 10:12 ` helmut.raiger at hale.at 0 siblings, 1 reply; 47+ messages in thread From: Mike Frysinger @ 2011-06-27 19:29 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 47+ messages in thread
* [U-Boot] [PATCH] smc911x MII made available 2011-06-27 19:29 ` Mike Frysinger @ 2011-06-29 10:12 ` helmut.raiger at hale.at 2011-06-30 13:57 ` Luca Ceresoli 0 siblings, 1 reply; 47+ messages in thread From: helmut.raiger at hale.at @ 2011-06-29 10:12 UTC (permalink / raw) To: u-boot 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. ^ permalink raw reply related [flat|nested] 47+ messages in thread
* [U-Boot] [PATCH] smc911x MII made available 2011-06-29 10:12 ` helmut.raiger at hale.at @ 2011-06-30 13:57 ` Luca Ceresoli 2011-06-30 14:02 ` [U-Boot] [PATCH RFC] smc911x: enable mii commands Luca Ceresoli 0 siblings, 1 reply; 47+ messages in thread From: Luca Ceresoli @ 2011-06-30 13:57 UTC (permalink / raw) To: u-boot 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. ^ permalink raw reply [flat|nested] 47+ messages in thread
* [U-Boot] [PATCH RFC] smc911x: enable mii commands 2011-06-30 13:57 ` Luca Ceresoli @ 2011-06-30 14:02 ` Luca Ceresoli 2011-07-04 9:41 ` Helmut Raiger 0 siblings, 1 reply; 47+ messages in thread From: Luca Ceresoli @ 2011-06-30 14:02 UTC (permalink / raw) To: u-boot 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. ^ permalink raw reply related [flat|nested] 47+ messages in thread
* [U-Boot] [PATCH RFC] smc911x: enable mii commands 2011-06-30 14:02 ` [U-Boot] [PATCH RFC] smc911x: enable mii commands Luca Ceresoli @ 2011-07-04 9:41 ` Helmut Raiger 2011-07-04 10:29 ` [U-Boot] [PATCH 1/2] net/eth.c: make eth_get_dev_by_name(NULL) safe helmut.raiger at hale.at 0 siblings, 1 reply; 47+ messages in thread From: Helmut Raiger @ 2011-07-04 9:41 UTC (permalink / raw) To: u-boot 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. ^ permalink raw reply [flat|nested] 47+ messages in thread
* [U-Boot] [PATCH 1/2] net/eth.c: make eth_get_dev_by_name(NULL) safe 2011-07-04 9:41 ` Helmut Raiger @ 2011-07-04 10:29 ` helmut.raiger at hale.at 2011-07-04 10:29 ` [U-Boot] [PATCH 2/2] smc911x MII made available helmut.raiger at hale.at ` (2 more replies) 0 siblings, 3 replies; 47+ messages in thread From: helmut.raiger at hale.at @ 2011-07-04 10:29 UTC (permalink / raw) To: u-boot 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. ^ permalink raw reply related [flat|nested] 47+ messages in thread
* [U-Boot] [PATCH 2/2] smc911x MII made available 2011-07-04 10:29 ` [U-Boot] [PATCH 1/2] net/eth.c: make eth_get_dev_by_name(NULL) safe helmut.raiger at hale.at @ 2011-07-04 10:29 ` helmut.raiger at hale.at 2011-09-07 12:33 ` [U-Boot] [U-Boot,2/2] " Stefano Babic ` (2 more replies) 2011-07-05 3:44 ` [U-Boot] [PATCH 1/2] net/eth.c: make eth_get_dev_by_name(NULL) safe Mike Frysinger 2011-07-07 16:46 ` Albert ARIBAUD 2 siblings, 3 replies; 47+ messages in thread From: helmut.raiger at hale.at @ 2011-07-04 10:29 UTC (permalink / raw) To: u-boot 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. ^ permalink raw reply related [flat|nested] 47+ messages in thread
* [U-Boot] [U-Boot,2/2] smc911x MII made available 2011-07-04 10:29 ` [U-Boot] [PATCH 2/2] smc911x MII made available helmut.raiger at hale.at @ 2011-09-07 12:33 ` Stefano Babic 2011-09-07 13:06 ` Helmut Raiger 2011-09-07 21:50 ` [U-Boot] [PATCH 2/2] " Wolfgang Denk 2011-09-08 11:04 ` [U-Boot] [PATCH] smc911x: Fix build warnings Wolfgang Denk 2 siblings, 1 reply; 47+ messages in thread From: Stefano Babic @ 2011-09-07 12:33 UTC (permalink / raw) To: u-boot 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 ===================================================================== ^ permalink raw reply [flat|nested] 47+ messages in thread
* [U-Boot] [U-Boot,2/2] smc911x MII made available 2011-09-07 12:33 ` [U-Boot] [U-Boot,2/2] " Stefano Babic @ 2011-09-07 13:06 ` Helmut Raiger 2011-09-07 13:12 ` Stefano Babic 0 siblings, 1 reply; 47+ messages in thread From: Helmut Raiger @ 2011-09-07 13:06 UTC (permalink / raw) To: u-boot 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. ^ permalink raw reply [flat|nested] 47+ messages in thread
* [U-Boot] [U-Boot,2/2] smc911x MII made available 2011-09-07 13:06 ` Helmut Raiger @ 2011-09-07 13:12 ` Stefano Babic 0 siblings, 0 replies; 47+ messages in thread From: Stefano Babic @ 2011-09-07 13:12 UTC (permalink / raw) To: u-boot 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 ===================================================================== ^ permalink raw reply [flat|nested] 47+ messages in thread
* [U-Boot] [PATCH 2/2] smc911x MII made available 2011-07-04 10:29 ` [U-Boot] [PATCH 2/2] smc911x MII made available helmut.raiger at hale.at 2011-09-07 12:33 ` [U-Boot] [U-Boot,2/2] " Stefano Babic @ 2011-09-07 21:50 ` Wolfgang Denk 2011-09-08 11:04 ` [U-Boot] [PATCH] smc911x: Fix build warnings Wolfgang Denk 2 siblings, 0 replies; 47+ messages in thread From: Wolfgang Denk @ 2011-09-07 21:50 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 47+ messages in thread
* [U-Boot] [PATCH] smc911x: Fix build warnings 2011-07-04 10:29 ` [U-Boot] [PATCH 2/2] smc911x MII made available helmut.raiger at hale.at 2011-09-07 12:33 ` [U-Boot] [U-Boot,2/2] " Stefano Babic 2011-09-07 21:50 ` [U-Boot] [PATCH 2/2] " Wolfgang Denk @ 2011-09-08 11:04 ` Wolfgang Denk 2011-09-09 21:57 ` Wolfgang Denk 2 siblings, 1 reply; 47+ messages in thread From: Wolfgang Denk @ 2011-09-08 11:04 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply related [flat|nested] 47+ messages in thread
* [U-Boot] [PATCH] smc911x: Fix build warnings 2011-09-08 11:04 ` [U-Boot] [PATCH] smc911x: Fix build warnings Wolfgang Denk @ 2011-09-09 21:57 ` Wolfgang Denk 0 siblings, 0 replies; 47+ messages in thread From: Wolfgang Denk @ 2011-09-09 21:57 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 47+ messages in thread
* [U-Boot] [PATCH 1/2] net/eth.c: make eth_get_dev_by_name(NULL) safe 2011-07-04 10:29 ` [U-Boot] [PATCH 1/2] net/eth.c: make eth_get_dev_by_name(NULL) safe helmut.raiger at hale.at 2011-07-04 10:29 ` [U-Boot] [PATCH 2/2] smc911x MII made available helmut.raiger at hale.at @ 2011-07-05 3:44 ` Mike Frysinger 2011-07-06 7:15 ` Helmut Raiger 2011-07-07 16:46 ` Albert ARIBAUD 2 siblings, 1 reply; 47+ messages in thread From: Mike Frysinger @ 2011-07-05 3:44 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 47+ messages in thread
* [U-Boot] [PATCH 1/2] net/eth.c: make eth_get_dev_by_name(NULL) safe 2011-07-05 3:44 ` [U-Boot] [PATCH 1/2] net/eth.c: make eth_get_dev_by_name(NULL) safe Mike Frysinger @ 2011-07-06 7:15 ` Helmut Raiger 2011-07-06 19:38 ` Mike Frysinger 0 siblings, 1 reply; 47+ messages in thread From: Helmut Raiger @ 2011-07-06 7:15 UTC (permalink / raw) To: u-boot 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. ^ permalink raw reply [flat|nested] 47+ messages in thread
* [U-Boot] [PATCH 1/2] net/eth.c: make eth_get_dev_by_name(NULL) safe 2011-07-06 7:15 ` Helmut Raiger @ 2011-07-06 19:38 ` Mike Frysinger 2011-07-07 6:12 ` Helmut Raiger 0 siblings, 1 reply; 47+ messages in thread From: Mike Frysinger @ 2011-07-06 19:38 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 47+ messages in thread
* [U-Boot] [PATCH 1/2] net/eth.c: make eth_get_dev_by_name(NULL) safe 2011-07-06 19:38 ` Mike Frysinger @ 2011-07-07 6:12 ` Helmut Raiger 2011-07-07 10:24 ` Detlev Zundel 2011-07-07 17:46 ` Mike Frysinger 0 siblings, 2 replies; 47+ messages in thread From: Helmut Raiger @ 2011-07-07 6:12 UTC (permalink / raw) To: u-boot 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. ^ permalink raw reply [flat|nested] 47+ messages in thread
* [U-Boot] [PATCH 1/2] net/eth.c: make eth_get_dev_by_name(NULL) safe 2011-07-07 6:12 ` Helmut Raiger @ 2011-07-07 10:24 ` Detlev Zundel 2011-07-07 17:46 ` Mike Frysinger 1 sibling, 0 replies; 47+ messages in thread From: Detlev Zundel @ 2011-07-07 10:24 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 47+ messages in thread
* [U-Boot] [PATCH 1/2] net/eth.c: make eth_get_dev_by_name(NULL) safe 2011-07-07 6:12 ` Helmut Raiger 2011-07-07 10:24 ` Detlev Zundel @ 2011-07-07 17:46 ` Mike Frysinger 2011-07-11 9:53 ` Helmut Raiger 1 sibling, 1 reply; 47+ messages in thread From: Mike Frysinger @ 2011-07-07 17:46 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 47+ messages in thread
* [U-Boot] [PATCH 1/2] net/eth.c: make eth_get_dev_by_name(NULL) safe 2011-07-07 17:46 ` Mike Frysinger @ 2011-07-11 9:53 ` Helmut Raiger 2011-07-12 6:37 ` Mike Frysinger 0 siblings, 1 reply; 47+ messages in thread From: Helmut Raiger @ 2011-07-11 9:53 UTC (permalink / raw) To: u-boot 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. ^ permalink raw reply [flat|nested] 47+ messages in thread
* [U-Boot] [PATCH 1/2] net/eth.c: make eth_get_dev_by_name(NULL) safe 2011-07-11 9:53 ` Helmut Raiger @ 2011-07-12 6:37 ` Mike Frysinger 2011-07-12 9:22 ` Detlev Zundel 0 siblings, 1 reply; 47+ messages in thread From: Mike Frysinger @ 2011-07-12 6:37 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 47+ messages in thread
* [U-Boot] [PATCH 1/2] net/eth.c: make eth_get_dev_by_name(NULL) safe 2011-07-12 6:37 ` Mike Frysinger @ 2011-07-12 9:22 ` Detlev Zundel 2011-07-12 20:49 ` Mike Frysinger 2011-07-13 6:32 ` Helmut Raiger 0 siblings, 2 replies; 47+ messages in thread From: Detlev Zundel @ 2011-07-12 9:22 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 47+ messages in thread
* [U-Boot] [PATCH 1/2] net/eth.c: make eth_get_dev_by_name(NULL) safe 2011-07-12 9:22 ` Detlev Zundel @ 2011-07-12 20:49 ` Mike Frysinger 2011-07-13 11:34 ` Detlev Zundel 2011-07-13 6:32 ` Helmut Raiger 1 sibling, 1 reply; 47+ messages in thread From: Mike Frysinger @ 2011-07-12 20:49 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 47+ messages in thread
* [U-Boot] [PATCH 1/2] net/eth.c: make eth_get_dev_by_name(NULL) safe 2011-07-12 20:49 ` Mike Frysinger @ 2011-07-13 11:34 ` Detlev Zundel 0 siblings, 0 replies; 47+ messages in thread From: Detlev Zundel @ 2011-07-13 11:34 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 47+ messages in thread
* [U-Boot] [PATCH 1/2] net/eth.c: make eth_get_dev_by_name(NULL) safe 2011-07-12 9:22 ` Detlev Zundel 2011-07-12 20:49 ` Mike Frysinger @ 2011-07-13 6:32 ` Helmut Raiger 2011-07-13 11:46 ` Detlev Zundel 2011-07-14 18:24 ` [U-Boot] [PATCH 1/2] net/eth.c: make eth_get_dev_by_name(NULL) safe Mike Frysinger 1 sibling, 2 replies; 47+ messages in thread From: Helmut Raiger @ 2011-07-13 6:32 UTC (permalink / raw) To: u-boot 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. ^ permalink raw reply [flat|nested] 47+ messages in thread
* [U-Boot] [PATCH 1/2] net/eth.c: make eth_get_dev_by_name(NULL) safe 2011-07-13 6:32 ` Helmut Raiger @ 2011-07-13 11:46 ` Detlev Zundel 2011-07-14 9:14 ` Helmut Raiger 2011-08-22 8:45 ` [U-Boot] [PATCH V2 1/2] net/eth.c: throw BUG for eth_get_dev_by_name(NULL) Helmut Raiger 2011-07-14 18:24 ` [U-Boot] [PATCH 1/2] net/eth.c: make eth_get_dev_by_name(NULL) safe Mike Frysinger 1 sibling, 2 replies; 47+ messages in thread From: Detlev Zundel @ 2011-07-13 11:46 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 47+ messages in thread
* [U-Boot] [PATCH 1/2] net/eth.c: make eth_get_dev_by_name(NULL) safe 2011-07-13 11:46 ` Detlev Zundel @ 2011-07-14 9:14 ` Helmut Raiger 2011-07-14 17:58 ` Mike Frysinger 2011-08-22 8:45 ` [U-Boot] [PATCH V2 1/2] net/eth.c: throw BUG for eth_get_dev_by_name(NULL) Helmut Raiger 1 sibling, 1 reply; 47+ messages in thread From: Helmut Raiger @ 2011-07-14 9:14 UTC (permalink / raw) To: u-boot 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. ^ permalink raw reply [flat|nested] 47+ messages in thread
* [U-Boot] [PATCH 1/2] net/eth.c: make eth_get_dev_by_name(NULL) safe 2011-07-14 9:14 ` Helmut Raiger @ 2011-07-14 17:58 ` Mike Frysinger 0 siblings, 0 replies; 47+ messages in thread From: Mike Frysinger @ 2011-07-14 17:58 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 47+ messages in thread
* [U-Boot] [PATCH V2 1/2] net/eth.c: throw BUG for eth_get_dev_by_name(NULL) 2011-07-13 11:46 ` Detlev Zundel 2011-07-14 9:14 ` Helmut Raiger @ 2011-08-22 8:45 ` Helmut Raiger 2011-08-22 10:05 ` Sergei Shtylyov 1 sibling, 1 reply; 47+ messages in thread From: Helmut Raiger @ 2011-08-22 8:45 UTC (permalink / raw) To: u-boot 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. ^ permalink raw reply related [flat|nested] 47+ messages in thread
* [U-Boot] [PATCH V2 1/2] net/eth.c: throw BUG for eth_get_dev_by_name(NULL) 2011-08-22 8:45 ` [U-Boot] [PATCH V2 1/2] net/eth.c: throw BUG for eth_get_dev_by_name(NULL) Helmut Raiger @ 2011-08-22 10:05 ` Sergei Shtylyov 2011-08-22 10:17 ` [U-Boot] [PATCH " Helmut Raiger 0 siblings, 1 reply; 47+ messages in thread From: Sergei Shtylyov @ 2011-08-22 10:05 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 47+ messages in thread
* [U-Boot] [PATCH 1/2] net/eth.c: throw BUG for eth_get_dev_by_name(NULL) 2011-08-22 10:05 ` Sergei Shtylyov @ 2011-08-22 10:17 ` Helmut Raiger 2011-08-22 16:05 ` Mike Frysinger 2011-09-08 11:08 ` Wolfgang Denk 0 siblings, 2 replies; 47+ messages in thread From: Helmut Raiger @ 2011-08-22 10:17 UTC (permalink / raw) To: u-boot 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. ^ permalink raw reply related [flat|nested] 47+ messages in thread
* [U-Boot] [PATCH 1/2] net/eth.c: throw BUG for eth_get_dev_by_name(NULL) 2011-08-22 10:17 ` [U-Boot] [PATCH " Helmut Raiger @ 2011-08-22 16:05 ` Mike Frysinger 2011-09-08 11:08 ` Wolfgang Denk 1 sibling, 0 replies; 47+ messages in thread From: Mike Frysinger @ 2011-08-22 16:05 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 47+ messages in thread
* [U-Boot] [PATCH 1/2] net/eth.c: throw BUG for eth_get_dev_by_name(NULL) 2011-08-22 10:17 ` [U-Boot] [PATCH " Helmut Raiger 2011-08-22 16:05 ` Mike Frysinger @ 2011-09-08 11:08 ` Wolfgang Denk 1 sibling, 0 replies; 47+ messages in thread From: Wolfgang Denk @ 2011-09-08 11:08 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 47+ messages in thread
* [U-Boot] [PATCH 1/2] net/eth.c: make eth_get_dev_by_name(NULL) safe 2011-07-13 6:32 ` Helmut Raiger 2011-07-13 11:46 ` Detlev Zundel @ 2011-07-14 18:24 ` Mike Frysinger 1 sibling, 0 replies; 47+ messages in thread From: Mike Frysinger @ 2011-07-14 18:24 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 47+ messages in thread
* [U-Boot] [PATCH 1/2] net/eth.c: make eth_get_dev_by_name(NULL) safe 2011-07-04 10:29 ` [U-Boot] [PATCH 1/2] net/eth.c: make eth_get_dev_by_name(NULL) safe helmut.raiger at hale.at 2011-07-04 10:29 ` [U-Boot] [PATCH 2/2] smc911x MII made available helmut.raiger at hale.at 2011-07-05 3:44 ` [U-Boot] [PATCH 1/2] net/eth.c: make eth_get_dev_by_name(NULL) safe Mike Frysinger @ 2011-07-07 16:46 ` Albert ARIBAUD 2011-07-11 10:10 ` Helmut Raiger 2 siblings, 1 reply; 47+ messages in thread From: Albert ARIBAUD @ 2011-07-07 16:46 UTC (permalink / raw) To: u-boot 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. ^ permalink raw reply [flat|nested] 47+ messages in thread
* [U-Boot] [PATCH 1/2] net/eth.c: make eth_get_dev_by_name(NULL) safe 2011-07-07 16:46 ` Albert ARIBAUD @ 2011-07-11 10:10 ` Helmut Raiger 2011-07-14 13:53 ` Albert ARIBAUD 0 siblings, 1 reply; 47+ messages in thread From: Helmut Raiger @ 2011-07-11 10:10 UTC (permalink / raw) To: u-boot 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. ^ permalink raw reply [flat|nested] 47+ messages in thread
* [U-Boot] [PATCH 1/2] net/eth.c: make eth_get_dev_by_name(NULL) safe 2011-07-11 10:10 ` Helmut Raiger @ 2011-07-14 13:53 ` Albert ARIBAUD 0 siblings, 0 replies; 47+ messages in thread From: Albert ARIBAUD @ 2011-07-14 13:53 UTC (permalink / raw) To: u-boot 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. ^ permalink raw reply [flat|nested] 47+ messages in thread
* [U-Boot] Anything missing? 2011-06-20 6:10 [U-Boot] [U-Boot PATCH MX31:] smc911x MII made available Helmut.Raiger at hale.at 2011-06-20 12:30 ` Stefano Babic @ 2011-08-31 7:41 ` Helmut Raiger 2011-08-31 7:45 ` Wolfgang Denk 2011-09-07 5:40 ` [U-Boot] [U-Boot PATCH MX31:] smc911x MII made available, ping? Helmut Raiger 2 siblings, 1 reply; 47+ messages in thread From: Helmut Raiger @ 2011-08-31 7:41 UTC (permalink / raw) To: u-boot Is there anything missing for this patch to be accepted? Helmut -- Scanned by MailScanner. ^ permalink raw reply [flat|nested] 47+ messages in thread
* [U-Boot] Anything missing? 2011-08-31 7:41 ` [U-Boot] Anything missing? Helmut Raiger @ 2011-08-31 7:45 ` Wolfgang Denk 0 siblings, 0 replies; 47+ messages in thread From: Wolfgang Denk @ 2011-08-31 7:45 UTC (permalink / raw) To: u-boot 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." ^ permalink raw reply [flat|nested] 47+ messages in thread
* [U-Boot] [U-Boot PATCH MX31:] smc911x MII made available, ping? 2011-06-20 6:10 [U-Boot] [U-Boot PATCH MX31:] smc911x MII made available Helmut.Raiger at hale.at 2011-06-20 12:30 ` Stefano Babic 2011-08-31 7:41 ` [U-Boot] Anything missing? Helmut Raiger @ 2011-09-07 5:40 ` Helmut Raiger 2011-09-07 12:37 ` Stefano Babic 2 siblings, 1 reply; 47+ messages in thread From: Helmut Raiger @ 2011-09-07 5:40 UTC (permalink / raw) To: u-boot This is sitting here for more than 2 months, could someone please ACK and/or apply. Helmut -- Scanned by MailScanner. ^ permalink raw reply [flat|nested] 47+ messages in thread
* [U-Boot] [U-Boot PATCH MX31:] smc911x MII made available, ping? 2011-09-07 5:40 ` [U-Boot] [U-Boot PATCH MX31:] smc911x MII made available, ping? Helmut Raiger @ 2011-09-07 12:37 ` Stefano Babic 2011-09-07 21:47 ` Wolfgang Denk 0 siblings, 1 reply; 47+ messages in thread From: Stefano Babic @ 2011-09-07 12:37 UTC (permalink / raw) To: u-boot 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 ===================================================================== ^ permalink raw reply [flat|nested] 47+ messages in thread
* [U-Boot] [U-Boot PATCH MX31:] smc911x MII made available, ping? 2011-09-07 12:37 ` Stefano Babic @ 2011-09-07 21:47 ` Wolfgang Denk 2011-09-08 6:13 ` Helmut Raiger 0 siblings, 1 reply; 47+ messages in thread From: Wolfgang Denk @ 2011-09-07 21:47 UTC (permalink / raw) To: u-boot 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_ ^ permalink raw reply [flat|nested] 47+ messages in thread
* [U-Boot] [U-Boot PATCH MX31:] smc911x MII made available, ping? 2011-09-07 21:47 ` Wolfgang Denk @ 2011-09-08 6:13 ` Helmut Raiger 0 siblings, 0 replies; 47+ messages in thread From: Helmut Raiger @ 2011-09-08 6:13 UTC (permalink / raw) To: u-boot 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. ^ permalink raw reply [flat|nested] 47+ messages in thread
end of thread, other threads:[~2011-09-09 21:57 UTC | newest] Thread overview: 47+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-06-20 6:10 [U-Boot] [U-Boot PATCH MX31:] smc911x MII made available Helmut.Raiger at hale.at 2011-06-20 12:30 ` Stefano Babic 2011-06-20 14:36 ` Helmut Raiger 2011-06-27 7:22 ` [U-Boot] [PATCH] " helmut.raiger at hale.at 2011-06-27 19:29 ` Mike Frysinger 2011-06-29 10:12 ` helmut.raiger at hale.at 2011-06-30 13:57 ` Luca Ceresoli 2011-06-30 14:02 ` [U-Boot] [PATCH RFC] smc911x: enable mii commands Luca Ceresoli 2011-07-04 9:41 ` Helmut Raiger 2011-07-04 10:29 ` [U-Boot] [PATCH 1/2] net/eth.c: make eth_get_dev_by_name(NULL) safe helmut.raiger at hale.at 2011-07-04 10:29 ` [U-Boot] [PATCH 2/2] smc911x MII made available helmut.raiger at hale.at 2011-09-07 12:33 ` [U-Boot] [U-Boot,2/2] " Stefano Babic 2011-09-07 13:06 ` Helmut Raiger 2011-09-07 13:12 ` Stefano Babic 2011-09-07 21:50 ` [U-Boot] [PATCH 2/2] " Wolfgang Denk 2011-09-08 11:04 ` [U-Boot] [PATCH] smc911x: Fix build warnings Wolfgang Denk 2011-09-09 21:57 ` Wolfgang Denk 2011-07-05 3:44 ` [U-Boot] [PATCH 1/2] net/eth.c: make eth_get_dev_by_name(NULL) safe Mike Frysinger 2011-07-06 7:15 ` Helmut Raiger 2011-07-06 19:38 ` Mike Frysinger 2011-07-07 6:12 ` Helmut Raiger 2011-07-07 10:24 ` Detlev Zundel 2011-07-07 17:46 ` Mike Frysinger 2011-07-11 9:53 ` Helmut Raiger 2011-07-12 6:37 ` Mike Frysinger 2011-07-12 9:22 ` Detlev Zundel 2011-07-12 20:49 ` Mike Frysinger 2011-07-13 11:34 ` Detlev Zundel 2011-07-13 6:32 ` Helmut Raiger 2011-07-13 11:46 ` Detlev Zundel 2011-07-14 9:14 ` Helmut Raiger 2011-07-14 17:58 ` Mike Frysinger 2011-08-22 8:45 ` [U-Boot] [PATCH V2 1/2] net/eth.c: throw BUG for eth_get_dev_by_name(NULL) Helmut Raiger 2011-08-22 10:05 ` Sergei Shtylyov 2011-08-22 10:17 ` [U-Boot] [PATCH " Helmut Raiger 2011-08-22 16:05 ` Mike Frysinger 2011-09-08 11:08 ` Wolfgang Denk 2011-07-14 18:24 ` [U-Boot] [PATCH 1/2] net/eth.c: make eth_get_dev_by_name(NULL) safe Mike Frysinger 2011-07-07 16:46 ` Albert ARIBAUD 2011-07-11 10:10 ` Helmut Raiger 2011-07-14 13:53 ` Albert ARIBAUD 2011-08-31 7:41 ` [U-Boot] Anything missing? Helmut Raiger 2011-08-31 7:45 ` Wolfgang Denk 2011-09-07 5:40 ` [U-Boot] [U-Boot PATCH MX31:] smc911x MII made available, ping? Helmut Raiger 2011-09-07 12:37 ` Stefano Babic 2011-09-07 21:47 ` Wolfgang Denk 2011-09-08 6:13 ` Helmut Raiger
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.