| Message ID | 20230725164104.273965-1-gatien.chevallier@foss.st.com |
|---|---|
| Headers | show |
| Series | Introduce STM32 Firewall framework | expand |
On Tue, 25 Jul 2023 18:40:56 +0200, Gatien Chevallier wrote: > Document ETZPC (Extended TrustZone protection controller). ETZPC is a > firewall controller. > > Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com> > --- > > Changes in V2: > - Corrected errors highlighted by Rob's robot > - No longer define the maxItems for the "feature-domains" > property > - Fix example (node name, status) > - Declare "feature-domain-names" as an optional > property for child nodes > - Fix description of "feature-domains" property > - Reorder the properties so it matches RIFSC > - Add missing "feature-domain-controller" property > > .../bindings/bus/st,stm32-etzpc.yaml | 96 +++++++++++++++++++ > 1 file changed, 96 insertions(+) > create mode 100644 Documentation/devicetree/bindings/bus/st,stm32-etzpc.yaml > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: dtschema/dtc warnings/errors: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/bus/st,stm32-etzpc.example.dtb: serial@4c001000: Unevaluated properties are not allowed ('feature-domains' was unexpected) from schema $id: http://devicetree.org/schemas/serial/st,stm32-uart.yaml# doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230725164104.273965-4-gatien.chevallier@foss.st.com The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.
On Tue, Jul 25, 2023 at 06:40:58PM +0200, Gatien Chevallier wrote: ... > diff --git a/drivers/bus/stm32_firewall.c b/drivers/bus/stm32_firewall.c ... > +int stm32_firewall_controller_register(struct stm32_firewall_controller *firewall_controller) > +{ > + struct stm32_firewall_controller *ctrl; > + > + pr_info("Registering %s firewall controller\n", firewall_controller->name); > + > + if (!firewall_controller) > + return -ENODEV; HI Gatien, Sorry, one more on this patch, that I missed before sending my previous email. firewall_controller is checked for NULL here. But it is already dereferenced on the line above the check. Flagged by Smatch. ...
On 7/26/23 12:19, Simon Horman wrote: > On Tue, Jul 25, 2023 at 06:40:58PM +0200, Gatien Chevallier wrote: >> Introduce a STM32 firewall framework that offers to firewall consumers >> different firewall services such as the ability to check their access >> rights against their firewall controller(s). >> >> The STM32 firewall framework offers a generic API for STM32 firewall >> controllers that is defined in their drivers to best fit the >> specificity of each firewall. >> >> There are various types of firewalls: >> -Peripheral firewalls that filter accesses to peripherals >> -Memory firewalls that filter accesses to memories or memory regions >> -No type for undefined type of firewall >> >> Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com> > > ... > >> diff --git a/drivers/bus/stm32_firewall.c b/drivers/bus/stm32_firewall.c > > ... > >> +int stm32_firewall_populate_bus(struct stm32_firewall_controller *firewall_controller) >> +{ >> + struct stm32_firewall *firewalls; >> + struct device_node *child; >> + struct device *parent; >> + unsigned int i; >> + int len; >> + int err; >> + >> + parent = firewall_controller->dev; >> + >> + dev_dbg(parent, "Populating %s system bus\n", dev_name(firewall_controller->dev)); >> + >> + for_each_available_child_of_node(dev_of_node(parent), child) { >> + /* The feature-domains property is mandatory for firewall bus devices */ >> + len = of_count_phandle_with_args(child, "feature-domains", "#feature-domain-cells"); >> + if (len <= 0) > > Coccinelle says that, due to breaking out of a > for_each_available_child_of_node() loop, a call to of_node_put() > is needed here > Hi Simon, Thank you, I already sent a V3 correcting the patch ordering issue. I will implement this for V4. >> + return -EINVAL; >> + >> + firewalls = kcalloc(len, sizeof(*firewalls), GFP_KERNEL); >> + if (!firewalls) > > And here. > ditto >> + return -ENOMEM; >> + >> + err = stm32_firewall_get_firewall(child, firewalls, (unsigned int)len); >> + if (err) { >> + kfree(firewalls); > > And here. > ditto >> + return err; >> + } >> + >> + for (i = 0; i < len; i++) { >> + if (firewall_controller->grant_access(firewall_controller, >> + firewalls[i].firewall_id)) { >> + /* >> + * Peripheral access not allowed or not defined. >> + * Mark the node as populated so platform bus won't probe it >> + */ >> + of_node_set_flag(child, OF_POPULATED); >> + dev_err(parent, "%s: Device driver will not be probed\n", >> + child->full_name); >> + } >> + } >> + >> + kfree(firewalls); >> + } >> + >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(stm32_firewall_populate_bus); > >> diff --git a/drivers/bus/stm32_firewall.h b/drivers/bus/stm32_firewall.h > > ... > >> +/** >> + * struct stm32_firewall_controller - Information on firewall controller supplying services >> + * >> + * @name Name of the firewall controller > > kernel-doc complains that name and the other fields of > struct stm32_firewall_controller are not documented. > I believe this is because a ':' is needed after the name of > the parameter (in this case 'name'). > > * @name: Name of the firewall controller > > Likewise, elsewhere. > I will implement it in V4, thank you. >> + * @dev Device reference of the firewall controller >> + * @mmio Base address of the firewall controller >> + * @entry List entry of the firewall controller list >> + * @type Type of firewall >> + * @max_entries Number of entries covered by the firewall >> + * @grant_access Callback used to grant access for a device access against a >> + * firewall controller >> + * @release_access Callback used to release resources taken by a device when access was >> + * granted >> + * @grant_memory_range_access Callback used to grant access for a device to a given memory region >> + */ >> +struct stm32_firewall_controller { >> + const char *name; >> + struct device *dev; >> + void __iomem *mmio; >> + struct list_head entry; >> + unsigned int type; >> + unsigned int max_entries; >> + >> + int (*grant_access)(struct stm32_firewall_controller *ctrl, u32 id); >> + void (*release_access)(struct stm32_firewall_controller *ctrl, u32 id); >> + int (*grant_memory_range_access)(struct stm32_firewall_controller *ctrl, phys_addr_t paddr, >> + size_t size); >> +}; >> + >> +/** >> + * int stm32_firewall_controller_register - Register a firewall controller to the STM32 firewall > > kernel-doc seems unhappy about the presence of 'int' on this line. > > * stm32_firewall_controller_register - Register a firewall controller to the STM32 firewall > > Likewise, elsewhere. > Yes, I will remove the type in V4. >> + * framework >> + * @firewall_controller Firewall controller to register >> + * >> + * Returns 0 in case of success or -ENODEV if no controller was given. >> + */ >> +int stm32_firewall_controller_register(struct stm32_firewall_controller *firewall_controller); > > ... > >> diff --git a/include/linux/bus/stm32_firewall_device.h b/include/linux/bus/stm32_firewall_device.h >> new file mode 100644 >> index 000000000000..9bdc4060154c >> --- /dev/null >> +++ b/include/linux/bus/stm32_firewall_device.h >> @@ -0,0 +1,140 @@ >> +/* SPDX-License-Identifier: GPL-2.0-only */ >> +/* >> + * Copyright (C) 2023, STMicroelectronics - All Rights Reserved >> + */ >> + >> +#ifndef STM32_FIREWALL_DEVICE_H >> +#define STM32_FIREWALL_DEVICE_H >> + >> +#include <linux/of.h> >> +#include <linux/platform_device.h> >> +#include <linux/types.h> >> + >> +#define STM32_FIREWALL_MAX_EXTRA_ARGS 5 >> + >> +/* Opaque reference to stm32_firewall_controller */ >> +struct stm32_firewall_controller; >> + >> +/** >> + * stm32_firewall - Information on a device's firewall. Each device can have more than one firewall. > > kernel-doc seems unhappy about the absence of struct on this line. > > * struct stm32_firewall - Information on a device's firewall. Each device can have more than one firewall. > Yes, I will add the struct in V4. >> + * >> + * @firewall_ctrl Pointer referencing a firewall controller of the device. It is >> + * opaque so a device cannot manipulate the controller's ops or access >> + * the controller's data >> + * @extra_args Extra arguments that are implementation dependent >> + * @entry Name of the firewall entry >> + * @extra_args_size Number of extra arguments >> + * @firewall_id Firewall ID associated the device for this firewall controller >> + */ >> +struct stm32_firewall { >> + struct stm32_firewall_controller *firewall_ctrl; >> + u32 extra_args[STM32_FIREWALL_MAX_EXTRA_ARGS]; >> + const char *entry; >> + size_t extra_args_size; >> + u32 firewall_id; >> +}; > > ... > Best regards, Gatien
On 7/26/23 12:22, Simon Horman wrote: > On Tue, Jul 25, 2023 at 06:40:58PM +0200, Gatien Chevallier wrote: > > ... > >> diff --git a/drivers/bus/stm32_firewall.c b/drivers/bus/stm32_firewall.c > > ... > >> +int stm32_firewall_controller_register(struct stm32_firewall_controller *firewall_controller) >> +{ >> + struct stm32_firewall_controller *ctrl; >> + >> + pr_info("Registering %s firewall controller\n", firewall_controller->name); >> + >> + if (!firewall_controller) >> + return -ENODEV; > > HI Gatien, > > Sorry, one more on this patch, that I missed before sending my previous > email. > > firewall_controller is checked for NULL here. > But it is already dereferenced on the line above the check. > > Flagged by Smatch. > > ... Indeed, thank you. I will change this for V4 Best regards, Gatien
On Wed, Jul 26, 2023 at 12:38:00PM +0200, Gatien CHEVALLIER wrote: > > > On 7/26/23 12:19, Simon Horman wrote: > > On Tue, Jul 25, 2023 at 06:40:58PM +0200, Gatien Chevallier wrote: > > > Introduce a STM32 firewall framework that offers to firewall consumers > > > different firewall services such as the ability to check their access > > > rights against their firewall controller(s). > > > > > > The STM32 firewall framework offers a generic API for STM32 firewall > > > controllers that is defined in their drivers to best fit the > > > specificity of each firewall. > > > > > > There are various types of firewalls: > > > -Peripheral firewalls that filter accesses to peripherals > > > -Memory firewalls that filter accesses to memories or memory regions > > > -No type for undefined type of firewall > > > > > > Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com> > > > > ... > > > > > diff --git a/drivers/bus/stm32_firewall.c b/drivers/bus/stm32_firewall.c > > > > ... > > > > > +int stm32_firewall_populate_bus(struct stm32_firewall_controller *firewall_controller) > > > +{ > > > + struct stm32_firewall *firewalls; > > > + struct device_node *child; > > > + struct device *parent; > > > + unsigned int i; > > > + int len; > > > + int err; > > > + > > > + parent = firewall_controller->dev; > > > + > > > + dev_dbg(parent, "Populating %s system bus\n", dev_name(firewall_controller->dev)); > > > + > > > + for_each_available_child_of_node(dev_of_node(parent), child) { > > > + /* The feature-domains property is mandatory for firewall bus devices */ > > > + len = of_count_phandle_with_args(child, "feature-domains", "#feature-domain-cells"); > > > + if (len <= 0) > > > > Coccinelle says that, due to breaking out of a > > for_each_available_child_of_node() loop, a call to of_node_put() > > is needed here > > > > Hi Simon, > > Thank you, I already sent a V3 correcting the patch ordering issue. I > will implement this for V4. Hi Gatien, Thanks and sorry for not noticing v3 - I think it arrived while I was in the middle of reviewing v2.