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

[Cédric Le Goater]

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 ? 

> 
>>> 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.

ok.
Reviewed-by: Cédric Le Goater <address@hidden>


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;
>>>       }
>>>
>>
>