Re: assumptions in acpi drivers

Re: assumptions in acpi drivers

[Alan Jenkins]

>> @@ -315,9 +312,6 @@ static int lis3lv02d_add(struct acpi_device *device)
>>
>>  static int lis3lv02d_remove(struct acpi_device *device, int type)
>>  {
>> -	if (!device)
>> -		return -EINVAL;
>> -
>>  	lis3lv02d_joystick_disable();
>>  	lis3lv02d_poweroff(&lis3_dev);
>
> Same basic issue as the kfree comments below; the fact that this
> remove method doesn't reference "device" is a symptom of a driver
> structure problem (beyond the scope of this patch, obviously).
>

>> @@ -1306,9 +1303,6 @@ end:
>>
>>  static int asus_hotk_remove(struct acpi_device *device, int type)
>>  {
>> -	if (!device || !acpi_driver_data(device))
>> -		return -EINVAL;
>> -
>>  	kfree(hotk->name);
>>  	kfree(hotk);
>
> Separate issue: we should kfree acpi_driver_data(device) here,
> not "hotk".  In general, the "remove" should deal with per-device
> data, not globals.  This driver only deals with a single instance,
> so they happen to be the same in this case, but you have to read
> more of the driver to verify that.
>

>> @@ -1276,9 +1274,6 @@ fail_platform_driver:
>>
>>  static int eeepc_hotk_remove(struct acpi_device *device, int type)
>>  {
>> -	if (!device || !acpi_driver_data(device))
>> -		return -EINVAL;
>> -
>>  	eeepc_backlight_exit();
>>  	eeepc_rfkill_exit();
>>  	eeepc_input_exit();
>
> The current upstream version of this function also has the kfree
> issue.  You're patching a different version, so it might be fixed
> there already.  But it seems like all these hotkey drivers (and the
> accelerometer driver) use the same pattern of assuming they'll only
> see a single device, and then they make assumptions like "hotk ==
> acpi_driver_data(device)".  That's just a bad example for people
> to follow, and might not even be true for things like accelerometers
> where you could imagine having more than one.

I've been thinking about this.  Frankly, I can't imagine a machine
having multiple ACPI accelerometer devices with the same interface.
It doesn't make sense to have multiple instances of any of the drivers
under /platform/.  And many of them create platform devices with fixed
names, so we can't pretend it would work.

Perhaps the best approach is to remove the references to
device->driverdata, and consistently use "hotk" or equivalent.  The
_core_ acpi devices would always provide correct examples of how to
use driverdata.

But we do need to make sure we don't crash if some strange machine has
multiple matching acpi devices.  Something like this -

static atomic_t device_present;

add():
        if (atomic_inc_return(&device_present) != 1)
                 return -EBUSY;

remove():
        atomic_dec(&device_present);


Regards
Alan

Re: assumptions in acpi drivers

[Bjorn Helgaas]

On Sat, 2009-10-10 at 14:16 +0100, Alan Jenkins wrote:

> > The current upstream version of this function also has the kfree
> > issue.  You're patching a different version, so it might be fixed
> > there already.  But it seems like all these hotkey drivers (and the
> > accelerometer driver) use the same pattern of assuming they'll only
> > see a single device, and then they make assumptions like "hotk ==
> > acpi_driver_data(device)".  That's just a bad example for people
> > to follow, and might not even be true for things like accelerometers
> > where you could imagine having more than one.
> 
> I've been thinking about this.  Frankly, I can't imagine a machine
> having multiple ACPI accelerometer devices with the same interface.
> It doesn't make sense to have multiple instances of any of the drivers
> under /platform/.  And many of them create platform devices with fixed
> names, so we can't pretend it would work.
> 
> Perhaps the best approach is to remove the references to
> device->driverdata, and consistently use "hotk" or equivalent.  The
> _core_ acpi devices would always provide correct examples of how to
> use driverdata.

I think the best pattern is something like this:

    struct acpi_device *singleton;

    xxx_add(struct acpi_device *device, ...)
    {
	if (singleton)
		return -EINVAL;

	xxx_data = kzalloc(...);
	xxx_data->foo = 0;

	device->driver_data = xxx_data;
	singleton = xxx_data;
	return 0;
    }

    xxx_remove(struct acpi_device *device)
    {
	kfree(acpi_driver_data(device));
    }

That way the assumption that there's only a single device is confined to
the add() method.  That makes the rest of the driver easier to read and
verify because it looks like every other ACPI driver.

Bjorn

Re: assumptions in acpi drivers

[Alan Jenkins]

On 10/10/09, Bjorn Helgaas <bjorn.helgaas@hp.com> wrote:
> On Sat, 2009-10-10 at 14:16 +0100, Alan Jenkins wrote:
>
>> > The current upstream version of this function also has the kfree
>> > issue.  You're patching a different version, so it might be fixed
>> > there already.  But it seems like all these hotkey drivers (and the
>> > accelerometer driver) use the same pattern of assuming they'll only
>> > see a single device, and then they make assumptions like "hotk ==
>> > acpi_driver_data(device)".  That's just a bad example for people
>> > to follow, and might not even be true for things like accelerometers
>> > where you could imagine having more than one.
>>
>> I've been thinking about this.  Frankly, I can't imagine a machine
>> having multiple ACPI accelerometer devices with the same interface.
>> It doesn't make sense to have multiple instances of any of the drivers
>> under /platform/.  And many of them create platform devices with fixed
>> names, so we can't pretend it would work.
>>
>> Perhaps the best approach is to remove the references to
>> device->driverdata, and consistently use "hotk" or equivalent.  The
>> _core_ acpi devices would always provide correct examples of how to
>> use driverdata.
>
> I think the best pattern is something like this:
>
>     struct acpi_device *singleton;
>
>     xxx_add(struct acpi_device *device, ...)
>     {
> 	if (singleton)
> 		return -EINVAL;
>
> 	xxx_data = kzalloc(...);
> 	xxx_data->foo = 0;
>
> 	device->driver_data = xxx_data;
> 	singleton = xxx_data;
> 	return 0;
>     }
>
>     xxx_remove(struct acpi_device *device)
>     {
> 	kfree(acpi_driver_data(device));
>     }
>
> That way the assumption that there's only a single device is confined to
> the add() method.  That makes the rest of the driver easier to read and
> verify because it looks like every other ACPI driver.
>
> Bjorn

Ok, you've prodded me enough to try converting eeepc-laptop.

Doesn't the singleton pattern need to be thread-safe though?  I don't
see any global lock (or a driver-specific one) in device_bind_driver()
or acpi_bind_device().  That's why I suggested atomics earlier.

Regards
Alan

Re: assumptions in acpi drivers

[Bjorn Helgaas]

On Sun, 2009-10-11 at 14:39 +0100, Alan Jenkins wrote:
> On 10/10/09, Bjorn Helgaas <bjorn.helgaas@hp.com> wrote:
> > On Sat, 2009-10-10 at 14:16 +0100, Alan Jenkins wrote:
> >
> >> > The current upstream version of this function also has the kfree
> >> > issue.  You're patching a different version, so it might be fixed
> >> > there already.  But it seems like all these hotkey drivers (and the
> >> > accelerometer driver) use the same pattern of assuming they'll only
> >> > see a single device, and then they make assumptions like "hotk ==
> >> > acpi_driver_data(device)".  That's just a bad example for people
> >> > to follow, and might not even be true for things like accelerometers
> >> > where you could imagine having more than one.
> >>
> >> I've been thinking about this.  Frankly, I can't imagine a machine
> >> having multiple ACPI accelerometer devices with the same interface.
> >> It doesn't make sense to have multiple instances of any of the drivers
> >> under /platform/.  And many of them create platform devices with fixed
> >> names, so we can't pretend it would work.
> >>
> >> Perhaps the best approach is to remove the references to
> >> device->driverdata, and consistently use "hotk" or equivalent.  The
> >> _core_ acpi devices would always provide correct examples of how to
> >> use driverdata.
> >
> > I think the best pattern is something like this:
> >
> >     struct acpi_device *singleton;
> >
> >     xxx_add(struct acpi_device *device, ...)
> >     {
> > 	if (singleton)
> > 		return -EINVAL;
> >
> > 	xxx_data = kzalloc(...);
> > 	xxx_data->foo = 0;
> >
> > 	device->driver_data = xxx_data;
> > 	singleton = xxx_data;
> > 	return 0;
> >     }
> >
> >     xxx_remove(struct acpi_device *device)
> >     {
> > 	kfree(acpi_driver_data(device));
> >     }
> >
> > That way the assumption that there's only a single device is confined to
> > the add() method.  That makes the rest of the driver easier to read and
> > verify because it looks like every other ACPI driver.
> >
> > Bjorn
> 
> Ok, you've prodded me enough to try converting eeepc-laptop.
> 
> Doesn't the singleton pattern need to be thread-safe though?  I don't
> see any global lock (or a driver-specific one) in device_bind_driver()
> or acpi_bind_device().  That's why I suggested atomics earlier.

Ooh, you're right, using atomic_inc_return() is much better.  I don't
know whether it needs to be thread-safe or not, but it doesn't hurt, and
it's nicer in the sense that it doesn't leave the singleton pointer
lying around where people would be tempted to use it instead of using
acpi_driver_data(device).

Bjorn

Re: assumptions in acpi drivers

[Dmitry Torokhov]

On Sun, Oct 11, 2009 at 10:07:32PM -0600, Bjorn Helgaas wrote:
> On Sun, 2009-10-11 at 14:39 +0100, Alan Jenkins wrote:
> > On 10/10/09, Bjorn Helgaas <bjorn.helgaas@hp.com> wrote:
> > > On Sat, 2009-10-10 at 14:16 +0100, Alan Jenkins wrote:
> > >
> > >> > The current upstream version of this function also has the kfree
> > >> > issue.  You're patching a different version, so it might be fixed
> > >> > there already.  But it seems like all these hotkey drivers (and the
> > >> > accelerometer driver) use the same pattern of assuming they'll only
> > >> > see a single device, and then they make assumptions like "hotk ==
> > >> > acpi_driver_data(device)".  That's just a bad example for people
> > >> > to follow, and might not even be true for things like accelerometers
> > >> > where you could imagine having more than one.
> > >>
> > >> I've been thinking about this.  Frankly, I can't imagine a machine
> > >> having multiple ACPI accelerometer devices with the same interface.
> > >> It doesn't make sense to have multiple instances of any of the drivers
> > >> under /platform/.  And many of them create platform devices with fixed
> > >> names, so we can't pretend it would work.
> > >>
> > >> Perhaps the best approach is to remove the references to
> > >> device->driverdata, and consistently use "hotk" or equivalent.  The
> > >> _core_ acpi devices would always provide correct examples of how to
> > >> use driverdata.
> > >
> > > I think the best pattern is something like this:
> > >
> > >     struct acpi_device *singleton;
> > >
> > >     xxx_add(struct acpi_device *device, ...)
> > >     {
> > > 	if (singleton)
> > > 		return -EINVAL;
> > >
> > > 	xxx_data = kzalloc(...);
> > > 	xxx_data->foo = 0;
> > >
> > > 	device->driver_data = xxx_data;
> > > 	singleton = xxx_data;
> > > 	return 0;
> > >     }
> > >
> > >     xxx_remove(struct acpi_device *device)
> > >     {
> > > 	kfree(acpi_driver_data(device));
> > >     }
> > >
> > > That way the assumption that there's only a single device is confined to
> > > the add() method.  That makes the rest of the driver easier to read and
> > > verify because it looks like every other ACPI driver.
> > >
> > > Bjorn
> > 
> > Ok, you've prodded me enough to try converting eeepc-laptop.
> > 
> > Doesn't the singleton pattern need to be thread-safe though?  I don't
> > see any global lock (or a driver-specific one) in device_bind_driver()
> > or acpi_bind_device().  That's why I suggested atomics earlier.
> 
> Ooh, you're right, using atomic_inc_return() is much better.  I don't
> know whether it needs to be thread-safe or not, but it doesn't hurt, and
> it's nicer in the sense that it doesn't leave the singleton pointer
> lying around where people would be tempted to use it instead of using
> acpi_driver_data(device).

A small note: while it might not be an issue for ACPI in general drivers
can be detached from devices via sysfs bind/unbind attributes and so if
using this singleton model xxx_remove() should take care of decrementing
the counter and xxx_add() should do the same in error path.

-- 
Dmitry

Re: assumptions in acpi drivers

[Alan Jenkins]

On 10/16/09, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> On Sun, Oct 11, 2009 at 10:07:32PM -0600, Bjorn Helgaas wrote:

>> Ooh, you're right, using atomic_inc_return() is much better.  I don't
>> know whether it needs to be thread-safe or not, but it doesn't hurt, and
>> it's nicer in the sense that it doesn't leave the singleton pointer
>> lying around where people would be tempted to use it instead of using
>> acpi_driver_data(device).
>
> A small note: while it might not be an issue for ACPI in general drivers
> can be detached from devices via sysfs bind/unbind attributes and so if
> using this singleton model xxx_remove() should take care of decrementing
> the counter and xxx_add() should do the same in error path.

I'm sure it does apply to ACPI drivers.  I'll make sure I do that, thanks!

Alan