[PATCH] acpi: update win8 OSI blacklist

[PATCH] acpi: update win8 OSI blacklist

[Felipe Contreras]

More people have reported they need this for their machines to work
correctly.

https://bugzilla.kernel.org/show_bug.cgi?id=60682

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 drivers/acpi/blacklist.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/drivers/acpi/blacklist.c b/drivers/acpi/blacklist.c
index 9515f18..f37dec5 100644
--- a/drivers/acpi/blacklist.c
+++ b/drivers/acpi/blacklist.c
@@ -297,6 +297,54 @@ static struct dmi_system_id acpi_osi_dmi_table[] __initdata = {
 		     DMI_MATCH(DMI_PRODUCT_VERSION, "3259A2G"),
 		},
 	},
+	{
+	.callback = dmi_disable_osi_win8,
+	.ident = "ThinkPad Edge E530",
+	.matches = {
+		     DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
+		     DMI_MATCH(DMI_PRODUCT_VERSION, "3259CTO"),
+		},
+	},
+	{
+	.callback = dmi_disable_osi_win8,
+	.ident = "ThinkPad Edge E530",
+	.matches = {
+		     DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
+		     DMI_MATCH(DMI_PRODUCT_VERSION, "3259HJG"),
+		},
+	},
+	{
+	.callback = dmi_disable_osi_win8,
+	.ident = "Acer Aspire V5-573G",
+	.matches = {
+		     DMI_MATCH(DMI_SYS_VENDOR, "Acer Aspire"),
+		     DMI_MATCH(DMI_PRODUCT_VERSION, "V5-573G/Dazzle_HW"),
+		},
+	},
+	{
+	.callback = dmi_disable_osi_win8,
+	.ident = "Acer Aspire V5-572G",
+	.matches = {
+		     DMI_MATCH(DMI_SYS_VENDOR, "Acer Aspire"),
+		     DMI_MATCH(DMI_PRODUCT_VERSION, "V5-572G/Dazzle_CX"),
+		},
+	},
+	{
+	.callback = dmi_disable_osi_win8,
+	.ident = "ThinkPad T431s",
+	.matches = {
+		     DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
+		     DMI_MATCH(DMI_PRODUCT_VERSION, "20AACTO1WW"),
+		},
+	},
+	{
+	.callback = dmi_disable_osi_win8,
+	.ident = "ThinkPad T430",
+	.matches = {
+		     DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
+		     DMI_MATCH(DMI_PRODUCT_VERSION, "2349D15"),
+		},
+	},
 
 	/*
 	 * BIOS invocation of _OSI(Linux) is almost always a BIOS bug.
-- 
1.8.4-fc

Re: [PATCH] acpi: update win8 OSI blacklist

[Felipe Contreras]

On Thu, Oct 3, 2013 at 12:13 PM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> More people have reported they need this for their machines to work
> correctly.
>
> https://bugzilla.kernel.org/show_bug.cgi?id=60682

Reported-by: Stefan Hellermann <bugzilla.kernel.org@the2masters.de>
Reported-by: Benedikt Sauer <filmor@gmail.com>
Reported-by: Erno Kuusela <erno@iki.fi>
Reported-by: Jonathan Doman <jonathan.doman@gmail.com>
Reported-by: Christoph Klaffl <christophklaffl@gmail.com>
Reported-by: Jan Hendrik Nielsen <jan.hendrik.nielsen@informatik.hu-berlin.de>

> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>

-- 
Felipe Contreras

Re: [PATCH] acpi: update win8 OSI blacklist

[Felipe Contreras]

On Fri, Oct 18, 2013 at 5:43 AM, Stefan Hellermann
<stefan@the2masters.de> wrote:
> 2013/10/3 Felipe Contreras <felipe.contreras@gmail.com>
>>
>> On Thu, Oct 3, 2013 at 12:13 PM, Felipe Contreras
>> <felipe.contreras@gmail.com> wrote:
>> > More people have reported they need this for their machines to work
>> > correctly.
>> >
>> > https://bugzilla.kernel.org/show_bug.cgi?id=60682
>>
>> Reported-by: Stefan Hellermann <bugzilla.kernel.org@the2masters.de>
>
>
> That's not entirely correct! I reported bios version 2.52 is affected, but
> 2.54 is fixed. I think we should match all Lenovo 3259*, but only bios
> version < 2.54

As far as I know there's no way to do a match like that. So we have to
choose, either we apply the blacklist for all, or for none. Since
removing the win8 OSI doesn't cause any negative effects in 2.54, I
say we would do the former.

-- 
Felipe Contreras

Re: [PATCH] acpi: update win8 OSI blacklist

[Matthew Garrett]

On Thu, Oct 03, 2013 at 12:13:03PM -0500, Felipe Contreras wrote:
> More people have reported they need this for their machines to work
> correctly.

Can you add a comment to each indicating why they're being added so we 
can easily remove them again once the problem is fixed?

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] acpi: update win8 OSI blacklist

[Felipe Contreras]

On Fri, Oct 4, 2013 at 11:12 AM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> On Thu, Oct 03, 2013 at 12:13:03PM -0500, Felipe Contreras wrote:
>> More people have reported they need this for their machines to work
>> correctly.
>
> Can you add a comment to each indicating why they're being added so we
> can easily remove them again once the problem is fixed?

Because that's what users report:
https://bugzilla.kernel.org/show_bug.cgi?id=60682

-- 
Felipe Contreras

Re: [PATCH] acpi: update win8 OSI blacklist

[Matthew Garrett]

On Sun, Oct 06, 2013 at 03:29:10PM -0500, Felipe Contreras wrote:
> On Fri, Oct 4, 2013 at 11:12 AM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> > On Thu, Oct 03, 2013 at 12:13:03PM -0500, Felipe Contreras wrote:
> >> More people have reported they need this for their machines to work
> >> correctly.
> >
> > Can you add a comment to each indicating why they're being added so we
> > can easily remove them again once the problem is fixed?
> 
> Because that's what users report:
> https://bugzilla.kernel.org/show_bug.cgi?id=60682

Yes. For each entry, add a comment describing the specific bug seen.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] acpi: update win8 OSI blacklist

[Felipe Contreras]

On Sun, Oct 6, 2013 at 3:33 PM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> On Sun, Oct 06, 2013 at 03:29:10PM -0500, Felipe Contreras wrote:
>> On Fri, Oct 4, 2013 at 11:12 AM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
>> > On Thu, Oct 03, 2013 at 12:13:03PM -0500, Felipe Contreras wrote:
>> >> More people have reported they need this for their machines to work
>> >> correctly.
>> >
>> > Can you add a comment to each indicating why they're being added so we
>> > can easily remove them again once the problem is fixed?
>>
>> Because that's what users report:
>> https://bugzilla.kernel.org/show_bug.cgi?id=60682
>
> Yes. For each entry, add a comment describing the specific bug seen.

In case you didn't hear, the plan was to add an entry if a user
reports the backlight not working correctly, and acpi_osi="!Windows
2012" fixing it with no negative effects. and that's what users did in
bug #60682.

-- 
Felipe Contreras

Re: [PATCH] acpi: update win8 OSI blacklist

[Matthew Garrett]

On Sun, Oct 06, 2013 at 03:40:42PM -0500, Felipe Contreras wrote:

> In case you didn't hear, the plan was to add an entry if a user
> reports the backlight not working correctly, and acpi_osi="!Windows
> 2012" fixing it with no negative effects. and that's what users did in
> bug #60682.

So add that to each entry you're adding to the list.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] acpi: update win8 OSI blacklist

[Felipe Contreras]

On Sun, Oct 6, 2013 at 3:45 PM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> On Sun, Oct 06, 2013 at 03:40:42PM -0500, Felipe Contreras wrote:
>
>> In case you didn't hear, the plan was to add an entry if a user
>> reports the backlight not working correctly, and acpi_osi="!Windows
>> 2012" fixing it with no negative effects. and that's what users did in
>> bug #60682.
>
> So add that to each entry you're adding to the list.

No. These entries are already there, I'm merely updating it. I added a
comment before the list, but the comment was removed.

-- 
Felipe Contreras

Re: [PATCH] acpi: update win8 OSI blacklist

[Matthew Garrett]

On Sun, Oct 06, 2013 at 03:51:05PM -0500, Felipe Contreras wrote:
> On Sun, Oct 6, 2013 at 3:45 PM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> > On Sun, Oct 06, 2013 at 03:40:42PM -0500, Felipe Contreras wrote:
> >
> >> In case you didn't hear, the plan was to add an entry if a user
> >> reports the backlight not working correctly, and acpi_osi="!Windows
> >> 2012" fixing it with no negative effects. and that's what users did in
> >> bug #60682.
> >
> > So add that to each entry you're adding to the list.
> 
> No. These entries are already there, I'm merely updating it. I added a
> comment before the list, but the comment was removed.

There's no point in having a comment that describes a list, because 
someone will inevitably reorder them or insert things in between at some 
point. Add a comment per entry.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] acpi: update win8 OSI blacklist

[Felipe Contreras]

On Sun, Oct 6, 2013 at 3:59 PM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> On Sun, Oct 06, 2013 at 03:51:05PM -0500, Felipe Contreras wrote:
>> On Sun, Oct 6, 2013 at 3:45 PM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
>> > On Sun, Oct 06, 2013 at 03:40:42PM -0500, Felipe Contreras wrote:
>> >
>> >> In case you didn't hear, the plan was to add an entry if a user
>> >> reports the backlight not working correctly, and acpi_osi="!Windows
>> >> 2012" fixing it with no negative effects. and that's what users did in
>> >> bug #60682.
>> >
>> > So add that to each entry you're adding to the list.
>>
>> No. These entries are already there, I'm merely updating it. I added a
>> comment before the list, but the comment was removed.
>
> There's no point in having a comment that describes a list, because
> someone will inevitably reorder them or insert things in between at some
> point. Add a comment per entry.

>From acpi_osi_dmi_table:

/*
* BIOS invocation of _OSI(Linux) is almost always a BIOS bug.
* Linux ignores it, except for the machines enumerated below.
*/

