[PATCH] serdev: Fix detection of UART devices on Apple machines.

[PATCH] serdev: Fix detection of UART devices on Apple machines.

[Ronald Tschalär]

On Apple devices the _CRS method returns an empty resource template, and
the resource settings are instead provided by the _DSM method. But
commit 33364d63c75d6182fa369cea80315cf1bb0ee38e (serdev: Add ACPI
devices by ResourceSource field) changed the search for serdev devices
to require valid, non-empty resource template, thereby breaking Apple
devices and causing bluetooth devices to not be found.

This expands the check so that if we don't find a valid template, and
we're on an Apple machine, then just check for the device being an
immediate child of the controller and having a "baud" property.

Cc: <stable@vger.kernel.org> # 5.5
Signed-off-by: Ronald Tschalär <ronald@innovation.ch>
---
 drivers/tty/serdev/core.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
index ce5309d00280..0f64a10ba51f 100644
--- a/drivers/tty/serdev/core.c
+++ b/drivers/tty/serdev/core.c
@@ -18,6 +18,7 @@
 #include <linux/sched.h>
 #include <linux/serdev.h>
 #include <linux/slab.h>
+#include <linux/platform_data/x86/apple.h>
 
 static bool is_registered;
 static DEFINE_IDA(ctrl_ida);
