| Message ID | 1473247953-24617-3-git-send-email-tomas.winkler@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Sep 07, 2016 at 02:32:32PM +0300, Tomas Winkler wrote: > INIT_LIST_HEAD(&resources); > - ret = acpi_dev_get_resources(device, &resources, crb_check_resource, > - &io_res); > + ret = acpi_dev_get_resources(device, &resources, > + crb_check_resource, &io_res); Do not randomly reflow unrelated text in patches > + /* > + * 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; Why change from the original hunk? - cmd_pa = ((u64) ioread32(&priv->cca->cmd_pa_high) << 32) | - (u64) ioread32(&priv->cca->cmd_pa_low); > priv->rsp = priv->cmd; > + > +out: > + __crb_go_idle(dev, priv); > return 0; Nope on the return 0.. return ret I guess? Jason ------------------------------------------------------------------------------
> > On Wed, Sep 07, 2016 at 02:32:32PM +0300, Tomas Winkler wrote: > > INIT_LIST_HEAD(&resources); > > - ret = acpi_dev_get_resources(device, &resources, > crb_check_resource, > > - &io_res); > > + ret = acpi_dev_get_resources(device, &resources, > > + crb_check_resource, &io_res); > > Do not randomly reflow unrelated text in patches It wasn't random, who breaks code like that... > > + /* > > + * 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; > > Why change from the original hunk? This is where the bug is visible... I'll put the debug print back it might be useful. > - cmd_pa = ((u64) ioread32(&priv->cca->cmd_pa_high) << 32) | > - (u64) ioread32(&priv->cca->cmd_pa_low); > > > priv->rsp = priv->cmd; > > + > > +out: > > + __crb_go_idle(dev, priv); > > return 0; > > Nope on the return 0.. return ret I guess? Right, good catch. ------------------------------------------------------------------------------
On Wed, Sep 07, 2016 at 09:21:34PM +0000, Winkler, Tomas wrote: > > > > > On Wed, Sep 07, 2016 at 02:32:32PM +0300, Tomas Winkler wrote: > > > INIT_LIST_HEAD(&resources); > > > - ret = acpi_dev_get_resources(device, &resources, > > crb_check_resource, > > > - &io_res); > > > + ret = acpi_dev_get_resources(device, &resources, > > > + crb_check_resource, &io_res); > > > > Do not randomly reflow unrelated text in patches > > It wasn't random, who breaks code like that... .. it has nothing to do with this patch. ., and I have no idea why you think that is better, the original is what clang-format produces and it computes an optimal break point using Knuth's algorithm... > > > + /* > > > + * 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; > > > > Why change from the original hunk? > > This is where the bug is visible... I'll put the debug print back it might be useful. I don't get it, what is the difference? read ordering? > > - cmd_pa = ((u64) ioread32(&priv->cca->cmd_pa_high) << 32) | > > - (u64) ioread32(&priv->cca->cmd_pa_low); Jason ------------------------------------------------------------------------------
> > On Wed, Sep 07, 2016 at 09:21:34PM +0000, Winkler, Tomas wrote: > > > > > > > > On Wed, Sep 07, 2016 at 02:32:32PM +0300, Tomas Winkler wrote: > > > > INIT_LIST_HEAD(&resources); > > > > - ret = acpi_dev_get_resources(device, &resources, > > > crb_check_resource, > > > > - &io_res); > > > > + ret = acpi_dev_get_resources(device, &resources, > > > > + crb_check_resource, &io_res); > > > > > > Do not randomly reflow unrelated text in patches > > > > It wasn't random, who breaks code like that... > > .. it has nothing to do with this patch. > > ., and I have no idea why you think that is better, the original is what clang- > format produces and it computes an optimal break point using Knuth's > algorithm... For me it lacks some visual symmetry, but what I can expect from an optimal algorithm :) I've revered it already :) > > > > > + /* > > > > + * 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; > > > > > > Why change from the original hunk? > > > > This is where the bug is visible... I'll put the debug print back it might be > useful. > > I don't get it, what is the difference? read ordering? No, read order is not relevant here, just wanted to have each register printed dev_dbg(dev, "cmd_hi = 0x%X cmd_low = 0x%X cmd_size %d\n", pa_high, pa_low, cmd_size); Tomas ------------------------------------------------------------------------------
On Wed, Sep 07, 2016 at 09:52:38PM +0000, Winkler, Tomas wrote: > > I don't get it, what is the difference? read ordering? > > No, read order is not relevant here, just wanted to have each register printed > > dev_dbg(dev, "cmd_hi = 0x%X cmd_low = 0x%X cmd_size %d\n", > pa_high, pa_low, cmd_size); Oh, well if you've dropped the dev_dbg then drop the needless change too. Jason ------------------------------------------------------------------------------
On Wed, Sep 07, 2016 at 02:32:32PM +0300, Tomas Winkler wrote: > There is a HW bug in Skylake, Kabylake, 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> I think I would add the dev_dbg line permanently with it that you have in responses in this thread. There's been some unrelated bugs before where this information in klog would have been very handy. Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> /Jarkko > --- > drivers/char/tpm/tpm_crb.c | 37 +++++++++++++++++++++++++++---------- > 1 file changed, 27 insertions(+), 10 deletions(-) > > diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c > index 98a7fdfe9936..cf9aab698dfe 100644 > --- a/drivers/char/tpm/tpm_crb.c > +++ b/drivers/char/tpm/tpm_crb.c > @@ -328,6 +328,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; > @@ -335,15 +336,14 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv, > int ret; > > INIT_LIST_HEAD(&resources); > - ret = acpi_dev_get_resources(device, &resources, crb_check_resource, > - &io_res); > + ret = acpi_dev_get_resources(device, &resources, > + crb_check_resource, &io_res); > if (ret < 0) > return ret; > acpi_dev_free_resource_list(&resources); > > if (resource_type(&io_res) != IORESOURCE_MEM) { > - dev_err(dev, > - FW_BUG "TPM2 ACPI table does not define a memory resource\n"); > + dev_err(dev, FW_BUG "TPM2 ACPI table does not define a memory resource\n"); > return -EINVAL; > } > > @@ -356,12 +356,24 @@ 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); > + > 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); > @@ -369,7 +381,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 > @@ -377,10 +390,14 @@ 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->rsp = priv->cmd; > + > +out: > + __crb_go_idle(dev, priv); > return 0; > } > > -- > 2.7.4 > ------------------------------------------------------------------------------
On Thu, Sep 08, 2016 at 02:14:58PM +0300, Jarkko Sakkinen wrote: > On Wed, Sep 07, 2016 at 02:32:32PM +0300, Tomas Winkler wrote: > > There is a HW bug in Skylake, Kabylake, 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> > > I think I would add the dev_dbg line permanently with it that you have > in responses in this thread. There's been some unrelated bugs before > where this information in klog would have been very handy. > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> (with Broxton PCH) /Jarkko ------------------------------------------------------------------------------
diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c index 98a7fdfe9936..cf9aab698dfe 100644 --- a/drivers/char/tpm/tpm_crb.c +++ b/drivers/char/tpm/tpm_crb.c @@ -328,6 +328,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; @@ -335,15 +336,14 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv, int ret; INIT_LIST_HEAD(&resources); - ret = acpi_dev_get_resources(device, &resources, crb_check_resource, - &io_res); + ret = acpi_dev_get_resources(device, &resources, + crb_check_resource, &io_res); if (ret < 0) return ret; acpi_dev_free_resource_list(&resources); if (resource_type(&io_res) != IORESOURCE_MEM) { - dev_err(dev, - FW_BUG "TPM2 ACPI table does not define a memory resource\n"); + dev_err(dev, FW_BUG "TPM2 ACPI table does not define a memory resource\n"); return -EINVAL; } @@ -356,12 +356,24 @@ 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); + 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); @@ -369,7 +381,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 @@ -377,10 +390,14 @@ 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->rsp = priv->cmd; + +out: + __crb_go_idle(dev, priv); return 0; }
There is a HW bug in Skylake, Kabylake, 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> --- drivers/char/tpm/tpm_crb.c | 37 +++++++++++++++++++++++++++---------- 1 file changed, 27 insertions(+), 10 deletions(-)