diff mbox series

[v3,2/2] mailbox: mpfs: read the system controller's status

Message ID 20221123175652.327859-3-conor@kernel.org (mailing list archive)
State Not Applicable
Headers show
Series mpfs: fix handling failed service requests | expand

Checks

Context Check Description
conchuod/patch_count success Link
conchuod/cover_letter success Series has a cover letter
conchuod/tree_selection success Guessed tree name to be fixes
conchuod/fixes_present success Fixes tag present in non-next series
conchuod/verify_signedoff success Signed-off-by tag matches author and committer
conchuod/kdoc success Errors and warnings before: 0 this patch: 0
conchuod/module_param success Was 0 now: 0
conchuod/build_rv32_defconfig success Build OK
conchuod/build_warn_rv64 success Errors and warnings before: 0 this patch: 0
conchuod/dtb_warn_rv64 success Errors and warnings before: 0 this patch: 0
conchuod/header_inline success No static functions without inline keyword in header files
conchuod/checkpatch success total: 0 errors, 0 warnings, 0 checks, 55 lines checked
conchuod/source_inline success Was 0 now: 0
conchuod/build_rv64_nommu_k210_defconfig success Build OK
conchuod/verify_fixes success Fixes tag looks correct
conchuod/build_rv64_nommu_virt_defconfig success Build OK

Commit Message

Conor Dooley Nov. 23, 2022, 5:56 p.m. UTC 
From: Conor Dooley <conor.dooley@microchip.com>

Some services explicitly return an error code in their response, but
others rely on the system controller to set a status in its status
register. The meaning of the bits varies based on what service is
requested, so pass it back up to the driver that requested the service
in the first place. The field in the message struct already existed, but
was unused until now.

If the system controller is busy, in which case we should never actually
be in the interrupt handler, or if the service fails the mailbox itself
should not be read. Callers should check the status before operating on
the response.

There's an existing, but unused, #define for the mailbox mask - but it
was incorrect. It was doing a GENMASK_ULL(32, 16) which should've just
been a GENMASK(31, 16), so fix that up and start using it.

Fixes: 83d7b1560810 ("mbox: add polarfire soc system controller mailbox")
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 drivers/mailbox/mailbox-mpfs.c | 31 ++++++++++++++++++++++++++++---
 1 file changed, 28 insertions(+), 3 deletions(-)

Comments

Palmer Dabbelt Dec. 9, 2022, 7:14 p.m. UTC | #1 
On Wed, 23 Nov 2022 09:56:52 PST (-0800), Conor Dooley wrote:
> From: Conor Dooley <conor.dooley@microchip.com>
>
> Some services explicitly return an error code in their response, but
> others rely on the system controller to set a status in its status
> register. The meaning of the bits varies based on what service is
> requested, so pass it back up to the driver that requested the service
> in the first place. The field in the message struct already existed, but
> was unused until now.
>
> If the system controller is busy, in which case we should never actually
> be in the interrupt handler, or if the service fails the mailbox itself
> should not be read. Callers should check the status before operating on
> the response.
>
> There's an existing, but unused, #define for the mailbox mask - but it
> was incorrect. It was doing a GENMASK_ULL(32, 16) which should've just
> been a GENMASK(31, 16), so fix that up and start using it.
>
> Fixes: 83d7b1560810 ("mbox: add polarfire soc system controller mailbox")
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
>  drivers/mailbox/mailbox-mpfs.c | 31 ++++++++++++++++++++++++++++---
>  1 file changed, 28 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mailbox/mailbox-mpfs.c b/drivers/mailbox/mailbox-mpfs.c
> index cfacb3f320a6..853901acaeec 100644
> --- a/drivers/mailbox/mailbox-mpfs.c
> +++ b/drivers/mailbox/mailbox-mpfs.c
> @@ -2,7 +2,7 @@
>  /*
>   * Microchip PolarFire SoC (MPFS) system controller/mailbox controller driver
>   *
> - * Copyright (c) 2020 Microchip Corporation. All rights reserved.
> + * Copyright (c) 2020-2022 Microchip Corporation. All rights reserved.
>   *
>   * Author: Conor Dooley <conor.dooley@microchip.com>
>   *
> @@ -56,7 +56,7 @@
>  #define SCB_STATUS_NOTIFY_MASK BIT(SCB_STATUS_NOTIFY)
>
>  #define SCB_STATUS_POS (16)
> -#define SCB_STATUS_MASK GENMASK_ULL(SCB_STATUS_POS + SCB_MASK_WIDTH, SCB_STATUS_POS)
> +#define SCB_STATUS_MASK GENMASK(SCB_STATUS_POS + SCB_MASK_WIDTH - 1, SCB_STATUS_POS)
>
>  struct mpfs_mbox {
>  	struct mbox_controller controller;
> @@ -130,13 +130,38 @@ static void mpfs_mbox_rx_data(struct mbox_chan *chan)
>  	struct mpfs_mbox *mbox = (struct mpfs_mbox *)chan->con_priv;
>  	struct mpfs_mss_response *response = mbox->response;
>  	u16 num_words = ALIGN((response->resp_size), (4)) / 4U;
> -	u32 i;
> +	u32 i, status;
>
>  	if (!response->resp_msg) {
>  		dev_err(mbox->dev, "failed to assign memory for response %d\n", -ENOMEM);
>  		return;
>  	}
>
> +	/*
> +	 * The status is stored in bits 31:16 of the SERVICES_SR register.
> +	 * It is only valid when BUSY == 0.
> +	 * We should *never* get an interrupt while the controller is
> +	 * still in the busy state. If we do, something has gone badly
> +	 * wrong & the content of the mailbox would not be valid.
> +	 */
> +	if (mpfs_mbox_busy(mbox)) {
> +		dev_err(mbox->dev, "got an interrupt but system controller is busy\n");
> +		response->resp_status = 0xDEAD;
> +		return;
> +	}
> +
> +	status = readl_relaxed(mbox->ctrl_base + SERVICES_SR_OFFSET);
> +
> +	/*
> +	 * If the status of the individual servers is non-zero, the service has
> +	 * failed. The contents of the mailbox at this point are not be valid,
> +	 * so don't bother reading them. Set the status so that the driver
> +	 * implementing the service can handle the result.
> +	 */
> +	response->resp_status = (status & SCB_STATUS_MASK) >> SCB_STATUS_POS;
> +	if (response->resp_status)
> +		return;
> +
>  	if (!mpfs_mbox_busy(mbox)) {
>  		for (i = 0; i < num_words; i++) {
>  			response->resp_msg[i] =

Reviewed-by: Palmer Dabbelt <palmer@rivosinc.com>

I'm assuming this is aimed at some other tree.
diff mbox series

Patch

diff --git a/drivers/mailbox/mailbox-mpfs.c b/drivers/mailbox/mailbox-mpfs.c
index cfacb3f320a6..853901acaeec 100644
--- a/drivers/mailbox/mailbox-mpfs.c
+++ b/drivers/mailbox/mailbox-mpfs.c
@@ -2,7 +2,7 @@ 
 /*
  * Microchip PolarFire SoC (MPFS) system controller/mailbox controller driver
  *
- * Copyright (c) 2020 Microchip Corporation. All rights reserved.
+ * Copyright (c) 2020-2022 Microchip Corporation. All rights reserved.
  *
  * Author: Conor Dooley <conor.dooley@microchip.com>
  *
@@ -56,7 +56,7 @@ 
 #define SCB_STATUS_NOTIFY_MASK BIT(SCB_STATUS_NOTIFY)
 
 #define SCB_STATUS_POS (16)
-#define SCB_STATUS_MASK GENMASK_ULL(SCB_STATUS_POS + SCB_MASK_WIDTH, SCB_STATUS_POS)
+#define SCB_STATUS_MASK GENMASK(SCB_STATUS_POS + SCB_MASK_WIDTH - 1, SCB_STATUS_POS)
 
 struct mpfs_mbox {
 	struct mbox_controller controller;
@@ -130,13 +130,38 @@  static void mpfs_mbox_rx_data(struct mbox_chan *chan)
 	struct mpfs_mbox *mbox = (struct mpfs_mbox *)chan->con_priv;
 	struct mpfs_mss_response *response = mbox->response;
 	u16 num_words = ALIGN((response->resp_size), (4)) / 4U;
-	u32 i;
+	u32 i, status;
 
 	if (!response->resp_msg) {
 		dev_err(mbox->dev, "failed to assign memory for response %d\n", -ENOMEM);
 		return;
 	}
 
+	/*
+	 * The status is stored in bits 31:16 of the SERVICES_SR register.
+	 * It is only valid when BUSY == 0.
+	 * We should *never* get an interrupt while the controller is
+	 * still in the busy state. If we do, something has gone badly
+	 * wrong & the content of the mailbox would not be valid.
+	 */
+	if (mpfs_mbox_busy(mbox)) {
+		dev_err(mbox->dev, "got an interrupt but system controller is busy\n");
+		response->resp_status = 0xDEAD;
+		return;
+	}
+
+	status = readl_relaxed(mbox->ctrl_base + SERVICES_SR_OFFSET);
+
+	/*
+	 * If the status of the individual servers is non-zero, the service has
+	 * failed. The contents of the mailbox at this point are not be valid,
+	 * so don't bother reading them. Set the status so that the driver
+	 * implementing the service can handle the result.
+	 */
+	response->resp_status = (status & SCB_STATUS_MASK) >> SCB_STATUS_POS;
+	if (response->resp_status)
+		return;
+
 	if (!mpfs_mbox_busy(mbox)) {
 		for (i = 0; i < num_words; i++) {
 			response->resp_msg[i] =