Patchwork [NET] several cleanups and bugfixes for fec.c: don't munge MAC address from platform data

login
register
mail settings
Submitter Lothar Waßmann
Date Dec. 6, 2011, 10:27 a.m.
Message ID <55b78c1766da5b0d0d679f5eae3fb9fc74a6ceef.1323163127.git.LW@KARO-electronics.de>
Download mbox | patch
Permalink /patch/129604/
State New
Headers show

Comments

Lothar Waßmann - Dec. 6, 2011, 10:27 a.m.
When the MAC address is supplied via platform_data it should be OK as
it is and should not be modified in case of a dual FEC setup.
Also copying the MAC from platform_data to the single 'macaddr'
variable will overwrite the MAC for the first interface in case of a
dual FEC setup.

Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
---
 drivers/net/ethernet/freescale/fec.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Lothar Waßmann - Dec. 6, 2011, 1:44 p.m.
Hi,

Baruch Siach writes:
> Hi Lothar,
> 
> On Tue, Dec 06, 2011 at 11:27:13AM +0100, Lothar Waßmann wrote:
> > When the MAC address is supplied via platform_data it should be OK as
> > it is and should not be modified in case of a dual FEC setup.
> > Also copying the MAC from platform_data to the single 'macaddr'
> > variable will overwrite the MAC for the first interface in case of a
> > dual FEC setup.
> > 
> > Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
> > ---
> >  drivers/net/ethernet/freescale/fec.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/freescale/fec.c b/drivers/net/ethernet/freescale/fec.c
> > index e2b5ce6..11534b9 100644
> > --- a/drivers/net/ethernet/freescale/fec.c
> > +++ b/drivers/net/ethernet/freescale/fec.c
> > @@ -818,7 +818,7 @@ static void __inline__ fec_get_mac(struct net_device *ndev)
> >  			iap = (unsigned char *)FEC_FLASHMAC;
> >  #else
> >  		if (pdata)
> > -			memcpy(iap, pdata->mac, ETH_ALEN);
> > +			iap = (unsigned char *)&pdata->mac;
> 
> Since pdata might point to __initdata struct, you must hold a copy of its 
> content.
> 
No. platform_data must be present during the life time of a driver
using it and thus must never be __initdata!


Lothar Waßmann
Lothar Waßmann - Dec. 7, 2011, 8:47 a.m.
Hi,

Shawn Guo writes:
> On Tue, Dec 06, 2011 at 11:27:13AM +0100, Lothar Waßmann wrote:
> > When the MAC address is supplied via platform_data it should be OK as
> > it is and should not be modified in case of a dual FEC setup.
> > Also copying the MAC from platform_data to the single 'macaddr'
> > variable will overwrite the MAC for the first interface in case of a
> > dual FEC setup.
> 
> Hmm, I do not follow that.  If 'macaddr' holds a valid mac address,
> the code path has no chance to be hit at all.  Otherwise, 'macaddr'
> is just a place holder for copying mac address from pdata, in which
> case the mac address will be fixed up at the end of the function for
> dual FEC setup.
> 
In case of dual FEC and MAC address provided by platform_data the
first mac address will be copied to the 'macaddr' variable with no
chance for the second interface to get its mac address assigned from
platform_data too.


Lothar Waßmann
Lothar Waßmann - Dec. 7, 2011, 8:47 a.m.
Hi,

Shawn Guo writes:
> On Tue, Dec 06, 2011 at 02:44:44PM +0100, Lothar Waßmann wrote:
> > Hi,
> > 
> > Baruch Siach writes:
> > > Hi Lothar,
> > > 
> > > On Tue, Dec 06, 2011 at 11:27:13AM +0100, Lothar Waßmann wrote:
> > > > When the MAC address is supplied via platform_data it should be OK as
> > > > it is and should not be modified in case of a dual FEC setup.
> > > > Also copying the MAC from platform_data to the single 'macaddr'
> > > > variable will overwrite the MAC for the first interface in case of a
> > > > dual FEC setup.
> > > > 
> > > > Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
> > > > ---
> > > >  drivers/net/ethernet/freescale/fec.c |    2 +-
> > > >  1 files changed, 1 insertions(+), 1 deletions(-)
> > > > 
> > > > diff --git a/drivers/net/ethernet/freescale/fec.c b/drivers/net/ethernet/freescale/fec.c
> > > > index e2b5ce6..11534b9 100644
> > > > --- a/drivers/net/ethernet/freescale/fec.c
> > > > +++ b/drivers/net/ethernet/freescale/fec.c
> > > > @@ -818,7 +818,7 @@ static void __inline__ fec_get_mac(struct net_device *ndev)
> > > >  			iap = (unsigned char *)FEC_FLASHMAC;
> > > >  #else
> > > >  		if (pdata)
> > > > -			memcpy(iap, pdata->mac, ETH_ALEN);
> > > > +			iap = (unsigned char *)&pdata->mac;
> > > 
> > > Since pdata might point to __initdata struct, you must hold a copy of its 
> > > content.
> > > 
> 
> As iap will anyway be copied to ndev->dev_addr after that, it may still
> be fine to take this patch, for obviously for different reason.
> 
> > No. platform_data must be present during the life time of a driver
> > using it and thus must never be __initdata!
> > 
> Then we need to fix a lot of imx/mxc platform codes, mach-mx28evk.c
> is the one for this case.
> 
Nope. The platform_data provided there is only a template that is
copied to kmalloc'ed memory by the mxs_add...() functions.

