[PATCH v3 00/13] mailbox: pcc: Fixes and cleanup/refactoring

[PATCH v3 00/13] mailbox: pcc: Fixes and cleanup/refactoring

[Sudeep Holla]

Here is a summary of the changes in this patch series:

1. Fix for race condition in updating of the chan_in_use flag

   Ensures correct updating of the chan_in_use flag to avoid potential race
   conditions.

2. Interrupt handling fix

   Ensures platform acknowledgment interrupts are always cleared to avoid
   leaving the interrupt asserted forever.

3. Endian conversion cleanup

   Removes unnecessary endianness conversion in the PCC mailbox driver.

4. Memory mapping improvements

   Uses acpi_os_ioremap() instead of direct mapping methods for better ACPI
   compatibility.

5. Return early if the command complete register is absent

   Ensures that if no GAS (Generic Address Structure) register is available,
   the function exits early.

6. Refactor IRQ handler and move error handling to a separate function

   Improves readability of error handling in the PCC mailbox driver’s
   interrupt handler.

7. Shared memory mapping refactoring/enhancements

   Ensures the shared memory is always mapped and unmapped in the PCC
   mailbox driver when the PCC channel is requested and release.

8. Refactored check_and_ack() Function

   Simplifies and improves the logic for handling type4 platform notification
   acknowledgments.

09-13. Shared memory handling simplifications across multiple drivers

    Simplifies shared memory handling in:
        Kunpeng HCCS driver (soc: hisilicon)
        Apm X-Gene Slimpro I2C driver
        X-Gene hardware monitoring driver (hwmon)
        ACPI PCC driver
        ACPI CPPC driver

The X-gene related changes now change the mapping attributes to align
with ACPI specification. There are possibilities for more cleanups on
top of these changes around how the shmem is accessed within these
driver.

Also, my main aim is to get 1-8 merged first and target 9-13 for
following merge window through respective tree.

Overall, the patch series focuses on improving correctness, efficiency, and
maintainability of the PCC mailbox driver and related components by fixing
race conditions, optimizing memory handling, simplifying shared memory
interactions, and refactoring code for clarity.

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
Jassi,

Please take patch [1-8]/13 through the mailbox tree if and when you are
happy with the changes. I haven't got Ack from I2C still, but if you are
happy to take [9-13]/13, I can check with I2C maintainer. Or else I am
happy to take it individually later once the PCC changes are merged. I
am still keeping it together if anyone is interested in testing.

Changes in v3:
- Updated the comment around updation of chan_in_use flag to keep it
  appropriate even after acknowledging the interrupt as first action
  in the irq handler
- Added all the review/ack/tested-by tags from Huisong Li, Adam Young
  and Robbie King
- Added a note that double mapping introduced temporarily will not
  impact any existing mbox client drivers as all the drivers move to
  using new and only mapping after all the changes
- s/pcc_chan_check_and_ack/pcc_chan_acknowledge/ which was originally
  check_and_ack()
- Link to v2: https://lore.kernel.org/r/20250305-pcc_fixes_updates-v2-0-1b1822bc8746@arm.com

Changes in v2:
- Improved time vs flow graph for the platform ack interrupt
  acknowledgment issue in patch 2
- Replaced PCC_ACK_FLAG_MASK with PCC_CMD_COMPLETION_NOTIFY in patch 3
- Fixed return value check from pcc_mbox_error_check_and_clear() in patch 6
- Dropped the change moving the function pcc_mbox_ioremap()
- Adjusted error message in kunpeng_hccs driver after the change
- Added the received ack/review tags
- Link to v1: https://lore.kernel.org/r/20250303-pcc_fixes_updates-v1-0-3b44f3d134b1@arm.com

---
Huisong Li (1):
      mailbox: pcc: Fix the possible race in updation of chan_in_use flag

