Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752383AbaJPM54 (ORCPT ); Thu, 16 Oct 2014 08:57:56 -0400 Received: from mail-yh0-f51.google.com ([209.85.213.51]:48354 "EHLO mail-yh0-f51.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752335AbaJPM5y (ORCPT ); Thu, 16 Oct 2014 08:57:54 -0400 MIME-Version: 1.0 In-Reply-To: <1413304389-6580-1-git-send-email-dianders@chromium.org> References: <1413304389-6580-1-git-send-email-dianders@chromium.org> From: Alim Akhtar Date: Thu, 16 Oct 2014 18:27:12 +0530 Message-ID: Subject: Re: [PATCH] mmc: dw_mmc: Remove old card detect infrastructure To: Doug Anderson Cc: Ulf Hansson , Seungwon Jeon , Jaehoon Chung , Addy Ke , Sonny Rao , Alim Akhtar , Andrew Bresticker , Chris Ball , "linux-mmc@vger.kernel.org" , "linux-kernel@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Doug, On Tue, Oct 14, 2014 at 10:03 PM, Doug Anderson wrote: > The dw_mmc driver had a bunch of code that ran whenever a card was > ejected and inserted. However, this code was old and crufty and > should be removed. Some evidence that it's really not needed: > > 1. Is is supposed to be legal to use 'cd-gpio' on dw_mmc instead of > using the built-in card detect mechanism. The 'cd-gpio' code > doesn't run any of the crufty old code but yet still works. > > 2. While looking at this, I realized that my old change (369ac86 mmc: > dw_mmc: don't queue up a card detect at slot startup) actually > castrated the old code a little bit already and nobody noticed. > Specifically "last_detect_state" was left as 0 at bootup. That > means that on the first card removal none of the crufty code ran. > Yes, right most of these codes are _almost_ never call. But I see dw_mci_reset() being called on card removal (after first insert/removal). I tested this on exynos5800 and this looks working fine. We need to test once cross suspend/resume as well. And as Jaehoon pointed out,probably lets look in TRM if there are some recommended steps for cd-detect. Otherwise this looks good to me. > 3. I can run "while true; do dd if=/dev/mmcblk1 of=/dev/null; done" > while ejecting and inserting an SD Card and the world doesn't > explode. > > If some of the crufty old code is actually needed, we should justify > it and also put it in some place where it will be run even with > "cd-gpio". > > Note that in my case I'm using the "cd-gpio" mechanism but for various > reasons the hardware triggers a dw_mmc "card detect" at bootup. That > was actually causing a real bug. The card detect workqueue was > running while the system was trying to enumerate the card. The > "present != slot->last_detect_state" triggered and we were doing all > kinds of crazy stuff and messing up enumeration. The new mechanism of > just asking the core to check the card is much safer and then the > bogus interrupt doesn't hurt. > > Signed-off-by: Doug Anderson > --- > drivers/mmc/host/dw_mmc.c | 121 ++++++++------------------------------------- > drivers/mmc/host/dw_mmc.h | 2 - > include/linux/mmc/dw_mmc.h | 2 - > 3 files changed, 20 insertions(+), 105 deletions(-) > > diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c > index 69f0cc6..6a86495 100644 > --- a/drivers/mmc/host/dw_mmc.c > +++ b/drivers/mmc/host/dw_mmc.c > @@ -34,7 +34,6 @@ > #include > #include > #include > -#include > #include > #include > #include > @@ -1954,6 +1953,23 @@ static void dw_mci_cmd_interrupt(struct dw_mci *host, u32 status) > tasklet_schedule(&host->tasklet); > } > > +static void dw_mci_handle_cd(struct dw_mci *host) > +{ > + int i; > + > + for (i = 0; i < host->num_slots; i++) { > + struct dw_mci_slot *slot = host->slot[i]; > + > + if (!slot) > + continue; > + > + if (slot->mmc->ops->card_event) > + slot->mmc->ops->card_event(slot->mmc); > + mmc_detect_change(slot->mmc, > + msecs_to_jiffies(host->pdata->detect_delay_ms)); > + } > +} > + > static irqreturn_t dw_mci_interrupt(int irq, void *dev_id) > { > struct dw_mci *host = dev_id; > @@ -2029,7 +2045,7 @@ static irqreturn_t dw_mci_interrupt(int irq, void *dev_id) > > if (pending & SDMMC_INT_CD) { > mci_writel(host, RINTSTS, SDMMC_INT_CD); > - queue_work(host->card_workqueue, &host->card_work); > + dw_mci_handle_cd(host); > } > > /* Handle SDIO Interrupts */ > @@ -2056,88 +2072,6 @@ static irqreturn_t dw_mci_interrupt(int irq, void *dev_id) > return IRQ_HANDLED; > } > > -static void dw_mci_work_routine_card(struct work_struct *work) > -{ > - struct dw_mci *host = container_of(work, struct dw_mci, card_work); > - int i; > - > - for (i = 0; i < host->num_slots; i++) { > - struct dw_mci_slot *slot = host->slot[i]; > - struct mmc_host *mmc = slot->mmc; > - struct mmc_request *mrq; > - int present; > - > - present = dw_mci_get_cd(mmc); > - while (present != slot->last_detect_state) { > - dev_dbg(&slot->mmc->class_dev, "card %s\n", > - present ? "inserted" : "removed"); > - > - spin_lock_bh(&host->lock); > - > - /* Card change detected */ > - slot->last_detect_state = present; > - > - /* Clean up queue if present */ > - mrq = slot->mrq; > - if (mrq) { > - if (mrq == host->mrq) { > - host->data = NULL; > - host->cmd = NULL; > - > - switch (host->state) { > - case STATE_IDLE: > - case STATE_WAITING_CMD11_DONE: > - break; > - case STATE_SENDING_CMD11: > - case STATE_SENDING_CMD: > - mrq->cmd->error = -ENOMEDIUM; > - if (!mrq->data) > - break; > - /* fall through */ > - case STATE_SENDING_DATA: > - mrq->data->error = -ENOMEDIUM; > - dw_mci_stop_dma(host); > - break; > - case STATE_DATA_BUSY: > - case STATE_DATA_ERROR: > - if (mrq->data->error == -EINPROGRESS) > - mrq->data->error = -ENOMEDIUM; > - /* fall through */ > - case STATE_SENDING_STOP: > - if (mrq->stop) > - mrq->stop->error = -ENOMEDIUM; > - break; > - } > - > - dw_mci_request_end(host, mrq); > - } else { > - list_del(&slot->queue_node); > - mrq->cmd->error = -ENOMEDIUM; > - if (mrq->data) > - mrq->data->error = -ENOMEDIUM; > - if (mrq->stop) > - mrq->stop->error = -ENOMEDIUM; > - > - spin_unlock(&host->lock); > - mmc_request_done(slot->mmc, mrq); > - spin_lock(&host->lock); > - } > - } > - > - /* Power down slot */ > - if (present == 0) > - dw_mci_reset(host); > - > - spin_unlock_bh(&host->lock); > - > - present = dw_mci_get_cd(mmc); > - } > - > - mmc_detect_change(slot->mmc, > - msecs_to_jiffies(host->pdata->detect_delay_ms)); > - } > -} > - > #ifdef CONFIG_OF > /* given a slot id, find out the device node representing that slot */ > static struct device_node *dw_mci_of_find_slot_node(struct device *dev, u8 slot) > @@ -2289,9 +2223,6 @@ static int dw_mci_init_slot(struct dw_mci *host, unsigned int id) > dw_mci_init_debugfs(slot); > #endif > > - /* Card initially undetected */ > - slot->last_detect_state = 0; > - > return 0; > > err_host_allocated: > @@ -2672,17 +2603,10 @@ int dw_mci_probe(struct dw_mci *host) > host->data_offset = DATA_240A_OFFSET; > > tasklet_init(&host->tasklet, dw_mci_tasklet_func, (unsigned long)host); > - host->card_workqueue = alloc_workqueue("dw-mci-card", > - WQ_MEM_RECLAIM, 1); > - if (!host->card_workqueue) { > - ret = -ENOMEM; > - goto err_dmaunmap; > - } > - INIT_WORK(&host->card_work, dw_mci_work_routine_card); > ret = devm_request_irq(host->dev, host->irq, dw_mci_interrupt, > host->irq_flags, "dw-mci", host); > if (ret) > - goto err_workqueue; > + goto err_dmaunmap; > > if (host->pdata->num_slots) > host->num_slots = host->pdata->num_slots; > @@ -2718,7 +2642,7 @@ int dw_mci_probe(struct dw_mci *host) > } else { > dev_dbg(host->dev, "attempted to initialize %d slots, " > "but failed on all\n", host->num_slots); > - goto err_workqueue; > + goto err_dmaunmap; > } > > if (host->quirks & DW_MCI_QUIRK_IDMAC_DTO) > @@ -2726,9 +2650,6 @@ int dw_mci_probe(struct dw_mci *host) > > return 0; > > -err_workqueue: > - destroy_workqueue(host->card_workqueue); > - > err_dmaunmap: > if (host->use_dma && host->dma_ops->exit) > host->dma_ops->exit(host); > @@ -2762,8 +2683,6 @@ void dw_mci_remove(struct dw_mci *host) > mci_writel(host, CLKENA, 0); > mci_writel(host, CLKSRC, 0); > > - destroy_workqueue(host->card_workqueue); > - > if (host->use_dma && host->dma_ops->exit) > host->dma_ops->exit(host); > > diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h > index 01b99e8..71d4995 100644 > --- a/drivers/mmc/host/dw_mmc.h > +++ b/drivers/mmc/host/dw_mmc.h > @@ -214,7 +214,6 @@ extern int dw_mci_resume(struct dw_mci *host); > * with CONFIG_MMC_CLKGATE. > * @flags: Random state bits associated with the slot. > * @id: Number of this slot. > - * @last_detect_state: Most recently observed card detect state. > */ > struct dw_mci_slot { > struct mmc_host *mmc; > @@ -234,7 +233,6 @@ struct dw_mci_slot { > #define DW_MMC_CARD_PRESENT 0 > #define DW_MMC_CARD_NEED_INIT 1 > int id; > - int last_detect_state; > }; > > struct dw_mci_tuning_data { > diff --git a/include/linux/mmc/dw_mmc.h b/include/linux/mmc/dw_mmc.h > index 0013669..69d0814 100644 > --- a/include/linux/mmc/dw_mmc.h > +++ b/include/linux/mmc/dw_mmc.h > @@ -135,7 +135,6 @@ struct dw_mci { > struct mmc_command stop_abort; > unsigned int prev_blksz; > unsigned char timing; > - struct workqueue_struct *card_workqueue; > > /* DMA interface members*/ > int use_dma; > @@ -154,7 +153,6 @@ struct dw_mci { > u32 stop_cmdr; > u32 dir_status; > struct tasklet_struct tasklet; > - struct work_struct card_work; > unsigned long pending_events; > unsigned long completed_events; > enum dw_mci_state state; > -- > 2.1.0.rc2.206.gedb03e5 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-mmc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Regards, Alim -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/