by Alim Akhtar

[permalink] [raw]

Subject: Re: [PATCH] mmc: dw_mmc: Remove old card detect infrastructure

Hi Doug,

On Wed, Oct 22, 2014 at 10:06 PM, Doug Anderson <[email protected]> wrote:
> Hi,
>
> On Sun, Oct 19, 2014 at 8:23 PM, Jaehoon Chung <[email protected]> wrote:
>> Hi.
>>
>> On 10/17/2014 09:44 PM, Alim Akhtar wrote:
>>> Hi Doug,
>>>
>>> On Thu, Oct 16, 2014 at 9:40 PM, Doug Anderson <[email protected]> wrote:
>>>> Alim,
>>>>
>>>> On Thu, Oct 16, 2014 at 5:57 AM, Alim Akhtar <[email protected]> wrote:
>>>>> Hi Doug,
>>>>>
>>>>> On Tue, Oct 14, 2014 at 10:03 PM, Doug Anderson <[email protected]> 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).
>>>>
>>>> Right. The old crufty code was called on the 2nd removal, not the
>>>> 1st. That meant that the two were accidentally different. My point
>>>> was that if the old code was really required that someone would have
>>>> noticed crashes on the 1st removal after each boot. Since nobody is
>>>> reporting crashes with that then it means it can't be too terrible.
>>>>
>>>> One thing to note: I remember in the last Chromebook project you were
>>>> trying to track down crashes associated with constant eject / insert
>>>> of SD Cards. I wonder if my patch will fix these crashes?
>>>>
>>> Ah, yes, reproducing that and checking with this patch will be really
>>> interesting.
>>>
>>>>
>>>>> I tested this on exynos5800 and this looks working fine. We need to
>>>>> test once cross suspend/resume as well.
>>>>
>>>> Good idea. Can you test that? I know that there's been lots of flux
>>>> with suspend/resume on exynos and I'm not sure I have all the latest
>>>> patches, but I'll search for them if you are unable to test easily.
>>>>
>>> Sure, I will do that..but probably sometime next week, as I will out
>>> of office for few days.
>>>>
>>>>> 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.
>>>>
>>>> If you see some other requirement than the one I addressed in my email
>>>> to Jaehoon, please let me know.
>>
>> I know there is no other requirement for detecting card.
>> So this patch can be applied after testing the above case(suspend/resume).
>
> I put a kernel based upon 3.17 on an exynos5250-snow (specifically
> git://git.collabora.co.uk/git/user/javier/linux.git branch
> max77802-op-modes-v3, git hash 98cf5a0). Snow uses the builtin card
> detect on dw_mmc. Resume wasn't terribly reliable to start with even
> without my patch (it often woke up right after suspend), but it worked
> well enough for testing. I tested the following scenarios:
>
> 1. Leave card in and mounted. Suspend/resume. Card is still usable
> after resume
>
> 2. Suspend and insert card. Resume. Card is detected upon resume.
>
> 3. Suspend and remove card. Resume. Card is removed upon resume.
>
> How does that sound?
>
Sounds good to me.
I tested this on Peach-Pi with v3.17 based kernel across suspend
resume and cards works fine.
Even I back-ported this to chromium 3.8 and tested on peach-pi, no
issue over a hundred s2r cycles.

Tested-by: alim.akhtar <[email protected]>
> -Doug

--
Regards,
Alim

2014-10-27 15:42:29

by Ulf Hansson

[permalink] [raw]

Subject: Re: [PATCH] mmc: dw_mmc: Remove old card detect infrastructure

On 14 October 2014 18:33, Doug Anderson <[email protected]> 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.
>
> 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 <[email protected]>

Thanks! Applied for next.

Kind regards
Uffe

> ---
> 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 <linux/mmc/dw_mmc.h>
> #include <linux/bitops.h>
> #include <linux/regulator/consumer.h>
> -#include <linux/workqueue.h>
> #include <linux/of.h>
> #include <linux/of_gpio.h>
> #include <linux/mmc/slot-gpio.h>
> @@ -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
>