-- 
Felipe Contreras

Re: [PATCH] acpi: update win8 OSI blacklist

[Matthew Garrett]

On Sun, Oct 06, 2013 at 06:27:28PM -0500, Felipe Contreras wrote:
> From acpi_osi_dmi_table:
> 
> /*
> * BIOS invocation of _OSI(Linux) is almost always a BIOS bug.
> * Linux ignores it, except for the machines enumerated below.
> */

Which was a mistake. We learn from mistakes rather than repeating them.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] acpi: update win8 OSI blacklist

[Felipe Contreras]

On Sun, Oct 6, 2013 at 6:31 PM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> On Sun, Oct 06, 2013 at 06:27:28PM -0500, Felipe Contreras wrote:
>> From acpi_osi_dmi_table:
>>
>> /*
>> * BIOS invocation of _OSI(Linux) is almost always a BIOS bug.
>> * Linux ignores it, except for the machines enumerated below.
>> */
>
> Which was a mistake. We learn from mistakes rather than repeating them.

According to you.

Sane human beings have no problem with divisions.

/* section 1 */

a();
b();
c();

/* section 2 */

d();
e();

/* section 3 */

f();

-- 
Felipe Contreras

Re: [PATCH] acpi: update win8 OSI blacklist

[Matthew Garrett]

