| Message ID | 20240811124645.246016-1-luzmaximilian@gmail.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Ilpo Järvinen |
| Headers | show |
| Series | platform/surface: aggregator: Fix warning when controller is destroyed in probe | expand |
On Sun, 11 Aug 2024 14:46:44 +0200, Maximilian Luz wrote: > There is a small window in ssam_serial_hub_probe() where the controller > is initialized but has not been started yet. Specifically, between > ssam_controller_init() and ssam_controller_start(). Any failure in this > window, for example caused by a failure of serdev_device_open(), > currently results in an incorrect warning being emitted. > > In particular, any failure in this window results in the controller > being destroyed via ssam_controller_destroy(). This function checks the > state of the controller and, in an attempt to validate that the > controller has been cleanly shut down before we try and deallocate any > resources, emits a warning if that state is not SSAM_CONTROLLER_STOPPED. > > [...] Thank you for your contribution, it has been applied to my local review-ilpo branch. Note it will show up in the public platform-drivers-x86/review-ilpo branch only once I've pushed my local branch there, which might take a while. The list of commits applied: [1/1] platform/surface: aggregator: Fix warning when controller is destroyed in probe commit: bc923d594db21bee0ead128eb4bb78f7e77467a4 -- i.
diff --git a/drivers/platform/surface/aggregator/controller.c b/drivers/platform/surface/aggregator/controller.c index 7fc602e01487..7e89f547999b 100644 --- a/drivers/platform/surface/aggregator/controller.c +++ b/drivers/platform/surface/aggregator/controller.c @@ -1354,7 +1354,8 @@ void ssam_controller_destroy(struct ssam_controller *ctrl) if (ctrl->state == SSAM_CONTROLLER_UNINITIALIZED) return; - WARN_ON(ctrl->state != SSAM_CONTROLLER_STOPPED); + WARN_ON(ctrl->state != SSAM_CONTROLLER_STOPPED && + ctrl->state != SSAM_CONTROLLER_INITIALIZED); /* * Note: New events could still have been received after the previous
There is a small window in ssam_serial_hub_probe() where the controller is initialized but has not been started yet. Specifically, between ssam_controller_init() and ssam_controller_start(). Any failure in this window, for example caused by a failure of serdev_device_open(), currently results in an incorrect warning being emitted. In particular, any failure in this window results in the controller being destroyed via ssam_controller_destroy(). This function checks the state of the controller and, in an attempt to validate that the controller has been cleanly shut down before we try and deallocate any resources, emits a warning if that state is not SSAM_CONTROLLER_STOPPED. However, since we have only just initialized the controller and have not yet started it, its state is SSAM_CONTROLLER_INITIALIZED. Note that this is the only point at which the controller has this state, as it will change after we start the controller with ssam_controller_start() and never revert back. Further, at this point no communication has taken place and the sender and receiver threads have not been started yet (and we may not even have an open serdev device either). Therefore, it is perfectly safe to call ssam_controller_destroy() with a state of SSAM_CONTROLLER_INITIALIZED. This, however, means that the warning currently being emitted is incorrect. Fix it by extending the check. Fixes: c167b9c7e3d6 ("platform/surface: Add Surface Aggregator subsystem") Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com> --- drivers/platform/surface/aggregator/controller.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)