Re: [PATCH 3/3] firmware: add mutex fw_lock_fallback for race condition

[Scott Branden]

On 2019-08-23 3:31 a.m., Takashi Iwai wrote:
> On Tue, 20 Aug 2019 03:26:55 +0200,
> Luis Chamberlain wrote:
> > On Mon, Aug 19, 2019 at 09:19:51AM -0700, Scott Branden wrote:
> > > To be honest, I find the entire firmware code sloppy.
> > And that is after years of cleanup on my part. Try going back to v4.1
> > for instance, check the code out then for an incredible horrific sight :)
> >
> > > I don't think the cache/no-cache feature is
> > > implemented or tested properly nor fallback to begin with.
> > I'm in total agreement! I *know* there must be holes in that code, and I
> > acknowledge a few possible gotchas on the commit logs. For instance, I
> > acknowledged that the firmware cache had a secondary purpose which was
> > not well documented or understood through commit e44565f62a720
> > ("firmware: fix batched requests - wake all waiters"). The firmware
> > cache allows for batching requests and sharing the same original request
> > for multiple consecutive requests which *race against each other*.
> > That's when I started having my doubts about the architecture of the
> > firmware cache mechanism, it seemed too complex and perhaps overkill
> > and considered killing it.
> >
> > As I noted in that commit, the firmware cache is used for:
> >      
> > 1) Addressing races with file lookups during the suspend/resume cycle by
> > keeping firmware in memory during the suspend/resume cycle
> Right, this one is the significant need.  And currently the fw loader
> core takes a complicated approach as:
>
> - Store firmware name string in devres for each firmware
> - Upon suspend, loop over all devices and associated firmware names,
>    create a list, then loop over the list for loading the firmware
>    files before sleeping.
> - Upon resume, release the firmware files that have been loaded at
>    suspend in a delayed manner.
>
> So we have different level of lists there, which make the code quite
> hard to understand.
>
> The reason of the above approach is because we didn't know which
> device driver would need the firmware at resume, so basically we do
> cache for all devices.  Maybe it'd better to look for the exact
> drivers that require the firmware at resume, and handle only such
> ones instead of catch-all approach.

Yes, that would be better. Or remove this cache mechanism entirely

and provide some helper functions of some sort to the limited

drivers that actually require such mechanism.

> OTOH, I find it's not bad to keep the loaded firmware file names per
> device and expose e.g. via sysfs.  Currently we have no way to look at
> which firmware files have been loaded afterwards; the only way to see
> it is enabling some debug option and read through kernel messages.
> (FWIW, I stumbled on this problem since I wanted to provide the split
>   kernel-firmware package on SUSE distro, and let the installer decide
>   which package to pick up.)
>
> > 2) Batched requests for the same file rely only on work from the first
> > file lookup, which keeps the firmware in memory until the last
> > release_firmware() is called
> IMO, this feature can be omitted if it makes things too complicated.
> I guess it were added because we handle the fw caching in anyway.
> There isn't a big need for this due to performance.  If the
> performance matters, such driver should re-use its own firmware by
> itself.

Any simplifications would be appreciated.

I sure don't understand what the code is trying to do.

> (snip)
> > > 3) I have a driver that uses request_firmware_into_buf and have multiple
> > > instances of the driver
> > Cool, is the driver upstream?
> >
> > > loading the same firmware in parallel. Some of the data is not read
> > > correctly in each instance.
> > Makes perfect sense considering the lack of testing I noted.
> >
> > > I haven't yet to reproduce this issue with the firmware test
> > That's because of batched firmware request mechanism.
> >
> > > but currently
> > > have a mutex around the entire
> > > call to request_firmware_into_buf in our driver.
> > I will take a look at this now.
> >
> > > Perhaps it is better at this point to add a mutex in
> > > request_firmware_into_buf to make is entirely safe?
> > No, that is not sufficient, although it would also solve the
> > issue.
> The mutex for request_firmware_into_buf() doesn't sound like a good
> approach.  Basically the direct fw loading should work in parallel
> for the same firmware file.  We might have some bug wrt cache stuff,
> but it can be fixed properly.
>
> However, the fw loading in fallback mode can't run in parallel for
> the same file, per design -- no matter whether cached or not.
> So, if any, we'd need put a mutex around the fallback loader code.
> And, the mutex should be rather per device, not a global one.

Sure, whatever solves the issue. All I wish to do is read

part of file into a buffer specified.

> Or we may trick it by appending the second parallel caller into the
> same wait queue, but the code will be more complex, so I don't think
> worth for it.
>
>
> thanks,
>
> Takashi