linux-kernel - Re: [PATCH 1/2] acpi: video: add function to support unregister backlight

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1367227167.22858.42.camel@linux-s257.site>
Date:	Mon, 29 Apr 2013 17:19:27 +0800
From:	joeyli <jlee@...e.com>
To:	Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc:	mjg@...hat.com, rjw@...k.pl, platform-driver-x86@...r.kernel.org,
	linux-acpi@...r.kernel.org, linux-kernel@...r.kernel.org,
	Carlos Corbacho <carlos@...angeworlds.co.uk>,
	Corentin Chary <corentincj@...aif.net>,
	Aaron Lu <aaron.lu@...el.com>, Thomas Renninger <trenn@...e.de>
Subject: Re: [PATCH 1/2] acpi: video: add function to support unregister
 backlight

Hi Dmitry, 

Thanks for your review and suggestions first!

於 一，2013-04-22 於 21:12 -0700，Dmitry Torokhov 提到：
> On Mon, Apr 22, 2013 at 08:39:15PM +0800, Chun-Yi Lee wrote:
> > From: "Lee, Chun-Yi" <jlee@...e.com>
> > +static acpi_status
> > +find_video_unregister_backlight(acpi_handle handle, u32 lvl, void *context,
> > +				void **rv)
> > +{
> > +	struct acpi_device *acpi_dev;
> > +	struct acpi_video_bus *video = NULL;
> 
> Gratuitous initialization of local variables prevents compiler from
> warning when you using variable uninitialized.

Yes, I agreed should not put gratuitous initialization, I will remove it
in v2 patch.

> 
> > +	struct acpi_video_device *dev, *next;
> > +
> > +	if (acpi_bus_get_device(handle, &acpi_dev))
> > +		return AE_OK;
> > +
> > +	if (!acpi_match_device_ids(acpi_dev, video_device_ids)) {
> > +		video = acpi_driver_data(acpi_dev);
> > +		acpi_video_bus_stop_devices(video);
> > +		mutex_lock(&video->device_list_lock);
> > +		list_for_each_entry_safe(dev, next, &video->video_device_list,
> > +					entry) {
> > +			if (dev->backlight) {
> > +				backlight_device_unregister(dev->backlight);
> > +				dev->backlight = NULL;
> > +				kfree(dev->brightness->levels);
> > +				kfree(dev->brightness);
> > +			}
> > +		}
> > +		mutex_unlock(&video->device_list_lock);
> > +		acpi_video_bus_start_devices(video);
> > +	}
> > +	return AE_OK;
> > +}
> > +
> > +void acpi_video_backlight_unregister(void)
> > +{
> > +	if (!register_count) {
> 
> Locking? It looks like the rest of the driver ignores locking too...

Did you mean locking video_device_list? 

The acpi/video locks video_device_list when add, remove and notify acpi
video bus driver. It always do the mutex lock before control
video_device_list, so I also add lock when unregister all backlight of
devices.


Thanks a lot!
Joey Lee

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/