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@xxxxxxx>
> > ---
> > 
> > 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/