Re: [PATCH 5/5] wcn36xx: pass information elements in scan requests

[Kalle Valo <kvalo@codeaurora.org>]

Daniel Mack <daniel@zonque.org> writes:

> On Monday, April 16, 2018 04:41 PM, Kalle Valo wrote:
>> Daniel Mack <daniel@zonque.org> writes:
>> 
>>> On Monday, April 16, 2018 04:13 PM, Kalle Valo wrote:
>>>> Daniel Mack <daniel@zonque.org> writes:
>>>>> On Monday, April 16, 2018 04:03 PM, Kalle Valo wrote:
>>>>>> Daniel Mack <daniel@zonque.org> writes:
>>>
>>>>>>> them to the firmware message. The driver currently tells the core that
>>>>>>> it is capable of attaching up to WCN36XX_MAX_SCAN_IE_LEN octets, but
>>>>>>> doesn't actually pass them to the the hardware.
>>>>>>>
>>>>>>> Some defines were moved around to avoid cyclic include dependencies.
>>>>>>
>>>>>> Does this fix anything or change functionality somehow? You should
>>>>>> document that also in the commit log.
>>>>>
>>>>> I don't have a test case for this, no. But as the change was pretty much
>>>>> straight forward, I sent it anyway.
>>>>
>>>> Ok, but you should still explain that in the commit log. In other words,
>>>> you should always answer the question "Why?" in the commit log.
>>>>
>>>>> I can resend with some more information on this if you like.
>>>>
>>>> No need to resend because of this, I can edit the commit log.
>>>
>>> Hmm, given that I can't even be sure the firmware does the right thing
>>> when instructed this way, we should probably just drop this patch from
>>> the series. The others are more important anyway, as they address bugs
>>> that hit me on actual hardware.
>> 
>> IMHO it's useful and if you don't see anything breaking I would prefer
>> to take it anyway. I can't immeadiately say in what use cases this helps
>> or fixes, though. But maybe you (or someone else) could verify that it
>> works correctly by comparing before and after probe requests with a
>> sniffer?
>
> It's certainly not a regression, no. I'm running extensive stress-tests
> with this driver. Then I guess adding the following to the commit log
> should suffice?
>
> "Note that this patch doesn't fix a bug that was observed. The change is
> merely done for the sake of completeness as the hardware supports
> appending IEs in scans. Tests show that network scans work fine with
> this patch applied."

Looks good, thanks. I see that you submitted v2 already with this added.

-- 
Kalle Valo