Re: [PATCH resend] brcmfmac: p2p and normal ap access are not always possible at the same time

[Arend van Spriel <arend.vanspriel@broadcom.com>]

On 3/1/2018 9:35 AM, Hans de Goede wrote:
> HI,
>
> On 28-02-18 20:43, Arend van Spriel wrote:
>> On 2/28/2018 3:52 PM, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 27-02-18 00:04, Arend van Spriel wrote:
>>>> On 2/26/2018 12:22 PM, Hans de Goede wrote:
>>>>> Hi,
>>>>>
>>>>> On 26-02-18 12:01, Arend van Spriel wrote:
>>>>>> On 2/26/2018 11:29 AM, Hans de Goede wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On 26-02-18 11:22, Arend van Spriel wrote:
>>>>>>>> On 2/25/2018 3:52 PM, Hans de Goede wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> On 26-05-17 12:57, Hans de Goede wrote:
>>>>>>>>>> The firmware responding with -EBUSY when trying to add an extra
>>>>>>>>>> virtual-if
>>>>>>>>>> is a normal thing, do not print an error for this.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>>>>>>>
>>>>>>>>> I'm now seeing this on another device too, but this time the error
>>>>>>>>> thrown is -EBADE, this seems to be new with recent kernels:
>>>>>>>>
>>>>>>>> Yup. Before we were passing firmware errors up to user-space, which
>>>>>>>> was confusing and potentially be misinterpreted. However,
>>>>>>>> looking at
>>>>>>>> the output below it would have been good to log the firmware
>>>>>>>> error as
>>>>>>>> well. And staring at it some more I suddenly realize I broke the
>>>>>>>> feature detection module with this change. Actually only the GSCAN
>>>>>>>> feature detection.
>>>>>>>>
>>>>>>>>> [root@localhost ~]# dmesg | grep brcmfmac
>>>>>>>>> [   34.265950] usbcore: registered new interface driver brcmfmac
>>>>>>>>> [   34.266059] brcmfmac 0000:01:00.0: enabling device (0000 ->
>>>>>>>>> 0002)
>>>>>>>>> [   34.376468] brcmfmac: brcmf_fw_map_chip_to_name: using
>>>>>>>>> brcm/brcmfmac4356-pcie.bin for chip 0x004356(17238) rev 0x000002
>>>>>>>>> [   34.855143] brcmfmac 0000:01:00.0: Direct firmware load for
>>>>>>>>> brcm/brcmfmac4356-pcie.clm_blob failed with error -2
>>>>>>>>> [   34.855147] brcmfmac: brcmf_c_process_clm_blob: no clm_blob
>>>>>>>>> available(err=-2), device may have limited channels available
>>>>>>>>> [   34.857029] brcmfmac: brcmf_c_preinit_dcmds: Firmware version =
>>>>>>>>> wl0:
>>>>>>>>> Jun  4 2017 16:50:07 version 7.35.101.6 (r702795) FWID 01-5e8eb735
>>>>>>>>> [   34.938854] brcmfmac 0000:01:00.0 wlp1s0: renamed from wlan0
>>>>>>>>> [   37.086420] brcmfmac: brcmf_p2p_create_p2pdev: set p2p_disc
>>>>>>>>> error
>>>>>>>>> [   37.086431] brcmfmac: brcmf_cfg80211_add_iface: add iface
>>>>>>>>> p2p-dev-wlp1s0 type 10 failed: err=-52
>>>>>>>>>
>>>>>>>>> [root@localhost ~]# strings
>>>>>>>>> /lib/firmware/brcm/brcmfmac4356-pcie.bin |
>>>>>>>>> tail -n 1
>>>>>>>>> 4356a2-roml/pcie-ag-msgbuf-splitrx-p2p-pno-aoe-pktfilter-keepalive-sr-mchan-pktctx-proptxstatus-ampduhostreorder-lpc-pwropt-txbf-wl11u-mfp-tdls-amsdutx-sarctrl-proxd-hs20sta-rcc-wepso-ndoe-linkstat-gscan-hchk-logtrace-roamexp-rmon
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Version: 7.35.101.6 (r702795) CRC: 4f3f65c5 Date: Sun 2017-06-04
>>>>>>>>> 16:51:38 PDT Ucode Ver: 963.316 FWID: 01-5e8eb735
>>>>>>>>>
>>>>>>>>> It would be good if we can silence these errors, or maybe at a
>>>>>>>>> minimum lower their log-level from error to warning?
>>>>>>>>
>>>>>>>> I had a look at it and it seems to be a difference in firmware api
>>>>>>>> that we need to support in the driver. Need to do a bit more
>>>>>>>> digging,
>>>>>>>> but it seems an actual issue. You could silence it for now, but I
>>>>>>>> prefer to wait for the fix.
>>>>>>>
>>>>>>> Ok, what is the ETA of a fix for this?
>>>>>>
>>>>>> Actually went back to an old log you sent and noticed:
>>>>>>
>>>>>> [   15.714569] brcmfmac: brcmf_attach Enter
>>>>>> [   15.714756] brcmfmac: brcmf_fweh_register event handler registered
>>>>>> for PSM_WATCHDOG
>>>>>> [   15.714757] brcmfmac: brcmf_proto_attach Enter
>>>>>> [   15.716598] brcmfmac: brcmf_bus_started
>>>>>> [   15.716603] brcmfmac: brcmf_add_if Enter, bsscfgidx=0, ifidx=0
>>>>>> [   15.716604] brcmfmac: brcmf_add_if allocate netdev interface
>>>>>> [   15.716622] brcmfmac: brcmf_add_if  ==== pid:2a, if:wlan%d
>>>>>> (00:00:00:00:00:00) created ===
>>>>>> [   15.716624] brcmfmac: brcmf_bus_change_state 0 -> 1
>>>>>> [   15.717841] brcmfmac: brcmf_fil_iovar_data_get ifidx=0,
>>>>>> name=cur_etheraddr, len=6
>>>>>> [   15.717843] brcmutil: data
>>>>>> [   15.717847] 00000000: 44 2c 05 9e c9 02   D,....
>>>>>>
>>>>>> So mac address of the device is 44:2c:05:9e:c9. However, further down
>>>>>> I see:
>>>>>>
>>>>>> [   17.819113] brcmfmac: brcmf_netdev_set_mac_address Enter,
>>>>>> bsscfgidx=0
>>>>>> [   17.819122] brcmfmac: brcmf_fil_iovar_data_set ifidx=0,
>>>>>> name=cur_etheraddr, len=6
>>>>>> [   17.819127] brcmutil: data
>>>>>> [   17.819135] 00000000: aa 3e 81 77 bc 40   .>.w.@
>>>>>> [   17.819864] brcmfmac: brcmf_netdev_set_mac_address updated to
>>>>>> aa:3e:81:77:bc:40
>>>>>>
>>>>>> So the mac address in a local admin variant.
>>>>>
>>>>> Right, this is likely NetworkManager randomizing the mac for privacy
>>>>> reasons.
>>>>>
>>>>>> Now our firmware has a requirement for the p2p-dev interface that it
>>>>>> should be different from the mac address of the primary interface,
>>>>>> ie.
>>>>>> wlp1s0 in this log. In brcmfmac we try to do that by setting the
>>>>>> local
>>>>>> admin bit, but... as it is already set we end up using the same mac
>>>>>> address hence the -EBUSY.
>>>>>
>>>>> Ah, that is good to know, so how can we fix this? Can userspace
>>>>> specify a
>>>>> different mac-address when it asks for the p2p-dev intf to be
>>>>> created? Or
>>>>> should we do something about this in the kernel?
>>>>
>>>> So this is the patch I tested. Maybe you can verify it works for you
>>>> as well.
>>>
>>> I can confirm that this fixes the errors, but I do not think that this
>>> is a good
>>> solution, using the permanent mac address for the p2p interface has the
>>> same
>>> privacy problem as using it regularly. IMHO it would be best to just
>>> generate
>>> a random mac address if none is specified. This is what the kernel seems
>>> to do
>>> in general when it needs a mac address and none is specified. You can
>>> use the
>>> eth_random_addr(u8 *addr) function for this, which will also set the
>>> local
>>> admin bit for you already.
>>
>> Hi Hans,
>>
>> Thanks for the suggestion. However, in wireless subsystem mac
>> randomization is requested by user-space upon scanning. This is still
>> possible with the p2p-dev interface.
>>
>> The mac randomization that NetworkManager seemed to be doing is simply
>> changing the mac address of the network interface. I suppose it does
>> that repeatedly upon some interval?
>
> I'm not familiar with the involved NetworkManager / wpa_supplicant code,
> but I would assume it uses something smarter where the MAC does not
> change while connected.
>
>> As the p2p-dev interface has no network interface associated with it
>> that is not possible. It can only specify a (random) mac address upon
>> creation.
>>
>> Still I do not really object to use eth_random_addr() but it does not
>> give any real privacy so that should not be used as an argument. I
>> will respin the patches.
>
> Great, thank you.

Ok. So I did sent out v3 of these patches, but now I am in doubt because 
I decided to check the WFA P2P spec and here is what it says about the 
P2P-Device address:

"""
The P2P Device Address of a P2P Device shall be its globally 
administered MAC address, or its globally administered MAC address with 
the locally administered bit set
"""

I read the term "globally administered MAC address" as the permanent mac 
address. Not sure if it really matters though. At least the P2P 
discovery seems to be working. Did not get to setting up an actual P2P 
connection yet.

Regards,
Arend