Re: [PATCH v2 05/11] firewall: introduce stm32_firewall framework

[Gatien CHEVALLIER <gatien.chevallier@xxxxxxxxxxx>]

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