Re: [PATCH 07/34] brcmfmac: Remove brcmf_sdiod_request_data()

[Ian Molton <ian@xxxxxxxxxxxxxx>]

On 07/08/17 18:51, Ian Molton wrote:
> On 07/08/17 12:25, Arend van Spriel wrote:
>>> 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