Date: Thu, 8 Jun 2017 01:02:04 +0200
From: "Luis R. Rodriguez" <mcgrof@kernel.org>
To: "Li, Yi" <yi1.li@linux.intel.com>
Cc: "Luis R. Rodriguez" <mcgrof@kernel.org>, atull@kernel.org,
        gregkh@linuxfoundation.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: [PATCHv2 0/3] Enable no_cache flag to driver_data
Message-ID: <20170607230204.GO27288@wotan.suse.de>
References: <1495262819-981-1-git-send-email-yi1.li@linux.intel.com>
 <20170524190357.GB8951@wotan.suse.de>
 <bc29b730-def3-a422-8f64-60111f8c3c02@linux.intel.com>
 <20170607175922.GM27288@wotan.suse.de>
 <917cb457-d0ae-aee6-246e-b3d80e6a9c45@linux.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <917cb457-d0ae-aee6-246e-b3d80e6a9c45@linux.intel.com>
User-Agent: Mutt/1.6.0 (2016-04-01)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4642
Lines: 126

On Wed, Jun 07, 2017 at 04:00:42PM -0500, Li, Yi wrote:
> On 6/7/2017 12:59 PM, Luis R. Rodriguez wrote:
> > Ah, consider adding PM to driver_data_test_device ?
> 
> That's what I am looking now. But per my understanding, the misc_device does
> not have the hook to add PM support like those platform device drivers do.
> Please let me know if there is a way to do that.

Ah, hrm. Yeah I'd have to look into it, why does misc devs not have support?

Otherwise you could see if you can just modify the test driver to be a platform
driver, these are easy to register, see. For a simple example see
platform_device_register_simple() use on net/wireless/reg.c.

> > > 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?
> > 
> > As it is on my latest 20170605-driver-data [0] (but should be just as yours if you
> > used a recent branch of mine), fw_pm_notify() uses:
> > 
> > static int fw_pm_notify(struct notifier_block *notify_block,
> > 			unsigned long mode, void *unused)
> > {
> > 	switch (mode) {
> > 	...
> > 	case PM_POST_SUSPEND:
> > 	case PM_POST_HIBERNATION:
> > 	case PM_POST_RESTORE:
> > 		/*
> > 		 * In case that system sleep failed and syscore_suspend is
> > 		 * not called.
> > 		 */
> > 		mutex_lock(&fw_lock);
> > 		fw_cache.state = FW_LOADER_NO_CACHE;
> > 		mutex_unlock(&fw_lock);
> > 		enable_firmware();
> > 
> > 		device_uncache_fw_images_delay(10 * MSEC_PER_SEC);
> > 		break;
> >         }
> > }
> > 
> > So it uses also PM_POST_RESTORE. I think that covers it all ? As it
> > is today we handle the firmware cache for all drivers, it should be
> > transparent to drivers.
> 
> Ah, the comment of "sleep failed" mislead me. :)

Ah no that comment is about more of a brain fuck with the internals of suspend
on Linux, this may be very confusing. Its for this reason why I recently
submitted a patch which helps explain all this more, the patch was recently
merged by Greg KH, it expands on the documentation of fw_pm_notify() so let me
paste the diagram which is relevant form the documentation added:

/**
 * fw_pm_notify - notifier for suspend/resume
 * @notify_block: unused
 * @mode: mode we are switching to
 * @unused: unused
 *
 * Used to modify the firmware_class state as we move in between states.
 * The firmware_class implements a firmware cache to enable device driver
 * to fetch firmware upon resume before the root filesystem is ready. We
 * disable API calls which do not use the built-in firmware or the firmware
 * cache when we know these calls will not work.
 *
 * The inner logic behind all this is a bit complex so it is worth summarizing
 * the kernel's own suspend/resume process with context and focus on how this
 * can impact the firmware API.
 *
 * First a review on how we go to suspend::
 *
 *	pm_suspend() --> enter_state() -->
 *	sys_sync()
 *	suspend_prepare() -->
 *		__pm_notifier_call_chain(PM_SUSPEND_PREPARE, ...);
 *		suspend_freeze_processes() -->
 *			freeze_processes() -->
 *				__usermodehelper_set_disable_depth(UMH_DISABLED);
 *				freeze all tasks ...
 *			freeze_kernel_threads()
 *	suspend_devices_and_enter() -->
 *		dpm_suspend_start() -->
 *				dpm_prepare()
 *				dpm_suspend()
 *		suspend_enter()  -->
 *			platform_suspend_prepare()
 *			dpm_suspend_late()
 *			freeze_enter()
 *			syscore_suspend()
 *
 * When we resume we bail out of a loop from suspend_devices_and_enter() and
 * unwind back out to the caller enter_state() where we were before as follows::
 *
 * 	enter_state() -->
 *	suspend_devices_and_enter() --> (bail from loop)
 *		dpm_resume_end() -->
 *			dpm_resume()
 *			dpm_complete()
 *	suspend_finish() -->
 *		suspend_thaw_processes() -->
 *			thaw_processes() -->
 *				__usermodehelper_set_disable_depth(UMH_FREEZING);
 *				thaw_workqueues();
 *				thaw all processes ...
 *				usermodehelper_enable();
 *		pm_notifier_call_chain(PM_POST_SUSPEND);
 *
 * fw_pm_notify() works through pm_notifier_call_chain().
 */

The key is that even if we fail at syscore_suspend() we will bail out of the
loop on suspend_devices_and_enter() and eventually get our notifier called
with PM_POST_SUSPEND, in the case of suspend/resume. The reason the
comment is there is that even though we have the notifier we also have
the fw_syscore_ops, and our fw_suspend(). If our fw_suspend() fails to
get called then this does not happen:

static int fw_suspend(void)
{
	fw_cache.state = FW_LOADER_NO_CACHE;
	return 0;                                                               
}

  Luis