Re: [PATCH 2/3] m25p80: Improve command handling for Jedec and unsupported commands

[Guenter Roeck]

On 2/4/20 12:53 AM, Cédric Le Goater wrote:
> On 2/3/20 7:09 PM, Guenter Roeck wrote:
> > Always report 6 bytes of JEDEC data. Fill remaining data with 0.
> >
> > For unsupported commands, keep sending a value of 0 until the chip
> > is deselected.
> >
> > Both changes avoid attempts to decode random commands. Up to now this
> > happened if the reported Jedec data was shorter than 6 bytes but the
> > host read 6 bytes, and with all unsupported commands.
>
> Do you have a concrete example for that ? machine and flash model.

I noticed it while tracking down the bug fixed in patch 3 of the series,
ie with AST2500 evb using w25q256 flash, but it happens with all machines
using SPI NOR flash (ie all aspeed bmc machines) when running the Linux
kernel. Most of the time it doesn't cause harm, unless the host sends
an "address" as part of an unsupported command which happens to include
a valid command code.

> > Signed-off-by: Guenter Roeck <address@hidden>
> > ---
> >   hw/block/m25p80.c | 10 +++++++++-
> >   1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> > index 63e050d7d3..aca75edcc1 100644
> > --- a/hw/block/m25p80.c
> > +++ b/hw/block/m25p80.c
> > @@ -1040,8 +1040,11 @@ static void decode_new_cmd(Flash *s, uint32_t value)
> >           for (i = 0; i < s->pi->id_len; i++) {
> >               s->data[i] = s->pi->id[i];
> >           }
> > +        for (; i < SPI_NOR_MAX_ID_LEN; i++) {
> > +            s->data[i] = 0;
> > +        }
>
> It seems that data should be reseted in m25p80_cs() also.
Are you sure ?

The current implementation sets s->data[] as needed when command decode
is complete. That seems less costly to me.

Thanks,
Guenter

> >   
> > -        s->len = s->pi->id_len;
> > +        s->len = SPI_NOR_MAX_ID_LEN;
> >           s->pos = 0;
> >           s->state = STATE_READING_DATA;
> >           break;
> > @@ -1158,6 +1161,11 @@ static void decode_new_cmd(Flash *s, uint32_t value)
> >           s->quad_enable = false;
> >           break;
> >       default:
> > +        s->pos = 0;
> > +        s->len = 1;
> > +        s->state = STATE_READING_DATA;
> > +        s->data_read_loop = true;
> > +        s->data[0] = 0;
> >           qemu_log_mask(LOG_GUEST_ERROR, "M25P80: Unknown cmd %x\n", value);
> >           break;
> >       }