LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Hans de Goede <hdegoede@redhat.com>
To: "Luis R. Rodriguez" <mcgrof@kernel.org>,
	Andres Rodriguez <andresx7@gmail.com>,
	Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Darren Hart <dvhart@infradead.org>,
	Andy Shevchenko <andy@infradead.org>,
	Ard Biesheuvel <ard.biesheuvel@linaro.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>, "H . Peter Anvin" <hpa@zytor.com>,
	platform-driver-x86@vger.kernel.org,
	linux-kernel@vger.kernel.org, Peter Jones <pjones@redhat.com>,
	Dave Olsthoorn <dave@bewaar.me>,
	Will Deacon <will.deacon@arm.com>,
	Andy Lutomirski <luto@kernel.org>,
	Matt Fleming <matt@codeblueprint.co.uk>,
	David Howells <dhowells@redhat.com>,
	Mimi Zohar <zohar@linux.vnet.ibm.com>,
	Josh Triplett <josh@joshtriplett.org>,
	dmitry.torokhov@gmail.com, mfuzzey@parkeon.com,
	keescook@chromium.org, Kalle Valo <kvalo@codeaurora.org>,
	Arend Van Spriel <arend.vanspriel@broadcom.com>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	nbroeking@me.com, bjorn.andersson@linaro.org,
	Torsten Duwe <duwe@suse.de>,
	x86@kernel.org, linux-efi@vger.kernel.org
Subject: Re: [PATCH v3 2/5] efi: Add embedded peripheral firmware support
Date: Tue, 17 Apr 2018 10:58:40 +0200	[thread overview]
Message-ID: <aff2d267-e06d-f422-5623-6f6cc25af155@redhat.com> (raw)
In-Reply-To: <20180417001738.GN30543@wotan.suse.de>

Hi,

On 17-04-18 02:17, Luis R. Rodriguez wrote:
> On Sun, Apr 08, 2018 at 07:40:11PM +0200, Hans de Goede wrote:
>>   static void firmware_free_data(const struct firmware *fw)
>>   {
>> @@ -576,6 +600,15 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
>>   		goto out;
>>   
>>   	ret = fw_get_filesystem_firmware(device, fw->priv);
>> +#ifdef CONFIG_EFI_EMBEDDED_FIRMWARE
>> +	if (ret && device &&
>> +	    device_property_read_bool(device, "efi-embedded-firmware")) {
>> +		ret = fw_get_efi_embedded_fw(device, fw->priv, ret);
>> +		if (ret == 0)
>> +			ret = assign_fw(fw, device, opt_flags | FW_OPT_NOCACHE);
>> +		goto out;
>> +	}
>> +#endif
> 
> You mussed what I asked for in terms of adding a new flag, (please work on top
> of Andre's patches as those likely will be merged first, and also have kdocs
> for the flags)

Ok I will base my next version on top of Andres' series.

> and then a new firmware API to wrap the above into a function
> which would only do something if the driver *asked* for it on their firmware
> API call.
> Ie, please add a new firmware_request_efi_fw().

As I tried to explain in the changelog the problem with doing this, is that
this makes it a driver decision, where it really needs to be platform-code driven,
not driver driven.

Take for example the drivers/input/touchscreen/silead.c code that is used on
a lot of 32 bit ARM platforms too, which don't have EFI at all, so if that
needs to call request_firmware_efi() then should I add:

#ifdef CONFIG_X86
	fw = request_firmware_efi(...);
#else
	fw = request_firmware(...);
#endif

? But even on x86 only some devices with a silead touchscreen have EFI
embedded firmware, so then I would need something like:

#ifdef CONFIG_X86
	if (device_property_get_bool(dev, "some-prop-name"))
		fw = request_firmware_efi(...);
	else
#else
		fw = request_firmware(...);
#endif

That is assuming I still want the normal fallback path in the
case no EFI firmware is available, which I do because then
something like packagekit may see if the firmware is packaged
in one of the configured distro repositories.

We already have (x86) platform code in place to attach
properties (like a board specific firmware filename) to the
device using device-properties so that drivers like silead.c
don't get filled / polluted with board/platform specific knowledge,
which IMHO is the place where the knowledge fallback to
an EFI embedded firmware copy belongs.

As the further patches in v3 of this series shows, this actually
works quite nicely, because this also allows bundling the
EFI-embedded firmware info (prefix, length, crc, name) together
with the other board specific properties.

TL;DR: using request_firmware_efi() vs request_firmware() is
a driver decision, but whether EFI firmware fallback should be
is board/platform specific not driver specific, therefor I
believe that using a device-property to signal this is better.


If you insist on me adding a request_firmware_efi() I can give
this a shot, but I know that Dmitry (the input maintainer) will
very much dislike the silead.c changes that implies...

Still a question for lets sat we go that route, what do we
then do with request_firmware_efi() when CONFIG_EFI is not set ?
Should it be defined then or not, and if it should be defined
when CONFIG_EFI is not set what should it do then?

> Also if you see the
> work I've done to remove the ifdefs over fallback mechanism you'll see it helps
> split code and make it easier to read. We should strive to not add any more
> ifdefery and instead make tehis code read easily.

So looking at how the CONFIG_FW_LOADER_USER_HELPER stuff deals
with this, I should:

1) Move the definition of fw_get_efi_embedded_fw() to a new
drivers/base/firmware_loader/fallback_efi.c,
which only gets build if CONFIG_EFI_EMBEDDED_FIRMWARE is set

2) Put the following in fallback.h:

#ifdef CONFIG_EFI_EMBEDDED_FIRMWARE
int fw_get_efi_embedded_fw(struct device *dev, struct fw_priv *fw_priv, int ret);
#else
static inline int
fw_get_efi_embedded_fw(struct device *dev, struct fw_priv *fw_priv, int ret)
{
	return ret;
}
#endif

have I got that right?

Regards,

Hans

  reply	other threads:[~2018-04-17  8:58 UTC|newest]

Thread overview: 36+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-04-08 17:40 [PATCH v3 0/5] efi/firmware/platform-x86: Add EFI embedded fw support Hans de Goede
2018-04-08 17:40 ` [PATCH v3 1/5] efi: Export boot-services code and data as debugfs-blobs Hans de Goede
2018-04-16  8:23   ` Ard Biesheuvel
2018-04-24 13:11     ` Hans de Goede
2018-04-23 11:55   ` Greg Kroah-Hartman
2018-04-08 17:40 ` [PATCH v3 2/5] efi: Add embedded peripheral firmware support Hans de Goede
2018-04-16  8:28   ` Ard Biesheuvel
2018-04-24 13:17     ` Hans de Goede
2018-04-17  0:17   ` Luis R. Rodriguez
2018-04-17  8:58     ` Hans de Goede [this message]
2018-04-17  9:19     ` Hans de Goede
2018-04-23 21:11   ` Luis R. Rodriguez
2018-04-24 15:09     ` Hans de Goede
2018-04-24 16:07       ` Mimi Zohar
2018-04-24 18:33         ` Hans de Goede
2018-04-24 23:42         ` Luis R. Rodriguez
2018-04-25  5:00           ` Mimi Zohar
2018-04-25 17:55             ` Luis R. Rodriguez
2018-05-04  0:21               ` Luis R. Rodriguez
2018-05-04 15:26                 ` Martijn Coenen
2018-05-04 19:44               ` Martijn Coenen
2018-05-08 15:38                 ` Luis R. Rodriguez
2018-05-08 16:10                   ` Luis R. Rodriguez
2018-06-07 16:49                     ` Bjorn Andersson
2018-06-07 18:22                       ` Luis R. Rodriguez
2018-06-01 19:23                   ` Luis R. Rodriguez
2018-06-06 20:32                     ` Do Qualcomm drivers use DMA buffers for request_firmware_into_buf()? Luis R. Rodriguez
2018-06-07 16:18                       ` Bjorn Andersson
     [not found]                         ` <CAKv+Gu8+Fq7BD4XD-YCyXzZh0mg6Z2k-0styj0cw6_uZfaqy4Q@mail.gmail.com>
     [not found]                           ` <20180607163308.GA18834@kroah.com>
2018-06-08  6:41                             ` Vlastimil Babka
2018-06-07 16:33             ` [PATCH v3 2/5] efi: Add embedded peripheral firmware support Bjorn Andersson
2018-04-08 17:40 ` [PATCH v3 3/5] platform/x86: Rename silead_dmi to touchscreen_dmi Hans de Goede
2018-04-09  8:07   ` Andy Shevchenko
2018-04-20  0:20     ` Darren Hart
2018-04-08 17:40 ` [PATCH v3 4/5] platform/x86: touchscreen_dmi: Add EFI embedded firmware info support Hans de Goede
2018-04-08 17:40 ` [PATCH v3 5/5] platform/x86: touchscreen_dmi: Add info for the Chuwi Vi8 Plus tablet Hans de Goede
2018-04-09  8:10   ` Andy Shevchenko

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=aff2d267-e06d-f422-5623-6f6cc25af155@redhat.com \
    --to=hdegoede@redhat.com \
    --cc=andresx7@gmail.com \
    --cc=andy@infradead.org \
    --cc=ard.biesheuvel@linaro.org \
    --cc=arend.vanspriel@broadcom.com \
    --cc=bjorn.andersson@linaro.org \
    --cc=dave@bewaar.me \
    --cc=dhowells@redhat.com \
    --cc=dmitry.torokhov@gmail.com \
    --cc=duwe@suse.de \
    --cc=dvhart@infradead.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=hpa@zytor.com \
    --cc=josh@joshtriplett.org \
    --cc=keescook@chromium.org \
    --cc=kvalo@codeaurora.org \
    --cc=linux-efi@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=luto@kernel.org \
    --cc=matt@codeblueprint.co.uk \
    --cc=mcgrof@kernel.org \
    --cc=mfuzzey@parkeon.com \
    --cc=mingo@redhat.com \
    --cc=nbroeking@me.com \
    --cc=pjones@redhat.com \
    --cc=platform-driver-x86@vger.kernel.org \
    --cc=tglx@linutronix.de \
    --cc=torvalds@linux-foundation.org \
    --cc=will.deacon@arm.com \
    --cc=x86@kernel.org \
    --cc=zohar@linux.vnet.ibm.com \
    --subject='Re: [PATCH v3 2/5] efi: Add embedded peripheral firmware support' \
    /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: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).