[PATCH] spi: gxp: removed unneeded call to platform_set_drvdata()

[PATCH] spi: gxp: removed unneeded call to platform_set_drvdata()

[Andrei Coardos]

This function call was found to be unnecessary as there is no equivalent
platform_get_drvdata() call to access the private data of the driver. Also,
the private data is defined in this driver, so there is no risk of it being
accessed outside of this driver file.

Signed-off-by: Andrei Coardos <aboutphysycs@gmail.com>
---
 drivers/spi/spi-gxp.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/spi/spi-gxp.c b/drivers/spi/spi-gxp.c
index 684d63f402f3..6261d9f036bc 100644
--- a/drivers/spi/spi-gxp.c
+++ b/drivers/spi/spi-gxp.c
@@ -264,7 +264,6 @@ static int gxp_spifi_probe(struct platform_device *pdev)
 
 	spifi = spi_controller_get_devdata(ctlr);
 
-	platform_set_drvdata(pdev, spifi);
 	spifi->data = data;
 	spifi->dev = dev;
 
-- 
2.34.1

Re: [PATCH] spi: gxp: removed unneeded call to platform_set_drvdata()

[Mark Brown]

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

On Mon, Aug 07, 2023 at 04:02:17PM +0300, Andrei Coardos wrote:

> This function call was found to be unnecessary as there is no equivalent
> platform_get_drvdata() call to access the private data of the driver. Also,
> the private data is defined in this driver, so there is no risk of it being
> accessed outside of this driver file.

That isn't enough of a check here - people can still reference the
driver data without going through the accessor function.

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

Re: [PATCH] spi: gxp: removed unneeded call to platform_set_drvdata()

[Alexandru Ardelean]

On Mon, Aug 7, 2023 at 4:27 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Mon, Aug 07, 2023 at 04:02:17PM +0300, Andrei Coardos wrote:
>
> > This function call was found to be unnecessary as there is no equivalent
> > platform_get_drvdata() call to access the private data of the driver. Also,
> > the private data is defined in this driver, so there is no risk of it being
> > accessed outside of this driver file.
>
> That isn't enough of a check here - people can still reference the
> driver data without going through the accessor function.

So, is that like calling `platform_get_drvdata()` in a parent/chid
device, to check if the driver-data is set?
Would it make sense for another driver to do that (i.e. check the
driver-data is non-null, but not access the data)?
I can imagine that being possible, but it's a bit quirky.

Or, is the issue with the wording of the comment?

Re: [PATCH] spi: gxp: removed unneeded call to platform_set_drvdata()

[Mark Brown]

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

On Mon, Aug 07, 2023 at 08:38:27PM +0300, Alexandru Ardelean wrote:
> On Mon, Aug 7, 2023 at 4:27 PM Mark Brown <broonie@kernel.org> wrote:

> > On Mon, Aug 07, 2023 at 04:02:17PM +0300, Andrei Coardos wrote:

> > > This function call was found to be unnecessary as there is no equivalent
> > > platform_get_drvdata() call to access the private data of the driver. Also,
> > > the private data is defined in this driver, so there is no risk of it being
> > > accessed outside of this driver file.

> > That isn't enough of a check here - people can still reference the
> > driver data without going through the accessor function.

> So, is that like calling `platform_get_drvdata()` in a parent/chid
> device, to check if the driver-data is set?

That wasn't what I was thinking of, waht I was thinking of was just open
coding platform_get_drvdata() and looking directly at struct device.
Another common case is where drivers that support multiple bus types
will pass around the struct device and use dev_get_drvdata() to read the
data rather than using platform_get_drvdata().  The driver data can be
allocated and initialised with bus specific bits before being passed off
to the generic code.

That said the looking at the parent's driver data is definitely a thing
that happens with MFDs.

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

Re: [PATCH] spi: gxp: removed unneeded call to platform_set_drvdata()

[Alexandru Ardelean]

On Mon, Aug 7, 2023 at 9:18 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Mon, Aug 07, 2023 at 08:38:27PM +0300, Alexandru Ardelean wrote:
> > On Mon, Aug 7, 2023 at 4:27 PM Mark Brown <broonie@kernel.org> wrote:
>
> > > On Mon, Aug 07, 2023 at 04:02:17PM +0300, Andrei Coardos wrote:
>
> > > > This function call was found to be unnecessary as there is no equivalent
> > > > platform_get_drvdata() call to access the private data of the driver. Also,
> > > > the private data is defined in this driver, so there is no risk of it being
> > > > accessed outside of this driver file.
>
> > > That isn't enough of a check here - people can still reference the
> > > driver data without going through the accessor function.
>
> > So, is that like calling `platform_get_drvdata()` in a parent/chid
> > device, to check if the driver-data is set?
>
> That wasn't what I was thinking of, waht I was thinking of was just open
> coding platform_get_drvdata() and looking directly at struct device.

Ah. Right.
I hadn't thought of checking "dev->driver_data" access.

> Another common case is where drivers that support multiple bus types
> will pass around the struct device and use dev_get_drvdata() to read the

Agree. I see that happening with PM routines.
It doesn't look like it's the case in this driver.

> data rather than using platform_get_drvdata().  The driver data can be
> allocated and initialised with bus specific bits before being passed off
> to the generic code.

If I'm looking more closely, I am seeing that the
"platform_set_drvdata(pdev, spifi);" has no equivalent access to
"pdev->dev.driver_data"
Nor by open-coding, nor by "dev_get_drvdata(&pdev->dev)"
But I do see that "spi_controller_get_devdata()" is calling
"dev_get_drvdata()" on a device object allocated here via
"devm_spi_alloc_master()"

So, I agree. That a more thorough check is needed here.

>
> That said the looking at the parent's driver data is definitely a thing
> that happens with MFDs.

Yep. MFDs is one case I was thinking of too (with respect to
parent/child lookup of driver data).