From: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> To: Tomas Winkler <tomas.winkler@intel.com> Cc: tpmdd-devel@lists.sourceforge.net, Jason Gunthorpe <jgunthorpe@obsidianresearch.com>, linux-kernel@vger.kernel.org Subject: Re: [PATCH v3 2/4] tmp/tpm_crb: fix Intel PTT hw bug during idle state Date: Tue, 27 Sep 2016 12:31:00 +0300 [thread overview] Message-ID: <20160927093100.GA19121@intel.com> (raw) In-Reply-To: <20160915062329.GB12289@intel.com> On Thu, Sep 15, 2016 at 09:23:29AM +0300, Jarkko Sakkinen wrote: > On Mon, Sep 12, 2016 at 04:04:19PM +0300, Tomas Winkler wrote: > > There is a HW bug in Skylake, and Broxton PCH Intel PTT device, where > > most of the registers in the control area except START, REQUEST, CANCEL, > > and LOC_CTRL lost retention when the device is in the idle state. Hence > > we need to bring the device to ready state before accessing the other > > registers. The fix brings device to ready state before trying to read > > command and response buffer addresses in order to remap the for access. > > > > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com> > > Tested-by: Jarkko Sakkinen <jarkko.sakkinn@linux.intel.com> > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinn@linux.intel.com> I noticed something odd or at least not described in the commit message. > /Jarkko > > > --- > > V2: cmd read need to be called also before crb_init as this will run > > self test. > > V3: resend. > > > > drivers/char/tpm/tpm_crb.c | 47 ++++++++++++++++++++++++++++++++++++++-------- > > 1 file changed, 39 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c > > index b6923a8b3ff7..e945177cf2c8 100644 > > --- a/drivers/char/tpm/tpm_crb.c > > +++ b/drivers/char/tpm/tpm_crb.c > > @@ -318,6 +318,7 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv, > > struct list_head resources; > > struct resource io_res; > > struct device *dev = &device->dev; > > + u32 pa_high, pa_low; > > u64 cmd_pa; > > u32 cmd_size; > > u64 rsp_pa; > > @@ -345,12 +346,27 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv, > > if (IS_ERR(priv->cca)) > > return PTR_ERR(priv->cca); > > > > - cmd_pa = ((u64) ioread32(&priv->cca->cmd_pa_high) << 32) | > > - (u64) ioread32(&priv->cca->cmd_pa_low); > > + /* > > + * PTT HW bug w/a: wake up the device to access > > + * possibly not retained registers. > > + */ > > + ret = crb_cmd_ready(dev, priv); > > + if (ret) > > + return ret; > > + > > + pa_high = ioread32(&priv->cca->cmd_pa_high); > > + pa_low = ioread32(&priv->cca->cmd_pa_low); > > + cmd_pa = ((u64)pa_high << 32) | pa_low; > > cmd_size = ioread32(&priv->cca->cmd_size); > > + > > + dev_dbg(dev, "cmd_hi = %X cmd_low = %X cmd_size %X\n", > > + pa_high, pa_low, cmd_size); > > + > > priv->cmd = crb_map_res(dev, priv, &io_res, cmd_pa, cmd_size); > > - if (IS_ERR(priv->cmd)) > > - return PTR_ERR(priv->cmd); > > + if (IS_ERR(priv->cmd)) { > > + ret = PTR_ERR(priv->cmd); > > + goto out; > > + } > > > > memcpy_fromio(&rsp_pa, &priv->cca->rsp_pa, 8); > > rsp_pa = le64_to_cpu(rsp_pa); > > @@ -358,7 +374,8 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv, > > > > if (cmd_pa != rsp_pa) { > > priv->rsp = crb_map_res(dev, priv, &io_res, rsp_pa, rsp_size); > > - return PTR_ERR_OR_ZERO(priv->rsp); > > + ret = PTR_ERR_OR_ZERO(priv->rsp); > > + goto out; > > } > > > > /* According to the PTP specification, overlapping command and response > > @@ -366,12 +383,18 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv, > > */ > > if (cmd_size != rsp_size) { > > dev_err(dev, FW_BUG "overlapping command and response buffer sizes are not identical"); > > - return -EINVAL; > > + ret = -EINVAL; > > + goto out; > > } > > + > > priv->cmd_size = cmd_size; > > > > priv->rsp = priv->cmd; > > - return 0; > > + > > +out: > > + crb_go_idle(dev, priv); > > + > > + return ret; > > } > > > > static int crb_acpi_add(struct acpi_device *device) > > @@ -415,7 +438,15 @@ static int crb_acpi_add(struct acpi_device *device) > > if (rc) > > return rc; > > > > - return crb_init(device, priv); > > + rc = crb_cmd_ready(dev, priv); > > + if (rc) > > + return rc; You do this already in crb_map_io() that is called before crb_init(). What is the purpose of this extra crb_cmd_ready()? Looks unrelated at least to the described workaround. > > + > > + rc = crb_init(device, priv); > > + if (rc) > > + crb_go_idle(dev, priv); > > + > > + return rc; > > } > > > > static int crb_acpi_remove(struct acpi_device *device) > > -- > > 2.7.4 /Jarkko
WARNING: multiple messages have this Message-ID (diff)
From: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> To: Tomas Winkler <tomas.winkler-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> Cc: tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org Subject: Re: [PATCH v3 2/4] tmp/tpm_crb: fix Intel PTT hw bug during idle state Date: Tue, 27 Sep 2016 12:31:00 +0300 [thread overview] Message-ID: <20160927093100.GA19121@intel.com> (raw) In-Reply-To: <20160915062329.GB12289-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> On Thu, Sep 15, 2016 at 09:23:29AM +0300, Jarkko Sakkinen wrote: > On Mon, Sep 12, 2016 at 04:04:19PM +0300, Tomas Winkler wrote: > > There is a HW bug in Skylake, and Broxton PCH Intel PTT device, where > > most of the registers in the control area except START, REQUEST, CANCEL, > > and LOC_CTRL lost retention when the device is in the idle state. Hence > > we need to bring the device to ready state before accessing the other > > registers. The fix brings device to ready state before trying to read > > command and response buffer addresses in order to remap the for access. > > > > Signed-off-by: Tomas Winkler <tomas.winkler-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> > > Tested-by: Jarkko Sakkinen <jarkko.sakkinn-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinn-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> I noticed something odd or at least not described in the commit message. > /Jarkko > > > --- > > V2: cmd read need to be called also before crb_init as this will run > > self test. > > V3: resend. > > > > drivers/char/tpm/tpm_crb.c | 47 ++++++++++++++++++++++++++++++++++++++-------- > > 1 file changed, 39 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c > > index b6923a8b3ff7..e945177cf2c8 100644 > > --- a/drivers/char/tpm/tpm_crb.c > > +++ b/drivers/char/tpm/tpm_crb.c > > @@ -318,6 +318,7 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv, > > struct list_head resources; > > struct resource io_res; > > struct device *dev = &device->dev; > > + u32 pa_high, pa_low; > > u64 cmd_pa; > > u32 cmd_size; > > u64 rsp_pa; > > @@ -345,12 +346,27 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv, > > if (IS_ERR(priv->cca)) > > return PTR_ERR(priv->cca); > > > > - cmd_pa = ((u64) ioread32(&priv->cca->cmd_pa_high) << 32) | > > - (u64) ioread32(&priv->cca->cmd_pa_low); > > + /* > > + * PTT HW bug w/a: wake up the device to access > > + * possibly not retained registers. > > + */ > > + ret = crb_cmd_ready(dev, priv); > > + if (ret) > > + return ret; > > + > > + pa_high = ioread32(&priv->cca->cmd_pa_high); > > + pa_low = ioread32(&priv->cca->cmd_pa_low); > > + cmd_pa = ((u64)pa_high << 32) | pa_low; > > cmd_size = ioread32(&priv->cca->cmd_size); > > + > > + dev_dbg(dev, "cmd_hi = %X cmd_low = %X cmd_size %X\n", > > + pa_high, pa_low, cmd_size); > > + > > priv->cmd = crb_map_res(dev, priv, &io_res, cmd_pa, cmd_size); > > - if (IS_ERR(priv->cmd)) > > - return PTR_ERR(priv->cmd); > > + if (IS_ERR(priv->cmd)) { > > + ret = PTR_ERR(priv->cmd); > > + goto out; > > + } > > > > memcpy_fromio(&rsp_pa, &priv->cca->rsp_pa, 8); > > rsp_pa = le64_to_cpu(rsp_pa); > > @@ -358,7 +374,8 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv, > > > > if (cmd_pa != rsp_pa) { > > priv->rsp = crb_map_res(dev, priv, &io_res, rsp_pa, rsp_size); > > - return PTR_ERR_OR_ZERO(priv->rsp); > > + ret = PTR_ERR_OR_ZERO(priv->rsp); > > + goto out; > > } > > > > /* According to the PTP specification, overlapping command and response > > @@ -366,12 +383,18 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv, > > */ > > if (cmd_size != rsp_size) { > > dev_err(dev, FW_BUG "overlapping command and response buffer sizes are not identical"); > > - return -EINVAL; > > + ret = -EINVAL; > > + goto out; > > } > > + > > priv->cmd_size = cmd_size; > > > > priv->rsp = priv->cmd; > > - return 0; > > + > > +out: > > + crb_go_idle(dev, priv); > > + > > + return ret; > > } > > > > static int crb_acpi_add(struct acpi_device *device) > > @@ -415,7 +438,15 @@ static int crb_acpi_add(struct acpi_device *device) > > if (rc) > > return rc; > > > > - return crb_init(device, priv); > > + rc = crb_cmd_ready(dev, priv); > > + if (rc) > > + return rc; You do this already in crb_map_io() that is called before crb_init(). What is the purpose of this extra crb_cmd_ready()? Looks unrelated at least to the described workaround. > > + > > + rc = crb_init(device, priv); > > + if (rc) > > + crb_go_idle(dev, priv); > > + > > + return rc; > > } > > > > static int crb_acpi_remove(struct acpi_device *device) > > -- > > 2.7.4 /Jarkko ------------------------------------------------------------------------------
next prev parent reply other threads:[~2016-09-27 9:31 UTC|newest] Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top 2016-09-12 13:04 [PATCH v3 0/4] tpm/tpm_crb: implement power management Tomas Winkler 2016-09-12 13:04 ` Tomas Winkler 2016-09-12 13:04 ` [PATCH v3 1/4] tpm/tpm_crb: implement tpm crb idle state Tomas Winkler 2016-09-12 13:04 ` Tomas Winkler 2016-09-15 6:22 ` Jarkko Sakkinen 2016-09-15 6:22 ` Jarkko Sakkinen 2016-09-15 6:28 ` Winkler, Tomas 2016-09-15 6:28 ` Winkler, Tomas 2016-09-15 8:20 ` Jarkko Sakkinen 2016-09-15 8:20 ` Jarkko Sakkinen 2016-09-15 8:23 ` Winkler, Tomas 2016-09-15 8:23 ` Winkler, Tomas 2016-09-15 10:53 ` Jarkko Sakkinen 2016-09-15 10:53 ` Jarkko Sakkinen 2016-09-15 13:09 ` Winkler, Tomas 2016-09-15 13:09 ` Winkler, Tomas 2016-09-12 13:04 ` [PATCH v3 2/4] tmp/tpm_crb: fix Intel PTT hw bug during " Tomas Winkler 2016-09-12 13:04 ` Tomas Winkler 2016-09-15 6:23 ` Jarkko Sakkinen 2016-09-27 9:31 ` Jarkko Sakkinen [this message] 2016-09-27 9:31 ` Jarkko Sakkinen 2016-09-27 9:54 ` Winkler, Tomas 2016-09-27 9:54 ` Winkler, Tomas 2016-09-12 13:04 ` [PATCH v3 3/4] tpm/tpm_crb: open code the crb_init into acpi_add Tomas Winkler 2016-09-12 13:04 ` Tomas Winkler 2016-09-15 6:24 ` Jarkko Sakkinen 2016-09-12 13:04 ` [PATCH v4 4/4] tmp/tpm_crb: implement runtime pm for tpm_crb Tomas Winkler 2016-09-12 13:04 ` Tomas Winkler 2016-09-14 6:28 ` [PATCH v3 0/4] tpm/tpm_crb: implement power management Winkler, Tomas 2016-09-14 6:28 ` Winkler, Tomas 2016-09-14 16:06 ` Jarkko Sakkinen 2016-09-14 16:06 ` Jarkko Sakkinen 2016-09-14 16:06 ` Jarkko Sakkinen 2016-09-14 16:06 ` Jarkko Sakkinen 2016-09-14 18:25 ` Jarkko Sakkinen 2016-09-14 18:25 ` Jarkko Sakkinen
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20160927093100.GA19121@intel.com \ --to=jarkko.sakkinen@linux.intel.com \ --cc=jgunthorpe@obsidianresearch.com \ --cc=linux-kernel@vger.kernel.org \ --cc=tomas.winkler@intel.com \ --cc=tpmdd-devel@lists.sourceforge.net \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.