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@xxxxxxxxxxx> > > > > > --- > > > > > > > > > > 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
