linux-kernel - Re: [RFC PATCH] mmc: Enable wakeup

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 for Android: free password hash cracker in your pocket

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

Date:	Thu, 5 Sep 2013 14:54:14 -0700
From:	Zoran Markovic <zoran.markovic@...aro.org>
To:	Ulf Hansson <ulf.hansson@...aro.org>
Cc:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	linux-mmc <linux-mmc@...r.kernel.org>,
	San Mehat <san@...gle.com>, Colin Cross <ccross@...roid.com>,
	John Stultz <john.stultz@...aro.org>,
	Chris Ball <cjb@...top.org>,
	Johan Rudholm <johan.rudholm@...ricsson.com>,
	Jaehoon Chung <jh80.chung@...sung.com>,
	Konstantin Dorfman <kdorfman@...eaurora.org>,
	Guennadi Liakhovetski <g.liakhovetski@....de>,
	Tejun Heo <tj@...nel.org>,
	Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [RFC PATCH] mmc: Enable wakeup_sources for mmc core

Hi Ulf,
Thanks for reviewing this, it was very helpful!

> 1. mmc_detect_change does obviously not have to be run the same number
> of times as the mmc_rescan function. In other words, the calls to
> __pm_stay_awake is not paired with __pm_relay, I suppose this does not
> matter?
It shouldn't, since a single __pm_relax() would cancel all previous
calls to __pm_stay_awake() on the same wakeup source. What is
important is that mmc_rescan() is scheduled after __pm_stay_awake() to
make sure wakeup source is released.

> 2. mmc_detect_change can for example be called while the device
> suspend sequence is progressing. At this point the rescan work is
> disabled, thus __pm_relax will not be called, until the next rescan
> work as executed which is after the complete resume cycle
> (mmc_pm_notify:PM_POST_SUSPEND). Is that an issue?
If started, mmc_detect_change() should run uninterrupted to call
__pm_stay_awake(), which should abort any previous suspend requests.
The abort sequence should restart the rescan work, so __pm_relax()
eventually gets called.

>>         /* If there is a non-removable card registered, only scan once */
>> -       if ((host->caps & MMC_CAP_NONREMOVABLE) && host->rescan_entered)
>> +       if ((host->caps & MMC_CAP_NONREMOVABLE) && host->rescan_entered) {
>> +               __pm_relax(host->ws);
>
> By calling __pm_relax here, this indicates to me that is seems like
> you might have prevented, even for a very small timeslot, with a
> MMC_CAP_NONREMOVABLE card/host from the system to suspend.
>
> For sure, you must not prevent the suspend even for small timeslots,
> when MMC_CAP_NONREMOVABLE is set.
I agree. It appears that the corresponding __pm_stay_awake() is
indiscriminately called on system resume regardless of card type, so
this needs to be fixed.

>>         mmc_release_host(host);
>>
>>   out:
>> -       if (host->caps & MMC_CAP_NEEDS_POLL)
>> +       if (extend_wakeup)
>> +               /* extra 1/2 second should be enough, hopefully */
>> +               __pm_wakeup_event(host->ws, MSEC_PER_SEC/2);
>> +       else
>> +               __pm_relax(host->ws);
>> +
>> +       if (host->caps & MMC_CAP_NEEDS_POLL) {
>> +               __pm_stay_awake(host->ws);
>
> This does not make sense.
>
> So when using polling mode to detect card insert/remove, you will
> prevent suspend forever? Maybe I missed a point somewhere?
>
>>                 mmc_schedule_delayed_work(&host->detect, HZ);
>> +       }
>>  }
You are right, and I find it interesting that the same wake_lock()
call exists in the Android kernel. Would someone from the Android team
be able to comment?

>>         /* clear pm flags now and let card drivers set them as needed */
>> @@ -2628,7 +2645,8 @@ int mmc_suspend_host(struct mmc_host *host)
>>  {
>
> This function has become deprecated. You need to rebase this patch and
> please do not add some new code in here.
>
If suspend is now initiated from the bus level, will there be a
host-level suspend/resume function at all? I need to know where this
code should move in the next revision of patch...

Regards, Zoran
--
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/