Re: [PATCH 2/3] soc: renesas: Add Renesas R-Car MFIS driver

[Geert Uytterhoeven]

Hi Wolfram,

On Sun, 22 Mar 2026 at 20:58, Wolfram Sang
<wsa+renesas@xxxxxxxxxxxxxxxxxxxx> wrote:
> > > +struct mfis_priv {
> > > +       struct device *dev;
> > > +       struct mfis_reg common_reg;
> > > +       struct mfis_reg mbox_reg;
> > > +       const struct mfis_info *info;
> > > +
> > > +       /* mailbox private data */
> > > +       struct mbox_controller mbox;
> > > +       struct mfis_per_chan_priv *per_chan_privs;
> >
> > This could be a flexible array, if it weren't for the hwspinlock array
> > you will have to add later, right?
>
> No, hwspinlock doesn't need any private data here. But something else
> could come in the future maybe? I also don't see a big advantage of the
> flexible array, too. Maybe it's too late in the evening...

You're right. Somehow I thought you were allocating priv and
per_chan_privs next to each other.

> > > +       chan_num = sp->args[0];
> >
> > "chan_num" is the hardware channel number, and should be validated
> > against mpriv->info->mb_num_channels.
>
> Ack!
>
> > > +       chan_flags = sp->args[1];
> > > +
> > > +       /* Channel layout is described in mfis_mb_probe() */
> >
> > Given you already use "chan += ..." below, perhaps:
> >
> >     chan = mbox->chans + chan_num;
> >
> > > +       if (priv->info->mb_channels_are_unidir) {
> > > +               is_only_rx = chan_flags & MFIS_CHANNEL_RX;
> > > +               chan = mbox->chans + 2 * chan_num + is_only_rx;
> >
> > ...and:
> >
> >     chan += chan_num + is_only_rx;
> >
> > > +       } else {
> > > +               is_only_rx = false;
> > > +               chan = mbox->chans + chan_num;
> >
> > ... and drop this line?
> > With a proper preinitalization of is_only_rx, the whole "else" branch
> > can be dropped.
>
> I agree it saves a few lines, but I really think the original code is
> easier to understand. Channel layout is already wickes and doing 'channel
> +=' twice is harder to understand than a simple assignment IMO.
>
> >
> > > +       }
> > > +
> > > +       if (priv->info->mb_reg_comes_from_dt) {
> > > +               tx_uses_eicr = chan_flags & MFIS_CHANNEL_EICR;
> > > +               if (tx_uses_eicr)
> > > +                       chan += mbox->num_chans / 2;
> > > +       } else {
> > > +               tx_uses_eicr = priv->info->mb_tx_uses_eicr;
> > > +       }
> >
> > "chan - mbox->chans" is the logical channel number, and should be
> > validated against mbox_num_chans, to avoid out-of-bound accesses.
>
> "chan - ..."? You mean "chan + ..."?

No, I did mean "-": you do have a pointer "chan" to the channel,
instead of an index into the mbox->chans[] array.
Using a  index would  make validation easier to read, though.

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