On Sun, Oct 06, 2013 at 06:36:57PM -0500, Felipe Contreras wrote:
> On Sun, Oct 6, 2013 at 6:31 PM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> > On Sun, Oct 06, 2013 at 06:27:28PM -0500, Felipe Contreras wrote:
> >> From acpi_osi_dmi_table:
> >>
> >> /*
> >> * BIOS invocation of _OSI(Linux) is almost always a BIOS bug.
> >> * Linux ignores it, except for the machines enumerated below.
> >> */
> >
> > Which was a mistake. We learn from mistakes rather than repeating them.
> 
> According to you.

Cool. Look at that file and, without resorting to git blame, tell me 
why each of those entries is there. If your answer is "Just use git 
blame", then that's fine up until the point where someone reformats the 
list or decides to change the order and now it's still *possible* it's 
just really annoying, so why not just add the comments? They're cheap 
and you could have done it trivially in the time it's taken you to reply 
to this thread.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] acpi: update win8 OSI blacklist

[Felipe Contreras]

On Sun, Oct 6, 2013 at 6:57 PM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> On Sun, Oct 06, 2013 at 06:36:57PM -0500, Felipe Contreras wrote:
>> On Sun, Oct 6, 2013 at 6:31 PM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
>> > On Sun, Oct 06, 2013 at 06:27:28PM -0500, Felipe Contreras wrote:
>> >> From acpi_osi_dmi_table:
>> >>
>> >> /*
>> >> * BIOS invocation of _OSI(Linux) is almost always a BIOS bug.
>> >> * Linux ignores it, except for the machines enumerated below.
>> >> */
>> >
>> > Which was a mistake. We learn from mistakes rather than repeating them.
>>
>> According to you.
>
> Cool. Look at that file and, without resorting to git blame, tell me
> why each of those entries is there.

If my original comment was kept, I could tell you why the three
entries I added were there.

---
/*
* The following machines have broken backlight support when reporting
* the Windows 2012 OSI, so disable it until their support is fixed.
*/
{
.callback = dmi_disable_osi_win8,
.ident = "ASUS Zenbook Prime UX31A",
.matches = {
    DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
    DMI_MATCH(DMI_PRODUCT_NAME, "UX31A"),
},
},
{
.callback = dmi_disable_osi_win8,
.ident = "Dell Inspiron 15R SE",
.matches = {
    DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
    DMI_MATCH(DMI_PRODUCT_NAME, "Inspiron 7520"),
},
},
{
.callback = dmi_disable_osi_win8,
.ident = "Lenovo ThinkPad Edge E530",
.matches = {
    DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
    DMI_MATCH(DMI_PRODUCT_VERSION, "3259A2G"),
},
},

/*
* BIOS invocation of _OSI(Linux) is almost always a BIOS bug.
* Linux ignores it, except for the machines enumerated below.
*/
---

It is clear as day.

> If your answer is "Just use git
> blame", then that's fine up until the point where someone reformats the
> list or decides to change the order and now it's still *possible* it's
> just really annoying

That's not my answer.

> so why not just add the comments? They're cheap
> and you could have done it trivially in the time it's taken you to reply
> to this thread.

Because it's the wrong thing to do. Adding four lines of comments for
each one of the nine entries is a waste of code, it's completely
unnecessary, and doesn't bring any advantage that a single comment, on
top of the list doesn't.

This patch brings real benefits to real users, and does so without
introducing any problem to the format of this list that wasn't already
there. There is no reason not to apply it.

If _you_ want to add comments for each entry in the list you can do so
after this patch is applied.

-- 
Felipe Contreras

Re: [PATCH] acpi: update win8 OSI blacklist

[Matthew Garrett]

On Sun, Oct 06, 2013 at 07:27:48PM -0500, Felipe Contreras wrote:

> If _you_ want to add comments for each entry in the list you can do so
> after this patch is applied.

If you want to participate in a collaborative development effort you 
should pay attention to other people's concerns. I don't get the final 
say in whether or not this patch gets merged, but there's a decent 
chance that I'm going to be the one who has to remove the entries again 
once the backlight mess is fixed up. My life would be significantly 
easier if the entries are unambiguously identified in such a way that I 
can remove them without having to dig through git history to figure out 
where each came from. Is that really an unreasonable request?

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] acpi: update win8 OSI blacklist

[Felipe Contreras]

On Sun, Oct 6, 2013 at 7:32 PM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> On Sun, Oct 06, 2013 at 07:27:48PM -0500, Felipe Contreras wrote:
>
>> If _you_ want to add comments for each entry in the list you can do so
>> after this patch is applied.
>
> If you want to participate in a collaborative development effort you
> should pay attention to other people's concerns.

I did that when I listened to your comment, and I argued against it.

Disagreeing is not the same as not paying attention.

> I don't get the final
> say in whether or not this patch gets merged, but there's a decent
> chance that I'm going to be the one who has to remove the entries again
> once the backlight mess is fixed up. My life would be significantly
> easier if the entries are unambiguously identified in such a way that I
> can remove them without having to dig through git history to figure out
> where each came from.

And a *single* comment on top of this group entries achieves that just
fine. You haven't provided a single argument as to why that wouldn't
be the case.

In fact, you are the one that is not paying attention.

> Is that really an unreasonable request?

That wasn't a request, that was an explanation of what would make your
life easier.

And if uncommented entries is a problem for you, you already have that
problem, because the entries to remove are already there, uncommented.
The original patch I sent had a comment, so that's not my fault.

This patch would not make your life any harder, so that is a red
herring. The problem is already there.

-- 
Felipe Contreras

Re: [PATCH] acpi: update win8 OSI blacklist

[Matthew Garrett]

On Sun, Oct 06, 2013 at 07:50:18PM -0500, Felipe Contreras wrote:
> On Sun, Oct 6, 2013 at 7:32 PM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> > I don't get the final
> > say in whether or not this patch gets merged, but there's a decent
> > chance that I'm going to be the one who has to remove the entries again
> > once the backlight mess is fixed up. My life would be significantly
> > easier if the entries are unambiguously identified in such a way that I
> > can remove them without having to dig through git history to figure out
> > where each came from.
> 
> And a *single* comment on top of this group entries achieves that just
> fine. You haven't provided a single argument as to why that wouldn't
> be the case.

