question about drivers/net/cpmac.c

question about drivers/net/cpmac.c

[Julia Lawall]

The function __devinit cpmac_probe in the file drivers/net/cpmac.c 
contains the following code:

memcpy(dev->dev_addr, pdata->dev_addr, sizeof(dev->dev_addr));

Is it correct that the size of the pointer is what is wanted?

julia

question about drivers/net/3c507.c

[Julia Lawall]

The function __devinit cpmac_probe in the file drivers/net/3c507.c 
contains the following code:

       memcpy_toio(lp->base+SA_OFFSET, dev->dev_addr,
                  sizeof(dev->dev_addr));
 
Is it correct that the size of the pointer is what is wanted?

julia

Re: question about drivers/net/3c507.c

[David Miller]

From: Julia Lawall <julia@diku.dk>
Date: Sun, 13 Dec 2009 11:25:02 +0100 (CET)

> The function __devinit cpmac_probe in the file drivers/net/3c507.c 
> contains the following code:
> 
>        memcpy_toio(lp->base+SA_OFFSET, dev->dev_addr,
>                   sizeof(dev->dev_addr));
>  
> Is it correct that the size of the pointer is what is wanted?

Same as the cpmac.c case, needs to be fixed.

Re: question about drivers/net/cpmac.c

[David Miller]

From: Julia Lawall <julia@diku.dk>
Date: Sun, 13 Dec 2009 11:22:54 +0100 (CET)

> The function __devinit cpmac_probe in the file drivers/net/cpmac.c 
> contains the following code:
> 
> memcpy(dev->dev_addr, pdata->dev_addr, sizeof(dev->dev_addr));
> 
> Is it correct that the size of the pointer is what is wanted?

Everything that does sizeof(netdev->dev_addr) is a bug.

At some point we changed netdev->dev_addr from an array of chars to a
pointer to a dynamically allocated buffer.

So these cases worked before that change and need to be updated
in order to be correct.

Looking quickly there are a couple of these things under
drivers/net

Re: question about drivers/net/cpmac.c

[Julia Lawall]

On Sun, 13 Dec 2009, David Miller wrote:

> From: Julia Lawall <julia@diku.dk>
> Date: Sun, 13 Dec 2009 11:22:54 +0100 (CET)
> 
> > The function __devinit cpmac_probe in the file drivers/net/cpmac.c 
> > contains the following code:
> > 
> > memcpy(dev->dev_addr, pdata->dev_addr, sizeof(dev->dev_addr));
> > 
> > Is it correct that the size of the pointer is what is wanted?
> 
> Everything that does sizeof(netdev->dev_addr) is a bug.
> 
> At some point we changed netdev->dev_addr from an array of chars to a
> pointer to a dynamically allocated buffer.
> 
> So these cases worked before that change and need to be updated
> in order to be correct.
> 
> Looking quickly there are a couple of these things under
> drivers/net

Fixed how?  I looked a bit to find where the field was initialized, but it 
is just initialized to a field of something else, so it was not so 
clear what the size should be.

I found the following elsewhere:

memcmp(eth->h_dest, dev->dev_addr, dev->addr_len)

Could these cases be changed to use the addr_len field? On the other hand, 
only rtl8150.c contains any mention of the addr_len field.

        memcpy(netdev->dev_addr, addr->sa_data, netdev->addr_len);
	dbg("%s: Setting MAC address to ", netdev->name);
        for (i = 0; i < 5; i++)
                dbg("%02X:", netdev->dev_addr[i]);
        dbg("%02X\n", netdev->dev_addr[i]);
        /* Set the IDR registers. */
        set_registers(dev, IDR, sizeof(netdev->dev_addr), netdev->dev_addr);


julia

Re: question about drivers/net/cpmac.c

[Ben Hutchings]

On Sun, 2009-12-13 at 12:00 +0100, Julia Lawall wrote:
> On Sun, 13 Dec 2009, David Miller wrote:
> 
> > From: Julia Lawall <julia@diku.dk>
> > Date: Sun, 13 Dec 2009 11:22:54 +0100 (CET)
> > 
> > > The function __devinit cpmac_probe in the file drivers/net/cpmac.c 
> > > contains the following code:
> > > 
> > > memcpy(dev->dev_addr, pdata->dev_addr, sizeof(dev->dev_addr));
> > > 
> > > Is it correct that the size of the pointer is what is wanted?
> > 
> > Everything that does sizeof(netdev->dev_addr) is a bug.
> > 
> > At some point we changed netdev->dev_addr from an array of chars to a
> > pointer to a dynamically allocated buffer.
> > 
> > So these cases worked before that change and need to be updated
> > in order to be correct.
> > 
> > Looking quickly there are a couple of these things under
> > drivers/net
> 
> Fixed how?  I looked a bit to find where the field was initialized, but it 
> is just initialized to a field of something else, so it was not so 
> clear what the size should be.
[...]

