Since c32569e358ad ("regulator: Use of_node_name_eq for node name
comparisons") Vivien reported the mc13892-regulator complaining about
not being able to find regulators.
This is because prior to that commit we used of_node_cmp() to compare
the regulator array passed from mc13892_regulators down to
mc13xxx_parse_regulators_dt() and they are all defined in uppercase
letters by the MC13892_*_DEFINE* macros, whereas they are defined as
lowercase in the DTS.
Fix this by use strncasecmp() since that makes sure the comparison is
case insensitive like what of_node_cmp() did.
Reported-by: Vivien Didelot <[email protected]>
Fixes: c32569e358ad ("regulator: Use of_node_name_eq for node name comparisons")
Signed-off-by: Florian Fainelli <[email protected]>
---
drivers/of/base.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/of/base.c b/drivers/of/base.c
index 5226e898476e..ff47c86277cb 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -66,7 +66,8 @@ bool of_node_name_eq(const struct device_node *np, const char *name)
node_name = kbasename(np->full_name);
len = strchrnul(node_name, '@') - node_name;
- return (strlen(name) == len) && (strncmp(node_name, name, len) == 0);
+ return (strlen(name) == len) &&
+ (strncasecmp(node_name, name, len) == 0);
}
EXPORT_SYMBOL(of_node_name_eq);
--
2.17.1
On Thu, 2019-01-24 at 12:08 -0800, Florian Fainelli wrote:
> Since c32569e358ad ("regulator: Use of_node_name_eq for node name
> comparisons") Vivien reported the mc13892-regulator complaining about
> not being able to find regulators.
>
> This is because prior to that commit we used of_node_cmp() to compare
> the regulator array passed from mc13892_regulators down to
> mc13xxx_parse_regulators_dt() and they are all defined in uppercase
> letters by the MC13892_*_DEFINE* macros, whereas they are defined as
> lowercase in the DTS.
>
> Fix this by use strncasecmp() since that makes sure the comparison is
> case insensitive like what of_node_cmp() did.
>
> Reported-by: Vivien Didelot <[email protected]>
> Fixes: c32569e358ad ("regulator: Use of_node_name_eq for node name comparisons")
> Signed-off-by: Florian Fainelli <[email protected]>
Then likely of_node_name_prefix should also use
case insensitive matching.
---
diff --git a/drivers/of/base.c b/drivers/of/base.c
index 5226e898476e..87a0bd7ef45e 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -75,7 +75,8 @@ bool of_node_name_prefix(const struct device_node *np, const char *prefix)
if (!np)
return false;
- return strncmp(kbasename(np->full_name), prefix, strlen(prefix)) == 0;
+ return strncasecmp(kbasename(np->full_name),
+ prefix, strlen(prefix)) == 0;
}
EXPORT_SYMBOL(of_node_name_prefix);
Hi,
On Thu, 24 Jan 2019 12:08:25 -0800, Florian Fainelli <[email protected]> wrote:
> Since c32569e358ad ("regulator: Use of_node_name_eq for node name
> comparisons") Vivien reported the mc13892-regulator complaining about
> not being able to find regulators.
>
> This is because prior to that commit we used of_node_cmp() to compare
> the regulator array passed from mc13892_regulators down to
> mc13xxx_parse_regulators_dt() and they are all defined in uppercase
> letters by the MC13892_*_DEFINE* macros, whereas they are defined as
> lowercase in the DTS.
>
> Fix this by use strncasecmp() since that makes sure the comparison is
> case insensitive like what of_node_cmp() did.
>
> Reported-by: Vivien Didelot <[email protected]>
> Fixes: c32569e358ad ("regulator: Use of_node_name_eq for node name comparisons")
> Signed-off-by: Florian Fainelli <[email protected]>
This fixes the boot on i.MX51 ZII RDU1, which was printing this:
[ 2.895302] imx-ipuv3 40000000.ipu: IPUv3EX probed
[ 2.903869] spi_imx 70010000.spi: dma setup error -19, use pio
[ 2.911943] mc13xxx spi0.0: mc13892: rev: 2.4, fin: 2, fab: 0, icid: 7/2
[ 2.921463] mc13892-regulator mc13892-regulator: Unknown regulator: sw1
[ 2.928207] mc13892-regulator mc13892-regulator: Unknown regulator: sw2
[ 2.934896] mc13892-regulator mc13892-regulator: Unknown regulator: sw3
[ 2.941575] mc13892-regulator mc13892-regulator: Unknown regulator: sw4
[ 2.948263] mc13892-regulator mc13892-regulator: Unknown regulator: vpll
[ 2.955050] mc13892-regulator mc13892-regulator: Unknown regulator: vdig
[ 2.961820] mc13892-regulator mc13892-regulator: Unknown regulator: vsd
[ 2.968464] mc13892-regulator mc13892-regulator: Unknown regulator: vusb
[ 2.975251] mc13892-regulator mc13892-regulator: Unknown regulator: vusb2
[ 2.982110] mc13892-regulator mc13892-regulator: Unknown regulator: vvideo
[ 2.989039] mc13892-regulator mc13892-regulator: Unknown regulator: vaudio
[ 2.995983] mc13892-regulator mc13892-regulator: Unknown regulator: vcam
[ 3.002754] mc13892-regulator mc13892-regulator: Unknown regulator: vgen1
[ 3.009597] mc13892-regulator mc13892-regulator: Unknown regulator: vgen2
[ 3.016458] mc13892-regulator mc13892-regulator: Unknown regulator: vgen3
before looping forever on the defered probe of the Marvell switch.
Tested-by: Vivien Didelot <[email protected]>
Thanks,
Vivien
On 1/24/19 12:08 PM, Florian Fainelli wrote:
> Since c32569e358ad ("regulator: Use of_node_name_eq for node name
> comparisons") Vivien reported the mc13892-regulator complaining about
> not being able to find regulators.
>
> This is because prior to that commit we used of_node_cmp() to compare
> the regulator array passed from mc13892_regulators down to
> mc13xxx_parse_regulators_dt() and they are all defined in uppercase
> letters by the MC13892_*_DEFINE* macros, whereas they are defined as
> lowercase in the DTS.
>
> Fix this by use strncasecmp() since that makes sure the comparison is
> case insensitive like what of_node_cmp() did.
>
> Reported-by: Vivien Didelot <[email protected]>
> Fixes: c32569e358ad ("regulator: Use of_node_name_eq for node name comparisons")
> Signed-off-by: Florian Fainelli <[email protected]>
> ---
> drivers/of/base.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 5226e898476e..ff47c86277cb 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -66,7 +66,8 @@ bool of_node_name_eq(const struct device_node *np, const char *name)
> node_name = kbasename(np->full_name);
> len = strchrnul(node_name, '@') - node_name;
>
> - return (strlen(name) == len) && (strncmp(node_name, name, len) == 0);
> + return (strlen(name) == len) &&
> + (strncasecmp(node_name, name, len) == 0);
> }
> EXPORT_SYMBOL(of_node_name_eq);
>
>
Node names are case sensitive. Please fix mc13xxx_parse_regulators_dt() to
properly handle case instead of changing of_node_name_eq().
-Frank
On 1/24/19 3:45 PM, Frank Rowand wrote:
> On 1/24/19 12:08 PM, Florian Fainelli wrote:
>> Since c32569e358ad ("regulator: Use of_node_name_eq for node name
>> comparisons") Vivien reported the mc13892-regulator complaining about
>> not being able to find regulators.
>>
>> This is because prior to that commit we used of_node_cmp() to compare
>> the regulator array passed from mc13892_regulators down to
>> mc13xxx_parse_regulators_dt() and they are all defined in uppercase
>> letters by the MC13892_*_DEFINE* macros, whereas they are defined as
>> lowercase in the DTS.
>>
>> Fix this by use strncasecmp() since that makes sure the comparison is
>> case insensitive like what of_node_cmp() did.
>>
>> Reported-by: Vivien Didelot <[email protected]>
>> Fixes: c32569e358ad ("regulator: Use of_node_name_eq for node name comparisons")
>> Signed-off-by: Florian Fainelli <[email protected]>
>> ---
>> drivers/of/base.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/of/base.c b/drivers/of/base.c
>> index 5226e898476e..ff47c86277cb 100644
>> --- a/drivers/of/base.c
>> +++ b/drivers/of/base.c
>> @@ -66,7 +66,8 @@ bool of_node_name_eq(const struct device_node *np, const char *name)
>> node_name = kbasename(np->full_name);
>> len = strchrnul(node_name, '@') - node_name;
>>
>> - return (strlen(name) == len) && (strncmp(node_name, name, len) == 0);
>> + return (strlen(name) == len) &&
>> + (strncasecmp(node_name, name, len) == 0);
>> }
>> EXPORT_SYMBOL(of_node_name_eq);
>>
>>
>
> Node names are case sensitive. Please fix mc13xxx_parse_regulators_dt() to
> properly handle case instead of changing of_node_name_eq().
Fair enough, should we issue a warning if np->full_name contains upper
case while name does not (and vice versa) to help troubleshoot cases
like the one we found with Vivien?
--
Florian
On 1/24/19 5:20 PM, Florian Fainelli wrote:
>
>
> On 1/24/19 3:45 PM, Frank Rowand wrote:
>> On 1/24/19 12:08 PM, Florian Fainelli wrote:
>>> Since c32569e358ad ("regulator: Use of_node_name_eq for node name
>>> comparisons") Vivien reported the mc13892-regulator complaining about
>>> not being able to find regulators.
>>>
>>> This is because prior to that commit we used of_node_cmp() to compare
>>> the regulator array passed from mc13892_regulators down to
>>> mc13xxx_parse_regulators_dt() and they are all defined in uppercase
>>> letters by the MC13892_*_DEFINE* macros, whereas they are defined as
>>> lowercase in the DTS.
>>>
>>> Fix this by use strncasecmp() since that makes sure the comparison is
>>> case insensitive like what of_node_cmp() did.
>>>
>>> Reported-by: Vivien Didelot <[email protected]>
>>> Fixes: c32569e358ad ("regulator: Use of_node_name_eq for node name comparisons")
>>> Signed-off-by: Florian Fainelli <[email protected]>
>>> ---
>>> drivers/of/base.c | 3 ++-
>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/of/base.c b/drivers/of/base.c
>>> index 5226e898476e..ff47c86277cb 100644
>>> --- a/drivers/of/base.c
>>> +++ b/drivers/of/base.c
>>> @@ -66,7 +66,8 @@ bool of_node_name_eq(const struct device_node *np, const char *name)
>>> node_name = kbasename(np->full_name);
>>> len = strchrnul(node_name, '@') - node_name;
>>>
>>> - return (strlen(name) == len) && (strncmp(node_name, name, len) == 0);
>>> + return (strlen(name) == len) &&
>>> + (strncasecmp(node_name, name, len) == 0);
>>> }
>>> EXPORT_SYMBOL(of_node_name_eq);
>>>
>>>
>>
>> Node names are case sensitive. Please fix mc13xxx_parse_regulators_dt() to
>> properly handle case instead of changing of_node_name_eq().
>
> Fair enough, should we issue a warning if np->full_name contains upper
> case while name does not (and vice versa) to help troubleshoot cases
> like the one we found with Vivien?
It seems like a lot of work to detect that specific case. If anything,
maybe just add some text to the existing "Unknown regulator: ..." warning
in mc13xxx_parse_regulators_dt() to mention that case matters.
-Frank
On 1/24/19 6:06 PM, Frank Rowand wrote:
> On 1/24/19 5:20 PM, Florian Fainelli wrote:
>>
>>
>> On 1/24/19 3:45 PM, Frank Rowand wrote:
>>> On 1/24/19 12:08 PM, Florian Fainelli wrote:
>>>> Since c32569e358ad ("regulator: Use of_node_name_eq for node name
>>>> comparisons") Vivien reported the mc13892-regulator complaining about
>>>> not being able to find regulators.
>>>>
>>>> This is because prior to that commit we used of_node_cmp() to compare
>>>> the regulator array passed from mc13892_regulators down to
>>>> mc13xxx_parse_regulators_dt() and they are all defined in uppercase
>>>> letters by the MC13892_*_DEFINE* macros, whereas they are defined as
>>>> lowercase in the DTS.
>>>>
>>>> Fix this by use strncasecmp() since that makes sure the comparison is
>>>> case insensitive like what of_node_cmp() did.
>>>>
>>>> Reported-by: Vivien Didelot <[email protected]>
>>>> Fixes: c32569e358ad ("regulator: Use of_node_name_eq for node name comparisons")
>>>> Signed-off-by: Florian Fainelli <[email protected]>
>>>> ---
>>>> drivers/of/base.c | 3 ++-
>>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/of/base.c b/drivers/of/base.c
>>>> index 5226e898476e..ff47c86277cb 100644
>>>> --- a/drivers/of/base.c
>>>> +++ b/drivers/of/base.c
>>>> @@ -66,7 +66,8 @@ bool of_node_name_eq(const struct device_node *np, const char *name)
>>>> node_name = kbasename(np->full_name);
>>>> len = strchrnul(node_name, '@') - node_name;
>>>>
>>>> - return (strlen(name) == len) && (strncmp(node_name, name, len) == 0);
>>>> + return (strlen(name) == len) &&
>>>> + (strncasecmp(node_name, name, len) == 0);
>>>> }
>>>> EXPORT_SYMBOL(of_node_name_eq);
>>>>
>>>>
>>>
>>> Node names are case sensitive. Please fix mc13xxx_parse_regulators_dt() to
>>> properly handle case instead of changing of_node_name_eq().
>>
>> Fair enough, should we issue a warning if np->full_name contains upper
>> case while name does not (and vice versa) to help troubleshoot cases
>> like the one we found with Vivien?
>
> It seems like a lot of work to detect that specific case. If anything,
> maybe just add some text to the existing "Unknown regulator: ..." warning
> in mc13xxx_parse_regulators_dt() to mention that case matters.
I was not thinking about something very clever, just issue a warning
like this, completely untested and likely flawed:
diff --git a/drivers/of/base.c b/drivers/of/base.c
index 5226e898476e..2505286c875b 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -58,6 +58,7 @@ DEFINE_RAW_SPINLOCK(devtree_lock);
bool of_node_name_eq(const struct device_node *np, const char *name)
{
const char *node_name;
+ int ret1, ret2;
size_t len;
if (!np)
@@ -66,7 +67,12 @@ bool of_node_name_eq(const struct device_node *np,
const char *name)
node_name = kbasename(np->full_name);
len = strchrnul(node_name, '@') - node_name;
- return (strlen(name) == len) && (strncmp(node_name, name, len)
== 0);
+ ret1 = strncmp(node_name, name, len);
+ ret2 = strncasecmp(node_name, len);
+
+ WARN(ret1 ^ ret2, "Comparing case sensitive names!");
+
+ return (strlen(name) == len && (strncmp(node_name, name, len) == 0);
}
EXPORT_SYMBOL(of_node_name_eq);
My concern is that we have identified one place here where the
conversion to of_node_name_eq() broke that particular driver in fact,
all other regulator drivers but qcom-rpmh-regulator.c that use
of_node_name_eq() are broken after that change, but presumably this is
not the only place in the kernel where things could break, so having a
warning could potentially help (also adding the backtrace which is neat).
What should the fix look like though? Add an of_node_casename_eq() and
use it, revert Rob's commit that changes these regulators to use
of_node_name_eq()?
I don't know the regulator framework enough to know whether forcibly
making the names lowercase is not breaking sysfs/debugfs...
--
Florian
On Thu, Jan 24, 2019 at 11:00 PM Florian Fainelli <[email protected]> wrote:
>
>
>
> On 1/24/19 6:06 PM, Frank Rowand wrote:
> > On 1/24/19 5:20 PM, Florian Fainelli wrote:
> >>
> >>
> >> On 1/24/19 3:45 PM, Frank Rowand wrote:
> >>> On 1/24/19 12:08 PM, Florian Fainelli wrote:
> >>>> Since c32569e358ad ("regulator: Use of_node_name_eq for node name
> >>>> comparisons") Vivien reported the mc13892-regulator complaining about
> >>>> not being able to find regulators.
> >>>>
> >>>> This is because prior to that commit we used of_node_cmp() to compare
> >>>> the regulator array passed from mc13892_regulators down to
> >>>> mc13xxx_parse_regulators_dt() and they are all defined in uppercase
> >>>> letters by the MC13892_*_DEFINE* macros, whereas they are defined as
> >>>> lowercase in the DTS.
> >>>>
> >>>> Fix this by use strncasecmp() since that makes sure the comparison is
> >>>> case insensitive like what of_node_cmp() did.
> >>>>
> >>>> Reported-by: Vivien Didelot <[email protected]>
> >>>> Fixes: c32569e358ad ("regulator: Use of_node_name_eq for node name comparisons")
> >>>> Signed-off-by: Florian Fainelli <[email protected]>
> >>>> ---
> >>>> drivers/of/base.c | 3 ++-
> >>>> 1 file changed, 2 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/drivers/of/base.c b/drivers/of/base.c
> >>>> index 5226e898476e..ff47c86277cb 100644
> >>>> --- a/drivers/of/base.c
> >>>> +++ b/drivers/of/base.c
> >>>> @@ -66,7 +66,8 @@ bool of_node_name_eq(const struct device_node *np, const char *name)
> >>>> node_name = kbasename(np->full_name);
> >>>> len = strchrnul(node_name, '@') - node_name;
> >>>>
> >>>> - return (strlen(name) == len) && (strncmp(node_name, name, len) == 0);
> >>>> + return (strlen(name) == len) &&
> >>>> + (strncasecmp(node_name, name, len) == 0);
> >>>> }
> >>>> EXPORT_SYMBOL(of_node_name_eq);
> >>>>
> >>>>
> >>>
> >>> Node names are case sensitive. Please fix mc13xxx_parse_regulators_dt() to
> >>> properly handle case instead of changing of_node_name_eq().
> >>
> >> Fair enough, should we issue a warning if np->full_name contains upper
> >> case while name does not (and vice versa) to help troubleshoot cases
> >> like the one we found with Vivien?
> >
> > It seems like a lot of work to detect that specific case. If anything,
> > maybe just add some text to the existing "Unknown regulator: ..." warning
> > in mc13xxx_parse_regulators_dt() to mention that case matters.
>
> I was not thinking about something very clever, just issue a warning
> like this, completely untested and likely flawed:
>
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 5226e898476e..2505286c875b 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -58,6 +58,7 @@ DEFINE_RAW_SPINLOCK(devtree_lock);
> bool of_node_name_eq(const struct device_node *np, const char *name)
> {
> const char *node_name;
> + int ret1, ret2;
> size_t len;
>
> if (!np)
> @@ -66,7 +67,12 @@ bool of_node_name_eq(const struct device_node *np,
> const char *name)
> node_name = kbasename(np->full_name);
> len = strchrnul(node_name, '@') - node_name;
>
> - return (strlen(name) == len) && (strncmp(node_name, name, len)
> == 0);
> + ret1 = strncmp(node_name, name, len);
> + ret2 = strncasecmp(node_name, len);
> +
> + WARN(ret1 ^ ret2, "Comparing case sensitive names!");
> +
> + return (strlen(name) == len && (strncmp(node_name, name, len) == 0);
If it comes to this, I'd rather not always do 3 string compares.
> }
> EXPORT_SYMBOL(of_node_name_eq);
>
>
> My concern is that we have identified one place here where the
> conversion to of_node_name_eq() broke that particular driver in fact,
> all other regulator drivers but qcom-rpmh-regulator.c that use
> of_node_name_eq() are broken after that change, but presumably this is
> not the only place in the kernel where things could break, so having a
> warning could potentially help (also adding the backtrace which is neat).
Most modern DT's don't really rely on the node names. Regulators are
the oddball.
I checked all the other affected regulator drivers. da9052 is also
affected. IMO, we should fix the drivers and I'm sending out patches
to do that.
> What should the fix look like though? Add an of_node_casename_eq() and
> use it, revert Rob's commit that changes these regulators to use
> of_node_name_eq()?
No, we don't want to add of_node_casename_eq.
We may have to internally go back to case insensitive compare, but I'd
like to avoid that and fix any specific cases if possible.
> I don't know the regulator framework enough to know whether forcibly
> making the names lowercase is not breaking sysfs/debugfs...
Userspace relying on specific regulator names seems questionable to
me. Certainly, debugfs is not an ABI.
Rob