No, it demonstrably doesn't. The comments that do exist refer to only a 
subset of the entries underneath them. Having a per-entry comment is 
significantly clearer. Given that I have to delete things from this file 
and you don't, I have absolutely no idea why you refuse to believe me on 
this.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] acpi: update win8 OSI blacklist

[Felipe Contreras]

On Sun, Oct 6, 2013 at 7:53 PM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> On Sun, Oct 06, 2013 at 07:50:18PM -0500, Felipe Contreras wrote:
>> On Sun, Oct 6, 2013 at 7:32 PM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
>> > I don't get the final
>> > say in whether or not this patch gets merged, but there's a decent
>> > chance that I'm going to be the one who has to remove the entries again
>> > once the backlight mess is fixed up. My life would be significantly
>> > easier if the entries are unambiguously identified in such a way that I
>> > can remove them without having to dig through git history to figure out
>> > where each came from.
>>
>> And a *single* comment on top of this group entries achieves that just
>> fine. You haven't provided a single argument as to why that wouldn't
>> be the case.
>
> No, it demonstrably doesn't. The comments that do exist refer to only a
> subset of the entries underneath them.

That's not true.

/*
* BIOS invocation of _OSI(Linux) is almost always a BIOS bug.
* Linux ignores it, except for the machines enumerated below.
*/

> Having a per-entry comment is significantly clearer.

That is your opinion, it's not a demonstrable fact.

And just to be clear, you are saying that in the following code, you
have no idea which statements correspond to which sections. Am I
correct?

/* section 1 */

a();
b();
c();

/* section 2 */

d();
e();

/* section 3 */

f();

And once again, the problem with the **current** format of the list is
orthogonal to this patch.

-- 
Felipe Contreras

Re: [PATCH] acpi: update win8 OSI blacklist

[Matthew Garrett]

On Sun, Oct 06, 2013 at 08:01:34PM -0500, Felipe Contreras wrote:
> On Sun, Oct 6, 2013 at 7:53 PM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> > No, it demonstrably doesn't. The comments that do exist refer to only a
> > subset of the entries underneath them.
> 
> That's not true.
> 
> /*
> * BIOS invocation of _OSI(Linux) is almost always a BIOS bug.
> * Linux ignores it, except for the machines enumerated below.
> */

You appear to have missed the continuation of that comment directly 
underneath which lists a subset of the devices covered by the quirks.

> > Having a per-entry comment is significantly clearer.
> 
> That is your opinion, it's not a demonstrable fact.

Say one of the machines turns out to need the quirk for two different 
reasons. How do we document that? Look, how about you add the comments 
and I'll do a patch that adds documentation to the existing entries? I'm 
not asking you to make up for other people's past mistakes, I'm asking 
you not to perpetuate them.

> And just to be clear, you are saying that in the following code, you
> have no idea which statements correspond to which sections. Am I
> correct?

No, that's not what I'm saying. But I'm now going to a bar and drink 
instead of having to justify why *clearly documenting this code* is a 
worthwhile thing to do.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] acpi: update win8 OSI blacklist

[Theodore Ts'o]

On Mon, Oct 07, 2013 at 02:27:04AM +0100, Matthew Garrett wrote:
> > > Having a per-entry comment is significantly clearer.
> > 
> > That is your opinion, it's not a demonstrable fact.
> 
> Say one of the machines turns out to need the quirk for two different 
> reasons. How do we document that? Look, how about you add the comments 
> and I'll do a patch that adds documentation to the existing entries? I'm 
> not asking you to make up for other people's past mistakes, I'm asking 
> you not to perpetuate them.

Felipe,

I have to agree with Matthew here.  Lists have a way of getting messed
up.  If not in the upstream kernel, can we be sure that none of the
distribution maintainers might not respect the ordering?

How about doing something like this:

/*
 * [1] Busted brightness controls
 * [2] Attempted compatibility with ancient enterprise Linux kernel causes
 *        20% performance regression on upstream kernels
 * [3] Disables video card functionaity to be bug-for-bug compatible with
 *      Windows after attempted hobbling in the propietary driver
 *	was wored around, etc.
 * etc.
 */

Then individual entries can be annotated with comments indicating
[1][2], etc.

That way, if someone clever decides that they want to alphabetize the
entries, or we have so many exceptions due to incompetent BIOS
programmers, and some future developers decides that he or she needs
to implement a binary search to speedup lookups, or some such, we
won't need to worry about ordering-specific semantics getting smashed.

Cheers,

					- Ted

Re: [PATCH] acpi: update win8 OSI blacklist

[Felipe Contreras]

On Sun, Oct 6, 2013 at 8:54 PM, Theodore Ts'o <tytso@mit.edu> wrote:
> On Mon, Oct 07, 2013 at 02:27:04AM +0100, Matthew Garrett wrote:
>> > > Having a per-entry comment is significantly clearer.
>> >
>> > That is your opinion, it's not a demonstrable fact.
>>
>> Say one of the machines turns out to need the quirk for two different
>> reasons. How do we document that? Look, how about you add the comments
>> and I'll do a patch that adds documentation to the existing entries? I'm
>> not asking you to make up for other people's past mistakes, I'm asking
>> you not to perpetuate them.
>
> Felipe,
>
> I have to agree with Matthew here.  Lists have a way of getting messed
> up.  If not in the upstream kernel, can we be sure that none of the
> distribution maintainers might not respect the ordering?

That would be a problem for the distribution maintainers, wouldn't it?
And regardless of how we document the list, they can still mess it up.

> How about doing something like this:
>
> /*
>  * [1] Busted brightness controls
>  * [2] Attempted compatibility with ancient enterprise Linux kernel causes
>  *        20% performance regression on upstream kernels
>  * [3] Disables video card functionaity to be bug-for-bug compatible with
>  *      Windows after attempted hobbling in the propietary driver
>  *      was wored around, etc.
>  * etc.
>  */
>
> Then individual entries can be annotated with comments indicating
> [1][2], etc.

That would be better than Matthew's proposal, but it would make the
code less readable, for the same reason spaghetti code is not readable
(you have to jump back and forth to understand what's going on).

> That way, if someone clever decides that they want to alphabetize the
> entries, or we have so many exceptions due to incompetent BIOS
> programmers, and some future developers decides that he or she needs
> to implement a binary search to speedup lookups, or some such, we
> won't need to worry about ordering-specific semantics getting smashed.

How about we worry about hypothetical issues when they arise? (which
is probably going to be never).

Personally I think this is more than enough:
http://article.gmane.org/gmane.linux.acpi.devel/64243

-- 
Felipe Contreras

Re: [PATCH] acpi: update win8 OSI blacklist

[Felipe Contreras]

On Sun, Oct 6, 2013 at 8:27 PM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> On Sun, Oct 06, 2013 at 08:01:34PM -0500, Felipe Contreras wrote:
>> On Sun, Oct 6, 2013 at 7:53 PM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
>> > No, it demonstrably doesn't. The comments that do exist refer to only a
>> > subset of the entries underneath them.
>>
>> That's not true.
>>
>> /*
>> * BIOS invocation of _OSI(Linux) is almost always a BIOS bug.
>> * Linux ignores it, except for the machines enumerated below.
>> */
>
> You appear to have missed the continuation of that comment directly
> underneath which lists a subset of the devices covered by the quirks.

What of it? The comment I'm referring to applies to *ALL* the entries
below, not a subset of them. All the entries below use
dmi_enable_osi_linux().

>> > Having a per-entry comment is significantly clearer.
>>
>> That is your opinion, it's not a demonstrable fact.
>
> Say one of the machines turns out to need the quirk for two different
> reasons. How do we document that?

  /* 0) The following... disable Windows 2012 OSI */
  a
  b
  /* 1) This particular... whatever */
  c
  d
  /* 2) The following... enable OSI Linux */

Is it not clear that the comment 1) applies only to c? If it's not
clear for you we can reorder:

  /* 0) The following... disable Windows 2012 OSI */
  a
  b
  d
  /* 1) This particular... whatever */
  c
  /* 2) The following... enable OSI Linux */

> Look, how about you add the comments
> and I'll do a patch that adds documentation to the existing entries? I'm
> not asking you to make up for other people's past mistakes, I'm asking
> you not to perpetuate them.

I will consider that *after* your patch lands. In the meantime I still
maintain that a single comment is better, and I think my patch should
land instead:

http://article.gmane.org/gmane.linux.acpi.devel/64243

>> And just to be clear, you are saying that in the following code, you
>> have no idea which statements correspond to which sections. Am I
>> correct?
>
> No, that's not what I'm saying. But I'm now going to a bar and drink
> instead of having to justify why *clearly documenting this code* is a
> worthwhile thing to do.

This is a rhetorical trick, by "clearly documenting this code" you
actually mean "format it in exactly the way I want". My way of
documenting this code[1] is also clear.

Ultimately it doesn't matter, because the fixes for the Intel driver
are supposed to come soon, and this blacklist should be short-lived,
thus this list is not going to be reordered, moved, or will have the
need for secondary comments.

Look, how about you set aside your objection to this patch so it can
go forward and fix real issues for real users, and deal with the
comments that are already missing anyway later?

[1] http://article.gmane.org/gmane.linux.acpi.devel/64243

-- 
Felipe Contreras

Re: [PATCH] acpi: update win8 OSI blacklist

[Felipe Contreras]

On Thu, Oct 3, 2013 at 12:13 PM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> More people have reported they need this for their machines to work
> correctly.
>
> https://bugzilla.kernel.org/show_bug.cgi?id=60682

I see this was merged to the linux-next branch. Is there any reason
why it's not proposed for v3.12?

-- 
Felipe Contreras

Re: [PATCH] acpi: update win8 OSI blacklist

[Rafael J. Wysocki]

On Sunday, October 13, 2013 10:30:50 PM Felipe Contreras wrote:
> On Thu, Oct 3, 2013 at 12:13 PM, Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
> > More people have reported they need this for their machines to work
> > correctly.
> >
> > https://bugzilla.kernel.org/show_bug.cgi?id=60682
> 
> I see this was merged to the linux-next branch. Is there any reason
> why it's not proposed for v3.12?

It is not a fix for a recent breakage, it doesn't fix an outright crash or
security issue and there is a chance, albeit arguably very small, that
something will not function on one of the affected systems for someone
as a result of the blacklisting.

Thanks!

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH] acpi: update win8 OSI blacklist

