Sometimes we want to make a case-insensitive comparison with strings, like
checking compatible devices in fwnode properties, so this commit abstracts
match_string to __match_string with a compare function. The original
match_string will call __match_string with strcmp, and the new
match_string_nocase will call it with strcasecmp.
Signed-off-by: Soha Jin <[email protected]>
---
include/linux/string.h | 31 ++++++++++++++++++++++++++++++-
lib/string_helpers.c | 10 ++++++----
2 files changed, 36 insertions(+), 5 deletions(-)
diff --git a/include/linux/string.h b/include/linux/string.h
index cf7607b32102..fcfa67f598f5 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -183,9 +183,38 @@ extern char **argv_split(gfp_t gfp, const char *str, int *argcp);
extern void argv_free(char **argv);
extern bool sysfs_streq(const char *s1, const char *s2);
-int match_string(const char * const *array, size_t n, const char *string);
+int __match_string(const char * const *array, size_t n, const char *string,
+ int (*cmp)(const char *, const char *));
int __sysfs_match_string(const char * const *array, size_t n, const char *s);
+/**
+ * match_string - matches given string in an array
+ * @_a: array of strings
+ * @_n: number of strings in the array
+ * @_s: string to match with
+ *
+ * Helper for __match_string(). Look for a string in an array of strings up to
+ * the n-th element in the array with a case-sensitive compare.
+ *
+ * Return:
+ * index of a @string in the @array if matches, or %-EINVAL otherwise.
+ */
+#define match_string(_a, _n, _s) __match_string(_a, _n, _s, strcmp)
+
+/**
+ * match_string_nocase - matches given string in an array
+ * @_a: array of strings
+ * @_n: number of strings in the array
+ * @_s: string to match with
+ *
+ * Helper for __match_string(). Look for a string in an array of strings up to
+ * the n-th element in the array with a case-insensitive compare.
+ *
+ * Return:
+ * index of a @string in the @array if matches, or %-EINVAL otherwise.
+ */
+#define match_string_nocase(_a, _n, _s) __match_string(_a, _n, _s, strcasecmp)
+
/**
* sysfs_match_string - matches given string in an array
* @_a: array of strings
diff --git a/lib/string_helpers.c b/lib/string_helpers.c
index 230020a2e076..52949adfdfe4 100644
--- a/lib/string_helpers.c
+++ b/lib/string_helpers.c
@@ -910,10 +910,11 @@ bool sysfs_streq(const char *s1, const char *s2)
EXPORT_SYMBOL(sysfs_streq);
/**
- * match_string - matches given string in an array
+ * __match_string - matches given string in an array
* @array: array of strings
* @n: number of strings in the array or -1 for NULL terminated arrays
* @string: string to match with
+ * @cmp: compare function
*
* This routine will look for a string in an array of strings up to the
* n-th element in the array or until the first NULL element.
@@ -926,7 +927,8 @@ EXPORT_SYMBOL(sysfs_streq);
* Return:
* index of a @string in the @array if matches, or %-EINVAL otherwise.
*/
-int match_string(const char * const *array, size_t n, const char *string)
+int __match_string(const char * const *array, size_t n, const char *string,
+ int (*cmp)(const char *, const char *))
{
int index;
const char *item;
@@ -935,13 +937,13 @@ int match_string(const char * const *array, size_t n, const char *string)
item = array[index];
if (!item)
break;
- if (!strcmp(item, string))
+ if (!cmp(item, string))
return index;
}
return -EINVAL;
}
-EXPORT_SYMBOL(match_string);
+EXPORT_SYMBOL(__match_string);
/**
* __sysfs_match_string - matches given string in an array
--
2.30.2
On Mon, Oct 10, 2022 at 12:21:53AM +0800, Soha Jin wrote:
> Sometimes we want to make a case-insensitive comparison with strings, like
> checking compatible devices in fwnode properties, so this commit abstracts
> match_string to __match_string with a compare function. The original
> match_string will call __match_string with strcmp, and the new
> match_string_nocase will call it with strcasecmp.
Wait, no, fwnode properties are case sensitive, why are you allowing
that to be changed? That sounds like broken firmware to me, right?
thanks,
greg k-h
Hi Greg,
> On Mon, Oct 10, 2022 at 12:21:53AM +0800, Soha Jin wrote:
> > Sometimes we want to make a case-insensitive comparison with strings,
> > like checking compatible devices in fwnode properties, so this commit
> > abstracts match_string to __match_string with a compare function. The
> > original match_string will call __match_string with strcmp, and the
> > new match_string_nocase will call it with strcasecmp.
>
> Wait, no, fwnode properties are case sensitive, why are you allowing that to
> be changed? That sounds like broken firmware to me, right?
>
I am writing regarding the compatibility. In `of_device_is_compatible`, it
uses `of_compat_cmp` which calls `strcasecmp` to match compatible property.
As the `fwnode_is_compatible` should be the replacement of the OF way, I
think we should make the fwnode way and the OF way the same, i.e. either
both case-insensitive or case-sensitive, to keep the consistency. I am
afraid that make `of_compat_cmp` case-sensitive may break a great many of
devices, that is why I am doing this.
Regards,
Soha
On Mon, Oct 10, 2022 at 11:02:39AM +0800, Soha Jin wrote:
> Hi Greg,
> > On Mon, Oct 10, 2022 at 12:21:53AM +0800, Soha Jin wrote:
> > > Sometimes we want to make a case-insensitive comparison with strings,
> > > like checking compatible devices in fwnode properties, so this commit
> > > abstracts match_string to __match_string with a compare function. The
> > > original match_string will call __match_string with strcmp, and the
> > > new match_string_nocase will call it with strcasecmp.
> >
> > Wait, no, fwnode properties are case sensitive, why are you allowing that to
> > be changed? That sounds like broken firmware to me, right?
> >
>
> I am writing regarding the compatibility. In `of_device_is_compatible`, it
> uses `of_compat_cmp` which calls `strcasecmp` to match compatible property.
>
> As the `fwnode_is_compatible` should be the replacement of the OF way, I
> think we should make the fwnode way and the OF way the same, i.e. either
> both case-insensitive or case-sensitive, to keep the consistency. I am
> afraid that make `of_compat_cmp` case-sensitive may break a great many of
> devices, that is why I am doing this.
Ok, but if you change this with the series, what will break? What needs
this new case-insensitive comparison that does not work today?
thanks,
greg k-h
Hi Greg and Andy,
> From: 'Greg Kroah-Hartman' <[email protected]>
> Sent: Monday, October 10, 2022 2:24 PM
>
> > I am writing regarding the compatibility. In
> > `of_device_is_compatible`, it uses `of_compat_cmp` which calls
> `strcasecmp` to match compatible property.
> >
> > As the `fwnode_is_compatible` should be the replacement of the OF way,
> > I think we should make the fwnode way and the OF way the same, i.e.
> > either both case-insensitive or case-sensitive, to keep the
> > consistency. I am afraid that make `of_compat_cmp` case-sensitive may
> > break a great many of devices, that is why I am doing this.
>
> Ok, but if you change this with the series, what will break?
My changeset will not break something, and make comparison case-sensitive
does. Some old device firmwares that did not care about letter case might
not function correctly in a newer kernel, because the current kernel checks
compatibility case-insensitively and the former developer did not notice
this with the just working kernel.
> What needs this
> new case-insensitive comparison that does not work today?
> From: Andy Shevchenko <[email protected]>
> Sent: Monday, October 10, 2022 2:24 PM
>
> So, why do we have such in the OF code and do we really need it in the
> modern world?
Frankly speaking, I think case-insensitive comparison is not needed TODAY,
and before I compose this change, I can see codes in kernel like this:
of_device_is_compatible(np, "U4-pcie") ||
of_device_is_compatible(np, "u4-pcie")
which means kernel codes is de facto case-sensitive, although this function
calls `strcasecmp`.
I do not know kernel maintainers' mind when I am composing this change, I
just chose the way which will not break anything. Anyway, I am also glad to
edit the patch to make it case-sensitive once maintainers have made the
final decision.
Regards,
Soha