On 10/10/09, Bjorn Helgaas <bjorn.helgaas@xxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
