Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1646069 > unrolled thread

[PATCHv2 0/3] Enable no_cache flag to driver_data

Back to article view | Back to linux.kernel

Contents

  [PATCHv2 0/3] Enable no_cache flag to driver_data  yi1.li@linux.intel.com - 2017-05-20 09:00 +0200
    [PATCHv2 1/3] firmware_class: move NO_CACHE from private to driver_data_req_params yi1.li@linux.intel.com - 2017-05-20 09:00 +0200
    Re: [PATCHv2 0/3] Enable no_cache flag to driver_data "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-05-24 21:10 +0200
      Re: [PATCHv2 0/3] Enable no_cache flag to driver_data "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-05-24 22:40 +0200
      Re: [PATCHv2 0/3] Enable no_cache flag to driver_data "Li, Yi" <yi1.li@linux.intel.com> - 2017-05-26 00:40 +0200
        Re: [PATCHv2 0/3] Enable no_cache flag to driver_data "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-05-26 00:50 +0200
          Re: [PATCHv2 0/3] Enable no_cache flag to driver_data "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-05-27 03:30 +0200
          Re: [PATCHv2 0/3] Enable no_cache flag to driver_data "Li, Yi" <yi1.li@linux.intel.com> - 2017-05-27 03:30 +0200
      Re: [PATCHv2 0/3] Enable no_cache flag to driver_data "Li, Yi" <yi1.li@linux.intel.com> - 2017-06-06 21:40 +0200

#1646069 — [PATCHv2 0/3] Enable no_cache flag to driver_data

From: Yi Li <yi1.li@linux.intel.com>

Changes in v2:

  - Rebase to Luis R. Rodriguez's 20170501-driver-data-try2
    branch 
  - Expose DRIVER_DATA_REQ_NO_CACHE flag to public 
    driver_data_req_params structure, so upper drivers can ask
    driver_data driver to bypass the internal caching mechanism.
    This will be used for streaming and other drivers maintains
    their own caching like iwlwifi. 
  - Add self test cases.


Yi Li (3):
  firmware_class: move NO_CACHE from private to driver_data_req_params
  iwlwifi: use DRIVER_DATA_REQ_NO_CACHE for driver_data
  test: add no_cache to driver_data load tester

 drivers/base/firmware_class.c                   | 16 ++++-----
 drivers/net/wireless/intel/iwlwifi/iwl-drv.c    |  2 ++
 include/linux/driver_data.h                     |  4 +++
 lib/test_driver_data.c                          | 43 +++++++++++++++++++++++--
 tools/testing/selftests/firmware/driver_data.sh | 36 +++++++++++++++++++++
 5 files changed, 89 insertions(+), 12 deletions(-)

-- 
2.7.4

[toc] | [next] | [standalone]

#1646071 — [PATCHv2 1/3] firmware_class: move NO_CACHE from private to driver_data_req_params

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 bypass the internal caching mechanism, its used by streaming
case and other drivers which implement their own cache thing.

Signed-off-by: Yi Li <yi1.li@linux.intel.com>
---
 drivers/base/firmware_class.c | 16 +++++++---------
 include/linux/driver_data.h   |  4 ++++
 2 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 7af430a..444c9a8 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -72,14 +72,10 @@ enum driver_data_mode {
  * 	issue a uevent to userspace. Userspace in turn is expected to be
  * 	monitoring for uevents for the firmware_class and will use the
  * 	exposted sysfs interface to upload the driver data for the caller.
- * @DRIVER_DATA_PRIV_REQ_NO_CACHE: indicates that the driver data request
- * 	should not set up and use the internal caching mechanism to assist
- * 	drivers from fetching driver data at resume time after suspend.
  */
 enum driver_data_priv_reqs {
 	DRIVER_DATA_PRIV_REQ_FALLBACK			= 1 << 0,
 	DRIVER_DATA_PRIV_REQ_FALLBACK_UEVENT		= 1 << 1,
-	DRIVER_DATA_PRIV_REQ_NO_CACHE			= 1 << 2,
 };
 
 /**
@@ -151,10 +147,12 @@ struct driver_data_params {
 	}
 
 #define __DATA_REQ_FIRMWARE_BUF(buf, size)				\
+	.req_params = {							\
+		.reqs = DRIVER_DATA_REQ_NO_CACHE,			\
+	},								\
 	.priv_params = {						\
 		.priv_reqs = DRIVER_DATA_PRIV_REQ_FALLBACK |		\
-			     DRIVER_DATA_PRIV_REQ_FALLBACK_UEVENT |	\
-			     DRIVER_DATA_PRIV_REQ_NO_CACHE,		\
+			     DRIVER_DATA_PRIV_REQ_FALLBACK_UEVENT,	\
 		.alloc_buf = buf,					\
 		.alloc_buf_size = size,					\
 	}
@@ -186,7 +184,7 @@ struct driver_data_params {
 #define driver_data_param_uevent(params)	\
 	(!!((params)->priv_reqs & DRIVER_DATA_PRIV_REQ_FALLBACK_UEVENT))
 #define driver_data_param_nocache(params)	\
-	(!!((params)->priv_reqs & DRIVER_DATA_PRIV_REQ_NO_CACHE))
+	(!!((params)->reqs & DRIVER_DATA_REQ_NO_CACHE))
 
 #define driver_data_param_optional(params)	\
 	(!!((params)->reqs & DRIVER_DATA_REQ_OPTIONAL))
@@ -793,14 +791,14 @@ static int assign_firmware_buf(struct firmware *fw, struct device *device,
 	/* don't cache firmware handled without uevent */
 	if (device &&
 	    driver_data_param_uevent(&data_params->priv_params) &&
-	    !driver_data_param_nocache(&data_params->priv_params))
+	    !driver_data_param_nocache(&data_params->req_params))
 		fw_add_devm_name(device, buf->fw_id);
 
 	/*
 	 * After caching firmware image is started, let it piggyback
 	 * on request firmware.
 	 */
-	if (!driver_data_param_nocache(&data_params->priv_params) &&
+	if (!driver_data_param_nocache(&data_params->req_params) &&
 	    buf->fwc->state == FW_LOADER_START_CACHE) {
 		if (fw_cache_piggyback_on_request(buf->fw_id))
 			kref_get(&buf->ref);
diff --git a/include/linux/driver_data.h b/include/linux/driver_data.h
index bf51e0b..b6ef5ee 100644
--- a/include/linux/driver_data.h
+++ b/include/linux/driver_data.h
@@ -117,11 +117,15 @@ union driver_data_cbs {
  *	file to be present given the API range, it is only required for one
  *	file in the API range to be present.  If the %DRIVER_DATA_REQ_OPTIONAL
  *	flag is also enabled then all files are treated as optional.
+ * @DRIVER_DATA_REQ_NO_CACHE: indicates that the driver data request
+ *	should not set up and use the internal caching mechanism to assist
+ *	drivers from fetching driver data at resume time after suspend.
  */
 enum driver_data_reqs {
 	DRIVER_DATA_REQ_OPTIONAL			= 1 << 0,
 	DRIVER_DATA_REQ_KEEP				= 1 << 1,
 	DRIVER_DATA_REQ_USE_API_VERSIONING		= 1 << 2,
+	DRIVER_DATA_REQ_NO_CACHE			= 1 << 3,
 };
 
 /**
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1649866

On Sat, May 20, 2017 at 01:46:56AM -0500, yi1.li@linux.intel.com wrote:
> From: Yi Li <yi1.li@linux.intel.com>
> 
> Changes in v2:
> 
>   - Rebase to Luis R. Rodriguez's 20170501-driver-data-try2
>     branch 
>   - Expose DRIVER_DATA_REQ_NO_CACHE flag to public 
>     driver_data_req_params structure, so upper drivers can ask
>     driver_data driver to bypass the internal caching mechanism.
>     This will be used for streaming and other drivers maintains
>     their own caching like iwlwifi. 
>   - Add self test cases.
> 
> 
> Yi Li (3):
>   firmware_class: move NO_CACHE from private to driver_data_req_params
>   iwlwifi: use DRIVER_DATA_REQ_NO_CACHE for driver_data
>   test: add no_cache to driver_data load tester
> 
>  drivers/base/firmware_class.c                   | 16 ++++-----
>  drivers/net/wireless/intel/iwlwifi/iwl-drv.c    |  2 ++
>  include/linux/driver_data.h                     |  4 +++
>  lib/test_driver_data.c                          | 43 +++++++++++++++++++++++--
>  tools/testing/selftests/firmware/driver_data.sh | 36 +++++++++++++++++++++
>  5 files changed, 89 insertions(+), 12 deletions(-)
> 

Good stuff, this series is looking good and very easy to read !  Only thing
though -- the cache test is just setting up the cache and ensuring it gets set,
it doesn't really *test* the cache is functional. Can you devise a test which
does ensure the cache is functional ?

We use the cache upon suspend to cache the firmware so that upon resume a
request will use that cache, to avoid the file lookup on disk. Doing a test
with qemu suspend + resume is possible but that requires having access to
the qemu monitor interface and doing something like this to trigger a wakeup:

echo system_wakeup | socat - /dev/pts/7,raw,echo=0,crnl

where /dev/pts/7 is assumed to be the PTY. This is rather complex for ksefltest,
so another option is to add a debug mode debugfs interface for firmware_class where
we can force-enable the cache mechanism without actually this being prompted by a
suspend/hibernate. Then we can just toggle this bit from a testing perspective and
excercise the caching mechanism.

So if you look at fw_pm_notify()

        switch (mode) {                                                         
        case PM_HIBERNATION_PREPARE:                                            
        case PM_SUSPEND_PREPARE:                                                
        case PM_RESTORE_PREPARE:                                                
                /*                                                              
                 * kill pending fallback requests with a custom fallback        
                 * to avoid stalling suspend.                                   
                 */                                                             
                kill_pending_fw_fallback_reqs(true);                            
                device_cache_fw_images();                                       
                disable_firmware();                                             
                break;    


kill_pending_fw_fallback_reqs(true), device_cache_fw_images(), and
disable_firmware() could be folder into a helper, then a debugfs interface
could kick that into action to put us cache mode as the
device_cache_fw_images() changes the cache state to FW_LOADER_START_CACHE, and
when this is done you'll notice that assign_firmware_buf() does:

        /*                                                                      
         * After caching firmware image is started, let it piggyback            
         * on request firmware.                                                 
         */                                                                     
        if (!driver_data_param_nocache(&data_params->priv_params) &&            
            buf->fwc->state == FW_LOADER_START_CACHE) {                         
                if (fw_cache_piggyback_on_request(buf->fw_id))                  
                        kref_get(&buf->ref);                                    
        }    

Which adds an incoming request to the cache. The first request that adds this cache
entry would be triggered by device_cache_fw_images() after the cache state is enabled:

static void device_cache_fw_images(void) 
{
	...
        fwc->state = FW_LOADER_START_CACHE;                                     
        dpm_for_each_dev(NULL, dev_cache_fw_image);  
	...
}

Subsequent requests then lookup for the cache through _request_firmware_prepare()
when fw_lookup_and_allocate_buf() is called. __fw_lookup_buf() really should be
renamed to something that reflects this is a cache lookup. In fact if you find
anything else that needs renaming to make it clear please feel free to send patches
for it.

We want to test that when caching is enabled, the cache is actually used.

Note that disable_firmware() above on the notifier *does* disable subsequent firmware
lookups but this is only *if* the lookup fails with _request_firmware_prepare():

_request_firmware(const struct firmware **firmware_p, const char *name,         
                  struct driver_data_params *data_params,                       
                  struct device *device)                                        
{                                                                               
        struct firmware *fw = NULL;                                             
        int ret;                                                                
                                                                                
        if (!firmware_p)                                                        
                return -EINVAL;                                                 
                                                                                
        if (!name || name[0] == '\0') {                                         
                ret = -EINVAL;                                                  
                goto out;                                                       
        }                                                                       
                                                                                
        ret = _request_firmware_prepare(&fw, name, device, data_params);        
        if (ret <= 0) /* error or already assigned */                           
                goto out;                                                       
                                                                                
        if (!firmware_enabled()) {                                              
                WARN(1, "firmware request while host is not available\n");      
                ret = -EHOSTDOWN;                                               
                goto out;                                                       
        }                                                 
	...
}

The idea is that _request_firmware_prepare() will have picked up the cache and
enabled use of that while the infrastructure for disk lookups is disabled. The
caching effect is lifted later on the same notifier fw_pm_notify() and it also
schedules a clearing of the cached firmwares with device_uncache_fw_images_delay().

To me this last part smells like a possible source of issue (not sure) if we might
suspend/resume a lot in short period of time, this theory could be tested by toggling
on/of this debugfs interface I suggested while having requests of different types
blast in.

This is the sort of testing which would really help here.

Likewise, if you are extending functionality please consider ways to break it :)
and test against it. Please think about these things carefully, its what will
change the stability for the better long term of our loader infrastructure.

  Luis

[toc] | [prev] | [next] | [standalone]

#1649933

On Wed, May 24, 2017 at 09:03:57PM +0200, Luis R. Rodriguez wrote:
> __fw_lookup_buf() really should be
> renamed to something that reflects this is a cache lookup.

Actually I take this back, other than the cache, note that when we
fw_lookup_and_allocate_buf() we first __fw_lookup_buf() but if the buf is not
there we allocate a new one and list_add() it to the cache regardless of
whether or not the cache thing has been enabled.

What this does then *also* other than caching for suspend/resume (which we
should document more formally) is to gather up contending lookups together
with one buf, and share the final status of just one lookup. If a buf is
found we fw_state_wait() on _request_firmware_prepare() until the buf clears.

John Ewalt recently reported some issues with loadng multiple files at the
same time. He also provided a patch. The swake_up() fix seems sensible
and would seem to have been caused by the swait transformation, but in
inspecting the other proposed changes it would seem we have had tons of
other lingering bugs which have probably existed for ages.

For instance, if there are pending requests for a leader request to send back
info, and one is about to complete but in the last moment on
assign_firmware_buf() fails, all the error paths lack a wake up call. As such
all pending requests may just wait and linger, and since none of these have
a timeout I would expect these to just linger forever. I'm not even sure we
kref_get() properly on the buf for pending requests when we are waiting for
a serialized request, ie, we might be able to take a buf underneath the nose
of a waiter.

Although some are new bugs, some seem to be really old bugs.

These are the sorts of issues I wish for a test driver to be able to uncover,
test and ensure we never regress again. This is also why I am being careful
about enabling a feature, we should *really* think things through well before
enabling on the new API.

[0] https://bugzilla.kernel.org/show_bug.cgi?id=195477

  Luis

[toc] | [prev] | [next] | [standalone]

#1650950

hi Luis


On 5/24/2017 2:03 PM, Luis R. Rodriguez wrote:
> On Sat, May 20, 2017 at 01:46:56AM -0500, yi1.li@linux.intel.com wrote:
>> From: Yi Li <yi1.li@linux.intel.com>
>>
>> Changes in v2:
>>
>>    - Rebase to Luis R. Rodriguez's 20170501-driver-data-try2
>>      branch
>>    - Expose DRIVER_DATA_REQ_NO_CACHE flag to public
>>      driver_data_req_params structure, so upper drivers can ask
>>      driver_data driver to bypass the internal caching mechanism.
>>      This will be used for streaming and other drivers maintains
>>      their own caching like iwlwifi.
>>    - Add self test cases.
>>
>>
>> Yi Li (3):
>>    firmware_class: move NO_CACHE from private to driver_data_req_params
>>    iwlwifi: use DRIVER_DATA_REQ_NO_CACHE for driver_data
>>    test: add no_cache to driver_data load tester
>>
>>   drivers/base/firmware_class.c                   | 16 ++++-----
>>   drivers/net/wireless/intel/iwlwifi/iwl-drv.c    |  2 ++
>>   include/linux/driver_data.h                     |  4 +++
>>   lib/test_driver_data.c                          | 43 +++++++++++++++++++++++--
>>   tools/testing/selftests/firmware/driver_data.sh | 36 +++++++++++++++++++++
>>   5 files changed, 89 insertions(+), 12 deletions(-)
>>
> Good stuff, this series is looking good and very easy to read !  Only thing
> though -- the cache test is just setting up the cache and ensuring it gets set,
> it doesn't really *test* the cache is functional. Can you devise a test which
> does ensure the cache is functional ?

This patch is for "disabling the cache" for streaming and iwlwifi case, 
adding the test to verify the cache function should be a separate patch, 
right? I can look more into the cache part.

Yi
>
> We use the cache upon suspend to cache the firmware so that upon resume a
> request will use that cache, to avoid the file lookup on disk. Doing a test
> with qemu suspend + resume is possible but that requires having access to
> the qemu monitor interface and doing something like this to trigger a wakeup:
>
> echo system_wakeup | socat - /dev/pts/7,raw,echo=0,crnl
>
> where /dev/pts/7 is assumed to be the PTY. This is rather complex for ksefltest,
> so another option is to add a debug mode debugfs interface for firmware_class where
> we can force-enable the cache mechanism without actually this being prompted by a
> suspend/hibernate. Then we can just toggle this bit from a testing perspective and
> excercise the caching mechanism.
>
> So if you look at fw_pm_notify()
>
>          switch (mode) {
>          case PM_HIBERNATION_PREPARE:
>          case PM_SUSPEND_PREPARE:
>          case PM_RESTORE_PREPARE:
>                  /*
>                   * kill pending fallback requests with a custom fallback
>                   * to avoid stalling suspend.
>                   */
>                  kill_pending_fw_fallback_reqs(true);
>                  device_cache_fw_images();
>                  disable_firmware();
>                  break;
>
>
> kill_pending_fw_fallback_reqs(true), device_cache_fw_images(), and
> disable_firmware() could be folder into a helper, then a debugfs interface
> could kick that into action to put us cache mode as the
> device_cache_fw_images() changes the cache state to FW_LOADER_START_CACHE, and
> when this is done you'll notice that assign_firmware_buf() does:
>
>          /*
>           * After caching firmware image is started, let it piggyback
>           * on request firmware.
>           */
>          if (!driver_data_param_nocache(&data_params->priv_params) &&
>              buf->fwc->state == FW_LOADER_START_CACHE) {
>                  if (fw_cache_piggyback_on_request(buf->fw_id))
>                          kref_get(&buf->ref);
>          }
>
> Which adds an incoming request to the cache. The first request that adds this cache
> entry would be triggered by device_cache_fw_images() after the cache state is enabled:
>
> static void device_cache_fw_images(void)
> {
> 	...
>          fwc->state = FW_LOADER_START_CACHE;
>          dpm_for_each_dev(NULL, dev_cache_fw_image);
> 	...
> }
>
> Subsequent requests then lookup for the cache through _request_firmware_prepare()
> when fw_lookup_and_allocate_buf() is called. __fw_lookup_buf() really should be
> renamed to something that reflects this is a cache lookup. In fact if you find
> anything else that needs renaming to make it clear please feel free to send patches
> for it.
>
> We want to test that when caching is enabled, the cache is actually used.
>
> Note that disable_firmware() above on the notifier *does* disable subsequent firmware
> lookups but this is only *if* the lookup fails with _request_firmware_prepare():
>
> _request_firmware(const struct firmware **firmware_p, const char *name,
>                    struct driver_data_params *data_params,
>                    struct device *device)
> {
>          struct firmware *fw = NULL;
>          int ret;
>                                                                                  
>          if (!firmware_p)
>                  return -EINVAL;
>                                                                                  
>          if (!name || name[0] == '\0') {
>                  ret = -EINVAL;
>                  goto out;
>          }
>                                                                                  
>          ret = _request_firmware_prepare(&fw, name, device, data_params);
>          if (ret <= 0) /* error or already assigned */
>                  goto out;
>                                                                                  
>          if (!firmware_enabled()) {
>                  WARN(1, "firmware request while host is not available\n");
>                  ret = -EHOSTDOWN;
>                  goto out;
>          }
> 	...
> }
>
> The idea is that _request_firmware_prepare() will have picked up the cache and
> enabled use of that while the infrastructure for disk lookups is disabled. The
> caching effect is lifted later on the same notifier fw_pm_notify() and it also
> schedules a clearing of the cached firmwares with device_uncache_fw_images_delay().
>
> To me this last part smells like a possible source of issue (not sure) if we might
> suspend/resume a lot in short period of time, this theory could be tested by toggling
> on/of this debugfs interface I suggested while having requests of different types
> blast in.
>
> This is the sort of testing which would really help here.
>
> Likewise, if you are extending functionality please consider ways to break it :)
> and test against it. Please think about these things carefully, its what will
> change the stability for the better long term of our loader infrastructure.
>
>    Luis
> --
> 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
>

[toc] | [prev] | [next] | [standalone]

#1650954

On Thu, May 25, 2017 at 3:30 PM, Li, Yi <yi1.li@linux.intel.com> wrote:
> This patch is for "disabling the cache" for streaming and iwlwifi case,
> adding the test to verify the cache function should be a separate patch,
> right? I can look more into the cache part.

How can we know cache was disabled without first testing what having
cache enabled could be like ?

  Luis

[toc] | [prev] | [next] | [standalone]

#1651619

On Fri, May 26, 2017 at 04:05:43PM -0500, Li, Yi wrote:
> hi Luis
> 
> 
> On 5/25/2017 5:43 PM, Luis R. Rodriguez wrote:
> > On Thu, May 25, 2017 at 3:30 PM, Li, Yi <yi1.li@linux.intel.com> wrote:
> > > This patch is for "disabling the cache" for streaming and iwlwifi case,
> > > adding the test to verify the cache function should be a separate patch,
> > > right? I can look more into the cache part.
> > How can we know cache was disabled without first testing what having
> > cache enabled could be like ?
> 
> Understand the point, adding the test for cache enabled still cannot cover
> the disable cache part though:-). Please give me couple days to think about
> it. Maybe will first have a new patch to test the cache part, likely through
> the debugfs as you suggested; then amend this series to complete the cache
> disabling test part. Thanks for the insight.

Sounds good!

  Luis

[toc] | [prev] | [next] | [standalone]

#1651649

hi Luis


On 5/25/2017 5:43 PM, Luis R. Rodriguez wrote:
> On Thu, May 25, 2017 at 3:30 PM, Li, Yi <yi1.li@linux.intel.com> wrote:
>> This patch is for "disabling the cache" for streaming and iwlwifi case,
>> adding the test to verify the cache function should be a separate patch,
>> right? I can look more into the cache part.
> How can we know cache was disabled without first testing what having
> cache enabled could be like ?

Understand the point, adding the test for cache enabled still cannot 
cover the disable cache part though:-). Please give me couple days to 
think about it. Maybe will first have a new patch to test the cache 
part, likely through the debugfs as you suggested; then amend this 
series to complete the cache disabling test part. Thanks for the insight.

Yi
>
>    Luis
> --
> 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

[toc] | [prev] | [next] | [standalone]

#1659153

Hi Luis,


On 5/24/2017 2:03 PM, Luis R. Rodriguez wrote:
> On Sat, May 20, 2017 at 01:46:56AM -0500, yi1.li@linux.intel.com wrote:
>> From: Yi Li <yi1.li@linux.intel.com>
>>
>> Changes in v2:
>>
>>    - Rebase to Luis R. Rodriguez's 20170501-driver-data-try2
>>      branch
>>    - Expose DRIVER_DATA_REQ_NO_CACHE flag to public
>>      driver_data_req_params structure, so upper drivers can ask
>>      driver_data driver to bypass the internal caching mechanism.
>>      This will be used for streaming and other drivers maintains
>>      their own caching like iwlwifi.
>>    - Add self test cases.
>>
>>
>> Yi Li (3):
>>    firmware_class: move NO_CACHE from private to driver_data_req_params
>>    iwlwifi: use DRIVER_DATA_REQ_NO_CACHE for driver_data
>>    test: add no_cache to driver_data load tester
>>
>>   drivers/base/firmware_class.c                   | 16 ++++-----
>>   drivers/net/wireless/intel/iwlwifi/iwl-drv.c    |  2 ++
>>   include/linux/driver_data.h                     |  4 +++
>>   lib/test_driver_data.c                          | 43 +++++++++++++++++++++++--
>>   tools/testing/selftests/firmware/driver_data.sh | 36 +++++++++++++++++++++
>>   5 files changed, 89 insertions(+), 12 deletions(-)
>>
> Good stuff, this series is looking good and very easy to read !  Only thing
> though -- the cache test is just setting up the cache and ensuring it gets set,
> it doesn't really *test* the cache is functional. Can you devise a test which
> does ensure the cache is functional ?
>
> We use the cache upon suspend to cache the firmware so that upon resume a
> request will use that cache, to avoid the file lookup on disk. Doing a test
> with qemu suspend + resume is possible but that requires having access to
> the qemu monitor interface and doing something like this to trigger a wakeup:
>
> echo system_wakeup | socat - /dev/pts/7,raw,echo=0,crnl
>
> where /dev/pts/7 is assumed to be the PTY. This is rather complex for ksefltest,
> so another option is to add a debug mode debugfs interface for firmware_class where
> we can force-enable the cache mechanism without actually this being prompted by a
> suspend/hibernate. Then we can just toggle this bit from a testing perspective and
> excercise the caching mechanism.
>
> So if you look at fw_pm_notify()
>
>          switch (mode) {
>          case PM_HIBERNATION_PREPARE:
>          case PM_SUSPEND_PREPARE:
>          case PM_RESTORE_PREPARE:
>                  /*
>                   * kill pending fallback requests with a custom fallback
>                   * to avoid stalling suspend.
>                   */
>                  kill_pending_fw_fallback_reqs(true);
>                  device_cache_fw_images();
>                  disable_firmware();
>                  break;
>
I am studying at the firmware caching codes and have to say it's very 
complicated. :-( Here are some questions:

1. Since device_cache_fw_images invokes dev_cache_fw_image through 
dpm_for_each_dev, adding a debugfs driver to kick it can only cache 
firmware for those associated with devices which has PM enabled, which 
do not include the driver_data_test_device. Any suggestions?

2. Look into dev_cache_fw_image function, devres_for_each_res will walk through the firmware have been loaded before (through assign_firmware_buf -> fw_add_devm_name) and add to the todo list, eventually it will create the fw_names list. So in the test driver, we need to load the firmware once before calling the kick?
dev_cache_fw_image(struct device *dev, void *data)

{

LIST_HEAD(todo);

struct fw_cache_entry *fce;

struct fw_cache_entry *fce_next;

struct firmware_cache *fwc = &fw_cache;

devres_for_each_res(dev, fw_name_devm_release,

devm_name_match, &fw_cache,

dev_create_fw_entry, &todo);

list_for_each_entry_safe(fce, fce_next, &todo, list) {

list_del(&fce->list);

spin_lock(&fwc->name_lock);

/* only one cache entry for one firmware */

if (!__fw_entry_found(fce->name)) {

list_add(&fce->list, &fwc->fw_names);

} else {

free_fw_cache_entry(fce);

           ...
}
> kill_pending_fw_fallback_reqs(true), device_cache_fw_images(), and
> disable_firmware() could be folder into a helper, then a debugfs interface
> could kick that into action to put us cache mode as the
> device_cache_fw_images() changes the cache state to FW_LOADER_START_CACHE, and
> when this is done you'll notice that assign_firmware_buf() does:
>
>          /*
>           * After caching firmware image is started, let it piggyback
>           * on request firmware.
>           */
>          if (!driver_data_param_nocache(&data_params->priv_params) &&
>              buf->fwc->state == FW_LOADER_START_CACHE) {
>                  if (fw_cache_piggyback_on_request(buf->fw_id))
>                          kref_get(&buf->ref);
>          }
>
> Which adds an incoming request to the cache. The first request that adds this cache
> entry would be triggered by device_cache_fw_images() after the cache state is enabled:
>
> static void device_cache_fw_images(void)
> {
> 	...
>          fwc->state = FW_LOADER_START_CACHE;
>          dpm_for_each_dev(NULL, dev_cache_fw_image);
> 	...
> }
This only applies to the devices have PM enabled.
> Subsequent requests then lookup for the cache through _request_firmware_prepare()
> when fw_lookup_and_allocate_buf() is called. __fw_lookup_buf() really should be
> renamed to something that reflects this is a cache lookup. In fact if you find
> anything else that needs renaming to make it clear please feel free to send patches
> for it.
>
> We want to test that when caching is enabled, the cache is actually used.
>
> Note that disable_firmware() above on the notifier *does* disable subsequent firmware
> lookups but this is only *if* the lookup fails with _request_firmware_prepare():
>
> _request_firmware(const struct firmware **firmware_p, const char *name,
>                    struct driver_data_params *data_params,
>                    struct device *device)
> {
>          struct firmware *fw = NULL;
>          int ret;
>                                                                                  
>          if (!firmware_p)
>                  return -EINVAL;
>                                                                                  
>          if (!name || name[0] == '\0') {
>                  ret = -EINVAL;
>                  goto out;
>          }
>                                                                                  
>          ret = _request_firmware_prepare(&fw, name, device, data_params);
>          if (ret <= 0) /* error or already assigned */
>                  goto out;
>                                                                                  
>          if (!firmware_enabled()) {
>                  WARN(1, "firmware request while host is not available\n");
>                  ret = -EHOSTDOWN;
>                  goto out;
>          }
> 	...
> }
>
> The idea is that _request_firmware_prepare() will have picked up the cache and
> enabled use of that while the infrastructure for disk lookups is disabled. The
> caching effect is lifted later on the same notifier fw_pm_notify() and it also
> schedules a clearing of the cached firmwares with device_uncache_fw_images_delay().

device_uncache_fw_images_delay() will be called for PM_POST_RESTORE, which means the hibernation restore got error. How about the successful restore case, calling driver will free its firmware_buf after loading?

> To me this last part smells like a possible source of issue (not sure) if we might
> suspend/resume a lot in short period of time, this theory could be tested by toggling
> on/of this debugfs interface I suggested while having requests of different types
> blast in.
Agree, it might create an issue if the system is get into 
restore_prepare again before the device_uncache_fw_images_delay clear 
the cache, why we need the 10 * MSEC_PER_SEC delay? In theory, 
fwc->name_lock should protect the case though.
>
> This is the sort of testing which would really help here.
>
> Likewise, if you are extending functionality please consider ways to break it :)
Understand the need to test the firmware caching part. For non-caching 
test case, will it be enough if we can test that the noncache setting 
will ban the firmware name be added to the fwc->fw_names list?
> and test against it. Please think about these things carefully, its what will
> change the stability for the better long term of our loader infrastructure.
>
>    Luis
> --
> 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
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web