mbox series

[v6,00/11] Introduce STM32 Firewall framework

Message ID 20231010125719.784627-1-gatien.chevallier@foss.st.com
Headers show
Series Introduce STM32 Firewall framework | expand

Message

Gatien Chevallier Oct. 10, 2023, 12:57 p.m. UTC
Introduce STM32 Firewall framework for STM32MP1x and STM32MP2x
platforms. STM32MP1x(ETZPC) and STM32MP2x(RIFSC) Firewall controllers
register to the framework to offer firewall services such as access
granting.

This series of patches is a new approach on the previous STM32 system
bus, history is available here:
https://lore.kernel.org/lkml/20230127164040.1047583/

The need for such framework arises from the fact that there are now
multiple hardware firewalls implemented across multiple products.
Drivers are shared between different products, using the same code.
When it comes to firewalls, the purpose mostly stays the same: Protect
hardware resources. But the implementation differs, and there are
multiple types of firewalls: peripheral, memory, ... 

Some hardware firewall controllers such as the RIFSC implemented on
STM32MP2x platforms may require to take ownership of a resource before
being able to use it, hence the requirement for firewall services to
take/release the ownership of such resources.

On the other hand, hardware firewall configurations are becoming
more and more complex. These mecanisms prevent platform crashes
or other firewall-related incoveniences by denying access to some
resources.

The stm32 firewall framework offers an API that is defined in
firewall controllers drivers to best fit the specificity of each
firewall.

For every peripherals protected by either the ETZPC or the RIFSC, the
firewall framework checks the firewall controlelr registers to see if
the peripheral's access is granted to the Linux kernel. If not, the
peripheral is configured as secure, the node is marked populated,
so that the driver is not probed for that device.

The firewall framework relies on the access-controller device tree
binding. It is used by peripherals to reference a domain access
controller. In this case a firewall controller. The bus uses the ID
referenced by the access-controller property to know where to look
in the firewall to get the security configuration for the peripheral.
This allows a device tree description rather than a hardcoded peripheral
table in the bus driver.

The STM32 ETZPC device is responsible for filtering accesses based on
security level, or co-processor isolation for any resource connected
to it.

The RIFSC is responsible for filtering accesses based on Compartment
ID / security level / privilege level for any resource connected to
it.

STM32MP13/15/25 SoC device tree files are updated in this series to
implement this mecanism.

Changes in V6:
	- Rename access-controller to access-controllers
	- Remove access-controller-provider
	- Update device trees and other bindings accordingly
	- Rework ETZPC/RIFSC bindings to define what access-controllers
	  cells contain inside #access-controller-cells
	- Some other minor fixes

Changes in V5:
	- Integrate and rework the "feature-domains" binding patch in
	  this patchset. The binding is renamed to "access-controller"
	- Rename every feature-domain* reference to access-control*
	  ones
	- Correct loop bug and missing select STM32_FIREWALL in 32-bit
	  platform Kconfig
	

Changes in V4:
	- Fix typo in commit message and YAML check errors in
	  "dt-bindings: Document common device controller bindings"
	  Note: This patch should be ignored as stated in the cover
	  letter. I've done this to avoid errors on this series of
	  patch
	- Correct code syntax/style issues reported by Simon Horman
	- Added Jonathan's tag for IIO on the treewide patch

Changes in V3:

	Change incorrect ordering for bindings commits leading
	to an error while running
	"make DT_CHECKER_FLAGS=-m dt_binding_check"

Changes in V2:

	generic:
		- Add fw_devlink dependency for "feature-domains"
		  property.

	bindings:
		- Corrected YAMLS errors highlighted by Rob's robot
		- Firewall controllers YAMLs no longer define the
		  maxItems for the "feature-domains" property
		- Renamed st,stm32-rifsc.yaml to
		  st,stm32mp25-rifsc.yaml
		- Fix examples in YAML files
		- Change feature-domains maxItems to 2 in firewall
		  consumer files as there should not be more than
		  2 entries for now
		- Declare "feature-domain-names" as an optional
		  property for firewall controllers child nodes.
		- Add missing "feature-domains" property declaration
		  in bosch,m_can.yaml and st,stm32-cryp.yaml files

	firewall framework:
		- Support multiple entries for "feature-domains"
		  property
		- Better handle the device-tree parsing using
		  phandle+args APIs
		- Remove "resource firewall" type
		- Add a field for the name of the firewall entry
		- Fix licenses
	
	RIFSC:
		- Add controller name
		- Driver is now a module_platform_driver
		- Fix license

	ETZPC:
		- Add controller name
		- Driver is now a module_platform_driver
		- Fix license

	Device trees:
		- Fix rifsc node name
		- Move the "ranges" property under the
		  "feature-domains" one
Gatien Chevallier (10):
  dt-bindings: treewide: add access-controllers description
  dt-bindings: bus: document RIFSC
  dt-bindings: bus: document ETZPC
  firewall: introduce stm32_firewall framework
  of: property: fw_devlink: Add support for "access-controller"
  bus: rifsc: introduce RIFSC firewall controller driver
  arm64: dts: st: add RIFSC as an access controller for STM32MP25x
    boards
  bus: etzpc: introduce ETZPC firewall controller driver
  ARM: dts: stm32: add ETZPC as a system bus for STM32MP15x boards
  ARM: dts: stm32: add ETZPC as a system bus for STM32MP13x boards

Oleksii Moisieiev (1):
  dt-bindings: document generic access controllers

 .../access-controllers.yaml                   |   84 +
 .../bindings/bus/st,stm32-etzpc.yaml          |   87 +
 .../bindings/bus/st,stm32mp25-rifsc.yaml      |   96 +
 .../bindings/crypto/st,stm32-cryp.yaml        |    4 +
 .../bindings/crypto/st,stm32-hash.yaml        |    4 +
 .../devicetree/bindings/dma/st,stm32-dma.yaml |    4 +
 .../bindings/dma/st,stm32-dmamux.yaml         |    4 +
 .../devicetree/bindings/i2c/st,stm32-i2c.yaml |    4 +
 .../bindings/iio/adc/st,stm32-adc.yaml        |    4 +
 .../bindings/iio/adc/st,stm32-dfsdm-adc.yaml  |    4 +
 .../bindings/iio/dac/st,stm32-dac.yaml        |    4 +
 .../bindings/media/cec/st,stm32-cec.yaml      |    4 +
 .../bindings/media/st,stm32-dcmi.yaml         |    4 +
 .../memory-controllers/st,stm32-fmc2-ebi.yaml |    4 +
 .../bindings/mfd/st,stm32-lptimer.yaml        |    4 +
 .../bindings/mfd/st,stm32-timers.yaml         |    4 +
 .../devicetree/bindings/mmc/arm,pl18x.yaml    |    4 +
 .../bindings/net/can/bosch,m_can.yaml         |    4 +
 .../devicetree/bindings/net/stm32-dwmac.yaml  |    4 +
 .../bindings/phy/phy-stm32-usbphyc.yaml       |    4 +
 .../bindings/regulator/st,stm32-vrefbuf.yaml  |    4 +
 .../devicetree/bindings/rng/st,stm32-rng.yaml |    4 +
 .../bindings/serial/st,stm32-uart.yaml        |    4 +
 .../bindings/sound/st,stm32-i2s.yaml          |    4 +
 .../bindings/sound/st,stm32-sai.yaml          |    4 +
 .../bindings/sound/st,stm32-spdifrx.yaml      |    4 +
 .../bindings/spi/st,stm32-qspi.yaml           |    4 +
 .../devicetree/bindings/spi/st,stm32-spi.yaml |    4 +
 .../devicetree/bindings/usb/dwc2.yaml         |    4 +
 MAINTAINERS                                   |    7 +
 arch/arm/boot/dts/st/stm32mp131.dtsi          | 1025 +++---
 arch/arm/boot/dts/st/stm32mp133.dtsi          |   51 +-
 arch/arm/boot/dts/st/stm32mp13xc.dtsi         |   19 +-
 arch/arm/boot/dts/st/stm32mp13xf.dtsi         |   19 +-
 arch/arm/boot/dts/st/stm32mp151.dtsi          | 2756 +++++++++--------
 arch/arm/boot/dts/st/stm32mp153.dtsi          |   52 +-
 arch/arm/boot/dts/st/stm32mp15xc.dtsi         |   19 +-
 arch/arm/mach-stm32/Kconfig                   |    1 +
 arch/arm64/Kconfig.platforms                  |    1 +
 arch/arm64/boot/dts/st/stm32mp251.dtsi        |    6 +-
 drivers/bus/Kconfig                           |    9 +
 drivers/bus/Makefile                          |    1 +
 drivers/bus/stm32_etzpc.c                     |  141 +
 drivers/bus/stm32_firewall.c                  |  294 ++
 drivers/bus/stm32_firewall.h                  |   83 +
 drivers/bus/stm32_rifsc.c                     |  252 ++
 drivers/of/property.c                         |    2 +
 include/linux/bus/stm32_firewall_device.h     |  141 +
 48 files changed, 3331 insertions(+), 1919 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/access-controllers/access-controllers.yaml
 create mode 100644 Documentation/devicetree/bindings/bus/st,stm32-etzpc.yaml
 create mode 100644 Documentation/devicetree/bindings/bus/st,stm32mp25-rifsc.yaml
 create mode 100644 drivers/bus/stm32_etzpc.c
 create mode 100644 drivers/bus/stm32_firewall.c
 create mode 100644 drivers/bus/stm32_firewall.h
 create mode 100644 drivers/bus/stm32_rifsc.c
 create mode 100644 include/linux/bus/stm32_firewall_device.h

