[PATCH] DMI: allow omitting ident strings in DMI tables

[PATCH] DMI: allow omitting ident strings in DMI tables

[Dmitry Torokhov]

The purpose of dmi->ident is twofold - it may be used by DMI callback
functions when composing log messages; it is also used to determine
end of DMI table in dmi_check_system() and dmi_first_match(). However,
in case when callbacks are not interested in using ident at all it just
wastes memory. Let's consider entries with empty ident but initialized
first match slot as a valid entry and not as end-of-table marker.

Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---

CCed a few random people since they touched dmi code in the last few
months...

 drivers/firmware/dmi_scan.c |   13 +++++++++++--
 1 files changed, 11 insertions(+), 2 deletions(-)


diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
index 938100f..9116aa7 100644
--- a/drivers/firmware/dmi_scan.c
+++ b/drivers/firmware/dmi_scan.c
@@ -440,6 +440,15 @@ static bool dmi_matches(const struct dmi_system_id *dmi)
 }
 
 /**
+ *	dmi_is_end_of_table - check for end-of-table marker
+ *	@dmi: pointer to the dmi_system_id structure to check
+ */
+static bool dmi_is_end_of_table(const struct dmi_system_id *dmi)
+{
+	return dmi->ident == NULL && dmi->matches[0].slot == DMI_NONE;
+}
+
+/**
  *	dmi_check_system - check system DMI data
  *	@list: array of dmi_system_id structures to match against
  *		All non-null elements of the list must match
@@ -457,7 +466,7 @@ int dmi_check_system(const struct dmi_system_id *list)
 	int count = 0;
 	const struct dmi_system_id *d;
 
-	for (d = list; d->ident; d++)
+	for (d = list; !dmi_is_end_of_table(d); d++)
 		if (dmi_matches(d)) {
 			count++;
 			if (d->callback && d->callback(d))
@@ -484,7 +493,7 @@ const struct dmi_system_id *dmi_first_match(const struct dmi_system_id *list)
 {
 	const struct dmi_system_id *d;
 
-	for (d = list; d->ident; d++)
+	for (d = list; !dmi_is_end_of_table(d); d++)
 		if (dmi_matches(d))
 			return d;
 
-- 
Dmitry

Re: [PATCH] DMI: allow omitting ident strings in DMI tables

[Jean Delvare]

Hi Dmitry,

On Wed, 2 Dec 2009 19:12:40 -0800, Dmitry Torokhov wrote:
> The purpose of dmi->ident is twofold - it may be used by DMI callback
> functions when composing log messages; it is also used to determine
> end of DMI table in dmi_check_system() and dmi_first_match(). However,
> in case when callbacks are not interested in using ident at all it just
> wastes memory. Let's consider entries with empty ident but initialized
> first match slot as a valid entry and not as end-of-table marker.

You are free to use an empty string ("") as the ident. This will use
1 byte of memory, I'm sure you can afford it. struct dmi_system_id is
332 bytes large on 32-bit systems (344 on 64-bit systems), and we use
such an empty structure as the list terminator. So I really doubt we
care about the extra few bytes used by the ident strings.

> 
> Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
> ---
> 
> CCed a few random people since they touched dmi code in the last few
> months...
> 
>  drivers/firmware/dmi_scan.c |   13 +++++++++++--
>  1 files changed, 11 insertions(+), 2 deletions(-)
> 
> 
> diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
> index 938100f..9116aa7 100644
> --- a/drivers/firmware/dmi_scan.c
> +++ b/drivers/firmware/dmi_scan.c
> @@ -440,6 +440,15 @@ static bool dmi_matches(const struct dmi_system_id *dmi)
>  }
>  
>  /**
> + *	dmi_is_end_of_table - check for end-of-table marker
> + *	@dmi: pointer to the dmi_system_id structure to check
> + */
> +static bool dmi_is_end_of_table(const struct dmi_system_id *dmi)
> +{
> +	return dmi->ident == NULL && dmi->matches[0].slot == DMI_NONE;

If you really want to allow for dmi->ident == NULL, then I guess you can
_only_ check for dmi->matches[0].slot == DMI_NONE. I can't think of any
legitimate use of DMI_NONE for a used slot. The only thing you have to
do then is to ensure that DMI_NONE = 0 in <linux/mod_devicetable.h>
(I'm not sure if any C standard guarantees that enums starts at 0.)

There's a possible optimization in dmi_matches(), BTW: DMI_NONE should
break, not continue.

-- 
Jean Delvare

Re: [PATCH] DMI: allow omitting ident strings in DMI tables

[Dmitry Torokhov]

Hi Jean,

On Thu, Dec 03, 2009 at 09:30:09AM +0100, Jean Delvare wrote:
> Hi Dmitry,
> 
> On Wed, 2 Dec 2009 19:12:40 -0800, Dmitry Torokhov wrote:
> > The purpose of dmi->ident is twofold - it may be used by DMI callback
> > functions when composing log messages; it is also used to determine
> > end of DMI table in dmi_check_system() and dmi_first_match(). However,
> > in case when callbacks are not interested in using ident at all it just
> > wastes memory. Let's consider entries with empty ident but initialized
> > first match slot as a valid entry and not as end-of-table marker.
> 
> You are free to use an empty string ("") as the ident. This will use
> 1 byte of memory, I'm sure you can afford it. struct dmi_system_id is
> 332 bytes large on 32-bit systems (344 on 64-bit systems), and we use
> such an empty structure as the list terminator. So I really doubt we
> care about the extra few bytes used by the ident strings.
> 

Almost all instances of DMI tables are maked __initdata. Because we use
arrays instead of pointers for match slots they all get discarded once
kernel is up and running so the cost of the terminator is not that bit.

The indent is different - since it is a pointer to string the string
ends up in constant data section and (as far as I understand) is kept
even though we discarded the pointer to it. We could work around it by
decaring 'char blah_ident[] = "This is blah";' and setting up pointer
but this is ugly. Setting ident to "" ugly as well (IMHO and admittedly
not as ugly as screwing with an array) but the most clean and concise
way (especially for large tables) is omit ident altogether.

> > 
> > Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
> > ---
> > 
> > CCed a few random people since they touched dmi code in the last few
> > months...
> > 
> >  drivers/firmware/dmi_scan.c |   13 +++++++++++--
> >  1 files changed, 11 insertions(+), 2 deletions(-)
> > 
> > 
> > diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
> > index 938100f..9116aa7 100644
> > --- a/drivers/firmware/dmi_scan.c
> > +++ b/drivers/firmware/dmi_scan.c
> > @@ -440,6 +440,15 @@ static bool dmi_matches(const struct dmi_system_id *dmi)
> >  }
> >  
> >  /**
> > + *	dmi_is_end_of_table - check for end-of-table marker
> > + *	@dmi: pointer to the dmi_system_id structure to check
> > + */
> > +static bool dmi_is_end_of_table(const struct dmi_system_id *dmi)
> > +{
> > +	return dmi->ident == NULL && dmi->matches[0].slot == DMI_NONE;
> 
> If you really want to allow for dmi->ident == NULL, then I guess you can
> _only_ check for dmi->matches[0].slot == DMI_NONE. I can't think of any
> legitimate use of DMI_NONE for a used slot.

Current behavior is that entry with ident and empty match table matches
everything. If we only check on the first slot then it will not match. I
wanted to preserve the current behavior.

> The only thing you have to
> do then is to ensure that DMI_NONE = 0 in <linux/mod_devicetable.h>
> (I'm not sure if any C standard guarantees that enums starts at 0.)

I believe it does. Otherwise we'd be comparing with 0 strings all the
time.

> 
> There's a possible optimization in dmi_matches(), BTW: DMI_NONE should
> break, not continue.

I agree.

-- 
Dmitry

Re: [PATCH] DMI: allow omitting ident strings in DMI tables

[Jean Delvare]

On Thu, 3 Dec 2009 00:56:30 -0800, Dmitry Torokhov wrote:
> On Thu, Dec 03, 2009 at 09:30:09AM +0100, Jean Delvare wrote:
> > On Wed, 2 Dec 2009 19:12:40 -0800, Dmitry Torokhov wrote:
> > > diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
> > > index 938100f..9116aa7 100644
> > > --- a/drivers/firmware/dmi_scan.c
> > > +++ b/drivers/firmware/dmi_scan.c
> > > @@ -440,6 +440,15 @@ static bool dmi_matches(const struct dmi_system_id *dmi)
> > >  }
> > >  
> > >  /**
> > > + *	dmi_is_end_of_table - check for end-of-table marker
> > > + *	@dmi: pointer to the dmi_system_id structure to check
> > > + */
> > > +static bool dmi_is_end_of_table(const struct dmi_system_id *dmi)
> > > +{
> > > +	return dmi->ident == NULL && dmi->matches[0].slot == DMI_NONE;
> > 
> > If you really want to allow for dmi->ident == NULL, then I guess you can
> > _only_ check for dmi->matches[0].slot == DMI_NONE. I can't think of any
> > legitimate use of DMI_NONE for a used slot.
> 
> Current behavior is that entry with ident and empty match table matches
> everything. If we only check on the first slot then it will not match. I
> wanted to preserve the current behavior.

Is there a use case for this behavior? If not then I don't see the
point of preserving it.

-- 
Jean Delvare

Re: [PATCH] DMI: allow omitting ident strings in DMI tables

[Dmitry Torokhov]

On Thu, Dec 03, 2009 at 10:25:42AM +0100, Jean Delvare wrote:
> On Thu, 3 Dec 2009 00:56:30 -0800, Dmitry Torokhov wrote:
> > On Thu, Dec 03, 2009 at 09:30:09AM +0100, Jean Delvare wrote:
> > > On Wed, 2 Dec 2009 19:12:40 -0800, Dmitry Torokhov wrote:
> > > > diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
> > > > index 938100f..9116aa7 100644
> > > > --- a/drivers/firmware/dmi_scan.c
> > > > +++ b/drivers/firmware/dmi_scan.c
> > > > @@ -440,6 +440,15 @@ static bool dmi_matches(const struct dmi_system_id *dmi)
> > > >  }
> > > >  
> > > >  /**
> > > > + *	dmi_is_end_of_table - check for end-of-table marker
> > > > + *	@dmi: pointer to the dmi_system_id structure to check
> > > > + */
> > > > +static bool dmi_is_end_of_table(const struct dmi_system_id *dmi)
> > > > +{
> > > > +	return dmi->ident == NULL && dmi->matches[0].slot == DMI_NONE;
> > > 
> > > If you really want to allow for dmi->ident == NULL, then I guess you can
> > > _only_ check for dmi->matches[0].slot == DMI_NONE. I can't think of any
> > > legitimate use of DMI_NONE for a used slot.
> > 
> > Current behavior is that entry with ident and empty match table matches
> > everything. If we only check on the first slot then it will not match. I
> > wanted to preserve the current behavior.
> 
> Is there a use case for this behavior? If not then I don't see the
> point of preserving it.
> 

I looked through current uses of dmi_check_system and could not find
any cases where anyone would rely on this behavior. Thus the updated
patch below.

Thanks.

-- 
Dmitry


DMI: allow omitting ident strings in DMI tables

From: Dmitry Torokhov <dmitry.torokhov@gmail.com>

The purpose of dmi->ident is twofold - it may be used by DMI callback
functions when composing log messages; it is also used to determine
end of DMI table in dmi_check_system() and dmi_first_match(). However,
in case when callbacks are not interested in using ident at all it just
wastes memory. Let's make enties with empty first match slot serve as
end-of-table markers instead.

Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---

 drivers/firmware/dmi_scan.c |   15 ++++++++++++---
 1 files changed, 12 insertions(+), 3 deletions(-)


diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
index 938100f..3a2ccb0 100644
--- a/drivers/firmware/dmi_scan.c
+++ b/drivers/firmware/dmi_scan.c
@@ -429,7 +429,7 @@ static bool dmi_matches(const struct dmi_system_id *dmi)
 	for (i = 0; i < ARRAY_SIZE(dmi->matches); i++) {
 		int s = dmi->matches[i].slot;
 		if (s == DMI_NONE)
-			continue;
+			break;
 		if (dmi_ident[s]
 		    && strstr(dmi_ident[s], dmi->matches[i].substr))
 			continue;
@@ -440,6 +440,15 @@ static bool dmi_matches(const struct dmi_system_id *dmi)
 }
 
 /**
+ *	dmi_is_end_of_table - check for end-of-table marker
+ *	@dmi: pointer to the dmi_system_id structure to check
+ */
+static bool dmi_is_end_of_table(const struct dmi_system_id *dmi)
+{
+	return dmi->matches[0].slot == DMI_NONE;
+}
+
+/**
  *	dmi_check_system - check system DMI data
  *	@list: array of dmi_system_id structures to match against
  *		All non-null elements of the list must match
@@ -457,7 +466,7 @@ int dmi_check_system(const struct dmi_system_id *list)
 	int count = 0;
 	const struct dmi_system_id *d;
 
-	for (d = list; d->ident; d++)
+	for (d = list; !dmi_is_end_of_table(d); d++)
 		if (dmi_matches(d)) {
 			count++;
 			if (d->callback && d->callback(d))
@@ -484,7 +493,7 @@ const struct dmi_system_id *dmi_first_match(const struct dmi_system_id *list)
 {
 	const struct dmi_system_id *d;
 
-	for (d = list; d->ident; d++)
+	for (d = list; !dmi_is_end_of_table(d); d++)
 		if (dmi_matches(d))
 			return d;
 

Re: [PATCH] DMI: allow omitting ident strings in DMI tables

[Jean Delvare]

> DMI: allow omitting ident strings in DMI tables
> 
> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> 
> The purpose of dmi->ident is twofold - it may be used by DMI callback
> functions when composing log messages; it is also used to determine
> end of DMI table in dmi_check_system() and dmi_first_match(). However,
> in case when callbacks are not interested in using ident at all it just
> wastes memory. Let's make enties with empty first match slot serve as
> end-of-table markers instead.
> 
> Signed-off-by: Dmitry Torokhov <dtor@mail.ru>

Looks good to me.

Acked-by: Jean Delvare <khali@linux-fr.org>

> ---
> 
>  drivers/firmware/dmi_scan.c |   15 ++++++++++++---
>  1 files changed, 12 insertions(+), 3 deletions(-)
> 
> 
> diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
> index 938100f..3a2ccb0 100644
> --- a/drivers/firmware/dmi_scan.c
> +++ b/drivers/firmware/dmi_scan.c
> @@ -429,7 +429,7 @@ static bool dmi_matches(const struct dmi_system_id *dmi)
>  	for (i = 0; i < ARRAY_SIZE(dmi->matches); i++) {
>  		int s = dmi->matches[i].slot;
>  		if (s == DMI_NONE)
> -			continue;
> +			break;
>  		if (dmi_ident[s]
>  		    && strstr(dmi_ident[s], dmi->matches[i].substr))
>  			continue;
> @@ -440,6 +440,15 @@ static bool dmi_matches(const struct dmi_system_id *dmi)
>  }
>  
>  /**
> + *	dmi_is_end_of_table - check for end-of-table marker
> + *	@dmi: pointer to the dmi_system_id structure to check
> + */
> +static bool dmi_is_end_of_table(const struct dmi_system_id *dmi)
> +{
> +	return dmi->matches[0].slot == DMI_NONE;
> +}
> +
> +/**
>   *	dmi_check_system - check system DMI data
>   *	@list: array of dmi_system_id structures to match against
>   *		All non-null elements of the list must match
> @@ -457,7 +466,7 @@ int dmi_check_system(const struct dmi_system_id *list)
>  	int count = 0;
>  	const struct dmi_system_id *d;
>  
> -	for (d = list; d->ident; d++)
> +	for (d = list; !dmi_is_end_of_table(d); d++)
>  		if (dmi_matches(d)) {
>  			count++;
>  			if (d->callback && d->callback(d))
> @@ -484,7 +493,7 @@ const struct dmi_system_id *dmi_first_match(const struct dmi_system_id *list)
>  {
>  	const struct dmi_system_id *d;
>  
> -	for (d = list; d->ident; d++)
> +	for (d = list; !dmi_is_end_of_table(d); d++)
>  		if (dmi_matches(d))
>  			return d;
>  


-- 
Jean Delvare