diff mbox series

ath10k: retrieve MAC address from firmware if provided

Message ID 20180810233948.144792-1-briannorris@chromium.org (mailing list archive)
State New, archived
Headers show
Series ath10k: retrieve MAC address from firmware if provided | expand

Commit Message

Brian Norris Aug. 10, 2018, 11:39 p.m. UTC
Devices may provide their own MAC address via system firmware (e.g.,
device tree), especially in the case where the device doesn't have a
useful EEPROM on which to store its MAC address (e.g., for integrated
Wifi).

Use the generic device helper to retrieve the MAC address, and (if
present) honor it above the MAC address advertised by the card.

Signed-off-by: Brian Norris <briannorris@chromium.org>
---
 drivers/net/wireless/ath/ath10k/core.c | 3 +++
 drivers/net/wireless/ath/ath10k/wmi.c  | 3 ++-
 2 files changed, 5 insertions(+), 1 deletion(-)

Comments

Arend van Spriel Aug. 11, 2018, 6:26 p.m. UTC | #1
On 8/11/2018 1:39 AM, Brian Norris wrote:
> Devices may provide their own MAC address via system firmware (e.g.,

You got me confused by using just "firmware" in the subject.

> device tree), especially in the case where the device doesn't have a
> useful EEPROM on which to store its MAC address (e.g., for integrated
> Wifi).
>
> Use the generic device helper to retrieve the MAC address, and (if
> present) honor it above the MAC address advertised by the card.

But this put me back on track ;-)

> Signed-off-by: Brian Norris <briannorris@chromium.org>
> ---
>   drivers/net/wireless/ath/ath10k/core.c | 3 +++
>   drivers/net/wireless/ath/ath10k/wmi.c  | 3 ++-
>   2 files changed, 5 insertions(+), 1 deletion(-)
Brian Norris Aug. 13, 2018, 5:14 p.m. UTC | #2
On Sat, Aug 11, 2018 at 11:26 AM Arend van Spriel
<arend.vanspriel@broadcom.com> wrote:
>
> On 8/11/2018 1:39 AM, Brian Norris wrote:
> > Devices may provide their own MAC address via system firmware (e.g.,
>
> You got me confused by using just "firmware" in the subject.

Yeah...I started by writing this patch with device tree specifically
(of_get_mac_address()), and then later found that there were generic
"device" helpers for this, which can assist with other sorts of
firmware nodes. It was easier to put a name on a device tree patch
than on a "device" patch. I suppose "system firmware" might be a
better description?

> > device tree), especially in the case where the device doesn't have a
> > useful EEPROM on which to store its MAC address (e.g., for integrated
> > Wifi).
> >
> > Use the generic device helper to retrieve the MAC address, and (if
> > present) honor it above the MAC address advertised by the card.
>
> But this put me back on track ;-)

Let me know if you have a better way to clarify. I can resend with a
slightly modified subject (s/firmware/system firmware/), or let Kalle
do it, if that's the only thing to change.

Brian
Arend van Spriel Aug. 13, 2018, 6:29 p.m. UTC | #3
On 8/13/2018 7:14 PM, Brian Norris wrote:
> On Sat, Aug 11, 2018 at 11:26 AM Arend van Spriel
> <arend.vanspriel@broadcom.com> wrote:
>>
>> On 8/11/2018 1:39 AM, Brian Norris wrote:
>>> Devices may provide their own MAC address via system firmware (e.g.,
>>
>> You got me confused by using just "firmware" in the subject.
>
> Yeah...I started by writing this patch with device tree specifically
> (of_get_mac_address()), and then later found that there were generic
> "device" helpers for this, which can assist with other sorts of
> firmware nodes. It was easier to put a name on a device tree patch
> than on a "device" patch. I suppose "system firmware" might be a
> better description?
>
>>> device tree), especially in the case where the device doesn't have a
>>> useful EEPROM on which to store its MAC address (e.g., for integrated
>>> Wifi).
>>>
>>> Use the generic device helper to retrieve the MAC address, and (if
>>> present) honor it above the MAC address advertised by the card.
>>
>> But this put me back on track ;-)
>
> Let me know if you have a better way to clarify. I can resend with a
> slightly modified subject (s/firmware/system firmware/), or let Kalle
> do it, if that's the only thing to change.

"system firmware" substitution works for me.

Regards,
Arend
Kalle Valo Aug. 28, 2018, 2:33 p.m. UTC | #4
Arend van Spriel <arend.vanspriel@broadcom.com> writes:

> On 8/13/2018 7:14 PM, Brian Norris wrote:
>> On Sat, Aug 11, 2018 at 11:26 AM Arend van Spriel
>> <arend.vanspriel@broadcom.com> wrote:
>>>
>>> On 8/11/2018 1:39 AM, Brian Norris wrote:
>>>> Devices may provide their own MAC address via system firmware (e.g.,
>>>
>>> You got me confused by using just "firmware" in the subject.
>>
>> Yeah...I started by writing this patch with device tree specifically
>> (of_get_mac_address()), and then later found that there were generic
>> "device" helpers for this, which can assist with other sorts of
>> firmware nodes. It was easier to put a name on a device tree patch
>> than on a "device" patch. I suppose "system firmware" might be a
>> better description?
>>
>>>> device tree), especially in the case where the device doesn't have a
>>>> useful EEPROM on which to store its MAC address (e.g., for integrated
>>>> Wifi).
>>>>
>>>> Use the generic device helper to retrieve the MAC address, and (if
>>>> present) honor it above the MAC address advertised by the card.
>>>
>>> But this put me back on track ;-)
>>
>> Let me know if you have a better way to clarify. I can resend with a
>> slightly modified subject (s/firmware/system firmware/), or let Kalle
>> do it, if that's the only thing to change.
>
> "system firmware" substitution works for me.

What about:

ath10k: retrieve MAC address from Device Tree if provided

Because from ath10k point of view we use Device Tree functions and don't
care if it's delivered by pidgeons or by system firmware :)

I can change this before I commit.
Brian Norris Aug. 29, 2018, 12:14 a.m. UTC | #5
On Tue, Aug 28, 2018 at 05:33:01PM +0300, Kalle Valo wrote:
> Arend van Spriel <arend.vanspriel@broadcom.com> writes:
> 
> > On 8/13/2018 7:14 PM, Brian Norris wrote:
> >> On Sat, Aug 11, 2018 at 11:26 AM Arend van Spriel
> >> <arend.vanspriel@broadcom.com> wrote:
> >>>
> >>> On 8/11/2018 1:39 AM, Brian Norris wrote:
> >>>> Devices may provide their own MAC address via system firmware (e.g.,
> >>>
> >>> You got me confused by using just "firmware" in the subject.
> >>
> >> Yeah...I started by writing this patch with device tree specifically
> >> (of_get_mac_address()), and then later found that there were generic
> >> "device" helpers for this, which can assist with other sorts of
> >> firmware nodes. It was easier to put a name on a device tree patch
> >> than on a "device" patch. I suppose "system firmware" might be a
> >> better description?
> >>
> >>>> device tree), especially in the case where the device doesn't have a
> >>>> useful EEPROM on which to store its MAC address (e.g., for integrated
> >>>> Wifi).
> >>>>
> >>>> Use the generic device helper to retrieve the MAC address, and (if
> >>>> present) honor it above the MAC address advertised by the card.
> >>>
> >>> But this put me back on track ;-)
> >>
> >> Let me know if you have a better way to clarify. I can resend with a
> >> slightly modified subject (s/firmware/system firmware/), or let Kalle
> >> do it, if that's the only thing to change.
> >
> > "system firmware" substitution works for me.
> 
> What about:
> 
> ath10k: retrieve MAC address from Device Tree if provided
> 
> Because from ath10k point of view we use Device Tree functions and don't
> care if it's delivered by pidgeons or by system firmware :)

I don't care too much, but note that Device Tree is a loaded term,
usually referring specifically to a method of describing system hardware
via the Flattened Device Tree format. If I were specifically targeting
Device Tree, I'd use helpers like of_get_mac_address() instead. (The
'of_*' prefix is a relic of OpenFirmware, an early firmware
implementation that used the Device Tree format.)

If you're trying to say "device tree" to refer to "the Linux device
hierarchy", then that's also a fair description.

But that's all starting to mince words. Device Tree (with or without
capitalization) is fine with me.

Thanks,
Brian

> I can change this before I commit.
> 
> -- 
> Kalle Valo
Kalle Valo Sept. 3, 2018, 4:49 p.m. UTC | #6
Brian Norris <briannorris@chromium.org> writes:

