RE: [PATCH v3 2/2] dmi-id: add dmi/id/oem group for exporting oem strings to sysfs

[Mario_Limonciello]

Jean,

> -Original Message-
> From: Jean Delvare [mailto:jdelv...@suse.de]
> Sent: Thursday, September 1, 2016 1:01 PM
> To: Limonciello, Mario 
> Cc: gre...@linuxfoundation.org; Hung, Allen ;
> rmk+ker...@arm.linux.org.uk; so...@cmu.edu; jens.wiklan...@linaro.org;
> agr...@codeaurora.org; a...@arndb.de; sudeep.ho...@arm.com;
> e...@anholt.net; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v3 2/2] dmi-id: add dmi/id/oem group for exporting oem
> strings to sysfs
> 
> Hi Mario,
> 
> On Wed, 31 Aug 2016 21:51:22 +, mario_limoncie...@dell.com wrote:
> > > > Ah, yeah, just use dmidecode, much simpler, keeps the kernel smaller,
> I
> > > > like it.
> >
> > The main fundamental difference between kernel and userspace
> > will be that applications will need to run dmidecode multiple times
> > to get at all this data rather than read in a handful of files from sysfs.
> 
> That's correct, and I agree it is slightly less efficient. But the
> number of strings being small I don't think it's really a problem in
> practice.
> 
> > I'd like to ask more on the history of why *any* SMBIOS data was
> > exposed to sysfs in the first place rather than making all of
> > userspace do this same exercise of calling dmidecode to get at data?
> 
> If I recall properly it was introduced for kernel module auto-loading
> based on DMI data, through udev.
> Specifically /sys/devices/virtual/dmi/id/uevent was for this purpose.
> The other attributes must have been added because it was cheap at that
> point.

Yeah that's sorta what I was thinking it was for too.  Considering it was 
cheap to create those sysfs nodes without a clear standard consumer
is what was making me think that exposing OEM strings in kernel space 
made sense too.

> 
> I suppose it could have been implemented using dmidecode as well, but
> doing it in sysfs was more natural because this is where "real" devices
> are also declared. So I guess there was almost no code to add to udev
> to make it work.
> 
> > Why are the strings already exposed by the kernel in sysfs any more
> > valuable than OEM strings?
> 
> Because OEM strings are not standard so generic tools have no use for
> them. As a matter of fact the other attributes were added 9 years ago
> and only now someone (you) is asking for OEM strings.
> 
> > (...)
> > I applied your patch locally and looked a little bit at it.
> 
> Thanks for testing.

Sure

> 
> > The main downside I see from this approach versus what Allen did in
> > the kernel is you don't know in advance how many OEM strings will
> > exist.
> >
> > Allen's kernel approach you knew how many would be there by the
> > number of sysfs items that were created.  Your userspace approach I
> > can only really see working by trial and error based upon the
> > argument you give it.
> >
> > For example on a Precision 5510 I see 7 OEM strings, but on a T5810 I
> > only have 4.
> 
> My expectation was that whoever needs some OEM string would know its
> index as well. There is no description attached to each string anyway,
> so the index is the only key to figure out what is what. The vendor is
> responsible for getting it right (that is, be consistent where it
> matters.)
> 
> > Maybe one way to solve this would be if no arguments were given to
> > --oem-string return the number of OEM strings rather than an error.
> 
> Can you explain why you need to know? If I knew your use case I would
> feel more motivated to come up with a solution ;-)
> 
> (I am also not a fan of parameters with optional arguments in general,
> as this can make the command lines ambiguous, but this is an
> implementation detail really.)
> 

At least on Dell systems the number and contents of OEM strings is 
dynamic.  You won't be able to know in advance how many strings
will exist on a given box since some strings may only be present
based upon configuration settings.

The original kernel patch this wasn't a concern because you could
count number of sysfs files to determine how many strings were
present.

With the current PoC implementation of yours, an app would need
to just keep calling with monotonically increasing OEM string index
values until an empty output was returned to find the number of 
strings present.

> (...)
> > 2) There is testing for some invalid arguments, but if you put a
> > larger number than number of OEM strings no error is displayed.
> 
> That was on purpose. People kept complaining to me over the past years
> when option --string returned an error message. I finally "fixed" it for
> --string so I thought --oem-string should behave the same for
> consistency. The assumption is that the caller would test if it gets an
> empty string. Empty strings are not allowed in DMI data so if you get
> an empty string it means there was no string by that index.
> 
> But I can print an error message on invalid index if you think it makes
> sense, that's easy.
>

I don't know the context of those complaints, 

RE: [PATCH v3 2/2] dmi-id: add dmi/id/oem group for exporting oem strings to sysfs

[Mario_Limonciello]

Jean,

> -Original Message-
> From: Jean Delvare [mailto:jdelv...@suse.de]
> Sent: Thursday, September 1, 2016 1:01 PM
> To: Limonciello, Mario 
> Cc: gre...@linuxfoundation.org; Hung, Allen ;
> rmk+ker...@arm.linux.org.uk; so...@cmu.edu; jens.wiklan...@linaro.org;
> agr...@codeaurora.org; a...@arndb.de; sudeep.ho...@arm.com;
> e...@anholt.net; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v3 2/2] dmi-id: add dmi/id/oem group for exporting oem
> strings to sysfs
> 
> Hi Mario,
> 
> On Wed, 31 Aug 2016 21:51:22 +, mario_limoncie...@dell.com wrote:
> > > > Ah, yeah, just use dmidecode, much simpler, keeps the kernel smaller,
> I
> > > > like it.
> >
> > The main fundamental difference between kernel and userspace
> > will be that applications will need to run dmidecode multiple times
> > to get at all this data rather than read in a handful of files from sysfs.
> 
> That's correct, and I agree it is slightly less efficient. But the
> number of strings being small I don't think it's really a problem in
> practice.
> 
> > I'd like to ask more on the history of why *any* SMBIOS data was
> > exposed to sysfs in the first place rather than making all of
> > userspace do this same exercise of calling dmidecode to get at data?
> 
> If I recall properly it was introduced for kernel module auto-loading
> based on DMI data, through udev.
> Specifically /sys/devices/virtual/dmi/id/uevent was for this purpose.
> The other attributes must have been added because it was cheap at that
> point.

Yeah that's sorta what I was thinking it was for too.  Considering it was 
cheap to create those sysfs nodes without a clear standard consumer
is what was making me think that exposing OEM strings in kernel space 
made sense too.

> 
> I suppose it could have been implemented using dmidecode as well, but
> doing it in sysfs was more natural because this is where "real" devices
> are also declared. So I guess there was almost no code to add to udev
> to make it work.
> 
> > Why are the strings already exposed by the kernel in sysfs any more
> > valuable than OEM strings?
> 
> Because OEM strings are not standard so generic tools have no use for
> them. As a matter of fact the other attributes were added 9 years ago
> and only now someone (you) is asking for OEM strings.
> 
> > (...)
> > I applied your patch locally and looked a little bit at it.
> 
> Thanks for testing.

Sure

> 
> > The main downside I see from this approach versus what Allen did in
> > the kernel is you don't know in advance how many OEM strings will
> > exist.
> >
> > Allen's kernel approach you knew how many would be there by the
> > number of sysfs items that were created.  Your userspace approach I
> > can only really see working by trial and error based upon the
> > argument you give it.
> >
> > For example on a Precision 5510 I see 7 OEM strings, but on a T5810 I
> > only have 4.
> 
> My expectation was that whoever needs some OEM string would know its
> index as well. There is no description attached to each string anyway,
> so the index is the only key to figure out what is what. The vendor is
> responsible for getting it right (that is, be consistent where it
> matters.)
> 
> > Maybe one way to solve this would be if no arguments were given to
> > --oem-string return the number of OEM strings rather than an error.
> 
> Can you explain why you need to know? If I knew your use case I would
> feel more motivated to come up with a solution ;-)
> 
> (I am also not a fan of parameters with optional arguments in general,
> as this can make the command lines ambiguous, but this is an
> implementation detail really.)
> 

At least on Dell systems the number and contents of OEM strings is 
dynamic.  You won't be able to know in advance how many strings
will exist on a given box since some strings may only be present
based upon configuration settings.

The original kernel patch this wasn't a concern because you could
count number of sysfs files to determine how many strings were
present.

With the current PoC implementation of yours, an app would need
to just keep calling with monotonically increasing OEM string index
values until an empty output was returned to find the number of 
strings present.

> (...)
> > 2) There is testing for some invalid arguments, but if you put a
> > larger number than number of OEM strings no error is displayed.
> 
> That was on purpose. People kept complaining to me over the past years
> when option --string returned an error message. I finally "fixed" it for
> --string so I thought --oem-string should behave the same for
> consistency. The assumption is that the caller would test if it gets an
> empty string. Empty strings are not allowed in DMI data so if you get
> an empty string it means there was no string by that index.
> 
> But I can print an error message on invalid index if you think it makes
> sense, that's easy.
>

I don't know the context of those complaints, but as long as you return 
an error code from 

RE: [PATCH v3 2/2] dmi-id: add dmi/id/oem group for exporting oem strings to sysfs

[Mario_Limonciello]

Hi Jean,

> -Original Message-
> From: Jean Delvare [mailto:jdelv...@suse.de]
> Sent: Wednesday, August 31, 2016 10:48 AM
> To: Greg KH 
> Cc: Limonciello, Mario ; Hung, Allen
> ; rmk+ker...@arm.linux.org.uk; so...@cmu.edu;
> bjorn.anders...@sonymobile.com; jens.wiklan...@linaro.org;
> agr...@codeaurora.org; a...@arndb.de; sudeep.ho...@arm.com;
> e...@anholt.net; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v3 2/2] dmi-id: add dmi/id/oem group for exporting oem
> strings to sysfs
> 
> Hi all,
> 
> On Wed, 31 Aug 2016 16:43:26 +0200, Greg KH wrote:
> > On Wed, Aug 31, 2016 at 02:01:23PM +, mario_limoncie...@dell.com
> wrote:
> > > Jean Delvare would rather see this implemented in userspace
> dmidecode.
> > > Jean raised a concern in an earlier submission that this runs on every
> > > machine (https://lkml.org/lkml/2016/8/2/799).
> >
> > Ah, yeah, just use dmidecode, much simpler, keeps the kernel smaller, I
> > like it.
> 

The main fundamental difference between kernel and userspace
will be that applications will need to run dmidecode multiple times
to get at all this data rather than read in a handful of files from sysfs.

I'd like to ask more on the history of why *any* SMBIOS data was
exposed to sysfs in the first place rather than making all of
userspace do this same exercise of calling dmidecode to get at data?

Why are the strings already exposed by the kernel in sysfs any more 
valuable than OEM strings?

> I wrote a proof of concept patch for dmidecode before my vacation, I
> can't remember if I sent it out or not, so I guess it did not happen.
> Here it is:
> 
> From: Jean Delvare 
> Subject: dmidecode: New option --oem-string
> 
> Add a new option to extract OEM strings, like we already have for
> many other strings.
> 
> Signed-off-by: Jean Delvare 
> ---
>  dmidecode.c |7 +++
>  dmiopt.c|   33 +
>  2 files changed, 40 insertions(+)
> 
> --- dmidecode.orig/dmiopt.c   2015-10-01 08:41:43.533806256 +0200
> +++ dmidecode/dmiopt.c2016-08-05 10:32:44.907196966 +0200
> @@ -171,6 +171,10 @@ static const struct string_keyword opt_s
>   { "processor-frequency", 4, 0x16 }, /* dmi_processor_frequency()
> */
>  };
> 
> +/* This is a template, 3rd field is set at runtime. */
> +static struct string_keyword opt_oem_string_keyword =
> + { NULL, 11, 0x00 };
> +
>  static void print_opt_string_list(void)
>  {
>   unsigned int i;
> @@ -206,6 +210,29 @@ static int parse_opt_string(const char *
>   return -1;
>  }
> 
> +static int parse_opt_oem_string(const char *arg)
> +{
> + unsigned long val;
> + char *next;
> +
> + if (opt.string)
> + {
> + fprintf(stderr, "Only one string can be specified\n");
> + return -1;
> + }
> +
> + val = strtoul(arg, , 10);
> + if (next == arg || val <= 0x00 || val > 0xff)
> + {
> + fprintf(stderr, "Invalid OEM string number: %s\n", arg);
> + return -1;
> + }
> +
> + opt_oem_string_keyword.offset = val;
> + opt.string = _oem_string_keyword;
> + return 0;
> +}
> +
> 
>  /*
>   * Command line options handling
> @@ -225,6 +252,7 @@ int parse_command_line(int argc, char *
>   { "dump", no_argument, NULL, 'u' },
>   { "dump-bin", required_argument, NULL, 'B' },
>   { "from-dump", required_argument, NULL, 'F' },
> + { "oem-string", required_argument, NULL, 'O' },
>   { "no-sysfs", no_argument, NULL, 'S' },
>   { "version", no_argument, NULL, 'V' },
>   { NULL, 0, NULL, 0 }
> @@ -255,6 +283,11 @@ int parse_command_line(int argc, char *
>   return -1;
>   opt.flags |= FLAG_QUIET;
>   break;
> + case 'O':
> + if (parse_opt_oem_string(optarg) < 0)
> + return -1;
> + opt.flags |= FLAG_QUIET;
> + break;
>   case 't':
>   opt.type = parse_opt_type(opt.type,
> optarg);
>   if (opt.type == NULL)
> --- dmidecode.orig/dmidecode.c2016-07-22 10:26:50.190119889 +0200
> +++ dmidecode/dmidecode.c 2016-08-05 10:41:53.746645533 +0200
> @@ -4370,6 +4370,13 @@ static void dmi_table_string(const struc
>   int key;
>   u8 offset = opt.string->offset;
> 
> + if (opt.string->type == 11) /* OEM strings */
> + {
> + if (h->length >= 5 && offset <= data[4])
> + printf("%s\n", dmi_string(h, offset));
> + return;
> + }
> +
>   if (offset >= h->length)
>   return;
> 
> I know it's not a universal way to decide where to put the code, but
> note how it's 

RE: [PATCH v3 2/2] dmi-id: add dmi/id/oem group for exporting oem strings to sysfs

[Mario_Limonciello]

Hi Jean,

> -Original Message-
> From: Jean Delvare [mailto:jdelv...@suse.de]
> Sent: Wednesday, August 31, 2016 10:48 AM
> To: Greg KH 
> Cc: Limonciello, Mario ; Hung, Allen
> ; rmk+ker...@arm.linux.org.uk; so...@cmu.edu;
> bjorn.anders...@sonymobile.com; jens.wiklan...@linaro.org;
> agr...@codeaurora.org; a...@arndb.de; sudeep.ho...@arm.com;
> e...@anholt.net; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v3 2/2] dmi-id: add dmi/id/oem group for exporting oem
> strings to sysfs
> 
> Hi all,
> 
> On Wed, 31 Aug 2016 16:43:26 +0200, Greg KH wrote:
> > On Wed, Aug 31, 2016 at 02:01:23PM +, mario_limoncie...@dell.com
> wrote:
> > > Jean Delvare would rather see this implemented in userspace
> dmidecode.
> > > Jean raised a concern in an earlier submission that this runs on every
> > > machine (https://lkml.org/lkml/2016/8/2/799).
> >
> > Ah, yeah, just use dmidecode, much simpler, keeps the kernel smaller, I
> > like it.
> 

The main fundamental difference between kernel and userspace
will be that applications will need to run dmidecode multiple times
to get at all this data rather than read in a handful of files from sysfs.

I'd like to ask more on the history of why *any* SMBIOS data was
exposed to sysfs in the first place rather than making all of
userspace do this same exercise of calling dmidecode to get at data?

Why are the strings already exposed by the kernel in sysfs any more 
valuable than OEM strings?

> I wrote a proof of concept patch for dmidecode before my vacation, I
> can't remember if I sent it out or not, so I guess it did not happen.
> Here it is:
> 
> From: Jean Delvare 
> Subject: dmidecode: New option --oem-string
> 
> Add a new option to extract OEM strings, like we already have for
> many other strings.
> 
> Signed-off-by: Jean Delvare 
> ---
>  dmidecode.c |7 +++
>  dmiopt.c|   33 +
>  2 files changed, 40 insertions(+)
> 
> --- dmidecode.orig/dmiopt.c   2015-10-01 08:41:43.533806256 +0200
> +++ dmidecode/dmiopt.c2016-08-05 10:32:44.907196966 +0200
> @@ -171,6 +171,10 @@ static const struct string_keyword opt_s
>   { "processor-frequency", 4, 0x16 }, /* dmi_processor_frequency()
> */
>  };
> 
> +/* This is a template, 3rd field is set at runtime. */
> +static struct string_keyword opt_oem_string_keyword =
> + { NULL, 11, 0x00 };
> +
>  static void print_opt_string_list(void)
>  {
>   unsigned int i;
> @@ -206,6 +210,29 @@ static int parse_opt_string(const char *
>   return -1;
>  }
> 
> +static int parse_opt_oem_string(const char *arg)
> +{
> + unsigned long val;
> + char *next;
> +
> + if (opt.string)
> + {
> + fprintf(stderr, "Only one string can be specified\n");
> + return -1;
> + }
> +
> + val = strtoul(arg, , 10);
> + if (next == arg || val <= 0x00 || val > 0xff)
> + {
> + fprintf(stderr, "Invalid OEM string number: %s\n", arg);
> + return -1;
> + }
> +
> + opt_oem_string_keyword.offset = val;
> + opt.string = _oem_string_keyword;
> + return 0;
> +}
> +
> 
>  /*
>   * Command line options handling
> @@ -225,6 +252,7 @@ int parse_command_line(int argc, char *
>   { "dump", no_argument, NULL, 'u' },
>   { "dump-bin", required_argument, NULL, 'B' },
>   { "from-dump", required_argument, NULL, 'F' },
> + { "oem-string", required_argument, NULL, 'O' },
>   { "no-sysfs", no_argument, NULL, 'S' },
>   { "version", no_argument, NULL, 'V' },
>   { NULL, 0, NULL, 0 }
> @@ -255,6 +283,11 @@ int parse_command_line(int argc, char *
>   return -1;
>   opt.flags |= FLAG_QUIET;
>   break;
> + case 'O':
> + if (parse_opt_oem_string(optarg) < 0)
> + return -1;
> + opt.flags |= FLAG_QUIET;
> + break;
>   case 't':
>   opt.type = parse_opt_type(opt.type,
> optarg);
>   if (opt.type == NULL)
> --- dmidecode.orig/dmidecode.c2016-07-22 10:26:50.190119889 +0200
> +++ dmidecode/dmidecode.c 2016-08-05 10:41:53.746645533 +0200
> @@ -4370,6 +4370,13 @@ static void dmi_table_string(const struc
>   int key;
>   u8 offset = opt.string->offset;
> 
> + if (opt.string->type == 11) /* OEM strings */
> + {
> + if (h->length >= 5 && offset <= data[4])
> + printf("%s\n", dmi_string(h, offset));
> + return;
> + }
> +
>   if (offset >= h->length)
>   return;
> 
> I know it's not a universal way to decide where to put the code, but
> note how it's half the side of your kernel-side implementation proposal.
> 

Thanks for doing that.  

I applied your patch 

RE: [PATCH v3 2/2] dmi-id: add dmi/id/oem group for exporting oem strings to sysfs

[Mario_Limonciello]

Hi Greg,

> -Original Message-
> From: Greg Kroah-Hartman [mailto:gre...@linuxfoundation.org]
> Sent: Wednesday, August 31, 2016 7:40 AM
> To: Hung, Allen 
> Cc: Jean Delvare ; Russell King
> ; Gabriel Somlo ; Bjorn
> Andersson ; Jens Wiklander
> ; Andy Gross ; Arnd
> Bergmann ; Sudeep Holla ; Eric
> Anholt ; linux-kernel@vger.kernel.org; Limonciello, Mario
> 
> Subject: Re: [PATCH v3 2/2] dmi-id: add dmi/id/oem group for exporting oem
> strings to sysfs
> 
> On Mon, Aug 15, 2016 at 05:22:05PM +0800, Allen Hung wrote:
> > The oem strings in DMI system identification information of the BIOS
> > have been parsed and stored as dmi devices in dmi_scan.c but they are
> > not exported to userspace via sysfs.
> >
> > The patch intends to export oem strings to sysfs device /sys/class/dmi/id.
> > As the number of oem strings are dynamic, a group "oem" is added to
> > the device and the strings will be added to the group as string1,
> > string2, ..., and stringN.
> >
> > Signed-off-by: Allen Hung 
> > ---
> >  drivers/firmware/Kconfig  |   9 
> >  drivers/firmware/dmi-id.c | 116
> > ++
> >  2 files changed, 125 insertions(+)
> >
> > diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig index
> > 6664f11..885a6c9 100644
> > --- a/drivers/firmware/Kconfig
> > +++ b/drivers/firmware/Kconfig
> > @@ -119,6 +119,15 @@ config DMIID
> >   information from userspace through /sys/class/dmi/id/ or if you want
> >   DMI-based module auto-loading.
> >
> > +config DMIID_OEM_STRINGS
> > +   bool "Export OEM strings in SMBIOS/DMI via sysfs to userspace"
> > +   depends on DMIID
> > +   default n
> > +   help
> > + Say Y here if you want to query OEM strings (as part of the 
> > information
> > + contained in SMBIOS/DMI system identification) from userspace
> through
> > + /sys/class/dmi/id/oem/.
> 
> Why wouldn't you want these?
> 

Jean Delvare would rather see this implemented in userspace dmidecode.
Jean raised a concern in an earlier submission that this runs on every
machine (https://lkml.org/lkml/2016/8/2/799).

> > +
> >  config DMI_SYSFS
> > tristate "DMI table support in sysfs"
> > depends on SYSFS && DMI
> 
> Shouldn't the new option, if you really want it, be below this one?
> 

Ah yes, I think so.  If this ends up being the right approach Allen
will need to adjust and resubmit it.

> But again, why not just always provide these values, if they are in the DMI
> tables, and you want sysfs DMI support?

>From our (Allen and myself) perspective this makes the most sense too,
Jean had pushed back on this, so Allen re-submitted as making it optional.

RE: [PATCH v3 2/2] dmi-id: add dmi/id/oem group for exporting oem strings to sysfs

[Mario_Limonciello]

Hi Greg,

> -Original Message-
> From: Greg Kroah-Hartman [mailto:gre...@linuxfoundation.org]
> Sent: Wednesday, August 31, 2016 7:40 AM
> To: Hung, Allen 
> Cc: Jean Delvare ; Russell King
> ; Gabriel Somlo ; Bjorn
> Andersson ; Jens Wiklander
> ; Andy Gross ; Arnd
> Bergmann ; Sudeep Holla ; Eric
> Anholt ; linux-kernel@vger.kernel.org; Limonciello, Mario
> 
> Subject: Re: [PATCH v3 2/2] dmi-id: add dmi/id/oem group for exporting oem
> strings to sysfs
> 
> On Mon, Aug 15, 2016 at 05:22:05PM +0800, Allen Hung wrote:
> > The oem strings in DMI system identification information of the BIOS
> > have been parsed and stored as dmi devices in dmi_scan.c but they are
> > not exported to userspace via sysfs.
> >
> > The patch intends to export oem strings to sysfs device /sys/class/dmi/id.
> > As the number of oem strings are dynamic, a group "oem" is added to
> > the device and the strings will be added to the group as string1,
> > string2, ..., and stringN.
> >
> > Signed-off-by: Allen Hung 
> > ---
> >  drivers/firmware/Kconfig  |   9 
> >  drivers/firmware/dmi-id.c | 116
> > ++
> >  2 files changed, 125 insertions(+)
> >
> > diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig index
> > 6664f11..885a6c9 100644
> > --- a/drivers/firmware/Kconfig
> > +++ b/drivers/firmware/Kconfig
> > @@ -119,6 +119,15 @@ config DMIID
> >   information from userspace through /sys/class/dmi/id/ or if you want
> >   DMI-based module auto-loading.
> >
> > +config DMIID_OEM_STRINGS
> > +   bool "Export OEM strings in SMBIOS/DMI via sysfs to userspace"
> > +   depends on DMIID
> > +   default n
> > +   help
> > + Say Y here if you want to query OEM strings (as part of the 
> > information
> > + contained in SMBIOS/DMI system identification) from userspace
> through
> > + /sys/class/dmi/id/oem/.
> 
> Why wouldn't you want these?
> 

Jean Delvare would rather see this implemented in userspace dmidecode.
Jean raised a concern in an earlier submission that this runs on every
machine (https://lkml.org/lkml/2016/8/2/799).

> > +
> >  config DMI_SYSFS
> > tristate "DMI table support in sysfs"
> > depends on SYSFS && DMI
> 
> Shouldn't the new option, if you really want it, be below this one?
> 

Ah yes, I think so.  If this ends up being the right approach Allen
will need to adjust and resubmit it.

> But again, why not just always provide these values, if they are in the DMI
> tables, and you want sysfs DMI support?

>From our (Allen and myself) perspective this makes the most sense too,
Jean had pushed back on this, so Allen re-submitted as making it optional.

RE: [PATCH v2 0/5] Allow the trampoline to use EFI boot services RAM

[Mario_Limonciello]

> -Original Message-
> From: Andy Lutomirski [mailto:l...@amacapital.net]
> Sent: Wednesday, August 10, 2016 8:22 AM
> To: Ingo Molnar 
> Cc: Andy Lutomirski ; H. Peter Anvin ;
> X86 ML ; Limonciello, Mario
> ; Matthew Garrett ;
> Borislav Petkov ; Matt Fleming ; linux-
> ker...@vger.kernel.org
> Subject: Re: [PATCH v2 0/5] Allow the trampoline to use EFI boot services
> RAM
> 
> On Wed, Aug 10, 2016 at 5:28 AM, Ingo Molnar  wrote:
> >
> > * Andy Lutomirski  wrote:
> >
> >> As currently configured, my laptop cannot boot any existing kernel
> >> because the real mode trampoline can't be reserved.  The ranges in
> >> which it could live are rejected by the kernel: one is EFI boot
> >> services data and the other is above the EBDA.
> >
> > Ok, so I like this series - if Matt acks it I can apply it.
> >
> > How urgent is it? The 'laptop does not boot' aspect worries me - how
> frequently
> > are systems hit by this?
> 
> As far as I know, I'm the only affected user unless Mario has heard
> otherwise.  I pinged the Fedora kernel maintainers and no one else has
> reported this issue.  I think it needs a fully up-to-date BIOS on a
> particular laptop with a non-default BIOS setting enabled that runs
> Fedora (or maybe RHEL or CentOS) -- my best guess is that this is only
> triggered when using Red Hat / Fedora's patched GRUB, and I have no
> clue why.  I've checked, and the reported EFI memory map is different
> if I boot using Fedora's GRUB and if I boot exactly the same system
> off a live USB stick.
> 

I haven't heard otherwise.  I've asked around for reports of problems
like this, but nothing has come up.  
Keep in mind two it's just recently SGX is getting availability in Linux; it's 
not upstream yet.  It's also not default to on for any BIOS Dell ships today.

So admittedly, the particular configuration that caused failures for Andy 
isn't tested by anyone in Dell or our partners with Linux to my knowledge.


> >
> > The approach you chose looks sufficiently robust and straightforward to
> me, so it
> > ought to work fine even for x86/urgent - but we can phase it into efi/core
> as well
> > if Matt prefers that.
> >
> 
> I like x86/urgent because I like to be able to boot stock kernels :)
> 

I'd agree if possible, if this does turn out to be something systemic with the
perfect storm that Andy created on his box but eventually can reproduce in 
other places I would prefer that the kernel was on top of it.

RE: [PATCH v2 0/5] Allow the trampoline to use EFI boot services RAM

[Mario_Limonciello]

> -Original Message-
> From: Andy Lutomirski [mailto:l...@amacapital.net]
> Sent: Wednesday, August 10, 2016 8:22 AM
> To: Ingo Molnar 
> Cc: Andy Lutomirski ; H. Peter Anvin ;
> X86 ML ; Limonciello, Mario
> ; Matthew Garrett ;
> Borislav Petkov ; Matt Fleming ; linux-
> ker...@vger.kernel.org
> Subject: Re: [PATCH v2 0/5] Allow the trampoline to use EFI boot services
> RAM
> 
> On Wed, Aug 10, 2016 at 5:28 AM, Ingo Molnar  wrote:
> >
> > * Andy Lutomirski  wrote:
> >
> >> As currently configured, my laptop cannot boot any existing kernel
> >> because the real mode trampoline can't be reserved.  The ranges in
> >> which it could live are rejected by the kernel: one is EFI boot
> >> services data and the other is above the EBDA.
> >
> > Ok, so I like this series - if Matt acks it I can apply it.
> >
> > How urgent is it? The 'laptop does not boot' aspect worries me - how
> frequently
> > are systems hit by this?
> 
> As far as I know, I'm the only affected user unless Mario has heard
> otherwise.  I pinged the Fedora kernel maintainers and no one else has
> reported this issue.  I think it needs a fully up-to-date BIOS on a
> particular laptop with a non-default BIOS setting enabled that runs
> Fedora (or maybe RHEL or CentOS) -- my best guess is that this is only
> triggered when using Red Hat / Fedora's patched GRUB, and I have no
> clue why.  I've checked, and the reported EFI memory map is different
> if I boot using Fedora's GRUB and if I boot exactly the same system
> off a live USB stick.
> 

I haven't heard otherwise.  I've asked around for reports of problems
like this, but nothing has come up.  
Keep in mind two it's just recently SGX is getting availability in Linux; it's 
not upstream yet.  It's also not default to on for any BIOS Dell ships today.

So admittedly, the particular configuration that caused failures for Andy 
isn't tested by anyone in Dell or our partners with Linux to my knowledge.


> >
> > The approach you chose looks sufficiently robust and straightforward to
> me, so it
> > ought to work fine even for x86/urgent - but we can phase it into efi/core
> as well
> > if Matt prefers that.
> >
> 
> I like x86/urgent because I like to be able to boot stock kernels :)
> 

I'd agree if possible, if this does turn out to be something systemic with the
perfect storm that Andy created on his box but eventually can reproduce in 
other places I would prefer that the kernel was on top of it.

RE: [PATCH 2/2] dmi-id: add dmi/id/oem group for exporting oem strings to sysfs

[Mario_Limonciello]

> -Original Message-
> From: Jean Delvare [mailto:jdelv...@suse.de]
> Sent: Tuesday, August 2, 2016 8:43 AM
> To: Limonciello, Mario 
> Cc: Hung, Allen ; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 2/2] dmi-id: add dmi/id/oem group for exporting oem
> strings to sysfs
> 
> Hi Mario, Allen,
> 
> On Tue, 19 Jul 2016 14:47:57 +, mario_limoncie...@dell.com wrote:
> > Hi Jean,
> >
> > I worked with Allen on this concept, so I've got some comments below.
> >
> > > -Original Message-
> > > From: Jean Delvare [mailto:jdelv...@suse.de]
> > > Sent: Tuesday, July 19, 2016 4:03 AM
> > > To: Hung, Allen 
> > > Cc: Jean Delvare ; linux-kernel@vger.kernel.org;
> > > Limonciello, Mario 
> > > Subject: Re: [PATCH 2/2] dmi-id: add dmi/id/oem group for exporting
> oem
> > > strings to sysfs
> > >
> > > Hello Allen,
> > >
> > > On Thu, 14 Jul 2016 16:01:23 +0800, Allen Hung wrote:
> > > > The oem strings in DMI system identification information of the BIOS
> have
> > > > been parsed and stored as dmi devices in dmi_scan.c but they are not
> > > > exported to userspace via sysfs.
> > >
> > > They are intended for internal consumption by the kernel drivers.
> > >
> > > > The patch intends to export oem strings to sysfs device
> /sys/class/dmi/id.
> > > > As the number of oem strings are dynamic, a group "oem" is added to
> the
> > > > device and the strings will be added to the group as string1, string2, 
> > > > ...,
> > > > and stringN.
> > >
> > > What is the use case? You can already get these strings easily using
> > > dmidecode:
> > >
> > > # dmidecode -qt 11
> > > OEM Strings
> > >   String 1: Dell System
> > >   String 2: 1[05A4]
> > >   String 3: 3[1.0]
> > >   String 4: 12[www.dell.com]
> > >   String 5: 14[1]
> > >   String 6: 15[3]
> > >   String 7:
> > >
> > > If needed, a dedicated option could be added to dmidecode to extract
> > > specific OEM strings. Or existing option -s could be extended for that
> > > purpose.
> >
> > The main purpose was to be able to parse these easily from userspace
> > without needing dmidecode installed and handling its output
> > (with tools such as grep, sed, and awk).
> 
> As I just stated above: dmidecode could be extended to extract the oem
> strings directly if there is a need for it.
> 
> > For example in an initramfs, typically dmidecode isn't included, but there
> > is value to being able to make decisions on things related to the values of
> > those OEM strings.
> 
> dmidecode is not included because nobody needs it. If you need it, you
> can include it. 15 years ago, udev was not included in initramfs
> either. But we still decided that this stuff should be done in
> user-space and we wrote udev and added it to initramfs. It is in the
> nature of initramfs to evolve with new needs, and to only include what
> is needed on a given machine. mkinitrd/dracut checks the needs
> dynamically. Why would it not work in your case?
> 
> I would appreciate more details than "there is value..." I would like
> to hear about an actual use case.
> 
> > Instead this allows userspace to iterate the oem/ directory and directly
> > look at the values of these strings.
> 
> At the cost of code which will run at every boot, and kernel memory
> which will be used forever, on all systems. This is why I am reluctant.
> You don't put things in the kernel because this is the easiest way to
> fulfill your immediate need. You put things in the kernel because you
> absolutely have to, or at the very least because it is where it makes
> the most sense. At this point I am not convinced this is the case here.
> I see no reason why the same can't be implemented easily in user-space
> (dmidecode and dracut.)

Thanks, when you put it this way your reluctance makes a lot more sense.
I don't disagree that this could live in several different levels of the 
software
stack.

The main reason that we want to have OEM tags exported is to access one
particular OEM strings on Dell systems from userspace applications that should
run on Dell systems (not just the initramfs).

There is string  that indicates that the system is a Dell System.  Normally this
would be obvious from other SMBIOS strings (such as System Vendor)
but Dell also does "OEM systems", which means that they might have 
custom branding applied that has otherwise removed the Vendor and
Product information.

Other strings indicate information that can be used to determine the
original product model number and lots of other details.

On a system like this it's not possible to know it's a Dell system and what
model it was before rebranding without looking at the OEM strings.

So by exporting the OEM strings from sysfs, it's possible to accurately 
identify Dell systems regardless of whether they have custom branding
applied without needing to rely on calling dmidecode.

RE: [PATCH 2/2] dmi-id: add dmi/id/oem group for exporting oem strings to sysfs

[Mario_Limonciello]

