Re: [PATCH v2 05/11] firewall: introduce stm32_firewall framework
From: Gatien CHEVALLIER
Date: Wed Jul 26 2023 - 06:38:45 EST
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@xxxxxxxxxxx>
...
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