[PATCH] ACPI / LPSS: Work around wrong sdio _ADR 0 entry on some byt/cht devices

[PATCH] ACPI / LPSS: Work around wrong sdio _ADR 0 entry on some byt/cht devices

[Hans de Goede]

The firmware on some cherrytrail devices wrongly adds _ADR 0 to their
entry describing the 80860F14 uid "2" sd-controller.

I believe the firmware writers intended this as a sdio function address,
but it is in the wrong place for this, so it gets interpreted as a pci
address, causing the node describing the sd-controller used for the
sdio-wifi to get seen as a firmware_node for the pci host bridge, rather
then being stand-alone device.

This commit adds a byt_sdio_setup function which detects this scenario
and removes the wrong firmware_node link from the pci host bridge, which
fixes acpi_create_platform_device returning NULL, leading to non-working
sdio-wifi.

BugLink: https://github.com/hadess/rtl8723bs/issues/80
Cc: stable@vger.kernel.org
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/acpi/acpi_lpss.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
index 373657f..df9cc66 100644
--- a/drivers/acpi/acpi_lpss.c
+++ b/drivers/acpi/acpi_lpss.c
@@ -84,6 +84,7 @@ static const struct lpss_device_desc lpss_dma_desc = {
 };
 
 struct lpss_private_data {
+	struct acpi_device *adev;
 	void __iomem *mmio_base;
 	resource_size_t mmio_size;
 	unsigned int fixed_clk_rate;
@@ -154,6 +155,33 @@ static void byt_i2c_setup(struct lpss_private_data *pdata)
 	writel(0, pdata->mmio_base + LPSS_I2C_ENABLE);
 }
 
