2022-07-06 19:46:42

by Andrew Davis

[permalink] [raw]
Subject: [PATCH v2 1/4] arm64: dts: ti: k3-am65-main: Disable RNG node

The hardware random number generator is used by OP-TEE and is access is
denied to other users with SoC level bus firewalls. Any access to this
device from Linux will result in firewall errors. Disable this node.

Signed-off-by: Andrew Davis <[email protected]>
---

Changes from v1:
- Added comment in dtsi file

arch/arm64/boot/dts/ti/k3-am65-main.dtsi | 1 +
1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/ti/k3-am65-main.dtsi b/arch/arm64/boot/dts/ti/k3-am65-main.dtsi
index e749343accedd..9de5a8294acd6 100644
--- a/arch/arm64/boot/dts/ti/k3-am65-main.dtsi
+++ b/arch/arm64/boot/dts/ti/k3-am65-main.dtsi
@@ -127,6 +127,7 @@ rng: rng@4e10000 {
reg = <0x0 0x4e10000 0x0 0x7d>;
interrupts = <GIC_SPI 24 IRQ_TYPE_LEVEL_HIGH>;
clocks = <&k3_clks 136 1>;
+ status = "disabled"; /* Used by OP-TEE */
};
};

--
2.36.1


2022-07-07 15:13:43

by Bryan Brattlof

[permalink] [raw]
Subject: Re: [PATCH v2 1/4] arm64: dts: ti: k3-am65-main: Disable RNG node

Hi Andrew