The size should be dev->addr_len (assuming that has already been
initialised) which will be ETH_ALEN for Ethernet devices.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: question about drivers/net/cpmac.c

[Florian Fainelli]

Le dimanche 13 décembre 2009 12:51:59, Ben Hutchings a écrit :
> On Sun, 2009-12-13 at 12:00 +0100, Julia Lawall wrote:
> > On Sun, 13 Dec 2009, David Miller wrote:
> > > From: Julia Lawall <julia@diku.dk>
> > > Date: Sun, 13 Dec 2009 11:22:54 +0100 (CET)
> > >
> > > > The function __devinit cpmac_probe in the file drivers/net/cpmac.c
> > > > contains the following code:
> > > >
> > > > memcpy(dev->dev_addr, pdata->dev_addr, sizeof(dev->dev_addr));
> > > >
> > > > Is it correct that the size of the pointer is what is wanted?
> > >
> > > Everything that does sizeof(netdev->dev_addr) is a bug.
> > >
> > > At some point we changed netdev->dev_addr from an array of chars to a
> > > pointer to a dynamically allocated buffer.
> > >
> > > So these cases worked before that change and need to be updated
> > > in order to be correct.
> > >
> > > Looking quickly there are a couple of these things under
> > > drivers/net
> >
> > Fixed how?  I looked a bit to find where the field was initialized, but
> > it is just initialized to a field of something else, so it was not so
> > clear what the size should be.
> 
> [...]
> 
> The size should be dev->addr_len (assuming that has already been
> initialised) which will be ETH_ALEN for Ethernet devices.

From arch/mips/include/asm/mach-ar7/ar7.h:

struct plat_cpmac_data {
[..]
        char dev_addr[6];
};

So this should be fine to either use ETH_ALEN or sizeof(pdata->dev_addr).
-- 
Best regards, Florian Fainelli
Email: florian@openwrt.org
Web: http://openwrt.org
IRC: [florian] on irc.freenode.net
-------------------------------

Re: question about drivers/net/cpmac.c

[Julia Lawall]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1655 bytes --]

On Sun, 13 Dec 2009, Florian Fainelli wrote:

> Le dimanche 13 décembre 2009 12:51:59, Ben Hutchings a écrit :
> > On Sun, 2009-12-13 at 12:00 +0100, Julia Lawall wrote:
> > > On Sun, 13 Dec 2009, David Miller wrote:
> > > > From: Julia Lawall <julia@diku.dk>
> > > > Date: Sun, 13 Dec 2009 11:22:54 +0100 (CET)
> > > >
> > > > > The function __devinit cpmac_probe in the file drivers/net/cpmac.c
> > > > > contains the following code:
> > > > >
> > > > > memcpy(dev->dev_addr, pdata->dev_addr, sizeof(dev->dev_addr));
> > > > >
> > > > > Is it correct that the size of the pointer is what is wanted?
> > > >
> > > > Everything that does sizeof(netdev->dev_addr) is a bug.
> > > >
> > > > At some point we changed netdev->dev_addr from an array of chars to a
> > > > pointer to a dynamically allocated buffer.
> > > >
> > > > So these cases worked before that change and need to be updated
> > > > in order to be correct.
> > > >
> > > > Looking quickly there are a couple of these things under
> > > > drivers/net
> > >
> > > Fixed how?  I looked a bit to find where the field was initialized, but
> > > it is just initialized to a field of something else, so it was not so
> > > clear what the size should be.
> > 
> > [...]
> > 
> > The size should be dev->addr_len (assuming that has already been
> > initialised) which will be ETH_ALEN for Ethernet devices.
> 
> From arch/mips/include/asm/mach-ar7/ar7.h:
> 
> struct plat_cpmac_data {
> [..]
>         char dev_addr[6];
> };
> 
> So this should be fine to either use ETH_ALEN or sizeof(pdata->dev_addr).

Thanks.  I will send a patch later today.

julia