Re: [PATCH 5/5] mtd: Stop updating erase_info->state and calling mtd_erase_callback()

[Bert Kenward]

On 12/02/18 21:03, Boris Brezillon wrote:
> MTD users are no longer checking erase_info->state to determine if the
> erase operation failed or succeeded. Moreover, mtd_erase_callback() is
> now a NOP.
> 
> We can safely get rid of all mtd_erase_callback() calls and all
> erase_info->state assignments. While at it, get rid of the
> erase_info->state field, all MTD_ERASE_XXX definitions and the
> mtd_erase_callback() function.
> 
> Signed-off-by: Boris Brezillon 

For sfc parts:

Acked-by: Bert Kenward 


Thanks,

Bert.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 5/5] mtd: Stop updating erase_info->state and calling mtd_erase_callback()

[Miquel Raynal]

Hi Boris,

On Tue, 13 Feb 2018 09:17:14 +0100, Boris Brezillon
 wrote:

> On Tue, 13 Feb 2018 08:42:46 +0100
> Miquel Raynal  wrote:
> 
> > Hi Boris,
> > 
> > Just a few comments about the form.
> > 
> > Otherwise:
> > Reviewed-by: Miquel Raynal 
> > 
> >   
> > > diff --git a/drivers/mtd/devices/lart.c b/drivers/mtd/devices/lart.c
> > > index 555b94406e0b..3d6c8ffd351f 100644
> > > --- a/drivers/mtd/devices/lart.c
> > > +++ b/drivers/mtd/devices/lart.c
> > > @@ -415,7 +415,6 @@ static int flash_erase (struct mtd_info *mtd,struct 
> > > erase_info *instr)
> > >{
> > >   if (!erase_block (addr))
> > > {
> > > -  instr->state = MTD_ERASE_FAILED;
> > >return (-EIO);
> > > }
> > 
> > You can also safely remove these '{' '}'  
> 
> Well, this patch is not about fixing coding style issues, otherwise I'd
> have a lot more work on this driver :-)

Sure, I was not referring to the weird style but just that you switch
from two to one line in the block, thus the braces are not needed
anymore.

> 
> >   
> > >  
> > > @@ -425,9 +424,6 @@ static int flash_erase (struct mtd_info *mtd,struct 
> > > erase_info *instr)
> > >   if (addr == mtd->eraseregions[i].offset + 
> > > (mtd->eraseregions[i].erasesize * mtd->eraseregions[i].numblocks)) i++;
> > >}
> > >  
> > > -   instr->state = MTD_ERASE_DONE;
> > > -   mtd_erase_callback(instr);
> > > -
> > > return (0);
> > >  }
> > >  
> > > diff --git a/drivers/mtd/devices/mtd_dataflash.c 
> > > b/drivers/mtd/devices/mtd_dataflash.c
> > > index 5dc8bd042cc5..aaaeaae01e1d 100644
> > > --- a/drivers/mtd/devices/mtd_dataflash.c
> > > +++ b/drivers/mtd/devices/mtd_dataflash.c
> > > @@ -220,10 +220,6 @@ static int dataflash_erase(struct mtd_info *mtd, 
> > > struct erase_info *instr)
> > >   }
> > >   mutex_unlock(&priv->lock);
> > >  
> > > - /* Inform MTD subsystem that erase is complete */
> > > - instr->state = MTD_ERASE_DONE;
> > > - mtd_erase_callback(instr);
> > > -
> > >   return 0;
> > >  }
> > >  
> > > diff --git a/drivers/mtd/devices/mtdram.c b/drivers/mtd/devices/mtdram.c
> > > index 0bf4aeaf0cb8..efef43c6684b 100644
> > > --- a/drivers/mtd/devices/mtdram.c
> > > +++ b/drivers/mtd/devices/mtdram.c
> > > @@ -60,8 +60,6 @@ static int ram_erase(struct mtd_info *mtd, struct 
> > > erase_info *instr)
> > >   if (check_offs_len(mtd, instr->addr, instr->len))
> > >   return -EINVAL;
> > >   memset((char *)mtd->priv + instr->addr, 0xff, instr->len);
> > > - instr->state = MTD_ERASE_DONE;
> > > - mtd_erase_callback(instr);
> > 
> > Space ?  
> 
> I could add a blank line, but again, I'm just following the coding style
> in place in this file :-).

Ok.

> 
> >   
> > >   return 0;
> > >  }
> > >  
> > > diff --git a/drivers/mtd/devices/phram.c b/drivers/mtd/devices/phram.c
> > > index 7287696a21f9..a963c88d392d 100644
> > > --- a/drivers/mtd/devices/phram.c
> > > +++ b/drivers/mtd/devices/phram.c
> > > @@ -44,8 +44,6 @@ static int phram_erase(struct mtd_info *mtd, struct 
> > > erase_info *instr)
> > >* I don't feel at all ashamed. This kind of thing is possible anyway
> > >* with flash, but unlikely.
> > >*/
> > 
> > Not sure this comment is still relevant? Maybe you could remove it
> > or at least change it? 
> >   
> > > - instr->state = MTD_ERASE_DONE;
> > > - mtd_erase_callback(instr);
> > 
> > Space ?
> >   
> > >   return 0;
> > >  }
> > >  
> > > diff --git a/drivers/mtd/devices/pmc551.c b/drivers/mtd/devices/pmc551.c
> > > index cadea0620cd0..5d842cbca3de 100644
> > > --- a/drivers/mtd/devices/pmc551.c
> > > +++ b/drivers/mtd/devices/pmc551.c
> > > @@ -184,12 +184,10 @@ static int pmc551_erase(struct mtd_info *mtd, 
> > > struct erase_info *instr)
> > >   }
> > >  
> > >out:
> > > - instr->state = MTD_ERASE_DONE;
> > >  #ifdef CONFIG_MTD_PMC551_DEBUG
> > >   printk(KERN_DEBUG "pmc551_erase() done\n");
> > >  #endif
> > >  
> > > - mtd_erase_callback(instr);
> > >   return 0;
> > >  }
> > >  
> > > diff --git a/drivers/mtd/devices/powernv_flash.c 
> > > b/drivers/mtd/devices/powernv_flash.c
> > > index 26f9feaa5d17..5f383630c16f 100644
> > > --- a/drivers/mtd/devices/powernv_flash.c
> > > +++ b/drivers/mtd/devices/powernv_flash.c
> > > @@ -175,19 +175,12 @@ static int powernv_flash_erase(struct mtd_info 
> > > *mtd, struct erase_info *erase)
> > >  {
> > >   int rc;
> > >  
> > > - erase->state = MTD_ERASING;
> > > -
> > >   /* todo: register our own notifier to do a true async implementation */
> > >   rc =  powernv_flash_async_op(mtd, FLASH_OP_ERASE, erase->addr,
> > >   erase->len, NULL, NULL);
> > 
> > Are you sure this is still needed? Maybe this should go away in your
> > first patch?  
> 
> Hm, indeed. This comment should be dropped.
> 
> >   
> > > -
> > > - if (rc) {
> > > + if (rc)
> > >   erase->fail_addr = erase->addr;
> > > - erase->state = MTD_ERASE_FAILED;
> > > - } else {
> > > - eras

Re: [PATCH 5/5] mtd: Stop updating erase_info->state and calling mtd_erase_callback()

[Boris Brezillon]

On Tue, 13 Feb 2018 08:42:46 +0100
Miquel Raynal  wrote:

> Hi Boris,
> 
> Just a few comments about the form.
> 
> Otherwise:
> Reviewed-by: Miquel Raynal 
> 
> 
> > diff --git a/drivers/mtd/devices/lart.c b/drivers/mtd/devices/lart.c
> > index 555b94406e0b..3d6c8ffd351f 100644
> > --- a/drivers/mtd/devices/lart.c
> > +++ b/drivers/mtd/devices/lart.c
> > @@ -415,7 +415,6 @@ static int flash_erase (struct mtd_info *mtd,struct 
> > erase_info *instr)
> >  {
> > if (!erase_block (addr))
> >   {
> > -instr->state = MTD_ERASE_FAILED;
> >  return (-EIO);
> >   }  
> 
> You can also safely remove these '{' '}'

Well, this patch is not about fixing coding style issues, otherwise I'd
have a lot more work on this driver :-)

> 
> >  
> > @@ -425,9 +424,6 @@ static int flash_erase (struct mtd_info *mtd,struct 
> > erase_info *instr)
> > if (addr == mtd->eraseregions[i].offset + 
> > (mtd->eraseregions[i].erasesize * mtd->eraseregions[i].numblocks)) i++;
> >  }
> >  
> > -   instr->state = MTD_ERASE_DONE;
> > -   mtd_erase_callback(instr);
> > -
> > return (0);
> >  }
> >  
> > diff --git a/drivers/mtd/devices/mtd_dataflash.c 
> > b/drivers/mtd/devices/mtd_dataflash.c
> > index 5dc8bd042cc5..aaaeaae01e1d 100644
> > --- a/drivers/mtd/devices/mtd_dataflash.c
> > +++ b/drivers/mtd/devices/mtd_dataflash.c
> > @@ -220,10 +220,6 @@ static int dataflash_erase(struct mtd_info *mtd, 
> > struct erase_info *instr)
> > }
> > mutex_unlock(&priv->lock);
> >  
> > -   /* Inform MTD subsystem that erase is complete */
> > -   instr->state = MTD_ERASE_DONE;
> > -   mtd_erase_callback(instr);
> > -
> > return 0;
> >  }
> >  
> > diff --git a/drivers/mtd/devices/mtdram.c b/drivers/mtd/devices/mtdram.c
> > index 0bf4aeaf0cb8..efef43c6684b 100644
> > --- a/drivers/mtd/devices/mtdram.c
> > +++ b/drivers/mtd/devices/mtdram.c
> > @@ -60,8 +60,6 @@ static int ram_erase(struct mtd_info *mtd, struct 
> > erase_info *instr)
> > if (check_offs_len(mtd, instr->addr, instr->len))
> > return -EINVAL;
> > memset((char *)mtd->priv + instr->addr, 0xff, instr->len);
> > -   instr->state = MTD_ERASE_DONE;
> > -   mtd_erase_callback(instr);  
> 
> Space ?

