[PATCH] soc: add polarfire soc system controller

[Arnd Bergmann]

On Mon, Nov 8, 2021 at 4:19 PM <Conor.Dooley at microchip.com> wrote:
>
> On 21/10/2021 19:34, Arnd Bergmann wrote:
> >>>> +int mpfs_blocking_transaction(struct mpfs_sys_controller *mpfs_client, void *msg)
> >>>> +{
> >>>> +    int ret;
> >>>> +
> >>>> +    mutex_lock_interruptible(&transaction_lock);
> >
> > When you do a mutex_lock_interruptible(), you have to check its return code and
> > handle the interruption, usually by passing down -EINTR to the caller.
> >
> >>>> +struct mpfs_sys_controller *
> >>>> +mpfs_sys_controller_get(struct device_node *mss_node)
> >>>> +{
> >>>> +    struct platform_device *pdev = of_find_device_by_node(mss_node);
> >>>> +
> >>>> +    if (!pdev)
> >>>> +            return NULL;
> >>>> +
> >>>> +    return platform_get_drvdata(pdev);
> >>>> +}
> >>>> +EXPORT_SYMBOL(mpfs_sys_controller_get);
> >
> > There should probably be a check in here to ensure that this is actually
> > a system controller and it's bound do this driver, rather than returning a
> > random device's driver data >
> > It might also help to make this take a phandle instead of a device node
> > for lookup, to spare the client the extra phandle to node conversion.
>
> Finally got around to this again, is it sufficient to just check that
> platform_get_drvdata returns non null, or should I also check if the
> pdev matches the drivers compatible strings? Only found one example of
> the latter and a mix of the former & what I had done when I went looking
> around at other drivers.

Neither of those helps at all, it could still be a random other device.

I would check that the device is bound to mpfs_sys_controller_driver here,
that should be the easiest way. Looking at it some more, I suspect the
reference counting needs to be improved as well, to ensure that the
device is not going away here. You only hold a reference on the
of_node (which you apparently never release either), but not on the device.

    Arnd