| Message ID | 20221118222043.1214776-3-conor@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | mpfs: fix handling failed service requests | expand |
| 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 |
On 18/11/2022 22:20, Conor Dooley wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > 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. > > 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, 29 insertions(+), 2 deletions(-) > > diff --git a/drivers/mailbox/mailbox-mpfs.c b/drivers/mailbox/mailbox-mpfs.c > index cfacb3f320a6..6b99abac0b11 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> > * > @@ -23,6 +23,8 @@ > #define MAILBOX_REG_OFFSET 0x800u > #define MSS_SYS_MAILBOX_DATA_OFFSET 0u > #define SCB_MASK_WIDTH 16u > +#define SCB_STATUS_SHIFT 16u > +#define SCB_STATUS_MASK GENMASK(31, SCB_STATUS_SHIFT) *sigh* these macros aren't needed & generate some -Wmacro-redefined issues. I'll do a v3 in a few days.
diff --git a/drivers/mailbox/mailbox-mpfs.c b/drivers/mailbox/mailbox-mpfs.c index cfacb3f320a6..6b99abac0b11 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> * @@ -23,6 +23,8 @@ #define MAILBOX_REG_OFFSET 0x800u #define MSS_SYS_MAILBOX_DATA_OFFSET 0u #define SCB_MASK_WIDTH 16u +#define SCB_STATUS_SHIFT 16u +#define SCB_STATUS_MASK GENMASK(31, SCB_STATUS_SHIFT) /* SCBCTRL service control register */ @@ -130,13 +132,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_SHIFT; + if (response->resp_status) + return; + if (!mpfs_mbox_busy(mbox)) { for (i = 0; i < num_words; i++) { response->resp_msg[i] =