* [PATCH] platform/x86: intel_pmc_core: do not create a static struct device
@ 2020-09-23 18:48 Greg Kroah-Hartman
2020-09-23 19:37 ` Rajat Jain
2020-09-24 8:39 ` Hans de Goede
0 siblings, 2 replies; 3+ messages in thread
From: Greg Kroah-Hartman @ 2020-09-23 18:48 UTC (permalink / raw)
To: rajneesh.bhardwaj, vishwanath.somayaji
Cc: platform-driver-x86, linux-kernel, Greg Kroah-Hartman,
Darren Hart, Andy Shevchenko, Rajat Jain, Maximilian Luz
A struct device is a dynamic structure, with reference counting.
"Tricking" the kernel to make a dynamic structure static, by working
around the driver core release detection logic, is not nice.
Because of this, this code has been used as an example for others on
"how to do things", which is just about the worst thing possible to have
happen.
Fix this all up by making the platform device dynamic and providing a
real release function.
Cc: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com>
Cc: Vishwanath Somayaji <vishwanath.somayaji@intel.com>
Cc: Darren Hart <dvhart@infradead.org>
Cc: Andy Shevchenko <andy@infradead.org>
Cc: Rajat Jain <rajatja@google.com>
Cc: platform-driver-x86@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Reported-by: Maximilian Luz <luzmaximilian@gmail.com>
Fixes: b02f6a2ef0a1 ("platform/x86: intel_pmc_core: Attach using APCI HID "INT33A1"")
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
drivers/platform/x86/intel_pmc_core_pltdrv.c | 26 +++++++++++++-------
1 file changed, 17 insertions(+), 9 deletions(-)
diff --git a/drivers/platform/x86/intel_pmc_core_pltdrv.c b/drivers/platform/x86/intel_pmc_core_pltdrv.c
index 731281855cc8..73797680b895 100644
--- a/drivers/platform/x86/intel_pmc_core_pltdrv.c
+++ b/drivers/platform/x86/intel_pmc_core_pltdrv.c
@@ -20,15 +20,10 @@
static void intel_pmc_core_release(struct device *dev)
{
- /* Nothing to do. */
+ kfree(dev);
}
-static struct platform_device pmc_core_device = {
- .name = "intel_pmc_core",
- .dev = {
- .release = intel_pmc_core_release,
- },
-};
+static struct platform_device *pmc_core_device;
/*
* intel_pmc_core_platform_ids is the list of platforms where we want to
@@ -52,6 +47,8 @@ MODULE_DEVICE_TABLE(x86cpu, intel_pmc_core_platform_ids);
static int __init pmc_core_platform_init(void)
{
+ int retval;
+
/* Skip creating the platform device if ACPI already has a device */
if (acpi_dev_present("INT33A1", NULL, -1))
return -ENODEV;
@@ -59,12 +56,23 @@ static int __init pmc_core_platform_init(void)
if (!x86_match_cpu(intel_pmc_core_platform_ids))
return -ENODEV;
- return platform_device_register(&pmc_core_device);
+ pmc_core_device = kzalloc(sizeof(*pmc_core_device), GFP_KERNEL);
+ if (!pmc_core_device)
+ return -ENOMEM;
+
+ pmc_core_device->name = "intel_pmc_core";
+ pmc_core_device->dev.release = intel_pmc_core_release;
+
+ retval = platform_device_register(pmc_core_device);
+ if (retval)
+ kfree(pmc_core_device);
+
+ return retval;
}
static void __exit pmc_core_platform_exit(void)
{
- platform_device_unregister(&pmc_core_device);
+ platform_device_unregister(pmc_core_device);
}
module_init(pmc_core_platform_init);
--
2.28.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] platform/x86: intel_pmc_core: do not create a static struct device
2020-09-23 18:48 [PATCH] platform/x86: intel_pmc_core: do not create a static struct device Greg Kroah-Hartman
@ 2020-09-23 19:37 ` Rajat Jain
2020-09-24 8:39 ` Hans de Goede
1 sibling, 0 replies; 3+ messages in thread
From: Rajat Jain @ 2020-09-23 19:37 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Bhardwaj, Rajneesh, Vishwanath Somayaji, Platform Driver,
Linux Kernel Mailing List, Darren Hart, Andy Shevchenko,
Maximilian Luz
On Wed, Sep 23, 2020 at 11:47 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> A struct device is a dynamic structure, with reference counting.
> "Tricking" the kernel to make a dynamic structure static, by working
> around the driver core release detection logic, is not nice.
>
> Because of this, this code has been used as an example for others on
> "how to do things", which is just about the worst thing possible to have
> happen.
>
> Fix this all up by making the platform device dynamic and providing a
> real release function.
>
> Cc: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com>
> Cc: Vishwanath Somayaji <vishwanath.somayaji@intel.com>
> Cc: Darren Hart <dvhart@infradead.org>
> Cc: Andy Shevchenko <andy@infradead.org>
> Cc: Rajat Jain <rajatja@google.com>
> Cc: platform-driver-x86@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Reported-by: Maximilian Luz <luzmaximilian@gmail.com>
> Fixes: b02f6a2ef0a1 ("platform/x86: intel_pmc_core: Attach using APCI HID "INT33A1"")
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Acked-by: Rajat Jain <rajatja@google.com>
> ---
> drivers/platform/x86/intel_pmc_core_pltdrv.c | 26 +++++++++++++-------
> 1 file changed, 17 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/platform/x86/intel_pmc_core_pltdrv.c b/drivers/platform/x86/intel_pmc_core_pltdrv.c
> index 731281855cc8..73797680b895 100644
> --- a/drivers/platform/x86/intel_pmc_core_pltdrv.c
> +++ b/drivers/platform/x86/intel_pmc_core_pltdrv.c
> @@ -20,15 +20,10 @@
>
> static void intel_pmc_core_release(struct device *dev)
> {
> - /* Nothing to do. */
> + kfree(dev);
> }
>
> -static struct platform_device pmc_core_device = {
> - .name = "intel_pmc_core",
> - .dev = {
> - .release = intel_pmc_core_release,
> - },
> -};
> +static struct platform_device *pmc_core_device;
>
> /*
> * intel_pmc_core_platform_ids is the list of platforms where we want to
> @@ -52,6 +47,8 @@ MODULE_DEVICE_TABLE(x86cpu, intel_pmc_core_platform_ids);
>
> static int __init pmc_core_platform_init(void)
> {
> + int retval;
> +
> /* Skip creating the platform device if ACPI already has a device */
> if (acpi_dev_present("INT33A1", NULL, -1))
> return -ENODEV;
> @@ -59,12 +56,23 @@ static int __init pmc_core_platform_init(void)
> if (!x86_match_cpu(intel_pmc_core_platform_ids))
> return -ENODEV;
>
> - return platform_device_register(&pmc_core_device);
> + pmc_core_device = kzalloc(sizeof(*pmc_core_device), GFP_KERNEL);
> + if (!pmc_core_device)
> + return -ENOMEM;
> +
> + pmc_core_device->name = "intel_pmc_core";
> + pmc_core_device->dev.release = intel_pmc_core_release;
> +
> + retval = platform_device_register(pmc_core_device);
> + if (retval)
> + kfree(pmc_core_device);
> +
> + return retval;
> }
>
> static void __exit pmc_core_platform_exit(void)
> {
> - platform_device_unregister(&pmc_core_device);
> + platform_device_unregister(pmc_core_device);
> }
>
> module_init(pmc_core_platform_init);
> --
> 2.28.0
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] platform/x86: intel_pmc_core: do not create a static struct device
2020-09-23 18:48 [PATCH] platform/x86: intel_pmc_core: do not create a static struct device Greg Kroah-Hartman
2020-09-23 19:37 ` Rajat Jain
@ 2020-09-24 8:39 ` Hans de Goede
1 sibling, 0 replies; 3+ messages in thread
From: Hans de Goede @ 2020-09-24 8:39 UTC (permalink / raw)
To: Greg Kroah-Hartman, rajneesh.bhardwaj, vishwanath.somayaji
Cc: platform-driver-x86, linux-kernel, Darren Hart, Andy Shevchenko,
Rajat Jain, Maximilian Luz
Hi,
On 9/23/20 8:48 PM, Greg Kroah-Hartman wrote:
> A struct device is a dynamic structure, with reference counting.
> "Tricking" the kernel to make a dynamic structure static, by working
> around the driver core release detection logic, is not nice.
>
> Because of this, this code has been used as an example for others on
> "how to do things", which is just about the worst thing possible to have
> happen.
>
> Fix this all up by making the platform device dynamic and providing a
> real release function.
>
> Cc: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com>
> Cc: Vishwanath Somayaji <vishwanath.somayaji@intel.com>
> Cc: Darren Hart <dvhart@infradead.org>
> Cc: Andy Shevchenko <andy@infradead.org>
> Cc: Rajat Jain <rajatja@google.com>
> Cc: platform-driver-x86@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Reported-by: Maximilian Luz <luzmaximilian@gmail.com>
> Fixes: b02f6a2ef0a1 ("platform/x86: intel_pmc_core: Attach using APCI HID "INT33A1"")
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Patch looks good to me:
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Regards,
Hans
> ---
> drivers/platform/x86/intel_pmc_core_pltdrv.c | 26 +++++++++++++-------
> 1 file changed, 17 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/platform/x86/intel_pmc_core_pltdrv.c b/drivers/platform/x86/intel_pmc_core_pltdrv.c
> index 731281855cc8..73797680b895 100644
> --- a/drivers/platform/x86/intel_pmc_core_pltdrv.c
> +++ b/drivers/platform/x86/intel_pmc_core_pltdrv.c
> @@ -20,15 +20,10 @@
>
> static void intel_pmc_core_release(struct device *dev)
> {
> - /* Nothing to do. */
> + kfree(dev);
> }
>
> -static struct platform_device pmc_core_device = {
> - .name = "intel_pmc_core",
> - .dev = {
> - .release = intel_pmc_core_release,
> - },
> -};
> +static struct platform_device *pmc_core_device;
>
> /*
> * intel_pmc_core_platform_ids is the list of platforms where we want to
> @@ -52,6 +47,8 @@ MODULE_DEVICE_TABLE(x86cpu, intel_pmc_core_platform_ids);
>
> static int __init pmc_core_platform_init(void)
> {
> + int retval;
> +
> /* Skip creating the platform device if ACPI already has a device */
> if (acpi_dev_present("INT33A1", NULL, -1))
> return -ENODEV;
> @@ -59,12 +56,23 @@ static int __init pmc_core_platform_init(void)
> if (!x86_match_cpu(intel_pmc_core_platform_ids))
> return -ENODEV;
>
> - return platform_device_register(&pmc_core_device);
> + pmc_core_device = kzalloc(sizeof(*pmc_core_device), GFP_KERNEL);
> + if (!pmc_core_device)
> + return -ENOMEM;
> +
> + pmc_core_device->name = "intel_pmc_core";
> + pmc_core_device->dev.release = intel_pmc_core_release;
> +
> + retval = platform_device_register(pmc_core_device);
> + if (retval)
> + kfree(pmc_core_device);
> +
> + return retval;
> }
>
> static void __exit pmc_core_platform_exit(void)
> {
> - platform_device_unregister(&pmc_core_device);
> + platform_device_unregister(pmc_core_device);
> }
>
> module_init(pmc_core_platform_init);
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-09-24 8:39 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-23 18:48 [PATCH] platform/x86: intel_pmc_core: do not create a static struct device Greg Kroah-Hartman
2020-09-23 19:37 ` Rajat Jain
2020-09-24 8:39 ` Hans de Goede
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.