Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1390694 > unrolled thread

RE: [PATCH v2 4/6] ACPI / osi: Fix default _OSI(Darwin) support

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  RE: [PATCH v2 4/6] ACPI / osi: Fix default _OSI(Darwin) support "Zheng, Lv" <lv.zheng@intel.com> - 2016-04-29 04:10 +0200
    Re: [PATCH v2 4/6] ACPI / osi: Fix default _OSI(Darwin) support Lukas Wunner <lukas@wunner.de> - 2016-04-29 15:10 +0200
      RE: [PATCH v2 4/6] ACPI / osi: Fix default _OSI(Darwin) support "Zheng, Lv" <lv.zheng@intel.com> - 2016-05-03 04:00 +0200

#1390694 — RE: [PATCH v2 4/6] ACPI / osi: Fix default _OSI(Darwin) support

Hi, Rafael

> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-
> owner@vger.kernel.org] On Behalf Of Zheng, Lv
> Subject: RE: [PATCH v2 4/6] ACPI / osi: Fix default _OSI(Darwin) support
> 

[Lv Zheng] 
Skip.

> > > > @@ -331,6 +336,24 @@ static struct dmi_system_id acpi_osi_dmi_table[]
> > > > __initdata = {
> > > >  		},
> > > >  	},
> > > >
> > > > +	/*
> > > > +	 * Enable _OSI("Darwin") for all apple platforms.
> > > > +	 */
> > > > +	{
> > > > +	.callback = dmi_enable_osi_darwin,
> > > > +	.ident = "Apple hardware",
> > > > +	.matches = {
> > > > +		     DMI_MATCH(DMI_SYS_VENDOR, "Apple INC."),
> > > > +		},
> > > > +	},
> > > > +	{
> > > > +	.callback = dmi_enable_osi_darwin,
> > > > +	.ident = "Apple hardware",
> > > > +	.matches = {
> > > > +		     DMI_MATCH(DMI_SYS_VENDOR, "Apple Computer, INC."),
> > > > +		},
> > > > +	},
> > > > +
> > > The  vendor id should be 'Apple Inc.' and 'Apple Computer, Inc.' instead.
> >
> > If this is the only problem with this patch, I can fix it up.  No need to resend.
> [Lv Zheng]
> This is the only problem.
> Thanks for the help.
> 

[Lv Zheng] 
I just sent UPDATE of PATCH 4/6 and PATCH 6/6 to the mailing list with this corrected.
I was hoping they could update patchwork content so that the Bugzilla reporters might use the updated patches for confirmation.
The Message-Id(s) of the 2 patches were kept as same as the old ones.

Thanks and best regards
-Lv

[toc] | [next] | [standalone]

#1391121

Hi Lv Zheng,

On Fri, Apr 29, 2016 at 02:07:53AM +0000, Zheng, Lv wrote:
> I just sent UPDATE of PATCH 4/6 and PATCH 6/6 to the mailing list with this
> corrected.
> I was hoping they could update patchwork content so that the Bugzilla
> reporters might use the updated patches for confirmation.
> The Message-Id(s) of the 2 patches were kept as same as the old ones.

As promised on Bugzilla I've tested v2 of this series (with the "INC"
manually fixed up) on a MacBookPro9,1.

I only reviewed the patches in a superficial fashion, but one issue I've
noticed is that

#define pr_fmt(fmt) "ACPI: " fmt

is missing in osi.c (patch [6/6]).

Without command line arguments, Linux responds yay to _OSI("Darwin")
and nothing else and the Thunderbolt controller is powered up.

With "acpi_osi=!Darwin", Linux responds nay to everything and the
Thunderbolt controller is powered down.

So it seems to work as intended. If you want me to test anything else
please let me know.

Thanks,

Lukas

[toc] | [prev] | [next] | [standalone]

#1392956

Hi, Lukas

First, thanks for the test. :)

