| Message ID | 20230829194200.1901988-1-festevam@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [v5,1/3] dt-bindings: thermal-zones: Document critical-action | expand |
On Tue, Aug 29, 2023 at 9:42 PM Fabio Estevam <festevam@gmail.com> wrote: > > From: Fabio Estevam <festevam@denx.de> > > Document the critical-action property to describe the thermal action > the OS should perform after the critical temperature is reached. > > The possible values are "shutdown" and "reboot". > > The motivation for introducing the critical-action property is that > different systems may need different thermal actions when the critical > temperature is reached. > > For example, a desktop PC may want the OS to trigger a shutdown > when the critical temperature is reached. > > However, in some embedded cases, such behavior does not suit well, > as the board may be unattended in the field and rebooting may be a > better approach. > > The bootloader may also benefit from this new property as it can check > the SoC temperature and in case the temperature is above the critical > point, it can trigger a shutdown or reboot accordingly. > > Signed-off-by: Fabio Estevam <festevam@denx.de> > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > --- > Changes since v4: > - None. > > .../devicetree/bindings/thermal/thermal-zones.yaml | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/Documentation/devicetree/bindings/thermal/thermal-zones.yaml b/Documentation/devicetree/bindings/thermal/thermal-zones.yaml > index 4f3acdc4dec0..c2e4d28f885b 100644 > --- a/Documentation/devicetree/bindings/thermal/thermal-zones.yaml > +++ b/Documentation/devicetree/bindings/thermal/thermal-zones.yaml > @@ -75,6 +75,15 @@ patternProperties: > framework and assumes that the thermal sensors in this zone > support interrupts. > > + critical-action: > + $ref: /schemas/types.yaml#/definitions/string > + description: > + The action the OS should perform after the critical temperature is reached. > + > + enum: > + - shutdown > + - reboot > + > thermal-sensors: > $ref: /schemas/types.yaml#/definitions/phandle-array > maxItems: 1 > -- I'm wondering if this should be a bool property called "critical-reboot", say, which when present would mean to carry out a reboot instead of a shutdown in an emergency. As defined above, the "shutdown" value is simply redundant, because the code will effectively convert any other value to "shutdown" anyway.
On 30/08/2023 13:37, Rafael J. Wysocki wrote: > On Tue, Aug 29, 2023 at 9:42 PM Fabio Estevam <festevam@gmail.com> wrote: >> >> From: Fabio Estevam <festevam@denx.de> >> >> Document the critical-action property to describe the thermal action >> the OS should perform after the critical temperature is reached. >> >> The possible values are "shutdown" and "reboot". >> >> The motivation for introducing the critical-action property is that >> different systems may need different thermal actions when the critical >> temperature is reached. >> >> For example, a desktop PC may want the OS to trigger a shutdown >> when the critical temperature is reached. >> >> However, in some embedded cases, such behavior does not suit well, >> as the board may be unattended in the field and rebooting may be a >> better approach. >> >> The bootloader may also benefit from this new property as it can check >> the SoC temperature and in case the temperature is above the critical >> point, it can trigger a shutdown or reboot accordingly. >> >> Signed-off-by: Fabio Estevam <festevam@denx.de> >> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >> --- >> Changes since v4: >> - None. >> >> .../devicetree/bindings/thermal/thermal-zones.yaml | 9 +++++++++ >> 1 file changed, 9 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/thermal/thermal-zones.yaml b/Documentation/devicetree/bindings/thermal/thermal-zones.yaml >> index 4f3acdc4dec0..c2e4d28f885b 100644 >> --- a/Documentation/devicetree/bindings/thermal/thermal-zones.yaml >> +++ b/Documentation/devicetree/bindings/thermal/thermal-zones.yaml >> @@ -75,6 +75,15 @@ patternProperties: >> framework and assumes that the thermal sensors in this zone >> support interrupts. >> >> + critical-action: >> + $ref: /schemas/types.yaml#/definitions/string >> + description: >> + The action the OS should perform after the critical temperature is reached. >> + >> + enum: >> + - shutdown >> + - reboot >> + >> thermal-sensors: >> $ref: /schemas/types.yaml#/definitions/phandle-array >> maxItems: 1 >> -- > > I'm wondering if this should be a bool property called > "critical-reboot", say, which when present would mean to carry out a > reboot instead of a shutdown in an emergency. > > As defined above, the "shutdown" value is simply redundant, because > the code will effectively convert any other value to "shutdown" > anyway. We covered this at v1. Bool does not allow this property to change in the future, e.g. for a third mode. And how would you change the action to shutdown if default action in the system was reboot? Best regards, Krzysztof
On Wed, Aug 30, 2023 at 3:07 PM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 30/08/2023 13:37, Rafael J. Wysocki wrote: > > On Tue, Aug 29, 2023 at 9:42 PM Fabio Estevam <festevam@gmail.com> wrote: > >> > >> From: Fabio Estevam <festevam@denx.de> > >> > >> Document the critical-action property to describe the thermal action > >> the OS should perform after the critical temperature is reached. > >> > >> The possible values are "shutdown" and "reboot". > >> > >> The motivation for introducing the critical-action property is that > >> different systems may need different thermal actions when the critical > >> temperature is reached. > >> > >> For example, a desktop PC may want the OS to trigger a shutdown > >> when the critical temperature is reached. > >> > >> However, in some embedded cases, such behavior does not suit well, > >> as the board may be unattended in the field and rebooting may be a > >> better approach. > >> > >> The bootloader may also benefit from this new property as it can check > >> the SoC temperature and in case the temperature is above the critical > >> point, it can trigger a shutdown or reboot accordingly. > >> > >> Signed-off-by: Fabio Estevam <festevam@denx.de> > >> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > >> --- > >> Changes since v4: > >> - None. > >> > >> .../devicetree/bindings/thermal/thermal-zones.yaml | 9 +++++++++ > >> 1 file changed, 9 insertions(+) > >> > >> diff --git a/Documentation/devicetree/bindings/thermal/thermal-zones.yaml b/Documentation/devicetree/bindings/thermal/thermal-zones.yaml > >> index 4f3acdc4dec0..c2e4d28f885b 100644 > >> --- a/Documentation/devicetree/bindings/thermal/thermal-zones.yaml > >> +++ b/Documentation/devicetree/bindings/thermal/thermal-zones.yaml > >> @@ -75,6 +75,15 @@ patternProperties: > >> framework and assumes that the thermal sensors in this zone > >> support interrupts. > >> > >> + critical-action: > >> + $ref: /schemas/types.yaml#/definitions/string > >> + description: > >> + The action the OS should perform after the critical temperature is reached. > >> + > >> + enum: > >> + - shutdown > >> + - reboot > >> + > >> thermal-sensors: > >> $ref: /schemas/types.yaml#/definitions/phandle-array > >> maxItems: 1 > >> -- > > > > I'm wondering if this should be a bool property called > > "critical-reboot", say, which when present would mean to carry out a > > reboot instead of a shutdown in an emergency. > > > > As defined above, the "shutdown" value is simply redundant, because > > the code will effectively convert any other value to "shutdown" > > anyway. > > We covered this at v1. Bool does not allow this property to change in > the future, e.g. for a third mode. And how would you change the action > to shutdown if default action in the system was reboot? Well, as a matter of fact, it isn't, so I'm not sure where this is going. Bool definitely allows the property to be not present, which means that the default behavior is intended and this is all about overriding a known default behavior. Anyway, if the maintainers of DT bindings are fine with the current definition, I'm not going to block this. I just wanted to make a note that it wasn't looking particularly straightforward to me.
Hi Rafael, On 30/08/2023 10:54, Rafael J. Wysocki wrote: > Well, as a matter of fact, it isn't, so I'm not sure where this is > going. > > Bool definitely allows the property to be not present, which means > that the default behavior is intended and this is all about overriding > a known default behavior. This devicetree property can be used on non-Linux environments, such as bootloaders. Bootloaders may have different default behavior than Linux, so explicitly listing "shutdown" or "reboot" make it clearer, independent of the OS. Thanks
On 30/08/2023 15:54, Rafael J. Wysocki wrote: > On Wed, Aug 30, 2023 at 3:07 PM Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: >> >> On 30/08/2023 13:37, Rafael J. Wysocki wrote: >>> On Tue, Aug 29, 2023 at 9:42 PM Fabio Estevam <festevam@gmail.com> wrote: >>>> >>>> From: Fabio Estevam <festevam@denx.de> >>>> >>>> Document the critical-action property to describe the thermal action >>>> the OS should perform after the critical temperature is reached. >>>> >>>> The possible values are "shutdown" and "reboot". >>>> >>>> The motivation for introducing the critical-action property is that >>>> different systems may need different thermal actions when the critical >>>> temperature is reached. >>>> >>>> For example, a desktop PC may want the OS to trigger a shutdown >>>> when the critical temperature is reached. >>>> >>>> However, in some embedded cases, such behavior does not suit well, >>>> as the board may be unattended in the field and rebooting may be a >>>> better approach. >>>> >>>> The bootloader may also benefit from this new property as it can check >>>> the SoC temperature and in case the temperature is above the critical >>>> point, it can trigger a shutdown or reboot accordingly. >>>> >>>> Signed-off-by: Fabio Estevam <festevam@denx.de> >>>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >>>> --- >>>> Changes since v4: >>>> - None. >>>> >>>> .../devicetree/bindings/thermal/thermal-zones.yaml | 9 +++++++++ >>>> 1 file changed, 9 insertions(+) >>>> >>>> diff --git a/Documentation/devicetree/bindings/thermal/thermal-zones.yaml b/Documentation/devicetree/bindings/thermal/thermal-zones.yaml >>>> index 4f3acdc4dec0..c2e4d28f885b 100644 >>>> --- a/Documentation/devicetree/bindings/thermal/thermal-zones.yaml >>>> +++ b/Documentation/devicetree/bindings/thermal/thermal-zones.yaml >>>> @@ -75,6 +75,15 @@ patternProperties: >>>> framework and assumes that the thermal sensors in this zone >>>> support interrupts. >>>> >>>> + critical-action: >>>> + $ref: /schemas/types.yaml#/definitions/string >>>> + description: >>>> + The action the OS should perform after the critical temperature is reached. >>>> + >>>> + enum: >>>> + - shutdown >>>> + - reboot >>>> + >>>> thermal-sensors: >>>> $ref: /schemas/types.yaml#/definitions/phandle-array >>>> maxItems: 1 >>>> -- >>> >>> I'm wondering if this should be a bool property called >>> "critical-reboot", say, which when present would mean to carry out a >>> reboot instead of a shutdown in an emergency. >>> >>> As defined above, the "shutdown" value is simply redundant, because >>> the code will effectively convert any other value to "shutdown" >>> anyway. >> >> We covered this at v1. Bool does not allow this property to change in >> the future, e.g. for a third mode. And how would you change the action >> to shutdown if default action in the system was reboot? > > Well, as a matter of fact, it isn't, so I'm not sure where this is going. It isn't in which system and firmware? Maybe most, but how can you know for sure. Bindings are independent of given OS implementation, thus relying on default OS choice is shortsighted. > > Bool definitely allows the property to be not present, which means > that the default behavior is intended and this is all about overriding > a known default behavior. > > Anyway, if the maintainers of DT bindings are fine with the current > definition, I'm not going to block this. I just wanted to make a note > that it wasn't looking particularly straightforward to me. Sure. It has one DT's maintainer Ack (mine) and maybe also Rob will comment on it. Best regards, Krzysztof
Hi Fabio, On Wed, Aug 30, 2023 at 5:14 PM Fabio Estevam <festevam@denx.de> wrote: > > Hi Rafael, > > On 30/08/2023 10:54, Rafael J. Wysocki wrote: > > > Well, as a matter of fact, it isn't, so I'm not sure where this is > > going. > > > > Bool definitely allows the property to be not present, which means > > that the default behavior is intended and this is all about overriding > > a known default behavior. > > This devicetree property can be used on non-Linux environments, such as > bootloaders. > > Bootloaders may have different default behavior than Linux, so > explicitly listing > "shutdown" or "reboot" make it clearer, independent of the OS. This property is defined in the Linux kernel source tree and it will only turn out whether or not any other projects adopt it, so I'm not really convinced by this argument. What I'm saying is that from the Linux kernel's POV, the definition is more complex than it needs to be. Moreover, to me it merely represents the preference of a system integrator and so in practice, it can be ignored.
On Wed, Aug 30, 2023 at 5:33 PM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 30/08/2023 15:54, Rafael J. Wysocki wrote: > > On Wed, Aug 30, 2023 at 3:07 PM Krzysztof Kozlowski > > <krzysztof.kozlowski@linaro.org> wrote: > >> > >> On 30/08/2023 13:37, Rafael J. Wysocki wrote: > >>> On Tue, Aug 29, 2023 at 9:42 PM Fabio Estevam <festevam@gmail.com> wrote: > >>>> > >>>> From: Fabio Estevam <festevam@denx.de> > >>>> > >>>> Document the critical-action property to describe the thermal action > >>>> the OS should perform after the critical temperature is reached. > >>>> > >>>> The possible values are "shutdown" and "reboot". > >>>> > >>>> The motivation for introducing the critical-action property is that > >>>> different systems may need different thermal actions when the critical > >>>> temperature is reached. > >>>> > >>>> For example, a desktop PC may want the OS to trigger a shutdown > >>>> when the critical temperature is reached. > >>>> > >>>> However, in some embedded cases, such behavior does not suit well, > >>>> as the board may be unattended in the field and rebooting may be a > >>>> better approach. > >>>> > >>>> The bootloader may also benefit from this new property as it can check > >>>> the SoC temperature and in case the temperature is above the critical > >>>> point, it can trigger a shutdown or reboot accordingly. > >>>> > >>>> Signed-off-by: Fabio Estevam <festevam@denx.de> > >>>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > >>>> --- > >>>> Changes since v4: > >>>> - None. > >>>> > >>>> .../devicetree/bindings/thermal/thermal-zones.yaml | 9 +++++++++ > >>>> 1 file changed, 9 insertions(+) > >>>> > >>>> diff --git a/Documentation/devicetree/bindings/thermal/thermal-zones.yaml b/Documentation/devicetree/bindings/thermal/thermal-zones.yaml > >>>> index 4f3acdc4dec0..c2e4d28f885b 100644 > >>>> --- a/Documentation/devicetree/bindings/thermal/thermal-zones.yaml > >>>> +++ b/Documentation/devicetree/bindings/thermal/thermal-zones.yaml > >>>> @@ -75,6 +75,15 @@ patternProperties: > >>>> framework and assumes that the thermal sensors in this zone > >>>> support interrupts. > >>>> > >>>> + critical-action: > >>>> + $ref: /schemas/types.yaml#/definitions/string > >>>> + description: > >>>> + The action the OS should perform after the critical temperature is reached. > >>>> + > >>>> + enum: > >>>> + - shutdown > >>>> + - reboot > >>>> + > >>>> thermal-sensors: > >>>> $ref: /schemas/types.yaml#/definitions/phandle-array > >>>> maxItems: 1 > >>>> -- > >>> > >>> I'm wondering if this should be a bool property called > >>> "critical-reboot", say, which when present would mean to carry out a > >>> reboot instead of a shutdown in an emergency. > >>> > >>> As defined above, the "shutdown" value is simply redundant, because > >>> the code will effectively convert any other value to "shutdown" > >>> anyway. > >> > >> We covered this at v1. Bool does not allow this property to change in > >> the future, e.g. for a third mode. And how would you change the action > >> to shutdown if default action in the system was reboot? > > > > Well, as a matter of fact, it isn't, so I'm not sure where this is going. > > It isn't in which system and firmware? There is a specific default behavior present in the Linux kernel today. The addition of this property isn't going to change it AFAICS. > Maybe most, but how can you know > for sure. Bindings are independent of given OS implementation, thus > relying on default OS choice is shortsighted. So can you point me to any project other than the Linux kernel that will support this property once it gets to the DT bindings documentation in the kernel source? > > > > Bool definitely allows the property to be not present, which means > > that the default behavior is intended and this is all about overriding > > a known default behavior. > > > > Anyway, if the maintainers of DT bindings are fine with the current > > definition, I'm not going to block this. I just wanted to make a note > > that it wasn't looking particularly straightforward to me. > > Sure. It has one DT's maintainer Ack (mine) and maybe also Rob will > comment on it. OK
diff --git a/Documentation/devicetree/bindings/thermal/thermal-zones.yaml b/Documentation/devicetree/bindings/thermal/thermal-zones.yaml index 4f3acdc4dec0..c2e4d28f885b 100644 --- a/Documentation/devicetree/bindings/thermal/thermal-zones.yaml +++ b/Documentation/devicetree/bindings/thermal/thermal-zones.yaml @@ -75,6 +75,15 @@ patternProperties: framework and assumes that the thermal sensors in this zone support interrupts. + critical-action: + $ref: /schemas/types.yaml#/definitions/string + description: + The action the OS should perform after the critical temperature is reached. + + enum: + - shutdown + - reboot + thermal-sensors: $ref: /schemas/types.yaml#/definitions/phandle-array maxItems: 1