* [PATCH v3 0/2] mpfs: fix handling failed service requests
@ 2022-11-23 17:56 Conor Dooley
2022-11-23 17:56 ` [PATCH v3 1/2] soc: microchip: mpfs: handle failed system " Conor Dooley
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Conor Dooley @ 2022-11-23 17:56 UTC (permalink / raw)
To: Conor Dooley, Jassi Brar; +Cc: Daire McNamara, linux-riscv, linux-kernel
From: Conor Dooley <conor.dooley@microchip.com>
As things stand, if a service request fails, we never know about it as
the status register is not read. This is quite obviously far from ideal,
as we would be reading garbage from the mailbox if a service was to
fail.
This pair of patches adds handling of the service status to both sides
of the mailbox implementation.
@Jassi, there should be no build, or really functional, dependency here
between patches. In the failing case, the buffer is allocated by the
service driver, so things will "work" just as badly as they did before
if only one of the patches is present. As a result, should be safe to do
the mailbox patch via your tree & the client via the soc tree.
Thanks,
Conor.
Changes since v2:
- Stop redefining the already existing #defines for status register.
Changes since v1:
- Send the patches I meant to send, with an extraneous ; removed.
Conor Dooley (2):
soc: microchip: mpfs: handle failed system service requests
mailbox: mpfs: read the system controller's status
drivers/mailbox/mailbox-mpfs.c | 31 +++++++++++++++++++--
drivers/soc/microchip/mpfs-sys-controller.c | 6 ++++
2 files changed, 34 insertions(+), 3 deletions(-)
--
2.38.1
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH v3 1/2] soc: microchip: mpfs: handle failed system service requests 2022-11-23 17:56 [PATCH v3 0/2] mpfs: fix handling failed service requests Conor Dooley @ 2022-11-23 17:56 ` Conor Dooley 2022-12-09 19:14 ` Palmer Dabbelt 2022-11-23 17:56 ` [PATCH v3 2/2] mailbox: mpfs: read the system controller's status Conor Dooley 2022-12-26 22:46 ` (subset) [PATCH v3 0/2] mpfs: fix handling failed service requests Conor Dooley 2 siblings, 1 reply; 6+ messages in thread From: Conor Dooley @ 2022-11-23 17:56 UTC (permalink / raw) To: Conor Dooley, Jassi Brar; +Cc: Daire McNamara, linux-riscv, linux-kernel From: Conor Dooley <conor.dooley@microchip.com> If a service request fails, a non-zero, per-service error code will be set. Since the individual service drivers may wish to handle things differently, there's little point trying to do anything intelligent in the system controller driver. Let the caller know that things went wrong & leave the details of handling the error to it. Fixes: d0054a470c33 ("soc: add microchip polarfire soc system controller") Signed-off-by: Conor Dooley <conor.dooley@microchip.com> --- drivers/soc/microchip/mpfs-sys-controller.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/drivers/soc/microchip/mpfs-sys-controller.c b/drivers/soc/microchip/mpfs-sys-controller.c index 6e20207b5756..539fc24b397d 100644 --- a/drivers/soc/microchip/mpfs-sys-controller.c +++ b/drivers/soc/microchip/mpfs-sys-controller.c @@ -52,6 +52,12 @@ int mpfs_blocking_transaction(struct mpfs_sys_controller *sys_controller, struct mutex_unlock(&transaction_lock); + if (ret) + return ret; + + if (msg->response->resp_status) + ret = -EIO; + return ret; } EXPORT_SYMBOL(mpfs_blocking_transaction); -- 2.38.1 _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v3 1/2] soc: microchip: mpfs: handle failed system service requests 2022-11-23 17:56 ` [PATCH v3 1/2] soc: microchip: mpfs: handle failed system " Conor Dooley @ 2022-12-09 19:14 ` Palmer Dabbelt 0 siblings, 0 replies; 6+ messages in thread From: Palmer Dabbelt @ 2022-12-09 19:14 UTC (permalink / raw) To: Conor Dooley Cc: Conor Dooley, jassisinghbrar, daire.mcnamara, linux-riscv, linux-kernel On Wed, 23 Nov 2022 09:56:51 PST (-0800), Conor Dooley wrote: > From: Conor Dooley <conor.dooley@microchip.com> > > If a service request fails, a non-zero, per-service error code will be > set. Since the individual service drivers may wish to handle things > differently, there's little point trying to do anything intelligent in > the system controller driver. Let the caller know that things went wrong > & leave the details of handling the error to it. > > Fixes: d0054a470c33 ("soc: add microchip polarfire soc system controller") > Signed-off-by: Conor Dooley <conor.dooley@microchip.com> > --- > drivers/soc/microchip/mpfs-sys-controller.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/soc/microchip/mpfs-sys-controller.c b/drivers/soc/microchip/mpfs-sys-controller.c > index 6e20207b5756..539fc24b397d 100644 > --- a/drivers/soc/microchip/mpfs-sys-controller.c > +++ b/drivers/soc/microchip/mpfs-sys-controller.c > @@ -52,6 +52,12 @@ int mpfs_blocking_transaction(struct mpfs_sys_controller *sys_controller, struct > > mutex_unlock(&transaction_lock); > > + if (ret) > + return ret; > + > + if (msg->response->resp_status) > + ret = -EIO; > + > return ret; > } > EXPORT_SYMBOL(mpfs_blocking_transaction); Reviewed-by: Palmer Dabbelt <palmer@rivosinc.com> _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v3 2/2] mailbox: mpfs: read the system controller's status 2022-11-23 17:56 [PATCH v3 0/2] mpfs: fix handling failed service requests Conor Dooley 2022-11-23 17:56 ` [PATCH v3 1/2] soc: microchip: mpfs: handle failed system " Conor Dooley @ 2022-11-23 17:56 ` Conor Dooley 2022-12-09 19:14 ` Palmer Dabbelt 2022-12-26 22:46 ` (subset) [PATCH v3 0/2] mpfs: fix handling failed service requests Conor Dooley 2 siblings, 1 reply; 6+ messages in thread From: Conor Dooley @ 2022-11-23 17:56 UTC (permalink / raw) To: Conor Dooley, Jassi Brar; +Cc: Daire McNamara, linux-riscv, linux-kernel 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] = -- 2.38.1 _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v3 2/2] mailbox: mpfs: read the system controller's status 2022-11-23 17:56 ` [PATCH v3 2/2] mailbox: mpfs: read the system controller's status Conor Dooley @ 2022-12-09 19:14 ` Palmer Dabbelt 0 siblings, 0 replies; 6+ messages in thread From: Palmer Dabbelt @ 2022-12-09 19:14 UTC (permalink / raw) To: Conor Dooley Cc: Conor Dooley, jassisinghbrar, daire.mcnamara, linux-riscv, linux-kernel 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. _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: (subset) [PATCH v3 0/2] mpfs: fix handling failed service requests 2022-11-23 17:56 [PATCH v3 0/2] mpfs: fix handling failed service requests Conor Dooley 2022-11-23 17:56 ` [PATCH v3 1/2] soc: microchip: mpfs: handle failed system " Conor Dooley 2022-11-23 17:56 ` [PATCH v3 2/2] mailbox: mpfs: read the system controller's status Conor Dooley @ 2022-12-26 22:46 ` Conor Dooley 2 siblings, 0 replies; 6+ messages in thread From: Conor Dooley @ 2022-12-26 22:46 UTC (permalink / raw) To: Conor Dooley, Jassi Brar Cc: Conor Dooley, linux-riscv, Daire McNamara, linux-kernel From: Conor Dooley <conor.dooley@microchip.com> On Wed, 23 Nov 2022 17:56:50 +0000, Conor Dooley wrote: > From: Conor Dooley <conor.dooley@microchip.com> > > As things stand, if a service request fails, we never know about it as > the status register is not read. This is quite obviously far from ideal, > as we would be reading garbage from the mailbox if a service was to > fail. > > [...] Applied to riscv-soc-fixes, thanks! [1/2] soc: microchip: mpfs: handle failed system service requests https://git.kernel.org/conor/c/730892135b7d Thanks, Conor. _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-12-26 22:46 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-11-23 17:56 [PATCH v3 0/2] mpfs: fix handling failed service requests Conor Dooley 2022-11-23 17:56 ` [PATCH v3 1/2] soc: microchip: mpfs: handle failed system " Conor Dooley 2022-12-09 19:14 ` Palmer Dabbelt 2022-11-23 17:56 ` [PATCH v3 2/2] mailbox: mpfs: read the system controller's status Conor Dooley 2022-12-09 19:14 ` Palmer Dabbelt 2022-12-26 22:46 ` (subset) [PATCH v3 0/2] mpfs: fix handling failed service requests Conor Dooley
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).