| Message ID | 20200203180904.2727-2-linux@roeck-us.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] m25p80: Convert to support tracing | expand |
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. > 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. > > - 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; > } >
Hi Guenter, 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. Two changes, I'd rather see 2 patches. If you happen to respin they are welcome. As the split is trivial maybe a block maintainer is OK to do it. Regardless the outcome: Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > > 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. > > 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; > + } > > - 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; > } >
On 2/4/20 9: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. > >> 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. I am not sure, since a guest can queue various requests in a single frame. Guenter patch seems correct to me. All the others uses in decode_new_cmd() fill data[] up to the correct length, so are safe. In complete_collecting_data() the access to data[] should be protected by STATE_COLLECTING_VAR_LEN_DATA (state only changes when enough data). >> >> - 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; >> } >> > >
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 <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. 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; >> } >> >
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 <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; >>> } >>> >> >
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; > >>> } > >>> > >> > > >
On Wed, 5 Feb 2020 at 17:43, Guenter Roeck <linux@roeck-us.net> wrote: > > On Wed, Feb 05, 2020 at 11:08:07AM +0100, Cédric Le Goater wrote: > > > > 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. I think Cédric was wondering which flash images and therefore filesystems you were using in your testing. I had a poke around here but I couldn't work out where 'mtd32' came from: https://github.com/groeck/linux-build-test/blob/master/rootfs/arm/run-qemu-arm.sh Cheers, Joel
On 2/5/20 11:04 PM, Joel Stanley wrote: > On Wed, 5 Feb 2020 at 17:43, Guenter Roeck <linux@roeck-us.net> wrote: >> >> On Wed, Feb 05, 2020 at 11:08:07AM +0100, Cédric Le Goater wrote: >>> >>> 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. > > I think Cédric was wondering which flash images and therefore > filesystems you were using in your testing. > Ah, ok. Sorry, misunderstood. > I had a poke around here but I couldn't work out where 'mtd32' came from: > > https://github.com/groeck/linux-build-test/blob/master/rootfs/arm/run-qemu-arm.sh > mtd32 tells the infrastructure that the boot device is mtd (flash) with 32MB capacity. The infrastructure uses that to create the actual flash image and to generate the qemu command line. The root file system is is rootfs-armv5.ext2 (located in the same directory as the run script). It was generated using buildroot. Guenter
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; + } - 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; }
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. Signed-off-by: Guenter Roeck <linux@roeck-us.net> --- hw/block/m25p80.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)