diff mbox series

ath10k: implement set_base_macaddr to fix rx-bssid mask in multiple APs conf

Message ID 20190114153558.972-1-chunkeey@gmail.com (mailing list archive)
State New, archived
Headers show
Series ath10k: implement set_base_macaddr to fix rx-bssid mask in multiple APs conf | expand

Commit Message

Christian Lamparter Jan. 14, 2019, 3:35 p.m. UTC
Many integrated QCA9984 WiFis in various IPQ806x platform routers
from various vendors (Netgear R7800, ZyXEL NBG6817, TP-LINK C2600,
etc.) have either blank, bogus or non-unique MAC-addresses in
their calibration data.

As a result, OpenWrt utilizes a discouraged binary calibration data
patching method that allows to modify the device's MAC-addresses right
at the source. This is because the ath10k' firmware extracts the MAC
address from the supplied radio/calibration data and issues a response
to the ath10k linux driver. Which was designed to take the main MAC in
ath10k_wmi_event_ready().

Part of the "setting an alternate MAC" issue was already tackled by a
patch from Brian Norris:
commit 9d5804662ce1
("ath10k: retrieve MAC address from system firmware if provided")
by allowing the option to specify an alternate MAC-address with the
established device_get_mac_address() function which extracts the right
address from DeviceTree/fwnode mac-address or local-mac-address
properties and saves it for later.

However, Ben Greear noted that the Qualcomm's ath10k firmware is liable
to not properly calculate its rx-bssid mask in this case. This can cause
issues in the popluar "multiple AP with a single ath10k instance"
configurations.

To improve MAC address handling, Felix Fietkau suggested to call
pdev_set_base_macaddr_cmdid before bringing up the first vif and
use the first vif MAC address there. Which is in ath10k_core_start().

This patch implement Felix Fietkau's request to
"call pdev_set_base_macaddr_cmdid before bringing up the first vif".
The pdev_set_base_macaddr_cmdid is already declared for all devices
and version. The driver just needed the support code for this
function.

BugLink: https://lists.openwrt.org/pipermail/openwrt-devel/2018-November/014595.html
Fixes: 9d5804662ce1 ("ath10k: retrieve MAC address from system firmware if provided")
Cc: Brian Norris <briannorris@chromium.org>
Cc: Ben Greear <greearb@candelatech.com>
Cc: Felix Fietkau <nbd@nbd.name>
Cc: Mathias Kresin <dev@kresin.me>
Signed-off-by: Christian Lamparter <chunkeey@gmail.com>
---

WARNING: line over 80 characters
L85: FILE: drivers/net/wireless/ath/ath10k/wmi-ops.h:68:
+                                 const u8 macaddr[ETH_ALEN]);

The function parameter has been aligned to match other functions
parameters that extend over 80 characters.

---
---
 drivers/net/wireless/ath/ath10k/core.c    |  7 +++++++
 drivers/net/wireless/ath/ath10k/wmi-ops.h | 18 +++++++++++++++++
 drivers/net/wireless/ath/ath10k/wmi-tlv.c | 24 +++++++++++++++++++++++
 drivers/net/wireless/ath/ath10k/wmi.c     | 24 +++++++++++++++++++++++
 drivers/net/wireless/ath/ath10k/wmi.h     |  4 ++++
 5 files changed, 77 insertions(+)

Comments

Brian Norris Feb. 2, 2019, 2:50 a.m. UTC | #1
Hello!

On Mon, Jan 14, 2019 at 7:36 AM Christian Lamparter <chunkeey@gmail.com> wrote:
> Part of the "setting an alternate MAC" issue was already tackled by a
> patch from Brian Norris:
> commit 9d5804662ce1
> ("ath10k: retrieve MAC address from system firmware if provided")
> by allowing the option to specify an alternate MAC-address with the
> established device_get_mac_address() function which extracts the right
> address from DeviceTree/fwnode mac-address or local-mac-address
> properties and saves it for later.
>
> However, Ben Greear noted that the Qualcomm's ath10k firmware is liable
> to not properly calculate its rx-bssid mask in this case. This can cause
> issues in the popluar "multiple AP with a single ath10k instance"
> configurations.

I'll admit that
(a) I wasn't testing AP mode, or any similar configuration to this and
(b) I'm not super familiar with the firmware features involved here.

That's to say, this patch could very well be completely justified and
correct, but I may not the right person to analyze it.

I do have it on my list to test out though, so I'll hopefully chime in
again eventually.

Regards,
Brian
Kalle Valo Feb. 4, 2019, 3:41 p.m. UTC | #2
Christian Lamparter <chunkeey@gmail.com> writes:

> Many integrated QCA9984 WiFis in various IPQ806x platform routers
> from various vendors (Netgear R7800, ZyXEL NBG6817, TP-LINK C2600,
> etc.) have either blank, bogus or non-unique MAC-addresses in
> their calibration data.
>
> As a result, OpenWrt utilizes a discouraged binary calibration data
> patching method that allows to modify the device's MAC-addresses right
> at the source. This is because the ath10k' firmware extracts the MAC
> address from the supplied radio/calibration data and issues a response
> to the ath10k linux driver. Which was designed to take the main MAC in
> ath10k_wmi_event_ready().
>
> Part of the "setting an alternate MAC" issue was already tackled by a
> patch from Brian Norris:
> commit 9d5804662ce1
> ("ath10k: retrieve MAC address from system firmware if provided")
> by allowing the option to specify an alternate MAC-address with the
> established device_get_mac_address() function which extracts the right
> address from DeviceTree/fwnode mac-address or local-mac-address
> properties and saves it for later.
>
> However, Ben Greear noted that the Qualcomm's ath10k firmware is liable
> to not properly calculate its rx-bssid mask in this case. This can cause
> issues in the popluar "multiple AP with a single ath10k instance"
> configurations.
>
> To improve MAC address handling, Felix Fietkau suggested to call
> pdev_set_base_macaddr_cmdid before bringing up the first vif and
> use the first vif MAC address there. Which is in ath10k_core_start().
>
> This patch implement Felix Fietkau's request to
> "call pdev_set_base_macaddr_cmdid before bringing up the first vif".
> The pdev_set_base_macaddr_cmdid is already declared for all devices
> and version. The driver just needed the support code for this
> function.

