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

Groups > linux.kernel > #1654962 > unrolled thread

Re: [PATCH v2 1/3] firmware: dmi: Add DMI_PRODUCT_FAMILY identification string

Started byJean Delvare <jdelvare@suse.de>
First post2017-06-01 11:30 +0200
Last post2017-06-01 13:10 +0200
Articles 4 — 2 participants

Back to article view | Back to linux.kernel

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

Contents

  Re: [PATCH v2 1/3] firmware: dmi: Add DMI_PRODUCT_FAMILY  identification string Jean Delvare <jdelvare@suse.de> - 2017-06-01 11:30 +0200
    Re: [PATCH v2 1/3] firmware: dmi: Add DMI_PRODUCT_FAMILY  identification string Mika Westerberg <mika.westerberg@linux.intel.com> - 2017-06-01 12:10 +0200
      Re: [PATCH v2 1/3] firmware: dmi: Add DMI_PRODUCT_FAMILY  identification string Jean Delvare <jdelvare@suse.de> - 2017-06-01 13:00 +0200
        Re: [PATCH v2 1/3] firmware: dmi: Add DMI_PRODUCT_FAMILY  identification string Mika Westerberg <mika.westerberg@linux.intel.com> - 2017-06-01 13:10 +0200

#1654962 — Re: [PATCH v2 1/3] firmware: dmi: Add DMI_PRODUCT_FAMILY identification string

FromJean Delvare <jdelvare@suse.de>
Date2017-06-01 11:30 +0200
SubjectRe: [PATCH v2 1/3] firmware: dmi: Add DMI_PRODUCT_FAMILY identification string
Message-ID<tNuAx-HW-7@gated-at.bofh.it>
Hi all,

Sorry for the late reply.

On Wed, 17 May 2017 13:25:12 +0300, Mika Westerberg wrote:
> Sometimes it is more convenient to be able to match a whole family of
> products, like in case of bunch of Chromebooks based on Intel_Strago to
> apply a driver quirk instead of quirking each machine one-by-one.
> 
> This adds support for DMI_PRODUCT_FAMILY identification string and also
> exports it to the userspace through sysfs attribute just like the
> existing ones.

dmidecode currently provides no direct access to this string. Do you
think it should?

> Suggested-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  drivers/firmware/dmi-id.c       | 2 ++
>  drivers/firmware/dmi_scan.c     | 1 +
>  include/linux/mod_devicetable.h | 1 +
>  3 files changed, 4 insertions(+)
> 
> diff --git a/drivers/firmware/dmi-id.c b/drivers/firmware/dmi-id.c
> index 44c01390d035..dc269cb288c2 100644
> --- a/drivers/firmware/dmi-id.c
> +++ b/drivers/firmware/dmi-id.c
> (...)
> @@ -191,6 +192,7 @@ static void __init dmi_id_init_attr_table(void)
>  	ADD_DMI_ATTR(product_version,   DMI_PRODUCT_VERSION);
>  	ADD_DMI_ATTR(product_serial,    DMI_PRODUCT_SERIAL);
>  	ADD_DMI_ATTR(product_uuid,      DMI_PRODUCT_UUID);
> +	ADD_DMI_ATTR(product_family,      DMI_PRODUCT_FAMILY);

Alignment, please!

>  	ADD_DMI_ATTR(board_vendor,      DMI_BOARD_VENDOR);
>  	ADD_DMI_ATTR(board_name,        DMI_BOARD_NAME);
>  	ADD_DMI_ATTR(board_version,     DMI_BOARD_VERSION);
> diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
> index 54be60ead08f..93f7acdaac7a 100644
> --- a/drivers/firmware/dmi_scan.c
> +++ b/drivers/firmware/dmi_scan.c
> @@ -430,6 +430,7 @@ static void __init dmi_decode(const struct dmi_header *dm, void *dummy)
>  		dmi_save_ident(dm, DMI_PRODUCT_VERSION, 6);
>  		dmi_save_ident(dm, DMI_PRODUCT_SERIAL, 7);
>  		dmi_save_uuid(dm, DMI_PRODUCT_UUID, 8);
> +		dmi_save_ident(dm, DMI_PRODUCT_FAMILY, 26);

This field only exists since SMBIOS 2.4. For older implementations, you
are accessing a random location of the DMI table. Most likely you'll
hit a character in one of the strings associated with the system
information structure. In turn this character will be interpreted as a
DMI string number. With some luck, number will be >= 32, so you'll get
a non-existent string and dmi_string will return "". But you could hit
a string terminator (0) and return the 1st string of the structure
instead (most likely the system manufacturer.)

