Currently there is a number of drivers that somewhat incorrectly
register themselves as acpi_driver, while they should be registering as
platform_drivers. acpi_device was never meant to represent actual
device, it only represents an entry in the ACPI table and ACPI driver
should be treated as a driver for the ACPI entry of the particular
device, not the device itself. Drivers of the devices itself should
register as platform_drivers. Replace acpi_driver with platform_driver
for all relevant drivers in drivers/acpi directory. This is just the
first part of the change, as many acpi_drivers are present in many files
outside of drivers/acpi directory.
Additionally during internal review we've decided that it's best to send
the relevant patches sequentially, in order not to overwhelm the reviewers.
So I'm starting with NFIT and AC drivers.
This change is possible thanks to recent .notify callback removal in
drivers/acpi [1]. So flow for the remaining acpi_drivers will look like
this:
1) Remove .notify callback with separate patchset for drivers/platform
and other outstanding places like drivers/hwmon, drivers/iio etc.
2) Replace acpi_driver with platform_driver, as aside for .notify
callback they're mostly functionally compatible.
Most of the patches in this series could be considered independent, and
could be merged separately, with a notable exception of the very first
patch in this series that changes notify installer wrappers to be more
generic. I decided it would be best not to force the user of this
wrappers to pass any specific structure, like acpi_device or
platform_device. It makes sense as it is very often the case that
drivers declare their own private structure which gets allocated during
the .probe() callback, and become the heart of the driver. In that case
it makes much more sense to pass this structure to notify handler.
Additionally some drivers, like acpi_video that already have custom
notify handlers do that already.
Link: https://lore.kernel.org/linux-acpi/[email protected]/ [1]
v2:
- dropped first, second and last commit. Last commit was deemed
pointless, first and second are already merged.
- rebased on top of modified first commit (changes in the wrappers)
Michal Wilczynski (6):
ACPI: AC: Remove unnecessary checks
ACPI: AC: Use string_choices API instead of ternary operator
ACPI: AC: Replace acpi_driver with platform_driver
ACPI: AC: Rename ACPI device from device to adev
ACPI: NFIT: Replace acpi_driver with platform_driver
ACPI: NFIT: Remove redundant call to to_acpi_dev()
drivers/acpi/ac.c | 108 +++++++++++++++++----------------------
drivers/acpi/nfit/core.c | 38 +++++++-------
2 files changed, 66 insertions(+), 80 deletions(-)
--
2.41.0
Remove unnecessary checks for NULL for variables that can't be NULL at
the point they're checked for it. Defensive programming is discouraged
in the kernel.
Signed-off-by: Michal Wilczynski <[email protected]>
---
drivers/acpi/ac.c | 27 ++++-----------------------
1 file changed, 4 insertions(+), 23 deletions(-)
diff --git a/drivers/acpi/ac.c b/drivers/acpi/ac.c
index aac3e561790c..83d45c681121 100644
--- a/drivers/acpi/ac.c
+++ b/drivers/acpi/ac.c
@@ -131,9 +131,6 @@ static void acpi_ac_notify(acpi_handle handle, u32 event, void *data)
struct acpi_device *device = data;
struct acpi_ac *ac = acpi_driver_data(device);
- if (!ac)
- return;
-
switch (event) {
default:
acpi_handle_debug(device->handle, "Unsupported event [0x%x]\n",
@@ -216,12 +213,8 @@ static const struct dmi_system_id ac_dmi_table[] __initconst = {
static int acpi_ac_add(struct acpi_device *device)
{
struct power_supply_config psy_cfg = {};
- int result = 0;
- struct acpi_ac *ac = NULL;
-
-
- if (!device)
- return -EINVAL;
+ struct acpi_ac *ac;
+ int result;
ac = kzalloc(sizeof(struct acpi_ac), GFP_KERNEL);
if (!ac)
@@ -275,16 +268,9 @@ static int acpi_ac_add(struct acpi_device *device)
#ifdef CONFIG_PM_SLEEP
static int acpi_ac_resume(struct device *dev)
{
- struct acpi_ac *ac;
+ struct acpi_ac *ac = acpi_driver_data(to_acpi_device(dev));
unsigned int old_state;
- if (!dev)
- return -EINVAL;
-
- ac = acpi_driver_data(to_acpi_device(dev));
- if (!ac)
- return -EINVAL;
-
old_state = ac->state;
if (acpi_ac_get_state(ac))
return 0;
@@ -299,12 +285,7 @@ static int acpi_ac_resume(struct device *dev)
static void acpi_ac_remove(struct acpi_device *device)
{
- struct acpi_ac *ac = NULL;
-
- if (!device || !acpi_driver_data(device))
- return;
-
- ac = acpi_driver_data(device);
+ struct acpi_ac *ac = acpi_driver_data(device);
acpi_dev_remove_notify_handler(device, ACPI_ALL_NOTIFY,
acpi_ac_notify);
--
2.41.0
Use modern string_choices API instead of manually determining the
output using ternary operator.
Suggested-by: Andy Shevchenko <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
Signed-off-by: Michal Wilczynski <[email protected]>
---
drivers/acpi/ac.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/acpi/ac.c b/drivers/acpi/ac.c
index 83d45c681121..f809f6889b4a 100644
--- a/drivers/acpi/ac.c
+++ b/drivers/acpi/ac.c
@@ -17,6 +17,7 @@
#include <linux/delay.h>
#include <linux/platform_device.h>
#include <linux/power_supply.h>
+#include <linux/string_choices.h>
#include <linux/acpi.h>
#include <acpi/battery.h>
@@ -243,8 +244,8 @@ static int acpi_ac_add(struct acpi_device *device)
goto err_release_ac;
}
- pr_info("%s [%s] (%s)\n", acpi_device_name(device),
- acpi_device_bid(device), ac->state ? "on-line" : "off-line");
+ pr_info("%s [%s] (%s-line)\n", acpi_device_name(device),
+ acpi_device_bid(device), str_on_off(ac->state));
ac->battery_nb.notifier_call = acpi_ac_battery_notify;
register_acpi_notifier(&ac->battery_nb);
--
2.41.0
AC driver uses struct acpi_driver incorrectly to register itself. This
is wrong as the instances of the ACPI devices are not meant to
be literal devices, they're supposed to describe ACPI entry of a
particular device.
Use platform_driver instead of acpi_driver. In relevant places call
platform devices instances pdev to make a distinction with ACPI
devices instances.
Drop unnecessary casts from acpi_bus_generate_netlink_event() and
acpi_notifier_call_chain().
Add a blank line to distinguish pdev API vs local ACPI notify function.
Suggested-by: Rafael J. Wysocki <[email protected]>
Signed-off-by: Michal Wilczynski <[email protected]>
---
drivers/acpi/ac.c | 70 +++++++++++++++++++++++++----------------------
1 file changed, 37 insertions(+), 33 deletions(-)
diff --git a/drivers/acpi/ac.c b/drivers/acpi/ac.c
index f809f6889b4a..298defeb5301 100644
--- a/drivers/acpi/ac.c
+++ b/drivers/acpi/ac.c
@@ -33,8 +33,9 @@ MODULE_AUTHOR("Paul Diefenbaugh");
MODULE_DESCRIPTION("ACPI AC Adapter Driver");
MODULE_LICENSE("GPL");
-static int acpi_ac_add(struct acpi_device *device);
-static void acpi_ac_remove(struct acpi_device *device);
+static int acpi_ac_probe(struct platform_device *pdev);
+static void acpi_ac_remove(struct platform_device *pdev);
+
static void acpi_ac_notify(acpi_handle handle, u32 event, void *data);
static const struct acpi_device_id ac_device_ids[] = {
@@ -51,21 +52,10 @@ static SIMPLE_DEV_PM_OPS(acpi_ac_pm, NULL, acpi_ac_resume);
static int ac_sleep_before_get_state_ms;
static int ac_only;
-static struct acpi_driver acpi_ac_driver = {
- .name = "ac",
- .class = ACPI_AC_CLASS,
- .ids = ac_device_ids,
- .ops = {
- .add = acpi_ac_add,
- .remove = acpi_ac_remove,
- },
- .drv.pm = &acpi_ac_pm,
-};
-
struct acpi_ac {
struct power_supply *charger;
struct power_supply_desc charger_desc;
- struct acpi_device *device;
+ struct device *dev;
unsigned long long state;
struct notifier_block battery_nb;
};
@@ -85,10 +75,10 @@ static int acpi_ac_get_state(struct acpi_ac *ac)
return 0;
}
- status = acpi_evaluate_integer(ac->device->handle, "_PSR", NULL,
+ status = acpi_evaluate_integer(ACPI_HANDLE(ac->dev), "_PSR", NULL,
&ac->state);
if (ACPI_FAILURE(status)) {
- acpi_handle_info(ac->device->handle,
+ acpi_handle_info(ACPI_HANDLE(ac->dev),
"Error reading AC Adapter state: %s\n",
acpi_format_exception(status));
ac->state = ACPI_AC_STATUS_UNKNOWN;
@@ -129,12 +119,12 @@ static enum power_supply_property ac_props[] = {
/* Driver Model */
static void acpi_ac_notify(acpi_handle handle, u32 event, void *data)
{
- struct acpi_device *device = data;
- struct acpi_ac *ac = acpi_driver_data(device);
+ struct acpi_ac *ac = data;
+ struct acpi_device *device = ACPI_COMPANION(ac->dev);
switch (event) {
default:
- acpi_handle_debug(device->handle, "Unsupported event [0x%x]\n",
+ acpi_handle_debug(ACPI_HANDLE(ac->dev), "Unsupported event [0x%x]\n",
event);
fallthrough;
case ACPI_AC_NOTIFY_STATUS:
@@ -152,9 +142,10 @@ static void acpi_ac_notify(acpi_handle handle, u32 event, void *data)
acpi_ac_get_state(ac);
acpi_bus_generate_netlink_event(device->pnp.device_class,
- dev_name(&device->dev), event,
- (u32) ac->state);
- acpi_notifier_call_chain(device, event, (u32) ac->state);
+ dev_name(ac->dev),
+ event,
+ ac->state);
+ acpi_notifier_call_chain(device, event, ac->state);
kobject_uevent(&ac->charger->dev.kobj, KOBJ_CHANGE);
}
}
@@ -211,8 +202,9 @@ static const struct dmi_system_id ac_dmi_table[] __initconst = {
{},
};
-static int acpi_ac_add(struct acpi_device *device)
+static int acpi_ac_probe(struct platform_device *pdev)
{
+ struct acpi_device *device = ACPI_COMPANION(&pdev->dev);
struct power_supply_config psy_cfg = {};
struct acpi_ac *ac;
int result;
@@ -221,10 +213,11 @@ static int acpi_ac_add(struct acpi_device *device)
if (!ac)
return -ENOMEM;
- ac->device = device;
+ ac->dev = &pdev->dev;
strcpy(acpi_device_name(device), ACPI_AC_DEVICE_NAME);
strcpy(acpi_device_class(device), ACPI_AC_CLASS);
- device->driver_data = ac;
+
+ platform_set_drvdata(pdev, ac);
result = acpi_ac_get_state(ac);
if (result)
@@ -237,7 +230,7 @@ static int acpi_ac_add(struct acpi_device *device)
ac->charger_desc.properties = ac_props;
ac->charger_desc.num_properties = ARRAY_SIZE(ac_props);
ac->charger_desc.get_property = get_ac_property;
- ac->charger = power_supply_register(&ac->device->dev,
+ ac->charger = power_supply_register(&pdev->dev,
&ac->charger_desc, &psy_cfg);
if (IS_ERR(ac->charger)) {
result = PTR_ERR(ac->charger);
@@ -251,7 +244,7 @@ static int acpi_ac_add(struct acpi_device *device)
register_acpi_notifier(&ac->battery_nb);
result = acpi_dev_install_notify_handler(device, ACPI_ALL_NOTIFY,
- acpi_ac_notify, device);
+ acpi_ac_notify, ac);
if (result)
goto err_unregister;
@@ -269,7 +262,7 @@ static int acpi_ac_add(struct acpi_device *device)
#ifdef CONFIG_PM_SLEEP
static int acpi_ac_resume(struct device *dev)
{
- struct acpi_ac *ac = acpi_driver_data(to_acpi_device(dev));
+ struct acpi_ac *ac = dev_get_drvdata(dev);
unsigned int old_state;
old_state = ac->state;
@@ -284,11 +277,12 @@ static int acpi_ac_resume(struct device *dev)
#define acpi_ac_resume NULL
#endif
-static void acpi_ac_remove(struct acpi_device *device)
+static void acpi_ac_remove(struct platform_device *pdev)
{
- struct acpi_ac *ac = acpi_driver_data(device);
+ struct acpi_ac *ac = platform_get_drvdata(pdev);
- acpi_dev_remove_notify_handler(device, ACPI_ALL_NOTIFY,
+ acpi_dev_remove_notify_handler(ACPI_COMPANION(ac->dev),
+ ACPI_ALL_NOTIFY,
acpi_ac_notify);
power_supply_unregister(ac->charger);
unregister_acpi_notifier(&ac->battery_nb);
@@ -296,6 +290,16 @@ static void acpi_ac_remove(struct acpi_device *device)
kfree(ac);
}
+static struct platform_driver acpi_ac_driver = {
+ .probe = acpi_ac_probe,
+ .remove_new = acpi_ac_remove,
+ .driver = {
+ .name = "ac",
+ .acpi_match_table = ac_device_ids,
+ .pm = &acpi_ac_pm,
+ },
+};
+
static int __init acpi_ac_init(void)
{
int result;
@@ -308,7 +312,7 @@ static int __init acpi_ac_init(void)
dmi_check_system(ac_dmi_table);
- result = acpi_bus_register_driver(&acpi_ac_driver);
+ result = platform_driver_register(&acpi_ac_driver);
if (result < 0)
return -ENODEV;
@@ -317,7 +321,7 @@ static int __init acpi_ac_init(void)
static void __exit acpi_ac_exit(void)
{
- acpi_bus_unregister_driver(&acpi_ac_driver);
+ platform_driver_unregister(&acpi_ac_driver);
}
module_init(acpi_ac_init);
module_exit(acpi_ac_exit);
--
2.41.0
Since transformation from ACPI driver to platform driver there are two
devices on which the driver operates - ACPI device and platform device.
For the sake of reader this calls for the distinction in their naming,
to avoid confusion. Rename device to adev, as corresponding
platform device is called pdev.
Signed-off-by: Michal Wilczynski <[email protected]>
---
drivers/acpi/ac.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/acpi/ac.c b/drivers/acpi/ac.c
index 298defeb5301..bb02e7f5d65a 100644
--- a/drivers/acpi/ac.c
+++ b/drivers/acpi/ac.c
@@ -120,7 +120,7 @@ static enum power_supply_property ac_props[] = {
static void acpi_ac_notify(acpi_handle handle, u32 event, void *data)
{
struct acpi_ac *ac = data;
- struct acpi_device *device = ACPI_COMPANION(ac->dev);
+ struct acpi_device *adev = ACPI_COMPANION(ac->dev);
switch (event) {
default:
@@ -141,11 +141,11 @@ static void acpi_ac_notify(acpi_handle handle, u32 event, void *data)
msleep(ac_sleep_before_get_state_ms);
acpi_ac_get_state(ac);
- acpi_bus_generate_netlink_event(device->pnp.device_class,
+ acpi_bus_generate_netlink_event(adev->pnp.device_class,
dev_name(ac->dev),
event,
ac->state);
- acpi_notifier_call_chain(device, event, ac->state);
+ acpi_notifier_call_chain(adev, event, ac->state);
kobject_uevent(&ac->charger->dev.kobj, KOBJ_CHANGE);
}
}
@@ -204,7 +204,7 @@ static const struct dmi_system_id ac_dmi_table[] __initconst = {
static int acpi_ac_probe(struct platform_device *pdev)
{
- struct acpi_device *device = ACPI_COMPANION(&pdev->dev);
+ struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
struct power_supply_config psy_cfg = {};
struct acpi_ac *ac;
int result;
@@ -214,8 +214,8 @@ static int acpi_ac_probe(struct platform_device *pdev)
return -ENOMEM;
ac->dev = &pdev->dev;
- strcpy(acpi_device_name(device), ACPI_AC_DEVICE_NAME);
- strcpy(acpi_device_class(device), ACPI_AC_CLASS);
+ strcpy(acpi_device_name(adev), ACPI_AC_DEVICE_NAME);
+ strcpy(acpi_device_class(adev), ACPI_AC_CLASS);
platform_set_drvdata(pdev, ac);
@@ -225,7 +225,7 @@ static int acpi_ac_probe(struct platform_device *pdev)
psy_cfg.drv_data = ac;
- ac->charger_desc.name = acpi_device_bid(device);
+ ac->charger_desc.name = acpi_device_bid(adev);
ac->charger_desc.type = POWER_SUPPLY_TYPE_MAINS;
ac->charger_desc.properties = ac_props;
ac->charger_desc.num_properties = ARRAY_SIZE(ac_props);
@@ -237,13 +237,13 @@ static int acpi_ac_probe(struct platform_device *pdev)
goto err_release_ac;
}
- pr_info("%s [%s] (%s-line)\n", acpi_device_name(device),
- acpi_device_bid(device), str_on_off(ac->state));
+ pr_info("%s [%s] (%s-line)\n", acpi_device_name(adev),
+ acpi_device_bid(adev), str_on_off(ac->state));
ac->battery_nb.notifier_call = acpi_ac_battery_notify;
register_acpi_notifier(&ac->battery_nb);
- result = acpi_dev_install_notify_handler(device, ACPI_ALL_NOTIFY,
+ result = acpi_dev_install_notify_handler(adev, ACPI_ALL_NOTIFY,
acpi_ac_notify, ac);
if (result)
goto err_unregister;
--
2.41.0
In acpi_nfit_ctl() ACPI handle is extracted using to_acpi_dev()
function and accessing handle field in ACPI device. After transformation
from ACPI driver to platform driver this is not optimal anymore. To get
a handle it's enough to just use ACPI_HANDLE() macro to extract the
handle.
Suggested-by: Andy Shevchenko <[email protected]>
Signed-off-by: Michal Wilczynski <[email protected]>
---
drivers/acpi/nfit/core.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index fb0bc16fa186..3d254b2cf2e7 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -475,8 +475,6 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
guid = to_nfit_uuid(nfit_mem->family);
handle = adev->handle;
} else {
- struct acpi_device *adev = to_acpi_dev(acpi_desc);
-
cmd_name = nvdimm_bus_cmd_name(cmd);
cmd_mask = nd_desc->cmd_mask;
if (cmd == ND_CMD_CALL && call_pkg->nd_family) {
@@ -493,7 +491,7 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
guid = to_nfit_uuid(NFIT_DEV_BUS);
}
desc = nd_cmd_bus_desc(cmd);
- handle = adev->handle;
+ handle = ACPI_HANDLE(dev);
dimm_name = "bus";
}
--
2.41.0
NFIT driver uses struct acpi_driver incorrectly to register itself.
This is wrong as the instances of the ACPI devices are not meant
to be literal devices, they're supposed to describe ACPI entry of a
particular device.
Use platform_driver instead of acpi_driver. In relevant places call
platform devices instances pdev to make a distinction with ACPI
devices instances.
NFIT driver uses devm_*() family of functions extensively. This change
has no impact on correct functioning of the whole devm_*() family of
functions, since the lifecycle of the device stays the same. It is still
being created during the enumeration, and destroyed on platform device
removal.
Suggested-by: Rafael J. Wysocki <[email protected]>
Signed-off-by: Michal Wilczynski <[email protected]>
---
drivers/acpi/nfit/core.c | 34 ++++++++++++++++++----------------
1 file changed, 18 insertions(+), 16 deletions(-)
diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index 942b84d94078..fb0bc16fa186 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -15,6 +15,7 @@
#include <linux/sort.h>
#include <linux/io.h>
#include <linux/nd.h>
+#include <linux/platform_device.h>
#include <asm/cacheflush.h>
#include <acpi/nfit.h>
#include "intel.h"
@@ -98,7 +99,7 @@ static struct acpi_device *to_acpi_dev(struct acpi_nfit_desc *acpi_desc)
|| strcmp(nd_desc->provider_name, "ACPI.NFIT") != 0)
return NULL;
- return to_acpi_device(acpi_desc->dev);
+ return ACPI_COMPANION(acpi_desc->dev);
}
static int xlat_bus_status(void *buf, unsigned int cmd, u32 status)
@@ -3284,11 +3285,11 @@ static void acpi_nfit_put_table(void *table)
static void acpi_nfit_notify(acpi_handle handle, u32 event, void *data)
{
- struct acpi_device *adev = data;
+ struct device *dev = data;
- device_lock(&adev->dev);
- __acpi_nfit_notify(&adev->dev, handle, event);
- device_unlock(&adev->dev);
+ device_lock(dev);
+ __acpi_nfit_notify(dev, handle, event);
+ device_unlock(dev);
}
static void acpi_nfit_remove_notify_handler(void *data)
@@ -3329,11 +3330,12 @@ void acpi_nfit_shutdown(void *data)
}
EXPORT_SYMBOL_GPL(acpi_nfit_shutdown);
-static int acpi_nfit_add(struct acpi_device *adev)
+static int acpi_nfit_probe(struct platform_device *pdev)
{
struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER, NULL };
struct acpi_nfit_desc *acpi_desc;
- struct device *dev = &adev->dev;
+ struct device *dev = &pdev->dev;
+ struct acpi_device *adev = ACPI_COMPANION(dev);
struct acpi_table_header *tbl;
acpi_status status = AE_OK;
acpi_size sz;
@@ -3360,7 +3362,7 @@ static int acpi_nfit_add(struct acpi_device *adev)
acpi_desc = devm_kzalloc(dev, sizeof(*acpi_desc), GFP_KERNEL);
if (!acpi_desc)
return -ENOMEM;
- acpi_nfit_desc_init(acpi_desc, &adev->dev);
+ acpi_nfit_desc_init(acpi_desc, dev);
/* Save the acpi header for exporting the revision via sysfs */
acpi_desc->acpi_header = *tbl;
@@ -3391,7 +3393,7 @@ static int acpi_nfit_add(struct acpi_device *adev)
return rc;
rc = acpi_dev_install_notify_handler(adev, ACPI_DEVICE_NOTIFY,
- acpi_nfit_notify, adev);
+ acpi_nfit_notify, dev);
if (rc)
return rc;
@@ -3475,11 +3477,11 @@ static const struct acpi_device_id acpi_nfit_ids[] = {
};
MODULE_DEVICE_TABLE(acpi, acpi_nfit_ids);
-static struct acpi_driver acpi_nfit_driver = {
- .name = KBUILD_MODNAME,
- .ids = acpi_nfit_ids,
- .ops = {
- .add = acpi_nfit_add,
+static struct platform_driver acpi_nfit_driver = {
+ .probe = acpi_nfit_probe,
+ .driver = {
+ .name = KBUILD_MODNAME,
+ .acpi_match_table = acpi_nfit_ids,
},
};
@@ -3517,7 +3519,7 @@ static __init int nfit_init(void)
return -ENOMEM;
nfit_mce_register();
- ret = acpi_bus_register_driver(&acpi_nfit_driver);
+ ret = platform_driver_register(&acpi_nfit_driver);
if (ret) {
nfit_mce_unregister();
destroy_workqueue(nfit_wq);
@@ -3530,7 +3532,7 @@ static __init int nfit_init(void)
static __exit void nfit_exit(void)
{
nfit_mce_unregister();
- acpi_bus_unregister_driver(&acpi_nfit_driver);
+ platform_driver_unregister(&acpi_nfit_driver);
destroy_workqueue(nfit_wq);
WARN_ON(!list_empty(&acpi_descs));
}
--
2.41.0
On Fri, Oct 06, 2023 at 08:30:52PM +0300, Michal Wilczynski wrote:
> AC driver uses struct acpi_driver incorrectly to register itself. This
> is wrong as the instances of the ACPI devices are not meant to
> be literal devices, they're supposed to describe ACPI entry of a
> particular device.
>
> Use platform_driver instead of acpi_driver. In relevant places call
> platform devices instances pdev to make a distinction with ACPI
> devices instances.
>
> Drop unnecessary casts from acpi_bus_generate_netlink_event() and
> acpi_notifier_call_chain().
>
> Add a blank line to distinguish pdev API vs local ACPI notify function.
...
> struct acpi_ac {
> struct power_supply *charger;
> struct power_supply_desc charger_desc;
> - struct acpi_device *device;
> + struct device *dev;
> unsigned long long state;
> struct notifier_block battery_nb;
> };
When changing this, also makes sense just to check if the moving a member in
the data structure makes code shorter, but it's not a show stopper.
...
> - status = acpi_evaluate_integer(ac->device->handle, "_PSR", NULL,
> + status = acpi_evaluate_integer(ACPI_HANDLE(ac->dev), "_PSR", NULL,
> &ac->state);
> if (ACPI_FAILURE(status)) {
> - acpi_handle_info(ac->device->handle,
> + acpi_handle_info(ACPI_HANDLE(ac->dev),
Can we call ACPI_HANDLE() only once and cache that in a local variable and use
in all places?
...
> - struct acpi_ac *ac = acpi_driver_data(device);
> + struct acpi_ac *ac = data;
> + struct acpi_device *device = ACPI_COMPANION(ac->dev);
>
> switch (event) {
> default:
> - acpi_handle_debug(device->handle, "Unsupported event [0x%x]\n",
> + acpi_handle_debug(ACPI_HANDLE(ac->dev), "Unsupported event [0x%x]\n",
> event);
Does it makes any sense now? Basically it duplicates the ACPI_COMPANION() call
as Rafael pointed out in previous version discussion.
> fallthrough;
--
With Best Regards,
Andy Shevchenko
On Fri, Oct 6, 2023 at 8:33 PM Michal Wilczynski
<[email protected]> wrote:
>
> AC driver uses struct acpi_driver incorrectly to register itself. This
> is wrong as the instances of the ACPI devices are not meant to
> be literal devices, they're supposed to describe ACPI entry of a
> particular device.
>
> Use platform_driver instead of acpi_driver. In relevant places call
> platform devices instances pdev to make a distinction with ACPI
> devices instances.
>
> Drop unnecessary casts from acpi_bus_generate_netlink_event() and
> acpi_notifier_call_chain().
>
> Add a blank line to distinguish pdev API vs local ACPI notify function.
>
> Suggested-by: Rafael J. Wysocki <[email protected]>
> Signed-off-by: Michal Wilczynski <[email protected]>
> ---
> drivers/acpi/ac.c | 70 +++++++++++++++++++++++++----------------------
> 1 file changed, 37 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/acpi/ac.c b/drivers/acpi/ac.c
> index f809f6889b4a..298defeb5301 100644
> --- a/drivers/acpi/ac.c
> +++ b/drivers/acpi/ac.c
> @@ -33,8 +33,9 @@ MODULE_AUTHOR("Paul Diefenbaugh");
> MODULE_DESCRIPTION("ACPI AC Adapter Driver");
> MODULE_LICENSE("GPL");
>
> -static int acpi_ac_add(struct acpi_device *device);
> -static void acpi_ac_remove(struct acpi_device *device);
> +static int acpi_ac_probe(struct platform_device *pdev);
> +static void acpi_ac_remove(struct platform_device *pdev);
> +
> static void acpi_ac_notify(acpi_handle handle, u32 event, void *data);
>
> static const struct acpi_device_id ac_device_ids[] = {
> @@ -51,21 +52,10 @@ static SIMPLE_DEV_PM_OPS(acpi_ac_pm, NULL, acpi_ac_resume);
> static int ac_sleep_before_get_state_ms;
> static int ac_only;
>
> -static struct acpi_driver acpi_ac_driver = {
> - .name = "ac",
> - .class = ACPI_AC_CLASS,
> - .ids = ac_device_ids,
> - .ops = {
> - .add = acpi_ac_add,
> - .remove = acpi_ac_remove,
> - },
> - .drv.pm = &acpi_ac_pm,
> -};
> -
> struct acpi_ac {
> struct power_supply *charger;
> struct power_supply_desc charger_desc;
> - struct acpi_device *device;
> + struct device *dev;
I'm not convinced about this change.
If I'm not mistaken, you only use the dev pointer above to get the
ACPI_COMPANION() of it, but the latter is already found in _probe(),
so it can be stored in struct acpi_ac for later use and then the dev
pointer in there will not be necessary any more.
That will save you a bunch of ACPI_HANDLE() evaluations and there's
nothing wrong with using ac->device->handle. The patch will then
become almost trivial AFAICS and if you really need to get from ac to
the underlying platform device, a pointer to it can be added to struct
acpi_ac without removing the ACPI device pointer from it.
> unsigned long long state;
> struct notifier_block battery_nb;
> };
On Fri, Oct 06, 2023 at 09:47:57PM +0200, Rafael J. Wysocki wrote:
> On Fri, Oct 6, 2023 at 8:33 PM Michal Wilczynski
> <[email protected]> wrote:
...
> > struct acpi_ac {
> > struct power_supply *charger;
> > struct power_supply_desc charger_desc;
> > - struct acpi_device *device;
> > + struct device *dev;
>
> I'm not convinced about this change.
>
> If I'm not mistaken, you only use the dev pointer above to get the
> ACPI_COMPANION() of it, but the latter is already found in _probe(),
> so it can be stored in struct acpi_ac for later use and then the dev
> pointer in there will not be necessary any more.
>
> That will save you a bunch of ACPI_HANDLE() evaluations and there's
> nothing wrong with using ac->device->handle. The patch will then
> become almost trivial AFAICS and if you really need to get from ac to
> the underlying platform device, a pointer to it can be added to struct
> acpi_ac without removing the ACPI device pointer from it.
The idea behind is to eliminate data duplication.
> > unsigned long long state;
> > struct notifier_block battery_nb;
> > };
--
With Best Regards,
Andy Shevchenko
On Sat, Oct 7, 2023 at 9:56 AM Andy Shevchenko
<[email protected]> wrote:
>
> On Fri, Oct 06, 2023 at 09:47:57PM +0200, Rafael J. Wysocki wrote:
> > On Fri, Oct 6, 2023 at 8:33 PM Michal Wilczynski
> > <[email protected]> wrote:
>
> ...
>
> > > struct acpi_ac {
> > > struct power_supply *charger;
> > > struct power_supply_desc charger_desc;
> > > - struct acpi_device *device;
> > > + struct device *dev;
> >
> > I'm not convinced about this change.
> >
> > If I'm not mistaken, you only use the dev pointer above to get the
> > ACPI_COMPANION() of it, but the latter is already found in _probe(),
> > so it can be stored in struct acpi_ac for later use and then the dev
> > pointer in there will not be necessary any more.
> >
> > That will save you a bunch of ACPI_HANDLE() evaluations and there's
> > nothing wrong with using ac->device->handle. The patch will then
> > become almost trivial AFAICS and if you really need to get from ac to
> > the underlying platform device, a pointer to it can be added to struct
> > acpi_ac without removing the ACPI device pointer from it.
>
> The idea behind is to eliminate data duplication.
What data duplication exactly do you mean?
struct acpi_device *device is replaced with struct device *dev which
is the same size. The latter is then used to obtain a struct
acpi_device pointer. Why is it better to do this than to store the
struct acpi_device itself?
On Sat, Oct 7, 2023 at 12:41 PM Rafael J. Wysocki <[email protected]> wrote:
>
> On Sat, Oct 7, 2023 at 9:56 AM Andy Shevchenko
> <[email protected]> wrote:
> >
> > On Fri, Oct 06, 2023 at 09:47:57PM +0200, Rafael J. Wysocki wrote:
> > > On Fri, Oct 6, 2023 at 8:33 PM Michal Wilczynski
> > > <[email protected]> wrote:
> >
> > ...
> >
> > > > struct acpi_ac {
> > > > struct power_supply *charger;
> > > > struct power_supply_desc charger_desc;
> > > > - struct acpi_device *device;
> > > > + struct device *dev;
> > >
> > > I'm not convinced about this change.
> > >
> > > If I'm not mistaken, you only use the dev pointer above to get the
> > > ACPI_COMPANION() of it, but the latter is already found in _probe(),
> > > so it can be stored in struct acpi_ac for later use and then the dev
> > > pointer in there will not be necessary any more.
> > >
> > > That will save you a bunch of ACPI_HANDLE() evaluations and there's
> > > nothing wrong with using ac->device->handle. The patch will then
> > > become almost trivial AFAICS and if you really need to get from ac to
> > > the underlying platform device, a pointer to it can be added to struct
> > > acpi_ac without removing the ACPI device pointer from it.
> >
> > The idea behind is to eliminate data duplication.
>
> What data duplication exactly do you mean?
>
> struct acpi_device *device is replaced with struct device *dev which
> is the same size. The latter is then used to obtain a struct
> acpi_device pointer. Why is it better to do this than to store the
> struct acpi_device itself?
This should be "store the struct acpi_device pointer itself", sorry.
Hi !
Thanks a lot for a review, to both of you ! :-)
On 10/7/2023 12:43 PM, Rafael J. Wysocki wrote:
> On Sat, Oct 7, 2023 at 12:41 PM Rafael J. Wysocki <[email protected]> wrote:
>> On Sat, Oct 7, 2023 at 9:56 AM Andy Shevchenko
>> <[email protected]> wrote:
>>> On Fri, Oct 06, 2023 at 09:47:57PM +0200, Rafael J. Wysocki wrote:
>>>> On Fri, Oct 6, 2023 at 8:33 PM Michal Wilczynski
>>>> <[email protected]> wrote:
>>> ...
>>>
>>>>> struct acpi_ac {
>>>>> struct power_supply *charger;
>>>>> struct power_supply_desc charger_desc;
>>>>> - struct acpi_device *device;
>>>>> + struct device *dev;
>>>> I'm not convinced about this change.
>>>>
>>>> If I'm not mistaken, you only use the dev pointer above to get the
>>>> ACPI_COMPANION() of it, but the latter is already found in _probe(),
>>>> so it can be stored in struct acpi_ac for later use and then the dev
>>>> pointer in there will not be necessary any more.
>>>>
>>>> That will save you a bunch of ACPI_HANDLE() evaluations and there's
>>>> nothing wrong with using ac->device->handle. The patch will then
>>>> become almost trivial AFAICS and if you really need to get from ac to
>>>> the underlying platform device, a pointer to it can be added to struct
>>>> acpi_ac without removing the ACPI device pointer from it.
Yeah we could add platform device without removing acpi device, and
yes that would introduce data duplication, like Andy noticed. And yes,
maybe for this particular driver there is little impact (as struct device is not
really used), but in my opinion we should adhere to some common coding
pattern among all acpi drivers in order for the code to be easier to maintain
and improve readability, also for any newcomer.
>>> The idea behind is to eliminate data duplication.
>> What data duplication exactly do you mean?
>>
>> struct acpi_device *device is replaced with struct device *dev which
>> is the same size. The latter is then used to obtain a struct
>> acpi_device pointer. Why is it better to do this than to store the
>> struct acpi_device itself?
> This should be "store the struct acpi_device pointer itself", sorry.
Hi,
So let me explain the reasoning here:
1) I've had a pleasure to work with different drivers in ACPI directory. In my
opinion the best ones I've seen, were build around the concept of struct
device (e.g NFIT). It's a struct that's well understood in the kernel, and
it's easier to understand without having to read any ACPI specific code.
If I see something like ACPI_HANDLE(dev), I think 'ah-ha - having a struct
device I can easily get struct acpi_device - they're connected'. And I think
using a standardized macro, instead of manually dereferencing pointers is
also much easier for ACPI newbs reading the code, as it hides a bit complexity
of getting acpi device from struct device. And to be honest I don't think there would
be any measurable performance change, as those code paths are far from
being considered 'hot paths'. So if we can get the code easier to understand
from a newcomer perspective, why not do it.
2) I think it would be good to stick to the convention, and introduce some coding
pattern, for now some drivers store the struct device pointer, and other store
acpi device pointer . As I'm doing this change acpi device pointer become less
relevant, as we're using platform device. So to reflect that loss of relevance
we can choose to adhere to a pattern where we use it less and less, and the
winning approach would be to use 'struct device' by default everywhere we can
so maybe eventually we would be able to lose acpi_device altogether at some point,
as most of the usage is to retrieve acpi handle and execute some AML method.
So in my understanding acpi device is already obsolete at this point, if we can
manage to use it less and less, and eventually wipe it out then why not ;-)
Thanks again !
Michał
On Mon, Oct 9, 2023 at 10:40 AM Wilczynski, Michal
<[email protected]> wrote:
>
>
> Hi !
>
> Thanks a lot for a review, to both of you ! :-)
>
> On 10/7/2023 12:43 PM, Rafael J. Wysocki wrote:
> > On Sat, Oct 7, 2023 at 12:41 PM Rafael J. Wysocki <[email protected]> wrote:
> >> On Sat, Oct 7, 2023 at 9:56 AM Andy Shevchenko
> >> <[email protected]> wrote:
> >>> On Fri, Oct 06, 2023 at 09:47:57PM +0200, Rafael J. Wysocki wrote:
> >>>> On Fri, Oct 6, 2023 at 8:33 PM Michal Wilczynski
> >>>> <[email protected]> wrote:
> >>> ...
> >>>
> >>>>> struct acpi_ac {
> >>>>> struct power_supply *charger;
> >>>>> struct power_supply_desc charger_desc;
> >>>>> - struct acpi_device *device;
> >>>>> + struct device *dev;
> >>>> I'm not convinced about this change.
> >>>>
> >>>> If I'm not mistaken, you only use the dev pointer above to get the
> >>>> ACPI_COMPANION() of it, but the latter is already found in _probe(),
> >>>> so it can be stored in struct acpi_ac for later use and then the dev
> >>>> pointer in there will not be necessary any more.
> >>>>
> >>>> That will save you a bunch of ACPI_HANDLE() evaluations and there's
> >>>> nothing wrong with using ac->device->handle. The patch will then
> >>>> become almost trivial AFAICS and if you really need to get from ac to
> >>>> the underlying platform device, a pointer to it can be added to struct
> >>>> acpi_ac without removing the ACPI device pointer from it.
>
> Yeah we could add platform device without removing acpi device, and
> yes that would introduce data duplication, like Andy noticed.
But he hasn't answered my question regarding what data duplication he
meant, exactly.
So what data duplication do you mean?
> And yes, maybe for this particular driver there is little impact (as struct device is not
> really used), but in my opinion we should adhere to some common coding
> pattern among all acpi drivers in order for the code to be easier to maintain
> and improve readability, also for any newcomer.
Well, maybe.
But then please minimize the number of code lines changed in this
particular patch and please avoid changes that are not directly
related to the purpose of the patch.
> >>> The idea behind is to eliminate data duplication.
> >> What data duplication exactly do you mean?
> >>
> >> struct acpi_device *device is replaced with struct device *dev which
> >> is the same size. The latter is then used to obtain a struct
> >> acpi_device pointer. Why is it better to do this than to store the
> >> struct acpi_device itself?
> > This should be "store the struct acpi_device pointer itself", sorry.
>
> Hi,
> So let me explain the reasoning here:
>
> 1) I've had a pleasure to work with different drivers in ACPI directory. In my
> opinion the best ones I've seen, were build around the concept of struct
> device (e.g NFIT). It's a struct that's well understood in the kernel, and
> it's easier to understand without having to read any ACPI specific code.
> If I see something like ACPI_HANDLE(dev), I think 'ah-ha - having a struct
> device I can easily get struct acpi_device - they're connected'. And I think
> using a standardized macro, instead of manually dereferencing pointers is
> also much easier for ACPI newbs reading the code, as it hides a bit complexity
> of getting acpi device from struct device. And to be honest I don't think there would
> be any measurable performance change, as those code paths are far from
> being considered 'hot paths'. So if we can get the code easier to understand
> from a newcomer perspective, why not do it.
I have a differing opinion on a couple of points above, and let's make
one change at a time.
>
> 2) I think it would be good to stick to the convention, and introduce some coding
> pattern, for now some drivers store the struct device pointer, and other store
> acpi device pointer . As I'm doing this change acpi device pointer become less
> relevant, as we're using platform device. So to reflect that loss of relevance
> we can choose to adhere to a pattern where we use it less and less, and the
> winning approach would be to use 'struct device' by default everywhere we can
> so maybe eventually we would be able to lose acpi_device altogether at some point,
> as most of the usage is to retrieve acpi handle and execute some AML method.
> So in my understanding acpi device is already obsolete at this point, if we can
> manage to use it less and less, and eventually wipe it out then why not ;-)
No, ACPI device is not obsolete, it will still be needed for various
things, like power management and hotplug.
Also, I'd rather store a struct acpi_device than acpi_handle, because
the latter is much better from the compile-time type correctness
checks and similar.
I can send my version of the $subject patch just fine if you strongly
disagree with me.
On 10/9/2023 2:27 PM, Rafael J. Wysocki wrote:
> On Mon, Oct 9, 2023 at 10:40 AM Wilczynski, Michal
> <[email protected]> wrote:
>>
>> Hi !
>>
>> Thanks a lot for a review, to both of you ! :-)
>>
>> On 10/7/2023 12:43 PM, Rafael J. Wysocki wrote:
>>> On Sat, Oct 7, 2023 at 12:41 PM Rafael J. Wysocki <[email protected]> wrote:
>>>> On Sat, Oct 7, 2023 at 9:56 AM Andy Shevchenko
>>>> <[email protected]> wrote:
>>>>> On Fri, Oct 06, 2023 at 09:47:57PM +0200, Rafael J. Wysocki wrote:
>>>>>> On Fri, Oct 6, 2023 at 8:33 PM Michal Wilczynski
>>>>>> <[email protected]> wrote:
>>>>> ...
>>>>>
>>>>>>> struct acpi_ac {
>>>>>>> struct power_supply *charger;
>>>>>>> struct power_supply_desc charger_desc;
>>>>>>> - struct acpi_device *device;
>>>>>>> + struct device *dev;
>>>>>> I'm not convinced about this change.
>>>>>>
>>>>>> If I'm not mistaken, you only use the dev pointer above to get the
>>>>>> ACPI_COMPANION() of it, but the latter is already found in _probe(),
>>>>>> so it can be stored in struct acpi_ac for later use and then the dev
>>>>>> pointer in there will not be necessary any more.
>>>>>>
>>>>>> That will save you a bunch of ACPI_HANDLE() evaluations and there's
>>>>>> nothing wrong with using ac->device->handle. The patch will then
>>>>>> become almost trivial AFAICS and if you really need to get from ac to
>>>>>> the underlying platform device, a pointer to it can be added to struct
>>>>>> acpi_ac without removing the ACPI device pointer from it.
>> Yeah we could add platform device without removing acpi device, and
>> yes that would introduce data duplication, like Andy noticed.
> But he hasn't answered my question regarding what data duplication he
> meant, exactly.
>
> So what data duplication do you mean?
So what I meant was that many drivers would find it useful to have
a struct device embedded in their 'private structure', and in that case
there would be a duplication of platform_device and acpi_device as
both pointers would be needed. Mind this if you only have struct device
it's easy to get acpi device, but it's not the case the other way around.
So for this driver it's just a matter of sticking to convention, for the others
like it would be duplication.
In my version of this patch we managed to avoid this duplication, thanks
to the contextual argument introduced before, but look at this patch:
https://github.com/mwilczy/linux-pm/commit/cc8ef52707341e67a12067d6ead991d56ea017ca
Author of this patch had to introduce platform_device and acpi_device to the struct ac, so
there was some duplication. That is the case for many drivers to come, so I decided it's better
to change this and have a pattern with keeping only 'struct device'.
>
>> And yes, maybe for this particular driver there is little impact (as struct device is not
>> really used), but in my opinion we should adhere to some common coding
>> pattern among all acpi drivers in order for the code to be easier to maintain
>> and improve readability, also for any newcomer.
> Well, maybe.
>
> But then please minimize the number of code lines changed in this
> particular patch and please avoid changes that are not directly
> related to the purpose of the patch.
Sure, like I've stated before I felt this is part of this patch, we only narrowly
escaped the duplication by introducing contextual argument before ;-)
>
>>>>> The idea behind is to eliminate data duplication.
>>>> What data duplication exactly do you mean?
>>>>
>>>> struct acpi_device *device is replaced with struct device *dev which
>>>> is the same size. The latter is then used to obtain a struct
>>>> acpi_device pointer. Why is it better to do this than to store the
>>>> struct acpi_device itself?
>>> This should be "store the struct acpi_device pointer itself", sorry.
>> Hi,
>> So let me explain the reasoning here:
>>
>> 1) I've had a pleasure to work with different drivers in ACPI directory. In my
>> opinion the best ones I've seen, were build around the concept of struct
>> device (e.g NFIT). It's a struct that's well understood in the kernel, and
>> it's easier to understand without having to read any ACPI specific code.
>> If I see something like ACPI_HANDLE(dev), I think 'ah-ha - having a struct
>> device I can easily get struct acpi_device - they're connected'. And I think
>> using a standardized macro, instead of manually dereferencing pointers is
>> also much easier for ACPI newbs reading the code, as it hides a bit complexity
>> of getting acpi device from struct device. And to be honest I don't think there would
>> be any measurable performance change, as those code paths are far from
>> being considered 'hot paths'. So if we can get the code easier to understand
>> from a newcomer perspective, why not do it.
> I have a differing opinion on a couple of points above, and let's make
> one change at a time.
OK
>
>> 2) I think it would be good to stick to the convention, and introduce some coding
>> pattern, for now some drivers store the struct device pointer, and other store
>> acpi device pointer . As I'm doing this change acpi device pointer become less
>> relevant, as we're using platform device. So to reflect that loss of relevance
>> we can choose to adhere to a pattern where we use it less and less, and the
>> winning approach would be to use 'struct device' by default everywhere we can
>> so maybe eventually we would be able to lose acpi_device altogether at some point,
>> as most of the usage is to retrieve acpi handle and execute some AML method.
>> So in my understanding acpi device is already obsolete at this point, if we can
>> manage to use it less and less, and eventually wipe it out then why not ;-)
> No, ACPI device is not obsolete, it will still be needed for various
> things, like power management and hotplug.
Sure, haven't reviewed all that use cases yet, but the name 'acpi device'
implies like it's part of a driver framework, and it won't be anymore after
transformations are completed.
>
> Also, I'd rather store a struct acpi_device than acpi_handle, because
> the latter is much better from the compile-time type correctness
> checks and similar.
Sure that makes sense
>
> I can send my version of the $subject patch just fine if you strongly
> disagree with me.
Well I can disagree, but still change it ;-). Just explained my reasoning so
you can make a decision with all the data points provided.
Thanks a lot !
Michał
>
On Mon, Oct 9, 2023 at 3:04 PM Wilczynski, Michal
<[email protected]> wrote:
>
>
>
> On 10/9/2023 2:27 PM, Rafael J. Wysocki wrote:
> > On Mon, Oct 9, 2023 at 10:40 AM Wilczynski, Michal
> > <[email protected]> wrote:
> >>
[cut]
> >> Yeah we could add platform device without removing acpi device, and
> >> yes that would introduce data duplication, like Andy noticed.
> > But he hasn't answered my question regarding what data duplication he
> > meant, exactly.
> >
> > So what data duplication do you mean?
>
> So what I meant was that many drivers would find it useful to have
> a struct device embedded in their 'private structure', and in that case
> there would be a duplication of platform_device and acpi_device as
> both pointers would be needed.
It all depends on how often each of them is going to be used in the
given driver.
This particular driver only needs a struct acpi_device pointer if I'm
not mistaken.
> Mind this if you only have struct device
> it's easy to get acpi device, but it's not the case the other way around.
>
> So for this driver it's just a matter of sticking to convention,
There is no convention in this respect and there is always a tradeoff
between using more memory and using more CPU time in computing in
general, but none of them should be wasted just for the sake of
following a convention.
> for the others like it would be duplication.
So I'm only talking about the driver modified by the patch at hand.
> In my version of this patch we managed to avoid this duplication, thanks
> to the contextual argument introduced before, but look at this patch:
> https://github.com/mwilczy/linux-pm/commit/cc8ef52707341e67a12067d6ead991d56ea017ca
>
> Author of this patch had to introduce platform_device and acpi_device to the struct ac, so
> there was some duplication. That is the case for many drivers to come, so I decided it's better
> to change this and have a pattern with keeping only 'struct device'.
Well, if the only thing you need from a struct device is its
ACPI_COMPANION(), it is better to store a pointer to the latter.
On 10/6/2023 7:30 PM, Michal Wilczynski wrote:
> NFIT driver uses struct acpi_driver incorrectly to register itself.
> This is wrong as the instances of the ACPI devices are not meant
> to be literal devices, they're supposed to describe ACPI entry of a
> particular device.
>
> Use platform_driver instead of acpi_driver. In relevant places call
> platform devices instances pdev to make a distinction with ACPI
> devices instances.
>
> NFIT driver uses devm_*() family of functions extensively. This change
> has no impact on correct functioning of the whole devm_*() family of
> functions, since the lifecycle of the device stays the same. It is still
> being created during the enumeration, and destroyed on platform device
> removal.
>
> Suggested-by: Rafael J. Wysocki <[email protected]>
> Signed-off-by: Michal Wilczynski <[email protected]>
Hi Dan,
Rafael already reviewed this patch series and merged patches
that he likes. We're waiting on your input regarding two NFIT
commits in this series.
Thanks a lot !
Michał
> ---
> drivers/acpi/nfit/core.c | 34 ++++++++++++++++++----------------
> 1 file changed, 18 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> index 942b84d94078..fb0bc16fa186 100644
> --- a/drivers/acpi/nfit/core.c
> +++ b/drivers/acpi/nfit/core.c
> @@ -15,6 +15,7 @@
> #include <linux/sort.h>
> #include <linux/io.h>
> #include <linux/nd.h>
> +#include <linux/platform_device.h>
> #include <asm/cacheflush.h>
> #include <acpi/nfit.h>
> #include "intel.h"
> @@ -98,7 +99,7 @@ static struct acpi_device *to_acpi_dev(struct acpi_nfit_desc *acpi_desc)
> || strcmp(nd_desc->provider_name, "ACPI.NFIT") != 0)
> return NULL;
>
> - return to_acpi_device(acpi_desc->dev);
> + return ACPI_COMPANION(acpi_desc->dev);
> }
>
> static int xlat_bus_status(void *buf, unsigned int cmd, u32 status)
> @@ -3284,11 +3285,11 @@ static void acpi_nfit_put_table(void *table)
>
> static void acpi_nfit_notify(acpi_handle handle, u32 event, void *data)
> {
> - struct acpi_device *adev = data;
> + struct device *dev = data;
>
> - device_lock(&adev->dev);
> - __acpi_nfit_notify(&adev->dev, handle, event);
> - device_unlock(&adev->dev);
> + device_lock(dev);
> + __acpi_nfit_notify(dev, handle, event);
> + device_unlock(dev);
> }
>
> static void acpi_nfit_remove_notify_handler(void *data)
> @@ -3329,11 +3330,12 @@ void acpi_nfit_shutdown(void *data)
> }
> EXPORT_SYMBOL_GPL(acpi_nfit_shutdown);
>
> -static int acpi_nfit_add(struct acpi_device *adev)
> +static int acpi_nfit_probe(struct platform_device *pdev)
> {
> struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER, NULL };
> struct acpi_nfit_desc *acpi_desc;
> - struct device *dev = &adev->dev;
> + struct device *dev = &pdev->dev;
> + struct acpi_device *adev = ACPI_COMPANION(dev);
> struct acpi_table_header *tbl;
> acpi_status status = AE_OK;
> acpi_size sz;
> @@ -3360,7 +3362,7 @@ static int acpi_nfit_add(struct acpi_device *adev)
> acpi_desc = devm_kzalloc(dev, sizeof(*acpi_desc), GFP_KERNEL);
> if (!acpi_desc)
> return -ENOMEM;
> - acpi_nfit_desc_init(acpi_desc, &adev->dev);
> + acpi_nfit_desc_init(acpi_desc, dev);
>
> /* Save the acpi header for exporting the revision via sysfs */
> acpi_desc->acpi_header = *tbl;
> @@ -3391,7 +3393,7 @@ static int acpi_nfit_add(struct acpi_device *adev)
> return rc;
>
> rc = acpi_dev_install_notify_handler(adev, ACPI_DEVICE_NOTIFY,
> - acpi_nfit_notify, adev);
> + acpi_nfit_notify, dev);
> if (rc)
> return rc;
>
> @@ -3475,11 +3477,11 @@ static const struct acpi_device_id acpi_nfit_ids[] = {
> };
> MODULE_DEVICE_TABLE(acpi, acpi_nfit_ids);
>
> -static struct acpi_driver acpi_nfit_driver = {
> - .name = KBUILD_MODNAME,
> - .ids = acpi_nfit_ids,
> - .ops = {
> - .add = acpi_nfit_add,
> +static struct platform_driver acpi_nfit_driver = {
> + .probe = acpi_nfit_probe,
> + .driver = {
> + .name = KBUILD_MODNAME,
> + .acpi_match_table = acpi_nfit_ids,
> },
> };
>
> @@ -3517,7 +3519,7 @@ static __init int nfit_init(void)
> return -ENOMEM;
>
> nfit_mce_register();
> - ret = acpi_bus_register_driver(&acpi_nfit_driver);
> + ret = platform_driver_register(&acpi_nfit_driver);
> if (ret) {
> nfit_mce_unregister();
> destroy_workqueue(nfit_wq);
> @@ -3530,7 +3532,7 @@ static __init int nfit_init(void)
> static __exit void nfit_exit(void)
> {
> nfit_mce_unregister();
> - acpi_bus_unregister_driver(&acpi_nfit_driver);
> + platform_driver_unregister(&acpi_nfit_driver);
> destroy_workqueue(nfit_wq);
> WARN_ON(!list_empty(&acpi_descs));
> }
On Tue, Oct 17, 2023 at 12:45 PM Wilczynski, Michal
<[email protected]> wrote:
>
>
> On 10/6/2023 7:30 PM, Michal Wilczynski wrote:
> > NFIT driver uses struct acpi_driver incorrectly to register itself.
> > This is wrong as the instances of the ACPI devices are not meant
> > to be literal devices, they're supposed to describe ACPI entry of a
> > particular device.
> >
> > Use platform_driver instead of acpi_driver. In relevant places call
> > platform devices instances pdev to make a distinction with ACPI
> > devices instances.
> >
> > NFIT driver uses devm_*() family of functions extensively. This change
> > has no impact on correct functioning of the whole devm_*() family of
> > functions, since the lifecycle of the device stays the same. It is still
> > being created during the enumeration, and destroyed on platform device
> > removal.
> >
> > Suggested-by: Rafael J. Wysocki <[email protected]>
> > Signed-off-by: Michal Wilczynski <[email protected]>
>
> Hi Dan,
> Rafael already reviewed this patch series and merged patches
> that he likes. We're waiting on your input regarding two NFIT
> commits in this series.
I actually haven't looked at the NFIT patches in this series myself
and this is not urgent at all IMV.
Thanks!
On Fri, Oct 6, 2023 at 8:33 PM Michal Wilczynski
<[email protected]> wrote:
>
> NFIT driver uses struct acpi_driver incorrectly to register itself.
> This is wrong as the instances of the ACPI devices are not meant
> to be literal devices, they're supposed to describe ACPI entry of a
> particular device.
>
> Use platform_driver instead of acpi_driver. In relevant places call
> platform devices instances pdev to make a distinction with ACPI
> devices instances.
>
> NFIT driver uses devm_*() family of functions extensively. This change
> has no impact on correct functioning of the whole devm_*() family of
> functions, since the lifecycle of the device stays the same. It is still
> being created during the enumeration, and destroyed on platform device
> removal.
>
> Suggested-by: Rafael J. Wysocki <[email protected]>
> Signed-off-by: Michal Wilczynski <[email protected]>
> ---
> drivers/acpi/nfit/core.c | 34 ++++++++++++++++++----------------
> 1 file changed, 18 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> index 942b84d94078..fb0bc16fa186 100644
> --- a/drivers/acpi/nfit/core.c
> +++ b/drivers/acpi/nfit/core.c
> @@ -15,6 +15,7 @@
> #include <linux/sort.h>
> #include <linux/io.h>
> #include <linux/nd.h>
> +#include <linux/platform_device.h>
> #include <asm/cacheflush.h>
> #include <acpi/nfit.h>
> #include "intel.h"
> @@ -98,7 +99,7 @@ static struct acpi_device *to_acpi_dev(struct acpi_nfit_desc *acpi_desc)
> || strcmp(nd_desc->provider_name, "ACPI.NFIT") != 0)
> return NULL;
>
> - return to_acpi_device(acpi_desc->dev);
> + return ACPI_COMPANION(acpi_desc->dev);
> }
>
> static int xlat_bus_status(void *buf, unsigned int cmd, u32 status)
> @@ -3284,11 +3285,11 @@ static void acpi_nfit_put_table(void *table)
>
> static void acpi_nfit_notify(acpi_handle handle, u32 event, void *data)
> {
> - struct acpi_device *adev = data;
> + struct device *dev = data;
>
> - device_lock(&adev->dev);
> - __acpi_nfit_notify(&adev->dev, handle, event);
> - device_unlock(&adev->dev);
> + device_lock(dev);
> + __acpi_nfit_notify(dev, handle, event);
> + device_unlock(dev);
Careful here.
The ACPI device locking is changed to platform device locking without
a word of explanation in the changelog.
Do you actually know what the role of the locking around
__acpi_nfit_notify() is and whether or not it can be replaced with
platform device locking safely?
> }
>
> static void acpi_nfit_remove_notify_handler(void *data)
> @@ -3329,11 +3330,12 @@ void acpi_nfit_shutdown(void *data)
> }
> EXPORT_SYMBOL_GPL(acpi_nfit_shutdown);
>
> -static int acpi_nfit_add(struct acpi_device *adev)
> +static int acpi_nfit_probe(struct platform_device *pdev)
> {
> struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER, NULL };
> struct acpi_nfit_desc *acpi_desc;
> - struct device *dev = &adev->dev;
> + struct device *dev = &pdev->dev;
> + struct acpi_device *adev = ACPI_COMPANION(dev);
> struct acpi_table_header *tbl;
> acpi_status status = AE_OK;
> acpi_size sz;
> @@ -3360,7 +3362,7 @@ static int acpi_nfit_add(struct acpi_device *adev)
> acpi_desc = devm_kzalloc(dev, sizeof(*acpi_desc), GFP_KERNEL);
> if (!acpi_desc)
> return -ENOMEM;
> - acpi_nfit_desc_init(acpi_desc, &adev->dev);
> + acpi_nfit_desc_init(acpi_desc, dev);
You seem to think that replacing adev->dev with pdev->dev everywhere
in this driver will work,
Have you verified that in any way? If so, then how?
>
> /* Save the acpi header for exporting the revision via sysfs */
> acpi_desc->acpi_header = *tbl;
> @@ -3391,7 +3393,7 @@ static int acpi_nfit_add(struct acpi_device *adev)
> return rc;
>
> rc = acpi_dev_install_notify_handler(adev, ACPI_DEVICE_NOTIFY,
> - acpi_nfit_notify, adev);
> + acpi_nfit_notify, dev);
> if (rc)
> return rc;
>
> @@ -3475,11 +3477,11 @@ static const struct acpi_device_id acpi_nfit_ids[] = {
> };
> MODULE_DEVICE_TABLE(acpi, acpi_nfit_ids);
>
> -static struct acpi_driver acpi_nfit_driver = {
> - .name = KBUILD_MODNAME,
> - .ids = acpi_nfit_ids,
> - .ops = {
> - .add = acpi_nfit_add,
> +static struct platform_driver acpi_nfit_driver = {
> + .probe = acpi_nfit_probe,
> + .driver = {
> + .name = KBUILD_MODNAME,
> + .acpi_match_table = acpi_nfit_ids,
> },
> };
>
> @@ -3517,7 +3519,7 @@ static __init int nfit_init(void)
> return -ENOMEM;
>
> nfit_mce_register();
> - ret = acpi_bus_register_driver(&acpi_nfit_driver);
> + ret = platform_driver_register(&acpi_nfit_driver);
> if (ret) {
> nfit_mce_unregister();
> destroy_workqueue(nfit_wq);
> @@ -3530,7 +3532,7 @@ static __init int nfit_init(void)
> static __exit void nfit_exit(void)
> {
> nfit_mce_unregister();
> - acpi_bus_unregister_driver(&acpi_nfit_driver);
> + platform_driver_unregister(&acpi_nfit_driver);
> destroy_workqueue(nfit_wq);
> WARN_ON(!list_empty(&acpi_descs));
> }
> --
Michal Wilczynski wrote:
> NFIT driver uses struct acpi_driver incorrectly to register itself.
> This is wrong as the instances of the ACPI devices are not meant
> to be literal devices, they're supposed to describe ACPI entry of a
> particular device.
>
> Use platform_driver instead of acpi_driver. In relevant places call
> platform devices instances pdev to make a distinction with ACPI
> devices instances.
>
> NFIT driver uses devm_*() family of functions extensively. This change
> has no impact on correct functioning of the whole devm_*() family of
> functions, since the lifecycle of the device stays the same. It is still
> being created during the enumeration, and destroyed on platform device
> removal.
I notice this verbiage has the same fundamental misunderstanding of devm
allocation lifetime as the acpi_nfit_init_interleave_set() discussion.
The devm allocation lifetime typically starts in driver->probe() and
ends either with driver->probe() failure, or the driver->remove() call.
Note that the driver->remove() call is invoked not only for
platform-device removal, but also driver "unbind" events. So the
"destroyed on platform device removal" is the least likely way that
these allocations are torn down given ACPI0012 devices are never
removed.
Outside of that, my main concern about this patch is that I expect it
breaks unit tests. The infrastructure in
tools/testing/nvdimm/test/nfit.c emulates an ACPI0012 device that allows
for deeper regression testing given hardware is difficult to come by,
and because QEMU does not implement some of the tricky corner cases that
the unit tests cover.
This needs to pass tests, but fair warning,
tools/testing/nvdimm/test/nfit.c does some non-idiomatic + "strange"
things to achieve deeper test coverage. So I expect that if this breaks
tests as I expect the effort needed to fix the emulation could be
significant.
If you want to give running the tests a try the easiest would be to use
"run_qemu.sh" with --nfit-test option [1], or you can try to setup an
environment manually using the ndctl instructions [2].
[1]: https://github.com/pmem/run_qemu
[2]: https://github.com/pmem/ndctl#readme
On 10/17/2023 8:24 PM, Dan Williams wrote:
> Michal Wilczynski wrote:
>> NFIT driver uses struct acpi_driver incorrectly to register itself.
>> This is wrong as the instances of the ACPI devices are not meant
>> to be literal devices, they're supposed to describe ACPI entry of a
>> particular device.
>>
>> Use platform_driver instead of acpi_driver. In relevant places call
>> platform devices instances pdev to make a distinction with ACPI
>> devices instances.
>>
>> NFIT driver uses devm_*() family of functions extensively. This change
>> has no impact on correct functioning of the whole devm_*() family of
>> functions, since the lifecycle of the device stays the same. It is still
>> being created during the enumeration, and destroyed on platform device
>> removal.
> I notice this verbiage has the same fundamental misunderstanding of devm
> allocation lifetime as the acpi_nfit_init_interleave_set() discussion.
> The devm allocation lifetime typically starts in driver->probe() and
> ends either with driver->probe() failure, or the driver->remove() call.
> Note that the driver->remove() call is invoked not only for
> platform-device removal, but also driver "unbind" events. So the
> "destroyed on platform device removal" is the least likely way that
> these allocations are torn down given ACPI0012 devices are never
> removed.
>
> Outside of that, my main concern about this patch is that I expect it
> breaks unit tests. The infrastructure in
> tools/testing/nvdimm/test/nfit.c emulates an ACPI0012 device that allows
> for deeper regression testing given hardware is difficult to come by,
> and because QEMU does not implement some of the tricky corner cases that
> the unit tests cover.
>
> This needs to pass tests, but fair warning,
> tools/testing/nvdimm/test/nfit.c does some non-idiomatic + "strange"
> things to achieve deeper test coverage. So I expect that if this breaks
> tests as I expect the effort needed to fix the emulation could be
> significant.
>
> If you want to give running the tests a try the easiest would be to use
> "run_qemu.sh" with --nfit-test option [1], or you can try to setup an
> environment manually using the ndctl instructions [2].
>
> [1]: https://github.com/pmem/run_qemu
> [2]: https://github.com/pmem/ndctl#readme
Thanks a lot !
I will run qemu tests and fix the verbiage,
Michał