From mboxrd@z Thu Jan 1 00:00:00 1970 From: Arnd Bergmann Subject: Re: [PATCH] RFC: mmc: block: replace semaphore with freezing Date: Tue, 22 Nov 2016 10:16:09 +0100 Message-ID: <2495798.NCupJYMGV1@wuerfel> References: <1479293464-4576-1-git-send-email-linus.walleij@linaro.org> <17679227.zg8zl3jhbr@wuerfel> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7Bit Return-path: Received: from mout.kundenserver.de ([212.227.126.134]:58399 "EHLO mout.kundenserver.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932273AbcKVJQU (ORCPT ); Tue, 22 Nov 2016 04:16:20 -0500 In-Reply-To: Sender: linux-pm-owner@vger.kernel.org List-Id: linux-pm@vger.kernel.org To: Linus Walleij Cc: Binoy Jayan , "Rafael J. Wysocki" , "linux-mmc@vger.kernel.org" , Ulf Hansson , Chunyan Zhang , Baolin Wang , Linux PM , "Rafael J . Wysocki" , Russell King , Jiri Kosina On Tuesday, November 22, 2016 9:54:18 AM CET Linus Walleij wrote: > On Wed, Nov 16, 2016 at 5:32 PM, Arnd Bergmann wrote: > > On Wednesday, November 16, 2016 4:20:47 PM CET Linus Walleij wrote: > >> On Wed, Nov 16, 2016 at 1:46 PM, Rafael J. Wysocki wrote: > >> > >> > Well, we had a session at the KS regarding usage of the freezer on > >> > kernel threads and the conclusion was to get rid of that (as opposed > >> > to freezing user space, which is necessary IMO). So this change would > >> > go in the opposite direction. > >> > >> Aha so I should not make this thread look like everyone else, instead > >> everyone else should look like this thread, haha > >> > >> Ah well, I'll just drop it. > > > > It would still be good to remove the semaphore and do something else, > > as we also want to remove all semaphores. > > > > We could check "mq->flags & MMC_QUEUE_SUSPENDED" in the kthread to see > > if the queue is currently suspended, and otherwise go to sleep there, > > and then call wake_up() in the resume function. > > Hm... so simply: > > if (mq->flags & MMC_QUEUE_SUSPENDED) > schedule(); > > ? Something like that. > This whole kthread business is pretty messy. I would prefer if I could > just convert it to a workqueue. Just that it's not very simple the way > it speculates around in the request queue from the block layer. I don't see how that would work, but might be worth trying. After doing that, a simple flush_workqueue() might be enough to take care of the suspend operation. > > While looking at that code, I just noticed that access to > > mq->flags is racy and should be fixed as well. > > I guess we should take the queue lock &q->lock around accessing > the flags. Yes, either that, or use set_bit/test_bit/test_and_set_bit for atomic access. For instance, this one if (mq->flags & MMC_QUEUE_NEW_REQUEST) { mq->flags &= ~MMC_QUEUE_NEW_REQUEST; Could be if (test_and_clear(MMC_QUEUE_NEW_REQUEST_BIT, &mq->flags)) ... Arnd