> From: Lukas Wunner [mailto:lukas@wunner.de]
> Subject: Re: [PATCH v2 4/6] ACPI / osi: Fix default _OSI(Darwin) support
> 
> Hi Lv Zheng,
> 
> On Fri, Apr 29, 2016 at 02:07:53AM +0000, Zheng, Lv wrote:
> > I just sent UPDATE of PATCH 4/6 and PATCH 6/6 to the mailing list with this
> > corrected.
> > I was hoping they could update patchwork content so that the Bugzilla
> > reporters might use the updated patches for confirmation.
> > The Message-Id(s) of the 2 patches were kept as same as the old ones.
> 
> As promised on Bugzilla I've tested v2 of this series (with the "INC"
> manually fixed up) on a MacBookPro9,1.
> 
> I only reviewed the patches in a superficial fashion, but one issue I've
> noticed is that
> 
> #define pr_fmt(fmt) "ACPI: " fmt
> 
> is missing in osi.c (patch [6/6]).
[Lv Zheng] 

This is a bug of internal.h.
I've been suffering from this in several my patches.
And people working for ACPI subsystem have been suffering from this for decades...
Sometimes we have to use printk(KERN_xxx PREFIX) here while checkpatch.pl complains coding style issues.
You comment makes it a good chance to escalate this issue to the maintainers.

This is a bug of internal.h because:
1. checkpatch.pl enforces pr_xxx() instead of printk(KERN_xxx...)
2. pr_xxx() macro is meant to eliminate KERN_xxx prefixes.
3. kernel provides pr_fmt(fmt) to allow drivers to add their own prefixes when pr_xxx() macros are used.
4. internal.h defines PREFIX for all ACPI drivers but doesn't provide the mean for the drivers to hide this prefix in pr_xxx() macros.
So this becomes a trap where developers can easily make mistakes.

Concluded from the point 1, 2, 3, the following code is wrong:
In drivers/acpi/osl.c:
		printk(KERN_ERR PREFIX "Cannot map memory that high\n");
printk(KERN_ERR PREFIX should be replaced by pr_fmt(

Concluded from the point 2, 3, the following code is wrong:
In drivers/acpi/osl.c:
	pr_info("ACPI: static SSDT installation disabled\n");
		pr_debug(PREFIX "%s: map reset_reg status %d\n", __func__, rv);
The PREFIX should be handled by pr_fmt().

Concluded from the point 3, 4, the following code is wrong:
In drivers/acpi/tables.c:
#define pr_fmt(fmt) "ACPI: " fmt
This should be handled by internal.h by default.

So in fact, my patch is written correctly on top of the assumption that the PREFIX have already been handled by internal.h not osi.c...

IMO, there are several ways to fix this long term issue:
1. Fix it with post-override:
This might be done in internal.h
We may
#ifndef PREFIX
#define PREFIX	"ACPI: "
#endif
#undef pr_fmt
#define pr_fmt(fmt)	PREFIX fmt
This enforces ec.c to be changed to define PREFIX instead of pr_fmt() which is not developers friendly as developers are trained to use pre-override for pr_fmt().

2. Fix it using pre-override:
The above fix looks a little bit confusing because the pr_fmt() is meant to be defined before including <linux/printk.h>
In internal.h:
#ifndef pr_fmt
#define pr_fmt(fmt) "ACPI: " fmt
#endif

#include <linux/xxxx.h>
...

In drivers/acpi/xxx.c, making internal.h the first one included by these files, so that they can override pr_fmt.
For example, in ec.c:

#define pr_fmt(fmt) "ACPI: EC: " fmt
#include "internal.h"
#include <linux/xxx.h>

It's hard for me to make a decision.
So I probably still have to work with this code quality issue and refresh this patchset.
Possibly I will still make same mistakes in the future.
Anyway, this is a chance for me to add your "Reported-and-tested-by:". :-)

> 
> Without command line arguments, Linux responds yay to _OSI("Darwin")
> and nothing else and the Thunderbolt controller is powered up.
> 
> With "acpi_osi=!Darwin", Linux responds nay to everything and the
> Thunderbolt controller is powered down.
> 
> So it seems to work as intended. If you want me to test anything else
> please let me know.
[Lv Zheng] 
Sounds good.
Thanks for the test.

Thanks and best regards
-Lv

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web