I could add a blank line, but again, I'm just following the coding style
in place in this file :-).

> 
> > return 0;
> >  }
> >  
> > diff --git a/drivers/mtd/devices/phram.c b/drivers/mtd/devices/phram.c
> > index 7287696a21f9..a963c88d392d 100644
> > --- a/drivers/mtd/devices/phram.c
> > +++ b/drivers/mtd/devices/phram.c
> > @@ -44,8 +44,6 @@ static int phram_erase(struct mtd_info *mtd, struct 
> > erase_info *instr)
> >  * I don't feel at all ashamed. This kind of thing is possible anyway
> >  * with flash, but unlikely.
> >  */  
> 
> Not sure this comment is still relevant? Maybe you could remove it
> or at least change it? 
> 
> > -   instr->state = MTD_ERASE_DONE;
> > -   mtd_erase_callback(instr);  
> 
> Space ?
> 
> > return 0;
> >  }
> >  
> > diff --git a/drivers/mtd/devices/pmc551.c b/drivers/mtd/devices/pmc551.c
> > index cadea0620cd0..5d842cbca3de 100644
> > --- a/drivers/mtd/devices/pmc551.c
> > +++ b/drivers/mtd/devices/pmc551.c
> > @@ -184,12 +184,10 @@ static int pmc551_erase(struct mtd_info *mtd, struct 
> > erase_info *instr)
> > }
> >  
> >out:
> > -   instr->state = MTD_ERASE_DONE;
> >  #ifdef CONFIG_MTD_PMC551_DEBUG
> > printk(KERN_DEBUG "pmc551_erase() done\n");
> >  #endif
> >  
> > -   mtd_erase_callback(instr);
> > return 0;
> >  }
> >  
> > diff --git a/drivers/mtd/devices/powernv_flash.c 
> > b/drivers/mtd/devices/powernv_flash.c
> > index 26f9feaa5d17..5f383630c16f 100644
> > --- a/drivers/mtd/devices/powernv_flash.c
> > +++ b/drivers/mtd/devices/powernv_flash.c
> > @@ -175,19 +175,12 @@ static int powernv_flash_erase(struct mtd_info *mtd, 
> > struct erase_info *erase)
> >  {
> > int rc;
> >  
> > -   erase->state = MTD_ERASING;
> > -
> > /* todo: register our own notifier to do a true async implementation */
> > rc =  powernv_flash_async_op(mtd, FLASH_OP_ERASE, erase->addr,
> > erase->len, NULL, NULL);  
> 
> Are you sure this is still needed? Maybe this should go away in your
> first patch?

Hm, indeed. This comment should be dropped.

> 
> > -
> > -   if (rc) {
> > +   if (rc)
> > erase->fail_addr = erase->addr;
> > -   erase->state = MTD_ERASE_FAILED;
> > -   } else {
> > -   erase->state = MTD_ERASE_DONE;
> > -   }
> > -   mtd_erase_callback(erase);
> > +
> > return rc;
> >  }
> >  
> > diff --git a/drivers/mtd/devices/slram.c b/drivers/mtd/devices/slram.c
> > index 0ec85f316d24..2f05e1801047 100644
> > --- a/drivers/mtd/devices/slram.c
> > +++ b/drivers/mtd/devices/slram.c
> > @@ -88,8 +88,6 @@ static int slram_erase(struct mtd_info *mtd, struct 
> > erase_info *instr)
> >  * I don't feel at all ashamed. This kind of thing is po

