lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Sun, 12 Aug 2012 12:43:12 -0400 (EDT)
From:	Alan Stern <stern@...land.harvard.edu>
To:	"Rafael J. Wysocki" <rjw@...k.pl>
cc:	Linux PM list <linux-pm@...r.kernel.org>,
	Ming Lei <tom.leiming@...il.com>,
	LKML <linux-kernel@...r.kernel.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: [PATCH][Alternative][RFC] PM / Runtime: Introduce driver runtime
 PM work routine

On Thu, 9 Aug 2012, Rafael J. Wysocki wrote:

> There are some known concerns about this approach.
> 
> First of all, the patch at
> 
> https://patchwork.kernel.org/patch/1299781/
> 
> increases the size of struct device by the size of a pointer, which may seem to
> be a bit excessive to somebody, although I personally don't think it's a big
> problem.  We don't use _that_ many struct device objects for it to matter much.
> 
> Second, which is more important to me, it seems that for a given device func()
> will always be the same pointer and it will be used by the device's driver
> only.  In that case, most likely, it will be possible to determine the
> address of func() at the time of driver initialization, so the setting and
> clearing of power.func and passing the address of func() as an argument every
> time __pm_runtime_get_and_call() is run may turn out to be an unnecessary
> overhead.  Thus it may be more efficient to use a function pointer in struct
> device_driver (it can't be located in struct dev_pm_ops, because some drivers
> don't use it at all, like USB drivers, and it wouldn't be useful for subsystems
> and PM domains) to store the address of func() permanently.
> 
> For the above reasons, the appended patch implements an alternative approach,
> which is to modify the way pm_runtime_get() works so that, when the device is
> not active, it will queue a resume request for the device _along_ _with_ the
> execution of a driver routine provided through a new function pointer
> .pm_runtime_work().  There also is pm_runtime_get_nowork() that won't do that
> and works in the same way as the "old" pm_runtime_get().
> 
> Of course, the disadvantage of the new patch is that it makes the change
> in struct device_driver, but perhaps it's not a big deal.
> 
> I wonder what you think.

I have some concerns about this patch.

Firstly, the patch doesn't do anything in the case where the device is
already at full power.  Should we invoke the callback routine 
synchronously?  This loses the advantage of a workqueue's "pristine" 
environment, but on the other hand it is much more efficient.  (And 
we're talking about hot pathways, where efficiency matters.)  The 
alternative, of course, is to have the driver invoke the callback 
directly if pm_runtime_get() returns 1.

Secondly, is it necessary for __pm_runtime_barrier() to wait for the 
callback routine?  More generally, does __pm_runtime_barrier() really 
do what it's supposed to do?  What happens if it runs at the same time 
as another thread is executing the pm_runtime_put(parent) call at the 
end of rpm_resume(), or the rpm_resume(parent, 0) in the middle?

Thirdly, I would reorganize the new code added to pm_runtime_work(); 
see below.


> @@ -533,6 +550,13 @@ static int rpm_resume(struct device *dev
>  		goto out;
>  	}
>  
> +	if ((rpmflags & RPM_ASYNC) && (rpmflags & RPM_RUN_WORK)) {
> +		dev->power.run_driver_work = true;
> +		rpm_queue_up_resume(dev);
> +		retval = 0;
> +		goto out;
> +	}
> +

The section of code just before the start of this hunk exits the
routine if the device is already active.  Do you want to put this new
section in the preceding "if" block?

Also, it feels odd to have this code here when there is another section
lower down that also tests for RPM_ASYNC and does almost the same
thing.  It suggests that this new section isn't in the right place.  
For instance, consider what happens in the "no_callbacks" case where
the parent is already active.

> @@ -715,11 +736,29 @@ static void pm_runtime_work(struct work_
>  		rpm_suspend(dev, RPM_NOWAIT | RPM_AUTO);
>  		break;
>  	case RPM_REQ_RESUME:
> -		rpm_resume(dev, RPM_NOWAIT);
> +		if (dev->power.run_driver_work && dev->driver->pm_runtime_work)
> +			driver_work = dev->driver->pm_runtime_work;
> +
> +		dev->power.run_driver_work = false;
> +		rpm_resume(dev, driver_work ? 0 : RPM_NOWAIT);
>  		break;
>  	}
>  
>   out:
> +	if (driver_work) {
> +		pm_runtime_get_noresume(dev);
> +		dev->power.work_in_progress = true;
> +		spin_unlock_irq(&dev->power.lock);
> +
> +		driver_work(dev);
> +
> +		spin_lock_irq(&dev->power.lock);
> +		dev->power.work_in_progress = false;
> +		wake_up_all(&dev->power.wait_queue);
> +		pm_runtime_put_noidle(dev);
> +		rpm_idle(dev, RPM_NOWAIT);
> +	}
> +

It seems very illgical to do all the callback stuff here, after the
"switch" statement.  IMO it would make more sense to put it all
together, more or less as follows:

	case RPM_REQ_RESUME:
		if (dev->power.run_driver_work && dev->driver->pm_runtime_work) {
			driver_work = dev->driver->pm_runtime_work;
			dev->power.run_driver_work = false;
			dev->power.work_in_progress = true;
			pm_runtime_get_noresume(dev);
			rpm_resume(dev, 0);
			spin_unlock_irq(&dev->power.lock);

			driver_work(dev);

			spin_lock_irq(&dev->power.lock);
			dev->power.work_in_progress = false;
			wake_up_all(&dev->power.wait_queue);
			pm_runtime_put_noidle(dev);
			rpm_idle(dev, RPM_NOWAIT);
		} else {
			rpm_resume(dev, RPM_NOWAIT);
		}
		break;

Notice also that it's important to do the _get_noresume _before_
calling rpm_resume().  Otherwise the device might get suspended again
before the callback gets a chance to run.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