[PATCH] mtd: rawnand: intel: remove broken code

[PATCH] mtd: rawnand: intel: remove broken code

[Martin Blumenstingl]

Drop the check for mtd->name as it's executed while the mtd variable is
always NULL. If some MTD name is needed then it should be validated by
the MTD core.

While here, also drop the NULL assignment to the mtd variable as it's
overwritten later on anyways and the NULL value is never read.

Fixes: 0b1039f016e8a3 ("mtd: rawnand: Add NAND controller support on Intel LGM SoC")
Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
I found this by looking at the new driver. This patch is compile-tested
only.


 drivers/mtd/nand/raw/intel-nand-controller.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/mtd/nand/raw/intel-nand-controller.c b/drivers/mtd/nand/raw/intel-nand-controller.c
index fdb112e8a90d..398de6ec68d7 100644
--- a/drivers/mtd/nand/raw/intel-nand-controller.c
+++ b/drivers/mtd/nand/raw/intel-nand-controller.c
@@ -579,7 +579,7 @@ static int ebu_nand_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct ebu_nand_controller *ebu_host;
 	struct nand_chip *nand;
-	struct mtd_info *mtd = NULL;
+	struct mtd_info *mtd;
 	struct resource *res;
 	char *resname;
 	int ret;
@@ -647,10 +647,6 @@ static int ebu_nand_probe(struct platform_device *pdev)
 	       ebu_host->ebu + EBU_ADDR_SEL(cs));
 
 	nand_set_flash_node(&ebu_host->chip, dev->of_node);
-	if (!mtd->name) {
-		dev_err(ebu_host->dev, "NAND label property is mandatory\n");
-		return -EINVAL;
-	}
 
 	mtd = nand_to_mtd(&ebu_host->chip);
 	mtd->dev.parent = dev;
-- 
2.29.2

Re: [PATCH] mtd: rawnand: intel: remove broken code

[Miquel Raynal]

Hi Martin,

Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote on Thu,
17 Dec 2020 23:11:48 +0100:

> Drop the check for mtd->name as it's executed while the mtd variable is
> always NULL. If some MTD name is needed then it should be validated by
> the MTD core.
> 
> While here, also drop the NULL assignment to the mtd variable as it's
> overwritten later on anyways and the NULL value is never read.
> 
> Fixes: 0b1039f016e8a3 ("mtd: rawnand: Add NAND controller support on Intel LGM SoC")
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
> I found this by looking at the new driver. This patch is compile-tested
> only.
> 
> 
>  drivers/mtd/nand/raw/intel-nand-controller.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/intel-nand-controller.c b/drivers/mtd/nand/raw/intel-nand-controller.c
> index fdb112e8a90d..398de6ec68d7 100644
> --- a/drivers/mtd/nand/raw/intel-nand-controller.c
> +++ b/drivers/mtd/nand/raw/intel-nand-controller.c
> @@ -579,7 +579,7 @@ static int ebu_nand_probe(struct platform_device *pdev)
>  	struct device *dev = &pdev->dev;
>  	struct ebu_nand_controller *ebu_host;
>  	struct nand_chip *nand;
> -	struct mtd_info *mtd = NULL;
> +	struct mtd_info *mtd;
>  	struct resource *res;
>  	char *resname;
>  	int ret;
> @@ -647,10 +647,6 @@ static int ebu_nand_probe(struct platform_device *pdev)
>  	       ebu_host->ebu + EBU_ADDR_SEL(cs));
>  
>  	nand_set_flash_node(&ebu_host->chip, dev->of_node);
> -	if (!mtd->name) {
> -		dev_err(ebu_host->dev, "NAND label property is mandatory\n");
> -		return -EINVAL;
> -	}

This is valid code, it's best to use a label = "my-storage"; property
in your NAND DT node. Then mtd->name will be updated by
nand_set_flash_node().

Thanks,
Miquèl

Re: [PATCH] mtd: rawnand: intel: remove broken code

[Martin Blumenstingl]

Hi Miquel,

thank you for looking into this

On Mon, Jan 4, 2021 at 9:48 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
[...]
> >       nand_set_flash_node(&ebu_host->chip, dev->of_node);
> > -     if (!mtd->name) {
> > -             dev_err(ebu_host->dev, "NAND label property is mandatory\n");
> > -             return -EINVAL;
> > -     }
>
> This is valid code, it's best to use a label = "my-storage"; property
> in your NAND DT node. Then mtd->name will be updated by
> nand_set_flash_node().
so you suggest moving the check instead?
the original code flow was:
  mtd = NULL;
  if (!mtd->name)
     return -EINVAL;
  mtd = nand_to_mtd(&ebu_host->chip);
  ...

by saying that the code itself is valid you're asking me to update the
flow to the following:
  mtd = nand_to_mtd(&ebu_host->chip);
  if (!mtd->name)
     return -EINVAL;


Best regards,
Martin

Re: [PATCH] mtd: rawnand: intel: remove broken code

[Miquel Raynal]

Hi Martin,

Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote on Mon,
4 Jan 2021 14:13:04 +0100:

> Hi Miquel,
> 
> thank you for looking into this
> 
> On Mon, Jan 4, 2021 at 9:48 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> [...]
> > >       nand_set_flash_node(&ebu_host->chip, dev->of_node);
> > > -     if (!mtd->name) {
> > > -             dev_err(ebu_host->dev, "NAND label property is mandatory\n");
> > > -             return -EINVAL;
> > > -     }  
> >
> > This is valid code, it's best to use a label = "my-storage"; property
> > in your NAND DT node. Then mtd->name will be updated by
> > nand_set_flash_node().  
> so you suggest moving the check instead?
> the original code flow was:
>   mtd = NULL;
>   if (!mtd->name)
>      return -EINVAL;
>   mtd = nand_to_mtd(&ebu_host->chip);
>   ...
> 
> by saying that the code itself is valid you're asking me to update the
> flow to the following:
>   mtd = nand_to_mtd(&ebu_host->chip);
>   if (!mtd->name)
>      return -EINVAL;

I actually missed the fact that mtd was used initialized, but yes that
is exactly what I meant!

Thanks,
Miquèl