Patchwork [03/11] cirrus/mac89x0: print MAC via printk format specifier

login
register
mail settings
Submitter Danny Kukawka
Date Feb. 24, 2012, 1:45 p.m.
Message ID <1330091162-8141-4-git-send-email-danny.kukawka@bisect.de>
Download mbox | patch
Permalink /patch/142836/
State Accepted
Delegated to: David Miller
Headers show

Comments

Danny Kukawka - Feb. 24, 2012, 1:45 p.m.
Print MAC/dev_addr via printk extended format specifier %pM instead
of custom code.

Use memcpy to set the address to dev->dev_addr in set_mac_address,
instead of mxing it up in a for loop with printing a debug msg.

Check also if the given address is valid.

Signed-off-by: Danny Kukawka <danny.kukawka@bisect.de>
---
 drivers/net/ethernet/cirrus/mac89x0.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)
Geert Uytterhoeven - Feb. 25, 2012, 10:15 a.m.
On Fri, Feb 24, 2012 at 14:45, Danny Kukawka <danny.kukawka@bisect.de> wrote:
> Print MAC/dev_addr via printk extended format specifier %pM instead
> of custom code.
>
> Use memcpy to set the address to dev->dev_addr in set_mac_address,
> instead of mxing it up in a for loop with printing a debug msg.
>
> Check also if the given address is valid.

Why do you sneak in this check in this patch?

> Signed-off-by: Danny Kukawka <danny.kukawka@bisect.de>
> ---
>  drivers/net/ethernet/cirrus/mac89x0.c |   12 ++++++++----
>  1 files changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/ethernet/cirrus/mac89x0.c b/drivers/net/ethernet/cirrus/mac89x0.c
> index 83781f3..a7324ce 100644
> --- a/drivers/net/ethernet/cirrus/mac89x0.c
> +++ b/drivers/net/ethernet/cirrus/mac89x0.c
> @@ -592,10 +592,14 @@ static void set_multicast_list(struct net_device *dev)
>  static int set_mac_address(struct net_device *dev, void *addr)
>  {
>        int i;
> -       printk("%s: Setting MAC address to ", dev->name);
> -       for (i = 0; i < 6; i++)
> -               printk(" %2.2x", dev->dev_addr[i] = ((unsigned char *)addr)[i]);
> -       printk(".\n");
> +       struct sockaddr *saddr = addr;
> +
> +       if (!is_valid_ether_addr(addr->sa_data))
> +               return -EADDRNOTAVAIL;
> +
> +       memcpy(dev->dev_addr, addr->sa_data, ETH_ALEN);
> +       printk("%s: Setting MAC address to %pM\n", dev->name, dev->dev_addr);
> +
>        /* set the Ethernet address */
>        for (i=0; i < ETH_ALEN/2; i++)
>                writereg(dev, PP_IA+i*2, dev->dev_addr[i*2] | (dev->dev_addr[i*2+1] << 8));

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Geert Uytterhoeven - Feb. 28, 2012, 8:45 p.m.
On Sat, Feb 25, 2012 at 11:15, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Fri, Feb 24, 2012 at 14:45, Danny Kukawka <danny.kukawka@bisect.de> wrote:
>> Print MAC/dev_addr via printk extended format specifier %pM instead
>> of custom code.
>>
>> Use memcpy to set the address to dev->dev_addr in set_mac_address,
>> instead of mxing it up in a for loop with printing a debug msg.
>>
>> Check also if the given address is valid.
>
> Why do you sneak in this check in this patch?

And it doesn't compile:

http://kisskb.ellerman.id.au/kisskb/buildresult/5752157/

drivers/net/ethernet/cirrus/mac89x0.c: In function ‘set_mac_address’:
drivers/net/ethernet/cirrus/mac89x0.c:597: warning: dereferencing
‘void *’ pointer
drivers/net/ethernet/cirrus/mac89x0.c:597: error: request for member
‘sa_data’ in something not a structure or union
drivers/net/ethernet/cirrus/mac89x0.c:600: warning: dereferencing
‘void *’ pointer
drivers/net/ethernet/cirrus/mac89x0.c:600: error: request for member
‘sa_data’ in something not a structure or union
drivers/net/ethernet/cirrus/mac89x0.c:595: warning: unused variable ‘saddr’

No patch included as I don't think this should have been applied as-is.

>> Signed-off-by: Danny Kukawka <danny.kukawka@bisect.de>
>> ---
>>  drivers/net/ethernet/cirrus/mac89x0.c |   12 ++++++++----
>>  1 files changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/cirrus/mac89x0.c b/drivers/net/ethernet/cirrus/mac89x0.c
>> index 83781f3..a7324ce 100644
>> --- a/drivers/net/ethernet/cirrus/mac89x0.c
>> +++ b/drivers/net/ethernet/cirrus/mac89x0.c
>> @@ -592,10 +592,14 @@ static void set_multicast_list(struct net_device *dev)
>>  static int set_mac_address(struct net_device *dev, void *addr)
>>  {
>>        int i;
>> -       printk("%s: Setting MAC address to ", dev->name);
>> -       for (i = 0; i < 6; i++)
>> -               printk(" %2.2x", dev->dev_addr[i] = ((unsigned char *)addr)[i]);
>> -       printk(".\n");
>> +       struct sockaddr *saddr = addr;
>> +
>> +       if (!is_valid_ether_addr(addr->sa_data))
>> +               return -EADDRNOTAVAIL;
>> +
>> +       memcpy(dev->dev_addr, addr->sa_data, ETH_ALEN);
>> +       printk("%s: Setting MAC address to %pM\n", dev->name, dev->dev_addr);
>> +
>>        /* set the Ethernet address */
>>        for (i=0; i < ETH_ALEN/2; i++)
>>                writereg(dev, PP_IA+i*2, dev->dev_addr[i*2] | (dev->dev_addr[i*2+1] << 8));

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Geert Uytterhoeven - Feb. 28, 2012, 9 p.m.
Hi David,

2012/2/28 David Miller <davem@davemloft.net>:
> From: Geert Uytterhoeven <geert@linux-m68k.org>
> Date: Tue, 28 Feb 2012 21:45:30 +0100
>
>> drivers/net/ethernet/cirrus/mac89x0.c: In function ‘set_mac_address’:
>> drivers/net/ethernet/cirrus/mac89x0.c:597: warning: dereferencing
>> ‘void *’ pointer
>> drivers/net/ethernet/cirrus/mac89x0.c:597: error: request for member
>> ‘sa_data’ in something not a structure or union
>> drivers/net/ethernet/cirrus/mac89x0.c:600: warning: dereferencing
>> ‘void *’ pointer
>> drivers/net/ethernet/cirrus/mac89x0.c:600: error: request for member
>> ‘sa_data’ in something not a structure or union
>> drivers/net/ethernet/cirrus/mac89x0.c:595: warning: unused variable ‘saddr’
>
> Thanks, I've fixed this as follows and pushed to net-next:
>
> --------------------
> mac89x0: Fix build error.
>
> Need to use the new 'saddr' variable not the void 'addr' in
> set_mac_address().
>
> Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
> Signed-off-by: David S. Miller <davem@davemloft.net>

Thanks, that fixed the build.

What about the is_valid_ether_addr() check?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Feb. 28, 2012, 9:06 p.m.
From: Geert Uytterhoeven <geert@linux-m68k.org>
Date: Tue, 28 Feb 2012 22:00:50 +0100

> What about the is_valid_ether_addr() check?

Patches welcome.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Danny Kukawka - Feb. 29, 2012, 7:22 a.m.
On Dienstag, 28. Februar 2012, Geert Uytterhoeven wrote:
> Hi David,
>
> 2012/2/28 David Miller <davem@davemloft.net>:
> > From: Geert Uytterhoeven <geert@linux-m68k.org>
> > Date: Tue, 28 Feb 2012 21:45:30 +0100
> >
> >> drivers/net/ethernet/cirrus/mac89x0.c: In function ‘set_mac_address’:
> >> drivers/net/ethernet/cirrus/mac89x0.c:597: warning: dereferencing
> >> ‘void *’ pointer
> >> drivers/net/ethernet/cirrus/mac89x0.c:597: error: request for member
> >> ‘sa_data’ in something not a structure or union
> >> drivers/net/ethernet/cirrus/mac89x0.c:600: warning: dereferencing
> >> ‘void *’ pointer
> >> drivers/net/ethernet/cirrus/mac89x0.c:600: error: request for member
> >> ‘sa_data’ in something not a structure or union
> >> drivers/net/ethernet/cirrus/mac89x0.c:595: warning: unused variable
> >> ‘saddr’
> >
> > Thanks, I've fixed this as follows and pushed to net-next:
> >
> > --------------------
> > mac89x0: Fix build error.
> >
> > Need to use the new 'saddr' variable not the void 'addr' in
> > set_mac_address().
> >
> > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
> > Signed-off-by: David S. Miller <davem@davemloft.net>
Thanks for the fix!

> Thanks, that fixed the build.
>
> What about the is_valid_ether_addr() check?

If you mean the discussion about checking if the MAC is valid in
.ndo_set_mac_address, I'm on that issue and will hopefully send a patch soon.

Danny
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/net/ethernet/cirrus/mac89x0.c b/drivers/net/ethernet/cirrus/mac89x0.c
index 83781f3..a7324ce 100644
--- a/drivers/net/ethernet/cirrus/mac89x0.c
+++ b/drivers/net/ethernet/cirrus/mac89x0.c
@@ -592,10 +592,14 @@  static void set_multicast_list(struct net_device *dev)
 static int set_mac_address(struct net_device *dev, void *addr)
 {
 	int i;
-	printk("%s: Setting MAC address to ", dev->name);
-	for (i = 0; i < 6; i++)
-		printk(" %2.2x", dev->dev_addr[i] = ((unsigned char *)addr)[i]);
-	printk(".\n");
+	struct sockaddr *saddr = addr;
+
+	if (!is_valid_ether_addr(addr->sa_data))
+		return -EADDRNOTAVAIL;
+
+	memcpy(dev->dev_addr, addr->sa_data, ETH_ALEN);
+	printk("%s: Setting MAC address to %pM\n", dev->name, dev->dev_addr);
+
 	/* set the Ethernet address */
 	for (i=0; i < ETH_ALEN/2; i++)
 		writereg(dev, PP_IA+i*2, dev->dev_addr[i*2] | (dev->dev_addr[i*2+1] << 8));