From: "Luis R. Rodriguez" <mcgrof@kernel.org> To: Pavel Machek <pavel@ucw.cz>, Daniel Wagner <wagi@monom.org>, Tom Gundersen <teg@jklm.no> Cc: "Arend Van Spriel" <arend.vanspriel@broadcom.com>, "Pali Rohár" <pali.rohar@gmail.com>, "Ming Lei" <ming.lei@canonical.com>, "Luis R. Rodriguez" <mcgrof@kernel.org>, "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>, "Kalle Valo" <kvalo@codeaurora.org>, "David Gnedt" <david.gnedt@davizone.at>, "Michal Kazior" <michal.kazior@tieto.com>, "Daniel Wagner" <wagi@monom.org>, "Tony Lindgren" <tony@atomide.com>, "Sebastian Reichel" <sre@kernel.org>, "Ivaylo Dimitrov" <ivo.g.dimitrov.75@gmail.com>, "Aaro Koskinen" <aaro.koskinen@iki.fi>, "Takashi Iwai" <tiwai@suse.de>, "David Woodhouse" <dwmw2@infradead.org>, "Bjorn Andersson" <bjorn.andersson@linaro.org>, "Grazvydas Ignotas" <notasas@gmail.com>, linux-kernel@vger.kernel.org, linux-wireless@vger.kernel.org, netdev@vger.kernel.org Subject: Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data Date: Tue, 3 Jan 2017 18:59:24 +0100 [thread overview] Message-ID: <20170103175924.GC13946@wotan.suse.de> (raw) In-Reply-To: <20161226163559.GB27087@amd> 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
WARNING: multiple messages have this Message-ID (diff)
From: "Luis R. Rodriguez" <mcgrof@kernel.org> To: Pavel Machek <pavel@ucw.cz>, Daniel Wagner <wagi@monom.org>, Tom Gundersen <teg@jklm.no> Cc: "Arend Van Spriel" <arend.vanspriel@broadcom.com>, "Pali Rohár" <pali.rohar@gmail.com>, "Ming Lei" <ming.lei@canonical.com>, "Luis R. Rodriguez" <mcgrof@kernel.org>, "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>, "Kalle Valo" <kvalo@codeaurora.org>, "David Gnedt" <david.gnedt@davizone.at>, "Michal Kazior" <michal.kazior@tieto.com>, "Daniel Wagner" <wagi@monom.org>, "Tony Lindgren" <tony@atomide.com>, "Sebastian Reichel" <sre@kernel.org>, "Ivaylo Dimitrov" <ivo.g.dimitrov.75@gmail.com>, "Aaro Koskinen" <aaro.koskinen@iki.fi>, "Takashi Iwai" <tiwai@suse.de>, "David Woodhouse" <dwmw2@infradead.org>, "Bjorn Andersson" <bjorn.andersson@linaro.org>, "Grazvydas Ignotas" <notasas@gmail.com>, linux-kernel@vger.kernel.org, linux-wireless@vger.kernel.org Subject: Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data Date: Tue, 3 Jan 2017 18:59:24 +0100 [thread overview] Message-ID: <20170103175924.GC13946@wotan.suse.de> (raw) In-Reply-To: <20161226163559.GB27087@amd> 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
next prev parent reply other threads:[~2017-01-03 17:59 UTC|newest] Thread overview: 111+ messages / expand[flat|nested] mbox.gz Atom feed top 2016-12-24 16:52 [PATCH 0/6] wl1251: Fix MAC address for Nokia N900 Pali Rohár 2016-12-24 16:52 ` Pali Rohár 2016-12-24 16:52 ` [PATCH 1/6] firmware: Add request_firmware_prefer_user() function Pali Rohár 2016-12-24 16:52 ` Pali Rohár 2016-12-24 16:52 ` [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data Pali Rohár 2016-12-24 16:52 ` Pali Rohár 2016-12-25 20:15 ` Arend Van Spriel 2016-12-25 20:46 ` Pali Rohár 2016-12-26 15:43 ` Pavel Machek 2016-12-26 16:04 ` Pali Rohár 2016-12-26 16:35 ` Pavel Machek 2017-01-03 17:59 ` Luis R. Rodriguez [this message] 2017-01-03 17:59 ` Luis R. Rodriguez 2017-05-03 19:02 ` Arend Van Spriel 2017-05-04 2:28 ` Luis R. Rodriguez 2017-05-04 2:28 ` Luis R. Rodriguez 2017-05-12 20:55 ` Arend Van Spriel 2017-01-27 7:33 ` Kalle Valo 2017-01-27 7:33 ` Kalle Valo 2017-01-27 8:58 ` Arend Van Spriel 2017-01-27 9:43 ` Pali Rohár 2017-01-27 10:05 ` Arend Van Spriel 2017-01-27 10:10 ` Pali Rohár 2017-01-27 10:19 ` Arend Van Spriel 2017-01-27 10:34 ` Pali Rohár 2017-01-27 11:49 ` Kalle Valo 2017-01-27 11:49 ` Kalle Valo 2017-01-27 11:57 ` Pali Rohár 2017-01-27 12:26 ` Kalle Valo 2017-01-27 12:26 ` Kalle Valo 2017-01-27 12:26 ` Kalle Valo 2017-01-27 12:53 ` Arend Van Spriel 2017-01-27 13:16 ` Pali Rohár 2017-01-27 13:11 ` Pali Rohár 2017-01-27 15:23 ` Kalle Valo 2017-01-27 15:23 ` Kalle Valo 2017-01-27 15:41 ` Pali Rohár 2017-01-27 19:40 ` Pavel Machek 2017-01-30 17:53 ` Tony Lindgren 2017-01-30 18:03 ` Pali Rohár 2017-01-31 6:35 ` Kalle Valo 2017-01-31 6:35 ` Kalle Valo 2017-01-31 6:35 ` Kalle Valo 2017-01-31 15:59 ` Tony Lindgren 2017-02-01 8:33 ` Pali Rohár 2017-02-01 9:35 ` Michal Kazior 2017-02-01 9:35 ` Michal Kazior 2017-01-29 17:10 ` Luis R. Rodriguez 2017-01-29 17:10 ` Luis R. Rodriguez 2017-01-27 12:03 ` Pavel Machek 2016-12-24 16:52 ` [PATCH 3/6] wl1251: Update wl->nvs_len after wl->nvs is valid Pali Rohár 2016-12-24 16:52 ` Pali Rohár 2016-12-24 18:02 ` Pavel Machek 2016-12-24 16:52 ` [PATCH 4/6] wl1251: Generate random MAC address only if driver does not have valid Pali Rohár 2016-12-24 18:08 ` Pavel Machek 2016-12-24 18:38 ` Pali Rohár 2016-12-24 16:53 ` [PATCH 5/6] wl1251: Parse and use MAC address from supplied NVS data Pali Rohár 2016-12-24 16:53 ` Pali Rohár 2016-12-24 18:14 ` Pavel Machek 2016-12-24 18:40 ` Pali Rohár 2017-01-27 7:54 ` Kalle Valo 2017-01-27 7:54 ` Kalle Valo 2016-12-24 16:53 ` [PATCH 6/6] wl1251: Set generated MAC address back to " Pali Rohár 2016-12-24 18:17 ` Pavel Machek 2016-12-24 18:44 ` Pali Rohár 2017-01-27 7:56 ` Kalle Valo 2017-01-27 7:56 ` Kalle Valo 2017-01-27 9:05 ` Pali Rohár 2017-01-27 9:30 ` Kalle Valo 2017-01-27 9:30 ` Kalle Valo 2017-11-09 23:38 ` [PATCH v2 0/6] wl1251: Fix MAC address for Nokia N900 Pali Rohár 2017-11-09 23:38 ` [PATCH v2 1/6] wl1251: Update wl->nvs_len after wl->nvs is valid Pali Rohár 2018-02-27 13:51 ` [v2,1/6] " Kalle Valo 2018-02-27 13:51 ` Kalle Valo 2017-11-09 23:38 ` [PATCH v2 2/6] wl1251: Generate random MAC address only if driver does not have valid Pali Rohár 2017-11-09 23:38 ` [PATCH v2 3/6] wl1251: Parse and use MAC address from supplied NVS data Pali Rohár 2017-11-09 23:38 ` [PATCH v2 4/6] wl1251: Set generated MAC address back to " Pali Rohár 2017-11-09 23:38 ` [PATCH v2 5/6] firmware: Add request_firmware_prefer_user() function Pali Rohár 2017-11-10 20:26 ` Luis R. Rodriguez 2017-11-10 20:28 ` Luis R. Rodriguez 2017-11-10 21:08 ` Pali Rohár 2017-11-10 23:35 ` Luis R. Rodriguez 2017-11-09 23:38 ` [PATCH v2 6/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data Pali Rohár 2018-01-02 19:23 ` [PATCH v2 0/6] wl1251: Fix MAC address for Nokia N900 Pali Rohár 2018-01-05 1:45 ` Luis R. Rodriguez 2018-01-27 14:14 ` Pali Rohár 2018-02-19 8:27 ` Kalle Valo 2018-02-19 8:27 ` Kalle Valo 2018-02-19 8:27 ` Kalle Valo [not found] <0fd90416-f33c-a6be-14fd-5e964583e9cb@broadcom.com> 2017-05-15 23:13 ` [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data Luis R. Rodriguez 2017-05-15 23:13 ` Luis R. Rodriguez 2017-05-16 8:41 ` Arend Van Spriel 2017-05-16 23:57 ` Luis R. Rodriguez 2017-05-16 23:57 ` Luis R. Rodriguez 2017-06-23 21:53 ` Luis R. Rodriguez 2017-06-23 21:53 ` Luis R. Rodriguez 2017-06-30 21:35 ` Arend van Spriel 2017-06-30 21:35 ` Arend van Spriel 2017-08-10 19:43 ` Luis R. Rodriguez 2017-08-10 19:43 ` Luis R. Rodriguez 2017-05-17 12:06 ` Johannes Berg 2017-05-17 12:53 ` Pali Rohár 2017-05-17 12:53 ` Pali Rohár 2017-05-17 13:04 ` Johannes Berg 2017-05-17 13:04 ` Johannes Berg 2017-05-17 13:21 ` Pali Rohár 2017-05-17 13:21 ` Pali Rohár 2017-05-17 13:22 ` Johannes Berg 2017-05-17 13:22 ` Johannes Berg 2017-05-18 0:08 ` Bjorn Andersson 2017-05-18 0:08 ` Bjorn Andersson
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20170103175924.GC13946@wotan.suse.de \ --to=mcgrof@kernel.org \ --cc=aaro.koskinen@iki.fi \ --cc=arend.vanspriel@broadcom.com \ --cc=bjorn.andersson@linaro.org \ --cc=david.gnedt@davizone.at \ --cc=dwmw2@infradead.org \ --cc=gregkh@linuxfoundation.org \ --cc=ivo.g.dimitrov.75@gmail.com \ --cc=kvalo@codeaurora.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-wireless@vger.kernel.org \ --cc=michal.kazior@tieto.com \ --cc=ming.lei@canonical.com \ --cc=netdev@vger.kernel.org \ --cc=notasas@gmail.com \ --cc=pali.rohar@gmail.com \ --cc=pavel@ucw.cz \ --cc=sre@kernel.org \ --cc=teg@jklm.no \ --cc=tiwai@suse.de \ --cc=tony@atomide.com \ --cc=wagi@monom.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.