> On Tue, Aug 28, 2018 at 05:33:01PM +0300, Kalle Valo wrote:
>> Arend van Spriel <arend.vanspriel@broadcom.com> writes:
>> 
>> > On 8/13/2018 7:14 PM, Brian Norris wrote:
>> >> On Sat, Aug 11, 2018 at 11:26 AM Arend van Spriel
>> >> <arend.vanspriel@broadcom.com> wrote:
>> >>>
>> >>> On 8/11/2018 1:39 AM, Brian Norris wrote:
>> >>>> Devices may provide their own MAC address via system firmware (e.g.,
>> >>>
>> >>> You got me confused by using just "firmware" in the subject.
>> >>
>> >> Yeah...I started by writing this patch with device tree specifically
>> >> (of_get_mac_address()), and then later found that there were generic
>> >> "device" helpers for this, which can assist with other sorts of
>> >> firmware nodes. It was easier to put a name on a device tree patch
>> >> than on a "device" patch. I suppose "system firmware" might be a
>> >> better description?
>> >>
>> >>>> device tree), especially in the case where the device doesn't have a
>> >>>> useful EEPROM on which to store its MAC address (e.g., for integrated
>> >>>> Wifi).
>> >>>>
>> >>>> Use the generic device helper to retrieve the MAC address, and (if
>> >>>> present) honor it above the MAC address advertised by the card.
>> >>>
>> >>> But this put me back on track ;-)
>> >>
>> >> Let me know if you have a better way to clarify. I can resend with a
>> >> slightly modified subject (s/firmware/system firmware/), or let Kalle
>> >> do it, if that's the only thing to change.
>> >
>> > "system firmware" substitution works for me.
>> 
>> What about:
>> 
>> ath10k: retrieve MAC address from Device Tree if provided
>> 
>> Because from ath10k point of view we use Device Tree functions and don't
>> care if it's delivered by pidgeons or by system firmware :)
>
> I don't care too much, but note that Device Tree is a loaded term,
> usually referring specifically to a method of describing system hardware
> via the Flattened Device Tree format. If I were specifically targeting
> Device Tree, I'd use helpers like of_get_mac_address() instead. (The
> 'of_*' prefix is a relic of OpenFirmware, an early firmware
> implementation that used the Device Tree format.)

My bad, I read your patch hastily and somehow understood you were using
of_get_mac_address() but you were actually using
device_get_mac_address(). So I'll change this to "system firmware" as
originally suggested.

Sorry for the confusion.
Kalle Valo Sept. 3, 2018, 4:54 p.m. UTC | #7
Brian Norris <briannorris@chromium.org> wrote:

> Devices may provide their own MAC address via system firmware (e.g.,
> device tree), especially in the case where the device doesn't have a
> useful EEPROM on which to store its MAC address (e.g., for integrated
> Wifi).
> 
> Use the generic device helper to retrieve the MAC address, and (if
> present) honor it above the MAC address advertised by the card.
> 
> Signed-off-by: Brian Norris <briannorris@chromium.org>
> Signed-off-by: Kalle Valo <kvalo@codeaurora.org>

Patch applied to ath-next branch of ath.git, thanks.

9d5804662ce1 ath10k: retrieve MAC address from system firmware if provided
diff mbox series

Patch

diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c
index c40cd129afe7..840c1f039098 100644
--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -19,6 +19,7 @@ 
 #include <linux/module.h>
 #include <linux/firmware.h>
 #include <linux/of.h>
+#include <linux/property.h>
 #include <linux/dmi.h>
 #include <linux/ctype.h>
 #include <asm/byteorder.h>
@@ -2602,6 +2603,8 @@  static int ath10k_core_probe_fw(struct ath10k *ar)
 		ath10k_debug_print_board_info(ar);
 	}
 
+	device_get_mac_address(ar->dev, ar->mac_addr, sizeof(ar->mac_addr));
+
 	ret = ath10k_core_init_firmware_features(ar);
 	if (ret) {
 		ath10k_err(ar, "fatal problem with firmware features: %d\n",
diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
index fd612d2905b0..3cfd98de8ad2 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.c
+++ b/drivers/net/wireless/ath/ath10k/wmi.c
@@ -5449,7 +5449,8 @@  int ath10k_wmi_event_ready(struct ath10k *ar, struct sk_buff *skb)
 		   arg.mac_addr,
 		   __le32_to_cpu(arg.status));
 
-	ether_addr_copy(ar->mac_addr, arg.mac_addr);
+	if (is_zero_ether_addr(ar->mac_addr))
+		ether_addr_copy(ar->mac_addr, arg.mac_addr);
 	complete(&ar->wmi.unified_ready);
 	return 0;
 }