linux-kernel - Re: [PATCH 2/5] PM / Wakeirq: Add automated device wake IRQ handling

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]

Message-ID: <20150516015651.GA7493@saruman.tx.rr.com>
Date:	Fri, 15 May 2015 20:56:51 -0500
From:	Felipe Balbi <balbi@...com>
To:	Tony Lindgren <tony@...mide.com>
CC:	"Rafael J. Wysocki" <rjw@...ysocki.net>,
	Felipe Balbi <balbi@...com>,
	"Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
	Alan Stern <stern@...land.harvard.edu>,
	Andreas Fenkart <afenkart@...il.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Huiquan Zhong <huiquan.zhong@...el.com>,
	Kevin Hilman <khilman@...nel.org>, NeilBrown <neilb@...e.de>,
	Mika Westerberg <mika.westerberg@...ux.intel.com>,
	Nishanth Menon <nm@...com>,
	Peter Hurley <peter@...leysoftware.com>,
	Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
	Ulf Hansson <ulf.hansson@...aro.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	<linux-pm@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<linux-serial@...r.kernel.org>, <linux-omap@...r.kernel.org>
Subject: Re: [PATCH 2/5] PM / Wakeirq: Add automated device wake IRQ handling

Hi,

On Fri, May 15, 2015 at 03:25:13PM -0700, Tony Lindgren wrote:

<snip>

> diff --git a/drivers/base/power/wakeirq.c b/drivers/base/power/wakeirq.c
> new file mode 100644
> index 0000000..1125481
> --- /dev/null
> +++ b/drivers/base/power/wakeirq.c
> @@ -0,0 +1,276 @@
> +/*
> + * wakeirq.c - Device wakeirq helper functions
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> + * kind, whether express or implied; without even the implied warranty
> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/slab.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/pm_wakeirq.h>
> +
> +#include "power.h"
> +
> +/**
> + * dev_pm_attach_wake_irq - Attach device interrupt as a wake IRQ
> + * @dev: Device entry
> + * @irq: Device wake-up capable interrupt
> + * @wirq: Wake irq specific data
> + *
> + * Internal function to attach either a device IO interrupt or a
> + * dedicated wake-up interrupt as a wake IRQ.
> + */
> +static int dev_pm_attach_wake_irq(struct device *dev, int irq,
> +				  struct wake_irq *wirq)
> +{
> +	unsigned long flags;
> +	int err;
> +
> +	if (!dev || !wirq)
> +		return -EINVAL;
> +
> +	if (!dev->power.wakeup) {
> +		dev_err(dev, "forgot to call call device_init_wakeup?\n");
> +		return -EINVAL;
> +	}
> +
> +	spin_lock_irqsave(&dev->power.lock, flags);
> +	if (WARN_ON(dev->power.wakeirq)) {
> +		dev_err(dev, "wake irq already initialized\n");

these two can be combined if you can live with a WARN_ONCE() instead:

	if (dev_WARN_ONCE(dev, dev->power.wakeirq,
		"wake irq already initialized\n")) {
		spin_unlock_irqrestore(&dev->power.lock, flags);
		return -EEXIST;
	}

dev_WARN() needs to be fixed at some point to accept a "condition"
argument :s

But really, no strong feelings.

> +static irqreturn_t handle_threaded_wakeirq(int wakeirq, void *_wirq)
> +{
> +	struct wake_irq *wirq = _wirq;
> +
> +	/* We don't want RPM_ASYNC or RPM_NOWAIT here */
> +	return pm_runtime_resume(wirq->dev) ? IRQ_NONE : IRQ_HANDLED;

I wonder if you should add a pm_runtime_mark_last_busy() here. I guess
not after your previous patch ?

> diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
> index 7726200..7191519 100644
> --- a/drivers/base/power/wakeup.c
> +++ b/drivers/base/power/wakeup.c
> @@ -238,6 +239,100 @@ int device_wakeup_enable(struct device *dev)
>  }
>  EXPORT_SYMBOL_GPL(device_wakeup_enable);
>  
> +#ifdef CONFIG_PM_WAKEIRQ
> +
> +/**
> + * device_wakeup_attach_irq - Attach a wakeirq to a wakeup source
> + * @dev: Device to handle
> + * @irq: Device specific wakeirq entry

s/irq/wakeirq to match argument name below ?

> + * Attach a device specific wakeirq to the device specific
> + * wakeup source so the device wakeirq can be configured
> + * automatically for suspend and resume.
> + */
> +int device_wakeup_attach_irq(struct device *dev,
> +			     struct wake_irq *wakeirq)
> +{
> +	struct wakeup_source *ws;
> +	int ret = 0;
> +
> +	spin_lock_irq(&dev->power.lock);
> +	ws = dev->power.wakeup;
> +	if (!ws) {
> +		ret = -EINVAL;
> +		goto unlock;
> +	}
> +
> +	if (ws->wakeirq) {
> +		ret = -EEXIST;
> +		goto unlock;
> +	}
> +
> +	ws->wakeirq = wakeirq;
> +
> +unlock:
> +	spin_unlock_irq(&dev->power.lock);
> +
> +	return ret;
> +}

<snip>

-- 
balbi

Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)