With actual platform_data in init memory no driver built as module
would work.


Lothar Waßmann
Shawn Guo - Dec. 7, 2011, 8:49 a.m.
On Tue, Dec 06, 2011 at 11:27:13AM +0100, Lothar Waßmann wrote:
> When the MAC address is supplied via platform_data it should be OK as
> it is and should not be modified in case of a dual FEC setup.
> Also copying the MAC from platform_data to the single 'macaddr'
> variable will overwrite the MAC for the first interface in case of a
> dual FEC setup.

Hmm, I do not follow that.  If 'macaddr' holds a valid mac address,
the code path has no chance to be hit at all.  Otherwise, 'macaddr'
is just a place holder for copying mac address from pdata, in which
case the mac address will be fixed up at the end of the function for
dual FEC setup.

Regards,
Shawn

> 
> Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
> ---
>  drivers/net/ethernet/freescale/fec.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec.c b/drivers/net/ethernet/freescale/fec.c
> index e2b5ce6..11534b9 100644
> --- a/drivers/net/ethernet/freescale/fec.c
> +++ b/drivers/net/ethernet/freescale/fec.c
> @@ -818,7 +818,7 @@ static void __inline__ fec_get_mac(struct net_device *ndev)
>  			iap = (unsigned char *)FEC_FLASHMAC;
>  #else
>  		if (pdata)
> -			memcpy(iap, pdata->mac, ETH_ALEN);
> +			iap = (unsigned char *)&pdata->mac;
>  #endif
>  	}
>  
> -- 
> 1.5.6.5
Shawn Guo - Dec. 7, 2011, 8:57 a.m.
On Tue, Dec 06, 2011 at 02:44:44PM +0100, Lothar Waßmann wrote:
> Hi,
> 
> Baruch Siach writes:
> > Hi Lothar,
> > 
> > On Tue, Dec 06, 2011 at 11:27:13AM +0100, Lothar Waßmann wrote:
> > > When the MAC address is supplied via platform_data it should be OK as
> > > it is and should not be modified in case of a dual FEC setup.
> > > Also copying the MAC from platform_data to the single 'macaddr'
> > > variable will overwrite the MAC for the first interface in case of a
> > > dual FEC setup.
> > > 
> > > Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
> > > ---
> > >  drivers/net/ethernet/freescale/fec.c |    2 +-
> > >  1 files changed, 1 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/drivers/net/ethernet/freescale/fec.c b/drivers/net/ethernet/freescale/fec.c
> > > index e2b5ce6..11534b9 100644
> > > --- a/drivers/net/ethernet/freescale/fec.c
> > > +++ b/drivers/net/ethernet/freescale/fec.c
> > > @@ -818,7 +818,7 @@ static void __inline__ fec_get_mac(struct net_device *ndev)
> > >  			iap = (unsigned char *)FEC_FLASHMAC;
> > >  #else
> > >  		if (pdata)
> > > -			memcpy(iap, pdata->mac, ETH_ALEN);
> > > +			iap = (unsigned char *)&pdata->mac;
> > 
> > Since pdata might point to __initdata struct, you must hold a copy of its 
> > content.
> > 

As iap will anyway be copied to ndev->dev_addr after that, it may still
be fine to take this patch, for obviously for different reason.

> No. platform_data must be present during the life time of a driver
> using it and thus must never be __initdata!
> 
Then we need to fix a lot of imx/mxc platform codes, mach-mx28evk.c
is the one for this case.
Shawn Guo - Dec. 7, 2011, 9:14 a.m.
On Wed, Dec 07, 2011 at 09:47:37AM +0100, Lothar Waßmann wrote:
> Hi,
> 
> Shawn Guo writes:
> > On Tue, Dec 06, 2011 at 11:27:13AM +0100, Lothar Waßmann wrote:
> > > When the MAC address is supplied via platform_data it should be OK as
> > > it is and should not be modified in case of a dual FEC setup.
> > > Also copying the MAC from platform_data to the single 'macaddr'
> > > variable will overwrite the MAC for the first interface in case of a
> > > dual FEC setup.
> > 
> > Hmm, I do not follow that.  If 'macaddr' holds a valid mac address,
> > the code path has no chance to be hit at all.  Otherwise, 'macaddr'
> > is just a place holder for copying mac address from pdata, in which
> > case the mac address will be fixed up at the end of the function for
> > dual FEC setup.
> > 
> In case of dual FEC and MAC address provided by platform_data the
> first mac address will be copied to the 'macaddr' variable with no
> chance for the second interface to get its mac address assigned from
> platform_data too.
> 
Indeed.  So again other than subject,

Acked-by: Shawn Guo <shawn.guo@linaro.org>

Patch

diff --git a/drivers/net/ethernet/freescale/fec.c b/drivers/net/ethernet/freescale/fec.c
index e2b5ce6..11534b9 100644
--- a/drivers/net/ethernet/freescale/fec.c
+++ b/drivers/net/ethernet/freescale/fec.c
@@ -818,7 +818,7 @@  static void __inline__ fec_get_mac(struct net_device *ndev)
 			iap = (unsigned char *)FEC_FLASHMAC;
 #else
 		if (pdata)
-			memcpy(iap, pdata->mac, ETH_ALEN);
+			iap = (unsigned char *)&pdata->mac;
 #endif
 	}