| Message ID | 20180122093801.19618-1-boris.brezillon@free-electrons.com |
|---|---|
| State | Accepted |
| Delegated to: | Boris Brezillon |
| Headers | show |
| Series | mtd: Make sure the device supports erase operations in mtd_erase() | expand |
Hello Boris, On Mon, 22 Jan 2018 10:38:01 +0100 Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > Some devices do not implement ->_erase() or have an invalid ->erasesize > value. In this case, mtd_erase() should return -ENOTSUPP. > > Note that the test is not done on the MTD_NO_ERASE flag because this > flag means 'erasing a block before writing to it is unnecessary', > not 'the erase operation is not supported'. Actually, some drivers are > setting the MTD_NO_ERASE flag but still implementing the ->_erase() > hook and setting a valid ->erasesize value. > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> > --- > drivers/mtd/mtdcore.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c > index d7ab091b36b2..f24144cbc99c 100644 > --- a/drivers/mtd/mtdcore.c > +++ b/drivers/mtd/mtdcore.c > @@ -971,10 +971,14 @@ EXPORT_SYMBOL_GPL(__put_mtd_device); > */ > int mtd_erase(struct mtd_info *mtd, struct erase_info *instr) > { > + if (!mtd->erasesize || !mtd->_erase) > + return -ENOTSUPP; > + > if (instr->addr >= mtd->size || instr->len > mtd->size - instr->addr) > return -EINVAL; > if (!(mtd->flags & MTD_WRITEABLE)) > return -EROFS; This remark is not inherent to this patch in particular but as we are adding a new error path, I thought it might be interesting to also patch: - INFTL_formatblock() from inftlmount.c [1] (mtd_erase called twice) - NFTL_formatblock() from nftlmount.c [2] They both call mtd_erase() without checking the return code and then error out only if instr->state == MTD_ERASE_FAILED, which has not been set before quitting mtd_erase() in the conditions above. I guess the right thing to do is to add another condition in both functions on the return code of mtd_erase(). What do you think? Otherwise: Reviewed-by: Miquel Raynal <miquel.raynal@free-electrons.com> Have a good day, Miquèl [1] http://elixir.free-electrons.com/linux/latest/source/drivers/mtd/inftlmount.c#L396 [2] http://elixir.free-electrons.com/linux/latest/source/drivers/mtd/nftlmount.c#L334
On Tue, 23 Jan 2018 09:00:46 +0100 Miquel Raynal <miquel.raynal@free-electrons.com> wrote: > Hello Boris, > > On Mon, 22 Jan 2018 10:38:01 +0100 > Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > > > Some devices do not implement ->_erase() or have an invalid ->erasesize > > value. In this case, mtd_erase() should return -ENOTSUPP. > > > > Note that the test is not done on the MTD_NO_ERASE flag because this > > flag means 'erasing a block before writing to it is unnecessary', > > not 'the erase operation is not supported'. Actually, some drivers are > > setting the MTD_NO_ERASE flag but still implementing the ->_erase() > > hook and setting a valid ->erasesize value. > > > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> > > --- > > drivers/mtd/mtdcore.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c > > index d7ab091b36b2..f24144cbc99c 100644 > > --- a/drivers/mtd/mtdcore.c > > +++ b/drivers/mtd/mtdcore.c > > @@ -971,10 +971,14 @@ EXPORT_SYMBOL_GPL(__put_mtd_device); > > */ > > int mtd_erase(struct mtd_info *mtd, struct erase_info *instr) > > { > > + if (!mtd->erasesize || !mtd->_erase) > > + return -ENOTSUPP; > > + > > if (instr->addr >= mtd->size || instr->len > mtd->size - instr->addr) > > return -EINVAL; > > if (!(mtd->flags & MTD_WRITEABLE)) > > return -EROFS; > > This remark is not inherent to this patch in particular but as we are > adding a new error path, I thought it might be interesting to also > patch: > - INFTL_formatblock() from inftlmount.c [1] (mtd_erase called twice) > - NFTL_formatblock() from nftlmount.c [2] > > They both call mtd_erase() without checking the return code and then > error out only if instr->state == MTD_ERASE_FAILED, which has not been > set before quitting mtd_erase() in the conditions above. I guess the > right thing to do is to add another condition in both functions on the > return code of mtd_erase(). What do you think? This sounds reasonable. > > Otherwise: > Reviewed-by: Miquel Raynal <miquel.raynal@free-electrons.com> Thanks. Boris > > Have a good day, > Miquèl > > [1] > http://elixir.free-electrons.com/linux/latest/source/drivers/mtd/inftlmount.c#L396 > [2] > http://elixir.free-electrons.com/linux/latest/source/drivers/mtd/nftlmount.c#L334
On Mon, 22 Jan 2018 10:38:01 +0100 Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > Some devices do not implement ->_erase() or have an invalid ->erasesize > value. In this case, mtd_erase() should return -ENOTSUPP. > > Note that the test is not done on the MTD_NO_ERASE flag because this > flag means 'erasing a block before writing to it is unnecessary', > not 'the erase operation is not supported'. Actually, some drivers are > setting the MTD_NO_ERASE flag but still implementing the ->_erase() > hook and setting a valid ->erasesize value. Applied. > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> > --- > drivers/mtd/mtdcore.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c > index d7ab091b36b2..f24144cbc99c 100644 > --- a/drivers/mtd/mtdcore.c > +++ b/drivers/mtd/mtdcore.c > @@ -971,10 +971,14 @@ EXPORT_SYMBOL_GPL(__put_mtd_device); > */ > int mtd_erase(struct mtd_info *mtd, struct erase_info *instr) > { > + if (!mtd->erasesize || !mtd->_erase) > + return -ENOTSUPP; > + > if (instr->addr >= mtd->size || instr->len > mtd->size - instr->addr) > return -EINVAL; > if (!(mtd->flags & MTD_WRITEABLE)) > return -EROFS; > + > instr->fail_addr = MTD_FAIL_ADDR_UNKNOWN; > if (!instr->len) { > instr->state = MTD_ERASE_DONE;
diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c index d7ab091b36b2..f24144cbc99c 100644 --- a/drivers/mtd/mtdcore.c +++ b/drivers/mtd/mtdcore.c @@ -971,10 +971,14 @@ EXPORT_SYMBOL_GPL(__put_mtd_device); */ int mtd_erase(struct mtd_info *mtd, struct erase_info *instr) { + if (!mtd->erasesize || !mtd->_erase) + return -ENOTSUPP; + if (instr->addr >= mtd->size || instr->len > mtd->size - instr->addr) return -EINVAL; if (!(mtd->flags & MTD_WRITEABLE)) return -EROFS; + instr->fail_addr = MTD_FAIL_ADDR_UNKNOWN; if (!instr->len) { instr->state = MTD_ERASE_DONE;
Some devices do not implement ->_erase() or have an invalid ->erasesize value. In this case, mtd_erase() should return -ENOTSUPP. Note that the test is not done on the MTD_NO_ERASE flag because this flag means 'erasing a block before writing to it is unnecessary', not 'the erase operation is not supported'. Actually, some drivers are setting the MTD_NO_ERASE flag but still implementing the ->_erase() hook and setting a valid ->erasesize value. Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> --- drivers/mtd/mtdcore.c | 4 ++++ 1 file changed, 4 insertions(+)