Re: [PATCH 2/2] [MMC] Secure Digital Host Controller Interface driver

[Andrew Morton <akpm@osdl.org>]

Pierre Ossman <drzeus@drzeus.cx> 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.

> +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().

> +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?

> +	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?


> +	mmc_free_host(mmc);
> +}