On July 6, 2022 thus sayeth Andrew Davis:
> The hardware random number generator is used by OP-TEE and is access is
> denied to other users with SoC level bus firewalls. Any access to this
> device from Linux will result in firewall errors. Disable this node.
>
> Signed-off-by: Andrew Davis <[email protected]>
> ---
>
> Changes from v1:
> - Added comment in dtsi file
>
> arch/arm64/boot/dts/ti/k3-am65-main.dtsi | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/arch/arm64/boot/dts/ti/k3-am65-main.dtsi b/arch/arm64/boot/dts/ti/k3-am65-main.dtsi
> index e749343accedd..9de5a8294acd6 100644
> --- a/arch/arm64/boot/dts/ti/k3-am65-main.dtsi
> +++ b/arch/arm64/boot/dts/ti/k3-am65-main.dtsi
> @@ -127,6 +127,7 @@ rng: rng@4e10000 {
> reg = <0x0 0x4e10000 0x0 0x7d>;
> interrupts = <GIC_SPI 24 IRQ_TYPE_LEVEL_HIGH>;
> clocks = <&k3_clks 136 1>;
> + status = "disabled"; /* Used by OP-TEE */

Just curious about how we should document disabling nodes. I was
assuming the reasoning should be described in the bindings?

I would like to start disabling nodes by default in our dtsi files and
enabling them in our top dts file, making it easier for others to use
our device tree for a more focused purpose than our dev boards. I just
didn't know where I should document why I disabled the nodes :)

~Bryan

2022-07-07 15:31:12

by Andrew Davis

[permalink] [raw]
Subject: Re: [PATCH v2 1/4] arm64: dts: ti: k3-am65-main: Disable RNG node

On 7/7/22 9:44 AM, Bryan Brattlof wrote:
> Hi Andrew
>
> On July 6, 2022 thus sayeth Andrew Davis:
>> The hardware random number generator is used by OP-TEE and is access is
>> denied to other users with SoC level bus firewalls. Any access to this
>> device from Linux will result in firewall errors. Disable this node.
>>
>> Signed-off-by: Andrew Davis <[email protected]>
>> ---
>>
>> Changes from v1:
>> - Added comment in dtsi file
>>
>> arch/arm64/boot/dts/ti/k3-am65-main.dtsi | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/arch/arm64/boot/dts/ti/k3-am65-main.dtsi b/arch/arm64/boot/dts/ti/k3-am65-main.dtsi
>> index e749343accedd..9de5a8294acd6 100644
>> --- a/arch/arm64/boot/dts/ti/k3-am65-main.dtsi
>> +++ b/arch/arm64/boot/dts/ti/k3-am65-main.dtsi
>> @@ -127,6 +127,7 @@ rng: rng@4e10000 {
>> reg = <0x0 0x4e10000 0x0 0x7d>;
>> interrupts = <GIC_SPI 24 IRQ_TYPE_LEVEL_HIGH>;
>> clocks = <&k3_clks 136 1>;
>> + status = "disabled"; /* Used by OP-TEE */
>
> Just curious about how we should document disabling nodes. I was
> assuming the reasoning should be described in the bindings?
>
> I would like to start disabling nodes by default in our dtsi files and
> enabling them in our top dts file, making it easier for others to use
> our device tree for a more focused purpose than our dev boards. I just
> didn't know where I should document why I disabled the nodes :)
>

This got push-back last time this was suggested, I'll have to lookup
the history. IIRC we landed on the other way around, all things should
be described by default, then the specific board can enable/disable
what is not used as needed.

I was worried this topic would come up with this patch series and was
almost just going to delete the whole RNG node instead of disabling it
to avoid that. My reasoning for disabling here anyway is that this device
*cannot* be used by *any* board, it is not just a board level configuration
decision like disabling I2C nodes by default or similar that was proposed
last time we had the "nodes disabled by default" discussion.

Andrew

> ~Bryan

2022-07-07 17:38:47

by Andrew Davis

[permalink] [raw]
Subject: Re: [PATCH v2 1/4] arm64: dts: ti: k3-am65-main: Disable RNG node

On 7/7/22 12:17 PM, Nishanth Menon wrote:
> On 10:01-20220707, Andrew Davis wrote:
>> On 7/7/22 9:44 AM, Bryan Brattlof wrote:
>>> Hi Andrew
>>>
>>> On July 6, 2022 thus sayeth Andrew Davis:
>>>> The hardware random number generator is used by OP-TEE and is access is
>>>> denied to other users with SoC level bus firewalls. Any access to this
>>>> device from Linux will result in firewall errors. Disable this node.
>>>>
>>>> Signed-off-by: Andrew Davis <[email protected]>
>>>> ---
>>>>
>>>> Changes from v1:
>>>> - Added comment in dtsi file
>>>>
>>>> arch/arm64/boot/dts/ti/k3-am65-main.dtsi | 1 +
>>>> 1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/arch/arm64/boot/dts/ti/k3-am65-main.dtsi b/arch/arm64/boot/dts/ti/k3-am65-main.dtsi
>>>> index e749343accedd..9de5a8294acd6 100644
>>>> --- a/arch/arm64/boot/dts/ti/k3-am65-main.dtsi
>>>> +++ b/arch/arm64/boot/dts/ti/k3-am65-main.dtsi
>>>> @@ -127,6 +127,7 @@ rng: rng@4e10000 {
>>>> reg = <0x0 0x4e10000 0x0 0x7d>;
>>>> interrupts = <GIC_SPI 24 IRQ_TYPE_LEVEL_HIGH>;
>>>> clocks = <&k3_clks 136 1>;
>>>> + status = "disabled"; /* Used by OP-TEE */
>>>
>>> Just curious about how we should document disabling nodes. I was
>>> assuming the reasoning should be described in the bindings?
>>>
>>> I would like to start disabling nodes by default in our dtsi files and
>>> enabling them in our top dts file, making it easier for others to use
>>> our device tree for a more focused purpose than our dev boards. I just
>>> didn't know where I should document why I disabled the nodes :)
>>>
>>
>> This got push-back last time this was suggested, I'll have to lookup
>> the history. IIRC we landed on the other way around, all things should
>> be described by default, then the specific board can enable/disable
>> what is not used as needed.
>
> See thread https://lore.kernel.org/linux-arm-kernel/[email protected]/
>
>>
>> I was worried this topic would come up with this patch series and was
>> almost just going to delete the whole RNG node instead of disabling it
>> to avoid that. My reasoning for disabling here anyway is that this device
>> *cannot* be used by *any* board, it is not just a board level configuration
>> decision like disabling I2C nodes by default or similar that was proposed
>> last time we had the "nodes disabled by default" discussion.
>
> Hmm.. If that is the case, then why even have it in dts - is that
> because of cases where OPTEE is'nt the TEE and users may want to
> directly use it? OR is it because OPTEE can potentially use device tree
> itself and discover the rng location from dt?
>

Hadn't thought about the second case, but it could also be valid if we
started using DT in our OPTEE. The intention was for the first case,
this node has valid hardware description, didn't feel right to delete
it due to it being unusable by Linux.

Andrew

2022-07-07 18:03:47

by Nishanth Menon

[permalink] [raw]
Subject: Re: [PATCH v2 1/4] arm64: dts: ti: k3-am65-main: Disable RNG node

On 10:01-20220707, Andrew Davis wrote:
> On 7/7/22 9:44 AM, Bryan Brattlof wrote:
> > Hi Andrew
> >
> > On July 6, 2022 thus sayeth Andrew Davis:
> > > The hardware random number generator is used by OP-TEE and is access is
> > > denied to other users with SoC level bus firewalls. Any access to this
> > > device from Linux will result in firewall errors. Disable this node.
> > >
> > > Signed-off-by: Andrew Davis <[email protected]>
> > > ---
> > >
> > > Changes from v1:
> > > - Added comment in dtsi file
> > >
> > > arch/arm64/boot/dts/ti/k3-am65-main.dtsi | 1 +
> > > 1 file changed, 1 insertion(+)
> > >
> > > diff --git a/arch/arm64/boot/dts/ti/k3-am65-main.dtsi b/arch/arm64/boot/dts/ti/k3-am65-main.dtsi
> > > index e749343accedd..9de5a8294acd6 100644
> > > --- a/arch/arm64/boot/dts/ti/k3-am65-main.dtsi
> > > +++ b/arch/arm64/boot/dts/ti/k3-am65-main.dtsi
> > > @@ -127,6 +127,7 @@ rng: rng@4e10000 {
> > > reg = <0x0 0x4e10000 0x0 0x7d>;
> > > interrupts = <GIC_SPI 24 IRQ_TYPE_LEVEL_HIGH>;
> > > clocks = <&k3_clks 136 1>;
> > > + status = "disabled"; /* Used by OP-TEE */
> >
> > Just curious about how we should document disabling nodes. I was
> > assuming the reasoning should be described in the bindings?
> >
> > I would like to start disabling nodes by default in our dtsi files and
> > enabling them in our top dts file, making it easier for others to use
> > our device tree for a more focused purpose than our dev boards. I just
> > didn't know where I should document why I disabled the nodes :)
> >
>
> This got push-back last time this was suggested, I'll have to lookup
> the history. IIRC we landed on the other way around, all things should
> be described by default, then the specific board can enable/disable
> what is not used as needed.

See thread https://lore.kernel.org/linux-arm-kernel/[email protected]/

>
> I was worried this topic would come up with this patch series and was
> almost just going to delete the whole RNG node instead of disabling it
> to avoid that. My reasoning for disabling here anyway is that this device
> *cannot* be used by *any* board, it is not just a board level configuration
> decision like disabling I2C nodes by default or similar that was proposed
> last time we had the "nodes disabled by default" discussion.

Hmm.. If that is the case, then why even have it in dts - is that
because of cases where OPTEE is'nt the TEE and users may want to
directly use it? OR is it because OPTEE can potentially use device tree
itself and discover the rng location from dt?


--
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3 1A34 DDB5 849D 1736 249D

2022-07-08 22:23:38

by Nishanth Menon

[permalink] [raw]
Subject: Re: [PATCH v2 1/4] arm64: dts: ti: k3-am65-main: Disable RNG node

On 12:36-20220707, Andrew Davis wrote:
> On 7/7/22 12:17 PM, Nishanth Menon wrote:
> > On 10:01-20220707, Andrew Davis wrote:
> > > On 7/7/22 9:44 AM, Bryan Brattlof wrote:
> > > > Hi Andrew
> > > >
> > > > On July 6, 2022 thus sayeth Andrew Davis:
> > > > > The hardware random number generator is used by OP-TEE and is access is
> > > > > denied to other users with SoC level bus firewalls. Any access to this
> > > > > device from Linux will result in firewall errors. Disable this node.
> > > > >
> > > > > Signed-off-by: Andrew Davis <[email protected]>
> > > > > ---
> > > > >
> > > > > Changes from v1:
> > > > > - Added comment in dtsi file
> > > > >
> > > > > arch/arm64/boot/dts/ti/k3-am65-main.dtsi | 1 +
> > > > > 1 file changed, 1 insertion(+)
> > > > >
> > > > > diff --git a/arch/arm64/boot/dts/ti/k3-am65-main.dtsi b/arch/arm64/boot/dts/ti/k3-am65-main.dtsi
> > > > > index e749343accedd..9de5a8294acd6 100644
> > > > > --- a/arch/arm64/boot/dts/ti/k3-am65-main.dtsi
> > > > > +++ b/arch/arm64/boot/dts/ti/k3-am65-main.dtsi
> > > > > @@ -127,6 +127,7 @@ rng: rng@4e10000 {
> > > > > reg = <0x0 0x4e10000 0x0 0x7d>;
> > > > > interrupts = <GIC_SPI 24 IRQ_TYPE_LEVEL_HIGH>;
> > > > > clocks = <&k3_clks 136 1>;
> > > > > + status = "disabled"; /* Used by OP-TEE */
> > > >
> > > > Just curious about how we should document disabling nodes. I was
> > > > assuming the reasoning should be described in the bindings?
> > > >
> > > > I would like to start disabling nodes by default in our dtsi files and
> > > > enabling them in our top dts file, making it easier for others to use
> > > > our device tree for a more focused purpose than our dev boards. I just
> > > > didn't know where I should document why I disabled the nodes :)
> > > >
> > >
> > > This got push-back last time this was suggested, I'll have to lookup
> > > the history. IIRC we landed on the other way around, all things should
> > > be described by default, then the specific board can enable/disable
> > > what is not used as needed.
> >
> > See thread https://lore.kernel.org/linux-arm-kernel/[email protected]/
> >
> > >
> > > I was worried this topic would come up with this patch series and was
> > > almost just going to delete the whole RNG node instead of disabling it
> > > to avoid that. My reasoning for disabling here anyway is that this device
> > > *cannot* be used by *any* board, it is not just a board level configuration
> > > decision like disabling I2C nodes by default or similar that was proposed
> > > last time we had the "nodes disabled by default" discussion.
> >
> > Hmm.. If that is the case, then why even have it in dts - is that
> > because of cases where OPTEE is'nt the TEE and users may want to
> > directly use it? OR is it because OPTEE can potentially use device tree
> > itself and discover the rng location from dt?
> >
>
> Hadn't thought about the second case, but it could also be valid if we
> started using DT in our OPTEE. The intention was for the first case,
> this node has valid hardware description, didn't feel right to delete
> it due to it being unusable by Linux.


Fair enough, it is a bit late for me to let this series cook in list and
then apply and wait for linux-next stability before I tag for next
kernel, I suggest improving the commit message with rationale in the
next rev.

--
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3 1A34 DDB5 849D 1736 249D