| Message ID | 542A4949.2020208@intel.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
On Tuesday, September 30, 2014 02:10:17 PM Aaron Lu wrote: > The _DOD method lists which video output device is currently attached so > we should only care about them and ignore others. An user recently > reported that there are two acpi_video interfaces appeared on his system > and one of them doesn't work. From the acpidump, it is found that there > are more than one video output devices that have _BCM control method but > the _DOD lists only one of them. So this patch checks if the video output > device is in the _DOD list and will not create backlight device if it is > not in the list. Also, we consider the broken _DOD case(reflected by the > video->attached_count is 0) and do not change behaviour for those broken > _DOD systems. > > Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=84111 > Reported-and-tested-by: ntrrgc@gmail.com > Signed-off-by: Aaron Lu <aaron.lu@intel.com> This looks reasonable to me, but I'm a little afraid that it may break systems which forget to list valid interfaces in the _DOD. For this reason, I'm queuing this up for 3.18, but not for -stable. We can request a backport after a while when we're reasonably sure that there are no regressions resulting from this. > --- > drivers/acpi/video.c | 26 ++++++++++++++++++++++++++ > 1 file changed, 26 insertions(+) > > diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c > index 7285c7f9a935..807a88a0f394 100644 > --- a/drivers/acpi/video.c > +++ b/drivers/acpi/video.c > @@ -1154,6 +1154,23 @@ acpi_video_device_bind(struct acpi_video_bus *video, > } > } > > +static bool acpi_video_device_in_dod(struct acpi_video_device *device) > +{ > + struct acpi_video_bus *video = device->video; > + int i; > + > + /* If we have a broken _DOD, no need to test */ > + if (!video->attached_count) > + return true; > + > + for (i = 0; i < video->attached_count; i++) { > + if (video->attached_array[i].bind_info == device) > + return true; > + } > + > + return false; > +} > + > /* > * Arg: > * video : video bus device > @@ -1593,6 +1610,15 @@ static void acpi_video_dev_register_backlight(struct acpi_video_device *device) > static int count; > char *name; > > + /* > + * Do not create backlight device for video output > + * device that is not in the enumerated list. > + */ > + if (!acpi_video_device_in_dod(device)) { > + dev_dbg(&device->dev->dev, "not in _DOD list, ignore\n"); > + return; > + } > + > result = acpi_video_init_brightness(device); > if (result) > return; >
On 10/01/2014 04:18 AM, Rafael J. Wysocki wrote: > On Tuesday, September 30, 2014 02:10:17 PM Aaron Lu wrote: >> The _DOD method lists which video output device is currently attached so >> we should only care about them and ignore others. An user recently >> reported that there are two acpi_video interfaces appeared on his system >> and one of them doesn't work. From the acpidump, it is found that there >> are more than one video output devices that have _BCM control method but >> the _DOD lists only one of them. So this patch checks if the video output >> device is in the _DOD list and will not create backlight device if it is >> not in the list. Also, we consider the broken _DOD case(reflected by the >> video->attached_count is 0) and do not change behaviour for those broken >> _DOD systems. >> >> Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=84111 >> Reported-and-tested-by: ntrrgc@gmail.com >> Signed-off-by: Aaron Lu <aaron.lu@intel.com> > > This looks reasonable to me, but I'm a little afraid that it may break > systems which forget to list valid interfaces in the _DOD. Indeed, this is possible. > > For this reason, I'm queuing this up for 3.18, but not for -stable. Agree. > > We can request a backport after a while when we're reasonably sure that > there are no regressions resulting from this. Sounds good, thanks for taking care of this! Regards, Aaron > >> --- >> drivers/acpi/video.c | 26 ++++++++++++++++++++++++++ >> 1 file changed, 26 insertions(+) >> >> diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c >> index 7285c7f9a935..807a88a0f394 100644 >> --- a/drivers/acpi/video.c >> +++ b/drivers/acpi/video.c >> @@ -1154,6 +1154,23 @@ acpi_video_device_bind(struct acpi_video_bus *video, >> } >> } >> >> +static bool acpi_video_device_in_dod(struct acpi_video_device *device) >> +{ >> + struct acpi_video_bus *video = device->video; >> + int i; >> + >> + /* If we have a broken _DOD, no need to test */ >> + if (!video->attached_count) >> + return true; >> + >> + for (i = 0; i < video->attached_count; i++) { >> + if (video->attached_array[i].bind_info == device) >> + return true; >> + } >> + >> + return false; >> +} >> + >> /* >> * Arg: >> * video : video bus device >> @@ -1593,6 +1610,15 @@ static void acpi_video_dev_register_backlight(struct acpi_video_device *device) >> static int count; >> char *name; >> >> + /* >> + * Do not create backlight device for video output >> + * device that is not in the enumerated list. >> + */ >> + if (!acpi_video_device_in_dod(device)) { >> + dev_dbg(&device->dev->dev, "not in _DOD list, ignore\n"); >> + return; >> + } >> + >> result = acpi_video_init_brightness(device); >> if (result) >> return; >> > -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Oct 09, 2014 at 04:27:55PM +0800, Aaron Lu wrote: > On 10/01/2014 04:18 AM, Rafael J. Wysocki wrote: > > On Tuesday, September 30, 2014 02:10:17 PM Aaron Lu wrote: > >> The _DOD method lists which video output device is currently attached so > >> we should only care about them and ignore others. An user recently > >> reported that there are two acpi_video interfaces appeared on his system > >> and one of them doesn't work. From the acpidump, it is found that there > >> are more than one video output devices that have _BCM control method but > >> the _DOD lists only one of them. So this patch checks if the video output > >> device is in the _DOD list and will not create backlight device if it is > >> not in the list. Also, we consider the broken _DOD case(reflected by the > >> video->attached_count is 0) and do not change behaviour for those broken > >> _DOD systems. > >> > >> Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=84111 > >> Reported-and-tested-by: ntrrgc@gmail.com > >> Signed-off-by: Aaron Lu <aaron.lu@intel.com> > > > > This looks reasonable to me, but I'm a little afraid that it may break > > systems which forget to list valid interfaces in the _DOD. > > Indeed, this is possible. And indeed, there is a regression! My Dell Latituded E6410's backlight control no longer works after this commit, and I get messages like this instead: [ 57.214610] ACPI: Failed to switch the brightness If I revert this commit, my backlight controls work again. Also, I regain a cooling device (?) that was being ignored: [ 1.332682] acpi device:02: registered as cooling_device0 Do you need any additional info to handle the regression, or should we just revert the patch? Brian -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/28/2014 05:59 PM, Brian Norris wrote: > On Thu, Oct 09, 2014 at 04:27:55PM +0800, Aaron Lu wrote: >> On 10/01/2014 04:18 AM, Rafael J. Wysocki wrote: >>> On Tuesday, September 30, 2014 02:10:17 PM Aaron Lu wrote: >>>> The _DOD method lists which video output device is currently attached so >>>> we should only care about them and ignore others. An user recently >>>> reported that there are two acpi_video interfaces appeared on his system >>>> and one of them doesn't work. From the acpidump, it is found that there >>>> are more than one video output devices that have _BCM control method but >>>> the _DOD lists only one of them. So this patch checks if the video output >>>> device is in the _DOD list and will not create backlight device if it is >>>> not in the list. Also, we consider the broken _DOD case(reflected by the >>>> video->attached_count is 0) and do not change behaviour for those broken >>>> _DOD systems. >>>> >>>> Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=84111 >>>> Reported-and-tested-by: ntrrgc@gmail.com >>>> Signed-off-by: Aaron Lu <aaron.lu@intel.com> >>> >>> This looks reasonable to me, but I'm a little afraid that it may break >>> systems which forget to list valid interfaces in the _DOD. >> >> Indeed, this is possible. > > And indeed, there is a regression! My Dell Latituded E6410's backlight > control no longer works after this commit, and I get messages like this > instead: > > [ 57.214610] ACPI: Failed to switch the brightness > > If I revert this commit, my backlight controls work again. Also, I > regain a cooling device (?) that was being ignored: > > [ 1.332682] acpi device:02: registered as cooling_device0 > > Do you need any additional info to handle the regression, or should we > just revert the patch? Please attach acpidump, dmesg with video.dyndbg="module video +pft" in kernel cmdline, list the /sys/class/backlight with and without this commit, thanks. -Aaron > > Brian > -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c index 7285c7f9a935..807a88a0f394 100644 --- a/drivers/acpi/video.c +++ b/drivers/acpi/video.c @@ -1154,6 +1154,23 @@ acpi_video_device_bind(struct acpi_video_bus *video, } } +static bool acpi_video_device_in_dod(struct acpi_video_device *device) +{ + struct acpi_video_bus *video = device->video; + int i; + + /* If we have a broken _DOD, no need to test */ + if (!video->attached_count) + return true; + + for (i = 0; i < video->attached_count; i++) { + if (video->attached_array[i].bind_info == device) + return true; + } + + return false; +} + /* * Arg: * video : video bus device @@ -1593,6 +1610,15 @@ static void acpi_video_dev_register_backlight(struct acpi_video_device *device) static int count; char *name; + /* + * Do not create backlight device for video output + * device that is not in the enumerated list. + */ + if (!acpi_video_device_in_dod(device)) { + dev_dbg(&device->dev->dev, "not in _DOD list, ignore\n"); + return; + } + result = acpi_video_init_brightness(device); if (result) return;
The _DOD method lists which video output device is currently attached so we should only care about them and ignore others. An user recently reported that there are two acpi_video interfaces appeared on his system and one of them doesn't work. From the acpidump, it is found that there are more than one video output devices that have _BCM control method but the _DOD lists only one of them. So this patch checks if the video output device is in the _DOD list and will not create backlight device if it is not in the list. Also, we consider the broken _DOD case(reflected by the video->attached_count is 0) and do not change behaviour for those broken _DOD systems. Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=84111 Reported-and-tested-by: ntrrgc@gmail.com Signed-off-by: Aaron Lu <aaron.lu@intel.com> --- drivers/acpi/video.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+)