Re: [PATCH v2 11/11] pinctrl: stm32: add firewall checks before probing the HDP driver

[Gatien CHEVALLIER]

On 1/18/26 23:19, Linus Walleij wrote:
> Hi Gatien,
>
> thanks for your patch!
>
> On Wed, Jan 14, 2026 at 11:31 AM Gatien Chevallier
> <gatien.chevallier@xxxxxxxxxxx> wrote:
>
> > Because the HDP peripheral both depends on debug and firewall
> > configuration, when CONFIG_STM32_FIREWALL is present, use the
> > stm32 firewall framework to be able to check these configuration against
> > the relevant controllers.
> >
> > Signed-off-by: Gatien Chevallier <gatien.chevallier@xxxxxxxxxxx>
> (...)
> > +#include <linux/bus/stm32_firewall_device.h>
> >   #include <linux/clk.h>
> >   #include <linux/gpio/driver.h>
> >   #include <linux/gpio/generic.h>
> > @@ -605,10 +606,50 @@ MODULE_DEVICE_TABLE(of, stm32_hdp_of_match);
> >   static int stm32_hdp_probe(struct platform_device *pdev)
> >   {
> >          struct gpio_generic_chip_config config;
> > +       struct stm32_firewall *firewall = NULL;
> >          struct device *dev = &pdev->dev;
> >          struct stm32_hdp *hdp;
> > +       int nb_firewall;
> >          u8 version;
> >          int err;
> > +       int i;
> > +
> > +       nb_firewall = of_count_phandle_with_args(pdev->dev.of_node, "access-controllers",
> > +                                                "#access-controller-cells");
> > +       if (IS_ENABLED(CONFIG_STM32_FIREWALL) && nb_firewall != -ENOENT) {
> > +               if (nb_firewall <= 0)
> > +                       return -EINVAL;
> > +
> > +               firewall = devm_kcalloc(dev, nb_firewall, sizeof(*firewall), GFP_KERNEL);
> > +               if (!firewall)
> > +                       return -ENOMEM;
> > +
> > +               /* Get stm32 firewall information */
> > +               err = stm32_firewall_get_firewall(dev->of_node, firewall, nb_firewall);
> > +               if (err)
> > +                       return dev_err_probe(dev, err, "Failed to get firewall controller\n");
> > +
> > +               for (i = 0; i < nb_firewall; i++) {
> > +                       err = stm32_firewall_grant_access_by_id(firewall + i,
> > +                                                               firewall[i].firewall_id);
> > +                       if (err) {
> > +                               while (i) {
> > +                                       u32 id;
> > +
> > +                                       i--;
> > +                                       id = firewall[i].firewall_id;
> > +                                       stm32_firewall_release_access_by_id(firewall + i, id);
> > +                               }
> > +                               if (err == -EACCES) {
> > +                                       dev_info(dev, "No firewall access\n");
> > +                                       return -ENODEV;
> > +                               }
> > +
> > +                               return dev_err_probe(dev, err, "Error checking firewall access\n");
> > +                       }
> > +               }
> > +       }
>
> Doesn't this whole piece of code look very generic?
>
> Point out to me if something is pin control-specific about it?
>
> Can't we just add a helper function such as
>
> stm32_firewall_of_check_access(struct device *dev)
> {
>      struct stm32_firewall *firewall = NULL;
>      int nb_firewall;
>
>      nb_firewall = of_count_phandle_with_args(pdev->dev.of_node,
> "access-controllers",
>                                          "#access-controller-cells");
> (...)
> }
>
> Then place the prototype for this in <linux/bus/stm32_firewall_device.h>.
>
> I think this will be helpful for the next driver that needs to check
> firewall access
> before continuing.
>
> Yours,
> Linus Walleij

Hello Linus,

Thanks for your feedback. There is already a function to check the
firewall access that is stm32_firewall_grant_access().

However, a helper could clearly implemented to wrap it with the
get when all elements should be assessed, as you're suggesting.
I'll submit V3 with a proposition, let's see.

Best regards,
Gatien