Dan found and fixed a bug in the SVC HELLO request error handling which was introduced when adding a temporary hack to reconfigure the link at HELLO.
The implementation of the hack abuses the deferred-request processing mechanism to send the link-configuration request, which makes the HELLO processing a bit hard to follow.
The last two patches attempt to remedy this.
This could go into either 5.17-rc or -next.
Johan
Dan Carpenter (1): greybus: svc: fix an error handling bug in gb_svc_hello()
Johan Hovold (2): greybus: svc: clean up hello error path greybus: svc: clean up link configuration hack at hello
drivers/greybus/svc.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)
From: Dan Carpenter dan.carpenter@oracle.com
Cleanup if gb_svc_queue_deferred_request() fails.
Fixes: ee2f2074fdb2 ("greybus: svc: reconfig APBridgeA-Switch link to handle required load") Signed-off-by: Dan Carpenter dan.carpenter@oracle.com Link: https://lore.kernel.org/r/20220202072016.GA6748@kili Cc: stable@vger.kernel.org # 4.9 [johan: fix commit summary prefix and rename label ] Signed-off-by: Johan Hovold johan@kernel.org --- drivers/greybus/svc.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/greybus/svc.c b/drivers/greybus/svc.c index ce7740ef449b..51d0875a3480 100644 --- a/drivers/greybus/svc.c +++ b/drivers/greybus/svc.c @@ -866,8 +866,14 @@ static int gb_svc_hello(struct gb_operation *op)
gb_svc_debugfs_init(svc);
- return gb_svc_queue_deferred_request(op); + ret = gb_svc_queue_deferred_request(op); + if (ret) + goto err_remove_debugfs; + + return 0;
+err_remove_debugfs: + gb_svc_debugfs_exit(svc); err_unregister_device: gb_svc_watchdog_destroy(svc); device_del(&svc->dev);
While currently safe, it is unnecessary (and confusing) to try to destroy the watchdog when watchdog creation fails.
Change the corresponding error path to only deregister the svc.
Signed-off-by: Johan Hovold johan@kernel.org --- drivers/greybus/svc.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/greybus/svc.c b/drivers/greybus/svc.c index 51d0875a3480..4f93d6b2f4ed 100644 --- a/drivers/greybus/svc.c +++ b/drivers/greybus/svc.c @@ -861,7 +861,7 @@ static int gb_svc_hello(struct gb_operation *op) ret = gb_svc_watchdog_create(svc); if (ret) { dev_err(&svc->dev, "failed to create watchdog: %d\n", ret); - goto err_unregister_device; + goto err_deregister_svc; }
gb_svc_debugfs_init(svc); @@ -874,9 +874,10 @@ static int gb_svc_hello(struct gb_operation *op)
err_remove_debugfs: gb_svc_debugfs_exit(svc); -err_unregister_device: gb_svc_watchdog_destroy(svc); +err_deregister_svc: device_del(&svc->dev); + return ret; }
Commit ee2f2074fdb2 ("greybus: svc: reconfig APBridgeA-Switch link to handle required load") added a temporary hack which reconfigures the link at HELLO by abusing the deferred request processing mechanism.
Restructure the HELLO request processing so that the link-configuration work is queued before creating the debugfs files and add a comment explaining why it's there.
Signed-off-by: Johan Hovold johan@kernel.org --- drivers/greybus/svc.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/drivers/greybus/svc.c b/drivers/greybus/svc.c index 4f93d6b2f4ed..56d2b44d6fef 100644 --- a/drivers/greybus/svc.c +++ b/drivers/greybus/svc.c @@ -864,16 +864,19 @@ static int gb_svc_hello(struct gb_operation *op) goto err_deregister_svc; }
- gb_svc_debugfs_init(svc); - + /* + * FIXME: This is a temporary hack to reconfigure the link at HELLO + * (which abuses the deferred request processing mechanism). + */ ret = gb_svc_queue_deferred_request(op); if (ret) - goto err_remove_debugfs; + goto err_destroy_watchdog; + + gb_svc_debugfs_init(svc);
return 0;
-err_remove_debugfs: - gb_svc_debugfs_exit(svc); +err_destroy_watchdog: gb_svc_watchdog_destroy(svc); err_deregister_svc: device_del(&svc->dev);
-
Dan Carpenter -
Johan Hovold