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

[Sergey Vlasov <vsu@altlinux.ru>]

On Sat, 11 Feb 2006 01:15:26 +0100 Pierre Ossman wrote:

> +static int __devinit sdhci_probe_slot(struct pci_dev *pdev, int slot)
> +{
> +	int ret;
> +	struct sdhci_chip *chip;
> +	struct mmc_host *mmc;
> +	struct sdhci_host *host;
> +
> +	u8 first_bar;
> +	unsigned int caps;
> +
> +	chip = pci_get_drvdata(pdev);
> +	BUG_ON(!chip);
> +
> +	ret = pci_read_config_byte(pdev, PCI_SLOT_INFO, &first_bar);
> +	if (ret)
> +		return ret;
> +
> +	first_bar &= PCI_SLOT_INFO_FIRST_BAR_MASK;
> +
> +	mmc = mmc_alloc_host(sizeof(struct sdhci_host), &pdev->dev);
> +	if (!mmc)
> +		return -ENOMEM;
> +
> +	host = mmc_priv(mmc);
> +	host->mmc = mmc;
> +
> +	host->bar = first_bar + slot;
> +
> +	host->addr = pci_resource_start(pdev, host->bar);
> +	host->irq = pdev->irq;
> +
> +	DBG("slot %d at 0x%08lx, irq %d\n", slot, host->addr, host->irq);
> +
> +	BUG_ON(!(pci_resource_flags(pdev, first_bar + slot) & IORESOURCE_MEM));

Oopsing the kernel when a broken device is found does not look nice.
Especially when we are not really sure that the device is broken
(because we don't have the official SDHCI spec).

Also, it would be good to sanity check all parameters you fetch from the
device - e.g., host->bar must be <= 5, pci_resource_len() of it must be
at least 0x100 - just to be safe in case we don't know about some thing
which exists in the official spec, but is not used by devices
encountered while writing and testing the driver.

> +
> +	snprintf(host->slot_descr, 20, "sdhci:slot%d", slot);
> +
> +	ret = pci_request_region(pdev, host->bar, host->slot_descr);
> +	if (ret)
> +		goto free;
> +
> +	host->ioaddr = ioremap_nocache(host->addr,
> +		pci_resource_len(pdev, host->bar));
> +	if (!host->ioaddr) {
> +		ret = -ENOMEM;
> +		goto release;
> +	}
> +
> +	ret = request_irq(host->irq, sdhci_irq, SA_SHIRQ,
> +		host->slot_descr, host);

The interrupt handler can be called immediately after request_irq()
completes (even if you are sure that the device itself cannot generate
interrupts at this point, the interrupt line can be shared).  And
host->lock is not yet initialized - oops...

> +	if (ret)
> +		goto unmap;
> +
> +	caps = readl(host->ioaddr + SDHCI_CAPABILITIES);
> +
> +	if ((caps & SDHCI_CAN_DO_DMA) && ((pdev->class & 0x0000FF) == 0x01))
> +		host->flags |= SDHCI_USE_DMA;
> +
> +	if (host->flags & SDHCI_USE_DMA) {
> +		if (pci_set_dma_mask(pdev, DMA_32BIT_MASK)) {
> +			printk(KERN_WARNING "%s: No suitable DMA available. "
> +				"Falling back to PIO.\n", host->slot_descr);
> +			host->flags &= ~SDHCI_USE_DMA;
> +		}
> +	}
> +
> +	if (host->flags & SDHCI_USE_DMA)
> +		pci_set_master(pdev);
> +	else /* XXX: Hack to get MMC layer to avoid highmem */
> +		pdev->dma_mask = 0;
> +
> +	host->max_clk = (caps & SDHCI_CLOCK_BASE_MASK) >> SDHCI_CLOCK_BASE_SHIFT;
> +	host->max_clk *= 1000000;
> +
> +	/*
> +	 * Set host parameters.
> +	 */
> +	mmc->ops = &sdhci_ops;
> +	mmc->f_min = host->max_clk / 256;
> +	mmc->f_max = host->max_clk;
> +	mmc->ocr_avail = MMC_VDD_32_33|MMC_VDD_33_34;
> +	mmc->caps = MMC_CAP_4_BIT_DATA;
> +
> +	spin_lock_init(&host->lock);
> +
> +	/*
> +	 * Maximum number of segments. Hardware cannot do scatter lists.
> +	 */
> +	if (host->flags & SDHCI_USE_DMA)
> +		mmc->max_hw_segs = 1;
> +	else
> +		mmc->max_hw_segs = 16;
> +	mmc->max_phys_segs = 16;
> +
> +	/*
> +	 * Maximum number of sectors in one transfer. Limited by sector
> +	 * count register.
> +	 */
> +	mmc->max_sectors = 0x3FFF;
> +
> +	/*
> +	 * Maximum segment size. Could be one segment with the maximum number
> +	 * of sectors.
> +	 */
> +	mmc->max_seg_size = mmc->max_sectors * 512;
> +
> +	/*
> +	 * Init tasklets.
> +	 */
> +	tasklet_init(&host->card_tasklet,
> +		sdhci_tasklet_card, (unsigned long)host);
> +	tasklet_init(&host->finish_tasklet,
> +		sdhci_tasklet_finish, (unsigned long)host);
> +
> +	init_timer(&host->timer);
> +	host->timer.data = (unsigned long)host;
> +	host->timer.function = sdhci_timeout_timer;
> +
> +	sdhci_init(host);
> +
> +#ifdef CONFIG_MMC_DEBUG
> +	sdhci_dumpregs(host);
> +#endif
> +
> +	host->chip = chip;
> +	chip->hosts[slot] = host;
> +
> +	mmc_add_host(mmc);
> +
> +	printk(KERN_INFO "%s: SDHCI at 0x%08lx irq %d %s\n", mmc_hostname(mmc),
> +		host->addr, host->irq,
> +		(host->flags & SDHCI_USE_DMA)?"DMA":"PIO");
> +
> +	return 0;
> +
> +unmap:
> +	iounmap(host->ioaddr);
> +release:
> +	pci_release_region(pdev, host->bar);
> +free:
> +	mmc_free_host(mmc);
> +
> +	return ret;
> +}
> +
> +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 same problem as with request_irq(), just from the other side - until
free_irq() returns, you may still get calls to your interrupt handler,
and host->ioaddr is already unmapped - oops again.

> +
> +	mmc_free_host(mmc);
> +}
> +
> +static int __devinit sdhci_probe(struct pci_dev *pdev,
> +	const struct pci_device_id *ent)
> +{
> +	int ret, i;
> +	u8 slots;
> +	struct sdhci_chip *chip;
> +
> +	BUG_ON(pdev == NULL);
> +	BUG_ON(ent == NULL);

IMHO these BUG_ON() calls are overkill.

[...]
> +typedef struct sdhci_host *sdhci_host_p;

The general policy seems to be "typedefs are evil"...

Attachment: pgp0AvMBTvhly.pgp
Description: PGP signature