diff mbox

[v2,16/22] mmc: tmio: fix never-detected card insertion bug

Message ID 1511540697-27387-17-git-send-email-yamada.masahiro@socionext.com (mailing list archive)
State New, archived
Headers show

Commit Message

Masahiro Yamada Nov. 24, 2017, 4:24 p.m. UTC
The TMIO mmc cannot detect the card insertion in native_hotplug mode
if the driver is probed without a card inserted.

The reason is obvious; all IRQs are disabled by tmio_mmc_host_probe(),
as follows:

  tmio_mmc_disable_mmc_irqs(_host, TMIO_MASK_ALL);

The IRQs are first enabled by tmio_mmc_start_command() as follows:

  if (!host->native_hotplug)
          irq_mask &= ~(TMIO_STAT_CARD_REMOVE | TMIO_STAT_CARD_INSERT);
  tmio_mmc_enable_mmc_irqs(host, irq_mask);

If the driver is probed without a card, tmio_mmc_start_command() is
never called in the first place.  So, the card is never detected.

The card event IRQs must be enabled in probe/resume functions.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

Changes in v2:
  - Newly added

 drivers/mmc/host/tmio_mmc_core.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

Comments

Wolfram Sang Jan. 2, 2018, 1:15 p.m. UTC | #1
Yamada-san,

> The TMIO mmc cannot detect the card insertion in native_hotplug mode
> if the driver is probed without a card inserted.

Hmm, it works for me without your patch just fine. Iam currently
researching it...

Happy new year, by the way! :)

Regards,

   Wolfram
Wolfram Sang Jan. 2, 2018, 5:13 p.m. UTC | #2
> > The TMIO mmc cannot detect the card insertion in native_hotplug mode
> > if the driver is probed without a card inserted.
> 
> Hmm, it works for me without your patch just fine. Iam currently
> researching it...

It really doesn't work for you?

mmc_add_host -> mmc_start_host -> _mmc_detect_change ->
mmc_schedule_delayed_work -> mmc_rescan -> mmc_start_request
Masahiro Yamada Jan. 5, 2018, 3:58 p.m. UTC | #3
Hi Wolfram,


2018-01-03 2:13 GMT+09:00 Wolfram Sang <wsa@the-dreams.de>:
>
>> > The TMIO mmc cannot detect the card insertion in native_hotplug mode
>> > if the driver is probed without a card inserted.
>>
>> Hmm, it works for me without your patch just fine. Iam currently
>> researching it...
>
> It really doesn't work for you?


Does not work.


> mmc_add_host -> mmc_start_host -> _mmc_detect_change ->
> mmc_schedule_delayed_work -> mmc_rescan -> mmc_start_request
>

Correct except "-> mmc_start_request".

If a card is not inserted, the power will go down. [1]
So, mmc_start_request will not happen.


BTW, did you understand my git-log?

I am talking about the card detection
by the IP-builtin circuit.

I am trying to support it by the following patch:
https://patchwork.kernel.org/patch/10074255/

TMIO MMC calls mmc_detect_change() [2],
but please note this _never_ detects the card event.
It just re-schedules mmc_rescan. [3]

To detect the card insertion/removal, .get_cd hook must be implemented,
but, it is fixed to mmc_gpio_get_cd. [4]

Unless you had already migrated your boards to built-in CD
based on my patch, GPIO is the only way to detect card events.

If so, you are testing unrelated things.

It is really hard to understand what is going on Renesas side,
but I imagined some possibilities:
 - Poll the GPIO for card events          -> out of interest of this patch
 - Use interrupt from GPIO irqchip        -> ditto
 - The media is non-removable like eMMC   -> ditto
 - GPIO is not set up                     -> mmc_gpio_get_cd() returns -ENOSYS


The last case is treated as "inserted", but the card removal will not
be detected.


[1] http://elixir.free-electrons.com/linux/v4.15-rc6/source/drivers/mmc/core/core.c#L2818
[2] http://elixir.free-electrons.com/linux/v4.15-rc6/source/drivers/mmc/host/tmio_mmc_core.c#L656
[3] http://elixir.free-electrons.com/linux/v4.15-rc6/source/drivers/mmc/core/core.c#L1995
[4] http://elixir.free-electrons.com/linux/v4.15-rc6/source/drivers/mmc/host/tmio_mmc_core.c#L1105
Wolfram Sang Jan. 13, 2018, 8:59 p.m. UTC | #4
> I am talking about the card detection
> by the IP-builtin circuit.

