mtd: spinand: wait on erase success in spinand_markbad?

mtd: spinand: wait on erase success in spinand_markbad?

[Emil Lenngren]

Hi,

I was just reading the spinand driver and found this piece of code:
static int spinand_markbad(struct nand_device *nand, const struct nand_pos *pos)
{
    struct spinand_device *spinand = nand_to_spinand(nand);
    struct nand_page_io_req req = {
        .pos = *pos,
        .ooboffs = 0,
        .ooblen = 2,
        .oobbuf.out = spinand->oobbuf,
    };
    int ret;

    /* Erase block before marking it bad. */
    ret = spinand_select_target(spinand, pos->target);
    if (ret)
        return ret;

    ret = spinand_write_enable_op(spinand);
    if (ret)
        return ret;

    spinand_erase_op(spinand, pos);

    memset(spinand->oobbuf, 0, 2);
    return spinand_write_page(spinand, &req);
}

Shouldn't there be spinand_wait call after spinand_erase_op and before
spinand_write_page?

/Emil

Re: mtd: spinand: wait on erase success in spinand_markbad?

[Boris Brezillon]

On Wed, 19 Dec 2018 14:27:51 +0100
Emil Lenngren <emil.lenngren@gmail.com> wrote:

> Hi,
> 
> I was just reading the spinand driver and found this piece of code:
> static int spinand_markbad(struct nand_device *nand, const struct nand_pos *pos)
> {
>     struct spinand_device *spinand = nand_to_spinand(nand);
>     struct nand_page_io_req req = {
>         .pos = *pos,
>         .ooboffs = 0,
>         .ooblen = 2,
>         .oobbuf.out = spinand->oobbuf,
>     };
>     int ret;
> 
>     /* Erase block before marking it bad. */
>     ret = spinand_select_target(spinand, pos->target);
>     if (ret)
>         return ret;
> 
>     ret = spinand_write_enable_op(spinand);
>     if (ret)
>         return ret;
> 
>     spinand_erase_op(spinand, pos);
> 
>     memset(spinand->oobbuf, 0, 2);
>     return spinand_write_page(spinand, &req);
> }
> 
> Shouldn't there be spinand_wait call after spinand_erase_op and before
> spinand_write_page?
> 

Absolutely. Can you send a patch to fix that?

Thanks,

Boris

Re: mtd: spinand: wait on erase success in spinand_markbad?

[Emil Lenngren]

Den ons 19 dec. 2018 kl 19:56 skrev Boris Brezillon <bbrezillon@kernel.org>:
>
> On Wed, 19 Dec 2018 14:27:51 +0100
> Emil Lenngren <emil.lenngren@gmail.com> wrote:
>
> > Hi,
> >
> > I was just reading the spinand driver and found this piece of code:
> > static int spinand_markbad(struct nand_device *nand, const struct nand_pos *pos)
> > {
> >     struct spinand_device *spinand = nand_to_spinand(nand);
> >     struct nand_page_io_req req = {
> >         .pos = *pos,
> >         .ooboffs = 0,
> >         .ooblen = 2,
> >         .oobbuf.out = spinand->oobbuf,
> >     };
> >     int ret;
> >
> >     /* Erase block before marking it bad. */
> >     ret = spinand_select_target(spinand, pos->target);
> >     if (ret)
> >         return ret;
> >
> >     ret = spinand_write_enable_op(spinand);
> >     if (ret)
> >         return ret;
> >
> >     spinand_erase_op(spinand, pos);
> >
> >     memset(spinand->oobbuf, 0, 2);
> >     return spinand_write_page(spinand, &req);
> > }
> >
> > Shouldn't there be spinand_wait call after spinand_erase_op and before
> > spinand_write_page?
> >
>
> Absolutely. Can you send a patch to fix that?
>

Sure, although I'm a bit unsure how the error handling should be done.
In the u-boot version
(https://github.com/u-boot/u-boot/blob/9e5c2a755a6ca5f3931de548f43101d0d18ac003/drivers/mtd/nand/spi/core.c#L718),
if the spinand_erase_op spi transfer fails, the function quits without
writing the bad block marker, while the linux version ignores the
error. Is the intention that the bad block marker should be written
regardless if the erase spi transfer fails or not, or is it just a
mistake?

> Thanks,
>
> Boris

Re: mtd: spinand: wait on erase success in spinand_markbad?

[Boris Brezillon]

On Wed, 19 Dec 2018 20:54:33 +0100
Emil Lenngren <emil.lenngren@gmail.com> wrote:

> Den ons 19 dec. 2018 kl 19:56 skrev Boris Brezillon <bbrezillon@kernel.org>:
> >
> > On Wed, 19 Dec 2018 14:27:51 +0100
> > Emil Lenngren <emil.lenngren@gmail.com> wrote:
> >  
> > > Hi,
> > >
> > > I was just reading the spinand driver and found this piece of code:
> > > static int spinand_markbad(struct nand_device *nand, const struct nand_pos *pos)
> > > {
> > >     struct spinand_device *spinand = nand_to_spinand(nand);
> > >     struct nand_page_io_req req = {
> > >         .pos = *pos,
> > >         .ooboffs = 0,
> > >         .ooblen = 2,
> > >         .oobbuf.out = spinand->oobbuf,
> > >     };
> > >     int ret;
> > >
> > >     /* Erase block before marking it bad. */
> > >     ret = spinand_select_target(spinand, pos->target);
> > >     if (ret)
> > >         return ret;
> > >
> > >     ret = spinand_write_enable_op(spinand);
> > >     if (ret)
> > >         return ret;
> > >
> > >     spinand_erase_op(spinand, pos);
> > >
> > >     memset(spinand->oobbuf, 0, 2);
> > >     return spinand_write_page(spinand, &req);
> > > }
> > >
> > > Shouldn't there be spinand_wait call after spinand_erase_op and before
> > > spinand_write_page?
> > >  
> >
> > Absolutely. Can you send a patch to fix that?
> >  
> 
> Sure, although I'm a bit unsure how the error handling should be done.
> In the u-boot version
> (https://github.com/u-boot/u-boot/blob/9e5c2a755a6ca5f3931de548f43101d0d18ac003/drivers/mtd/nand/spi/core.c#L718),
> if the spinand_erase_op spi transfer fails, the function quits without
> writing the bad block marker, while the linux version ignores the
> error. Is the intention that the bad block marker should be written
> regardless if the erase spi transfer fails or not, or is it just a
> mistake?

To be honest, I don't think the erase operation is needed (we just
want to write 0 in the BBM), so we don't really care if it fails, which
means Linux version is correct and u-boot is probably buggy. Note that
you're unlikely to hit a failure on the erase operation anyway, so
it's more a theoretical bug than a real one.