Skip to contentSkip to conversation

Patch 'usb: convert drivers/usb/* to use module_platform_driver_probe()' applied to my tree

DISCUSSION Russell King - ARM Linux 1 participant 1 message Jan 10, 2013 3 messages not in the archive MMC, SD and SDIO

lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

A message that is not in the archive (sent off-list or not yet received): CAHkwnC-vaeZ53chLcgLW3H8_MR-yG9uNoPQ+qg4ddjzVT-8qUQ@mail.gmail.com

A message that is not in the archive (sent off-list or not yet received): [email protected]

#3 lore raw
Quoted 3 lines · Felipe Balbi

we just need to make sure that Greg merges my pull request after driver-core. Another possibility is that I drop this patch from my tree and Greg takes the entire series through driver-core.

What would also be good is if someone also reviews the probe cleanup and remove paths too. I just grepped for platform_driver_probe() in drivers/mmc - which hit three drivers. Of those three drivers, all _three_ had bugs there. It took me about two seconds of looking to find all these, and I wasn't even looking for this.

First off, mvsdio.c. C _is_ _not_ PYTHON!

        if (r)
                release_resource(r);
        if (mmc)
                if (!IS_ERR_OR_NULL(host->clk)) {
                        clk_disable_unprepare(host->clk);
                        clk_put(host->clk);
                }
                mmc_free_host(mmc);

        return ret;

and yes, this path can be reached with mmc = NULL.

Secondly:

                if (host->gpio_card_detect) {
                        free_irq(gpio_to_irq(host->gpio_card_detect), host);
                        gpio_free(host->gpio_card_detect);
                }
                if (host->gpio_write_protect)
                        gpio_free(host->gpio_write_protect);

GPIO 0 is a valid GPIO. Luckily, we won't probe GPIO 0 at probe time, but this is still buggy.

Thirdly:

probe():

        r = request_mem_region(r->start, SZ_1K, DRIVER_NAME);
        if (!r)
                return -EBUSY;
        host->res = r;
...
        if (r)
                release_resource(r);
...
release():
                release_resource(host->res);

This is not how we free resources allocated by request_mem_region().

Next up, atmel-mci.c:

        if (pdata->slot[0].bus_width) {
                ret = atmci_init_slot(host, &pdata->slot[0],
                                0, ATMCI_SDCSEL_SLOT_A, ATMCI_SDIOIRQA);
...
        if (pdata->slot[1].bus_width) {
                ret = atmci_init_slot(host, &pdata->slot[1],
                                1, ATMCI_SDCSEL_SLOT_B, ATMCI_SDIOIRQB);
...
        if (!host->caps.has_rwproof) {
                host->buffer = dma_alloc_coherent(&pdev->dev, host->buf_size,
                                                  &host->buf_phys_addr,
                                                  GFP_KERNEL);
                if (!host->buffer) {
                        ret = -ENOMEM;
                        dev_err(&pdev->dev, "buffer allocation failed\n");
                        goto err_init_slot;
                }
        }
...
err_init_slot:
        if (host->dma.chan)
                dma_release_channel(host->dma.chan);
        free_irq(irq, host);
err_request_irq:
        iounmap(host->regs);
err_ioremap:
        clk_put(host->mck);
err_clk_get:
        kfree(host);
        return ret;

Great, so on DMA buffer failure (allocated _after_ we publish the interfaces to the rest of the system - which is in itself buggy) we don't remove these interfaces, but we free all the resources and memory beneath them.

Finally, davinci_mmc.c:

probe():

        mem = request_mem_region(r->start, mem_size, pdev->name);
...
        host->mem_res = mem;
...
        if (mem)
                release_resource(mem);
...
release():
                release_resource(host->mem_res);

Again, this is not how we free resources allocated by request_mem_region() and leads to memory leaks.