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 11:37 PM, Hans de Goede wrote:
> Hi,
>
> On 01-03-18 20:54, Arend van Spriel wrote:
>> 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
>> """
>
> That is nice and all, but given all the efforts we've done to not make a
> device uniquely identifiable to peers this seems like a bad idea. And
> the peer
> cannot really check this, so things should work fine anyways.

Let drop a bomb :-p MAC randomization is a stupid marketing trick to get 
the paranoid people into using a device so the big internet companies 
can keep perfect taps on them through the apps they use. Just a random 
thought from a different kind of paranoid guy.

>> 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.
>
> Right, I don't think that this matters.

Indeed. Let's stick to v3.

Regards,
Arend