From 5ca631ec757bed5843e39c91c8f52d1789991756 Mon Sep 17 00:00:00 2001 From: Conor Dooley Date: Tue, 7 Mar 2023 20:22:55 +0000 Subject: soc: microchip: mpfs: fix some horrible alignment mpfs_sys_controller_delete() has some horrible alignment that upsets my OCD... Move the RHS of the assignment to a new line for greater satifaction. Tested-by: Valentina Fernandez Signed-off-by: Conor Dooley --- drivers/soc/microchip/mpfs-sys-controller.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'drivers/soc/microchip/mpfs-sys-controller.c') diff --git a/drivers/soc/microchip/mpfs-sys-controller.c b/drivers/soc/microchip/mpfs-sys-controller.c index 6e20207b57567..12039cb38b336 100644 --- a/drivers/soc/microchip/mpfs-sys-controller.c +++ b/drivers/soc/microchip/mpfs-sys-controller.c @@ -66,8 +66,8 @@ static void rx_callback(struct mbox_client *client, void *msg) static void mpfs_sys_controller_delete(struct kref *kref) { - struct mpfs_sys_controller *sys_controller = container_of(kref, struct mpfs_sys_controller, - consumers); + struct mpfs_sys_controller *sys_controller = + container_of(kref, struct mpfs_sys_controller, consumers); mbox_free_channel(sys_controller->chan); kfree(sys_controller); -- cgit v1.2.3 From 4f739af1934ad7195db35f74cfbdca264668d8fc Mon Sep 17 00:00:00 2001 From: Conor Dooley Date: Tue, 7 Mar 2023 20:22:56 +0000 Subject: soc: microchip: mpfs: use a consistent completion timeout Completion timeouts use jiffies, so passing a number directly will produce inconsistent timeouts depending on config. Define the timeout in ms and convert it to jiffies instead. Tested-by: Valentina Fernandez Signed-off-by: Conor Dooley --- drivers/soc/microchip/mpfs-sys-controller.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) (limited to 'drivers/soc/microchip/mpfs-sys-controller.c') diff --git a/drivers/soc/microchip/mpfs-sys-controller.c b/drivers/soc/microchip/mpfs-sys-controller.c index 12039cb38b336..738ecd624d64d 100644 --- a/drivers/soc/microchip/mpfs-sys-controller.c +++ b/drivers/soc/microchip/mpfs-sys-controller.c @@ -11,12 +11,15 @@ #include #include #include +#include #include #include #include #include #include +#define MPFS_SYS_CTRL_TIMEOUT_MS 100 + static DEFINE_MUTEX(transaction_lock); struct mpfs_sys_controller { @@ -28,6 +31,7 @@ struct mpfs_sys_controller { int mpfs_blocking_transaction(struct mpfs_sys_controller *sys_controller, struct mpfs_mss_msg *msg) { + unsigned long timeout = msecs_to_jiffies(MPFS_SYS_CTRL_TIMEOUT_MS); int ret, err; err = mutex_lock_interruptible(&transaction_lock); @@ -38,7 +42,7 @@ int mpfs_blocking_transaction(struct mpfs_sys_controller *sys_controller, struct ret = mbox_send_message(sys_controller->chan, msg); if (ret >= 0) { - if (wait_for_completion_timeout(&sys_controller->c, HZ)) { + if (wait_for_completion_timeout(&sys_controller->c, timeout)) { ret = 0; } else { ret = -ETIMEDOUT; -- cgit v1.2.3 From 7606f4dfffa7a4e4aadd8aef918cc8ac1f1e2196 Mon Sep 17 00:00:00 2001 From: Conor Dooley Date: Tue, 7 Mar 2023 20:22:57 +0000 Subject: soc: microchip: mpfs: simplify error handling in mpfs_blocking_transaction() The error handling has a kinda weird nested-if setup that is not really adding anything. Switch it to more of an early return arrangement as a predatory step for adding different handing for timeouts and failed services. Tested-by: Valentina Fernandez Signed-off-by: Conor Dooley --- drivers/soc/microchip/mpfs-sys-controller.c | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-) (limited to 'drivers/soc/microchip/mpfs-sys-controller.c') diff --git a/drivers/soc/microchip/mpfs-sys-controller.c b/drivers/soc/microchip/mpfs-sys-controller.c index 738ecd624d64d..e61ba9b7aae30 100644 --- a/drivers/soc/microchip/mpfs-sys-controller.c +++ b/drivers/soc/microchip/mpfs-sys-controller.c @@ -32,28 +32,27 @@ struct mpfs_sys_controller { int mpfs_blocking_transaction(struct mpfs_sys_controller *sys_controller, struct mpfs_mss_msg *msg) { unsigned long timeout = msecs_to_jiffies(MPFS_SYS_CTRL_TIMEOUT_MS); - int ret, err; + int ret; - err = mutex_lock_interruptible(&transaction_lock); - if (err) - return err; + ret = mutex_lock_interruptible(&transaction_lock); + if (ret) + return ret; reinit_completion(&sys_controller->c); ret = mbox_send_message(sys_controller->chan, msg); - if (ret >= 0) { - if (wait_for_completion_timeout(&sys_controller->c, timeout)) { - ret = 0; - } else { - ret = -ETIMEDOUT; - dev_warn(sys_controller->client.dev, - "MPFS sys controller transaction timeout\n"); - } + if (ret < 0) + goto out; + + if (!wait_for_completion_timeout(&sys_controller->c, timeout)) { + ret = -ETIMEDOUT; + dev_warn(sys_controller->client.dev, "MPFS sys controller transaction timeout\n"); } else { - dev_err(sys_controller->client.dev, - "mpfs sys controller transaction returned %d\n", ret); + /* mbox_send_message() returns positive integers on success */ + ret = 0; } +out: mutex_unlock(&transaction_lock); return ret; -- cgit v1.2.3 From 8f943dd12eeff71857b9a1ca45adbaaba379bf30 Mon Sep 17 00:00:00 2001 From: Conor Dooley Date: Tue, 7 Mar 2023 20:22:58 +0000 Subject: soc: microchip: mpfs: handle timeouts and failed services differently The system controller will only deliver an interrupt if a service succeeds. This leaves us in the unfortunate position with current code where there is no way to differentiate between a legitimate timeout where the service has not completed & where it has completed, but failed. mbox_send_message() has its own completion, and it will time out of the system controller does not lower the busy flag. In this case, a timeout has occurred and the error can be propagated back to the caller. If the busy flag is lowered, but no interrupt has arrived to trigger the rx callback, the service can be deemed to have failed. Report -EBADMSG in this case so that callers can differentiate. Tested-by: Valentina Fernandez Signed-off-by: Conor Dooley --- drivers/soc/microchip/mpfs-sys-controller.c | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-) (limited to 'drivers/soc/microchip/mpfs-sys-controller.c') diff --git a/drivers/soc/microchip/mpfs-sys-controller.c b/drivers/soc/microchip/mpfs-sys-controller.c index e61ba9b7aae30..ceaeebc1fc6be 100644 --- a/drivers/soc/microchip/mpfs-sys-controller.c +++ b/drivers/soc/microchip/mpfs-sys-controller.c @@ -18,7 +18,11 @@ #include #include -#define MPFS_SYS_CTRL_TIMEOUT_MS 100 +/* + * This timeout must be long, as some services (example: image authentication) + * take significant time to complete + */ +#define MPFS_SYS_CTRL_TIMEOUT_MS 30000 static DEFINE_MUTEX(transaction_lock); @@ -41,14 +45,26 @@ int mpfs_blocking_transaction(struct mpfs_sys_controller *sys_controller, struct reinit_completion(&sys_controller->c); ret = mbox_send_message(sys_controller->chan, msg); - if (ret < 0) + if (ret < 0) { + dev_warn(sys_controller->client.dev, "MPFS sys controller service timeout\n"); goto out; + } + /* + * Unfortunately, the system controller will only deliver an interrupt + * if a service succeeds. mbox_send_message() will block until the busy + * flag is gone. If the busy flag is gone but no interrupt has arrived + * to trigger the rx callback then the service can be deemed to have + * failed. + * The caller can then interrogate msg::response::resp_status to + * determine the cause of the failure. + * mbox_send_message() returns positive integers in the success path, so + * ret needs to be cleared if we do get an interrupt. + */ if (!wait_for_completion_timeout(&sys_controller->c, timeout)) { - ret = -ETIMEDOUT; - dev_warn(sys_controller->client.dev, "MPFS sys controller transaction timeout\n"); + ret = -EBADMSG; + dev_warn(sys_controller->client.dev, "MPFS sys controller service failed\n"); } else { - /* mbox_send_message() returns positive integers on success */ ret = 0; } @@ -107,6 +123,7 @@ static int mpfs_sys_controller_probe(struct platform_device *pdev) sys_controller->client.dev = dev; sys_controller->client.rx_callback = rx_callback; sys_controller->client.tx_block = 1U; + sys_controller->client.tx_tout = msecs_to_jiffies(MPFS_SYS_CTRL_TIMEOUT_MS); sys_controller->chan = mbox_request_channel(&sys_controller->client, 0); if (IS_ERR(sys_controller->chan)) { -- cgit v1.2.3 From 4dd472bdafcb660bea17bc63a97d06e24fcb36ed Mon Sep 17 00:00:00 2001 From: Conor Dooley Date: Fri, 31 Mar 2023 08:18:17 +0100 Subject: soc: microchip: mpfs: add a prefix to rx_callback() Add a prefix to the function name to match the rest of the file. Signed-off-by: Conor Dooley --- drivers/soc/microchip/mpfs-sys-controller.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'drivers/soc/microchip/mpfs-sys-controller.c') diff --git a/drivers/soc/microchip/mpfs-sys-controller.c b/drivers/soc/microchip/mpfs-sys-controller.c index ceaeebc1fc6be..216d9f4ea0ce9 100644 --- a/drivers/soc/microchip/mpfs-sys-controller.c +++ b/drivers/soc/microchip/mpfs-sys-controller.c @@ -75,7 +75,7 @@ out: } EXPORT_SYMBOL(mpfs_blocking_transaction); -static void rx_callback(struct mbox_client *client, void *msg) +static void mpfs_sys_controller_rx_callback(struct mbox_client *client, void *msg) { struct mpfs_sys_controller *sys_controller = container_of(client, struct mpfs_sys_controller, client); @@ -121,7 +121,7 @@ static int mpfs_sys_controller_probe(struct platform_device *pdev) return -ENOMEM; sys_controller->client.dev = dev; - sys_controller->client.rx_callback = rx_callback; + sys_controller->client.rx_callback = mpfs_sys_controller_rx_callback; sys_controller->client.tx_block = 1U; sys_controller->client.tx_tout = msecs_to_jiffies(MPFS_SYS_CTRL_TIMEOUT_MS); -- cgit v1.2.3