[Felipe Contreras]

On Mon, Oct 14, 2013 at 6:58 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Sunday, October 13, 2013 10:30:50 PM Felipe Contreras wrote:
>> On Thu, Oct 3, 2013 at 12:13 PM, Felipe Contreras
>> <felipe.contreras@gmail.com> wrote:
>> > More people have reported they need this for their machines to work
>> > correctly.
>> >
>> > https://bugzilla.kernel.org/show_bug.cgi?id=60682
>>
>> I see this was merged to the linux-next branch. Is there any reason
>> why it's not proposed for v3.12?
>
> It is not a fix for a recent breakage, it doesn't fix an outright crash or
> security issue and there is a chance, albeit arguably very small, that
> something will not function on one of the affected systems for someone
> as a result of the blacklisting.

So? That chance is already there because the blacklist is already
there, but we cannot know until v3.12 is released, because that's when
most people would be testing that code.

It seems to me your worry is that somebody would report a regression
that happened v3.12-rc6 (if it has this patch applied), while it
worked correctly in v3.12-rc5, in which case you would get a complaint
from Linus.

But we all know that's not going to happen. The complaints (if any)
would arrive in v3.12, and that would happen regardless of this patch.

Besides, our goal is to make v3.12 as stable as possible, and it seems
to me you are blindly following the letter of a rule, instead if
interpreting what is the intent of rule. If I had sent the patch the
29th of June, it would have been aplied by now and be part of the
current blacklist, and the result would be the same; we cannot really
know until v3.12 is released.

-- 
Felipe Contreras

Re: [PATCH] acpi: update win8 OSI blacklist

[Rafael J. Wysocki]

On Monday, October 14, 2013 08:14:28 AM Felipe Contreras wrote:
> On Mon, Oct 14, 2013 at 6:58 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Sunday, October 13, 2013 10:30:50 PM Felipe Contreras wrote:
> >> On Thu, Oct 3, 2013 at 12:13 PM, Felipe Contreras
> >> <felipe.contreras@gmail.com> wrote:
> >> > More people have reported they need this for their machines to work
> >> > correctly.
> >> >
> >> > https://bugzilla.kernel.org/show_bug.cgi?id=60682
> >>
> >> I see this was merged to the linux-next branch. Is there any reason
> >> why it's not proposed for v3.12?
> >
> > It is not a fix for a recent breakage, it doesn't fix an outright crash or
> > security issue and there is a chance, albeit arguably very small, that
> > something will not function on one of the affected systems for someone
> > as a result of the blacklisting.
> 
> So? That chance is already there because the blacklist is already
> there, but we cannot know until v3.12 is released, because that's when
> most people would be testing that code.
> 
> It seems to me your worry is that somebody would report a regression
> that happened v3.12-rc6 (if it has this patch applied), while it
> worked correctly in v3.12-rc5, in which case you would get a complaint
> from Linus.
> 
> But we all know that's not going to happen. The complaints (if any)
> would arrive in v3.12, and that would happen regardless of this patch.
> 
> Besides, our goal is to make v3.12 as stable as possible, and it seems
> to me you are blindly following the letter of a rule, instead if
> interpreting what is the intent of rule. If I had sent the patch the
> 29th of June, it would have been aplied by now and be part of the
> current blacklist, and the result would be the same; we cannot really
> know until v3.12 is released.

