Re: [PATCH v3] dax/kmem: Pass valid argument to memory_group_register_static
Hi,
This is just a gentle reminder, If any other information is needed.
Tarun Sahu writes:
> memory_group_register_static takes maximum number of pages as the argument
> while dev_dax_kmem_probe passes total_len (in bytes) as the argument.
>
> IIUC, I don't see any crash/panic impact as such. As,
> memory_group_register_static just set the max_pages limit which is used in
> auto_movable_zone_for_pfn to determine the zone.
>
> which might cause these condition to behave differently,
>
> This will be true always so jump will happen to kernel_zone
> ...
> if (!auto_movable_can_online_movable(NUMA_NO_NODE, group, nr_pages))
> goto kernel_zone;
>
> ...
> kernel_zone:
> return default_kernel_zone_for_pfn(nid, pfn, nr_pages);
>
> Here, In below, zone_intersects compare range will be larger as nr_pages
> will be higher (derived from total_len passed in dev_dax_kmem_probe).
>
> ...
> static struct zone *default_kernel_zone_for_pfn(int nid, unsigned long
> start_pfn,
> unsigned long nr_pages)
> {
> struct pglist_data *pgdat = NODE_DATA(nid);
> int zid;
>
> for (zid = 0; zid < ZONE_NORMAL; zid++) {
> struct zone *zone = >node_zones[zid];
>
> if (zone_intersects(zone, start_pfn, nr_pages))
> return zone;
> }
>
> return >node_zones[ZONE_NORMAL];
> }
>
> Incorrect zone will be returned here, which in later time might cause bigger
> problem.
>
> Fixes: eedf634aac3b ("dax/kmem: use a single static memory group for a single
> probed unit")
> Signed-off-by: Tarun Sahu
> Reviewed-by: Vishal Verma
> ---
> V3<-V2
> 1. Removed skip characters.
>
> V2<-V1
> 1. Added more details to commit message
>
>
> drivers/dax/kmem.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/dax/kmem.c b/drivers/dax/kmem.c
> index 7b36db6f1cbd..898ca9505754 100644
> --- a/drivers/dax/kmem.c
> +++ b/drivers/dax/kmem.c
> @@ -99,7 +99,7 @@ static int dev_dax_kmem_probe(struct dev_dax *dev_dax)
> if (!data->res_name)
> goto err_res_name;
>
> - rc = memory_group_register_static(numa_node, total_len);
> + rc = memory_group_register_static(numa_node, PFN_UP(total_len));
> if (rc < 0)
> goto err_reg_mgid;
> data->mgid = rc;
> --
> 2.31.1
~Tarun
RE: [PATCH v5 09/10] acpi/nfit: Move handler installing logic to driver
Michal Wilczynski wrote:
> Currently logic for installing notifications from ACPI devices is
> implemented using notify callback in struct acpi_driver. Preparations
> are being made to replace acpi_driver with more generic struct
> platform_driver, which doesn't contain notify callback. Furthermore
> as of now handlers are being called indirectly through
> acpi_notify_device(), which decreases performance.
>
> Call acpi_dev_install_notify_handler() at the end of .add() callback.
> Call acpi_dev_remove_notify_handler() at the beginning of .remove()
> callback. Change arguments passed to the notify function to match with
> what's required by acpi_install_notify_handler(). Remove .notify
> callback initialization in acpi_driver.
>
> Suggested-by: Rafael J. Wysocki
> Signed-off-by: Michal Wilczynski
> ---
> drivers/acpi/nfit/core.c | 24 ++--
> 1 file changed, 18 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> index 95930e9d776c..a281bdfee8a0 100644
> --- a/drivers/acpi/nfit/core.c
> +++ b/drivers/acpi/nfit/core.c
> @@ -3312,11 +3312,13 @@ void acpi_nfit_shutdown(void *data)
> }
> EXPORT_SYMBOL_GPL(acpi_nfit_shutdown);
>
> -static void acpi_nfit_notify(struct acpi_device *adev, u32 event)
> +static void acpi_nfit_notify(acpi_handle handle, u32 event, void *data)
> {
> - device_lock(>dev);
> - __acpi_nfit_notify(>dev, adev->handle, event);
> - device_unlock(>dev);
> + struct acpi_device *device = data;
> +
> + device_lock(>dev);
> + __acpi_nfit_notify(>dev, handle, event);
> + device_unlock(>dev);
> }
>
> static int acpi_nfit_add(struct acpi_device *adev)
> @@ -3375,12 +3377,23 @@ static int acpi_nfit_add(struct acpi_device *adev)
>
> if (rc)
> return rc;
> - return devm_add_action_or_reset(dev, acpi_nfit_shutdown, acpi_desc);
> +
> + rc = devm_add_action_or_reset(dev, acpi_nfit_shutdown, acpi_desc);
> + if (rc)
> + return rc;
> +
> + return acpi_dev_install_notify_handler(adev,
> +ACPI_DEVICE_NOTIFY,
> +acpi_nfit_notify);
> }
>
> static void acpi_nfit_remove(struct acpi_device *adev)
> {
> /* see acpi_nfit_unregister */
> +
> + acpi_dev_remove_notify_handler(adev,
> +ACPI_DEVICE_NOTIFY,
> +acpi_nfit_notify);
Please use devm to trigger this release rather than making
acpi_nfit_remove() contain any logic.
An additional cleanup opportunity with the ->add() path fully devm
instrumented would be to just delete acpi_nfit_remove() since it is
optional and serves no purpose.
RE: [PATCH v5 08/10] acpi/nfit: Improve terminator line in acpi_nfit_ids
Michal Wilczynski wrote:
> Currently terminator line contains redunant characters. Remove them and
> also remove a comma at the end.
>
> Signed-off-by: Michal Wilczynski
> ---
> drivers/acpi/nfit/core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> index aff79cbc2190..95930e9d776c 100644
> --- a/drivers/acpi/nfit/core.c
> +++ b/drivers/acpi/nfit/core.c
> @@ -3455,7 +3455,7 @@ EXPORT_SYMBOL_GPL(__acpi_nfit_notify);
>
> static const struct acpi_device_id acpi_nfit_ids[] = {
> { "ACPI0012", 0 },
> - { "", 0 },
> + {}
Looks like a pointless change to me.
Re: [PATCH v5 09/10] acpi/nfit: Move handler installing logic to driver
On Fri, Jun 16, 2023 at 6:51 PM Michal Wilczynski
wrote:
>
> Currently logic for installing notifications from ACPI devices is
> implemented using notify callback in struct acpi_driver. Preparations
> are being made to replace acpi_driver with more generic struct
> platform_driver, which doesn't contain notify callback. Furthermore
> as of now handlers are being called indirectly through
> acpi_notify_device(), which decreases performance.
>
> Call acpi_dev_install_notify_handler() at the end of .add() callback.
> Call acpi_dev_remove_notify_handler() at the beginning of .remove()
> callback. Change arguments passed to the notify function to match with
> what's required by acpi_install_notify_handler(). Remove .notify
> callback initialization in acpi_driver.
>
> Suggested-by: Rafael J. Wysocki
> Signed-off-by: Michal Wilczynski
> ---
> drivers/acpi/nfit/core.c | 24 ++--
> 1 file changed, 18 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> index 95930e9d776c..a281bdfee8a0 100644
> --- a/drivers/acpi/nfit/core.c
> +++ b/drivers/acpi/nfit/core.c
> @@ -3312,11 +3312,13 @@ void acpi_nfit_shutdown(void *data)
> }
> EXPORT_SYMBOL_GPL(acpi_nfit_shutdown);
>
> -static void acpi_nfit_notify(struct acpi_device *adev, u32 event)
> +static void acpi_nfit_notify(acpi_handle handle, u32 event, void *data)
> {
> - device_lock(>dev);
> - __acpi_nfit_notify(>dev, adev->handle, event);
> - device_unlock(>dev);
It's totally not necessary to rename the ACPI device variable here.
Just add
struct acpi_device *adev = data;
to this function.
> + struct acpi_device *device = data;
> +
> + device_lock(>dev);
> + __acpi_nfit_notify(>dev, handle, event);
> + device_unlock(>dev);
> }
>
> static int acpi_nfit_add(struct acpi_device *adev)
> @@ -3375,12 +3377,23 @@ static int acpi_nfit_add(struct acpi_device *adev)
>
> if (rc)
> return rc;
> - return devm_add_action_or_reset(dev, acpi_nfit_shutdown, acpi_desc);
> +
> + rc = devm_add_action_or_reset(dev, acpi_nfit_shutdown, acpi_desc);
> + if (rc)
> + return rc;
> +
> + return acpi_dev_install_notify_handler(adev,
> + ACPI_DEVICE_NOTIFY,
> + acpi_nfit_notify);
> }
>
> static void acpi_nfit_remove(struct acpi_device *adev)
> {
> /* see acpi_nfit_unregister */
> +
> + acpi_dev_remove_notify_handler(adev,
> + ACPI_DEVICE_NOTIFY,
> + acpi_nfit_notify);
> }
>
> static void acpi_nfit_update_notify(struct device *dev, acpi_handle handle)
> @@ -3465,7 +3478,6 @@ static struct acpi_driver acpi_nfit_driver = {
> .ops = {
> .add = acpi_nfit_add,
> .remove = acpi_nfit_remove,
> - .notify = acpi_nfit_notify,
> },
> };
>
> --
Re: [PATCH v5 08/10] acpi/nfit: Improve terminator line in acpi_nfit_ids
On Fri, Jun 16, 2023 at 6:51 PM Michal Wilczynski
wrote:
>
> Currently terminator line contains redunant characters.
Well, they are terminating the list properly AFAICS, so they aren't
redundant and the size of it before and after the change is actually
the same, isn't it?
> Remove them and also remove a comma at the end.
I suppose that this change is made for consistency with the other ACPI
code, so this would be the motivation to give in the changelog.
In any case, it doesn't seem to be related to the rest of the series.
> Signed-off-by: Michal Wilczynski
> ---
> drivers/acpi/nfit/core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> index aff79cbc2190..95930e9d776c 100644
> --- a/drivers/acpi/nfit/core.c
> +++ b/drivers/acpi/nfit/core.c
> @@ -3455,7 +3455,7 @@ EXPORT_SYMBOL_GPL(__acpi_nfit_notify);
>
> static const struct acpi_device_id acpi_nfit_ids[] = {
> { "ACPI0012", 0 },
> - { "", 0 },
> + {}
> };
> MODULE_DEVICE_TABLE(acpi, acpi_nfit_ids);
>
> --
> 2.41.0
>
Re: [PATCH v5 07/10] acpi/nfit: Move acpi_nfit_notify() before acpi_nfit_add()
On Fri, Jun 16, 2023 at 6:51 PM Michal Wilczynski
wrote:
>
> To use new style of installing event handlers acpi_nfit_notify() needs
> to be known inside acpi_nfit_add(). Move acpi_nfit_notify() upwards in
> the file, so it can be used inside acpi_nfit_add().
>
> Signed-off-by: Michal Wilczynski
> ---
> drivers/acpi/nfit/core.c | 14 +++---
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> index 07204d482968..aff79cbc2190 100644
> --- a/drivers/acpi/nfit/core.c
> +++ b/drivers/acpi/nfit/core.c
> @@ -3312,6 +3312,13 @@ void acpi_nfit_shutdown(void *data)
> }
> EXPORT_SYMBOL_GPL(acpi_nfit_shutdown);
>
> +static void acpi_nfit_notify(struct acpi_device *adev, u32 event)
> +{
> + device_lock(>dev);
> + __acpi_nfit_notify(>dev, adev->handle, event);
> + device_unlock(>dev);
> +}
> +
> static int acpi_nfit_add(struct acpi_device *adev)
> {
> struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER, NULL };
> @@ -3446,13 +3453,6 @@ void __acpi_nfit_notify(struct device *dev,
> acpi_handle handle, u32 event)
> }
> EXPORT_SYMBOL_GPL(__acpi_nfit_notify);
>
> -static void acpi_nfit_notify(struct acpi_device *adev, u32 event)
> -{
> - device_lock(>dev);
> - __acpi_nfit_notify(>dev, adev->handle, event);
> - device_unlock(>dev);
> -}
> -
> static const struct acpi_device_id acpi_nfit_ids[] = {
> { "ACPI0012", 0 },
> { "", 0 },
> --
Please fold this patch into the next one. By itself, it is an
artificial change IMV.
Re: [PATCH v5 05/10] acpi/battery: Move handler installing logic to driver
On Fri, Jun 16, 2023 at 6:51 PM Michal Wilczynski
wrote:
>
> Currently logic for installing notifications from ACPI devices is
> implemented using notify callback in struct acpi_driver. Preparations
> are being made to replace acpi_driver with more generic struct
> platform_driver, which doesn't contain notify callback. Furthermore
> as of now handlers are being called indirectly through
> acpi_notify_device(), which decreases performance.
>
> Call acpi_dev_install_notify_handler() at the end of .add() callback.
> Call acpi_dev_remove_notify_handler() at the beginning of .remove()
> callback. Change arguments passed to the notify function to match with
> what's required by acpi_install_notify_handler(). Remove .notify
> callback initialization in acpi_driver.
>
> While at it, fix lack of whitespaces in .remove() callback.
>
> Suggested-by: Rafael J. Wysocki
> Signed-off-by: Michal Wilczynski
> ---
> drivers/acpi/battery.c | 30 --
> 1 file changed, 24 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
> index 9c67ed02d797..6337e7b1f6e1 100644
> --- a/drivers/acpi/battery.c
> +++ b/drivers/acpi/battery.c
> @@ -1034,11 +1034,14 @@ static void acpi_battery_refresh(struct acpi_battery
> *battery)
> }
>
> /* Driver Interface */
> -static void acpi_battery_notify(struct acpi_device *device, u32 event)
> +static void acpi_battery_notify(acpi_handle handle, u32 event, void *data)
> {
> - struct acpi_battery *battery = acpi_driver_data(device);
> + struct acpi_device *device = data;
> + struct acpi_battery *battery;
> struct power_supply *old;
>
> + battery = acpi_driver_data(device);
> +
> if (!battery)
> return;
> old = battery->bat;
> @@ -1212,13 +1215,23 @@ static int acpi_battery_add(struct acpi_device
> *device)
>
> device_init_wakeup(>dev, 1);
>
> - return result;
> + result = acpi_dev_install_notify_handler(device,
> +ACPI_ALL_NOTIFY,
> +acpi_battery_notify);
> + if (result)
> + goto fail_deinit_wakup_and_unregister;
You could call the label "fail_pm", for example, which would be more
concise and so slightly easier to follow, without any loss of clarity
AFAICS.
> +
> + return 0;
>
> +fail_deinit_wakup_and_unregister:
> + device_init_wakeup(>dev, 0);
> + unregister_pm_notifier(>pm_nb);
> fail:
> sysfs_remove_battery(battery);
> mutex_destroy(>lock);
> mutex_destroy(>sysfs_lock);
> kfree(battery);
> +
> return result;
> }
>
> @@ -1228,10 +1241,17 @@ static void acpi_battery_remove(struct acpi_device
> *device)
>
> if (!device || !acpi_driver_data(device))
> return;
> - device_init_wakeup(>dev, 0);
> +
> battery = acpi_driver_data(device);
> +
> + acpi_dev_remove_notify_handler(device,
> + ACPI_ALL_NOTIFY,
> + acpi_battery_notify);
> +
> + device_init_wakeup(>dev, 0);
> unregister_pm_notifier(>pm_nb);
> sysfs_remove_battery(battery);
> +
> mutex_destroy(>lock);
> mutex_destroy(>sysfs_lock);
> kfree(battery);
> @@ -1264,11 +1284,9 @@ static struct acpi_driver acpi_battery_driver = {
> .name = "battery",
> .class = ACPI_BATTERY_CLASS,
> .ids = battery_device_ids,
> - .flags = ACPI_DRIVER_ALL_NOTIFY_EVENTS,
> .ops = {
> .add = acpi_battery_add,
> .remove = acpi_battery_remove,
> - .notify = acpi_battery_notify,
> },
> .drv.pm = _battery_pm,
> };
> --
> 2.41.0
>
Re: [PATCH v5 04/10] acpi/video: Move handler installing logic to driver
On Fri, Jun 16, 2023 at 6:51 PM Michal Wilczynski
wrote:
>
> Currently logic for installing notifications from ACPI devices is
> implemented using notify callback in struct acpi_driver. Preparations
> are being made to replace acpi_driver with more generic struct
> platform_driver, which doesn't contain notify callback. Furthermore
> as of now handlers are being called indirectly through
> acpi_notify_device(), which decreases performance.
>
> Call acpi_dev_install_notify_handler() at the end of .add() callback.
> Call acpi_dev_remove_notify_handler() at the beginning of .remove()
> callback. Change arguments passed to the notify function to match with
> what's required by acpi_install_notify_handler(). Remove .notify
> callback initialization in acpi_driver.
>
> Suggested-by: Rafael J. Wysocki
> Signed-off-by: Michal Wilczynski
> ---
> drivers/acpi/acpi_video.c | 26 ++
> 1 file changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
> index 62f4364e4460..60b7013d0009 100644
> --- a/drivers/acpi/acpi_video.c
> +++ b/drivers/acpi/acpi_video.c
> @@ -77,7 +77,7 @@ static DEFINE_MUTEX(video_list_lock);
> static LIST_HEAD(video_bus_head);
> static int acpi_video_bus_add(struct acpi_device *device);
> static void acpi_video_bus_remove(struct acpi_device *device);
> -static void acpi_video_bus_notify(struct acpi_device *device, u32 event);
> +static void acpi_video_bus_notify(acpi_handle handle, u32 event, void *data);
>
> /*
> * Indices in the _BCL method response: the first two items are special,
> @@ -104,7 +104,6 @@ static struct acpi_driver acpi_video_bus = {
> .ops = {
> .add = acpi_video_bus_add,
> .remove = acpi_video_bus_remove,
> - .notify = acpi_video_bus_notify,
> },
> };
>
> @@ -1527,12 +1526,15 @@ static int acpi_video_bus_stop_devices(struct
> acpi_video_bus *video)
> acpi_osi_is_win8() ? 0 : 1);
> }
>
> -static void acpi_video_bus_notify(struct acpi_device *device, u32 event)
> +static void acpi_video_bus_notify(acpi_handle handle, u32 event, void *data)
> {
> - struct acpi_video_bus *video = acpi_driver_data(device);
> + struct acpi_device *device = data;
> + struct acpi_video_bus *video;
> struct input_dev *input;
> int keycode = 0;
>
> + video = acpi_driver_data(device);
> +
> if (!video || !video->input)
> return;
>
> @@ -2053,8 +2055,20 @@ static int acpi_video_bus_add(struct acpi_device
> *device)
>
> acpi_video_bus_add_notify_handler(video);
>
> + error = acpi_dev_install_notify_handler(device,
> + ACPI_DEVICE_NOTIFY,
> + acpi_video_bus_notify);
> + if (error)
> + goto err_remove_and_unregister_video;
This label name is a bit too long and the second half of it doesn't
really add any value IMV. err_remove would be sufficient.
> +
> return 0;
>
> +err_remove_and_unregister_video:
> + mutex_lock(_list_lock);
> + list_del(>entry);
> + mutex_unlock(_list_lock);
> + acpi_video_bus_remove_notify_handler(video);
> + acpi_video_bus_unregister_backlight(video);
> err_put_video:
> acpi_video_bus_put_devices(video);
> kfree(video->attached_array);
> @@ -2075,6 +2089,10 @@ static void acpi_video_bus_remove(struct acpi_device
> *device)
>
> video = acpi_driver_data(device);
>
> + acpi_dev_remove_notify_handler(device,
> + ACPI_DEVICE_NOTIFY,
> + acpi_video_bus_notify);
> +
> mutex_lock(_list_lock);
> list_del(>entry);
> mutex_unlock(_list_lock);
> --
> 2.41.0
>
Re: [PATCH v5 03/10] acpi/ac: Move handler installing logic to driver
On Fri, Jun 16, 2023 at 6:51 PM Michal Wilczynski
wrote:
>
> Currently logic for installing notifications from ACPI devices is
> implemented using notify callback in struct acpi_driver. Preparations
> are being made to replace acpi_driver with more generic struct
> platform_driver, which doesn't contain notify callback. Furthermore
> as of now handlers are being called indirectly through
> acpi_notify_device(), which decreases performance.
>
> Call acpi_dev_install_notify_handler() at the end of .add() callback.
> Call acpi_dev_remove_notify_handler() at the beginning of .remove()
> callback. Change arguments passed to the notify function to match with
> what's required by acpi_install_notify_handler(). Remove .notify
> callback initialization in acpi_driver.
>
> Suggested-by: Rafael J. Wysocki
> Signed-off-by: Michal Wilczynski
> ---
> drivers/acpi/ac.c | 33 -
> 1 file changed, 24 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/acpi/ac.c b/drivers/acpi/ac.c
> index 1ace70b831cd..207ee3c85bad 100644
> --- a/drivers/acpi/ac.c
> +++ b/drivers/acpi/ac.c
> @@ -34,7 +34,7 @@ MODULE_LICENSE("GPL");
>
> static int acpi_ac_add(struct acpi_device *device);
> static void acpi_ac_remove(struct acpi_device *device);
> -static void acpi_ac_notify(struct acpi_device *device, u32 event);
> +static void acpi_ac_notify(acpi_handle handle, u32 event, void *data);
>
> static const struct acpi_device_id ac_device_ids[] = {
> {"ACPI0003", 0},
> @@ -54,11 +54,9 @@ static struct acpi_driver acpi_ac_driver = {
> .name = "ac",
> .class = ACPI_AC_CLASS,
> .ids = ac_device_ids,
> - .flags = ACPI_DRIVER_ALL_NOTIFY_EVENTS,
> .ops = {
> .add = acpi_ac_add,
> .remove = acpi_ac_remove,
> - .notify = acpi_ac_notify,
> },
> .drv.pm = _ac_pm,
> };
> @@ -128,9 +126,12 @@ static enum power_supply_property ac_props[] = {
> };
>
> /* Driver Model */
> -static void acpi_ac_notify(struct acpi_device *device, u32 event)
> +static void acpi_ac_notify(acpi_handle handle, u32 event, void *data)
> {
> - struct acpi_ac *ac = acpi_driver_data(device);
This line doesn't need to be changed. Just add the device variable
definition above it.
And the same pattern is present in the other patches in the series.
> + struct acpi_device *device = data;
> + struct acpi_ac *ac;
> +
> + ac = acpi_driver_data(device);
>
> if (!ac)
> return;