I prefer listing hardware and firmware version(s) tested in the commit
log. That way it's easier to understand and lates track how this is
supposed to work.

> BugLink: https://lists.openwrt.org/pipermail/openwrt-devel/2018-November/014595.html
> Fixes: 9d5804662ce1 ("ath10k: retrieve MAC address from system firmware if provided")
> Cc: Brian Norris <briannorris@chromium.org>
> Cc: Ben Greear <greearb@candelatech.com>
> Cc: Felix Fietkau <nbd@nbd.name>
> Cc: Mathias Kresin <dev@kresin.me>
> Signed-off-by: Christian Lamparter <chunkeey@gmail.com>
> ---
>
> WARNING: line over 80 characters
> L85: FILE: drivers/net/wireless/ath/ath10k/wmi-ops.h:68:
> +                                 const u8 macaddr[ETH_ALEN]);
>
> The function parameter has been aligned to match other functions
> parameters that extend over 80 characters.

That's fine. In my ath10k-check script I have max-line-lenght 90 chars
anyway.

https://github.com/qca/qca-swiss-army-knife/blob/master/tools/scripts/ath10k/ath10k-check

> @@ -8885,6 +8904,7 @@ static const struct wmi_ops wmi_ops = {
>  
>  	.gen_pdev_suspend = ath10k_wmi_op_gen_pdev_suspend,
>  	.gen_pdev_resume = ath10k_wmi_op_gen_pdev_resume,
> +	.gen_pdev_set_base_macaddr = ath10k_wmi_op_gen_pdev_set_base_macaddr,
>  	.gen_pdev_set_rd = ath10k_wmi_op_gen_pdev_set_rd,
>  	.gen_pdev_set_param = ath10k_wmi_op_gen_pdev_set_param,
>  	.gen_init = ath10k_wmi_op_gen_init,
> @@ -8960,6 +8980,7 @@ static const struct wmi_ops wmi_10_1_ops = {
>  
>  	.gen_pdev_suspend = ath10k_wmi_op_gen_pdev_suspend,
>  	.gen_pdev_resume = ath10k_wmi_op_gen_pdev_resume,
> +	.gen_pdev_set_base_macaddr = ath10k_wmi_op_gen_pdev_set_base_macaddr,
>  	.gen_pdev_set_param = ath10k_wmi_op_gen_pdev_set_param,
>  	.gen_stop_scan = ath10k_wmi_op_gen_stop_scan,
>  	.gen_vdev_create = ath10k_wmi_op_gen_vdev_create,
> @@ -9032,6 +9053,7 @@ static const struct wmi_ops wmi_10_2_ops = {
>  
>  	.gen_pdev_suspend = ath10k_wmi_op_gen_pdev_suspend,
>  	.gen_pdev_resume = ath10k_wmi_op_gen_pdev_resume,
> +	.gen_pdev_set_base_macaddr = ath10k_wmi_op_gen_pdev_set_base_macaddr,
>  	.gen_pdev_set_param = ath10k_wmi_op_gen_pdev_set_param,
>  	.gen_stop_scan = ath10k_wmi_op_gen_stop_scan,
>  	.gen_vdev_create = ath10k_wmi_op_gen_vdev_create,

These are in practise obsolete WMI interfaces so not sure if it makes it
worth to support this parameter in them. But on the other hand it won't
hurt either, so dunno.

> @@ -9115,6 +9137,7 @@ static const struct wmi_ops wmi_10_2_4_ops = {
>  	.gen_peer_create = ath10k_wmi_op_gen_peer_create,
>  	.gen_peer_delete = ath10k_wmi_op_gen_peer_delete,
>  	.gen_peer_flush = ath10k_wmi_op_gen_peer_flush,
> +	.gen_pdev_set_base_macaddr = ath10k_wmi_op_gen_pdev_set_base_macaddr,
>  	.gen_peer_set_param = ath10k_wmi_op_gen_peer_set_param,
>  	.gen_set_psmode = ath10k_wmi_op_gen_set_psmode,
>  	.gen_set_sta_ps = ath10k_wmi_op_gen_set_sta_ps,

This is only used by QCA988X. Did you test with that? If not, IMHO
better not to enable it for 10.2.4 until it's tested.

> @@ -9166,6 +9189,7 @@ static const struct wmi_ops wmi_10_4_ops = {
>  
>  	.gen_pdev_suspend = ath10k_wmi_op_gen_pdev_suspend,
>  	.gen_pdev_resume = ath10k_wmi_op_gen_pdev_resume,
> +	.gen_pdev_set_base_macaddr = ath10k_wmi_op_gen_pdev_set_base_macaddr,
>  	.gen_pdev_set_rd = ath10k_wmi_10x_op_gen_pdev_set_rd,
>  	.gen_pdev_set_param = ath10k_wmi_op_gen_pdev_set_param,
>  	.gen_init = ath10k_wmi_10_4_op_gen_init,

This looks fine.
Kalle Valo Feb. 4, 2019, 3:45 p.m. UTC | #3
Christian Lamparter <chunkeey@gmail.com> writes:

> Many integrated QCA9984 WiFis in various IPQ806x platform routers
> from various vendors (Netgear R7800, ZyXEL NBG6817, TP-LINK C2600,
> etc.) have either blank, bogus or non-unique MAC-addresses in
> their calibration data.
>
> As a result, OpenWrt utilizes a discouraged binary calibration data
> patching method that allows to modify the device's MAC-addresses right
> at the source. This is because the ath10k' firmware extracts the MAC
> address from the supplied radio/calibration data and issues a response
> to the ath10k linux driver. Which was designed to take the main MAC in
> ath10k_wmi_event_ready().
>
> Part of the "setting an alternate MAC" issue was already tackled by a
> patch from Brian Norris:
> commit 9d5804662ce1
> ("ath10k: retrieve MAC address from system firmware if provided")
> by allowing the option to specify an alternate MAC-address with the
> established device_get_mac_address() function which extracts the right
> address from DeviceTree/fwnode mac-address or local-mac-address
> properties and saves it for later.
>
> However, Ben Greear noted that the Qualcomm's ath10k firmware is liable
> to not properly calculate its rx-bssid mask in this case. This can cause
> issues in the popluar "multiple AP with a single ath10k instance"
> configurations.
>
> To improve MAC address handling, Felix Fietkau suggested to call
> pdev_set_base_macaddr_cmdid before bringing up the first vif and
> use the first vif MAC address there. Which is in ath10k_core_start().
>
> This patch implement Felix Fietkau's request to
> "call pdev_set_base_macaddr_cmdid before bringing up the first vif".
> The pdev_set_base_macaddr_cmdid is already declared for all devices
> and version. The driver just needed the support code for this
> function.
>
> BugLink: https://lists.openwrt.org/pipermail/openwrt-devel/2018-November/014595.html
> Fixes: 9d5804662ce1 ("ath10k: retrieve MAC address from system firmware if provided")
> Cc: Brian Norris <briannorris@chromium.org>
> Cc: Ben Greear <greearb@candelatech.com>
> Cc: Felix Fietkau <nbd@nbd.name>
> Cc: Mathias Kresin <dev@kresin.me>
> Signed-off-by: Christian Lamparter <chunkeey@gmail.com>

[...]

> --- a/drivers/net/wireless/ath/ath10k/core.c
> +++ b/drivers/net/wireless/ath/ath10k/core.c
> @@ -2649,6 +2649,13 @@ int ath10k_core_start(struct ath10k *ar, enum ath10k_firmware_mode mode,
>  		goto err_hif_stop;
>  	}
>  
> +	status = ath10k_wmi_pdev_set_base_macaddr(ar, ar->mac_addr);
> +	if (status) {
> +		ath10k_err(ar,
> +			   "failed to set base mac address: %d\n", status);
> +		goto err_hif_stop;
> +	}

Oh, and as the new parameter is not supported with WMI TLV interface
(QCA6174, WCN3990 etc) this will print an error on those. I think you
need to check for -EOPNOTSUPP and then just ignore the error on that
case. IIRC we have similar checks elsewhere in ath10k.
Christian Lamparter Feb. 4, 2019, 6:10 p.m. UTC | #4
On Monday, February 4, 2019 4:45:12 PM CET Kalle Valo wrote:
> Christian Lamparter <chunkeey@gmail.com> writes:
> 
> > Many integrated QCA9984 WiFis in various IPQ806x platform routers
> > from various vendors (Netgear R7800, ZyXEL NBG6817, TP-LINK C2600,
> > etc.) have either blank, bogus or non-unique MAC-addresses in
> > their calibration data.
> >
> > As a result, OpenWrt utilizes a discouraged binary calibration data
> > patching method that allows to modify the device's MAC-addresses right
> > at the source. This is because the ath10k' firmware extracts the MAC
> > address from the supplied radio/calibration data and issues a response
> > to the ath10k linux driver. Which was designed to take the main MAC in
> > ath10k_wmi_event_ready().
> >
> > Part of the "setting an alternate MAC" issue was already tackled by a
> > patch from Brian Norris:
> > commit 9d5804662ce1
> > ("ath10k: retrieve MAC address from system firmware if provided")
> > by allowing the option to specify an alternate MAC-address with the
> > established device_get_mac_address() function which extracts the right
> > address from DeviceTree/fwnode mac-address or local-mac-address
> > properties and saves it for later.
> >
> > However, Ben Greear noted that the Qualcomm's ath10k firmware is liable
> > to not properly calculate its rx-bssid mask in this case. This can cause
> > issues in the popluar "multiple AP with a single ath10k instance"
> > configurations.
> >
> > To improve MAC address handling, Felix Fietkau suggested to call
> > pdev_set_base_macaddr_cmdid before bringing up the first vif and
> > use the first vif MAC address there. Which is in ath10k_core_start().
> >
> > This patch implement Felix Fietkau's request to
> > "call pdev_set_base_macaddr_cmdid before bringing up the first vif".
> > The pdev_set_base_macaddr_cmdid is already declared for all devices
> > and version. The driver just needed the support code for this
> > function.
> >
> > BugLink: https://lists.openwrt.org/pipermail/openwrt-devel/2018-November/014595.html
> > Fixes: 9d5804662ce1 ("ath10k: retrieve MAC address from system firmware if provided")
> > Cc: Brian Norris <briannorris@chromium.org>
> > Cc: Ben Greear <greearb@candelatech.com>
> > Cc: Felix Fietkau <nbd@nbd.name>
> > Cc: Mathias Kresin <dev@kresin.me>
> > Signed-off-by: Christian Lamparter <chunkeey@gmail.com>
> 

I concated both reviews into this response.

> > @@ -8885,6 +8904,7 @@ static const struct wmi_ops wmi_ops = {
> >  
> >  	.gen_pdev_suspend = ath10k_wmi_op_gen_pdev_suspend,
> >  	.gen_pdev_resume = ath10k_wmi_op_gen_pdev_resume,
> > +	.gen_pdev_set_base_macaddr = ath10k_wmi_op_gen_pdev_set_base_macaddr,
> >  	.gen_pdev_set_rd = ath10k_wmi_op_gen_pdev_set_rd,
> >  	.gen_pdev_set_param = ath10k_wmi_op_gen_pdev_set_param,
> >  	.gen_init = ath10k_wmi_op_gen_init,
> > @@ -8960,6 +8980,7 @@ static const struct wmi_ops wmi_10_1_ops = {
> >  
> >  	.gen_pdev_suspend = ath10k_wmi_op_gen_pdev_suspend,
> >  	.gen_pdev_resume = ath10k_wmi_op_gen_pdev_resume,
> > +	.gen_pdev_set_base_macaddr = ath10k_wmi_op_gen_pdev_set_base_macaddr,
> >  	.gen_pdev_set_param = ath10k_wmi_op_gen_pdev_set_param,
> >  	.gen_stop_scan = ath10k_wmi_op_gen_stop_scan,
> >  	.gen_vdev_create = ath10k_wmi_op_gen_vdev_create,
> > @@ -9032,6 +9053,7 @@ static const struct wmi_ops wmi_10_2_ops = {
> >  
> >  	.gen_pdev_suspend = ath10k_wmi_op_gen_pdev_suspend,
> >  	.gen_pdev_resume = ath10k_wmi_op_gen_pdev_resume,
> > +	.gen_pdev_set_base_macaddr = ath10k_wmi_op_gen_pdev_set_base_macaddr,
> >  	.gen_pdev_set_param = ath10k_wmi_op_gen_pdev_set_param,
> >  	.gen_stop_scan = ath10k_wmi_op_gen_stop_scan,
> >  	.gen_vdev_create = ath10k_wmi_op_gen_vdev_create,
> 
> These are in practise obsolete WMI interfaces so not sure if it makes it
> worth to support this parameter in them. But on the other hand it won't
> hurt either, so dunno.
Ok. I looked what firmware interfaces (wmi_cmd_map) supported the 
pdev_set_base_macaddr_cmdid and all did (including the old and tlv)
so I added the line everywhere I could.
As far as the support for the old firmwares goes: I don't think
anybody with a current ath10k is willingly still stuck on the 10.1,
10.2 firmware. So, I might as well just remove those for 10_2, 10_1 and MAIN. 
 
> > @@ -9115,6 +9137,7 @@ static const struct wmi_ops wmi_10_2_4_ops = {
> >  	.gen_peer_create = ath10k_wmi_op_gen_peer_create,
> >  	.gen_peer_delete = ath10k_wmi_op_gen_peer_delete,
> >  	.gen_peer_flush = ath10k_wmi_op_gen_peer_flush,
> > +	.gen_pdev_set_base_macaddr = ath10k_wmi_op_gen_pdev_set_base_macaddr,
> >  	.gen_peer_set_param = ath10k_wmi_op_gen_peer_set_param,
> >  	.gen_set_psmode = ath10k_wmi_op_gen_set_psmode,
> >  	.gen_set_sta_ps = ath10k_wmi_op_gen_set_sta_ps,
> 
> This is only used by QCA988X. Did you test with that? If not, IMHO
> better not to enable it for 10.2.4 until it's tested.
Yes. I tested it on a CUS-223 with 10.2.4-1.0-00041 and after. I also know
that Mathias Kresin tested it on his Homehub 5a Router (QCA9880) with both
ath10k (most likely with the same 10.2.4-1.0-00041) and Ben's ath10k-ct.

Ben also confirmed in the thread that the patch was working fine on his
R7800 (QCA9984).

> > --- a/drivers/net/wireless/ath/ath10k/core.c
> > +++ b/drivers/net/wireless/ath/ath10k/core.c
> > @@ -2649,6 +2649,13 @@ int ath10k_core_start(struct ath10k *ar, enum ath10k_firmware_mode mode,
> >  		goto err_hif_stop;
> >  	}
> >  
> > +	status = ath10k_wmi_pdev_set_base_macaddr(ar, ar->mac_addr);
> > +	if (status) {
> > +		ath10k_err(ar,
> > +			   "failed to set base mac address: %d\n", status);
> > +		goto err_hif_stop;
> > +	}
> 
> Oh, and as the new parameter is not supported with WMI TLV interface
> (QCA6174, WCN3990 etc) this will print an error on those.

This also means that Brian won't be able to test/verify this anyway?
Should I also drop ath10k_wmi_tlv_op_gen_pdev_set_base_macaddr() then?
Because it won't make sense to have the support function if
"WMI_TLV_TAG_STRUCT_PDEV_SET_BASE_MACADDR_CMD" is just a dummy in this
context.

> I think you
> need to check for -EOPNOTSUPP and then just ignore the error on that
> case. IIRC we have similar checks elsewhere in ath10k.

Ok, I think I know what you are looking for:
| if (status && status != -EOPNOTSUPP) { ...

Yes, this should work.

Thanks,
Christian
Brian Norris Feb. 4, 2019, 6:32 p.m. UTC | #5
On Mon, Feb 4, 2019 at 10:10 AM Christian Lamparter <chunkeey@gmail.com> wrote:
> On Monday, February 4, 2019 4:45:12 PM CET Kalle Valo wrote:
> > Christian Lamparter <chunkeey@gmail.com> writes:
> > > --- a/drivers/net/wireless/ath/ath10k/core.c
> > > +++ b/drivers/net/wireless/ath/ath10k/core.c
> > > @@ -2649,6 +2649,13 @@ int ath10k_core_start(struct ath10k *ar, enum ath10k_firmware_mode mode,
> > >             goto err_hif_stop;
> > >     }
> > >
> > > +   status = ath10k_wmi_pdev_set_base_macaddr(ar, ar->mac_addr);
> > > +   if (status) {
> > > +           ath10k_err(ar,
> > > +                      "failed to set base mac address: %d\n", status);
> > > +           goto err_hif_stop;
> > > +   }
> >
> > Oh, and as the new parameter is not supported with WMI TLV interface
> > (QCA6174, WCN3990 etc) this will print an error on those.
>
> This also means that Brian won't be able to test/verify this anyway?

Well, I'd be able to tell you if this started dumping new errors to the log :)

And in fact, it seems this crashes my firmware:

[  150.091401] qcom-q6v5-mss 4080000.remoteproc: fatal error received:
err_qdi.c:456:EF:wlan_process:1:cmnos_thread.c:3921:Asserted in
wlan_dev.c:WLAN_GET_MAC_ID_FROM_WMI_PDEV_ID:536

Note that I'm running WCN3990, and I haven't configure any sort of PD
restart -- so any WiFi firmware assertions/crashes take down the
entire modem/WiFi.

IOW:

Tested-and-found-wanting-by: Brian Norris <briannorris@chromium.org>

Willing to test a v2!

Regards,
Brian
Kalle Valo Feb. 7, 2019, 2:19 p.m. UTC | #6
Christian Lamparter <chunkeey@gmail.com> writes:

> On Monday, February 4, 2019 4:45:12 PM CET Kalle Valo wrote:
>> Christian Lamparter <chunkeey@gmail.com> writes:
>> 
>> > @@ -8885,6 +8904,7 @@ static const struct wmi_ops wmi_ops = {
>> >  
>> >  	.gen_pdev_suspend = ath10k_wmi_op_gen_pdev_suspend,
>> >  	.gen_pdev_resume = ath10k_wmi_op_gen_pdev_resume,
>> > +	.gen_pdev_set_base_macaddr = ath10k_wmi_op_gen_pdev_set_base_macaddr,
>> >  	.gen_pdev_set_rd = ath10k_wmi_op_gen_pdev_set_rd,
>> >  	.gen_pdev_set_param = ath10k_wmi_op_gen_pdev_set_param,
>> >  	.gen_init = ath10k_wmi_op_gen_init,
>> > @@ -8960,6 +8980,7 @@ static const struct wmi_ops wmi_10_1_ops = {
>> >  
>> >  	.gen_pdev_suspend = ath10k_wmi_op_gen_pdev_suspend,
>> >  	.gen_pdev_resume = ath10k_wmi_op_gen_pdev_resume,
>> > +	.gen_pdev_set_base_macaddr = ath10k_wmi_op_gen_pdev_set_base_macaddr,
>> >  	.gen_pdev_set_param = ath10k_wmi_op_gen_pdev_set_param,
>> >  	.gen_stop_scan = ath10k_wmi_op_gen_stop_scan,
>> >  	.gen_vdev_create = ath10k_wmi_op_gen_vdev_create,
>> > @@ -9032,6 +9053,7 @@ static const struct wmi_ops wmi_10_2_ops = {
>> >  
>> >  	.gen_pdev_suspend = ath10k_wmi_op_gen_pdev_suspend,
>> >  	.gen_pdev_resume = ath10k_wmi_op_gen_pdev_resume,
>> > +	.gen_pdev_set_base_macaddr = ath10k_wmi_op_gen_pdev_set_base_macaddr,
>> >  	.gen_pdev_set_param = ath10k_wmi_op_gen_pdev_set_param,
>> >  	.gen_stop_scan = ath10k_wmi_op_gen_stop_scan,
>> >  	.gen_vdev_create = ath10k_wmi_op_gen_vdev_create,
>> 
>> These are in practise obsolete WMI interfaces so not sure if it makes it
>> worth to support this parameter in them. But on the other hand it won't
>> hurt either, so dunno.
>
> Ok. I looked what firmware interfaces (wmi_cmd_map) supported the 
> pdev_set_base_macaddr_cmdid and all did (including the old and tlv)
> so I added the line everywhere I could.
> As far as the support for the old firmwares goes: I don't think
> anybody with a current ath10k is willingly still stuck on the 10.1,
> 10.2 firmware. So, I might as well just remove those for 10_2, 10_1 and MAIN. 

Yeah, that's the best. BTW I'm planning (or better hoping) to remove
10.1, 10.2 and main WMI interfaces altogether. They are just making
these unnecessary complex.

>> > @@ -9115,6 +9137,7 @@ static const struct wmi_ops wmi_10_2_4_ops = {
>> >  	.gen_peer_create = ath10k_wmi_op_gen_peer_create,
>> >  	.gen_peer_delete = ath10k_wmi_op_gen_peer_delete,
>> >  	.gen_peer_flush = ath10k_wmi_op_gen_peer_flush,
>> > +	.gen_pdev_set_base_macaddr = ath10k_wmi_op_gen_pdev_set_base_macaddr,
>> >  	.gen_peer_set_param = ath10k_wmi_op_gen_peer_set_param,
>> >  	.gen_set_psmode = ath10k_wmi_op_gen_set_psmode,
>> >  	.gen_set_sta_ps = ath10k_wmi_op_gen_set_sta_ps,
>> 
>> This is only used by QCA988X. Did you test with that? If not, IMHO
>> better not to enable it for 10.2.4 until it's tested.
>
> Yes. I tested it on a CUS-223 with 10.2.4-1.0-00041 and after. I also know
> that Mathias Kresin tested it on his Homehub 5a Router (QCA9880) with both
> ath10k (most likely with the same 10.2.4-1.0-00041) and Ben's ath10k-ct.
>
> Ben also confirmed in the thread that the patch was working fine on his
> R7800 (QCA9984).

Great. And I see that you added that to the commit log in v2 already.

>> > --- a/drivers/net/wireless/ath/ath10k/core.c
>> > +++ b/drivers/net/wireless/ath/ath10k/core.c
>> > @@ -2649,6 +2649,13 @@ int ath10k_core_start(struct ath10k *ar, enum ath10k_firmware_mode mode,
>> >  		goto err_hif_stop;
>> >  	}
>> >  
>> > +	status = ath10k_wmi_pdev_set_base_macaddr(ar, ar->mac_addr);
>> > +	if (status) {
>> > +		ath10k_err(ar,
>> > +			   "failed to set base mac address: %d\n", status);
>> > +		goto err_hif_stop;
>> > +	}
>> 
>> Oh, and as the new parameter is not supported with WMI TLV interface
>> (QCA6174, WCN3990 etc) this will print an error on those.
>
> This also means that Brian won't be able to test/verify this anyway?
> Should I also drop ath10k_wmi_tlv_op_gen_pdev_set_base_macaddr() then?

Yes, and I see that you already did :)

> Because it won't make sense to have the support function if
> "WMI_TLV_TAG_STRUCT_PDEV_SET_BASE_MACADDR_CMD" is just a dummy in this
> context.
>
>> I think you
>> need to check for -EOPNOTSUPP and then just ignore the error on that
>> case. IIRC we have similar checks elsewhere in ath10k.
>
> Ok, I think I know what you are looking for:
> | if (status && status != -EOPNOTSUPP) { ...

> Yes, this should work.

Yup, this is what I meant.
Ben Greear Feb. 7, 2019, 2:23 p.m. UTC | #7
On 02/07/2019 06:19 AM, Kalle Valo wrote:
> Christian Lamparter <chunkeey@gmail.com> writes:
>
>> On Monday, February 4, 2019 4:45:12 PM CET Kalle Valo wrote:
>>> Christian Lamparter <chunkeey@gmail.com> writes:
>>>
>>>> @@ -8885,6 +8904,7 @@ static const struct wmi_ops wmi_ops = {
>>>>
>>>>  	.gen_pdev_suspend = ath10k_wmi_op_gen_pdev_suspend,
>>>>  	.gen_pdev_resume = ath10k_wmi_op_gen_pdev_resume,
>>>> +	.gen_pdev_set_base_macaddr = ath10k_wmi_op_gen_pdev_set_base_macaddr,
>>>>  	.gen_pdev_set_rd = ath10k_wmi_op_gen_pdev_set_rd,
>>>>  	.gen_pdev_set_param = ath10k_wmi_op_gen_pdev_set_param,
>>>>  	.gen_init = ath10k_wmi_op_gen_init,
>>>> @@ -8960,6 +8980,7 @@ static const struct wmi_ops wmi_10_1_ops = {
>>>>
>>>>  	.gen_pdev_suspend = ath10k_wmi_op_gen_pdev_suspend,
>>>>  	.gen_pdev_resume = ath10k_wmi_op_gen_pdev_resume,
>>>> +	.gen_pdev_set_base_macaddr = ath10k_wmi_op_gen_pdev_set_base_macaddr,
>>>>  	.gen_pdev_set_param = ath10k_wmi_op_gen_pdev_set_param,
>>>>  	.gen_stop_scan = ath10k_wmi_op_gen_stop_scan,
>>>>  	.gen_vdev_create = ath10k_wmi_op_gen_vdev_create,
>>>> @@ -9032,6 +9053,7 @@ static const struct wmi_ops wmi_10_2_ops = {
>>>>
>>>>  	.gen_pdev_suspend = ath10k_wmi_op_gen_pdev_suspend,
>>>>  	.gen_pdev_resume = ath10k_wmi_op_gen_pdev_resume,
>>>> +	.gen_pdev_set_base_macaddr = ath10k_wmi_op_gen_pdev_set_base_macaddr,
>>>>  	.gen_pdev_set_param = ath10k_wmi_op_gen_pdev_set_param,
>>>>  	.gen_stop_scan = ath10k_wmi_op_gen_stop_scan,
>>>>  	.gen_vdev_create = ath10k_wmi_op_gen_vdev_create,
>>>
>>> These are in practise obsolete WMI interfaces so not sure if it makes it
>>> worth to support this parameter in them. But on the other hand it won't
>>> hurt either, so dunno.
>>
>> Ok. I looked what firmware interfaces (wmi_cmd_map) supported the
>> pdev_set_base_macaddr_cmdid and all did (including the old and tlv)
>> so I added the line everywhere I could.
>> As far as the support for the old firmwares goes: I don't think
>> anybody with a current ath10k is willingly still stuck on the 10.1,
>> 10.2 firmware. So, I might as well just remove those for 10_2, 10_1 and MAIN.
>
> Yeah, that's the best. BTW I'm planning (or better hoping) to remove
> 10.1, 10.2 and main WMI interfaces altogether. They are just making
> these unnecessary complex.

My wave-1 firmware uses the 10.1 interface and it is used by a fair number of people,
so please leave that one in place.

Thanks,
Ben
diff mbox series

Patch

diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c
index 399b501f3c3c..6b037e282fb9 100644
--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -2649,6 +2649,13 @@  int ath10k_core_start(struct ath10k *ar, enum ath10k_firmware_mode mode,
 		goto err_hif_stop;
 	}
 
+	status = ath10k_wmi_pdev_set_base_macaddr(ar, ar->mac_addr);
+	if (status) {
+		ath10k_err(ar,
+			   "failed to set base mac address: %d\n", status);
+		goto err_hif_stop;
+	}
+
 	/* Some firmware revisions do not properly set up hardware rx filter
 	 * registers.
 	 *
diff --git a/drivers/net/wireless/ath/ath10k/wmi-ops.h b/drivers/net/wireless/ath/ath10k/wmi-ops.h
index 04663076d27a..4f8c7e402e6f 100644
--- a/drivers/net/wireless/ath/ath10k/wmi-ops.h
+++ b/drivers/net/wireless/ath/ath10k/wmi-ops.h
@@ -66,6 +66,8 @@  struct wmi_ops {
 
 	struct sk_buff *(*gen_pdev_suspend)(struct ath10k *ar, u32 suspend_opt);
 	struct sk_buff *(*gen_pdev_resume)(struct ath10k *ar);
+	struct sk_buff *(*gen_pdev_set_base_macaddr)(struct ath10k *ar,
+						     const u8 macaddr[ETH_ALEN]);
 	struct sk_buff *(*gen_pdev_set_rd)(struct ath10k *ar, u16 rd, u16 rd2g,
 					   u16 rd5g, u16 ctl2g, u16 ctl5g,
 					   enum wmi_dfs_region dfs_reg);
@@ -506,6 +508,22 @@  ath10k_wmi_pdev_set_regdomain(struct ath10k *ar, u16 rd, u16 rd2g, u16 rd5g,
 				   ar->wmi.cmd->pdev_set_regdomain_cmdid);
 }
 
+static inline int
+ath10k_wmi_pdev_set_base_macaddr(struct ath10k *ar, const u8 macaddr[ETH_ALEN])
+{
+	struct sk_buff *skb;
+
+	if (!ar->wmi.ops->gen_pdev_set_base_macaddr)
+		return -EOPNOTSUPP;
+
+	skb = ar->wmi.ops->gen_pdev_set_base_macaddr(ar, macaddr);
+	if (IS_ERR(skb))
+		return PTR_ERR(skb);
+
+	return ath10k_wmi_cmd_send(ar, skb,
+				   ar->wmi.cmd->pdev_set_base_macaddr_cmdid);
+}
+
 static inline int
 ath10k_wmi_pdev_suspend_target(struct ath10k *ar, u32 suspend_opt)
 {
diff --git a/drivers/net/wireless/ath/ath10k/wmi-tlv.c b/drivers/net/wireless/ath/ath10k/wmi-tlv.c
index 892bd8c30dd9..8a17a905548c 100644
--- a/drivers/net/wireless/ath/ath10k/wmi-tlv.c
+++ b/drivers/net/wireless/ath/ath10k/wmi-tlv.c
@@ -2228,6 +2228,28 @@  ath10k_wmi_tlv_op_gen_peer_create(struct ath10k *ar, u32 vdev_id,
 	return skb;
 }
 
+static struct sk_buff *
+ath10k_wmi_tlv_op_gen_pdev_set_base_macaddr(struct ath10k *ar,
+					    const u8 addr[ETH_ALEN])
+{
+	struct wmi_pdev_set_base_macaddr_cmd *cmd;
+	struct wmi_tlv *tlv;
+	struct sk_buff *skb;
+
+	skb = ath10k_wmi_alloc_skb(ar, sizeof(*tlv) + sizeof(*cmd));
+	if (!skb)
+		return ERR_PTR(-ENOMEM);
+
+	tlv = (void *)skb->data;
+	tlv->tag = __cpu_to_le16(WMI_TLV_TAG_STRUCT_PDEV_SET_BASE_MACADDR_CMD);
+	tlv->len = __cpu_to_le16(sizeof(*cmd));
+	cmd = (void *)tlv->value;
+	ether_addr_copy(cmd->mac_addr.addr, addr);
+
+	ath10k_dbg(ar, ATH10K_DBG_WMI, "wmi tlv set base macaddr\n");
+	return skb;
+}
+
 static struct sk_buff *
 ath10k_wmi_tlv_op_gen_peer_delete(struct ath10k *ar, u32 vdev_id,
 				  const u8 peer_addr[ETH_ALEN])
@@ -4110,6 +4132,8 @@  static const struct wmi_ops wmi_tlv_ops = {
 
 	.gen_pdev_suspend = ath10k_wmi_tlv_op_gen_pdev_suspend,
 	.gen_pdev_resume = ath10k_wmi_tlv_op_gen_pdev_resume,
+	.gen_pdev_set_base_macaddr =
+			ath10k_wmi_tlv_op_gen_pdev_set_base_macaddr,
 	.gen_pdev_set_rd = ath10k_wmi_tlv_op_gen_pdev_set_rd,
 	.gen_pdev_set_param = ath10k_wmi_tlv_op_gen_pdev_set_param,
 	.gen_init = ath10k_wmi_tlv_op_gen_init,
diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
index ba837403e266..d4886eb989dc 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.c
+++ b/drivers/net/wireless/ath/ath10k/wmi.c
@@ -6239,6 +6239,25 @@  int ath10k_wmi_connect(struct ath10k *ar)
 	return 0;
 }
 
+static struct sk_buff *
+ath10k_wmi_op_gen_pdev_set_base_macaddr(struct ath10k *ar,
+					const u8 macaddr[ETH_ALEN])
+{
+	struct wmi_pdev_set_base_macaddr_cmd *cmd;
+	struct sk_buff *skb;
+
+	skb = ath10k_wmi_alloc_skb(ar, sizeof(*cmd));
+	if (!skb)
+		return ERR_PTR(-ENOMEM);
+
+	cmd = (struct wmi_pdev_set_base_macaddr_cmd *)skb->data;
+	ether_addr_copy(cmd->mac_addr.addr, macaddr);
+
+	ath10k_dbg(ar, ATH10K_DBG_WMI,
+		   "wmi pdev basemac %pM\n", macaddr);
+	return skb;
+}
+
 static struct sk_buff *
 ath10k_wmi_op_gen_pdev_set_rd(struct ath10k *ar, u16 rd, u16 rd2g, u16 rd5g,
 			      u16 ctl2g, u16 ctl5g,
@@ -8885,6 +8904,7 @@  static const struct wmi_ops wmi_ops = {
 
 	.gen_pdev_suspend = ath10k_wmi_op_gen_pdev_suspend,
 	.gen_pdev_resume = ath10k_wmi_op_gen_pdev_resume,
+	.gen_pdev_set_base_macaddr = ath10k_wmi_op_gen_pdev_set_base_macaddr,
 	.gen_pdev_set_rd = ath10k_wmi_op_gen_pdev_set_rd,
 	.gen_pdev_set_param = ath10k_wmi_op_gen_pdev_set_param,
 	.gen_init = ath10k_wmi_op_gen_init,
@@ -8960,6 +8980,7 @@  static const struct wmi_ops wmi_10_1_ops = {
 
 	.gen_pdev_suspend = ath10k_wmi_op_gen_pdev_suspend,
 	.gen_pdev_resume = ath10k_wmi_op_gen_pdev_resume,
+	.gen_pdev_set_base_macaddr = ath10k_wmi_op_gen_pdev_set_base_macaddr,
 	.gen_pdev_set_param = ath10k_wmi_op_gen_pdev_set_param,
 	.gen_stop_scan = ath10k_wmi_op_gen_stop_scan,
 	.gen_vdev_create = ath10k_wmi_op_gen_vdev_create,
@@ -9032,6 +9053,7 @@  static const struct wmi_ops wmi_10_2_ops = {
 
 	.gen_pdev_suspend = ath10k_wmi_op_gen_pdev_suspend,
 	.gen_pdev_resume = ath10k_wmi_op_gen_pdev_resume,
+	.gen_pdev_set_base_macaddr = ath10k_wmi_op_gen_pdev_set_base_macaddr,
 	.gen_pdev_set_param = ath10k_wmi_op_gen_pdev_set_param,
 	.gen_stop_scan = ath10k_wmi_op_gen_stop_scan,
 	.gen_vdev_create = ath10k_wmi_op_gen_vdev_create,
@@ -9115,6 +9137,7 @@  static const struct wmi_ops wmi_10_2_4_ops = {
 	.gen_peer_create = ath10k_wmi_op_gen_peer_create,
 	.gen_peer_delete = ath10k_wmi_op_gen_peer_delete,
 	.gen_peer_flush = ath10k_wmi_op_gen_peer_flush,
+	.gen_pdev_set_base_macaddr = ath10k_wmi_op_gen_pdev_set_base_macaddr,
 	.gen_peer_set_param = ath10k_wmi_op_gen_peer_set_param,
 	.gen_set_psmode = ath10k_wmi_op_gen_set_psmode,
 	.gen_set_sta_ps = ath10k_wmi_op_gen_set_sta_ps,
@@ -9166,6 +9189,7 @@  static const struct wmi_ops wmi_10_4_ops = {
 
 	.gen_pdev_suspend = ath10k_wmi_op_gen_pdev_suspend,
 	.gen_pdev_resume = ath10k_wmi_op_gen_pdev_resume,
+	.gen_pdev_set_base_macaddr = ath10k_wmi_op_gen_pdev_set_base_macaddr,
 	.gen_pdev_set_rd = ath10k_wmi_10x_op_gen_pdev_set_rd,
 	.gen_pdev_set_param = ath10k_wmi_op_gen_pdev_set_param,
 	.gen_init = ath10k_wmi_10_4_op_gen_init,
diff --git a/drivers/net/wireless/ath/ath10k/wmi.h b/drivers/net/wireless/ath/ath10k/wmi.h
index 2034ccc7cc72..151abcb5ec35 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.h
+++ b/drivers/net/wireless/ath/ath10k/wmi.h
@@ -4083,6 +4083,10 @@  struct wmi_pdev_set_param_cmd {
 	__le32 param_value;
 } __packed;
 
+struct wmi_pdev_set_base_macaddr_cmd {
+	struct wmi_mac_addr mac_addr;
+} __packed;
+
 /* valid period is 1 ~ 60000ms, unit in millisecond */
 #define WMI_PDEV_PARAM_CAL_PERIOD_MAX 60000