Page 1 of 1

Bug in the m25qStop() function in m25q.c driver  Topic is solved

Posted: Mon Sep 24, 2018 6:13 pm
by wpaul
Hello:

This weekend I wrote a QSPI driver for ChibiOS for a project I'm working on. I got things to work, but along the way I discovered a bug in the m25qStop() function which I thought I should report.

Relevant details:
OS version: ChibiOS 18.1.2 release
Platform: Nordic nRF52840 DK development board
MCU: Nordic nRF52840 (1MB flash, 256KB RAM)
CPU: ARM Cortex-M4 running at 64MHz
QSPI flash: Macronix MX25R6435F
Compiler: GCC 7.1.0
Driver modules: hal_qspi, hal_flash, m25q

I'm using a modified version of the NRF52 platform support code from the ChibiOS community repo. (The NRF52840 is very similar to the NRF52832; it just has more flash/RAM and some added peripherals.) While I used the 18.1.2 release tarball for my project, the bug seems to be present in the latest code in the git repo too:

https://github.com/ChibiOS/ChibiOS/blob ... ron/m25q.c

Here we can see that the m25qStop() routine looks like this:

[...]
void m25qStop(M25QDriver *devp) {

osalDbgCheck(devp != NULL);
osalDbgAssert(devp->state != FLASH_UNINIT, "invalid state");

if (devp->state != FLASH_STOP) {

/* Bus acquisition.*/
jesd216_bus_acquire(devp->config->busp, devp->config->buscfg);

/* Stopping bus device.*/
jesd216_stop(devp->config->busp);

/* Deleting current configuration.*/
devp->config = NULL;

/* Driver stopped.*/
devp->state = FLASH_STOP;

/* Bus release.*/
jesd216_bus_release(devp->config->busp);
}
}
[...]

The bug is pretty simple: the devp->config pointer is set to NULL, but then two lines later it's dereferenced again in the call to jesd216_bus_release(). At least on the ARM processor in the Nordic MCU, this triggers a hard fault trap. In my local sources I corrected this by moving the "Deleting current configuration" part after the "Bus release" part.

Aside from this, things worked pretty much as expected. Note that I'm aware that the m25q driver is for Micron parts and my board has a Macronix part. But the m25q driver seems to work well enough that all the functionality I need works (read/erase/program/memmap).

-Bill

Re: Bug in the m25qStop() function in m25q.c driver

Posted: Mon Sep 24, 2018 10:08 pm
by wpaul
Oops, I just realized: instead of version 18.1.2, that should be 18.2.1. My bad.

-Bill

Re: Bug in the m25qStop() function in m25q.c driver

Posted: Tue Sep 25, 2018 12:17 pm
by Giovanni
Hi,

Note that under \os\hal\lib\complex\m25q there is a new version of the driver that allows to support multiple vendors, it will replace the current one. Probably it has the same bug, have to verify.

Giovanni

Re: Bug in the m25qStop() function in m25q.c driver

Posted: Thu Nov 15, 2018 11:09 am
by Giovanni
Fixed as bug #988.

Note that the whole flash infrastructure has been modified in trunk code, you may want to give it a try.

Giovanni