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

[James Bottomley <James.Bottomley@suse.de>]

On Wed, 2010-06-09 at 17:27 +0200, Florian Mickler wrote:
> On Wed, 09 Jun 2010 08:38:39 -0400
> James Bottomley <James.Bottomley@suse.de> wrote:
> 
> > On Wed, 2010-06-09 at 11:46 +0200, Thomas Gleixner wrote:
> > > On Wed, 9 Jun 2010, florian@mickler.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? 

Because there are no current users that will trip the BUG_ON ... and we
want to keep it that way.  Code doesn't go into the kernel if it BUGs on
boot.

The point about failing hard for an abuse of a kernel API isn't to trap
current abusers because you fix those before you add it.  It's to
prevent future abuse.  If your kernel BUGs under test you tend to fix
the code, so it becomes impossible for anyone to add any users which
abuse the API in this fashion.

James