| Message ID | 20240505130800.2546640-1-weifeng.liu.z@gmail.com |
|---|---|
| Headers | show |
| Series | Defer probing of SAM if serdev device is not ready | expand |
On Sun, May 05, 2024 at 09:07:49PM +0800, Weifeng Liu wrote: > This is an attempt to alleviate race conditions in the SAM driver where > essential resources like serial device and GPIO pins are not ready at > the time ssam_serial_hub_probe() is called. Instead of giving up > probing, a better way would be to defer the probing by returning > -EPROBE_DEFER, allowing the kernel try again later. > > However, there is no way of identifying all such cases from other real > errors in a few days. So let's take a gradual approach identify and > address these cases as they arise. This commit marks the initial step > in this process. It's a bit pointless to send a new version while we haven't settled yet down on the previous one. Moreover, there is no added details as I asked in the previous round of review. The decision of moving this part to serdev is up to Hans, but I think we also can at least put TODO line here with explanations you gave in the reply to v2 that this is currently the only driver needs this and there is still a chance that more might need it. While writing the above paragraph I realised that this might be due to non-standard appearance of the device in DSDT, that it gets enumerated before the controller. Do you have a DSDT excerpt for the controller and device parts in the order of appearance?
Hi, On 5/5/24 3:07 PM, Weifeng Liu wrote: > v3 changes: > * better formatting, nothing special > > v2 changes: > * resolves Andy's comments > > Original letter: > > Greetings, > > This series is intended to remedy a race condition where surface > aggregator module (SAM) which is a serdev driver could fail to probe if > the underlying UART port is not ready to open. In such circumstance, > invoking serdev_device_open() gets errno -ENXIO, leading to failure in > probing of SAM. However, if the probe is retried in a short delay, > serdev_device_open() would work as expected and everything just goes > fine. As a workaround, adding the serial driver (8250_dw) into > initramfs or building it into the kernel image significantly mitigates > the likelihood of encountering this race condition, as in this way the > serial device would be initialized much earlier than probing of SAM. > > However, IMO we should reliably avoid this sort of race condition. A > good way is returning -EPROBE_DEFER when serdev_device_open returns > -ENXIO so that the kernel will be able to retry the probing later. This > is what the first patch tries to do. > > Though this solution might be a good enough solution for this specific > issue, I am wondering why this kind of race condition could ever happen, > i.e., why a serdes device could be not ready yet at the moment the > serdev driver gets called and tries to bind it. And even if this is an > expected behavior how serdev driver works, I do feel it a little bit > weird that we need to identify serdev_device_open() returning -ENXIO as > non-fatal error and thus return -EPROBE_DEFER manually in such case, as > I don't see this sort of behavior in other serdev driver. Maximilian > and Hans suggested discussing the root cause of the race condition here. > I will be grateful if you could provide some reasoning and insights on > this. > > Following is the code path when the issue occurs: > > ssam_serial_hub_probe() > serdev_device_open() > ctrl->ops->open() /* this callback being ttyport_open() */ > tty->ops->open() /* this callback being uart_open() */ > tty_port_open() > port->ops->activate() /* this callback being uart_port_activate() */ > Find bit UPF_DEAD is set in uport->flags and fail with errno -ENXIO. > > I notice that flag UPF_DEAD would be set in serial_core_register_port() > during calling serial_core_add_one_port() but don't have much idea > what's going on under the hood. Thank you this explanation + Andy's questions about this were quite useful. I think I have found the root cause of this problem and I have attached a patch which should fix this. After dropping your own fix from your local kernel you should be able to reproduce this 100% of the time by making the surface_aggregator module builtin (CONFIG_SURFACE_AGGREGATOR=y) while making 8250_dw a module (CONFIG_SERIAL_8250_DW=m). Although I'm not sure if the uart will then not simply be initialzed as something generic. You could also try building both into the kernel and see if the issue reproduces then. Once you can reproduce this reliably, give the attached patch a try to fix this please. Regards, Hans > > Additionally, add logs to the probe procedure of SAM in the second > patch, hoping it could help provide context to user when something > miserable happens. > > Context of this series is available in [1]. > > Best regards, > Weifeng > > Weifeng Liu (2): > platform/surface: aggregator: Defer probing when serdev is not ready > platform/surface: aggregator: Log critical errors during SAM probing > > drivers/platform/surface/aggregator/core.c | 53 ++++++++++++++++------ > 1 file changed, 39 insertions(+), 14 deletions(-) >
On Mon, May 06, 2024 at 04:41:19PM +0200, Hans de Goede wrote: > On 5/5/24 3:07 PM, Weifeng Liu wrote: ... > If a serdev_device_driver is already loaded for a serdev_tty_port when it > gets registered by tty_port_register_device_attr_serdev() then that > driver's probe() method will be called immediately. > > The serdev_device_driver's probe() method should then be able to call > serdev_device_open() successfully, but because UPF_DEAD is still dead > serdev_device_open() will fail with -ENXIO in this scenario: > > serdev_device_open() > ctrl->ops->open() /* this callback being ttyport_open() */ > tty->ops->open() /* this callback being uart_open() */ > tty_port_open() > port->ops->activate() /* this callback being uart_port_activate() */ > Find bit UPF_DEAD is set in uport->flags and fail with errno -ENXIO. > > Fix this be clearing UPF_DEAD before tty_port_register_device_attr_serdev() > note this only moves up the UPD_DEAD clearing a small bit, before: > > tty_port_register_device_attr_serdev(); > mutex_unlock(&tty_port.mutex); > uart_port.flags &= ~UPF_DEAD; > mutex_unlock(&port_mutex); > > after: > > uart_port.flags &= ~UPF_DEAD; > tty_port_register_device_attr_serdev(); > mutex_unlock(&tty_port.mutex); > mutex_unlock(&port_mutex); > Reported-by: Weifeng Liu <weifeng.liu.z@gmail.com> > Closes: https://lore.kernel.org/platform-driver-x86/20240505130800.2546640-1-weifeng.liu.z@gmail.com/ > Cc: Maximilian Luz <luzmaximilian@gmail.com> > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Can you move Cc after the cutter '---' line? The patch makes sense to me, FWIW, Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> but Cc'ed Tony just in case. > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/tty/serial/serial_core.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c > index ff85ebd3a007..d9424fe6513b 100644 > --- a/drivers/tty/serial/serial_core.c > +++ b/drivers/tty/serial/serial_core.c > @@ -3196,6 +3196,9 @@ static int serial_core_add_one_port(struct uart_driver *drv, struct uart_port *u > if (uport->attr_group) > uport->tty_groups[1] = uport->attr_group; > > + /* Ensure serdev drivers can call serdev_device_open() right away */ > + uport->flags &= ~UPF_DEAD; > + > /* > * Register the port whether it's detected or not. This allows > * setserial to be used to alter this port's parameters. > @@ -3206,6 +3209,7 @@ static int serial_core_add_one_port(struct uart_driver *drv, struct uart_port *u > if (!IS_ERR(tty_dev)) { > device_set_wakeup_capable(tty_dev, 1); > } else { > + uport->flags |= UPF_DEAD; > dev_err(uport->dev, "Cannot register tty device on line %d\n", > uport->line); > } > @@ -3411,8 +3415,6 @@ int serial_core_register_port(struct uart_driver *drv, struct uart_port *port) > if (ret) > goto err_unregister_port_dev; > > - port->flags &= ~UPF_DEAD; > - > mutex_unlock(&port_mutex); > > return 0;
Hi, On Mon, 2024-05-06 at 16:41 +0200, Hans de Goede wrote: > Hi, > > On 5/5/24 3:07 PM, Weifeng Liu wrote: > > v3 changes: > > * better formatting, nothing special > > > > v2 changes: > > * resolves Andy's comments > > > > Original letter: > > > > Greetings, > > > > This series is intended to remedy a race condition where surface > > aggregator module (SAM) which is a serdev driver could fail to probe if > > the underlying UART port is not ready to open. In such circumstance, > > invoking serdev_device_open() gets errno -ENXIO, leading to failure in > > probing of SAM. However, if the probe is retried in a short delay, > > serdev_device_open() would work as expected and everything just goes > > fine. As a workaround, adding the serial driver (8250_dw) into > > initramfs or building it into the kernel image significantly mitigates > > the likelihood of encountering this race condition, as in this way the > > serial device would be initialized much earlier than probing of SAM. > > > > However, IMO we should reliably avoid this sort of race condition. A > > good way is returning -EPROBE_DEFER when serdev_device_open returns > > -ENXIO so that the kernel will be able to retry the probing later. This > > is what the first patch tries to do. > > > > Though this solution might be a good enough solution for this specific > > issue, I am wondering why this kind of race condition could ever happen, > > i.e., why a serdes device could be not ready yet at the moment the > > serdev driver gets called and tries to bind it. And even if this is an > > expected behavior how serdev driver works, I do feel it a little bit > > weird that we need to identify serdev_device_open() returning -ENXIO as > > non-fatal error and thus return -EPROBE_DEFER manually in such case, as > > I don't see this sort of behavior in other serdev driver. Maximilian > > and Hans suggested discussing the root cause of the race condition here. > > I will be grateful if you could provide some reasoning and insights on > > this. > > > > Following is the code path when the issue occurs: > > > > ssam_serial_hub_probe() > > serdev_device_open() > > ctrl->ops->open() /* this callback being ttyport_open() */ > > tty->ops->open() /* this callback being uart_open() */ > > tty_port_open() > > port->ops->activate() /* this callback being uart_port_activate() */ > > Find bit UPF_DEAD is set in uport->flags and fail with errno -ENXIO. > > > > I notice that flag UPF_DEAD would be set in serial_core_register_port() > > during calling serial_core_add_one_port() but don't have much idea > > what's going on under the hood. > > Thank you this explanation + Andy's questions about this were quite > useful. I think I have found the root cause of this problem and I have > attached a patch which should fix this. > > After dropping your own fix from your local kernel you should be able > to reproduce this 100% of the time by making the surface_aggregator > module builtin (CONFIG_SURFACE_AGGREGATOR=y) while making 8250_dw > a module (CONFIG_SERIAL_8250_DW=m). Although I'm not sure if the uart > will then not simply be initialzed as something generic. You could also > try building both into the kernel and see if the issue reproduces then. > As per your instructions, I * removed my fixes, * built surface_aggregator into the kernal, * built 8250_dw as a module and * removed 8250_dw from initramfs as well, and found the occurrence rate of the issue was around 30%. With your patch applied, the issue disappeared completely. Tested-by: Weifeng Liu <weifeng.liu.z@gmail.com> Really glad to see the final solution in serial core for this issue. This might also help other serdev drivers. Thank you all. Best regards, Weifeng > Once you can reproduce this reliably, give the attached patch a try > to fix this please. > > Regards, > > Hans > > > > > > > Additionally, add logs to the probe procedure of SAM in the second > > patch, hoping it could help provide context to user when something > > miserable happens. > > > > Context of this series is available in [1]. > > > > Best regards, > > Weifeng > > > > Weifeng Liu (2): > > platform/surface: aggregator: Defer probing when serdev is not ready > > platform/surface: aggregator: Log critical errors during SAM probing > > > > drivers/platform/surface/aggregator/core.c | 53 ++++++++++++++++------ > > 1 file changed, 39 insertions(+), 14 deletions(-)