Andrew Morton wrote:
> Pierre Ossman <[email protected]> wrote:
>> Driver for the Secure Digital Host Controller Interface specification.
>>
>> ...
>>
>> +static void sdhci_transfer_pio(struct sdhci_host *host)
>> +{
>> + char *buffer;
>> + u32 mask;
>> + int bytes, size;
>> + unsigned long max_jiffies;
>> +
>> + BUG_ON(!host->data);
>> +
>> + if (host->num_sg == 0)
>> + return;
>> +
>> + bytes = 0;
>> + if (host->data->flags & MMC_DATA_READ)
>> + mask = SDHCI_DATA_AVAILABLE;
>> + else
>> + mask = SDHCI_SPACE_AVAILABLE;
>> +
>> + buffer = sdhci_kmap_sg(host) + host->offset;
>> +
>> + /* Transfer shouldn't take more than 5 s */
>> + max_jiffies = jiffies + HZ * 5;
>> +
>> + while (host->size > 0) {
>> + if (time_after(jiffies, max_jiffies)) {
>> + printk(KERN_ERR "%s: PIO transfer stalled. "
>> + "Please report this to "
>> + BUGMAIL ".\n", mmc_hostname(host->mmc));
>> + sdhci_dumpregs(host);
>> + host->data->error = MMC_ERR_FAILED;
>> + sdhci_finish_data(host);
>> + return;
>
> This returns with the atomic kmap still held. Ugly things will happen.
>
Ouch. Missed that. Thanks for pointing it out.
>> +static void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>> +{
>> + struct sdhci_host *host;
>> + unsigned long flags;
>> + u8 ctrl;
>> +
>> + host = mmc_priv(mmc);
>> +
>> + spin_lock_irqsave(&host->lock, flags);
>> +
>> + DBG("clock %uHz busmode %u powermode %u cs %u Vdd %u width %u\n",
>> + ios->clock, ios->bus_mode, ios->power_mode, ios->chip_select,
>> + ios->vdd, ios->bus_width);
>> +
>> + /*
>> + * Reset the chip on each power off.
>> + * Should clear out any weird states.
>> + */
>> + if (ios->power_mode == MMC_POWER_OFF)
>> + sdhci_init(host);
>
> This can do a 50 millisecond busywait with interrupts disabled. I'd
> suggest you pop the lock and do a schedule_timeout().
>
I'll have to have an extra look at what can happen in interrupt land first.
Not sure we can use a scheduled wait though. set_ios could be reachable
from interrupt context.
>> +static int sdhci_get_ro(struct mmc_host *mmc)
>> +{
>> + struct sdhci_host *host;
>> + unsigned long flags;
>> + int present;
>> +
>> + host = mmc_priv(mmc);
>> +
>> + spin_lock_irqsave(&host->lock, flags);
>> +
>> + present = readl(host->ioaddr + SDHCI_PRESENT_STATE);
>> +
>> + spin_unlock_irqrestore(&host->lock, flags);
>
> Does a readl() need a lock?
>
Probably not. It's more a consistency thing. :)
>> + init_timer(&host->timer);
>> + host->timer.data = (unsigned long)host;
>> + host->timer.function = sdhci_timeout_timer;
>
> setup_timer().
>
>> +static void sdhci_remove_slot(struct pci_dev *pdev, int slot)
>> +{
>> + struct sdhci_chip *chip;
>> + struct mmc_host *mmc;
>> + struct sdhci_host *host;
>> +
>> + chip = pci_get_drvdata(pdev);
>> + host = chip->hosts[slot];
>> + mmc = host->mmc;
>> +
>> + chip->hosts[slot] = NULL;
>> +
>> + mmc_remove_host(mmc);
>> +
>> + del_timer_sync(&host->timer);
>> +
>> + sdhci_reset(host, SDHCI_RESET_ALL);
>> +
>> + tasklet_kill(&host->card_tasklet);
>> + tasklet_kill(&host->finish_tasklet);
>> +
>> + iounmap(host->ioaddr);
>> +
>> + pci_release_region(pdev, host->bar);
>> +
>> + free_irq(host->irq, host);
>
> The mmc_detect_change() callback could run right here. Is everything which
> it touches still allocated and in an appropriate state?
>
No, it's not. But the host should be left alone after mmc_remove_host().
Otherwise I don't see a way to deallocate structures in a safe order.
>
>> + mmc_free_host(mmc);
>> +}
>
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
