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: <20230629134306.95823-1-jonas.gorski@gmail.com> <CAKekbeuMjUPpzfgKrxgZzFpiQ4FbeYtBtbrzkWKeBy4u2Symhg@mail.gmail.com>
In-Reply-To: <CAKekbeuMjUPpzfgKrxgZzFpiQ4FbeYtBtbrzkWKeBy4u2Symhg@mail.gmail.com>
From:   Jonas Gorski <jonas.gorski@gmail.com>
Date:   Thu, 29 Jun 2023 17:38:15 +0200
Message-ID: <CAOiHx=kh79=zBM=P8BhrU+3BHcgF71ZZdJR-hV9Tsm59=jELHw@mail.gmail.com>
Subject: Re: [PATCH] spi: bcm-qspi: return error if neither hif_mspi nor mspi
 is available
To:     Kamal Dasu <kamal.dasu@broadcom.com>
Cc:     Kamal Dasu <kdasu.kdev@gmail.com>,
        Broadcom internal kernel review list 
        <bcm-kernel-feedback-list@broadcom.com>,
        Mark Brown <broonie@kernel.org>, linux-spi@vger.kernel.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Thu, 29 Jun 2023 at 17:07, Kamal Dasu <kamal.dasu@broadcom.com> wrote:
>
> On Thu, Jun 29, 2023 at 9:43=E2=80=AFAM Jonas Gorski <jonas.gorski@gmail.=
com> wrote:
> >
> > If neither a "hif_mspi" nor "mspi" resource is present, the driver will
> > just early exit in probe but still return success. Apart from not doing
> > anything meaningful, this would then also lead to a null pointer access
> > on removal, as platform_get_drvdata() would return NULL, which it would
> > then try to dereferce when trying to unregister the spi master.
> >
> > Fix this by unconditionally calling devm_ioremap_resource(), as it can
> > handle a NULL res and will then return a viable ERR_PTR() if we get one=
.
> >
> > The "return 0;" was previously a "goto qspi_resource_err;" where then
> > ret was returned, but since ret was still initialized to 0 at this plac=
e
> > this was a valid conversion in 63c5395bb7a9 ("spi: bcm-qspi: Fix
> > use-after-free on unbind"). The issue was not introduced by this commit=
,
> > only made more obvious.
> >
> > Fixes: fa236a7ef240 ("spi: bcm-qspi: Add Broadcom MSPI driver")
> > Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com>
> > ---
> > Found by looking a the driver while comparing it to its bindings.
> >
> > Only build tested, not runtested.
> >
> >  drivers/spi/spi-bcm-qspi.c | 10 +++-------
> >  1 file changed, 3 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/spi/spi-bcm-qspi.c b/drivers/spi/spi-bcm-qspi.c
> > index 6b46a3b67c41..d91dfbe47aa5 100644
> > --- a/drivers/spi/spi-bcm-qspi.c
> > +++ b/drivers/spi/spi-bcm-qspi.c
> > @@ -1543,13 +1543,9 @@ int bcm_qspi_probe(struct platform_device *pdev,
> >                 res =3D platform_get_resource_byname(pdev, IORESOURCE_M=
EM,
> >                                                    "mspi");
> >
> > -       if (res) {
> > -               qspi->base[MSPI]  =3D devm_ioremap_resource(dev, res);
> > -               if (IS_ERR(qspi->base[MSPI]))
> > -                       return PTR_ERR(qspi->base[MSPI]);
> > -       } else {
> > -               return 0;
> > -       }
>
> I would rather just do this in the else case
>
> } else {
>  -              return 0;
>  +             return -ENODEV;
> }
>
>  The change below does not check the return of
> platform_get_resource_byname() in my opinion rather relies on
> devm_ioremap_resource() doing the right thing.

This is how devm_ioremap_resource() is intended to be used, see e.g.
the example in its kernel documentation:

https://elixir.bootlin.com/linux/latest/source/lib/devres.c#L167

So I don't see what's wrong with relying on functions doing the right thing=
.

Also AFAIU the appropriate return code in this case would be rather
-EINVAL, not -ENODEV.

>
> > +       qspi->base[MSPI]  =3D devm_ioremap_resource(dev, res);
> > +       if (IS_ERR(qspi->base[MSPI]))
> > +               return PTR_ERR(qspi->base[MSPI]);
> >
> >         res =3D platform_get_resource_byname(pdev, IORESOURCE_MEM, "bsp=
i");
> >         if (res) {
> > --
> > 2.34.1
> >

Regards,
Jonas