Sudeep Holla (12):
      mailbox: pcc: Always clear the platform ack interrupt first
      mailbox: pcc: Drop unnecessary endianness conversion of pcc_hdr.flags
      mailbox: pcc: Return early if no GAS register from pcc_mbox_cmd_complete_check
      mailbox: pcc: Use acpi_os_ioremap() instead of ioremap()
      mailbox: pcc: Refactor error handling in irq handler into separate function
      mailbox: pcc: Always map the shared memory communication address
      mailbox: pcc: Refactor and simplify check_and_ack()
      soc: hisilicon: kunpeng_hccs: Simplify PCC shared memory region handling
      i2c: xgene-slimpro: Simplify PCC shared memory region handling
      hwmon: (xgene-hwmon) Simplify PCC shared memory region handling
      ACPI: PCC: Simplify PCC shared memory region handling
      ACPI: CPPC: Simplify PCC shared memory region handling

 drivers/acpi/acpi_pcc.c                |  13 +---
 drivers/acpi/cppc_acpi.c               |  16 +----
 drivers/hwmon/xgene-hwmon.c            |  40 ++----------
 drivers/i2c/busses/i2c-xgene-slimpro.c |  39 ++----------
 drivers/mailbox/pcc.c                  | 113 ++++++++++++++++-----------------
 drivers/soc/hisilicon/kunpeng_hccs.c   |  42 +++++-------
 drivers/soc/hisilicon/kunpeng_hccs.h   |   2 -
 include/acpi/pcc.h                     |   6 --
 8 files changed, 84 insertions(+), 187 deletions(-)
---
base-commit: 4d872d51bc9d7b899c1f61534e3dbde72613f627
change-id: 20250303-pcc_fixes_updates-55a17fd28e76
-- 
Regards,
Sudeep

[PATCH v3 11/13] hwmon: (xgene-hwmon) Simplify PCC shared memory region handling

[Sudeep Holla]

The PCC driver now handles mapping and unmapping of shared memory
areas as part of pcc_mbox_{request,free}_channel(). Without these before,
this xgene hwmon driver did handling of those mappings like several
other PCC mailbox client drivers.

There were redundant operations, leading to unnecessary code. Maintaining
the consistency across these driver was harder due to scattered handling
of shmem.

Just use the mapped shmem and remove all redundant operations from this
driver.

Cc: Jean Delvare <jdelvare@suse.com>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: linux-hwmon@vger.kernel.org
Acked-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/hwmon/xgene-hwmon.c | 40 ++++------------------------------------
 1 file changed, 4 insertions(+), 36 deletions(-)

diff --git a/drivers/hwmon/xgene-hwmon.c b/drivers/hwmon/xgene-hwmon.c
index 7087197383c96cb97e4623f419afed01d4f3c716..ea350d4de902c4e6fc4de1cd54a8b75edfad1119 100644
--- a/drivers/hwmon/xgene-hwmon.c
+++ b/drivers/hwmon/xgene-hwmon.c
@@ -102,9 +102,6 @@ struct xgene_hwmon_dev {
 
 	struct device		*hwmon_dev;
 	bool			temp_critical_alarm;
-
-	phys_addr_t		comm_base_addr;
-	void			*pcc_comm_addr;
 	u64			usecs_lat;
 };
 
