Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data
On Fri, Jun 30, 2017 at 11:35:41PM +0200, Arend van Spriel wrote: > On 23-06-17 23:53, Luis R. Rodriguez wrote: > > On Tue, May 16, 2017 at 10:41:08AM +0200, Arend Van Spriel wrote: > >> On 16-5-2017 1:13, Luis R. Rodriguez wrote: > >>> Since no upstream delta is needed for firmwared I'd like to first > >>> encourage > >>> evaluating the above. While distributions don't carry it yet that may be > >>> seen as > >>> an issue but since what we are looking for are corner cases, only folks > >>> needing > >>> to deploy a specific solution would need it or a custom proprietary > >>> solution. > >> > >> Ok. I will go try and run firmwared in OpenWrt on a router platform. > >> Have to steal one from a colleague :-p Will study firmwared. > > > > Arend, curious how this effort goes. Its important to me as we know then > > that > > if this works its a good approach to recommend moving forward which should > > also > > prove less complex than that soup we had with the custom fallback stuff. > > Hi Luis, > > So I looked at firmwared and we basically need to extend it. And actually as me and Johannes spoke long ago at Plumbers, its rather expected folks would need this or just fork it off completely. firmwared would just be a reference codebase on how to do these custom loaders. *How regular* Linux distributions embrace this is still up in the air then, because as Lennart has pointed out there is no plan to merge firmwared to systemd. *If* this is a requirement only for non-upstream drivers or patches on which will not be merged upstream then this will work long term. If you need regular distros to do something custom then their respective upstream tools would need to be evaluated for how something similar would be done. As far as I can tell perhaps the remote-proc thing would count as one possible use-case for upstream drivers -- but even then the systems that use it seem likely to use Android and then some custom userspace. > For our > router platform we need to obtain nvram calibration data from an MTD > partition which contains the raw data, ie. no file-system on it. So > firmwared would need some sort of configuration to map a particular > firmware file to some action obtaining the data like kicking off some > mtd-utils in my case. I see. So platform specific. Thanks for the update! Luis
Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data
On 23-06-17 23:53, Luis R. Rodriguez wrote: > On Tue, May 16, 2017 at 10:41:08AM +0200, Arend Van Spriel wrote: >> On 16-5-2017 1:13, Luis R. Rodriguez wrote: >>> Since no upstream delta is needed for firmwared I'd like to first encourage >>> evaluating the above. While distributions don't carry it yet that may be >>> seen as >>> an issue but since what we are looking for are corner cases, only folks >>> needing >>> to deploy a specific solution would need it or a custom proprietary >>> solution. >> >> Ok. I will go try and run firmwared in OpenWrt on a router platform. >> Have to steal one from a colleague :-p Will study firmwared. > > Arend, curious how this effort goes. Its important to me as we know then that > if this works its a good approach to recommend moving forward which should > also > prove less complex than that soup we had with the custom fallback stuff. Hi Luis, So I looked at firmwared and we basically need to extend it. For our router platform we need to obtain nvram calibration data from an MTD partition which contains the raw data, ie. no file-system on it. So firmwared would need some sort of configuration to map a particular firmware file to some action obtaining the data like kicking off some mtd-utils in my case. Regards, Arend
Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data
On Tue, May 16, 2017 at 10:41:08AM +0200, Arend Van Spriel wrote: > On 16-5-2017 1:13, Luis R. Rodriguez wrote: > > Since no upstream delta is needed for firmwared I'd like to first encourage > > evaluating the above. While distributions don't carry it yet that may be > > seen as > > an issue but since what we are looking for are corner cases, only folks > > needing > > to deploy a specific solution would need it or a custom proprietary > > solution. > > Ok. I will go try and run firmwared in OpenWrt on a router platform. > Have to steal one from a colleague :-p Will study firmwared. Arend, curious how this effort goes. Its important to me as we know then that if this works its a good approach to recommend moving forward which should also prove less complex than that soup we had with the custom fallback stuff. Luis
Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data
On Wed 17 May 05:53 PDT 2017, Pali Roh?r wrote: > On Wednesday 17 May 2017 14:06:06 Johannes Berg wrote: > > On Tue, 2017-05-16 at 01:13 +0200, Luis R. Rodriguez wrote: > > > > > Now for N900 case there is a similar scenario > > > > > alhtough it has additional requirement to go to user-space due to > > > > > need to use a proprietary library to obtain the NVS calibration > > > > > data. My thought: Why should firmware_class care? > > > > > > Agreed. > > > > In fact, why should the *driver* care either? IOW - why should > > "request_firmware_prefer_user()" even exist? > > There are default/example NVS data, which are stored in /lib/firmware > and installed by linux-firmware package. Those example calibration data > should not be used for real usage, but Pavel told us that on N900 they > are enough for working WIFI connection. They does not contain valid MAC > address, so kernel should generate some (random?). > > So kernel driver should get NVS calibration data from userspace (which > know how where to get or how to prepare them) and in case userspace do > not have it, then we can try fallback to those example data (as people > reported us they can be useful instead of non-working WIFI). > We're going to see a similar case with the Qualcomm DB410c WiFi soon, where there is default calibration for the chip (wcn3620) but specific calibration data for the particular board or product using this chip. As with your case we expect to have a "generic" calibration file integrated in linux-firmware, but providing means to supporting device-specific calibration is probably going to be requested shortly. We have however altered the reference design of picking the MAC address from the calibration data and have the bootloader pass it via DT - so our calibration data doesn't need to be specific to each unit. Regards, Bjorn
Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data
On Wed, 2017-05-17 at 15:21 +0200, Pali Rohár wrote: > On Wednesday 17 May 2017 15:04:50 Johannes Berg wrote: > > On Wed, 2017-05-17 at 14:53 +0200, Pali Rohár wrote: > > > > > > In fact, why should the *driver* care either? IOW - why should > > > > "request_firmware_prefer_user()" even exist? > > > > > > There are default/example NVS data, which are stored in > > > /lib/firmware > > > and installed by linux-firmware package. > > > > [...] > > > > Oh, so you're saying you want this to invert the order ... Ok, that > > makes some sense. > > Yes! I thought that this fact can be understood from commit message. > If not, I can change it, but provide how to improve it. It probably can, I was only Cc'ed later :) Sorry for the noise. johannes
Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data
On Wednesday 17 May 2017 15:04:50 Johannes Berg wrote: > On Wed, 2017-05-17 at 14:53 +0200, Pali Rohár wrote: > > > > In fact, why should the *driver* care either? IOW - why should > > > "request_firmware_prefer_user()" even exist? > > > > There are default/example NVS data, which are stored in /lib/firmware > > and installed by linux-firmware package. > [...] > > Oh, so you're saying you want this to invert the order ... Ok, that > makes some sense. Yes! I thought that this fact can be understood from commit message. If not, I can change it, but provide how to improve it. -- Pali Rohár pali.ro...@gmail.com
Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data
On Wed, 2017-05-17 at 14:53 +0200, Pali Rohár wrote: > > In fact, why should the *driver* care either? IOW - why should > > "request_firmware_prefer_user()" even exist? > > There are default/example NVS data, which are stored in /lib/firmware > and installed by linux-firmware package. [...] Oh, so you're saying you want this to invert the order ... Ok, that makes some sense. I still hope that all other requests will eventually fall back to user loading though, I think that's important to system integration in general. johannes
Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data
On Wednesday 17 May 2017 14:06:06 Johannes Berg wrote: > On Tue, 2017-05-16 at 01:13 +0200, Luis R. Rodriguez wrote: > > > > Now for N900 case there is a similar scenario > > > > alhtough it has additional requirement to go to user-space due to > > > > need to use a proprietary library to obtain the NVS calibration > > > > data. My thought: Why should firmware_class care? > > > > Agreed. > > In fact, why should the *driver* care either? IOW - why should > "request_firmware_prefer_user()" even exist? There are default/example NVS data, which are stored in /lib/firmware and installed by linux-firmware package. Those example calibration data should not be used for real usage, but Pavel told us that on N900 they are enough for working WIFI connection. They does not contain valid MAC address, so kernel should generate some (random?). So kernel driver should get NVS calibration data from userspace (which know how where to get or how to prepare them) and in case userspace do not have it, then we can try fallback to those example data (as people reported us they can be useful instead of non-working WIFI). And that fallback is working by direct firmware loading from kernel. -- Pali Rohár pali.ro...@gmail.com
Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data
On Tue, 2017-05-16 at 01:13 +0200, Luis R. Rodriguez wrote: > > > Now for N900 case there is a similar scenario > > > alhtough it has additional requirement to go to user-space due to > > > need to use a proprietary library to obtain the NVS calibration > > > data. My thought: Why should firmware_class care? > > Agreed. In fact, why should the *driver* care either? IOW - why should "request_firmware_prefer_user()" even exist? > > > So the idea is that firmware_class provides > > > a registry for modules that can produce a certain firmware > > > "file". Those > > > modules can do whatever is needed. If they need to use umh so be > > > it. > > > They would only register themselves with firmware_class on > > > platforms > > > that need them. It would basically be replacing the fallback > > > mechanism > > > and only be effective on certain platforms. > > Sure, so it sounds like the work that Daniel Wagner and Tom Gundersen > worked [0] on which provides a firmwared with two modes: best-effort, > and final-mode, would address what you are looking for but without > requiring any upstream changes, *and* it also helps solve the rootfs > race remote-proc folks had concerns over. Right. > The other added gain over this solution is if folks need their own > proprietary concoction they can just fork firmwared Or just reimplement it to the same kernel API, no need to even use the same code base. > Lastly, if we did not want to deal with timeouts for the way the > driver data API implements it I think we might be able to do away > with them for for async requests if we assume there will be a daemon > that spawns in final-mode eventually, and since it *knows* when the > rootfs is ready it should be able to do a final lookup, if it returns > -ENOENT; then indeed we know we can give up. Now, perhaps how and if > we want to deal with timeouts when using the driver data API for the > fallback mechanism is worth considering given it does not have a > fallback mechanism support yet. If we *add* them it would seem this > would also put an implicit race against userspace finishing > initialization and running firmwared in final-mode. > > Johannes, do you recall the corner cases we spoke about regarding > timeouts? Does this match what we spoke about? I think we have to protect against userspace code crashing, not existing, etc. - so I think we do need a timeout anyway. However, I don't recall any (other) corner cases we might have spoken about. > Note that firmware signing will require an additional file, the > detached signature. Is anything like that happening finally? :) johannes >
Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data
On Tue, May 16, 2017 at 10:41:08AM +0200, Arend Van Spriel wrote: > On 16-5-2017 1:13, Luis R. Rodriguez wrote: > > On Fri, May 12, 2017 at 11:02:26PM +0200, Arend Van Spriel wrote: > >> try again.. replacing email address from Michał > >> On 12-5-2017 22:55, Arend Van Spriel wrote: > >>> Let me explain the idea to refresh your memory (and mine). It started > >>> when we were working on adding driver support for OpenWrt in brcmfmac. > >>> The driver requests for firmware calibration data, but on routers it is > >>> stored in flash. So after failing on the firmware request we now call a > >>> platform specific API. That was my itch, but it was not bad enough to go > >>> and scratch. Now for N900 case there is a similar scenario alhtough it > >>> has additional requirement to go to user-space due to need to use a > >>> proprietary library to obtain the NVS calibration data. My thought: Why > >>> should firmware_class care? > > > > Agreed. > > > >>> So the idea is that firmware_class provides > >>> a registry for modules that can produce a certain firmware "file". Those > >>> modules can do whatever is needed. If they need to use umh so be it. > >>> They would only register themselves with firmware_class on platforms > >>> that need them. It would basically be replacing the fallback mechanism > >>> and only be effective on certain platforms. > > > > Sure, so it sounds like the work that Daniel Wagner and Tom Gundersen worked > > [0] on which provides a firmwared with two modes: best-effort, and > > final-mode, > > would address what you are looking for but without requiring any upstream > > changes, *and* it also helps solve the rootfs race remote-proc folks had > > concerns over. > > > > The other added gain over this solution is if folks need their own > > proprietary > > concoction they can just fork firmwared and have that do whatever it needs > > for the specific device on the specific rootfs. That is, firmwared can be > > the > > upstream solution if folks need it, but if folks need something custom they > > can > > just mimic the implementation: best-effort, and and final-mode. > > > > Yet another added gain over this solution we can do *not* support the > > custom fallback mechanism as its not needed, the udev event should suffice > > to let userspace do what it needs. > > > > Lastly, if we did not want to deal with timeouts for the way the driver data > > API implements it I think we might be able to do away with them for for > > async > > requests if we assume there will be a daemon that spawns in final-mode > > eventually, > > and since it *knows* when the rootfs is ready it should be able to do a > > final > > lookup, if it returns -ENOENT; then indeed we know we can give up. Now, > > perhaps > > how and if we want to deal with timeouts when using the driver data API for > > the fallback mechanism is worth considering given it does not have a > > fallback > > mechanism support yet. If we *add* them it would seem this would also put an > > implicit race against userspace finishing initialization and running > > firmwared > > in final-mode. > > Just to be clear. When you are saying "rootfs" in this story, you mean > any (mounted) file-system which may hold the firmware. At least that was > one of the arguments. In kernel space we can not know how the system is > setup in terms of mount points, let alone on which mounted file-system > the firmware resides. Right, wherever the hell that thing is on, which could be on a crypic fuse drive waiting for some bits to be decrypted from Elon Musk on a spaceship on his way to Mars, and only userspace knows how to decrypt this thing through some evil proprietary thing, way way after a full bootup. > > Johannes, do you recall the corner cases we spoke about regarding timeouts? > > Does this match what we spoke about? > > > >>> Let me know if this idea is still of interest and I will rebase what I > >>> have for an RFC round. > > > > Since no upstream delta is needed for firmwared I'd like to first encourage > > evaluating the above. While distributions don't carry it yet that may be > > seen as > > an issue but since what we are looking for are corner cases, only folks > > needing > > to deploy a specific solution would need it or a custom proprietary > > solution. > > Ok. I will go try and run firmwared in OpenWrt on a router platform. > Have to steal one from a colleague :-p Will study firmwared. The finale-mode is the trick. > > [0] https://github.com/teg/firmwared.git > > > > PS. > > > > Note that firmware signing will require an additional file, the detached > > signature. The driver data API does not currently support the fallback > > mechanism so we would not have to worry about that yet but once we add > > fallback support we'd need to consider this. > > Do you have references to the firmware signing design. Is the idea to > have one signature and all "firmware files" need to be signed with it? Nope, I'm afraid a lot has be
Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data
On 16-5-2017 1:13, Luis R. Rodriguez wrote: > On Fri, May 12, 2017 at 11:02:26PM +0200, Arend Van Spriel wrote: >> try again.. replacing email address from Michał >> On 12-5-2017 22:55, Arend Van Spriel wrote: >>> Let me explain the idea to refresh your memory (and mine). It started >>> when we were working on adding driver support for OpenWrt in brcmfmac. >>> The driver requests for firmware calibration data, but on routers it is >>> stored in flash. So after failing on the firmware request we now call a >>> platform specific API. That was my itch, but it was not bad enough to go >>> and scratch. Now for N900 case there is a similar scenario alhtough it >>> has additional requirement to go to user-space due to need to use a >>> proprietary library to obtain the NVS calibration data. My thought: Why >>> should firmware_class care? > > Agreed. > >>> So the idea is that firmware_class provides >>> a registry for modules that can produce a certain firmware "file". Those >>> modules can do whatever is needed. If they need to use umh so be it. >>> They would only register themselves with firmware_class on platforms >>> that need them. It would basically be replacing the fallback mechanism >>> and only be effective on certain platforms. > > Sure, so it sounds like the work that Daniel Wagner and Tom Gundersen worked > [0] on which provides a firmwared with two modes: best-effort, and final-mode, > would address what you are looking for but without requiring any upstream > changes, *and* it also helps solve the rootfs race remote-proc folks had > concerns over. > > The other added gain over this solution is if folks need their own proprietary > concoction they can just fork firmwared and have that do whatever it needs > for the specific device on the specific rootfs. That is, firmwared can be the > upstream solution if folks need it, but if folks need something custom they > can > just mimic the implementation: best-effort, and and final-mode. > > Yet another added gain over this solution we can do *not* support the > custom fallback mechanism as its not needed, the udev event should suffice > to let userspace do what it needs. > > Lastly, if we did not want to deal with timeouts for the way the driver data > API implements it I think we might be able to do away with them for for async > requests if we assume there will be a daemon that spawns in final-mode > eventually, > and since it *knows* when the rootfs is ready it should be able to do a final > lookup, if it returns -ENOENT; then indeed we know we can give up. Now, > perhaps > how and if we want to deal with timeouts when using the driver data API for > the fallback mechanism is worth considering given it does not have a fallback > mechanism support yet. If we *add* them it would seem this would also put an > implicit race against userspace finishing initialization and running firmwared > in final-mode. Just to be clear. When you are saying "rootfs" in this story, you mean any (mounted) file-system which may hold the firmware. At least that was one of the arguments. In kernel space we can not know how the system is setup in terms of mount points, let alone on which mounted file-system the firmware resides. > Johannes, do you recall the corner cases we spoke about regarding timeouts? > Does this match what we spoke about? > >>> Let me know if this idea is still of interest and I will rebase what I >>> have for an RFC round. > > Since no upstream delta is needed for firmwared I'd like to first encourage > evaluating the above. While distributions don't carry it yet that may be seen > as > an issue but since what we are looking for are corner cases, only folks > needing > to deploy a specific solution would need it or a custom proprietary solution. Ok. I will go try and run firmwared in OpenWrt on a router platform. Have to steal one from a colleague :-p Will study firmwared. > [0] https://github.com/teg/firmwared.git > > PS. > > Note that firmware signing will require an additional file, the detached > signature. The driver data API does not currently support the fallback > mechanism so we would not have to worry about that yet but once we add > fallback support we'd need to consider this. Do you have references to the firmware signing design. Is the idea to have one signature and all "firmware files" need to be signed with it? Thanks, Arend
Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data
On Fri, May 12, 2017 at 11:02:26PM +0200, Arend Van Spriel wrote: > try again.. replacing email address from Michał > On 12-5-2017 22:55, Arend Van Spriel wrote: > > Let me explain the idea to refresh your memory (and mine). It started > > when we were working on adding driver support for OpenWrt in brcmfmac. > > The driver requests for firmware calibration data, but on routers it is > > stored in flash. So after failing on the firmware request we now call a > > platform specific API. That was my itch, but it was not bad enough to go > > and scratch. Now for N900 case there is a similar scenario alhtough it > > has additional requirement to go to user-space due to need to use a > > proprietary library to obtain the NVS calibration data. My thought: Why > > should firmware_class care? Agreed. > > So the idea is that firmware_class provides > > a registry for modules that can produce a certain firmware "file". Those > > modules can do whatever is needed. If they need to use umh so be it. > > They would only register themselves with firmware_class on platforms > > that need them. It would basically be replacing the fallback mechanism > > and only be effective on certain platforms. Sure, so it sounds like the work that Daniel Wagner and Tom Gundersen worked [0] on which provides a firmwared with two modes: best-effort, and final-mode, would address what you are looking for but without requiring any upstream changes, *and* it also helps solve the rootfs race remote-proc folks had concerns over. The other added gain over this solution is if folks need their own proprietary concoction they can just fork firmwared and have that do whatever it needs for the specific device on the specific rootfs. That is, firmwared can be the upstream solution if folks need it, but if folks need something custom they can just mimic the implementation: best-effort, and and final-mode. Yet another added gain over this solution we can do *not* support the custom fallback mechanism as its not needed, the udev event should suffice to let userspace do what it needs. Lastly, if we did not want to deal with timeouts for the way the driver data API implements it I think we might be able to do away with them for for async requests if we assume there will be a daemon that spawns in final-mode eventually, and since it *knows* when the rootfs is ready it should be able to do a final lookup, if it returns -ENOENT; then indeed we know we can give up. Now, perhaps how and if we want to deal with timeouts when using the driver data API for the fallback mechanism is worth considering given it does not have a fallback mechanism support yet. If we *add* them it would seem this would also put an implicit race against userspace finishing initialization and running firmwared in final-mode. Johannes, do you recall the corner cases we spoke about regarding timeouts? Does this match what we spoke about? > > Let me know if this idea is still of interest and I will rebase what I > > have for an RFC round. Since no upstream delta is needed for firmwared I'd like to first encourage evaluating the above. While distributions don't carry it yet that may be seen as an issue but since what we are looking for are corner cases, only folks needing to deploy a specific solution would need it or a custom proprietary solution. [0] https://github.com/teg/firmwared.git PS. Note that firmware signing will require an additional file, the detached signature. The driver data API does not currently support the fallback mechanism so we would not have to worry about that yet but once we add fallback support we'd need to consider this. Luis
Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data
On 4-5-2017 4:28, Luis R. Rodriguez wrote: > On Wed, May 03, 2017 at 09:02:20PM +0200, Arend Van Spriel wrote: >> On 3-1-2017 18:59, Luis R. Rodriguez wrote: >>> On Mon, Dec 26, 2016 at 05:35:59PM +0100, Pavel Machek wrote: Right question is "should we solve it without user-space help"? Answer is no, too. Way too complex. Yes, it would be nice if hardware was designed in such a way that getting calibration data from kernel is easy, and if you design hardware, please design it like that. But N900 is not designed like that and getting the calibration through userspace looks like only reasonable solution. >>> >>> Arend seems to have a better alternative in mind possible for other >>> devices which *can* probably pull of doing this easily and nicely, >>> given the nasty history of the usermode helper crap we should not >>> in any way discourage such efforts. >>> >>> Arend -- please look at the firmware cache, it not a hash but a hash >>> table for an O(1) lookups would be a welcomed change, then it could >>> be repurposed for what you describe, I think the only difference is >>> you'd perhaps want a custom driver hook to fetch the calibration data >>> so the driver does whatever it needs. >> >> Hi Luis, >> >> I let my idea catch dust on the shelf for a while. > > :) BTW did you get to check out Daniel Wagner and Tom Gundersen's firmware > work > [0] ? This can provide a proper clear fallback mechanism which *also* helps > address the race between "critical mount points ready" problem we had > discussed > long ago. IIRC it did this by having two modes of operation a best effort-mode > and a final-mode. The final-mode would only be used once all the real rootfs > is > ready. Userspace decides when to kick / signal firmwared to switch to > final-mode > as only userspace will know for sure when that is ready. The best-effort mode > would be used in initramfs. There is also no need for a "custom fallback", the > uevent fallback mechanism can be relied upon alone. Custom tools can just fork > off and do something similar to firmward then in terms of architecture. This > is > a form of fallback mechanism I think I'd be happy to see enabled on the new > driver data API. But of course, I recall also liking what you had in mind as > well > so would be happy to see more alternatives! The cleaner the better ! > > [0] https://github.com/teg/firmwared > >> Actually had a couple >> of patches ready, but did get around testing them. So I wanted to rebase >> them on your linux-next tree. I bumped into the umh lock thing and was >> wondering why direct filesystem access was under that lock as well. > > Indeed, it was an odd thing. > >> In your tree I noticed a fix for that. > > Yup! > > It took a lot of git archeology to reach a sound approach forward which makes > me feel comfortable without regressing the kernel, this should not regress > the kernel. > >> The more reason to base my work on >> top of your firmware_class changes. Now my question is what is the best >> branch to choose, because you have a "few" in that repo to choose from ;-) > > I have a series of topical changes, and I rebase onto the latest linux-next > regularly before posting patches, if 0-day finds issues, I dish out a next > try2 or tryX iteration until all issues are fixed. So in this case you'd > just go for the latest driver-data-$(next_date) and if there is a try > postfix use the latest tryX. > > In this case 20170501-driver-data-try2, this is based on linux-next tag > next-20170501. If you have issues booting on that next tag though you > could instead try the v4.11-rc8-driver-data-try3 branch based on v4.11-rc8. > But naturally patches ultimately should be based on the latest linux-next > tag for actual submission. > > PS. after my changes the fallback mechanism can easily be shoved into its > own file, not sure if that helps with how clean of a solution your work > will be. Let me explain the idea to refresh your memory (and mine). It started when we were working on adding driver support for OpenWrt in brcmfmac. The driver requests for firmware calibration data, but on routers it is stored in flash. So after failing on the firmware request we now call a platform specific API. That was my itch, but it was not bad enough to go and scratch. Now for N900 case there is a similar scenario alhtough it has additional requirement to go to user-space due to need to use a proprietary library to obtain the NVS calibration data. My thought: Why should firmware_class care? So the idea is that firmware_class provides a registry for modules that can produce a certain firmware "file". Those modules can do whatever is needed. If they need to use umh so be it. They would only register themselves with firmware_class on platforms that need them. It would basically be replacing the fallback mechanism and only be effective on certain platforms. Let me know if this idea is still of interest and I will rebase wh
Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data
On Wed, May 03, 2017 at 09:02:20PM +0200, Arend Van Spriel wrote: > On 3-1-2017 18:59, Luis R. Rodriguez wrote: > > On Mon, Dec 26, 2016 at 05:35:59PM +0100, Pavel Machek wrote: > >> > >> Right question is "should we solve it without user-space help"? > >> > >> Answer is no, too. Way too complex. Yes, it would be nice if hardware > >> was designed in such a way that getting calibration data from kernel > >> is easy, and if you design hardware, please design it like that. But > >> N900 is not designed like that and getting the calibration through > >> userspace looks like only reasonable solution. > > > > Arend seems to have a better alternative in mind possible for other > > devices which *can* probably pull of doing this easily and nicely, > > given the nasty history of the usermode helper crap we should not > > in any way discourage such efforts. > > > > Arend -- please look at the firmware cache, it not a hash but a hash > > table for an O(1) lookups would be a welcomed change, then it could > > be repurposed for what you describe, I think the only difference is > > you'd perhaps want a custom driver hook to fetch the calibration data > > so the driver does whatever it needs. > > Hi Luis, > > I let my idea catch dust on the shelf for a while. :) BTW did you get to check out Daniel Wagner and Tom Gundersen's firmware work [0] ? This can provide a proper clear fallback mechanism which *also* helps address the race between "critical mount points ready" problem we had discussed long ago. IIRC it did this by having two modes of operation a best effort-mode and a final-mode. The final-mode would only be used once all the real rootfs is ready. Userspace decides when to kick / signal firmwared to switch to final-mode as only userspace will know for sure when that is ready. The best-effort mode would be used in initramfs. There is also no need for a "custom fallback", the uevent fallback mechanism can be relied upon alone. Custom tools can just fork off and do something similar to firmward then in terms of architecture. This is a form of fallback mechanism I think I'd be happy to see enabled on the new driver data API. But of course, I recall also liking what you had in mind as well so would be happy to see more alternatives! The cleaner the better ! [0] https://github.com/teg/firmwared > Actually had a couple > of patches ready, but did get around testing them. So I wanted to rebase > them on your linux-next tree. I bumped into the umh lock thing and was > wondering why direct filesystem access was under that lock as well. Indeed, it was an odd thing. > In your tree I noticed a fix for that. Yup! It took a lot of git archeology to reach a sound approach forward which makes me feel comfortable without regressing the kernel, this should not regress the kernel. > The more reason to base my work on > top of your firmware_class changes. Now my question is what is the best > branch to choose, because you have a "few" in that repo to choose from ;-) I have a series of topical changes, and I rebase onto the latest linux-next regularly before posting patches, if 0-day finds issues, I dish out a next try2 or tryX iteration until all issues are fixed. So in this case you'd just go for the latest driver-data-$(next_date) and if there is a try postfix use the latest tryX. In this case 20170501-driver-data-try2, this is based on linux-next tag next-20170501. If you have issues booting on that next tag though you could instead try the v4.11-rc8-driver-data-try3 branch based on v4.11-rc8. But naturally patches ultimately should be based on the latest linux-next tag for actual submission. PS. after my changes the fallback mechanism can easily be shoved into its own file, not sure if that helps with how clean of a solution your work will be. Luis
Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data
On 3-1-2017 18:59, Luis R. Rodriguez wrote: > On Mon, Dec 26, 2016 at 05:35:59PM +0100, Pavel Machek wrote: >> >> Right question is "should we solve it without user-space help"? >> >> Answer is no, too. Way too complex. Yes, it would be nice if hardware >> was designed in such a way that getting calibration data from kernel >> is easy, and if you design hardware, please design it like that. But >> N900 is not designed like that and getting the calibration through >> userspace looks like only reasonable solution. > > Arend seems to have a better alternative in mind possible for other > devices which *can* probably pull of doing this easily and nicely, > given the nasty history of the usermode helper crap we should not > in any way discourage such efforts. > > Arend -- please look at the firmware cache, it not a hash but a hash > table for an O(1) lookups would be a welcomed change, then it could > be repurposed for what you describe, I think the only difference is > you'd perhaps want a custom driver hook to fetch the calibration data > so the driver does whatever it needs. Hi Luis, I let my idea catch dust on the shelf for a while. Actually had a couple of patches ready, but did get around testing them. So I wanted to rebase them on your linux-next tree. I bumped into the umh lock thing and was wondering why direct filesystem access was under that lock as well. In your tree I noticed a fix for that. The more reason to base my work on top of your firmware_class changes. Now my question is what is the best branch to choose, because you have a "few" in that repo to choose from ;-) Regards, Arend
Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data
On 1 February 2017 at 09:33, Pali Rohár wrote: > On Tuesday 31 January 2017 07:59:18 Tony Lindgren wrote: >> * Kalle Valo [170130 22:36]: [...] >> > * before distro updates linux-firmware create yours own deb/rpm/whatever >> > package "wl1251-firmware" which installs your flavor of nvs file (or >> > the user fallback helper if more dynamic functionality is preferred) >> >> And that won't work when using the same file system on other machines. >> >> Think NFSroot for example. At least I'm using the same NFSroot across >> about 15 different machines including one n900 macro board with smc91x >> Ethernet. > > Exactly problem which we already discussed in previous emails. You > cannot serve one file (loaded by direct request_firmware) when your > rootfs is readonly, e.g. comes via NFS shared for more devices... You can extract the nvs blob, put it in tmpfs and bind-mount (or symlink) it to /lib/firmware/ via modprobe install hook (or init scripts). Michał
Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data
On Tuesday 31 January 2017 07:59:18 Tony Lindgren wrote: > * Kalle Valo [170130 22:36]: > > Tony Lindgren writes: > > > > > * Pavel Machek [170127 11:41]: > > >> On Fri 2017-01-27 17:23:07, Kalle Valo wrote: > > >> > Pali Rohár writes: > > >> > > > >> > > On Friday 27 January 2017 14:26:22 Kalle Valo wrote: > > >> > >> Pali Rohár writes: > > >> > >> > > >> > >> > 2) It was already tested that example NVS data can be used for > > >> > >> > N900 e.g. > > >> > >> > for SSH connection. If real correct data are not available it is > > >> > >> > better > > >> > >> > to use at least those example (and probably log warning message) > > >> > >> > so user > > >> > >> > can connect via SSH and start investigating where is problem. > > >> > >> > > >> > >> I disagree. Allowing default calibration data to be used can be > > >> > >> unnoticed by user and left her wondering why wifi works so badly. > > >> > > > > >> > > So there are only two options: > > >> > > > > >> > > 1) Disallow it and so these users will have non-working wifi. > > >> > > > > >> > > 2) Allow those data to be used as fallback mechanism. > > >> > > > > >> > > And personally I'm against 1) because it will break wifi support for > > >> > > *all* Nokia N900 devices right now. > > >> > > > >> > All two of them? :) > > >> > > >> Umm. You clearly want a flock of angry penguins at your doorsteps :-). > > > > > > Well this silly issue of symlinking and renaming nvs files in a standard > > > Linux distro was also hitting me on various devices with wl12xx/wl18xx > > > trying to use the same rootfs. > > > > > > Why don't we just set a custom compatible property for n900 that then > > > picks up some other nvs file instead of the default? > > > > Please don't. An ugly kernel workaround in kernel because of user space > > problems is a bad idea. wl1251 should just ask for NVS file from user > > space, it shouldn't care if it's a "default" file or something else. > > That's a user space policy decision. > > Grr I keep forgetting it needs to be for each device manufactured so > yeah that won't work. > > The names of standard distro files are hardcoded into the kernel > driver so it's also a kernel problem though :p > > How about a custom devices tree property saying "needs-custom-firmware"? How does it help request_firmware() call which automatically loads firmware file from VFS (if is available)? > Something that would prevent anything being loaded until user space > loads the firmware. It could also be set in the driver automatically > based on the compatible flag if we always want it enabled. And we could > have some cmdline option to ignore it. Or the other way around whatever > makes sense. So you just want to kernel automatically prevent loading firmware file (based on flag which driver can set). That is similar approach as mine. > > Why can't you do something like this: > > > > * rename the NVS file linux-firmware to wl1251-nvs.bin.example > > As that name is hardcoded in the kernel and that file is provided by > all standard distros, let's assume we just have to deal with that ABI > forever. Yes. > > * before distro updates linux-firmware create yours own deb/rpm/whatever > > package "wl1251-firmware" which installs your flavor of nvs file (or > > the user fallback helper if more dynamic functionality is preferred) > > And that won't work when using the same file system on other machines. > > Think NFSroot for example. At least I'm using the same NFSroot across > about 15 different machines including one n900 macro board with smc91x > Ethernet. Exactly problem which we already discussed in previous emails. You cannot serve one file (loaded by direct request_firmware) when your rootfs is readonly, e.g. comes via NFS shared for more devices... -- Pali Rohár pali.ro...@gmail.com
Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data
* Kalle Valo [170130 22:36]: > Tony Lindgren writes: > > > * Pavel Machek [170127 11:41]: > >> On Fri 2017-01-27 17:23:07, Kalle Valo wrote: > >> > Pali Rohár writes: > >> > > >> > > On Friday 27 January 2017 14:26:22 Kalle Valo wrote: > >> > >> Pali Rohár writes: > >> > >> > >> > >> > 2) It was already tested that example NVS data can be used for N900 > >> > >> > e.g. > >> > >> > for SSH connection. If real correct data are not available it is > >> > >> > better > >> > >> > to use at least those example (and probably log warning message) so > >> > >> > user > >> > >> > can connect via SSH and start investigating where is problem. > >> > >> > >> > >> I disagree. Allowing default calibration data to be used can be > >> > >> unnoticed by user and left her wondering why wifi works so badly. > >> > > > >> > > So there are only two options: > >> > > > >> > > 1) Disallow it and so these users will have non-working wifi. > >> > > > >> > > 2) Allow those data to be used as fallback mechanism. > >> > > > >> > > And personally I'm against 1) because it will break wifi support for > >> > > *all* Nokia N900 devices right now. > >> > > >> > All two of them? :) > >> > >> Umm. You clearly want a flock of angry penguins at your doorsteps :-). > > > > Well this silly issue of symlinking and renaming nvs files in a standard > > Linux distro was also hitting me on various devices with wl12xx/wl18xx > > trying to use the same rootfs. > > > > Why don't we just set a custom compatible property for n900 that then > > picks up some other nvs file instead of the default? > > Please don't. An ugly kernel workaround in kernel because of user space > problems is a bad idea. wl1251 should just ask for NVS file from user > space, it shouldn't care if it's a "default" file or something else. > That's a user space policy decision. Grr I keep forgetting it needs to be for each device manufactured so yeah that won't work. The names of standard distro files are hardcoded into the kernel driver so it's also a kernel problem though :p How about a custom devices tree property saying "needs-custom-firmware"? Something that would prevent anything being loaded until user space loads the firmware. It could also be set in the driver automatically based on the compatible flag if we always want it enabled. And we could have some cmdline option to ignore it. Or the other way around whatever makes sense. > Why can't you do something like this: > > * rename the NVS file linux-firmware to wl1251-nvs.bin.example As that name is hardcoded in the kernel and that file is provided by all standard distros, let's assume we just have to deal with that ABI forever. > * before distro updates linux-firmware create yours own deb/rpm/whatever > package "wl1251-firmware" which installs your flavor of nvs file (or > the user fallback helper if more dynamic functionality is preferred) And that won't work when using the same file system on other machines. Think NFSroot for example. At least I'm using the same NFSroot across about 15 different machines including one n900 macro board with smc91x Ethernet. Regards, Tony
Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data
Tony Lindgren writes: > * Pavel Machek [170127 11:41]: >> On Fri 2017-01-27 17:23:07, Kalle Valo wrote: >> > Pali Rohár writes: >> > >> > > On Friday 27 January 2017 14:26:22 Kalle Valo wrote: >> > >> Pali Rohár writes: >> > >> >> > >> > 2) It was already tested that example NVS data can be used for N900 >> > >> > e.g. >> > >> > for SSH connection. If real correct data are not available it is >> > >> > better >> > >> > to use at least those example (and probably log warning message) so >> > >> > user >> > >> > can connect via SSH and start investigating where is problem. >> > >> >> > >> I disagree. Allowing default calibration data to be used can be >> > >> unnoticed by user and left her wondering why wifi works so badly. >> > > >> > > So there are only two options: >> > > >> > > 1) Disallow it and so these users will have non-working wifi. >> > > >> > > 2) Allow those data to be used as fallback mechanism. >> > > >> > > And personally I'm against 1) because it will break wifi support for >> > > *all* Nokia N900 devices right now. >> > >> > All two of them? :) >> >> Umm. You clearly want a flock of angry penguins at your doorsteps :-). > > Well this silly issue of symlinking and renaming nvs files in a standard > Linux distro was also hitting me on various devices with wl12xx/wl18xx > trying to use the same rootfs. > > Why don't we just set a custom compatible property for n900 that then > picks up some other nvs file instead of the default? Please don't. An ugly kernel workaround in kernel because of user space problems is a bad idea. wl1251 should just ask for NVS file from user space, it shouldn't care if it's a "default" file or something else. That's a user space policy decision. Why can't you do something like this: * rename the NVS file linux-firmware to wl1251-nvs.bin.example * before distro updates linux-firmware create yours own deb/rpm/whatever package "wl1251-firmware" which installs your flavor of nvs file (or the user fallback helper if more dynamic functionality is preferred) -- Kalle Valo
Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data
On Monday 30 January 2017 18:53:09 Tony Lindgren wrote: > * Pavel Machek [170127 11:41]: > > On Fri 2017-01-27 17:23:07, Kalle Valo wrote: > > > Pali Rohár writes: > > > > On Friday 27 January 2017 14:26:22 Kalle Valo wrote: > > > >> Pali Rohár writes: > > > >> > 2) It was already tested that example NVS data can be used > > > >> > for N900 e.g. for SSH connection. If real correct data are > > > >> > not available it is better to use at least those example > > > >> > (and probably log warning message) so user can connect via > > > >> > SSH and start investigating where is problem. > > > >> > > > >> I disagree. Allowing default calibration data to be used can > > > >> be unnoticed by user and left her wondering why wifi works so > > > >> badly. > > > > > > > > So there are only two options: > > > > > > > > 1) Disallow it and so these users will have non-working wifi. > > > > > > > > 2) Allow those data to be used as fallback mechanism. > > > > > > > > And personally I'm against 1) because it will break wifi > > > > support for *all* Nokia N900 devices right now. > > > > > > All two of them? :) > > > > Umm. You clearly want a flock of angry penguins at your doorsteps > > :-). > > Well this silly issue of symlinking and renaming nvs files in a > standard Linux distro was also hitting me on various devices with > wl12xx/wl18xx trying to use the same rootfs. wl12xx/wl18xx have probably exactly same problem as wl1251. > Why don't we just set a custom compatible property for n900 that then > picks up some other nvs file instead of the default? But that still does not solve this problem correctly. Every n900 device have different NVS file. If we allow to load firmware directly from VFS without userspace helper we would see again same problem. -- Pali Rohár pali.ro...@gmail.com signature.asc Description: This is a digitally signed message part.
Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data
* Pavel Machek [170127 11:41]: > On Fri 2017-01-27 17:23:07, Kalle Valo wrote: > > Pali Rohár writes: > > > > > On Friday 27 January 2017 14:26:22 Kalle Valo wrote: > > >> Pali Rohár writes: > > >> > > >> > 2) It was already tested that example NVS data can be used for N900 > > >> > e.g. > > >> > for SSH connection. If real correct data are not available it is better > > >> > to use at least those example (and probably log warning message) so > > >> > user > > >> > can connect via SSH and start investigating where is problem. > > >> > > >> I disagree. Allowing default calibration data to be used can be > > >> unnoticed by user and left her wondering why wifi works so badly. > > > > > > So there are only two options: > > > > > > 1) Disallow it and so these users will have non-working wifi. > > > > > > 2) Allow those data to be used as fallback mechanism. > > > > > > And personally I'm against 1) because it will break wifi support for > > > *all* Nokia N900 devices right now. > > > > All two of them? :) > > Umm. You clearly want a flock of angry penguins at your doorsteps :-). Well this silly issue of symlinking and renaming nvs files in a standard Linux distro was also hitting me on various devices with wl12xx/wl18xx trying to use the same rootfs. Why don't we just set a custom compatible property for n900 that then picks up some other nvs file instead of the default? Regards, Tony
Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data
On Fri, Jan 27, 2017 at 02:11:46PM +0100, Pali Rohár wrote: > So there are only two options: > > 1) Disallow it and so these users will have non-working wifi. > > 2) Allow those data to be used as fallback mechanism. There is one "custom fallback" user in kernel which we recently determined was a total mistake. A sysfs interface should have been defined to enable custom LED settings. Can't a series of sysfs interfaces be used to enable override ? So is that not a third option worth consideration? Luis
Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data
On 27-1-2017 8:33, Kalle Valo wrote: > Pali Rohár 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 >> --- >> 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? Indeed. The only remaining issue would be that an error message is logged. Also note the fallback is only used if selected in Kconfig. > Like we discussed earlier, the default nvs file should not be used by > normal users. Yup. Regards, Arend
Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data
On Fri 2017-01-27 17:23:07, Kalle Valo wrote: > Pali Rohár writes: > > > On Friday 27 January 2017 14:26:22 Kalle Valo wrote: > >> Pali Rohár writes: > >> > >> > 2) It was already tested that example NVS data can be used for N900 e.g. > >> > for SSH connection. If real correct data are not available it is better > >> > to use at least those example (and probably log warning message) so user > >> > can connect via SSH and start investigating where is problem. > >> > >> I disagree. Allowing default calibration data to be used can be > >> unnoticed by user and left her wondering why wifi works so badly. > > > > So there are only two options: > > > > 1) Disallow it and so these users will have non-working wifi. > > > > 2) Allow those data to be used as fallback mechanism. > > > > And personally I'm against 1) because it will break wifi support for > > *all* Nokia N900 devices right now. > > All two of them? :) Umm. You clearly want a flock of angry penguins at your doorsteps :-). > But not working is exactly my point, if correct calibration data is not > available wifi should not work. And it's not only about functionality > problems, there's also the regulatory aspect. If you break existing configuration that's called "regression". > >> > 3) If we do rename *now* we will totally break wifi support on Nokia > >> > N900. > >> > >> Then the distro should fix that when updating the linux-firmware > >> packages. Can you provide details about the setup, what distro etc? > > > > Debian stable, Ubuntu LTSs 14.04, 16.04. > > You can run these out of box on N900? Debian stable does. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data
On Friday 27 January 2017 17:23:07 Kalle Valo wrote: > Pali Rohár writes: > > > On Friday 27 January 2017 14:26:22 Kalle Valo wrote: > >> Pali Rohár writes: > >> > >> > 2) It was already tested that example NVS data can be used for N900 e.g. > >> > for SSH connection. If real correct data are not available it is better > >> > to use at least those example (and probably log warning message) so user > >> > can connect via SSH and start investigating where is problem. > >> > >> I disagree. Allowing default calibration data to be used can be > >> unnoticed by user and left her wondering why wifi works so badly. > > > > So there are only two options: > > > > 1) Disallow it and so these users will have non-working wifi. > > > > 2) Allow those data to be used as fallback mechanism. > > > > And personally I'm against 1) because it will break wifi support for > > *all* Nokia N900 devices right now. > > All two of them? :) Ehm... > But not working is exactly my point, if correct calibration data is not > available wifi should not work. And it's not only about functionality > problems, there's also the regulatory aspect. About functionality, Pavel confirmed too that SSH is somehow working... Regulatory aspect is different, but via iw can be manually configured some settings. > >> > 3) If we do rename *now* we will totally break wifi support on Nokia > >> > N900. > >> > >> Then the distro should fix that when updating the linux-firmware > >> packages. Can you provide details about the setup, what distro etc? > > > > Debian stable, Ubuntu LTSs 14.04, 16.04. > > You can run these out of box on N900? Out-of-box I can run Kubuntu 12.04 (which is LTS too). They had prepared special image for N900 and I still have it on uSD card. I guess that new versions of Ubuntu could somehow work (maybe not out-of-box but with some changes) and Pavel has working Debian. Also basic support needed for wifi and SSH server is probably working with any distribution targeting armv7-a or omap3. So yes, I can say it is out-of-box. We will not have GSM calls or camera support, but wifi breakage is there. > > And I think that other LTS distributions contains that example nvs > > file too (I'm not going to verify others, but list will be probably > > long). Upgrading linux-firmware is against policy of those > > distributions. So no this is not an solution. > > So instead we should workaround distro policies in kernel? Come on. > > Seriously, just rename the file in linux-firmware and file a bug (with a > patch) to distros. If they don't fix the bug you just have to do a > custom hack for N900. But such is life. I do not see point what will be changed. I rename that file and after system update (or integrity check) it will be there again. And if I do that, what prevents kernel to stop using NVS file from /lib/firmware/? Nothing, original problem (which is going solved by this patch series) still remains. -- Pali Rohár pali.ro...@gmail.com
Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data
Pali Rohár writes: > On Friday 27 January 2017 14:26:22 Kalle Valo wrote: >> Pali Rohár writes: >> >> > 2) It was already tested that example NVS data can be used for N900 e.g. >> > for SSH connection. If real correct data are not available it is better >> > to use at least those example (and probably log warning message) so user >> > can connect via SSH and start investigating where is problem. >> >> I disagree. Allowing default calibration data to be used can be >> unnoticed by user and left her wondering why wifi works so badly. > > So there are only two options: > > 1) Disallow it and so these users will have non-working wifi. > > 2) Allow those data to be used as fallback mechanism. > > And personally I'm against 1) because it will break wifi support for > *all* Nokia N900 devices right now. All two of them? :) But not working is exactly my point, if correct calibration data is not available wifi should not work. And it's not only about functionality problems, there's also the regulatory aspect. >> > 3) If we do rename *now* we will totally break wifi support on Nokia >> > N900. >> >> Then the distro should fix that when updating the linux-firmware >> packages. Can you provide details about the setup, what distro etc? > > Debian stable, Ubuntu LTSs 14.04, 16.04. You can run these out of box on N900? > And I think that other LTS distributions contains that example nvs > file too (I'm not going to verify others, but list will be probably > long). Upgrading linux-firmware is against policy of those > distributions. So no this is not an solution. So instead we should workaround distro policies in kernel? Come on. Seriously, just rename the file in linux-firmware and file a bug (with a patch) to distros. If they don't fix the bug you just have to do a custom hack for N900. But such is life. -- Kalle Valo
Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data
On Friday 27 January 2017 13:53:28 Arend Van Spriel wrote: > On 27-1-2017 13:26, Kalle Valo wrote: > > Pali Rohár writes: > > > >> On Friday 27 January 2017 13:49:03 Kalle Valo wrote: > >>> Pali Rohár writes: > >>> > > 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. > > Yes, there will be. But there is no easy way to fix this problem that > kernel is trying to use default/example NVS data... > >>> > >>> Kernel is doing correctly and requesting NVS data as expected, the > >>> problem here is that linux-firmware claims that the example NVS data is > >>> real calibration data (which it is not). Distros should not use that, > >>> only developers for testing purposes. We should not courage users using > >>> example calibration data. > >>> > >>> The simple fix is to rename the NVS file in linux-firmware to something > >>> like wl1251-nvs.bin.example, no need to workaround this in kernel. If > >>> you send a patch to linux-firmware I'm happy to ack that. > >> > >> I agree with rename and fact that default/example data should not be > >> used. > >> > >> But... > >> > >> 1) Kernel should not read device/model specific data from VFS where > >> are stored not-device-specific files preinstalled by linux > >> distributions. > >> > >> And linux distributions are already putting files into VFS and kernel > >> cannot enforce userspace to not do that (as they are already doing it). > > > > I'm having problems to understand what you are saying here. > > This is a personal opinion. I read it as: /lib/firmware can only contain > files for from linux-firmware. > > At least the device-specific vs. non-device-specific does not seem to > hold. The firmware files that we have in the linux-firmware repository > are very device-specific. Unless you mean the 'platform' when talking > about 'device'. Here I'm talking about files which are specific per unit. Every one N900 has different NVS file (stored in CAL) and so every one N900 device needs its own NVS file. And we cannot store thousands of NVS files into linux-firmware repository for each N900 which was ever produced in factory. Firmware files in linux-firmware repository are "device" specific, but "filename" of that file describe exactly for which "device" it is specific. But there are thousands of different NVS files for one filename "wl1251-nvs.bin" and we cannot use one particular for another device. In linux-firmware is stored "wl1251-nvs.bin" file with example data. -- Pali Rohár pali.ro...@gmail.com
Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data
On Friday 27 January 2017 14:26:22 Kalle Valo wrote: > Pali Rohár writes: > > > On Friday 27 January 2017 13:49:03 Kalle Valo wrote: > >> Pali Rohár writes: > >> > >> >> 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. > >> > > >> > Yes, there will be. But there is no easy way to fix this problem that > >> > kernel is trying to use default/example NVS data... > >> > >> Kernel is doing correctly and requesting NVS data as expected, the > >> problem here is that linux-firmware claims that the example NVS data is > >> real calibration data (which it is not). Distros should not use that, > >> only developers for testing purposes. We should not courage users using > >> example calibration data. > >> > >> The simple fix is to rename the NVS file in linux-firmware to something > >> like wl1251-nvs.bin.example, no need to workaround this in kernel. If > >> you send a patch to linux-firmware I'm happy to ack that. > > > > I agree with rename and fact that default/example data should not be > > used. > > > > But... > > > > 1) Kernel should not read device/model specific data from VFS where > > are stored not-device-specific files preinstalled by linux > > distributions. > > > > And linux distributions are already putting files into VFS and kernel > > cannot enforce userspace to not do that (as they are already doing it). > > I'm having problems to understand what you are saying here. I'm saying that linux distributions are putting files to /lib/firmware which comes from some sources already released. You cannot force linux distributions to stop putting particular file to /lib/firmware *now* after it was already released and recommended. > > 2) It was already tested that example NVS data can be used for N900 e.g. > > for SSH connection. If real correct data are not available it is better > > to use at least those example (and probably log warning message) so user > > can connect via SSH and start investigating where is problem. > > I disagree. Allowing default calibration data to be used can be > unnoticed by user and left her wondering why wifi works so badly. So there are only two options: 1) Disallow it and so these users will have non-working wifi. 2) Allow those data to be used as fallback mechanism. And personally I'm against 1) because it will break wifi support for *all* Nokia N900 devices right now. > > 3) If we do rename *now* we will totally break wifi support on Nokia > > N900. > > Then the distro should fix that when updating the linux-firmware > packages. Can you provide details about the setup, what distro etc? Debian stable, Ubuntu LTSs 14.04, 16.04. And I think that other LTS distributions contains that example nvs file too (I'm not going to verify others, but list will be probably long). Upgrading linux-firmware is against policy of those distributions. So no this is not an solution. -- Pali Rohár pali.ro...@gmail.com
Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data
On 27-1-2017 13:26, Kalle Valo wrote: > Pali Rohár writes: > >> On Friday 27 January 2017 13:49:03 Kalle Valo wrote: >>> Pali Rohár writes: >>> > 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. Yes, there will be. But there is no easy way to fix this problem that kernel is trying to use default/example NVS data... >>> >>> Kernel is doing correctly and requesting NVS data as expected, the >>> problem here is that linux-firmware claims that the example NVS data is >>> real calibration data (which it is not). Distros should not use that, >>> only developers for testing purposes. We should not courage users using >>> example calibration data. >>> >>> The simple fix is to rename the NVS file in linux-firmware to something >>> like wl1251-nvs.bin.example, no need to workaround this in kernel. If >>> you send a patch to linux-firmware I'm happy to ack that. >> >> I agree with rename and fact that default/example data should not be >> used. >> >> But... >> >> 1) Kernel should not read device/model specific data from VFS where >> are stored not-device-specific files preinstalled by linux >> distributions. >> >> And linux distributions are already putting files into VFS and kernel >> cannot enforce userspace to not do that (as they are already doing it). > > I'm having problems to understand what you are saying here. This is a personal opinion. I read it as: /lib/firmware can only contain files for from linux-firmware. At least the device-specific vs. non-device-specific does not seem to hold. The firmware files that we have in the linux-firmware repository are very device-specific. Unless you mean the 'platform' when talking about 'device'. Regards, Arend
Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data
Pali Rohár writes: > On Friday 27 January 2017 13:49:03 Kalle Valo wrote: >> Pali Rohár writes: >> >> >> 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. >> > >> > Yes, there will be. But there is no easy way to fix this problem that >> > kernel is trying to use default/example NVS data... >> >> Kernel is doing correctly and requesting NVS data as expected, the >> problem here is that linux-firmware claims that the example NVS data is >> real calibration data (which it is not). Distros should not use that, >> only developers for testing purposes. We should not courage users using >> example calibration data. >> >> The simple fix is to rename the NVS file in linux-firmware to something >> like wl1251-nvs.bin.example, no need to workaround this in kernel. If >> you send a patch to linux-firmware I'm happy to ack that. > > I agree with rename and fact that default/example data should not be > used. > > But... > > 1) Kernel should not read device/model specific data from VFS where > are stored not-device-specific files preinstalled by linux > distributions. > > And linux distributions are already putting files into VFS and kernel > cannot enforce userspace to not do that (as they are already doing it). I'm having problems to understand what you are saying here. > 2) It was already tested that example NVS data can be used for N900 e.g. > for SSH connection. If real correct data are not available it is better > to use at least those example (and probably log warning message) so user > can connect via SSH and start investigating where is problem. I disagree. Allowing default calibration data to be used can be unnoticed by user and left her wondering why wifi works so badly. > 3) If we do rename *now* we will totally break wifi support on Nokia > N900. Then the distro should fix that when updating the linux-firmware packages. Can you provide details about the setup, what distro etc? -- Kalle Valo
Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data
Hi! > > 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. Yes, wifi somehow works on N900. .. depending on userspace and kernel versions. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data
On Friday 27 January 2017 13:49:03 Kalle Valo wrote: > Pali Rohár writes: > > >> 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. > > > > Yes, there will be. But there is no easy way to fix this problem that > > kernel is trying to use default/example NVS data... > > Kernel is doing correctly and requesting NVS data as expected, the > problem here is that linux-firmware claims that the example NVS data is > real calibration data (which it is not). Distros should not use that, > only developers for testing purposes. We should not courage users using > example calibration data. > > The simple fix is to rename the NVS file in linux-firmware to something > like wl1251-nvs.bin.example, no need to workaround this in kernel. If > you send a patch to linux-firmware I'm happy to ack that. I agree with rename and fact that default/example data should not be used. But... 1) Kernel should not read device/model specific data from VFS where are stored not-device-specific files preinstalled by linux distributions. And linux distributions are already putting files into VFS and kernel cannot enforce userspace to not do that (as they are already doing it). 2) It was already tested that example NVS data can be used for N900 e.g. for SSH connection. If real correct data are not available it is better to use at least those example (and probably log warning message) so user can connect via SSH and start investigating where is problem. 3) If we do rename *now* we will totally break wifi support on Nokia N900. -- Pali Rohár pali.ro...@gmail.com
Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data
Pali Rohár writes: >> 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. > > Yes, there will be. But there is no easy way to fix this problem that > kernel is trying to use default/example NVS data... Kernel is doing correctly and requesting NVS data as expected, the problem here is that linux-firmware claims that the example NVS data is real calibration data (which it is not). Distros should not use that, only developers for testing purposes. We should not courage users using example calibration data. The simple fix is to rename the NVS file in linux-firmware to something like wl1251-nvs.bin.example, no need to workaround this in kernel. If you send a patch to linux-firmware I'm happy to ack that. -- Kalle Valo
Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data
On Friday 27 January 2017 11:19:25 Arend Van Spriel wrote: > 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 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 > > --- > > 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? No. Some wl1251 chips have internal EEPROM where is st
Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data
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 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 > --- > 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
Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data
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 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 > >>> --- > >>> 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. -- Pali Rohár pali.ro...@gmail.com
Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data
On 27-1-2017 10:43, Pali Rohár wrote: > On Friday 27 January 2017 09:33:40 Kalle Valo wrote: >> Pali Rohár 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 >>> --- >>> 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. >> 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? Are there other platforms that use the default nvs file and have a working wifi. So your "removing is not possible" would be about regression for those? Regards, Arend
Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data
On Friday 27 January 2017 09:33:40 Kalle Valo wrote: > Pali Rohár 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 > > --- > > 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... > 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. -- Pali Rohár pali.ro...@gmail.com
Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data
Pali Rohár 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 > --- > 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? Like we discussed earlier, the default nvs file should not be used by normal users. -- Kalle Valo
Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data
On Mon, Dec 26, 2016 at 05:35:59PM +0100, Pavel Machek wrote: > > Right question is "should we solve it without user-space help"? > > Answer is no, too. Way too complex. Yes, it would be nice if hardware > was designed in such a way that getting calibration data from kernel > is easy, and if you design hardware, please design it like that. But > N900 is not designed like that and getting the calibration through > userspace looks like only reasonable solution. Arend seems to have a better alternative in mind possible for other devices which *can* probably pull of doing this easily and nicely, given the nasty history of the usermode helper crap we should not in any way discourage such efforts. Arend -- please look at the firmware cache, it not a hash but a hash table for an O(1) lookups would be a welcomed change, then it could be repurposed for what you describe, I think the only difference is you'd perhaps want a custom driver hook to fetch the calibration data so the driver does whatever it needs. > Now... how exactly to do that is other question. (But this is looks > very reasonable. Maybe I'd add request_firmware_with_flags(, ...int > flags), but.. that's a tiny detail.). But userspace needs to be > involved. No, no, we keep adding yet-another-exported symbol for requesting firmware, instead of just adding a set of parameters possible and easily extending functionality. Please review the patches posted on my last set which adds a flexible API with only 2 calls, sync and async, and lets us customize our requests using a parameter: https://git.kernel.org/cgit/linux/kernel/git/mcgrof/linux-next.git/log/?h=20161216-drvdata-v3-try3 This also documents the "usermode helper" properly and explains some of the issues and limitations you will need to consider if you use it, its one reason I'd highly encourage to consider an alternative as what Arend is considering. *Iff* you insist on using the (now using the proper term, as per the documentation update I am providing) "custom fallback mechanism" I welcome such a change but I ask we *really* think this through well so we avoid the stupid issues which have historically made the custom fallback mechanism more of a nuisance for Linux distributions, users and developers. To this end -- I ask you check out Daniel Wagner and Tom Gundersen's firmwared work [0] which I referred you to in December. Although the drvdata API does not yet use a custom fallback mechanism, after and its merged the goal here would be to *only* support a clean custom fallback mechanism which aligns itself *well* with firmwared or solutions like it. Your patch set then could just become a patch set to add the custom fallback mechaism support to drvdata API with the new options/prefernce you are looking for to be specified in the new parameters, not a new exported symbol. One of the cruxes we should consider addressing before the drvdata API gets a custom fallback mechanism support added is that the usermode helper lock should be replaced with a generic solution for the races it was intended to address: use of the API on suspend/resume and implicitly later avoid a race on init. To this end we should consider the same race for *other* real kernel "user mode helpers", I've documented this on a wiki [1] which documents the *real* kernel usermode helpers users, one of which was the kobject uevent which is one of the fallback mechanisms. I should also note that the idea of fallback mechanism using kobject uevents should really suffice, in review with Johannes Berg at least, he seemed convinced just letting either the upstream firmwared, a custom firmwared or a custom userspace solution should be able to just monitor for uevents for drvdata and do the right thing, this whole "custom fallback mechanism" in retrospect seems not really needed as far as I can tell. [0] https://github.com/teg/firmwared [1] https://kernelnewbies.org/KernelProjects/usermode-helper-enhancements Luis
Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data
On Sun 2016-12-25 21:15:40, Arend Van Spriel wrote: > On 24-12-2016 17:52, Pali Rohár wrote: > > 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. > > Responding to this patch as it provides a lot of context to discuss. As > you might have gathered from earlier discussions I am not a fan of using > user-space helper. I can agree that the kernel driver, wl1251 in this > case, should be agnostic to platform specific details regarding storage > solutions and the firmware api should hide that. However, it seems your > only solution is adding user-space to the mix and changing the api > towards that. Can we solve it without user-space help? Answer is no, due to licensing. But that's wrong question to ask. Right question is "should we solve it without user-space help"? Answer is no, too. Way too complex. Yes, it would be nice if hardware was designed in such a way that getting calibration data from kernel is easy, and if you design hardware, please design it like that. But N900 is not designed like that and getting the calibration through userspace looks like only reasonable solution. Now... how exactly to do that is other question. (But this is looks very reasonable. Maybe I'd add request_firmware_with_flags(, ...int flags), but.. that's a tiny detail.). But userspace needs to be involved. Thanks, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data
On Monday 26 December 2016 16:43:53 Pavel Machek wrote: > Hi! > > > > > 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. > > > > > > Responding to this patch as it provides a lot of context to > > > discuss. As you might have gathered from earlier discussions I > > > am not a fan of using user-space helper. I can agree that the > > > kernel driver, wl1251 in this case, should be agnostic to > > > platform specific details regarding storage solutions and the > > > firmware api should hide that. However, it seems your only > > > solution is adding user-space to the mix and changing the api > > > towards that. Can we solve it without user-space help? > > > > Without userspace helper it means that userspace helper code must > > be integrated into kernel. > > > > So what is userspace helper doing? > > > > 1) Read MAC address from CAL > > 2) Read NVS data from CAL > > 3) Modify MAC address in memory NVS data (new for this patch > > series) 4) Modify in memory NVS data if we in FCC country > > > > Checking for country is done via dbus call to either Maemo cellular > > daemon or alternatively via REGDOMAIN in /etc/default/crda. I have > > plan to use ofono (instead Maemo cellular daemon) too... > > > > Currently we are using closed Nokia proprietary CAL library. > > > > Steps 1) and 2) needs closed library, step 4) needs dbus call. > > I guess pointer to the source code implementing this would be > welcome. Here is current code: https://github.com/community-ssu/wl1251-cal (there is implemented also Maemo netlink interface) > > > But on other devices that use wl1251, but for instance have no > > > userspace helper the request to userspace will fail (after 60 > > > sec?) and try VFS after that. Maybe not so nice. > > > > Currently support for those devices is broken (like for N900) as > > without proper NVS data they do not work correctly... > > Is it expected to work at all, perhaps with degraded performance / > range? Because it seems to work for me. Yes, some degraded performance or problems with connecting is expected. And random MAC address at every boot. Plus some regulatory problems in FCC countries. -- Pali Rohár pali.ro...@gmail.com signature.asc Description: This is a digitally signed message part.
Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data
Hi! > > > 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. > > > > Responding to this patch as it provides a lot of context to discuss. > > As you might have gathered from earlier discussions I am not a fan > > of using user-space helper. I can agree that the kernel driver, > > wl1251 in this case, should be agnostic to platform specific details > > regarding storage solutions and the firmware api should hide that. > > However, it seems your only solution is adding user-space to the mix > > and changing the api towards that. Can we solve it without > > user-space help? > > Without userspace helper it means that userspace helper code must be > integrated into kernel. > > So what is userspace helper doing? > > 1) Read MAC address from CAL > 2) Read NVS data from CAL > 3) Modify MAC address in memory NVS data (new for this patch series) > 4) Modify in memory NVS data if we in FCC country > > Checking for country is done via dbus call to either Maemo cellular > daemon or alternatively via REGDOMAIN in /etc/default/crda. I have plan > to use ofono (instead Maemo cellular daemon) too... > > Currently we are using closed Nokia proprietary CAL library. > > Steps 1) and 2) needs closed library, step 4) needs dbus call. I guess pointer to the source code implementing this would be welcome. > > But on other devices that use wl1251, but for instance have no > > userspace helper the request to userspace will fail (after 60 sec?) > > and try VFS after that. Maybe not so nice. > > Currently support for those devices is broken (like for N900) as without > proper NVS data they do not work correctly... Is it expected to work at all, perhaps with degraded performance / range? Because it seems to work for me. Thanks, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data
On Sunday 25 December 2016 21:15:40 Arend Van Spriel wrote: > On 24-12-2016 17:52, Pali Rohár wrote: > > 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. > > Responding to this patch as it provides a lot of context to discuss. > As you might have gathered from earlier discussions I am not a fan > of using user-space helper. I can agree that the kernel driver, > wl1251 in this case, should be agnostic to platform specific details > regarding storage solutions and the firmware api should hide that. > However, it seems your only solution is adding user-space to the mix > and changing the api towards that. Can we solve it without > user-space help? Without userspace helper it means that userspace helper code must be integrated into kernel. So what is userspace helper doing? 1) Read MAC address from CAL 2) Read NVS data from CAL 3) Modify MAC address in memory NVS data (new for this patch series) 4) Modify in memory NVS data if we in FCC country Checking for country is done via dbus call to either Maemo cellular daemon or alternatively via REGDOMAIN in /etc/default/crda. I have plan to use ofono (instead Maemo cellular daemon) too... Currently we are using closed Nokia proprietary CAL library. Steps 1) and 2) needs closed library, step 4) needs dbus call. In current state I do not see way to integrate it into kernel. And because wl1251 currently uses request_firmware() to load those nvs data I think it is still the best way how to handle it... And IIRC there was already discussion about Nokia CAL parser in kernel and it was declined. > The firmware_class already supports a number of path prefixes it > traverses looking for the requested firmware. So I was thinking about > adding a hashtable in which a platform driver can add firmware which > are stored in the hashtable using the hashed firmware name. Upon a > firmware request from the driver we could check the hashtable before > traversing the path prefixes on VFS. The obvious problem is that the > request may come before the firmware is added to the hashtable. Just > wanted to pitch the idea first and hear what others think about it > and maybe someone has a nice solution for this problem. Fingers > crossed :-p > > > 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 the firmware hashtable api on N900 a platform driver could > interpret the CAL data in the nand partition and provide it through > the firmware_class. > > > With this patch it is finally possible to load correct model > > specific NVS calibration data for Nokia N900. > > But on other devices that use wl1251, but for instance have no > userspace helper the request to userspace will fail (after 60 sec?) > and try VFS after that. Maybe not so nice. Currently support for those devices is broken (like for N900) as without proper NVS data they do not work correctly... > You should consider other device configurations. Not just N900. I do not have any other wl1251 devices. I know that pandora has wl1251 too, but it has wl1251 with eeprom where is stored NVS. And in this case request_firmware() is not used there. > Regards, > Arend > > > Signed-off-by: Pali Rohár > > --- > > > > 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/ma
Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data
On 24-12-2016 17:52, Pali Rohár wrote:
> 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.
Responding to this patch as it provides a lot of context to discuss. As
you might have gathered from earlier discussions I am not a fan of using
user-space helper. I can agree that the kernel driver, wl1251 in this
case, should be agnostic to platform specific details regarding storage
solutions and the firmware api should hide that. However, it seems your
only solution is adding user-space to the mix and changing the api
towards that. Can we solve it without user-space help?
The firmware_class already supports a number of path prefixes it
traverses looking for the requested firmware. So I was thinking about
adding a hashtable in which a platform driver can add firmware which are
stored in the hashtable using the hashed firmware name. Upon a firmware
request from the driver we could check the hashtable before traversing
the path prefixes on VFS. The obvious problem is that the request may
come before the firmware is added to the hashtable. Just wanted to pitch
the idea first and hear what others think about it and maybe someone has
a nice solution for this problem. Fingers crossed :-p
> 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 the firmware hashtable api on N900 a platform driver could
interpret the CAL data in the nand partition and provide it through the
firmware_class.
> With this patch it is finally possible to load correct model specific NVS
> calibration data for Nokia N900.
But on other devices that use wl1251, but for instance have no userspace
helper the request to userspace will fail (after 60 sec?) and try VFS
after that. Maybe not so nice. You should consider other device
configurations. Not just N900.
Regards,
Arend
> Signed-off-by: Pali Rohár
> ---
> 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);
>
> if (ret < 0) {
> wl1251_error("could not get nvs file: %d", ret);
>
