Date:	Wed, 12 Aug 2015 19:51:35 -0700
From:	James Ausmus <james.ausmus@...el.com>
To:	intel-gfx@...ts.freedesktop.org
Cc:	Daniel Vetter <daniel@...ll.ch>,
	Greg KH <gregkh@...uxfoundation.org>,
	LKML <linux-kernel@...r.kernel.org>,
	Joe Konno <joe.konno@...ux.intel.com>
Subject: Re: [Intel-gfx] [RFC 1/1] drm/i915 : Wait until SYSTEM_RUNNING before loading CSR firmware

On Wednesday 15 July 2015 11:34:43 Daniel Vetter wrote:
> On Tue, Jul 14, 2015 at 01:37:32PM -0700, Greg KH wrote:
> > On Tue, Jul 14, 2015 at 11:22:35AM +0200, Daniel Vetter wrote:
> > > On Mon, Jul 13, 2015 at 09:36:45AM -0700, jay.p.patel@...el.com wrote:
> > > > From: Jay Patel <jay.p.patel@...el.com>
> > > > 
> > > > NOTE: This is an interim solution which is targeted towards
> > > > Chrome OS/Android to be used until a long term solution is available.
> > > > 
> > > > In this patch, request_firmware() is called in a worker thread
> > > > which initially waits for file system to be initialized and then
> > > > attempts to load the firmware.
> > > 
> > > Aside: I posted a bunch of proof-of-principle patches to clean up dmc
> > > loading and convert over to using an explicit workqueue. They're being
> > > tested/made-to-actually-work right now I think.
> > > 
> > > > "request_firmware_nowait()" is also using an asynchronous thread
> > > > running concurrently with the rest of the system initialization.
> > > > However, it tries to load firmware only once without checking the
> > > > sytem status and fails most of the time.
> > > > 
> > > > Change-Id: I2cb16a768e54a85f48a6682d9690b4c8af844668
> > 
> > What's this line for?  :)
> > 
> > > > Signed-off-by: Jay Patel <jay.p.patel@...el.com>
> > > > ---
> > > > 
> > > >  drivers/gpu/drm/i915/i915_drv.c  |  2 ++
> > > >  drivers/gpu/drm/i915/intel_csr.c | 58
> > > >  ++++++++++++++++++++++++++++++++-------- 2 files changed, 49
> > > >  insertions(+), 11 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c
> > > > b/drivers/gpu/drm/i915/i915_drv.c index 8c8407d..eb6f7e3 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > > @@ -559,6 +559,7 @@ void intel_hpd_cancel_work(struct drm_i915_private
> > > > *dev_priv)> > > 
> > > >  void i915_firmware_load_error_print(const char *fw_path, int err)
> > > >  {
> > > >  
> > > >  	DRM_ERROR("failed to load firmware %s (%d)\n", fw_path, err);
> > > > 
> > > > +	DRM_ERROR("The firmware file may be missing\n");
> > > > 
> > > >  	/*
> > > >  	
> > > >  	 * If the reason is not known assume -ENOENT since that's the most
> > > > 
> > > > @@ -574,6 +575,7 @@ void i915_firmware_load_error_print(const char
> > > > *fw_path, int err)> > > 
> > > >  	  "The driver is built-in, so to load the firmware you need to\n"
> > > >  	  "include it either in the kernel (see CONFIG_EXTRA_FIRMWARE) or\n"
> > > >  	  "in your initrd/initramfs image.\n");
> > > > 
> > > > +
> > > > 
> > > >  }
> > > >  
> > > >  static void intel_suspend_encoders(struct drm_i915_private *dev_priv)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_csr.c
> > > > b/drivers/gpu/drm/i915/intel_csr.c index 9311cdd..8d1f08c 100644
> > > > --- a/drivers/gpu/drm/i915/intel_csr.c
> > > > +++ b/drivers/gpu/drm/i915/intel_csr.c
> > > > @@ -349,7 +349,7 @@ static void finish_csr_load(const struct firmware
> > > > *fw, void *context)> > > 
> > > >  	/* load csr program during system boot, as needed for DC states */
> > > >  	intel_csr_load_program(dev);
> > > >  	fw_loaded = true;
> > > > 
> > > > -
> > > > +	DRM_INFO("CSR Firmware Loaded\n");
> > > > 
> > > >  out:
> > > >  	if (fw_loaded)
> > > >  	
> > > >  		intel_runtime_pm_put(dev_priv);
> > > > 
> > > > @@ -359,11 +359,46 @@ out:
> > > >  	release_firmware(fw);
> > > >  
> > > >  }
> > > > 
> > > > +struct csr_firmware_work {
> > > > +	struct work_struct work;
> > > > +	struct module *module;
> > > > +	struct drm_device *dev;
> > > > +};
> > > > +
> > > > +/* csr_firmware_work_func() - thread function for loading the
> > > > firmware*/
> > > > +static void csr_firmware_work_func(struct work_struct *work)
> > > > +{
> > > > +	const struct firmware *fw;
> > > > +	const struct csr_firmware_work *fw_work = container_of(work, 
struct
> > > > csr_firmware_work, work); +	int ret;
> > > > +	struct drm_device *dev = fw_work->dev;
> > > > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > > > +	struct intel_csr *csr = &dev_priv->csr;
> > > > +
> > > > +	/* Wait until root filesystem is loaded in case the firmware
> > > > +	 * is not built-in but in /lib/firmware */
> > > > +	while(system_state !=  SYSTEM_RUNNING){
> > > > +		msleep(500);
> > > > +	}
> > > 
> > > Yeah, not going to merge that right now until we've had a decent
> > > discussion with Greg KH (since imo his stance of every driver creating
> > > it's own retry loop just doesn't work, especially not with gfx where
> > > init
> > > is hairy and you just don't want to retry without end).
> > 
> > Exactly, this type of thing isn't good at all (especially given that
> > the code isn't even checkpatch clean...)
> > 
> > Don't do this.  If you really want to somehow handle built-in drivers
> > that need firmware before the root filesystem is present, then use the
> > async firmware loading interface, don't sit and spin, that's crazy.
> 
> This code is called from a work queue already to facilitate async loading.
> I want an explicit work queue so that we properly sync with it everywhere
> like driver unload or resume (otherwise we need a completion or
> something). And with an explicit worker I can put the entire init sequence
> for that component of the gpu in there, which means whether we require
> firmware or no doesn't change how the driver is loaded. Unified driver
> load paths is a fairly strict requirement I have (because otherwise
> testing is nigh impossible due to combinatorial explosion). I also don't
> want to ever reattempt loading the firmware since those kind of fallback
> paths are equally horrible from a testing perspective. If fw loading fails
> for some reason we'll just move on and declare that particular gpu part
> dead/unsupported.
> 
> The other issue with request_firmware_nowait is that it doesn't do the
> uevent + udev fallback afaiui, see
> 
> commit 5a1379e8748a5cfa3eb068f812d61bde849ef76c
> Author: Takashi Iwai <tiwai@...e.de>
> Date:   Wed Jun 4 17:48:15 2014 +0200
> 
>     firmware loader: allow disabling of udev as firmware loader
> 
> Only request_firmware seems to do that combo.
> 
> > > Aside: Another solution might be the wait_for_rootfs from
> > > 
> > > http://www.gossamer-threads.com/lists/linux/kernel/2010793
> > > 
> > > But if Greg insists that each driver needs to solve this themselves then
> > > I'll pull something like this into upstream, but probably with a Kconfig
> > > option to disable it for normal linux userspace.
> > 
> > "solve" this by just not sitting and spining, wait for userspace to load
> > your firmware if it needs it.  Or, even better yet, if you really need
> > firmare at early boot before a rootfs, build the firmware into the
> > kernel image, like we used to do for a few decades.
> 
> That's exactly what this tries to do (not in a terribly pretty way I
> admit).
> 
> And building the firmware into the image isn't an option since that seems
> to freak out legal or something like that. And loading modules really
> early in initrd (like it's done on desktop linux distros) is also not
> something since for a pile of reasons cros/android want monolithic kernel
> images.
> 
> > > The other option would
> > > be to use CONFIG_FW_LOADER_USERSPACE_FALLBACK udev helper. That might be
> > > an option for intel android, but it sounds like not something cros wants
> > > to do. Therefore
> > 
> > why would chromeos not want to use the udev helper?
> 
> I'm trying to sell them on it and haven't yet figured out why it's not ok,
> but it seems to be a popular request. Other folks also came up with
> similar hacks (the wait_for_rootfs one linked above) so I'm assume it's
> not entirely context free. On these machines everything is static making a
> lot of hotplug processing unecessary.

(Resending without the gmail-forced html content-type)

Finally got some time to chase this down - it's not a technical limitation 
(ChromeOS does use udev) - it's a violation of the security model. With direct 
kernel loading of firmware, checks can happen that ensure the firmware is coming 
from the dm-verity RO-mounted root fs. If a userspace process is providing the 
firmware through the sysfs entry, there's no way to verify that the firmware is 
coming from a trusted file/partition, as the kernel has no knowledge of the 
source of the incoming data.

-James


> 
> > > Acked-by: Daniel Vetter <daniel.vetter@...ll.ch>
> > > 
> > > Also adding Greg so he knows what's happening here.
> > 
> > Ick, please don't take this as-is.
> 
> Well I'd prefer if request_firmware just handles this for me since it
> seems to be a general need. But I'm ok with carrying this around in i915
> only too.
> -Daniel

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists