[PATCH RESEND] mmc: Delay the card_event callback into the mmc_rescan worker

[PATCH RESEND] mmc: Delay the card_event callback into the mmc_rescan worker

[Markus Mayer]

This change removes the callback from atomic context which it doesn't
need to be in, and puts it in line with the debounced rescan.

This code is based on these e-mail threads with Christian Daudt:

  https://lkml.org/lkml/2013/8/19/539
  https://lkml.org/lkml/2014/3/19/79

Signed-off-by: Markus Mayer <markus.mayer@linaro.org>
---
 drivers/mmc/core/core.c      |    5 +++++
 drivers/mmc/core/slot-gpio.c |    4 +---
 include/linux/mmc/host.h     |    2 ++
 3 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 098374b..ff7fd2e 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -2421,6 +2421,11 @@ void mmc_rescan(struct work_struct *work)
 		container_of(work, struct mmc_host, detect.work);
 	int i;
 
+	if (host->trigger_card_event && host->ops->card_event) {
+		host->ops->card_event(host);
+		host->trigger_card_event = false;
+	}
+
 	if (host->rescan_disable)
 		return;
 
diff --git a/drivers/mmc/core/slot-gpio.c b/drivers/mmc/core/slot-gpio.c
index 46596b71..4029c85 100644
--- a/drivers/mmc/core/slot-gpio.c
+++ b/drivers/mmc/core/slot-gpio.c
@@ -29,9 +29,7 @@ static irqreturn_t mmc_gpio_cd_irqt(int irq, void *dev_id)
 	/* Schedule a card detection after a debounce timeout */
 	struct mmc_host *host = dev_id;
 
-	if (host->ops->card_event)
-		host->ops->card_event(host);
-
+	host->trigger_card_event = true;
 	mmc_detect_change(host, msecs_to_jiffies(200));
 
 	return IRQ_HANDLED;
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 99f5709..63b983b 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -322,6 +322,8 @@ struct mmc_host {
 	int			rescan_disable;	/* disable card detection */
 	int			rescan_entered;	/* used with nonremovable devices */
 
+	bool			trigger_card_event; /* card_event necessary */
+
 	struct mmc_card		*card;		/* device attached to this host */
 
 	wait_queue_head_t	wq;
-- 
1.7.9.5

Re: [PATCH RESEND] mmc: Delay the card_event callback into the mmc_rescan worker

[Ulf Hansson]

On 9 April 2014 00:19, Markus Mayer <markus.mayer@linaro.org> wrote:
> This change removes the callback from atomic context which it doesn't
> need to be in, and puts it in line with the debounced rescan.
>
> This code is based on these e-mail threads with Christian Daudt:
>
>   https://lkml.org/lkml/2013/8/19/539
>   https://lkml.org/lkml/2014/3/19/79
>
> Signed-off-by: Markus Mayer <markus.mayer@linaro.org>
> ---
>  drivers/mmc/core/core.c      |    5 +++++
>  drivers/mmc/core/slot-gpio.c |    4 +---
>  include/linux/mmc/host.h     |    2 ++
>  3 files changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 098374b..ff7fd2e 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -2421,6 +2421,11 @@ void mmc_rescan(struct work_struct *work)
>                 container_of(work, struct mmc_host, detect.work);
>         int i;
>
> +       if (host->trigger_card_event && host->ops->card_event) {
> +               host->ops->card_event(host);
> +               host->trigger_card_event = false;
> +       }
> +

So this might be a stupid question. :-)

Could you elaborate on why the host->ops->card_event then is needed at
all. Why can't host drivers use host->ops->get_cd to perform the
actions they need?

The reason for bringing this up, is because this patch moves the
invokes of the these callbacks quite close to each other. Earlier they
were more separated.

Kind regards
Ulf Hansson

>         if (host->rescan_disable)
>                 return;
>
> diff --git a/drivers/mmc/core/slot-gpio.c b/drivers/mmc/core/slot-gpio.c
> index 46596b71..4029c85 100644
> --- a/drivers/mmc/core/slot-gpio.c
> +++ b/drivers/mmc/core/slot-gpio.c
> @@ -29,9 +29,7 @@ static irqreturn_t mmc_gpio_cd_irqt(int irq, void *dev_id)
>         /* Schedule a card detection after a debounce timeout */
>         struct mmc_host *host = dev_id;
>
> -       if (host->ops->card_event)
> -               host->ops->card_event(host);
> -
> +       host->trigger_card_event = true;
>         mmc_detect_change(host, msecs_to_jiffies(200));
>
>         return IRQ_HANDLED;
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index 99f5709..63b983b 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -322,6 +322,8 @@ struct mmc_host {
>         int                     rescan_disable; /* disable card detection */
>         int                     rescan_entered; /* used with nonremovable devices */
>
> +       bool                    trigger_card_event; /* card_event necessary */
> +
>         struct mmc_card         *card;          /* device attached to this host */
>
>         wait_queue_head_t       wq;
> --
> 1.7.9.5
>

Re: [PATCH RESEND] mmc: Delay the card_event callback into the mmc_rescan worker

[Markus Mayer]

On 9 April 2014 01:56, Ulf Hansson <ulf.hansson@linaro.org> wrote:

>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>> index 098374b..ff7fd2e 100644
>> --- a/drivers/mmc/core/core.c
>> +++ b/drivers/mmc/core/core.c
>> @@ -2421,6 +2421,11 @@ void mmc_rescan(struct work_struct *work)
>>                 container_of(work, struct mmc_host, detect.work);
>>         int i;
>>
>> +       if (host->trigger_card_event && host->ops->card_event) {
>> +               host->ops->card_event(host);
>> +               host->trigger_card_event = false;
>> +       }
>> +
>
> So this might be a stupid question. :-)
>
> Could you elaborate on why the host->ops->card_event then is needed at
> all. Why can't host drivers use host->ops->get_cd to perform the
> actions they need?
>
> The reason for bringing this up, is because this patch moves the
> invokes of the these callbacks quite close to each other. Earlier they
> were more separated.

