Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230807130217.17853-1-aboutphysycs@gmail.com>
 <c867e685-7665-420b-9478-bce6eb067463@sirena.org.uk> <CAH3L5Qrzk-7WoF8+GKMYS40Dj_fxGpiLrTvVB_PJdA_VZVsDAQ@mail.gmail.com>
 <196642e7-4136-4ba6-a918-8c759f27f818@sirena.org.uk>
In-Reply-To: <196642e7-4136-4ba6-a918-8c759f27f818@sirena.org.uk>
From:   Alexandru Ardelean <alex@shruggie.ro>
Date:   Mon, 7 Aug 2023 21:57:18 +0300
Message-ID: <CAH3L5QoF1pyexq-QNJAy4j-X_0EFhAvzpt7tD-Tg+JFjymQg1A@mail.gmail.com>
Subject: Re: [PATCH] spi: gxp: removed unneeded call to platform_set_drvdata()
To:     Mark Brown <broonie@kernel.org>
Cc:     Andrei Coardos <aboutphysycs@gmail.com>,
        linux-kernel@vger.kernel.org, linux-spi@vger.kernel.org,
        nick.hawkins@hpe.com, verdun@hpe.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Mon, Aug 7, 2023 at 9:18=E2=80=AFPM Mark Brown <broonie@kernel.org> wrot=
e:
>
> On Mon, Aug 07, 2023 at 08:38:27PM +0300, Alexandru Ardelean wrote:
> > On Mon, Aug 7, 2023 at 4:27=E2=80=AFPM 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 equiv=
alent
> > > > platform_get_drvdata() call to access the private data of the drive=
r. 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).