Oh, please give me a break.

I've applied your patch even though I really should have waited for an ACK from
Matthew before doing that, but that only made you *more* demanding.

I generally tend to be nice to people, but you evidently take that as a sign of
weakness, so trying to be nice to you doesn't really make sense to me.  On the
other hand, I'm not too good at throwing flames at people and it distracts me
so much that I can't really afford it.  Please find yourself someone else to
fight with.

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH] acpi: update win8 OSI blacklist

[Felipe Contreras]

On Mon, Oct 14, 2013 at 9:16 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Monday, October 14, 2013 08:14:28 AM Felipe Contreras wrote:
>> On Mon, Oct 14, 2013 at 6:58 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> > On Sunday, October 13, 2013 10:30:50 PM Felipe Contreras wrote:
>> >> On Thu, Oct 3, 2013 at 12:13 PM, Felipe Contreras
>> >> <felipe.contreras@gmail.com> wrote:
>> >> > More people have reported they need this for their machines to work
>> >> > correctly.
>> >> >
>> >> > https://bugzilla.kernel.org/show_bug.cgi?id=60682
>> >>
>> >> I see this was merged to the linux-next branch. Is there any reason
>> >> why it's not proposed for v3.12?
>> >
>> > It is not a fix for a recent breakage, it doesn't fix an outright crash or
>> > security issue and there is a chance, albeit arguably very small, that
>> > something will not function on one of the affected systems for someone
>> > as a result of the blacklisting.
>>
>> So? That chance is already there because the blacklist is already
>> there, but we cannot know until v3.12 is released, because that's when
>> most people would be testing that code.
>>
>> It seems to me your worry is that somebody would report a regression
>> that happened v3.12-rc6 (if it has this patch applied), while it
>> worked correctly in v3.12-rc5, in which case you would get a complaint
>> from Linus.
>>
>> But we all know that's not going to happen. The complaints (if any)
>> would arrive in v3.12, and that would happen regardless of this patch.
>>
>> Besides, our goal is to make v3.12 as stable as possible, and it seems
>> to me you are blindly following the letter of a rule, instead if
>> interpreting what is the intent of rule. If I had sent the patch the
>> 29th of June, it would have been aplied by now and be part of the
>> current blacklist, and the result would be the same; we cannot really
>> know until v3.12 is released.
>
> Oh, please give me a break.
>
> I've applied your patch even though I really should have waited for an ACK from
> Matthew before doing that, but that only made you *more* demanding.

I'm not demanding anything, I'm making a case.

> I generally tend to be nice to people, but you evidently take that as a sign of
> weakness, so trying to be nice to you doesn't really make sense to me.  On the
> other hand, I'm not too good at throwing flames at people and it distracts me
> so much that I can't really afford it.  Please find yourself someone else to
> fight with.

I don't care if you are nice to me, be nice, be aggressive, I do not care.

All I care is that we do what is best here, and since many people's
back-light has been broken for so many releases and we have a very
simple fix that we know it works, because v3.6 worked on these laptops
(and all the previous releases), we *know* this patch cannot make
things worst than v3.6, and we *know* we broke things since v3.7.

If you don't want to fix the backlight in these laptops for v3.12,
fine, but don't blame it on my attitude, your users don't care about
my attitude, your users only care that Linux works correctly, and as
things stand v3.12 will *not* work correctly for these laptops, even
though we know how to fix it, and the fix is simple, and cannot make
things worst than v3.6.

We are going to fix this regression only on certain laptops, but not
others, even though we already know the fix works for those too. I
don't see how that makes sense to you, but it's your call.

-- 
Felipe Contreras