Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1572027 > unrolled thread

[PATCH 0/2] net: ethernet: bgmac: bug fixes

Started byJon Mason <jon.mason@broadcom.com>
First post2017-02-01 23:40 +0100
Last post2017-02-02 11:00 +0100
Articles 12 — 4 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/2] net: ethernet: bgmac: bug fixes Jon Mason <jon.mason@broadcom.com> - 2017-02-01 23:40 +0100
    [PATCH 1/2] net: ethernet: bgmac: init sequence bug Jon Mason <jon.mason@broadcom.com> - 2017-02-01 23:40 +0100
      Re: [PATCH 1/2] net: ethernet: bgmac: init sequence bug Rafał Miłecki <rafal@milecki.pl> - 2017-02-02 00:50 +0100
        Re: [PATCH 1/2] net: ethernet: bgmac: init sequence bug Zac Schroff <zachary.schroff@broadcom.com> - 2017-02-02 01:40 +0100
          Re: [PATCH 1/2] net: ethernet: bgmac: init sequence bug Jon Mason <jon.mason@broadcom.com> - 2017-02-02 21:30 +0100
          Re: [PATCH 1/2] net: ethernet: bgmac: init sequence bug Rafał Miłecki <rafal@milecki.pl> - 2017-02-03 03:20 +0100
        Re: [PATCH 1/2] net: ethernet: bgmac: init sequence bug Jon Mason <jon.mason@broadcom.com> - 2017-02-02 20:00 +0100
      Re: [PATCH 1/2] net: ethernet: bgmac: init sequence bug Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> - 2017-02-02 10:50 +0100
    [PATCH 2/2] net: ethernet: bgmac: mac address change bug Jon Mason <jon.mason@broadcom.com> - 2017-02-01 23:50 +0100
      Re: [PATCH 2/2] net: ethernet: bgmac: mac address change bug Rafał Miłecki <rafal@milecki.pl> - 2017-02-02 01:30 +0100
        Re: [PATCH 2/2] net: ethernet: bgmac: mac address change bug Jon Mason <jon.mason@broadcom.com> - 2017-02-02 19:20 +0100
      Re: [PATCH 2/2] net: ethernet: bgmac: mac address change bug Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> - 2017-02-02 11:00 +0100

#1572027 — [PATCH 0/2] net: ethernet: bgmac: bug fixes

FromJon Mason <jon.mason@broadcom.com>
Date2017-02-01 23:40 +0100
Subject[PATCH 0/2] net: ethernet: bgmac: bug fixes
Message-ID<t6bJf-3KV-15@gated-at.bofh.it>
Bug fixes for bgmac driver

Hari Vyas (1):
  net: ethernet: bgmac: mac address change bug

Zac Schroff (1):
  net: ethernet: bgmac: init sequence bug

 drivers/net/ethernet/broadcom/bgmac-platform.c | 10 +++++++---
 drivers/net/ethernet/broadcom/bgmac.c          |  6 +++++-
 include/linux/bcma/bcma_regs.h                 |  1 +
 3 files changed, 13 insertions(+), 4 deletions(-)

-- 
2.7.4

[toc] | [next] | [standalone]

#1572028 — [PATCH 1/2] net: ethernet: bgmac: init sequence bug

FromJon Mason <jon.mason@broadcom.com>
Date2017-02-01 23:40 +0100
Subject[PATCH 1/2] net: ethernet: bgmac: init sequence bug
Message-ID<t6bJg-3KV-21@gated-at.bofh.it>
In reply to#1572027
From: Zac Schroff <zschroff@broadcom.com>

Fix a bug in the 'bgmac' driver init sequence that blind writes for init
sequence where it should preserve most bits other than the ones it is
deliberately manipulating.

Signed-off-by: Zac Schroff <zschroff@broadcom.com>
Signed-off-by: Jon Mason <jon.mason@broadcom.com>
Fixes: f6a95a24957 ("net: ethernet: bgmac: Add platform device support")
---
 drivers/net/ethernet/broadcom/bgmac-platform.c | 10 +++++++---
 include/linux/bcma/bcma_regs.h                 |  1 +
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bgmac-platform.c b/drivers/net/ethernet/broadcom/bgmac-platform.c
index 6f736c1..9bbe05c 100644
--- a/drivers/net/ethernet/broadcom/bgmac-platform.c
+++ b/drivers/net/ethernet/broadcom/bgmac-platform.c
@@ -61,15 +61,19 @@ static bool platform_bgmac_clk_enabled(struct bgmac *bgmac)
 
 static void platform_bgmac_clk_enable(struct bgmac *bgmac, u32 flags)
 {
-	bgmac_idm_write(bgmac, BCMA_IOCTL,
-			(BCMA_IOCTL_CLK | BCMA_IOCTL_FGC | flags));
+	u32 regval;
+
+	/* Some bits of BCMA_IOCTL set by HW/ATF & should not change */
+	regval = bgmac_idm_read(bgmac, BCMA_IOCTL) & BCMA_IOCTL_DO_NOT_MODIFY;
+	regval |= ((flags & (~BCMA_IOCTL_DO_NOT_MODIFY)) | BCMA_IOCTL_CLK);
+	bgmac_idm_write(bgmac, BCMA_IOCTL, regval | BCMA_IOCTL_FGC);
 	bgmac_idm_read(bgmac, BCMA_IOCTL);
 
 	bgmac_idm_write(bgmac, BCMA_RESET_CTL, 0);
 	bgmac_idm_read(bgmac, BCMA_RESET_CTL);
 	udelay(1);
 
-	bgmac_idm_write(bgmac, BCMA_IOCTL, (BCMA_IOCTL_CLK | flags));
+	bgmac_idm_write(bgmac, BCMA_IOCTL, regval);
 	bgmac_idm_read(bgmac, BCMA_IOCTL);
 	udelay(1);
 }
diff --git a/include/linux/bcma/bcma_regs.h b/include/linux/bcma/bcma_regs.h
index 9986f82..41d7404 100644
--- a/include/linux/bcma/bcma_regs.h
+++ b/include/linux/bcma/bcma_regs.h
@@ -31,6 +31,7 @@
 #define  BCMA_IOCTL_CORE_BITS		0x3FFC
 #define  BCMA_IOCTL_PME_EN		0x4000
 #define  BCMA_IOCTL_BIST_EN		0x8000
+#define  BCMA_IOCTL_DO_NOT_MODIFY	0x7FFFFF80
 #define BCMA_IOST			0x0500 /* IO status */
 #define  BCMA_IOST_CORE_BITS		0x0FFF
 #define  BCMA_IOST_DMA64		0x1000
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1572073 — Re: [PATCH 1/2] net: ethernet: bgmac: init sequence bug

FromRafał Miłecki <rafal@milecki.pl>
Date2017-02-02 00:50 +0100
SubjectRe: [PATCH 1/2] net: ethernet: bgmac: init sequence bug
Message-ID<t6cOZ-4om-9@gated-at.bofh.it>
In reply to#1572028
On 02/01/2017 11:39 PM, Jon Mason wrote:
> From: Zac Schroff <zschroff@broadcom.com>
>
> Fix a bug in the 'bgmac' driver init sequence that blind writes for init
> sequence where it should preserve most bits other than the ones it is
> deliberately manipulating.
>
> Signed-off-by: Zac Schroff <zschroff@broadcom.com>
> Signed-off-by: Jon Mason <jon.mason@broadcom.com>
> Fixes: f6a95a24957 ("net: ethernet: bgmac: Add platform device support")
> ---
>  drivers/net/ethernet/broadcom/bgmac-platform.c | 10 +++++++---
>  include/linux/bcma/bcma_regs.h                 |  1 +
>  2 files changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/bgmac-platform.c b/drivers/net/ethernet/broadcom/bgmac-platform.c
> index 6f736c1..9bbe05c 100644
> --- a/drivers/net/ethernet/broadcom/bgmac-platform.c
> +++ b/drivers/net/ethernet/broadcom/bgmac-platform.c
> @@ -61,15 +61,19 @@ static bool platform_bgmac_clk_enabled(struct bgmac *bgmac)
>
>  static void platform_bgmac_clk_enable(struct bgmac *bgmac, u32 flags)
>  {
> -	bgmac_idm_write(bgmac, BCMA_IOCTL,
> -			(BCMA_IOCTL_CLK | BCMA_IOCTL_FGC | flags));
> +	u32 regval;
> +
> +	/* Some bits of BCMA_IOCTL set by HW/ATF & should not change */
> +	regval = bgmac_idm_read(bgmac, BCMA_IOCTL) & BCMA_IOCTL_DO_NOT_MODIFY;
> +	regval |= ((flags & (~BCMA_IOCTL_DO_NOT_MODIFY)) | BCMA_IOCTL_CLK);

You don't need these braces around whole calculation.
This should work the same:
(flags & (~BCMA_IOCTL_DO_NOT_MODIFY)) | BCMA_IOCTL_CLK


> +	bgmac_idm_write(bgmac, BCMA_IOCTL, regval | BCMA_IOCTL_FGC);
>  	bgmac_idm_read(bgmac, BCMA_IOCTL);
>
>  	bgmac_idm_write(bgmac, BCMA_RESET_CTL, 0);
>  	bgmac_idm_read(bgmac, BCMA_RESET_CTL);
>  	udelay(1);
>
> -	bgmac_idm_write(bgmac, BCMA_IOCTL, (BCMA_IOCTL_CLK | flags));
> +	bgmac_idm_write(bgmac, BCMA_IOCTL, regval);
>  	bgmac_idm_read(bgmac, BCMA_IOCTL);
>  	udelay(1);
>  }
> diff --git a/include/linux/bcma/bcma_regs.h b/include/linux/bcma/bcma_regs.h
> index 9986f82..41d7404 100644
> --- a/include/linux/bcma/bcma_regs.h
> +++ b/include/linux/bcma/bcma_regs.h
> @@ -31,6 +31,7 @@
>  #define  BCMA_IOCTL_CORE_BITS		0x3FFC
>  #define  BCMA_IOCTL_PME_EN		0x4000
>  #define  BCMA_IOCTL_BIST_EN		0x8000
> +#define  BCMA_IOCTL_DO_NOT_MODIFY	0x7FFFFF80

