Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data

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

On 27-1-2017 11:10, Pali Rohár wrote:
> On Friday 27 January 2017 11:05:32 Arend Van Spriel wrote:
>> On 27-1-2017 10:43, Pali Rohár wrote:
>>> On Friday 27 January 2017 09:33:40 Kalle Valo wrote:
>>>> Pali Rohár <pali.rohar@gmail.com> writes:
>>>>
>>>>> NVS calibration data for wl1251 are model specific. Every one device with
>>>>> wl1251 chip has different and calibrated in factory.
>>>>>
>>>>> Not all wl1251 chips have own EEPROM where are calibration data stored. And
>>>>> in that case there is no "standard" place. Every device has stored them on
>>>>> different place (some in rootfs file, some in dedicated nand partition,
>>>>> some in another proprietary structure).
>>>>>
>>>>> Kernel wl1251 driver cannot support every one different storage decided by
>>>>> device manufacture so it will use request_firmware_prefer_user() call for
>>>>> loading NVS calibration data and userspace helper will be responsible to
>>>>> prepare correct data.
>>>>>
>>>>> In case userspace helper fails request_firmware_prefer_user() still try to
>>>>> load data file directly from VFS as fallback mechanism.
>>>>>
>>>>> On Nokia N900 device which has wl1251 chip, NVS calibration data are stored
>>>>> in CAL nand partition. CAL is proprietary Nokia key/value format for nand
>>>>> devices.
>>>>>
>>>>> With this patch it is finally possible to load correct model specific NVS
>>>>> calibration data for Nokia N900.
>>>>>
>>>>> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
>>>>> ---
>>>>>  drivers/net/wireless/ti/wl1251/Kconfig |    1 +
>>>>>  drivers/net/wireless/ti/wl1251/main.c  |    2 +-
>>>>>  2 files changed, 2 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/net/wireless/ti/wl1251/Kconfig b/drivers/net/wireless/ti/wl1251/Kconfig
>>>>> index 7142ccf..affe154 100644
>>>>> --- a/drivers/net/wireless/ti/wl1251/Kconfig
>>>>> +++ b/drivers/net/wireless/ti/wl1251/Kconfig
>>>>> @@ -2,6 +2,7 @@ config WL1251
>>>>>  	tristate "TI wl1251 driver support"
>>>>>  	depends on MAC80211
>>>>>  	select FW_LOADER
>>>>> +	select FW_LOADER_USER_HELPER
>>>>>  	select CRC7
>>>>>  	---help---
>>>>>  	  This will enable TI wl1251 driver support. The drivers make
>>>>> diff --git a/drivers/net/wireless/ti/wl1251/main.c b/drivers/net/wireless/ti/wl1251/main.c
>>>>> index 208f062..24f8866 100644
>>>>> --- a/drivers/net/wireless/ti/wl1251/main.c
>>>>> +++ b/drivers/net/wireless/ti/wl1251/main.c
>>>>> @@ -110,7 +110,7 @@ static int wl1251_fetch_nvs(struct wl1251 *wl)
>>>>>  	struct device *dev = wiphy_dev(wl->hw->wiphy);
>>>>>  	int ret;
>>>>>  
>>>>> -	ret = request_firmware(&fw, WL1251_NVS_NAME, dev);
>>>>> +	ret = request_firmware_prefer_user(&fw, WL1251_NVS_NAME, dev);
>>>>
>>>> I don't see the need for this. Just remove the default nvs file from
>>>> filesystem and the fallback user helper will be always used, right?
>>>
>>> It is part of linux-firmware repository. And already part of all
>>> previous versions of linux-firmware packages in lot of linux
>>> distributions. So removing it is not possible...
>>
>> You are probably saying that on your platform you can not remove
>> anything from /lib/firmware, right? I don't see how you come from "it is
>> part of firmware package" to "removing is not possible". Trying to
>> understand this and it makes no sense.
> 
> It is already in linux distribution packages. If I remove that file from
> file system it will be placed there again by package management or it it
> will throw error message about system integrity (missing file, etc...).
> 
> Also that file is already in linux-firmware git and so is propagated to
> /lib/firmware by anybody who is using linux-firmware.
> 
>>>> Like we discussed earlier, the default nvs file should not be used by
>>>> normal users.
>>>
>>> But already is and we need to deal with this fact.
>>
>> Why?
> 
> Because everybody has already installed it.
> 
>> Are there other platforms that use the default nvs file and have a
>> working wifi.
> 
> I do not know.
> 
>> So your "removing is not possible" would be about
>> regression for those?
> 
> Yes, that is possible.
> 
> Also you can use wifi on Nokia N900 with this default file. Yes it is
> not recommended and probably has performance problems... but more people
> use it for SSH and it is working. Pavel could confirm this.
> 
> So yes, if you remove that file *now* there is regression for Nokia N900
> when you are using SSH over wifi.

So you are changing the behavior for all platforms using wl1251, but the
user-helper preference is (probably) only applicable for N900, right? So
for those other platforms there will be a delay waiting for user-mode
helper to fail, before trying to get nvs file from /lib/firmware.

Regards,
Arend