| Message ID | a6ba75577552abe8d5045434030c276d5f45779a.1300724245.git.LW@KARO-electronics.de |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Mon, Mar 21, 2011 at 05:37:36PM +0100, Lothar Waßmann wrote: > The FEC driver currently uses the MAC address assigned to the first > interface incremented by one for the second interface. > > Change this to be able to configure distinct MAC addresses via > platform_data. > > Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de> > --- > drivers/net/fec.c | 9 ++------- > 1 files changed, 2 insertions(+), 7 deletions(-) > > diff --git a/drivers/net/fec.c b/drivers/net/fec.c > index 3666524..9d89e99 100644 > --- a/drivers/net/fec.c > +++ b/drivers/net/fec.c > @@ -750,13 +750,12 @@ static void __inline__ fec_get_mac(struct net_device *ndev) > /* > * 2) from flash or fuse (via platform data) > */ > + if (pdata) > + memcpy(iap, pdata->mac, ETH_ALEN); > if (!is_valid_ether_addr(iap)) { > #ifdef CONFIG_M5272 > if (FEC_FLASHMAC) > iap = (unsigned char *)FEC_FLASHMAC; > -#else > - if (pdata) > - memcpy(iap, pdata->mac, ETH_ALEN); > #endif > } > > @@ -772,10 +771,6 @@ static void __inline__ fec_get_mac(struct net_device *ndev) > } > > memcpy(ndev->dev_addr, iap, ETH_ALEN); > - > - /* Adjust MAC if using macaddr */ > - if (iap == macaddr) > - ndev->dev_addr[ETH_ALEN-1] = macaddr[ETH_ALEN-1] + fep->pdev->id; > } > > /* ------------------------------------------------------------------------- */ > -- > 1.5.6.5 > > NAK. The 'mac' was designed as an optional member of fec platform_data, as some platforms may not be able to provide it. This patch breaks the order of getting mac, and will not work on the platform that does not provide an valid mac with either platform_data or controller mac registers, even there may be one in the kernel command line. The existing code takes mac address from kernel command line as the first choice, and only falls through on other options when it's invalid/unavailable. So an valid mac from kernel command line will always get fec work anyway.
Hi, Shawn Guo writes: > On Mon, Mar 21, 2011 at 05:37:36PM +0100, Lothar Waßmann wrote: > > The FEC driver currently uses the MAC address assigned to the first > > interface incremented by one for the second interface. > > > > Change this to be able to configure distinct MAC addresses via > > platform_data. > > > > Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de> > > --- > > drivers/net/fec.c | 9 ++------- > > 1 files changed, 2 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/net/fec.c b/drivers/net/fec.c > > index 3666524..9d89e99 100644 > > --- a/drivers/net/fec.c > > +++ b/drivers/net/fec.c > > @@ -750,13 +750,12 @@ static void __inline__ fec_get_mac(struct net_device *ndev) > > /* > > * 2) from flash or fuse (via platform data) > > */ > > + if (pdata) > > + memcpy(iap, pdata->mac, ETH_ALEN); > > if (!is_valid_ether_addr(iap)) { > > #ifdef CONFIG_M5272 > > if (FEC_FLASHMAC) > > iap = (unsigned char *)FEC_FLASHMAC; > > -#else > > - if (pdata) > > - memcpy(iap, pdata->mac, ETH_ALEN); > > #endif > > } > > > > @@ -772,10 +771,6 @@ static void __inline__ fec_get_mac(struct net_device *ndev) > > } > > > > memcpy(ndev->dev_addr, iap, ETH_ALEN); > > - > > - /* Adjust MAC if using macaddr */ > > - if (iap == macaddr) > > - ndev->dev_addr[ETH_ALEN-1] = macaddr[ETH_ALEN-1] + fep->pdev->id; > > } > > > > /* ------------------------------------------------------------------------- */ > > -- > > 1.5.6.5 > > > > > NAK. > > The 'mac' was designed as an optional member of fec > platform_data, as some platforms may not be able to provide it. This > patch breaks the order of getting mac, and will not work on the > platform that does not provide an valid mac with either platform_data > or controller mac registers, even there may be one in the kernel > command line. > > The existing code takes mac address from kernel command line as the > first choice, and only falls through on other options when it's > invalid/unavailable. So an valid mac from kernel command line will > always get fec work anyway. > The problem is that you can only assign one mac address on the kernel cmdline and the second one will be derived from the first one. And even if the MAC address is set via platform_data it will not be used for the second interface. IMO an ethernet driver has no business in inventing arbitrary MAC addresses. Either the MAC address is supplied by the boot loader or the platform code or it has to be assigned from userspace via ifconfig. Lothar Waßmann
On Tue, Mar 22, 2011 at 08:49:41AM +0100, Lothar Waßmann wrote: > Hi, > > Shawn Guo writes: > > On Mon, Mar 21, 2011 at 05:37:36PM +0100, Lothar Waßmann wrote: > > > The FEC driver currently uses the MAC address assigned to the first > > > interface incremented by one for the second interface. > > > > > > Change this to be able to configure distinct MAC addresses via > > > platform_data. > > > > > > Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de> > > > --- > > > drivers/net/fec.c | 9 ++------- > > > 1 files changed, 2 insertions(+), 7 deletions(-) > > > > > > diff --git a/drivers/net/fec.c b/drivers/net/fec.c > > > index 3666524..9d89e99 100644 > > > --- a/drivers/net/fec.c > > > +++ b/drivers/net/fec.c > > > @@ -750,13 +750,12 @@ static void __inline__ fec_get_mac(struct net_device *ndev) > > > /* > > > * 2) from flash or fuse (via platform data) > > > */ > > > + if (pdata) > > > + memcpy(iap, pdata->mac, ETH_ALEN); > > > if (!is_valid_ether_addr(iap)) { > > > #ifdef CONFIG_M5272 > > > if (FEC_FLASHMAC) > > > iap = (unsigned char *)FEC_FLASHMAC; > > > -#else > > > - if (pdata) > > > - memcpy(iap, pdata->mac, ETH_ALEN); > > > #endif > > > } > > > > > > @@ -772,10 +771,6 @@ static void __inline__ fec_get_mac(struct net_device *ndev) > > > } > > > > > > memcpy(ndev->dev_addr, iap, ETH_ALEN); > > > - > > > - /* Adjust MAC if using macaddr */ > > > - if (iap == macaddr) > > > - ndev->dev_addr[ETH_ALEN-1] = macaddr[ETH_ALEN-1] + fep->pdev->id; > > > } > > > > > > /* ------------------------------------------------------------------------- */ > > > -- > > > 1.5.6.5 > > > > > > > > NAK. > > > > The 'mac' was designed as an optional member of fec > > platform_data, as some platforms may not be able to provide it. This > > patch breaks the order of getting mac, and will not work on the > > platform that does not provide an valid mac with either platform_data > > or controller mac registers, even there may be one in the kernel > > command line. > > > > The existing code takes mac address from kernel command line as the > > first choice, and only falls through on other options when it's > > invalid/unavailable. So an valid mac from kernel command line will > > always get fec work anyway. > > > The problem is that you can only assign one mac address on the kernel > cmdline and the second one will be derived from the first one. > And even if the MAC address is set via platform_data it will not be > used for the second interface. > Yes, these are problems. But we can fix them like the following while still make fec0 work with fec.macaddr. This is useful on the platform that neither platform code nor boot loader gives an valid mac address. @@ -721,7 +721,7 @@ static void __inline__ fec_get_mac(struct net_device *ndev) { struct fec_enet_private *fep = netdev_priv(ndev); struct fec_platform_data *pdata = fep->pdev->dev.platform_data; - unsigned char *iap, tmpaddr[ETH_ALEN]; + unsigned char iap[ETH_ALEN], tmpaddr[ETH_ALEN]; /* * try to get mac address in following order: @@ -729,7 +729,10 @@ static void __inline__ fec_get_mac(struct net_device *ndev) * 1) module parameter via kernel command line in form * fec.macaddr=0x00,0x04,0x9f,0x01,0x30,0xe0 */ - iap = macaddr; + if (fep->pdev->id == 0) + memcpy(iap, macaddr, ETH_ALEN); + else + memset(iap, 0, ETH_ALEN); /* * 2) from flash or fuse (via platform data) @@ -752,14 +755,10 @@ static void __inline__ fec_get_mac(struct net_device *ndev) be32_to_cpu(readl(fep->hwp + FEC_ADDR_LOW)); *((unsigned short *) &tmpaddr[4]) = be16_to_cpu(readl(fep->hwp + FEC_ADDR_HIGH) >> 16); - iap = &tmpaddr[0]; + memcpy(iap, tmpaddr, ETH_ALEN); } memcpy(ndev->dev_addr, iap, ETH_ALEN); - - /* Adjust MAC if using macaddr */ - if (iap == macaddr) - ndev->dev_addr[ETH_ALEN-1] = macaddr[ETH_ALEN-1] + fep->pdev->id; } > IMO an ethernet driver has no business in inventing arbitrary MAC > addresses. Either the MAC address is supplied by the boot loader or > the platform code or it has to be assigned from userspace via > ifconfig. > ifconfig will not be an option if we need to mount nfs as rootfs. So we at lease need to have kernel command line for fec0 mac address for the last chance, if neither platform data nor boot loader provides mac address. And let ifconfig provide mac for fec1 from user space.
diff --git a/drivers/net/fec.c b/drivers/net/fec.c index 3666524..9d89e99 100644 --- a/drivers/net/fec.c +++ b/drivers/net/fec.c @@ -750,13 +750,12 @@ static void __inline__ fec_get_mac(struct net_device *ndev) /* * 2) from flash or fuse (via platform data) */ + if (pdata) + memcpy(iap, pdata->mac, ETH_ALEN); if (!is_valid_ether_addr(iap)) { #ifdef CONFIG_M5272 if (FEC_FLASHMAC) iap = (unsigned char *)FEC_FLASHMAC; -#else - if (pdata) - memcpy(iap, pdata->mac, ETH_ALEN); #endif } @@ -772,10 +771,6 @@ static void __inline__ fec_get_mac(struct net_device *ndev) } memcpy(ndev->dev_addr, iap, ETH_ALEN); - - /* Adjust MAC if using macaddr */ - if (iap == macaddr) - ndev->dev_addr[ETH_ALEN-1] = macaddr[ETH_ALEN-1] + fep->pdev->id; } /* ------------------------------------------------------------------------- */
The FEC driver currently uses the MAC address assigned to the first interface incremented by one for the second interface. Change this to be able to configure distinct MAC addresses via platform_data. Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de> --- drivers/net/fec.c | 9 ++------- 1 files changed, 2 insertions(+), 7 deletions(-)