| Message ID | 1465340522-1112-4-git-send-email-jarkko.sakkinen@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jun 8, 2016 at 2:02 AM, Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> 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 <jarkko.sakkinen@linux.intel.com> > --- > 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 <linux/mutex.h> > #include <linux/spinlock.h> > #include <linux/freezer.h> > +#include <linux/pm_runtime.h> > > #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 <linux/rculist.h> > #include <linux/module.h> > #include <linux/platform_device.h> > +#include <linux/pm_runtime.h> > #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 ------------------------------------------------------------------------------ What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic patterns at an interface-level. Reveals which users, apps, and protocols are consuming the most bandwidth. Provides multi-vendor support for NetFlow, J-Flow, sFlow and other flows. Make informed decisions using capacity planning reports. https://ad.doubleclick.net/ddm/clk/305295220;132659582;e
Hi Thomas, I'm on a vacation this week but I'll give you quick answers :) On Tue, Jun 14, 2016 at 04:14:58PM +0300, Tomas Winkler wrote: > On Wed, Jun 8, 2016 at 2:02 AM, Jarkko Sakkinen > <jarkko.sakkinen@linux.intel.com> 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 <jarkko.sakkinen@linux.intel.com> > > --- > > 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 <linux/mutex.h> > > #include <linux/spinlock.h> > > #include <linux/freezer.h> > > +#include <linux/pm_runtime.h> > > > > #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 <linux/rculist.h> > > #include <linux/module.h> > > #include <linux/platform_device.h> > > +#include <linux/pm_runtime.h> > > #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); Ugh, my bad. This actually should not be declared at all. Will remove it from the next version and NULL should be passed to wait_for_tpm_stat() as the driver does not yet support interrupts (Haswell did not have them, not sure about later gens). > > }; > > > > -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) And I do exactly what you are asking me to do. > > + > > + 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? It is compiled out if it is unused. Why would you want to trash the code with #ifdef cages if they are not necessary? I can add /* CONFIG_PM */ before the function if that makes it cleaner. > > +{ > > + 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. It does exactly that. I'm not using CRB status register for anything. > > + > > + 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 I'm not sure what you are trying to say. /Jarkko ------------------------------------------------------------------------------ What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic patterns at an interface-level. Reveals which users, apps, and protocols are consuming the most bandwidth. Provides multi-vendor support for NetFlow, J-Flow, sFlow and other flows. Make informed decisions using capacity planning reports. http://sdm.link/zohomanageengine
On Thu, Jun 16, 2016 at 09:57:35PM +0200, Jarkko Sakkinen wrote: > Hi Thomas, > > I'm on a vacation this week but I'll give you quick answers :) > > On Tue, Jun 14, 2016 at 04:14:58PM +0300, Tomas Winkler wrote: > > On Wed, Jun 8, 2016 at 2:02 AM, Jarkko Sakkinen > > <jarkko.sakkinen@linux.intel.com> 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 <jarkko.sakkinen@linux.intel.com> > > > --- > > > 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 <linux/mutex.h> > > > #include <linux/spinlock.h> > > > #include <linux/freezer.h> > > > +#include <linux/pm_runtime.h> > > > > > > #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 <linux/rculist.h> > > > #include <linux/module.h> > > > #include <linux/platform_device.h> > > > +#include <linux/pm_runtime.h> > > > #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); > > Ugh, my bad. This actually should not be declared at all. Will remove it > from the next version and NULL should be passed to wait_for_tpm_stat() > as the driver does not yet support interrupts (Haswell did not have > them, not sure about later gens). > > > > > }; > > > > > > -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) > > And I do exactly what you are asking me to do. > > > > + > > > + 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? > > It is compiled out if it is unused. Why would you want to trash the code > with #ifdef cages if they are not necessary? I can add /* CONFIG_PM */ > before the function if that makes it cleaner. > > > > +{ > > > + 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. > > It does exactly that. I'm not using CRB status register for anything. > > > > + > > > + 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 > > I'm not sure what you are trying to say. ... but I can undertand why this looks so confusing to you. Maybe it would be a better idea to completely discard the use of wait_for_tpm_stat() and changes to crb_status() and do instead the following in runtime_suspend/resume (example is for resume): 1. Set REQ_CMD_READY. 2. Sleep for TIMEOUT C. 3. Check the register and return -ETIME if it is not cleared. I think this patch is abusing wait_for_tpm_stat() and it is not really good fit here. How does this sound? /Jarkko ------------------------------------------------------------------------------ What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic patterns at an interface-level. Reveals which users, apps, and protocols are consuming the most bandwidth. Provides multi-vendor support for NetFlow, J-Flow, sFlow and other flows. Make informed decisions using capacity planning reports. http://sdm.link/zohomanageengine
On Thu, Jun 16, 2016 at 10:57 PM, Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote: > Hi Thomas, > > I'm on a vacation this week but I'll give you quick answers :) > > On Tue, Jun 14, 2016 at 04:14:58PM +0300, Tomas Winkler wrote: >> On Wed, Jun 8, 2016 at 2:02 AM, Jarkko Sakkinen >> <jarkko.sakkinen@linux.intel.com> 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 <jarkko.sakkinen@linux.intel.com> >> > --- >> > 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 <linux/mutex.h> >> > #include <linux/spinlock.h> >> > #include <linux/freezer.h> >> > +#include <linux/pm_runtime.h> >> > >> > #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 <linux/rculist.h> >> > #include <linux/module.h> >> > #include <linux/platform_device.h> >> > +#include <linux/pm_runtime.h> >> > #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); > > Ugh, my bad. This actually should not be declared at all. Will remove it > from the next version and NULL should be passed to wait_for_tpm_stat() > as the driver does not yet support interrupts (Haswell did not have > them, not sure about later gens). > > >> > }; >> > >> > -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) > > And I do exactly what you are asking me to do. > >> > + >> > + 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? > > It is compiled out if it is unused. Why would you want to trash the code > with #ifdef cages if they are not necessary? I can add /* CONFIG_PM */ > before the function if that makes it cleaner. I'm not sure about that, I believe it just suppresses warnings. You will need something --gc-sessions int the linker, I'm not sure this is used by kernel. > >> > +{ >> > + 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. > > It does exactly that. I'm not using CRB status register for anything. > >> > + >> > + 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 > > I'm not sure what you are trying to say. Sorry, my bad, it should be: There is NO meaning for checking CRB_CA_REQ_CMD_READY is 0 if this wasn't asserted before, In interrupt language it's not edge sensitive not level sensitive Tomas ------------------------------------------------------------------------------ What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic patterns at an interface-level. Reveals which users, apps, and protocols are consuming the most bandwidth. Provides multi-vendor support for NetFlow, J-Flow, sFlow and other flows. Make informed decisions using capacity planning reports. http://sdm.link/zohomanageengine
On Thu, Jun 16, 2016 at 11:26:48PM +0300, Tomas Winkler wrote: > > It is compiled out if it is unused. Why would you want to trash the code > > with #ifdef cages if they are not necessary? I can add /* CONFIG_PM */ > > before the function if that makes it cleaner. > > I'm not sure about that, I believe it just suppresses warnings. > You will need something --gc-sessions int the linker, I'm not sure > this is used by kernel. No, it is fine. The compiler drops unused static functions, that is what the attribute is for. It always does this, so supressing the warning is the desire behavior. The kernel preference is to avoid ifdefs and always compile all the code to avoid problems with config option combinations. --gc-sections isn't useful unless -ffunction-section is used, which the kernel doesn't do. Fortunately the dead code is removed by the compiler, not the linker. Jason ------------------------------------------------------------------------------ What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic patterns at an interface-level. Reveals which users, apps, and protocols are consuming the most bandwidth. Provides multi-vendor support for NetFlow, J-Flow, sFlow and other flows. Make informed decisions using capacity planning reports. http://sdm.link/zohomanageengine
On Thu, Jun 16, 2016 at 11:26:48PM +0300, Tomas Winkler wrote: > On Thu, Jun 16, 2016 at 10:57 PM, Jarkko Sakkinen > <jarkko.sakkinen@linux.intel.com> wrote: > > Hi Thomas, > > > > I'm on a vacation this week but I'll give you quick answers :) > > > > On Tue, Jun 14, 2016 at 04:14:58PM +0300, Tomas Winkler wrote: > >> On Wed, Jun 8, 2016 at 2:02 AM, Jarkko Sakkinen > >> <jarkko.sakkinen@linux.intel.com> 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 <jarkko.sakkinen@linux.intel.com> > >> > --- > >> > 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 <linux/mutex.h> > >> > #include <linux/spinlock.h> > >> > #include <linux/freezer.h> > >> > +#include <linux/pm_runtime.h> > >> > > >> > #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 <linux/rculist.h> > >> > #include <linux/module.h> > >> > #include <linux/platform_device.h> > >> > +#include <linux/pm_runtime.h> > >> > #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); > > > > Ugh, my bad. This actually should not be declared at all. Will remove it > > from the next version and NULL should be passed to wait_for_tpm_stat() > > as the driver does not yet support interrupts (Haswell did not have > > them, not sure about later gens). > > > > > >> > }; > >> > > >> > -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) > > > > And I do exactly what you are asking me to do. > > > >> > + > >> > + 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? > > > > It is compiled out if it is unused. Why would you want to trash the code > > with #ifdef cages if they are not necessary? I can add /* CONFIG_PM */ > > before the function if that makes it cleaner. > > I'm not sure about that, I believe it just suppresses warnings. > You will need something --gc-sessions int the linker, I'm not sure > this is used by kernel. It is used in lot of places. git grep gave me 1482 matches. > >> > +{ > >> > + 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. > > > > It does exactly that. I'm not using CRB status register for anything. > > > >> > + > >> > + 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 > > > > I'm not sure what you are trying to say. > > Sorry, my bad, it should be: There is NO meaning for checking > CRB_CA_REQ_CMD_READY is 0 if this wasn't asserted before, In > interrupt language it's not edge sensitive not level sensitive Got you but it should work because I take advatange of this in the case I first set it. Anyway, this approach that I chose is crap and I'll revise the whole patch in a way that I explained in my follow-up reply :) > Tomas /Jarkko ------------------------------------------------------------------------------ What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic patterns at an interface-level. Reveals which users, apps, and protocols are consuming the most bandwidth. Provides multi-vendor support for NetFlow, J-Flow, sFlow and other flows. Make informed decisions using capacity planning reports. http://sdm.link/zohomanageengine
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 <linux/mutex.h> #include <linux/spinlock.h> #include <linux/freezer.h> +#include <linux/pm_runtime.h> #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 <linux/rculist.h> #include <linux/module.h> #include <linux/platform_device.h> +#include <linux/pm_runtime.h> #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; }; -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"); + + return 0; +} + +static int __maybe_unused crb_runtime_resume(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_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"); + + 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; + + if ((ioread32(&priv->cca->req) & CRB_CA_REQ_GO_IDLE) != + CRB_CA_REQ_GO_IDLE) + sts |= CRB_STS_IDLE; + 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; }
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 <jarkko.sakkinen@linux.intel.com> --- drivers/char/tpm/tpm-interface.c | 3 ++ drivers/char/tpm/tpm_crb.c | 62 ++++++++++++++++++++++++++++++++++++++-- 2 files changed, 63 insertions(+), 2 deletions(-)