This sounds like a pretty bad name.

Take a look at brcmsmac and SICF_*:
http://lxr.free-electrons.com/source/drivers/net/wireless/broadcom/brcm80211/brcmsmac/d11.h?v=4.9#L1737

Or b43 and B43_BCMA_IOCTL_*:
http://lxr.free-electrons.com/source/drivers/net/wireless/broadcom/b43/b43.h?v=4.9#L494

Both drives modify bits you marked as DO_NOT_MODIFY and they are OK.

[toc] | [prev] | [next] | [standalone]

#1572094 — Re: [PATCH 1/2] net: ethernet: bgmac: init sequence bug

FromZac Schroff <zachary.schroff@broadcom.com>
Date2017-02-02 01:40 +0100
SubjectRe: [PATCH 1/2] net: ethernet: bgmac: init sequence bug
Message-ID<t6dBn-4UR-3@gated-at.bofh.it>
In reply to#1572073
How about BCMA_IOCTL_PRESERVE_ACROSS_INIT?

On Wed, Feb 1, 2017 at 6:06 PM, Rafał Miłecki <rafal@milecki.pl> wrote:
> On 02/01/2017 11:39 PM, Jon Mason wrote:
>>
>> From: Zac Schroff <zschroff@broadcom.com>
>>
>> Fix a bug in the 'bgmac' driver init sequence that blind writes for init
>> sequence where it should preserve most bits other than the ones it is
>> deliberately manipulating.
>>
>> Signed-off-by: Zac Schroff <zschroff@broadcom.com>
>> Signed-off-by: Jon Mason <jon.mason@broadcom.com>
>> Fixes: f6a95a24957 ("net: ethernet: bgmac: Add platform device support")
>> ---
>>  drivers/net/ethernet/broadcom/bgmac-platform.c | 10 +++++++---
>>  include/linux/bcma/bcma_regs.h                 |  1 +
>>  2 files changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/broadcom/bgmac-platform.c
>> b/drivers/net/ethernet/broadcom/bgmac-platform.c
>> index 6f736c1..9bbe05c 100644
>> --- a/drivers/net/ethernet/broadcom/bgmac-platform.c
>> +++ b/drivers/net/ethernet/broadcom/bgmac-platform.c
>> @@ -61,15 +61,19 @@ static bool platform_bgmac_clk_enabled(struct bgmac
>> *bgmac)
>>
>>  static void platform_bgmac_clk_enable(struct bgmac *bgmac, u32 flags)
>>  {
>> -       bgmac_idm_write(bgmac, BCMA_IOCTL,
>> -                       (BCMA_IOCTL_CLK | BCMA_IOCTL_FGC | flags));
>> +       u32 regval;
>> +
>> +       /* Some bits of BCMA_IOCTL set by HW/ATF & should not change */
>> +       regval = bgmac_idm_read(bgmac, BCMA_IOCTL) &
>> BCMA_IOCTL_DO_NOT_MODIFY;
>> +       regval |= ((flags & (~BCMA_IOCTL_DO_NOT_MODIFY)) |
>> BCMA_IOCTL_CLK);
>
>
> You don't need these braces around whole calculation.
> This should work the same:
> (flags & (~BCMA_IOCTL_DO_NOT_MODIFY)) | BCMA_IOCTL_CLK
>
>
>> +       bgmac_idm_write(bgmac, BCMA_IOCTL, regval | BCMA_IOCTL_FGC);
>>         bgmac_idm_read(bgmac, BCMA_IOCTL);
>>
>>         bgmac_idm_write(bgmac, BCMA_RESET_CTL, 0);
>>         bgmac_idm_read(bgmac, BCMA_RESET_CTL);
>>         udelay(1);
>>
>> -       bgmac_idm_write(bgmac, BCMA_IOCTL, (BCMA_IOCTL_CLK | flags));
>> +       bgmac_idm_write(bgmac, BCMA_IOCTL, regval);
>>         bgmac_idm_read(bgmac, BCMA_IOCTL);
>>         udelay(1);
>>  }
>> diff --git a/include/linux/bcma/bcma_regs.h
>> b/include/linux/bcma/bcma_regs.h
>> index 9986f82..41d7404 100644
>> --- a/include/linux/bcma/bcma_regs.h
>> +++ b/include/linux/bcma/bcma_regs.h
>> @@ -31,6 +31,7 @@
>>  #define  BCMA_IOCTL_CORE_BITS          0x3FFC
>>  #define  BCMA_IOCTL_PME_EN             0x4000
>>  #define  BCMA_IOCTL_BIST_EN            0x8000
>> +#define  BCMA_IOCTL_DO_NOT_MODIFY      0x7FFFFF80
>
>
> This sounds like a pretty bad name.
>
> Take a look at brcmsmac and SICF_*:
> http://lxr.free-electrons.com/source/drivers/net/wireless/broadcom/brcm80211/brcmsmac/d11.h?v=4.9#L1737
>
> Or b43 and B43_BCMA_IOCTL_*:
> http://lxr.free-electrons.com/source/drivers/net/wireless/broadcom/b43/b43.h?v=4.9#L494
>
> Both drives modify bits you marked as DO_NOT_MODIFY and they are OK.

[toc] | [prev] | [next] | [standalone]

#1572731 — Re: [PATCH 1/2] net: ethernet: bgmac: init sequence bug

FromJon Mason <jon.mason@broadcom.com>
Date2017-02-02 21:30 +0100
SubjectRe: [PATCH 1/2] net: ethernet: bgmac: init sequence bug
Message-ID<t6wb0-hZ-11@gated-at.bofh.it>
In reply to#1572094
On Thu, Feb 2, 2017 at 3:15 PM, Rafał Miłecki <rafal@milecki.pl> wrote:
> On 2017-02-02 01:31, Zac Schroff wrote:
>>
>> How about BCMA_IOCTL_PRESERVE_ACROSS_INIT?
>
>
> I think wireless drivers may still set some these bits during init.
>
> I've a simpler idea: make it bgmac specific. Call it sth like
> BGMAC_BCMA_IOCTL_PRESERVE
> BGMAC_BCMA_IOCTL_RESERVED
> BGMAC_BCMA_IOCTL_DONT_TOUCH

Yes, I am listing out all of the fields in that register.  We can be
intelligent about what we mask off :)

[toc] | [prev] | [next] | [standalone]

#1572855 — Re: [PATCH 1/2] net: ethernet: bgmac: init sequence bug

FromRafał Miłecki <rafal@milecki.pl>
Date2017-02-03 03:20 +0100
SubjectRe: [PATCH 1/2] net: ethernet: bgmac: init sequence bug
Message-ID<t6wb0-hZ-13@gated-at.bofh.it>
In reply to#1572094
On 2017-02-02 01:31, Zac Schroff wrote:
> How about BCMA_IOCTL_PRESERVE_ACROSS_INIT?

I think wireless drivers may still set some these bits during init.

I've a simpler idea: make it bgmac specific. Call it sth like
BGMAC_BCMA_IOCTL_PRESERVE
BGMAC_BCMA_IOCTL_RESERVED
BGMAC_BCMA_IOCTL_DONT_TOUCH

[toc] | [prev] | [next] | [standalone]

#1572681 — Re: [PATCH 1/2] net: ethernet: bgmac: init sequence bug

FromJon Mason <jon.mason@broadcom.com>
Date2017-02-02 20:00 +0100
SubjectRe: [PATCH 1/2] net: ethernet: bgmac: init sequence bug
Message-ID<t6uLW-7JC-47@gated-at.bofh.it>
In reply to#1572073
On Wed, Feb 1, 2017 at 6:06 PM, Rafał Miłecki <rafal@milecki.pl> wrote:
> On 02/01/2017 11:39 PM, Jon Mason wrote:
>>
>> From: Zac Schroff <zschroff@broadcom.com>
>>
>> Fix a bug in the 'bgmac' driver init sequence that blind writes for init
>> sequence where it should preserve most bits other than the ones it is
>> deliberately manipulating.
>>
>> Signed-off-by: Zac Schroff <zschroff@broadcom.com>
>> Signed-off-by: Jon Mason <jon.mason@broadcom.com>
>> Fixes: f6a95a24957 ("net: ethernet: bgmac: Add platform device support")
>> ---
>>  drivers/net/ethernet/broadcom/bgmac-platform.c | 10 +++++++---
>>  include/linux/bcma/bcma_regs.h                 |  1 +
>>  2 files changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/broadcom/bgmac-platform.c
>> b/drivers/net/ethernet/broadcom/bgmac-platform.c
>> index 6f736c1..9bbe05c 100644
>> --- a/drivers/net/ethernet/broadcom/bgmac-platform.c
>> +++ b/drivers/net/ethernet/broadcom/bgmac-platform.c
>> @@ -61,15 +61,19 @@ static bool platform_bgmac_clk_enabled(struct bgmac
>> *bgmac)
>>
>>  static void platform_bgmac_clk_enable(struct bgmac *bgmac, u32 flags)
>>  {
>> -       bgmac_idm_write(bgmac, BCMA_IOCTL,
>> -                       (BCMA_IOCTL_CLK | BCMA_IOCTL_FGC | flags));
>> +       u32 regval;
>> +
>> +       /* Some bits of BCMA_IOCTL set by HW/ATF & should not change */
>> +       regval = bgmac_idm_read(bgmac, BCMA_IOCTL) &
>> BCMA_IOCTL_DO_NOT_MODIFY;
>> +       regval |= ((flags & (~BCMA_IOCTL_DO_NOT_MODIFY)) |
>> BCMA_IOCTL_CLK);
>
>
> You don't need these braces around whole calculation.
> This should work the same:
> (flags & (~BCMA_IOCTL_DO_NOT_MODIFY)) | BCMA_IOCTL_CLK

Fair enough

>
>
>> +       bgmac_idm_write(bgmac, BCMA_IOCTL, regval | BCMA_IOCTL_FGC);
>>         bgmac_idm_read(bgmac, BCMA_IOCTL);
>>
>>         bgmac_idm_write(bgmac, BCMA_RESET_CTL, 0);
>>         bgmac_idm_read(bgmac, BCMA_RESET_CTL);
>>         udelay(1);
>>
>> -       bgmac_idm_write(bgmac, BCMA_IOCTL, (BCMA_IOCTL_CLK | flags));
>> +       bgmac_idm_write(bgmac, BCMA_IOCTL, regval);
>>         bgmac_idm_read(bgmac, BCMA_IOCTL);
>>         udelay(1);
>>  }
>> diff --git a/include/linux/bcma/bcma_regs.h
>> b/include/linux/bcma/bcma_regs.h
>> index 9986f82..41d7404 100644
>> --- a/include/linux/bcma/bcma_regs.h
>> +++ b/include/linux/bcma/bcma_regs.h
>> @@ -31,6 +31,7 @@
>>  #define  BCMA_IOCTL_CORE_BITS          0x3FFC
>>  #define  BCMA_IOCTL_PME_EN             0x4000
>>  #define  BCMA_IOCTL_BIST_EN            0x8000
>> +#define  BCMA_IOCTL_DO_NOT_MODIFY      0x7FFFFF80
>
>
> This sounds like a pretty bad name.

Name change coming

> Take a look at brcmsmac and SICF_*:
> http://lxr.free-electrons.com/source/drivers/net/wireless/broadcom/brcm80211/brcmsmac/d11.h?v=4.9#L1737
>
> Or b43 and B43_BCMA_IOCTL_*:
> http://lxr.free-electrons.com/source/drivers/net/wireless/broadcom/b43/b43.h?v=4.9#L494
>
> Both drives modify bits you marked as DO_NOT_MODIFY and they are OK.

I think the point Zac was trying to make is that this is changing bits
that aren't meaning to be modified.  We should only be flipping the
bits necessary to enable the clocks, etc.  Bootloaders, etc might be
setting bits (and in our case they are) which are being removed
forcing it to a predefined value.

Thanks,
Jon

[toc] | [prev] | [next] | [standalone]

#1572227 — Re: [PATCH 1/2] net: ethernet: bgmac: init sequence bug

FromSergei Shtylyov <sergei.shtylyov@cogentembedded.com>
Date2017-02-02 10:50 +0100
SubjectRe: [PATCH 1/2] net: ethernet: bgmac: init sequence bug
Message-ID<t6mbD-26H-13@gated-at.bofh.it>
In reply to#1572028
Hello!

On 2/2/2017 1:39 AM, Jon Mason wrote:

> From: Zac Schroff <zschroff@broadcom.com>
>
> Fix a bug in the 'bgmac' driver init sequence that blind writes for init
> sequence where it should preserve most bits other than the ones it is
> deliberately manipulating.
>
> Signed-off-by: Zac Schroff <zschroff@broadcom.com>
> Signed-off-by: Jon Mason <jon.mason@broadcom.com>
> Fixes: f6a95a24957 ("net: ethernet: bgmac: Add platform device support")
> ---
>  drivers/net/ethernet/broadcom/bgmac-platform.c | 10 +++++++---
>  include/linux/bcma/bcma_regs.h                 |  1 +
>  2 files changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/bgmac-platform.c b/drivers/net/ethernet/broadcom/bgmac-platform.c
> index 6f736c1..9bbe05c 100644
> --- a/drivers/net/ethernet/broadcom/bgmac-platform.c
> +++ b/drivers/net/ethernet/broadcom/bgmac-platform.c
> @@ -61,15 +61,19 @@ static bool platform_bgmac_clk_enabled(struct bgmac *bgmac)
>
>  static void platform_bgmac_clk_enable(struct bgmac *bgmac, u32 flags)
>  {
> -	bgmac_idm_write(bgmac, BCMA_IOCTL,
> -			(BCMA_IOCTL_CLK | BCMA_IOCTL_FGC | flags));
> +	u32 regval;
> +
> +	/* Some bits of BCMA_IOCTL set by HW/ATF & should not change */
> +	regval = bgmac_idm_read(bgmac, BCMA_IOCTL) & BCMA_IOCTL_DO_NOT_MODIFY;
> +	regval |= ((flags & (~BCMA_IOCTL_DO_NOT_MODIFY)) | BCMA_IOCTL_CLK);

    The innermost parens aren't necessary. And the outermost as well.

[...]

MBR, Sergei

[toc] | [prev] | [next] | [standalone]

#1572031 — [PATCH 2/2] net: ethernet: bgmac: mac address change bug

FromJon Mason <jon.mason@broadcom.com>
Date2017-02-01 23:50 +0100
Subject[PATCH 2/2] net: ethernet: bgmac: mac address change bug
Message-ID<t6bSV-3Oo-5@gated-at.bofh.it>
In reply to#1572027
From: Hari Vyas <hariv@broadcom.com>

ndo_set_mac_address() passes struct sockaddr * as 2nd parameter to
bgmac_set_mac_address() but code assumed u8 *.  This caused two bytes
chopping and the wrong mac address was configured.

Signed-off-by: Hari Vyas <hariv@broadcom.com>
Signed-off-by: Jon Mason <jon.mason@broadcom.com>
Fixes: 4e209001b86 ("bgmac: write mac address to hardware in ndo_set_mac_address")
---
 drivers/net/ethernet/broadcom/bgmac.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
index 0e066dc6..ea24072 100644
--- a/drivers/net/ethernet/broadcom/bgmac.c
+++ b/drivers/net/ethernet/broadcom/bgmac.c
@@ -1222,11 +1222,15 @@ static int bgmac_set_mac_address(struct net_device *net_dev, void *addr)
 {
 	struct bgmac *bgmac = netdev_priv(net_dev);
 	int ret;
+	struct sockaddr *sa = addr;
 
 	ret = eth_prepare_mac_addr_change(net_dev, addr);
 	if (ret < 0)
 		return ret;
-	bgmac_write_mac_address(bgmac, (u8 *)addr);
+
+	ether_addr_copy(bgmac->mac_addr, sa->sa_data);
+	bgmac_write_mac_address(bgmac, bgmac->mac_addr);
+
 	eth_commit_mac_addr_change(net_dev, addr);
 	return 0;
 }
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1572093 — Re: [PATCH 2/2] net: ethernet: bgmac: mac address change bug

FromRafał Miłecki <rafal@milecki.pl>
Date2017-02-02 01:30 +0100
SubjectRe: [PATCH 2/2] net: ethernet: bgmac: mac address change bug
Message-ID<t6drH-4QO-1@gated-at.bofh.it>
In reply to#1572031
On 02/01/2017 11:39 PM, Jon Mason wrote:
> From: Hari Vyas <hariv@broadcom.com>
>
> ndo_set_mac_address() passes struct sockaddr * as 2nd parameter to
> bgmac_set_mac_address() but code assumed u8 *.  This caused two bytes
> chopping and the wrong mac address was configured.
>
> Signed-off-by: Hari Vyas <hariv@broadcom.com>
> Signed-off-by: Jon Mason <jon.mason@broadcom.com>
> Fixes: 4e209001b86 ("bgmac: write mac address to hardware in ndo_set_mac_address")

Sounds OK, would it make sense to Cc stable?

[toc] | [prev] | [next] | [standalone]

#1572580 — Re: [PATCH 2/2] net: ethernet: bgmac: mac address change bug

FromJon Mason <jon.mason@broadcom.com>
Date2017-02-02 19:20 +0100
SubjectRe: [PATCH 2/2] net: ethernet: bgmac: mac address change bug
Message-ID<t6u9c-7tT-3@gated-at.bofh.it>
In reply to#1572093
On Wed, Feb 1, 2017 at 6:12 PM, Rafał Miłecki <rafal@milecki.pl> wrote:
> On 02/01/2017 11:39 PM, Jon Mason wrote:
>>
>> From: Hari Vyas <hariv@broadcom.com>
>>
>> ndo_set_mac_address() passes struct sockaddr * as 2nd parameter to
>> bgmac_set_mac_address() but code assumed u8 *.  This caused two bytes
>> chopping and the wrong mac address was configured.
>>
>> Signed-off-by: Hari Vyas <hariv@broadcom.com>
>> Signed-off-by: Jon Mason <jon.mason@broadcom.com>
>> Fixes: 4e209001b86 ("bgmac: write mac address to hardware in
>> ndo_set_mac_address")
>
>
> Sounds OK, would it make sense to Cc stable?

Sure, I'll add Sergei's mods and do a v2 with stable on the Cc list

[toc] | [prev] | [next] | [standalone]

#1572230 — Re: [PATCH 2/2] net: ethernet: bgmac: mac address change bug

FromSergei Shtylyov <sergei.shtylyov@cogentembedded.com>
Date2017-02-02 11:00 +0100
SubjectRe: [PATCH 2/2] net: ethernet: bgmac: mac address change bug
Message-ID<t6mlk-2a2-5@gated-at.bofh.it>
In reply to#1572031
On 2/2/2017 1:39 AM, Jon Mason wrote:

> From: Hari Vyas <hariv@broadcom.com>
>
> ndo_set_mac_address() passes struct sockaddr * as 2nd parameter to
> bgmac_set_mac_address() but code assumed u8 *.  This caused two bytes
> chopping and the wrong mac address was configured.
>
> Signed-off-by: Hari Vyas <hariv@broadcom.com>
> Signed-off-by: Jon Mason <jon.mason@broadcom.com>
> Fixes: 4e209001b86 ("bgmac: write mac address to hardware in ndo_set_mac_address")
> ---
>  drivers/net/ethernet/broadcom/bgmac.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
> index 0e066dc6..ea24072 100644
> --- a/drivers/net/ethernet/broadcom/bgmac.c
> +++ b/drivers/net/ethernet/broadcom/bgmac.c
> @@ -1222,11 +1222,15 @@ static int bgmac_set_mac_address(struct net_device *net_dev, void *addr)
>  {
>  	struct bgmac *bgmac = netdev_priv(net_dev);
>  	int ret;
> +	struct sockaddr *sa = addr;

    DaveM prefers the declarations to be arranged from longest to shortest.

[...]

MBR, Sergei

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web