@@ -125,7 +122,8 @@ static u16 xgene_word_tst_and_clr(u16 *addr, u16 mask)
 
 static int xgene_hwmon_pcc_rd(struct xgene_hwmon_dev *ctx, u32 *msg)
 {
-	struct acpi_pcct_shared_memory *generic_comm_base = ctx->pcc_comm_addr;
+	struct acpi_pcct_shared_memory __iomem *generic_comm_base =
+							ctx->pcc_chan->shmem;
 	u32 *ptr = (void *)(generic_comm_base + 1);
 	int rc, i;
 	u16 val;
@@ -523,7 +521,8 @@ static void xgene_hwmon_rx_cb(struct mbox_client *cl, void *msg)
 static void xgene_hwmon_pcc_rx_cb(struct mbox_client *cl, void *msg)
 {
 	struct xgene_hwmon_dev *ctx = to_xgene_hwmon_dev(cl);
-	struct acpi_pcct_shared_memory *generic_comm_base = ctx->pcc_comm_addr;
+	struct acpi_pcct_shared_memory __iomem *generic_comm_base =
+							ctx->pcc_chan->shmem;
 	struct slimpro_resp_msg amsg;
 
 	/*
@@ -649,7 +648,6 @@ static int xgene_hwmon_probe(struct platform_device *pdev)
 	} else {
 		struct pcc_mbox_chan *pcc_chan;
 		const struct acpi_device_id *acpi_id;
-		int version;
 
 		acpi_id = acpi_match_device(pdev->dev.driver->acpi_match_table,
 					    &pdev->dev);
@@ -658,8 +656,6 @@ static int xgene_hwmon_probe(struct platform_device *pdev)
 			goto out_mbox_free;
 		}
 
-		version = (int)acpi_id->driver_data;
-
 		if (device_property_read_u32(&pdev->dev, "pcc-channel",
 					     &ctx->mbox_idx)) {
 			dev_err(&pdev->dev, "no pcc-channel property\n");
@@ -685,34 +681,6 @@ static int xgene_hwmon_probe(struct platform_device *pdev)
 			goto out;
 		}
 
-		/*
-		 * This is the shared communication region
-		 * for the OS and Platform to communicate over.
-		 */
-		ctx->comm_base_addr = pcc_chan->shmem_base_addr;
-		if (ctx->comm_base_addr) {
-			if (version == XGENE_HWMON_V2)
-				ctx->pcc_comm_addr = (void __force *)devm_ioremap(&pdev->dev,
-								  ctx->comm_base_addr,
-								  pcc_chan->shmem_size);
-			else
-				ctx->pcc_comm_addr = devm_memremap(&pdev->dev,
-								   ctx->comm_base_addr,
-								   pcc_chan->shmem_size,
-								   MEMREMAP_WB);
-		} else {
-			dev_err(&pdev->dev, "Failed to get PCC comm region\n");
-			rc = -ENODEV;
-			goto out;
-		}
-
-		if (IS_ERR_OR_NULL(ctx->pcc_comm_addr)) {
-			dev_err(&pdev->dev,
-				"Failed to ioremap PCC comm region\n");
-			rc = -ENOMEM;
-			goto out;
-		}
-
 		/*
 		 * pcc_chan->latency is just a Nominal value. In reality
 		 * the remote processor could be much slower to reply.

-- 
2.34.1

Re: [PATCH v3 00/13] mailbox: pcc: Fixes and cleanup/refactoring

[Sudeep Holla]

On Thu, Mar 13, 2025 at 03:28:46PM +0000, Sudeep Holla wrote:
> Here is a summary of the changes in this patch series:
> 
> 1. Fix for race condition in updating of the chan_in_use flag
> 
>    Ensures correct updating of the chan_in_use flag to avoid potential race
>    conditions.
> 
> 2. Interrupt handling fix
> 
>    Ensures platform acknowledgment interrupts are always cleared to avoid
>    leaving the interrupt asserted forever.
> 
> 3. Endian conversion cleanup
> 
>    Removes unnecessary endianness conversion in the PCC mailbox driver.
> 
> 4. Memory mapping improvements
> 
>    Uses acpi_os_ioremap() instead of direct mapping methods for better ACPI
>    compatibility.
> 
> 5. Return early if the command complete register is absent
> 
>    Ensures that if no GAS (Generic Address Structure) register is available,
>    the function exits early.
> 
> 6. Refactor IRQ handler and move error handling to a separate function
> 
>    Improves readability of error handling in the PCC mailbox driver’s
>    interrupt handler.
> 
> 7. Shared memory mapping refactoring/enhancements
> 
>    Ensures the shared memory is always mapped and unmapped in the PCC
>    mailbox driver when the PCC channel is requested and release.
> 
> 8. Refactored check_and_ack() Function
> 
>    Simplifies and improves the logic for handling type4 platform notification
>    acknowledgments.
> 
> 09-13. Shared memory handling simplifications across multiple drivers
> 
>     Simplifies shared memory handling in:
>         Kunpeng HCCS driver (soc: hisilicon)
>         Apm X-Gene Slimpro I2C driver
>         X-Gene hardware monitoring driver (hwmon)
>         ACPI PCC driver
>         ACPI CPPC driver
> 
> The X-gene related changes now change the mapping attributes to align
> with ACPI specification. There are possibilities for more cleanups on
> top of these changes around how the shmem is accessed within these
> driver.
> 
> Also, my main aim is to get 1-8 merged first and target 9-13 for
> following merge window through respective tree.
> 
> Overall, the patch series focuses on improving correctness, efficiency, and
> maintainability of the PCC mailbox driver and related components by fixing
> race conditions, optimizing memory handling, simplifying shared memory
> interactions, and refactoring code for clarity.
> 
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
> Jassi,
> 
> Please take patch [1-8]/13 through the mailbox tree if and when you are
> happy with the changes.

Hi Jassi,

I2C change is also acked now. Let me know if you prefer pull request or
you prefer to take it via ACPI tree which may need you ACK.

-- 
Regards,
Sudeep

Re: [PATCH v3 11/13] hwmon: (xgene-hwmon) Simplify PCC shared memory region handling

[Guenter Roeck]

On Thu, Mar 13, 2025 at 03:28:57PM +0000, Sudeep Holla wrote:
> The PCC driver now handles mapping and unmapping of shared memory
> areas as part of pcc_mbox_{request,free}_channel(). Without these before,
> this xgene hwmon driver did handling of those mappings like several
> other PCC mailbox client drivers.
> 
> There were redundant operations, leading to unnecessary code. Maintaining
> the consistency across these driver was harder due to scattered handling
> of shmem.
> 
> Just use the mapped shmem and remove all redundant operations from this
> driver.
> 
> Cc: Jean Delvare <jdelvare@suse.com>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: linux-hwmon@vger.kernel.org
> Acked-by: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

Not that it matters, but I keep wondering: Why don't people use auxiliary
devices for situations like this, and keep the subsystem code where it
belongs ?

I am not requesting that you do it, I just wonder why the mechanism isn't
used. I would have thought that it would be perfect for situations like
this, so I guess I must be missing something, and I'd like to understand
what that something is.

Thanks,
Guenter

Re: [PATCH v3 11/13] hwmon: (xgene-hwmon) Simplify PCC shared memory region handling

[Sudeep Holla]

On Fri, Apr 11, 2025 at 07:15:22AM -0700, Guenter Roeck wrote:
> On Thu, Mar 13, 2025 at 03:28:57PM +0000, Sudeep Holla wrote:
> > The PCC driver now handles mapping and unmapping of shared memory
> > areas as part of pcc_mbox_{request,free}_channel(). Without these before,
> > this xgene hwmon driver did handling of those mappings like several
> > other PCC mailbox client drivers.
> > 
> > There were redundant operations, leading to unnecessary code. Maintaining
> > the consistency across these driver was harder due to scattered handling
> > of shmem.
> > 
> > Just use the mapped shmem and remove all redundant operations from this
> > driver.
> > 
> > Cc: Jean Delvare <jdelvare@suse.com>
> > Cc: Guenter Roeck <linux@roeck-us.net>
> > Cc: linux-hwmon@vger.kernel.org
> > Acked-by: Guenter Roeck <linux@roeck-us.net>
> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> 
> Not that it matters, but I keep wondering: Why don't people use auxiliary
> devices for situations like this, and keep the subsystem code where it
> belongs ?
> 

Good question. I haven't used auxiliary devices but did looks at it recently
when I stumbled across some x86 telemetry code just last week. I need to go
and understand it better to see how it can be used here as I don't have much
understanding ATM other than its uses in GPU and Audio subsystems.

> I am not requesting that you do it, I just wonder why the mechanism isn't
> used. I would have thought that it would be perfect for situations like
> this, so I guess I must be missing something, and I'd like to understand
> what that something is.
> 

Not sure, just that no one spent time think about it and see what is missing
if any and make it work.

-- 
Regards,
Sudeep