linux-kernel - Re: [RFC v0 7/8] Input: ims-pcu: use firmware

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160801194408.GZ3296@wotan.suse.de>
Date:	Mon, 1 Aug 2016 21:44:08 +0200
From:	"Luis R. Rodriguez" <mcgrof@...nel.org>
To:	Daniel Wagner <daniel.wagner@...-carit.de>
Cc:	Dmitry Torokhov <dmitry.torokhov@...il.com>,
	"Luis R. Rodriguez" <mcgrof@...nel.org>,
	Arend van Spriel <arend.vanspriel@...adcom.com>,
	Bjorn Andersson <bjorn.andersson@...aro.org>,
	Daniel Wagner <wagi@...om.org>,
	Bastien Nocera <hadess@...ess.net>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Johannes Berg <johannes.berg@...el.com>,
	Kalle Valo <kvalo@...eaurora.org>,
	Ohad Ben-Cohen <ohad@...ery.com>,
	Mimi Zohar <zohar@...ux.vnet.ibm.com>,
	David Howells <dhowells@...hat.com>,
	Andy Lutomirski <luto@...capital.net>,
	David Woodhouse <dwmw2@...radead.org>,
	Julia Lawall <julia.lawall@...6.fr>,
	linux-input@...r.kernel.org, linux-kselftest@...r.kernel.org,
	linux-wireless@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC v0 7/8] Input: ims-pcu: use firmware_stat instead of
 completion

On Mon, Aug 01, 2016 at 02:26:04PM +0200, Daniel Wagner wrote:
> On 07/31/2016 09:23 AM, Dmitry Torokhov wrote:
> >On July 30, 2016 9:58:17 AM PDT, "Luis R. Rodriguez" <mcgrof@...nel.org> wrote:
> >>On Sat, Jul 30, 2016 at 02:42:41PM +0200, Arend van Spriel wrote:
> >>>On 29-07-16 08:13, Daniel Wagner wrote:
> >>>>On 07/28/2016 09:01 PM, Bjorn Andersson wrote:
> >>>>>On Thu 28 Jul 11:33 PDT 2016, Dmitry Torokhov wrote:
> 
> >>>+ Luis (again) ;-)
> 
> That was not on purpose :) My attempt to keep the Cc list a bit
> shorter was a failure.
> 
> >>>>>>Do not quite like it... I'd rather asynchronous request give out
> >>>>>>firmware status pointer that could be used later on.
> >>>
> >>>Excellent. Why not get rid of the callback function as well and have
> >>>fw_loading_wait() return result (0 = firmware available, < 0 = fail).
> >>>Just to confirm, you are proposing a new API function next to
> >>>request_firmware_nowait(), right?
> >>
> >>If proposing new firmware_class patches please bounce / Cc me, I've
> >>recently asked for me to be added to MAINTAINERS so I get these
> >>e-mails as I'm working on a new flexible API which would allow us
> >>to extend the firmware API without having to care about the old
> >>stupid usermode helper at all.
> 
> These patches here are a first attempt to clean up a bit of the code
> around the completion API. As Dmitry correctly pointed out, it makes
> more sense to go bit further and make the async loading a bit more
> convenient for the drivers.
> 
> >I am not sure why we started calling usermode helper "stupid". We
> >only  had to implement direct kernel firmware loading because udev/stsremd
> >folks had "interesting" ideas how events should be handled; but having
> >userspace to feed us data is not stupid.
> 
> I was ignorant on all the nasty details around the firmware loading.
> If I parse Luis' patches correctly they introduce an API which calls
> kernel_read_file_from_path() asynchronously:
> 
> sysdata_file_request_async(..., &cookie)
>   *coookie = async_schedule_domain(request_sysdata_file_work_func(), ..)
> 
>   request_sysdata_file_work_fun()
>     _sysdata_file_request()
>       fw_get_filesystem_firmware()
> 	kernel_read_file_from_path()
> 
> sysdata_synchronize_request(&cookie);
> 
> Doesn't look like what your asking for.

No, but its also a generic kernel read issue as I noted in my last
reply.

> >If we want to overhaul firmware loading support we need to figure
> >out  how to support case when a driver want to [asynchronously] request
> >firmware/config/blob and the rest of the system is not ready. Even if we
> >want kernel to do read/load the data we need userspace to tell kernel
> >when firmware partition is available, until then the kernel should not
> >fail the request.
> 
> I gather from Luis' blog post and comments that he is on the quest
> on removing userspace support completely.

No, I explained in my last proposed documentation patch series that we cannot
get rid of the usermode helper. Its not well understood why so I explained and
documented why. Best we can do is compartamentalize its uses.

The sysdata API's main goal rather is to provide a flexible API first,
compartamentalizing the usermode helper was secondary. But now it seems
I may just also add devm support too to help simplify code further.

What Dmitry notes is an existential issue with kernel_read_file_from_path()
and we need a common solution for it.

> Maybe this attempt here could be a step before. Step 1 would be
> changing request_firmware_nowait() to request_firmware_async so
> drivers don't have to come up with their own sync primitives, e.g.
> 
>   cookie = request_firmware_async()
>   fw_load_wait(cookie)

That's one of the features already part of async mechanism of the sysdata API :)

  Luis