From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 3797DC433EF for ; Thu, 10 Mar 2022 19:05:26 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 9B28010E199; Thu, 10 Mar 2022 19:05:25 +0000 (UTC) Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by gabe.freedesktop.org (Postfix) with ESMTPS id 7777010E199 for ; Thu, 10 Mar 2022 19:05:23 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1646939123; x=1678475123; h=date:from:to:cc:subject:message-id:references: mime-version:in-reply-to; bh=WnFGNKbJzycKyV82fMk3LWMbBnyeS+s9105OQY/fMpc=; b=JO1Mvr+6LrGPZMIk1+fL3pdZ7BrMfKurzG3i8bmMpzTWnBzlj7XGO/04 zG62XT129C1g4JcCGHyXJQv9Ty0KtCzczFxnrXAJrIIII8qrqs88RHt3j UYl9x/sVPbnKN+ZLrZnVGrpkomsub38pfc4MF6dsohwvB/sEPgnDaQlYp vTym3UvYTdtFSBaQJg+cQ7k05g/0vRpPBDAJ22RhUYnjfOouoEdcNHBif z4EcSLvfpocgScmDmxCBExQfP0lZQPz08MPQ1uJcLM9o1j1KhBRHaAu6N elge/s1TaQxmEfuu+KRhiADwShe4stu1Bllt+BiyBx/q46GWgv3pa8MKU g==; X-IronPort-AV: E=McAfee;i="6200,9189,10282"; a="235294008" X-IronPort-AV: E=Sophos;i="5.90,171,1643702400"; d="scan'208";a="235294008" Received: from orsmga008.jf.intel.com ([10.7.209.65]) by fmsmga106.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 10 Mar 2022 11:03:49 -0800 X-IronPort-AV: E=Sophos;i="5.90,171,1643702400"; d="scan'208";a="554794723" Received: from jdubrow-mobl.amr.corp.intel.com (HELO intel.com) ([10.255.39.9]) by orsmga008-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 10 Mar 2022 11:03:46 -0800 Date: Thu, 10 Mar 2022 14:03:45 -0500 From: Rodrigo Vivi To: Alexander Usyskin Message-ID: References: <20220308163654.942820-1-alexander.usyskin@intel.com> <20220308163654.942820-5-alexander.usyskin@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20220308163654.942820-5-alexander.usyskin@intel.com> Subject: Re: [Intel-gfx] [PATCH v10 4/5] mei: gsc: add runtime pm handlers X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: David Airlie , Greg Kroah-Hartman , intel-gfx@lists.freedesktop.org, linux-kernel@vger.kernel.org, Tomas Winkler , Vitaly Lubart Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" On Tue, Mar 08, 2022 at 06:36:53PM +0200, Alexander Usyskin wrote: > From: Tomas Winkler > > Implement runtime handlers for mei-gsc, to track > idle state of the device properly. > > CC: Rodrigo Vivi > Signed-off-by: Tomas Winkler > Signed-off-by: Alexander Usyskin > --- > drivers/misc/mei/gsc-me.c | 67 ++++++++++++++++++++++++++++++++++++++- > 1 file changed, 66 insertions(+), 1 deletion(-) > > diff --git a/drivers/misc/mei/gsc-me.c b/drivers/misc/mei/gsc-me.c > index cf427f6fdec9..dac482ddab51 100644 > --- a/drivers/misc/mei/gsc-me.c > +++ b/drivers/misc/mei/gsc-me.c > @@ -152,7 +152,72 @@ static int __maybe_unused mei_gsc_pm_resume(struct device *device) > return 0; > } > > -static SIMPLE_DEV_PM_OPS(mei_gsc_pm_ops, mei_gsc_pm_suspend, mei_gsc_pm_resume); > +static int __maybe_unused mei_gsc_pm_runtime_idle(struct device *device) > +{ > + struct mei_device *dev = dev_get_drvdata(device); > + > + if (!dev) > + return -ENODEV; > + if (mei_write_is_idle(dev)) > + pm_runtime_autosuspend(device); This is not needed. The _idle() callback is called right before the autosuspend. so you just need to return -EBUSY if not idle. But also I'm missing the call to enable the autosuspend and set the delay. Is this flow really working and you are getting device suspended when not in use? (Maybe it is just my ignorance on other flow types here) > + > + return -EBUSY; > +} > + > +static int __maybe_unused mei_gsc_pm_runtime_suspend(struct device *device) > +{ > + struct mei_device *dev = dev_get_drvdata(device); > + struct mei_me_hw *hw; > + int ret; > + > + if (!dev) > + return -ENODEV; > + > + mutex_lock(&dev->device_lock); > + > + if (mei_write_is_idle(dev)) { > + hw = to_me_hw(dev); > + hw->pg_state = MEI_PG_ON; > + ret = 0; > + } else { > + ret = -EAGAIN; > + } probably not needed this here... but it would be good if you use the runtime_pm{get,put} to protect your write operations as well... > + > + mutex_unlock(&dev->device_lock); > + > + return ret; > +} > + > +static int __maybe_unused mei_gsc_pm_runtime_resume(struct device *device) > +{ > + struct mei_device *dev = dev_get_drvdata(device); > + struct mei_me_hw *hw; > + irqreturn_t irq_ret; > + > + if (!dev) > + return -ENODEV; > + > + mutex_lock(&dev->device_lock); > + > + hw = to_me_hw(dev); > + hw->pg_state = MEI_PG_OFF; > + > + mutex_unlock(&dev->device_lock); > + > + irq_ret = mei_me_irq_thread_handler(1, dev); > + if (irq_ret != IRQ_HANDLED) > + dev_err(dev->dev, "thread handler fail %d\n", irq_ret); > + > + return 0; > +} > + > +static const struct dev_pm_ops mei_gsc_pm_ops = { > + SET_SYSTEM_SLEEP_PM_OPS(mei_gsc_pm_suspend, > + mei_gsc_pm_resume) > + SET_RUNTIME_PM_OPS(mei_gsc_pm_runtime_suspend, > + mei_gsc_pm_runtime_resume, > + mei_gsc_pm_runtime_idle) > +}; > > static const struct auxiliary_device_id mei_gsc_id_table[] = { > { > -- > 2.32.0 >