Thanks for the question. I looked at the code and I don't think
host->ops->get_cd would work in my case.

I proposed this change based on behaviour exhibited by
sdhci-bcm-kona.c (mutex sleeps in the IRQ handler cause "scheduling
while atomic" errors). The idea of this patch is to move the card
event processing (which does the actual card detection) out of the
interrupt handler, so the mutex in my callback can be used without
problems.

https://git.linaro.org/landing-teams/working/broadcom/kernel.git/blob/refs/heads/review/mmc-card-event:/drivers/mmc/host/sdhci-bcm-kona.c#l160

An earlier proposal to fix this issue was to simply replace the mutex
with a spinlock in sdhci-bcm-kona.c, but that was not seen as good
solution. (https://lkml.org/lkml/2014/3/5/527)

Based on your question, I looked through the code some more. From what
I can tell, SDHCI hosts have their host->ops->get_cd set to
sdhci_get_cd() in sdhci.c (specifically by sdhci_add_host()). So,
get_cd is not available to me to customize.

Maybe we need a different solution from what I proposed above, but I
don't think get_cd will do the trick.

I can see how moving card_event and get_cd closer together, like I
have done, can be a reason for concern. On the flip-side, de-coupling
card_event from IRQ processing seems to be beneficial.

What do you think?

Regards,
-Markus

Re: [PATCH RESEND] mmc: Delay the card_event callback into the mmc_rescan worker

[Ulf Hansson]

On 9 April 2014 00:19, Markus Mayer <markus.mayer@linaro.org> wrote:
> This change removes the callback from atomic context which it doesn't
> need to be in, and puts it in line with the debounced rescan.
>
> This code is based on these e-mail threads with Christian Daudt:
>
>   https://lkml.org/lkml/2013/8/19/539
>   https://lkml.org/lkml/2014/3/19/79
>
> Signed-off-by: Markus Mayer <markus.mayer@linaro.org>

Acked-by: Ulf Hansson <ulf.hansson@linaro.org>

> ---
>  drivers/mmc/core/core.c      |    5 +++++
>  drivers/mmc/core/slot-gpio.c |    4 +---
>  include/linux/mmc/host.h     |    2 ++
>  3 files changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 098374b..ff7fd2e 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -2421,6 +2421,11 @@ void mmc_rescan(struct work_struct *work)
>                 container_of(work, struct mmc_host, detect.work);
>         int i;
>
> +       if (host->trigger_card_event && host->ops->card_event) {
> +               host->ops->card_event(host);
> +               host->trigger_card_event = false;
> +       }
> +
>         if (host->rescan_disable)
>                 return;
>
> diff --git a/drivers/mmc/core/slot-gpio.c b/drivers/mmc/core/slot-gpio.c
> index 46596b71..4029c85 100644
> --- a/drivers/mmc/core/slot-gpio.c
> +++ b/drivers/mmc/core/slot-gpio.c
> @@ -29,9 +29,7 @@ static irqreturn_t mmc_gpio_cd_irqt(int irq, void *dev_id)
>         /* Schedule a card detection after a debounce timeout */
>         struct mmc_host *host = dev_id;
>
> -       if (host->ops->card_event)
> -               host->ops->card_event(host);
> -
> +       host->trigger_card_event = true;
>         mmc_detect_change(host, msecs_to_jiffies(200));
>
>         return IRQ_HANDLED;
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index 99f5709..63b983b 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -322,6 +322,8 @@ struct mmc_host {
>         int                     rescan_disable; /* disable card detection */
>         int                     rescan_entered; /* used with nonremovable devices */
>
> +       bool                    trigger_card_event; /* card_event necessary */
> +
>         struct mmc_card         *card;          /* device attached to this host */
>
>         wait_queue_head_t       wq;
> --
> 1.7.9.5
>