From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752522AbcFNNPE (ORCPT ); Tue, 14 Jun 2016 09:15:04 -0400 Received: from mail-wm0-f67.google.com ([74.125.82.67]:34287 "EHLO mail-wm0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751873AbcFNNPB (ORCPT ); Tue, 14 Jun 2016 09:15:01 -0400 MIME-Version: 1.0 In-Reply-To: <1465340522-1112-4-git-send-email-jarkko.sakkinen@linux.intel.com> References: <1465340522-1112-1-git-send-email-jarkko.sakkinen@linux.intel.com> <1465340522-1112-4-git-send-email-jarkko.sakkinen@linux.intel.com> From: Tomas Winkler Date: Tue, 14 Jun 2016 16:14:58 +0300 Message-ID: Subject: Re: [tpmdd-devel] [PATCH 3/3] tpm, tpm_crb: runtime power management To: Jarkko Sakkinen Cc: Peter Huewe , open list , linux-security-module@vger.kernel.org, "moderated list:TPM DEVICE DRIVER" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Jun 8, 2016 at 2:02 AM, Jarkko Sakkinen wrote: > The register TPM_CRB_CTRL_REQ_0 contains bits goIdle and cmdReady for > invoking the chip to suspend and resume. This commit implements runtime > PM for tpm_crb by using these bits. > > The legacy ACPI start (SMI + DMA) based devices do not support these > bits. Thus this functionality only is enabled only for CRB start (MMIO) > based devices. > > Signed-off-by: Jarkko Sakkinen > --- > drivers/char/tpm/tpm-interface.c | 3 ++ > drivers/char/tpm/tpm_crb.c | 62 ++++++++++++++++++++++++++++++++++++++-- > 2 files changed, 63 insertions(+), 2 deletions(-) > > diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c > index 5e3c1b6..3b85648 100644 > --- a/drivers/char/tpm/tpm-interface.c > +++ b/drivers/char/tpm/tpm-interface.c > @@ -29,6 +29,7 @@ > #include > #include > #include > +#include > > #include "tpm.h" > #include "tpm_eventlog.h" > @@ -350,6 +351,7 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf, > return -E2BIG; > } > > + pm_runtime_get_sync(chip->dev.parent); > mutex_lock(&chip->tpm_mutex); > > rc = chip->ops->send(chip, (u8 *) buf, count); > @@ -394,6 +396,7 @@ out_recv: > "tpm_transmit: tpm_recv: error %zd\n", rc); > out: > mutex_unlock(&chip->tpm_mutex); > + pm_runtime_put_sync(chip->dev.parent); > return rc; > } > > diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c > index ca2cad9..71cc7cd 100644 > --- a/drivers/char/tpm/tpm_crb.c > +++ b/drivers/char/tpm/tpm_crb.c > @@ -20,6 +20,7 @@ > #include > #include > #include > +#include > #include "tpm.h" > > #define ACPI_SIG_TPM2 "TPM2" > @@ -41,7 +42,6 @@ enum crb_ca_request { > > enum crb_ca_status { > CRB_CA_STS_ERROR = BIT(0), > - CRB_CA_STS_TPM_IDLE = BIT(1), > }; > > enum crb_start { > @@ -68,6 +68,8 @@ struct crb_control_area { > > enum crb_status { > CRB_STS_COMPLETE = BIT(0), > + CRB_STS_READY = BIT(1), > + CRB_STS_IDLE = BIT(2), > }; > > enum crb_flags { > @@ -81,9 +83,52 @@ struct crb_priv { > struct crb_control_area __iomem *cca; > u8 __iomem *cmd; > u8 __iomem *rsp; > + wait_queue_head_t idle_queue; I'm failing to find the code that is calling wake_up_interruptible(idle_queue); > }; > > -static SIMPLE_DEV_PM_OPS(crb_pm, tpm_pm_suspend, tpm_pm_resume); > +static int __maybe_unused crb_runtime_suspend(struct device *dev) > +{ > + struct tpm_chip *chip = dev_get_drvdata(dev); > + struct crb_priv *priv = dev_get_drvdata(&chip->dev); > + u32 req; > + > + if (priv->flags & CRB_FL_ACPI_START) > + return 0; > + > + req = ioread32(&priv->cca->req); > + > + iowrite32(cpu_to_le32(req | CRB_CA_REQ_GO_IDLE), &priv->cca->req); > + > + if (wait_for_tpm_stat(chip, CRB_STS_IDLE, chip->timeout_c, > + &priv->idle_queue, false)) > + dev_warn(&chip->dev, "idle timed out\n"); Unfortunately you cannot do that as there is an HW errata, the status register value might not be defined here during power transition You should poll for request CRB_CA_REQ_GO_IDLE goes to 0 as descried in the spec . only after that you can check for the status register (thought it's maybe not needed) > + > + return 0; > +} > + > +static int __maybe_unused crb_runtime_resume(struct device *dev) why this is marked unused, why just not compile it out? if the CONFIG_PM is not set? > +{ > + struct tpm_chip *chip = dev_get_drvdata(dev); > + struct crb_priv *priv = dev_get_drvdata(&chip->dev); > + u32 req; > + > + if (priv->flags & CRB_FL_ACPI_START) > + return 0; > + > + req = ioread32(&priv->cca->req); > + iowrite32(cpu_to_le32(req | CRB_CA_REQ_CMD_READY), &priv->cca->req); > + > + if (wait_for_tpm_stat(chip, CRB_STS_READY, chip->timeout_c, > + &priv->idle_queue, false)) > + dev_warn(&chip->dev, "wake timed out\n"); Same here, you should wait for CRB_CA_REQ_CMD_READ to get cleared, only after that it is safe to check the status register. > + > + return 0; > +} > + > +static const struct dev_pm_ops crb_pm = { > + SET_RUNTIME_PM_OPS(crb_runtime_suspend, crb_runtime_resume, NULL) > + SET_SYSTEM_SLEEP_PM_OPS(tpm_pm_suspend, tpm_pm_resume) > +}; > > static u8 crb_status(struct tpm_chip *chip) > { > @@ -94,6 +139,14 @@ static u8 crb_status(struct tpm_chip *chip) > CRB_START_INVOKE) > sts |= CRB_STS_COMPLETE; > > + if ((ioread32(&priv->cca->req) & CRB_CA_REQ_CMD_READY) != > + CRB_CA_REQ_CMD_READY) > + sts |= CRB_STS_READY; There is meaning for checking this w/o the actual transition i.e. setting the before > + if ((ioread32(&priv->cca->req) & CRB_CA_REQ_GO_IDLE) != > + CRB_CA_REQ_GO_IDLE) > + sts |= CRB_STS_IDLE; Same here. > + > return sts; > } > > @@ -206,6 +259,8 @@ static int crb_init(struct acpi_device *device, struct crb_priv *priv) > if (IS_ERR(chip)) > return PTR_ERR(chip); > > + pm_runtime_set_active(&device->dev); > + pm_runtime_enable(&device->dev); > dev_set_drvdata(&chip->dev, priv); > chip->acpi_dev_handle = device->handle; > chip->flags = TPM_CHIP_FLAG_TPM2; > @@ -348,6 +403,8 @@ static int crb_acpi_add(struct acpi_device *device) > !strcmp(acpi_device_hid(device), "MSFT0101")) > priv->flags |= CRB_FL_CRB_START; > > + init_waitqueue_head(&priv->idle_queue); > + > if (sm == ACPI_TPM2_START_METHOD || > sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD) > priv->flags |= CRB_FL_ACPI_START; > @@ -366,6 +423,7 @@ static int crb_acpi_remove(struct acpi_device *device) > > tpm_chip_unregister(chip); > > + pm_runtime_disable(dev); > return 0; > } > Thanks Tomas