From: Scott Wood <hidden> Date: 2007-07-03 21:27:45
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
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
From: Scott Wood <hidden> Date: 2007-07-03 22:15:18
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.
quoted
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
From: Olof Johansson <hidden> Date: 2007-07-03 22:25:43
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
From: Scott Wood <hidden> Date: 2007-07-03 22:33:46
Olof Johansson wrote:
On Tue, Jul 03, 2007 at 05:15:11PM -0500, Scott Wood wrote:
quoted
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
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
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".
quoted
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.
From: Paul Mackerras <hidden> Date: 2007-07-04 03:28:27
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.
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