Comments

Gatien Chevallier Oct. 11, 2023, 8:49 a.m. UTC | #1
Hi Rob,

On 10/10/23 20:42, Rob Herring wrote:
> On Tue, Oct 10, 2023 at 02:57:18PM +0200, Gatien Chevallier wrote:
>> ETZPC is a firewall controller. Put all peripherals filtered by the
>> ETZPC as ETZPC subnodes and reference ETZPC as an
>> access-control-provider.
>>
>> For more information on which peripheral is securable or supports MCU
>> isolation, please read the STM32MP15 reference manual.
>>
>> Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com>
>> ---
>>
>> Changes in V6:
>>      	- Renamed access-controller to access-controllers
>>      	- Removal of access-control-provider property
>>
>> Changes in V5:
>>      	- Renamed feature-domain* to access-control*
>>
>>   arch/arm/boot/dts/st/stm32mp151.dtsi  | 2756 +++++++++++++------------
>>   arch/arm/boot/dts/st/stm32mp153.dtsi  |   52 +-
>>   arch/arm/boot/dts/st/stm32mp15xc.dtsi |   19 +-
>>   3 files changed, 1450 insertions(+), 1377 deletions(-)
> 
> This is not reviewable. Change the indentation and any non-functional
> change in one patch and then actual changes in another.

Ok, I'll make it easier to read.

> 
> This is also an ABI break. Though I'm not sure it's avoidable. All the
> devices below the ETZPC node won't probe on existing kernel. A
> simple-bus fallback for ETZPC node should solve that.
> 

I had one issue when trying with a simple-bus fallback that was the
drivers were probing even though the access rights aren't correct.
Hence the removal of the simple-bus compatible in the STM32MP25 patch.

Even though a node is tagged with the OF_POPULATED flag when checking
the access rights with the firewall controller, it seems that when
simple-bus is probing, there's no check of this flag.

of_platform_populate() checks and sets the OF_POPULATED_BUS flag.
Maybe that is my error and the firewall bus populate should set
OF_POPULATED_BUS instead of OF_POPULATED. Is that correct?

Best regards,
Gatien

> Rob
Rob Herring Oct. 12, 2023, 3:30 p.m. UTC | #2
On Wed, Oct 11, 2023 at 10:49:58AM +0200, Gatien CHEVALLIER wrote:
> Hi Rob,
> 
> On 10/10/23 20:42, Rob Herring wrote:
> > On Tue, Oct 10, 2023 at 02:57:18PM +0200, Gatien Chevallier wrote:
> > > ETZPC is a firewall controller. Put all peripherals filtered by the
> > > ETZPC as ETZPC subnodes and reference ETZPC as an
> > > access-control-provider.
> > > 
> > > For more information on which peripheral is securable or supports MCU
> > > isolation, please read the STM32MP15 reference manual.
> > > 
> > > Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com>
> > > ---
> > > 
> > > Changes in V6:
> > >      	- Renamed access-controller to access-controllers
> > >      	- Removal of access-control-provider property
> > > 
> > > Changes in V5:
> > >      	- Renamed feature-domain* to access-control*
> > > 
> > >   arch/arm/boot/dts/st/stm32mp151.dtsi  | 2756 +++++++++++++------------
> > >   arch/arm/boot/dts/st/stm32mp153.dtsi  |   52 +-
> > >   arch/arm/boot/dts/st/stm32mp15xc.dtsi |   19 +-
> > >   3 files changed, 1450 insertions(+), 1377 deletions(-)
> > 
> > This is not reviewable. Change the indentation and any non-functional
> > change in one patch and then actual changes in another.
> 
> Ok, I'll make it easier to read.
> 
> > 
> > This is also an ABI break. Though I'm not sure it's avoidable. All the
> > devices below the ETZPC node won't probe on existing kernel. A
> > simple-bus fallback for ETZPC node should solve that.
> > 
> 
> I had one issue when trying with a simple-bus fallback that was the
> drivers were probing even though the access rights aren't correct.
> Hence the removal of the simple-bus compatible in the STM32MP25 patch.

But it worked before, right? So the difference is you have either added 
new devices which need setup or your firmware changed how devices are 
setup (or not setup). Certainly can't fix the latter case. You just need 
to be explicit about what you are doing to users.


> Even though a node is tagged with the OF_POPULATED flag when checking
> the access rights with the firewall controller, it seems that when
> simple-bus is probing, there's no check of this flag.

It shouldn't. Those flags are for creating the devices (or not) and 
removing only devices of_platform_populate() created.

> of_platform_populate() checks and sets the OF_POPULATED_BUS flag.
> Maybe that is my error and the firewall bus populate should set
> OF_POPULATED_BUS instead of OF_POPULATED. Is that correct?

Shrug. Off hand, I'd say probably not, but am not certain.

Rob
Gatien Chevallier Oct. 16, 2023, 12:02 p.m. UTC | #3
Hi Rob,

On 10/12/23 17:30, Rob Herring wrote:
> On Wed, Oct 11, 2023 at 10:49:58AM +0200, Gatien CHEVALLIER wrote:
>> Hi Rob,
>>
>> On 10/10/23 20:42, Rob Herring wrote:
>>> On Tue, Oct 10, 2023 at 02:57:18PM +0200, Gatien Chevallier wrote:
>>>> ETZPC is a firewall controller. Put all peripherals filtered by the
>>>> ETZPC as ETZPC subnodes and reference ETZPC as an
>>>> access-control-provider.
>>>>
>>>> For more information on which peripheral is securable or supports MCU
>>>> isolation, please read the STM32MP15 reference manual.
>>>>
>>>> Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com>
>>>> ---
>>>>
>>>> Changes in V6:
>>>>       	- Renamed access-controller to access-controllers
>>>>       	- Removal of access-control-provider property
>>>>
>>>> Changes in V5:
>>>>       	- Renamed feature-domain* to access-control*
>>>>
>>>>    arch/arm/boot/dts/st/stm32mp151.dtsi  | 2756 +++++++++++++------------
>>>>    arch/arm/boot/dts/st/stm32mp153.dtsi  |   52 +-
>>>>    arch/arm/boot/dts/st/stm32mp15xc.dtsi |   19 +-
>>>>    3 files changed, 1450 insertions(+), 1377 deletions(-)
>>>
>>> This is not reviewable. Change the indentation and any non-functional
>>> change in one patch and then actual changes in another.
>>
>> Ok, I'll make it easier to read.
>>
>>>
>>> This is also an ABI break. Though I'm not sure it's avoidable. All the
>>> devices below the ETZPC node won't probe on existing kernel. A
>>> simple-bus fallback for ETZPC node should solve that.
>>>
>>
>> I had one issue when trying with a simple-bus fallback that was the
>> drivers were probing even though the access rights aren't correct.
>> Hence the removal of the simple-bus compatible in the STM32MP25 patch.
> 
> But it worked before, right? So the difference is you have either added
> new devices which need setup or your firmware changed how devices are
> setup (or not setup). Certainly can't fix the latter case. You just need
> to be explicit about what you are doing to users.
> 