> -Original Message-
> From: Jean Delvare [mailto:jdelv...@suse.de]
> Sent: Tuesday, August 2, 2016 8:43 AM
> To: Limonciello, Mario 
> Cc: Hung, Allen ; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 2/2] dmi-id: add dmi/id/oem group for exporting oem
> strings to sysfs
> 
> Hi Mario, Allen,
> 
> On Tue, 19 Jul 2016 14:47:57 +, mario_limoncie...@dell.com wrote:
> > Hi Jean,
> >
> > I worked with Allen on this concept, so I've got some comments below.
> >
> > > -Original Message-
> > > From: Jean Delvare [mailto:jdelv...@suse.de]
> > > Sent: Tuesday, July 19, 2016 4:03 AM
> > > To: Hung, Allen 
> > > Cc: Jean Delvare ; linux-kernel@vger.kernel.org;
> > > Limonciello, Mario 
> > > Subject: Re: [PATCH 2/2] dmi-id: add dmi/id/oem group for exporting
> oem
> > > strings to sysfs
> > >
> > > Hello Allen,
> > >
> > > On Thu, 14 Jul 2016 16:01:23 +0800, Allen Hung wrote:
> > > > The oem strings in DMI system identification information of the BIOS
> have
> > > > been parsed and stored as dmi devices in dmi_scan.c but they are not
> > > > exported to userspace via sysfs.
> > >
> > > They are intended for internal consumption by the kernel drivers.
> > >
> > > > The patch intends to export oem strings to sysfs device
> /sys/class/dmi/id.
> > > > As the number of oem strings are dynamic, a group "oem" is added to
> the
> > > > device and the strings will be added to the group as string1, string2, 
> > > > ...,
> > > > and stringN.
> > >
> > > What is the use case? You can already get these strings easily using
> > > dmidecode:
> > >
> > > # dmidecode -qt 11
> > > OEM Strings
> > >   String 1: Dell System
> > >   String 2: 1[05A4]
> > >   String 3: 3[1.0]
> > >   String 4: 12[www.dell.com]
> > >   String 5: 14[1]
> > >   String 6: 15[3]
> > >   String 7:
> > >
> > > If needed, a dedicated option could be added to dmidecode to extract
> > > specific OEM strings. Or existing option -s could be extended for that
> > > purpose.
> >
> > The main purpose was to be able to parse these easily from userspace
> > without needing dmidecode installed and handling its output
> > (with tools such as grep, sed, and awk).
> 
> As I just stated above: dmidecode could be extended to extract the oem
> strings directly if there is a need for it.
> 
> > For example in an initramfs, typically dmidecode isn't included, but there
> > is value to being able to make decisions on things related to the values of
> > those OEM strings.
> 
> dmidecode is not included because nobody needs it. If you need it, you
> can include it. 15 years ago, udev was not included in initramfs
> either. But we still decided that this stuff should be done in
> user-space and we wrote udev and added it to initramfs. It is in the
> nature of initramfs to evolve with new needs, and to only include what
> is needed on a given machine. mkinitrd/dracut checks the needs
> dynamically. Why would it not work in your case?
> 
> I would appreciate more details than "there is value..." I would like
> to hear about an actual use case.
> 
> > Instead this allows userspace to iterate the oem/ directory and directly
> > look at the values of these strings.
> 
> At the cost of code which will run at every boot, and kernel memory
> which will be used forever, on all systems. This is why I am reluctant.
> You don't put things in the kernel because this is the easiest way to
> fulfill your immediate need. You put things in the kernel because you
> absolutely have to, or at the very least because it is where it makes
> the most sense. At this point I am not convinced this is the case here.
> I see no reason why the same can't be implemented easily in user-space
> (dmidecode and dracut.)

Thanks, when you put it this way your reluctance makes a lot more sense.
I don't disagree that this could live in several different levels of the 
software
stack.

The main reason that we want to have OEM tags exported is to access one
particular OEM strings on Dell systems from userspace applications that should
run on Dell systems (not just the initramfs).

There is string  that indicates that the system is a Dell System.  Normally this
would be obvious from other SMBIOS strings (such as System Vendor)
but Dell also does "OEM systems", which means that they might have 
custom branding applied that has otherwise removed the Vendor and
Product information.

Other strings indicate information that can be used to determine the
original product model number and lots of other details.

On a system like this it's not possible to know it's a Dell system and what
model it was before rebranding without looking at the OEM strings.

So by exporting the OEM strings from sysfs, it's possible to accurately 
identify Dell systems regardless of whether they have custom branding
applied without needing to rely on calling dmidecode.

RE: [PATCH] dell-wmi: Add events created by Dell Rugged 2-in-1s

[Mario_Limonciello]

> -Original Message-
> From: Pali Rohár [mailto:pali.ro...@gmail.com]
> Sent: Thursday, July 28, 2016 2:23 PM
> To: Dmitry Torokhov 
> Cc: Limonciello, Mario ;
> dvh...@infradead.org; linux-kernel@vger.kernel.org; platform-driver-
> x...@vger.kernel.org; pa...@ucw.cz
> Subject: Re: [PATCH] dell-wmi: Add events created by Dell Rugged 2-in-1s
> 
> On Thursday 28 July 2016 20:49:30 Dmitry Torokhov wrote:
> > On Thu, Jul 28, 2016 at 03:52:11PM +, mario_limoncie...@dell.com
> > wrote:
> > > > - Even if something is a key/button/switch it does not
> > > > necessarily need to be sent through input subsystem or we may
> > > > want to wait until we add a new input event. This is because
> > > > vendors love to come up with "value add" features that require
> > > > vendor specific driver that noone ends up using. I mean, for the
> > > > "WiFi catcher" do you have any kind of numbers for the users of
> > > > the feature?
> > >
> > > Like I said, this particular feature hasn't been in use for quite a
> > > few generations. In its time it was deemed "beneficial" in that
> > > day because of the amount of time it took for waking up the
> > > machine only to determine there was no available wifi nearby. It's
> > > been superseded by other technology improvements.
> > >
> > > There are other types of "notification" only events that could be
> > > useful to userspace. I don't think they need to all be generally
> > > demonized with the connotation as a negative vendor specific value
> > > add, there are plenty of generic things that userspace could
> > > notify on
> >
> > Generic things should find a specific system they are part of (i.e.
> > wifi notifications -> rflill, wifi switching -> input, etc), it is
> > one-off that "sounded good" but are hard to discover by user and
> > nobody ends up using that I am demonizing.
> 
> User (as consumer) of applications should not discovering such parts.
> User just open/configure his favourite gnome/kde/unity/xface/...
> application. But developers of wifi/power/led applications must know
> where to find those events.
> 
> And I need to say, if I change wifi state via rfkill interface, I would
> expect that also rfkill interface provides me information about changes.
> If I change keyboard backlight level via LED interface, then I somehow
> expect that LED interface should be able to tell me information that it
> was changed (but that's not truth... yet).
> 
> > > that are essentially "killed" at the WMI driver today.
> > >
> > > Here's a few I find:
> > > "Keyboard illumination toggle"
> > > "Mic mute toggle"
> > > "ALS sensor toggle"
> > > " Rotation lock toggle"
> > > "Airplane mode toggle"
> >
> > These all seem valid key events (unless they are not requests to
> > execute said actions but rather notifiers of events already
> > happened).
> 
> Previously "Keyboard illumination toggle" event was translated to
> KEY_KBDILLUMTOGGLE, but it later after implementing dell::kbd_backlight
> LED driver (software control via /sys/ of keyboard backlight) it was
> changed to KE_IGNORE.
> 
> Reason was following: All userspace applications (KDE, Unity, upowerd)
> understand KEY_KBDILLUMTOGGLE input event as "key for toggling
> keyboard
> backlight level" was pressed. Their answer to that input key is to find
> *::kbd_backlight led device in /sys/class/leds and switch keyboard
> backlight level to next value.
> 
> Dell ACPI/firmware send "Keyboard illumination toggle" event every time
> when keyboard backlight level is changed (software via driver or by HW
> key press) and all above applications started infinite loop...
> 
> So there are difference between:
> 
> *) user pressed key with icon of "keyboard illumination", but hardware
> did nothing
> 
> *) hardware changed keyboard illumination level (either by its own ---
> e.g. because of long inactivity of user or user closed LID --- or as
> reaction of user request via special driver)
> 
> Months ago we agreed that if pressing key with icon "keyboard
> illumination" cause 1) emitting key press event (via ACPI/WMI) and also
> 2) hardware unconditionally change keyboard backlight level, then we
> should not send that key press to userspace (as it was already processed
> by hardware). Same is for hardware key which block WIFI (as userspace
> react on input KEY_RFKILL to block all rfkills).
> 
> But I agree that could somehow inform userspace about changes which
> hardware did. For rfkills we already support interface when rfkill
> device send to userspace all change events.
> 
> For keyboard backlight we do not have notify interface yet, but I
> proposed that LED subsystem could be extended, so select/poll syscalls
> could be used on "brightness" sysfs node (which is used for changing LED
> level).
> 
> > Are they "killed" because they are also delivered via i8042?
> 
> No, (at least on my machine they are sent via WMI).

It really depends upon the vendor's implementation and 

RE: [PATCH] dell-wmi: Add events created by Dell Rugged 2-in-1s

[Mario_Limonciello]

> -Original Message-
> From: Pali Rohár [mailto:pali.ro...@gmail.com]
> Sent: Thursday, July 28, 2016 2:23 PM
> To: Dmitry Torokhov 
> Cc: Limonciello, Mario ;
> dvh...@infradead.org; linux-kernel@vger.kernel.org; platform-driver-
> x...@vger.kernel.org; pa...@ucw.cz
> Subject: Re: [PATCH] dell-wmi: Add events created by Dell Rugged 2-in-1s
> 
> On Thursday 28 July 2016 20:49:30 Dmitry Torokhov wrote:
> > On Thu, Jul 28, 2016 at 03:52:11PM +, mario_limoncie...@dell.com
> > wrote:
> > > > - Even if something is a key/button/switch it does not
> > > > necessarily need to be sent through input subsystem or we may
> > > > want to wait until we add a new input event. This is because
> > > > vendors love to come up with "value add" features that require
> > > > vendor specific driver that noone ends up using. I mean, for the
> > > > "WiFi catcher" do you have any kind of numbers for the users of
> > > > the feature?
> > >
> > > Like I said, this particular feature hasn't been in use for quite a
> > > few generations. In its time it was deemed "beneficial" in that
> > > day because of the amount of time it took for waking up the
> > > machine only to determine there was no available wifi nearby. It's
> > > been superseded by other technology improvements.
> > >
> > > There are other types of "notification" only events that could be
> > > useful to userspace. I don't think they need to all be generally
> > > demonized with the connotation as a negative vendor specific value
> > > add, there are plenty of generic things that userspace could
> > > notify on
> >
> > Generic things should find a specific system they are part of (i.e.
> > wifi notifications -> rflill, wifi switching -> input, etc), it is
> > one-off that "sounded good" but are hard to discover by user and
> > nobody ends up using that I am demonizing.
> 
> User (as consumer) of applications should not discovering such parts.
> User just open/configure his favourite gnome/kde/unity/xface/...
> application. But developers of wifi/power/led applications must know
> where to find those events.
> 
> And I need to say, if I change wifi state via rfkill interface, I would
> expect that also rfkill interface provides me information about changes.
> If I change keyboard backlight level via LED interface, then I somehow
> expect that LED interface should be able to tell me information that it
> was changed (but that's not truth... yet).
> 
> > > that are essentially "killed" at the WMI driver today.
> > >
> > > Here's a few I find:
> > > "Keyboard illumination toggle"
> > > "Mic mute toggle"
> > > "ALS sensor toggle"
> > > " Rotation lock toggle"
> > > "Airplane mode toggle"
> >
> > These all seem valid key events (unless they are not requests to
> > execute said actions but rather notifiers of events already
> > happened).
> 
> Previously "Keyboard illumination toggle" event was translated to
> KEY_KBDILLUMTOGGLE, but it later after implementing dell::kbd_backlight
> LED driver (software control via /sys/ of keyboard backlight) it was
> changed to KE_IGNORE.
> 
> Reason was following: All userspace applications (KDE, Unity, upowerd)
> understand KEY_KBDILLUMTOGGLE input event as "key for toggling
> keyboard
> backlight level" was pressed. Their answer to that input key is to find
> *::kbd_backlight led device in /sys/class/leds and switch keyboard
> backlight level to next value.
> 
> Dell ACPI/firmware send "Keyboard illumination toggle" event every time
> when keyboard backlight level is changed (software via driver or by HW
> key press) and all above applications started infinite loop...
> 
> So there are difference between:
> 
> *) user pressed key with icon of "keyboard illumination", but hardware
> did nothing
> 
> *) hardware changed keyboard illumination level (either by its own ---
> e.g. because of long inactivity of user or user closed LID --- or as
> reaction of user request via special driver)
> 
> Months ago we agreed that if pressing key with icon "keyboard
> illumination" cause 1) emitting key press event (via ACPI/WMI) and also
> 2) hardware unconditionally change keyboard backlight level, then we
> should not send that key press to userspace (as it was already processed
> by hardware). Same is for hardware key which block WIFI (as userspace
> react on input KEY_RFKILL to block all rfkills).
> 
> But I agree that could somehow inform userspace about changes which
> hardware did. For rfkills we already support interface when rfkill
> device send to userspace all change events.
> 
> For keyboard backlight we do not have notify interface yet, but I
> proposed that LED subsystem could be extended, so select/poll syscalls
> could be used on "brightness" sysfs node (which is used for changing LED
> level).
> 
> > Are they "killed" because they are also delivered via i8042?
> 
> No, (at least on my machine they are sent via WMI).

It really depends upon the vendor's implementation and that key's particular
implementation.  Some of the 

RE: [PATCH] dell-wmi: Add events created by Dell Rugged 2-in-1s

[Mario_Limonciello]

> -Original Message-
> From: Dmitry Torokhov [mailto:dmitry.torok...@gmail.com]
> Sent: Wednesday, July 27, 2016 6:08 PM
> To: Darren Hart 
> Cc: Pali Rohár ; Limonciello, Mario
> ; lkml ;
> platform-driver-...@vger.kernel.org
> Subject: Re: [PATCH] dell-wmi: Add events created by Dell Rugged 2-in-1s
> 
> Hi Darren,
> 
> On Wed, Jul 27, 2016 at 3:43 PM, Darren Hart  wrote:
> > Dmitry, we'd appreciate your thoughts as the input maintainer on a
> question
> > below (left the thread in tact so you have all the context).
> >
> > On Wed, Jul 27, 2016 at 10:15:12PM +0200, Pali Rohár wrote:
> >> On Wednesday 27 July 2016 21:03:57 mario_limoncie...@dell.com wrote:
> >> > > -Original Message-
> >> > > From: Pali Rohár [mailto:pali.ro...@gmail.com]
> >> > > Sent: Wednesday, July 27, 2016 11:06 AM
> >> > > To: Limonciello, Mario 
> >> > > Cc: dvh...@infradead.org; linux-kernel@vger.kernel.org;
> >> > > platform-driver- x...@vger.kernel.org
> >> > > Subject: Re: [PATCH] dell-wmi: Add events created by Dell Rugged
> >> > > 2-in-1s
> >> > >
> >> > > On Wednesday 27 July 2016 17:55:09 mario_limoncie...@dell.com
> wrote:
> >> > > > > Hi! I'm not sure if KEY_PROG1/2/3 are good names here as we
> >> > > > > already use
> >> > > > >
> >> > > > >those for other actions (see bios_to_linux_keycode). Also there
> >> > > > >is:
> >> > > > I had missed this, do you have some recommendations on what
> would
> >> > > > be better codes to map this to?
> >> > >
> >> > > Problem is that I do not know when those KEY_PROG keys from
> >> > > bios_to_linux_keycode table are emitted. There are missing
> comments
> >> > > or description.
> >> > >
> >> > > Are you able to find out description for all those keys in that
> >> > > table? Maybe now (when linux key constants are extended), there
> >> > > could be better candidates...
> >> >
> >> > I'll ask around.  It's not immediately obvious to me though, maybe
> >> > from old specs?
> >>
> >> Do not know if it is old. At least some codes from it are used on my
> >> E6440 machine. Table itself was added by Rezwanul Kabir in this commit:
> >>
> >>
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=5e
> a2559726b786283236835dc2905c23b36ac91c
> >>
> >> Maybe commit message could help you to indentify what it is...
> >>
> >> > > > I'll double check what the things that "were" mapping to
> >> > > > KEY_PROG1 etc actually were.  This might be a case of an
> >> > > > expected clash if the functions aren't in current generations.
> >> > > >
> >> > > > >/* Wifi Catcher */
> >> > > > >
> >> > > > > { KE_KEY,0xe011, { KEY_PROG2 } },
> >> > > >
> >> > > > It's worth mentioning that Wifi Catcher hasn't been used for any
> >> > > > Dell laptops for a handful generations.  The rugged 2in1's are
> >> > > > current generation that have these programmable buttons and
> >> > > > don't have wifi catcher.
> >> > >
> >> > > Anyway, what is "Wifi Catcher"? HW switch buttton which
> >> > > enable/disable wifi? Or something else?
> >> >
> >> > Wifi catcher was a special hardware switch slider switch.  When the
> >> > machine was turned in S3 the sliding the switch beyond the on
> >> > position would scan for available wifi networks and light up an LED
> >> > if some from your predefined list were found.
> >> >
> >> > When the machine was on, it would open up the applet that let you
> >> > configure the behavior for the switch in S3.
> >>
> >> Hm... maybe it better fit KEY_WLAN then? Just speculation.
> >>
> >> > > > So there should be no "real" clash here.
> >> > >
> >> > > Problem can be in future. This driver is unified for all Dell
> >> > > products with wmi interface and so future product could do some
> >> > > nasty things...
> >> >
> >> > I agree with Darren here.
> >> >
> >> > At least on Dell side we're creating new codes for "new" buttons and
> >> > the limitation is really on the kernel side for how many KEY_PROG#
> >> > or similar functions they can be re-assigned to.
> >>
> >> Ok.
> >>
> >> > Maybe it's time to think of another way to get this information to
> >> > userspace rather that always translating them into key codes?
> >>
> >> If event is generated by pressing key, button or hw switch, then input
> >> key is correct way. If there is no KEY define which fit for new vendor
> >> hw button, then I think we should start discussion with input subsystem
> >> how to handle such situation.
> >>
> >> > There's a lot that are marked as KE_IGNORE because the kernel can't
> >> > do anything interesting with them as no 1-1 mapping exists to a
> >> > keycode.
> >>
> >> Most marked as KE_IGNORE are because duplicate event is sent via
> >> keyboard controller or via other subsystem (e.g. rfkill).
> >>
> >> But events which are not keys or buttons should not be sent via input
> >> devices. At least I think it is against 

RE: [PATCH] dell-wmi: Add events created by Dell Rugged 2-in-1s

[Mario_Limonciello]

> -Original Message-
> From: Dmitry Torokhov [mailto:dmitry.torok...@gmail.com]
> Sent: Wednesday, July 27, 2016 6:08 PM
> To: Darren Hart 
> Cc: Pali Rohár ; Limonciello, Mario
> ; lkml ;
> platform-driver-...@vger.kernel.org
> Subject: Re: [PATCH] dell-wmi: Add events created by Dell Rugged 2-in-1s
> 
> Hi Darren,
> 
> On Wed, Jul 27, 2016 at 3:43 PM, Darren Hart  wrote:
> > Dmitry, we'd appreciate your thoughts as the input maintainer on a
> question
> > below (left the thread in tact so you have all the context).
> >
> > On Wed, Jul 27, 2016 at 10:15:12PM +0200, Pali Rohár wrote:
> >> On Wednesday 27 July 2016 21:03:57 mario_limoncie...@dell.com wrote:
> >> > > -Original Message-
> >> > > From: Pali Rohár [mailto:pali.ro...@gmail.com]
> >> > > Sent: Wednesday, July 27, 2016 11:06 AM
> >> > > To: Limonciello, Mario 
> >> > > Cc: dvh...@infradead.org; linux-kernel@vger.kernel.org;
> >> > > platform-driver- x...@vger.kernel.org
> >> > > Subject: Re: [PATCH] dell-wmi: Add events created by Dell Rugged
> >> > > 2-in-1s
> >> > >
> >> > > On Wednesday 27 July 2016 17:55:09 mario_limoncie...@dell.com
> wrote:
> >> > > > > Hi! I'm not sure if KEY_PROG1/2/3 are good names here as we
> >> > > > > already use
> >> > > > >
> >> > > > >those for other actions (see bios_to_linux_keycode). Also there
> >> > > > >is:
> >> > > > I had missed this, do you have some recommendations on what
> would
> >> > > > be better codes to map this to?
> >> > >
> >> > > Problem is that I do not know when those KEY_PROG keys from
> >> > > bios_to_linux_keycode table are emitted. There are missing
> comments
> >> > > or description.
> >> > >
> >> > > Are you able to find out description for all those keys in that
> >> > > table? Maybe now (when linux key constants are extended), there
> >> > > could be better candidates...
> >> >
> >> > I'll ask around.  It's not immediately obvious to me though, maybe
> >> > from old specs?
> >>
> >> Do not know if it is old. At least some codes from it are used on my
> >> E6440 machine. Table itself was added by Rezwanul Kabir in this commit:
> >>
> >>
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=5e
> a2559726b786283236835dc2905c23b36ac91c
> >>
> >> Maybe commit message could help you to indentify what it is...
> >>
> >> > > > I'll double check what the things that "were" mapping to
> >> > > > KEY_PROG1 etc actually were.  This might be a case of an
> >> > > > expected clash if the functions aren't in current generations.
> >> > > >
> >> > > > >/* Wifi Catcher */
> >> > > > >
> >> > > > > { KE_KEY,0xe011, { KEY_PROG2 } },
> >> > > >
> >> > > > It's worth mentioning that Wifi Catcher hasn't been used for any
> >> > > > Dell laptops for a handful generations.  The rugged 2in1's are
> >> > > > current generation that have these programmable buttons and
> >> > > > don't have wifi catcher.
> >> > >
> >> > > Anyway, what is "Wifi Catcher"? HW switch buttton which
> >> > > enable/disable wifi? Or something else?
> >> >
> >> > Wifi catcher was a special hardware switch slider switch.  When the
> >> > machine was turned in S3 the sliding the switch beyond the on
> >> > position would scan for available wifi networks and light up an LED
> >> > if some from your predefined list were found.
> >> >
> >> > When the machine was on, it would open up the applet that let you
> >> > configure the behavior for the switch in S3.
> >>
> >> Hm... maybe it better fit KEY_WLAN then? Just speculation.
> >>
> >> > > > So there should be no "real" clash here.
> >> > >
> >> > > Problem can be in future. This driver is unified for all Dell
> >> > > products with wmi interface and so future product could do some
> >> > > nasty things...
> >> >
> >> > I agree with Darren here.
> >> >
> >> > At least on Dell side we're creating new codes for "new" buttons and
> >> > the limitation is really on the kernel side for how many KEY_PROG#
> >> > or similar functions they can be re-assigned to.
> >>
> >> Ok.
> >>
> >> > Maybe it's time to think of another way to get this information to
> >> > userspace rather that always translating them into key codes?
> >>
> >> If event is generated by pressing key, button or hw switch, then input
> >> key is correct way. If there is no KEY define which fit for new vendor
> >> hw button, then I think we should start discussion with input subsystem
> >> how to handle such situation.
> >>
> >> > There's a lot that are marked as KE_IGNORE because the kernel can't
> >> > do anything interesting with them as no 1-1 mapping exists to a
> >> > keycode.
> >>
> >> Most marked as KE_IGNORE are because duplicate event is sent via
> >> keyboard controller or via other subsystem (e.g. rfkill).
> >>
> >> But events which are not keys or buttons should not be sent via input
> >> devices. At least I think it is against usage of input devices.
> >>
> >> Events like "battery was removed" or "keyboard backlight was changed"
> or
> >> "battery lifetime decreased under X %" 

RE: [PATCH] dell-wmi: Add events created by Dell Rugged 2-in-1s

[Mario_Limonciello]

> > >
> > > Anyway, what is "Wifi Catcher"? HW switch buttton which
> > > enable/disable wifi? Or something else?
> >
> > Wifi catcher was a special hardware switch slider switch.  When the
> > machine was turned in S3 the sliding the switch beyond the on
> > position would scan for available wifi networks and light up an LED
> > if some from your predefined list were found.
> >
> > When the machine was on, it would open up the applet that let you
> > configure the behavior for the switch in S3.
> 
> Hm... maybe it better fit KEY_WLAN then? Just speculation.
> 

Yeah, I think that's a good fit.  I'll adjust that when I resubmit the
rugged patch shortly.

> > > > So there should be no "real" clash here.
> > >
> > > Problem can be in future. This driver is unified for all Dell
> > > products with wmi interface and so future product could do some
> > > nasty things...
> >
> > I agree with Darren here.
> >
> > At least on Dell side we're creating new codes for "new" buttons and
> > the limitation is really on the kernel side for how many KEY_PROG#
> > or similar functions they can be re-assigned to.
> 
> Ok.
> 
> > Maybe it's time to think of another way to get this information to
> > userspace rather that always translating them into key codes?
> 
> If event is generated by pressing key, button or hw switch, then input
> key is correct way. If there is no KEY define which fit for new vendor
> hw button, then I think we should start discussion with input subsystem
> how to handle such situation.
> 
> > There's a lot that are marked as KE_IGNORE because the kernel can't
> > do anything interesting with them as no 1-1 mapping exists to a
> > keycode.
> 
> Most marked as KE_IGNORE are because duplicate event is sent via
> keyboard controller or via other subsystem (e.g. rfkill).
> 
> But events which are not keys or buttons should not be sent via input
> devices. At least I think it is against usage of input devices.
> 
> Events like "battery was removed" or "keyboard backlight was changed" or
> "battery lifetime decreased under X %" can be useful for userspace
> applications. I agree. But I think we do not have any subsystem or
> interface for sending them from kernel to userspace.
> 
> If we start talking about creating interface for it, I would rather see
> something more generic, not Dell-only specific or created specially for
> Dell machines which will not fit for others...
> 

I don't think it needs to be Dell specific, I'm sure plenty of other platform
drivers would use it too.

If this interface is created we can figure out which ones really are useful
notifications to userspace.

RE: [PATCH] dell-wmi: Add events created by Dell Rugged 2-in-1s

[Mario_Limonciello]

> > >
> > > Anyway, what is "Wifi Catcher"? HW switch buttton which
> > > enable/disable wifi? Or something else?
> >
> > Wifi catcher was a special hardware switch slider switch.  When the
> > machine was turned in S3 the sliding the switch beyond the on
> > position would scan for available wifi networks and light up an LED
> > if some from your predefined list were found.
> >
> > When the machine was on, it would open up the applet that let you
> > configure the behavior for the switch in S3.
> 
> Hm... maybe it better fit KEY_WLAN then? Just speculation.
> 

Yeah, I think that's a good fit.  I'll adjust that when I resubmit the
rugged patch shortly.

> > > > So there should be no "real" clash here.
> > >
> > > Problem can be in future. This driver is unified for all Dell
> > > products with wmi interface and so future product could do some
> > > nasty things...
> >
> > I agree with Darren here.
> >
> > At least on Dell side we're creating new codes for "new" buttons and
> > the limitation is really on the kernel side for how many KEY_PROG#
> > or similar functions they can be re-assigned to.
> 
> Ok.
> 
> > Maybe it's time to think of another way to get this information to
> > userspace rather that always translating them into key codes?
> 
> If event is generated by pressing key, button or hw switch, then input
> key is correct way. If there is no KEY define which fit for new vendor
> hw button, then I think we should start discussion with input subsystem
> how to handle such situation.
> 
> > There's a lot that are marked as KE_IGNORE because the kernel can't
> > do anything interesting with them as no 1-1 mapping exists to a
> > keycode.
> 
> Most marked as KE_IGNORE are because duplicate event is sent via
> keyboard controller or via other subsystem (e.g. rfkill).
> 
> But events which are not keys or buttons should not be sent via input
> devices. At least I think it is against usage of input devices.
> 
> Events like "battery was removed" or "keyboard backlight was changed" or
> "battery lifetime decreased under X %" can be useful for userspace
> applications. I agree. But I think we do not have any subsystem or
> interface for sending them from kernel to userspace.
> 
> If we start talking about creating interface for it, I would rather see
> something more generic, not Dell-only specific or created specially for
> Dell machines which will not fit for others...
> 

I don't think it needs to be Dell specific, I'm sure plenty of other platform
drivers would use it too.

If this interface is created we can figure out which ones really are useful
notifications to userspace.

RE: [PATCH] dell-wmi: Add events created by Dell Rugged 2-in-1s

[Mario_Limonciello]

> -Original Message-
> From: Pali Rohár [mailto:pali.ro...@gmail.com]
> Sent: Wednesday, July 27, 2016 11:06 AM
> To: Limonciello, Mario 
> Cc: dvh...@infradead.org; linux-kernel@vger.kernel.org; platform-driver-
> x...@vger.kernel.org
> Subject: Re: [PATCH] dell-wmi: Add events created by Dell Rugged 2-in-1s
> 
> On Wednesday 27 July 2016 17:55:09 mario_limoncie...@dell.com wrote:
> > > Hi! I'm not sure if KEY_PROG1/2/3 are good names here as we already
> > > use
> > >
> > >those for other actions (see bios_to_linux_keycode). Also there is:
> > I had missed this, do you have some recommendations on what would be
> > better codes to map this to?
> 
> Problem is that I do not know when those KEY_PROG keys from
> bios_to_linux_keycode table are emitted. There are missing comments or
> description.
> 
> Are you able to find out description for all those keys in that table?
> Maybe now (when linux key constants are extended), there could be better
> candidates...

I'll ask around.  It's not immediately obvious to me though, maybe from old
specs?

> 
> > I'll double check what the things that "were" mapping to KEY_PROG1 etc
> > actually were.  This might be a case of an expected clash if the
> > functions aren't in current generations.
> >
> > >/* Wifi Catcher */
> > >
> > > { KE_KEY,0xe011, { KEY_PROG2 } },
> >
> > It's worth mentioning that Wifi Catcher hasn't been used for any Dell
> > laptops for a handful generations.  The rugged 2in1's are current
> > generation that have these programmable buttons and don't have wifi
> > catcher.
> 
> Anyway, what is "Wifi Catcher"? HW switch buttton which enable/disable wifi?
> Or something else?
>

Wifi catcher was a special hardware switch slider switch.  When the machine
was turned in S3 the sliding the switch beyond the on position would scan for 
available wifi networks and light up an LED if some from your predefined list
were found.

When the machine was on, it would open up the applet that let you configure
the behavior for the switch in S3.

> > So there should be no "real" clash here.
> 
> Problem can be in future. This driver is unified for all Dell products with 
> wmi
> interface and so future product could do some nasty things...

I agree with Darren here.  

At least on Dell side we're creating new codes for "new" buttons and the
limitation is really on the kernel side for how many KEY_PROG# or similar
functions they can be re-assigned to.

Maybe it's time to think of another way to get this information to userspace
rather that always translating them into key codes?

There's a lot that are marked as KE_IGNORE because the kernel can't do
anything interesting with them as no 1-1 mapping exists to a keycode.

Those could still be passed on somehow to have things like gnome-settings-daemon
or other userspace tools to react however and show a notification.

> 
> > > But probably we do not have better names...
> >
> > In this particular case, I was thinking PROG1/2/3 were really the best
> > option and would be most intuitive because the keys really are labels
> > "P1" "P2" and "P3".
> 
> Probably yes, as I wrote I do not have in my mind better names for now.
> 

OK, I'll resend with the cosmetic tabbing change and a clearer description,
but keep the same mapping.

RE: [PATCH] dell-wmi: Add events created by Dell Rugged 2-in-1s

[Mario_Limonciello]

> -Original Message-
> From: Pali Rohár [mailto:pali.ro...@gmail.com]
> Sent: Wednesday, July 27, 2016 11:06 AM
> To: Limonciello, Mario 
> Cc: dvh...@infradead.org; linux-kernel@vger.kernel.org; platform-driver-
> x...@vger.kernel.org
> Subject: Re: [PATCH] dell-wmi: Add events created by Dell Rugged 2-in-1s
> 
> On Wednesday 27 July 2016 17:55:09 mario_limoncie...@dell.com wrote:
> > > Hi! I'm not sure if KEY_PROG1/2/3 are good names here as we already
> > > use
> > >
> > >those for other actions (see bios_to_linux_keycode). Also there is:
> > I had missed this, do you have some recommendations on what would be
> > better codes to map this to?
> 
> Problem is that I do not know when those KEY_PROG keys from
> bios_to_linux_keycode table are emitted. There are missing comments or
> description.
> 
> Are you able to find out description for all those keys in that table?
> Maybe now (when linux key constants are extended), there could be better
> candidates...

I'll ask around.  It's not immediately obvious to me though, maybe from old
specs?

> 
> > I'll double check what the things that "were" mapping to KEY_PROG1 etc
> > actually were.  This might be a case of an expected clash if the
> > functions aren't in current generations.
> >
> > >/* Wifi Catcher */
> > >
> > > { KE_KEY,0xe011, { KEY_PROG2 } },
> >
> > It's worth mentioning that Wifi Catcher hasn't been used for any Dell
> > laptops for a handful generations.  The rugged 2in1's are current
> > generation that have these programmable buttons and don't have wifi
> > catcher.
> 
> Anyway, what is "Wifi Catcher"? HW switch buttton which enable/disable wifi?
> Or something else?
>

Wifi catcher was a special hardware switch slider switch.  When the machine
was turned in S3 the sliding the switch beyond the on position would scan for 
available wifi networks and light up an LED if some from your predefined list
were found.

When the machine was on, it would open up the applet that let you configure
the behavior for the switch in S3.

> > So there should be no "real" clash here.
> 
> Problem can be in future. This driver is unified for all Dell products with 
> wmi
> interface and so future product could do some nasty things...

I agree with Darren here.  

At least on Dell side we're creating new codes for "new" buttons and the
limitation is really on the kernel side for how many KEY_PROG# or similar
functions they can be re-assigned to.

Maybe it's time to think of another way to get this information to userspace
rather that always translating them into key codes?

There's a lot that are marked as KE_IGNORE because the kernel can't do
anything interesting with them as no 1-1 mapping exists to a keycode.

Those could still be passed on somehow to have things like gnome-settings-daemon
or other userspace tools to react however and show a notification.

> 
> > > But probably we do not have better names...
> >
> > In this particular case, I was thinking PROG1/2/3 were really the best
> > option and would be most intuitive because the keys really are labels
> > "P1" "P2" and "P3".
> 
> Probably yes, as I wrote I do not have in my mind better names for now.
> 

OK, I'll resend with the cosmetic tabbing change and a clearer description,
but keep the same mapping.

Re: [PATCH] dell-wmi: Add events created by Dell Rugged 2-in-1s

[Mario_Limonciello]

> Hi! I'm not sure if KEY_PROG1/2/3 are good names here as we already use
>those for other actions (see bios_to_linux_keycode). Also there is:

I had missed this, do you have some recommendations on what would be
better codes to map this to?

I'll double check what the things that "were" mapping to KEY_PROG1 etc
actually were.  This might be a case of an expected clash if the functions
aren't in current generations.

>
>/* Wifi Catcher */
> { KE_KEY,0xe011, { KEY_PROG2 } },
>

It's worth mentioning that Wifi Catcher hasn't been used for any Dell laptops
for a handful generations.  The rugged 2in1's are current generation that have 
these programmable buttons and don't have wifi catcher.

So there should be no "real" clash here.
   
> But probably we do not have better names...

In this particular case, I was thinking PROG1/2/3 were really the best option
and would be most intuitive because the keys really are labels "P1" "P2" and
"P3".

Here's the front of the tablet:
http://shop-media.intel.com/api/v2/helperservice/getimage?url=http://images.icecat.biz/img/norm/high/30031725-706.jpg=550=550

>
> Another small cosmetic change: align WMI codes, so 0x157 and 0x850 are
> at some column (similar like are formatted other actions).

OK.

Re: [PATCH] dell-wmi: Add events created by Dell Rugged 2-in-1s