@@ -630,6 +631,15 @@ static int acpi_serdev_check_resources(struct serdev_controller *ctrl,
 	if (ret)
 		return ret;
 
+	/*
+	 * Apple machines provide an empty resource template, so on those
+	 * machines just look for immediate children with a "baud" property
+	 * (from the _DSM method) instead.
+	 */
+	if (!lookup.controller_handle && x86_apple_machine &&
+	    !acpi_dev_get_property(adev, "baud", ACPI_TYPE_BUFFER, NULL))
+		acpi_get_parent(adev->handle, &lookup.controller_handle);
+
 	/* Make sure controller and ResourceSource handle match */
 	if (ACPI_HANDLE(ctrl->dev.parent) != lookup.controller_handle)
 		return -ENODEV;
-- 
2.24.1

Re: [PATCH] serdev: Fix detection of UART devices on Apple machines.

[Greg Kroah-Hartman]

On Tue, Feb 11, 2020 at 11:47:23AM -0800, Ronald Tschalär wrote:
> On Apple devices the _CRS method returns an empty resource template, and
> the resource settings are instead provided by the _DSM method. But
> commit 33364d63c75d6182fa369cea80315cf1bb0ee38e (serdev: Add ACPI
> devices by ResourceSource field) changed the search for serdev devices
> to require valid, non-empty resource template, thereby breaking Apple
> devices and causing bluetooth devices to not be found.
> 
> This expands the check so that if we don't find a valid template, and
> we're on an Apple machine, then just check for the device being an
> immediate child of the controller and having a "baud" property.
> 
> Cc: <stable@vger.kernel.org> # 5.5
> Signed-off-by: Ronald Tschalär <ronald@innovation.ch>
> ---
>  drivers/tty/serdev/core.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
> index ce5309d00280..0f64a10ba51f 100644
> --- a/drivers/tty/serdev/core.c
> +++ b/drivers/tty/serdev/core.c
> @@ -18,6 +18,7 @@
>  #include <linux/sched.h>
>  #include <linux/serdev.h>
>  #include <linux/slab.h>
> +#include <linux/platform_data/x86/apple.h>

Why is this needed?  Just for the x86_apple_machine variable?

Why do we still have platform_data for new systems anymore?  Can't this
go into a much more generic location?  Like as an inline function?

thanks,

greg k-h

Re: [PATCH] serdev: Fix detection of UART devices on Apple machines.

[Life is hard, and then you die]

On Wed, Feb 19, 2020 at 12:15:19PM +0100, Greg Kroah-Hartman wrote:
> On Tue, Feb 11, 2020 at 11:47:23AM -0800, Ronald Tschalär wrote:
> > On Apple devices the _CRS method returns an empty resource template, and
> > the resource settings are instead provided by the _DSM method. But
> > commit 33364d63c75d6182fa369cea80315cf1bb0ee38e (serdev: Add ACPI
> > devices by ResourceSource field) changed the search for serdev devices
> > to require valid, non-empty resource template, thereby breaking Apple
> > devices and causing bluetooth devices to not be found.
> > 
> > This expands the check so that if we don't find a valid template, and
> > we're on an Apple machine, then just check for the device being an
> > immediate child of the controller and having a "baud" property.
> > 
> > Cc: <stable@vger.kernel.org> # 5.5
> > Signed-off-by: Ronald Tschalär <ronald@innovation.ch>
> > ---
> >  drivers/tty/serdev/core.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
> > index ce5309d00280..0f64a10ba51f 100644
> > --- a/drivers/tty/serdev/core.c
> > +++ b/drivers/tty/serdev/core.c
> > @@ -18,6 +18,7 @@
> >  #include <linux/sched.h>
> >  #include <linux/serdev.h>
> >  #include <linux/slab.h>
> > +#include <linux/platform_data/x86/apple.h>
> 
> Why is this needed?  Just for the x86_apple_machine variable?

Yes.

> Why do we still have platform_data for new systems anymore?  Can't this
> go into a much more generic location?  Like as an inline function?

I'm not sure I follow you. What exactly would you like to see in the
function? The check that sets this variable? Note that this was
originally pulled out into a variable that is set once for performance
reasons - see commit 630b3aff8a51c.


  Cheers,

  Ronald

Re: [PATCH] serdev: Fix detection of UART devices on Apple machines.

[Greg Kroah-Hartman]

On Wed, Feb 19, 2020 at 10:33:35PM -0800, Life is hard, and then you die wrote:
> 
> On Wed, Feb 19, 2020 at 12:15:19PM +0100, Greg Kroah-Hartman wrote:
> > On Tue, Feb 11, 2020 at 11:47:23AM -0800, Ronald Tschalär wrote:
> > > On Apple devices the _CRS method returns an empty resource template, and
> > > the resource settings are instead provided by the _DSM method. But
> > > commit 33364d63c75d6182fa369cea80315cf1bb0ee38e (serdev: Add ACPI
> > > devices by ResourceSource field) changed the search for serdev devices
> > > to require valid, non-empty resource template, thereby breaking Apple
> > > devices and causing bluetooth devices to not be found.
> > > 
> > > This expands the check so that if we don't find a valid template, and
> > > we're on an Apple machine, then just check for the device being an
> > > immediate child of the controller and having a "baud" property.
> > > 
> > > Cc: <stable@vger.kernel.org> # 5.5
> > > Signed-off-by: Ronald Tschalär <ronald@innovation.ch>
> > > ---
> > >  drivers/tty/serdev/core.c | 10 ++++++++++
> > >  1 file changed, 10 insertions(+)
> > > 
> > > diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
> > > index ce5309d00280..0f64a10ba51f 100644
> > > --- a/drivers/tty/serdev/core.c
> > > +++ b/drivers/tty/serdev/core.c
> > > @@ -18,6 +18,7 @@
> > >  #include <linux/sched.h>
> > >  #include <linux/serdev.h>
> > >  #include <linux/slab.h>
> > > +#include <linux/platform_data/x86/apple.h>
> > 
> > Why is this needed?  Just for the x86_apple_machine variable?
> 
> Yes.
> 
> > Why do we still have platform_data for new systems anymore?  Can't this
> > go into a much more generic location?  Like as an inline function?
> 
> I'm not sure I follow you. What exactly would you like to see in the
> function? The check that sets this variable? Note that this was
> originally pulled out into a variable that is set once for performance
> reasons - see commit 630b3aff8a51c.

That's fine, but what I am objecting to is platform-specific include
files being added to random common kernel code.  There's no real reason
for this other than one specific hardware platform has a quirk.  Are we
supposed to keep this pattern up by doing tons of:
	#include <linux/platform_data/x86/vendor_X>
	#include <linux/platform_data/x86/vendor_Y>
	#include <linux/platform_data/x86/vendor_Z>
all through the kernel?

That's a serious regression to the "bad old days" of platform specific
crud being required in each and every driver subsystem.

Now I know it's not your fault this is needed for your one change, but
can you work on a patch series to fix this all up so that it is not
needed?  I'm sure the x86 maintainers don't want to see this spread
around.

Heck, ARM doesn't even need this kind of mess :)

thanks,

greg k-h

Re: [PATCH] serdev: Fix detection of UART devices on Apple machines.

[Lukas Wunner]

On Thu, Feb 20, 2020 at 07:47:23AM +0100, Greg Kroah-Hartman wrote:
> On Wed, Feb 19, 2020 at 10:33:35PM -0800, Ronald Tschalär wrote:
> > On Wed, Feb 19, 2020 at 12:15:19PM +0100, Greg Kroah-Hartman wrote:
> > > On Tue, Feb 11, 2020 at 11:47:23AM -0800, Ronald Tschalär wrote:
> > > > +#include <linux/platform_data/x86/apple.h>
> > > 
> > > Why is this needed?  Just for the x86_apple_machine variable?
> 
> That's fine, but what I am objecting to is platform-specific include
> files being added to random common kernel code.  There's no real reason
> for this other than one specific hardware platform has a quirk.  Are we
> supposed to keep this pattern up by doing tons of:
> 	#include <linux/platform_data/x86/vendor_X>
> 	#include <linux/platform_data/x86/vendor_Y>
> 	#include <linux/platform_data/x86/vendor_Z>
> all through the kernel?
> 
> That's a serious regression to the "bad old days" of platform specific
> crud being required in each and every driver subsystem.
> 
> Now I know it's not your fault this is needed for your one change, but
> can you work on a patch series to fix this all up so that it is not
> needed?  I'm sure the x86 maintainers don't want to see this spread
> around.

Andy (+cc) submitted a patch for the change you're requesting in January:

https://lore.kernel.org/lkml/20200122112306.64598-2-andriy.shevchenko@linux.intel.com/

The x86 maintainers haven't picked it up yet.

Ronald's patch fixes a regression.  Please apply it at your earliest
convenience.

Thanks,

Lukas