Yes, I know. As I wrote in one of the previous patches when reviewing
it, I disabled GPIO CD and used the internal mechanism (for tests where
it is relevant). Like here, too.

>  - GPIO is not set up                     -> mmc_gpio_get_cd() returns -ENOSYS

Thanks! That pointed me to the right direction. I missed that patch
10/22 was still under discussion and not applied to mmc/next, so I had
to pick it manually.

I can confirm now that there is an issue and your patch fixes it for the
non-GPIO case. For the GPIO case, however, the TMIO_STAT_CARD_REMOVE |
TMIO_STAT_CARD_INSERT interrupts are enabled now, too. It didn't harm
when doing my tests, but we shouldn't do it, to be safe IMO.
Masahiro Yamada Jan. 17, 2018, 4:32 p.m. UTC | #5
2018-01-14 5:59 GMT+09:00 Wolfram Sang <wsa@the-dreams.de>:
>
>> I am talking about the card detection
>> by the IP-builtin circuit.
>
> Yes, I know. As I wrote in one of the previous patches when reviewing
> it, I disabled GPIO CD and used the internal mechanism (for tests where
> it is relevant). Like here, too.
>
>>  - GPIO is not set up                     -> mmc_gpio_get_cd() returns -ENOSYS
>
> Thanks! That pointed me to the right direction. I missed that patch
> 10/22 was still under discussion and not applied to mmc/next, so I had
> to pick it manually.
>
> I can confirm now that there is an issue and your patch fixes it for the
> non-GPIO case. For the GPIO case, however, the TMIO_STAT_CARD_REMOVE |
> TMIO_STAT_CARD_INSERT interrupts are enabled now, too. It didn't harm
> when doing my tests, but we shouldn't do it, to be safe IMO.
>


Could you explain why?


    _host->native_hotplug = !(mmc_can_gpio_cd(mmc) ||
                              mmc->caps & MMC_CAP_NEEDS_POLL ||
                             !mmc_card_is_removable(mmc));

For the GPIO case, mmc_can_gpio_cd(mmc) is 1,
so _host_>native_hogplug is 0.


Then, my code does nothing, doesn't it?

+       if (_host->native_hotplug)
+               tmio_mmc_enable_mmc_irqs(_host,
+                               TMIO_STAT_CARD_REMOVE | TMIO_STAT_CARD_INSERT);
diff mbox

Patch

diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c
index b51bb06..7d169ed 100644
--- a/drivers/mmc/host/tmio_mmc_core.c
+++ b/drivers/mmc/host/tmio_mmc_core.c
@@ -350,8 +350,6 @@  static int tmio_mmc_start_command(struct tmio_mmc_host *host,
 			c |= TRANSFER_READ;
 	}
 
-	if (!host->native_hotplug)
-		irq_mask &= ~(TMIO_STAT_CARD_REMOVE | TMIO_STAT_CARD_INSERT);
 	tmio_mmc_enable_mmc_irqs(host, irq_mask);
 
 	/* Fire off the command */
@@ -1292,11 +1290,13 @@  int tmio_mmc_host_probe(struct tmio_mmc_host *_host,
 		irq_mask |= TMIO_MASK_READOP;
 	if (!_host->chan_tx)
 		irq_mask |= TMIO_MASK_WRITEOP;
-	if (!_host->native_hotplug)
-		irq_mask &= ~(TMIO_STAT_CARD_REMOVE | TMIO_STAT_CARD_INSERT);
 
 	_host->sdcard_irq_mask &= ~irq_mask;
 
+	if (_host->native_hotplug)
+		tmio_mmc_enable_mmc_irqs(_host,
+				TMIO_STAT_CARD_REMOVE | TMIO_STAT_CARD_INSERT);
+
 	spin_lock_init(&_host->lock);
 	mutex_init(&_host->ios_lock);
 
@@ -1383,6 +1383,10 @@  int tmio_mmc_host_runtime_resume(struct device *dev)
 	if (host->clk_cache)
 		tmio_mmc_set_clock(host, host->clk_cache);
 
+	if (host->native_hotplug)
+		tmio_mmc_enable_mmc_irqs(host,
+				TMIO_STAT_CARD_REMOVE | TMIO_STAT_CARD_INSERT);
+
 	tmio_mmc_enable_dma(host, true);
 
 	if (tmio_mmc_can_retune(host) && host->select_tuning(host))