+static void byt_sdio_setup(struct lpss_private_data *pdata)
+{
+	unsigned long long adr;
+	acpi_status status;
+	struct device *dev;
+
+	/*
+	 * Some firmware has a broken _ADR 0 enter for the 80860F14:2
+	 * device, which causes it to get seen as the firmware_node
+	 * for the pci host bridge, rather then a stand alone device.
+	 *
+	 * Check if this is the case, and if it is remove the link.
+	 */
+	if (strcmp(acpi_device_uid(pdata->adev), "2") != 0)
+		return;
+
+	status = acpi_evaluate_integer(pdata->adev->handle, "_ADR", NULL, &adr);
+	if (ACPI_FAILURE(status) || adr != 0)
+		return;
+
+	dev = acpi_get_first_physical_node(pdata->adev);
+	if (!dev)
+		return;
+
+	acpi_unbind_one(dev);
+}
+
 static const struct lpss_device_desc lpt_dev_desc = {
 	.flags = LPSS_CLK | LPSS_CLK_GATE | LPSS_CLK_DIVIDER | LPSS_LTR,
 	.prv_offset = 0x800,
@@ -217,6 +245,7 @@ static const struct lpss_device_desc byt_spi_dev_desc = {
 
 static const struct lpss_device_desc byt_sdio_dev_desc = {
 	.flags = LPSS_CLK,
+	.setup = byt_sdio_setup,
 };
 
 static const struct lpss_device_desc byt_i2c_dev_desc = {
@@ -425,6 +454,7 @@ static int acpi_lpss_create_device(struct acpi_device *adev,
 		goto err_out;
 	}
 
+	pdata->adev = adev;
 	pdata->dev_desc = dev_desc;
 
 	if (dev_desc->setup)
-- 
2.9.3

Re: [PATCH] ACPI / LPSS: Work around wrong sdio _ADR 0 entry on some byt/cht devices

[Rafael J. Wysocki]

CC Mika and Andy.

Plus I don't think -stable is going to take your patches directly.

On Sun, Dec 25, 2016 at 11:21 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> The firmware on some cherrytrail devices wrongly adds _ADR 0 to their
> entry describing the 80860F14 uid "2" sd-controller.
>
> I believe the firmware writers intended this as a sdio function address,
> but it is in the wrong place for this, so it gets interpreted as a pci
> address, causing the node describing the sd-controller used for the
> sdio-wifi to get seen as a firmware_node for the pci host bridge, rather
> then being stand-alone device.
>
> This commit adds a byt_sdio_setup function which detects this scenario
> and removes the wrong firmware_node link from the pci host bridge, which
> fixes acpi_create_platform_device returning NULL, leading to non-working
> sdio-wifi.
>
> BugLink: https://github.com/hadess/rtl8723bs/issues/80
> Cc: stable@vger.kernel.org
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/acpi/acpi_lpss.c | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
>
> diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
> index 373657f..df9cc66 100644
> --- a/drivers/acpi/acpi_lpss.c
> +++ b/drivers/acpi/acpi_lpss.c
> @@ -84,6 +84,7 @@ static const struct lpss_device_desc lpss_dma_desc = {
>  };
>
>  struct lpss_private_data {
> +       struct acpi_device *adev;
>         void __iomem *mmio_base;
>         resource_size_t mmio_size;
>         unsigned int fixed_clk_rate;
> @@ -154,6 +155,33 @@ static void byt_i2c_setup(struct lpss_private_data *pdata)
>         writel(0, pdata->mmio_base + LPSS_I2C_ENABLE);
>  }
>
> +static void byt_sdio_setup(struct lpss_private_data *pdata)
> +{
> +       unsigned long long adr;
> +       acpi_status status;
> +       struct device *dev;
> +
> +       /*
> +        * Some firmware has a broken _ADR 0 enter for the 80860F14:2
> +        * device, which causes it to get seen as the firmware_node
> +        * for the pci host bridge, rather then a stand alone device.
> +        *
> +        * Check if this is the case, and if it is remove the link.
> +        */
> +       if (strcmp(acpi_device_uid(pdata->adev), "2") != 0)
> +               return;
> +
> +       status = acpi_evaluate_integer(pdata->adev->handle, "_ADR", NULL, &adr);
> +       if (ACPI_FAILURE(status) || adr != 0)
> +               return;
> +
> +       dev = acpi_get_first_physical_node(pdata->adev);
> +       if (!dev)
> +               return;
> +
> +       acpi_unbind_one(dev);
> +}
> +
>  static const struct lpss_device_desc lpt_dev_desc = {
>         .flags = LPSS_CLK | LPSS_CLK_GATE | LPSS_CLK_DIVIDER | LPSS_LTR,
>         .prv_offset = 0x800,
> @@ -217,6 +245,7 @@ static const struct lpss_device_desc byt_spi_dev_desc = {
>
>  static const struct lpss_device_desc byt_sdio_dev_desc = {
>         .flags = LPSS_CLK,
> +       .setup = byt_sdio_setup,
>  };
>
>  static const struct lpss_device_desc byt_i2c_dev_desc = {
> @@ -425,6 +454,7 @@ static int acpi_lpss_create_device(struct acpi_device *adev,
>                 goto err_out;
>         }
>
> +       pdata->adev = adev;
>         pdata->dev_desc = dev_desc;
>
>         if (dev_desc->setup)
> --
> 2.9.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ACPI / LPSS: Work around wrong sdio _ADR 0 entry on some byt/cht devices

[Hans de Goede]

Hi,

On 26-12-16 00:25, Rafael J. Wysocki wrote:
> CC Mika and Andy.
>
> Plus I don't think -stable is going to take your patches directly.

Not sure what you mean with this remark? According to:

Documentation/stable_kernel_rules.txt

Option 1
********

To have the patch automatically included in the stable tree, add the tag

.. code-block:: none

      Cc: stable@vger.kernel.org

in the sign-off area. Once the patch is merged it will be applied to
the stable tree without anything else needing to be done by the author
or subsystem maintainer.


So yes, it won't get merged until it has been merged into Linus'
tree. But AFAICT adding Cc: stable@vger.kernel.org is the right way
to indicate that a patch is a bug-fix which should be applied to
stable kernels once merged, which is my intention of adding the Cc.

Regards,

Hans



>
> On Sun, Dec 25, 2016 at 11:21 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>> The firmware on some cherrytrail devices wrongly adds _ADR 0 to their
>> entry describing the 80860F14 uid "2" sd-controller.
>>
>> I believe the firmware writers intended this as a sdio function address,
>> but it is in the wrong place for this, so it gets interpreted as a pci
>> address, causing the node describing the sd-controller used for the
>> sdio-wifi to get seen as a firmware_node for the pci host bridge, rather
>> then being stand-alone device.
>>
>> This commit adds a byt_sdio_setup function which detects this scenario
>> and removes the wrong firmware_node link from the pci host bridge, which
>> fixes acpi_create_platform_device returning NULL, leading to non-working
>> sdio-wifi.
>>
>> BugLink: https://github.com/hadess/rtl8723bs/issues/80
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/acpi/acpi_lpss.c | 30 ++++++++++++++++++++++++++++++
>>  1 file changed, 30 insertions(+)
>>
>> diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
>> index 373657f..df9cc66 100644
>> --- a/drivers/acpi/acpi_lpss.c
>> +++ b/drivers/acpi/acpi_lpss.c
>> @@ -84,6 +84,7 @@ static const struct lpss_device_desc lpss_dma_desc = {
>>  };
>>
>>  struct lpss_private_data {
>> +       struct acpi_device *adev;
>>         void __iomem *mmio_base;
>>         resource_size_t mmio_size;
>>         unsigned int fixed_clk_rate;
>> @@ -154,6 +155,33 @@ static void byt_i2c_setup(struct lpss_private_data *pdata)
>>         writel(0, pdata->mmio_base + LPSS_I2C_ENABLE);
>>  }
>>
>> +static void byt_sdio_setup(struct lpss_private_data *pdata)
>> +{
>> +       unsigned long long adr;
>> +       acpi_status status;
>> +       struct device *dev;
>> +
>> +       /*
>> +        * Some firmware has a broken _ADR 0 enter for the 80860F14:2
>> +        * device, which causes it to get seen as the firmware_node
>> +        * for the pci host bridge, rather then a stand alone device.
>> +        *
>> +        * Check if this is the case, and if it is remove the link.
>> +        */
>> +       if (strcmp(acpi_device_uid(pdata->adev), "2") != 0)
>> +               return;
>> +
>> +       status = acpi_evaluate_integer(pdata->adev->handle, "_ADR", NULL, &adr);
>> +       if (ACPI_FAILURE(status) || adr != 0)
>> +               return;
>> +
>> +       dev = acpi_get_first_physical_node(pdata->adev);
>> +       if (!dev)
>> +               return;
>> +
>> +       acpi_unbind_one(dev);
>> +}
>> +
>>  static const struct lpss_device_desc lpt_dev_desc = {
>>         .flags = LPSS_CLK | LPSS_CLK_GATE | LPSS_CLK_DIVIDER | LPSS_LTR,
>>         .prv_offset = 0x800,
>> @@ -217,6 +245,7 @@ static const struct lpss_device_desc byt_spi_dev_desc = {
>>
>>  static const struct lpss_device_desc byt_sdio_dev_desc = {
>>         .flags = LPSS_CLK,
>> +       .setup = byt_sdio_setup,
>>  };
>>
>>  static const struct lpss_device_desc byt_i2c_dev_desc = {
>> @@ -425,6 +454,7 @@ static int acpi_lpss_create_device(struct acpi_device *adev,
>>                 goto err_out;
>>         }
>>
>> +       pdata->adev = adev;
>>         pdata->dev_desc = dev_desc;
>>
>>         if (dev_desc->setup)
>> --
>> 2.9.3
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ACPI / LPSS: Work around wrong sdio _ADR 0 entry on some byt/cht devices

[Rafael J. Wysocki]

On Mon, Dec 26, 2016 at 11:48 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
> On 26-12-16 00:25, Rafael J. Wysocki wrote:
>>
>> CC Mika and Andy.
>>
>> Plus I don't think -stable is going to take your patches directly.
>
>
> Not sure what you mean with this remark? According to:
>
> Documentation/stable_kernel_rules.txt
>
> Option 1
> ********
>
> To have the patch automatically included in the stable tree, add the tag
>
> .. code-block:: none
>
>      Cc: stable@vger.kernel.org
>
> in the sign-off area. Once the patch is merged it will be applied to
> the stable tree without anything else needing to be done by the author
> or subsystem maintainer.
>
> So yes, it won't get merged until it has been merged into Linus'
> tree. But AFAICT adding Cc: stable@vger.kernel.org is the right way
> to indicate that a patch is a bug-fix which should be applied to
> stable kernels once merged, which is my intention of adding the Cc.

Yes, you can add a CC: <stable@...> tag to indicate that the patch is
-stable material (in which case please also indicate which -stable
series you want it to go to).

No, you should not CC -stable on the patch submission.

The CC: <stable@...> tag is for the maintainer you're sending the
patch to rather than for -stable itself.

Thanks,
Rafael

Re: [PATCH] ACPI / LPSS: Work around wrong sdio _ADR 0 entry on some byt/cht devices

[Mika Westerberg]

On Mon, Dec 26, 2016 at 12:25:19AM +0100, Rafael J. Wysocki wrote:
> CC Mika and Andy.
> 
> Plus I don't think -stable is going to take your patches directly.
> 
> On Sun, Dec 25, 2016 at 11:21 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> > The firmware on some cherrytrail devices wrongly adds _ADR 0 to their
> > entry describing the 80860F14 uid "2" sd-controller.
> >
> > I believe the firmware writers intended this as a sdio function address,
> > but it is in the wrong place for this, so it gets interpreted as a pci
> > address, causing the node describing the sd-controller used for the
> > sdio-wifi to get seen as a firmware_node for the pci host bridge, rather
> > then being stand-alone device.
> >
> > This commit adds a byt_sdio_setup function which detects this scenario
> > and removes the wrong firmware_node link from the pci host bridge, which
> > fixes acpi_create_platform_device returning NULL, leading to non-working
> > sdio-wifi.
> >
> > BugLink: https://github.com/hadess/rtl8723bs/issues/80
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > ---
> >  drivers/acpi/acpi_lpss.c | 30 ++++++++++++++++++++++++++++++
> >  1 file changed, 30 insertions(+)
> >
> > diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
> > index 373657f..df9cc66 100644
> > --- a/drivers/acpi/acpi_lpss.c
> > +++ b/drivers/acpi/acpi_lpss.c
> > @@ -84,6 +84,7 @@ static const struct lpss_device_desc lpss_dma_desc = {
> >  };
> >
> >  struct lpss_private_data {
> > +       struct acpi_device *adev;
> >         void __iomem *mmio_base;
> >         resource_size_t mmio_size;
> >         unsigned int fixed_clk_rate;
> > @@ -154,6 +155,33 @@ static void byt_i2c_setup(struct lpss_private_data *pdata)
> >         writel(0, pdata->mmio_base + LPSS_I2C_ENABLE);
> >  }
> >
> > +static void byt_sdio_setup(struct lpss_private_data *pdata)
> > +{
> > +       unsigned long long adr;
> > +       acpi_status status;
> > +       struct device *dev;
> > +
> > +       /*
> > +        * Some firmware has a broken _ADR 0 enter for the 80860F14:2
> > +        * device, which causes it to get seen as the firmware_node
> > +        * for the pci host bridge, rather then a stand alone device.
> > +        *
> > +        * Check if this is the case, and if it is remove the link.
> > +        */
> > +       if (strcmp(acpi_device_uid(pdata->adev), "2") != 0)
> > +               return;
> > +
> > +       status = acpi_evaluate_integer(pdata->adev->handle, "_ADR", NULL, &adr);
> > +       if (ACPI_FAILURE(status) || adr != 0)
> > +               return;
> > +
> > +       dev = acpi_get_first_physical_node(pdata->adev);
> > +       if (!dev)
> > +               return;
> > +
> > +       acpi_unbind_one(dev);
> > +}

IIRC the _ADR 0 problem is not only limited to SDIO devices. I think we
should just fix the match heuristics in acpi_find_child_device() to
cover all other devices as well.

Something like below might do the job.

diff --git a/drivers/acpi/glue.c b/drivers/acpi/glue.c
index 5ea5dc219f56..7cfd48f55b6f 100644
--- a/drivers/acpi/glue.c
+++ b/drivers/acpi/glue.c
@@ -85,7 +85,7 @@ static struct acpi_bus_type *acpi_get_bus_type(struct device *dev)
 
 static int find_child_checks(struct acpi_device *adev, bool check_children)
 {
-	bool sta_present = true;
+	bool hid_present, sta_present = true;
 	unsigned long long sta;
 	acpi_status status;
 
@@ -98,7 +98,22 @@ static int find_child_checks(struct acpi_device *adev, bool check_children)
 	if (check_children && list_empty(&adev->children))
 		return -ENODEV;
 
-	return sta_present ? FIND_CHILD_MAX_SCORE : FIND_CHILD_MIN_SCORE;
+	/*
+	 * If there is a _HID or _CID present on a bus that is supposed to
+	 * be matched using _ADR, we prioritize those devices without ID
+	 * higher.
+	 *
+	 * This is because there are many BIOSes out there having ACPI
+	 * enumerated devices and the _ADR is set to 0. This will match the
+	 * root PCI bridge to the first found device with _ADR 0 which is
+	 * not always the right device.
+	 */
+	hid_present = !list_empty(&adev->pnp.ids);
+
+	if (sta_present)
+		return hid_present ? FIND_CHILD_MIN_SCORE : FIND_CHILD_MAX_SCORE;
+
+	return FIND_CHILD_MIN_SCORE;
 }
 
 struct acpi_device *acpi_find_child_device(struct acpi_device *parent,

Re: [PATCH] ACPI / LPSS: Work around wrong sdio _ADR 0 entry on some byt/cht devices

[Rafael J. Wysocki]

On Wed, Dec 28, 2016 at 10:14 AM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Mon, Dec 26, 2016 at 12:25:19AM +0100, Rafael J. Wysocki wrote:
>> CC Mika and Andy.
>>
>> Plus I don't think -stable is going to take your patches directly.
>>
>> On Sun, Dec 25, 2016 at 11:21 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>> > The firmware on some cherrytrail devices wrongly adds _ADR 0 to their
>> > entry describing the 80860F14 uid "2" sd-controller.
>> >
>> > I believe the firmware writers intended this as a sdio function address,
>> > but it is in the wrong place for this, so it gets interpreted as a pci
>> > address, causing the node describing the sd-controller used for the
>> > sdio-wifi to get seen as a firmware_node for the pci host bridge, rather
>> > then being stand-alone device.
>> >
>> > This commit adds a byt_sdio_setup function which detects this scenario
>> > and removes the wrong firmware_node link from the pci host bridge, which
>> > fixes acpi_create_platform_device returning NULL, leading to non-working
>> > sdio-wifi.
>> >
>> > BugLink: https://github.com/hadess/rtl8723bs/issues/80
>> > Cc: stable@vger.kernel.org
>> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> > ---
>> >  drivers/acpi/acpi_lpss.c | 30 ++++++++++++++++++++++++++++++
>> >  1 file changed, 30 insertions(+)
>> >
>> > diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
>> > index 373657f..df9cc66 100644
>> > --- a/drivers/acpi/acpi_lpss.c
>> > +++ b/drivers/acpi/acpi_lpss.c
>> > @@ -84,6 +84,7 @@ static const struct lpss_device_desc lpss_dma_desc = {
>> >  };
>> >
>> >  struct lpss_private_data {
>> > +       struct acpi_device *adev;
>> >         void __iomem *mmio_base;
>> >         resource_size_t mmio_size;
>> >         unsigned int fixed_clk_rate;
>> > @@ -154,6 +155,33 @@ static void byt_i2c_setup(struct lpss_private_data *pdata)
>> >         writel(0, pdata->mmio_base + LPSS_I2C_ENABLE);
>> >  }
>> >
>> > +static void byt_sdio_setup(struct lpss_private_data *pdata)
>> > +{
>> > +       unsigned long long adr;
>> > +       acpi_status status;
>> > +       struct device *dev;
>> > +
>> > +       /*
>> > +        * Some firmware has a broken _ADR 0 enter for the 80860F14:2
>> > +        * device, which causes it to get seen as the firmware_node
>> > +        * for the pci host bridge, rather then a stand alone device.
>> > +        *
>> > +        * Check if this is the case, and if it is remove the link.
>> > +        */
>> > +       if (strcmp(acpi_device_uid(pdata->adev), "2") != 0)
>> > +               return;
>> > +
>> > +       status = acpi_evaluate_integer(pdata->adev->handle, "_ADR", NULL, &adr);
>> > +       if (ACPI_FAILURE(status) || adr != 0)
>> > +               return;
>> > +
>> > +       dev = acpi_get_first_physical_node(pdata->adev);
>> > +       if (!dev)
>> > +               return;
>> > +
>> > +       acpi_unbind_one(dev);
>> > +}
>
> IIRC the _ADR 0 problem is not only limited to SDIO devices. I think we
> should just fix the match heuristics in acpi_find_child_device() to
> cover all other devices as well.
>
> Something like below might do the job.
>
> diff --git a/drivers/acpi/glue.c b/drivers/acpi/glue.c
> index 5ea5dc219f56..7cfd48f55b6f 100644
> --- a/drivers/acpi/glue.c
> +++ b/drivers/acpi/glue.c
> @@ -85,7 +85,7 @@ static struct acpi_bus_type *acpi_get_bus_type(struct device *dev)
>
>  static int find_child_checks(struct acpi_device *adev, bool check_children)
>  {
> -       bool sta_present = true;
> +       bool hid_present, sta_present = true;
>         unsigned long long sta;
>         acpi_status status;
>
> @@ -98,7 +98,22 @@ static int find_child_checks(struct acpi_device *adev, bool check_children)
>         if (check_children && list_empty(&adev->children))
>                 return -ENODEV;
>
> -       return sta_present ? FIND_CHILD_MAX_SCORE : FIND_CHILD_MIN_SCORE;
> +       /*
> +        * If there is a _HID or _CID present on a bus that is supposed to
> +        * be matched using _ADR, we prioritize those devices without ID
> +        * higher.
> +        *
> +        * This is because there are many BIOSes out there having ACPI
> +        * enumerated devices and the _ADR is set to 0. This will match the
> +        * root PCI bridge to the first found device with _ADR 0 which is
> +        * not always the right device.
> +        */
> +       hid_present = !list_empty(&adev->pnp.ids);
> +
> +       if (sta_present)
> +               return hid_present ? FIND_CHILD_MIN_SCORE : FIND_CHILD_MAX_SCORE;
> +
> +       return FIND_CHILD_MIN_SCORE;
>  }
>
>  struct acpi_device *acpi_find_child_device(struct acpi_device *parent,
>

I'd rather only change the return statement, like this:

return sta_present && list_empty(&adev->pnp.ids) ?
FIND_CHILD_MAX_SCORE : FIND_CHILD_MIN_SCORE;

I'll send a patch tomorrow unless there's something wrong with this.

BTW, the comment is not correct too, because the host bridge has a
_HID.  What is matched against _ADR(0) is the first function on bus 0
IIRC.

Thanks,
Rafael

Re: [PATCH] ACPI / LPSS: Work around wrong sdio _ADR 0 entry on some byt/cht devices

[Mika Westerberg]

On Wed, Dec 28, 2016 at 11:30:35PM +0100, Rafael J. Wysocki wrote:
> On Wed, Dec 28, 2016 at 10:14 AM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > On Mon, Dec 26, 2016 at 12:25:19AM +0100, Rafael J. Wysocki wrote:
> >> CC Mika and Andy.
> >>
> >> Plus I don't think -stable is going to take your patches directly.
> >>
> >> On Sun, Dec 25, 2016 at 11:21 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> >> > The firmware on some cherrytrail devices wrongly adds _ADR 0 to their
> >> > entry describing the 80860F14 uid "2" sd-controller.
> >> >
> >> > I believe the firmware writers intended this as a sdio function address,
> >> > but it is in the wrong place for this, so it gets interpreted as a pci
> >> > address, causing the node describing the sd-controller used for the
> >> > sdio-wifi to get seen as a firmware_node for the pci host bridge, rather
> >> > then being stand-alone device.
> >> >
> >> > This commit adds a byt_sdio_setup function which detects this scenario
> >> > and removes the wrong firmware_node link from the pci host bridge, which
> >> > fixes acpi_create_platform_device returning NULL, leading to non-working
> >> > sdio-wifi.
> >> >
> >> > BugLink: https://github.com/hadess/rtl8723bs/issues/80
> >> > Cc: stable@vger.kernel.org
> >> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >> > ---
> >> >  drivers/acpi/acpi_lpss.c | 30 ++++++++++++++++++++++++++++++
> >> >  1 file changed, 30 insertions(+)
> >> >
> >> > diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
> >> > index 373657f..df9cc66 100644
> >> > --- a/drivers/acpi/acpi_lpss.c
> >> > +++ b/drivers/acpi/acpi_lpss.c
> >> > @@ -84,6 +84,7 @@ static const struct lpss_device_desc lpss_dma_desc = {
> >> >  };
> >> >
> >> >  struct lpss_private_data {
> >> > +       struct acpi_device *adev;
> >> >         void __iomem *mmio_base;
> >> >         resource_size_t mmio_size;
> >> >         unsigned int fixed_clk_rate;
> >> > @@ -154,6 +155,33 @@ static void byt_i2c_setup(struct lpss_private_data *pdata)
> >> >         writel(0, pdata->mmio_base + LPSS_I2C_ENABLE);
> >> >  }
> >> >
> >> > +static void byt_sdio_setup(struct lpss_private_data *pdata)
> >> > +{
> >> > +       unsigned long long adr;
> >> > +       acpi_status status;
> >> > +       struct device *dev;
> >> > +
> >> > +       /*
> >> > +        * Some firmware has a broken _ADR 0 enter for the 80860F14:2
> >> > +        * device, which causes it to get seen as the firmware_node
> >> > +        * for the pci host bridge, rather then a stand alone device.
> >> > +        *
> >> > +        * Check if this is the case, and if it is remove the link.
> >> > +        */
> >> > +       if (strcmp(acpi_device_uid(pdata->adev), "2") != 0)
> >> > +               return;
> >> > +
> >> > +       status = acpi_evaluate_integer(pdata->adev->handle, "_ADR", NULL, &adr);
> >> > +       if (ACPI_FAILURE(status) || adr != 0)
> >> > +               return;
> >> > +
> >> > +       dev = acpi_get_first_physical_node(pdata->adev);
> >> > +       if (!dev)
> >> > +               return;
> >> > +
> >> > +       acpi_unbind_one(dev);
> >> > +}
> >
> > IIRC the _ADR 0 problem is not only limited to SDIO devices. I think we
> > should just fix the match heuristics in acpi_find_child_device() to
> > cover all other devices as well.
> >
> > Something like below might do the job.
> >
> > diff --git a/drivers/acpi/glue.c b/drivers/acpi/glue.c
> > index 5ea5dc219f56..7cfd48f55b6f 100644
> > --- a/drivers/acpi/glue.c
> > +++ b/drivers/acpi/glue.c
> > @@ -85,7 +85,7 @@ static struct acpi_bus_type *acpi_get_bus_type(struct device *dev)
> >
> >  static int find_child_checks(struct acpi_device *adev, bool check_children)
> >  {
> > -       bool sta_present = true;
> > +       bool hid_present, sta_present = true;
> >         unsigned long long sta;
> >         acpi_status status;
> >
> > @@ -98,7 +98,22 @@ static int find_child_checks(struct acpi_device *adev, bool check_children)
> >         if (check_children && list_empty(&adev->children))
> >                 return -ENODEV;
> >
> > -       return sta_present ? FIND_CHILD_MAX_SCORE : FIND_CHILD_MIN_SCORE;
> > +       /*
> > +        * If there is a _HID or _CID present on a bus that is supposed to
> > +        * be matched using _ADR, we prioritize those devices without ID
> > +        * higher.
> > +        *
> > +        * This is because there are many BIOSes out there having ACPI
> > +        * enumerated devices and the _ADR is set to 0. This will match the
> > +        * root PCI bridge to the first found device with _ADR 0 which is
> > +        * not always the right device.
> > +        */
> > +       hid_present = !list_empty(&adev->pnp.ids);
> > +
> > +       if (sta_present)
> > +               return hid_present ? FIND_CHILD_MIN_SCORE : FIND_CHILD_MAX_SCORE;
> > +
> > +       return FIND_CHILD_MIN_SCORE;
> >  }
> >
> >  struct acpi_device *acpi_find_child_device(struct acpi_device *parent,
> >
> 
> I'd rather only change the return statement, like this:
> 
> return sta_present && list_empty(&adev->pnp.ids) ?
> FIND_CHILD_MAX_SCORE : FIND_CHILD_MIN_SCORE;
> 
> I'll send a patch tomorrow unless there's something wrong with this.

Looks good to me, thanks.

> BTW, the comment is not correct too, because the host bridge has a
> _HID.  What is matched against _ADR(0) is the first function on bus 0
> IIRC.

Yes, you are right.

[PATCH] ACPI / scan: Prefer devices without _HID/_CID for _ADR matching

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

The way acpi_find_child_device() works currently is that, if there
are two (or more) devices with the same _ADR value in the same
namespace scope (which is not specifically allowed by the spec and
the OS behavior in that case is not defined), the first one of them
found to be present (with the help of _STA) will be returned.

This covers the majority of cases, but is not sufficient if some of
the devices in question have a _HID (or _CID) returning some valid
ACPI/PNP device IDs (which is disallowed by the spec) and the
ASL writers' expectation appears to be that the OS will match
devices without a valid ACPI/PNP device ID against a given bus
address first.

To cover this special case as well, modify find_child_checks()
to prefer devices without ACPI/PNP device IDs over devices that
have them.

Suggested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

I'm not actually sure if this is sufficient to fix the original 80860F14 uid "2"
sd-controller problem on CherryTrail.  Hans, can you please check?

If it is not sufficient, we may need to fail find_child_checks() right away
for devices with non-empty pnp.ids lists.

Thanks,
Rafael

---
 drivers/acpi/glue.c |   10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

Index: linux-pm/drivers/acpi/glue.c
===================================================================
--- linux-pm.orig/drivers/acpi/glue.c
+++ linux-pm/drivers/acpi/glue.c
@@ -98,7 +98,15 @@ static int find_child_checks(struct acpi
 	if (check_children && list_empty(&adev->children))
 		return -ENODEV;
 
-	return sta_present ? FIND_CHILD_MAX_SCORE : FIND_CHILD_MIN_SCORE;
+	/*
+	 * If the device has a _HID (or _CID) returning a valid ACPI/PNP
+	 * device ID, it is better to make it look less attractive here, so that
+	 * the other device with the same _ADR value (that may not have a valid
+	 * device ID) can be matched going forward.  [This means a second spec
+	 * violation in a row, so whatever we do here is best effort anyway.]
+	 */
+	return sta_present && list_empty(&adev->pnp.ids) ?
+			FIND_CHILD_MAX_SCORE : FIND_CHILD_MIN_SCORE;
 }
 
 struct acpi_device *acpi_find_child_device(struct acpi_device *parent,

Re: [PATCH] ACPI / scan: Prefer devices without _HID/_CID for _ADR matching

[Hans de Goede]

Hi,

On 30-12-16 02:27, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> The way acpi_find_child_device() works currently is that, if there
> are two (or more) devices with the same _ADR value in the same
> namespace scope (which is not specifically allowed by the spec and
> the OS behavior in that case is not defined), the first one of them
> found to be present (with the help of _STA) will be returned.
>
> This covers the majority of cases, but is not sufficient if some of
> the devices in question have a _HID (or _CID) returning some valid
> ACPI/PNP device IDs (which is disallowed by the spec) and the
> ASL writers' expectation appears to be that the OS will match
> devices without a valid ACPI/PNP device ID against a given bus
> address first.
>
> To cover this special case as well, modify find_child_checks()
> to prefer devices without ACPI/PNP device IDs over devices that
> have them.
>
> Suggested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>
> I'm not actually sure if this is sufficient to fix the original 80860F14 uid "2"
> sd-controller problem on CherryTrail.  Hans, can you please check?

Ok, just booted a kernel with this patch replacing my own attempt
at fixing this, and the kernel still sees and initializes the
mmc controller in question correctly with this patch:

Tested-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans



> Thanks,
> Rafael
>
> ---
>  drivers/acpi/glue.c |   10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> Index: linux-pm/drivers/acpi/glue.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/glue.c
> +++ linux-pm/drivers/acpi/glue.c
> @@ -98,7 +98,15 @@ static int find_child_checks(struct acpi
>  	if (check_children && list_empty(&adev->children))
>  		return -ENODEV;
>
> -	return sta_present ? FIND_CHILD_MAX_SCORE : FIND_CHILD_MIN_SCORE;
> +	/*
> +	 * If the device has a _HID (or _CID) returning a valid ACPI/PNP
> +	 * device ID, it is better to make it look less attractive here, so that
> +	 * the other device with the same _ADR value (that may not have a valid
> +	 * device ID) can be matched going forward.  [This means a second spec
> +	 * violation in a row, so whatever we do here is best effort anyway.]
> +	 */
> +	return sta_present && list_empty(&adev->pnp.ids) ?
> +			FIND_CHILD_MAX_SCORE : FIND_CHILD_MIN_SCORE;
>  }
>
>  struct acpi_device *acpi_find_child_device(struct acpi_device *parent,
>

Re: [PATCH] ACPI / scan: Prefer devices without _HID/_CID for _ADR matching

[Rafael J. Wysocki]

On Sunday, January 01, 2017 09:30:06 PM Hans de Goede wrote:
> Hi,
> 
> On 30-12-16 02:27, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > The way acpi_find_child_device() works currently is that, if there
> > are two (or more) devices with the same _ADR value in the same
> > namespace scope (which is not specifically allowed by the spec and
> > the OS behavior in that case is not defined), the first one of them
> > found to be present (with the help of _STA) will be returned.
> >
> > This covers the majority of cases, but is not sufficient if some of
> > the devices in question have a _HID (or _CID) returning some valid
> > ACPI/PNP device IDs (which is disallowed by the spec) and the
> > ASL writers' expectation appears to be that the OS will match
> > devices without a valid ACPI/PNP device ID against a given bus
> > address first.
> >
> > To cover this special case as well, modify find_child_checks()
> > to prefer devices without ACPI/PNP device IDs over devices that
> > have them.
> >
> > Suggested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >
> > I'm not actually sure if this is sufficient to fix the original 80860F14 uid "2"
> > sd-controller problem on CherryTrail.  Hans, can you please check?
> 
> Ok, just booted a kernel with this patch replacing my own attempt
> at fixing this, and the kernel still sees and initializes the
> mmc controller in question correctly with this patch:
> 
> Tested-by: Hans de Goede <hdegoede@redhat.com>

Unfortunately, this breaks discrete graphics enumeration in

https://bugzilla.kernel.org/show_bug.cgi?id=194889

so can you please check if the patch below doesn't break the platform fixed by
the above?

Thanks,
Rafael


---
 drivers/acpi/glue.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Index: linux-pm/drivers/acpi/glue.c
===================================================================
--- linux-pm.orig/drivers/acpi/glue.c
+++ linux-pm/drivers/acpi/glue.c
@@ -99,13 +99,13 @@ static int find_child_checks(struct acpi
 		return -ENODEV;
 
 	/*
-	 * If the device has a _HID (or _CID) returning a valid ACPI/PNP
-	 * device ID, it is better to make it look less attractive here, so that
-	 * the other device with the same _ADR value (that may not have a valid
-	 * device ID) can be matched going forward.  [This means a second spec
-	 * violation in a row, so whatever we do here is best effort anyway.]
+	 * If the device has a _HID returning a valid ACPI/PNP device ID, it is
+	 * better to make it look less attractive here, so that the other device
+	 * with the same _ADR value (that may not have a valid device ID) can be
+	 * matched going forward.  [This means a second spec violation in a row,
+	 * so whatever we do here is best effort anyway.]
 	 */
-	return sta_present && list_empty(&adev->pnp.ids) ?
+	return sta_present && !adev->pnp.type.platform_id ?
 			FIND_CHILD_MAX_SCORE : FIND_CHILD_MIN_SCORE;
 }
 

Re: [PATCH] ACPI / scan: Prefer devices without _HID/_CID for _ADR matching

[Hans de Goede]

Hi,

On 30-03-17 22:56, Rafael J. Wysocki wrote:
> On Sunday, January 01, 2017 09:30:06 PM Hans de Goede wrote:
>> Hi,
>>
>> On 30-12-16 02:27, Rafael J. Wysocki wrote:
>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>
>>> The way acpi_find_child_device() works currently is that, if there
>>> are two (or more) devices with the same _ADR value in the same
>>> namespace scope (which is not specifically allowed by the spec and
>>> the OS behavior in that case is not defined), the first one of them
>>> found to be present (with the help of _STA) will be returned.
>>>
>>> This covers the majority of cases, but is not sufficient if some of
>>> the devices in question have a _HID (or _CID) returning some valid
>>> ACPI/PNP device IDs (which is disallowed by the spec) and the
>>> ASL writers' expectation appears to be that the OS will match
>>> devices without a valid ACPI/PNP device ID against a given bus
>>> address first.
>>>
>>> To cover this special case as well, modify find_child_checks()
>>> to prefer devices without ACPI/PNP device IDs over devices that
>>> have them.
>>>
>>> Suggested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
>>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>> ---
>>>
>>> I'm not actually sure if this is sufficient to fix the original 80860F14 uid "2"
>>> sd-controller problem on CherryTrail.  Hans, can you please check?
>>
>> Ok, just booted a kernel with this patch replacing my own attempt
>> at fixing this, and the kernel still sees and initializes the
>> mmc controller in question correctly with this patch:
>>
>> Tested-by: Hans de Goede <hdegoede@redhat.com>
>
> Unfortunately, this breaks discrete graphics enumeration in
>
> https://bugzilla.kernel.org/show_bug.cgi?id=194889
>
> so can you please check if the patch below doesn't break the platform fixed by
> the above?

I've just tried this and this patch does not regress the platform fixed
by the original commit.

Regards,

Hans


>
> Thanks,
> Rafael
>
>
> ---
>  drivers/acpi/glue.c |   12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> Index: linux-pm/drivers/acpi/glue.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/glue.c
> +++ linux-pm/drivers/acpi/glue.c
> @@ -99,13 +99,13 @@ static int find_child_checks(struct acpi
>  		return -ENODEV;
>
>  	/*
> -	 * If the device has a _HID (or _CID) returning a valid ACPI/PNP
> -	 * device ID, it is better to make it look less attractive here, so that
> -	 * the other device with the same _ADR value (that may not have a valid
> -	 * device ID) can be matched going forward.  [This means a second spec
> -	 * violation in a row, so whatever we do here is best effort anyway.]
> +	 * If the device has a _HID returning a valid ACPI/PNP device ID, it is
> +	 * better to make it look less attractive here, so that the other device
> +	 * with the same _ADR value (that may not have a valid device ID) can be
> +	 * matched going forward.  [This means a second spec violation in a row,
> +	 * so whatever we do here is best effort anyway.]
>  	 */
> -	return sta_present && list_empty(&adev->pnp.ids) ?
> +	return sta_present && !adev->pnp.type.platform_id ?
>  			FIND_CHILD_MAX_SCORE : FIND_CHILD_MIN_SCORE;
>  }
>
>

Re: [PATCH] ACPI / scan: Prefer devices without _HID/_CID for _ADR matching

[Rafael J. Wysocki]

On Friday, March 31, 2017 12:39:35 PM Hans de Goede wrote:
> Hi,
> 
> On 30-03-17 22:56, Rafael J. Wysocki wrote:
> > On Sunday, January 01, 2017 09:30:06 PM Hans de Goede wrote:
> >> Hi,
> >>
> >> On 30-12-16 02:27, Rafael J. Wysocki wrote:
> >>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >>>
> >>> The way acpi_find_child_device() works currently is that, if there
> >>> are two (or more) devices with the same _ADR value in the same
> >>> namespace scope (which is not specifically allowed by the spec and
> >>> the OS behavior in that case is not defined), the first one of them
> >>> found to be present (with the help of _STA) will be returned.
> >>>
> >>> This covers the majority of cases, but is not sufficient if some of
> >>> the devices in question have a _HID (or _CID) returning some valid
> >>> ACPI/PNP device IDs (which is disallowed by the spec) and the
> >>> ASL writers' expectation appears to be that the OS will match
> >>> devices without a valid ACPI/PNP device ID against a given bus
> >>> address first.
> >>>
> >>> To cover this special case as well, modify find_child_checks()
> >>> to prefer devices without ACPI/PNP device IDs over devices that
> >>> have them.
> >>>
> >>> Suggested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> >>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >>> ---
> >>>
> >>> I'm not actually sure if this is sufficient to fix the original 80860F14 uid "2"
> >>> sd-controller problem on CherryTrail.  Hans, can you please check?
> >>
> >> Ok, just booted a kernel with this patch replacing my own attempt
> >> at fixing this, and the kernel still sees and initializes the
> >> mmc controller in question correctly with this patch:
> >>
> >> Tested-by: Hans de Goede <hdegoede@redhat.com>
> >
> > Unfortunately, this breaks discrete graphics enumeration in
> >
> > https://bugzilla.kernel.org/show_bug.cgi?id=194889
> >
> > so can you please check if the patch below doesn't break the platform fixed by
> > the above?
> 
> I've just tried this and this patch does not regress the platform fixed
> by the original commit.

OK, thanks!

Re: [PATCH] ACPI / scan: Prefer devices without _HID/_CID for _ADR matching

[Mika Westerberg]

On Fri, Dec 30, 2016 at 02:27:31AM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> The way acpi_find_child_device() works currently is that, if there
> are two (or more) devices with the same _ADR value in the same
> namespace scope (which is not specifically allowed by the spec and
> the OS behavior in that case is not defined), the first one of them
> found to be present (with the help of _STA) will be returned.
> 
> This covers the majority of cases, but is not sufficient if some of
> the devices in question have a _HID (or _CID) returning some valid
> ACPI/PNP device IDs (which is disallowed by the spec) and the
> ASL writers' expectation appears to be that the OS will match
> devices without a valid ACPI/PNP device ID against a given bus
> address first.
> 
> To cover this special case as well, modify find_child_checks()
> to prefer devices without ACPI/PNP device IDs over devices that
> have them.
> 
> Suggested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Thanks for taking care of this Rafael :)