[Mario_Limonciello]

> Hi! I'm not sure if KEY_PROG1/2/3 are good names here as we already use
>those for other actions (see bios_to_linux_keycode). Also there is:

I had missed this, do you have some recommendations on what would be
better codes to map this to?

I'll double check what the things that "were" mapping to KEY_PROG1 etc
actually were.  This might be a case of an expected clash if the functions
aren't in current generations.

>
>/* Wifi Catcher */
> { KE_KEY,0xe011, { KEY_PROG2 } },
>

It's worth mentioning that Wifi Catcher hasn't been used for any Dell laptops
for a handful generations.  The rugged 2in1's are current generation that have 
these programmable buttons and don't have wifi catcher.

So there should be no "real" clash here.
   
> But probably we do not have better names...

In this particular case, I was thinking PROG1/2/3 were really the best option
and would be most intuitive because the keys really are labels "P1" "P2" and
"P3".

Here's the front of the tablet:
http://shop-media.intel.com/api/v2/helperservice/getimage?url=http://images.icecat.biz/img/norm/high/30031725-706.jpg=550=550

>
> Another small cosmetic change: align WMI codes, so 0x157 and 0x850 are
> at some column (similar like are formatted other actions).

OK.

RE: [PATCH 2/2] dmi-id: add dmi/id/oem group for exporting oem strings to sysfs

[Mario_Limonciello]

> -Original Message-
> From: Limonciello, Mario
> Sent: Tuesday, July 19, 2016 9:48 AM
> To: 'Jean Delvare' ; Hung, Allen 
> Cc: Jean Delvare ; linux-kernel@vger.kernel.org
> Subject: RE: [PATCH 2/2] dmi-id: add dmi/id/oem group for exporting oem
> strings to sysfs
> 
> Hi Jean,
> 
> I worked with Allen on this concept, so I've got some comments below.
> 
> > -Original Message-
> > From: Jean Delvare [mailto:jdelv...@suse.de]
> > Sent: Tuesday, July 19, 2016 4:03 AM
> > To: Hung, Allen 
> > Cc: Jean Delvare ; linux-kernel@vger.kernel.org;
> > Limonciello, Mario 
> > Subject: Re: [PATCH 2/2] dmi-id: add dmi/id/oem group for exporting oem
> > strings to sysfs
> >
> > Hello Allen,
> >
> > On Thu, 14 Jul 2016 16:01:23 +0800, Allen Hung wrote:
> > > The oem strings in DMI system identification information of the BIOS
> have
> > > been parsed and stored as dmi devices in dmi_scan.c but they are not
> > > exported to userspace via sysfs.
> >
> > They are intended for internal consumption by the kernel drivers.
> >
> > > The patch intends to export oem strings to sysfs device /sys/class/dmi/id.
> > > As the number of oem strings are dynamic, a group "oem" is added to the
> > > device and the strings will be added to the group as string1, string2, 
> > > ...,
> > > and stringN.
> >
> > What is the use case? You can already get these strings easily using
> > dmidecode:
> >
> > # dmidecode -qt 11
> > OEM Strings
> > String 1: Dell System
> > String 2: 1[05A4]
> > String 3: 3[1.0]
> > String 4: 12[www.dell.com]
> > String 5: 14[1]
> > String 6: 15[3]
> > String 7:
> >
> > If needed, a dedicated option could be added to dmidecode to extract
> > specific OEM strings. Or existing option -s could be extended for that
> > purpose.
> 
> The main purpose was to be able to parse these easily from userspace
> without needing dmidecode installed and handling its output
> (with tools such as grep, sed, and awk).
> 
> For example in an initramfs, typically dmidecode isn't included, but there
> is value to being able to make decisions on things related to the values of
> those OEM strings.
> 
> Instead this allows userspace to iterate the oem/ directory and directly
> look at the values of these strings.
> 
> >
> > Also your code doesn't even build. I won't review this patch until I
> > know why it is needed, and it builds (without warning.)
> >
> 
> Allen had a mistake in that submission when he was refactoring it prior to
> LKML submission.
> He resubmitted it the next day fixing that mistake:
> https://patchwork.kernel.org/patch/9231473/
> 
> > One comment below though:
> >
> > >
> > > Signed-off-by: Allen Hung 
> > > ---
> > >  drivers/firmware/dmi-id.c | 108
> > ++
> > >  1 file changed, 108 insertions(+)
> > >
> > > diff --git a/drivers/firmware/dmi-id.c b/drivers/firmware/dmi-id.c
> > > index 44c0139..f284a07 100644
> > > --- a/drivers/firmware/dmi-id.c
> > > +++ b/drivers/firmware/dmi-id.c
> > > (...)
> > > +static int __init dmi_id_init_oem_attr_group(void)
> > > +{
> > > + int i, ret;
> > > + const struct dmi_device *dev;
> > > + struct dmi_oem_attribute *oa, *tmp;
> > > + struct device_attribute dev_attr_tmpl =
> > > + __ATTR(, 0444, sys_dmi_oem_show, NULL);
> >
> > I'd be very careful about permissions. OEM strings could contain pretty
> > much everything, including serial numbers or passwords. Making these
> > files world-readable doesn't strike me as the best of the ideas.
> >
> 
> At least on Dell systems, the values in these strings are OK to be world
> readable, but I understand this concern and agree that Allen should adjust
> these permissions in the next version if you agree with the concept of this
> patch.
> 
> Thanks,

Hi jean,

Did you have any comments about Allen's updated patch or my above
comments?

If necessary, Allen can resend with the fix to OEM strings permissions
and we can discuss further then.

Thanks,

RE: [PATCH 2/2] dmi-id: add dmi/id/oem group for exporting oem strings to sysfs

[Mario_Limonciello]

> -Original Message-
> From: Limonciello, Mario
> Sent: Tuesday, July 19, 2016 9:48 AM
> To: 'Jean Delvare' ; Hung, Allen 
> Cc: Jean Delvare ; linux-kernel@vger.kernel.org
> Subject: RE: [PATCH 2/2] dmi-id: add dmi/id/oem group for exporting oem
> strings to sysfs
> 
> Hi Jean,
> 
> I worked with Allen on this concept, so I've got some comments below.
> 
> > -Original Message-
> > From: Jean Delvare [mailto:jdelv...@suse.de]
> > Sent: Tuesday, July 19, 2016 4:03 AM
> > To: Hung, Allen 
> > Cc: Jean Delvare ; linux-kernel@vger.kernel.org;
> > Limonciello, Mario 
> > Subject: Re: [PATCH 2/2] dmi-id: add dmi/id/oem group for exporting oem
> > strings to sysfs
> >
> > Hello Allen,
> >
> > On Thu, 14 Jul 2016 16:01:23 +0800, Allen Hung wrote:
> > > The oem strings in DMI system identification information of the BIOS
> have
> > > been parsed and stored as dmi devices in dmi_scan.c but they are not
> > > exported to userspace via sysfs.
> >
> > They are intended for internal consumption by the kernel drivers.
> >
> > > The patch intends to export oem strings to sysfs device /sys/class/dmi/id.
> > > As the number of oem strings are dynamic, a group "oem" is added to the
> > > device and the strings will be added to the group as string1, string2, 
> > > ...,
> > > and stringN.
> >
> > What is the use case? You can already get these strings easily using
> > dmidecode:
> >
> > # dmidecode -qt 11
> > OEM Strings
> > String 1: Dell System
> > String 2: 1[05A4]
> > String 3: 3[1.0]
> > String 4: 12[www.dell.com]
> > String 5: 14[1]
> > String 6: 15[3]
> > String 7:
> >
> > If needed, a dedicated option could be added to dmidecode to extract
> > specific OEM strings. Or existing option -s could be extended for that
> > purpose.
> 
> The main purpose was to be able to parse these easily from userspace
> without needing dmidecode installed and handling its output
> (with tools such as grep, sed, and awk).
> 
> For example in an initramfs, typically dmidecode isn't included, but there
> is value to being able to make decisions on things related to the values of
> those OEM strings.
> 
> Instead this allows userspace to iterate the oem/ directory and directly
> look at the values of these strings.
> 
> >
> > Also your code doesn't even build. I won't review this patch until I
> > know why it is needed, and it builds (without warning.)
> >
> 
> Allen had a mistake in that submission when he was refactoring it prior to
> LKML submission.
> He resubmitted it the next day fixing that mistake:
> https://patchwork.kernel.org/patch/9231473/
> 
> > One comment below though:
> >
> > >
> > > Signed-off-by: Allen Hung 
> > > ---
> > >  drivers/firmware/dmi-id.c | 108
> > ++
> > >  1 file changed, 108 insertions(+)
> > >
> > > diff --git a/drivers/firmware/dmi-id.c b/drivers/firmware/dmi-id.c
> > > index 44c0139..f284a07 100644
> > > --- a/drivers/firmware/dmi-id.c
> > > +++ b/drivers/firmware/dmi-id.c
> > > (...)
> > > +static int __init dmi_id_init_oem_attr_group(void)
> > > +{
> > > + int i, ret;
> > > + const struct dmi_device *dev;
> > > + struct dmi_oem_attribute *oa, *tmp;
> > > + struct device_attribute dev_attr_tmpl =
> > > + __ATTR(, 0444, sys_dmi_oem_show, NULL);
> >
> > I'd be very careful about permissions. OEM strings could contain pretty
> > much everything, including serial numbers or passwords. Making these
> > files world-readable doesn't strike me as the best of the ideas.
> >
> 
> At least on Dell systems, the values in these strings are OK to be world
> readable, but I understand this concern and agree that Allen should adjust
> these permissions in the next version if you agree with the concept of this
> patch.
> 
> Thanks,

Hi jean,

Did you have any comments about Allen's updated patch or my above
comments?

If necessary, Allen can resend with the fix to OEM strings permissions
and we can discuss further then.

Thanks,

RE: [PATCH 2/2] dmi-id: add dmi/id/oem group for exporting oem strings to sysfs

[Mario_Limonciello]

Hi Jean,

I worked with Allen on this concept, so I've got some comments below.

> -Original Message-
> From: Jean Delvare [mailto:jdelv...@suse.de]
> Sent: Tuesday, July 19, 2016 4:03 AM
> To: Hung, Allen 
> Cc: Jean Delvare ; linux-kernel@vger.kernel.org;
> Limonciello, Mario 
> Subject: Re: [PATCH 2/2] dmi-id: add dmi/id/oem group for exporting oem
> strings to sysfs
> 
> Hello Allen,
> 
> On Thu, 14 Jul 2016 16:01:23 +0800, Allen Hung wrote:
> > The oem strings in DMI system identification information of the BIOS have
> > been parsed and stored as dmi devices in dmi_scan.c but they are not
> > exported to userspace via sysfs.
> 
> They are intended for internal consumption by the kernel drivers.
> 
> > The patch intends to export oem strings to sysfs device /sys/class/dmi/id.
> > As the number of oem strings are dynamic, a group "oem" is added to the
> > device and the strings will be added to the group as string1, string2, ...,
> > and stringN.
> 
> What is the use case? You can already get these strings easily using
> dmidecode:
> 
> # dmidecode -qt 11
> OEM Strings
>   String 1: Dell System
>   String 2: 1[05A4]
>   String 3: 3[1.0]
>   String 4: 12[www.dell.com]
>   String 5: 14[1]
>   String 6: 15[3]
>   String 7:
> 
> If needed, a dedicated option could be added to dmidecode to extract
> specific OEM strings. Or existing option -s could be extended for that
> purpose.

The main purpose was to be able to parse these easily from userspace
without needing dmidecode installed and handling its output 
(with tools such as grep, sed, and awk).

For example in an initramfs, typically dmidecode isn't included, but there
is value to being able to make decisions on things related to the values of 
those OEM strings.

Instead this allows userspace to iterate the oem/ directory and directly
look at the values of these strings.

> 
> Also your code doesn't even build. I won't review this patch until I
> know why it is needed, and it builds (without warning.)
> 

Allen had a mistake in that submission when he was refactoring it prior to 
LKML submission.  
He resubmitted it the next day fixing that mistake:
https://patchwork.kernel.org/patch/9231473/

> One comment below though:
> 
> >
> > Signed-off-by: Allen Hung 
> > ---
> >  drivers/firmware/dmi-id.c | 108
> ++
> >  1 file changed, 108 insertions(+)
> >
> > diff --git a/drivers/firmware/dmi-id.c b/drivers/firmware/dmi-id.c
> > index 44c0139..f284a07 100644
> > --- a/drivers/firmware/dmi-id.c
> > +++ b/drivers/firmware/dmi-id.c
> > (...)
> > +static int __init dmi_id_init_oem_attr_group(void)
> > +{
> > +   int i, ret;
> > +   const struct dmi_device *dev;
> > +   struct dmi_oem_attribute *oa, *tmp;
> > +   struct device_attribute dev_attr_tmpl =
> > +   __ATTR(, 0444, sys_dmi_oem_show, NULL);
> 
> I'd be very careful about permissions. OEM strings could contain pretty
> much everything, including serial numbers or passwords. Making these
> files world-readable doesn't strike me as the best of the ideas.
> 

At least on Dell systems, the values in these strings are OK to be world
readable, but I understand this concern and agree that Allen should adjust
these permissions in the next version if you agree with the concept of this
patch.

Thanks,

RE: [PATCH 2/2] dmi-id: add dmi/id/oem group for exporting oem strings to sysfs

[Mario_Limonciello]

Hi Jean,

I worked with Allen on this concept, so I've got some comments below.

> -Original Message-
> From: Jean Delvare [mailto:jdelv...@suse.de]
> Sent: Tuesday, July 19, 2016 4:03 AM
> To: Hung, Allen 
> Cc: Jean Delvare ; linux-kernel@vger.kernel.org;
> Limonciello, Mario 
> Subject: Re: [PATCH 2/2] dmi-id: add dmi/id/oem group for exporting oem
> strings to sysfs
> 
> Hello Allen,
> 
> On Thu, 14 Jul 2016 16:01:23 +0800, Allen Hung wrote:
> > The oem strings in DMI system identification information of the BIOS have
> > been parsed and stored as dmi devices in dmi_scan.c but they are not
> > exported to userspace via sysfs.
> 
> They are intended for internal consumption by the kernel drivers.
> 
> > The patch intends to export oem strings to sysfs device /sys/class/dmi/id.
> > As the number of oem strings are dynamic, a group "oem" is added to the
> > device and the strings will be added to the group as string1, string2, ...,
> > and stringN.
> 
> What is the use case? You can already get these strings easily using
> dmidecode:
> 
> # dmidecode -qt 11
> OEM Strings
>   String 1: Dell System
>   String 2: 1[05A4]
>   String 3: 3[1.0]
>   String 4: 12[www.dell.com]
>   String 5: 14[1]
>   String 6: 15[3]
>   String 7:
> 
> If needed, a dedicated option could be added to dmidecode to extract
> specific OEM strings. Or existing option -s could be extended for that
> purpose.

The main purpose was to be able to parse these easily from userspace
without needing dmidecode installed and handling its output 
(with tools such as grep, sed, and awk).

For example in an initramfs, typically dmidecode isn't included, but there
is value to being able to make decisions on things related to the values of 
those OEM strings.

Instead this allows userspace to iterate the oem/ directory and directly
look at the values of these strings.

> 
> Also your code doesn't even build. I won't review this patch until I
> know why it is needed, and it builds (without warning.)
> 

Allen had a mistake in that submission when he was refactoring it prior to 
LKML submission.  
He resubmitted it the next day fixing that mistake:
https://patchwork.kernel.org/patch/9231473/

> One comment below though:
> 
> >
> > Signed-off-by: Allen Hung 
> > ---
> >  drivers/firmware/dmi-id.c | 108
> ++
> >  1 file changed, 108 insertions(+)
> >
> > diff --git a/drivers/firmware/dmi-id.c b/drivers/firmware/dmi-id.c
> > index 44c0139..f284a07 100644
> > --- a/drivers/firmware/dmi-id.c
> > +++ b/drivers/firmware/dmi-id.c
> > (...)
> > +static int __init dmi_id_init_oem_attr_group(void)
> > +{
> > +   int i, ret;
> > +   const struct dmi_device *dev;
> > +   struct dmi_oem_attribute *oa, *tmp;
> > +   struct device_attribute dev_attr_tmpl =
> > +   __ATTR(, 0444, sys_dmi_oem_show, NULL);
> 
> I'd be very careful about permissions. OEM strings could contain pretty
> much everything, including serial numbers or passwords. Making these
> files world-readable doesn't strike me as the best of the ideas.
> 

At least on Dell systems, the values in these strings are OK to be world
readable, but I understand this concern and agree that Allen should adjust
these permissions in the next version if you agree with the concept of this
patch.

Thanks,

RE: [PATCH v6 RESEND] r8152: Add support for setting pass through MAC address on RTL8153-AD

[Mario_Limonciello]

> -Original Message-
> From: Michał Pecio [mailto:michal.pe...@gmail.com]
> Sent: Tuesday, July 12, 2016 2:03 AM
> To: David Miller 
> Cc: Limonciello, Mario ; linux-
> ker...@vger.kernel.org; net...@vger.kernel.org; linux-
> u...@vger.kernel.org; anthony.w...@canonical.com
> Subject: Re: [PATCH v6 RESEND] r8152: Add support for setting pass through
> MAC address on RTL8153-AD
> 
> > From: Mario Limonciello 
> > Date: Mon, 11 Jul 2016 19:58:04 -0500
> >
> > > The RTL8153-AD supports a persistent system specific MAC address.
> > > This means a device plugged into two different systems with host
> > > side support will show different (but persistent) MAC addresses.
> > >
> > > This information for the system's persistent MAC address is burned
> > > in when the system HW is built and available under \_SB.AMAC in the
> > > DSDT at runtime.
> > >
> > > This technology is currently implemented in the Dell TB15 and WD15
> > > Type-C docks.  More information is available here:
> > > http://www.dell.com/support/article/us/en/04/SLN301147
> > >
> > > Signed-off-by: Mario Limonciello 
> >
> > Applied.
> 
> Hi,
> 
> Isn't it possible that the same ACPI name could be used for other
> vendor-specific extensions on other laptops and that r8152 will now
> wreak havoc there?
> 
> Regards,
> MP

This has been discussed a bit previously in earlier submissions.

In short, this is an extreme corner case.  Some changes were made 
to diminish potential impact. The ACPI code is resolved only when 
the specific variant of RTL8135 (-AD) has a bit set on the efuse.

The patch also explicitly checks the return type and contents of the
ACPI object and won't proceed if it's invalid.

The Type-C devices that provide this are currently only sold by Dell.  
This of course may change one day if other OEM's add this
feature, but it just shows that device scope is limited.

For now the way this is implemented if Realtek does choose to 
work with another OEM on this feature, there should be no
kernel code change necessary for interoperability of peripherals
on other OEM machines.

So in order to hit this hypothetical corner case today you would 
need to be using a real world type-C device something such as a 
Dell WD15 on another OEM's machine that has type-C and the exact
same ACPI object name that does $BADSTUFF other than return a
buffer.

If that situation arises please alert me and I'll send a follow up
patch that whitelists this to match DMI vendor of only Dell 
systems.

RE: [PATCH v6 RESEND] r8152: Add support for setting pass through MAC address on RTL8153-AD

[Mario_Limonciello]

> -Original Message-
> From: Michał Pecio [mailto:michal.pe...@gmail.com]
> Sent: Tuesday, July 12, 2016 2:03 AM
> To: David Miller 
> Cc: Limonciello, Mario ; linux-
> ker...@vger.kernel.org; net...@vger.kernel.org; linux-
> u...@vger.kernel.org; anthony.w...@canonical.com
> Subject: Re: [PATCH v6 RESEND] r8152: Add support for setting pass through
> MAC address on RTL8153-AD
> 
> > From: Mario Limonciello 
> > Date: Mon, 11 Jul 2016 19:58:04 -0500
> >
> > > The RTL8153-AD supports a persistent system specific MAC address.
> > > This means a device plugged into two different systems with host
> > > side support will show different (but persistent) MAC addresses.
> > >
> > > This information for the system's persistent MAC address is burned
> > > in when the system HW is built and available under \_SB.AMAC in the
> > > DSDT at runtime.
> > >
> > > This technology is currently implemented in the Dell TB15 and WD15
> > > Type-C docks.  More information is available here:
> > > http://www.dell.com/support/article/us/en/04/SLN301147
> > >
> > > Signed-off-by: Mario Limonciello 
> >
> > Applied.
> 
> Hi,
> 
> Isn't it possible that the same ACPI name could be used for other
> vendor-specific extensions on other laptops and that r8152 will now
> wreak havoc there?
> 
> Regards,
> MP

This has been discussed a bit previously in earlier submissions.

In short, this is an extreme corner case.  Some changes were made 
to diminish potential impact. The ACPI code is resolved only when 
the specific variant of RTL8135 (-AD) has a bit set on the efuse.

The patch also explicitly checks the return type and contents of the
ACPI object and won't proceed if it's invalid.

The Type-C devices that provide this are currently only sold by Dell.  
This of course may change one day if other OEM's add this
feature, but it just shows that device scope is limited.

For now the way this is implemented if Realtek does choose to 
work with another OEM on this feature, there should be no
kernel code change necessary for interoperability of peripherals
on other OEM machines.

So in order to hit this hypothetical corner case today you would 
need to be using a real world type-C device something such as a 
Dell WD15 on another OEM's machine that has type-C and the exact
same ACPI object name that does $BADSTUFF other than return a
buffer.

If that situation arises please alert me and I'll send a follow up
patch that whitelists this to match DMI vendor of only Dell 
systems.

RE: [PATCH v6] r8152: Add support for setting pass through MAC address on RTL8153-AD

[Mario_Limonciello]

> David,
> 
> Did you have any more thoughts about this?  I'm happy to make some other
> adjustments to the patch, if you have some recommendations.

Hi,

I just wanted to share that the maintenance BIOSes released for the Dell 
platforms with Type-C this past week enables the MAC address pass 
through feature in UEFI, so any network booted machines will offer
the auxiliary MAC to the DHCP server.  In Windows a network booted
machine will always use auxiliary MAC now.

XPS 9350 (BIOS 1.4.4): http://goo.gl/7Sw2DZ

Please let me know what else can be done for this patch to make it
acceptable so we can have parity for Linux.

Thanks,

RE: [PATCH v6] r8152: Add support for setting pass through MAC address on RTL8153-AD

[Mario_Limonciello]

> David,
> 
> Did you have any more thoughts about this?  I'm happy to make some other
> adjustments to the patch, if you have some recommendations.

Hi,

I just wanted to share that the maintenance BIOSes released for the Dell 
platforms with Type-C this past week enables the MAC address pass 
through feature in UEFI, so any network booted machines will offer
the auxiliary MAC to the DHCP server.  In Windows a network booted
machine will always use auxiliary MAC now.

XPS 9350 (BIOS 1.4.4): http://goo.gl/7Sw2DZ

Please let me know what else can be done for this patch to make it
acceptable so we can have parity for Linux.

Thanks,

RE: [1/2] Revert "HID: multitouch: enable palm rejection if device implements confidence usage"

[Mario_Limonciello]

> -Original Message-
> From: Jiri Kosina [mailto:ji...@kernel.org]
> Sent: Thursday, June 30, 2016 2:40 AM
> To: Limonciello, Mario 
> Cc: Hung, Allen ; Benjamin Tissoires
> ; linux-in...@vger.kernel.org; LKML
> 
> Subject: Re: [1/2] Revert "HID: multitouch: enable palm rejection if device
> implements confidence usage"
> 
> On Wed, 29 Jun 2016, Mario Limonciello wrote:
> 
> > Would you also submit this to -stable?
> > I think it should be generally applicable at least a few releases back
> > since Allen's original submit.
> 
> Both commits (already in Linus' tree as of today) contain the -stable
> kernel annotation.
> 
> --
> Jiri Kosina
> SUSE Labs

Sorry I missed that, thanks.

RE: [1/2] Revert "HID: multitouch: enable palm rejection if device implements confidence usage"

[Mario_Limonciello]

> -Original Message-
> From: Jiri Kosina [mailto:ji...@kernel.org]
> Sent: Thursday, June 30, 2016 2:40 AM
> To: Limonciello, Mario 
> Cc: Hung, Allen ; Benjamin Tissoires
> ; linux-in...@vger.kernel.org; LKML
> 
> Subject: Re: [1/2] Revert "HID: multitouch: enable palm rejection if device
> implements confidence usage"
> 
> On Wed, 29 Jun 2016, Mario Limonciello wrote:
> 
> > Would you also submit this to -stable?
> > I think it should be generally applicable at least a few releases back
> > since Allen's original submit.
> 
> Both commits (already in Linus' tree as of today) contain the -stable
> kernel annotation.
> 
> --
> Jiri Kosina
> SUSE Labs

Sorry I missed that, thanks.

RE: [PATCH] ACPI: don't show an error when we're not in charge of PCIe hotplug.

[Mario_Limonciello]

> -Original Message-
> From: rjwyso...@gmail.com [mailto:rjwyso...@gmail.com] On Behalf Of
> Rafael J. Wysocki
> Sent: Wednesday, June 22, 2016 5:48 PM
> To: Andy Lutomirski ; Limonciello, Mario
> 
> Cc: Rafael Wysocki ; Peter Jones ;
> Linux ACPI ; linux-kernel@vger.kernel.org; Len
> Brown ; Rafael J. Wysocki 
> Subject: Re: [PATCH] ACPI: don't show an error when we're not in charge of
> PCIe hotplug.
> 
> On Wed, Jun 22, 2016 at 10:53 PM, Andy Lutomirski 
> wrote:
> > On Wed, Jun 22, 2016 at 12:43 PM,   wrote:
> >>> -Original Message-
> >>> From: rjwyso...@gmail.com [mailto:rjwyso...@gmail.com] On Behalf Of
> 
> [cut]
> 
> >> I think changing that would help communicate what's going on here and at
> >> least let the user know the result will be that the firmware is still
> controlling
> >> ASPM due to the _OSC failure.
> 
> You seem to be assuming that all systems returning "unsupported UUID"
> from the PCI host bridge _OSC will always fall into the same category,
> but what if they don't?  What if at least some of them are really
> broken?
> 

Even if they are broken, the net result will be the firmware is in control
of ASPM, won't it?  At least outside of the group on this mailing list that
might not be very apparent from the current set of errors output into
logs.

> >> Something else that I think Andy recommended a while back was at that
> >> time try to evaluate NEXP and display its value and an associated message
> >> in debug logs when _OSC fails.  Would you be amenable to a change like
> that?
> >
> > That seems dangerous if NEXP is anything other than a SystemMemory
> > variable.  I don't know if there's a clean way to check that before
> > evaluating it.  (i.e. we don't want to hit some other thing called
> > NEXP that has side effects.)
> 
> Well, that's generic code and NEXP is not generic really, so agreed.

OK. 

RE: [PATCH] ACPI: don't show an error when we're not in charge of PCIe hotplug.

[Mario_Limonciello]

> -Original Message-
> From: rjwyso...@gmail.com [mailto:rjwyso...@gmail.com] On Behalf Of
> Rafael J. Wysocki
> Sent: Wednesday, June 22, 2016 5:48 PM
> To: Andy Lutomirski ; Limonciello, Mario
> 
> Cc: Rafael Wysocki ; Peter Jones ;
> Linux ACPI ; linux-kernel@vger.kernel.org; Len
> Brown ; Rafael J. Wysocki 
> Subject: Re: [PATCH] ACPI: don't show an error when we're not in charge of
> PCIe hotplug.
> 
> On Wed, Jun 22, 2016 at 10:53 PM, Andy Lutomirski 
> wrote:
> > On Wed, Jun 22, 2016 at 12:43 PM,   wrote:
> >>> -Original Message-
> >>> From: rjwyso...@gmail.com [mailto:rjwyso...@gmail.com] On Behalf Of
> 
> [cut]
> 
> >> I think changing that would help communicate what's going on here and at
> >> least let the user know the result will be that the firmware is still
> controlling
> >> ASPM due to the _OSC failure.
> 
> You seem to be assuming that all systems returning "unsupported UUID"
> from the PCI host bridge _OSC will always fall into the same category,
> but what if they don't?  What if at least some of them are really
> broken?
> 

Even if they are broken, the net result will be the firmware is in control
of ASPM, won't it?  At least outside of the group on this mailing list that
might not be very apparent from the current set of errors output into
logs.

> >> Something else that I think Andy recommended a while back was at that
> >> time try to evaluate NEXP and display its value and an associated message
> >> in debug logs when _OSC fails.  Would you be amenable to a change like
> that?
> >
> > That seems dangerous if NEXP is anything other than a SystemMemory
> > variable.  I don't know if there's a clean way to check that before
> > evaluating it.  (i.e. we don't want to hit some other thing called
> > NEXP that has side effects.)
> 
> Well, that's generic code and NEXP is not generic really, so agreed.

OK. 

RE: [PATCH v6] r8152: Add support for setting pass through MAC address on RTL8153-AD

[Mario_Limonciello]

> -Original Message-
> From: Limonciello, Mario
> Sent: Tuesday, June 14, 2016 5:27 PM
> To: 'David Miller' ; pali.ro...@gmail.com
> Cc: gre...@linuxfoundation.org; and...@lunn.ch;
> hayesw...@realtek.com; linux-kernel@vger.kernel.org;
> net...@vger.kernel.org; linux-...@vger.kernel.org;
> anthony.w...@canonical.com
> Subject: RE: [PATCH v6] r8152: Add support for setting pass through MAC
> address on RTL8153-AD
> 
> > -Original Message-
> > From: David Miller [mailto:da...@davemloft.net]
> > Sent: Tuesday, June 14, 2016 1:35 PM
> > To: pali.ro...@gmail.com
> > Cc: gre...@linuxfoundation.org; and...@lunn.ch; Limonciello, Mario
> > ; hayesw...@realtek.com; linux-
> > ker...@vger.kernel.org; net...@vger.kernel.org; linux-
> > u...@vger.kernel.org; anthony.w...@canonical.com
> > Subject: Re: [PATCH v6] r8152: Add support for setting pass through MAC
> > address on RTL8153-AD
> >
> > From: Pali Rohár 
> > Date: Tue, 14 Jun 2016 18:47:36 +0200
> >
> > > You have never seen two ethernet cards with same MAC addresses?
> Right
> > I
> > > have not seen two USB, but there is non zero chance that could happen.
> >
> > It would be an error scenerio, and something to be avoided.
> >
> > It is a valid and correct assumption that one is able to put
> > several devices at the same time on the same physical network
> > and expect it to work.
> >
> > The behavior added by the change in question invalidates that.
> >
> > I'm trying to consider the long term aspects of this, which is that if
> > more devices adopt this scheme we're in trouble if we blindly
> > interpret the MAC address in this way.
> >
> 
> Do you mean if other manufacturers start to ship devices with
> RTL8135-AD's w/ this pass through bit set and people start to try to
> mix and match?
> 
> > This firmware MAC property facility seems to be designed with only an
> > extremely narrow use case being considered.
> 
> Yes, as I understand it this is the reason that it's only on such specific 
> devices
> that the mac address pass through bit is actually set on the efuse.

David,

Did you have any more thoughts about this?  I'm happy to make some other
adjustments to the patch, if you have some recommendations.

RE: [PATCH v6] r8152: Add support for setting pass through MAC address on RTL8153-AD

[Mario_Limonciello]

> -Original Message-
> From: Limonciello, Mario
> Sent: Tuesday, June 14, 2016 5:27 PM
> To: 'David Miller' ; pali.ro...@gmail.com
> Cc: gre...@linuxfoundation.org; and...@lunn.ch;
> hayesw...@realtek.com; linux-kernel@vger.kernel.org;
> net...@vger.kernel.org; linux-...@vger.kernel.org;
> anthony.w...@canonical.com
> Subject: RE: [PATCH v6] r8152: Add support for setting pass through MAC
> address on RTL8153-AD
> 
> > -Original Message-
> > From: David Miller [mailto:da...@davemloft.net]
> > Sent: Tuesday, June 14, 2016 1:35 PM
> > To: pali.ro...@gmail.com
> > Cc: gre...@linuxfoundation.org; and...@lunn.ch; Limonciello, Mario
> > ; hayesw...@realtek.com; linux-
> > ker...@vger.kernel.org; net...@vger.kernel.org; linux-
> > u...@vger.kernel.org; anthony.w...@canonical.com
> > Subject: Re: [PATCH v6] r8152: Add support for setting pass through MAC
> > address on RTL8153-AD
> >
> > From: Pali Rohár 
> > Date: Tue, 14 Jun 2016 18:47:36 +0200
> >
> > > You have never seen two ethernet cards with same MAC addresses?
> Right
> > I
> > > have not seen two USB, but there is non zero chance that could happen.
> >
> > It would be an error scenerio, and something to be avoided.
> >
> > It is a valid and correct assumption that one is able to put
> > several devices at the same time on the same physical network
> > and expect it to work.
> >
> > The behavior added by the change in question invalidates that.
> >
> > I'm trying to consider the long term aspects of this, which is that if
> > more devices adopt this scheme we're in trouble if we blindly
> > interpret the MAC address in this way.
> >
> 
> Do you mean if other manufacturers start to ship devices with
> RTL8135-AD's w/ this pass through bit set and people start to try to
> mix and match?
> 
> > This firmware MAC property facility seems to be designed with only an
> > extremely narrow use case being considered.
> 
> Yes, as I understand it this is the reason that it's only on such specific 
> devices
> that the mac address pass through bit is actually set on the efuse.

David,

Did you have any more thoughts about this?  I'm happy to make some other
adjustments to the patch, if you have some recommendations.

RE: [PATCH] ACPI: don't show an error when we're not in charge of PCIe hotplug.

[Mario_Limonciello]

> -Original Message-
> From: rjwyso...@gmail.com [mailto:rjwyso...@gmail.com] On Behalf Of
> Rafael J. Wysocki
> Sent: Tuesday, June 21, 2016 5:51 PM
> To: Limonciello, Mario 
> Cc: Peter Jones ; Rafael J. Wysocki
> ; ACPI Devel Maling List ;
> Linux Kernel Mailing List ; Len Brown
> ; Rafael J. Wysocki ; Andy Lutomirski
> 
> Subject: Re: [PATCH] ACPI: don't show an error when we're not in charge of
> PCIe hotplug.
> 
> On Tue, Jun 21, 2016 at 8:01 PM,   wrote:
> >> -Original Message-
> >> From: Peter Jones [mailto:pjo...@redhat.com]
> >> Sent: Tuesday, June 21, 2016 10:19 AM
> >> To: Rafael J. Wysocki 
> >> Cc: ACPI Devel Maling List ; Limonciello,
> Mario
> >> ; Linux Kernel Mailing List  >> ker...@vger.kernel.org>; Len Brown ; Rafael J .
> Wysocki
> >> ; Andy Lutomirski 
> >> Subject: Re: [PATCH] ACPI: don't show an error when we're not in charge
> of
> >> PCIe hotplug.
> >>
> >> (Sorry for the slow response - it's deadline time over here.)
> >>
> >> On Thu, Jun 16, 2016 at 04:56:57PM +0200, Rafael J. Wysocki wrote:
> >> > On Thu, Jun 16, 2016 at 2:12 AM, Rafael J. Wysocki 
> >> wrote:
> >> > > On Thu, Jun 16, 2016 at 12:15 AM, Peter Jones 
> >> wrote:
> >> > >> Right now when booting, on many laptops the firmware manages
> the
> >> PCIe
> >> > >> bus.  As a result, when we call the _OSC ACPI method, it returns an
> >> > >> error code.  Unfortunately the errors are not very articulate.
> >> > >
> >> > > What exactly do you mean here?
> >> > >
> >> > >>  As a result, we show:
> >> > >>
> >> > >> ACPI: PCI Root Bridge [PCI0] (domain  [bus 00-fe])
> >> > >> acpi PNP0A08:00: _OSC: OS supports [ExtendedConfig ASPM
> ClockPM
> >> Segments MSI]
> >> > >> \_SB_.PCI0 (33DB4D5B-1FF7-401C-9657-7441C03DD766): _OSC invalid
> >> UUID
> >> > >> _OSC request data: 1 1f 0
> >> > >
> >> > > So _OSC told us that the UUID was invalid, didn't it?
> >> >
> >> > BTW, the above messages are KERN_DEBUG, so at least in theory they
> >> > shouldn't be visible in production runs.
> >> >
> >> > Maybe the bug to fix is that they show up when they aren't supposed
> to?
> >>
> >> No - the workflow that I am really trying to remedy is this:
> >>
> >>  1) S3 resume sometimes isn't working on some laptop you've got.
> >>  2) start looking at debug messages
> >>  3) this shows an error, so it looks like it's probably the problem
> >>  4) go fishing for red herring
> >>  5) if you happen to know who maintains the DSDT for the platform in
> >> question, eventually work out that this is working as intended and
> >> the bug is someplace else.
> >> 5b) if you don't know that person, eventually work out that it /might/
> >>  be someplace else...
> >>
> >> So the idea was to make it look more like an indication of status, and
> >> less like an error that's causing unrelated problems.
> >>
> >> When I talked to Mario at Dell (Cc'd), it wasn't clear to us that
> >> there's a way to distinguish the between the UUID being
> >> invalid/malformed, being merely unsupported, or being supported in
> some
> >> configurations but not the current one.  In this particular DSDT, the
> >> machine doesn't support the OS controlling any of this if USB-C /
> >> thunderbolt are enabled.  The DSDT is clearly written with the belief
> >> that you have to completely disable the handling for that UUID in this
> >> case, and googling for this looks like it's not the only one written
> >> with that belief.
> >>
> >> Reading the spec (v6.1, sections 6.2.11.3 and 6.2.11.4), it seems
> >> plausible that you can express this instead by handling the UUID but
> >> choosing each individual query/status bit in the way that accomplishes
> >> the OS doing nothing with the response.  So it may well be that that's
> >> just more code that vendors have thought wasn't necessary (or wasn't
> >> correct for some reason.)
> >>
> >> Mario, want to jump in on your thinking here?
> >>
> >> --
> >>   Peter
> >
> > After talking to the team, I was told this particular implementation to not
> let
> > OS take control when acting on that specific UUID based upon a variable
> > (NEXP in this case) came from Intel RC code.
> >
> > That's probably why this is all across a lot of platforms, including 
> > non-Dell.
> >
> > At least in the context of the laptop Peter noticed this on (Dell XPS 13 
> > 9350)
> > NEXP is set in GNVS based upon Thunderbolt capability.
> >
> > As for why they return unrecognized UUID instead of just masking all the
> > capabilities bits?  It's the same net functional result.  If the vendor 
> > provided
> > RC code doesn't caused WCHK problems or functional 

RE: [PATCH] ACPI: don't show an error when we're not in charge of PCIe hotplug.

[Mario_Limonciello]

> -Original Message-
> From: rjwyso...@gmail.com [mailto:rjwyso...@gmail.com] On Behalf Of
> Rafael J. Wysocki
> Sent: Tuesday, June 21, 2016 5:51 PM
> To: Limonciello, Mario 
> Cc: Peter Jones ; Rafael J. Wysocki
> ; ACPI Devel Maling List ;
> Linux Kernel Mailing List ; Len Brown
> ; Rafael J. Wysocki ; Andy Lutomirski
> 
> Subject: Re: [PATCH] ACPI: don't show an error when we're not in charge of
> PCIe hotplug.
> 
> On Tue, Jun 21, 2016 at 8:01 PM,   wrote:
> >> -Original Message-
> >> From: Peter Jones [mailto:pjo...@redhat.com]
> >> Sent: Tuesday, June 21, 2016 10:19 AM
> >> To: Rafael J. Wysocki 
> >> Cc: ACPI Devel Maling List ; Limonciello,
> Mario
> >> ; Linux Kernel Mailing List  >> ker...@vger.kernel.org>; Len Brown ; Rafael J .
> Wysocki
> >> ; Andy Lutomirski 
> >> Subject: Re: [PATCH] ACPI: don't show an error when we're not in charge
> of
> >> PCIe hotplug.
> >>
> >> (Sorry for the slow response - it's deadline time over here.)
> >>
> >> On Thu, Jun 16, 2016 at 04:56:57PM +0200, Rafael J. Wysocki wrote:
> >> > On Thu, Jun 16, 2016 at 2:12 AM, Rafael J. Wysocki 
> >> wrote:
> >> > > On Thu, Jun 16, 2016 at 12:15 AM, Peter Jones 
> >> wrote:
> >> > >> Right now when booting, on many laptops the firmware manages
> the
> >> PCIe
> >> > >> bus.  As a result, when we call the _OSC ACPI method, it returns an
> >> > >> error code.  Unfortunately the errors are not very articulate.
> >> > >
> >> > > What exactly do you mean here?
> >> > >
> >> > >>  As a result, we show:
> >> > >>
> >> > >> ACPI: PCI Root Bridge [PCI0] (domain  [bus 00-fe])
> >> > >> acpi PNP0A08:00: _OSC: OS supports [ExtendedConfig ASPM
> ClockPM
> >> Segments MSI]
> >> > >> \_SB_.PCI0 (33DB4D5B-1FF7-401C-9657-7441C03DD766): _OSC invalid
> >> UUID
> >> > >> _OSC request data: 1 1f 0
> >> > >
> >> > > So _OSC told us that the UUID was invalid, didn't it?
> >> >
> >> > BTW, the above messages are KERN_DEBUG, so at least in theory they
> >> > shouldn't be visible in production runs.
> >> >
> >> > Maybe the bug to fix is that they show up when they aren't supposed
> to?
> >>
> >> No - the workflow that I am really trying to remedy is this:
> >>
> >>  1) S3 resume sometimes isn't working on some laptop you've got.
> >>  2) start looking at debug messages
> >>  3) this shows an error, so it looks like it's probably the problem
> >>  4) go fishing for red herring
> >>  5) if you happen to know who maintains the DSDT for the platform in
> >> question, eventually work out that this is working as intended and
> >> the bug is someplace else.
> >> 5b) if you don't know that person, eventually work out that it /might/
> >>  be someplace else...
> >>
> >> So the idea was to make it look more like an indication of status, and
> >> less like an error that's causing unrelated problems.
> >>
> >> When I talked to Mario at Dell (Cc'd), it wasn't clear to us that
> >> there's a way to distinguish the between the UUID being
> >> invalid/malformed, being merely unsupported, or being supported in
> some
> >> configurations but not the current one.  In this particular DSDT, the
> >> machine doesn't support the OS controlling any of this if USB-C /
> >> thunderbolt are enabled.  The DSDT is clearly written with the belief
> >> that you have to completely disable the handling for that UUID in this
> >> case, and googling for this looks like it's not the only one written
> >> with that belief.
> >>
> >> Reading the spec (v6.1, sections 6.2.11.3 and 6.2.11.4), it seems
> >> plausible that you can express this instead by handling the UUID but
> >> choosing each individual query/status bit in the way that accomplishes
> >> the OS doing nothing with the response.  So it may well be that that's
> >> just more code that vendors have thought wasn't necessary (or wasn't
> >> correct for some reason.)
> >>
> >> Mario, want to jump in on your thinking here?
> >>
> >> --
> >>   Peter
> >
> > After talking to the team, I was told this particular implementation to not
> let
> > OS take control when acting on that specific UUID based upon a variable
> > (NEXP in this case) came from Intel RC code.
> >
> > That's probably why this is all across a lot of platforms, including 
> > non-Dell.
> >
> > At least in the context of the laptop Peter noticed this on (Dell XPS 13 
> > 9350)
> > NEXP is set in GNVS based upon Thunderbolt capability.
> >
> > As for why they return unrecognized UUID instead of just masking all the
> > capabilities bits?  It's the same net functional result.  If the vendor 
> > provided
> > RC code doesn't caused WCHK problems or functional problems it's hard to
> > make a case for why it needs to be changed by the OEM.
> >
> > I think that Peter's patch is appropriate to message this is specifically
> > what's going on.
> 
> No, it may hide real (ie. non-intentional) bugs in _OSC, so it is not
> appropriate.
> 
> Debug-level messages really should not hurt anyone (and should never
> show up in production anyway).
> 
> We 

RE: [PATCH 3/4] dell-wmi: Add information about other WMI event codes

[Mario_Limonciello]

> -Original Message-
> From: Gabriele Mazzotta [mailto:gabriele@gmail.com]
> Sent: Wednesday, June 22, 2016 9:40 AM
> To: Limonciello, Mario ; pali.ro...@gmail.com
> Cc: mj...@srcf.ucam.org; dvh...@infradead.org; ker...@kempniu.pl;
> l...@kernel.org; alex.h...@canonical.com; platform-driver-
> x...@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 3/4] dell-wmi: Add information about other WMI event
> codes
> 
> On 22/06/2016 16:21, mario_limoncie...@dell.com wrote:
> >> -Original Message-
> >> From: Pali Rohár [mailto:pali.ro...@gmail.com]
> >> Sent: Wednesday, June 22, 2016 9:13 AM
> >> To: Limonciello, Mario 
> >> Cc: gabriele@gmail.com; mj...@srcf.ucam.org;
> dvh...@infradead.org;
> >> ker...@kempniu.pl; l...@kernel.org; alex.h...@canonical.com;
> platform-
> >> driver-...@vger.kernel.org; linux-kernel@vger.kernel.org
> >> Subject: Re: [PATCH 3/4] dell-wmi: Add information about other WMI
> event
> >> codes
> >>
> >> On Wednesday 22 June 2016 13:40:57 mario_limoncie...@dell.com
> wrote:
> > You aren't seeing this on the DSDT of your Latitude right?
> 
>  Yes, I do not see it on Latitude.
> >>>
> >>> Thanks, the usage of this scan code is specific to consumer BIOSes.
> >>>
> 
> > Gabriele,
> >
> > Your machine is from the year before XPS switched over to running
> the
> > Dell business client (eg Latitude, Precision, Optiplex) BIOS.
> >
> > The EC in that machine does have support for "Battery Health" via that
> > scancode.  On Windows it's used for relaying battery information to an
> > application called Quick Set.
> 
>  Do you have some details when it is send to OS? And how to read that
> >> that
>  "battery health"?
> >>>
> >>> When a battery is removed or inserted this event is supposed to be
> >> received
> >>> by quickset over WMI and then Quickset will re-read battery
> information.
> >>
> >> So event is sent only if battery is removed or inserted?
> >>
> >
> > Yeah, that's what my spec says, I haven't tested this on actual system to
> see.
> >
> > I'm guessing what's going on is that during suspend ACPI battery drops
> > off system and comes back up on resume.
> >
> > Maybe Gabriele can comment if any other times were noticed, but in any
> > case I think it's appropriate for dell-wmi driver when receiving this on WMI
> > to not do anything.  Sending KEY_BATTERY would be wrong behavior.
> 
> I think I saw the event only after resume, but I don't read my dmesg
> that often to notice other special cases. Surely it's not related to
> any hotkey nor actual battery removal.
> 
> FYI I have a "battery button" and the associated code is 0xe007. I
> guess most of the laptop nowadays use that code for QuickSet, given
> that the entry for it was added to dell-wmi.c back in 2009.

Ah yes to make sure that's clear - that battery button is 
"Battery Health Meter". For QuickSet that function was for just showing
a little popup with remaining battery life.  For Linux I do think KEY_BATTERY
is still appropriate there.

> 
> I would also like to remind that my laptop receives four WMI events
> with code 0xe00e after resume. If we send an input event for each
> WMI event with code 0xe00e, I'd get four bogus button keypresses.

Yep, exactly why dell-wmi should just ignore this code.

RE: [PATCH 3/4] dell-wmi: Add information about other WMI event codes

[Mario_Limonciello]

> -Original Message-
> From: Gabriele Mazzotta [mailto:gabriele@gmail.com]
> Sent: Wednesday, June 22, 2016 9:40 AM
> To: Limonciello, Mario ; pali.ro...@gmail.com
> Cc: mj...@srcf.ucam.org; dvh...@infradead.org; ker...@kempniu.pl;
> l...@kernel.org; alex.h...@canonical.com; platform-driver-
> x...@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 3/4] dell-wmi: Add information about other WMI event
> codes
> 
> On 22/06/2016 16:21, mario_limoncie...@dell.com wrote:
> >> -Original Message-
> >> From: Pali Rohár [mailto:pali.ro...@gmail.com]
> >> Sent: Wednesday, June 22, 2016 9:13 AM
> >> To: Limonciello, Mario 
> >> Cc: gabriele@gmail.com; mj...@srcf.ucam.org;
> dvh...@infradead.org;
> >> ker...@kempniu.pl; l...@kernel.org; alex.h...@canonical.com;
> platform-
> >> driver-...@vger.kernel.org; linux-kernel@vger.kernel.org
> >> Subject: Re: [PATCH 3/4] dell-wmi: Add information about other WMI
> event
> >> codes
> >>
> >> On Wednesday 22 June 2016 13:40:57 mario_limoncie...@dell.com
> wrote:
> > You aren't seeing this on the DSDT of your Latitude right?
> 
>  Yes, I do not see it on Latitude.
> >>>
> >>> Thanks, the usage of this scan code is specific to consumer BIOSes.
> >>>
> 
> > Gabriele,
> >
> > Your machine is from the year before XPS switched over to running
> the
> > Dell business client (eg Latitude, Precision, Optiplex) BIOS.
> >
> > The EC in that machine does have support for "Battery Health" via that
> > scancode.  On Windows it's used for relaying battery information to an
> > application called Quick Set.
> 
>  Do you have some details when it is send to OS? And how to read that
> >> that
>  "battery health"?
> >>>
> >>> When a battery is removed or inserted this event is supposed to be
> >> received
> >>> by quickset over WMI and then Quickset will re-read battery
> information.
> >>
> >> So event is sent only if battery is removed or inserted?
> >>
> >
> > Yeah, that's what my spec says, I haven't tested this on actual system to
> see.
> >
> > I'm guessing what's going on is that during suspend ACPI battery drops
> > off system and comes back up on resume.
> >
> > Maybe Gabriele can comment if any other times were noticed, but in any
> > case I think it's appropriate for dell-wmi driver when receiving this on WMI
> > to not do anything.  Sending KEY_BATTERY would be wrong behavior.
> 
> I think I saw the event only after resume, but I don't read my dmesg
> that often to notice other special cases. Surely it's not related to
> any hotkey nor actual battery removal.
> 
> FYI I have a "battery button" and the associated code is 0xe007. I
> guess most of the laptop nowadays use that code for QuickSet, given
> that the entry for it was added to dell-wmi.c back in 2009.

Ah yes to make sure that's clear - that battery button is 
"Battery Health Meter". For QuickSet that function was for just showing
a little popup with remaining battery life.  For Linux I do think KEY_BATTERY
is still appropriate there.

> 
> I would also like to remind that my laptop receives four WMI events
> with code 0xe00e after resume. If we send an input event for each
> WMI event with code 0xe00e, I'd get four bogus button keypresses.

Yep, exactly why dell-wmi should just ignore this code.

RE: [PATCH 3/4] dell-wmi: Add information about other WMI event codes

[Mario_Limonciello]

> -Original Message-
> From: Pali Rohár [mailto:pali.ro...@gmail.com]
> Sent: Wednesday, June 22, 2016 9:31 AM
> To: Limonciello, Mario 
> Cc: gabriele@gmail.com; mj...@srcf.ucam.org; dvh...@infradead.org;
> ker...@kempniu.pl; l...@kernel.org; alex.h...@canonical.com; platform-
> driver-...@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 3/4] dell-wmi: Add information about other WMI event
> codes
> 
> On Wednesday 22 June 2016 14:28:37 mario_limoncie...@dell.com wrote:
> > > -Original Message-
> > > From: Pali Rohár [mailto:pali.ro...@gmail.com]
> > > Sent: Wednesday, June 22, 2016 9:24 AM
> > > To: Limonciello, Mario 
> > > Cc: gabriele@gmail.com; mj...@srcf.ucam.org;
> dvh...@infradead.org;
> > > ker...@kempniu.pl; l...@kernel.org; alex.h...@canonical.com;
> platform-
> > > driver-...@vger.kernel.org; linux-kernel@vger.kernel.org
> > > Subject: Re: [PATCH 3/4] dell-wmi: Add information about other WMI
> event
> > > codes
> > >
> > > On Wednesday 22 June 2016 14:21:25 mario_limoncie...@dell.com
> wrote:
> > > > > > For Linux I don’t think this is necessary and a NOOP is appropriate.
> > > > > >
> > > > > > There is also a second place that some older laptops had a battery
> > > "hotkey"
> > > > > > that would also emit 0xE00E.  This was also picked up by quickset
> and
> > > would
> > > > > > show battery information.
> > > > > >
> > > > > > This shouldn't be blocked by kernel, I'd expect if someone wants to
> > > bind
> > > > > this
> > > > > > to another application from userspace they should be able to.
> > > > >
> > > > > Great! Can I send patch after which 0xe00e will be send to input layer
> as
> > > > > event KEY_BATTERY?
> > > > >
> > > >
> > > > Well that's why I was mentioning this in two places.  If it's received 
> > > > from
> > > keyboard
> > > > recoding it as KEY_BATTERY sounds appropriate to me.  If it's received
> from
> > > WMI,
> > > > it really shouldn't be set as anything for Linux to do.
> > > >
> > > > I don't think any apps will benefit from the notification to re-read
> battery
> > > > Information today.
> > >
> > > I mean for dell-wmi.c code.
> > > Why it should not be set to anything on Linux?
> > >
> >
> > On Windows Quickset is a separate application from built-in Windows
> battery
> > applet.  It shows similar types of information as gnome-power-manager
> does,
> > but because it's not part of the OS, it doesn't get notifications like this 
> > so
> > special BIOS hooks are in place for it to know when to re-read battery
> > information.
> >
> > On Linux, there are already infrastructure for battery removal and adding
> > notifications from ACPI.  Sending this up to user space won't be useful.
> 
> Errr. I mean WMI event 0xe00e which you wrote is emitted when user press
> battery key. And I think this event should be sent by dell-wmi.c as
> KEY_BATTERY.
> 

No, I mean 0xe00e scancode is emitted when user presses the battery key on
older laptops.  I'm only mentioning it because it happens to have the same
scancode as is received for WMI for another purpose and I wanted to make sure
it was clear what both are for.  
This button press isn't emitted via WMI, it's over standard serio.

The event received over WMI for battery refresh that shares the same code 
shouldn't
be emitted as KEY_BATTERY to the OS is all I was trying to say.

RE: [PATCH 3/4] dell-wmi: Add information about other WMI event codes

[Mario_Limonciello]

> -Original Message-
> From: Pali Rohár [mailto:pali.ro...@gmail.com]
> Sent: Wednesday, June 22, 2016 9:31 AM
> To: Limonciello, Mario 
> Cc: gabriele@gmail.com; mj...@srcf.ucam.org; dvh...@infradead.org;
> ker...@kempniu.pl; l...@kernel.org; alex.h...@canonical.com; platform-
> driver-...@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 3/4] dell-wmi: Add information about other WMI event
> codes
> 
> On Wednesday 22 June 2016 14:28:37 mario_limoncie...@dell.com wrote:
> > > -Original Message-
> > > From: Pali Rohár [mailto:pali.ro...@gmail.com]
> > > Sent: Wednesday, June 22, 2016 9:24 AM
> > > To: Limonciello, Mario 
> > > Cc: gabriele@gmail.com; mj...@srcf.ucam.org;
> dvh...@infradead.org;
> > > ker...@kempniu.pl; l...@kernel.org; alex.h...@canonical.com;
> platform-
> > > driver-...@vger.kernel.org; linux-kernel@vger.kernel.org
> > > Subject: Re: [PATCH 3/4] dell-wmi: Add information about other WMI
> event
> > > codes
> > >
> > > On Wednesday 22 June 2016 14:21:25 mario_limoncie...@dell.com
> wrote:
> > > > > > For Linux I don’t think this is necessary and a NOOP is appropriate.
> > > > > >
> > > > > > There is also a second place that some older laptops had a battery
> > > "hotkey"
> > > > > > that would also emit 0xE00E.  This was also picked up by quickset
> and
> > > would
> > > > > > show battery information.
> > > > > >
> > > > > > This shouldn't be blocked by kernel, I'd expect if someone wants to
> > > bind
> > > > > this
> > > > > > to another application from userspace they should be able to.
> > > > >
> > > > > Great! Can I send patch after which 0xe00e will be send to input layer
> as
> > > > > event KEY_BATTERY?
> > > > >
> > > >
> > > > Well that's why I was mentioning this in two places.  If it's received 
> > > > from
> > > keyboard
> > > > recoding it as KEY_BATTERY sounds appropriate to me.  If it's received
> from
> > > WMI,
> > > > it really shouldn't be set as anything for Linux to do.
> > > >
> > > > I don't think any apps will benefit from the notification to re-read
> battery
> > > > Information today.
> > >
> > > I mean for dell-wmi.c code.
> > > Why it should not be set to anything on Linux?
> > >
> >
> > On Windows Quickset is a separate application from built-in Windows
> battery
> > applet.  It shows similar types of information as gnome-power-manager
> does,
> > but because it's not part of the OS, it doesn't get notifications like this 
> > so
> > special BIOS hooks are in place for it to know when to re-read battery
> > information.
> >
> > On Linux, there are already infrastructure for battery removal and adding
> > notifications from ACPI.  Sending this up to user space won't be useful.
> 
> Errr. I mean WMI event 0xe00e which you wrote is emitted when user press
> battery key. And I think this event should be sent by dell-wmi.c as
> KEY_BATTERY.
> 

No, I mean 0xe00e scancode is emitted when user presses the battery key on
older laptops.  I'm only mentioning it because it happens to have the same
scancode as is received for WMI for another purpose and I wanted to make sure
it was clear what both are for.  
This button press isn't emitted via WMI, it's over standard serio.

The event received over WMI for battery refresh that shares the same code 
shouldn't
be emitted as KEY_BATTERY to the OS is all I was trying to say.

RE: [PATCH 3/4] dell-wmi: Add information about other WMI event codes

[Mario_Limonciello]

> -Original Message-
> From: Pali Rohár [mailto:pali.ro...@gmail.com]
> Sent: Wednesday, June 22, 2016 9:24 AM
> To: Limonciello, Mario 
> Cc: gabriele@gmail.com; mj...@srcf.ucam.org; dvh...@infradead.org;
> ker...@kempniu.pl; l...@kernel.org; alex.h...@canonical.com; platform-
> driver-...@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 3/4] dell-wmi: Add information about other WMI event
> codes
> 
> On Wednesday 22 June 2016 14:21:25 mario_limoncie...@dell.com wrote:
> > > > For Linux I don’t think this is necessary and a NOOP is appropriate.
> > > >
> > > > There is also a second place that some older laptops had a battery
> "hotkey"
> > > > that would also emit 0xE00E.  This was also picked up by quickset and
> would
> > > > show battery information.
> > > >
> > > > This shouldn't be blocked by kernel, I'd expect if someone wants to
> bind
> > > this
> > > > to another application from userspace they should be able to.
> > >
> > > Great! Can I send patch after which 0xe00e will be send to input layer as
> > > event KEY_BATTERY?
> > >
> >
> > Well that's why I was mentioning this in two places.  If it's received from
> keyboard
> > recoding it as KEY_BATTERY sounds appropriate to me.  If it's received from
> WMI,
> > it really shouldn't be set as anything for Linux to do.
> >
> > I don't think any apps will benefit from the notification to re-read battery
> > Information today.
> 
> I mean for dell-wmi.c code.
> Why it should not be set to anything on Linux?
> 

On Windows Quickset is a separate application from built-in Windows battery 
applet.  It shows similar types of information as gnome-power-manager does, 
but because it's not part of the OS, it doesn't get notifications like this so 
special BIOS hooks are in place for it to know when to re-read battery
information.

On Linux, there are already infrastructure for battery removal and adding 
notifications from ACPI.  Sending this up to user space won't be useful.

RE: [PATCH 3/4] dell-wmi: Add information about other WMI event codes

[Mario_Limonciello]

> -Original Message-
> From: Pali Rohár [mailto:pali.ro...@gmail.com]
> Sent: Wednesday, June 22, 2016 9:24 AM
> To: Limonciello, Mario 
> Cc: gabriele@gmail.com; mj...@srcf.ucam.org; dvh...@infradead.org;
> ker...@kempniu.pl; l...@kernel.org; alex.h...@canonical.com; platform-
> driver-...@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 3/4] dell-wmi: Add information about other WMI event
> codes
> 
> On Wednesday 22 June 2016 14:21:25 mario_limoncie...@dell.com wrote:
> > > > For Linux I don’t think this is necessary and a NOOP is appropriate.
> > > >
> > > > There is also a second place that some older laptops had a battery
> "hotkey"
> > > > that would also emit 0xE00E.  This was also picked up by quickset and
> would
> > > > show battery information.
> > > >
> > > > This shouldn't be blocked by kernel, I'd expect if someone wants to
> bind
> > > this
> > > > to another application from userspace they should be able to.
> > >
> > > Great! Can I send patch after which 0xe00e will be send to input layer as
> > > event KEY_BATTERY?
> > >
> >
> > Well that's why I was mentioning this in two places.  If it's received from
> keyboard
> > recoding it as KEY_BATTERY sounds appropriate to me.  If it's received from
> WMI,
> > it really shouldn't be set as anything for Linux to do.
> >
> > I don't think any apps will benefit from the notification to re-read battery
> > Information today.
> 
> I mean for dell-wmi.c code.
> Why it should not be set to anything on Linux?
> 

On Windows Quickset is a separate application from built-in Windows battery 
applet.  It shows similar types of information as gnome-power-manager does, 
but because it's not part of the OS, it doesn't get notifications like this so 
special BIOS hooks are in place for it to know when to re-read battery
information.

On Linux, there are already infrastructure for battery removal and adding 
notifications from ACPI.  Sending this up to user space won't be useful.

RE: [PATCH 3/4] dell-wmi: Add information about other WMI event codes

[Mario_Limonciello]

> -Original Message-
> From: Pali Rohár [mailto:pali.ro...@gmail.com]
> Sent: Wednesday, June 22, 2016 9:13 AM
> To: Limonciello, Mario 
> Cc: gabriele@gmail.com; mj...@srcf.ucam.org; dvh...@infradead.org;
> ker...@kempniu.pl; l...@kernel.org; alex.h...@canonical.com; platform-
> driver-...@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 3/4] dell-wmi: Add information about other WMI event
> codes
> 
> On Wednesday 22 June 2016 13:40:57 mario_limoncie...@dell.com wrote:
> > > > You aren't seeing this on the DSDT of your Latitude right?
> > >
> > > Yes, I do not see it on Latitude.
> >
> > Thanks, the usage of this scan code is specific to consumer BIOSes.
> >
> > >
> > > > Gabriele,
> > > >
> > > > Your machine is from the year before XPS switched over to running the
> > > > Dell business client (eg Latitude, Precision, Optiplex) BIOS.
> > > >
> > > > The EC in that machine does have support for "Battery Health" via that
> > > > scancode.  On Windows it's used for relaying battery information to an
> > > > application called Quick Set.
> > >
> > > Do you have some details when it is send to OS? And how to read that
> that
> > > "battery health"?
> >
> > When a battery is removed or inserted this event is supposed to be
> received
> > by quickset over WMI and then Quickset will re-read battery information.
> 
> So event is sent only if battery is removed or inserted?
> 

Yeah, that's what my spec says, I haven't tested this on actual system to see.

I'm guessing what's going on is that during suspend ACPI battery drops
off system and comes back up on resume.

Maybe Gabriele can comment if any other times were noticed, but in any
case I think it's appropriate for dell-wmi driver when receiving this on WMI
to not do anything.  Sending KEY_BATTERY would be wrong behavior.

> > For Linux I don’t think this is necessary and a NOOP is appropriate.
> >
> > There is also a second place that some older laptops had a battery "hotkey"
> > that would also emit 0xE00E.  This was also picked up by quickset and would
> > show battery information.
> >
> > This shouldn't be blocked by kernel, I'd expect if someone wants to bind
> this
> > to another application from userspace they should be able to.
> 
> Great! Can I send patch after which 0xe00e will be send to input layer as
> event KEY_BATTERY?
> 

Well that's why I was mentioning this in two places.  If it's received from 
keyboard
recoding it as KEY_BATTERY sounds appropriate to me.  If it's received from WMI,
it really shouldn't be set as anything for Linux to do.

I don't think any apps will benefit from the notification to re-read battery
Information today.

RE: [PATCH 3/4] dell-wmi: Add information about other WMI event codes

[Mario_Limonciello]

> -Original Message-
> From: Pali Rohár [mailto:pali.ro...@gmail.com]
> Sent: Wednesday, June 22, 2016 9:13 AM
> To: Limonciello, Mario 
> Cc: gabriele@gmail.com; mj...@srcf.ucam.org; dvh...@infradead.org;
> ker...@kempniu.pl; l...@kernel.org; alex.h...@canonical.com; platform-
> driver-...@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 3/4] dell-wmi: Add information about other WMI event
> codes
> 
> On Wednesday 22 June 2016 13:40:57 mario_limoncie...@dell.com wrote:
> > > > You aren't seeing this on the DSDT of your Latitude right?
> > >
> > > Yes, I do not see it on Latitude.
> >
> > Thanks, the usage of this scan code is specific to consumer BIOSes.
> >
> > >
> > > > Gabriele,
> > > >
> > > > Your machine is from the year before XPS switched over to running the
> > > > Dell business client (eg Latitude, Precision, Optiplex) BIOS.
> > > >
> > > > The EC in that machine does have support for "Battery Health" via that
> > > > scancode.  On Windows it's used for relaying battery information to an
> > > > application called Quick Set.
> > >
> > > Do you have some details when it is send to OS? And how to read that
> that
> > > "battery health"?
> >
> > When a battery is removed or inserted this event is supposed to be
> received
> > by quickset over WMI and then Quickset will re-read battery information.
> 
> So event is sent only if battery is removed or inserted?
> 

Yeah, that's what my spec says, I haven't tested this on actual system to see.

I'm guessing what's going on is that during suspend ACPI battery drops
off system and comes back up on resume.

Maybe Gabriele can comment if any other times were noticed, but in any
case I think it's appropriate for dell-wmi driver when receiving this on WMI
to not do anything.  Sending KEY_BATTERY would be wrong behavior.

> > For Linux I don’t think this is necessary and a NOOP is appropriate.
> >
> > There is also a second place that some older laptops had a battery "hotkey"
> > that would also emit 0xE00E.  This was also picked up by quickset and would
> > show battery information.
> >
> > This shouldn't be blocked by kernel, I'd expect if someone wants to bind
> this
> > to another application from userspace they should be able to.
> 
> Great! Can I send patch after which 0xe00e will be send to input layer as
> event KEY_BATTERY?
> 

Well that's why I was mentioning this in two places.  If it's received from 
keyboard
recoding it as KEY_BATTERY sounds appropriate to me.  If it's received from WMI,
it really shouldn't be set as anything for Linux to do.

I don't think any apps will benefit from the notification to re-read battery
Information today.

RE: [PATCH 3/4] dell-wmi: Add information about other WMI event codes

[Mario_Limonciello]

> > You aren't seeing this on the DSDT of your Latitude right?
> 
> Yes, I do not see it on Latitude.

Thanks, the usage of this scan code is specific to consumer BIOSes.

> 
> > Gabriele,
> >
> > Your machine is from the year before XPS switched over to running the
> > Dell business client (eg Latitude, Precision, Optiplex) BIOS.
> >
> > The EC in that machine does have support for "Battery Health" via that
> > scancode.  On Windows it's used for relaying battery information to an
> > application called Quick Set.
> 
> Do you have some details when it is send to OS? And how to read that that
> "battery health"?

When a battery is removed or inserted this event is supposed to be received
by quickset over WMI and then Quickset will re-read battery information.

For Linux I don’t think this is necessary and a NOOP is appropriate.

There is also a second place that some older laptops had a battery "hotkey"
that would also emit 0xE00E.  This was also picked up by quickset and would
show battery information.

This shouldn't be blocked by kernel, I'd expect if someone wants to bind this
to another application from userspace they should be able to.

RE: [PATCH 3/4] dell-wmi: Add information about other WMI event codes

[Mario_Limonciello]

> > You aren't seeing this on the DSDT of your Latitude right?
> 
> Yes, I do not see it on Latitude.

Thanks, the usage of this scan code is specific to consumer BIOSes.

> 
> > Gabriele,
> >
> > Your machine is from the year before XPS switched over to running the
> > Dell business client (eg Latitude, Precision, Optiplex) BIOS.
> >
> > The EC in that machine does have support for "Battery Health" via that
> > scancode.  On Windows it's used for relaying battery information to an
> > application called Quick Set.
> 
> Do you have some details when it is send to OS? And how to read that that
> "battery health"?

When a battery is removed or inserted this event is supposed to be received
by quickset over WMI and then Quickset will re-read battery information.

For Linux I don’t think this is necessary and a NOOP is appropriate.

There is also a second place that some older laptops had a battery "hotkey"
that would also emit 0xE00E.  This was also picked up by quickset and would
show battery information.

This shouldn't be blocked by kernel, I'd expect if someone wants to bind this
to another application from userspace they should be able to.

RE: [PATCH v3 0/4] dell-wmi: Changes in WMI event code handling

[Mario_Limonciello]

> > I talked to the EC team about this a while back when it was first 
> > implemented.
> > That's not possible without _OSI detection of Linux.  _OSI detection
> > could be used to relay to the EC to behave differently, but otherwise
> > the EC will have no idea what OS it's on for which way to behave.
> 
> ACPI code should not behave differently for different operating systems.
> If there is bug in kernel, report bug to kernel, subtree maintainer for 
> fixing it.
> And not doing workaround and hacks in ACPI.

This isn't ACPI code, this is EC code.

> 
> In this case there could be (standardized) ACPI function for turning off this
> nonsense functionality and supported kernel could call it.
> 

I think you might have interpreted my response differently than I intended.
I know that the Linux kernel has chosen to respond as the latest Windows
version for _OSI, and that's why it's not possible to do a different behavior
for Linux and Windows.

If there is a desire to go down the route of having different behavior for 
what the EC does in different OS'es, _OSI is only way to accomplish this.

> Is not there such ACPI function? Or Dell specific SMBIOS call?
> 

I'm not aware of any standard ACPI function or Dell function for this type 
of request.  Last time this was discussed I was told the EC would emit 
single display scan code for Windows < 7 (as Windows Vista and earlier
doesn't support super + p).  Windows > 7 (as detected  by _OSI and 
passed to EC) will emit super + p.

> > I don't remember all the history behind the switch over from a single
> > scan code To  + P, but I think it's along the lines of Windows
> > 8/Windows 10 allow you to iterate the display selection menu based
> > upon holding super and pressing P multiple times and waiting for you to 
> > stop.
> 
> Windows systems doing stupid things and fixing their bugs in ACPI is wrong. It
> broke for example this key on all other systems (Linux too).
> 

There's no bug in this behavior, it's intended behavior.

Like I said, previously display switch hotkey would immediately cycle outputs.
The behavior followed with super + p allows for OS to toggle through a menu
of outputs in this OS.

" Toggle through the projection mode (new with Windows 7)."
https://msdn.microsoft.com/en-us/library/ms971323.aspx

> > Sending a single scan code will change displays immediately, so having
> > the EC send super+p unifies the behavior of fn-f8 and super+p.
> 
> And due to this OS/kernel cannot distinguish between Fn-F8 and Super+p keys...

Which is intended behavior from system designer's perspective.

RE: [PATCH v3 0/4] dell-wmi: Changes in WMI event code handling

[Mario_Limonciello]

> > I talked to the EC team about this a while back when it was first 
> > implemented.
> > That's not possible without _OSI detection of Linux.  _OSI detection
> > could be used to relay to the EC to behave differently, but otherwise
> > the EC will have no idea what OS it's on for which way to behave.
> 
> ACPI code should not behave differently for different operating systems.
> If there is bug in kernel, report bug to kernel, subtree maintainer for 
> fixing it.
> And not doing workaround and hacks in ACPI.

This isn't ACPI code, this is EC code.

> 
> In this case there could be (standardized) ACPI function for turning off this
> nonsense functionality and supported kernel could call it.
> 

I think you might have interpreted my response differently than I intended.
I know that the Linux kernel has chosen to respond as the latest Windows
version for _OSI, and that's why it's not possible to do a different behavior
for Linux and Windows.

If there is a desire to go down the route of having different behavior for 
what the EC does in different OS'es, _OSI is only way to accomplish this.

> Is not there such ACPI function? Or Dell specific SMBIOS call?
> 

I'm not aware of any standard ACPI function or Dell function for this type 
of request.  Last time this was discussed I was told the EC would emit 
single display scan code for Windows < 7 (as Windows Vista and earlier
doesn't support super + p).  Windows > 7 (as detected  by _OSI and 
passed to EC) will emit super + p.

> > I don't remember all the history behind the switch over from a single
> > scan code To  + P, but I think it's along the lines of Windows
> > 8/Windows 10 allow you to iterate the display selection menu based
> > upon holding super and pressing P multiple times and waiting for you to 
> > stop.
> 
> Windows systems doing stupid things and fixing their bugs in ACPI is wrong. It
> broke for example this key on all other systems (Linux too).
> 

There's no bug in this behavior, it's intended behavior.

Like I said, previously display switch hotkey would immediately cycle outputs.
The behavior followed with super + p allows for OS to toggle through a menu
of outputs in this OS.

" Toggle through the projection mode (new with Windows 7)."
https://msdn.microsoft.com/en-us/library/ms971323.aspx

> > Sending a single scan code will change displays immediately, so having
> > the EC send super+p unifies the behavior of fn-f8 and super+p.
> 
> And due to this OS/kernel cannot distinguish between Fn-F8 and Super+p keys...

Which is intended behavior from system designer's perspective.

RE: [PATCH 3/4] dell-wmi: Add information about other WMI event codes

[Mario_Limonciello]

> -Original Message-
> From: Pali Rohár [mailto:pali.ro...@gmail.com]
> Sent: Wednesday, June 15, 2016 2:51 PM
> To: Limonciello, Mario 
> Cc: Gabriele Mazzotta ; mj...@srcf.ucam.org;
> dvh...@infradead.org; ker...@kempniu.pl; l...@kernel.org;
> alex.h...@canonical.com; platform-driver-...@vger.kernel.org; linux-
> ker...@vger.kernel.org
> Subject: Re: [PATCH 3/4] dell-wmi: Add information about other WMI event
> codes
> 
> On Wednesday 08 June 2016 12:44:44 Gabriele Mazzotta wrote:
> > On 08/06/2016 08:02, mario_limoncie...@dell.com wrote:
> > >> -Original Message-
> > >> From: Pali Rohár [mailto:pali.ro...@gmail.com]
> > >> Sent: Tuesday, June 7, 2016 6:00 PM
> > >> To: Gabriele Mazzotta ; Limonciello, Mario
> > >> 
> > >> Cc: Matthew Garrett ; Darren Hart
> > >> ; Michał Kępień ; Andy
> > >> Lutomirski ; Alex Hung
> > >> ; platform-driver- x...@vger.kernel.org;
> > >> linux-kernel@vger.kernel.org
> > >> Subject: Re: [PATCH 3/4] dell-wmi: Add information about other WMI
> > >> event codes
> > >>
> > >> On Friday 27 May 2016 00:04:23 Gabriele Mazzotta wrote:
> > >>> On 22/05/2016 13:36, Pali Rohár wrote:
> >  ACPI DSDT tables have defined other WMI codes, but does not
> >  contain any description when those codes are emitted. Some
> >  other codes can be found in logs on internet. In this patch are
> >  all which I saw, but lot of them are not tested properly (e.g.
> >  for duplicate events with AT keyboard). Now we have all WMI
> >  event codes at one place and in future after proper testing
> >  those codes can be correctly enabled or
> > >>
> > >> disabled...
> > >>
> >  Signed-off-by: Pali Rohár 
> >  ---
> > 
> >   drivers/platform/x86/dell-wmi.c |   32
> > >>
> > >> 
> > >>
> >   1 file changed, 32 insertions(+)
> > 
> >  diff --git a/drivers/platform/x86/dell-wmi.c
> >  b/drivers/platform/x86/dell-wmi.c index 363d927..7aac1dc 100644
> >  --- a/drivers/platform/x86/dell-wmi.c
> >  +++ b/drivers/platform/x86/dell-wmi.c
> >  @@ -110,6 +110,9 @@ static const struct key_entry
> > >>
> > >> dell_wmi_legacy_keymap[] __initconst = {
> > >>
> > /* BIOS error detected */
> > { KE_IGNORE, 0xe00d, { KEY_RESERVED } },
> > 
> >  +  /* Unknown, defined in ACPI DSDT */
> >  +  /* { KE_IGNORE, 0xe00e, { KEY_RESERVED } }, */
> >  +
> > >>>
> > >>> I'm interested in knowing what's the meaning of this 0xe00e. This
> > >>> event is sent multiple times when I suspend/resume my laptop and
> > >>> it's definitely not a keypress.
> > >>
> > >> From DSDT dumps which I have seen, I guess it could be something
> > >> with battery charging... but that is only my guess.
> > >>
> > >> Mario, do you have any idea, what these unknown events are?
> > >
> > > Off-hand I'm not sure, it would require some more digging.
> > >
> > > Can you please remind me what model numbers and BIOS combinations
> > > you have found e00e in DSDT and what context the events are
> > > actually happening? Anything released in the past two years?
> >
> > XPS13 9333, BIOS A07.
> >
> > I think I saw the event only after resuming from suspend and
> > it's sent four times in a row.
> >
> > As Pali says, it seems to be related to the battery. There are
> > three _Qxx ACPI methods in my DSDT sending this event: one stops
> > battery charging, one detaches the battery and the last one stores
> > a value on the GNVS.
> 
> Mario, were you able to identify something?
>
Pali,

You aren't seeing this on the DSDT of your Latitude right?

Gabriele,

Your machine is from the year before XPS switched over to running
the Dell business client (eg Latitude, Precision, Optiplex) BIOS.

The EC in that machine does have support for "Battery Health" via
that scancode.  On Windows it's used for relaying battery information
to an application called Quick Set.

Thanks,

RE: [PATCH 3/4] dell-wmi: Add information about other WMI event codes

[Mario_Limonciello]

> -Original Message-
> From: Pali Rohár [mailto:pali.ro...@gmail.com]
> Sent: Wednesday, June 15, 2016 2:51 PM
> To: Limonciello, Mario 
> Cc: Gabriele Mazzotta ; mj...@srcf.ucam.org;
> dvh...@infradead.org; ker...@kempniu.pl; l...@kernel.org;
> alex.h...@canonical.com; platform-driver-...@vger.kernel.org; linux-
> ker...@vger.kernel.org
> Subject: Re: [PATCH 3/4] dell-wmi: Add information about other WMI event
> codes
> 
> On Wednesday 08 June 2016 12:44:44 Gabriele Mazzotta wrote:
> > On 08/06/2016 08:02, mario_limoncie...@dell.com wrote:
> > >> -Original Message-
> > >> From: Pali Rohár [mailto:pali.ro...@gmail.com]
> > >> Sent: Tuesday, June 7, 2016 6:00 PM
> > >> To: Gabriele Mazzotta ; Limonciello, Mario
> > >> 
> > >> Cc: Matthew Garrett ; Darren Hart
> > >> ; Michał Kępień ; Andy
> > >> Lutomirski ; Alex Hung
> > >> ; platform-driver- x...@vger.kernel.org;
> > >> linux-kernel@vger.kernel.org
> > >> Subject: Re: [PATCH 3/4] dell-wmi: Add information about other WMI
> > >> event codes
> > >>
> > >> On Friday 27 May 2016 00:04:23 Gabriele Mazzotta wrote:
> > >>> On 22/05/2016 13:36, Pali Rohár wrote:
> >  ACPI DSDT tables have defined other WMI codes, but does not
> >  contain any description when those codes are emitted. Some
> >  other codes can be found in logs on internet. In this patch are
> >  all which I saw, but lot of them are not tested properly (e.g.
> >  for duplicate events with AT keyboard). Now we have all WMI
> >  event codes at one place and in future after proper testing
> >  those codes can be correctly enabled or
> > >>
> > >> disabled...
> > >>
> >  Signed-off-by: Pali Rohár 
> >  ---
> > 
> >   drivers/platform/x86/dell-wmi.c |   32
> > >>
> > >> 
> > >>
> >   1 file changed, 32 insertions(+)
> > 
> >  diff --git a/drivers/platform/x86/dell-wmi.c
> >  b/drivers/platform/x86/dell-wmi.c index 363d927..7aac1dc 100644
> >  --- a/drivers/platform/x86/dell-wmi.c
> >  +++ b/drivers/platform/x86/dell-wmi.c
> >  @@ -110,6 +110,9 @@ static const struct key_entry
> > >>
> > >> dell_wmi_legacy_keymap[] __initconst = {
> > >>
> > /* BIOS error detected */
> > { KE_IGNORE, 0xe00d, { KEY_RESERVED } },
> > 
> >  +  /* Unknown, defined in ACPI DSDT */
> >  +  /* { KE_IGNORE, 0xe00e, { KEY_RESERVED } }, */
> >  +
> > >>>
> > >>> I'm interested in knowing what's the meaning of this 0xe00e. This
> > >>> event is sent multiple times when I suspend/resume my laptop and
> > >>> it's definitely not a keypress.
> > >>
> > >> From DSDT dumps which I have seen, I guess it could be something
> > >> with battery charging... but that is only my guess.
> > >>
> > >> Mario, do you have any idea, what these unknown events are?
> > >
> > > Off-hand I'm not sure, it would require some more digging.
> > >
> > > Can you please remind me what model numbers and BIOS combinations
> > > you have found e00e in DSDT and what context the events are
> > > actually happening? Anything released in the past two years?
> >
> > XPS13 9333, BIOS A07.
> >
> > I think I saw the event only after resuming from suspend and
> > it's sent four times in a row.
> >
> > As Pali says, it seems to be related to the battery. There are
> > three _Qxx ACPI methods in my DSDT sending this event: one stops
> > battery charging, one detaches the battery and the last one stores
> > a value on the GNVS.
> 
> Mario, were you able to identify something?
>
Pali,

You aren't seeing this on the DSDT of your Latitude right?

Gabriele,

Your machine is from the year before XPS switched over to running
the Dell business client (eg Latitude, Precision, Optiplex) BIOS.

The EC in that machine does have support for "Battery Health" via
that scancode.  On Windows it's used for relaying battery information
to an application called Quick Set.

Thanks,

RE: [PATCH v3 0/4] dell-wmi: Changes in WMI event code handling

[Mario_Limonciello]

> -Original Message-
> From: Pali Rohár [mailto:pali.ro...@gmail.com]
> Sent: Tuesday, June 21, 2016 1:29 PM
> To: Limonciello, Mario 
> Cc: dvh...@infradead.org; gabriele@gmail.com; l...@kernel.org;
> alex.h...@canonical.com; mj...@srcf.ucam.org; ker...@kempniu.pl;
> platform-driver-...@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v3 0/4] dell-wmi: Changes in WMI event code handling
> 
> On Tuesday 21 June 2016 20:16:09 mario_limoncie...@dell.com wrote:
> > > -Original Message-
> > > From: Darren Hart [mailto:dvh...@infradead.org]
> > > Sent: Tuesday, June 21, 2016 1:06 PM
> > > To: Pali Rohár 
> > > Cc: Gabriele Mazzotta ; Andy Lutomirski
> > > ; Alex Hung ; Matthew
> > > Garrett ; Michał Kępień ;
> > > Limonciello, Mario ; platform-driver-
> > > x...@vger.kernel.org; linux-kernel@vger.kernel.org
> > > Subject: Re: [PATCH v3 0/4] dell-wmi: Changes in WMI event code
> > > handling
> > >
> > > On Thu, Jun 16, 2016 at 09:33:02AM +0200, Pali Rohár wrote:
> > > > On Wednesday 15 June 2016 20:19:58 Darren Hart wrote:
> > > > > On Wed, Jun 15, 2016 at 09:49:09PM +0200, Pali Rohár wrote:
> > > > > > First patch describe problem about 0xe045 code. Second and
> > > > > > third are
> > >
> > > just
> > >
> > > > > > cosmetic and last rework code which processing WMI events. It
> > > > > > should
> > >
> > > be
> > >
> > > > > > properly tested on more Dell machines, to check that
> > > > > > everything is still working correctly.
> > > > >
> > > > > Is this "should be properly tested on more Dell machines" still
> > > > > the case?
> > >
> > > Are
> > >
> > > > > you ready for this to go into linux-next?
> > > >
> > > > Series should be OK, but I would like to see if someone else test
> > > > this series... Gabriele, Alex or Andy? Do you have time?
> > >
> > > Tested on a Dell XPS 13 2016 (9350). All hotkeys appear to work
> > > without warning
> > > messages. I didn't get anything out of Fn-F8 which has a picture of
> > > a laptop and
> > > white screen behind it. Not sure what that is supposed to do - if
> > > it was meant to blank the screen, it did not, perhaps it is meant
> > > to toggle screen outputs... will test that when I have access to
> > > an external display.
> >
> > That key is meant to toggle screen outputs.  I believe it's still
> > done by the EC emitting  + p.  If your WM doesn't recognize
> > that, it won't do much, but you can see in xev the key combinations.
> 
> I still do not understand this stupidity, pressing *one* key cause
> emitting two keys to OS and then OS needs to handle combinations of keys
> and acts on it correctly (like windows manager)
> 
> Is there some way to disable this insane nonsense activity of BIOS,
> firmware or whatever it is doing in HW to send *one* key scancode when
> pressing *one* key?
> 

I talked to the EC team about this a while back when it was first implemented.
That's not possible without _OSI detection of Linux.  _OSI detection could be 
used to relay to the EC to behave differently, but otherwise the EC will have 
no idea what OS it's on for which way to behave.

I don't remember all the history behind the switch over from a single scan code 
To  + P, but I think it's along the lines of Windows 8/Windows 10 allow
you to iterate the display selection menu based upon holding super and pressing
P multiple times and waiting for you to stop.  

Sending a single scan code will change displays immediately, so having the 
EC send super+p unifies the behavior of fn-f8 and super+p.

RE: [PATCH v3 0/4] dell-wmi: Changes in WMI event code handling

[Mario_Limonciello]

> -Original Message-
> From: Pali Rohár [mailto:pali.ro...@gmail.com]
> Sent: Tuesday, June 21, 2016 1:29 PM
> To: Limonciello, Mario 
> Cc: dvh...@infradead.org; gabriele@gmail.com; l...@kernel.org;
> alex.h...@canonical.com; mj...@srcf.ucam.org; ker...@kempniu.pl;
> platform-driver-...@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v3 0/4] dell-wmi: Changes in WMI event code handling
> 
> On Tuesday 21 June 2016 20:16:09 mario_limoncie...@dell.com wrote:
> > > -Original Message-
> > > From: Darren Hart [mailto:dvh...@infradead.org]
> > > Sent: Tuesday, June 21, 2016 1:06 PM
> > > To: Pali Rohár 
> > > Cc: Gabriele Mazzotta ; Andy Lutomirski
> > > ; Alex Hung ; Matthew
> > > Garrett ; Michał Kępień ;
> > > Limonciello, Mario ; platform-driver-
> > > x...@vger.kernel.org; linux-kernel@vger.kernel.org
> > > Subject: Re: [PATCH v3 0/4] dell-wmi: Changes in WMI event code
> > > handling
> > >
> > > On Thu, Jun 16, 2016 at 09:33:02AM +0200, Pali Rohár wrote:
> > > > On Wednesday 15 June 2016 20:19:58 Darren Hart wrote:
> > > > > On Wed, Jun 15, 2016 at 09:49:09PM +0200, Pali Rohár wrote:
> > > > > > First patch describe problem about 0xe045 code. Second and
> > > > > > third are
> > >
> > > just
> > >
> > > > > > cosmetic and last rework code which processing WMI events. It
> > > > > > should
> > >
> > > be
> > >
> > > > > > properly tested on more Dell machines, to check that
> > > > > > everything is still working correctly.
> > > > >
> > > > > Is this "should be properly tested on more Dell machines" still
> > > > > the case?
> > >
> > > Are
> > >
> > > > > you ready for this to go into linux-next?
> > > >
> > > > Series should be OK, but I would like to see if someone else test
> > > > this series... Gabriele, Alex or Andy? Do you have time?
> > >
> > > Tested on a Dell XPS 13 2016 (9350). All hotkeys appear to work
> > > without warning
> > > messages. I didn't get anything out of Fn-F8 which has a picture of
> > > a laptop and
> > > white screen behind it. Not sure what that is supposed to do - if
> > > it was meant to blank the screen, it did not, perhaps it is meant
> > > to toggle screen outputs... will test that when I have access to
> > > an external display.
> >
> > That key is meant to toggle screen outputs.  I believe it's still
> > done by the EC emitting  + p.  If your WM doesn't recognize
> > that, it won't do much, but you can see in xev the key combinations.
> 
> I still do not understand this stupidity, pressing *one* key cause
> emitting two keys to OS and then OS needs to handle combinations of keys
> and acts on it correctly (like windows manager)
> 
> Is there some way to disable this insane nonsense activity of BIOS,
> firmware or whatever it is doing in HW to send *one* key scancode when
> pressing *one* key?
> 

I talked to the EC team about this a while back when it was first implemented.
That's not possible without _OSI detection of Linux.  _OSI detection could be 
used to relay to the EC to behave differently, but otherwise the EC will have 
no idea what OS it's on for which way to behave.

I don't remember all the history behind the switch over from a single scan code 
To  + P, but I think it's along the lines of Windows 8/Windows 10 allow
you to iterate the display selection menu based upon holding super and pressing
P multiple times and waiting for you to stop.  

Sending a single scan code will change displays immediately, so having the 
EC send super+p unifies the behavior of fn-f8 and super+p.

RE: [PATCH v3 0/4] dell-wmi: Changes in WMI event code handling

[Mario_Limonciello]

> -Original Message-
> From: Darren Hart [mailto:dvh...@infradead.org]
> Sent: Tuesday, June 21, 2016 1:06 PM
> To: Pali Rohár 
> Cc: Gabriele Mazzotta ; Andy Lutomirski
> ; Alex Hung ; Matthew
> Garrett ; Michał Kępień ;
> Limonciello, Mario ; platform-driver-
> x...@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v3 0/4] dell-wmi: Changes in WMI event code handling
> 
> On Thu, Jun 16, 2016 at 09:33:02AM +0200, Pali Rohár wrote:
> > On Wednesday 15 June 2016 20:19:58 Darren Hart wrote:
> > > On Wed, Jun 15, 2016 at 09:49:09PM +0200, Pali Rohár wrote:
> > > > First patch describe problem about 0xe045 code. Second and third are
> just
> > > > cosmetic and last rework code which processing WMI events. It should
> be
> > > > properly tested on more Dell machines, to check that everything is still
> > > > working correctly.
> > >
> > > Is this "should be properly tested on more Dell machines" still the case?
> Are
> > > you ready for this to go into linux-next?
> >
> > Series should be OK, but I would like to see if someone else test this
> > series... Gabriele, Alex or Andy? Do you have time?
> 
> Tested on a Dell XPS 13 2016 (9350). All hotkeys appear to work without
> warning
> messages. I didn't get anything out of Fn-F8 which has a picture of a laptop
> and
> white screen behind it. Not sure what that is supposed to do - if it was meant
> to blank the screen, it did not, perhaps it is meant to toggle screen 
> outputs...
> will test that when I have access to an external display.

That key is meant to toggle screen outputs.  I believe it's still done by the EC
emitting  + p.  If your WM doesn't recognize that, it won't do much,
but you can see in xev the key combinations.

RE: [PATCH v3 0/4] dell-wmi: Changes in WMI event code handling

[Mario_Limonciello]

> -Original Message-
> From: Darren Hart [mailto:dvh...@infradead.org]
> Sent: Tuesday, June 21, 2016 1:06 PM
> To: Pali Rohár 
> Cc: Gabriele Mazzotta ; Andy Lutomirski
> ; Alex Hung ; Matthew
> Garrett ; Michał Kępień ;
> Limonciello, Mario ; platform-driver-
> x...@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v3 0/4] dell-wmi: Changes in WMI event code handling
> 
> On Thu, Jun 16, 2016 at 09:33:02AM +0200, Pali Rohár wrote:
> > On Wednesday 15 June 2016 20:19:58 Darren Hart wrote:
> > > On Wed, Jun 15, 2016 at 09:49:09PM +0200, Pali Rohár wrote:
> > > > First patch describe problem about 0xe045 code. Second and third are
> just
> > > > cosmetic and last rework code which processing WMI events. It should
> be
> > > > properly tested on more Dell machines, to check that everything is still
> > > > working correctly.
> > >
> > > Is this "should be properly tested on more Dell machines" still the case?
> Are
> > > you ready for this to go into linux-next?
> >
> > Series should be OK, but I would like to see if someone else test this
> > series... Gabriele, Alex or Andy? Do you have time?
> 
> Tested on a Dell XPS 13 2016 (9350). All hotkeys appear to work without
> warning
> messages. I didn't get anything out of Fn-F8 which has a picture of a laptop
> and
> white screen behind it. Not sure what that is supposed to do - if it was meant
> to blank the screen, it did not, perhaps it is meant to toggle screen 
> outputs...
> will test that when I have access to an external display.

That key is meant to toggle screen outputs.  I believe it's still done by the EC
emitting  + p.  If your WM doesn't recognize that, it won't do much,
but you can see in xev the key combinations.

RE: [PATCH] ACPI: don't show an error when we're not in charge of PCIe hotplug.

[Mario_Limonciello]

> -Original Message-
> From: Peter Jones [mailto:pjo...@redhat.com]
> Sent: Tuesday, June 21, 2016 10:19 AM
> To: Rafael J. Wysocki 
> Cc: ACPI Devel Maling List ; Limonciello, Mario
> ; Linux Kernel Mailing List  ker...@vger.kernel.org>; Len Brown ; Rafael J . Wysocki
> ; Andy Lutomirski 
> Subject: Re: [PATCH] ACPI: don't show an error when we're not in charge of
> PCIe hotplug.
> 
> (Sorry for the slow response - it's deadline time over here.)
> 
> On Thu, Jun 16, 2016 at 04:56:57PM +0200, Rafael J. Wysocki wrote:
> > On Thu, Jun 16, 2016 at 2:12 AM, Rafael J. Wysocki 
> wrote:
> > > On Thu, Jun 16, 2016 at 12:15 AM, Peter Jones 
> wrote:
> > >> Right now when booting, on many laptops the firmware manages the
> PCIe
> > >> bus.  As a result, when we call the _OSC ACPI method, it returns an
> > >> error code.  Unfortunately the errors are not very articulate.
> > >
> > > What exactly do you mean here?
> > >
> > >>  As a result, we show:
> > >>
> > >> ACPI: PCI Root Bridge [PCI0] (domain  [bus 00-fe])
> > >> acpi PNP0A08:00: _OSC: OS supports [ExtendedConfig ASPM ClockPM
> Segments MSI]
> > >> \_SB_.PCI0 (33DB4D5B-1FF7-401C-9657-7441C03DD766): _OSC invalid
> UUID
> > >> _OSC request data: 1 1f 0
> > >
> > > So _OSC told us that the UUID was invalid, didn't it?
> >
> > BTW, the above messages are KERN_DEBUG, so at least in theory they
> > shouldn't be visible in production runs.
> >
> > Maybe the bug to fix is that they show up when they aren't supposed to?
> 
> No - the workflow that I am really trying to remedy is this:
> 
>  1) S3 resume sometimes isn't working on some laptop you've got.
>  2) start looking at debug messages
>  3) this shows an error, so it looks like it's probably the problem
>  4) go fishing for red herring
>  5) if you happen to know who maintains the DSDT for the platform in
> question, eventually work out that this is working as intended and
> the bug is someplace else.
> 5b) if you don't know that person, eventually work out that it /might/
>  be someplace else...
> 
> So the idea was to make it look more like an indication of status, and
> less like an error that's causing unrelated problems.
> 
> When I talked to Mario at Dell (Cc'd), it wasn't clear to us that
> there's a way to distinguish the between the UUID being
> invalid/malformed, being merely unsupported, or being supported in some
> configurations but not the current one.  In this particular DSDT, the
> machine doesn't support the OS controlling any of this if USB-C /
> thunderbolt are enabled.  The DSDT is clearly written with the belief
> that you have to completely disable the handling for that UUID in this
> case, and googling for this looks like it's not the only one written
> with that belief.
> 
> Reading the spec (v6.1, sections 6.2.11.3 and 6.2.11.4), it seems
> plausible that you can express this instead by handling the UUID but
> choosing each individual query/status bit in the way that accomplishes
> the OS doing nothing with the response.  So it may well be that that's
> just more code that vendors have thought wasn't necessary (or wasn't
> correct for some reason.)
> 
> Mario, want to jump in on your thinking here?
> 
> --
>   Peter

After talking to the team, I was told this particular implementation to not let
OS take control when acting on that specific UUID based upon a variable 
(NEXP in this case) came from Intel RC code.  

That's probably why this is all across a lot of platforms, including non-Dell.

At least in the context of the laptop Peter noticed this on (Dell XPS 13 9350)
NEXP is set in GNVS based upon Thunderbolt capability.

As for why they return unrecognized UUID instead of just masking all the
capabilities bits?  It's the same net functional result.  If the vendor provided
RC code doesn't caused WCHK problems or functional problems it's hard to 
make a case for why it needs to be changed by the OEM.

I think that Peter's patch is appropriate to message this is specifically
what's going on.

RE: [PATCH] ACPI: don't show an error when we're not in charge of PCIe hotplug.

[Mario_Limonciello]

> -Original Message-
> From: Peter Jones [mailto:pjo...@redhat.com]
> Sent: Tuesday, June 21, 2016 10:19 AM
> To: Rafael J. Wysocki 
> Cc: ACPI Devel Maling List ; Limonciello, Mario
> ; Linux Kernel Mailing List  ker...@vger.kernel.org>; Len Brown ; Rafael J . Wysocki
> ; Andy Lutomirski 
> Subject: Re: [PATCH] ACPI: don't show an error when we're not in charge of
> PCIe hotplug.
> 
> (Sorry for the slow response - it's deadline time over here.)
> 
> On Thu, Jun 16, 2016 at 04:56:57PM +0200, Rafael J. Wysocki wrote:
> > On Thu, Jun 16, 2016 at 2:12 AM, Rafael J. Wysocki 
> wrote:
> > > On Thu, Jun 16, 2016 at 12:15 AM, Peter Jones 
> wrote:
> > >> Right now when booting, on many laptops the firmware manages the
> PCIe
> > >> bus.  As a result, when we call the _OSC ACPI method, it returns an
> > >> error code.  Unfortunately the errors are not very articulate.
> > >
> > > What exactly do you mean here?
> > >
> > >>  As a result, we show:
> > >>
> > >> ACPI: PCI Root Bridge [PCI0] (domain  [bus 00-fe])
> > >> acpi PNP0A08:00: _OSC: OS supports [ExtendedConfig ASPM ClockPM
> Segments MSI]
> > >> \_SB_.PCI0 (33DB4D5B-1FF7-401C-9657-7441C03DD766): _OSC invalid
> UUID
> > >> _OSC request data: 1 1f 0
> > >
> > > So _OSC told us that the UUID was invalid, didn't it?
> >
> > BTW, the above messages are KERN_DEBUG, so at least in theory they
> > shouldn't be visible in production runs.
> >
> > Maybe the bug to fix is that they show up when they aren't supposed to?
> 
> No - the workflow that I am really trying to remedy is this:
> 
>  1) S3 resume sometimes isn't working on some laptop you've got.
>  2) start looking at debug messages
>  3) this shows an error, so it looks like it's probably the problem
>  4) go fishing for red herring
>  5) if you happen to know who maintains the DSDT for the platform in
> question, eventually work out that this is working as intended and
> the bug is someplace else.
> 5b) if you don't know that person, eventually work out that it /might/
>  be someplace else...
> 
> So the idea was to make it look more like an indication of status, and
> less like an error that's causing unrelated problems.
> 
> When I talked to Mario at Dell (Cc'd), it wasn't clear to us that
> there's a way to distinguish the between the UUID being
> invalid/malformed, being merely unsupported, or being supported in some
> configurations but not the current one.  In this particular DSDT, the
> machine doesn't support the OS controlling any of this if USB-C /
> thunderbolt are enabled.  The DSDT is clearly written with the belief
> that you have to completely disable the handling for that UUID in this
> case, and googling for this looks like it's not the only one written
> with that belief.
> 
> Reading the spec (v6.1, sections 6.2.11.3 and 6.2.11.4), it seems
> plausible that you can express this instead by handling the UUID but
> choosing each individual query/status bit in the way that accomplishes
> the OS doing nothing with the response.  So it may well be that that's
> just more code that vendors have thought wasn't necessary (or wasn't
> correct for some reason.)
> 
> Mario, want to jump in on your thinking here?
> 
> --
>   Peter

After talking to the team, I was told this particular implementation to not let
OS take control when acting on that specific UUID based upon a variable 
(NEXP in this case) came from Intel RC code.  

That's probably why this is all across a lot of platforms, including non-Dell.

At least in the context of the laptop Peter noticed this on (Dell XPS 13 9350)
NEXP is set in GNVS based upon Thunderbolt capability.

As for why they return unrecognized UUID instead of just masking all the
capabilities bits?  It's the same net functional result.  If the vendor provided
RC code doesn't caused WCHK problems or functional problems it's hard to 
make a case for why it needs to be changed by the OEM.

I think that Peter's patch is appropriate to message this is specifically
what's going on.

RE: [PATCH v6] r8152: Add support for setting pass through MAC address on RTL8153-AD

[Mario_Limonciello]

> -Original Message-
> From: David Miller [mailto:da...@davemloft.net]
> Sent: Tuesday, June 14, 2016 1:35 PM
> To: pali.ro...@gmail.com
> Cc: gre...@linuxfoundation.org; and...@lunn.ch; Limonciello, Mario
> ; hayesw...@realtek.com; linux-
> ker...@vger.kernel.org; net...@vger.kernel.org; linux-
> u...@vger.kernel.org; anthony.w...@canonical.com
> Subject: Re: [PATCH v6] r8152: Add support for setting pass through MAC
> address on RTL8153-AD
> 
> From: Pali Rohár 
> Date: Tue, 14 Jun 2016 18:47:36 +0200
> 
> > You have never seen two ethernet cards with same MAC addresses? Right
> I
> > have not seen two USB, but there is non zero chance that could happen.
> 
> It would be an error scenerio, and something to be avoided.
> 
> It is a valid and correct assumption that one is able to put
> several devices at the same time on the same physical network
> and expect it to work.
> 
> The behavior added by the change in question invalidates that.
> 
> I'm trying to consider the long term aspects of this, which is that if
> more devices adopt this scheme we're in trouble if we blindly
> interpret the MAC address in this way.
> 

Do you mean if other manufacturers start to ship devices with 
RTL8135-AD's w/ this pass through bit set and people start to try to 
mix and match?

> This firmware MAC property facility seems to be designed with only an
> extremely narrow use case being considered.

Yes, as I understand it this is the reason that it's only on such specific 
devices
that the mac address pass through bit is actually set on the efuse.

RE: [PATCH v6] r8152: Add support for setting pass through MAC address on RTL8153-AD

[Mario_Limonciello]

> -Original Message-
> From: David Miller [mailto:da...@davemloft.net]
> Sent: Tuesday, June 14, 2016 1:35 PM
> To: pali.ro...@gmail.com
> Cc: gre...@linuxfoundation.org; and...@lunn.ch; Limonciello, Mario
> ; hayesw...@realtek.com; linux-
> ker...@vger.kernel.org; net...@vger.kernel.org; linux-
> u...@vger.kernel.org; anthony.w...@canonical.com
> Subject: Re: [PATCH v6] r8152: Add support for setting pass through MAC
> address on RTL8153-AD
> 
> From: Pali Rohár 
> Date: Tue, 14 Jun 2016 18:47:36 +0200
> 
> > You have never seen two ethernet cards with same MAC addresses? Right
> I
> > have not seen two USB, but there is non zero chance that could happen.
> 
> It would be an error scenerio, and something to be avoided.
> 
> It is a valid and correct assumption that one is able to put
> several devices at the same time on the same physical network
> and expect it to work.
> 
> The behavior added by the change in question invalidates that.
> 
> I'm trying to consider the long term aspects of this, which is that if
> more devices adopt this scheme we're in trouble if we blindly
> interpret the MAC address in this way.
> 

Do you mean if other manufacturers start to ship devices with 
RTL8135-AD's w/ this pass through bit set and people start to try to 
mix and match?

> This firmware MAC property facility seems to be designed with only an
> extremely narrow use case being considered.

Yes, as I understand it this is the reason that it's only on such specific 
devices
that the mac address pass through bit is actually set on the efuse.

RE: [PATCH v6] r8152: Add support for setting pass through MAC address on RTL8153-AD

[Mario_Limonciello]

> -Original Message-
> From: Andrew Lunn [mailto:and...@lunn.ch]
> Sent: Tuesday, June 14, 2016 12:23 PM
> To: Limonciello, Mario 
> Cc: pali.ro...@gmail.com; gre...@linuxfoundation.org;
> da...@davemloft.net; hayesw...@realtek.com; linux-
> ker...@vger.kernel.org; net...@vger.kernel.org; linux-
> u...@vger.kernel.org; anthony.w...@canonical.com
> Subject: Re: [PATCH v6] r8152: Add support for setting pass through MAC
> address on RTL8153-AD
> 
> > > It is same, how to handle two network cards which tell us, that they
> > > have same MAC addresses.
> > >
> >
> > The kernel handles this just fine.  In doing this patch I checked to see
> > what it does in that scenario.  Two devices are made.  systemd doesn't
> > rename the second device via the MAC name (eg enxAABBCCDDEEFF).
> 
> What does you dhcp server do? Does it gives out the same IP address?
> You then have two interfaces on the same network, with the same MAC
> address and IP address. Then what happens?
> 
>  Andrew

I didn't test it on the same network, I used two separate networks.

I expect that the DHCP server would be awfully confused and you'd
run down an interesting problem path if it got the same MAC twice.

RE: [PATCH v6] r8152: Add support for setting pass through MAC address on RTL8153-AD

[Mario_Limonciello]

> -Original Message-
> From: Andrew Lunn [mailto:and...@lunn.ch]
> Sent: Tuesday, June 14, 2016 12:23 PM
> To: Limonciello, Mario 
> Cc: pali.ro...@gmail.com; gre...@linuxfoundation.org;
> da...@davemloft.net; hayesw...@realtek.com; linux-
> ker...@vger.kernel.org; net...@vger.kernel.org; linux-
> u...@vger.kernel.org; anthony.w...@canonical.com
> Subject: Re: [PATCH v6] r8152: Add support for setting pass through MAC
> address on RTL8153-AD
> 
> > > It is same, how to handle two network cards which tell us, that they
> > > have same MAC addresses.
> > >
> >
> > The kernel handles this just fine.  In doing this patch I checked to see
> > what it does in that scenario.  Two devices are made.  systemd doesn't
> > rename the second device via the MAC name (eg enxAABBCCDDEEFF).
> 
> What does you dhcp server do? Does it gives out the same IP address?
> You then have two interfaces on the same network, with the same MAC
> address and IP address. Then what happens?
> 
>  Andrew

I didn't test it on the same network, I used two separate networks.

I expect that the DHCP server would be awfully confused and you'd
run down an interesting problem path if it got the same MAC twice.

RE: [PATCH v6] r8152: Add support for setting pass through MAC address on RTL8153-AD

[Mario_Limonciello]

> -Original Message-
> From: Pali Rohár [mailto:pali.ro...@gmail.com]
> Sent: Tuesday, June 14, 2016 11:48 AM
> To: Greg KH 
> Cc: David Miller ; and...@lunn.ch; Limonciello,
> Mario ; hayesw...@realtek.com; linux-
> ker...@vger.kernel.org; net...@vger.kernel.org; linux-
> u...@vger.kernel.org; anthony.w...@canonical.com
> Subject: Re: [PATCH v6] r8152: Add support for setting pass through MAC
> address on RTL8153-AD
> 
> On Tuesday 14 June 2016 18:40:17 Greg KH wrote:
> > On Tue, Jun 14, 2016 at 06:28:10PM +0200, Pali Rohár wrote:
> > > On Saturday 11 June 2016 19:42:26 David Miller wrote:
> > > > From: Andrew Lunn 
> > > > Date: Sat, 11 Jun 2016 17:39:21 +0200
> > > >
> > > > > What is still open is do we want to accept it at all? Do we
> > > > > accept the concept of putting the same MAC address on multiple
> > > > > interfaces at hotplug time? Do we trust BIOS vendors to not
> > > > > keep changing DSDT property name, since it is not
> > > > > standardised?
> > > > >
> > > > > Do we want this at all should be decided by somebody more
> > > > > senior then those passing comments on the code.
> > > >
> > > > Indeed, I think the behavior of using the same MAC address on
> > > > multiple interfaces if we plug several of these in at once is not
> > > > good.
> > > >
> > > > We shouldn't behave this way just because the Microsoft driver
> > > > does.
> > >
> > > I agree, but in some cases it is night mare for local admins when
> > > booting different OS cause changing MAC address on local network.
> > >
> > > Another similar situation: Imagine that you have two USB network
> > > cards and both have "burned" into their registers same MAC
> > > address. If you connect both those USB network cards, linux kernel
> > > bind appropriate driver which read MAC address for both those
> > > cards. But those addresses are same. What will linux kernel do in
> > > this case?
> >
> > If you can find such a broken USB device, try it and see :)
> 
> What do you mean by broken USB device?
> 
> You have never seen two ethernet cards with same MAC addresses? Right I
> have not seen two USB, but there is non zero chance that could happen.
> Specially now when more and more people starts using USB network cards.
> 
> > (hint, might be hard to find, I've never seen such a device before.)
> >
> > I don't see how that pertains to this issue, sorry, how does broken
> > USB hardware compare to a working Dell device?
> 
> It is same, how to handle two network cards which tell us, that they
> have same MAC addresses.
> 

The kernel handles this just fine.  In doing this patch I checked to see
what it does in that scenario.  Two devices are made.  systemd doesn't
rename the second device via the MAC name (eg enxAABBCCDDEEFF).

RE: [PATCH v6] r8152: Add support for setting pass through MAC address on RTL8153-AD

[Mario_Limonciello]

> -Original Message-
> From: Pali Rohár [mailto:pali.ro...@gmail.com]
> Sent: Tuesday, June 14, 2016 11:48 AM
> To: Greg KH 
> Cc: David Miller ; and...@lunn.ch; Limonciello,
> Mario ; hayesw...@realtek.com; linux-
> ker...@vger.kernel.org; net...@vger.kernel.org; linux-
> u...@vger.kernel.org; anthony.w...@canonical.com
> Subject: Re: [PATCH v6] r8152: Add support for setting pass through MAC
> address on RTL8153-AD
> 
> On Tuesday 14 June 2016 18:40:17 Greg KH wrote:
> > On Tue, Jun 14, 2016 at 06:28:10PM +0200, Pali Rohár wrote:
> > > On Saturday 11 June 2016 19:42:26 David Miller wrote:
> > > > From: Andrew Lunn 
> > > > Date: Sat, 11 Jun 2016 17:39:21 +0200
> > > >
> > > > > What is still open is do we want to accept it at all? Do we
> > > > > accept the concept of putting the same MAC address on multiple
> > > > > interfaces at hotplug time? Do we trust BIOS vendors to not
> > > > > keep changing DSDT property name, since it is not
> > > > > standardised?
> > > > >
> > > > > Do we want this at all should be decided by somebody more
> > > > > senior then those passing comments on the code.
> > > >
> > > > Indeed, I think the behavior of using the same MAC address on
> > > > multiple interfaces if we plug several of these in at once is not
> > > > good.
> > > >
> > > > We shouldn't behave this way just because the Microsoft driver
> > > > does.
> > >
> > > I agree, but in some cases it is night mare for local admins when
> > > booting different OS cause changing MAC address on local network.
> > >
> > > Another similar situation: Imagine that you have two USB network
> > > cards and both have "burned" into their registers same MAC
> > > address. If you connect both those USB network cards, linux kernel
> > > bind appropriate driver which read MAC address for both those
> > > cards. But those addresses are same. What will linux kernel do in
> > > this case?
> >
> > If you can find such a broken USB device, try it and see :)
> 
> What do you mean by broken USB device?
> 
> You have never seen two ethernet cards with same MAC addresses? Right I
> have not seen two USB, but there is non zero chance that could happen.
> Specially now when more and more people starts using USB network cards.
> 
> > (hint, might be hard to find, I've never seen such a device before.)
> >
> > I don't see how that pertains to this issue, sorry, how does broken
> > USB hardware compare to a working Dell device?
> 
> It is same, how to handle two network cards which tell us, that they
> have same MAC addresses.
> 

The kernel handles this just fine.  In doing this patch I checked to see
what it does in that scenario.  Two devices are made.  systemd doesn't
rename the second device via the MAC name (eg enxAABBCCDDEEFF).

RE: [PATCH v6] r8152: Add support for setting pass through MAC address on RTL8153-AD

[Mario_Limonciello]

> -Original Message-
> From: David Miller [mailto:da...@davemloft.net]
> Sent: Saturday, June 11, 2016 12:42 PM
> To: and...@lunn.ch
> Cc: Limonciello, Mario ;
> hayesw...@realtek.com; linux-kernel@vger.kernel.org;
> net...@vger.kernel.org; linux-...@vger.kernel.org; pali.ro...@gmail.com;
> anthony.w...@canonical.com; gre...@linuxfoundation.org
> Subject: Re: [PATCH v6] r8152: Add support for setting pass through MAC
> address on RTL8153-AD
> 
> From: Andrew Lunn 
> Date: Sat, 11 Jun 2016 17:39:21 +0200
> 
> > What is still open is do we want to accept it at all? Do we accept the
> > concept of putting the same MAC address on multiple interfaces at
> > hotplug time? Do we trust BIOS vendors to not keep changing DSDT
> > property name, since it is not standardised?
> >

It's worth saying - standardized a property name doesn't indemnify it.
Properties change all the time from one version of a spec to another.

I can only speak for Dell, but it's in our best interest to keep the BIOS
side of the codebase around this simpler too.

> > Do we want this at all should be decided by somebody more senior then
> > those passing comments on the code.
> 
> Indeed, I think the behavior of using the same MAC address on multiple
> interfaces if we plug several of these in at once is not good.
> 
> We shouldn't behave this way just because the Microsoft driver does.

This is really grasping at an extreme corner case scenario.

Dell TB15 and WD15 docks are currently only ones on the market with
RTL8135-AD and MAC address pass through efuse bit set.  These docks
are not inexpensive.  If someone really wants multiple USB NIC's plugged
in, they can pick up a second USB NIC from the web for far cheaper than
buying a second dock.

Also for what it's worth, the docks don't allow daisy chaining.  The dock EC's 
will reject the second dock from functioning through the downstream 
connection.  The only way that two docks could be hooked up and
functional is on a machine with multiple type C ports.

If you still think it's worth solving, what would you like done as an 
alternative?  I would really like to have some implementation of this
that you guys are comfortable with upstream.

There was already discussion and an implementation in this 
thread about tracking if the aux MAC was assigned to something and only 
allowing one device to get that assignment.  There was a limitation that 
you won't be able to know which device gets the auxiliary MAC address.  
It will be based solely upon hotplug order.

There was also in one version of the patch a way to turn off this behavior
In a module parameter.  Greg KH wasn't fond of that, so it's not present
in the current version.

I can add either of those back in if they would help the case.

Another option I wanted to offer was turning this behavior on via kernel
configuration option and let the distros decide if they want to turn it on
for users.

RE: [PATCH v6] r8152: Add support for setting pass through MAC address on RTL8153-AD

[Mario_Limonciello]

> -Original Message-
> From: David Miller [mailto:da...@davemloft.net]
> Sent: Saturday, June 11, 2016 12:42 PM
> To: and...@lunn.ch
> Cc: Limonciello, Mario ;
> hayesw...@realtek.com; linux-kernel@vger.kernel.org;
> net...@vger.kernel.org; linux-...@vger.kernel.org; pali.ro...@gmail.com;
> anthony.w...@canonical.com; gre...@linuxfoundation.org
> Subject: Re: [PATCH v6] r8152: Add support for setting pass through MAC
> address on RTL8153-AD
> 
> From: Andrew Lunn 
> Date: Sat, 11 Jun 2016 17:39:21 +0200
> 
> > What is still open is do we want to accept it at all? Do we accept the
> > concept of putting the same MAC address on multiple interfaces at
> > hotplug time? Do we trust BIOS vendors to not keep changing DSDT
> > property name, since it is not standardised?
> >

It's worth saying - standardized a property name doesn't indemnify it.
Properties change all the time from one version of a spec to another.

I can only speak for Dell, but it's in our best interest to keep the BIOS
side of the codebase around this simpler too.

> > Do we want this at all should be decided by somebody more senior then
> > those passing comments on the code.
> 
> Indeed, I think the behavior of using the same MAC address on multiple
> interfaces if we plug several of these in at once is not good.
> 
> We shouldn't behave this way just because the Microsoft driver does.

This is really grasping at an extreme corner case scenario.

Dell TB15 and WD15 docks are currently only ones on the market with
RTL8135-AD and MAC address pass through efuse bit set.  These docks
are not inexpensive.  If someone really wants multiple USB NIC's plugged
in, they can pick up a second USB NIC from the web for far cheaper than
buying a second dock.

Also for what it's worth, the docks don't allow daisy chaining.  The dock EC's 
will reject the second dock from functioning through the downstream 
connection.  The only way that two docks could be hooked up and
functional is on a machine with multiple type C ports.

If you still think it's worth solving, what would you like done as an 
alternative?  I would really like to have some implementation of this
that you guys are comfortable with upstream.

There was already discussion and an implementation in this 
thread about tracking if the aux MAC was assigned to something and only 
allowing one device to get that assignment.  There was a limitation that 
you won't be able to know which device gets the auxiliary MAC address.  
It will be based solely upon hotplug order.

There was also in one version of the patch a way to turn off this behavior
In a module parameter.  Greg KH wasn't fond of that, so it's not present
in the current version.

I can add either of those back in if they would help the case.

Another option I wanted to offer was turning this behavior on via kernel
configuration option and let the distros decide if they want to turn it on
for users.

RE: [PATCH 3/4] dell-wmi: Add information about other WMI event codes

[Mario_Limonciello]

> -Original Message-
> From: Pali Rohár [mailto:pali.ro...@gmail.com]
> Sent: Tuesday, June 7, 2016 6:00 PM
> To: Gabriele Mazzotta ; Limonciello, Mario
> 
> Cc: Matthew Garrett ; Darren Hart
> ; Michał Kępień ; Andy Lutomirski
> ; Alex Hung ; platform-driver-
> x...@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 3/4] dell-wmi: Add information about other WMI event
> codes
> 
> On Friday 27 May 2016 00:04:23 Gabriele Mazzotta wrote:
> > On 22/05/2016 13:36, Pali Rohár wrote:
> > > ACPI DSDT tables have defined other WMI codes, but does not contain
> > > any description when those codes are emitted. Some other codes can
> > > be found in logs on internet. In this patch are all which I saw, but
> > > lot of them are not tested properly (e.g. for duplicate events with
> > > AT keyboard). Now we have all WMI event codes at one place and in
> > > future after proper testing those codes can be correctly enabled or
> disabled...
> > >
> > > Signed-off-by: Pali Rohár 
> > > ---
> > >  drivers/platform/x86/dell-wmi.c |   32
> 
> > >  1 file changed, 32 insertions(+)
> > >
> > > diff --git a/drivers/platform/x86/dell-wmi.c
> > > b/drivers/platform/x86/dell-wmi.c index 363d927..7aac1dc 100644
> > > --- a/drivers/platform/x86/dell-wmi.c
> > > +++ b/drivers/platform/x86/dell-wmi.c
> > > @@ -110,6 +110,9 @@ static const struct key_entry
> dell_wmi_legacy_keymap[] __initconst = {
> > >   /* BIOS error detected */
> > >   { KE_IGNORE, 0xe00d, { KEY_RESERVED } },
> > >
> > > + /* Unknown, defined in ACPI DSDT */
> > > + /* { KE_IGNORE, 0xe00e, { KEY_RESERVED } }, */
> > > +
> >
> > I'm interested in knowing what's the meaning of this 0xe00e. This
> > event is sent multiple times when I suspend/resume my laptop and it's
> > definitely not a keypress.
> 
> From DSDT dumps which I have seen, I guess it could be something with battery
> charging... but that is only my guess.
> 
> Mario, do you have any idea, what these unknown events are?

Off-hand I'm not sure, it would require some more digging.

Can you please remind me what model numbers and BIOS combinations you have 
found e00e in DSDT and what context the events are actually happening?   
Anything released in the past two years?

RE: [PATCH 3/4] dell-wmi: Add information about other WMI event codes

[Mario_Limonciello]

> -Original Message-
> From: Pali Rohár [mailto:pali.ro...@gmail.com]
> Sent: Tuesday, June 7, 2016 6:00 PM
> To: Gabriele Mazzotta ; Limonciello, Mario
> 
> Cc: Matthew Garrett ; Darren Hart
> ; Michał Kępień ; Andy Lutomirski
> ; Alex Hung ; platform-driver-
> x...@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 3/4] dell-wmi: Add information about other WMI event
> codes
> 
> On Friday 27 May 2016 00:04:23 Gabriele Mazzotta wrote:
> > On 22/05/2016 13:36, Pali Rohár wrote:
> > > ACPI DSDT tables have defined other WMI codes, but does not contain
> > > any description when those codes are emitted. Some other codes can
> > > be found in logs on internet. In this patch are all which I saw, but
> > > lot of them are not tested properly (e.g. for duplicate events with
> > > AT keyboard). Now we have all WMI event codes at one place and in
> > > future after proper testing those codes can be correctly enabled or
> disabled...
> > >
> > > Signed-off-by: Pali Rohár 
> > > ---
> > >  drivers/platform/x86/dell-wmi.c |   32
> 
> > >  1 file changed, 32 insertions(+)
> > >
> > > diff --git a/drivers/platform/x86/dell-wmi.c
> > > b/drivers/platform/x86/dell-wmi.c index 363d927..7aac1dc 100644
> > > --- a/drivers/platform/x86/dell-wmi.c
> > > +++ b/drivers/platform/x86/dell-wmi.c
> > > @@ -110,6 +110,9 @@ static const struct key_entry
> dell_wmi_legacy_keymap[] __initconst = {
> > >   /* BIOS error detected */
> > >   { KE_IGNORE, 0xe00d, { KEY_RESERVED } },
> > >
> > > + /* Unknown, defined in ACPI DSDT */
> > > + /* { KE_IGNORE, 0xe00e, { KEY_RESERVED } }, */
> > > +
> >
> > I'm interested in knowing what's the meaning of this 0xe00e. This
> > event is sent multiple times when I suspend/resume my laptop and it's
> > definitely not a keypress.
> 
> From DSDT dumps which I have seen, I guess it could be something with battery
> charging... but that is only my guess.
> 
> Mario, do you have any idea, what these unknown events are?

Off-hand I'm not sure, it would require some more digging.

Can you please remind me what model numbers and BIOS combinations you have 
found e00e in DSDT and what context the events are actually happening?   
Anything released in the past two years?

RE: [EXT] [PATCH v4] r8152: Add support for setting pass through MAC address on RTL8153-AD

[Mario_Limonciello]

> -Original Message-
> From: Konstantin Shkolnyy [mailto:konstantin.shkol...@silabs.com]
> Sent: Monday, June 6, 2016 12:43 PM
> To: Limonciello, Mario ;
> hayesw...@realtek.com
> Cc: LKML ; Netdev
> ; Linux USB ;
> pali.ro...@gmail.com; anthony.w...@canonical.com; Greg KH
> 
> Subject: RE: [EXT] [PATCH v4] r8152: Add support for setting pass through
> MAC address on RTL8153-AD
> 
> > -Original Message-
> > From: linux-usb-ow...@vger.kernel.org [mailto:linux-usb-
> > ow...@vger.kernel.org] On Behalf Of Mario Limonciello
> > Sent: Monday, June 06, 2016 12:19
> > To: hayesw...@realtek.com
> > Cc: LKML; Netdev; Linux USB; pali.ro...@gmail.com;
> > anthony.w...@canonical.com; Greg KH; Mario Limonciello
> > Subject: [EXT] [PATCH v4] r8152: Add support for setting pass through MAC
> > address on RTL8153-AD
> >
> > The RTL8153-AD supports a persistent system specific MAC address.
> > This means a device plugged into two different systems with host side
> > support will show different (but persistent) MAC addresses.
> >
> > This information for the system's persistent MAC address is burned in
> when
> > the system HW is built and available under _SB\AMAC in the DSDT at
> > runtime.
> >
> > This technology is currently implemented in the Dell TB15 and WD15 Type-C
> > docks.  More information is available here:
> > http://www.dell.com/support/article/us/en/04/SLN301147
> 
> What is going to happen if I connect multiple dongles? Will they all get the
> same address?

If you connect a dongle without a RTL8153-AD or without that bit they'll
get the MAC that was burned into the dongle.

If you connect multiple docks that have this Realtek chip with this bit set,
yes they'll all get the same address.
I confirmed that's what happens on Windows too.

RE: [EXT] [PATCH v4] r8152: Add support for setting pass through MAC address on RTL8153-AD

[Mario_Limonciello]

> -Original Message-
> From: Konstantin Shkolnyy [mailto:konstantin.shkol...@silabs.com]
> Sent: Monday, June 6, 2016 12:43 PM
> To: Limonciello, Mario ;
> hayesw...@realtek.com
> Cc: LKML ; Netdev
> ; Linux USB ;
> pali.ro...@gmail.com; anthony.w...@canonical.com; Greg KH
> 
> Subject: RE: [EXT] [PATCH v4] r8152: Add support for setting pass through
> MAC address on RTL8153-AD
> 
> > -Original Message-
> > From: linux-usb-ow...@vger.kernel.org [mailto:linux-usb-
> > ow...@vger.kernel.org] On Behalf Of Mario Limonciello
> > Sent: Monday, June 06, 2016 12:19
> > To: hayesw...@realtek.com
> > Cc: LKML; Netdev; Linux USB; pali.ro...@gmail.com;
> > anthony.w...@canonical.com; Greg KH; Mario Limonciello
> > Subject: [EXT] [PATCH v4] r8152: Add support for setting pass through MAC
> > address on RTL8153-AD
> >
> > The RTL8153-AD supports a persistent system specific MAC address.
> > This means a device plugged into two different systems with host side
> > support will show different (but persistent) MAC addresses.
> >
> > This information for the system's persistent MAC address is burned in
> when
> > the system HW is built and available under _SB\AMAC in the DSDT at
> > runtime.
> >
> > This technology is currently implemented in the Dell TB15 and WD15 Type-C
> > docks.  More information is available here:
> > http://www.dell.com/support/article/us/en/04/SLN301147
> 
> What is going to happen if I connect multiple dongles? Will they all get the
> same address?

If you connect a dongle without a RTL8153-AD or without that bit they'll
get the MAC that was burned into the dongle.

If you connect multiple docks that have this Realtek chip with this bit set,
yes they'll all get the same address.
I confirmed that's what happens on Windows too.

RE: [PATCH v3] r8152: Add support for setting pass through MAC address on RTL8153-AD

[Mario_Limonciello]

> > Realtek has this in their Windows driver that all OEM's will be taking.
> > Another OEM would just need to burn the right information into the SPI at
> > manufacturing and expose it to the DSDT.
> 
> Where it the match up for the Realtek bit to corrispond with this
> specific ACPI field?  If it's not in the ACPI spec, then vendors _WILL_
> do this in different ways.
> 
> Again, document it, in the code, what is going on here, that's all I'm
> asking.  I'm not asking you to change the logic at all!

I've added additional comments for v4.  I strongly believe that even if
another vendor does do this differently for their implementation of
\\_SB.AMAC this code will be safe to run.

All of the output from the field are tested for exactly what the field
should look like.

That said, I would be highly surprised if Realtek decided to implement
with another OEM differently.  It would increase their code complexity
on Windows as well since this is part of the generic driver.

RE: [PATCH v3] r8152: Add support for setting pass through MAC address on RTL8153-AD

[Mario_Limonciello]

> > Realtek has this in their Windows driver that all OEM's will be taking.
> > Another OEM would just need to burn the right information into the SPI at
> > manufacturing and expose it to the DSDT.
> 
> Where it the match up for the Realtek bit to corrispond with this
> specific ACPI field?  If it's not in the ACPI spec, then vendors _WILL_
> do this in different ways.
> 
> Again, document it, in the code, what is going on here, that's all I'm
> asking.  I'm not asking you to change the logic at all!

I've added additional comments for v4.  I strongly believe that even if
another vendor does do this differently for their implementation of
\\_SB.AMAC this code will be safe to run.

All of the output from the field are tested for exactly what the field
should look like.

That said, I would be highly surprised if Realtek decided to implement
with another OEM differently.  It would increase their code complexity
on Windows as well since this is part of the generic driver.

RE: [PATCH v3] r8152: Add support for setting pass through MAC address on RTL8153-AD

[Mario_Limonciello]

> -Original Message-
> From: Andrew Lunn [mailto:and...@lunn.ch]
> Sent: Monday, June 6, 2016 9:41 AM
> To: Limonciello, Mario 
> Cc: hayesw...@realtek.com; LKML ; Netdev
> ; Linux USB ;
> pali.ro...@gmail.com; anthony.w...@canonical.com; Greg KH
> 
> Subject: Re: [PATCH v3] r8152: Add support for setting pass through MAC
> address on RTL8153-AD
> 
> > +   /* returns _AUXMAC_#AABBCCDDEEFF# */
> > +   status = acpi_evaluate_object(NULL, "\\_SB.AMAC", NULL, );
> > +   obj = (union acpi_object *)buffer.pointer;
> > +   if (ACPI_SUCCESS(status)) {
> > +   if (obj->type != ACPI_TYPE_BUFFER ||
> > +   obj->string.length != 0x17) {
> > +   pr_warn("r8152: get_passthru_addr: Invalid buffer");
> 
> Please don't use pr_warn() when you can use the better alternatives
> like dev_warn().
> 
>  Andrew

OK thanks, I'll adjust that to use a different warn.

RE: [PATCH v3] r8152: Add support for setting pass through MAC address on RTL8153-AD

[Mario_Limonciello]

> -Original Message-
> From: Andrew Lunn [mailto:and...@lunn.ch]
> Sent: Monday, June 6, 2016 9:41 AM
> To: Limonciello, Mario 
> Cc: hayesw...@realtek.com; LKML ; Netdev
> ; Linux USB ;
> pali.ro...@gmail.com; anthony.w...@canonical.com; Greg KH
> 
> Subject: Re: [PATCH v3] r8152: Add support for setting pass through MAC
> address on RTL8153-AD
> 
> > +   /* returns _AUXMAC_#AABBCCDDEEFF# */
> > +   status = acpi_evaluate_object(NULL, "\\_SB.AMAC", NULL, );
> > +   obj = (union acpi_object *)buffer.pointer;
> > +   if (ACPI_SUCCESS(status)) {
> > +   if (obj->type != ACPI_TYPE_BUFFER ||
> > +   obj->string.length != 0x17) {
> > +   pr_warn("r8152: get_passthru_addr: Invalid buffer");
> 
> Please don't use pr_warn() when you can use the better alternatives
> like dev_warn().
> 
>  Andrew

OK thanks, I'll adjust that to use a different warn.

RE: [PATCH v3] r8152: Add support for setting pass through MAC address on RTL8153-AD

[Mario_Limonciello]

> -Original Message-
> From: Greg KH [mailto:gre...@linuxfoundation.org]
> Sent: Monday, June 6, 2016 9:40 AM
> To: Limonciello, Mario 
> Cc: hayesw...@realtek.com; LKML ; Netdev
> ; Linux USB ;
> pali.ro...@gmail.com; anthony.w...@canonical.com
> Subject: Re: [PATCH v3] r8152: Add support for setting pass through MAC
> address on RTL8153-AD
> 
> On Mon, Jun 06, 2016 at 09:15:21AM -0500, Mario Limonciello wrote:
> > The RTL8153-AD supports a persistent system specific MAC address.
> > This means a device plugged into two different systems with host side
> > support will show different (but persistent) MAC addresses.
> >
> > This information for the system's persistent MAC address is burned in
> when
> > the system HW is built and available under _SB\AMAC in the DSDT at
> runtime.
> >
> > This technology is currently implemented in the Dell TB15 and WD15 Type-C
> > docks.  More information is available here:
> > http://www.dell.com/support/article/us/en/04/SLN301147
> >
> > Signed-off-by: Mario Limonciello 
> > ---
> >  drivers/net/usb/r8152.c | 60
> +++--
> >  1 file changed, 58 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> > index 3f9f6ed..f4bd46d 100644
> > --- a/drivers/net/usb/r8152.c
> > +++ b/drivers/net/usb/r8152.c
> > @@ -26,6 +26,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >
> >  /* Information for net-next */
> >  #define NETNEXT_VERSION"08"
> > @@ -455,6 +456,11 @@
> >  /* SRAM_IMPEDANCE */
> >  #define RX_DRIVING_MASK0x6000
> >
> > +/* MAC PASSTHRU */
> > +#define AD_MASK0xfee0
> > +#define EFUSE  0xcfdb
> > +#define PASS_THRU_MASK 0x1
> > +
> >  enum rtl_register_content {
> > _1000bps= 0x10,
> > _100bps = 0x08,
> > @@ -1030,6 +1036,53 @@ out1:
> > return ret;
> >  }
> >
> > +static int get_passthru_addr(struct r8152 *tp, struct sockaddr *sa)
> 
> I see no "Dell" specific markings or tests here at all, please be
> EXPLICIT in the code exactly what this is for, and what it is doing.
> Otherwise no one can tell you what machines/devices this will trigger on
> at all.
> 
> As it is, it's totally vague and unknown what this whole function is
> here for.
> 

I intentionally removed the Dell tests.  I tried to go into this in the cover
Letter as I knew there would be discussion about it.

This is intended because the better approach (as mentioned by this ML)
is to look for the exact chip that supports this feature, and query the bit
that can be set on that device's eFuse to indicate it supports this feature.

That device is an RTL8153-AD and there is a bit on the eFuse for MAC
pass through.

The ACPI code is intended to run on any device that has one of these
Devices plugged in.  From what I learned discussing with others is that
is the approach the Realtek driver takes on the Windows side as well.

If the Dell TB15 (containin RTL8153-AD with this pass through bit set)
is plugged into for example an HP machine with the latest Realtek driver
installed this ACPI query will be run on Windows too.  It will fail and fall
back to the address burned into the HW.

If the netdev maintainers still want a DMI check put in place again 
here I'll put it back in, but I really think this is overkill.

> 
> 
> > +{
> > +   acpi_status status;
> > +   struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> > +   union acpi_object *obj;
> > +   int ret = -EINVAL;
> > +   u32 ocp_data;
> > +   unsigned char buf[6];
> > +
> > +   ocp_data = ocp_read_word(tp, MCU_TYPE_USB, USB_MISC_0);
> > +   if ((ocp_data & AD_MASK) != 0x1000)
> > +   return -ENODEV;
> > +
> > +   ocp_data = ocp_read_byte(tp, MCU_TYPE_USB, EFUSE);
> > +   if ((ocp_data & PASS_THRU_MASK) != 1)
> > +   return -ENODEV;
> > +
> > +   /* returns _AUXMAC_#AABBCCDDEEFF# */
> > +   status = acpi_evaluate_object(NULL, "\\_SB.AMAC", NULL, );
> > +   obj = (union acpi_object *)buffer.pointer;
> > +   if (ACPI_SUCCESS(status)) {
> > +   if (obj->type != ACPI_TYPE_BUFFER ||
> > +   obj->string.length != 0x17) {
> > +   pr_warn("r8152: get_passthru_addr: Invalid buffer");
> > +   goto amacout;
> > +   }
> > +   if (strncmp(obj->string.pointer, "_AUXMAC_#", 9) != 0) {
> > +   pr_warn("r8152: get_passthru_addr: Invalid
> header");
> > +   goto amacout;
> > +   }
> > +   ret = hex2bin(buf, obj->string.pointer + 9, 6);
> > +   if (ret < 0 || !is_valid_ether_addr(buf)) {
> > +   pr_warn("r8152: get_passthru_addr: Invalid MAC");
> > +   goto amacout;
> > +   }
> > +   memcpy(sa->sa_data, buf, 6);
> > +   

RE: [PATCH v3] r8152: Add support for setting pass through MAC address on RTL8153-AD

[Mario_Limonciello]

> -Original Message-
> From: Greg KH [mailto:gre...@linuxfoundation.org]
> Sent: Monday, June 6, 2016 9:40 AM
> To: Limonciello, Mario 
> Cc: hayesw...@realtek.com; LKML ; Netdev
> ; Linux USB ;
> pali.ro...@gmail.com; anthony.w...@canonical.com
> Subject: Re: [PATCH v3] r8152: Add support for setting pass through MAC
> address on RTL8153-AD
> 
> On Mon, Jun 06, 2016 at 09:15:21AM -0500, Mario Limonciello wrote:
> > The RTL8153-AD supports a persistent system specific MAC address.
> > This means a device plugged into two different systems with host side
> > support will show different (but persistent) MAC addresses.
> >
> > This information for the system's persistent MAC address is burned in
> when
> > the system HW is built and available under _SB\AMAC in the DSDT at
> runtime.
> >
> > This technology is currently implemented in the Dell TB15 and WD15 Type-C
> > docks.  More information is available here:
> > http://www.dell.com/support/article/us/en/04/SLN301147
> >
> > Signed-off-by: Mario Limonciello 
> > ---
> >  drivers/net/usb/r8152.c | 60
> +++--
> >  1 file changed, 58 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> > index 3f9f6ed..f4bd46d 100644
> > --- a/drivers/net/usb/r8152.c
> > +++ b/drivers/net/usb/r8152.c
> > @@ -26,6 +26,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >
> >  /* Information for net-next */
> >  #define NETNEXT_VERSION"08"
> > @@ -455,6 +456,11 @@
> >  /* SRAM_IMPEDANCE */
> >  #define RX_DRIVING_MASK0x6000
> >
> > +/* MAC PASSTHRU */
> > +#define AD_MASK0xfee0
> > +#define EFUSE  0xcfdb
> > +#define PASS_THRU_MASK 0x1
> > +
> >  enum rtl_register_content {
> > _1000bps= 0x10,
> > _100bps = 0x08,
> > @@ -1030,6 +1036,53 @@ out1:
> > return ret;
> >  }
> >
> > +static int get_passthru_addr(struct r8152 *tp, struct sockaddr *sa)
> 
> I see no "Dell" specific markings or tests here at all, please be
> EXPLICIT in the code exactly what this is for, and what it is doing.
> Otherwise no one can tell you what machines/devices this will trigger on
> at all.
> 
> As it is, it's totally vague and unknown what this whole function is
> here for.
> 

I intentionally removed the Dell tests.  I tried to go into this in the cover
Letter as I knew there would be discussion about it.

This is intended because the better approach (as mentioned by this ML)
is to look for the exact chip that supports this feature, and query the bit
that can be set on that device's eFuse to indicate it supports this feature.

That device is an RTL8153-AD and there is a bit on the eFuse for MAC
pass through.

The ACPI code is intended to run on any device that has one of these
Devices plugged in.  From what I learned discussing with others is that
is the approach the Realtek driver takes on the Windows side as well.

If the Dell TB15 (containin RTL8153-AD with this pass through bit set)
is plugged into for example an HP machine with the latest Realtek driver
installed this ACPI query will be run on Windows too.  It will fail and fall
back to the address burned into the HW.

If the netdev maintainers still want a DMI check put in place again 
here I'll put it back in, but I really think this is overkill.

> 
> 
> > +{
> > +   acpi_status status;
> > +   struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> > +   union acpi_object *obj;
> > +   int ret = -EINVAL;
> > +   u32 ocp_data;
> > +   unsigned char buf[6];
> > +
> > +   ocp_data = ocp_read_word(tp, MCU_TYPE_USB, USB_MISC_0);
> > +   if ((ocp_data & AD_MASK) != 0x1000)
> > +   return -ENODEV;
> > +
> > +   ocp_data = ocp_read_byte(tp, MCU_TYPE_USB, EFUSE);
> > +   if ((ocp_data & PASS_THRU_MASK) != 1)
> > +   return -ENODEV;
> > +
> > +   /* returns _AUXMAC_#AABBCCDDEEFF# */
> > +   status = acpi_evaluate_object(NULL, "\\_SB.AMAC", NULL, );
> > +   obj = (union acpi_object *)buffer.pointer;
> > +   if (ACPI_SUCCESS(status)) {
> > +   if (obj->type != ACPI_TYPE_BUFFER ||
> > +   obj->string.length != 0x17) {
> > +   pr_warn("r8152: get_passthru_addr: Invalid buffer");
> > +   goto amacout;
> > +   }
> > +   if (strncmp(obj->string.pointer, "_AUXMAC_#", 9) != 0) {
> > +   pr_warn("r8152: get_passthru_addr: Invalid
> header");
> > +   goto amacout;
> > +   }
> > +   ret = hex2bin(buf, obj->string.pointer + 9, 6);
> > +   if (ret < 0 || !is_valid_ether_addr(buf)) {
> > +   pr_warn("r8152: get_passthru_addr: Invalid MAC");
> > +   goto amacout;
> > +   }
> > +   memcpy(sa->sa_data, buf, 6);
> > +   ether_addr_copy(tp->netdev->dev_addr, sa->sa_data);
> > +   netdev_info(tp->netdev, "Using pass-through MAC address
> %pM\n",
> > 

RE: [PATCH v3] r8152: Add support for setting pass through MAC address on RTL8153-AD

[Mario_Limonciello]

> -Original Message-
> From: Greg KH [mailto:gre...@linuxfoundation.org]
> Sent: Monday, June 6, 2016 9:50 AM
> To: Limonciello, Mario 
> Cc: hayesw...@realtek.com; linux-kernel@vger.kernel.org;
> net...@vger.kernel.org; linux-...@vger.kernel.org; pali.ro...@gmail.com;
> anthony.w...@canonical.com
> Subject: Re: [PATCH v3] r8152: Add support for setting pass through MAC
> address on RTL8153-AD
> 
> On Mon, Jun 06, 2016 at 02:43:37PM +, mario_limoncie...@dell.com
> wrote:
> > > -Original Message-
> > > From: Greg KH [mailto:gre...@linuxfoundation.org]
> > > Sent: Monday, June 6, 2016 9:40 AM
> > > To: Limonciello, Mario 
> > > Cc: hayesw...@realtek.com; LKML ;
> Netdev
> > > ; Linux USB ;
> > > pali.ro...@gmail.com; anthony.w...@canonical.com
> > > Subject: Re: [PATCH v3] r8152: Add support for setting pass through MAC
> > > address on RTL8153-AD
> > >
> > > On Mon, Jun 06, 2016 at 09:15:20AM -0500, Mario Limonciello wrote:
> > > > Since this is a Realtek feature, I feel this shouldn't be moved into a
> platform
> > > > MAC address lookup.  The code should only be run when the correct
> > > Realtek device
> > > > is plugged in.
> > > >
> > > > Changes from v2:
> > > >  * Only apply to RTL8153-AD w/ eFuse pass through mac address pass
> thru
> > > >bit set.
> > > >  * Drop matching DMI information on Dell.  Although this is
> implemented
> > > on
> > > >Dell, this is a Realtek feature that may may be implemented on other
> > > >OEMs as well.
> > > >  * Test that pass through MAC address is valid, fall back to HW address 
> > > > if
> > > >invalid.
> > > >  * Don't track status of which device has MAC pass through activated.
> > > >  - Expected experience is that if two docks (RTL8153-AD's w/ mac pass
> thru
> > > >bit set) were plugged in both should have MAC pass through
> activated.
> > >
> > > cover letters for single-patch submissions is overkill and confusing,
> > > please don't.
> >
> > I was trying to convey differences between versions of this patch, I'll 
> > avoid
> > that in the future and let the audience find them themselves.
> 
> No, put them in the patch itself, under the --- line, like is documented
> to do so.  Don't make people "find them themselves", if you do that,
> your patch will just be ignored.
> 
> greg k-h

Sorry I was not aware of that.  I'll do that in the next patch.

RE: [PATCH v3] r8152: Add support for setting pass through MAC address on RTL8153-AD

[Mario_Limonciello]

> -Original Message-
> From: Greg KH [mailto:gre...@linuxfoundation.org]
> Sent: Monday, June 6, 2016 9:50 AM
> To: Limonciello, Mario 
> Cc: hayesw...@realtek.com; linux-kernel@vger.kernel.org;
> net...@vger.kernel.org; linux-...@vger.kernel.org; pali.ro...@gmail.com;
> anthony.w...@canonical.com
> Subject: Re: [PATCH v3] r8152: Add support for setting pass through MAC
> address on RTL8153-AD
> 
> On Mon, Jun 06, 2016 at 02:43:37PM +, mario_limoncie...@dell.com
> wrote:
> > > -Original Message-
> > > From: Greg KH [mailto:gre...@linuxfoundation.org]
> > > Sent: Monday, June 6, 2016 9:40 AM
> > > To: Limonciello, Mario 
> > > Cc: hayesw...@realtek.com; LKML ;
> Netdev
> > > ; Linux USB ;
> > > pali.ro...@gmail.com; anthony.w...@canonical.com
> > > Subject: Re: [PATCH v3] r8152: Add support for setting pass through MAC
> > > address on RTL8153-AD
> > >
> > > On Mon, Jun 06, 2016 at 09:15:20AM -0500, Mario Limonciello wrote:
> > > > Since this is a Realtek feature, I feel this shouldn't be moved into a
> platform
> > > > MAC address lookup.  The code should only be run when the correct
> > > Realtek device
> > > > is plugged in.
> > > >
> > > > Changes from v2:
> > > >  * Only apply to RTL8153-AD w/ eFuse pass through mac address pass
> thru
> > > >bit set.
> > > >  * Drop matching DMI information on Dell.  Although this is
> implemented
> > > on
> > > >Dell, this is a Realtek feature that may may be implemented on other
> > > >OEMs as well.
> > > >  * Test that pass through MAC address is valid, fall back to HW address 
> > > > if
> > > >invalid.
> > > >  * Don't track status of which device has MAC pass through activated.
> > > >  - Expected experience is that if two docks (RTL8153-AD's w/ mac pass
> thru
> > > >bit set) were plugged in both should have MAC pass through
> activated.
> > >
> > > cover letters for single-patch submissions is overkill and confusing,
> > > please don't.
> >
> > I was trying to convey differences between versions of this patch, I'll 
> > avoid
> > that in the future and let the audience find them themselves.
> 
> No, put them in the patch itself, under the --- line, like is documented
> to do so.  Don't make people "find them themselves", if you do that,
> your patch will just be ignored.
> 
> greg k-h

Sorry I was not aware of that.  I'll do that in the next patch.

RE: [PATCH v3] r8152: Add support for setting pass through MAC address on RTL8153-AD

[Mario_Limonciello]

> -Original Message-
> From: Greg KH [mailto:gre...@linuxfoundation.org]
> Sent: Monday, June 6, 2016 9:40 AM
> To: Limonciello, Mario 
> Cc: hayesw...@realtek.com; LKML ; Netdev
> ; Linux USB ;
> pali.ro...@gmail.com; anthony.w...@canonical.com
> Subject: Re: [PATCH v3] r8152: Add support for setting pass through MAC
> address on RTL8153-AD
> 
> On Mon, Jun 06, 2016 at 09:15:20AM -0500, Mario Limonciello wrote:
> > Since this is a Realtek feature, I feel this shouldn't be moved into a 
> > platform
> > MAC address lookup.  The code should only be run when the correct
> Realtek device
> > is plugged in.
> >
> > Changes from v2:
> >  * Only apply to RTL8153-AD w/ eFuse pass through mac address pass thru
> >bit set.
> >  * Drop matching DMI information on Dell.  Although this is implemented
> on
> >Dell, this is a Realtek feature that may may be implemented on other
> >OEMs as well.
> >  * Test that pass through MAC address is valid, fall back to HW address if
> >invalid.
> >  * Don't track status of which device has MAC pass through activated.
> >  - Expected experience is that if two docks (RTL8153-AD's w/ mac pass thru
> >bit set) were plugged in both should have MAC pass through activated.
> 
> cover letters for single-patch submissions is overkill and confusing,
> please don't.

I was trying to convey differences between versions of this patch, I'll avoid
that in the future and let the audience find them themselves.

RE: [PATCH v3] r8152: Add support for setting pass through MAC address on RTL8153-AD

[Mario_Limonciello]

> -Original Message-
> From: Greg KH [mailto:gre...@linuxfoundation.org]
> Sent: Monday, June 6, 2016 9:40 AM
> To: Limonciello, Mario 
> Cc: hayesw...@realtek.com; LKML ; Netdev
> ; Linux USB ;
> pali.ro...@gmail.com; anthony.w...@canonical.com
> Subject: Re: [PATCH v3] r8152: Add support for setting pass through MAC
> address on RTL8153-AD
> 
> On Mon, Jun 06, 2016 at 09:15:20AM -0500, Mario Limonciello wrote:
> > Since this is a Realtek feature, I feel this shouldn't be moved into a 
> > platform
> > MAC address lookup.  The code should only be run when the correct
> Realtek device
> > is plugged in.
> >
> > Changes from v2:
> >  * Only apply to RTL8153-AD w/ eFuse pass through mac address pass thru
> >bit set.
> >  * Drop matching DMI information on Dell.  Although this is implemented
> on
> >Dell, this is a Realtek feature that may may be implemented on other
> >OEMs as well.
> >  * Test that pass through MAC address is valid, fall back to HW address if
> >invalid.
> >  * Don't track status of which device has MAC pass through activated.
> >  - Expected experience is that if two docks (RTL8153-AD's w/ mac pass thru
> >bit set) were plugged in both should have MAC pass through activated.
> 
> cover letters for single-patch submissions is overkill and confusing,
> please don't.

I was trying to convey differences between versions of this patch, I'll avoid
that in the future and let the audience find them themselves.

RE: [PATCH v2] r8152: Add support for setting MAC to system's Auxiliary MAC address

[Mario_Limonciello]

> -Original Message-
> From: Greg KH [mailto:gre...@linuxfoundation.org]
> Sent: Thursday, June 2, 2016 9:02 PM
> To: Limonciello, Mario 
> Cc: hayesw...@realtek.com; linux-kernel@vger.kernel.org;
> net...@vger.kernel.org; linux-...@vger.kernel.org; pali.ro...@gmail.com;
> anthony.w...@canonical.com
> Subject: Re: [PATCH v2] r8152: Add support for setting MAC to system's
> Auxiliary MAC address
> 
> On Thu, Jun 02, 2016 at 06:32:42PM +, mario_limoncie...@dell.com
> wrote:
> > > And you want to check this for all Dell devices?  Please be model
> > > specific, I doubt a bunch of Dell servers wants to run this code...
> > >
> >
> > Tracking model specific is really going to turn into a giant list never 
> > ending
> list.
> > To drill down more specifically, I can match on chassis too.
> 
> Yes, as this is a vendor/platform-specific "quirk", you will have to
> update it for each and every individual device you want it enabled as it
> is so different from what all other drivers do.
> 
> thanks,
> 
> greg k-h

After finding out that there is a characteristic specific to Realtek chip in 
this USB
device, I think it would be best to only run the \\_SB.AMAC lookup when this 
specific USB device is connected.  

Realtek is not maintaining a whitelist of systems in their driver for the other 
OS,
they approach it from the other end and only lookup auxiliary MAC if the right
device is plugged in.

RE: [PATCH v2] r8152: Add support for setting MAC to system's Auxiliary MAC address

[Mario_Limonciello]

> -Original Message-
> From: Greg KH [mailto:gre...@linuxfoundation.org]
> Sent: Thursday, June 2, 2016 9:02 PM
> To: Limonciello, Mario 
> Cc: hayesw...@realtek.com; linux-kernel@vger.kernel.org;
> net...@vger.kernel.org; linux-...@vger.kernel.org; pali.ro...@gmail.com;
> anthony.w...@canonical.com
> Subject: Re: [PATCH v2] r8152: Add support for setting MAC to system's
> Auxiliary MAC address
> 
> On Thu, Jun 02, 2016 at 06:32:42PM +, mario_limoncie...@dell.com
> wrote:
> > > And you want to check this for all Dell devices?  Please be model
> > > specific, I doubt a bunch of Dell servers wants to run this code...
> > >
> >
> > Tracking model specific is really going to turn into a giant list never 
> > ending
> list.
> > To drill down more specifically, I can match on chassis too.
> 
> Yes, as this is a vendor/platform-specific "quirk", you will have to
> update it for each and every individual device you want it enabled as it
> is so different from what all other drivers do.
> 
> thanks,
> 
> greg k-h

After finding out that there is a characteristic specific to Realtek chip in 
this USB
device, I think it would be best to only run the \\_SB.AMAC lookup when this 
specific USB device is connected.  

Realtek is not maintaining a whitelist of systems in their driver for the other 
OS,
they approach it from the other end and only lookup auxiliary MAC if the right
device is plugged in.

RE: [PATCH v2] r8152: Add support for setting MAC to system's Auxiliary MAC address

[Mario_Limonciello]

> -Original Message-
> From: Hayes Wang [mailto:hayesw...@realtek.com]
> Sent: Friday, June 3, 2016 4:44 AM
> To: Limonciello, Mario 
> Cc: LKML ; Netdev
> ; Linux USB ;
> pali.ro...@gmail.com; anthony.w...@canonical.com; Greg KH
> 
> Subject: RE: [PATCH v2] r8152: Add support for setting MAC to system's
> Auxiliary MAC address
> 
> Mario Limonciello [mailto:mario_limoncie...@dell.com]
> > Sent: Friday, June 03, 2016 12:58 AM
> [...]
> > @@ -500,6 +502,7 @@ enum rtl8152_flags {
> > SELECTIVE_SUSPEND,
> > PHY_RESET,
> > SCHEDULE_NAPI,
> > +   MAC_PASSTHRU = 0,
> 
> I don't think you have to give this a value.
> 
> >  };
> >
> [...]
> >  static int set_ethernet_addr(struct r8152 *tp)
> >  {
> > struct net_device *dev = tp->netdev;
> > @@ -1041,6 +1088,10 @@ static int set_ethernet_addr(struct r8152 *tp)
> > else
> > ret = pla_ocp_read(tp, PLA_BACKUP, 8, sa.sa_data);
> >
> > +   /* if system provides auxiliary MAC address */
> > +   if (get_auxiliary_addr(tp, ))
> > +   ret = 0;
> 
> It still has problem when tp->version == RTL_VER_01.
> First, you would read the current MAC address (MAC1) to sa.sa_data.
> Then sa.sa_data may be modified by MAC2 after get_auxiliary_addr().
> However, the MAC2 wouldn't be set to the device, because
> 
>   if (tp->version == RTL_VER_01)
>   ether_addr_copy(dev->dev_addr, sa.sa_data);
> 
> Therefore, you would find that dev_addr is MAC2, and the device
> uses MAC1.
> 
> Best Regards,
> Hayes
> 

Hayes,

>From the information that I got that this is only a valid thing to do on
RTL-8153-AD (when dock bit set), should this only match with 
tp->version  >= RTL_VER_03? 

How can I confirm that the chip is 8153-AD specifically?  I didn't see
something obvious in driver to tell this.

Thanks,

RE: [PATCH v2] r8152: Add support for setting MAC to system's Auxiliary MAC address

[Mario_Limonciello]

> -Original Message-
> From: Hayes Wang [mailto:hayesw...@realtek.com]
> Sent: Friday, June 3, 2016 4:44 AM
> To: Limonciello, Mario 
> Cc: LKML ; Netdev
> ; Linux USB ;
> pali.ro...@gmail.com; anthony.w...@canonical.com; Greg KH
> 
> Subject: RE: [PATCH v2] r8152: Add support for setting MAC to system's
> Auxiliary MAC address
> 
> Mario Limonciello [mailto:mario_limoncie...@dell.com]
> > Sent: Friday, June 03, 2016 12:58 AM
> [...]
> > @@ -500,6 +502,7 @@ enum rtl8152_flags {
> > SELECTIVE_SUSPEND,
> > PHY_RESET,
> > SCHEDULE_NAPI,
> > +   MAC_PASSTHRU = 0,
> 
> I don't think you have to give this a value.
> 
> >  };
> >
> [...]
> >  static int set_ethernet_addr(struct r8152 *tp)
> >  {
> > struct net_device *dev = tp->netdev;
> > @@ -1041,6 +1088,10 @@ static int set_ethernet_addr(struct r8152 *tp)
> > else
> > ret = pla_ocp_read(tp, PLA_BACKUP, 8, sa.sa_data);
> >
> > +   /* if system provides auxiliary MAC address */
> > +   if (get_auxiliary_addr(tp, ))
> > +   ret = 0;
> 
> It still has problem when tp->version == RTL_VER_01.
> First, you would read the current MAC address (MAC1) to sa.sa_data.
> Then sa.sa_data may be modified by MAC2 after get_auxiliary_addr().
> However, the MAC2 wouldn't be set to the device, because
> 
>   if (tp->version == RTL_VER_01)
>   ether_addr_copy(dev->dev_addr, sa.sa_data);
> 
> Therefore, you would find that dev_addr is MAC2, and the device
> uses MAC1.
> 
> Best Regards,
> Hayes
> 

Hayes,

>From the information that I got that this is only a valid thing to do on
RTL-8153-AD (when dock bit set), should this only match with 
tp->version  >= RTL_VER_03? 

How can I confirm that the chip is 8153-AD specifically?  I didn't see
something obvious in driver to tell this.

Thanks,

RE: [PATCH] r8152: Add support for setting MAC to system's Auxiliary MAC address

[Mario_Limonciello]

> -Original Message-
> From: Hayes Wang [mailto:hayesw...@realtek.com]
> Sent: Friday, June 3, 2016 4:23 AM
> To: Limonciello, Mario ;
> gre...@linuxfoundation.org
> Cc: linux-kernel@vger.kernel.org; net...@vger.kernel.org; linux-
> u...@vger.kernel.org; pali.ro...@gmail.com; anthony.w...@canonical.com
> Subject: RE: [PATCH] r8152: Add support for setting MAC to system's
> Auxiliary MAC address
> 
> mario_limoncie...@dell.com [mailto:mario_limoncie...@dell.com]
> > Sent: Friday, June 03, 2016 12:58 AM
> [...]
> > > Why generate a random one and not just use the one that the network
> > > controler already provides?
> >
> > That's how the flow works in r8152 already and I'm not overriding it.
> > Again, I'll send V2 and you'll see what I did.
> 
> The original flows are
> 1. Read MAC address 1 from device.
> 2. Check if the MAC address 1 is valid.
> 3. Use random MAC address if MAC address 1 is invalid.
> 
> However, your flow would be
> 1. Read MAC address 1 from device.
> 2. Read MAC address 2 from ACPI.
> 3. Check if the MAC address 2 is valid.
> 4. Use random MAC address if MAC address 2 is invalid.
> 
> Although MAC address 2 may be invalid, MAC address 1 may be valid.
> For this situation, you have to use MAC address 1, not random one.
> 
> Best Regards,
> Hayes

Hi Hayes,

Once I get information about how to read docking bit, I'll adjust that for v3
to ensure that it falls back to MAC address 1 if #2 is invalid.

Thanks

RE: [PATCH] r8152: Add support for setting MAC to system's Auxiliary MAC address

[Mario_Limonciello]

> -Original Message-
> From: Hayes Wang [mailto:hayesw...@realtek.com]
> Sent: Friday, June 3, 2016 4:23 AM
> To: Limonciello, Mario ;
> gre...@linuxfoundation.org
> Cc: linux-kernel@vger.kernel.org; net...@vger.kernel.org; linux-
> u...@vger.kernel.org; pali.ro...@gmail.com; anthony.w...@canonical.com
> Subject: RE: [PATCH] r8152: Add support for setting MAC to system's
> Auxiliary MAC address
> 
> mario_limoncie...@dell.com [mailto:mario_limoncie...@dell.com]
> > Sent: Friday, June 03, 2016 12:58 AM
> [...]
> > > Why generate a random one and not just use the one that the network
> > > controler already provides?
> >
> > That's how the flow works in r8152 already and I'm not overriding it.
> > Again, I'll send V2 and you'll see what I did.
> 
> The original flows are
> 1. Read MAC address 1 from device.
> 2. Check if the MAC address 1 is valid.
> 3. Use random MAC address if MAC address 1 is invalid.
> 
> However, your flow would be
> 1. Read MAC address 1 from device.
> 2. Read MAC address 2 from ACPI.
> 3. Check if the MAC address 2 is valid.
> 4. Use random MAC address if MAC address 2 is invalid.
> 
> Although MAC address 2 may be invalid, MAC address 1 may be valid.
> For this situation, you have to use MAC address 1, not random one.
> 
> Best Regards,
> Hayes

Hi Hayes,

Once I get information about how to read docking bit, I'll adjust that for v3
to ensure that it falls back to MAC address 1 if #2 is invalid.

Thanks

RE: [PATCH v2] r8152: Add support for setting MAC to system's Auxiliary MAC address

[Mario_Limonciello]

> -Original Message-
> From: Andrew Lunn [mailto:and...@lunn.ch]
> Sent: Thursday, June 2, 2016 2:10 PM
> To: Limonciello, Mario 
> Cc: gre...@linuxfoundation.org; hayesw...@realtek.com; linux-
> ker...@vger.kernel.org; net...@vger.kernel.org; linux-
> u...@vger.kernel.org; pali.ro...@gmail.com; anthony.w...@canonical.com
> Subject: Re: [PATCH v2] r8152: Add support for setting MAC to system's
> Auxiliary MAC address
> 
> On Thu, Jun 02, 2016 at 07:04:32PM +, mario_limoncie...@dell.com
> wrote:
> > > -Original Message-
> > > From: Andrew Lunn [mailto:and...@lunn.ch]
> > > Sent: Thursday, June 2, 2016 2:03 PM
> > > To: Limonciello, Mario 
> > > Cc: gre...@linuxfoundation.org; hayesw...@realtek.com; linux-
> > > ker...@vger.kernel.org; net...@vger.kernel.org; linux-
> > > u...@vger.kernel.org; pali.ro...@gmail.com;
> anthony.w...@canonical.com
> > > Subject: Re: [PATCH v2] r8152: Add support for setting MAC to system's
> > > Auxiliary MAC address
> > >
> > > > > And you want to check this for all Dell devices?  Please be model
> > > > > specific, I doubt a bunch of Dell servers wants to run this code...
> > > > >
> > > >
> > > > Tracking model specific is really going to turn into a giant list never
> ending
> > > list.
> > > > To drill down more specifically, I can match on chassis too.
> > >
> > > Does Dell happen to use its own USB Vendor ID for the USB device in
> > > the dock? You could go at this problem from the other direction if it
> > > does have a unique vendor ID.
> > >
> > >  Andrew
> >
> > Unfortunately it's not a Dell specific VID/PID.  I'm asking around to find 
> > out
> > if there is something else identifiable about this dock's NIC (maybe that
> r8152
> > can query).
> 
> lsusb -v
> 
> I assume there is a USB hub in the dock, maybe that has a Dell VID?
> Going one level up the USB tree hierarchy should not be too hard.
> 
>   Andrew

I confirmed with some internal contacts a few extra bits about questions 
about this from throughout the conversation.

1) This feature IS in the general Realtek driver for Windows, not a special 
Dell driver.
2) The feature only offered to the Realtek 8153-AD, no other Realtek chip will 
do it 
(even Realtek 8153-VB won't).
3) The feature is activated when a special "Docking" bit is set on the 8153-AD.
4) The feature is activated on all Realtek 8153-AD's on the host system with the
docking bit set.  So on Windows if two Dell docks were hooked up they would 
both 
have the same AUX MAC. 

So considering that I think it would make sense to only activate in those same
special circumstances (RTL8153-AD, docking bit set, system has \\_SB.AMAC).  
I also feel this should stay in the Realtek driver as this as this 
implementation is
very Realtek specific.

RE: [PATCH v2] r8152: Add support for setting MAC to system's Auxiliary MAC address

[Mario_Limonciello]

> -Original Message-
> From: Andrew Lunn [mailto:and...@lunn.ch]
> Sent: Thursday, June 2, 2016 2:10 PM
> To: Limonciello, Mario 
> Cc: gre...@linuxfoundation.org; hayesw...@realtek.com; linux-
> ker...@vger.kernel.org; net...@vger.kernel.org; linux-
> u...@vger.kernel.org; pali.ro...@gmail.com; anthony.w...@canonical.com
> Subject: Re: [PATCH v2] r8152: Add support for setting MAC to system's
> Auxiliary MAC address
> 
> On Thu, Jun 02, 2016 at 07:04:32PM +, mario_limoncie...@dell.com
> wrote:
> > > -Original Message-
> > > From: Andrew Lunn [mailto:and...@lunn.ch]
> > > Sent: Thursday, June 2, 2016 2:03 PM
> > > To: Limonciello, Mario 
> > > Cc: gre...@linuxfoundation.org; hayesw...@realtek.com; linux-
> > > ker...@vger.kernel.org; net...@vger.kernel.org; linux-
> > > u...@vger.kernel.org; pali.ro...@gmail.com;
> anthony.w...@canonical.com
> > > Subject: Re: [PATCH v2] r8152: Add support for setting MAC to system's
> > > Auxiliary MAC address
> > >
> > > > > And you want to check this for all Dell devices?  Please be model
> > > > > specific, I doubt a bunch of Dell servers wants to run this code...
> > > > >
> > > >
> > > > Tracking model specific is really going to turn into a giant list never
> ending
> > > list.
> > > > To drill down more specifically, I can match on chassis too.
> > >
> > > Does Dell happen to use its own USB Vendor ID for the USB device in
> > > the dock? You could go at this problem from the other direction if it
> > > does have a unique vendor ID.
> > >
> > >  Andrew
> >
> > Unfortunately it's not a Dell specific VID/PID.  I'm asking around to find 
> > out
> > if there is something else identifiable about this dock's NIC (maybe that
> r8152
> > can query).
> 
> lsusb -v
> 
> I assume there is a USB hub in the dock, maybe that has a Dell VID?
> Going one level up the USB tree hierarchy should not be too hard.
> 
>   Andrew

I confirmed with some internal contacts a few extra bits about questions 
about this from throughout the conversation.

1) This feature IS in the general Realtek driver for Windows, not a special 
Dell driver.
2) The feature only offered to the Realtek 8153-AD, no other Realtek chip will 
do it 
(even Realtek 8153-VB won't).
3) The feature is activated when a special "Docking" bit is set on the 8153-AD.
4) The feature is activated on all Realtek 8153-AD's on the host system with the
docking bit set.  So on Windows if two Dell docks were hooked up they would 
both 
have the same AUX MAC. 

So considering that I think it would make sense to only activate in those same
special circumstances (RTL8153-AD, docking bit set, system has \\_SB.AMAC).  
I also feel this should stay in the Realtek driver as this as this 
implementation is
very Realtek specific.

RE: [PATCH] r8152: Add support for setting MAC to system's Auxiliary MAC address

[Mario_Limonciello]

> I have some other questions which answers should we know:
> 
> 1) Is that AUX MAC address implemented only in customized windows Dell
> driver? Or also in "upstream" windows Realtek driver and all users of
> Realtek hw can install it (or update via next driver update)?
> 

I don't have the information on this.  Realtek will have to comment here as this
part is a black box to me.  I'm asking my internal colleagues about this too.

> 2) Can you share pseudo code or description of algorithm which decide
> MAC address for newly connected r8152 device on windows? This could help
> us to decide if something similar/same cannot be implemented also on
> linux (either in kernel or userspace). What I would like to know are
> those situations when you connect more r8152 devices (some Dell and some
> non-Dell).
> 

This is another thing I don't have the information for right now.
I can install Windows on a laptop, install the Realtek driver and experiment,
but it would be better to get this directly from Realtek if at all possible.

> > I do have a way to query if a dock is plugged in via SMM, but I doubt
> > that's what Realtek is using on the Windows side.
> 
> So there is some way to check if Dell dock is plugged, right? But what
> happen if you connect Dell dock and also non-Dell r8152 device? Can you
> distinguish which device is Dell and which non-Dell?

Yes, when querying if a Dell dock is plugged in, a "location" and "count" 
parameter is returned.  I'd have to figure out how to translate that into
what the Linux kernel sees.  Actually the information for how to do this 
is already public too.  It's in a pull request for Dock FW updating in the 
fwupd project.

https://github.com/hughsie/fwupd/pull/49/files#diff-81b55c87ce1542a18b0a4b2b228b9129R189

> 
> Anyway, I think that by SMM you mean dell smbios API call. Cannot you
> guys in Dell release documentation of all smbios calls to community?

Well dell SMBIOS API call really means to use dcdbas kernel module which
does SMM..

> Time to time you release some small parts in libsmbios project which
> then we can use for implementing useful parts in kernel (e.g. LED driver
> for controlling keyboard backlight). But there are couple of
> undocumented APIs and maybe some can also help with this problem...
> 

Releasing different bits of our SMBIOS document requires approvals.
We can't just release the whole thing as there are lots of interfaces that
aren't intended for the OS to be using.  They're used only by Dell tools.

For example we just had approval for information about querying TPM
and dock information and those are present in the fwupd pull request
for dock and TPM FW updates you see above.

If you have some API's in particular you would like more information on,
I'm happy to have internal discussion to see if we can release information
on those.

> > I'd leave that as
> > a second to last resort (last resort being move back to userspace
> > again).
> >
> > > What you definitely should not do is to change the mac for some
> > > arbitrary "first" device.  Then you are better off with the
> > > userspace proposal where you and your users have some chance to
> > > implement a sensible policy based on e.g. usb port numbers.
> >
> > OK, if I can't come up with a way to key on the device being a Dell
> > dock I'll scrap this entirely kernel approach.
> 
> E.g. PCI devices have ordinary PCI device & vendor IDs, but have Dell
> specific subsystem IDs. And via subsystem IDs we can distinguish between
> Intel graphics card on Dell laptop and on non-Dell laptop.
> 
> Does not you have some special/modified firmware in those Dell realtek
> docks (and ability to check from OS some registers)?

I think so.  Otherwise there would be all the same concerns you have outlined
with generic devices.  Like I said this part is currently a black box to me.  
I hope Realtek can publicly comment on this, or I can get some information 
from my colleagues.

RE: [PATCH] r8152: Add support for setting MAC to system's Auxiliary MAC address

[Mario_Limonciello]

> I have some other questions which answers should we know:
> 
> 1) Is that AUX MAC address implemented only in customized windows Dell
> driver? Or also in "upstream" windows Realtek driver and all users of
> Realtek hw can install it (or update via next driver update)?
> 

I don't have the information on this.  Realtek will have to comment here as this
part is a black box to me.  I'm asking my internal colleagues about this too.

> 2) Can you share pseudo code or description of algorithm which decide
> MAC address for newly connected r8152 device on windows? This could help
> us to decide if something similar/same cannot be implemented also on
> linux (either in kernel or userspace). What I would like to know are
> those situations when you connect more r8152 devices (some Dell and some
> non-Dell).
> 

This is another thing I don't have the information for right now.
I can install Windows on a laptop, install the Realtek driver and experiment,
but it would be better to get this directly from Realtek if at all possible.

> > I do have a way to query if a dock is plugged in via SMM, but I doubt
> > that's what Realtek is using on the Windows side.
> 
> So there is some way to check if Dell dock is plugged, right? But what
> happen if you connect Dell dock and also non-Dell r8152 device? Can you
> distinguish which device is Dell and which non-Dell?

Yes, when querying if a Dell dock is plugged in, a "location" and "count" 
parameter is returned.  I'd have to figure out how to translate that into
what the Linux kernel sees.  Actually the information for how to do this 
is already public too.  It's in a pull request for Dock FW updating in the 
fwupd project.

https://github.com/hughsie/fwupd/pull/49/files#diff-81b55c87ce1542a18b0a4b2b228b9129R189

> 
> Anyway, I think that by SMM you mean dell smbios API call. Cannot you
> guys in Dell release documentation of all smbios calls to community?

Well dell SMBIOS API call really means to use dcdbas kernel module which
does SMM..

> Time to time you release some small parts in libsmbios project which
> then we can use for implementing useful parts in kernel (e.g. LED driver
> for controlling keyboard backlight). But there are couple of
> undocumented APIs and maybe some can also help with this problem...
> 

Releasing different bits of our SMBIOS document requires approvals.
We can't just release the whole thing as there are lots of interfaces that
aren't intended for the OS to be using.  They're used only by Dell tools.

For example we just had approval for information about querying TPM
and dock information and those are present in the fwupd pull request
for dock and TPM FW updates you see above.

If you have some API's in particular you would like more information on,
I'm happy to have internal discussion to see if we can release information
on those.

> > I'd leave that as
> > a second to last resort (last resort being move back to userspace
> > again).
> >
> > > What you definitely should not do is to change the mac for some
> > > arbitrary "first" device.  Then you are better off with the
> > > userspace proposal where you and your users have some chance to
> > > implement a sensible policy based on e.g. usb port numbers.
> >
> > OK, if I can't come up with a way to key on the device being a Dell
> > dock I'll scrap this entirely kernel approach.
> 
> E.g. PCI devices have ordinary PCI device & vendor IDs, but have Dell
> specific subsystem IDs. And via subsystem IDs we can distinguish between
> Intel graphics card on Dell laptop and on non-Dell laptop.
> 
> Does not you have some special/modified firmware in those Dell realtek
> docks (and ability to check from OS some registers)?

I think so.  Otherwise there would be all the same concerns you have outlined
with generic devices.  Like I said this part is currently a black box to me.  
I hope Realtek can publicly comment on this, or I can get some information 
from my colleagues.

RE: [PATCH v2] r8152: Add support for setting MAC to system's Auxiliary MAC address

[Mario_Limonciello]

> -Original Message-
> From: Andrew Lunn [mailto:and...@lunn.ch]
> Sent: Thursday, June 2, 2016 2:03 PM
> To: Limonciello, Mario 
> Cc: gre...@linuxfoundation.org; hayesw...@realtek.com; linux-
> ker...@vger.kernel.org; net...@vger.kernel.org; linux-
> u...@vger.kernel.org; pali.ro...@gmail.com; anthony.w...@canonical.com
> Subject: Re: [PATCH v2] r8152: Add support for setting MAC to system's
> Auxiliary MAC address
> 
> > > And you want to check this for all Dell devices?  Please be model
> > > specific, I doubt a bunch of Dell servers wants to run this code...
> > >
> >
> > Tracking model specific is really going to turn into a giant list never 
> > ending
> list.
> > To drill down more specifically, I can match on chassis too.
> 
> Does Dell happen to use its own USB Vendor ID for the USB device in
> the dock? You could go at this problem from the other direction if it
> does have a unique vendor ID.
> 
>  Andrew

Unfortunately it's not a Dell specific VID/PID.  I'm asking around to find out
if there is something else identifiable about this dock's NIC (maybe that r8152 
can query).

RE: [PATCH v2] r8152: Add support for setting MAC to system's Auxiliary MAC address

[Mario_Limonciello]

> -Original Message-
> From: Andrew Lunn [mailto:and...@lunn.ch]
> Sent: Thursday, June 2, 2016 2:03 PM
> To: Limonciello, Mario 
> Cc: gre...@linuxfoundation.org; hayesw...@realtek.com; linux-
> ker...@vger.kernel.org; net...@vger.kernel.org; linux-
> u...@vger.kernel.org; pali.ro...@gmail.com; anthony.w...@canonical.com
> Subject: Re: [PATCH v2] r8152: Add support for setting MAC to system's
> Auxiliary MAC address
> 
> > > And you want to check this for all Dell devices?  Please be model
> > > specific, I doubt a bunch of Dell servers wants to run this code...
> > >
> >
> > Tracking model specific is really going to turn into a giant list never 
> > ending
> list.
> > To drill down more specifically, I can match on chassis too.
> 
> Does Dell happen to use its own USB Vendor ID for the USB device in
> the dock? You could go at this problem from the other direction if it
> does have a unique vendor ID.
> 
>  Andrew

Unfortunately it's not a Dell specific VID/PID.  I'm asking around to find out
if there is something else identifiable about this dock's NIC (maybe that r8152 
can query).

RE: [PATCH v2] r8152: Add support for setting MAC to system's Auxiliary MAC address

[Mario_Limonciello]

> -Original Message-
> From: Greg KH [mailto:gre...@linuxfoundation.org]
> Sent: Thursday, June 2, 2016 12:48 PM
> To: Limonciello, Mario 
> Cc: hayesw...@realtek.com; LKML ; Netdev
> ; Linux USB ;
> pali.ro...@gmail.com; anthony.w...@canonical.com
> Subject: Re: [PATCH v2] r8152: Add support for setting MAC to system's
> Auxiliary MAC address
> 
> On Thu, Jun 02, 2016 at 11:58:07AM -0500, Mario Limonciello wrote:
> > Dell systems with Type-C ports have support for a persistent system
> > specific MAC address when used with Dell Type-C docks and dongles.
> > This means a dock plugged into two different systems will show different
> > (but persistent) MAC addresses.  Dell Type-C docks and dongles use the
> > r8152 driver.
> >
> > This information for the system's persistent MAC address is burned in
> when
> > the HW is built and available under _SB\AMAC in the DSDT at runtime.
> >
> > More information about the technology is available here:
> > http://www.dell.com/support/article/us/en/04/SLN301147
> >
> > Signed-off-by: Mario Limonciello 
> > ---
> >  drivers/net/usb/r8152.c | 53
> +
> >  1 file changed, 53 insertions(+)
> >
> > diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> > index 3f9f6ed..6dea542 100644
> > --- a/drivers/net/usb/r8152.c
> > +++ b/drivers/net/usb/r8152.c
> > @@ -26,6 +26,8 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> > +#include 
> >
> >  /* Information for net-next */
> >  #define NETNEXT_VERSION"08"
> > @@ -500,6 +502,7 @@ enum rtl8152_flags {
> > SELECTIVE_SUSPEND,
> > PHY_RESET,
> > SCHEDULE_NAPI,
> > +   MAC_PASSTHRU = 0,
> 
> Does setting that to 0 really work?  You just did this for two enum
> values, what is the compiler supposed to do?

Very silly of me.  I was rushing to send a v2.  
I'm surprised this worked.  Shouldn't be assigned to anything.

> 
> >  };
> >
> >  /* Define these values to match your device */
> > @@ -653,6 +656,7 @@ enum tx_csum_stat {
> >   */
> >  static const int multicast_filter_limit = 32;
> >  static unsigned int agg_buf_sz = 16384;
> > +static bool mac_passthru_active;
> 
> very generic name for a platform-specific feature :(

Once this is broken up into an x86 platform provided method I'll rename this 
to platform_mac_active (or something similar).

> 
> 
> >
> >  #define RTL_LIMITED_TSO_SIZE   (agg_buf_sz - sizeof(struct tx_desc) -
> \
> >  VLAN_ETH_HLEN - VLAN_HLEN)
> > @@ -1030,6 +1034,49 @@ out1:
> > return ret;
> >  }
> >
> > +static int get_auxiliary_addr(struct r8152 *tp, struct sockaddr *sa)
> 
> What about the platform mac address api that was pointed out?

I mentioned this in the cover letter - I haven't gotten a chance to move it 
over there yet.
I sent v2 before I did so that you can see what I've been doing as it was 
relevant to your
other comments.

> 
> > +{
> > +   acpi_status status;
> > +   struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> > +   union acpi_object *obj;
> > +   int ret = -1;
> > +   unsigned char buf[6];
> > +
> > +   if (!dmi_name_in_vendors("Dell Inc.") || mac_passthru_active)
> > +   return -1;
> 
> Don't make up random error values, please use "real" ones.

OK.

> 
> And you want to check this for all Dell devices?  Please be model
> specific, I doubt a bunch of Dell servers wants to run this code...
> 

Tracking model specific is really going to turn into a giant list never ending 
list.
To drill down more specifically, I can match on chassis too.

> > +
> > +   /* returns _AUXMAC_#AABBCCDDEEFF# */
> > +   status = acpi_evaluate_object(NULL, "\\_SB.AMAC", NULL, );
> > +   obj = (union acpi_object *)buffer.pointer;
> > +   if (ACPI_SUCCESS(status)) {
> > +   if (obj->type != ACPI_TYPE_BUFFER ||
> > +   obj->string.length != 0x17) {
> > +   pr_warn("r8152: get_auxiliary_addr: Invalid buffer");
> > +   goto amacout;
> > +   }
> > +   if (strncmp(obj->string.pointer, "_AUXMAC_#", 9) != 0) {
> > +   pr_warn("r8152: get_auxiliary_addr: Invalid header");
> > +   goto amacout;
> > +   }
> > +   ret = hex2bin(buf, obj->string.pointer + 9, 6);
> > +   if (ret < 0) {
> > +   pr_warn("r8152: get_auxiliary_addr: Invalid MAC");
> > +   goto amacout;
> > +   }
> > +   memcpy(sa->sa_data, buf, 6);
> > +   ether_addr_copy(tp->netdev->dev_addr, sa->sa_data);
> > +   netdev_info(tp->netdev, "Using system MAC address
> %pM\n",
> > +   sa->sa_data);
> > +   set_bit(MAC_PASSTHRU, >flags);
> > +   mac_passthru_active = true;
> > +   ret = 1;
> 
> 1 is not a "all is good" return value.

OK will switch 

RE: [PATCH v2] r8152: Add support for setting MAC to system's Auxiliary MAC address

[Mario_Limonciello]

> -Original Message-
> From: Greg KH [mailto:gre...@linuxfoundation.org]
> Sent: Thursday, June 2, 2016 12:48 PM
> To: Limonciello, Mario 
> Cc: hayesw...@realtek.com; LKML ; Netdev
> ; Linux USB ;
> pali.ro...@gmail.com; anthony.w...@canonical.com
> Subject: Re: [PATCH v2] r8152: Add support for setting MAC to system's
> Auxiliary MAC address
> 
> On Thu, Jun 02, 2016 at 11:58:07AM -0500, Mario Limonciello wrote:
> > Dell systems with Type-C ports have support for a persistent system
> > specific MAC address when used with Dell Type-C docks and dongles.
> > This means a dock plugged into two different systems will show different
> > (but persistent) MAC addresses.  Dell Type-C docks and dongles use the
> > r8152 driver.
> >
> > This information for the system's persistent MAC address is burned in
> when
> > the HW is built and available under _SB\AMAC in the DSDT at runtime.
> >
> > More information about the technology is available here:
> > http://www.dell.com/support/article/us/en/04/SLN301147
> >
> > Signed-off-by: Mario Limonciello 
> > ---
> >  drivers/net/usb/r8152.c | 53
> +
> >  1 file changed, 53 insertions(+)
> >
> > diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> > index 3f9f6ed..6dea542 100644
> > --- a/drivers/net/usb/r8152.c
> > +++ b/drivers/net/usb/r8152.c
> > @@ -26,6 +26,8 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> > +#include 
> >
> >  /* Information for net-next */
> >  #define NETNEXT_VERSION"08"
> > @@ -500,6 +502,7 @@ enum rtl8152_flags {
> > SELECTIVE_SUSPEND,
> > PHY_RESET,
> > SCHEDULE_NAPI,
> > +   MAC_PASSTHRU = 0,
> 
> Does setting that to 0 really work?  You just did this for two enum
> values, what is the compiler supposed to do?

Very silly of me.  I was rushing to send a v2.  
I'm surprised this worked.  Shouldn't be assigned to anything.

> 
> >  };
> >
> >  /* Define these values to match your device */
> > @@ -653,6 +656,7 @@ enum tx_csum_stat {
> >   */
> >  static const int multicast_filter_limit = 32;
> >  static unsigned int agg_buf_sz = 16384;
> > +static bool mac_passthru_active;
> 
> very generic name for a platform-specific feature :(

Once this is broken up into an x86 platform provided method I'll rename this 
to platform_mac_active (or something similar).

> 
> 
> >
> >  #define RTL_LIMITED_TSO_SIZE   (agg_buf_sz - sizeof(struct tx_desc) -
> \
> >  VLAN_ETH_HLEN - VLAN_HLEN)
> > @@ -1030,6 +1034,49 @@ out1:
> > return ret;
> >  }
> >
> > +static int get_auxiliary_addr(struct r8152 *tp, struct sockaddr *sa)
> 
> What about the platform mac address api that was pointed out?

I mentioned this in the cover letter - I haven't gotten a chance to move it 
over there yet.
I sent v2 before I did so that you can see what I've been doing as it was 
relevant to your
other comments.

> 
> > +{
> > +   acpi_status status;
> > +   struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> > +   union acpi_object *obj;
> > +   int ret = -1;
> > +   unsigned char buf[6];
> > +
> > +   if (!dmi_name_in_vendors("Dell Inc.") || mac_passthru_active)
> > +   return -1;
> 
> Don't make up random error values, please use "real" ones.

OK.

> 
> And you want to check this for all Dell devices?  Please be model
> specific, I doubt a bunch of Dell servers wants to run this code...
> 

Tracking model specific is really going to turn into a giant list never ending 
list.
To drill down more specifically, I can match on chassis too.

> > +
> > +   /* returns _AUXMAC_#AABBCCDDEEFF# */
> > +   status = acpi_evaluate_object(NULL, "\\_SB.AMAC", NULL, );
> > +   obj = (union acpi_object *)buffer.pointer;
> > +   if (ACPI_SUCCESS(status)) {
> > +   if (obj->type != ACPI_TYPE_BUFFER ||
> > +   obj->string.length != 0x17) {
> > +   pr_warn("r8152: get_auxiliary_addr: Invalid buffer");
> > +   goto amacout;
> > +   }
> > +   if (strncmp(obj->string.pointer, "_AUXMAC_#", 9) != 0) {
> > +   pr_warn("r8152: get_auxiliary_addr: Invalid header");
> > +   goto amacout;
> > +   }
> > +   ret = hex2bin(buf, obj->string.pointer + 9, 6);
> > +   if (ret < 0) {
> > +   pr_warn("r8152: get_auxiliary_addr: Invalid MAC");
> > +   goto amacout;
> > +   }
> > +   memcpy(sa->sa_data, buf, 6);
> > +   ether_addr_copy(tp->netdev->dev_addr, sa->sa_data);
> > +   netdev_info(tp->netdev, "Using system MAC address
> %pM\n",
> > +   sa->sa_data);
> > +   set_bit(MAC_PASSTHRU, >flags);
> > +   mac_passthru_active = true;
> > +   ret = 1;
> 
> 1 is not a "all is good" return value.

OK will switch to 0.

RE: [PATCH] r8152: Add support for setting MAC to system's Auxiliary MAC address

[Mario_Limonciello]

> -Original Message-
> From: Bjørn Mork [mailto:bj...@mork.no]
> Sent: Thursday, June 2, 2016 1:04 PM
> To: Limonciello, Mario 
> Cc: gre...@linuxfoundation.org; hayesw...@realtek.com; linux-
> ker...@vger.kernel.org; net...@vger.kernel.org; linux-
> u...@vger.kernel.org; pali.ro...@gmail.com; anthony.w...@canonical.com
> Subject: Re: [PATCH] r8152: Add support for setting MAC to system's
> Auxiliary MAC address
> 
>  writes:
> 
> >> > 2) Track whether this is the first or second USB NIC plugged in.  Only
> offer it
> >> on the first NIC detected by r8152.  When the second NIC is plugged in
> don't
> >> match from ACPI.
> >> > There would be a question of what to do if the first NIC is removed and
> >> added back if it should get the persistent system MAC or not.
> >> > I'd say yes, just make sure that only one NIC can have it at a time.
> >>
> >> You are going to get things very complex very quickly if you try to do 
> >> this.
> >
> > It's really not that hard, track a module wide static variable whether
> > the feature is in use.  Track in each device whether the feature was
> > in use.  If it in use, don't assign the next device plugged in via the
> > ACPI string.  If a device is removed that has the feature activated,
> > change the module wide static variable.
> 
> Having the mac address jump around in an arbitrary way like this is
> going to confuse the hell out of your users.  Consider what happens if
> the user docks a laptop with an r8152 usb dongle already plugged in...
> How are you going to explain that the dock gets some other mac address
> in this case? How are you going to explain the difference between using
> an r8152 based dongle and some other ethernet usb dongle with your
> systems?

Yeah I understand the concern.  I agree that would be very confusing
to a user.  This does need to match only on Dell docks then.

> 
> Make it behave consistently if you're going to add this.  Which can be
> done by specifically matching the Dell dock (doesn't it have an unique
> Dell device ID?) and ignoring any other r8152 device.  You could also
> choose to set the same mac for all r8152 devices.  Which is fine, but
> will probably confuse many users.

Unfortunately there is no Dell specific VID/PID.  I checked a no-name dongle
that used r8152 and it was the same (0bda:8153).  Maybe Hayes Wang can 
check with his Windows driver colleagues if there was anything else to key
off when this was implemented on the Windows Realtek driver.  If there 
is something else to key off of, I'm not aware what it is.  I'll check with 
some of my colleagues too.

I do have a way to query if a dock is plugged in via SMM, but I doubt that's
what Realtek is using on the Windows side.  I'd leave that as a second to
last resort (last resort being move back to userspace again).

> 
> What you definitely should not do is to change the mac for some
> arbitrary "first" device.  Then you are better off with the userspace
> proposal where you and your users have some chance to implement a
> sensible policy based on e.g. usb port numbers.

OK, if I can't come up with a way to key on the device being a Dell dock 
I'll scrap this entirely kernel approach.

RE: [PATCH] r8152: Add support for setting MAC to system's Auxiliary MAC address

[Mario_Limonciello]

> -Original Message-
> From: Bjørn Mork [mailto:bj...@mork.no]
> Sent: Thursday, June 2, 2016 1:04 PM
> To: Limonciello, Mario 
> Cc: gre...@linuxfoundation.org; hayesw...@realtek.com; linux-
> ker...@vger.kernel.org; net...@vger.kernel.org; linux-
> u...@vger.kernel.org; pali.ro...@gmail.com; anthony.w...@canonical.com
> Subject: Re: [PATCH] r8152: Add support for setting MAC to system's
> Auxiliary MAC address
> 
>  writes:
> 
> >> > 2) Track whether this is the first or second USB NIC plugged in.  Only
> offer it
> >> on the first NIC detected by r8152.  When the second NIC is plugged in
> don't
> >> match from ACPI.
> >> > There would be a question of what to do if the first NIC is removed and
> >> added back if it should get the persistent system MAC or not.
> >> > I'd say yes, just make sure that only one NIC can have it at a time.
> >>
> >> You are going to get things very complex very quickly if you try to do 
> >> this.
> >
> > It's really not that hard, track a module wide static variable whether
> > the feature is in use.  Track in each device whether the feature was
> > in use.  If it in use, don't assign the next device plugged in via the
> > ACPI string.  If a device is removed that has the feature activated,
> > change the module wide static variable.
> 
> Having the mac address jump around in an arbitrary way like this is
> going to confuse the hell out of your users.  Consider what happens if
> the user docks a laptop with an r8152 usb dongle already plugged in...
> How are you going to explain that the dock gets some other mac address
> in this case? How are you going to explain the difference between using
> an r8152 based dongle and some other ethernet usb dongle with your
> systems?

Yeah I understand the concern.  I agree that would be very confusing
to a user.  This does need to match only on Dell docks then.

> 
> Make it behave consistently if you're going to add this.  Which can be
> done by specifically matching the Dell dock (doesn't it have an unique
> Dell device ID?) and ignoring any other r8152 device.  You could also
> choose to set the same mac for all r8152 devices.  Which is fine, but
> will probably confuse many users.

Unfortunately there is no Dell specific VID/PID.  I checked a no-name dongle
that used r8152 and it was the same (0bda:8153).  Maybe Hayes Wang can 
check with his Windows driver colleagues if there was anything else to key
off when this was implemented on the Windows Realtek driver.  If there 
is something else to key off of, I'm not aware what it is.  I'll check with 
some of my colleagues too.

I do have a way to query if a dock is plugged in via SMM, but I doubt that's
what Realtek is using on the Windows side.  I'd leave that as a second to
last resort (last resort being move back to userspace again).

> 
> What you definitely should not do is to change the mac for some
> arbitrary "first" device.  Then you are better off with the userspace
> proposal where you and your users have some chance to implement a
> sensible policy based on e.g. usb port numbers.

OK, if I can't come up with a way to key on the device being a Dell dock 
I'll scrap this entirely kernel approach.

RE: [PATCH] r8152: Add support for setting MAC to system's Auxiliary MAC address

[Mario_Limonciello]

Some of my comments are getting stale with what I've done in response to all 
these emails.
Let me send a v2 that we can better iterate on, a few comments below though.

> -Original Message-
> From: Greg KH [mailto:gre...@linuxfoundation.org]
> Sent: Thursday, June 2, 2016 11:09 AM
> To: Limonciello, Mario 
> Cc: hayesw...@realtek.com; linux-kernel@vger.kernel.org;
> net...@vger.kernel.org; linux-...@vger.kernel.org; pali.ro...@gmail.com;
> anthony.w...@canonical.com
> Subject: Re: [PATCH] r8152: Add support for setting MAC to system's
> Auxiliary MAC address
> 
> On Thu, Jun 02, 2016 at 03:46:41PM +, mario_limoncie...@dell.com
> wrote:
> > > >
> > > > This isn't something part of ACPI - it's been added specifically for a
> > > > selection of Dell machines.
> > >
> > > Ah, but isn't ACPI supposed to be a "standard"?  :)
> > >
> >
> > Heh.
> > It's also possible to get this from an SMM routine.  Lesser of two evils to
> fetch the information this way, right? :)
> 
> Yes, but again, please only do this for machines you _know_ this value
> will be present on.  Otherwise you will end up with problems.

I'm going to send a V2, I'd like to know where and how this could still break. 
I am having a hard time grasping this.  

> 
> > > And please wrap your email lines, there is a "standard" for that...
> >
> > I'm unfortunately not limited to an evil mail client at my workplace since 
> > our
> mail server migration.   My apologies, I've got it set to wrap at 76 
> characters
> and I'm trying to make it as LKML friendly as possible.
> 
> It's not working as you can see here :(

Ugh, sorry.  Stupid outlook.  It seems to only be doing it on replies.
I'll manually just chop the lines when they're around that size until I've got a
better solution.

> 
> > > > I would rather not hardcode to the specific DMI model strings of those
> > > > Dell machines as it's certainly going to be a feature that expands to
> > > > more machines.  Since it is Dell specific though, if you would rather
> > > > me just match to the sys vendor Dell Inc., that seems like a pretty
> > > > good compromise to me.
> > >
> > > You need to only do this on machines you "know" have this set to a
> correct
> > > value, otherwise if some other random BIOS happens to set that field to
> > > some random value, you will have problems.
> >
> > Pali had recommended in another message to check the buffer header.  I
> was intending to do this along with check ACPI buffer output type, and
> output size in the next revision I submitted.  By switching to hex2bin, I'll 
> also
> validate that the string has correct values (0-F or 0-f).  If somehow all of 
> that
> fails, the set_ethernet_addr  checks if the address is valid.  If it's 
> invalid it will
> generate a random one.
> 
> Why generate a random one and not just use the one that the network
> controler already provides?

That's how the flow works in r8152 already and I'm not overriding it.
Again, I'll send V2 and you'll see what I did.

> >
> > It's really not that hard, track a module wide static variable whether the
> feature is in use.  Track in each device whether the feature was in use.  If 
> it in
> use, don't assign the next device plugged in via the ACPI string.  If a 
> device is
> removed that has the feature activated, change the module wide static
> variable.
> 
> Ok, let's see the code before I say anymore about this.
> 
> > > What's wrong with a "simple" script to set the mac address from
> userspace if
> > > the user wants something like this?  Provide it as a system package and
> then
> > > no kernel changes are needed at all.  Much easier to support on your end
> > > (you don't have to maintain this odd kernel code for
> > > 10+ years), the default behavior is as Linux users expect, and your
> > > limited number of people who want this crazy behaviour can install your
> > > script if they want it.
> > >
> >
> > This was my original approach.  It involved a network manager script,
> network manager code changes to support this, and exposing this
> somewhere in a platform module (like dell-laptop).  I was told I'm better off
> doing it directly in the network module, so here I am.
> 
> Why not a small systemd unit file for this that sets things up when the
> device is found in the system?  Why mess with network manager and a
> platform kernel driver at all?  That seems very complex for such a
> simple operation where the kernel doesn't need to be involved at all,
> especially for such a "niche" product.
> 
> See this link:
>   https://wiki.archlinux.org/index.php/MAC_address_spoofing#Auto
> matically
> 

The ACPI subsystem doesn't create a sysfs node for a random buffer under _SB.
I don't think the ACPI guys would be crazy about this either.

So you need a platform kernel driver to pull this out of ACPI (or SMM) and 
expose
into userspace somewhere in the first place.  I was putting it into a random 
sysfs
attribute when I did my first attempts 

RE: [PATCH] r8152: Add support for setting MAC to system's Auxiliary MAC address

[Mario_Limonciello]

Some of my comments are getting stale with what I've done in response to all 
these emails.
Let me send a v2 that we can better iterate on, a few comments below though.

> -Original Message-
> From: Greg KH [mailto:gre...@linuxfoundation.org]
> Sent: Thursday, June 2, 2016 11:09 AM
> To: Limonciello, Mario 
> Cc: hayesw...@realtek.com; linux-kernel@vger.kernel.org;
> net...@vger.kernel.org; linux-...@vger.kernel.org; pali.ro...@gmail.com;
> anthony.w...@canonical.com
> Subject: Re: [PATCH] r8152: Add support for setting MAC to system's
> Auxiliary MAC address
> 
> On Thu, Jun 02, 2016 at 03:46:41PM +, mario_limoncie...@dell.com
> wrote:
> > > >
> > > > This isn't something part of ACPI - it's been added specifically for a
> > > > selection of Dell machines.
> > >
> > > Ah, but isn't ACPI supposed to be a "standard"?  :)
> > >
> >
> > Heh.
> > It's also possible to get this from an SMM routine.  Lesser of two evils to
> fetch the information this way, right? :)
> 
> Yes, but again, please only do this for machines you _know_ this value
> will be present on.  Otherwise you will end up with problems.

I'm going to send a V2, I'd like to know where and how this could still break. 
I am having a hard time grasping this.  

> 
> > > And please wrap your email lines, there is a "standard" for that...
> >
> > I'm unfortunately not limited to an evil mail client at my workplace since 
> > our
> mail server migration.   My apologies, I've got it set to wrap at 76 
> characters
> and I'm trying to make it as LKML friendly as possible.
> 
> It's not working as you can see here :(

Ugh, sorry.  Stupid outlook.  It seems to only be doing it on replies.
I'll manually just chop the lines when they're around that size until I've got a
better solution.

> 
> > > > I would rather not hardcode to the specific DMI model strings of those
> > > > Dell machines as it's certainly going to be a feature that expands to
> > > > more machines.  Since it is Dell specific though, if you would rather
> > > > me just match to the sys vendor Dell Inc., that seems like a pretty
> > > > good compromise to me.
> > >
> > > You need to only do this on machines you "know" have this set to a
> correct
> > > value, otherwise if some other random BIOS happens to set that field to
> > > some random value, you will have problems.
> >
> > Pali had recommended in another message to check the buffer header.  I
> was intending to do this along with check ACPI buffer output type, and
> output size in the next revision I submitted.  By switching to hex2bin, I'll 
> also
> validate that the string has correct values (0-F or 0-f).  If somehow all of 
> that
> fails, the set_ethernet_addr  checks if the address is valid.  If it's 
> invalid it will
> generate a random one.
> 
> Why generate a random one and not just use the one that the network
> controler already provides?

That's how the flow works in r8152 already and I'm not overriding it.
Again, I'll send V2 and you'll see what I did.

> >
> > It's really not that hard, track a module wide static variable whether the
> feature is in use.  Track in each device whether the feature was in use.  If 
> it in
> use, don't assign the next device plugged in via the ACPI string.  If a 
> device is
> removed that has the feature activated, change the module wide static
> variable.
> 
> Ok, let's see the code before I say anymore about this.
> 
> > > What's wrong with a "simple" script to set the mac address from
> userspace if
> > > the user wants something like this?  Provide it as a system package and
> then
> > > no kernel changes are needed at all.  Much easier to support on your end
> > > (you don't have to maintain this odd kernel code for
> > > 10+ years), the default behavior is as Linux users expect, and your
> > > limited number of people who want this crazy behaviour can install your
> > > script if they want it.
> > >
> >
> > This was my original approach.  It involved a network manager script,
> network manager code changes to support this, and exposing this
> somewhere in a platform module (like dell-laptop).  I was told I'm better off
> doing it directly in the network module, so here I am.
> 
> Why not a small systemd unit file for this that sets things up when the
> device is found in the system?  Why mess with network manager and a
> platform kernel driver at all?  That seems very complex for such a
> simple operation where the kernel doesn't need to be involved at all,
> especially for such a "niche" product.
> 
> See this link:
>   https://wiki.archlinux.org/index.php/MAC_address_spoofing#Auto
> matically
> 

The ACPI subsystem doesn't create a sysfs node for a random buffer under _SB.
I don't think the ACPI guys would be crazy about this either.

So you need a platform kernel driver to pull this out of ACPI (or SMM) and 
expose
into userspace somewhere in the first place.  I was putting it into a random 
sysfs
attribute when I did my first attempts at a userspace approach.

So 

RE: [PATCH] r8152: Add support for setting MAC to system's Auxiliary MAC address

[Mario_Limonciello]

> >
> > This isn't something part of ACPI - it's been added specifically for a
> > selection of Dell machines.
> 
> Ah, but isn't ACPI supposed to be a "standard"?  :)
> 

Heh.
It's also possible to get this from an SMM routine.  Lesser of two evils to 
fetch the information this way, right? :)

> And please wrap your email lines, there is a "standard" for that...

I'm unfortunately not limited to an evil mail client at my workplace since our 
mail server migration.   My apologies, I've got it set to wrap at 76 characters 
and I'm trying to make it as LKML friendly as possible.

> 
> > I would rather not hardcode to the specific DMI model strings of those
> > Dell machines as it's certainly going to be a feature that expands to
> > more machines.  Since it is Dell specific though, if you would rather
> > me just match to the sys vendor Dell Inc., that seems like a pretty
> > good compromise to me.
> 
> You need to only do this on machines you "know" have this set to a correct
> value, otherwise if some other random BIOS happens to set that field to
> some random value, you will have problems.

Pali had recommended in another message to check the buffer header.  I was 
intending to do this along with check ACPI buffer output type, and output size 
in the next revision I submitted.  By switching to hex2bin, I'll also validate 
that the string has correct values (0-F or 0-f).  If somehow all of that fails, 
the set_ethernet_addr  checks if the address is valid.  If it's invalid it will 
generate a random one.

> 
> > > And finally, this seems odd overall given that a MAC address should
> > > be associated with the specific network device, not the overall system.
> >
> > The whole reasoning behind this is to bring the behavior that E-Docks
> > had to TBT docks.
> 
> What is "TBT"?

Thunderbolt

> 
> > With E-docks they were really just extensions of the pins for the
> > already onboard NIC of the system.  When you docked in an E-dock you
> > inherently would use the same MAC everywhere you went.  If you had two
> > cubes your network admin would see your same MAC in both.
> >
> > With TBT docks and this patch not present docking in different places
> > will give you the MAC of the dock rather than something persistent
> > that your network admin could identify.  Solving this was something
> > that was explicitly asked for in case studies during conceptualization
> > of these docks and is a feature present in the Realtek Windows driver.
> 
> But you are dealing with different "devices" here, thunderbolt is a PCI
> device, not a "pin pass-through", and to treat it differently like you want 
> to is
> going to cause confusion as well.

Is it not also going to be confusing if someone boots Windows and Linux on the 
same laptop and has a different behavior with their dock's MAC address?  I'm 
trying to get parity here for organizations that are supporting both Windows 
and Linux for their employees.

> > In addition to limiting to Dell DMI system vendor string how about I do two
> more things about this:
> >
> > 1) Add a module parameter to disable this behavior altogether in r8152 if
> it's not desired by the user or admin (but leave it on by default).
> 
> No module parameters, this isn't the 1990's anymore, and you aren't going to
> be modifying module config files for everyone, are you?
> 
> And this seems like a "default" that should be turned off anyway, as it goes
> against the model of all of our other network devices in Linux.
> 
> > 2) Track whether this is the first or second USB NIC plugged in.  Only 
> > offer it
> on the first NIC detected by r8152.  When the second NIC is plugged in don't
> match from ACPI.
> > There would be a question of what to do if the first NIC is removed and
> added back if it should get the persistent system MAC or not.
> > I'd say yes, just make sure that only one NIC can have it at a time.
> 
> You are going to get things very complex very quickly if you try to do this.

It's really not that hard, track a module wide static variable whether the 
feature is in use.  Track in each device whether the feature was in use.  If it 
in use, don't assign the next device plugged in via the ACPI string.  If a 
device is removed that has the feature activated, change the module wide static 
variable.

> 
> What's wrong with a "simple" script to set the mac address from userspace if
> the user wants something like this?  Provide it as a system package and then
> no kernel changes are needed at all.  Much easier to support on your end
> (you don't have to maintain this odd kernel code for
> 10+ years), the default behavior is as Linux users expect, and your
> limited number of people who want this crazy behaviour can install your
> script if they want it.
> 

This was my original approach.  It involved a network manager script, network 
manager code changes to support this, and exposing this somewhere in a platform 
module (like dell-laptop).  I was told I'm better off doing it directly 

RE: [PATCH] r8152: Add support for setting MAC to system's Auxiliary MAC address

[Mario_Limonciello]

> >
> > This isn't something part of ACPI - it's been added specifically for a
> > selection of Dell machines.
> 
> Ah, but isn't ACPI supposed to be a "standard"?  :)
> 

Heh.
It's also possible to get this from an SMM routine.  Lesser of two evils to 
fetch the information this way, right? :)

> And please wrap your email lines, there is a "standard" for that...

I'm unfortunately not limited to an evil mail client at my workplace since our 
mail server migration.   My apologies, I've got it set to wrap at 76 characters 
and I'm trying to make it as LKML friendly as possible.

> 
> > I would rather not hardcode to the specific DMI model strings of those
> > Dell machines as it's certainly going to be a feature that expands to
> > more machines.  Since it is Dell specific though, if you would rather
> > me just match to the sys vendor Dell Inc., that seems like a pretty
> > good compromise to me.
> 
> You need to only do this on machines you "know" have this set to a correct
> value, otherwise if some other random BIOS happens to set that field to
> some random value, you will have problems.

Pali had recommended in another message to check the buffer header.  I was 
intending to do this along with check ACPI buffer output type, and output size 
in the next revision I submitted.  By switching to hex2bin, I'll also validate 
that the string has correct values (0-F or 0-f).  If somehow all of that fails, 
the set_ethernet_addr  checks if the address is valid.  If it's invalid it will 
generate a random one.

> 
> > > And finally, this seems odd overall given that a MAC address should
> > > be associated with the specific network device, not the overall system.
> >
> > The whole reasoning behind this is to bring the behavior that E-Docks
> > had to TBT docks.
> 
> What is "TBT"?

Thunderbolt

> 
> > With E-docks they were really just extensions of the pins for the
> > already onboard NIC of the system.  When you docked in an E-dock you
> > inherently would use the same MAC everywhere you went.  If you had two
> > cubes your network admin would see your same MAC in both.
> >
> > With TBT docks and this patch not present docking in different places
> > will give you the MAC of the dock rather than something persistent
> > that your network admin could identify.  Solving this was something
> > that was explicitly asked for in case studies during conceptualization
> > of these docks and is a feature present in the Realtek Windows driver.
> 
> But you are dealing with different "devices" here, thunderbolt is a PCI
> device, not a "pin pass-through", and to treat it differently like you want 
> to is
> going to cause confusion as well.

Is it not also going to be confusing if someone boots Windows and Linux on the 
same laptop and has a different behavior with their dock's MAC address?  I'm 
trying to get parity here for organizations that are supporting both Windows 
and Linux for their employees.

> > In addition to limiting to Dell DMI system vendor string how about I do two
> more things about this:
> >
> > 1) Add a module parameter to disable this behavior altogether in r8152 if
> it's not desired by the user or admin (but leave it on by default).
> 
> No module parameters, this isn't the 1990's anymore, and you aren't going to
> be modifying module config files for everyone, are you?
> 
> And this seems like a "default" that should be turned off anyway, as it goes
> against the model of all of our other network devices in Linux.
> 
> > 2) Track whether this is the first or second USB NIC plugged in.  Only 
> > offer it
> on the first NIC detected by r8152.  When the second NIC is plugged in don't
> match from ACPI.
> > There would be a question of what to do if the first NIC is removed and
> added back if it should get the persistent system MAC or not.
> > I'd say yes, just make sure that only one NIC can have it at a time.
> 
> You are going to get things very complex very quickly if you try to do this.

It's really not that hard, track a module wide static variable whether the 
feature is in use.  Track in each device whether the feature was in use.  If it 
in use, don't assign the next device plugged in via the ACPI string.  If a 
device is removed that has the feature activated, change the module wide static 
variable.

> 
> What's wrong with a "simple" script to set the mac address from userspace if
> the user wants something like this?  Provide it as a system package and then
> no kernel changes are needed at all.  Much easier to support on your end
> (you don't have to maintain this odd kernel code for
> 10+ years), the default behavior is as Linux users expect, and your
> limited number of people who want this crazy behaviour can install your
> script if they want it.
> 

This was my original approach.  It involved a network manager script, network 
manager code changes to support this, and exposing this somewhere in a platform 
module (like dell-laptop).  I was told I'm better off doing it directly 

RE: [PATCH] r8152: Add support for setting MAC to system's Auxiliary MAC address

[Mario_Limonciello]

> -Original Message-
> From: Andrew Lunn [mailto:and...@lunn.ch]
> Sent: Thursday, June 2, 2016 10:01 AM
> To: Limonciello, Mario 
> Cc: pali.ro...@gmail.com; hayesw...@realtek.com; linux-
> ker...@vger.kernel.org; net...@vger.kernel.org; linux-
> u...@vger.kernel.org; anthony.w...@canonical.com
> Subject: Re: [PATCH] r8152: Add support for setting MAC to system's
> Auxiliary MAC address
> 
> > >
> > > > +   pr_info("r8152: Using system auxiliary MAC address");
> > >
> > > It would be great to write also mac address into that pr_info
> 
> And since there could be multiple r8152 in the system, it would be good to
> indicate which of them is having its MAC changed. So
> netdev_info() or dev_info().
> 
>   Andrew

Thanks, will do that in next submission too.

RE: [PATCH] r8152: Add support for setting MAC to system's Auxiliary MAC address

[Mario_Limonciello]

> -Original Message-
> From: Andrew Lunn [mailto:and...@lunn.ch]
> Sent: Thursday, June 2, 2016 10:01 AM
> To: Limonciello, Mario 
> Cc: pali.ro...@gmail.com; hayesw...@realtek.com; linux-
> ker...@vger.kernel.org; net...@vger.kernel.org; linux-
> u...@vger.kernel.org; anthony.w...@canonical.com
> Subject: Re: [PATCH] r8152: Add support for setting MAC to system's
> Auxiliary MAC address
> 
> > >
> > > > +   pr_info("r8152: Using system auxiliary MAC address");
> > >
> > > It would be great to write also mac address into that pr_info
> 
> And since there could be multiple r8152 in the system, it would be good to
> indicate which of them is having its MAC changed. So
> netdev_info() or dev_info().
> 
>   Andrew

Thanks, will do that in next submission too.