Note that the problem is not specific to this field, it is just more
likely to break because all other fields are defined by SMBIOS 2.0, or
for the product UUID, SMBIOS 2.1. The fact that all dmi_save_*
functions blindly assume that the structure is long enough to contain
all the fields they want to save is problematic. This should be fixed
separately.

>  		break;
>  	case 2:		/* Base Board Information */
>  		dmi_save_ident(dm, DMI_BOARD_VENDOR, 4);
> diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
> index 566fda587fcf..3f74ef2281e8 100644
> --- a/include/linux/mod_devicetable.h
> +++ b/include/linux/mod_devicetable.h
> @@ -467,6 +467,7 @@ enum dmi_field {
>  	DMI_PRODUCT_VERSION,
>  	DMI_PRODUCT_SERIAL,
>  	DMI_PRODUCT_UUID,
> +	DMI_PRODUCT_FAMILY,
>  	DMI_BOARD_VENDOR,
>  	DMI_BOARD_NAME,
>  	DMI_BOARD_VERSION,


-- 
Jean Delvare
SUSE L3 Support

[toc] | [next] | [standalone]

#1654981

FromMika Westerberg <mika.westerberg@linux.intel.com>
Date2017-06-01 12:10 +0200
Message-ID<tNvdf-1cV-15@gated-at.bofh.it>
In reply to#1654962
On Thu, Jun 01, 2017 at 11:29:26AM +0200, Jean Delvare wrote:
> Hi all,
> 
> Sorry for the late reply.
> 
> On Wed, 17 May 2017 13:25:12 +0300, Mika Westerberg wrote:
> > Sometimes it is more convenient to be able to match a whole family of
> > products, like in case of bunch of Chromebooks based on Intel_Strago to
> > apply a driver quirk instead of quirking each machine one-by-one.
> > 
> > This adds support for DMI_PRODUCT_FAMILY identification string and also
> > exports it to the userspace through sysfs attribute just like the
> > existing ones.
> 
> dmidecode currently provides no direct access to this string. Do you
> think it should?

Yeah, why not. I always just run "dmidecode" without any arguments and
that field is printed nicely among others :)

> > Suggested-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > ---
> >  drivers/firmware/dmi-id.c       | 2 ++
> >  drivers/firmware/dmi_scan.c     | 1 +
> >  include/linux/mod_devicetable.h | 1 +
> >  3 files changed, 4 insertions(+)
> > 
> > diff --git a/drivers/firmware/dmi-id.c b/drivers/firmware/dmi-id.c
> > index 44c01390d035..dc269cb288c2 100644
> > --- a/drivers/firmware/dmi-id.c
> > +++ b/drivers/firmware/dmi-id.c
> > (...)
> > @@ -191,6 +192,7 @@ static void __init dmi_id_init_attr_table(void)
> >  	ADD_DMI_ATTR(product_version,   DMI_PRODUCT_VERSION);
> >  	ADD_DMI_ATTR(product_serial,    DMI_PRODUCT_SERIAL);
> >  	ADD_DMI_ATTR(product_uuid,      DMI_PRODUCT_UUID);
> > +	ADD_DMI_ATTR(product_family,      DMI_PRODUCT_FAMILY);
> 
> Alignment, please!

The patch is already applied and I suppose merged in v4.12-rc3+. Should
I send a fixup patch to fix this?

> 
> >  	ADD_DMI_ATTR(board_vendor,      DMI_BOARD_VENDOR);
> >  	ADD_DMI_ATTR(board_name,        DMI_BOARD_NAME);
> >  	ADD_DMI_ATTR(board_version,     DMI_BOARD_VERSION);
> > diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
> > index 54be60ead08f..93f7acdaac7a 100644
> > --- a/drivers/firmware/dmi_scan.c
> > +++ b/drivers/firmware/dmi_scan.c
> > @@ -430,6 +430,7 @@ static void __init dmi_decode(const struct dmi_header *dm, void *dummy)
> >  		dmi_save_ident(dm, DMI_PRODUCT_VERSION, 6);
> >  		dmi_save_ident(dm, DMI_PRODUCT_SERIAL, 7);
> >  		dmi_save_uuid(dm, DMI_PRODUCT_UUID, 8);
> > +		dmi_save_ident(dm, DMI_PRODUCT_FAMILY, 26);
> 
> This field only exists since SMBIOS 2.4. For older implementations, you
> are accessing a random location of the DMI table. Most likely you'll
> hit a character in one of the strings associated with the system
> information structure. In turn this character will be interpreted as a
> DMI string number. With some luck, number will be >= 32, so you'll get
> a non-existent string and dmi_string will return "". But you could hit
> a string terminator (0) and return the 1st string of the structure
> instead (most likely the system manufacturer.)
> 
> Note that the problem is not specific to this field, it is just more
> likely to break because all other fields are defined by SMBIOS 2.0, or
> for the product UUID, SMBIOS 2.1. The fact that all dmi_save_*
> functions blindly assume that the structure is long enough to contain
> all the fields they want to save is problematic. This should be fixed
> separately.

I see. Since you are more familiar with the DMI code, do you have time
to do that or should I try?

Thanks.

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

#1655008

FromJean Delvare <jdelvare@suse.de>
Date2017-06-01 13:00 +0200
Message-ID<tNvZE-1vK-11@gated-at.bofh.it>
In reply to#1654981
On Thu, 1 Jun 2017 13:09:40 +0300, Mika Westerberg wrote:
> On Thu, Jun 01, 2017 at 11:29:26AM +0200, Jean Delvare wrote:
> > Hi all,
> > 
> > Sorry for the late reply.
> > 
> > On Wed, 17 May 2017 13:25:12 +0300, Mika Westerberg wrote:  
> > > Sometimes it is more convenient to be able to match a whole family of
> > > products, like in case of bunch of Chromebooks based on Intel_Strago to
> > > apply a driver quirk instead of quirking each machine one-by-one.
> > > 
> > > This adds support for DMI_PRODUCT_FAMILY identification string and also
> > > exports it to the userspace through sysfs attribute just like the
> > > existing ones.  
> > 
> > dmidecode currently provides no direct access to this string. Do you
> > think it should?  
> 
> Yeah, why not. I always just run "dmidecode" without any arguments and
> that field is printed nicely among others :)

Correct. But I know many people out there use option -s for various
purposes.

> > > (...)
> > > diff --git a/drivers/firmware/dmi-id.c b/drivers/firmware/dmi-id.c
> > > index 44c01390d035..dc269cb288c2 100644
> > > --- a/drivers/firmware/dmi-id.c
> > > +++ b/drivers/firmware/dmi-id.c
> > > (...)
> > > @@ -191,6 +192,7 @@ static void __init dmi_id_init_attr_table(void)
> > >  	ADD_DMI_ATTR(product_version,   DMI_PRODUCT_VERSION);
> > >  	ADD_DMI_ATTR(product_serial,    DMI_PRODUCT_SERIAL);
> > >  	ADD_DMI_ATTR(product_uuid,      DMI_PRODUCT_UUID);
> > > +	ADD_DMI_ATTR(product_family,      DMI_PRODUCT_FAMILY);  
> > 
> > Alignment, please!  
> 
> The patch is already applied and I suppose merged in v4.12-rc3+. Should
> I send a fixup patch to fix this?

I will do it, no worry.

> > >  	ADD_DMI_ATTR(board_vendor,      DMI_BOARD_VENDOR);
> > >  	ADD_DMI_ATTR(board_name,        DMI_BOARD_NAME);
> > >  	ADD_DMI_ATTR(board_version,     DMI_BOARD_VERSION);
> > > diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
> > > index 54be60ead08f..93f7acdaac7a 100644
> > > --- a/drivers/firmware/dmi_scan.c
> > > +++ b/drivers/firmware/dmi_scan.c
> > > @@ -430,6 +430,7 @@ static void __init dmi_decode(const struct dmi_header *dm, void *dummy)
> > >  		dmi_save_ident(dm, DMI_PRODUCT_VERSION, 6);
> > >  		dmi_save_ident(dm, DMI_PRODUCT_SERIAL, 7);
> > >  		dmi_save_uuid(dm, DMI_PRODUCT_UUID, 8);
> > > +		dmi_save_ident(dm, DMI_PRODUCT_FAMILY, 26);  
> > 
> > This field only exists since SMBIOS 2.4. For older implementations, you
> > are accessing a random location of the DMI table. Most likely you'll
> > hit a character in one of the strings associated with the system
> > information structure. In turn this character will be interpreted as a
> > DMI string number. With some luck, number will be >= 32, so you'll get
> > a non-existent string and dmi_string will return "". But you could hit
> > a string terminator (0) and return the 1st string of the structure
> > instead (most likely the system manufacturer.)
> > 
> > Note that the problem is not specific to this field, it is just more
> > likely to break because all other fields are defined by SMBIOS 2.0, or
> > for the product UUID, SMBIOS 2.1. The fact that all dmi_save_*
> > functions blindly assume that the structure is long enough to contain
> > all the fields they want to save is problematic. This should be fixed
> > separately.  
> 
> I see. Since you are more familiar with the DMI code, do you have time
> to do that or should I try?

I'm already working on it.

-- 
Jean Delvare
SUSE L3 Support

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

#1655028

FromMika Westerberg <mika.westerberg@linux.intel.com>
Date2017-06-01 13:10 +0200
Message-ID<tNw9l-1Oq-47@gated-at.bofh.it>
In reply to#1655008
On Thu, Jun 01, 2017 at 12:54:51PM +0200, Jean Delvare wrote:
> On Thu, 1 Jun 2017 13:09:40 +0300, Mika Westerberg wrote:
> > On Thu, Jun 01, 2017 at 11:29:26AM +0200, Jean Delvare wrote:
> > > Hi all,
> > > 
> > > Sorry for the late reply.
> > > 
> > > On Wed, 17 May 2017 13:25:12 +0300, Mika Westerberg wrote:  
> > > > Sometimes it is more convenient to be able to match a whole family of
> > > > products, like in case of bunch of Chromebooks based on Intel_Strago to
> > > > apply a driver quirk instead of quirking each machine one-by-one.
> > > > 
> > > > This adds support for DMI_PRODUCT_FAMILY identification string and also
> > > > exports it to the userspace through sysfs attribute just like the
> > > > existing ones.  
> > > 
> > > dmidecode currently provides no direct access to this string. Do you
> > > think it should?  
> > 
> > Yeah, why not. I always just run "dmidecode" without any arguments and
> > that field is printed nicely among others :)
> 
> Correct. But I know many people out there use option -s for various
> purposes.

OK, I can send a patch adding it there.

> > > > (...)
> > > > diff --git a/drivers/firmware/dmi-id.c b/drivers/firmware/dmi-id.c
> > > > index 44c01390d035..dc269cb288c2 100644
> > > > --- a/drivers/firmware/dmi-id.c
> > > > +++ b/drivers/firmware/dmi-id.c
> > > > (...)
> > > > @@ -191,6 +192,7 @@ static void __init dmi_id_init_attr_table(void)
> > > >  	ADD_DMI_ATTR(product_version,   DMI_PRODUCT_VERSION);
> > > >  	ADD_DMI_ATTR(product_serial,    DMI_PRODUCT_SERIAL);
> > > >  	ADD_DMI_ATTR(product_uuid,      DMI_PRODUCT_UUID);
> > > > +	ADD_DMI_ATTR(product_family,      DMI_PRODUCT_FAMILY);  
> > > 
> > > Alignment, please!  
> > 
> > The patch is already applied and I suppose merged in v4.12-rc3+. Should
> > I send a fixup patch to fix this?
> 
> I will do it, no worry.
> 
> > > >  	ADD_DMI_ATTR(board_vendor,      DMI_BOARD_VENDOR);
> > > >  	ADD_DMI_ATTR(board_name,        DMI_BOARD_NAME);
> > > >  	ADD_DMI_ATTR(board_version,     DMI_BOARD_VERSION);
> > > > diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
> > > > index 54be60ead08f..93f7acdaac7a 100644
> > > > --- a/drivers/firmware/dmi_scan.c
> > > > +++ b/drivers/firmware/dmi_scan.c
> > > > @@ -430,6 +430,7 @@ static void __init dmi_decode(const struct dmi_header *dm, void *dummy)
> > > >  		dmi_save_ident(dm, DMI_PRODUCT_VERSION, 6);
> > > >  		dmi_save_ident(dm, DMI_PRODUCT_SERIAL, 7);
> > > >  		dmi_save_uuid(dm, DMI_PRODUCT_UUID, 8);
> > > > +		dmi_save_ident(dm, DMI_PRODUCT_FAMILY, 26);  
> > > 
> > > This field only exists since SMBIOS 2.4. For older implementations, you
> > > are accessing a random location of the DMI table. Most likely you'll
> > > hit a character in one of the strings associated with the system
> > > information structure. In turn this character will be interpreted as a
> > > DMI string number. With some luck, number will be >= 32, so you'll get
> > > a non-existent string and dmi_string will return "". But you could hit
> > > a string terminator (0) and return the 1st string of the structure
> > > instead (most likely the system manufacturer.)
> > > 
> > > Note that the problem is not specific to this field, it is just more
> > > likely to break because all other fields are defined by SMBIOS 2.0, or
> > > for the product UUID, SMBIOS 2.1. The fact that all dmi_save_*
> > > functions blindly assume that the structure is long enough to contain
> > > all the fields they want to save is problematic. This should be fixed
> > > separately.  
> > 
> > I see. Since you are more familiar with the DMI code, do you have time
> > to do that or should I try?
> 
> I'm already working on it.

Thanks!

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web