[PATCH] surface: aggregator: propagate -EPROBE_DEFER from ssam_irq_setup()

[PATCH] surface: aggregator: propagate -EPROBE_DEFER from ssam_irq_setup()

[Janus]

From: janus <janussss@163.com>

gpiod_get() can fail with -EPROBE_DEFER when the GPIO controller
providing the "ssam_wakeup-int" GPIO has not been probed yet. The
current error path falls back to fwnode_irq_get(), which returns
-EINVAL because this device only declares GPIO resources. The probe
then fails hard and is never retried.

This leaves the SSAM controller down on devices like the Surface Pro 6
when probe ordering is unfavorable: dependent ACPI devices, e.g. the
battery with its _DEP on the SAN node, are never enumerated and battery
status is missing.

Return -EPROBE_DEFER directly so that the probe is deferred until the
GPIO controller becomes available

Signed-off-by: janus <janussss@163.com>
---
 drivers/platform/surface/aggregator/controller.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/platform/surface/aggregator/controller.c b/drivers/platform/surface/aggregator/controller.c
index 6a8430cb9..4f9f4595d 100644
--- a/drivers/platform/surface/aggregator/controller.c
+++ b/drivers/platform/surface/aggregator/controller.c
@@ -2755,6 +2755,8 @@ int ssam_irq_setup(struct ssam_controller *ctrl)
 
 	gpiod = gpiod_get(dev, "ssam_wakeup-int", GPIOD_ASIS);
 	if (IS_ERR(gpiod)) {
+		if (PTR_ERR(gpiod) == -EPROBE_DEFER)
+			return -EPROBE_DEFER;
 		irq = fwnode_irq_get(dev_fwnode(dev), 0);
 	} else {
 		irq = gpiod_to_irq(gpiod);
-- 
2.55.0

Re: [PATCH] surface: aggregator: propagate -EPROBE_DEFER from ssam_irq_setup()

[Ilpo Järvinen]

On Sat, 3 Oct 2026, Janus wrote:

> From: janus <janussss@163.com>
> 
> gpiod_get() can fail with -EPROBE_DEFER when the GPIO controller
> providing the "ssam_wakeup-int" GPIO has not been probed yet. The
> current error path falls back to fwnode_irq_get(), which returns
> -EINVAL because this device only declares GPIO resources. The probe
> then fails hard and is never retried.
> 
> This leaves the SSAM controller down on devices like the Surface Pro 6
> when probe ordering is unfavorable: dependent ACPI devices, e.g. the
> battery with its _DEP on the SAN node, are never enumerated and battery
> status is missing.
> 
> Return -EPROBE_DEFER directly so that the probe is deferred until the
> GPIO controller becomes available

Missing .

> 
> Signed-off-by: janus <janussss@163.com>

Using e.g. just a first name in Signed-off is not enough. Use a name which 
you would use for signing off a paper document.

For more information about Signed-off-by tag, please see:
  Documentation/process/submitting-patches.rst

> ---
>  drivers/platform/surface/aggregator/controller.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/platform/surface/aggregator/controller.c b/drivers/platform/surface/aggregator/controller.c
> index 6a8430cb9..4f9f4595d 100644
> --- a/drivers/platform/surface/aggregator/controller.c
> +++ b/drivers/platform/surface/aggregator/controller.c
> @@ -2755,6 +2755,8 @@ int ssam_irq_setup(struct ssam_controller *ctrl)
>  
>  	gpiod = gpiod_get(dev, "ssam_wakeup-int", GPIOD_ASIS);
>  	if (IS_ERR(gpiod)) {
> +		if (PTR_ERR(gpiod) == -EPROBE_DEFER)
> +			return -EPROBE_DEFER;

This should just pass the same error on with return PTR_ERR(gpiod).

>  		irq = fwnode_irq_get(dev_fwnode(dev), 0);
>  	} else {
>  		irq = gpiod_to_irq(gpiod);
> 

-- 
 i.

Re: [PATCH] surface: aggregator: propagate -EPROBE_DEFER from ssam_irq_setup()

[Chris Simpson]

Hi,

Same failure here on a Surface Pro 9 (Tiger Lake, INTC1055 driven by
pinctrl_tigerlake as a module): whenever surface_aggregator is loaded from
the initramfs about 30 ms after /init, before udev has bound the GPIO
controller, probe ends with

  surface_serial_hub serial0-0: error -EINVAL: failed to setup IRQ

and is never retried, so Type Cover, battery and tablet-mode switch are
gone until a manual rebind. linux-surface has several reports of this
since v6.12 (linux-surface/linux-surface#2219, #1645, #2064, #2039), and
it is a regression from b27622f13172 ("platform/surface: Add OF support"),
which used to propagate the gpiod_get() error.

Janus, are you planning a v2? If not, below is a variant that follows
Ilpo's comment by using gpiod_get_optional(): only "no GPIO described"
falls back to fwnode_irq_get(), every other error including
-EPROBE_DEFER is passed on. I'm happy to send it properly (Fixes:
b27622f13172, Cc: stable, your patch credited), or to test your v2 on
the Pro 9 instead, whichever you prefer.

I've tested it on the Pro 9 (Fedora 43, 7.2.12 with the linux-surface
patches), with surface_aggregator forced into the initramfs. Unpatched,
the probe fails with the -EINVAL above and the Type Cover, battery and
tablet-mode switch are missing. Patched, the probe defers three times
and completes about 0.2 s later, with all of them present and the
device no longer listed in devices_deferred. Suspend/resume behaves as
on an unpatched kernel. The analysis and the diff were produced with an
AI coding assistant (Claude Code), and reviewed and tested by me.

Chris Simpson

--- a/drivers/platform/surface/aggregator/controller.c
+++ b/drivers/platform/surface/aggregator/controller.c
@@ -2753,12 +2753,20 @@
 	 */
 	const int irqf = IRQF_ONESHOT | IRQF_TRIGGER_RISING | IRQF_NO_AUTOEN;
 
-	gpiod = gpiod_get(dev, "ssam_wakeup-int", GPIOD_ASIS);
-	if (IS_ERR(gpiod)) {
-		irq = fwnode_irq_get(dev_fwnode(dev), 0);
-	} else {
+	/*
+	 * Only fall back to the fwnode IRQ if no wakeup GPIO is described
+	 * at all. Any other error, in particular -EPROBE_DEFER while the
+	 * GPIO controller has not been registered yet, must reach the probe.
+	 */
+	gpiod = gpiod_get_optional(dev, "ssam_wakeup-int", GPIOD_ASIS);
+	if (IS_ERR(gpiod))
+		return PTR_ERR(gpiod);
+
+	if (gpiod) {
 		irq = gpiod_to_irq(gpiod);
 		gpiod_put(gpiod);
+	} else {
+		irq = fwnode_irq_get(dev_fwnode(dev), 0);
 	}
 
 	if (irq < 0)

[PATCH v2] surface: aggregator: propagate -EPROBE_DEFER from ssam_irq_setup()

[Wu Enqiang]

gpiod_get() can fail with -EPROBE_DEFER when the GPIO controller
providing the "ssam_wakeup-int" GPIO has not been probed yet. The
current error path falls back to fwnode_irq_get(), which returns
-EINVAL because this device only declares GPIO resources. The probe
then fails hard and is never retried.

This has been reported across various Surface devices since v6.12,
including the Pro 6 and Pro 9 that were used to reproduce it. With
the SSAM controller down, dependent ACPI devices, e.g. the battery
with its _DEP on the SAN node, are never enumerated and battery
status is missing.

Use gpiod_get_optional() and propagate errors to the caller so that
probing is deferred until the GPIO controller becomes available. Only
when no wakeup GPIO is described at all, fall back to fwnode_irq_get().

Fixes: b27622f13172 ("platform/surface: Add OF support")
Reported-by: Chris Simpson <chris@nilk.net>
Link: https://lore.kernel.org/platform-driver-x86/d0bb21cc-6d3c-4526-8204-2511da51a6ac@nilk.net
Link: https://github.com/linux-surface/linux-surface/issues/2219
Tested-by: Chris Simpson <chris@nilk.net>
Tested-by: Wu Enqiang <janussss@163.com>
Cc: stable@vger.kernel.org
Signed-off-by: Wu Enqiang <janussss@163.com>
---
v2:
 * Per Ilpo's comment, pass any gpiod_get_optional() error on directly
   instead of special-casing -EPROBE_DEFER; keep the fwnode_irq_get()
   fallback only for the "no GPIO described" (NULL) case. Suggested by
   Ilpo Järvinen and matching the variant Chris Simpson tested on a
   Surface Pro 9.
 * Add missing final period (Ilpo Järvinen).
 * Use full name in Signed-off-by (Ilpo Järvinen).
 * Add Reported-by/Tested-by for Chris Simpson, and Fixes:/Cc: stable
   for the v6.12 regression he identified.
 * Note the reports span various Surface devices (linux-surface
   #2219, #1645, #2064, #2039); the Pro 6/Pro 9 are just the ones
   used to reproduce.
 * Tested on a Surface Pro 6: probe no longer fails with the -EINVAL
   "failed to setup IRQ" in dmesg, and battery plus dependent ACPI
   devices are enumerated again.
---
 drivers/platform/surface/aggregator/controller.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/surface/aggregator/controller.c b/drivers/platform/surface/aggregator/controller.c
index 6a8430cb9..8846d22c6 100644
--- a/drivers/platform/surface/aggregator/controller.c
+++ b/drivers/platform/surface/aggregator/controller.c
@@ -2753,12 +2753,20 @@ int ssam_irq_setup(struct ssam_controller *ctrl)
 	 */
 	const int irqf = IRQF_ONESHOT | IRQF_TRIGGER_RISING | IRQF_NO_AUTOEN;
 
-	gpiod = gpiod_get(dev, "ssam_wakeup-int", GPIOD_ASIS);
-	if (IS_ERR(gpiod)) {
-		irq = fwnode_irq_get(dev_fwnode(dev), 0);
-	} else {
+	/*
+	 * Only fall back to the fwnode IRQ if no wakeup GPIO is described
+	 * at all. Any other error, in particular -EPROBE_DEFER while the
+	 * GPIO controller has not been registered yet, must reach the probe.
+	 */
+	gpiod = gpiod_get_optional(dev, "ssam_wakeup-int", GPIOD_ASIS);
+	if (IS_ERR(gpiod))
+		return PTR_ERR(gpiod);
+
+	if (gpiod) {
 		irq = gpiod_to_irq(gpiod);
 		gpiod_put(gpiod);
+	} else {
+		irq = fwnode_irq_get(dev_fwnode(dev), 0);
 	}
 
 	if (irq < 0)
-- 
2.55.0

Re: [PATCH] surface: aggregator: propagate -EPROBE_DEFER from ssam_irq_setup()

[Ilpo Järvinen]

+ Konrad, Hans, Andy.

On Fri, 23 Oct 2026, Chris Simpson wrote:

> Hi,
> 
> Same failure here on a Surface Pro 9 (Tiger Lake, INTC1055 driven by
> pinctrl_tigerlake as a module): whenever surface_aggregator is loaded from
> the initramfs about 30 ms after /init, before udev has bound the GPIO
> controller, probe ends with
> 
>   surface_serial_hub serial0-0: error -EINVAL: failed to setup IRQ
> 
> and is never retried, so Type Cover, battery and tablet-mode switch are
> gone until a manual rebind. linux-surface has several reports of this
> since v6.12 (linux-surface/linux-surface#2219, #1645, #2064, #2039), and
> it is a regression from b27622f13172 ("platform/surface: Add OF support"),
> which used to propagate the gpiod_get() error.
> 
> Janus, are you planning a v2? If not, below is a variant that follows
> Ilpo's comment by using gpiod_get_optional(): only "no GPIO described"
> falls back to fwnode_irq_get(), every other error including
> -EPROBE_DEFER is passed on. I'm happy to send it properly (Fixes:
> b27622f13172, Cc: stable, your patch credited), or to test your v2 on
> the Pro 9 instead, whichever you prefer.
> 
> I've tested it on the Pro 9 (Fedora 43, 7.2.12 with the linux-surface
> patches), with surface_aggregator forced into the initramfs. Unpatched,
> the probe fails with the -EINVAL above and the Type Cover, battery and
> tablet-mode switch are missing. Patched, the probe defers three times
> and completes about 0.2 s later, with all of them present and the
> device no longer listed in devices_deferred. Suspend/resume behaves as
> on an unpatched kernel. The analysis and the diff were produced with an
> AI coding assistant (Claude Code), and reviewed and tested by me.
> 
> Chris Simpson
> 
> --- a/drivers/platform/surface/aggregator/controller.c
> +++ b/drivers/platform/surface/aggregator/controller.c
> @@ -2753,12 +2753,20 @@
>  	 */
>  	const int irqf = IRQF_ONESHOT | IRQF_TRIGGER_RISING | IRQF_NO_AUTOEN;
>  
> -	gpiod = gpiod_get(dev, "ssam_wakeup-int", GPIOD_ASIS);
> -	if (IS_ERR(gpiod)) {
> -		irq = fwnode_irq_get(dev_fwnode(dev), 0);
> -	} else {
> +	/*
> +	 * Only fall back to the fwnode IRQ if no wakeup GPIO is described
> +	 * at all. Any other error, in particular -EPROBE_DEFER while the
> +	 * GPIO controller has not been registered yet, must reach the probe.
> +	 */
> +	gpiod = gpiod_get_optional(dev, "ssam_wakeup-int", GPIOD_ASIS);
> +	if (IS_ERR(gpiod))
> +		return PTR_ERR(gpiod);
> +
> +	if (gpiod) {
>  		irq = gpiod_to_irq(gpiod);
>  		gpiod_put(gpiod);
> +	} else {
> +		irq = fwnode_irq_get(dev_fwnode(dev), 0);

Hi,

I have to admit this was not exactly what I meant (I only meant to not 
use -Exx literal in the return statement), but now that I see this v2, 
I'm not saying your approach is wrong either (I'm left unsure).

Are there some legimate cases where error other than -EDEFER_PROBE should 
end up into this else like it previously did? The return on error was 
removed by b27622f13172 ("platform/surface: Add OF support") so I assume 
there was some good reason for that but the commit message isn't helpful 
at all... :-(

Konrad, you did that change, can you explain the reason why the return was 
removed from the error path?

-- 
 i.

Re: [PATCH] surface: aggregator: propagate -EPROBE_DEFER from ssam_irq_setup()

[Andy Shevchenko]

On Mon, Oct 26, 2026 at 12:42:01PM +0200, Ilpo Järvinen wrote:
> On Fri, 23 Oct 2026, Chris Simpson wrote:

> > Same failure here on a Surface Pro 9 (Tiger Lake, INTC1055 driven by
> > pinctrl_tigerlake as a module): whenever surface_aggregator is loaded from
> > the initramfs about 30 ms after /init, before udev has bound the GPIO
> > controller, probe ends with
> > 
> >   surface_serial_hub serial0-0: error -EINVAL: failed to setup IRQ
> > 
> > and is never retried, so Type Cover, battery and tablet-mode switch are
> > gone until a manual rebind. linux-surface has several reports of this
> > since v6.12 (linux-surface/linux-surface#2219, #1645, #2064, #2039), and
> > it is a regression from b27622f13172 ("platform/surface: Add OF support"),
> > which used to propagate the gpiod_get() error.
> > 
> > Janus, are you planning a v2? If not, below is a variant that follows
> > Ilpo's comment by using gpiod_get_optional(): only "no GPIO described"
> > falls back to fwnode_irq_get(), every other error including
> > -EPROBE_DEFER is passed on. I'm happy to send it properly (Fixes:
> > b27622f13172, Cc: stable, your patch credited), or to test your v2 on
> > the Pro 9 instead, whichever you prefer.
> > 
> > I've tested it on the Pro 9 (Fedora 43, 7.2.12 with the linux-surface
> > patches), with surface_aggregator forced into the initramfs. Unpatched,
> > the probe fails with the -EINVAL above and the Type Cover, battery and
> > tablet-mode switch are missing. Patched, the probe defers three times
> > and completes about 0.2 s later, with all of them present and the
> > device no longer listed in devices_deferred. Suspend/resume behaves as
> > on an unpatched kernel. The analysis and the diff were produced with an
> > AI coding assistant (Claude Code), and reviewed and tested by me.
> > 
> > Chris Simpson
> > 
> > --- a/drivers/platform/surface/aggregator/controller.c
> > +++ b/drivers/platform/surface/aggregator/controller.c
> > @@ -2753,12 +2753,20 @@
> >  	 */
> >  	const int irqf = IRQF_ONESHOT | IRQF_TRIGGER_RISING | IRQF_NO_AUTOEN;
> >  
> > -	gpiod = gpiod_get(dev, "ssam_wakeup-int", GPIOD_ASIS);
> > -	if (IS_ERR(gpiod)) {
> > -		irq = fwnode_irq_get(dev_fwnode(dev), 0);
> > -	} else {
> > +	/*
> > +	 * Only fall back to the fwnode IRQ if no wakeup GPIO is described
> > +	 * at all. Any other error, in particular -EPROBE_DEFER while the
> > +	 * GPIO controller has not been registered yet, must reach the probe.
> > +	 */
> > +	gpiod = gpiod_get_optional(dev, "ssam_wakeup-int", GPIOD_ASIS);
> > +	if (IS_ERR(gpiod))
> > +		return PTR_ERR(gpiod);
> > +
> > +	if (gpiod) {
> >  		irq = gpiod_to_irq(gpiod);
> >  		gpiod_put(gpiod);
> > +	} else {
> > +		irq = fwnode_irq_get(dev_fwnode(dev), 0);
> 
> I have to admit this was not exactly what I meant (I only meant to not 
> use -Exx literal in the return statement), but now that I see this v2, 
> I'm not saying your approach is wrong either (I'm left unsure).
> 
> Are there some legimate cases where error other than -EDEFER_PROBE should 
> end up into this else like it previously did? The return on error was 
> removed by b27622f13172 ("platform/surface: Add OF support") so I assume 
> there was some good reason for that but the commit message isn't helpful 
> at all... :-(
> 
> Konrad, you did that change, can you explain the reason why the return was 
> removed from the error path?

Looking from the code perspective I agree that the patch above seems relevant
and correct, we need to propagate all error codes in case GPIO is present,
but can't be retrieved for some reason. I would implement it a bit differently,
but it's another story.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] surface: aggregator: propagate -EPROBE_DEFER from ssam_irq_setup()

[Chris Simpson]

Hi Ilpo,

> Are there some legimate cases where error other than -EDEFER_PROBE should
> end up into this else like it previously did?

Two corrections to my earlier mail first. The Surface Pro 9 here is
Alder Lake (i7-1255U), not Tiger Lake; its GPIO controller, INTC1055,
is handled by pinctrl-tigerlake, which is where my mix-up came from.
And "follows Ilpo's comment" was my misreading of your review: the
gpiod_get_optional() idea was mine, sorry for attributing it to you
(v2's "Suggested by Ilpo" comes from that too).

On the question, I couldn't find such a case:

- DT: microsoft,surface-sam.yaml requires "interrupts" and has
  additionalProperties: false, and the in-tree DTS files
  (sc8280xp-microsoft-arcata, x1e80100-microsoft-romulus,
  x1-microsoft-denali, x1p42100-microsoft-sp12in) all use
  interrupts-extended. So on DT gpiod_get() returns -ENOENT, which
  gpiod_get_optional() turns into NULL, and v2 still takes the
  fwnode_irq_get() path.

- ACPI: fwnode_irq_get() ends up in acpi_irq_get(), which only looks at
  Interrupt()/IRQ() resources, not GpioInt(). On the Pro 9 (UEFI
  22.107.143) the MSHW0084 _CRS has UartSerialBusV2 plus GpioInt and
  GpioIo on pin 174 of INTC1055, and no Interrupt() or IRQ(), and the
  Surface Laptop 4 tables in linux-surface#2219 have the same layout.
  So on these two machines the fallback can only turn -EPROBE_DEFER, or
  a real GPIO error, into -EINVAL; for -EPROBE_DEFER that is the probe
  failure v2 fixes.

What I can't tell is whether Konrad had another case in mind, or whether
some other Surface model has a plain Interrupt() there; I've only looked
at these two. I'm happy to test any variant on the Pro 9. As before, I
did the analysis with Claude Code.

Chris Simpson

Re: [PATCH] surface: aggregator: propagate -EPROBE_DEFER from ssam_irq_setup()

[Ilpo Järvinen]

On Mon, 26 Oct 2026, Chris Simpson wrote:

> Hi Ilpo,
> 
> > Are there some legimate cases where error other than -EDEFER_PROBE should
> > end up into this else like it previously did?
> 
> Two corrections to my earlier mail first. The Surface Pro 9 here is
> Alder Lake (i7-1255U), not Tiger Lake; its GPIO controller, INTC1055,
> is handled by pinctrl-tigerlake, which is where my mix-up came from.
> And "follows Ilpo's comment" was my misreading of your review: the
> gpiod_get_optional() idea was mine, sorry for attributing it to you
> (v2's "Suggested by Ilpo" comes from that too).

Np. The end result seems better even if there was misread which triggered 
that. At least sometimes misreading ends up being useful. :-)

--
 i.

> On the question, I couldn't find such a case:
> 
> - DT: microsoft,surface-sam.yaml requires "interrupts" and has
>   additionalProperties: false, and the in-tree DTS files
>   (sc8280xp-microsoft-arcata, x1e80100-microsoft-romulus,
>   x1-microsoft-denali, x1p42100-microsoft-sp12in) all use
>   interrupts-extended. So on DT gpiod_get() returns -ENOENT, which
>   gpiod_get_optional() turns into NULL, and v2 still takes the
>   fwnode_irq_get() path.
> 
> - ACPI: fwnode_irq_get() ends up in acpi_irq_get(), which only looks at
>   Interrupt()/IRQ() resources, not GpioInt(). On the Pro 9 (UEFI
>   22.107.143) the MSHW0084 _CRS has UartSerialBusV2 plus GpioInt and
>   GpioIo on pin 174 of INTC1055, and no Interrupt() or IRQ(), and the
>   Surface Laptop 4 tables in linux-surface#2219 have the same layout.
>   So on these two machines the fallback can only turn -EPROBE_DEFER, or
>   a real GPIO error, into -EINVAL; for -EPROBE_DEFER that is the probe
>   failure v2 fixes.
> 
> What I can't tell is whether Konrad had another case in mind, or whether
> some other Surface model has a plain Interrupt() there; I've only looked
> at these two. I'm happy to test any variant on the Pro 9. As before, I
> did the analysis with Claude Code.
> 
> Chris Simpson
>