On 07/08/17 18:51, Ian Molton wrote:

On 07/08/17 12:25, Arend van Spriel wrote:

quoted

quoted

Handling of -ENOMEDIUM is altered, but as that's pretty much broken
anyway
we can ignore that.

Please explain why you think it is broken.

Not got the code to hand right now, but from memory, theres a trapdoor
case where the state can wind up set to something that prevents it ever
being changed again. I'll dig it up when I get back from holiday (this
next few days).

Hi,

Here is the function I had in mind:


void brcmf_sdiod_change_state(struct brcmf_sdio_dev *sdiodev,
                              enum brcmf_sdiod_state state)
{
        if (sdiodev->state == BRCMF_SDIOD_NOMEDIUM ||
            state == sdiodev->state)
                return;

        brcmf_dbg(TRACE, "%d -> %d\n", sdiodev->state, state);
        switch (sdiodev->state) {
        case BRCMF_SDIOD_DATA:
                /* any other state means bus interface is down */
                brcmf_bus_change_state(sdiodev->bus_if, BRCMF_BUS_DOWN);
                break;
        case BRCMF_SDIOD_DOWN:
                /* transition from DOWN to DATA means bus interface is up */
                if (state == BRCMF_SDIOD_DATA)
                        brcmf_bus_change_state(sdiodev->bus_if,
BRCMF_BUS_UP);
                break;
        default:
                break;
        }
        sdiodev->state = state;
}


If it's *ever*  called with state = BRCMF_SDIOD_NOMEDIUM it will
eventually (last line) set sdiodev->state to the same value.

If its ever called again, the first if() statement will make it return
before ever changing sdiodev->state again, no matter what value is
passed for state.

This has to be a bug, surely?

-Ian