[PATCH] mtd: nand: raw: qcom_nandc: handle error pointer from adm prep_slave_sg

[PATCH] mtd: nand: raw: qcom_nandc: handle error pointer from adm prep_slave_sg

[Christian Marangi]

ADM dma engine has changed to also provide error pointer instaed of
plain NULL pointer on invalid configuration of prep_slave_sg function.
Currently this is not handled and an error pointer is detected as a
valid dma_desc. This cause kernel panic as the driver doesn't fail
with an invalid dma engine configuration.

Correctly handle this case by checking if dma_desc is NULL or IS_ERR.

Fixes: 03de6b273805 ("dmaengine: qcom-adm: stop abusing slave_id config")
Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
Cc: stable@vger.kernel.org # v5.17+
---
 drivers/mtd/nand/raw/qcom_nandc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
index 8f80019a9f01..d7eac196dde0 100644
--- a/drivers/mtd/nand/raw/qcom_nandc.c
+++ b/drivers/mtd/nand/raw/qcom_nandc.c
@@ -1054,7 +1054,7 @@ static int prep_adm_dma_desc(struct qcom_nand_controller *nandc, bool read,
 	}
 
 	dma_desc = dmaengine_prep_slave_sg(nandc->chan, sgl, 1, dir_eng, 0);
-	if (!dma_desc) {
+	if (IS_ERR_OR_NULL(dma_desc)) {
 		dev_err(nandc->dev, "failed to prepare desc\n");
 		ret = -EINVAL;
 		goto err;
-- 
2.37.2

[PATCH] mtd: nand: raw: qcom_nandc: handle error pointer from adm prep_slave_sg

[Christian Marangi]

ADM dma engine has changed to also provide error pointer instaed of
plain NULL pointer on invalid configuration of prep_slave_sg function.
Currently this is not handled and an error pointer is detected as a
valid dma_desc. This cause kernel panic as the driver doesn't fail
with an invalid dma engine configuration.

Correctly handle this case by checking if dma_desc is NULL or IS_ERR.

Fixes: 03de6b273805 ("dmaengine: qcom-adm: stop abusing slave_id config")
Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
Cc: stable@vger.kernel.org # v5.17+
---
 drivers/mtd/nand/raw/qcom_nandc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
index 8f80019a9f01..d7eac196dde0 100644
--- a/drivers/mtd/nand/raw/qcom_nandc.c
+++ b/drivers/mtd/nand/raw/qcom_nandc.c
@@ -1054,7 +1054,7 @@ static int prep_adm_dma_desc(struct qcom_nand_controller *nandc, bool read,
 	}
 
 	dma_desc = dmaengine_prep_slave_sg(nandc->chan, sgl, 1, dir_eng, 0);
-	if (!dma_desc) {
+	if (IS_ERR_OR_NULL(dma_desc)) {
 		dev_err(nandc->dev, "failed to prepare desc\n");
 		ret = -EINVAL;
 		goto err;
-- 
2.37.2


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd: nand: raw: qcom_nandc: handle error pointer from adm prep_slave_sg

[Christian Marangi]

On Fri, Sep 16, 2022 at 11:01:11AM +0200, Arnd Bergmann wrote:
> On Fri, Sep 16, 2022, at 2:10 AM, Christian Marangi wrote:
> > ADM dma engine has changed to also provide error pointer instaed of
> > plain NULL pointer on invalid configuration of prep_slave_sg function.
> > Currently this is not handled and an error pointer is detected as a
> > valid dma_desc. This cause kernel panic as the driver doesn't fail
> > with an invalid dma engine configuration.
> >
> > Correctly handle this case by checking if dma_desc is NULL or IS_ERR.
> 
> Using IS_ERR_OR_NULL() is almost never a correct solution. I think
> in this case the problem is the adm_prep_slave_sg() function
> that returns an invalid error code.
>

Yes I was honestly not certain on what to fix... The adm driver or the
nand driver. Dediced to fix the nand driver since it was the one that
was causing the panic and to me it seemd bad to remove error code from
the adm driver. (But we have debug log anyway so...)

> While error pointers are often better than NULL pointers for
> passing information to the caller, a driver can't just change
> the calling conventions on its own. If we want to change
> the dmaengine_prep_slave_sg() API, I would suggest coming
> up with a new name for a replacement interface that uses
> error pointers instead of NULL first, and then changing
> all callers to the new interface.
> 
>        Arnd

I also notice this that dmaengine_prep_slave_sg always return NULL in
case of error so you are right and the real problem here is the changed
calling concention. Seems overkill to introduce a new API just for a
commit that changed the naming convention...

So I think we should just ignore this and I will send a better fix that
will just return NULL and fix the adm driver. 

Thanks for the review and the clarification!
(Also extra point the fixes tag will match the driver)

-- 
	Ansuel

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd: nand: raw: qcom_nandc: handle error pointer from adm prep_slave_sg

[Christian Marangi]

On Fri, Sep 16, 2022 at 11:01:11AM +0200, Arnd Bergmann wrote:
> On Fri, Sep 16, 2022, at 2:10 AM, Christian Marangi wrote:
> > ADM dma engine has changed to also provide error pointer instaed of
> > plain NULL pointer on invalid configuration of prep_slave_sg function.
> > Currently this is not handled and an error pointer is detected as a
> > valid dma_desc. This cause kernel panic as the driver doesn't fail
> > with an invalid dma engine configuration.
> >
> > Correctly handle this case by checking if dma_desc is NULL or IS_ERR.
> 
> Using IS_ERR_OR_NULL() is almost never a correct solution. I think
> in this case the problem is the adm_prep_slave_sg() function
> that returns an invalid error code.
>

Yes I was honestly not certain on what to fix... The adm driver or the
nand driver. Dediced to fix the nand driver since it was the one that
was causing the panic and to me it seemd bad to remove error code from
the adm driver. (But we have debug log anyway so...)

> While error pointers are often better than NULL pointers for
> passing information to the caller, a driver can't just change
> the calling conventions on its own. If we want to change
> the dmaengine_prep_slave_sg() API, I would suggest coming
> up with a new name for a replacement interface that uses
> error pointers instead of NULL first, and then changing
> all callers to the new interface.
> 
>        Arnd

I also notice this that dmaengine_prep_slave_sg always return NULL in
case of error so you are right and the real problem here is the changed
calling concention. Seems overkill to introduce a new API just for a
commit that changed the naming convention...

So I think we should just ignore this and I will send a better fix that
will just return NULL and fix the adm driver. 

Thanks for the review and the clarification!
(Also extra point the fixes tag will match the driver)

-- 
	Ansuel

Re: [PATCH] mtd: nand: raw: qcom_nandc: handle error pointer from adm prep_slave_sg

[Christian Marangi]

On Fri, Sep 16, 2022 at 01:45:17PM +0200, Arnd Bergmann wrote:
> On Fri, Sep 16, 2022, at 5:11 AM, Christian Marangi wrote:
> > On Fri, Sep 16, 2022 at 11:01:11AM +0200, Arnd Bergmann wrote:
> >
> > Thanks for the review and the clarification!
> > (Also extra point the fixes tag will match the driver)
> 
> Regarding the fixes tag, how did you actually get to my patch?
> While it's possible that it caused the regression, it did not
> introduce the ERR_PTR() usage that was already there in
> 5c9f8c2dbdbe ("dmaengine: qcom: Add ADM driver").
> 
> Maybe there is another bug that needs to be addressed in this
> driver?
> 
>       Arnd

Don't know if you received the other fix, but 03de6b273805 broke
ipq806x. I already sent a fix for that but since it was added extra
check for crci, it seems logical that 03de6b273805 should have also
updated the nandc driver to handle the new pointer error. This was not
done so I added that fixes tag.

Tell me if the logic was wrong...

-- 
	Ansuel

Re: [PATCH] mtd: nand: raw: qcom_nandc: handle error pointer from adm prep_slave_sg

[Christian Marangi]

On Fri, Sep 16, 2022 at 01:45:17PM +0200, Arnd Bergmann wrote:
> On Fri, Sep 16, 2022, at 5:11 AM, Christian Marangi wrote:
> > On Fri, Sep 16, 2022 at 11:01:11AM +0200, Arnd Bergmann wrote:
> >
> > Thanks for the review and the clarification!
> > (Also extra point the fixes tag will match the driver)
> 
> Regarding the fixes tag, how did you actually get to my patch?
> While it's possible that it caused the regression, it did not
> introduce the ERR_PTR() usage that was already there in
> 5c9f8c2dbdbe ("dmaengine: qcom: Add ADM driver").
> 
> Maybe there is another bug that needs to be addressed in this
> driver?
> 
>       Arnd

Don't know if you received the other fix, but 03de6b273805 broke
ipq806x. I already sent a fix for that but since it was added extra
check for crci, it seems logical that 03de6b273805 should have also
updated the nandc driver to handle the new pointer error. This was not
done so I added that fixes tag.

Tell me if the logic was wrong...

-- 
	Ansuel

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd: nand: raw: qcom_nandc: handle error pointer from adm prep_slave_sg

[Arnd Bergmann]

On Fri, Sep 16, 2022, at 2:10 AM, Christian Marangi wrote:
> ADM dma engine has changed to also provide error pointer instaed of
> plain NULL pointer on invalid configuration of prep_slave_sg function.
> Currently this is not handled and an error pointer is detected as a
> valid dma_desc. This cause kernel panic as the driver doesn't fail
> with an invalid dma engine configuration.
>
> Correctly handle this case by checking if dma_desc is NULL or IS_ERR.

Using IS_ERR_OR_NULL() is almost never a correct solution. I think
in this case the problem is the adm_prep_slave_sg() function
that returns an invalid error code.

While error pointers are often better than NULL pointers for
passing information to the caller, a driver can't just change
the calling conventions on its own. If we want to change
the dmaengine_prep_slave_sg() API, I would suggest coming
up with a new name for a replacement interface that uses
error pointers instead of NULL first, and then changing
all callers to the new interface.

       Arnd

Re: [PATCH] mtd: nand: raw: qcom_nandc: handle error pointer from adm prep_slave_sg

[Arnd Bergmann]

On Fri, Sep 16, 2022, at 2:10 AM, Christian Marangi wrote:
> ADM dma engine has changed to also provide error pointer instaed of
> plain NULL pointer on invalid configuration of prep_slave_sg function.
> Currently this is not handled and an error pointer is detected as a
> valid dma_desc. This cause kernel panic as the driver doesn't fail
> with an invalid dma engine configuration.
>
> Correctly handle this case by checking if dma_desc is NULL or IS_ERR.

Using IS_ERR_OR_NULL() is almost never a correct solution. I think
in this case the problem is the adm_prep_slave_sg() function
that returns an invalid error code.

While error pointers are often better than NULL pointers for
passing information to the caller, a driver can't just change
the calling conventions on its own. If we want to change
the dmaengine_prep_slave_sg() API, I would suggest coming
up with a new name for a replacement interface that uses
error pointers instead of NULL first, and then changing
all callers to the new interface.

       Arnd

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd: nand: raw: qcom_nandc: handle error pointer from adm prep_slave_sg

[Arnd Bergmann]

On Fri, Sep 16, 2022, at 5:11 AM, Christian Marangi wrote:
> On Fri, Sep 16, 2022 at 11:01:11AM +0200, Arnd Bergmann wrote:
>
> Thanks for the review and the clarification!
> (Also extra point the fixes tag will match the driver)

Regarding the fixes tag, how did you actually get to my patch?
While it's possible that it caused the regression, it did not
introduce the ERR_PTR() usage that was already there in
5c9f8c2dbdbe ("dmaengine: qcom: Add ADM driver").

Maybe there is another bug that needs to be addressed in this
driver?

      Arnd

Re: [PATCH] mtd: nand: raw: qcom_nandc: handle error pointer from adm prep_slave_sg

[Arnd Bergmann]

On Fri, Sep 16, 2022, at 5:11 AM, Christian Marangi wrote:
> On Fri, Sep 16, 2022 at 11:01:11AM +0200, Arnd Bergmann wrote:
>
> Thanks for the review and the clarification!
> (Also extra point the fixes tag will match the driver)

Regarding the fixes tag, how did you actually get to my patch?
While it's possible that it caused the regression, it did not
introduce the ERR_PTR() usage that was already there in
5c9f8c2dbdbe ("dmaengine: qcom: Add ADM driver").

Maybe there is another bug that needs to be addressed in this
driver?

      Arnd

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/