[PATCH] ACPI / LPSS: Only call pwm_add_table for the first PWM controller

[PATCH] ACPI / LPSS: Only call pwm_add_table for the first PWM controller

[Hans de Goede]

At least on the UP board SBC both PWMs are enabled leading to us
trying to add the same pwm_lookup twice, which leads to the following:

[    0.902224] list_add double add: new=ffffffffb8efd400,
               prev=ffffffffb8efd400, next=ffffffffb8eeede0.
[    0.912466] ------------[ cut here ]------------
[    0.917624] kernel BUG at lib/list_debug.c:31!
[    0.922588] invalid opcode: 0000 [#1] SMP
...
[    1.027450] Call Trace:
[    1.030185]  pwm_add_table+0x4c/0x90
[    1.034181]  bsw_pwm_setup+0x1a/0x20
[    1.038175]  acpi_lpss_create_device+0xfe/0x420
...

This commit fixes this by only calling pwm_add_table for the first
PWM controller (which is the one used for the backlight).

Cc: stable@vger.kernel.org
BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1458599
Fixes: bf7696a12071 ("acpi: lpss: call pwm_add_table() for BSW...")
Fixes: 04434ab5120a ("ACPI / LPSS: Call pwm_add_table() for Bay Trail...")
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/acpi/acpi_lpss.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
index 10347e3d73ad..5bd58bd4ab05 100644
--- a/drivers/acpi/acpi_lpss.c
+++ b/drivers/acpi/acpi_lpss.c
@@ -85,6 +85,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;
@@ -155,6 +156,12 @@ static struct pwm_lookup byt_pwm_lookup[] = {
 
 static void byt_pwm_setup(struct lpss_private_data *pdata)
 {
+	struct acpi_device *adev = pdata->adev;
+
+	/* Only call pwm_add_table for the first PWM controller */
+	if (!adev->pnp.unique_id || strcmp(adev->pnp.unique_id, "1"))
+		return;
+
 	if (!acpi_dev_present("INT33FD", NULL, -1))
 		pwm_add_table(byt_pwm_lookup, ARRAY_SIZE(byt_pwm_lookup));
 }
@@ -180,6 +187,12 @@ static struct pwm_lookup bsw_pwm_lookup[] = {
 
 static void bsw_pwm_setup(struct lpss_private_data *pdata)
 {
+	struct acpi_device *adev = pdata->adev;
+
+	/* Only call pwm_add_table for the first PWM controller */
+	if (!adev->pnp.unique_id || strcmp(adev->pnp.unique_id, "1"))
+		return;
+
 	pwm_add_table(bsw_pwm_lookup, ARRAY_SIZE(bsw_pwm_lookup));
 }
 
@@ -456,6 +469,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.13.0

Re: [PATCH] ACPI / LPSS: Only call pwm_add_table for the first PWM controller

[Andy Shevchenko]

On Thu, 2017-07-06 at 18:49 +0200, Hans de Goede wrote:
> At least on the UP board SBC both PWMs are enabled leading to us
> trying to add the same pwm_lookup twice, which leads to the following:
> 
> [    0.902224] list_add double add: new=ffffffffb8efd400,
>                prev=ffffffffb8efd400, next=ffffffffb8eeede0.
> [    0.912466] ------------[ cut here ]------------
> [    0.917624] kernel BUG at lib/list_debug.c:31!
> [    0.922588] invalid opcode: 0000 [#1] SMP
> ...
> [    1.027450] Call Trace:
> [    1.030185]  pwm_add_table+0x4c/0x90
> [    1.034181]  bsw_pwm_setup+0x1a/0x20
> [    1.038175]  acpi_lpss_create_device+0xfe/0x420
> ...
> 
> This commit fixes this by only calling pwm_add_table for the first
> PWM controller (which is the one used for the backlight).
> 

Thanks, my comment below.

For the quick fix I agree on this:
Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

By the way, do you need a shell script that allows to setup pin muxing
via external CPLD?

> Cc: stable@vger.kernel.org
> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1458599
> Fixes: bf7696a12071 ("acpi: lpss: call pwm_add_table() for BSW...")
> Fixes: 04434ab5120a ("ACPI / LPSS: Call pwm_add_table() for Bay
> Trail...")
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/acpi/acpi_lpss.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
> index 10347e3d73ad..5bd58bd4ab05 100644
> --- a/drivers/acpi/acpi_lpss.c
> +++ b/drivers/acpi/acpi_lpss.c
> @@ -85,6 +85,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;
> @@ -155,6 +156,12 @@ static struct pwm_lookup byt_pwm_lookup[] = {
>  
>  static void byt_pwm_setup(struct lpss_private_data *pdata)
>  {
> +	struct acpi_device *adev = pdata->adev;
> +
> +	/* Only call pwm_add_table for the first PWM controller */
> +	if (!adev->pnp.unique_id || strcmp(adev->pnp.unique_id, "1"))
> +		return;
> +

It would be nice to have a separate mapping between UID and lookup
table.

Though, for now it's only one case, perhaps we may do this later.

>  	if (!acpi_dev_present("INT33FD", NULL, -1))
>  		pwm_add_table(byt_pwm_lookup,
> ARRAY_SIZE(byt_pwm_lookup));
>  }
> @@ -180,6 +187,12 @@ static struct pwm_lookup bsw_pwm_lookup[] = {
>  
>  static void bsw_pwm_setup(struct lpss_private_data *pdata)
>  {
> +	struct acpi_device *adev = pdata->adev;
> +
> +	/* Only call pwm_add_table for the first PWM controller */
> +	if (!adev->pnp.unique_id || strcmp(adev->pnp.unique_id, "1"))
> +		return;
> +
>  	pwm_add_table(bsw_pwm_lookup, ARRAY_SIZE(bsw_pwm_lookup));
>  }
>  
> @@ -456,6 +469,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)

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

Re: [PATCH] ACPI / LPSS: Only call pwm_add_table for the first PWM controller

[Hans de Goede]

Hi,

On 06-07-17 19:11, Andy Shevchenko wrote:
> On Thu, 2017-07-06 at 18:49 +0200, Hans de Goede wrote:
>> At least on the UP board SBC both PWMs are enabled leading to us
>> trying to add the same pwm_lookup twice, which leads to the following:
>>
>> [    0.902224] list_add double add: new=ffffffffb8efd400,
>>                 prev=ffffffffb8efd400, next=ffffffffb8eeede0.
>> [    0.912466] ------------[ cut here ]------------
>> [    0.917624] kernel BUG at lib/list_debug.c:31!
>> [    0.922588] invalid opcode: 0000 [#1] SMP
>> ...
>> [    1.027450] Call Trace:
>> [    1.030185]  pwm_add_table+0x4c/0x90
>> [    1.034181]  bsw_pwm_setup+0x1a/0x20
>> [    1.038175]  acpi_lpss_create_device+0xfe/0x420
>> ...
>>
>> This commit fixes this by only calling pwm_add_table for the first
>> PWM controller (which is the one used for the backlight).
>>
> 
> Thanks, my comment below.
> 
> For the quick fix I agree on this:
> Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Thank you.

Rafael, can we get this added to 4.13 soonish please ? One of the 2
commits this fixes has been causing trouble for some users since 4.11.3.

> By the way, do you need a shell script that allows to setup pin muxing
> via external CPLD?

Thanks, but no thanks I don't want to make physical alterations to
my boards.

>> Cc: stable@vger.kernel.org
>> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1458599
>> Fixes: bf7696a12071 ("acpi: lpss: call pwm_add_table() for BSW...")
>> Fixes: 04434ab5120a ("ACPI / LPSS: Call pwm_add_table() for Bay
>> Trail...")
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/acpi/acpi_lpss.c | 14 ++++++++++++++
>>   1 file changed, 14 insertions(+)
>>
>> diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
>> index 10347e3d73ad..5bd58bd4ab05 100644
>> --- a/drivers/acpi/acpi_lpss.c
>> +++ b/drivers/acpi/acpi_lpss.c
>> @@ -85,6 +85,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;
>> @@ -155,6 +156,12 @@ static struct pwm_lookup byt_pwm_lookup[] = {
>>   
>>   static void byt_pwm_setup(struct lpss_private_data *pdata)
>>   {
>> +	struct acpi_device *adev = pdata->adev;
>> +
>> +	/* Only call pwm_add_table for the first PWM controller */
>> +	if (!adev->pnp.unique_id || strcmp(adev->pnp.unique_id, "1"))
>> +		return;
>> +
> 
> It would be nice to have a separate mapping between UID and lookup
> table.

If we ever have the need for mappings for more then the first
PWM controller, then yes we should add something like that.

Regards,

Hans



> 
> Though, for now it's only one case, perhaps we may do this later.
> 
>>   	if (!acpi_dev_present("INT33FD", NULL, -1))
>>   		pwm_add_table(byt_pwm_lookup,
>> ARRAY_SIZE(byt_pwm_lookup));
>>   }
>> @@ -180,6 +187,12 @@ static struct pwm_lookup bsw_pwm_lookup[] = {
>>   
>>   static void bsw_pwm_setup(struct lpss_private_data *pdata)
>>   {
>> +	struct acpi_device *adev = pdata->adev;
>> +
>> +	/* Only call pwm_add_table for the first PWM controller */
>> +	if (!adev->pnp.unique_id || strcmp(adev->pnp.unique_id, "1"))
>> +		return;
>> +
>>   	pwm_add_table(bsw_pwm_lookup, ARRAY_SIZE(bsw_pwm_lookup));
>>   }
>>   
>> @@ -456,6 +469,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)
> 

Re: [PATCH] ACPI / LPSS: Only call pwm_add_table for the first PWM controller

[Andy Shevchenko]

On Thu, 2017-07-06 at 20:08 +0200, Hans de Goede wrote:
> On 06-07-17 19:11, Andy Shevchenko wrote:
> > On Thu, 2017-07-06 at 18:49 +0200, Hans de Goede wrote:
> > > 

> > By the way, do you need a shell script that allows to setup pin
> > muxing
> > via external CPLD?
> 
> Thanks, but no thanks I don't want to make physical alterations to
> my boards.

I surprised it works for you by default. I have UP board (BIOS v0.4)
which doesn't provide any useful output since CPLD is configured by BIOS
to most safe state.

But okay, feel free to ask when you need it. Perhaps at some point I
just submit it to Github as a gist.

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

Re: [PATCH] ACPI / LPSS: Only call pwm_add_table for the first PWM controller

[Hans de Goede]

Hi,

On 06-07-17 20:19, Andy Shevchenko wrote:
> On Thu, 2017-07-06 at 20:08 +0200, Hans de Goede wrote:
>> On 06-07-17 19:11, Andy Shevchenko wrote:
>>> On Thu, 2017-07-06 at 18:49 +0200, Hans de Goede wrote:
>>>>
> 
>>> By the way, do you need a shell script that allows to setup pin
>>> muxing
>>> via external CPLD?
>>
>> Thanks, but no thanks I don't want to make physical alterations to
>> my boards.
> 
> I surprised it works for you by default. I have UP board (BIOS v0.4)
> which doesn't provide any useful output since CPLD is configured by BIOS
> to most safe state.

Ah, I did not understand this script is specific to the UP,
I don't have a UP, the reporter of the problem does :)

Regards,

Hans

Re: [PATCH] ACPI / LPSS: Only call pwm_add_table for the first PWM controller

[Rafael J. Wysocki]

On Thursday, July 06, 2017 08:08:03 PM Hans de Goede wrote:
> Hi,
> 
> On 06-07-17 19:11, Andy Shevchenko wrote:
> > On Thu, 2017-07-06 at 18:49 +0200, Hans de Goede wrote:
> >> At least on the UP board SBC both PWMs are enabled leading to us
> >> trying to add the same pwm_lookup twice, which leads to the following:
> >>
> >> [    0.902224] list_add double add: new=ffffffffb8efd400,
> >>                 prev=ffffffffb8efd400, next=ffffffffb8eeede0.
> >> [    0.912466] ------------[ cut here ]------------
> >> [    0.917624] kernel BUG at lib/list_debug.c:31!
> >> [    0.922588] invalid opcode: 0000 [#1] SMP
> >> ...
> >> [    1.027450] Call Trace:
> >> [    1.030185]  pwm_add_table+0x4c/0x90
> >> [    1.034181]  bsw_pwm_setup+0x1a/0x20
> >> [    1.038175]  acpi_lpss_create_device+0xfe/0x420
> >> ...
> >>
> >> This commit fixes this by only calling pwm_add_table for the first
> >> PWM controller (which is the one used for the backlight).
> >>
> > 
> > Thanks, my comment below.
> > 
> > For the quick fix I agree on this:
> > Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> Thank you.
> 
> Rafael, can we get this added to 4.13 soonish please ? One of the 2
> commits this fixes has been causing trouble for some users since 4.11.3.

Should be possible.

Thanks,
Rafael