Re: [PATCH] soc: add polarfire soc system controller

From: Arnd Bergmann <arnd@arndb.de>
To: Conor.Dooley@microchip.com
Cc: Arnd Bergmann <arnd@arndb.de>, Olof Johansson <olof@lixom.net>,
	SoC Team <soc@kernel.org>,  Rob Herring <robh+dt@kernel.org>,
	Damien Le Moal <Damien.LeMoal@wdc.com>,
	 Jassi Brar <jassisinghbrar@gmail.com>,
	Albert Ou <aou@eecs.berkeley.edu>,
	 Paul Walmsley <paul.walmsley@sifive.com>,
	linux-riscv <linux-riscv@lists.infradead.org>,
	 "nathan=20Neusch=C3=A4fer?=" <j.neuschaefer@gmx.net>,
	Stephen Rothwell <sfr@canb.auug.org.au>,
	Cyril.Jean@microchip.com,
	 Daire McNamara <Daire.McNamara@microchip.com>,
	Atish Patra <Atish.Patra@wdc.com>,
	 Nicolas Ferre <Nicolas.Ferre@microchip.com>,
	 Alexandre Belloni <alexandre.belloni@bootlin.com>,
	 Ludovic Desroches <Ludovic.Desroches@microchip.com>,
	Palmer Dabbelt <palmer@dabbelt.com>
Subject: Re: [PATCH] soc: add polarfire soc system controller
Date: Mon, 8 Nov 2021 16:38:44 +0100	[thread overview]
Message-ID: <CAK8P3a1m_LhOg5JGMqPz6sohJa2hPZ3GN-jQDPxigZ5DaqAGxQ@mail.gmail.com> (raw)
In-Reply-To: <46f04c45-cd50-9484-de41-be8c390bf8e3@microchip.com>

On Mon, Nov 8, 2021 at 4:19 PM <Conor.Dooley@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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv