BT / serial regression introduced during the recent 4.9-rc1 merge?

BT / serial regression introduced during the recent 4.9-rc1 merge?

[Jérôme de Bretagne]

[-- Attachment #1: Type: text/plain, Size: 2302 bytes --]

Hi Marcel, hi everyone,

I've compiled the latest bluetooth-next branch and I'm facing what looks
like a regression to me (still on a Lenovo ThinkPad 8 tablet): btattach
doesn't properly initialize the Broadcom BCM2E55 chipset anymore. 

I'm getting various timeout messages in dmesg:

[   13.188057] Bluetooth: hci0 command 0xfc45 tx timeout
[   16.093068] serial8250_interrupt: 4158 callbacks suppressed
[   16.093072] serial8250: too much work for irq39
[   16.094287] serial8250: too much work for irq39
...
[   16.103868] serial8250: too much work for irq39
[   21.100041] serial8250_interrupt: 4167 callbacks suppressed
[   21.100044] serial8250: too much work for irq39
...
[   21.222065] Bluetooth: hci0: BCM: failed to write clock (-110)
[   23.238528] Bluetooth: hci0 command 0x0c03 tx timeout
[   26.104253] serial8250_interrupt: 4165 callbacks suppressed
[   26.104257] serial8250: too much work for irq39

which I never had before and Bluetooth never actually starts.

Bluetooth doesn't init with a kernel built with the latest commit from
yesterday 526c86021e5102b8a4b5555b4196f7a19f44e2c4. 

I've gone back in time and it doesn't work either with a kernel built at 	
e6445f52d9c8b0e6557a45fa7d0e8e088d430a8c "Merge tag 'usb-4.9-rc1' of
git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb".

It still worked at commit a2f195a73eba807006fb0cb882ecb552c06eea00
"bluetooth: bcm203x: don't print error when allocating urb fails" though,
which was the last previous commit modifying files in drivers/bluetooth in
the bluetooth-next branch.


I've attached the output of btmon when it used to work and one not working
(prefixed with .e6445f5) if that may be useful.


I'll continue my investigation in my spare time, trying to bissect to find
the culprit commit but there are hundreds of them so it will take me quite a
while. I'll focus on patches touching the serial 8250 driver to start with,
as there are only a few of them, but feel free to point me into a different
direction if you have another idea or suggestion. 

I wanted to share this early with the Bluetooth list as I didn't see the
issue mentioned until now. Feel free to forward this message to other lists
as it's likely coming from another kernel subsystem.   

Thanks,

Jérôme

[-- Attachment #2: btmon.thinkpad8-init.btsnoop --]
[-- Type: application/octet-stream, Size: 53920 bytes --]

[-- Attachment #3: btmon.thinkpad8-init.btsnoop.e6445f5 --]
[-- Type: application/octet-stream, Size: 444 bytes --]

Re: BT / serial regression introduced during the recent 4.9-rc1 merge?

[Jérôme de Bretagne]

Hi,

> I've compiled the latest bluetooth-next branch and I'm facing what looks
> like a regression to me (still on a Lenovo ThinkPad 8 tablet): btattach
> doesn't properly initialize the Broadcom BCM2E55 chipset anymore. 
> 
> I'm getting various timeout messages in dmesg:
> 
> [   13.188057] Bluetooth: hci0 command 0xfc45 tx timeout
> [   16.093068] serial8250_interrupt: 4158 callbacks suppressed
> [   16.093072] serial8250: too much work for irq39
> [   16.094287] serial8250: too much work for irq39
> ...
> [   16.103868] serial8250: too much work for irq39
> [   21.100041] serial8250_interrupt: 4167 callbacks suppressed
> [   21.100044] serial8250: too much work for irq39
> ...
> [   21.222065] Bluetooth: hci0: BCM: failed to write clock (-110)
> [   23.238528] Bluetooth: hci0 command 0x0c03 tx timeout
> [   26.104253] serial8250_interrupt: 4165 callbacks suppressed
> [   26.104257] serial8250: too much work for irq39
> 
> which I never had before and Bluetooth never actually starts.
> 
> I've gone back in time and it doesn't work either with a kernel built at
> e6445f52d9c8b0e6557a45fa7d0e8e088d430a8c "Merge tag 'usb-4.9-rc1' of
> git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb".
> 
> It still worked at commit a2f195a73eba807006fb0cb882ecb552c06eea00
> "bluetooth: bcm203x: don't print error when allocating urb fails" though,
> which was the last previous commit modifying files in drivers/bluetooth in
> the bluetooth-next branch.

Continuing to bissect, the regression seems to have appeared during commit:
   e6dce825fba05f447bd22c865e27233182ab3d79    Merge tag 'tty-4.9-rc1' of
   git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty

   Pull tty and serial updates from Greg KH:
     "Here is the big tty and serial patch set for 4.9-rc1. [...]"

just to let you know the state of my investigation. Now, I'll switch to
bissect the 111 commits introduced within that merge. To be continued.

Regards,

Jérôme

Re: [PATCHv2 3/3] serial: 8250_dw: Add quirk for APM X-Gene SoC (was: BT / serial regression introduced during the recent 4.9-rc1 merge?)

[Jérôme de Bretagne]

Hi Heikki, Andy, Greg, Rafael,

I've detected a regression on my Lenovo ThinkPad 8 tablet introduced during
4.9-rc1, preventing the built-in Bluetooth Broadcom chipset from properly
initializing, with many timeout messages in dmesg like these:
   "serial8250_interrupt: WXYZ callbacks suppressed"
   "serial8250: too much work for irq39"
cf. my earlier report on linux-bluetooth at the very end for reference.


After a long git bisect, I've found the commit that's triggering the issue:

commit 20a875e2e86e73d13ec256781a7d55a7885868ec
Author: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
Date:   Tue Aug 23 11:33:28 2016 +0300

    serial: 8250_dw: Add quirk for APM X-Gene SoC
    
    The APM X-Gene SoC UART is the only board that still needs
    the hard-coded values, so handle it separately in
    dw8250_quirks(). The other ACPI platforms are able to
    provide the values with device properties.
    
    Signed-off-by: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
    Reviewed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
    Acked-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
    Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>

diff --git a/drivers/tty/serial/8250/8250_dw.c
b/drivers/tty/serial/8250/8250_dw.c
index e199696..5c0c123 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -298,12 +298,17 @@ static void dw8250_quirks(struct uart_port *p, struct
dw8250_data *data)
                        p->serial_out = dw8250_serial_out32be;
                }
        } else if (has_acpi_companion(p->dev)) {
-               p->iotype = UPIO_MEM32;
-               p->regshift = 2;
-               p->serial_in = dw8250_serial_in32;
+               const struct acpi_device_id *id;
+
+               id = acpi_match_device(p->dev->driver->acpi_match_table,
+                                      p->dev);
+               if (id && !strcmp(id->id, "APMC0D08")) {
+                       p->iotype = UPIO_MEM32;
+                       p->regshift = 2;
+                       p->serial_in = dw8250_serial_in32;
+                       data->uart_16550_compatible = true;
+               }
                p->set_termios = dw8250_set_termios;
-               /* So far none of there implement the Busy Functionality */
-               data->uart_16550_compatible = true;
        }
 
        /* Platforms with iDMA */


This is fully reproducible on my end: removing this specific patch gets rid
of the issue; with the patch applied, Bluetooth simply doesn't work anymore.

As opposed to the commit description, it seems that the Lenovo ThinkPad 8
still needs the original hard-coded values currently (even if it could be
possible to provide them in another way in the future, if I interpret the
commit message the right way).

Could this patch be reverted for the moment to remove the regression, until
a proper fix is found?

Thanks,
Jérôme


> I've compiled the latest bluetooth-next branch and I'm facing what looks
> like a regression to me (still on a Lenovo ThinkPad 8 tablet): btattach
> doesn't properly initialize the Broadcom BCM2E55 chipset anymore. 
> 
> I'm getting various timeout messages in dmesg:
> 
> [   13.188057] Bluetooth: hci0 command 0xfc45 tx timeout
> [   16.093068] serial8250_interrupt: 4158 callbacks suppressed
> [   16.093072] serial8250: too much work for irq39
> [   16.094287] serial8250: too much work for irq39
> ...
> [   16.103868] serial8250: too much work for irq39
> [   21.100041] serial8250_interrupt: 4167 callbacks suppressed
> [   21.100044] serial8250: too much work for irq39
> ...
> [   21.222065] Bluetooth: hci0: BCM: failed to write clock (-110)
> [   23.238528] Bluetooth: hci0 command 0x0c03 tx timeout
> [   26.104253] serial8250_interrupt: 4165 callbacks suppressed
> [   26.104257] serial8250: too much work for irq39
> 
> which I never had before and Bluetooth never actually starts.
> 
> Bluetooth doesn't init with a kernel built with the latest commit from
> yesterday 526c86021e5102b8a4b5555b4196f7a19f44e2c4. 
> 
> I've gone back in time and it doesn't work either with a kernel built at 	
> e6445f52d9c8b0e6557a45fa7d0e8e088d430a8c "Merge tag 'usb-4.9-rc1' of
> git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb".
> 
> It still worked at commit a2f195a73eba807006fb0cb882ecb552c06eea00
> "bluetooth: bcm203x: don't print error when allocating urb fails" though,
> which was the last previous commit modifying files in drivers/bluetooth in
> the bluetooth-next branch.
> 
> I've attached the output of btmon when it used to work and one not working
> (prefixed with .e6445f5) if that may be useful.
> 
> I'll continue my investigation in my spare time, trying to bissect to find
> a while. I'll focus on patches touching the serial 8250 driver to start
> with, as there are only a few of them, but feel free to point me into a
> different direction if you have another idea or suggestion.

Re: [PATCHv2 3/3] serial: 8250_dw: Add quirk for APM X-Gene SoC (was: BT / serial regression introduced during the recent 4.9-rc1 merge?)

[Rafael J. Wysocki]

On Tue, Nov 1, 2016 at 5:58 PM, J=C3=A9r=C3=B4me de Bretagne
<jerome.debretagne@gmail.com> wrote:
> Hi Heikki, Andy, Greg, Rafael,
>
> I've detected a regression on my Lenovo ThinkPad 8 tablet introduced duri=
ng
> 4.9-rc1, preventing the built-in Bluetooth Broadcom chipset from properly
> initializing, with many timeout messages in dmesg like these:
>    "serial8250_interrupt: WXYZ callbacks suppressed"
>    "serial8250: too much work for irq39"
> cf. my earlier report on linux-bluetooth at the very end for reference.
>
>
> After a long git bisect, I've found the commit that's triggering the issu=
e:
>
> commit 20a875e2e86e73d13ec256781a7d55a7885868ec
> Author: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> Date:   Tue Aug 23 11:33:28 2016 +0300
>
>     serial: 8250_dw: Add quirk for APM X-Gene SoC
>
>     The APM X-Gene SoC UART is the only board that still needs
>     the hard-coded values, so handle it separately in
>     dw8250_quirks(). The other ACPI platforms are able to
>     provide the values with device properties.
>
>     Signed-off-by: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
>     Reviewed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
>     Acked-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
>     Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
>
> diff --git a/drivers/tty/serial/8250/8250_dw.c
> b/drivers/tty/serial/8250/8250_dw.c
> index e199696..5c0c123 100644
> --- a/drivers/tty/serial/8250/8250_dw.c
> +++ b/drivers/tty/serial/8250/8250_dw.c
> @@ -298,12 +298,17 @@ static void dw8250_quirks(struct uart_port *p, stru=
ct
> dw8250_data *data)
>                         p->serial_out =3D dw8250_serial_out32be;
>                 }
>         } else if (has_acpi_companion(p->dev)) {
> -               p->iotype =3D UPIO_MEM32;
> -               p->regshift =3D 2;
> -               p->serial_in =3D dw8250_serial_in32;
> +               const struct acpi_device_id *id;
> +
> +               id =3D acpi_match_device(p->dev->driver->acpi_match_table=
,
> +                                      p->dev);
> +               if (id && !strcmp(id->id, "APMC0D08")) {
> +                       p->iotype =3D UPIO_MEM32;
> +                       p->regshift =3D 2;
> +                       p->serial_in =3D dw8250_serial_in32;
> +                       data->uart_16550_compatible =3D true;
> +               }
>                 p->set_termios =3D dw8250_set_termios;
> -               /* So far none of there implement the Busy Functionality =
*/
> -               data->uart_16550_compatible =3D true;
>         }
>
>         /* Platforms with iDMA */
>
>
> This is fully reproducible on my end: removing this specific patch gets r=
id
> of the issue; with the patch applied, Bluetooth simply doesn't work anymo=
re.
>
> As opposed to the commit description, it seems that the Lenovo ThinkPad 8
> still needs the original hard-coded values currently (even if it could be
> possible to provide them in another way in the future, if I interpret the
> commit message the right way).
>
> Could this patch be reverted for the moment to remove the regression, unt=
il
> a proper fix is found?

Heikki, I don't think anything depends on this commit, is that correct?

Thanks,
Rafael

Re: [PATCHv2 3/3] serial: 8250_dw: Add quirk for APM X-Gene SoC (was: BT / serial regression introduced during the recent 4.9-rc1 merge?)

[Heikki Krogerus]

Hi,

On Wed, Nov 02, 2016 at 04:49:42AM +0100, Rafael J. Wysocki wrote:
> On Tue, Nov 1, 2016 at 5:58 PM, Jérôme de Bretagne
> <jerome.debretagne@gmail.com> wrote:
> > Hi Heikki, Andy, Greg, Rafael,
> >
> > I've detected a regression on my Lenovo ThinkPad 8 tablet introduced during
> > 4.9-rc1, preventing the built-in Bluetooth Broadcom chipset from properly
> > initializing, with many timeout messages in dmesg like these:
> >    "serial8250_interrupt: WXYZ callbacks suppressed"
> >    "serial8250: too much work for irq39"
> > cf. my earlier report on linux-bluetooth at the very end for reference.

Jérôme! Could you send us acpidump output and your kernel
configuration.

> > After a long git bisect, I've found the commit that's triggering the issue:
> >
> > commit 20a875e2e86e73d13ec256781a7d55a7885868ec
> > Author: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> > Date:   Tue Aug 23 11:33:28 2016 +0300
> >
> >     serial: 8250_dw: Add quirk for APM X-Gene SoC
> >
> >     The APM X-Gene SoC UART is the only board that still needs
> >     the hard-coded values, so handle it separately in
> >     dw8250_quirks(). The other ACPI platforms are able to
> >     provide the values with device properties.
> >
> >     Signed-off-by: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> >     Reviewed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> >     Acked-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> >     Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> >
> > diff --git a/drivers/tty/serial/8250/8250_dw.c
> > b/drivers/tty/serial/8250/8250_dw.c
> > index e199696..5c0c123 100644
> > --- a/drivers/tty/serial/8250/8250_dw.c
> > +++ b/drivers/tty/serial/8250/8250_dw.c
> > @@ -298,12 +298,17 @@ static void dw8250_quirks(struct uart_port *p, struct
> > dw8250_data *data)
> >                         p->serial_out = dw8250_serial_out32be;
> >                 }
> >         } else if (has_acpi_companion(p->dev)) {
> > -               p->iotype = UPIO_MEM32;
> > -               p->regshift = 2;
> > -               p->serial_in = dw8250_serial_in32;
> > +               const struct acpi_device_id *id;
> > +
> > +               id = acpi_match_device(p->dev->driver->acpi_match_table,
> > +                                      p->dev);
> > +               if (id && !strcmp(id->id, "APMC0D08")) {
> > +                       p->iotype = UPIO_MEM32;
> > +                       p->regshift = 2;
> > +                       p->serial_in = dw8250_serial_in32;
> > +                       data->uart_16550_compatible = true;
> > +               }
> >                 p->set_termios = dw8250_set_termios;
> > -               /* So far none of there implement the Busy Functionality */
> > -               data->uart_16550_compatible = true;
> >         }
> >
> >         /* Platforms with iDMA */
> >
> >
> > This is fully reproducible on my end: removing this specific patch gets rid
> > of the issue; with the patch applied, Bluetooth simply doesn't work anymore.
> >
> > As opposed to the commit description, it seems that the Lenovo ThinkPad 8
> > still needs the original hard-coded values currently (even if it could be
> > possible to provide them in another way in the future, if I interpret the
> > commit message the right way).
> >
> > Could this patch be reverted for the moment to remove the regression, until
> > a proper fix is found?
> 
> Heikki, I don't think anything depends on this commit, is that correct?

Unfortunately we can't fix the values for every UART that has ACPI
companion like we did before anymore.

There is at least one new platform, that the driver supports, which
does not have the Designware UART in this "16550 compatible" mode, and
I guess it's possible that there are others as well. And I guess the
other values can also be what ever on those platforms.

Jérôme, can you test if using the quirk on Baytrails works:

@@ -302,7 +302,8 @@ static void dw8250_quirks(struct uart_port *p, struct dw8250_data *data)
 
                id = acpi_match_device(p->dev->driver->acpi_match_table,
                                       p->dev);
-               if (id && !strcmp(id->id, "APMC0D08")) {
+               if (id && (!strcmp(id->id, "APMC0D08") ||
+                   !strcmp(id->id, "80860F0A"))) {
                        p->iotype = UPIO_MEM32;
                        p->regshift = 2;
                        p->serial_in = dw8250_serial_in32;

Please note that this really is not a fix, not even a temporary one
for this issue. There are a lot of Baytrails on the market. I just
want to make sure there really is a problem delivering those values as
device properties with your board.


Thanks,

-- 
heikki

Re: [PATCHv2 3/3] serial: 8250_dw: Add quirk for APM X-Gene SoC (was: BT / serial regression introduced during the recent 4.9-rc1 merge?)

[Jérôme de Bretagne]

Hi again,

> > Heikki, I don't think anything depends on this commit, is that correct?
> 
> Unfortunately we can't fix the values for every UART that has ACPI
> companion like we did before anymore.
> 
> There is at least one new platform, that the driver supports, which
> does not have the Designware UART in this "16550 compatible" mode, and
> I guess it's possible that there are others as well. And I guess the
> other values can also be what ever on those platforms.
> 
> Jérôme, can you test if using the quirk on Baytrails works:
> 
> @@ -302,7 +302,8 @@ static void dw8250_quirks(struct uart_port *p, struct
> dw8250_data *data)
>  
>                 id = acpi_match_device(p->dev->driver->acpi_match_table,
>                                        p->dev);
> -               if (id && !strcmp(id->id, "APMC0D08")) {
> +               if (id && (!strcmp(id->id, "APMC0D08") ||
> +                   !strcmp(id->id, "80860F0A"))) {
>                         p->iotype = UPIO_MEM32;
>                         p->regshift = 2;
>                         p->serial_in = dw8250_serial_in32;

Compilation finished with that small modification applied: Bluetooth works
indeed in that case and I don't have the error messages in dmesg anymore.

> 
> Please note that this really is not a fix, not even a temporary one
> for this issue. There are a lot of Baytrails on the market. I just
> want to make sure there really is a problem delivering those values as
> device properties with your board.

I guess this confirms there is indeed a problem delivering those values as
devices properties on that specific Baytrail device at least.

Let me know if there is anything else useful I could share/test.

Thanks,
Jérôme

Re: [PATCHv2 3/3] serial: 8250_dw: Add quirk for APM X-Gene SoC (was: BT / serial regression introduced during the recent 4.9-rc1 merge?)

[Heikki Krogerus]

On Wed, Nov 02, 2016 at 02:52:12PM +0100, Jérôme de Bretagne wrote:
> Hi again,
> 
> > > Heikki, I don't think anything depends on this commit, is that correct?
> > 
> > Unfortunately we can't fix the values for every UART that has ACPI
> > companion like we did before anymore.
> > 
> > There is at least one new platform, that the driver supports, which
> > does not have the Designware UART in this "16550 compatible" mode, and
> > I guess it's possible that there are others as well. And I guess the
> > other values can also be what ever on those platforms.
> > 
> > Jérôme, can you test if using the quirk on Baytrails works:
> > 
> > @@ -302,7 +302,8 @@ static void dw8250_quirks(struct uart_port *p, struct
> > dw8250_data *data)
> >  
> >                 id = acpi_match_device(p->dev->driver->acpi_match_table,
> >                                        p->dev);
> > -               if (id && !strcmp(id->id, "APMC0D08")) {
> > +               if (id && (!strcmp(id->id, "APMC0D08") ||
> > +                   !strcmp(id->id, "80860F0A"))) {
> >                         p->iotype = UPIO_MEM32;
> >                         p->regshift = 2;
> >                         p->serial_in = dw8250_serial_in32;
> 
> Compilation finished with that small modification applied: Bluetooth works
> indeed in that case and I don't have the error messages in dmesg anymore.
> 
> > 
> > Please note that this really is not a fix, not even a temporary one
> > for this issue. There are a lot of Baytrails on the market. I just
> > want to make sure there really is a problem delivering those values as
> > device properties with your board.
> 
> I guess this confirms there is indeed a problem delivering those values as
> devices properties on that specific Baytrail device at least.

I can reproduce this now with a Thinkpad10 we have.


Cheers,

-- 
heikki

[PATCH] ACPI / platform: Add support for build-in properties

[Heikki Krogerus]

We have a couple of drivers, acpi_apd.c and acpi_lpss.c,
that need to pass extra build-in properties to the devices
they create. Previously the drivers added those properties
to the struct device which is member of the struct
acpi_device, but that does not work. Those properties need
to be assigned to the struct device of the platform device
instead in order for them to become available to the
drivers.

To fix this, this patch changes acpi_create_platform_device
function to take struct property_entry pointer as parameter.

Fixes: 20a875e2e86e ("serial: 8250_dw: Add quirk for APM X-Gene SoC")
Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/acpi/acpi_apd.c             | 10 ++--------
 drivers/acpi/acpi_lpss.c            | 10 ++--------
 drivers/acpi/acpi_platform.c        |  5 ++++-
 drivers/acpi/dptf/int340x_thermal.c |  4 ++--
 drivers/acpi/scan.c                 |  2 +-
 drivers/platform/x86/intel-hid.c    |  2 +-
 drivers/platform/x86/intel-vbtn.c   |  2 +-
 include/linux/acpi.h                |  3 ++-
 8 files changed, 15 insertions(+), 23 deletions(-)

Hi,

Found the problem, and this is my proposal for a fix.

Jérôme, please test it when you have time.


Thanks,

diff --git a/drivers/acpi/acpi_apd.c b/drivers/acpi/acpi_apd.c
index 5ec7f3a..26696b6 100644
--- a/drivers/acpi/acpi_apd.c
+++ b/drivers/acpi/acpi_apd.c
@@ -127,7 +127,7 @@ static int acpi_apd_create_device(struct acpi_device *adev,
 	int ret;
 
 	if (!dev_desc) {
-		pdev = acpi_create_platform_device(adev);
+		pdev = acpi_create_platform_device(adev, NULL);
 		return IS_ERR_OR_NULL(pdev) ? PTR_ERR(pdev) : 1;
 	}
 
@@ -144,14 +144,8 @@ static int acpi_apd_create_device(struct acpi_device *adev,
 			goto err_out;
 	}
 
-	if (dev_desc->properties) {
-		ret = device_add_properties(&adev->dev, dev_desc->properties);
-		if (ret)
-			goto err_out;
-	}
-
 	adev->driver_data = pdata;
-	pdev = acpi_create_platform_device(adev);
+	pdev = acpi_create_platform_device(adev, dev_desc->properties);
 	if (!IS_ERR_OR_NULL(pdev))
 		return 1;
 
diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
index 5520102..373657f 100644
--- a/drivers/acpi/acpi_lpss.c
+++ b/drivers/acpi/acpi_lpss.c
@@ -395,7 +395,7 @@ static int acpi_lpss_create_device(struct acpi_device *adev,
 
 	dev_desc = (const struct lpss_device_desc *)id->driver_data;
 	if (!dev_desc) {
-		pdev = acpi_create_platform_device(adev);
+		pdev = acpi_create_platform_device(adev, NULL);
 		return IS_ERR_OR_NULL(pdev) ? PTR_ERR(pdev) : 1;
 	}
 	pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
@@ -451,14 +451,8 @@ static int acpi_lpss_create_device(struct acpi_device *adev,
 		goto err_out;
 	}
 
-	if (dev_desc->properties) {
-		ret = device_add_properties(&adev->dev, dev_desc->properties);
-		if (ret)
-			goto err_out;
-	}
-
 	adev->driver_data = pdata;
-	pdev = acpi_create_platform_device(adev);
+	pdev = acpi_create_platform_device(adev, dev_desc->properties);
 	if (!IS_ERR_OR_NULL(pdev)) {
 		return 1;
 	}
diff --git a/drivers/acpi/acpi_platform.c b/drivers/acpi/acpi_platform.c
index b200ae1..b4c1a6a 100644
--- a/drivers/acpi/acpi_platform.c
+++ b/drivers/acpi/acpi_platform.c
@@ -50,6 +50,7 @@ static void acpi_platform_fill_resource(struct acpi_device *adev,
 /**
  * acpi_create_platform_device - Create platform device for ACPI device node
  * @adev: ACPI device node to create a platform device for.
+ * @properties: Optional collection of build-in properties.
  *
  * Check if the given @adev can be represented as a platform device and, if
  * that's the case, create and register a platform device, populate its common
@@ -57,7 +58,8 @@ static void acpi_platform_fill_resource(struct acpi_device *adev,
  *
  * Name of the platform device will be the same as @adev's.
  */
-struct platform_device *acpi_create_platform_device(struct acpi_device *adev)
+struct platform_device *acpi_create_platform_device(struct acpi_device *adev,
+					struct property_entry *properties)
 {
 	struct platform_device *pdev = NULL;
 	struct platform_device_info pdevinfo;
@@ -106,6 +108,7 @@ struct platform_device *acpi_create_platform_device(struct acpi_device *adev)
 	pdevinfo.res = resources;
 	pdevinfo.num_res = count;
 	pdevinfo.fwnode = acpi_fwnode_handle(adev);
+	pdevinfo.properties = properties;
 
 	if (acpi_dma_supported(adev))
 		pdevinfo.dma_mask = DMA_BIT_MASK(32);
diff --git a/drivers/acpi/dptf/int340x_thermal.c b/drivers/acpi/dptf/int340x_thermal.c
index 33505c6..8636409 100644
--- a/drivers/acpi/dptf/int340x_thermal.c
+++ b/drivers/acpi/dptf/int340x_thermal.c
@@ -34,11 +34,11 @@ static int int340x_thermal_handler_attach(struct acpi_device *adev,
 					const struct acpi_device_id *id)
 {
 	if (IS_ENABLED(CONFIG_INT340X_THERMAL))
-		acpi_create_platform_device(adev);
+		acpi_create_platform_device(adev, NULL);
 	/* Intel SoC DTS thermal driver needs INT3401 to set IRQ descriptor */
 	else if (IS_ENABLED(CONFIG_INTEL_SOC_DTS_THERMAL) &&
 		 id->driver_data == INT3401_DEVICE)
-		acpi_create_platform_device(adev);
+		acpi_create_platform_device(adev, NULL);
 	return 1;
 }
 
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 035ac64..3d1856f 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1734,7 +1734,7 @@ static void acpi_default_enumeration(struct acpi_device *device)
 			       &is_spi_i2c_slave);
 	acpi_dev_free_resource_list(&resource_list);
 	if (!is_spi_i2c_slave) {
-		acpi_create_platform_device(device);
+		acpi_create_platform_device(device, NULL);
 		acpi_device_set_enumerated(device);
 	} else {
 		blocking_notifier_call_chain(&acpi_reconfig_chain,
diff --git a/drivers/platform/x86/intel-hid.c b/drivers/platform/x86/intel-hid.c
index ed58742..12dbb50 100644
--- a/drivers/platform/x86/intel-hid.c
+++ b/drivers/platform/x86/intel-hid.c
@@ -264,7 +264,7 @@ check_acpi_dev(acpi_handle handle, u32 lvl, void *context, void **rv)
 		return AE_OK;
 
 	if (acpi_match_device_ids(dev, ids) == 0)
-		if (acpi_create_platform_device(dev))
+		if (acpi_create_platform_device(dev, NULL))
 			dev_info(&dev->dev,
 				 "intel-hid: created platform device\n");
 
diff --git a/drivers/platform/x86/intel-vbtn.c b/drivers/platform/x86/intel-vbtn.c
index 146d02f..7808076 100644
--- a/drivers/platform/x86/intel-vbtn.c
+++ b/drivers/platform/x86/intel-vbtn.c
@@ -164,7 +164,7 @@ check_acpi_dev(acpi_handle handle, u32 lvl, void *context, void **rv)
 		return AE_OK;
 
 	if (acpi_match_device_ids(dev, ids) == 0)
-		if (acpi_create_platform_device(dev))
+		if (acpi_create_platform_device(dev, NULL))
 			dev_info(&dev->dev,
 				 "intel-vbtn: created platform device\n");
 
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 689a8b9..61a3d90 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -555,7 +555,8 @@ int acpi_device_uevent_modalias(struct device *, struct kobj_uevent_env *);
 int acpi_device_modalias(struct device *, char *, int);
 void acpi_walk_dep_device_list(acpi_handle handle);
 
-struct platform_device *acpi_create_platform_device(struct acpi_device *);
+struct platform_device *acpi_create_platform_device(struct acpi_device *,
+						    struct property_entry *);
 #define ACPI_PTR(_ptr)	(_ptr)
 
 static inline void acpi_device_set_enumerated(struct acpi_device *adev)
-- 
2.9.3

Re: [PATCH] ACPI / platform: Add support for build-in properties

[Yazen Ghannam]

On Thu, Nov 03, 2016 at 04:21:26PM +0200, Heikki Krogerus wrote:
> We have a couple of drivers, acpi_apd.c and acpi_lpss.c,
> that need to pass extra build-in properties to the devices
> they create. Previously the drivers added those properties
> to the struct device which is member of the struct
> acpi_device, but that does not work. Those properties need
> to be assigned to the struct device of the platform device
> instead in order for them to become available to the
> drivers.
> 
> To fix this, this patch changes acpi_create_platform_device
> function to take struct property_entry pointer as parameter.
> 
> Fixes: 20a875e2e86e ("serial: 8250_dw: Add quirk for APM X-Gene SoC")
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

I can confirm this patch fixes issues seen with AMD devices that were
introduced with 20a875e2e86e.

Please add:
Tested-by: Yazen Ghannam <yazen.ghannam@amd.com>

Thanks,
Yazen

Re: [PATCH] ACPI / platform: Add support for build-in properties

[Jérôme de Bretagne]

Le jeudi 03 novembre 2016 à 16:21 +0200, Heikki Krogerus a écrit :
> We have a couple of drivers, acpi_apd.c and acpi_lpss.c,
> that need to pass extra build-in properties to the devices
> they create. Previously the drivers added those properties
> to the struct device which is member of the struct
> acpi_device, but that does not work. Those properties need
> to be assigned to the struct device of the platform device
> instead in order for them to become available to the
> drivers.
> 
> To fix this, this patch changes acpi_create_platform_device
> function to take struct property_entry pointer as parameter.
> 
> Fixes: 20a875e2e86e ("serial: 8250_dw: Add quirk for APM X-Gene SoC")
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
> 
> Hi,
> 
> Found the problem, and this is my proposal for a fix.
> 
> Jérôme, please test it when you have time.
> 

Hi Heikki,

I can confirm that this patch fixes the issue I've reported on the Lenovo
ThinkPad 8 tablet. You were fast to find the root cause, impressive, really
appreciated.

Feel free to add:
Tested-by: Jérôme de Bretagne <jerome.debretagne@gmail.com> 

Let's hope it can still make it during the remaining 4.9 rc timing.

Cheers,
Jérôme

Re: [PATCH] ACPI / platform: Add support for build-in properties

[Rafael J. Wysocki]

On Sun, Nov 6, 2016 at 5:09 PM, J=C3=A9r=C3=B4me de Bretagne
<jerome.debretagne@gmail.com> wrote:
> Le jeudi 03 novembre 2016 =C3=A0 16:21 +0200, Heikki Krogerus a =C3=A9cri=
t :
>> We have a couple of drivers, acpi_apd.c and acpi_lpss.c,
>> that need to pass extra build-in properties to the devices
>> they create. Previously the drivers added those properties
>> to the struct device which is member of the struct
>> acpi_device, but that does not work. Those properties need
>> to be assigned to the struct device of the platform device
>> instead in order for them to become available to the
>> drivers.
>>
>> To fix this, this patch changes acpi_create_platform_device
>> function to take struct property_entry pointer as parameter.
>>
>> Fixes: 20a875e2e86e ("serial: 8250_dw: Add quirk for APM X-Gene SoC")
>> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>> ---
>>
>> Hi,
>>
>> Found the problem, and this is my proposal for a fix.
>>
>> J=C3=A9r=C3=B4me, please test it when you have time.
>>
>
> Hi Heikki,
>
> I can confirm that this patch fixes the issue I've reported on the Lenovo
> ThinkPad 8 tablet. You were fast to find the root cause, impressive, real=
ly
> appreciated.
>
> Feel free to add:
> Tested-by: J=C3=A9r=C3=B4me de Bretagne <jerome.debretagne@gmail.com>
>
> Let's hope it can still make it during the remaining 4.9 rc timing.

It's in my linux-next branch now (it should hit linux-next on Friday)
and I'm going to push it for -rc5 if it doesn't lead to problems.

Thanks,
Rafael

Re: [PATCH] ACPI / platform: Add support for build-in properties

[Andy Shevchenko]

On Thu, 2016-11-03 at 16:21 +0200, Heikki Krogerus wrote:
> We have a couple of drivers, acpi_apd.c and acpi_lpss.c,
> that need to pass extra build-in properties to the devices
> they create. Previously the drivers added those properties
> to the struct device which is member of the struct
> acpi_device, but that does not work. Those properties need
> to be assigned to the struct device of the platform device
> instead in order for them to become available to the
> drivers.
> 
> To fix this, this patch changes acpi_create_platform_device
> function to take struct property_entry pointer as parameter.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> 
> Fixes: 20a875e2e86e ("serial: 8250_dw: Add quirk for APM X-Gene SoC")
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
>  drivers/acpi/acpi_apd.c             | 10 ++--------
>  drivers/acpi/acpi_lpss.c            | 10 ++--------
>  drivers/acpi/acpi_platform.c        |  5 ++++-
>  drivers/acpi/dptf/int340x_thermal.c |  4 ++--
>  drivers/acpi/scan.c                 |  2 +-
>  drivers/platform/x86/intel-hid.c    |  2 +-
>  drivers/platform/x86/intel-vbtn.c   |  2 +-
>  include/linux/acpi.h                |  3 ++-
>  8 files changed, 15 insertions(+), 23 deletions(-)
> 
> Hi,
> 
> Found the problem, and this is my proposal for a fix.
> 
> Jérôme, please test it when you have time.
> 
> 
> Thanks,
> 
> diff --git a/drivers/acpi/acpi_apd.c b/drivers/acpi/acpi_apd.c
> index 5ec7f3a..26696b6 100644
> --- a/drivers/acpi/acpi_apd.c
> +++ b/drivers/acpi/acpi_apd.c
> @@ -127,7 +127,7 @@ static int acpi_apd_create_device(struct
> acpi_device *adev,
>  	int ret;
>  
>  	if (!dev_desc) {
> -		pdev = acpi_create_platform_device(adev);
> +		pdev = acpi_create_platform_device(adev, NULL);
>  		return IS_ERR_OR_NULL(pdev) ? PTR_ERR(pdev) : 1;
>  	}
>  
> @@ -144,14 +144,8 @@ static int acpi_apd_create_device(struct
> acpi_device *adev,
>  			goto err_out;
>  	}
>  
> -	if (dev_desc->properties) {
> -		ret = device_add_properties(&adev->dev, dev_desc-
> >properties);
> -		if (ret)
> -			goto err_out;
> -	}
> -
>  	adev->driver_data = pdata;
> -	pdev = acpi_create_platform_device(adev);
> +	pdev = acpi_create_platform_device(adev, dev_desc-
> >properties);
>  	if (!IS_ERR_OR_NULL(pdev))
>  		return 1;
>  
> diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
> index 5520102..373657f 100644
> --- a/drivers/acpi/acpi_lpss.c
> +++ b/drivers/acpi/acpi_lpss.c
> @@ -395,7 +395,7 @@ static int acpi_lpss_create_device(struct
> acpi_device *adev,
>  
>  	dev_desc = (const struct lpss_device_desc *)id->driver_data;
>  	if (!dev_desc) {
> -		pdev = acpi_create_platform_device(adev);
> +		pdev = acpi_create_platform_device(adev, NULL);
>  		return IS_ERR_OR_NULL(pdev) ? PTR_ERR(pdev) : 1;
>  	}
>  	pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
> @@ -451,14 +451,8 @@ static int acpi_lpss_create_device(struct
> acpi_device *adev,
>  		goto err_out;
>  	}
>  
> -	if (dev_desc->properties) {
> -		ret = device_add_properties(&adev->dev, dev_desc-
> >properties);
> -		if (ret)
> -			goto err_out;
> -	}
> -
>  	adev->driver_data = pdata;
> -	pdev = acpi_create_platform_device(adev);
> +	pdev = acpi_create_platform_device(adev, dev_desc-
> >properties);
>  	if (!IS_ERR_OR_NULL(pdev)) {
>  		return 1;
>  	}
> diff --git a/drivers/acpi/acpi_platform.c
> b/drivers/acpi/acpi_platform.c
> index b200ae1..b4c1a6a 100644
> --- a/drivers/acpi/acpi_platform.c
> +++ b/drivers/acpi/acpi_platform.c
> @@ -50,6 +50,7 @@ static void acpi_platform_fill_resource(struct
> acpi_device *adev,
>  /**
>   * acpi_create_platform_device - Create platform device for ACPI
> device node
>   * @adev: ACPI device node to create a platform device for.
> + * @properties: Optional collection of build-in properties.
>   *
>   * Check if the given @adev can be represented as a platform device
> and, if
>   * that's the case, create and register a platform device, populate
> its common
> @@ -57,7 +58,8 @@ static void acpi_platform_fill_resource(struct
> acpi_device *adev,
>   *
>   * Name of the platform device will be the same as @adev's.
>   */
> -struct platform_device *acpi_create_platform_device(struct
> acpi_device *adev)
> +struct platform_device *acpi_create_platform_device(struct
> acpi_device *adev,
> +					struct property_entry
> *properties)
>  {
>  	struct platform_device *pdev = NULL;
>  	struct platform_device_info pdevinfo;
> @@ -106,6 +108,7 @@ struct platform_device
> *acpi_create_platform_device(struct acpi_device *adev)
>  	pdevinfo.res = resources;
>  	pdevinfo.num_res = count;
>  	pdevinfo.fwnode = acpi_fwnode_handle(adev);
> +	pdevinfo.properties = properties;
>  
>  	if (acpi_dma_supported(adev))
>  		pdevinfo.dma_mask = DMA_BIT_MASK(32);
> diff --git a/drivers/acpi/dptf/int340x_thermal.c
> b/drivers/acpi/dptf/int340x_thermal.c
> index 33505c6..8636409 100644
> --- a/drivers/acpi/dptf/int340x_thermal.c
> +++ b/drivers/acpi/dptf/int340x_thermal.c
> @@ -34,11 +34,11 @@ static int int340x_thermal_handler_attach(struct
> acpi_device *adev,
>  					const struct acpi_device_id
> *id)
>  {
>  	if (IS_ENABLED(CONFIG_INT340X_THERMAL))
> -		acpi_create_platform_device(adev);
> +		acpi_create_platform_device(adev, NULL);
>  	/* Intel SoC DTS thermal driver needs INT3401 to set IRQ
> descriptor */
>  	else if (IS_ENABLED(CONFIG_INTEL_SOC_DTS_THERMAL) &&
>  		 id->driver_data == INT3401_DEVICE)
> -		acpi_create_platform_device(adev);
> +		acpi_create_platform_device(adev, NULL);
>  	return 1;
>  }
>  
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 035ac64..3d1856f 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -1734,7 +1734,7 @@ static void acpi_default_enumeration(struct
> acpi_device *device)
>  			       &is_spi_i2c_slave);
>  	acpi_dev_free_resource_list(&resource_list);
>  	if (!is_spi_i2c_slave) {
> -		acpi_create_platform_device(device);
> +		acpi_create_platform_device(device, NULL);
>  		acpi_device_set_enumerated(device);
>  	} else {
>  		blocking_notifier_call_chain(&acpi_reconfig_chain,
> diff --git a/drivers/platform/x86/intel-hid.c
> b/drivers/platform/x86/intel-hid.c
> index ed58742..12dbb50 100644
> --- a/drivers/platform/x86/intel-hid.c
> +++ b/drivers/platform/x86/intel-hid.c
> @@ -264,7 +264,7 @@ check_acpi_dev(acpi_handle handle, u32 lvl, void
> *context, void **rv)
>  		return AE_OK;
>  
>  	if (acpi_match_device_ids(dev, ids) == 0)
> -		if (acpi_create_platform_device(dev))
> +		if (acpi_create_platform_device(dev, NULL))
>  			dev_info(&dev->dev,
>  				 "intel-hid: created platform
> device\n");
>  
> diff --git a/drivers/platform/x86/intel-vbtn.c
> b/drivers/platform/x86/intel-vbtn.c
> index 146d02f..7808076 100644
> --- a/drivers/platform/x86/intel-vbtn.c
> +++ b/drivers/platform/x86/intel-vbtn.c
> @@ -164,7 +164,7 @@ check_acpi_dev(acpi_handle handle, u32 lvl, void
> *context, void **rv)
>  		return AE_OK;
>  
>  	if (acpi_match_device_ids(dev, ids) == 0)
> -		if (acpi_create_platform_device(dev))
> +		if (acpi_create_platform_device(dev, NULL))
>  			dev_info(&dev->dev,
>  				 "intel-vbtn: created platform
> device\n");
>  
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 689a8b9..61a3d90 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -555,7 +555,8 @@ int acpi_device_uevent_modalias(struct device *,
> struct kobj_uevent_env *);
>  int acpi_device_modalias(struct device *, char *, int);
>  void acpi_walk_dep_device_list(acpi_handle handle);
>  
> -struct platform_device *acpi_create_platform_device(struct
> acpi_device *);
> +struct platform_device *acpi_create_platform_device(struct
> acpi_device *,
> +						    struct
> property_entry *);
>  #define ACPI_PTR(_ptr)	(_ptr)
>  
>  static inline void acpi_device_set_enumerated(struct acpi_device
> *adev)

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy