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

Groups > linux.kernel > #1650950 > unrolled thread

Re: [PATCHv2 0/3] Enable no_cache flag to driver_data

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  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

#1650950 — Re: [PATCHv2 0/3] Enable no_cache flag to driver_data

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] | [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] | [standalone]

Back to top | Article view | linux.kernel

csiph-web