On 2/21/24 09:10, Michael Brunner wrote:
> The current implementation to retrieve ACPI resources is faulty
> and may cause issues that even can lead to non-booting systems.
>
> When adding data from an ACPI device, the resources are already
> assigned to the platform device. Therefore there is no need to
> retrieve the resource list from ACPI and manually assign it to
> the platform device. Also there shouldn't be any BIOS in the wild
> anymore, that does not have resources added to the KEMPLD ACPI
> data.
>
> In particular this fixes an issue where the retrieval of the
> resource list using /proc/ioports is disturbed and does not list
> the assigned resource for the kempld device or even no resources
> at all.
> On some distributions this also leads to problems during system
> initialization (e.g. with udev) and causes the system to not
> boot at all.
>
> I have reproduced the issue with the following kernel versions:
> 5.10.209
> 5.15.148
> 6.1.25
> 6.6.17
> 6.7.5
> 6.8-rc5
>
> The patch applies to all of those versions and seems to resolve
> the issue.
>
> Signed-off-by: Michael Brunner <[email protected]>
Reviewed-by: Guenter Roeck <[email protected]>
> ---
> drivers/mfd/kempld-core.c | 37 -------------------------------------
> 1 file changed, 37 deletions(-)
>
> diff --git a/drivers/mfd/kempld-core.c b/drivers/mfd/kempld-core.c
> index 67af36a38913..5557f023a173 100644
> --- a/drivers/mfd/kempld-core.c
> +++ b/drivers/mfd/kempld-core.c
> @@ -428,50 +428,13 @@ static int kempld_detect_device(struct kempld_device_data *pld)
> #ifdef CONFIG_ACPI
> static int kempld_get_acpi_data(struct platform_device *pdev)
> {
> - struct list_head resource_list;
> - struct resource *resources;
> - struct resource_entry *rentry;
> struct device *dev = &pdev->dev;
> - struct acpi_device *acpi_dev = ACPI_COMPANION(dev);
> const struct kempld_platform_data *pdata;
> int ret;
> - int count;
>
> pdata = acpi_device_get_match_data(dev);
> ret = platform_device_add_data(pdev, pdata,
> sizeof(struct kempld_platform_data));
> - if (ret)
> - return ret;
> -
> - INIT_LIST_HEAD(&resource_list);
> - ret = acpi_dev_get_resources(acpi_dev, &resource_list, NULL, NULL);
> - if (ret < 0)
> - goto out;
> -
> - count = ret;
> -
> - if (count == 0) {
> - ret = platform_device_add_resources(pdev, pdata->ioresource, 1);
> - goto out;
> - }
> -
> - resources = devm_kcalloc(&acpi_dev->dev, count, sizeof(*resources),
> - GFP_KERNEL);
> - if (!resources) {
> - ret = -ENOMEM;
> - goto out;
> - }
> -
> - count = 0;
> - list_for_each_entry(rentry, &resource_list, node) {
> - memcpy(&resources[count], rentry->res,
> - sizeof(*resources));
> - count++;
> - }
> - ret = platform_device_add_resources(pdev, resources, count);
> -
> -out:
> - acpi_dev_free_resource_list(&resource_list);
>
> return ret;
> }
Added Lee Jones again, as I erroneously used an old eMail address in
my original mail.
Thanks Guenter, for the review!
On Wed, 2024-02-21 at 09:34 -0800, Guenter Roeck wrote:
>> The current implementation to retrieve ACPI resources is faulty
>> and may cause issues that even can lead to non-booting systems.
>>
>> When adding data from an ACPI device, the resources are already
>> assigned to the platform device. Therefore there is no need to
>> retrieve the resource list from ACPI and manually assign it to
>> the platform device. Also there shouldn't be any BIOS in the wild
>> anymore, that does not have resources added to the KEMPLD ACPI
>> data.
>>
>> In particular this fixes an issue where the retrieval of the
>> resource list using /proc/ioports is disturbed and does not list
>> the assigned resource for the kempld device or even no resources
>> at all.
>> On some distributions this also leads to problems during system
>> initialization (e.g. with udev) and causes the system to not
>> boot at all.
>>
>> I have reproduced the issue with the following kernel versions:
>> 5.10.209
>> 5.15.148
>> 6.1.25
>> 6.6.17
>> 6.7.5
>> 6.8-rc5
>>
>> The patch applies to all of those versions and seems to resolve
>> the issue.
>>
>> Signed-off-by: Michael Brunner <[email protected]>
>
>Reviewed-by: Guenter Roeck <[email protected]>
>
>> ---
>> drivers/mfd/kempld-core.c | 37 -------------------------------------
>> 1 file changed, 37 deletions(-)
>>
>> diff --git a/drivers/mfd/kempld-core.c b/drivers/mfd/kempld-core.c
>> index 67af36a38913..5557f023a173 100644
>> --- a/drivers/mfd/kempld-core.c
>> +++ b/drivers/mfd/kempld-core.c
>> @@ -428,50 +428,13 @@ static int kempld_detect_device(struct kempld_device_data *pld)
>> #ifdef CONFIG_ACPI
>> static int kempld_get_acpi_data(struct platform_device *pdev)
>> {
>> - struct list_head resource_list;
>> - struct resource *resources;
>> - struct resource_entry *rentry;
>> struct device *dev = &pdev->dev;
>> - struct acpi_device *acpi_dev = ACPI_COMPANION(dev);
>> const struct kempld_platform_data *pdata;
>> int ret;
>> - int count;
>>
>> pdata = acpi_device_get_match_data(dev);
>> ret = platform_device_add_data(pdev, pdata,
>> sizeof(struct kempld_platform_data));
>> - if (ret)
>> - return ret;
>> -
>> - INIT_LIST_HEAD(&resource_list);
>> - ret = acpi_dev_get_resources(acpi_dev, &resource_list, NULL, NULL);
>> - if (ret < 0)
>> - goto out;
>> -
>> - count = ret;
>> -
>> - if (count == 0) {
>> - ret = platform_device_add_resources(pdev, pdata->ioresource, 1);
>> - goto out;
>> - }
>> -
>> - resources = devm_kcalloc(&acpi_dev->dev, count, sizeof(*resources),
>> - GFP_KERNEL);
>> - if (!resources) {
>> - ret = -ENOMEM;
>> - goto out;
>> - }
>> -
>> - count = 0;
>> - list_for_each_entry(rentry, &resource_list, node) {
>> - memcpy(&resources[count], rentry->res,
>> - sizeof(*resources));
>> - count++;
>> - }
>> - ret = platform_device_add_resources(pdev, resources, count);
>> -
>> -out:
>> - acpi_dev_free_resource_list(&resource_list);
>>
>> return ret;
>> }