On Sun, Dec 08, 2013 at 04:38:58PM +0000, Peter Maydell wrote:
Sorry, but there is already shipping software (kvmtool and QEMU) which isn't emitting clock-frequency properties for cpu nodes, based on what you documented in the kernel doc file, which says:
I didn't document anything here except this patch, I was just trying to reconcile the implementation with the documentation.
"The ARM architecture, in accordance with the ePAPR, requires the cpus and cpu nodes to be present and contain the properties described below."
not "must contain the properties described below and also any others that the ePAPR spec says are mandatory".
I think that's a fairly tortured way of reading the language there to be honest (and doesn't reflect the actual deployed code reading the binding which does use this property without documentation outside ePAPR and does warn if it's absent). If that really is how we want to read things then we probably ought to delete the reference to ePAPR both here and in the other binding documentation we have and fork the specs.
So I'm afraid you're stuck with this being an optional property.
Like I say I think a reasonable and robust implementation shouldn't reject a device tree with it missing but that doesn't stop the device tree being out of spec. This is also the existing kernel behaviour for this property so we're stuck with it anyway and my goal here was to minimise our deviation from the spec so I introduced the minimum practical change in the process of copying it in for discoverability.
This sort of situation is going to become more and more common as people actually look at device trees in production; the kernel will have to be robust against device trees that it previously accepted even if they are out of spec (and should just generally be robust in parsing). We've got to understand that the kernel will fill the role Windows does for PCs - things that run well enough with existing kernels are going to end up being released regardless of spec conformance. Kernels should be liberal in what they accept, DTs should be conservative in what they contain and both need to understand that the other is going to get it wrong some of the time.
(It's also not at all clear what a virtual machine's devicetree should set the clock-frequency properties to anyway...)
Yes, it's a poorly considered property all round. Most currently available silicon has variable clocks for the cores which is an issue with a fixed DT like FDT provides and like you say for simulators and so on it's meaningless.
Ideally someone with the time/enthuisiasm will get this dealt with more sensibly in a future revision of ePAPR.
On 8 December 2013 19:22, Mark Brown broonie@kernel.org wrote:
On Sun, Dec 08, 2013 at 04:38:58PM +0000, Peter Maydell wrote:
Sorry, but there is already shipping software (kvmtool and QEMU) which isn't emitting clock-frequency properties for cpu nodes, based on what you documented in the kernel doc file, which says:
I didn't document anything here except this patch, I was just trying to reconcile the implementation with the documentation.
I mean "you" in the sense of "the Linux kernel developers in general" :-)
"The ARM architecture, in accordance with the ePAPR, requires the cpus and cpu nodes to be present and contain the properties described below."
not "must contain the properties described below and also any others that the ePAPR spec says are mandatory".
I think that's a fairly tortured way of reading the language there to be honest
It's the way that I read it, and presumably also the way it was read by Marc Z when he wrote the kvmtool device tree writing code, and also I would assume the way it was read by the people who wrote the random three or four dts files in arch/arm/boot/dts that I checked, since none of them include clock-frequency properties as far as I can see. If half a dozen different people all produced device trees with cpu nodes without a clock-frequency property then I think it's a bit hard to claim that it requires a tortured reading of the documentation to decide that it's not mandatory.
(and doesn't reflect the actual deployed code reading the binding which does use this property without documentation outside ePAPR and does warn if it's absent)
This code should be fixed, then... (I'm pretty sure the kernel has not always warned about the absence of this property, or there would not be such a wide prevalence of DTs and DT generating code which omitted it.)
So I'm afraid you're stuck with this being an optional property.
Like I say I think a reasonable and robust implementation shouldn't reject a device tree with it missing but that doesn't stop the device tree being out of spec. This is also the existing kernel behaviour for this property so we're stuck with it anyway and my goal here was to minimise our deviation from the spec so I introduced the minimum practical change in the process of copying it in for discoverability.
This sort of situation is going to become more and more common as people actually look at device trees in production; the kernel will have to be robust against device trees that it previously accepted even if they are out of spec (and should just generally be robust in parsing). We've got to understand that the kernel will fill the role Windows does for PCs - things that run well enough with existing kernels are going to end up being released regardless of spec conformance. Kernels should be liberal in what they accept, DTs should be conservative in what they contain and both need to understand that the other is going to get it wrong some of the time.
I agree generally with this, and I think I would also include that the kernel should not start warning about things which it has not warned about in the past, because this basically manifests as "user experiences warning which they can't do much about"; it's directed at the wrong target and much too late.
If you want to make properties mandatory then you should have some sort of setup for validating an entire device tree including all the properties which the kernel doesn't happen to use yet or doesn't use in every configuration. That can then feel free to warn copiously about any missing mandatory properties, obviously.
(It's also not at all clear what a virtual machine's devicetree should set the clock-frequency properties to anyway...)
Yes, it's a poorly considered property all round. Most currently available silicon has variable clocks for the cores which is an issue with a fixed DT like FDT provides and like you say for simulators and so on it's meaningless.
So what would we lose by making it genuinely optional and just silently doing something reasonable if it's not set?
thanks -- PMM
On Sun, Dec 08, 2013 at 07:51:59PM +0000, Peter Maydell wrote:
On 8 December 2013 19:22, Mark Brown broonie@kernel.org wrote:
On Sun, Dec 08, 2013 at 04:38:58PM +0000, Peter Maydell wrote:
"The ARM architecture, in accordance with the ePAPR, requires the cpus and cpu nodes to be present and contain the properties described below."
not "must contain the properties described below and also any others that the ePAPR spec says are mandatory".
I think that's a fairly tortured way of reading the language there to be honest
It's the way that I read it, and presumably also the way it was read by Marc Z when he wrote the kvmtool device tree writing code, and also I would assume the way it was read by the people who wrote the random three or four dts files in arch/arm/boot/dts that I checked, since none of them include clock-frequency properties as far as I can see. If half a dozen different people all produced device trees with cpu nodes without a clock-frequency property then I think it's a bit hard to claim that it requires a tortured reading of the documentation to decide that it's not mandatory.
I think you're assuming that people are reading the binding documents at all when they write things and that should they look at them they will follow any references to ePAPR (which requires a signup to download) and then pay attention to the requirements annotations (this is done a lot more clearly in most of the in-tree documentation) all without making any mistakes. I think each of these assumptions is optimistic.
(and doesn't reflect the actual deployed code reading the binding which does use this property without documentation outside ePAPR and does warn if it's absent)
This code should be fixed, then... (I'm pretty sure the kernel has not always warned about the absence of this property, or there would not be such a wide prevalence of DTs and DT generating code which omitted it.)
Too late for that, it's shipping in stable kernels.
being released regardless of spec conformance. Kernels should be liberal in what they accept, DTs should be conservative in what they contain and both need to understand that the other is going to get it wrong some of the time.
I agree generally with this, and I think I would also include that the kernel should not start warning about things which it has not warned about in the past, because this basically manifests as "user experiences warning which they can't do much about"; it's directed at the wrong target and much too late.
I think it's reasonable to warn on things, especially in cases where it's risky or difficult to try to figure out what to do without the property. At this point most people can update their DTs and obviously there's an awful lot of cases where the only people who would ever see warnings are definitely people in a position to do so (things like consumer electronics for example).
There does come a point where it's just nitpicking and not helpful but if it has a substantial effect on functionality then it's useful. In this case suppressing the warning for non-asymmetric systems might be sensible.
If you want to make properties mandatory then you should have some sort of setup for validating an entire device tree including all the properties which the kernel doesn't happen to use yet or doesn't use in every configuration. That can then feel free to warn copiously about any missing mandatory properties, obviously.
That's one of the goals with the DT validation software that people like Tomasz are working on. Sadly it doesn't exist yet, it would definitely make life a lot easier.
(It's also not at all clear what a virtual machine's devicetree should set the clock-frequency properties to anyway...)
Yes, it's a poorly considered property all round. Most currently available silicon has variable clocks for the cores which is an issue with a fixed DT like FDT provides and like you say for simulators and so on it's meaningless.
So what would we lose by making it genuinely optional and just silently doing something reasonable if it's not set?
For all practical purposes it is currently optional but the spec says it is mandatory. I would rather err on the side of not changing the documentation in case someone does work based on ePAPR and/or an old kernel and since doing that keeps the spec more stable even if we do implement in a more tolerant fashion within Linux (as we should).
On 8 December 2013 21:50, Mark Brown broonie@kernel.org wrote:
On Sun, Dec 08, 2013 at 07:51:59PM +0000, Peter Maydell wrote:
On 8 December 2013 19:22, Mark Brown broonie@kernel.org wrote:
(and doesn't reflect the actual deployed code reading the binding which does use this property without documentation outside ePAPR and does warn if it's absent)
This code should be fixed, then... (I'm pretty sure the kernel has not always warned about the absence of this property, or there would not be such a wide prevalence of DTs and DT generating code which omitted it.)
Too late for that, it's shipping in stable kernels.
There does come a point where it's just nitpicking and not helpful but if it has a substantial effect on functionality then it's useful. In this case suppressing the warning for non-asymmetric systems might be sensible.
Hmm, so "mandatory for non-symmetric [I assume you mean that and not really 'non-asymmetric'?], otherwise optional" ? I think that would be reasonable and preserve backwards compatibility.
For all practical purposes it is currently optional but the spec says it is mandatory. I would rather err on the side of not changing the documentation in case someone does work based on ePAPR and/or an old kernel and since doing that keeps the spec more stable even if we do implement in a more tolerant fashion within Linux (as we should).
As I say, I don't think your specification currently does say it is mandatory. If the documentation doesn't clearly list it as a mandatory parameter, and a large number of people writing DTS files or DT generation code haven't put it in, and the kernel didn't complain about it not being present for a long long time, then de facto it is optional, and you should make your documentation conform with reality and fix bugs where the kernel isn't coping with that.
thanks -- PMM
On Sun, Dec 08, 2013 at 10:55:28PM +0000, Peter Maydell wrote:
On 8 December 2013 21:50, Mark Brown broonie@kernel.org wrote:
There does come a point where it's just nitpicking and not helpful but if it has a substantial effect on functionality then it's useful. In this case suppressing the warning for non-asymmetric systems might be sensible.
Hmm, so "mandatory for non-symmetric [I assume you mean that and not really 'non-asymmetric'?], otherwise optional" ? I think that would be reasonable and preserve backwards compatibility.
No, I really mean asymmetric - I'm talking about the cases where we suppress the warning.
For all practical purposes it is currently optional but the spec says it is mandatory. I would rather err on the side of not changing the documentation in case someone does work based on ePAPR and/or an old kernel and since doing that keeps the spec more stable even if we do implement in a more tolerant fashion within Linux (as we should).
As I say, I don't think your specification currently does say it is mandatory. If the documentation doesn't clearly list it as a mandatory parameter, and a large number of
Like I say I don't think that's a sensible interpretation and that if it is what we want to do then someone's got to find the time to copy all the bindings out of the spec into the kernel.
people writing DTS files or DT generation code haven't put it in, and the kernel didn't complain about it not being present for a long long time, then de facto it is optional, and you should make your documentation conform with reality and fix bugs where the kernel isn't coping with that.
The kernel currently copes fine with this, welcome to the world of writing things down in specifications.
linaro-kernel@lists.linaro.org
-
Mark Brown -
Peter Maydell