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

[Guenter Roeck <linux@roeck-us.net>]

On Wed, Feb 05, 2020 at 11:08:07AM +0100, Cédric Le Goater wrote:
> On 2/4/20 3:28 PM, Guenter Roeck wrote:
> > 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.
> 
> ok. we will need to model SFDP one day.
> 
> Are you using the OpenBMC images or do you have your own ? 
> 

I am running images built from upstream/stable kernel branches.

Guenter

> > 
> >>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> >>> ---
> >>>   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.
> 
> ok.
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> 
> 
> Thanks,
> 
> C.
>  
> > 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;
> >>>       }
> >>>
> >>
> > 
>