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

Arend Van Spriel
2017-01-27T10:27:26Z

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