Should of_device_is_compatible() use strcmp() rather than strncasecmp()?

Should of_device_is_compatible() use strcmp() rather than strncasecmp()?

[Scott Wood]

Is there any particular reason that of_device_is_compatible uses 
strncasecmp()?  Besides the OF spec saying that names (and thus 
compatibles) are case sensitive, the "n" part screws up matching when a 
subset of a string is not a more generic version thereof.  For example, 
ucc_geth v. ucc_geth_phy, or fsl,cpm v. fsl,cpm-enet.

Does anything actually rely on this behavior?

-Scott

Re: Should of_device_is_compatible() use strcmp() rather than strncasecmp()?

[Segher Boessenkool]

> Is there any particular reason that of_device_is_compatible uses
> strncasecmp()?

It always has, so some things might break with certain
device trees when we make the matching more strict/correct.

> Besides the OF spec saying that names (and thus
> compatibles) are case sensitive, the "n" part screws up matching  
> when a
> subset of a string is not a more generic version thereof.  For  
> example,
> ucc_geth v. ucc_geth_phy, or fsl,cpm v. fsl,cpm-enet.

Huh?  Are you saying things are matching on substrings?
That's even more wrong!

> Does anything actually rely on this behavior?

Yeah I'm pretty sure some things do.  Those are bugs that
need fixing.  My plan was to print a big fat warning whenever
a case-insensitive match wouldn't match when following the
rules; I never got around to actually implementing that though.
Maybe some day ;-)


Segher

Re: Should of_device_is_compatible() use strcmp() rather than strncasecmp()?

[Scott Wood]

Segher Boessenkool wrote:
> Huh?  Are you saying things are matching on substrings?
> That's even more wrong!

Yes, unfortunately.  I tried getting the BRG frequency from the fsl,cpm 
node and I got the fsl,cpm-enet node instead. :-P

ucc_geth requires device_type == network to avoid matching ucc_geth phy.

>> Does anything actually rely on this behavior?
> 
> 
> Yeah I'm pretty sure some things do.  Those are bugs that
> need fixing.  My plan was to print a big fat warning whenever
> a case-insensitive match wouldn't match when following the
> rules; I never got around to actually implementing that though.
> Maybe some day ;-)

How about a CONFIG_NOT_BROKEN_DEVICE_TREE that disables this and other 
legacy stuff?  If unset, warnings will be printed whenever broken things 
are detected.  Boards which need proper device tree parsing can select 
the config option.

I'd prefer the other way around (CONFIG_BROKEN_DEVICE_TREE), but it'd 
need to default y at first, and kconfig has no unselect command that I 
can see...

-Scott

Re: Should of_device_is_compatible() use strcmp() rather than strncasecmp()?

[Olof Johansson]

On Tue, Jul 03, 2007 at 05:15:11PM -0500, Scott Wood wrote:

> How about a CONFIG_NOT_BROKEN_DEVICE_TREE that disables this and other 
> legacy stuff?  If unset, warnings will be printed whenever broken things 
> are detected.  Boards which need proper device tree parsing can select 
> the config option.
> 
> I'd prefer the other way around (CONFIG_BROKEN_DEVICE_TREE), but it'd 
> need to default y at first, and kconfig has no unselect command that I 
> can see...

What's the value add in doing this? The code to handle both cases still
has to be in there (just under ifdef now). Is there actually any harm
in doing case-insensitive matching today, where things break because
there are conflicting properties with different cases?

Device trees will never be perfect, and it's idiotic of the kernel to
expect them to be. Even with the dts files, there are several of them
that have errors in them. There will be more in the future as well.

-Olof

Re: Should of_device_is_compatible() use strcmp() rather than strncasecmp()?

[Scott Wood]

Olof Johansson wrote:
> On Tue, Jul 03, 2007 at 05:15:11PM -0500, Scott Wood wrote:
> 
> 
>>How about a CONFIG_NOT_BROKEN_DEVICE_TREE that disables this and other 
>>legacy stuff?  If unset, warnings will be printed whenever broken things 
>>are detected.  Boards which need proper device tree parsing can select 
>>the config option.
>>
>>I'd prefer the other way around (CONFIG_BROKEN_DEVICE_TREE), but it'd 
>>need to default y at first, and kconfig has no unselect command that I 
>>can see...
> 
> 
> What's the value add in doing this? The code to handle both cases still
> has to be in there (just under ifdef now). Is there actually any harm
> in doing case-insensitive matching today, where things break because
> there are conflicting properties with different cases?

Not for case insensitivity (that I know of), but the whole reason I 
posted this was because of time spent trying to figure out why my serial 
port recently stopped working -- apparently, it's checking nodes in a 
different order now (or something along those lines), causing the 
substring match to match against the wrong thing.

I'd really rather not have to go out of my way to avoid picking 
compatible names that break Linux.

