* [PATCH v2 0/2] Defer probing of SAM if serdev device is not ready
@ 2024-05-03 3:08 Weifeng Liu
2024-05-03 3:08 ` [PATCH v2 1/2] platform/surface: aggregator: Defer probing when serdev " Weifeng Liu
2024-05-03 3:08 ` [PATCH v2 2/2] platform/surface: aggregator: Log critical errors during SAM probing Weifeng Liu
0 siblings, 2 replies; 6+ messages in thread
From: Weifeng Liu @ 2024-05-03 3:08 UTC (permalink / raw
To: platform-driver-x86, linux-serial
Cc: Weifeng Liu, Andy Shevchenko, Maximilian Luz, Hans de Goede
Hi all,
This version simply resolves Andy's comments.
Best regards,
Weifeng
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.
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
[1] https://github.com/linux-surface/kernel/pull/152
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 | 47 +++++++++++++++++-----
1 file changed, 38 insertions(+), 9 deletions(-)
--
2.44.0
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 1/2] platform/surface: aggregator: Defer probing when serdev is not ready
2024-05-03 3:08 [PATCH v2 0/2] Defer probing of SAM if serdev device is not ready Weifeng Liu
@ 2024-05-03 3:08 ` Weifeng Liu
2024-05-03 17:03 ` Andy Shevchenko
2024-05-03 3:08 ` [PATCH v2 2/2] platform/surface: aggregator: Log critical errors during SAM probing Weifeng Liu
1 sibling, 1 reply; 6+ messages in thread
From: Weifeng Liu @ 2024-05-03 3:08 UTC (permalink / raw
To: platform-driver-x86, linux-serial
Cc: Weifeng Liu, Andy Shevchenko, Maximilian Luz, Hans de Goede
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.
Suggested-by: Maximilian Luz <luzmaximilian@gmail.com>
Reviewed-by: Maximilian Luz <luzmaximilian@gmail.com>
Acked-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Weifeng Liu <weifeng.liu.z@gmail.com>
---
drivers/platform/surface/aggregator/core.c | 15 ++++++++++++++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/drivers/platform/surface/aggregator/core.c b/drivers/platform/surface/aggregator/core.c
index 9591a28bc38a..87dea91f91fe 100644
--- a/drivers/platform/surface/aggregator/core.c
+++ b/drivers/platform/surface/aggregator/core.c
@@ -645,9 +645,22 @@ static int ssam_serial_hub_probe(struct serdev_device *serdev)
/* Set up serdev device. */
serdev_device_set_drvdata(serdev, ctrl);
serdev_device_set_client_ops(serdev, &ssam_serdev_ops);
+
+ /*
+ * The following step can fail when it's called too early before the
+ * underlying UART device is ready (in this case -ENXIO is returned).
+ * Instead of simply giving up and losing everything, we can defer
+ * the probing by returning -EPROBE_DEFER so that the kernel would be
+ * able to retry later.
+ */
status = serdev_device_open(serdev);
- if (status)
+ if (status == -ENXIO)
+ status = -EPROBE_DEFER;
+ if (status) {
+ dev_err_probe(&serdev->dev, status,
+ "failed to open serdev device\n");
goto err_devopen;
+ }
astatus = ssam_serdev_setup_via_acpi(ssh->handle, serdev);
if (ACPI_FAILURE(astatus)) {
--
2.44.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2 2/2] platform/surface: aggregator: Log critical errors during SAM probing
2024-05-03 3:08 [PATCH v2 0/2] Defer probing of SAM if serdev device is not ready Weifeng Liu
2024-05-03 3:08 ` [PATCH v2 1/2] platform/surface: aggregator: Defer probing when serdev " Weifeng Liu
@ 2024-05-03 3:08 ` Weifeng Liu
2024-05-03 16:36 ` Andy Shevchenko
1 sibling, 1 reply; 6+ messages in thread
From: Weifeng Liu @ 2024-05-03 3:08 UTC (permalink / raw
To: platform-driver-x86, linux-serial
Cc: Weifeng Liu, Andy Shevchenko, Maximilian Luz, Hans de Goede
Emits messages upon errors during probing of SAM. Hopefully this could
provide useful context to user for the purpose of diagnosis when
something miserable happen.
Reviewed-by: Maximilian Luz <luzmaximilian@gmail.com>
Acked-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Weifeng Liu <weifeng.liu.z@gmail.com>
---
drivers/platform/surface/aggregator/core.c | 32 ++++++++++++++++------
1 file changed, 24 insertions(+), 8 deletions(-)
diff --git a/drivers/platform/surface/aggregator/core.c b/drivers/platform/surface/aggregator/core.c
index 87dea91f91fe..b3359ce13e0d 100644
--- a/drivers/platform/surface/aggregator/core.c
+++ b/drivers/platform/surface/aggregator/core.c
@@ -623,8 +623,9 @@ static int ssam_serial_hub_probe(struct serdev_device *serdev)
acpi_status astatus;
int status;
- if (gpiod_count(&serdev->dev, NULL) < 0)
- return -ENODEV;
+ status = gpiod_count(&serdev->dev, NULL);
+ if (status < 0)
+ return dev_err_probe(&serdev->dev, status, "no GPIO found\n");
status = devm_acpi_dev_add_driver_gpios(&serdev->dev, ssam_acpi_gpios);
if (status)
@@ -637,8 +638,11 @@ static int ssam_serial_hub_probe(struct serdev_device *serdev)
/* Initialize controller. */
status = ssam_controller_init(ctrl, serdev);
- if (status)
+ if (status) {
+ dev_err_probe(&serdev->dev, status,
+ "failed to initialize ssam controller\n");
goto err_ctrl_init;
+ }
ssam_controller_lock(ctrl);
@@ -664,7 +668,8 @@ static int ssam_serial_hub_probe(struct serdev_device *serdev)
astatus = ssam_serdev_setup_via_acpi(ssh->handle, serdev);
if (ACPI_FAILURE(astatus)) {
- status = -ENXIO;
+ status = dev_err_probe(&serdev->dev, -ENXIO,
+ "failed to setup serdev\n");
goto err_devinit;
}
@@ -680,16 +685,25 @@ static int ssam_serial_hub_probe(struct serdev_device *serdev)
* states.
*/
status = ssam_log_firmware_version(ctrl);
- if (status)
+ if (status) {
+ dev_err_probe(&serdev->dev, status,
+ "failed to get firmware version\n");
goto err_initrq;
+ }
status = ssam_ctrl_notif_d0_entry(ctrl);
- if (status)
+ if (status) {
+ dev_err_probe(&serdev->dev, status,
+ "failed to notify EC of entry of D0\n");
goto err_initrq;
+ }
status = ssam_ctrl_notif_display_on(ctrl);
- if (status)
+ if (status) {
+ dev_err_probe(&serdev->dev, status,
+ "failed to notify EC of display on\n");
goto err_initrq;
+ }
status = sysfs_create_group(&serdev->dev.kobj, &ssam_sam_group);
if (status)
@@ -697,8 +711,10 @@ static int ssam_serial_hub_probe(struct serdev_device *serdev)
/* Set up IRQ. */
status = ssam_irq_setup(ctrl);
- if (status)
+ if (status) {
+ dev_err_probe(&serdev->dev, status, "failed to setup IRQ\n");
goto err_irq;
+ }
/* Finally, set main controller reference. */
status = ssam_try_set_controller(ctrl);
--
2.44.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] platform/surface: aggregator: Log critical errors during SAM probing
2024-05-03 3:08 ` [PATCH v2 2/2] platform/surface: aggregator: Log critical errors during SAM probing Weifeng Liu
@ 2024-05-03 16:36 ` Andy Shevchenko
0 siblings, 0 replies; 6+ messages in thread
From: Andy Shevchenko @ 2024-05-03 16:36 UTC (permalink / raw
To: Weifeng Liu
Cc: platform-driver-x86, linux-serial, Maximilian Luz, Hans de Goede
On Fri, May 03, 2024 at 11:08:47AM +0800, Weifeng Liu wrote:
> Emits messages upon errors during probing of SAM. Hopefully this could
> provide useful context to user for the purpose of diagnosis when
> something miserable happen.
...
> acpi_status astatus;
> int status;
>
> - if (gpiod_count(&serdev->dev, NULL) < 0)
> - return -ENODEV;
> + status = gpiod_count(&serdev->dev, NULL);
> + if (status < 0)
> + return dev_err_probe(&serdev->dev, status, "no GPIO found\n");
Note, with
struct device *dev = &serdev->dev;
this and other lines become shorter and you may join some of them...
...
> + status = dev_err_probe(&serdev->dev, -ENXIO,
> + "failed to setup serdev\n");
...like here:
status = dev_err_probe(dev, -ENXIO, "failed to setup serdev\n");
...
> + dev_err_probe(&serdev->dev, status,
> + "failed to get firmware version\n");
...or here.
...
With the above being addressed,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] platform/surface: aggregator: Defer probing when serdev is not ready
2024-05-03 3:08 ` [PATCH v2 1/2] platform/surface: aggregator: Defer probing when serdev " Weifeng Liu
@ 2024-05-03 17:03 ` Andy Shevchenko
2024-05-04 4:17 ` Weifeng Liu
0 siblings, 1 reply; 6+ messages in thread
From: Andy Shevchenko @ 2024-05-03 17:03 UTC (permalink / raw
To: Weifeng Liu
Cc: platform-driver-x86, linux-serial, Maximilian Luz, Hans de Goede
On Fri, May 03, 2024 at 11:08:46AM +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.
...
> + /*
> + * The following step can fail when it's called too early before the
> + * underlying UART device is ready (in this case -ENXIO is returned).
> + * Instead of simply giving up and losing everything, we can defer
> + * the probing by returning -EPROBE_DEFER so that the kernel would be
> + * able to retry later.
> + */
You can add the following to the
serial_core.c (at the top after the headers)
#undef ENXIO
#define ENXIO __LINE__
And I'm pretty much sure it will point out you to the uart_port_activate().
If it's the case you may elaborate this in the comment.
Otherwise you may add the same hack to other files and find the culprit.
Also it might be that we add some error code substitution inside serdev core.
At least there more data is available to make the (better) decision.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] platform/surface: aggregator: Defer probing when serdev is not ready
2024-05-03 17:03 ` Andy Shevchenko
@ 2024-05-04 4:17 ` Weifeng Liu
0 siblings, 0 replies; 6+ messages in thread
From: Weifeng Liu @ 2024-05-04 4:17 UTC (permalink / raw
To: Andy Shevchenko
Cc: platform-driver-x86, linux-serial, Maximilian Luz, Hans de Goede
Hi Andy,
On Fri, 2024-05-03 at 20:03 +0300, Andy Shevchenko wrote:
> On Fri, May 03, 2024 at 11:08:46AM +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.
>
> ...
>
> > + /*
> > + * The following step can fail when it's called too early before the
> > + * underlying UART device is ready (in this case -ENXIO is returned).
> > + * Instead of simply giving up and losing everything, we can defer
> > + * the probing by returning -EPROBE_DEFER so that the kernel would be
> > + * able to retry later.
> > + */
>
> You can add the following to the
> serial_core.c (at the top after the headers)
>
> #undef ENXIO
> #define ENXIO __LINE__
>
> And I'm pretty much sure it will point out you to the uart_port_activate().
> If it's the case you may elaborate this in the comment.
> Otherwise you may add the same hack to other files and find the culprit.
Indeed, uart_port_activate() is the function where we gets errno -
ENXIO. Please see the cover letter [1] of this series:
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.
What confuses me is that when ssam_serial_hub_probe() (probe callback
of a serdev driver) gets called and tries to open the provided serdev
device by serdev_device_open(), it simply fails to open it... The
serdev device is not the kind of auxiliary devices but the main device
this driver is to manage. And I don't find other serdev driver doing
this sort of checking and returning -EPROBE_DEFER thing when opening
serdev device. Thus, from the perspective of a newcomer to this area,
I think this phenomenon is somewhat abnormal. Maybe somehow the
initializing of the UART device and probing of serdev driver are
interleaved...
Andy, do you have any idea what's going wrong here? Or do you think
this is an expected behavior?
>
> Also it might be that we add some error code substitution inside serdev core.
> At least there more data is available to make the (better) decision.
>
Agree. If failing in serdev_device_open() is common in a serdev
driver, we'd better handle it properly inside serdev core and convey
explicit semantics to the caller.
Best regards,
Weifeng
[1]
https://lore.kernel.org/linux-serial/20240503030900.1334763-1-weifeng.liu.z@gmail.com/T/#m40d73c44bf92ad45e4fca5ed5f01f9c11e30adb1
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-05-04 4:17 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-05-03 3:08 [PATCH v2 0/2] Defer probing of SAM if serdev device is not ready Weifeng Liu
2024-05-03 3:08 ` [PATCH v2 1/2] platform/surface: aggregator: Defer probing when serdev " Weifeng Liu
2024-05-03 17:03 ` Andy Shevchenko
2024-05-04 4:17 ` Weifeng Liu
2024-05-03 3:08 ` [PATCH v2 2/2] platform/surface: aggregator: Log critical errors during SAM probing Weifeng Liu
2024-05-03 16:36 ` Andy Shevchenko
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.