Return-path: Received: from mail-wg0-f42.google.com ([74.125.82.42]:34320 "EHLO mail-wg0-f42.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750980AbaLHTgS (ORCPT ); Mon, 8 Dec 2014 14:36:18 -0500 From: Pali =?utf-8?q?Roh=C3=A1r?= To: Dan Williams Subject: Re: wl1251: NVS firmware data Date: Mon, 8 Dec 2014 20:36:14 +0100 Cc: Marcel Holtmann , "Greg Kroah-Hartman" , Ming Lei , Pavel Machek , "John W. Linville" , Grazvydas Ignotas , "linux-wireless@vger.kernel.org" , Network Development , Linux Kernel Mailing List , Ivaylo Dimitrov , Aaro Koskinen , Kalle Valo , Sebastian Reichel , David Gnedt References: <201411271506.20457@pali> <201412082015.18501@pali> <1418066813.1350.18.camel@dcbw.local> In-Reply-To: <1418066813.1350.18.camel@dcbw.local> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="nextPart2685103.Amq4U7Xg8r"; protocol="application/pgp-signature"; micalg=pgp-sha1 Message-Id: <201412082036.14609@pali> (sfid-20141208_203638_189561_8AC3FC09) Sender: linux-wireless-owner@vger.kernel.org List-ID: --nextPart2685103.Amq4U7Xg8r Content-Type: Text/Plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable On Monday 08 December 2014 20:26:53 Dan Williams wrote: > On Mon, 2014-12-08 at 20:15 +0100, Pali Roh=C3=A1r wrote: > > On Monday 08 December 2014 19:50:17 Marcel Holtmann wrote: > > > Hi Pali, > > >=20 > > > >>>>>> On Saturday 06 December 2014 13:49:54 Pavel Machek > > > >>>>>> wrote: /** > > > >>>>>>=20 > > > >>>>>> + * request_firmware_prefer_user: - prefer usermode > > > >>>>>> helper for loading firmware + * @firmware_p: > > > >>>>>> pointer to firmware image > > > >>>>>> + * @name: name of firmware file > > > >>>>>> + * @device: device for which firmware is being > > > >>>>>> loaded + * > > > >>>>>> + * This function works pretty much like > > > >>>>>> request_firmware(), but it prefer + * usermode > > > >>>>>> helper. If usermode helper fails then it fallback > > > >>>>>> to direct access. + * Usefull for dynamic or model > > > >>>>>> specific firmware data. + **/ > > > >>>>>> +int request_firmware_prefer_user(const struct > > > >>>>>> firmware **firmware_p, + =20 > > > >>>>>> const char *name, struct device *device) +{ > > > >>>>>> + int ret; > > > >>>>>> + __module_get(THIS_MODULE); > > > >>>>>> + ret =3D _request_firmware(firmware_p, name, > > > >>>>>> device, + =20 > > > >>>>>> FW_OPT_UEVENT > > > >>>>>>=20 > > > >>>>>> | FW_OPT_PREFER_USER); + > > > >>>>>>=20 > > > >>>>>> module_put(THIS_MODULE); + return ret; > > > >>>>>> +} > > > >>>>>> +EXPORT_SYMBOL_GPL(request_firmware_prefer_user); > > > >>>>>=20 > > > >>>>> I'd like to introduce request_firmware_user() which > > > >>>>> only requests firmware from user space, and this > > > >>>>> way is simpler and more flexible since we have > > > >>>>> request_firmware_direct() already. > > > >>>>=20 > > > >>>> Why would a driver care about what program provides > > > >>>> the firmware? It shouldn't at all, and we want to > > > >>>> get rid of the userspace firmware loader, not > > > >>>> encourage drivers to use it "exclusively" at all. > > > >>>=20 > > > >>> Do not remove it! Without userspace firmware loader it > > > >>> is impossible to load dynamic firmware files. > > > >>=20 > > > >> why is this dynamic in the first place. It does not > > > >> sound like dynamic data to me at all. This is like the > > > >> WiFi MAC address(es) or Bluetooth BD_ADDR. They are > > > >> all static information. The only difference is that > > > >> they are on the host accessibly filesystem or storage > > > >> and not on the device itself. > > > >>=20 > > > >> To be honest, for Bluetooth we solved this now. If the > > > >> device is missing key information like the calibration > > > >> data or BD_ADDR, then it comes up unconfigured. A > > > >> userspace process can then go and load the right data > > > >> into it and then the device becomes available as > > > >> Bluetooth device. > > > >>=20 > > > >> Trying to use request_firmware to load some random data > > > >> and insist on going through userspace helper for that > > > >> sounds crazy to me. Especially since we are trying > > > >> hard to get away from the userspace loader. Forcing to > > > >> keep it for new stuff sounds backwards to me. > > > >>=20 > > > >> With the special Nokia partition in mind, why hasn't > > > >> this been turned into a mountable filesystem or into a > > > >> driver/subsystem that can access the data direct from > > > >> the kernel. I advocated for this some time ago. Maybe > > > >> there should be a special subsystem for access to > > > >> these factory persistent information that drivers then > > > >> just can access. I seem to remember that some systems > > > >> provide these via ACPI. Why does the ARM platform has > > > >> to be special here? > > > >>=20 > > > >> And the problem of getting Ethernet and WiFi MAC > > > >> address and Bluetooth BD_ADDR comes up many many > > > >> times. Why not have something generic here. And don't > > > >> tell me request_firmware is that generic solution ;) > > > >>=20 > > > >> Regards > > > >>=20 > > > >> Marcel > > > >=20 > > > > Hi Marcel. I think you did not understand this problem. > > > > This discussion is not about mac address. Please read > > > > email thread again and if there are some unclear pars, > > > > then ask. Thanks! > > >=20 > > > I think that I pretty clearly understand the problem. > > > Calibration data, MAC address, what is the difference? For > > > me this is all the same. It is data that is specific to a > > > device or type of devices and it is stored somewhere > > > else. In most cases in some immutable memory/flash area. > >=20 > > Those calibration data (in form of binary NVS firmware file) > > needs to be sent to wl1251 chip. Mac address is not needed > > at this step (and kernel generate some random if is not > > provided). > >=20 > > (Just to note wl1271 driver loads both MAC address and NVS > > data via one firmware file which is prepared by userspace, > > but this discussion is about wl1251...) > >=20 > > > What you want is access to this data since the kernel > > > driver needs it. Do I get this so far ;) > >=20 > > Yes, we need to provide NVS data to kernel when kernel ask > > for them. > >=20 > > > So my take is that request_firmware is not the right way > > > to get this data. Or more precisely make sure that this > > > data is available to kernel drivers. And what I am seeing > > > here is that instead of actually solving the bigger > > > problem, we just hack around it with request_firmware. > > > Now surprisingly the request_firmware loads files > > > directly from the kernel and all the hacks do not work > > > anymore. > > >=20 > > > Regards > > >=20 > > > Marcel > >=20 > > Just read emails again... > >=20 > > Our problem is: > >=20 > > linux-firmware.git tree provides two binary firmware files: > >=20 > > ti-connectivity/wl1251-fw.bin > > ti-connectivity/wl1251-nvs.bin > >=20 > > First is firmware file, second NVS file with generic > > calibration data. Kernel driver wl1251 now loads both > > firmware files via request_firmware. Generic calibration > > data are enough for wl1251 chip (it should work). But > > devices have own calibration data stored somewhere else. > >=20 > > On Nokia N900 NVS data are generated on-the-fly from some > > bytes from CAL (/dev/mtd1), from state of cellular network > > and from some other regulation settings. > >=20 > > So I think that files stored in linux-firmware.git tree > > (which are also installed into /lib/firmware/) should be > > loaded with request_firmware function. Or not? Do you think > > something else? What other developers think? > >=20 > > I'm against kernel driver for CAL (/dev/mtd1) for more > > reasons: > >=20 > > 1) we have userspace open source code, but licensed under > > GPLv3. And until kernel change license, we cannot include > > it. > >=20 > > 2) NVS data are (probably) not in one place, plus they > > depends on something other. > >=20 > > 3) If manufacture XYZ create new device with its own storage > > format of calibration data this means that correct solution > > for XYZ is also to implement new kernel fs driver for its > > own format. Do you really want to have in kernel all those > > drivers for all different (proprietary) storage formats? > >=20 > > 4) It does not help us with existence of generic file > > /lib/firmware/ti-connectivity/wl1251-nvs.bin which comes > > from linux-firmware.git tree. >=20 > a) change driver to prefer a new "wl1251-nvs-n900.bin" file, Why to "*-n900.bin" ? wl1251 driver is used on other devices too. > but fall back to "wl1251-nvs.bin" if the first one isn't > present > b) have a "wl1251-cal-nvs-update" service that, if > wl1521-nvs-n900.bin is *not* present mounts the CAL MTD, > reads the data, writes it out into wl1521-nvs-n900.bin, and > the rmmod/modprobes the driver >=20 Quote: > On Nokia N900 NVS data are generated on-the-fly from some bytes=20 > from CAL (/dev/mtd1), from state of cellular network and from=20 > some other regulation settings. This basically means to rewrite it every boot or everytime when=20 country was changed (for regulation settings). And Ii really do=20 not want to do that. And rmmod is not working on statically linked drivers into=20 zImage. So this is not solution. > and done? Stuff that's not N900 just wouldn't ship the update > service and would proceed like none of this happened. >=20 > Dan Again, what is wrong with userspace firmware helper? I think that=20 it fix this problem in a clean way without any hacks (like CAL in=20 kernel or creating new FS specially for parsing NVS and so on) in=20 kernel. And in userspace we can implement program which generate=20 NVS firmware data on-the-fly and send them to kernel in=20 compatible format of ti-connectivity/wl1251-nvs.bin =2D-=20 Pali Roh=C3=A1r pali.rohar@gmail.com --nextPart2685103.Amq4U7Xg8r Content-Type: application/pgp-signature; name=signature.asc Content-Description: This is a digitally signed message part. -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (GNU/Linux) iEYEABECAAYFAlSF/a4ACgkQi/DJPQPkQ1KdwwCfcRIPIHM8Jw/RdmLcKJBQeB5o tBIAoIGzEQeFxha424ubCK3EUQdeaqIm =2KZO -----END PGP SIGNATURE----- --nextPart2685103.Amq4U7Xg8r--