linux-kernel - Re: [PATCH 2/2] pm_qos: make update_request callable from interrupt context

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:	Wed, 9 Jun 2010 17:27:14 +0200
From:	Florian Mickler <florian@...kler.org>
To:	James Bottomley <James.Bottomley@...e.de>
Cc:	Thomas Gleixner <tglx@...utronix.de>,
	pm list <linux-pm@...ts.linux-foundation.org>,
	markgross@...gnar.org, mgross@...ux.intel.com,
	Frederic Weisbecker <fweisbec@...il.com>,
	Jonathan Corbet <corbet@....net>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] pm_qos: make update_request callable from interrupt
 context

On Wed, 09 Jun 2010 08:38:39 -0400
James Bottomley <James.Bottomley@...e.de> wrote:

> On Wed, 2010-06-09 at 11:46 +0200, Thomas Gleixner wrote:
> > On Wed, 9 Jun 2010, florian@...kler.org wrote:
> > 
> > > The pm_qos framework has to guarantee atomic notifications so that
> > > drivers can request and release constraints at all times while no races
> > > occur.
> > > 
> > > In order to avoid implementing a secondary notification chain in which
> > > listeners might sleep, we demand that every listener implements it's
> > > notification so that it can run in interrupt context. The listener is in
> > > a better position to know if races are harmful or not.
> > 
> > That breaks existing notifiers.
> 
> Right ... and we don't want to do that.  Which is why I think we just
> use blocking notifiers as they are but allow for creating atomic ones
> which may use atomic update sites.
> 
> This is the solution I have in my tree ... it preserves existing
> semantics because all the update and add sites are in user context, but
> it allows for notifiers with purely atomic semantics and will do a
> runtime warn if anyone tries to use them in a blocking fashion (or if
> anyone adds an atomic update to an existing blocking notifier).
> 
> James


> 
> @@ -302,8 +330,12 @@ int pm_qos_add_notifier(int pm_qos_class, struct notifier_block *notifier)
>  {
>  	int retval;
>  
> +	/* someone tried to register a blocking notifier to a
> +	 * qos object that only supports atomic ones */
> +	BUG_ON(!pm_qos_array[pm_qos_class]->blocking_notifiers);
> +
>  	retval = blocking_notifier_chain_register(
> -			pm_qos_array[pm_qos_class]->notifiers, notifier);
> +			pm_qos_array[pm_qos_class]->blocking_notifiers, notifier);
>  
>  	return retval;
>  }

Why not:

	retval = 1;
	if(pm_qos_array[pm_qos_class]->blocking_notifiers) 
		retval = blocking_notifier_chain_register(..
	else 
		WARN();
	return retval;

That way, the offending programmer could eventually fix it, without
having to reboot? 

> @@ -319,15 +351,41 @@ EXPORT_SYMBOL_GPL(pm_qos_add_notifier);


The rest looks good to me. I posted another variant using
schedule_work(). 

As currently no atomic notifications are needed and critical operations
probably have to check pm_qos_get_request manually anyway to be shure
this would be an alternative. Whatever. 

Cheers,
Flo
--
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/