Re: [PATCH 5/5] mtd: Stop updating erase_info->state and calling mtd_erase_callback()

[Miquel Raynal]

Hi Boris,

Just a few comments about the form.

Otherwise:
Reviewed-by: Miquel Raynal 


> diff --git a/drivers/mtd/devices/lart.c b/drivers/mtd/devices/lart.c
> index 555b94406e0b..3d6c8ffd351f 100644
> --- a/drivers/mtd/devices/lart.c
> +++ b/drivers/mtd/devices/lart.c
> @@ -415,7 +415,6 @@ static int flash_erase (struct mtd_info *mtd,struct 
> erase_info *instr)
>{
>   if (!erase_block (addr))
> {
> -  instr->state = MTD_ERASE_FAILED;
>return (-EIO);
> }

You can also safely remove these '{' '}'

>  
> @@ -425,9 +424,6 @@ static int flash_erase (struct mtd_info *mtd,struct 
> erase_info *instr)
>   if (addr == mtd->eraseregions[i].offset + 
> (mtd->eraseregions[i].erasesize * mtd->eraseregions[i].numblocks)) i++;
>}
>  
> -   instr->state = MTD_ERASE_DONE;
> -   mtd_erase_callback(instr);
> -
> return (0);
>  }
>  
> diff --git a/drivers/mtd/devices/mtd_dataflash.c 
> b/drivers/mtd/devices/mtd_dataflash.c
> index 5dc8bd042cc5..aaaeaae01e1d 100644
> --- a/drivers/mtd/devices/mtd_dataflash.c
> +++ b/drivers/mtd/devices/mtd_dataflash.c
> @@ -220,10 +220,6 @@ static int dataflash_erase(struct mtd_info *mtd, struct 
> erase_info *instr)
>   }
>   mutex_unlock(&priv->lock);
>  
> - /* Inform MTD subsystem that erase is complete */
> - instr->state = MTD_ERASE_DONE;
> - mtd_erase_callback(instr);
> -
>   return 0;
>  }
>  
> diff --git a/drivers/mtd/devices/mtdram.c b/drivers/mtd/devices/mtdram.c
> index 0bf4aeaf0cb8..efef43c6684b 100644
> --- a/drivers/mtd/devices/mtdram.c
> +++ b/drivers/mtd/devices/mtdram.c
> @@ -60,8 +60,6 @@ static int ram_erase(struct mtd_info *mtd, struct 
> erase_info *instr)
>   if (check_offs_len(mtd, instr->addr, instr->len))
>   return -EINVAL;
>   memset((char *)mtd->priv + instr->addr, 0xff, instr->len);
> - instr->state = MTD_ERASE_DONE;
> - mtd_erase_callback(instr);

Space ?

>   return 0;
>  }
>  
> diff --git a/drivers/mtd/devices/phram.c b/drivers/mtd/devices/phram.c
> index 7287696a21f9..a963c88d392d 100644
> --- a/drivers/mtd/devices/phram.c
> +++ b/drivers/mtd/devices/phram.c
> @@ -44,8 +44,6 @@ static int phram_erase(struct mtd_info *mtd, struct 
> erase_info *instr)
>* I don't feel at all ashamed. This kind of thing is possible anyway
>* with flash, but unlikely.
>*/

Not sure this comment is still relevant? Maybe you could remove it
or at least change it? 

> - instr->state = MTD_ERASE_DONE;
> - mtd_erase_callback(instr);

Space ?

>   return 0;
>  }
>  
> diff --git a/drivers/mtd/devices/pmc551.c b/drivers/mtd/devices/pmc551.c
> index cadea0620cd0..5d842cbca3de 100644
> --- a/drivers/mtd/devices/pmc551.c
> +++ b/drivers/mtd/devices/pmc551.c
> @@ -184,12 +184,10 @@ static int pmc551_erase(struct mtd_info *mtd, struct 
> erase_info *instr)
>   }
>  
>out:
> - instr->state = MTD_ERASE_DONE;
>  #ifdef CONFIG_MTD_PMC551_DEBUG
>   printk(KERN_DEBUG "pmc551_erase() done\n");
>  #endif
>  
> - mtd_erase_callback(instr);
>   return 0;
>  }
>  
> diff --git a/drivers/mtd/devices/powernv_flash.c 
> b/drivers/mtd/devices/powernv_flash.c
> index 26f9feaa5d17..5f383630c16f 100644
> --- a/drivers/mtd/devices/powernv_flash.c
> +++ b/drivers/mtd/devices/powernv_flash.c
> @@ -175,19 +175,12 @@ static int powernv_flash_erase(struct mtd_info *mtd, 
> struct erase_info *erase)
>  {
>   int rc;
>  
> - erase->state = MTD_ERASING;
> -
>   /* todo: register our own notifier to do a true async implementation */
>   rc =  powernv_flash_async_op(mtd, FLASH_OP_ERASE, erase->addr,
>   erase->len, NULL, NULL);

Are you sure this is still needed? Maybe this should go away in your
first patch?

> -
> - if (rc) {
> + if (rc)
>   erase->fail_addr = erase->addr;
> - erase->state = MTD_ERASE_FAILED;
> - } else {
> - erase->state = MTD_ERASE_DONE;
> - }
> - mtd_erase_callback(erase);
> +
>   return rc;
>  }
>  
> diff --git a/drivers/mtd/devices/slram.c b/drivers/mtd/devices/slram.c
> index 0ec85f316d24..2f05e1801047 100644
> --- a/drivers/mtd/devices/slram.c
> +++ b/drivers/mtd/devices/slram.c
> @@ -88,8 +88,6 @@ static int slram_erase(struct mtd_info *mtd, struct 
> erase_info *instr)
>* I don't feel at all ashamed. This kind of thing is possible anyway
>* with flash, but unlikely.
>*/

Same with this comment.

> - instr->state = MTD_ERASE_DONE;
> - mtd_erase_callback(instr);

Space ?

>   return(0);
>  }
>  




-- 
Miquel Raynal, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 5/5] mtd: Stop updating erase_info->state and calling mtd_erase_callback()

[Richard Weinberger]

Am Montag, 12. Februar 2018, 22:03:11 CET schrieb Boris Brezillon:
> MTD users are no longer checking erase_info->state to determine if the
> erase operation failed or succeeded. Moreover, mtd_erase_callback() is
> now a NOP.
> 
> We can safely get rid of all mtd_erase_callback() calls and all
> erase_info->state assignments. While at it, get rid of the
> erase_info->state field, all MTD_ERASE_XXX definitions and the
> mtd_erase_callback() function.
> 
> Signed-off-by: Boris Brezillon 

Reviewed-by: Richard Weinberger 

Thanks,
//richard

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel