From mboxrd@z Thu Jan 1 00:00:00 1970 From: Aaron Lu Subject: Re: [PATCH v7 3/4] block: implement runtime pm strategy Date: Thu, 17 Jan 2013 14:31:47 +0800 Message-ID: <20130117063147.GA25352@aaronlu.sh.intel.com> References: <1358326941-3725-4-git-send-email-aaron.lu@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Received: from mga11.intel.com ([192.55.52.93]:13528 "EHLO mga11.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754465Ab3AQGbF (ORCPT ); Thu, 17 Jan 2013 01:31:05 -0500 Content-Disposition: inline In-Reply-To: Sender: linux-pm-owner@vger.kernel.org List-Id: linux-pm@vger.kernel.org To: Alan Stern Cc: Jens Axboe , "Rafael J. Wysocki" , James Bottomley , linux-pm@vger.kernel.org, linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org, Aaron Lu , Shane Huang On Wed, Jan 16, 2013 at 10:30:45AM -0500, Alan Stern wrote: > On Wed, 16 Jan 2013, Aaron Lu wrote: > > > From: Lin Ming > > > > When a request is added: > > If device is suspended or is suspending and the request is not a > > PM request, resume the device. > > > > When the last request finishes: > > Call pm_runtime_mark_last_busy(). > > > > When pick a request: > > If device is resuming/suspending, then only PM request is allowed > > to go. > > > > Signed-off-by: Lin Ming > > Signed-off-by: Aaron Lu > > Just a couple of minor problems remaining... > > > --- a/block/blk-core.c > > +++ b/block/blk-core.c > > > @@ -2051,6 +2063,28 @@ static void blk_account_io_done(struct request *req) > > } > > } > > > > +#ifdef CONFIG_PM_RUNTIME > > +/* > > + * Don't process normal requests when queue is suspended > > + * or in the process of suspending/resuming > > + */ > > +static struct request *blk_pm_peek_request(struct request_queue *q, > > + struct request *rq) > > +{ > > + if (q->rpm_status == RPM_SUSPENDED || > > + (q->rpm_status != RPM_ACTIVE && !(rq->cmd_flags & REQ_PM))) > > + return NULL; > > + else > > + return rq; > > +} > > You don't check q->dev here, so the result is indefinite for devices > that don't use runtime PM. (Actually it will work out because > RPM_ACTIVE is defined as 0, but that's a pretty fragile approach.) > > Either do check q->dev here, or else explicitly initialize > q->rpm_status when the queue is created. > > > > --- a/block/elevator.c > > +++ b/block/elevator.c > > @@ -34,6 +34,7 @@ > > #include > > #include > > #include > > +#include > > > > #include > > > > @@ -515,6 +516,27 @@ void elv_bio_merged(struct request_queue *q, struct request *rq, > > e->type->ops.elevator_bio_merged_fn(q, rq, bio); > > } > > > > +#ifdef CONFIG_PM_RUNTIME > > +static void blk_pm_requeue_request(struct request *rq) > > +{ > > + if (!(rq->cmd_flags & REQ_PM)) > > + rq->q->nr_pending--; > > +} > > You don't check q->dev here. That's okay, but it means that > q->nr_pending will be meaningless or wrong if any I/O takes place > before blk_pm_runtime_init is called. > > Therefore the kerneldoc for blk_pm_runtime_init should mention that it > must not be called after any requests have been submitted. Also ------------ > mention that blk_pm_runtime_init enables runtime PM for q->dev, so the > caller shouldn't do it. I may misunderstandd this in last email. We didn't enable runtime PM for the device in blk_pm_runtime_init, just some auto suspend related settings. Thanks, Aaron