Should of_device_is_compatible() use strcmp() rather than strncasecmp()?
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
Quoted 2 lines
Is there any particular reason that
of_device_is_compatibleusesstrncasecmp()?
It always has, so some things might break with certain device trees when we make the matching more strict/correct.
Quoted 4 lines
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_gethv.ucc_geth_phy, or fsl,cpm v. fsl,cpm-enet.
Huh? Are you saying things are matching on substrings? That's even more wrong!
Quoted 1 line
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
Quoted 2 lines · Segher Boessenkool
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.
Quoted 8 lines
Quoted 1 line
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
Quoted 8 lines · Scott Wood
How about a
CONFIG_NOT_BROKEN_DEVICE_TREEthat 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
Quoted 5 lines · Olof Johansson
Quoted 2 lines · Scott Wood
How about a
CONFIG_NOT_BROKEN_DEVICE_TREEthat 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.
Quoted 7 lines
Quoted 1 line
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.
Quoted 2 lines
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.
Quoted 2 lines
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
Quoted 6 lines
Quoted 4 lines
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.
Quoted 1 line
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".
Quoted 4 lines
Quoted 2 lines
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
Quoted 10 lines
Quoted 8 lines
How about a
CONFIG_NOT_BROKEN_DEVICE_TREEthat 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).
Quoted 4 lines
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.
Quoted 1 line
Device trees will never be perfect,
But the kernel applies workarounds _way_ too generously. This is starting to cause more problems than it solves.
Quoted 3 lines
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
Quoted 7 lines · Scott Wood
Is there any particular reason that
of_device_is_compatibleusesstrncasecmp()? 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_gethv.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.
Quoted 5 lines
Quoted 1 line
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...
Quoted 3 lines
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