Re: Sashiko review feedback (was Re: [PATCH 2/3] soc: renesas: Add Renesas R-Car MFIS driver)

[Geert Uytterhoeven]

Hi Wolfram,

On Sun, 22 Mar 2026 at 09:59, Wolfram Sang
<wsa+renesas@xxxxxxxxxxxxxxxxxxxx> wrote:
> I am usually not into AI but I can definitely see value in its review
> assistance. So, I checked what Sashiko thinks of my series and found the
> comments to be largely reasonable. I'll copy and answer them here, a)
> for a more complete review process and b) to give feedback to the
> Sashiko-devs (CCed). Thank you for the service and efforts, I hope we
> can justify the additional energy consumption here. Please let me know
> if such replies are actually helpful for you.
>
> > > +static void mfis_write(struct mfis_reg *mreg, u32 reg, u32 val)
> > > +{
> > > +   struct mfis_priv *priv = mreg->priv;
> > > +   u32 unprotect_mask = priv->info->unprotect_mask;
> > > +   u32 unprotect_code;
> > > +
> > > +   /*
> > > +    * [Gen4] key: 0xACCE0000, mask: 0x0000FFFF
> > > +    * [Gen5] key: 0xACC00000, mask: 0x000FFFFF
> > > +    */
> > > +   unprotect_code = (MFIS_UNPROTECT_KEY & ~unprotect_mask) |
> > > +                    ((mreg->start | reg) & unprotect_mask);
> >
> > The mreg->start and reg variables are combined with a bitwise OR. If
> > mreg->start has overlapping bits with the reg offset, could this yield an
> > incorrect physical address for the unprotect code? Should this use
> > addition like mreg->start + reg instead?
>
> No, if a device tree is so wrongly setup that the bits overlap, then '+'
> instead of '|' will only lead to a different wrong address.

While there are no overlapping bits, and thus both OR and PLUS have
the same effect, I agree PLUS is More Correct(TM), as that matches
what you are using in the second iowrite32() below.

> > > +
> > > +   iowrite32(unprotect_code, priv->common_reg.base + MFISWACNTR);
> > > +   iowrite32(val, mreg->base + reg);
> > > +}
>
> > Can mfis_write() be called concurrently by different processors or
> > interrupted by an interrupt handler like mfis_mb_iicr_interrupt()?
>
> Yes, it can. It even needs a hwspinlock instead of a regular spinlock
> because it needs protection against cores which may not run Linux.
> hwspinlock support will only be added later because of dependency
> issues. My plan was to add the protection once we got hwspinlocks. But
> maybe I should add just spinlocks now and convert them to hwspinlocks
> once their support is in.

Have you ever triggered this race condition when running the mbox
test? Or is that impossible due to locking elsewhere?

> > > +static struct mbox_chan *mfis_mb_of_xlate(struct mbox_controller *mbox,
> > > +                                     const struct of_phandle_args *sp)
> > > +{
> > > +   struct mfis_priv *priv = mfis_mb_mbox_to_priv(mbox);
> > > +   struct mfis_per_chan_priv *per_chan_priv;
> > > +   struct mbox_chan *chan;
> > > +   u32 chan_num, chan_flags;
> > > +   bool tx_uses_eicr, is_only_rx;
> > > +
> > > +   if (sp->args_count != 2)
> > > +           return ERR_PTR(-EINVAL);
> > > +
> > > +   chan_num = sp->args[0];
> > > +   chan_flags = sp->args[1];
> > > +
> > > +   /* Channel layout is described in mfis_mb_probe() */
> > > +   if (priv->info->mb_channels_are_unidir) {
> > > +           is_only_rx = chan_flags & MFIS_CHANNEL_RX;
> > > +           chan = mbox->chans + 2 * chan_num + is_only_rx;
>
> > Does this code allow out-of-bounds access if a malformed device tree
> > provides an arbitrarily large chan_num? Should chan_num be checked against
> > priv->info->mb_num_channels before computing the chan pointer?
>
> Yes, Geert pointed this out as well.
>
> > [ ... ]
> > > +static int mfis_mb_probe(struct mfis_priv *priv)
> > > +{
> > [ ... ]
> > > +   mbox->chans = chan;
> > > +   mbox->num_chans = num_chan;
> > > +   mbox->txdone_poll = true;
> > > +   mbox->ops = &mfis_iicr_ops;
> > > +   mbox->dev = dev;
> > > +   mbox->of_xlate = mfis_mb_of_xlate;
> > > +
> > > +   return mbox_controller_register(mbox);
> > > +}
> > The mailbox controller is registered using mbox_controller_register(), but
> > the driver lacks a remove callback to unregister it.
> > If the driver is unbound, devres will free the devm_kzalloc structures
> > backing the mailbox. Will this result in a use-after-free regression since
> > the mailbox subsystem still retains the controller in its global lists?
> > Should devm_mbox_controller_register() be used instead?
> > [ ... ]
>
> Yes, this was overlooked so far.

.suppress_bind_attrs to the rescue? ;-)

> > > +static int mfis_probe(struct platform_device *pdev)
> > > +{
> > > +   struct mfis_priv *priv;
> > > +   struct device *dev = &pdev->dev;
> > > +   int ret;
> > > +
> > > +   priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > > +   if (!priv)
> > > +           return -ENOMEM;
> > > +
> > > +   priv->dev = dev;
> > > +   priv->info = of_device_get_match_data(dev);
> > Can of_device_get_match_data() return a null pointer here?
> > If the driver is bound manually via sysfs or instantiated in an environment
> > lacking device tree match data, would this lead to a null pointer
> > dereference regression later when accessing priv->info->mb_num_channels in
> > mfis_mb_probe()?
>
> The latter case would be clearly a driver bug. In addition with the
> former case, it probably makes sense to handle this more gracefully,
> though.

That is an interesting one I never considered before!
So far we always assumed of_device_get_match_data() cannot return
NULL if all match table entries have their .data members filled in.
However, you can indeed still trigger this by using driver_override
to bind to the wrong device, which obviously has no match entry at all:

On Salvator-XS:

# echo fe960000.vsp > /sys/bus/platform/drivers/vsp1/unbind
# echo renesas_spi > /sys/bus/platform/devices/fe960000.vsp/driver_override
# echo fe960000.vsp > /sys/bus/platform/drivers/renesas_spi/bind

BOOM!

Do we care about that?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds