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

[Wolfram Sang <wsa+renesas@sang-engineering.com>]

[-- Attachment #1: Type: text/plain, Size: 5267 bytes --]

Hi all,

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.

> > +
> > +	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.

> > +static void mfis_mb_shutdown(struct mbox_chan *chan)
> > +{
> > +	struct mfis_per_chan_priv *per_chan_priv = chan->con_priv;
> > +
> > +	free_irq(per_chan_priv->irq, chan);
> > +}

> Since mfis_mb_of_xlate() skips interrupt lookup for transmit-only channels,
> leaving per_chan_priv->irq as 0, and mfis_mb_startup() guards the request
> with if (per_chan_priv->irq), will this unconditional free_irq() trigger a
> kernel warning when shutting down a transmit-only channel?

Yes, I seemed to remember that free_irq() can handle this, but it will
in-deed trigger a warning. Also, the reasons Geert mentioned in his
review apply.

> [ ... ]
> > +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.

> > +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.

Happy hacking,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]