From e61985d0550df8c2078310202aaad9b41049c36c Mon Sep 17 00:00:00 2001 From: Junxiao Chang Date: Mon, 8 Apr 2019 17:40:22 +0800 Subject: platform/x86: intel_pmc_ipc: adding error handling If punit or telemetry device initialization fails, pmc driver should unregister and return failure. This change is to fix a kernel panic when removing kernel module intel_pmc_ipc. Fixes: 48c1917088ba ("platform:x86: Add Intel telemetry platform device") Signed-off-by: Junxiao Chang Signed-off-by: Andy Shevchenko --- drivers/platform/x86/intel_pmc_ipc.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) (limited to 'drivers/platform/x86/intel_pmc_ipc.c') diff --git a/drivers/platform/x86/intel_pmc_ipc.c b/drivers/platform/x86/intel_pmc_ipc.c index 7964ba22ef8d..d37cbd1cf58c 100644 --- a/drivers/platform/x86/intel_pmc_ipc.c +++ b/drivers/platform/x86/intel_pmc_ipc.c @@ -771,13 +771,17 @@ static int ipc_create_pmc_devices(void) if (ret) { dev_err(ipcdev.dev, "Failed to add punit platform device\n"); platform_device_unregister(ipcdev.tco_dev); + return ret; } if (!ipcdev.telem_res_inval) { ret = ipc_create_telemetry_device(); - if (ret) + if (ret) { dev_warn(ipcdev.dev, "Failed to add telemetry platform device\n"); + platform_device_unregister(ipcdev.punit_dev); + platform_device_unregister(ipcdev.tco_dev); + } } return ret; -- cgit v1.2.3 From 0084cf6a504347da07066e020d0cf7b87eb2a274 Mon Sep 17 00:00:00 2001 From: Andy Shevchenko Date: Tue, 9 Apr 2019 14:25:12 +0300 Subject: platform/x86: intel_pmc_ipc: Use BIT() macro Use BIT() and BIT_MASK() macros for definitions. Signed-off-by: Andy Shevchenko Reviewed-by: Kuppuswamy Sathyanarayanan --- drivers/platform/x86/intel_pmc_ipc.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) (limited to 'drivers/platform/x86/intel_pmc_ipc.c') diff --git a/drivers/platform/x86/intel_pmc_ipc.c b/drivers/platform/x86/intel_pmc_ipc.c index d37cbd1cf58c..eb0b342996ca 100644 --- a/drivers/platform/x86/intel_pmc_ipc.c +++ b/drivers/platform/x86/intel_pmc_ipc.c @@ -41,13 +41,13 @@ * the IPC1 registers, updates the IPC_STS response register with the status. */ #define IPC_CMD 0x0 -#define IPC_CMD_MSI 0x100 +#define IPC_CMD_MSI BIT(8) #define IPC_CMD_SIZE 16 #define IPC_CMD_SUBCMD 12 #define IPC_STATUS 0x04 -#define IPC_STATUS_IRQ 0x4 -#define IPC_STATUS_ERR 0x2 -#define IPC_STATUS_BUSY 0x1 +#define IPC_STATUS_IRQ BIT(2) +#define IPC_STATUS_ERR BIT(1) +#define IPC_STATUS_BUSY BIT(0) #define IPC_SPTR 0x08 #define IPC_DPTR 0x0C #define IPC_WRITE_BUFFER 0x80 @@ -107,7 +107,7 @@ /* PMC register bit definitions */ /* PMC_CFG_REG bit masks */ -#define PMC_CFG_NO_REBOOT_MASK (1 << 4) +#define PMC_CFG_NO_REBOOT_MASK BIT_MASK(4) #define PMC_CFG_NO_REBOOT_EN (1 << 4) #define PMC_CFG_NO_REBOOT_DIS (0 << 4) -- cgit v1.2.3 From 9eac0d75f132608159eb649ceadfc4e53b2a1686 Mon Sep 17 00:00:00 2001 From: Andy Shevchenko Date: Tue, 9 Apr 2019 14:25:13 +0300 Subject: platform/x86: intel_pmc_ipc: Apply same width for offset definitions Apply same width for offset definitions to make code more consistent. Signed-off-by: Andy Shevchenko Reviewed-by: Kuppuswamy Sathyanarayanan --- drivers/platform/x86/intel_pmc_ipc.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'drivers/platform/x86/intel_pmc_ipc.c') diff --git a/drivers/platform/x86/intel_pmc_ipc.c b/drivers/platform/x86/intel_pmc_ipc.c index eb0b342996ca..9007aa717586 100644 --- a/drivers/platform/x86/intel_pmc_ipc.c +++ b/drivers/platform/x86/intel_pmc_ipc.c @@ -40,7 +40,7 @@ * The ARC handles the interrupt and services it, writing optional data to * the IPC1 registers, updates the IPC_STS response register with the status. */ -#define IPC_CMD 0x0 +#define IPC_CMD 0x00 #define IPC_CMD_MSI BIT(8) #define IPC_CMD_SIZE 16 #define IPC_CMD_SUBCMD 12 @@ -101,8 +101,8 @@ #define TELEM_SSRAM_SIZE 240 #define TELEM_PMC_SSRAM_OFFSET 0x1B00 #define TELEM_PUNIT_SSRAM_OFFSET 0x1A00 -#define TCO_PMC_OFFSET 0x8 -#define TCO_PMC_SIZE 0x4 +#define TCO_PMC_OFFSET 0x08 +#define TCO_PMC_SIZE 0x04 /* PMC register bit definitions */ -- cgit v1.2.3 From af6c7e1ffcb6db6c1265295fa1b20fc27a0753f2 Mon Sep 17 00:00:00 2001 From: Andy Shevchenko Date: Tue, 9 Apr 2019 14:25:14 +0300 Subject: platform/x86: intel_pmc_ipc: Don't map non-used optional resources The intel_pmc_ipc driver has a placeholder for all possible resources that may have been provided by ACPI. Since there are few optional ones, the driver still uses them and binds to wrong ranges in resource tree: # grep intel_punit_ipc /proc/iomem 00000000-00000000 : intel_punit_ipc 00000000-00000000 : intel_punit_ipc 00000000-00000000 : intel_punit_ipc 00000000-00000000 : intel_punit_ipc This leads to issues with resource management during inserting and removing modules, such as intel_pmc_ipc itself, which can't be inserted anymore after first removal. Count the actual resources provided and supply only them to the child device. This is a real fix of the commit 8cc7fb4a6523 ("intel_pmc_ipc: update acpi resource structure for Punit") that also fixes a symptoms described in the commit 6cc8cbbc8868 ("platform/x86: intel_punit_ipc: Fix resource ioremap warning") that is going to be reverted afterwards. Reported-by: Junxiao Chang Signed-off-by: Andy Shevchenko Cc: Qipeng Zha Cc: Kuppuswamy Sathyanarayanan Reviewed-by: Kuppuswamy Sathyanarayanan --- drivers/platform/x86/intel_pmc_ipc.c | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-) (limited to 'drivers/platform/x86/intel_pmc_ipc.c') diff --git a/drivers/platform/x86/intel_pmc_ipc.c b/drivers/platform/x86/intel_pmc_ipc.c index 9007aa717586..55037ff258f8 100644 --- a/drivers/platform/x86/intel_pmc_ipc.c +++ b/drivers/platform/x86/intel_pmc_ipc.c @@ -131,6 +131,7 @@ static struct intel_pmc_ipc_dev { /* punit */ struct platform_device *punit_dev; + unsigned int punit_res_count; /* Telemetry */ resource_size_t telem_pmc_ssram_base; @@ -682,7 +683,7 @@ static int ipc_create_punit_device(void) .name = PUNIT_DEVICE_NAME, .id = -1, .res = punit_res_array, - .num_res = ARRAY_SIZE(punit_res_array), + .num_res = ipcdev.punit_res_count, }; pdev = platform_device_register_full(&pdevinfo); @@ -789,7 +790,7 @@ static int ipc_create_pmc_devices(void) static int ipc_plat_get_res(struct platform_device *pdev) { - struct resource *res, *punit_res; + struct resource *res, *punit_res = punit_res_array; void __iomem *addr; int size; @@ -804,7 +805,8 @@ static int ipc_plat_get_res(struct platform_device *pdev) ipcdev.acpi_io_size = size; dev_info(&pdev->dev, "io res: %pR\n", res); - punit_res = punit_res_array; + ipcdev.punit_res_count = 0; + /* This is index 0 to cover BIOS data register */ res = platform_get_resource(pdev, IORESOURCE_MEM, PLAT_RESOURCE_BIOS_DATA_INDEX); @@ -812,7 +814,7 @@ static int ipc_plat_get_res(struct platform_device *pdev) dev_err(&pdev->dev, "Failed to get res of punit BIOS data\n"); return -ENXIO; } - *punit_res = *res; + punit_res[ipcdev.punit_res_count++] = *res; dev_info(&pdev->dev, "punit BIOS data res: %pR\n", res); /* This is index 1 to cover BIOS interface register */ @@ -822,42 +824,38 @@ static int ipc_plat_get_res(struct platform_device *pdev) dev_err(&pdev->dev, "Failed to get res of punit BIOS iface\n"); return -ENXIO; } - *++punit_res = *res; + punit_res[ipcdev.punit_res_count++] = *res; dev_info(&pdev->dev, "punit BIOS interface res: %pR\n", res); /* This is index 2 to cover ISP data register, optional */ res = platform_get_resource(pdev, IORESOURCE_MEM, PLAT_RESOURCE_ISP_DATA_INDEX); - ++punit_res; if (res) { - *punit_res = *res; + punit_res[ipcdev.punit_res_count++] = *res; dev_info(&pdev->dev, "punit ISP data res: %pR\n", res); } /* This is index 3 to cover ISP interface register, optional */ res = platform_get_resource(pdev, IORESOURCE_MEM, PLAT_RESOURCE_ISP_IFACE_INDEX); - ++punit_res; if (res) { - *punit_res = *res; + punit_res[ipcdev.punit_res_count++] = *res; dev_info(&pdev->dev, "punit ISP interface res: %pR\n", res); } /* This is index 4 to cover GTD data register, optional */ res = platform_get_resource(pdev, IORESOURCE_MEM, PLAT_RESOURCE_GTD_DATA_INDEX); - ++punit_res; if (res) { - *punit_res = *res; + punit_res[ipcdev.punit_res_count++] = *res; dev_info(&pdev->dev, "punit GTD data res: %pR\n", res); } /* This is index 5 to cover GTD interface register, optional */ res = platform_get_resource(pdev, IORESOURCE_MEM, PLAT_RESOURCE_GTD_IFACE_INDEX); - ++punit_res; if (res) { - *punit_res = *res; + punit_res[ipcdev.punit_res_count++] = *res; dev_info(&pdev->dev, "punit GTD interface res: %pR\n", res); } -- cgit v1.2.3