linux-kernel - Re: [PATCH] mmc: dw_mmc: Disable low power mode if SDIO interrupts are used

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAD=FV=WxcXFgf+0a1UfAPRWuvWptLm9RjSqW3tEZjyT0Z+4gkA@mail.gmail.com>
Date:	Sun, 22 Jul 2012 19:48:34 -0700
From:	Doug Anderson <dianders@...omium.org>
To:	Will Newton <will.newton@...il.com>
Cc:	linux-mmc@...r.kernel.org, Chris Ball <cjb@...top.org>,
	James Hogan <james.hogan@...tec.com>,
	Seungwon Jeon <tgih.jun@...sung.com>,
	Jaehoon Chung <jh80.chung@...sung.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mmc: dw_mmc: Disable low power mode if SDIO interrupts
 are used

On Sat, Jul 21, 2012 at 3:40 AM, Will Newton <will.newton@...il.com> wrote:
>>  static void dw_mci_enable_sdio_irq(struct mmc_host *mmc, int enb)
>>  {
>>         struct dw_mci_slot *slot = mmc_priv(mmc);
>> @@ -871,6 +896,14 @@ static void dw_mci_enable_sdio_irq(struct mmc_host *mmc, int enb)
>>         /* Enable/disable Slot Specific SDIO interrupt */
>>         int_mask = mci_readl(host, INTMASK);
>>         if (enb) {
>> +               /*
>> +                * Turn off low power mode if it was enabled.  This is a bit of
>> +                * a heavy operation and we disable / enable IRQs a lot, so
>> +                * we'll leave them disabled; they will get re-enabled again in
>> +                * dw_mci_setup_bus().
>> +                */
>> +               dw_mci_disable_low_power(mmc);
>> +
>
> Is it safe to just disable low power here or could the setting be
> overwritten in setup_bus?

Very good question.  In my current setup I don't see setup_bus()
called during normal operation.  If it were, my kernel messages would
be constantly spammed with messages like:
    Bus speed (slot %d) = %dHz (slot req %dHz, actual %dHZ div = %d)

...and they're not.  Things may be different with different SDIO cards perhaps?

In any case, it's pretty easy for me to spin the patch so that we
don't clobber the low power bit in setup_bus() if SDIO interrupts are
enabled.  That makes a lot of sense, though I'd need to make sure that
low power mode does eventually get set again if someone ejects the
SDIO card and puts in a non-SDIO card.

I'll spin the patch tomorrow when I can test it properly and also
address some commenting concerns another engineer at chromium.org had.

It still feels to me like there ought to be a better place to put this
code.  I'd rather disable low power mode as soon as we detect an SDIO
card.  I spent time searching and the best I could find was
dw_mci_enable_sdio_irq(), but I'm all ears if someone has a better
idea!  :)  Certainly this code needs to go somewhere if we want SDIO
interrupts to be reliable.

Thanks for your feeback!  :)

-Doug
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/