Re: hp_accel: do not call ACPI from invalid context

[Pavel Machek <pavel@suse.cz>]

On Tue 2009-01-13 14:17:21, Andrew Morton wrote:
> On Mon, 12 Jan 2009 11:31:55 +0100
> Pavel Machek <pavel@suse.cz> wrote:
> 
> > 
> > LED on HP notebooks is connected through ACPI. That unfortunately
> > means that it needs to be delayed by using schedule_work() to avoid
> > calling ACPI interpretter in invalid context. This patch fixes that.
> > 
> > ...
> >  
> > +/* Delayed LEDs infrastructure ------------------------------------ */ 
> > +
> > +/* Special LED class that can defer work */
> > +struct delayed_led_classdev {
> > +	struct led_classdev led_classdev;
> > +	struct work_struct work;
> > +	enum led_brightness new_brightness;
> > +
> > +	unsigned int led;		/* For driver */
> > +	void (*set_brightness)(struct delayed_led_classdev *data, enum led_brightness value);
> > +};
> > +
> > +static inline void delayed_set_status_worker(struct work_struct *work)
> > +{
> > +	struct delayed_led_classdev *data =
> > +			container_of(work, struct delayed_led_classdev, work);
> > +
> > +	data->set_brightness(data, data->new_brightness);
> > +}
> > +
> > +static inline void delayed_sysfs_set(struct led_classdev *led_cdev,
> > +			      enum led_brightness brightness)
> > +{
> > +	struct delayed_led_classdev *data = container_of(led_cdev,
> > +			     struct delayed_led_classdev, led_classdev);
> > +	data->new_brightness = brightness;
> > +	schedule_work(&data->work);
> > +}
> 
> Is this the only LED driver int he current and future history of the
> world which uses ACPI?
> 
> coz otherwise, this code might better live in LEDs core somewhere, so
> those other drivers can use it?

There are 2 other drivers (thinkpad_acpi and asus-something IIRC) that
want the same infrastructure. So I put that code in
include/linux/delayed-leds.h in my tree, but then I realized I'd need
to coordinate too many maintainers and just inlined it for the
merge. (We do not want to schedule from atomic, right?)

I already have a patch for thinkpad_acpi, but it needs a bit more
testing and perhaps some tweaks by maintainer. So yes, eventually
putting that into shared place is a plan.

> >  	if (ret) {
> > -		led_classdev_unregister(&hpled_led);
> > +		while (work_pending(&hpled_led.work))
> > +			schedule();
> 
> We have flush_work() for this?

I did not realize flush_work() exists/does what I need... sorry.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html