| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20200819101539.GE4354@dell>
Date: Wed, 19 Aug 2020 11:15:39 +0100
From: Lee Jones <lee.jones@...aro.org>
To: Michael Brunner <Michael.Brunner@...tron.com>
Cc: "linux@...ck-us.net" <linux@...ck-us.net>,
"sameo@...ux.intel.com" <sameo@...ux.intel.com>,
"dvhart@...ux.intel.com" <dvhart@...ux.intel.com>,
"mibru@....de" <mibru@....de>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"kevin.strasser@...ux.intel.com" <kevin.strasser@...ux.intel.com>
Subject: Re: [PATCH] mfd: Add ACPI support to Kontron PLD driver
On Wed, 12 Aug 2020, Michael Brunner wrote:
> Recent Kontron COMe modules identify the PLD device using the hardware
> id KEM0001 in the ACPI table.
> This patch adds support for probing the device using the HID and also
> retrieving the resources.
>
> As this is not available for all products, the DMI based detection still
> needs to be around for older systems. It is executed if no matching ACPI
> HID is found during registering the platform driver or no specific
> device id is forced.
> If a device is detected using ACPI and no resource information is
> available, the default io resource is used.
>
> Forcing a device id with the force_device_id parameter and therefore
> manually generating a platform device takes precedence over ACPI during
> probing.
>
> Signed-off-by: Michael Brunner <michael.brunner@...tron.com>
> ---
> drivers/mfd/kempld-core.c | 97 ++++++++++++++++++++++++++++++++++++---
> 1 file changed, 91 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/mfd/kempld-core.c b/drivers/mfd/kempld-core.c
> index f48e21d8b97c..408cad1958d9 100644
> --- a/drivers/mfd/kempld-core.c
> +++ b/drivers/mfd/kempld-core.c
> @@ -13,6 +13,7 @@
> #include <linux/dmi.h>
> #include <linux/io.h>
> #include <linux/delay.h>
> +#include <linux/acpi.h>
>
> #define MAX_ID_LEN 4
> static char force_device_id[MAX_ID_LEN + 1] = "";
> @@ -132,6 +133,7 @@ static const struct kempld_platform_data kempld_platform_data_generic = {
> };
>
> static struct platform_device *kempld_pdev;
> +static bool kempld_acpi_mode;
>
> static int kempld_create_platform_device(const struct dmi_system_id *id)
> {
> @@ -434,13 +436,87 @@ static int kempld_detect_device(struct kempld_device_data *pld)
> return ret;
> }
>
> +#ifdef CONFIG_ACPI
Not keen on #ifdefery if at all avoidable.
Can you use if (IS_ENABLED(CONFIG_ACPI)) at the call-site instead?
The compiler should take care of the rest, no?
> +static const struct acpi_device_id kempld_acpi_table[] = {
> + { "KEM0001", (kernel_ulong_t)&kempld_platform_data_generic },
> + {}
> +};
> +MODULE_DEVICE_TABLE(acpi, kempld_acpi_table);
I'd prefer if this was moved down to just above where it's used
i.e. where we usually place the of_device_id tables.
> +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;
}
Then drop the next check and pull the indented code back:
> + if (count > 0) {
> + resources = devm_kcalloc(&acpi_dev->dev, count,
> + sizeof(struct resource), GFP_KERNEL);
sizeof(*resources) is preferred.
> + 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);
> + if (ret)
> + goto out;
> + } else
> + ret = platform_device_add_resources(pdev, pdata->ioresource, 1);
> +
> +out:
> + acpi_dev_free_resource_list(&resource_list);
> +
> + return ret;
> +}
> +#else
> +static int kempld_get_acpi_data(struct platform_device *pdev)
> +{
> + return -ENODEV;
> +}
> +#endif /* CONFIG_ACPI */
> +
> static int kempld_probe(struct platform_device *pdev)
> {
> - const struct kempld_platform_data *pdata =
> - dev_get_platdata(&pdev->dev);
> + const struct kempld_platform_data *pdata;
> struct device *dev = &pdev->dev;
> struct kempld_device_data *pld;
> struct resource *ioport;
> + int ret;
> +
> + if (kempld_pdev == NULL) {
Comment please. What does !kempld_pdev actually imply?
> + ret = kempld_get_acpi_data(pdev);
> + if (ret < 0)
> + return ret;
Is 'ret > 0' valid?
If not, then just 'if (ret)'.
> + kempld_acpi_mode = true;
> + } else if (kempld_pdev != pdev) {
> + dev_notice(dev, "platform device exists - not using ACPI\n");
Why dev_notice() and not dev_err()?
Is that what 'kempld_pdev != pdev' means?
Could you explain this to me in more depth please?
> + return -ENODEV;
> + }
> + pdata = dev_get_platdata(dev);
>
> pld = devm_kzalloc(dev, sizeof(*pld), GFP_KERNEL);
> if (!pld)
> @@ -482,6 +558,7 @@ static int kempld_remove(struct platform_device *pdev)
> static struct platform_driver kempld_driver = {
> .driver = {
> .name = "kempld",
> + .acpi_match_table = ACPI_PTR(kempld_acpi_table),
> },
> .probe = kempld_probe,
> .remove = kempld_remove,
> @@ -800,6 +877,7 @@ MODULE_DEVICE_TABLE(dmi, kempld_dmi_table);
> static int __init kempld_init(void)
> {
> const struct dmi_system_id *id;
> + int ret;
>
> if (force_device_id[0]) {
> for (id = kempld_dmi_table;
> @@ -809,12 +887,19 @@ static int __init kempld_init(void)
> break;
> if (id->matches[0].slot == DMI_NONE)
> return -ENODEV;
> - } else {
> - if (!dmi_check_system(kempld_dmi_table))
> - return -ENODEV;
> }
>
> - return platform_driver_register(&kempld_driver);
> + ret = platform_driver_register(&kempld_driver);
> + if (ret)
> + return ret;
Is it guaranteed that the child device has probed at this point?
> + if (!kempld_pdev && !kempld_acpi_mode)
Again, comment please. What has gone on to get to this point?
> + if (!dmi_check_system(kempld_dmi_table)) {
> + platform_driver_unregister(&kempld_driver);
> + return -ENODEV;
> + }
> +
> + return 0;
> }
>
> static void __exit kempld_exit(void)
--
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
Powered by blists - more mailing lists