> Device trees will never be perfect, and it's idiotic of the kernel to
> expect them to be.

It's even more idiotic to break valid device trees just because the 
kernel has always done so.  Putting it under ifdef, especially once the 
default is conformant behavior, will make it more likely that future 
device trees are compliant in that regrad, as long as Linux is involved 
in the testing process.

> Even with the dts files, there are several of them
> that have errors in them. There will be more in the future as well.

And the earlier we implement stricter checking, the fewer of them there 
will be.

-Scott

Re: Should of_device_is_compatible() use strcmp() rather than strncasecmp()?

[Segher Boessenkool]

>> What's the value add in doing this? The code to handle both cases  
>> still
>> has to be in there (just under ifdef now). Is there actually any harm
>> in doing case-insensitive matching today, where things break because
>> there are conflicting properties with different cases?
>
> Not for case insensitivity (that I know of), but the whole reason I  
> posted this was because of time spent trying to figure out why my  
> serial port recently stopped working -- apparently, it's checking  
> nodes in a different order now (or something along those lines),  
> causing the substring match to match against the wrong thing.

Substring matching is WRONG WRONG WRONG.

> It's even more idiotic to break valid device trees just because the  
> kernel has always done so.  Putting it under ifdef, especially once  
> the default is conformant behavior, will make it more likely that  
> future device trees are compliant in that regrad, as long as Linux  
> is involved in the testing process.

We should just implement *targeted* workarounds, not say
"oh, some trees use bad upper/lower casing?  Let's just match
case-insensitive then".

>> Even with the dts files, there are several of them
>> that have errors in them. There will be more in the future as well.
>
> And the earlier we implement stricter checking, the fewer of them  
> there will be.

Yes, unfortunately.


Segher

Re: Should of_device_is_compatible() use strcmp() rather than strncasecmp()?

[Segher Boessenkool]

>> How about a CONFIG_NOT_BROKEN_DEVICE_TREE that disables this and  
>> other
>> legacy stuff?  If unset, warnings will be printed whenever broken  
>> things
>> are detected.  Boards which need proper device tree parsing can  
>> select
>> the config option.
>>
>> I'd prefer the other way around (CONFIG_BROKEN_DEVICE_TREE), but it'd
>> need to default y at first, and kconfig has no unselect command  
>> that I
>> can see...
>
> What's the value add in doing this?

None.  The kernel should either allow or not allow certain
classes of brokenness; and if it does allow it, it is better
to only allow it in those cases where it expects the brokenness
(i.e., on certain platforms, in this case).

> The code to handle both cases still
> has to be in there (just under ifdef now). Is there actually any harm
> in doing case-insensitive matching today, where things break because
> there are conflicting properties with different cases?

There is a big potential harm in not looking at case for the
part of the name before the comma (upper case is stock symbol,
lower case isn't).  An actual real life example?  Dunno, I'm
sure we'll find some if we change to case-sensitive matching.

> Device trees will never be perfect,

But the kernel applies workarounds _way_ too generously.  This
is starting to cause more problems than it solves.

> and it's idiotic of the kernel to
> expect them to be. Even with the dts files, there are several of them
> that have errors in them. There will be more in the future as well.

Sure, especially since the kernel allows all kinds of wrong
things and second guesses what is meant.  Sometimes it guesses
right, and then the device tree is shipped, since "it works so
it must be right".


Segher

Re: Should of_device_is_compatible() use strcmp() rather than strncasecmp()?

[Paul Mackerras]

Scott Wood writes:

> Is there any particular reason that of_device_is_compatible uses 
> strncasecmp()?  Besides the OF spec saying that names (and thus 
> compatibles) are case sensitive, the "n" part screws up matching when a 
> subset of a string is not a more generic version thereof.  For example, 
> ucc_geth v. ucc_geth_phy, or fsl,cpm v. fsl,cpm-enet.
> 
> Does anything actually rely on this behavior?

Things did in the past rely on the case-insensitive comparison but
probably don't any more.  I recall an issue with "ata" vs. "ATA" on
the powerbook 3400.

As for the "n" part, I don't recall exactly why that was done.  I
think we should change it to use strcmp and fix any drivers that
break.

Paul.

Re: Should of_device_is_compatible() use strcmp() rather than strncasecmp()?

[Segher Boessenkool]

>> Does anything actually rely on this behavior?
>
> Things did in the past rely on the case-insensitive comparison but
> probably don't any more.  I recall an issue with "ata" vs. "ATA" on
> the powerbook 3400.

Yes, and there are some AAPL, vs. aapl, things too; and an
i2c thing as well.  Probably more.  Nothing unfixable of course,
but...

> As for the "n" part, I don't recall exactly why that was done.  I
> think we should change it to use strcmp and fix any drivers that
> break.

...we first need to find the problem spots.  If we switch early
in the .23 series, will that be enough time to find all (important)
issues?


Segher