From: Vanya Lazeev <[email protected]>
Bug link: https://bugzilla.kernel.org/show_bug.cgi?id=195657
cmd/rsp buffers are expected to be in the same ACPI region.
For Zen+ CPUs BIOS's might report two different regions, some of
them also report region sizes inconsistent with values from TPM
registers.
Memory configuration on ASRock x470 ITX:
db0a0000-dc59efff : Reserved
dc57e000-dc57efff : MSFT0101:00
dc582000-dc582fff : MSFT0101:00
Work around the issue by storing ACPI regions declared for the
device in a fixed array and adding an array for pointers to
corresponding possibly allocated resources in crb_map_io function.
This data was previously held for a single resource
in struct crb_priv (iobase field) and local variable io_res in
crb_map_io function. ACPI resources array is used to find index of
corresponding region for each buffer and make the buffer size
consistent with region's length. Array of pointers to allocated
resources is used to map the region at most once.
Signed-off-by: Ivan Lazeev <[email protected]>
---
Changes in v6:
- got rid of new structures
- open coded helper functions
- removed incorrect FW_BUG
drivers/char/tpm/tpm_crb.c | 126 +++++++++++++++++++++++++++----------
1 file changed, 93 insertions(+), 33 deletions(-)
diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index e59f1f91d7f3..8177406aecd6 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -22,6 +22,7 @@
#include "tpm.h"
#define ACPI_SIG_TPM2 "TPM2"
+#define TPM_CRB_MAX_RESOURCES 3
static const guid_t crb_acpi_start_guid =
GUID_INIT(0x6BBF6CAB, 0x5463, 0x4714,
@@ -91,7 +92,6 @@ enum crb_status {
struct crb_priv {
u32 sm;
const char *hid;
- void __iomem *iobase;
struct crb_regs_head __iomem *regs_h;
struct crb_regs_tail __iomem *regs_t;
u8 __iomem *cmd;
@@ -434,21 +434,27 @@ static const struct tpm_class_ops tpm_crb = {
static int crb_check_resource(struct acpi_resource *ares, void *data)
{
- struct resource *io_res = data;
+ struct resource **iores_range = data;
struct resource_win win;
struct resource *res = &(win.res);
if (acpi_dev_resource_memory(ares, res) ||
acpi_dev_resource_address_space(ares, &win)) {
- *io_res = *res;
- io_res->name = NULL;
+ if (iores_range[0] == iores_range[1])
+ iores_range[0] = NULL;
+
+ if (iores_range[0]) {
+ *iores_range[0] = *res;
+ iores_range[0]->name = NULL;
+ iores_range[0] += 1;
+ }
}
return 1;
}
-static void __iomem *crb_map_res(struct device *dev, struct crb_priv *priv,
- struct resource *io_res, u64 start, u32 size)
+static void __iomem *crb_map_res(struct device *dev, struct resource *iores,
+ void __iomem **iobase_ptr, u64 start, u32 size)
{
struct resource new_res = {
.start = start,
@@ -460,10 +466,16 @@ static void __iomem *crb_map_res(struct device *dev, struct crb_priv *priv,
if (start != new_res.start)
return (void __iomem *) ERR_PTR(-EINVAL);
- if (!resource_contains(io_res, &new_res))
+ if (!iores)
return devm_ioremap_resource(dev, &new_res);
- return priv->iobase + (new_res.start - io_res->start);
+ if (!*iobase_ptr) {
+ *iobase_ptr = devm_ioremap_resource(dev, iores);
+ if (IS_ERR(*iobase_ptr))
+ return *iobase_ptr;
+ }
+
+ return *iobase_ptr + (new_res.start - iores->start);
}
/*
@@ -490,9 +502,17 @@ static u64 crb_fixup_cmd_size(struct device *dev, struct resource *io_res,
static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
struct acpi_table_tpm2 *buf)
{
- struct list_head resources;
- struct resource io_res;
+ struct list_head acpi_resource_list;
+ struct resource iores_array[TPM_CRB_MAX_RESOURCES];
+ void __iomem *iobase_array[TPM_CRB_MAX_RESOURCES] = {0};
+ struct resource *iores_range[] = {
+ iores_array,
+ iores_array + TPM_CRB_MAX_RESOURCES
+ };
struct device *dev = &device->dev;
+ struct resource *iores;
+ void __iomem **iobase_ptr;
+ int num_resources, i;
u32 pa_high, pa_low;
u64 cmd_pa;
u32 cmd_size;
@@ -501,21 +521,40 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
u32 rsp_size;
int ret;
- INIT_LIST_HEAD(&resources);
- ret = acpi_dev_get_resources(device, &resources, crb_check_resource,
- &io_res);
+ INIT_LIST_HEAD(&acpi_resource_list);
+ ret = acpi_dev_get_resources(device, &acpi_resource_list,
+ crb_check_resource, iores_range);
if (ret < 0)
return ret;
- acpi_dev_free_resource_list(&resources);
+ acpi_dev_free_resource_list(&acpi_resource_list);
- if (resource_type(&io_res) != IORESOURCE_MEM) {
+ if (iores_range[0] == iores_array) {
dev_err(dev, FW_BUG "TPM2 ACPI table does not define a memory resource\n");
return -EINVAL;
+ } else if (!iores_range[0]) {
+ dev_warn(dev, "TPM2 ACPI table defines too many memory resources\n");
+ iores_range[0] = iores_range[1];
}
- priv->iobase = devm_ioremap_resource(dev, &io_res);
- if (IS_ERR(priv->iobase))
- return PTR_ERR(priv->iobase);
+ num_resources = iores_range[0] - iores_array;
+
+ iores = NULL;
+ iobase_ptr = NULL;
+ for (i = 0; i < num_resources; ++i) {
+ if (buf->control_address >= iores_array[i].start &&
+ buf->control_address + sizeof(struct crb_regs_tail) - 1 <=
+ iores_array[i].end) {
+ iores = iores_array + i;
+ iobase_ptr = iobase_array + i;
+ break;
+ }
+ }
+
+ priv->regs_t = crb_map_res(dev, iores, iobase_ptr, buf->control_address,
+ sizeof(struct crb_regs_tail));
+
+ if (IS_ERR(priv->regs_t))
+ return PTR_ERR(priv->regs_t);
/* The ACPI IO region starts at the head area and continues to include
* the control area, as one nice sane region except for some older
@@ -523,9 +562,10 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
*/
if ((priv->sm == ACPI_TPM2_COMMAND_BUFFER) ||
(priv->sm == ACPI_TPM2_MEMORY_MAPPED)) {
- if (buf->control_address == io_res.start +
+ if (iores &&
+ buf->control_address == iores->start +
sizeof(*priv->regs_h))
- priv->regs_h = priv->iobase;
+ priv->regs_h = *iobase_ptr;
else
dev_warn(dev, FW_BUG "Bad ACPI memory layout");
}
@@ -534,13 +574,6 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
if (ret)
return ret;
- priv->regs_t = crb_map_res(dev, priv, &io_res, buf->control_address,
- sizeof(struct crb_regs_tail));
- if (IS_ERR(priv->regs_t)) {
- ret = PTR_ERR(priv->regs_t);
- goto out_relinquish_locality;
- }
-
/*
* PTT HW bug w/a: wake up the device to access
* possibly not retained registers.
@@ -552,13 +585,26 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
pa_high = ioread32(&priv->regs_t->ctrl_cmd_pa_high);
pa_low = ioread32(&priv->regs_t->ctrl_cmd_pa_low);
cmd_pa = ((u64)pa_high << 32) | pa_low;
- cmd_size = crb_fixup_cmd_size(dev, &io_res, cmd_pa,
- ioread32(&priv->regs_t->ctrl_cmd_size));
+ cmd_size = ioread32(&priv->regs_t->ctrl_cmd_size);
+
+ iores = NULL;
+ iobase_ptr = NULL;
+ for (i = 0; i < num_resources; ++i) {
+ if (cmd_pa >= iores_array[i].start &&
+ cmd_pa <= iores_array[i].end) {
+ iores = iores_array + i;
+ iobase_ptr = iobase_array + i;
+ break;
+ }
+ }
+
+ if (iores)
+ cmd_size = crb_fixup_cmd_size(dev, iores, cmd_pa, 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);
+ priv->cmd = crb_map_res(dev, iores, iobase_ptr, cmd_pa, cmd_size);
if (IS_ERR(priv->cmd)) {
ret = PTR_ERR(priv->cmd);
goto out;
@@ -566,11 +612,25 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
memcpy_fromio(&__rsp_pa, &priv->regs_t->ctrl_rsp_pa, 8);
rsp_pa = le64_to_cpu(__rsp_pa);
- rsp_size = crb_fixup_cmd_size(dev, &io_res, rsp_pa,
- ioread32(&priv->regs_t->ctrl_rsp_size));
+ rsp_size = ioread32(&priv->regs_t->ctrl_rsp_size);
+
+ iores = NULL;
+ iobase_ptr = NULL;
+ for (i = 0; i < num_resources; ++i) {
+ if (rsp_pa >= iores_array[i].start &&
+ rsp_pa <= iores_array[i].end) {
+ iores = iores_array + i;
+ iobase_ptr = iobase_array + i;
+ break;
+ }
+ }
+
+ if (iores)
+ rsp_size = crb_fixup_cmd_size(dev, iores, rsp_pa, rsp_size);
if (cmd_pa != rsp_pa) {
- priv->rsp = crb_map_res(dev, priv, &io_res, rsp_pa, rsp_size);
+ priv->rsp = crb_map_res(dev, iores, iobase_ptr,
+ rsp_pa, rsp_size);
ret = PTR_ERR_OR_ZERO(priv->rsp);
goto out;
}
--
2.20.1
On Wed, Oct 02, 2019 at 11:12:12PM +0300, [email protected] wrote:
> From: Vanya Lazeev <[email protected]>
>
> Bug link: https://bugzilla.kernel.org/show_bug.cgi?id=195657
>
> cmd/rsp buffers are expected to be in the same ACPI region.
> For Zen+ CPUs BIOS's might report two different regions, some of
> them also report region sizes inconsistent with values from TPM
> registers.
>
> Memory configuration on ASRock x470 ITX:
>
> db0a0000-dc59efff : Reserved
> dc57e000-dc57efff : MSFT0101:00
> dc582000-dc582fff : MSFT0101:00
>
> Work around the issue by storing ACPI regions declared for the
> device in a fixed array and adding an array for pointers to
> corresponding possibly allocated resources in crb_map_io function.
> This data was previously held for a single resource
> in struct crb_priv (iobase field) and local variable io_res in
> crb_map_io function. ACPI resources array is used to find index of
> corresponding region for each buffer and make the buffer size
> consistent with region's length. Array of pointers to allocated
> resources is used to map the region at most once.
>
> Signed-off-by: Ivan Lazeev <[email protected]>
I'm getting soon Udoo Bolt that I should be able to use to test this
change.
> ---
> Changes in v6:
> - got rid of new structures
> - open coded helper functions
> - removed incorrect FW_BUG
>
> drivers/char/tpm/tpm_crb.c | 126 +++++++++++++++++++++++++++----------
> 1 file changed, 93 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> index e59f1f91d7f3..8177406aecd6 100644
> --- a/drivers/char/tpm/tpm_crb.c
> +++ b/drivers/char/tpm/tpm_crb.c
> @@ -22,6 +22,7 @@
> #include "tpm.h"
>
> #define ACPI_SIG_TPM2 "TPM2"
> +#define TPM_CRB_MAX_RESOURCES 3
>
> static const guid_t crb_acpi_start_guid =
> GUID_INIT(0x6BBF6CAB, 0x5463, 0x4714,
> @@ -91,7 +92,6 @@ enum crb_status {
> struct crb_priv {
> u32 sm;
> const char *hid;
> - void __iomem *iobase;
> struct crb_regs_head __iomem *regs_h;
> struct crb_regs_tail __iomem *regs_t;
> u8 __iomem *cmd;
> @@ -434,21 +434,27 @@ static const struct tpm_class_ops tpm_crb = {
>
> static int crb_check_resource(struct acpi_resource *ares, void *data)
> {
> - struct resource *io_res = data;
> + struct resource **iores_range = data;
> struct resource_win win;
> struct resource *res = &(win.res);
>
> if (acpi_dev_resource_memory(ares, res) ||
> acpi_dev_resource_address_space(ares, &win)) {
> - *io_res = *res;
> - io_res->name = NULL;
> + if (iores_range[0] == iores_range[1])
> + iores_range[0] = NULL;
> +
> + if (iores_range[0]) {
> + *iores_range[0] = *res;
> + iores_range[0]->name = NULL;
> + iores_range[0] += 1;
> + }
You could get away without iores_range by having an extra
terminator entry in the iores_array.
Overally this starts to look good.
/Jarkko
On Thu, Oct 03, 2019 at 03:03:02PM +0300, Jarkko Sakkinen wrote:
> You could get away without iores_range by having an extra
> terminator entry in the iores_array.
>
> Overally this starts to look good.
I'd also advice you to memset it with zeros so that you don't need to
have a variable for number of entries.
/Jarkko
Hi,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on jss-tpmdd/next]
[cannot apply to v5.4-rc3 next-20191011]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/ivan-lazeev-gmail-com/tpm_crb-fix-fTPM-on-AMD-Zen-CPUs/20191003-054300
base: git://git.infradead.org/users/jjs/linux-tpmdd next
reproduce:
# apt-get install sparse
# sparse version: v0.6.1-rc1-43-g0ccb3b4-dirty
make ARCH=x86_64 allmodconfig
make C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <[email protected]>
sparse warnings: (new ones prefixed by >>)
>> drivers/char/tpm/tpm_crb.c:507:62: sparse: sparse: Using plain integer as NULL pointer
vim +507 drivers/char/tpm/tpm_crb.c
501
502 static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
503 struct acpi_table_tpm2 *buf)
504 {
505 struct list_head acpi_resource_list;
506 struct resource iores_array[TPM_CRB_MAX_RESOURCES];
> 507 void __iomem *iobase_array[TPM_CRB_MAX_RESOURCES] = {0};
508 struct resource *iores_range[] = {
509 iores_array,
510 iores_array + TPM_CRB_MAX_RESOURCES
511 };
512 struct device *dev = &device->dev;
513 struct resource *iores;
514 void __iomem **iobase_ptr;
515 int num_resources, i;
516 u32 pa_high, pa_low;
517 u64 cmd_pa;
518 u32 cmd_size;
519 __le64 __rsp_pa;
520 u64 rsp_pa;
521 u32 rsp_size;
522 int ret;
523
524 INIT_LIST_HEAD(&acpi_resource_list);
525 ret = acpi_dev_get_resources(device, &acpi_resource_list,
526 crb_check_resource, iores_range);
527 if (ret < 0)
528 return ret;
529 acpi_dev_free_resource_list(&acpi_resource_list);
530
531 if (iores_range[0] == iores_array) {
532 dev_err(dev, FW_BUG "TPM2 ACPI table does not define a memory resource\n");
533 return -EINVAL;
534 } else if (!iores_range[0]) {
535 dev_warn(dev, "TPM2 ACPI table defines too many memory resources\n");
536 iores_range[0] = iores_range[1];
537 }
538
539 num_resources = iores_range[0] - iores_array;
540
541 iores = NULL;
542 iobase_ptr = NULL;
543 for (i = 0; i < num_resources; ++i) {
544 if (buf->control_address >= iores_array[i].start &&
545 buf->control_address + sizeof(struct crb_regs_tail) - 1 <=
546 iores_array[i].end) {
547 iores = iores_array + i;
548 iobase_ptr = iobase_array + i;
549 break;
550 }
551 }
552
553 priv->regs_t = crb_map_res(dev, iores, iobase_ptr, buf->control_address,
554 sizeof(struct crb_regs_tail));
555
556 if (IS_ERR(priv->regs_t))
557 return PTR_ERR(priv->regs_t);
558
559 /* The ACPI IO region starts at the head area and continues to include
560 * the control area, as one nice sane region except for some older
561 * stuff that puts the control area outside the ACPI IO region.
562 */
563 if ((priv->sm == ACPI_TPM2_COMMAND_BUFFER) ||
564 (priv->sm == ACPI_TPM2_MEMORY_MAPPED)) {
565 if (iores &&
566 buf->control_address == iores->start +
567 sizeof(*priv->regs_h))
568 priv->regs_h = *iobase_ptr;
569 else
570 dev_warn(dev, FW_BUG "Bad ACPI memory layout");
571 }
572
573 ret = __crb_request_locality(dev, priv, 0);
574 if (ret)
575 return ret;
576
577 /*
578 * PTT HW bug w/a: wake up the device to access
579 * possibly not retained registers.
580 */
581 ret = __crb_cmd_ready(dev, priv);
582 if (ret)
583 goto out_relinquish_locality;
584
585 pa_high = ioread32(&priv->regs_t->ctrl_cmd_pa_high);
586 pa_low = ioread32(&priv->regs_t->ctrl_cmd_pa_low);
587 cmd_pa = ((u64)pa_high << 32) | pa_low;
588 cmd_size = ioread32(&priv->regs_t->ctrl_cmd_size);
589
590 iores = NULL;
591 iobase_ptr = NULL;
592 for (i = 0; i < num_resources; ++i) {
593 if (cmd_pa >= iores_array[i].start &&
594 cmd_pa <= iores_array[i].end) {
595 iores = iores_array + i;
596 iobase_ptr = iobase_array + i;
597 break;
598 }
599 }
600
601 if (iores)
602 cmd_size = crb_fixup_cmd_size(dev, iores, cmd_pa, cmd_size);
603
604 dev_dbg(dev, "cmd_hi = %X cmd_low = %X cmd_size %X\n",
605 pa_high, pa_low, cmd_size);
606
607 priv->cmd = crb_map_res(dev, iores, iobase_ptr, cmd_pa, cmd_size);
608 if (IS_ERR(priv->cmd)) {
609 ret = PTR_ERR(priv->cmd);
610 goto out;
611 }
612
613 memcpy_fromio(&__rsp_pa, &priv->regs_t->ctrl_rsp_pa, 8);
614 rsp_pa = le64_to_cpu(__rsp_pa);
615 rsp_size = ioread32(&priv->regs_t->ctrl_rsp_size);
616
617 iores = NULL;
618 iobase_ptr = NULL;
619 for (i = 0; i < num_resources; ++i) {
620 if (rsp_pa >= iores_array[i].start &&
621 rsp_pa <= iores_array[i].end) {
622 iores = iores_array + i;
623 iobase_ptr = iobase_array + i;
624 break;
625 }
626 }
627
628 if (iores)
629 rsp_size = crb_fixup_cmd_size(dev, iores, rsp_pa, rsp_size);
630
631 if (cmd_pa != rsp_pa) {
632 priv->rsp = crb_map_res(dev, iores, iobase_ptr,
633 rsp_pa, rsp_size);
634 ret = PTR_ERR_OR_ZERO(priv->rsp);
635 goto out;
636 }
637
638 /* According to the PTP specification, overlapping command and response
639 * buffer sizes must be identical.
640 */
641 if (cmd_size != rsp_size) {
642 dev_err(dev, FW_BUG "overlapping command and response buffer sizes are not identical");
643 ret = -EINVAL;
644 goto out;
645 }
646
647 priv->rsp = priv->cmd;
648
649 out:
650 if (!ret)
651 priv->cmd_size = cmd_size;
652
653 __crb_go_idle(dev, priv);
654
655 out_relinquish_locality:
656
657 __crb_relinquish_locality(dev, priv, 0);
658
659 return ret;
660 }
661
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Mon, Oct 14, 2019 at 04:32:20PM +0800, kbuild test robot wrote:
> Hi,
>
> Thank you for the patch! Perhaps something to improve:
Please fix this and send v8 (with full change log).
/Jarkko