Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752056AbdFZP2R (ORCPT ); Mon, 26 Jun 2017 11:28:17 -0400 Received: from mga06.intel.com ([134.134.136.31]:21834 "EHLO mga06.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751392AbdFZP2P (ORCPT ); Mon, 26 Jun 2017 11:28:15 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.39,396,1493708400"; d="scan'208";a="1187088923" Subject: Re: [PATCHv3 1/3] firmware_class: move NO_CACHE from private to driver_data_req_params To: Greg KH Cc: mcgrof@kernel.org, atull@kernel.org, wagi@monom.org, dwmw2@infradead.org, rafal@milecki.pl, arend.vanspriel@broadcom.com, rjw@rjwysocki.net, moritz.fischer@ettus.com, pmladek@suse.com, johannes.berg@intel.com, emmanuel.grumbach@intel.com, luciano.coelho@intel.com, kvalo@codeaurora.org, luto@kernel.org, takahiro.akashi@linaro.org, dhowells@redhat.com, pjones@redhat.com, linux-kernel@vger.kernel.org, linux-fpga@vger.kernel.org References: <1497653911-11944-1-git-send-email-yi1.li@linux.intel.com> <1497653911-11944-2-git-send-email-yi1.li@linux.intel.com> <20170623154116.GA3565@kroah.com> From: "Li, Yi" Message-ID: <7c6f5288-8009-e70f-b132-35a51dd9964c@linux.intel.com> Date: Mon, 26 Jun 2017 10:28:12 -0500 User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.2.0 MIME-Version: 1.0 In-Reply-To: <20170623154116.GA3565@kroah.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3490 Lines: 89 hi Greg On 6/23/2017 10:41 AM, Greg KH wrote: > On Fri, Jun 16, 2017 at 05:58:29PM -0500, yi1.li@linux.intel.com wrote: >> From: Yi Li >> >> This adds DRIVER_DATA_REQ_NO_CACHE flag with .req flag under struct >> driver_data_req_params. When this flag is set, the driver_data driver >> will not cache the firmware during PM cycle, which is expensive. > > Why is it "expensive"? What do you mean by this? Caching was added to > help things out, why would you not want it? > Say we have couple drivers (GPU, wifi, FPGA, etc) loading firmwares during init, with firmware cache feature on, each time the system going to PM suspend, the firmware framework will cache all the firmware files (which have been loaded before) from fs to RAM, just in case during PM resume the drivers need to re-program firmware. If say FPGA driver does not need to re-program firmware to hardware for suspend/resume, it's "expensive/wasted" to load those in suspend phase and do not use them. Or I am missing something? >> It will be used by streaming case and other drivers which implement >> their own cache thing. > > Why would a driver implement their own cache? And what do you mean by > "streaming case"? When the firmware file size is too big to fit into the RAM, we try to streaming it (load a page and program the page to the hardware and repeat till the end of file). For this case, the firmware was not loaded to RAM as a whole piece, there is no point/way to cache it. > > Again, as I said to Luis's patches, you need to show that the added code > is actually needed. You only convert one driver to this new interface > in this series, is that the only user of it? If so, that seems > excessive don't you think? > I started adding the new streaming use case with current firmware code back on March, saw Luis is working on the new driver data, which suppose to make expending new features cleaner, that's the reason I switched to driver data vehicle. New feature/requirement normally come one at a time, with this logic it's always excessive to add new interface. :) But if you all decide to keep the current firmware framework, I can switch back. >> Also added the debugfs interface to selftest. > > In this patch? Why not have it be in a separate patch like it should > be? Sure, I can do that. > >> diff --git a/drivers/base/firmware_class-dbg.c b/drivers/base/firmware_class-dbg.c >> new file mode 100644 >> index 0000000..102a4cd >> --- /dev/null >> +++ b/drivers/base/firmware_class-dbg.c >> @@ -0,0 +1,108 @@ >> +/* >> + * Copyright (c) 2017 by Yi Li >> + * >> + */ >> +/* This is part of firmware_class.c for testing firmware cache */ >> + >> +#ifndef CONFIG_TEST_DRIVER_DATA >> +static inline void create_debug_files(struct firmware_cache *cache) { } >> +static inline void remove_debug_files(struct firmware_cache *cache) { } >> +#else >> +#include >> +#include > > This implies you are including a .c file into another one. Please never > do that. > >> - /* don't cache firmware handled without uevent */ > > Um, I thought we didn't have this feature already? But this implies > that we do have it, so what exactly does this patch do? > > thanks, > > greg k-h > -- > To unsubscribe from this list: send the line "unsubscribe linux-fpga" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >