Re: [PATCH] acpiphp extension for 2.6.7

[Vernon Mauery <vernux@us.ibm.com>]

On Thu, 2004-06-24 at 14:45, Greg KH wrote: 
> > +int acpiphp_register_attention_info(struct acpiphp_attention_info *info)
> > +{
> > +	int retval = 0;
> > +	unsigned long flags;
> > +
> > +	if (!info || !info->owner || !info->set_attn || !info->get_attn)
> > +		retval = -1;
> > +	else {
> > +		spin_lock_irqsave(&attn_info_lock, flags);
> 
> Why lock here?  What could race?

If this function is only ever called from a module's init_module
function, then our global data is protected by the kernel's
module_mutex.  But is the assumption that it is never called from other
code safe to make?  It manipulates global data, so it needs to be
protected somehow...

> And why the irqsave lock?
Not sure.  That can be changed.

> > -static int set_attention_status(struct hotplug_slot *hotplug_slot, u8 status)
> > +static int set_attention_status (struct hotplug_slot *hotplug_slot, u8 status)
> >  {
> > +	int retval = -1;
> > +	unsigned long flags;
> > +	struct acpiphp_attention_info info;
> > +
> >  	dbg("%s - physical_slot = %s\n", __FUNCTION__, hotplug_slot->name);
> >  
> > -	switch (status) {
> > -		case 0:
> > -			/* FIXME turn light off */
> > -			hotplug_slot->info->attention_status = 0;
> > -			break;
> > -
> > -		case 1:
> > -		default:
> > -			/* FIXME turn light on */
> > -			hotplug_slot->info->attention_status = 1;
> > -			break;
> > +	spin_lock_irqsave(&attn_info_lock, flags);
> > +	memcpy(&info, &attention_info, sizeof(struct acpiphp_attention_info));
> > +	spin_unlock_irqrestore(&attn_info_lock, flags);
> 
> Again, why lock?  And why copy the whole structure?  And it's on the
> stack, which isn't very nice.  Same comment applies to the get_
> function.

Getting a local copy of the data structure within the lock ensures that
this function is reentrant.  But if we can make the same guarantee that
this funtion is called only on module_exit (again protected by the
module_mutex) then we can move to a pointer and no local lock.

> > +
> > +	if (info.set_attn && try_module_get(info.owner)) {
> > +		retval = info.set_attn(hotplug_slot, status);
> > +		module_put(info.owner);
> >  	}
> >  
> > -	return 0;
> > +	if (!retval)
> > +		hotplug_slot->info->attention_status = (status) ? 1 : 0;
> 
> Why change the value based on the return value of the call?  This
> shouldn't be set at all.

Oops.  This was a snippet of legacy code that was around before I
figured out how to read the LED value from hardware.  I will drop it.

--Vernon