From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-0.8 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id ECE22C3A5A2 for ; Fri, 23 Aug 2019 10:31:48 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id C7D2823402 for ; Fri, 23 Aug 2019 10:31:48 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2404604AbfHWKbn (ORCPT ); Fri, 23 Aug 2019 06:31:43 -0400 Received: from mx2.suse.de ([195.135.220.15]:54406 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1733308AbfHWKbn (ORCPT ); Fri, 23 Aug 2019 06:31:43 -0400 X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.220.254]) by mx1.suse.de (Postfix) with ESMTP id BDA51AD94; Fri, 23 Aug 2019 10:31:40 +0000 (UTC) Date: Fri, 23 Aug 2019 12:31:40 +0200 Message-ID: From: Takashi Iwai To: Luis Chamberlain Cc: Scott Branden , Greg Kroah-Hartman , Andy Gross , David Brown , Alexander Viro , Shuah Khan , bjorn.andersson@linaro.org, "Rafael J . Wysocki" , linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org, linux-fsdevel@vger.kernel.org, BCM Kernel Feedback , Olof Johansson , Andrew Morton , Dan Carpenter , Colin Ian King , Kees Cook , linux-kselftest@vger.kernel.org Subject: Re: [PATCH 3/3] firmware: add mutex fw_lock_fallback for race condition In-Reply-To: <20190820012655.GU16384@42.do-not-panic.com> References: <20190816000945.29810-1-scott.branden@broadcom.com> <20190816000945.29810-4-scott.branden@broadcom.com> <20190819053937.GR16384@42.do-not-panic.com> <16823ee6-c52a-b3b5-caed-79c00772fa68@broadcom.com> <20190820012655.GU16384@42.do-not-panic.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/25.3 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sender: linux-arm-msm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-arm-msm@vger.kernel.org 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. 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. (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. 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