* [PATCH] DMI: allow omitting ident strings in DMI tables @ 2009-12-03 3:12 Dmitry Torokhov 2009-12-03 8:30 ` Jean Delvare 0 siblings, 1 reply; 6+ messages in thread From: Dmitry Torokhov @ 2009-12-03 3:12 UTC (permalink / raw) To: LKML Cc: Tejun Heo, Jeff Garzik, Jean Delvare, Rafael J. Wysocki, H. Peter Anvin 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] DMI: allow omitting ident strings in DMI tables 2009-12-03 3:12 [PATCH] DMI: allow omitting ident strings in DMI tables Dmitry Torokhov @ 2009-12-03 8:30 ` Jean Delvare 2009-12-03 8:56 ` Dmitry Torokhov 0 siblings, 1 reply; 6+ messages in thread From: Jean Delvare @ 2009-12-03 8:30 UTC (permalink / raw) To: Dmitry Torokhov Cc: LKML, Tejun Heo, Jeff Garzik, Rafael J. Wysocki, H. Peter Anvin 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] DMI: allow omitting ident strings in DMI tables 2009-12-03 8:30 ` Jean Delvare @ 2009-12-03 8:56 ` Dmitry Torokhov 2009-12-03 9:25 ` Jean Delvare 0 siblings, 1 reply; 6+ messages in thread From: Dmitry Torokhov @ 2009-12-03 8:56 UTC (permalink / raw) To: Jean Delvare Cc: LKML, Tejun Heo, Jeff Garzik, Rafael J. Wysocki, H. Peter Anvin 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] DMI: allow omitting ident strings in DMI tables 2009-12-03 8:56 ` Dmitry Torokhov @ 2009-12-03 9:25 ` Jean Delvare 2009-12-04 7:34 ` Dmitry Torokhov 0 siblings, 1 reply; 6+ messages in thread From: Jean Delvare @ 2009-12-03 9:25 UTC (permalink / raw) To: Dmitry Torokhov Cc: LKML, Tejun Heo, Jeff Garzik, Rafael J. Wysocki, H. Peter Anvin 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] DMI: allow omitting ident strings in DMI tables 2009-12-03 9:25 ` Jean Delvare @ 2009-12-04 7:34 ` Dmitry Torokhov 2009-12-04 16:19 ` Jean Delvare 0 siblings, 1 reply; 6+ messages in thread From: Dmitry Torokhov @ 2009-12-04 7:34 UTC (permalink / raw) To: Jean Delvare Cc: LKML, Tejun Heo, Jeff Garzik, Rafael J. Wysocki, H. Peter Anvin 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; ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] DMI: allow omitting ident strings in DMI tables 2009-12-04 7:34 ` Dmitry Torokhov @ 2009-12-04 16:19 ` Jean Delvare 0 siblings, 0 replies; 6+ messages in thread From: Jean Delvare @ 2009-12-04 16:19 UTC (permalink / raw) To: Dmitry Torokhov Cc: LKML, Tejun Heo, Jeff Garzik, Rafael J. Wysocki, H. Peter Anvin > 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2009-12-04 16:19 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2009-12-03 3:12 [PATCH] DMI: allow omitting ident strings in DMI tables Dmitry Torokhov 2009-12-03 8:30 ` Jean Delvare 2009-12-03 8:56 ` Dmitry Torokhov 2009-12-03 9:25 ` Jean Delvare 2009-12-04 7:34 ` Dmitry Torokhov 2009-12-04 16:19 ` Jean Delvare
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.