| Message ID | 20221229094738.2304044-1-tzungbi@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 56d4b33d45e39557ac512c10e047fa87f97c24a4 |
| Headers | show |
| Series | platform/chrome: cros_ec_uart: fix race condition | expand |
> diff --git a/drivers/platform/chrome/cros_ec_uart.c b/drivers/platform/chrome/cros_ec_uart.c > index 0cef2888dffd..6916069f1599 100644 > --- a/drivers/platform/chrome/cros_ec_uart.c > +++ b/drivers/platform/chrome/cros_ec_uart.c > @@ -270,7 +270,6 @@ static int cros_ec_uart_probe(struct serdev_device *serdev) > } > > serdev_device_set_drvdata(serdev, ec_dev); > - serdev_device_set_client_ops(serdev, &cros_ec_uart_client_ops); > init_waitqueue_head(&ec_uart->response.wait_queue); > > ec_uart->serdev = serdev; > @@ -300,6 +299,8 @@ static int cros_ec_uart_probe(struct serdev_device *serdev) > sizeof(struct ec_response_get_protocol_info); > ec_dev->dout_size = sizeof(struct ec_host_request); > > + serdev_device_set_client_ops(serdev, &cros_ec_uart_client_ops); > + > return cros_ec_register(ec_dev); > } Thanks for catching this. As a sanity check, I compiled and loaded the driver on my local setup with these changes. LGTM.
On Thu, Dec 29, 2022 at 1:47 AM Tzung-Bi Shih <tzungbi@kernel.org> wrote: > > From: Robert Zieba <robertzieba@google.com> > > serdev_device_set_client_ops() is called before `ec_dev` is fully > initialized. This can result in cros_ec_uart_rx_bytes() being called > while `ec_dev` is still not initialized, resulting in a kernel panic. > > Call serdev_device_set_client_ops() after `ec_dev` is initialized. > > Fixes: 04a8bdd135cc ("platform/chrome: cros_ec_uart: Add transport layer") > Signed-off-by: Robert Zieba <robertzieba@google.com> > [tzungbi: modified commit message and fixed context conflict.] > Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org> Reviewed-by: Guenter Roeck <groeck@chromium.org> > --- > This is from https://crrev.com/c/3490635. I just found the patch when > reviewing some downstream patches. > > drivers/platform/chrome/cros_ec_uart.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/platform/chrome/cros_ec_uart.c b/drivers/platform/chrome/cros_ec_uart.c > index 0cef2888dffd..6916069f1599 100644 > --- a/drivers/platform/chrome/cros_ec_uart.c > +++ b/drivers/platform/chrome/cros_ec_uart.c > @@ -270,7 +270,6 @@ static int cros_ec_uart_probe(struct serdev_device *serdev) > } > > serdev_device_set_drvdata(serdev, ec_dev); > - serdev_device_set_client_ops(serdev, &cros_ec_uart_client_ops); > init_waitqueue_head(&ec_uart->response.wait_queue); > > ec_uart->serdev = serdev; > @@ -300,6 +299,8 @@ static int cros_ec_uart_probe(struct serdev_device *serdev) > sizeof(struct ec_response_get_protocol_info); > ec_dev->dout_size = sizeof(struct ec_host_request); > > + serdev_device_set_client_ops(serdev, &cros_ec_uart_client_ops); > + > return cros_ec_register(ec_dev); > } > > -- > 2.39.0.314.g84b9a713c41-goog >
Hi Mark, On Thu, Dec 29, 2022 at 8:58 AM Mark Hasemeyer <markhas@chromium.org> wrote: > > > diff --git a/drivers/platform/chrome/cros_ec_uart.c b/drivers/platform/chrome/cros_ec_uart.c > > index 0cef2888dffd..6916069f1599 100644 > > --- a/drivers/platform/chrome/cros_ec_uart.c > > +++ b/drivers/platform/chrome/cros_ec_uart.c > > @@ -270,7 +270,6 @@ static int cros_ec_uart_probe(struct serdev_device *serdev) > > } > > > > serdev_device_set_drvdata(serdev, ec_dev); > > - serdev_device_set_client_ops(serdev, &cros_ec_uart_client_ops); > > init_waitqueue_head(&ec_uart->response.wait_queue); > > > > ec_uart->serdev = serdev; > > @@ -300,6 +299,8 @@ static int cros_ec_uart_probe(struct serdev_device *serdev) > > sizeof(struct ec_response_get_protocol_info); > > ec_dev->dout_size = sizeof(struct ec_host_request); > > > > + serdev_device_set_client_ops(serdev, &cros_ec_uart_client_ops); > > + > > return cros_ec_register(ec_dev); > > } > Thanks for catching this. As a sanity check, I compiled and loaded the driver on > my local setup with these changes. LGTM. Please consider replying with a Reviewed-by: and/or Tested-by: tag. Thanks, Guenter
Hello: This patch was applied to chrome-platform/linux.git (for-kernelci) by Tzung-Bi Shih <tzungbi@kernel.org>: On Thu, 29 Dec 2022 17:47:38 +0800 you wrote: > From: Robert Zieba <robertzieba@google.com> > > serdev_device_set_client_ops() is called before `ec_dev` is fully > initialized. This can result in cros_ec_uart_rx_bytes() being called > while `ec_dev` is still not initialized, resulting in a kernel panic. > > Call serdev_device_set_client_ops() after `ec_dev` is initialized. > > [...] Here is the summary with links: - platform/chrome: cros_ec_uart: fix race condition https://git.kernel.org/chrome-platform/c/56d4b33d45e3 You are awesome, thank you!
Hello: This patch was applied to chrome-platform/linux.git (for-next) by Tzung-Bi Shih <tzungbi@kernel.org>: On Thu, 29 Dec 2022 17:47:38 +0800 you wrote: > From: Robert Zieba <robertzieba@google.com> > > serdev_device_set_client_ops() is called before `ec_dev` is fully > initialized. This can result in cros_ec_uart_rx_bytes() being called > while `ec_dev` is still not initialized, resulting in a kernel panic. > > Call serdev_device_set_client_ops() after `ec_dev` is initialized. > > [...] Here is the summary with links: - platform/chrome: cros_ec_uart: fix race condition https://git.kernel.org/chrome-platform/c/56d4b33d45e3 You are awesome, thank you!
> > > diff --git a/drivers/platform/chrome/cros_ec_uart.c b/drivers/platform/chrome/cros_ec_uart.c > > > index 0cef2888dffd..6916069f1599 100644 > > > --- a/drivers/platform/chrome/cros_ec_uart.c > > > +++ b/drivers/platform/chrome/cros_ec_uart.c > > > @@ -270,7 +270,6 @@ static int cros_ec_uart_probe(struct serdev_device *serdev) > > > } > > > > > > serdev_device_set_drvdata(serdev, ec_dev); > > > - serdev_device_set_client_ops(serdev, &cros_ec_uart_client_ops); > > > init_waitqueue_head(&ec_uart->response.wait_queue); > > > > > > ec_uart->serdev = serdev; > > > @@ -300,6 +299,8 @@ static int cros_ec_uart_probe(struct serdev_device *serdev) > > > sizeof(struct ec_response_get_protocol_info); > > > ec_dev->dout_size = sizeof(struct ec_host_request); > > > > > > + serdev_device_set_client_ops(serdev, &cros_ec_uart_client_ops); > > > + > > > return cros_ec_register(ec_dev); > > > } > > Thanks for catching this. As a sanity check, I compiled and loaded the driver on > > my local setup with these changes. LGTM. > > Please consider replying with a Reviewed-by: and/or Tested-by: tag. > FWIW - Tested-by: Mark Hasemeyer <markhas@chromium.org>
diff --git a/drivers/platform/chrome/cros_ec_uart.c b/drivers/platform/chrome/cros_ec_uart.c index 0cef2888dffd..6916069f1599 100644 --- a/drivers/platform/chrome/cros_ec_uart.c +++ b/drivers/platform/chrome/cros_ec_uart.c @@ -270,7 +270,6 @@ static int cros_ec_uart_probe(struct serdev_device *serdev) } serdev_device_set_drvdata(serdev, ec_dev); - serdev_device_set_client_ops(serdev, &cros_ec_uart_client_ops); init_waitqueue_head(&ec_uart->response.wait_queue); ec_uart->serdev = serdev; @@ -300,6 +299,8 @@ static int cros_ec_uart_probe(struct serdev_device *serdev) sizeof(struct ec_response_get_protocol_info); ec_dev->dout_size = sizeof(struct ec_host_request); + serdev_device_set_client_ops(serdev, &cros_ec_uart_client_ops); + return cros_ec_register(ec_dev); }