I should've specified it was during a test where I deliberately set
incorrect rights on a peripheral and enabled its node to see if the
firewall would allow the creation of the device.

> 
>> Even though a node is tagged with the OF_POPULATED flag when checking
>> the access rights with the firewall controller, it seems that when
>> simple-bus is probing, there's no check of this flag.
> 
> It shouldn't. Those flags are for creating the devices (or not) and
> removing only devices of_platform_populate() created.
> 

About the "simple-bus" being a fallback, I think I understood why I saw
that the devices were created.

All devices under a node whose compatible is "simple-bus" are created
in of_platform_device_create_pdata(), called by
of_platform_default_populate_init() at arch_initcall level. This
before the firewall-controller has a chance to populate it's bus.

Therefore, when I flag nodes when populating the firewall-bus, the
devices are already created. The "simple-bus" mechanism is not a
fallback here as it precedes the driver probe.

Is there a safe way to safely remove/disable a device created this way?
Devices that are under the firewall controller (simple-bus) node
should not be probed before it as they're child of it.

Best regards,
Gatien

>> of_platform_populate() checks and sets the OF_POPULATED_BUS flag.
>> Maybe that is my error and the firewall bus populate should set
>> OF_POPULATED_BUS instead of OF_POPULATED. Is that correct?
> 
> Shrug. Off hand, I'd say probably not, but am not certain.
> 
> Rob
Rob Herring Oct. 24, 2023, 4:39 p.m. UTC | #4
On Mon, Oct 16, 2023 at 02:02:39PM +0200, Gatien CHEVALLIER wrote:
> Hi Rob,
> 
> On 10/12/23 17:30, Rob Herring wrote:
> > On Wed, Oct 11, 2023 at 10:49:58AM +0200, Gatien CHEVALLIER wrote:
> > > Hi Rob,
> > > 
> > > On 10/10/23 20:42, Rob Herring wrote:
> > > > On Tue, Oct 10, 2023 at 02:57:18PM +0200, Gatien Chevallier wrote:
> > > > > ETZPC is a firewall controller. Put all peripherals filtered by the
> > > > > ETZPC as ETZPC subnodes and reference ETZPC as an
> > > > > access-control-provider.
> > > > > 
> > > > > For more information on which peripheral is securable or supports MCU
> > > > > isolation, please read the STM32MP15 reference manual.
> > > > > 
> > > > > Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com>
> > > > > ---
> > > > > 
> > > > > Changes in V6:
> > > > >       	- Renamed access-controller to access-controllers
> > > > >       	- Removal of access-control-provider property
> > > > > 
> > > > > Changes in V5:
> > > > >       	- Renamed feature-domain* to access-control*
> > > > > 
> > > > >    arch/arm/boot/dts/st/stm32mp151.dtsi  | 2756 +++++++++++++------------
> > > > >    arch/arm/boot/dts/st/stm32mp153.dtsi  |   52 +-
> > > > >    arch/arm/boot/dts/st/stm32mp15xc.dtsi |   19 +-
> > > > >    3 files changed, 1450 insertions(+), 1377 deletions(-)
> > > > 
> > > > This is not reviewable. Change the indentation and any non-functional
> > > > change in one patch and then actual changes in another.
> > > 
> > > Ok, I'll make it easier to read.
> > > 
> > > > 
> > > > This is also an ABI break. Though I'm not sure it's avoidable. All the
> > > > devices below the ETZPC node won't probe on existing kernel. A
> > > > simple-bus fallback for ETZPC node should solve that.
> > > > 
> > > 
> > > I had one issue when trying with a simple-bus fallback that was the
> > > drivers were probing even though the access rights aren't correct.
> > > Hence the removal of the simple-bus compatible in the STM32MP25 patch.
> > 
> > But it worked before, right? So the difference is you have either added
> > new devices which need setup or your firmware changed how devices are
> > setup (or not setup). Certainly can't fix the latter case. You just need
> > to be explicit about what you are doing to users.
> > 
> 
> I should've specified it was during a test where I deliberately set
> incorrect rights on a peripheral and enabled its node to see if the
> firewall would allow the creation of the device.
> 
> > 
> > > Even though a node is tagged with the OF_POPULATED flag when checking
> > > the access rights with the firewall controller, it seems that when
> > > simple-bus is probing, there's no check of this flag.
> > 
> > It shouldn't. Those flags are for creating the devices (or not) and
> > removing only devices of_platform_populate() created.
> > 
> 
> About the "simple-bus" being a fallback, I think I understood why I saw
> that the devices were created.
> 
> All devices under a node whose compatible is "simple-bus" are created
> in of_platform_device_create_pdata(), called by
> of_platform_default_populate_init() at arch_initcall level. This
> before the firewall-controller has a chance to populate it's bus.
> 
> Therefore, when I flag nodes when populating the firewall-bus, the
> devices are already created. The "simple-bus" mechanism is not a
> fallback here as it precedes the driver probe.
> 
> Is there a safe way to safely remove/disable a device created this way?

