Date: Fri, 23 Jun 2017 23:41:16 +0800
From: Greg KH <gregkh@linuxfoundation.org>
To: yi1.li@linux.intel.com
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
Subject: Re: [PATCHv3 1/3] firmware_class: move NO_CACHE from private to
 driver_data_req_params
Message-ID: <20170623154116.GA3565@kroah.com>
References: <1497653911-11944-1-git-send-email-yi1.li@linux.intel.com>
 <1497653911-11944-2-git-send-email-yi1.li@linux.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1497653911-11944-2-git-send-email-yi1.li@linux.intel.com>
User-Agent: Mutt/1.8.3 (2017-05-23)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1887
Lines: 56

On Fri, Jun 16, 2017 at 05:58:29PM -0500, yi1.li@linux.intel.com wrote:
> From: Yi Li <yi1.li@linux.intel.com>
> 
> 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?

> 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"?

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?

> Also added the debugfs interface to selftest.

In this patch?  Why not have it be in a separate patch like it should
be?

> 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 <yi1.li@linux.intel.com>
> + *
> + */
> +/* 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 <linux/debugfs.h>
> +#include <linux/seq_file.h>

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