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

[Palmer Dabbelt]

On Wed, 23 Nov 2022 09:56:52 PST (-0800), Conor Dooley wrote:
> From: Conor Dooley <conor.dooley@xxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxx>
> ---
>  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@xxxxxxxxxxxxx>
>   *
> @@ -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@xxxxxxxxxxxx>

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