There's 2 ways to handle this. Either controlling creating the device or 
controlling probing the device. The latter should just work with 
fw_devlink dependency. The former probably needs some adjustment to 
simple-pm-bus driver if you have 'simple-bus' compatible. You want it to 
probe on old kernels and not probe on new kernels with your firewall 
driver. Look at the commit history for simple-pm-bus. There was some 
discussion on it as well.

> Devices that are under the firewall controller (simple-bus) node
> should not be probed before it as they're child of it.

fw_devlink should take care of parent/child dependencies without any 
explicit handling of the access ctrl binding.

Rob
Gatien Chevallier Nov. 27, 2023, 1:46 p.m. UTC | #5
Hi,

A gentle reminder on the questions below.

I'm also thinking about moving the STM32 firewall framework to a
specific access-controllers folder if that's ok.

Best regards,
Gatien

On 10/27/23 17:37, Gatien CHEVALLIER wrote:
> 
> 
> On 10/24/23 18:39, Rob Herring wrote:
>> On Mon, Oct 16, 2023 at 02:02:39PM +0200, Gatien CHEVALLIER wrote:
>>> Hi Rob,
>>>
>>> On 10/12/23 17:30, Rob Herring wrote:
>>>> On Wed, Oct 11, 2023 at 10:49:58AM +0200, Gatien CHEVALLIER wrote:
>>>>> Hi Rob,
>>>>>
>>>>> On 10/10/23 20:42, Rob Herring wrote:
>>>>>> On Tue, Oct 10, 2023 at 02:57:18PM +0200, Gatien Chevallier wrote:
>>>>>>> ETZPC is a firewall controller. Put all peripherals filtered by the
>>>>>>> ETZPC as ETZPC subnodes and reference ETZPC as an
>>>>>>> access-control-provider.
>>>>>>>
>>>>>>> For more information on which peripheral is securable or supports 
>>>>>>> MCU
>>>>>>> isolation, please read the STM32MP15 reference manual.
>>>>>>>
>>>>>>> Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com>
>>>>>>> ---
>>>>>>>
>>>>>>> Changes in V6:
>>>>>>>            - Renamed access-controller to access-controllers
>>>>>>>            - Removal of access-control-provider property
>>>>>>>
>>>>>>> Changes in V5:
>>>>>>>            - Renamed feature-domain* to access-control*
>>>>>>>
>>>>>>>     arch/arm/boot/dts/st/stm32mp151.dtsi  | 2756 
>>>>>>> +++++++++++++------------
>>>>>>>     arch/arm/boot/dts/st/stm32mp153.dtsi  |   52 +-
>>>>>>>     arch/arm/boot/dts/st/stm32mp15xc.dtsi |   19 +-
>>>>>>>     3 files changed, 1450 insertions(+), 1377 deletions(-)
>>>>>>
>>>>>> This is not reviewable. Change the indentation and any non-functional
>>>>>> change in one patch and then actual changes in another.
>>>>>
>>>>> Ok, I'll make it easier to read.
>>>>>
>>>>>>
>>>>>> This is also an ABI break. Though I'm not sure it's avoidable. All 
>>>>>> the
>>>>>> devices below the ETZPC node won't probe on existing kernel. A
>>>>>> simple-bus fallback for ETZPC node should solve that.
>>>>>>
>>>>>
>>>>> I had one issue when trying with a simple-bus fallback that was the
>>>>> drivers were probing even though the access rights aren't correct.
>>>>> Hence the removal of the simple-bus compatible in the STM32MP25 patch.
>>>>
>>>> But it worked before, right? So the difference is you have either added
>>>> new devices which need setup or your firmware changed how devices are
>>>> setup (or not setup). Certainly can't fix the latter case. You just 
>>>> need
>>>> to be explicit about what you are doing to users.
>>>>
>>>
>>> I should've specified it was during a test where I deliberately set
>>> incorrect rights on a peripheral and enabled its node to see if the
>>> firewall would allow the creation of the device.
>>>
>>>>
>>>>> Even though a node is tagged with the OF_POPULATED flag when checking
>>>>> the access rights with the firewall controller, it seems that when
>>>>> simple-bus is probing, there's no check of this flag.
>>>>
>>>> It shouldn't. Those flags are for creating the devices (or not) and
>>>> removing only devices of_platform_populate() created.
>>>>
>>>
>>> About the "simple-bus" being a fallback, I think I understood why I saw
>>> that the devices were created.
>>>
>>> All devices under a node whose compatible is "simple-bus" are created
>>> in of_platform_device_create_pdata(), called by
>>> of_platform_default_populate_init() at arch_initcall level. This
>>> before the firewall-controller has a chance to populate it's bus.
>>>
>>> Therefore, when I flag nodes when populating the firewall-bus, the
>>> devices are already created. The "simple-bus" mechanism is not a
>>> fallback here as it precedes the driver probe.
>>>
>>> Is there a safe way to safely remove/disable a device created this way?
>>
>> There's 2 ways to handle this. Either controlling creating the device or
>> controlling probing the device. The latter should just work with
>> fw_devlink dependency. The former probably needs some adjustment to
>> simple-pm-bus driver if you have 'simple-bus' compatible. You want it to
>> probe on old kernels and not probe on new kernels with your firewall
>> driver. Look at the commit history for simple-pm-bus. There was some
>> discussion on it as well.
>>
> 
> Hi Rob,
> 
> First, thank you for your suggestions.
> 
> Regarding controlling probing the device: the philosophy of the firewall
> controller was to check a device secure configuration to determine if
> its associated driver should be probed (+handle some firewall
> resources). I'd rather avoid it so that the device isn't created at all.
> 
> I took a look on the simple-bus driver side. I don't see an obvious way
> on how to do it as the firewall controller driver is a module while the
> devices being populated is done at arch initcall level.
> 
> I ended up with two propositions:
> 
> 1)I took a shot at implementing a new flag "OF_ACCESS_GRANTED" that
> should be set in the first call of the of_platform_bus_create()
> function for every child node of a "default bus" (simple-bus,
> simple-pm-bus, ...) having the access-controllers property.
> This flag should be unset by the access controller if the access is
> not granted. This covers the particular case where the access controller
> has a simple-bus fallback whilst not creating the devices on the first
> try for the bus' childs.
> 
> This way, the first round of of_platform_populate() done at arch init
> call level won't create the devices of an access controller child
> nodes. Then, the firewall controller has a chance to clear the flag
> before the second call to this function by the simple-pm-bus driver.
> 
> If the controller module isn't present, then it's a simple-bus
> behavior to extent of the child devices not being all created in the
> first place. This shouldn't be an issue as in only concerns childs
> of such bus that aren't probed before the bus driver.
> 
> I have a patch that I can send as RFC on top of my series if my
> explanation isn't clear enough.
> 
> 2)Make the STM32_FIREWALL configuration switch select the OF_DYNAMIC
> one. This way I can use of_detach_node() function to remove the node
> from the device tree. The cons of this is the device tree is now
> used at runtime.
> 
> Are you considering one of these two proposition as a viable solution?
> 
> Best regards,
> Gatien
> 
>>> Devices that are under the firewall controller (simple-bus) node
>>> should not be probed before it as they're child of it.
>>
>> fw_devlink should take care of parent/child dependencies without any
>> explicit handling of the access ctrl binding.
>>
>> Rob