Re: [PATCH] tracing/treewide: Remove second parameter of __assign_str()
On Thu, May 16, 2024 at 7:35 PM Steven Rostedt wrote: > > From: "Steven Rostedt (Google)" > > [ >This is a treewide change. I will likely re-create this patch again in >the second week of the merge window of v6.10 and submit it then. Hoping >to keep the conflicts that it will cause to a minimum. > ] > > With the rework of how the __string() handles dynamic strings where it > saves off the source string in field in the helper structure[1], the > assignment of that value to the trace event field is stored in the helper > value and does not need to be passed in again. > > This means that with: > > __string(field, mystring) > > Which use to be assigned with __assign_str(field, mystring), no longer > needs the second parameter and it is unused. With this, __assign_str() > will now only get a single parameter. > > There's over 700 users of __assign_str() and because coccinelle does not > handle the TRACE_EVENT() macro I ended up using the following sed script: > > git grep -l __assign_str | while read a ; do > sed -e 's/\(__assign_str([^,]*[^ ,]\) *,[^;]*/\1)/' $a > /tmp/test-file; > mv /tmp/test-file $a; > done > > I then searched for __assign_str() that did not end with ';' as those > were multi line assignments that the sed script above would fail to catch. > > Note, the same updates will need to be done for: > > __assign_str_len() > __assign_rel_str() > __assign_rel_str_len() > > I tested this with both an allmodconfig and an allyesconfig (build only for > both). > > [1] > https://lore.kernel.org/linux-trace-kernel/2024011442.634192...@goodmis.org/ > > Cc: Masami Hiramatsu > Cc: Mathieu Desnoyers > Cc: Linus Torvalds > Cc: Julia Lawall > Signed-off-by: Steven Rostedt (Google) Acked-by: Rafael J. Wysocki # for thermal
Re: [PATCH v2] PM: runtime: add tracepoint for runtime_status changes
On Wed, Feb 21, 2024 at 8:49 PM Vilas Bhat wrote:
>
> Existing runtime PM ftrace events (`rpm_suspend`, `rpm_resume`,
> `rpm_return_int`) offer limited visibility into the exact timing of device
> runtime power state transitions, particularly when asynchronous operations
> are involved. When the `rpm_suspend` or `rpm_resume` functions are invoked
> with the `RPM_ASYNC` flag, a return value of 0 i.e., success merely
> indicates that the device power state request has been queued, not that
> the device has yet transitioned.
>
> A new ftrace event, `rpm_status`, is introduced. This event directly logs
> the `power.runtime_status` value of a device whenever it changes providing
> granular tracking of runtime power state transitions regardless of
> synchronous or asynchronous `rpm_suspend` / `rpm_resume` usage.
>
> Signed-off-by: Vilas Bhat
> ---
> V1 -> V2: Modified enum value definition as per reviewer comments.
> ---
> drivers/base/power/runtime.c | 1 +
> include/trace/events/rpm.h | 42
> 2 files changed, 43 insertions(+)
>
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index 05793c9fbb84..d10354847878 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -94,6 +94,7 @@ static void update_pm_runtime_accounting(struct device *dev)
> static void __update_runtime_status(struct device *dev, enum rpm_status
> status)
> {
> update_pm_runtime_accounting(dev);
> + trace_rpm_status(dev, status);
> dev->power.runtime_status = status;
> }
>
> diff --git a/include/trace/events/rpm.h b/include/trace/events/rpm.h
> index 3c716214dab1..bd120e23ce12 100644
> --- a/include/trace/events/rpm.h
> +++ b/include/trace/events/rpm.h
> @@ -101,6 +101,48 @@ TRACE_EVENT(rpm_return_int,
> __entry->ret)
> );
>
> +#define RPM_STATUS_STRINGS \
> + EM(RPM_INVALID, "RPM_INVALID") \
> + EM(RPM_ACTIVE, "RPM_ACTIVE") \
> + EM(RPM_RESUMING, "RPM_RESUMING") \
> + EM(RPM_SUSPENDED, "RPM_SUSPENDED") \
> + EMe(RPM_SUSPENDING, "RPM_SUSPENDING")
> +
> +/* Enums require being exported to userspace, for user tool parsing. */
> +#undef EM
> +#undef EMe
> +#define EM(a, b) TRACE_DEFINE_ENUM(a);
> +#define EMe(a, b) TRACE_DEFINE_ENUM(a);
> +
> +RPM_STATUS_STRINGS
> +
> +/*
> + * Now redefine the EM() and EMe() macros to map the enums to the strings
> that
> + * will be printed in the output.
> + */
> +#undef EM
> +#undef EMe
> +#define EM(a, b) { a, b },
> +#define EMe(a, b) { a, b }
> +
> +TRACE_EVENT(rpm_status,
> + TP_PROTO(struct device *dev, enum rpm_status status),
> + TP_ARGS(dev, status),
> +
> + TP_STRUCT__entry(
> + __string(name, dev_name(dev))
> + __field(int,status)
> + ),
> +
> + TP_fast_assign(
> + __assign_str(name, dev_name(dev));
> + __entry->status = status;
> + ),
> +
> + TP_printk("%s status=%s", __get_str(name),
> + __print_symbolic(__entry->status, RPM_STATUS_STRINGS))
> +);
> +
> #endif /* _TRACE_RUNTIME_POWER_H */
>
> /* This part must be outside protection */
> --
Applied as 6.9 material, thanks!
Re: [PATCH v1 1/1] ACPI: NFIT: Switch to use acpi_evaluate_dsm_typed()
On Fri, Feb 2, 2024 at 4:49 PM Andy Shevchenko wrote: > > On Mon, Nov 20, 2023 at 07:19:44PM +0200, Andy Shevchenko wrote: > > On Mon, Nov 20, 2023 at 04:11:54PM +0100, Rafael J. Wysocki wrote: > > > On Mon, Nov 20, 2023 at 4:03 PM Andy Shevchenko > > > wrote: > > > > On Thu, Oct 19, 2023 at 06:03:28PM -0700, Dan Williams wrote: > > > > > Andy Shevchenko wrote: > > > > > > The acpi_evaluate_dsm_typed() provides a way to check the type of > > > > > > the > > > > > > object evaluated by _DSM call. Use it instead of open coded variant. > > > > > > > > > > Looks good to me. > > > > > > > > > > Reviewed-by: Dan Williams > > > > > > > > Thank you! > > > > > > > > Who is taking care of this? Rafael? > > > > > > I can apply it. > > > > Would be nice, thank you! > > Any news on this? Fell through the cracks, sorry about that. Applied now (as 6.9 material). Thanks!
Re: [PATCH v2 1/5] cpufreq: Add a cpufreq pressure feedback for the scheduler
On Thu, Dec 21, 2023 at 8:57 PM Rafael J. Wysocki wrote:
>
> On Thu, Dec 21, 2023 at 4:24 PM Vincent Guittot
> wrote:
> >
> > Provide to the scheduler a feedback about the temporary max available
> > capacity. Unlike arch_update_thermal_pressure, this doesn't need to be
> > filtered as the pressure will happen for dozens ms or more.
> >
> > Signed-off-by: Vincent Guittot
> > ---
> > drivers/cpufreq/cpufreq.c | 34 ++
> > include/linux/cpufreq.h | 10 ++
> > 2 files changed, 44 insertions(+)
> >
> > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > index 44db4f59c4cc..15bd41f9bb5e 100644
> > --- a/drivers/cpufreq/cpufreq.c
> > +++ b/drivers/cpufreq/cpufreq.c
> > @@ -2563,6 +2563,38 @@ int cpufreq_get_policy(struct cpufreq_policy
> > *policy, unsigned int cpu)
> > }
> > EXPORT_SYMBOL(cpufreq_get_policy);
> >
> > +DEFINE_PER_CPU(unsigned long, cpufreq_pressure);
> > +
> > +/**
> > + * cpufreq_update_pressure() - Update cpufreq pressure for CPUs
> > + * @policy: cpufreq policy of the CPUs.
> > + *
> > + * Update the value of cpufreq pressure for all @cpus in the policy.
> > + */
> > +static void cpufreq_update_pressure(struct cpufreq_policy *policy)
> > +{
> > + unsigned long max_capacity, capped_freq, pressure;
> > + u32 max_freq;
> > + int cpu;
> > +
> > + cpu = cpumask_first(policy->related_cpus);
> > + pressure = max_capacity = arch_scale_cpu_capacity(cpu);
>
> I would prefer two separate statements instead of the above.
>
> Other than this
>
> Acked-by: Rafael J. Wysocki
>
> > + capped_freq = policy->max;
> > + max_freq = arch_scale_freq_ref(cpu);
> > +
> > + /*
> > +* Handle properly the boost frequencies, which should simply clean
> > +* the thermal pressure value.
> > +*/
> > + if (max_freq <= capped_freq)
> > + pressure -= max_capacity;
Actually, it would be somewhat cleaner to do
pressure = 0;
here and
> > + else
> > + pressure -= mult_frac(max_capacity, capped_freq, max_freq);
pressure = max_capacity - mult_frac(max_capacity, capped_freq, max_freq);
and it would not be necessary to initialize pressure.
> > +
> > + for_each_cpu(cpu, policy->related_cpus)
> > + WRITE_ONCE(per_cpu(cpufreq_pressure, cpu), pressure);
> > +}
> > +
> > /**
> > * cpufreq_set_policy - Modify cpufreq policy parameters.
> > * @policy: Policy object to modify.
> > @@ -2618,6 +2650,8 @@ static int cpufreq_set_policy(struct cpufreq_policy
> > *policy,
> > policy->max = __resolve_freq(policy, policy->max,
> > CPUFREQ_RELATION_H);
> > trace_cpu_frequency_limits(policy);
> >
> > + cpufreq_update_pressure(policy);
> > +
> > policy->cached_target_freq = UINT_MAX;
> >
> > pr_debug("new min and max freqs are %u - %u kHz\n",
> > diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
> > index afda5f24d3dd..b1d97edd3253 100644
> > --- a/include/linux/cpufreq.h
> > +++ b/include/linux/cpufreq.h
> > @@ -241,6 +241,12 @@ struct kobject *get_governor_parent_kobj(struct
> > cpufreq_policy *policy);
> > void cpufreq_enable_fast_switch(struct cpufreq_policy *policy);
> > void cpufreq_disable_fast_switch(struct cpufreq_policy *policy);
> > bool has_target_index(void);
> > +
> > +DECLARE_PER_CPU(unsigned long, cpufreq_pressure);
> > +static inline unsigned long cpufreq_get_pressure(int cpu)
> > +{
> > + return per_cpu(cpufreq_pressure, cpu);
> > +}
> > #else
> > static inline unsigned int cpufreq_get(unsigned int cpu)
> > {
> > @@ -263,6 +269,10 @@ static inline bool
> > cpufreq_supports_freq_invariance(void)
> > return false;
> > }
> > static inline void disable_cpufreq(void) { }
> > +static inline unsigned long cpufreq_get_pressure(int cpu)
> > +{
> > + return 0;
> > +}
> > #endif
> >
> > #ifdef CONFIG_CPU_FREQ_STAT
> > --
> > 2.34.1
> >
Re: [PATCH v2 1/5] cpufreq: Add a cpufreq pressure feedback for the scheduler
On Thu, Dec 21, 2023 at 4:24 PM Vincent Guittot
wrote:
>
> Provide to the scheduler a feedback about the temporary max available
> capacity. Unlike arch_update_thermal_pressure, this doesn't need to be
> filtered as the pressure will happen for dozens ms or more.
>
> Signed-off-by: Vincent Guittot
> ---
> drivers/cpufreq/cpufreq.c | 34 ++
> include/linux/cpufreq.h | 10 ++
> 2 files changed, 44 insertions(+)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 44db4f59c4cc..15bd41f9bb5e 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -2563,6 +2563,38 @@ int cpufreq_get_policy(struct cpufreq_policy *policy,
> unsigned int cpu)
> }
> EXPORT_SYMBOL(cpufreq_get_policy);
>
> +DEFINE_PER_CPU(unsigned long, cpufreq_pressure);
> +
> +/**
> + * cpufreq_update_pressure() - Update cpufreq pressure for CPUs
> + * @policy: cpufreq policy of the CPUs.
> + *
> + * Update the value of cpufreq pressure for all @cpus in the policy.
> + */
> +static void cpufreq_update_pressure(struct cpufreq_policy *policy)
> +{
> + unsigned long max_capacity, capped_freq, pressure;
> + u32 max_freq;
> + int cpu;
> +
> + cpu = cpumask_first(policy->related_cpus);
> + pressure = max_capacity = arch_scale_cpu_capacity(cpu);
I would prefer two separate statements instead of the above.
Other than this
Acked-by: Rafael J. Wysocki
> + capped_freq = policy->max;
> + max_freq = arch_scale_freq_ref(cpu);
> +
> + /*
> +* Handle properly the boost frequencies, which should simply clean
> +* the thermal pressure value.
> +*/
> + if (max_freq <= capped_freq)
> + pressure -= max_capacity;
> + else
> + pressure -= mult_frac(max_capacity, capped_freq, max_freq);
> +
> + for_each_cpu(cpu, policy->related_cpus)
> + WRITE_ONCE(per_cpu(cpufreq_pressure, cpu), pressure);
> +}
> +
> /**
> * cpufreq_set_policy - Modify cpufreq policy parameters.
> * @policy: Policy object to modify.
> @@ -2618,6 +2650,8 @@ static int cpufreq_set_policy(struct cpufreq_policy
> *policy,
> policy->max = __resolve_freq(policy, policy->max, CPUFREQ_RELATION_H);
> trace_cpu_frequency_limits(policy);
>
> + cpufreq_update_pressure(policy);
> +
> policy->cached_target_freq = UINT_MAX;
>
> pr_debug("new min and max freqs are %u - %u kHz\n",
> diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
> index afda5f24d3dd..b1d97edd3253 100644
> --- a/include/linux/cpufreq.h
> +++ b/include/linux/cpufreq.h
> @@ -241,6 +241,12 @@ struct kobject *get_governor_parent_kobj(struct
> cpufreq_policy *policy);
> void cpufreq_enable_fast_switch(struct cpufreq_policy *policy);
> void cpufreq_disable_fast_switch(struct cpufreq_policy *policy);
> bool has_target_index(void);
> +
> +DECLARE_PER_CPU(unsigned long, cpufreq_pressure);
> +static inline unsigned long cpufreq_get_pressure(int cpu)
> +{
> + return per_cpu(cpufreq_pressure, cpu);
> +}
> #else
> static inline unsigned int cpufreq_get(unsigned int cpu)
> {
> @@ -263,6 +269,10 @@ static inline bool cpufreq_supports_freq_invariance(void)
> return false;
> }
> static inline void disable_cpufreq(void) { }
> +static inline unsigned long cpufreq_get_pressure(int cpu)
> +{
> + return 0;
> +}
> #endif
>
> #ifdef CONFIG_CPU_FREQ_STAT
> --
> 2.34.1
>
Re: [PATCH 1/4] cpufreq: Add a cpufreq pressure feedback for the scheduler
On Thu, Dec 14, 2023 at 10:07 AM Lukasz Luba wrote: > > On 12/14/23 07:57, Vincent Guittot wrote: > > On Thu, 14 Dec 2023 at 06:43, Viresh Kumar wrote: > >> > >> On 12-12-23, 15:27, Vincent Guittot wrote: > >>> @@ -2618,6 +2663,9 @@ static int cpufreq_set_policy(struct cpufreq_policy > >>> *policy, > >>>policy->max = __resolve_freq(policy, policy->max, > >>> CPUFREQ_RELATION_H); > >>>trace_cpu_frequency_limits(policy); > >>> > >>> + cpus = policy->related_cpus; > >>> + cpufreq_update_pressure(cpus, policy->max); > >>> + > >>>policy->cached_target_freq = UINT_MAX; > >> > >> One more question, why are you doing this from cpufreq_set_policy ? If > >> due to cpufreq cooling or from userspace, we end up limiting the > >> maximum possible frequency, will this routine always get called ? > > > > Yes, any update of a FREQ_QOS_MAX ends up calling cpufreq_set_policy() > > to update the policy->max > > > > Agree, cpufreq sysfs scaling_max_freq is also important to handle > in this new design. Currently we don't reflect that as reduced CPU > capacity in the scheduler. There was discussion when I proposed to feed > that CPU frequency reduction into thermal_pressure [1]. > > The same applies for the DTPM which is missing currently the proper > impact to the CPU reduced capacity in the scheduler. > > IMHO any limit set into FREQ_QOS_MAX should be visible in this > new design of capacity reduction signaling. > > [1] https://lore.kernel.org/lkml/20220930094821.31665-2-lukasz.l...@arm.com/ Actually, freq_qos_read_value(>constraints, FREQ_QOS_MAX) will return the requisite limit.
Re: [PATCH v1 1/1] ACPI: NFIT: Switch to use acpi_evaluate_dsm_typed()
On Mon, Nov 20, 2023 at 4:03 PM Andy Shevchenko wrote: > > On Thu, Oct 19, 2023 at 06:03:28PM -0700, Dan Williams wrote: > > Andy Shevchenko wrote: > > > The acpi_evaluate_dsm_typed() provides a way to check the type of the > > > object evaluated by _DSM call. Use it instead of open coded variant. > > > > Looks good to me. > > > > Reviewed-by: Dan Williams > > Thank you! > > Who is taking care of this? Rafael? I can apply it.
[GIT PULL] ACPI fix for v6.6-rc8
Hi Linus, Please pull from the tag git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git \ acpi-6.6-rc8 with top-most commit 9b311b7313d6c104dd4a2d43ab54536dce07f960 ACPI: NFIT: Install Notify() handler before getting NFIT table on top of commit f20f29cbcb438ca37962d22735f74a143cbeb28c Merge tag 'acpi-6.6-rc7' of git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm to receive an ACPI fix for 6.6. This unbreaks the ACPI NFIT driver after a recent change that inadvertently altered its behavior (Xiang Chen). Thanks! --- Xiang Chen (1): ACPI: NFIT: Install Notify() handler before getting NFIT table --- drivers/acpi/nfit/core.c | 22 +++--- 1 file changed, 11 insertions(+), 11 deletions(-)
Re: [PATCH v2] ACPI: NFIT: Optimize nfit_mem_cmp() for efficiency
On Fri, Oct 13, 2023 at 2:22 PM Kuan-Wei Chiu wrote: > > The original code used conditional branching in the nfit_mem_cmp > function to compare two values and return -1, 1, or 0 based on the > result. However, the list_sort comparison function only needs results > <0, >0, or =0. This patch optimizes the code to make the comparison > branchless, improving efficiency and reducing code size. This change > reduces the number of comparison operations from 1-2 to a single > subtraction operation, thereby saving the number of instructions. > > Signed-off-by: Kuan-Wei Chiu > --- > v1 -> v2: > - Add explicit type cast in case the sizes of u32 and int differ. > > drivers/acpi/nfit/core.c | 6 +- > 1 file changed, 1 insertion(+), 5 deletions(-) > > diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c > index f96bf32cd368..563a32eba888 100644 > --- a/drivers/acpi/nfit/core.c > +++ b/drivers/acpi/nfit/core.c > @@ -1138,11 +1138,7 @@ static int nfit_mem_cmp(void *priv, const struct > list_head *_a, > > handleA = __to_nfit_memdev(a)->device_handle; > handleB = __to_nfit_memdev(b)->device_handle; > - if (handleA < handleB) > - return -1; > - else if (handleA > handleB) > - return 1; > - return 0; > + return (int)handleA - (int)handleB; Are you sure that you are not losing bits in these conversions? > } > > static int nfit_mem_init(struct acpi_nfit_desc *acpi_desc) > --
Re: [PATCH v3] ACPI: NFIT: Use cleanup.h helpers instead of devm_*()
On Wed, Oct 18, 2023 at 6:28 AM Dan Williams wrote: > > Michal Wilczynski wrote: > > The new cleanup.h facilities that arrived in v6.5-rc1 can replace the > > the usage of devm semantics in acpi_nfit_init_interleave_set(). That > > routine appears to only be using devm to avoid goto statements. The > > new __free() annotation at variable declaration time can achieve the same > > effect more efficiently. > > > > There is no end user visible side effects of this patch, I was > > motivated to send this cleanup to practice using the new helpers. > > > > Suggested-by: Dave Jiang > > Suggested-by: Andy Shevchenko > > Reviewed-by: Dave Jiang > > Reviewed-by: Andy Shevchenko > > Signed-off-by: Michal Wilczynski > > --- > > > > Dan, would you like me to give you credit for the changelog changes > > with Co-developed-by tag ? > > Nope, this looks good to me, thanks for fixing it up. > > Reviewed-by: Dan Williams Are you going to apply it too, or should I take it?
Re: [PATCH v2 5/6] ACPI: NFIT: Replace acpi_driver with platform_driver
On Fri, Oct 6, 2023 at 8:33 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
> Signed-off-by: Michal Wilczynski
> ---
> 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
> #include
> #include
> +#include
> #include
> #include
> #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(>dev);
> - __acpi_nfit_notify(>dev, handle, event);
> - device_unlock(>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 = >dev;
> + struct device *dev = >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, >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
Re: [PATCH v3 4/6] ACPI: AC: Rename ACPI device from device to adev
On Wed, Oct 11, 2023 at 10:34 AM Michal Wilczynski
wrote:
>
> 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
> ---
> drivers/acpi/ac.c | 24
> 1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/acpi/ac.c b/drivers/acpi/ac.c
> index 1dd4919be7ac..2618a7ccc11c 100644
> --- a/drivers/acpi/ac.c
> +++ b/drivers/acpi/ac.c
> @@ -121,11 +121,11 @@ static void acpi_ac_notify(acpi_handle handle, u32
> event, void *data)
> {
> struct device *dev = data;
> struct acpi_ac *ac = dev_get_drvdata(dev);
> - struct acpi_device *device = ACPI_COMPANION(dev);
> + struct acpi_device *adev = ACPI_COMPANION(dev);
>
> switch (event) {
> default:
> - acpi_handle_debug(device->handle, "Unsupported event
> [0x%x]\n",
> + acpi_handle_debug(adev->handle, "Unsupported event [0x%x]\n",
> event);
> fallthrough;
> case ACPI_AC_NOTIFY_STATUS:
> @@ -142,11 +142,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(dev),
> event,
> ac->state);
> - acpi_notifier_call_chain(device, event, ac->state);
> + acpi_notifier_call_chain(adev, event, ac->state);
> kobject_uevent(>charger->dev.kobj, KOBJ_CHANGE);
> }
> }
> @@ -205,7 +205,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(>dev);
> + struct acpi_device *adev = ACPI_COMPANION(>dev);
> struct power_supply_config psy_cfg = {};
> struct acpi_ac *ac;
> int result;
> @@ -214,9 +214,9 @@ static int acpi_ac_probe(struct platform_device *pdev)
> if (!ac)
> return -ENOMEM;
>
> - ac->device = device;
> - strcpy(acpi_device_name(device), ACPI_AC_DEVICE_NAME);
> - strcpy(acpi_device_class(device), ACPI_AC_CLASS);
> + ac->device = adev;
> + strcpy(acpi_device_name(adev), ACPI_AC_DEVICE_NAME);
> + strcpy(acpi_device_class(adev), ACPI_AC_CLASS);
>
> platform_set_drvdata(pdev, ac);
>
> @@ -226,7 +226,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);
> @@ -238,13 +238,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(>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, >dev);
> if (result)
> goto err_unregister;
> --
Rebased on top of current linux-next and applied as 6.7 material, thanks!
Re: [PATCH v2 5/6] ACPI: NFIT: Replace acpi_driver with platform_driver
On Tue, Oct 17, 2023 at 12:45 PM Wilczynski, Michal 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 > > Signed-off-by: Michal Wilczynski > > 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!
Re: [PATCH v3 2/6] ACPI: AC: Use string_choices API instead of ternary operator
On Wed, Oct 11, 2023 at 10:34 AM Michal Wilczynski
wrote:
>
> Use modern string_choices API instead of manually determining the
> output using ternary operator.
>
> Suggested-by: Andy Shevchenko
> Reviewed-by: Andy Shevchenko
> Signed-off-by: Michal Wilczynski
> ---
> 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
> #include
> #include
> +#include
> #include
> #include
>
> @@ -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(>battery_nb);
> --
Applied as 6.7 material, thanks!
Re: [PATCH v3 1/6] ACPI: AC: Remove unnecessary checks
On Wed, Oct 11, 2023 at 10:34 AM Michal Wilczynski
wrote:
>
> 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
> ---
> 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);
> --
Applied as 6.7 material with edits in the subject and changelog, thanks!
Re: [PATCH v2 3/6] ACPI: AC: Replace acpi_driver with platform_driver
On Mon, Oct 9, 2023 at 3:04 PM Wilczynski, Michal wrote: > > > > On 10/9/2023 2:27 PM, Rafael J. Wysocki wrote: > > On Mon, Oct 9, 2023 at 10:40 AM Wilczynski, Michal > > 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.
Re: [PATCH v2 3/6] ACPI: AC: Replace acpi_driver with platform_driver
On Mon, Oct 9, 2023 at 10:40 AM Wilczynski, Michal
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 wrote:
> >> On Sat, Oct 7, 2023 at 9:56 AM Andy Shevchenko
> >> 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
> >>>> 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.
Re: [PATCH v2 3/6] ACPI: AC: Replace acpi_driver with platform_driver
On Sat, Oct 7, 2023 at 12:41 PM Rafael J. Wysocki wrote:
>
> On Sat, Oct 7, 2023 at 9:56 AM Andy Shevchenko
> 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
> > > 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.
Re: [PATCH v2 3/6] ACPI: AC: Replace acpi_driver with platform_driver
On Sat, Oct 7, 2023 at 9:56 AM Andy Shevchenko
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
> > 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?
Re: [PATCH v2 3/6] ACPI: AC: Replace acpi_driver with platform_driver
On Fri, Oct 6, 2023 at 8:33 PM 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.
>
> Suggested-by: Rafael J. Wysocki
> Signed-off-by: Michal Wilczynski
> ---
> 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 = _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;
> };
Re: [PATCH v1 2/9] docs: firmware-guide: ACPI: Clarify ACPI bus concepts
On Thu, Oct 5, 2023 at 10:39 PM Wilczynski, Michal wrote: > > > > On 10/5/2023 8:28 PM, Wilczynski, Michal wrote: > > > > On 10/5/2023 7:57 PM, Rafael J. Wysocki wrote: > >> On Monday, September 25, 2023 4:48:35 PM CEST Michal Wilczynski wrote: > >>> Some devices implement ACPI driver as a way to manage devices > >>> enumerated by the ACPI. This might be confusing as a preferred way to > >>> implement a driver for devices not connected to any bus is a platform > >>> driver, as stated in the documentation. Clarify relationships between > >>> ACPI device, platform device and ACPI entries. > >>> > >>> Suggested-by: Elena Reshetova > >>> Signed-off-by: Michal Wilczynski > >>> --- > >>> Documentation/firmware-guide/acpi/enumeration.rst | 13 + > >>> 1 file changed, 13 insertions(+) > >>> > >>> diff --git a/Documentation/firmware-guide/acpi/enumeration.rst > >>> b/Documentation/firmware-guide/acpi/enumeration.rst > >>> index 56d9913a3370..f56cc79a9e83 100644 > >>> --- a/Documentation/firmware-guide/acpi/enumeration.rst > >>> +++ b/Documentation/firmware-guide/acpi/enumeration.rst > >>> @@ -64,6 +64,19 @@ If the driver needs to perform more complex > >>> initialization like getting and > >>> configuring GPIOs it can get its ACPI handle and extract this information > >>> from ACPI tables. > >>> > >>> +ACPI bus > >>> + > >>> + > >>> +Historically some devices not connected to any bus were represented as > >>> ACPI > >>> +devices, and had to implement ACPI driver. This is not a preferred way > >>> for new > >>> +drivers. As explained above devices not connected to any bus should > >>> implement > >>> +platform driver. ACPI device would be created during enumeration > >>> nonetheless, > >>> +and would be accessible through ACPI_COMPANION() macro, and the ACPI > >>> handle would > >>> +be accessible through ACPI_HANDLE() macro. ACPI device is meant to > >>> describe > >>> +information related to ACPI entry e.g. handle of the ACPI entry. Think - > >>> +ACPI device interfaces with the FW, and the platform device with the > >>> rest of > >>> +the system. > >>> + > >>> DMA support > >>> === > >> I rewrote the above entirely, so here's a new patch to replace this one: > >> > >> --- > >> From: Rafael J. Wysocki > >> Subject: [PATCH v2 2/9] ACPI: docs: enumeration: Clarify ACPI bus concepts > >> > >> In some cases, ACPI drivers are implemented as a way to manage devices > >> enumerated with the help of the platform firmware through ACPI. > >> > >> This might be confusing, since the preferred way to implement a driver > >> for a device that cannot be enumerated natively, is a platform > >> driver, as stated in the documentation. > >> > >> Clarify relationships between ACPI device objects, platform devices and > >> ACPI Namespace entries. > >> > >> Suggested-by: Elena Reshetova > >> Co-developed-by: Michal Wilczynski > >> Signed-off-by: Michal Wilczynski > >> Signed-off-by: Rafael J. Wysocki > >> --- > >> Documentation/firmware-guide/acpi/enumeration.rst | 43 > >> ++ > >> 1 file changed, 43 insertions(+) > >> > >> Index: linux-pm/Documentation/firmware-guide/acpi/enumeration.rst > >> === > >> --- linux-pm.orig/Documentation/firmware-guide/acpi/enumeration.rst > >> +++ linux-pm/Documentation/firmware-guide/acpi/enumeration.rst > >> @@ -64,6 +64,49 @@ If the driver needs to perform more comp > >> configuring GPIOs it can get its ACPI handle and extract this information > >> from ACPI tables. > >> > >> +ACPI device objects > >> +=== > >> + > >> +Generally speaking, there are two categories of devices in a system in > >> which > >> +ACPI is used as an interface between the platform firmware and the OS: > >> Devices > >> +that can be discovered and enumerated natively, through a protocol > >> defined for > >> +the specific bus that they are on (for example, configuration space in > >> PCI), > >> +without the pla
Re: [PATCH v1 1/9] ACPI: bus: Make notify wrappers more generic
On Thu, Oct 5, 2023 at 8:27 PM Wilczynski, Michal wrote: > > > > On 10/5/2023 7:03 PM, Rafael J. Wysocki wrote: > > On Thursday, October 5, 2023 5:30:59 PM CEST Rafael J. Wysocki wrote: > >> On Thu, Oct 5, 2023 at 2:05 PM Wilczynski, Michal > >> wrote: > >>> On 10/5/2023 12:57 PM, Rafael J. Wysocki wrote: > >>>> On Thu, Oct 5, 2023 at 10:10 AM Wilczynski, Michal > >>>> wrote: > >> [cut] > >> > >>>>>> That said, why exactly is it better to use acpi_handle instead of a > >>>>>> struct acpi_device pointer? > >>>>> I wanted to make the wrapper as close as possible to the wrapped > >>>>> function. > >>>>> This way it would be easier to remove it in the future i.e if we ever > >>>>> deem > >>>>> extra synchronization not worth it etc. What the ACPICA function need to > >>>>> install a wrapper is a handle not a pointer to a device. > >>>>> So there is no need for a middle man. > >>>> Taking a struct acpi_device pointer as the first argument is part of > >>>> duplication reduction, however, because in the most common case it > >>>> saves the users of it the need to dereference the struct acpi_device > >>>> they get from ACPI_COMPANION() in order to obtain the handle. > >>> User don't even have to use acpi device anywhere, as he can choose > >>> to use ACPI_HANDLE() instead on 'struct device*' and never interact > >>> with acpi device directly. > >> Have you actually looked at this macro? It is a wrapper around > >> ACPI_COMPANION(). > >> > >> So they may think that they don't use struct acpi_device pointers, but > >> in fact they do. > >> > >>>> Arguably, acpi_handle is an ACPICA concept and it is better to reduce > >>>> its usage outside ACPICA. > >>> Use of acpi_handle is deeply entrenched in the kernel. There is even > >>> a macro ACPI_HANDLE() that returns acpi_handle. I would say it's > >>> way too late to limit it to ACPICA internal code. > >> So there is a difference between "limiting to ACPICA" and "reducing". > >> It cannot be limited to ACPICA, because the code outside ACPICA needs > >> to evaluate ACPI objects sometimes and ACPI handles are needed for > >> that. > >> > >> And this observation doesn't invalidate the point. > >> > >>>>>> Realistically, in a platform driver you'll need the latter to obtain > >>>>>> the former anyway. > >>>>> I don't want to introduce arbitrary limitations where they are not > >>>>> necessary. > >>>> I'm not sure what you mean. This patch is changing existing functions. > >>> That's true, but those functions aren't yet deeply entrenched in the > >>> kernel yet, so in my view how they should look like should still be > >>> a subject for discussion, as for now they're only used locally in > >>> drivers/acpi, and my next patchset, that would remove .notify in > >>> platform directory would spread them more, and would > >>> make them harder to change. For now we can change how they > >>> work pretty painlessly. > >> I see no particular reason to do that, though. > >> > >> What specifically is a problem with passing struct acpi_device > >> pointers to the wrappers? I don't see any TBH. > >> > >>>>> It is often the case that driver allocates it's own private struct > >>>>> using kmalloc > >>>>> family of functions, and that structure already contains everything > >>>>> that is > >>>>> needed to remove the handler, so why force ? There are already examples > >>>>> in the drivers that do that i.e in acpi_video the function > >>>>> acpi_video_dev_add_notify_handler() uses raw ACPICA handler to install > >>>>> a notify handler and it passes private structure there. > >>>>> So there is value in leaving the choice of an actual type to the user > >>>>> of the > >>>>> API. > >>>> No, if the user has a pointer to struct acpi_device already, there is > >>>> no difference between passing this and passing the acpi_handle from it > >>>> except for the extra dereference in the latter case. > >>> Dereference would happen anyway in the wrapper, and it doe
Re: [PATCH v1 2/9] docs: firmware-guide: ACPI: Clarify ACPI bus concepts
On Monday, September 25, 2023 4:48:35 PM CEST Michal Wilczynski wrote: > Some devices implement ACPI driver as a way to manage devices > enumerated by the ACPI. This might be confusing as a preferred way to > implement a driver for devices not connected to any bus is a platform > driver, as stated in the documentation. Clarify relationships between > ACPI device, platform device and ACPI entries. > > Suggested-by: Elena Reshetova > Signed-off-by: Michal Wilczynski > --- > Documentation/firmware-guide/acpi/enumeration.rst | 13 + > 1 file changed, 13 insertions(+) > > diff --git a/Documentation/firmware-guide/acpi/enumeration.rst > b/Documentation/firmware-guide/acpi/enumeration.rst > index 56d9913a3370..f56cc79a9e83 100644 > --- a/Documentation/firmware-guide/acpi/enumeration.rst > +++ b/Documentation/firmware-guide/acpi/enumeration.rst > @@ -64,6 +64,19 @@ If the driver needs to perform more complex initialization > like getting and > configuring GPIOs it can get its ACPI handle and extract this information > from ACPI tables. > > +ACPI bus > + > + > +Historically some devices not connected to any bus were represented as ACPI > +devices, and had to implement ACPI driver. This is not a preferred way for > new > +drivers. As explained above devices not connected to any bus should implement > +platform driver. ACPI device would be created during enumeration nonetheless, > +and would be accessible through ACPI_COMPANION() macro, and the ACPI handle > would > +be accessible through ACPI_HANDLE() macro. ACPI device is meant to describe > +information related to ACPI entry e.g. handle of the ACPI entry. Think - > +ACPI device interfaces with the FW, and the platform device with the rest of > +the system. > + > DMA support > === I rewrote the above entirely, so here's a new patch to replace this one: --- From: Rafael J. Wysocki Subject: [PATCH v2 2/9] ACPI: docs: enumeration: Clarify ACPI bus concepts In some cases, ACPI drivers are implemented as a way to manage devices enumerated with the help of the platform firmware through ACPI. This might be confusing, since the preferred way to implement a driver for a device that cannot be enumerated natively, is a platform driver, as stated in the documentation. Clarify relationships between ACPI device objects, platform devices and ACPI Namespace entries. Suggested-by: Elena Reshetova Co-developed-by: Michal Wilczynski Signed-off-by: Michal Wilczynski Signed-off-by: Rafael J. Wysocki --- Documentation/firmware-guide/acpi/enumeration.rst | 43 ++ 1 file changed, 43 insertions(+) Index: linux-pm/Documentation/firmware-guide/acpi/enumeration.rst === --- linux-pm.orig/Documentation/firmware-guide/acpi/enumeration.rst +++ linux-pm/Documentation/firmware-guide/acpi/enumeration.rst @@ -64,6 +64,49 @@ If the driver needs to perform more comp configuring GPIOs it can get its ACPI handle and extract this information from ACPI tables. +ACPI device objects +=== + +Generally speaking, there are two categories of devices in a system in which +ACPI is used as an interface between the platform firmware and the OS: Devices +that can be discovered and enumerated natively, through a protocol defined for +the specific bus that they are on (for example, configuration space in PCI), +without the platform firmware assistance, and devices that need to be described +by the platform firmware so that they can be discovered. Still, for any device +known to the platform firmware, regardless of which category it falls into, +there can be a corresponding ACPI device object in the ACPI Namespace in which +case the Linux kernel will create a struct acpi_device object based on it for +that device. + +Those struct acpi_device objects are never used for binding drivers to natively +discoverable devices, because they are represented by other types of device +objects (for example, struct pci_dev for PCI devices) that are bound to by +device drivers (the corresponding struct acpi_device object is then used as +an additional source of information on the configuration of the given device). +Moreover, the core ACPI device enumeration code creates struct platform_device +objects for the majority of devices that are discovered and enumerated with the +help of the platform firmware and those platform device objects can be bound to +by platform drivers in direct analogy with the natively enumerable devices +case. Therefore it is logically inconsistent and so generally invalid to bind +drivers to struct acpi_device objects, including drivers for devices that are +discovered with the help of the platform firmware. + +Historically, ACPI drivers that bound directly to struct acpi_device objects +were implemented for some devic
Re: [PATCH v1 1/9] ACPI: bus: Make notify wrappers more generic
On Thursday, October 5, 2023 5:30:59 PM CEST Rafael J. Wysocki wrote: > On Thu, Oct 5, 2023 at 2:05 PM Wilczynski, Michal > wrote: > > On 10/5/2023 12:57 PM, Rafael J. Wysocki wrote: > > > On Thu, Oct 5, 2023 at 10:10 AM Wilczynski, Michal > > > wrote: > > [cut] > > > >>> > > >>> That said, why exactly is it better to use acpi_handle instead of a > > >>> struct acpi_device pointer? > > >> I wanted to make the wrapper as close as possible to the wrapped > > >> function. > > >> This way it would be easier to remove it in the future i.e if we ever > > >> deem > > >> extra synchronization not worth it etc. What the ACPICA function need to > > >> install a wrapper is a handle not a pointer to a device. > > >> So there is no need for a middle man. > > > Taking a struct acpi_device pointer as the first argument is part of > > > duplication reduction, however, because in the most common case it > > > saves the users of it the need to dereference the struct acpi_device > > > they get from ACPI_COMPANION() in order to obtain the handle. > > > > User don't even have to use acpi device anywhere, as he can choose > > to use ACPI_HANDLE() instead on 'struct device*' and never interact > > with acpi device directly. > > Have you actually looked at this macro? It is a wrapper around > ACPI_COMPANION(). > > So they may think that they don't use struct acpi_device pointers, but > in fact they do. > > > > > > > Arguably, acpi_handle is an ACPICA concept and it is better to reduce > > > its usage outside ACPICA. > > > > Use of acpi_handle is deeply entrenched in the kernel. There is even > > a macro ACPI_HANDLE() that returns acpi_handle. I would say it's > > way too late to limit it to ACPICA internal code. > > So there is a difference between "limiting to ACPICA" and "reducing". > It cannot be limited to ACPICA, because the code outside ACPICA needs > to evaluate ACPI objects sometimes and ACPI handles are needed for > that. > > And this observation doesn't invalidate the point. > > > > > > >>> Realistically, in a platform driver you'll need the latter to obtain > > >>> the former anyway. > > >> I don't want to introduce arbitrary limitations where they are not > > >> necessary. > > > I'm not sure what you mean. This patch is changing existing functions. > > > > That's true, but those functions aren't yet deeply entrenched in the > > kernel yet, so in my view how they should look like should still be > > a subject for discussion, as for now they're only used locally in > > drivers/acpi, and my next patchset, that would remove .notify in > > platform directory would spread them more, and would > > make them harder to change. For now we can change how they > > work pretty painlessly. > > I see no particular reason to do that, though. > > What specifically is a problem with passing struct acpi_device > pointers to the wrappers? I don't see any TBH. > > > > > > >> It is often the case that driver allocates it's own private struct using > > >> kmalloc > > >> family of functions, and that structure already contains everything that > > >> is > > >> needed to remove the handler, so why force ? There are already examples > > >> in the drivers that do that i.e in acpi_video the function > > >> acpi_video_dev_add_notify_handler() uses raw ACPICA handler to install > > >> a notify handler and it passes private structure there. > > >> So there is value in leaving the choice of an actual type to the user of > > >> the > > >> API. > > > No, if the user has a pointer to struct acpi_device already, there is > > > no difference between passing this and passing the acpi_handle from it > > > except for the extra dereference in the latter case. > > > > Dereference would happen anyway in the wrapper, and it doesn't cause > > any harm anyway for readability in my opinion. And of course you don't > > have to use acpi device at all, you can use ACPI_HANDLE() macro. > > So one can use ACPI_COMPANION() just as well and it is slightly less overhead. > > > > > > > If the user doesn't have a struct acpi_device pointer, let them use > > > the raw ACPICA handler directly and worry about the synchronization > > > themselves. > > > > As mentioned acpi_device pointer is not really required to use the wrapper. > &g
Re: [PATCH v1 1/9] ACPI: bus: Make notify wrappers more generic
On Thu, Oct 5, 2023 at 2:05 PM Wilczynski, Michal wrote: > On 10/5/2023 12:57 PM, Rafael J. Wysocki wrote: > > On Thu, Oct 5, 2023 at 10:10 AM Wilczynski, Michal > > wrote: [cut] > >>> > >>> That said, why exactly is it better to use acpi_handle instead of a > >>> struct acpi_device pointer? > >> I wanted to make the wrapper as close as possible to the wrapped function. > >> This way it would be easier to remove it in the future i.e if we ever deem > >> extra synchronization not worth it etc. What the ACPICA function need to > >> install a wrapper is a handle not a pointer to a device. > >> So there is no need for a middle man. > > Taking a struct acpi_device pointer as the first argument is part of > > duplication reduction, however, because in the most common case it > > saves the users of it the need to dereference the struct acpi_device > > they get from ACPI_COMPANION() in order to obtain the handle. > > User don't even have to use acpi device anywhere, as he can choose > to use ACPI_HANDLE() instead on 'struct device*' and never interact > with acpi device directly. Have you actually looked at this macro? It is a wrapper around ACPI_COMPANION(). So they may think that they don't use struct acpi_device pointers, but in fact they do. > > > > Arguably, acpi_handle is an ACPICA concept and it is better to reduce > > its usage outside ACPICA. > > Use of acpi_handle is deeply entrenched in the kernel. There is even > a macro ACPI_HANDLE() that returns acpi_handle. I would say it's > way too late to limit it to ACPICA internal code. So there is a difference between "limiting to ACPICA" and "reducing". It cannot be limited to ACPICA, because the code outside ACPICA needs to evaluate ACPI objects sometimes and ACPI handles are needed for that. And this observation doesn't invalidate the point. > > > >>> Realistically, in a platform driver you'll need the latter to obtain > >>> the former anyway. > >> I don't want to introduce arbitrary limitations where they are not > >> necessary. > > I'm not sure what you mean. This patch is changing existing functions. > > That's true, but those functions aren't yet deeply entrenched in the > kernel yet, so in my view how they should look like should still be > a subject for discussion, as for now they're only used locally in > drivers/acpi, and my next patchset, that would remove .notify in > platform directory would spread them more, and would > make them harder to change. For now we can change how they > work pretty painlessly. I see no particular reason to do that, though. What specifically is a problem with passing struct acpi_device pointers to the wrappers? I don't see any TBH. > > > >> It is often the case that driver allocates it's own private struct using > >> kmalloc > >> family of functions, and that structure already contains everything that is > >> needed to remove the handler, so why force ? There are already examples > >> in the drivers that do that i.e in acpi_video the function > >> acpi_video_dev_add_notify_handler() uses raw ACPICA handler to install > >> a notify handler and it passes private structure there. > >> So there is value in leaving the choice of an actual type to the user of > >> the > >> API. > > No, if the user has a pointer to struct acpi_device already, there is > > no difference between passing this and passing the acpi_handle from it > > except for the extra dereference in the latter case. > > Dereference would happen anyway in the wrapper, and it doesn't cause > any harm anyway for readability in my opinion. And of course you don't > have to use acpi device at all, you can use ACPI_HANDLE() macro. So one can use ACPI_COMPANION() just as well and it is slightly less overhead. > > > > If the user doesn't have a struct acpi_device pointer, let them use > > the raw ACPICA handler directly and worry about the synchronization > > themselves. > > As mentioned acpi_device pointer is not really required to use the wrapper. > Instead we can use ACPI_HANDLE() macro directly. Look at the usage of > the wrapper in the AC driver [1]. You don't really have to repeat the same argument several times and I know how ACPI_HANDLE() works. Also I don't like some of the things done by this patch. Whoever uses ACPI_HANDLE(), they also use ACPI_COMPANION() which is hidden in the former. If they don't need to store either the acpi_handle or the struct acpi_device pointer, there is no reason at all to use the former instead of the latter. If they get an acpi_handle from somewhere else than ACP
Re: [PATCH v1 1/9] ACPI: bus: Make notify wrappers more generic
On Thu, Oct 5, 2023 at 10:10 AM Wilczynski, Michal wrote: > > Hi, > > Thanks for your review ! > > On 10/4/2023 9:09 PM, Rafael J. Wysocki wrote: > > On Mon, Sep 25, 2023 at 6:31 PM Michal Wilczynski > > wrote: > >> acpi_dev_install_notify_handler() and acpi_dev_remove_notify_handler() > >> are wrappers around ACPICA installers. They are meant to save some > >> duplicated code from drivers. However as we're moving towards drivers > >> operating on platform_device they become a bit inconvenient to use as > >> inside the driver code we mostly want to use driver data of platform > >> device instead of ACPI device. > > That's fair enough, but -> > > > >> Make notify handlers installer wrappers more generic, while still > >> saving some code that would be duplicated otherwise. > >> > >> Reviewed-by: Andy Shevchenko > >> Signed-off-by: Michal Wilczynski > >> --- > >> > >> Notes: > >> So one solution could be to just replace acpi_device with > >> platform_device as an argument in those functions. However I don't > >> believe this is a correct solution, as it is very often the case that > >> drivers declare their own private structures which gets allocated > >> during > >> the .probe() callback, and become the heart of the driver. When drivers > >> do that it makes much more sense to just pass the private structure > >> to the notify handler instead of forcing user to dance with the > >> platform_device or acpi_device. > >> > >> drivers/acpi/ac.c | 6 +++--- > >> drivers/acpi/acpi_video.c | 6 +++--- > >> drivers/acpi/battery.c| 6 +++--- > >> drivers/acpi/bus.c| 14 ++ > >> drivers/acpi/hed.c| 6 +++--- > >> drivers/acpi/nfit/core.c | 6 +++--- > >> drivers/acpi/thermal.c| 6 +++--- > >> include/acpi/acpi_bus.h | 9 - > >> 8 files changed, 28 insertions(+), 31 deletions(-) > >> > >> diff --git a/drivers/acpi/ac.c b/drivers/acpi/ac.c > >> index 225dc6818751..0b245f9f7ec8 100644 > >> --- a/drivers/acpi/ac.c > >> +++ b/drivers/acpi/ac.c > >> @@ -256,8 +256,8 @@ static int acpi_ac_add(struct acpi_device *device) > >> ac->battery_nb.notifier_call = acpi_ac_battery_notify; > >> register_acpi_notifier(>battery_nb); > >> > >> - result = acpi_dev_install_notify_handler(device, ACPI_ALL_NOTIFY, > >> -acpi_ac_notify); > >> + result = acpi_dev_install_notify_handler(device->handle, > >> ACPI_ALL_NOTIFY, > >> +acpi_ac_notify, device); > >> if (result) > >> goto err_unregister; > >> > >> @@ -306,7 +306,7 @@ static void acpi_ac_remove(struct acpi_device *device) > >> > >> ac = acpi_driver_data(device); > >> > >> - acpi_dev_remove_notify_handler(device, ACPI_ALL_NOTIFY, > >> + acpi_dev_remove_notify_handler(device->handle, ACPI_ALL_NOTIFY, > >>acpi_ac_notify); > >> power_supply_unregister(ac->charger); > >> unregister_acpi_notifier(>battery_nb); > >> diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c > >> index 948e31f7ce6e..025c17890127 100644 > >> --- a/drivers/acpi/acpi_video.c > >> +++ b/drivers/acpi/acpi_video.c > >> @@ -2059,8 +2059,8 @@ 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); > >> + error = acpi_dev_install_notify_handler(device->handle, > >> ACPI_DEVICE_NOTIFY, > >> + acpi_video_bus_notify, > >> device); > >> if (error) > >> goto err_remove; > >> > >> @@ -2092,7 +2092,7 @@ 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_dev_remove_notify_handler(device->handle, ACPI_DEVICE_NOTIFY, &
Re: [PATCH v1 1/9] ACPI: bus: Make notify wrappers more generic
On Mon, Sep 25, 2023 at 6:31 PM Michal Wilczynski wrote: > > acpi_dev_install_notify_handler() and acpi_dev_remove_notify_handler() > are wrappers around ACPICA installers. They are meant to save some > duplicated code from drivers. However as we're moving towards drivers > operating on platform_device they become a bit inconvenient to use as > inside the driver code we mostly want to use driver data of platform > device instead of ACPI device. That's fair enough, but -> > Make notify handlers installer wrappers more generic, while still > saving some code that would be duplicated otherwise. > > Reviewed-by: Andy Shevchenko > Signed-off-by: Michal Wilczynski > --- > > Notes: > So one solution could be to just replace acpi_device with > platform_device as an argument in those functions. However I don't > believe this is a correct solution, as it is very often the case that > drivers declare their own private structures which gets allocated during > the .probe() callback, and become the heart of the driver. When drivers > do that it makes much more sense to just pass the private structure > to the notify handler instead of forcing user to dance with the > platform_device or acpi_device. > > drivers/acpi/ac.c | 6 +++--- > drivers/acpi/acpi_video.c | 6 +++--- > drivers/acpi/battery.c| 6 +++--- > drivers/acpi/bus.c| 14 ++ > drivers/acpi/hed.c| 6 +++--- > drivers/acpi/nfit/core.c | 6 +++--- > drivers/acpi/thermal.c| 6 +++--- > include/acpi/acpi_bus.h | 9 - > 8 files changed, 28 insertions(+), 31 deletions(-) > > diff --git a/drivers/acpi/ac.c b/drivers/acpi/ac.c > index 225dc6818751..0b245f9f7ec8 100644 > --- a/drivers/acpi/ac.c > +++ b/drivers/acpi/ac.c > @@ -256,8 +256,8 @@ static int acpi_ac_add(struct acpi_device *device) > ac->battery_nb.notifier_call = acpi_ac_battery_notify; > register_acpi_notifier(>battery_nb); > > - result = acpi_dev_install_notify_handler(device, ACPI_ALL_NOTIFY, > -acpi_ac_notify); > + result = acpi_dev_install_notify_handler(device->handle, > ACPI_ALL_NOTIFY, > +acpi_ac_notify, device); > if (result) > goto err_unregister; > > @@ -306,7 +306,7 @@ static void acpi_ac_remove(struct acpi_device *device) > > ac = acpi_driver_data(device); > > - acpi_dev_remove_notify_handler(device, ACPI_ALL_NOTIFY, > + acpi_dev_remove_notify_handler(device->handle, ACPI_ALL_NOTIFY, >acpi_ac_notify); > power_supply_unregister(ac->charger); > unregister_acpi_notifier(>battery_nb); > diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c > index 948e31f7ce6e..025c17890127 100644 > --- a/drivers/acpi/acpi_video.c > +++ b/drivers/acpi/acpi_video.c > @@ -2059,8 +2059,8 @@ 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); > + error = acpi_dev_install_notify_handler(device->handle, > ACPI_DEVICE_NOTIFY, > + acpi_video_bus_notify, > device); > if (error) > goto err_remove; > > @@ -2092,7 +2092,7 @@ 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_dev_remove_notify_handler(device->handle, ACPI_DEVICE_NOTIFY, >acpi_video_bus_notify); > > mutex_lock(_list_lock); > diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c > index 969bf81e8d54..45dae32a8646 100644 > --- a/drivers/acpi/battery.c > +++ b/drivers/acpi/battery.c > @@ -1213,8 +1213,8 @@ static int acpi_battery_add(struct acpi_device *device) > > device_init_wakeup(>dev, 1); > > - result = acpi_dev_install_notify_handler(device, ACPI_ALL_NOTIFY, > -acpi_battery_notify); > + result = acpi_dev_install_notify_handler(device->handle, > ACPI_ALL_NOTIFY, > +acpi_battery_notify, device); > if (result) > goto fail_pm; > > @@ -1241,7 +1241,7 @@ static void acpi_battery_remove(struct acpi_device > *device) > > battery = acpi_driver_data(device); > > - acpi_dev_remove_notify_handler(device, ACPI_ALL_NOTIFY, > + acpi_dev_remove_notify_handler(device->handle, ACPI_ALL_NOTIFY, >acpi_battery_notify); > > device_init_wakeup(>dev, 0); > diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c > index f41dda2d3493..479fe888d629 100644 > ---
Re: [PATCH v7 0/9] Remove .notify callback in acpi_device_ops
On Mon, Jul 3, 2023 at 10:03 AM Michal Wilczynski wrote: > > *** IMPORTANT *** > This is part 1 - only drivers in acpi directory to ease up review > process. Rest of the drivers will be handled in separate patchsets. > > Currently drivers support ACPI event handlers by defining .notify > callback in acpi_device_ops. This solution is suboptimal as event > handler installer installs intermediary function acpi_notify_device as a > handler in every driver. Also this approach requires extra variable > 'flags' for specifying event types that the driver want to subscribe to. > Additionally this is a pre-work required to align acpi_driver with > platform_driver and eventually replace acpi_driver with platform_driver. > > Remove .notify callback from the acpi_device_ops. Replace it with each > driver installing and removing it's event handlers. > > This is part 1 - only drivers in acpi directory to ease up review > process. > > v7: > - fix warning by declaring acpi_nfit_remove_notify_handler() as static > > v6: > - fixed unnecessary RCT in all drivers, as it's not a purpose of >this patch series > - changed error label names to simplify them > - dropped commit that remove a comma > - squashed commit moving code for nfit > - improved nfit driver to use devm instead of .remove() > - re-based as Rafael changes [1] are merged already > > v5: > - rebased on top of Rafael changes [1], they're not merged yet > - fixed rollback in multiple drivers so they don't leak resources on >failure > - made this part 1, meaning only drivers in acpi directory, rest of >the drivers will be handled in separate patchsets to ease up review > > v4: > - added one commit for previously missed driver sony-laptop, >refactored return statements, added NULL check for event installer > v3: > - lkp still reported some failures for eeepc, fujitsu and >toshiba_bluetooth, fix those > v2: > - fix compilation errors for drivers > > [1]: https://lore.kernel.org/linux-acpi/1847933.atdPhlSkOF@kreacher/ > > Michal Wilczynski (9): > acpi/bus: Introduce wrappers for ACPICA event handler install/remove > acpi/bus: Set driver_data to NULL every time .add() fails > acpi/ac: Move handler installing logic to driver > acpi/video: Move handler installing logic to driver > acpi/battery: Move handler installing logic to driver > acpi/hed: Move handler installing logic to driver > acpi/nfit: Move handler installing logic to driver > acpi/nfit: Remove unnecessary .remove callback > acpi/thermal: Move handler installing logic to driver Dan hasn't spoken up yet, but I went ahead and queued up the patches (with some modifications) for 6.6. I've edited the subjects and rewritten the changelogs and I've adjusted some white space around function calls (nothing major). Thanks!
Re: [PATCH v7 8/9] acpi/nfit: Remove unnecessary .remove callback
On Mon, Jul 3, 2023 at 10:03 AM Michal Wilczynski
wrote:
>
> Nfit driver doesn't use .remove() callback and provide an empty function
> as it's .remove() callback. Remove empty acpi_nfit_remove() and
> initialization of callback.
>
> Suggested-by: Dan Williams
> Signed-off-by: Michal Wilczynski
This one is not strictly related to the rest of the series, but it
does depend on the previous one, so assuming that the previous one is
not objectionable, I suppose I can take them both. Dan?
> ---
> drivers/acpi/nfit/core.c | 6 --
> 1 file changed, 6 deletions(-)
>
> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> index 124e928647d3..16bf17a3d6ff 100644
> --- a/drivers/acpi/nfit/core.c
> +++ b/drivers/acpi/nfit/core.c
> @@ -3402,11 +3402,6 @@ static int acpi_nfit_add(struct acpi_device *adev)
> adev);
> }
>
> -static void acpi_nfit_remove(struct acpi_device *adev)
> -{
> - /* see acpi_nfit_unregister */
> -}
> -
> static void acpi_nfit_update_notify(struct device *dev, acpi_handle handle)
> {
> struct acpi_nfit_desc *acpi_desc = dev_get_drvdata(dev);
> @@ -3488,7 +3483,6 @@ static struct acpi_driver acpi_nfit_driver = {
> .ids = acpi_nfit_ids,
> .ops = {
> .add = acpi_nfit_add,
> - .remove = acpi_nfit_remove,
> },
> };
>
> --
> 2.41.0
>
Re: [PATCH v7 7/9] acpi/nfit: Move handler installing logic to driver
On Mon, Jul 3, 2023 at 10:03 AM 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
> acpi_nfit_shutdown(). Change arguments passed to the notify function to
> match with what's required by acpi_dev_install_notify_handler(). Remove
> .notify callback initialization in acpi_driver.
>
> Introduce a new devm action acpi_nfit_remove_notify_handler.
>
> Move acpi_nfit_notify() upwards in the file, so it can be used inside
> acpi_nfit_add() and acpi_nfit_remove_notify_handler().
Dan, any objections?
> Suggested-by: Rafael J. Wysocki
> Signed-off-by: Michal Wilczynski
> ---
> drivers/acpi/nfit/core.c | 41 +++-
> 1 file changed, 32 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> index 07204d482968..124e928647d3 100644
> --- a/drivers/acpi/nfit/core.c
> +++ b/drivers/acpi/nfit/core.c
> @@ -3282,6 +3282,24 @@ static void acpi_nfit_put_table(void *table)
> acpi_put_table(table);
> }
>
> +static void acpi_nfit_notify(acpi_handle handle, u32 event, void *data)
> +{
> + struct acpi_device *adev = data;
> +
> + device_lock(>dev);
> + __acpi_nfit_notify(>dev, handle, event);
> + device_unlock(>dev);
> +}
> +
> +static void acpi_nfit_remove_notify_handler(void *data)
> +{
> + struct acpi_device *adev = data;
> +
> + acpi_dev_remove_notify_handler(adev,
> + ACPI_DEVICE_NOTIFY,
> + acpi_nfit_notify);
> +}
> +
> void acpi_nfit_shutdown(void *data)
> {
> struct acpi_nfit_desc *acpi_desc = data;
> @@ -3368,7 +3386,20 @@ 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;
> +
> + rc = acpi_dev_install_notify_handler(adev,
> +ACPI_DEVICE_NOTIFY,
> +acpi_nfit_notify);
> + if (rc)
> + return rc;
> +
> + return devm_add_action_or_reset(dev,
> + acpi_nfit_remove_notify_handler,
> + adev);
> }
>
> static void acpi_nfit_remove(struct acpi_device *adev)
> @@ -3446,13 +3477,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 },
> @@ -3465,7 +3489,6 @@ static struct acpi_driver acpi_nfit_driver = {
> .ops = {
> .add = acpi_nfit_add,
> .remove = acpi_nfit_remove,
> - .notify = acpi_nfit_notify,
> },
> };
>
> --
Re: [PATCH] ACPICA: actbl2: change to be16/be32 types for big endian data
On Mon, Jul 3, 2023 at 3:01 PM Ben Dooks wrote:
>
> Some of the fields in struct acpi_nfit_control_region are used in big
> endian format, and thus are generatng warnings from spare where the
> member is passed to one of the conversion functions.
>
> Fix the following sparse warnings by changing the data types:
>
> drivers/acpi/nfit/core.c:1403:41: warning: cast to restricted __be16
> drivers/acpi/nfit/core.c:1403:41: warning: cast to restricted __be16
> drivers/acpi/nfit/core.c:1403:41: warning: cast to restricted __be16
> drivers/acpi/nfit/core.c:1403:41: warning: cast to restricted __be16
> drivers/acpi/nfit/core.c:1412:41: warning: cast to restricted __be16
> drivers/acpi/nfit/core.c:1412:41: warning: cast to restricted __be16
> drivers/acpi/nfit/core.c:1412:41: warning: cast to restricted __be16
> drivers/acpi/nfit/core.c:1412:41: warning: cast to restricted __be16
> drivers/acpi/nfit/core.c:1421:41: warning: cast to restricted __be16
> drivers/acpi/nfit/core.c:1421:41: warning: cast to restricted __be16
> drivers/acpi/nfit/core.c:1421:41: warning: cast to restricted __be16
> drivers/acpi/nfit/core.c:1421:41: warning: cast to restricted __be16
> drivers/acpi/nfit/core.c:1430:41: warning: cast to restricted __be16
> drivers/acpi/nfit/core.c:1430:41: warning: cast to restricted __be16
> drivers/acpi/nfit/core.c:1430:41: warning: cast to restricted __be16
> drivers/acpi/nfit/core.c:1430:41: warning: cast to restricted __be16
> drivers/acpi/nfit/core.c:1440:25: warning: cast to restricted __be16
> drivers/acpi/nfit/core.c:1440:25: warning: cast to restricted __be16
> drivers/acpi/nfit/core.c:1440:25: warning: cast to restricted __be16
> drivers/acpi/nfit/core.c:1440:25: warning: cast to restricted __be16
> drivers/acpi/nfit/core.c:1449:41: warning: cast to restricted __be16
> drivers/acpi/nfit/core.c:1449:41: warning: cast to restricted __be16
> drivers/acpi/nfit/core.c:1449:41: warning: cast to restricted __be16
> drivers/acpi/nfit/core.c:1449:41: warning: cast to restricted __be16
> drivers/acpi/nfit/core.c:1468:41: warning: cast to restricted __le16
> drivers/acpi/nfit/core.c:1502:41: warning: cast to restricted __le16
> drivers/acpi/nfit/core.c:1527:41: warning: cast to restricted __be32
> drivers/acpi/nfit/core.c:1527:41: warning: cast to restricted __be32
> drivers/acpi/nfit/core.c:1527:41: warning: cast to restricted __be32
> drivers/acpi/nfit/core.c:1527:41: warning: cast to restricted __be32
> drivers/acpi/nfit/core.c:1527:41: warning: cast to restricted __be32
> drivers/acpi/nfit/core.c:1527:41: warning: cast to restricted __be32
> drivers/acpi/nfit/core.c:1792:33: warning: cast to restricted __be16
> drivers/acpi/nfit/core.c:1792:33: warning: cast to restricted __be16
> drivers/acpi/nfit/core.c:1792:33: warning: cast to restricted __be16
> drivers/acpi/nfit/core.c:1792:33: warning: cast to restricted __be16
> drivers/acpi/nfit/core.c:1794:33: warning: cast to restricted __be16
> drivers/acpi/nfit/core.c:1794:33: warning: cast to restricted __be16
> drivers/acpi/nfit/core.c:1794:33: warning: cast to restricted __be16
> drivers/acpi/nfit/core.c:1794:33: warning: cast to restricted __be16
> drivers/acpi/nfit/core.c:1795:33: warning: cast to restricted __be32
> drivers/acpi/nfit/core.c:1795:33: warning: cast to restricted __be32
> drivers/acpi/nfit/core.c:1795:33: warning: cast to restricted __be32
> drivers/acpi/nfit/core.c:1795:33: warning: cast to restricted __be32
> drivers/acpi/nfit/core.c:1795:33: warning: cast to restricted __be32
> drivers/acpi/nfit/core.c:1795:33: warning: cast to restricted __be32
> drivers/acpi/nfit/core.c:1798:33: warning: cast to restricted __be16
> drivers/acpi/nfit/core.c:1798:33: warning: cast to restricted __be16
> drivers/acpi/nfit/core.c:1798:33: warning: cast to restricted __be16
> drivers/acpi/nfit/core.c:1798:33: warning: cast to restricted __be16
> drivers/acpi/nfit/core.c:1799:33: warning: cast to restricted __be32
> drivers/acpi/nfit/core.c:1799:33: warning: cast to restricted __be32
> drivers/acpi/nfit/core.c:1799:33: warning: cast to restricted __be32
> drivers/acpi/nfit/core.c:1799:33: warning: cast to restricted __be32
> drivers/acpi/nfit/core.c:1799:33: warning: cast to restricted __be32
> drivers/acpi/nfit/core.c:1799:33: warning: cast to restricted __be32
>
> Signed-off-by: Ben Dooks
First off, this falls under the ACPICA rule mentioned before.
Second, all ACPI is little-endian by the spec, so I'm not sure what is
going on here.
> ---
> drivers/acpi/nfit/core.c | 8
> include/acpi/actbl2.h| 18 +-
> 2 files changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> index 07204d482968..0fcc247fdfac 100644
> --- a/drivers/acpi/nfit/core.c
> +++ b/drivers/acpi/nfit/core.c
> @@ -2194,15 +2194,15 @@ static const struct attribute_group
> *acpi_nfit_region_attribute_groups[] = {
> /* enough info to uniquely specify an interleave set */
> struct nfit_set_info {
>
Re: [PATCH v5 08/10] acpi/nfit: Improve terminator line in acpi_nfit_ids
On Fri, Jun 30, 2023 at 1:04 PM Rafael J. Wysocki wrote: > > On Fri, Jun 30, 2023 at 11:52 AM Wilczynski, Michal > wrote: > > > > > > > > On 6/29/2023 6:14 PM, Rafael J. Wysocki wrote: > > > 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? > > > > This syntax is correct of course, but we have an internal guidelines > > specifically > > saying that terminator line should NOT contain a comma at the end. > > Justification: > > > > "Terminator line is established for the data structure arrays which may > > have unknown, > > to the caller, sizes. The purpose of it is to stop iteration over an array > > and avoid > > out-of-boundary access. Nevertheless, we may apply a bit more stricter rule > > to avoid > > potential, but unlike, event of adding the entry after terminator, already > > at compile time. > > This will be achieved by not putting comma at the end of terminator line" > > This certainly applies to any new code. > > The existing code, however, is what it is and the question is how much > of an improvement the given change makes. > > So yes, it may not follow the current rules for new code, but then it > may not be worth changing to follow these rules anyway. This is a bit like housing in a city. Usually, there are strict requirements that must be followed while constructing a new building, but existing buildings are not reconstructed to follow them in the majority of cases. It may not even be a good idea to do that.
Re: [PATCH v5 08/10] acpi/nfit: Improve terminator line in acpi_nfit_ids
On Fri, Jun 30, 2023 at 11:52 AM Wilczynski, Michal wrote: > > > > On 6/29/2023 6:14 PM, Rafael J. Wysocki wrote: > > 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? > > This syntax is correct of course, but we have an internal guidelines > specifically > saying that terminator line should NOT contain a comma at the end. > Justification: > > "Terminator line is established for the data structure arrays which may have > unknown, > to the caller, sizes. The purpose of it is to stop iteration over an array > and avoid > out-of-boundary access. Nevertheless, we may apply a bit more stricter rule > to avoid > potential, but unlike, event of adding the entry after terminator, already at > compile time. > This will be achieved by not putting comma at the end of terminator line" This certainly applies to any new code. The existing code, however, is what it is and the question is how much of an improvement the given change makes. So yes, it may not follow the current rules for new code, but then it may not be worth changing to follow these rules anyway.
Re: [PATCH v5 09/10] acpi/nfit: Move handler installing logic to driver
On Fri, Jun 30, 2023 at 11:55 AM Wilczynski, Michal
wrote:
>
>
>
> On 6/29/2023 6:18 PM, Rafael J. Wysocki wrote:
> > 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.
>
> Sure, is adev a preferred name for acpi_device ?
In new code, it is.
In the existing code, it depends. If you do a one-line change, it is
better to retain the original naming (for the sake of clarity of the
change itself). If you rearrange it completely, you may as well
change the names while at it. And there is a spectrum in between.
> I've seen a mix of different naming
> in drivers, some use device, adev, acpi_dev and so on. I suppose it's not a
> big deal, but
> it would be good to know.
Personally, I prefer adev, but this isn't a very strong preference.
Using "device" as a name of a struct acpi_device object (or a pointer
to one of these for that matter) is slightly misleading IMV, because
those things represent AML entities rather than actual hardware.
Re: [PATCH v5 07/10] acpi/nfit: Move acpi_nfit_notify() before acpi_nfit_add()
On Fri, Jun 30, 2023 at 11:48 AM Wilczynski, Michal
wrote:
>
>
>
> On 6/29/2023 6:06 PM, Rafael J. Wysocki wrote:
> > 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.
>
> I agree with you, but I got told specifically to do that.
> https://lore.kernel.org/linux-acpi/e0f67199-9feb-432c-f0cb-7bdbdaf9f...@linux.intel.com/
Whether or not this is easier to review is kind of subjective.
If there were more code to move, I would agree, but in this particular
case having to review two patches instead of just one is a bit of a
hassle IMV.
Re: [PATCH v5 03/10] acpi/ac: Move handler installing logic to driver
On Fri, Jun 30, 2023 at 11:41 AM Rafael J. Wysocki wrote:
>
> On Fri, Jun 30, 2023 at 11:39 AM Wilczynski, Michal
> wrote:
> >
> >
> >
> > On 6/29/2023 5:55 PM, Rafael J. Wysocki wrote:
> > > 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.
> >
> > I like the Reverse Christmas Tree, but sure will change that
>
> I do too, but in the cases when it costs 3 extra lines of code I'd
> rather have something simpler.
Besides, moving code around is not strictly related to the functional
changes made by the patch and it kind of hides those changes. It is
better to move code around in a separate patch if you really want to
do that.
Re: [PATCH v5 03/10] acpi/ac: Move handler installing logic to driver
On Fri, Jun 30, 2023 at 11:39 AM Wilczynski, Michal
wrote:
>
>
>
> On 6/29/2023 5:55 PM, Rafael J. Wysocki wrote:
> > 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.
>
> I like the Reverse Christmas Tree, but sure will change that
I do too, but in the cases when it costs 3 extra lines of code I'd
rather have something simpler.
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;
Re: [PATCH 1/3] acpi: nfit: add declaration in a local header
On Mon, May 22, 2023 at 5:22 PM Dave Jiang wrote: > > > > On 5/16/23 1:14 PM, Arnd Bergmann wrote: > > From: Arnd Bergmann > > > > The nfit_intel_shutdown_status() function has a __weak defintion > > in nfit.c and an override in acpi_nfit_test.c for testing > > purposes. This works without an extern declaration, but causes > > a W=1 build warning: > > > > drivers/acpi/nfit/core.c:1717:13: error: no previous prototype for > > 'nfit_intel_shutdown_status' [-Werror=missing-prototypes] > > > > Add a declaration in a header that gets included from both > > sides to shut up the warning and ensure that the prototypes > > actually match. > > > > Signed-off-by: Arnd Bergmann > > Reviewed-by: Dave Jiang Applied as 6.5 material, thanks! > > --- > > drivers/acpi/nfit/nfit.h | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/acpi/nfit/nfit.h b/drivers/acpi/nfit/nfit.h > > index 6023ad61831a..573bc0de2990 100644 > > --- a/drivers/acpi/nfit/nfit.h > > +++ b/drivers/acpi/nfit/nfit.h > > @@ -347,4 +347,6 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor > > *nd_desc, struct nvdimm *nvdimm, > > void acpi_nfit_desc_init(struct acpi_nfit_desc *acpi_desc, struct device > > *dev); > > bool intel_fwa_supported(struct nvdimm_bus *nvdimm_bus); > > extern struct device_attribute dev_attr_firmware_activate_noidle; > > +void nfit_intel_shutdown_status(struct nfit_mem *nfit_mem); > > + > > #endif /* __NFIT_H__ */
Re: [PATCH v2 2/2] ACPI: HMAT: Fix initiator registration for single-initiator systems
On 11/17/2022 12:37 AM, Vishal Verma wrote:
In a system with a single initiator node, and one or more memory-only
'target' nodes, the memory-only node(s) would fail to register their
initiator node correctly. i.e. in sysfs:
# ls /sys/devices/system/node/node0/access0/targets/
node0
Where as the correct behavior should be:
# ls /sys/devices/system/node/node0/access0/targets/
node0 node1
This happened because hmat_register_target_initiators() uses list_sort()
to sort the initiator list, but the sort comparision function
(initiator_cmp()) is overloaded to also set the node mask's bits.
In a system with a single initiator, the list is singular, and list_sort
elides the comparision helper call. Thus the node mask never gets set,
and the subsequent search for the best initiator comes up empty.
Add a new helper to consume the sorted initiator list, and generate the
nodemask, decoupling it from the overloaded initiator_cmp() comparision
callback. This prevents the singular list corner case naturally, and
makes the code easier to follow as well.
Cc:
Cc: Rafael J. Wysocki
Cc: Liu Shixin
Cc: Dan Williams
Cc: Kirill A. Shutemov
Reported-by: Chris Piper
Signed-off-by: Vishal Verma
Acked-by: Rafael J. Wysocki
---
drivers/acpi/numa/hmat.c | 26 --
1 file changed, 20 insertions(+), 6 deletions(-)
diff --git a/drivers/acpi/numa/hmat.c b/drivers/acpi/numa/hmat.c
index 144a84f429ed..6cceca64a6bc 100644
--- a/drivers/acpi/numa/hmat.c
+++ b/drivers/acpi/numa/hmat.c
@@ -562,17 +562,26 @@ static int initiator_cmp(void *priv, const struct
list_head *a,
{
struct memory_initiator *ia;
struct memory_initiator *ib;
- unsigned long *p_nodes = priv;
ia = list_entry(a, struct memory_initiator, node);
ib = list_entry(b, struct memory_initiator, node);
- set_bit(ia->processor_pxm, p_nodes);
- set_bit(ib->processor_pxm, p_nodes);
-
return ia->processor_pxm - ib->processor_pxm;
}
+static int initiators_to_nodemask(unsigned long *p_nodes)
+{
+ struct memory_initiator *initiator;
+
+ if (list_empty())
+ return -ENXIO;
+
+ list_for_each_entry(initiator, , node)
+ set_bit(initiator->processor_pxm, p_nodes);
+
+ return 0;
+}
+
static void hmat_register_target_initiators(struct memory_target *target)
{
static DECLARE_BITMAP(p_nodes, MAX_NUMNODES);
@@ -609,7 +618,10 @@ static void hmat_register_target_initiators(struct
memory_target *target)
* initiators.
*/
bitmap_zero(p_nodes, MAX_NUMNODES);
- list_sort(p_nodes, , initiator_cmp);
+ list_sort(NULL, , initiator_cmp);
+ if (initiators_to_nodemask(p_nodes) < 0)
+ return;
+
if (!access0done) {
for (i = WRITE_LATENCY; i <= READ_BANDWIDTH; i++) {
loc = localities_types[i];
@@ -643,7 +655,9 @@ static void hmat_register_target_initiators(struct
memory_target *target)
/* Access 1 ignores Generic Initiators */
bitmap_zero(p_nodes, MAX_NUMNODES);
- list_sort(p_nodes, , initiator_cmp);
+ if (initiators_to_nodemask(p_nodes) < 0)
+ return;
+
for (i = WRITE_LATENCY; i <= READ_BANDWIDTH; i++) {
loc = localities_types[i];
if (!loc)
Re: [PATCH 0/2] ACPI: HMAT: fix single-initiator target registrations
On Wed, Nov 16, 2022 at 8:57 AM Vishal Verma wrote: > > Patch 1 is an obvious cleanup found while fixing this problem. > > Patch 2 Fixes a bug with initiator registration for single-initiator > systems. More details on this in its commit message. > > > Vishal Verma (2): > ACPI: HMAT: remove unnecessary variable initialization > ACPI: HMAT: Fix initiator registration for single-initiator systems Acked-by: Rafael J. Wysocki for both and please feel free to ask Dan to take them. Alternatively, if you want me to apply them, please let me know. > drivers/acpi/numa/hmat.c | 33 ++--- > 1 file changed, 30 insertions(+), 3 deletions(-) > > > base-commit: 9abf2313adc1ca1b6180c508c25f22f9395cc780 > --
Re: [PATCH v1 1/1] ACPI: Switch to use list_entry_is_head() helper
On Wed, Mar 2, 2022 at 4:50 PM Andy Shevchenko wrote: > > On Fri, Feb 11, 2022 at 01:04:23PM +0200, Andy Shevchenko wrote: > > Since we got list_entry_is_head() helper in the generic header, > > we may switch the ACPI modules to use it. This eliminates the > > need in additional variable. In some cases it reduces critical > > sections as well. > > Besides the work required in a couple of cases (LKP) there is an > ongoing discussion about list loops (and this particular API). > > Rafael, what do you think is the best course of action here? I think the current approach is to do the opposite of what this patch is attempting to do: avoid using the list iterator outside of the loop.
Re: [PATCH v1 1/1] ACPI: NFIT: Import GUID before use
On Mon, Dec 13, 2021 at 9:46 PM Andy Shevchenko
wrote:
>
> Strictly speaking the comparison between guid_t and raw buffer
> is not correct. Import GUID to variable of guid_t type and then
> compare.
>
> Signed-off-by: Andy Shevchenko
Dan, are you going to take care of this or should I?
> ---
> drivers/acpi/nfit/core.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> index 7dd80acf92c7..e5d7f2bda13f 100644
> --- a/drivers/acpi/nfit/core.c
> +++ b/drivers/acpi/nfit/core.c
> @@ -678,10 +678,12 @@ static const char *spa_type_name(u16 type)
>
> int nfit_spa_type(struct acpi_nfit_system_address *spa)
> {
> + guid_t guid;
> int i;
>
> + import_guid(, spa->range_guid);
> for (i = 0; i < NFIT_UUID_MAX; i++)
> - if (guid_equal(to_nfit_uuid(i), (guid_t *)>range_guid))
> + if (guid_equal(to_nfit_uuid(i), ))
> return i;
> return -1;
> }
> --
> 2.33.0
>
Re: [PATCH] base: power: runtime.c: Remove a unnecessary space
On Sun, Apr 18, 2021 at 11:22 AM Joe Perches wrote: > > On Sun, 2021-04-18 at 09:11 +, Sebastian Fricke wrote: > > Hey Joe, > > Hi Sebastian. > > > On 18.04.2021 00:09, Joe Perches wrote: > > > On Sun, 2021-04-18 at 06:08 +, Sebastian Fricke wrote: > > > > Remove a redundant space to improve the quality of the comment. > > > I think this patch is not useful. > > > It's not redundant. > > > > Thank you, I actually found this pattern a few more times but I wanted > > to check first if this is a mistake or chosen consciously. I write a double space after a period ending a sentence as a rule and it is not a mistake. > [] > > > For drivers/base/power/runtime.c, that 2 space after period style is used > > > dozens of times and changing a single instance of it isn't very useful. > > Even in that single file it's not consistent. > It's something like 3:1 for 2 spaces over 1 space after period. > > I believe it's done more by habit and author age than anything. > If you learned to type using a typewriter and not a keyboard, then > you likely still use 2 spaces after a period. By habit and because I prefer it this way (I find it somewhat easier to separate sentences from one another this way). > > True and if I understand you correctly you would rather keep it as is > > right? > > Yes. IMO: Whitespace in comments like this should not be changed > unless there's some other significant benefit like better alignment. Agreed.
Re: [PATCH] ACPI: PM: s2idle: Invoke _PTS for s2idle
On Mon, Apr 19, 2021 at 11:08 AM Kai-Heng Feng
wrote:
>
> HP EliteBook 840 G8 reboots on s2idle resume, and HP EliteBook 845 G8
> wakes up immediately on s2idle. Both are caused by the XMM7360 WWAN PCI
> card.
>
> There's a WWAN specific method to really turn off the WWAN via EC:
> Method (_PTS, 1, NotSerialized) // _PTS: Prepare To Sleep
> {
> ...
> If (CondRefOf (\_SB.PCI0.GP12.PTS))
> {
> \_SB.PCI0.GP12.PTS (Arg0)
> }
> ...
> }
>
> Scope (_SB.PCI0.GP12)
> {
> ...
> Method (PTS, 1, Serialized)
> {
> If (^^LPCB.EC0.ECRG)
> {
> If ((PDID == 0x))
> {
> Return (Zero)
> }
>
> POFF ()
> SGIO (WWBR, One)
> Sleep (0x1E)
> Acquire (^^LPCB.EC0.ECMX, 0x)
> ^^LPCB.EC0.WWP = One
> Release (^^LPCB.EC0.ECMX)
> Sleep (0x01F4)
> }
>
> Return (Zero)
> }
> ...
> }
>
> So let's also invok _PTS for s2idle.
>
> Signed-off-by: Kai-Heng Feng
> ---
> drivers/acpi/sleep.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
> index 09fd13757b65..7e84b4b09919 100644
> --- a/drivers/acpi/sleep.c
> +++ b/drivers/acpi/sleep.c
> @@ -698,6 +698,7 @@ int acpi_s2idle_prepare(void)
> }
>
> acpi_enable_wakeup_devices(ACPI_STATE_S0);
> + acpi_enter_sleep_state_prep(ACPI_STATE_S0);
The system is in S0 already at this point, so not really.
Please use a quirk to address this.
>
> /* Change the configuration of GPEs to avoid spurious wakeup. */
> acpi_enable_all_wakeup_gpes();
> --
Re: [PATCH v1 1/1] PM / wakeup: use dev_set_name() directly
On Wed, Apr 14, 2021 at 2:53 PM Andy Shevchenko wrote: > > We have open coded dev_set_name() implementation, replace that > with a direct call. > > Signed-off-by: Andy Shevchenko > --- > drivers/base/power/wakeup_stats.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/base/power/wakeup_stats.c > b/drivers/base/power/wakeup_stats.c > index d638259b829a..5ade7539ac02 100644 > --- a/drivers/base/power/wakeup_stats.c > +++ b/drivers/base/power/wakeup_stats.c > @@ -154,7 +154,7 @@ static struct device *wakeup_source_device_create(struct > device *parent, > dev_set_drvdata(dev, ws); > device_set_pm_not_required(dev); > > - retval = kobject_set_name(>kobj, "wakeup%d", ws->id); > + retval = dev_set_name(dev, "wakeup%d", ws->id); > if (retval) > goto error; > > -- Applied as 5.13 material, thanks!
[GIT PULL] ACPI fix for v5.12-rc8
Hi Linus, Please pull from the tag git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git \ acpi-5.12-rc8 with top-most commit 6998a8800d73116187aad542391ce3b2dd0f9e30 ACPI: x86: Call acpi_boot_table_init() after acpi_table_upgrade() on top of commit d434405aaab7d0ebc516b68a8fc4100922d7f5ef Linux 5.12-rc7 to receive an ACPI fix for 5.12-rc8. This restores the initrd-based ACPI table override functionality broken by one of the recent fixes. Thanks! --- Rafael J. Wysocki (1): ACPI: x86: Call acpi_boot_table_init() after acpi_table_upgrade() --- arch/x86/kernel/setup.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
Re: "Reporting issues" document feedback
On Wed, Apr 14, 2021 at 3:22 PM w4v3 wrote:
>
> Hi Thorsten,
>
> Thanks for the quick and illuminating response :)
>
> > Links to your bug report and the thread on the mailing list would have
> > helped here to understand better what's going on, but whatever, they are
> > not that important.
>
> Here you go: https://bugzilla.kernel.org/show_bug.cgi?id=212643
> https://marc.info/?l=linux-acpi=161824910030600=2
>
> > But it should, otherwise the subsystem should remove the line starting
> > with B: ("bugs:" in the webview).
> >
> > Rafael might be able to clarify things.
>
> > But afais it's appropriate there is a B: line: just a few weeks ago I
> > took a quick look at bugzilla and ACPI bugs in particular, and back then
> > most of the bug reports there got handled by the maintainers. That's why
> > I assume you were just unlucky and your report fall through the cracks
> > (but obviously I might be wrong here). And maybe your report even did
> > help: the developer that fixed the issue might have seen both the bug
> > entry and the mailed report, but simply forget to close the former.
>
> Good to know. It does seem like many recent ACPI bug reports on bugzilla
> have been processed by maintainers. Maybe it is the ACPI-subcomponent I
> chose for the bug: in Config-Tables, only two other bugs were submitted
> and they did not attract comments. Anyways, I understand now that it's
> not an issue with the document so thanks for forwarding it to Rafael.
As a rule, ACPI bugs submitted through the BZ are processed by the
ACPI team (not necessarily by me in person, though), but the response
time may vary, so it's better to report urgent issues by sending
e-mail to linux-a...@vger.kernel.org.
Definitely issues where table dumps or similar are requested are best
handled in the BZ, so reporters can be asked to create a BZ entry for
a bug reported by e-mail anyway.
If you are interested in the history (ie. what issues were reported in
the past), you need to look at both the BZ and the ml record.
HTH
Re: [PATCH] ACPI: x86: Call acpi_boot_table_init() after acpi_table_upgrade()
On Wed, Apr 14, 2021 at 10:13 AM Mike Rapoport wrote:
>
> On Wed, Apr 14, 2021 at 09:42:01AM +0200, David Hildenbrand wrote:
> > On 13.04.21 19:53, Rafael J. Wysocki wrote:
> > > On Tue, Apr 13, 2021 at 7:43 PM David Hildenbrand
> > > wrote:
> > > >
> > > > On 13.04.21 16:01, Rafael J. Wysocki wrote:
> > > > > From: Rafael J. Wysocki
> > > > >
> > > > > Commit 1a1c130ab757 ("ACPI: tables: x86: Reserve memory occupied by
> > > > > ACPI tables") attempted to address an issue with reserving the memory
> > > > > occupied by ACPI tables, but it broke the initrd-based table override
> > > > > mechanism relied on by multiple users.
> > > > >
> > > > > To restore the initrd-based ACPI table override functionality, move
> > > > > the acpi_boot_table_init() invocation in setup_arch() on x86 after
> > > > > the acpi_table_upgrade() one.
> > > > >
> > > > > Fixes: 1a1c130ab757 ("ACPI: tables: x86: Reserve memory occupied by
> > > > > ACPI tables")
> > > > > Reported-by: Hans de Goede
> > > > > Tested-by: Hans de Goede
> > > > > Signed-off-by: Rafael J. Wysocki
> > > > > ---
> > > > >
> > > > > George, can you please check if this reintroduces the issue addressed
> > > > > by
> > > > > the above commit for you?
> > > > >
> > > > > ---
> > > > >arch/x86/kernel/setup.c |5 ++---
> > > > >1 file changed, 2 insertions(+), 3 deletions(-)
> > > > >
> > > > > Index: linux-pm/arch/x86/kernel/setup.c
> > > > > ===
> > > > > --- linux-pm.orig/arch/x86/kernel/setup.c
> > > > > +++ linux-pm/arch/x86/kernel/setup.c
> > > > > @@ -1045,9 +1045,6 @@ void __init setup_arch(char **cmdline_p)
> > > > >
> > > > >cleanup_highmap();
> > > > >
> > > > > - /* Look for ACPI tables and reserve memory occupied by them. */
> > > > > - acpi_boot_table_init();
> > > > > -
> > > > >memblock_set_current_limit(ISA_END_ADDRESS);
> > > > >e820__memblock_setup();
> > > > >
> > > > > @@ -1132,6 +1129,8 @@ void __init setup_arch(char **cmdline_p)
> > > > >reserve_initrd();
> > > > >
> > > > >acpi_table_upgrade();
> > > > > + /* Look for ACPI tables and reserve memory occupied by them. */
> > > > > + acpi_boot_table_init();
> > > > >
> > > > >vsmp_init();
> > > >
> > > > This is fairly late; especially, it's after actual allocations -- see
> > > > e820__memblock_alloc_reserved_mpc_new().
> > > >
> > > > Can't the table upgrade mechanism fix up when adjusting something?
> > >
> > > Not at this point of the cycle I'm afraid.
> > >
> > > > Some details on what actually breaks would be helpful.
> > >
> > > Generally speaking, the table overrides that come from the initrd are
> > > not taken into account if acpi_boot_table_init() runs before
> > > acpi_table_upgrade() and the latter cannot run before
> > > reserve_initrd().
> >
> > I see. (looking at Documentation/acpi/initrd_table_override.txt I understand
> > what acpi table overrides are for :) )
> >
> > >
> > > Honestly, I'm not sure how much effort it would take to untangle this ATM.
> >
> > Also true; ideally, we wouldn't have any allocations (find+reserve) before
> > ordinary reservations are done.
> >
> > However, I have no idea if we can move
> > e820__memblock_alloc_reserved_mpc_new() and reserve_real_mode() around
> > easily. Also, reserve_initrd()->relocate_initrd() does allocations.
>
> Even if we can move e820__memblock_alloc_reserved_mpc_new() and
> reserve_real_mode(), the allocation in reserve_initrd() has to be before
> the tables override, we would only reduce the probability of allocating an
> ACPI page.
>
> I think what we can do is to override the ACPI tables separately from their
> initial parsing. Rafael, what do you say?
Well, possibly. With one caveat that parsing a table that's going to
be overridden subsequently may not be a good idea.
Anyway, things like that can only be done in the next cycle or later.
Re: [PATCH] xen: Remove support for PV ACPI cpu/memory hotplug
On Tue, Apr 13, 2021 at 7:53 PM Boris Ostrovsky
wrote:
>
> Commit 76fc253723ad ("xen/acpi-stub: Disable it b/c the acpi_processor_add
> is no longer called.") declared as BROKEN support for Xen ACPI stub (which
> is required for xen-acpi-{cpu|memory}-hotplug) and suggested that this
> is temporary and will be soon fixed. This was in March 2013.
>
> Further, commit cfafae940381 ("xen: rename dom0_op to platform_op")
> renamed an interface used by memory hotplug code without updating that
> code (as it was BROKEN and therefore not compiled). This was
> in November 2015 and has gone unnoticed for over 5 year.
>
> It is now clear that this code is of no interest to anyone and therefore
> should be removed.
>
> Signed-off-by: Boris Ostrovsky
Acked-by: Rafael J. Wysocki
Thanks for doing this!
> ---
> drivers/xen/Kconfig | 31 ---
> drivers/xen/Makefile | 3 -
> drivers/xen/pcpu.c| 35 ---
> drivers/xen/xen-acpi-cpuhotplug.c | 446 ---
> drivers/xen/xen-acpi-memhotplug.c | 475
> --
> drivers/xen/xen-stub.c| 90
> include/xen/acpi.h| 35 ---
> 7 files changed, 1115 deletions(-)
> delete mode 100644 drivers/xen/xen-acpi-cpuhotplug.c
> delete mode 100644 drivers/xen/xen-acpi-memhotplug.c
> delete mode 100644 drivers/xen/xen-stub.c
>
> diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
> index ea0efd290c37..5f1ce59b44b9 100644
> --- a/drivers/xen/Kconfig
> +++ b/drivers/xen/Kconfig
> @@ -238,37 +238,6 @@ config XEN_PRIVCMD
> depends on XEN
> default m
>
> -config XEN_STUB
> - bool "Xen stub drivers"
> - depends on XEN && X86_64 && BROKEN
> - help
> - Allow kernel to install stub drivers, to reserve space for Xen
> drivers,
> - i.e. memory hotplug and cpu hotplug, and to block native drivers
> loaded,
> - so that real Xen drivers can be modular.
> -
> - To enable Xen features like cpu and memory hotplug, select Y here.
> -
> -config XEN_ACPI_HOTPLUG_MEMORY
> - tristate "Xen ACPI memory hotplug"
> - depends on XEN_DOM0 && XEN_STUB && ACPI
> - help
> - This is Xen ACPI memory hotplug.
> -
> - Currently Xen only support ACPI memory hot-add. If you want
> - to hot-add memory at runtime (the hot-added memory cannot be
> - removed until machine stop), select Y/M here, otherwise select N.
> -
> -config XEN_ACPI_HOTPLUG_CPU
> - tristate "Xen ACPI cpu hotplug"
> - depends on XEN_DOM0 && XEN_STUB && ACPI
> - select ACPI_CONTAINER
> - help
> - Xen ACPI cpu enumerating and hotplugging
> -
> - For hotplugging, currently Xen only support ACPI cpu hotadd.
> - If you want to hotadd cpu at runtime (the hotadded cpu cannot
> - be removed until machine stop), select Y/M here.
> -
> config XEN_ACPI_PROCESSOR
> tristate "Xen ACPI processor"
> depends on XEN && XEN_DOM0 && X86 && ACPI_PROCESSOR && CPU_FREQ
> diff --git a/drivers/xen/Makefile b/drivers/xen/Makefile
> index c3621b9f4012..3434593455b2 100644
> --- a/drivers/xen/Makefile
> +++ b/drivers/xen/Makefile
> @@ -26,9 +26,6 @@ obj-$(CONFIG_SWIOTLB_XEN) += swiotlb-xen.o
> obj-$(CONFIG_XEN_MCE_LOG) += mcelog.o
> obj-$(CONFIG_XEN_PCIDEV_BACKEND) += xen-pciback/
> obj-$(CONFIG_XEN_PRIVCMD) += xen-privcmd.o
> -obj-$(CONFIG_XEN_STUB) += xen-stub.o
> -obj-$(CONFIG_XEN_ACPI_HOTPLUG_MEMORY) += xen-acpi-memhotplug.o
> -obj-$(CONFIG_XEN_ACPI_HOTPLUG_CPU) += xen-acpi-cpuhotplug.o
> obj-$(CONFIG_XEN_ACPI_PROCESSOR) += xen-acpi-processor.o
> obj-$(CONFIG_XEN_EFI) += efi.o
> obj-$(CONFIG_XEN_SCSI_BACKEND) += xen-scsiback.o
> diff --git a/drivers/xen/pcpu.c b/drivers/xen/pcpu.c
> index cdc6daa7a9f6..1bcdd5227771 100644
> --- a/drivers/xen/pcpu.c
> +++ b/drivers/xen/pcpu.c
> @@ -345,41 +345,6 @@ static irqreturn_t xen_pcpu_interrupt(int irq, void
> *dev_id)
> return IRQ_HANDLED;
> }
>
> -/* Sync with Xen hypervisor after cpu hotadded */
> -void xen_pcpu_hotplug_sync(void)
> -{
> - schedule_work(_pcpu_work);
> -}
> -EXPORT_SYMBOL_GPL(xen_pcpu_hotplug_sync);
> -
> -/*
> - * For hypervisor presented cpu, return logic cpu id;
> - * For hypervisor non-presented cpu, return -ENODEV.
> - */
> -int xen_pcpu_id(uint32_t acpi_id)
> -{
> - int cpu_id = 0, max_id = 0;
Re: [PATCH] ACPI: x86: Call acpi_boot_table_init() after acpi_table_upgrade()
On Tue, Apr 13, 2021 at 7:43 PM David Hildenbrand wrote:
>
> On 13.04.21 16:01, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki
> >
> > Commit 1a1c130ab757 ("ACPI: tables: x86: Reserve memory occupied by
> > ACPI tables") attempted to address an issue with reserving the memory
> > occupied by ACPI tables, but it broke the initrd-based table override
> > mechanism relied on by multiple users.
> >
> > To restore the initrd-based ACPI table override functionality, move
> > the acpi_boot_table_init() invocation in setup_arch() on x86 after
> > the acpi_table_upgrade() one.
> >
> > Fixes: 1a1c130ab757 ("ACPI: tables: x86: Reserve memory occupied by ACPI
> > tables")
> > Reported-by: Hans de Goede
> > Tested-by: Hans de Goede
> > Signed-off-by: Rafael J. Wysocki
> > ---
> >
> > George, can you please check if this reintroduces the issue addressed by
> > the above commit for you?
> >
> > ---
> > arch/x86/kernel/setup.c |5 ++---
> > 1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > Index: linux-pm/arch/x86/kernel/setup.c
> > ===
> > --- linux-pm.orig/arch/x86/kernel/setup.c
> > +++ linux-pm/arch/x86/kernel/setup.c
> > @@ -1045,9 +1045,6 @@ void __init setup_arch(char **cmdline_p)
> >
> > cleanup_highmap();
> >
> > - /* Look for ACPI tables and reserve memory occupied by them. */
> > - acpi_boot_table_init();
> > -
> > memblock_set_current_limit(ISA_END_ADDRESS);
> > e820__memblock_setup();
> >
> > @@ -1132,6 +1129,8 @@ void __init setup_arch(char **cmdline_p)
> > reserve_initrd();
> >
> > acpi_table_upgrade();
> > + /* Look for ACPI tables and reserve memory occupied by them. */
> > + acpi_boot_table_init();
> >
> > vsmp_init();
>
> This is fairly late; especially, it's after actual allocations -- see
> e820__memblock_alloc_reserved_mpc_new().
>
> Can't the table upgrade mechanism fix up when adjusting something?
Not at this point of the cycle I'm afraid.
> Some details on what actually breaks would be helpful.
Generally speaking, the table overrides that come from the initrd are
not taken into account if acpi_boot_table_init() runs before
acpi_table_upgrade() and the latter cannot run before
reserve_initrd().
Honestly, I'm not sure how much effort it would take to untangle this ATM.
[PATCH] ACPI: x86: Call acpi_boot_table_init() after acpi_table_upgrade()
From: Rafael J. Wysocki
Commit 1a1c130ab757 ("ACPI: tables: x86: Reserve memory occupied by
ACPI tables") attempted to address an issue with reserving the memory
occupied by ACPI tables, but it broke the initrd-based table override
mechanism relied on by multiple users.
To restore the initrd-based ACPI table override functionality, move
the acpi_boot_table_init() invocation in setup_arch() on x86 after
the acpi_table_upgrade() one.
Fixes: 1a1c130ab757 ("ACPI: tables: x86: Reserve memory occupied by ACPI
tables")
Reported-by: Hans de Goede
Tested-by: Hans de Goede
Signed-off-by: Rafael J. Wysocki
---
George, can you please check if this reintroduces the issue addressed by
the above commit for you?
---
arch/x86/kernel/setup.c |5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
Index: linux-pm/arch/x86/kernel/setup.c
===
--- linux-pm.orig/arch/x86/kernel/setup.c
+++ linux-pm/arch/x86/kernel/setup.c
@@ -1045,9 +1045,6 @@ void __init setup_arch(char **cmdline_p)
cleanup_highmap();
- /* Look for ACPI tables and reserve memory occupied by them. */
- acpi_boot_table_init();
-
memblock_set_current_limit(ISA_END_ADDRESS);
e820__memblock_setup();
@@ -1132,6 +1129,8 @@ void __init setup_arch(char **cmdline_p)
reserve_initrd();
acpi_table_upgrade();
+ /* Look for ACPI tables and reserve memory occupied by them. */
+ acpi_boot_table_init();
vsmp_init();
Re: [PATCH v2 2/2] ACPI: utils: Capitalize abbreviations in the comments
On Tue, Apr 13, 2021 at 1:21 AM Andy Shevchenko wrote: > > The DSDT and ACPI should be capitalized. > > Signed-off-by: Andy Shevchenko > --- > v2: split from patch 1 as per Rafael's request > drivers/acpi/utils.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c > index 60e46efc1bc8..3b54b8fd7396 100644 > --- a/drivers/acpi/utils.c > +++ b/drivers/acpi/utils.c > @@ -811,7 +811,7 @@ static int acpi_dev_match_cb(struct device *dev, const > void *data) > * Note that if the device is pluggable, it may since have disappeared. > * > * Note that unlike acpi_dev_found() this function checks the status > - * of the device. So for devices which are present in the dsdt, but > + * of the device. So for devices which are present in the DSDT, but > * which are disabled (their _STA callback returns 0) this function > * will return false. > * > @@ -838,7 +838,7 @@ EXPORT_SYMBOL(acpi_dev_present); > > /** > * acpi_dev_get_next_match_dev - Return the next match of ACPI device > - * @adev: Pointer to the previous acpi_device matching this @hid, @uid and > @hrv > + * @adev: Pointer to the previous ACPI device matching this @hid, @uid and > @hrv > * @hid: Hardware ID of the device. > * @uid: Unique ID of the device, pass NULL to not check _UID > * @hrv: Hardware Revision of the device, pass -1 to not check _HRV > -- Applied as 5.13 material along with the [1/2], thanks!
Re: [PATCH v2 1/1] ACPI: bus: Introduce acpi_dev_get() and reuse it in ACPI code
On Tue, Apr 13, 2021 at 12:24 AM Andy Shevchenko
wrote:
>
> Introduce acpi_dev_get() to have a symmetrical API with acpi_dev_put()
> and reuse both in ACPI code under drivers/acpi folder.
>
> While at it, use acpi_bus_put_acpi_device() in one place rather than above.
>
> Signed-off-by: Andy Shevchenko
> ---
> v2: made acpi_dev_get() to return pointer as get_device() does (Rafael)
> drivers/acpi/device_sysfs.c | 4 ++--
> drivers/acpi/glue.c | 8
> drivers/acpi/scan.c | 9 -
> include/acpi/acpi_bus.h | 5 +
> 4 files changed, 15 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/acpi/device_sysfs.c b/drivers/acpi/device_sysfs.c
> index a07d4ade5835..fa2c1c93072c 100644
> --- a/drivers/acpi/device_sysfs.c
> +++ b/drivers/acpi/device_sysfs.c
> @@ -377,12 +377,12 @@ eject_store(struct device *d, struct device_attribute
> *attr,
> if (ACPI_FAILURE(status) || !acpi_device->flags.ejectable)
> return -ENODEV;
>
> - get_device(_device->dev);
> + acpi_dev_get(acpi_device);
> status = acpi_hotplug_schedule(acpi_device, ACPI_OST_EC_OSPM_EJECT);
> if (ACPI_SUCCESS(status))
> return count;
>
> - put_device(_device->dev);
> + acpi_dev_put(acpi_device);
> acpi_evaluate_ost(acpi_device->handle, ACPI_OST_EC_OSPM_EJECT,
> ACPI_OST_SC_NON_SPECIFIC_FAILURE, NULL);
> return status == AE_NO_MEMORY ? -ENOMEM : -EAGAIN;
> diff --git a/drivers/acpi/glue.c b/drivers/acpi/glue.c
> index 36b24b0658cb..0715e3be99a0 100644
> --- a/drivers/acpi/glue.c
> +++ b/drivers/acpi/glue.c
> @@ -190,7 +190,7 @@ int acpi_bind_one(struct device *dev, struct acpi_device
> *acpi_dev)
> if (!acpi_dev)
> return -EINVAL;
>
> - get_device(_dev->dev);
> + acpi_dev_get(acpi_dev);
> get_device(dev);
> physical_node = kzalloc(sizeof(*physical_node), GFP_KERNEL);
> if (!physical_node) {
> @@ -217,7 +217,7 @@ int acpi_bind_one(struct device *dev, struct acpi_device
> *acpi_dev)
> goto err;
>
> put_device(dev);
> - put_device(_dev->dev);
> + acpi_dev_put(acpi_dev);
> return 0;
> }
> if (pn->node_id == node_id) {
> @@ -257,7 +257,7 @@ int acpi_bind_one(struct device *dev, struct acpi_device
> *acpi_dev)
> err:
> ACPI_COMPANION_SET(dev, NULL);
> put_device(dev);
> - put_device(_dev->dev);
> + acpi_dev_put(acpi_dev);
> return retval;
> }
> EXPORT_SYMBOL_GPL(acpi_bind_one);
> @@ -285,7 +285,7 @@ int acpi_unbind_one(struct device *dev)
> ACPI_COMPANION_SET(dev, NULL);
> /* Drop references taken by acpi_bind_one(). */
> put_device(dev);
> - put_device(_dev->dev);
> + acpi_dev_put(acpi_dev);
> kfree(entry);
> break;
> }
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index d9e024fc6e70..bc973fbd70b2 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -530,7 +530,7 @@ static void acpi_device_del_work_fn(struct work_struct
> *work_not_used)
> * used by the device.
> */
> acpi_power_transition(adev, ACPI_STATE_D3_COLD);
> - put_device(>dev);
> + acpi_dev_put(adev);
> }
> }
>
> @@ -604,8 +604,7 @@ EXPORT_SYMBOL(acpi_bus_get_device);
>
> static void get_acpi_device(void *dev)
> {
> - if (dev)
> - get_device(&((struct acpi_device *)dev)->dev);
> + acpi_dev_get(dev);
> }
>
> struct acpi_device *acpi_bus_get_acpi_device(acpi_handle handle)
> @@ -615,7 +614,7 @@ struct acpi_device *acpi_bus_get_acpi_device(acpi_handle
> handle)
>
> void acpi_bus_put_acpi_device(struct acpi_device *adev)
> {
> - put_device(>dev);
> + acpi_dev_put(adev);
> }
>
> static struct acpi_device_bus_id *acpi_device_bus_id_match(const char
> *dev_id)
> @@ -2355,7 +2354,7 @@ int __init acpi_scan_init(void)
> acpi_detach_data(acpi_root->handle,
> acpi_scan_drop_device);
> acpi_device_del(acpi_root);
> - put_device(_root->dev);
> + acpi_bus_put_acpi_device(acpi_root);
> goto out;
> }
> }
> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index d631cb52283e..d2f5afb04a0b 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -695,6 +695,11 @@ acpi_dev_get_first_match_dev(const char *hid, const char
> *uid, s64 hrv);
> adev; \
> adev =
Re: [PATCH v1 1/1] ACPI: bus: Introduce acpi_dev_get() and reuse it in ACPI code
On Mon, Apr 12, 2021 at 8:10 PM Andy Shevchenko
wrote:
>
> On Mon, Apr 12, 2021 at 9:05 PM Rafael J. Wysocki wrote:
> >
> > On Mon, Apr 12, 2021 at 7:47 PM Andy Shevchenko
> > wrote:
> > >
> > > On Mon, Apr 12, 2021 at 8:32 PM Rafael J. Wysocki
> > > wrote:
> > > > On Sat, Apr 10, 2021 at 3:47 PM Andy Shevchenko
> > > > wrote:
> > >
> > > ...
> > >
> > > > > static void get_acpi_device(void *dev)
> > > > > {
> > > > > - if (dev)
> > > > > - get_device(&((struct acpi_device *)dev)->dev);
> > > > > + acpi_dev_get(dev);
> > > >
> > > > I would do
> > > >
> > > > if (dev)
> > > > acpi_dev_get(dev);
> > > >
> > > > here.
> > >
> > > Hmm... I don't see a point. acpi_dev_get() guaranteed to perform this
> > > check.
> > >
> > > > > }
> > >
> > >
> > > > > +static inline void acpi_dev_get(struct acpi_device *adev)
> > > > > +{
> > > > > + if (adev)
> > > > > + get_device(>dev);
> > > >
> > > > And I would drop the adev check from here (because the code calling it
> > > > may be running with wrong assumptions if adev is NULL). Or it should
> > > > return adev and the caller should be held responsible for checking it
> > > > against NULL (if they care).
> > >
> > > But this follows the get_device() / put_device() logic.
> >
> > Not really. get_device() returns a pointer.
> >
> > > Personally I don't think this is a good idea to deviate.
> >
> > Well, exactly. :-)
> >
> > > Note the acpi_bus_get_acpi_device()
> >
> > This also returns a pointer.
>
> Is it okay to return a pointer in acpi_dev_get() then?
Yes, it is, as I've said already.
Re: [PATCH v1 1/1] ACPI: bus: Introduce acpi_dev_get() and reuse it in ACPI code
On Mon, Apr 12, 2021 at 7:47 PM Andy Shevchenko
wrote:
>
> On Mon, Apr 12, 2021 at 8:32 PM Rafael J. Wysocki wrote:
> > On Sat, Apr 10, 2021 at 3:47 PM Andy Shevchenko
> > wrote:
>
> ...
>
> > > static void get_acpi_device(void *dev)
> > > {
> > > - if (dev)
> > > - get_device(&((struct acpi_device *)dev)->dev);
> > > + acpi_dev_get(dev);
> >
> > I would do
> >
> > if (dev)
> > acpi_dev_get(dev);
> >
> > here.
>
> Hmm... I don't see a point. acpi_dev_get() guaranteed to perform this check.
>
> > > }
>
>
> > > +static inline void acpi_dev_get(struct acpi_device *adev)
> > > +{
> > > + if (adev)
> > > + get_device(>dev);
> >
> > And I would drop the adev check from here (because the code calling it
> > may be running with wrong assumptions if adev is NULL). Or it should
> > return adev and the caller should be held responsible for checking it
> > against NULL (if they care).
>
> But this follows the get_device() / put_device() logic.
Not really. get_device() returns a pointer.
> Personally I don't think this is a good idea to deviate.
Well, exactly. :-)
> Note the acpi_bus_get_acpi_device()
This also returns a pointer.
> / acpi_bus_put_acpi_device() as well.
Re: [PATCH v1 1/1] ACPI: scan: Utilize match_string() API
On Sat, Apr 10, 2021 at 4:02 PM Andy Shevchenko
wrote:
>
> We have already an API to match a string in the array of strings.
> Utilize it instead of open coded analogues.
>
> Signed-off-by: Andy Shevchenko
> ---
> drivers/acpi/scan.c | 22 ++
> 1 file changed, 10 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index b1d1f1a8ce69..bba6b529cf6c 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -756,27 +756,25 @@ static bool acpi_info_matches_ids(struct
> acpi_device_info *info,
> const char * const ids[])
> {
> struct acpi_pnp_device_id_list *cid_list = NULL;
> - int i;
> + int i, index;
>
> if (!(info->valid & ACPI_VALID_HID))
> return false;
>
> + index = match_string(ids, -1, info->hardware_id.string);
> + if (index >= 0)
> + return true;
> +
> if (info->valid & ACPI_VALID_CID)
> cid_list = >compatible_id_list;
>
> - for (i = 0; ids[i]; i++) {
> - int j;
> + if (!cid_list)
> + return false;
>
> - if (!strcmp(info->hardware_id.string, ids[i]))
> + for (i = 0; i < cid_list->count; i++) {
> + index = match_string(ids, -1, cid_list->ids[i].string);
> + if (index >= 0)
> return true;
> -
> - if (!cid_list)
> - continue;
> -
> - for (j = 0; j < cid_list->count; j++) {
> - if (!strcmp(cid_list->ids[j].string, ids[i]))
> - return true;
> - }
> }
>
> return false;
> --
Applied as 5.13 material, thanks!
Re: [PATCH v1 1/1] ACPI: bus: Introduce acpi_dev_get() and reuse it in ACPI code
On Sat, Apr 10, 2021 at 3:47 PM Andy Shevchenko
wrote:
>
> Introduce acpi_dev_get() to have a symmetrical API with acpi_dev_put()
> and reuse both in ACPI code under drivers/acpi folder.
>
> While at it, use acpi_bus_put_acpi_device() in one place rather than above.
>
> Signed-off-by: Andy Shevchenko
> ---
> drivers/acpi/device_sysfs.c | 4 ++--
> drivers/acpi/glue.c | 8
> drivers/acpi/scan.c | 9 -
> include/acpi/acpi_bus.h | 6 ++
> 4 files changed, 16 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/acpi/device_sysfs.c b/drivers/acpi/device_sysfs.c
> index da4ff2a8b06a..35757c3c1b71 100644
> --- a/drivers/acpi/device_sysfs.c
> +++ b/drivers/acpi/device_sysfs.c
> @@ -376,12 +376,12 @@ eject_store(struct device *d, struct device_attribute
> *attr,
> if (ACPI_FAILURE(status) || !acpi_device->flags.ejectable)
> return -ENODEV;
>
> - get_device(_device->dev);
> + acpi_dev_get(acpi_device);
> status = acpi_hotplug_schedule(acpi_device, ACPI_OST_EC_OSPM_EJECT);
> if (ACPI_SUCCESS(status))
> return count;
>
> - put_device(_device->dev);
> + acpi_dev_put(acpi_device);
> acpi_evaluate_ost(acpi_device->handle, ACPI_OST_EC_OSPM_EJECT,
> ACPI_OST_SC_NON_SPECIFIC_FAILURE, NULL);
> return status == AE_NO_MEMORY ? -ENOMEM : -EAGAIN;
> diff --git a/drivers/acpi/glue.c b/drivers/acpi/glue.c
> index 36b24b0658cb..0715e3be99a0 100644
> --- a/drivers/acpi/glue.c
> +++ b/drivers/acpi/glue.c
> @@ -190,7 +190,7 @@ int acpi_bind_one(struct device *dev, struct acpi_device
> *acpi_dev)
> if (!acpi_dev)
> return -EINVAL;
>
> - get_device(_dev->dev);
> + acpi_dev_get(acpi_dev);
> get_device(dev);
> physical_node = kzalloc(sizeof(*physical_node), GFP_KERNEL);
> if (!physical_node) {
> @@ -217,7 +217,7 @@ int acpi_bind_one(struct device *dev, struct acpi_device
> *acpi_dev)
> goto err;
>
> put_device(dev);
> - put_device(_dev->dev);
> + acpi_dev_put(acpi_dev);
> return 0;
> }
> if (pn->node_id == node_id) {
> @@ -257,7 +257,7 @@ int acpi_bind_one(struct device *dev, struct acpi_device
> *acpi_dev)
> err:
> ACPI_COMPANION_SET(dev, NULL);
> put_device(dev);
> - put_device(_dev->dev);
> + acpi_dev_put(acpi_dev);
> return retval;
> }
> EXPORT_SYMBOL_GPL(acpi_bind_one);
> @@ -285,7 +285,7 @@ int acpi_unbind_one(struct device *dev)
> ACPI_COMPANION_SET(dev, NULL);
> /* Drop references taken by acpi_bind_one(). */
> put_device(dev);
> - put_device(_dev->dev);
> + acpi_dev_put(acpi_dev);
> kfree(entry);
> break;
> }
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index ad2541c0aece..bba6b529cf6c 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -530,7 +530,7 @@ static void acpi_device_del_work_fn(struct work_struct
> *work_not_used)
> * used by the device.
> */
> acpi_power_transition(adev, ACPI_STATE_D3_COLD);
> - put_device(>dev);
> + acpi_dev_put(adev);
> }
> }
>
> @@ -604,8 +604,7 @@ EXPORT_SYMBOL(acpi_bus_get_device);
>
> static void get_acpi_device(void *dev)
> {
> - if (dev)
> - get_device(&((struct acpi_device *)dev)->dev);
> + acpi_dev_get(dev);
I would do
if (dev)
acpi_dev_get(dev);
here.
> }
>
> struct acpi_device *acpi_bus_get_acpi_device(acpi_handle handle)
> @@ -615,7 +614,7 @@ struct acpi_device *acpi_bus_get_acpi_device(acpi_handle
> handle)
>
> void acpi_bus_put_acpi_device(struct acpi_device *adev)
> {
> - put_device(>dev);
> + acpi_dev_put(adev);
> }
>
> static struct acpi_device_bus_id *acpi_device_bus_id_match(const char
> *dev_id)
> @@ -2386,7 +2385,7 @@ int __init acpi_scan_init(void)
> acpi_detach_data(acpi_root->handle,
> acpi_scan_drop_device);
> acpi_device_del(acpi_root);
> - put_device(_root->dev);
> + acpi_bus_put_acpi_device(acpi_root);
> goto out;
> }
> }
> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index 834b7a1f7405..b728173a6171 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -707,6 +707,12 @@ acpi_dev_get_first_match_dev(const char *hid, const char
> *uid, s64 hrv);
> adev; \
> adev = acpi_dev_get_next_match_dev(adev, hid, uid,
Re: [PATCH v1 1/1] ACPI: utils: Document for_each_acpi_dev_match() macro
On Sat, Apr 10, 2021 at 3:29 PM Andy Shevchenko
wrote:
>
> The macro requires to call acpi_dev_put() on each iteration.
> Due to this it doesn't tolerate sudden disappearence of the devices.
>
> Document all these nuances to prevent users blindly call it without
> understanding the possible issues.
>
> While at it, add the note to the acpi_dev_get_next_match_dev() and
> advertise acpi_dev_put() instead of put_device() in the whole family
> of the helper functions.
>
> Fixes: bf263f64e804 ("media: ACPI / bus: Add acpi_dev_get_next_match_dev()
> and helper macro")
> Cc: Daniel Scally
> Signed-off-by: Andy Shevchenko
> ---
> drivers/acpi/utils.c| 12
> include/acpi/acpi_bus.h | 13 +
> 2 files changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
> index f1aff4dab476..3f3171e9aef5 100644
> --- a/drivers/acpi/utils.c
> +++ b/drivers/acpi/utils.c
> @@ -811,7 +811,7 @@ static int acpi_dev_match_cb(struct device *dev, const
> void *data)
> * Note that if the device is pluggable, it may since have disappeared.
> *
> * Note that unlike acpi_dev_found() this function checks the status
> - * of the device. So for devices which are present in the dsdt, but
> + * of the device. So for devices which are present in the DSDT, but
> * which are disabled (their _STA callback returns 0) this function
> * will return false.
> *
> @@ -838,7 +838,7 @@ EXPORT_SYMBOL(acpi_dev_present);
>
> /**
> * acpi_dev_get_next_match_dev - Return the next match of ACPI device
> - * @adev: Pointer to the previous acpi_device matching this @hid, @uid and
> @hrv
> + * @adev: Pointer to the previous ACPI device matching this @hid, @uid and
> @hrv
> * @hid: Hardware ID of the device.
> * @uid: Unique ID of the device, pass NULL to not check _UID
> * @hrv: Hardware Revision of the device, pass -1 to not check _HRV
The two cleanups above are not related to the subject of the patch.
Please separate them.
> @@ -846,7 +846,11 @@ EXPORT_SYMBOL(acpi_dev_present);
> * Return the next match of ACPI device if another matching device was
> present
> * at the moment of invocation, or NULL otherwise.
> *
> - * The caller is responsible to call put_device() on the returned device.
> + * Note, the function does not tolerate the sudden disappearance of @adev,
> e.g.
> + * in the case of hotplug event.
"of a hotplug event"
> That said, caller should ensure that this will
"the caller"
> + * never happen.
> + *
> + * The caller is responsible to call acpi_dev_put() on the returned device.
"responsible for"
And I would say "responsible for invoking".
> *
> * See additional information in acpi_dev_present() as well.
> */
> @@ -875,7 +879,7 @@ EXPORT_SYMBOL(acpi_dev_get_next_match_dev);
> * Return the first match of ACPI device if a matching device was present
> * at the moment of invocation, or NULL otherwise.
> *
> - * The caller is responsible to call put_device() on the returned device.
> + * The caller is responsible to call acpi_dev_put() on the returned device.
> *
> * See additional information in acpi_dev_present() as well.
> */
> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index f28b097c658f..834b7a1f7405 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -689,6 +689,19 @@ acpi_dev_get_next_match_dev(struct acpi_device *adev,
> const char *hid, const cha
> struct acpi_device *
> acpi_dev_get_first_match_dev(const char *hid, const char *uid, s64 hrv);
>
> +/**
> + * for_each_acpi_dev_match - iterate over ACPI devices that matching the
> criteria
> + * @adev: pointer to the matching ACPI device, NULL at the end of the loop
> + * @hid: Hardware ID of the device.
> + * @uid: Unique ID of the device, pass NULL to not check _UID
> + * @hrv: Hardware Revision of the device, pass -1 to not check _HRV
> + *
> + * The caller is responsible to call acpi_dev_put() on the returned device.
As per the above.
> + *
> + * Due to above requirement there is a window that may invalidate @adev and
> + * next iteration will use a dangling pointer, e.g. in the case of hotplug
> + * event. That said, caller should ensure that this will never happen.
> + */
> #define for_each_acpi_dev_match(adev, hid, uid, hrv) \
> for (adev = acpi_dev_get_first_match_dev(hid, uid, hrv);\
> adev; \
> --
> 2.31.1
>
[GIT PULL] ACPI fix for v5.12-rc7
Hi Linus, Please pull from the tag git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git \ acpi-5.12-rc7 with top-most commit fa26d0c778b432d3d9814ea82552e813b33eeb5c ACPI: processor: Fix build when CONFIG_ACPI_PROCESSOR=m on top of commit e49d033bddf5b565044e2abe4241353959bc9120 Linux 5.12-rc6 to receive an ACPI fix for 5.12-rc7. This fixes a build issue introduced by a previous fix in the ACPI processor driver (Vitaly Kuznetsov). Thanks! --- Vitaly Kuznetsov (1): ACPI: processor: Fix build when CONFIG_ACPI_PROCESSOR=m --- arch/x86/include/asm/smp.h| 2 +- arch/x86/kernel/smpboot.c | 26 -- drivers/acpi/processor_idle.c | 4 +--- 3 files changed, 14 insertions(+), 18 deletions(-)
Re: [PATCH 1/6] PM: runtime: enable wake irq after runtime_suspend hook called
On Fri, Apr 9, 2021 at 10:36 AM Chunfeng Yun wrote: > > On Fri, 2021-04-09 at 08:39 +0300, Tony Lindgren wrote: > > * Chunfeng Yun [210409 01:54]: > > > On Thu, 2021-04-08 at 19:41 +0200, Rafael J. Wysocki wrote: > > > > On Thu, Apr 8, 2021 at 11:35 AM Chunfeng Yun > > > > wrote: > > > > > > > > > > When the dedicated wake irq is level trigger, enable it before > > > > > calling runtime_suspend, will trigger an interrupt. > > > > > > > > > > e.g. > > > > > for a low level trigger type, it's low level at running time (0), > > > > > and becomes high level when enters suspend (runtime_suspend (1) is > > > > > called), a wakeup signal at (2) make it become low level, wake irq > > > > > will be triggered. > > > > > > > > > > -- > > > > >| ^ ^| > > > > > | | -- > > > > > |<---(0)--->|<--(1)--| (3) (2)(4) > > > > > > > > > > if we enable the wake irq before calling runtime_suspend during (0), > > > > > an interrupt will arise, it causes resume immediately; > > > > > > > > But that's necessary to avoid missing a wakeup interrupt, isn't it? > > > That's also what I worry about. > > > > Yeah sounds like this patch will lead into missed wakeirqs. > If miss level trigger wakeirqs, that means HW doesn't latch it? is it HW > limitation? If it's level-triggered, it won't be missed, but then it is just pointless to suspend the device when wakeup is being signaled in the first place. I'm not sure if I understand the underlying problem correctly. Is it about addressing spurious wakeups?
Re: [GIT PULL] cpuidle for v5.13-rc1
On Thu, Apr 8, 2021 at 8:02 PM Daniel Lezcano wrote: > > On 08/04/2021 19:24, Rafael J. Wysocki wrote: > > On Thu, Apr 8, 2021 at 5:10 PM Daniel Lezcano > > wrote: > >> > >> > >> Hi Rafael, > >> > >> please consider pulling the following change for cpuidle on ARM for > >> v5.13-rc1 > >> > >> Thanks > >> > >> -- Daniel > >> > >> > >> The following changes since commit > >> dde8740bd9b505c58ec8b2277d5d55c6951b7e42: > >> > >> Merge branch 'acpi-processor-fixes' into linux-next (2021-04-07 > >> 19:02:56 +0200) > > > > Can you please rebase this on 5.12-rc6? My linux-next branch is > > re-merged on a regular basis. > > > > Generally speaking, if you want me to pull from a branch, please make > > sure that this branch is based on a commit present in the Linus' tree, > > preferably one of the commits tagged as -rc or a specific merge. > > > > Sure, here is the pull request based on v5.12-rc6 with the signed tag > cpuidle-v5.13-rc1 Pulled, thanks! > The following changes since commit e49d033bddf5b565044e2abe4241353959bc9120: > > Linux 5.12-rc6 (2021-04-04 14:15:36 -0700) > > are available in the Git repository at: > > https://git.linaro.org/people/daniel.lezcano/linux tags/cpuidle-v5.13-rc1 > > for you to fetch changes up to 498ba2a8a2756694b6f357426dbc8a5e6b6c: > > cpuidle: Fix ARM_QCOM_SPM_CPUIDLE configuration (2021-04-08 19:54:14 > +0200) > > > - Fix the C7 state on the tegra114 by setting the L2-no-flush flag > unconditionally (Dmitry Osipenko) > > - Remove the do_idle firmware call as it is not supported by the ATF > on tegra SoC (Dmitry Osipenko) > > - Add a missing dependency on CONFIG_MMU to prevent linkage error (He > Ying) > > > Dmitry Osipenko (2): > cpuidle: tegra: Fix C7 idling state on Tegra114 > cpuidle: tegra: Remove do_idle firmware call > > He Ying (1): > cpuidle: Fix ARM_QCOM_SPM_CPUIDLE configuration > > drivers/cpuidle/Kconfig.arm | 2 +- > drivers/cpuidle/cpuidle-tegra.c | 19 --- > 2 files changed, 5 insertions(+), 16 deletions(-) > > -- > <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs > > Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | > <http://twitter.com/#!/linaroorg> Twitter | > <http://www.linaro.org/linaro-blog/> Blog
Re: [PATCH] ACPI / CPPC: Replace cppc_attr with kobj_attribute
On Wed, Apr 7, 2021 at 11:32 PM Nathan Chancellor wrote:
>
> All of the CPPC sysfs show functions are called via indirect call in
> kobj_attr_show(), where they should be of type
>
> ssize_t (*show)(struct kobject *kobj, struct kobj_attribute *attr, char *buf);
>
> because that is the type of the ->show() member in
> 'struct kobj_attribute' but they are actually of type
>
> ssize_t (*show)(struct kobject *kobj, struct attribute *attr, char *buf);
>
> because of the ->show() member in 'struct cppc_attr', resulting in a
> Control Flow Integrity violation [1].
>
> $ cat /sys/devices/system/cpu/cpu0/acpi_cppc/highest_perf
> 3400
>
> $ dmesg | grep "CFI failure"
> [ 175.970559] CFI failure (target: show_highest_perf+0x0/0x8):
>
> As far as I can tell, the only different between 'struct cppc_attr' and
> 'struct kobj_attribute' aside from the type of the attr parameter is the
> type of the count parameter in the ->store() member (ssize_t vs.
> size_t), which does not actually matter because all of these nodes are
> read-only.
>
> Eliminate 'struct cppc_attr' in favor of 'struct kobj_attribute' to fix
> the violation.
>
> [1]:
> https://lore.kernel.org/r/20210401233216.2540591-1-samitolva...@google.com/
>
> Fixes: 158c998ea44b ("ACPI / CPPC: add sysfs support to compute delivered
> performance")
> Link: https://github.com/ClangBuiltLinux/linux/issues/1343
> Signed-off-by: Nathan Chancellor
> ---
> drivers/acpi/cppc_acpi.c | 14 +++---
> 1 file changed, 3 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
> index 69057fcd2c04..a5e6fd0bafa1 100644
> --- a/drivers/acpi/cppc_acpi.c
> +++ b/drivers/acpi/cppc_acpi.c
> @@ -119,23 +119,15 @@ static DEFINE_PER_CPU(struct cpc_desc *, cpc_desc_ptr);
> */
> #define NUM_RETRIES 500ULL
>
> -struct cppc_attr {
> - struct attribute attr;
> - ssize_t (*show)(struct kobject *kobj,
> - struct attribute *attr, char *buf);
> - ssize_t (*store)(struct kobject *kobj,
> - struct attribute *attr, const char *c, ssize_t count);
> -};
> -
> #define define_one_cppc_ro(_name) \
> -static struct cppc_attr _name =\
> +static struct kobj_attribute _name = \
> __ATTR(_name, 0444, show_##_name, NULL)
>
> #define to_cpc_desc(a) container_of(a, struct cpc_desc, kobj)
>
> #define show_cppc_data(access_fn, struct_name, member_name)\
> static ssize_t show_##member_name(struct kobject *kobj, \
> - struct attribute *attr, char *buf) \
> + struct kobj_attribute *attr, char *buf) \
> { \
> struct cpc_desc *cpc_ptr = to_cpc_desc(kobj); \
> struct struct_name st_name = {0}; \
> @@ -161,7 +153,7 @@ show_cppc_data(cppc_get_perf_ctrs, cppc_perf_fb_ctrs,
> reference_perf);
> show_cppc_data(cppc_get_perf_ctrs, cppc_perf_fb_ctrs, wraparound_time);
>
> static ssize_t show_feedback_ctrs(struct kobject *kobj,
> - struct attribute *attr, char *buf)
> + struct kobj_attribute *attr, char *buf)
> {
> struct cpc_desc *cpc_ptr = to_cpc_desc(kobj);
> struct cppc_perf_fb_ctrs fb_ctrs = {0};
>
> base-commit: 454859c552da78b0f587205d308401922b56863e
> --
Applied as 5.13 material, thanks!
Re: [PATCH 1/6] PM: runtime: enable wake irq after runtime_suspend hook called
On Thu, Apr 8, 2021 at 11:35 AM Chunfeng Yun wrote: > > When the dedicated wake irq is level trigger, enable it before > calling runtime_suspend, will trigger an interrupt. > > e.g. > for a low level trigger type, it's low level at running time (0), > and becomes high level when enters suspend (runtime_suspend (1) is > called), a wakeup signal at (2) make it become low level, wake irq > will be triggered. > > -- >| ^ ^| > | | -- > |<---(0)--->|<--(1)--| (3) (2)(4) > > if we enable the wake irq before calling runtime_suspend during (0), > an interrupt will arise, it causes resume immediately; But that's necessary to avoid missing a wakeup interrupt, isn't it? > enable wake irq after calling runtime_suspend, e.g. at (3) or (4), > will works. > > This patch seems no side effect on edge trigger wake irq. > > Signed-off-by: Chunfeng Yun > --- > drivers/base/power/runtime.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c > index a46a7e30881b..796739a015a5 100644 > --- a/drivers/base/power/runtime.c > +++ b/drivers/base/power/runtime.c > @@ -619,12 +619,12 @@ static int rpm_suspend(struct device *dev, int rpmflags) > __update_runtime_status(dev, RPM_SUSPENDING); > > callback = RPM_GET_CALLBACK(dev, runtime_suspend); > - > - dev_pm_enable_wake_irq_check(dev, true); > retval = rpm_callback(callback, dev); > if (retval) > goto fail; > > + dev_pm_enable_wake_irq_check(dev, true); > + > no_callback: > __update_runtime_status(dev, RPM_SUSPENDED); > pm_runtime_deactivate_timer(dev); > @@ -659,7 +659,6 @@ static int rpm_suspend(struct device *dev, int rpmflags) > return retval; > > fail: > - dev_pm_disable_wake_irq_check(dev); > __update_runtime_status(dev, RPM_ACTIVE); > dev->power.deferred_resume = false; > wake_up_all(>power.wait_queue); > -- > 2.18.0 >
Re: [PATCH -next] PM: fix typos in comments
On Thu, Apr 8, 2021 at 10:14 AM Lu Jialin wrote: > > Change occured to occurred in kernel/power/autosleep.c. > Change consiting to consisting in kernel/power/snapshot.c. > Change avaiable to available in kernel/power/swap.c. > No functionality changed. > > Signed-off-by: Lu Jialin > --- > kernel/power/autosleep.c | 2 +- > kernel/power/snapshot.c | 2 +- > kernel/power/swap.c | 2 +- > 3 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/kernel/power/autosleep.c b/kernel/power/autosleep.c > index 9af5a50d3489..b29c8aca7486 100644 > --- a/kernel/power/autosleep.c > +++ b/kernel/power/autosleep.c > @@ -54,7 +54,7 @@ static void try_to_suspend(struct work_struct *work) > goto out; > > /* > -* If the wakeup occured for an unknown reason, wait to prevent the > +* If the wakeup occurred for an unknown reason, wait to prevent the > * system from trying to suspend and waking up in a tight loop. > */ > if (final_count == initial_count) > diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c > index 64b7aab9aee4..27cb4e7086b7 100644 > --- a/kernel/power/snapshot.c > +++ b/kernel/power/snapshot.c > @@ -329,7 +329,7 @@ static void *chain_alloc(struct chain_allocator *ca, > unsigned int size) > /** > * Data types related to memory bitmaps. > * > - * Memory bitmap is a structure consiting of many linked lists of > + * Memory bitmap is a structure consisting of many linked lists of > * objects. The main list's elements are of type struct zone_bitmap > * and each of them corresonds to one zone. For each zone bitmap > * object there is a list of objects of type struct bm_block that > diff --git a/kernel/power/swap.c b/kernel/power/swap.c > index 72e33054a2e1..bea3cb8afa11 100644 > --- a/kernel/power/swap.c > +++ b/kernel/power/swap.c > @@ -884,7 +884,7 @@ static int save_image_lzo(struct swap_map_handle *handle, > * enough_swap - Make sure we have enough swap to save the image. > * > * Returns TRUE or FALSE after checking the total amount of swap > - * space avaiable from the resume partition. > + * space available from the resume partition. > */ > > static int enough_swap(unsigned int nr_pages) > -- Applied as 5.13 material, thanks!
Re: [GIT PULL] devfreq next for v5.13
On Thu, Apr 8, 2021 at 8:12 AM Chanwoo Choi wrote: > > Dear Rafael, > > This is devfreq-next pull request for v5.13-rc1. I add detailed description of > this pull request on the following tag. Please pull devfreq with following > updates. > - tag name : devfreq-next-for-5.12 > > This pull request contains the immutable branch to keep the immutable patch[1] > between devfreq and drm for gpu driver. > [1] > https://patchwork.kernel.org/project/linux-pm/patch/20210308133041.10516-1-daniel.lezc...@linaro.org/ > > Best Regards, > Chanwoo Choi > > > The following changes since commit e49d033bddf5b565044e2abe4241353959bc9120: > > Linux 5.12-rc6 (2021-04-04 14:15:36 -0700) > > are available in the Git repository at: > > git://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/linux.git > tags/devfreq-next-for-5.13 > > for you to fetch changes up to 0a7dc8318c2817fb33dc50946f7ca6e0ff28f036: > > PM / devfreq: imx8m-ddrc: Remove unneeded of_match_ptr() (2021-04-08 > 13:14:51 +0900) Pulled, thanks!
Re: [GIT PULL] cpuidle for v5.13-rc1
On Thu, Apr 8, 2021 at 5:10 PM Daniel Lezcano wrote: > > > Hi Rafael, > > please consider pulling the following change for cpuidle on ARM for > v5.13-rc1 > > Thanks > > -- Daniel > > > The following changes since commit dde8740bd9b505c58ec8b2277d5d55c6951b7e42: > > Merge branch 'acpi-processor-fixes' into linux-next (2021-04-07 > 19:02:56 +0200) Can you please rebase this on 5.12-rc6? My linux-next branch is re-merged on a regular basis. Generally speaking, if you want me to pull from a branch, please make sure that this branch is based on a commit present in the Linus' tree, preferably one of the commits tagged as -rc or a specific merge.
Re: [PATCH -next] PM: runtime: Replace inline function pm_runtime_callbacks_present()
On Fri, Apr 2, 2021 at 8:14 AM YueHaibing wrote:
>
> commit 9a7875461fd0 ("PM: runtime: Replace pm_runtime_callbacks_present()")
> forget to change the inline version.
>
> Fixes: 9a7875461fd0 ("PM: runtime: Replace pm_runtime_callbacks_present()")
> Signed-off-by: YueHaibing
> ---
> include/linux/pm_runtime.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
> index b492ae00cc90..6c08a085367b 100644
> --- a/include/linux/pm_runtime.h
> +++ b/include/linux/pm_runtime.h
> @@ -265,7 +265,7 @@ static inline void pm_runtime_no_callbacks(struct device
> *dev) {}
> static inline void pm_runtime_irq_safe(struct device *dev) {}
> static inline bool pm_runtime_is_irq_safe(struct device *dev) { return
> false; }
>
> -static inline bool pm_runtime_callbacks_present(struct device *dev) { return
> false; }
> +static inline bool pm_runtime_has_no_callbacks(struct device *dev) { return
> false; }
> static inline void pm_runtime_mark_last_busy(struct device *dev) {}
> static inline void __pm_runtime_use_autosuspend(struct device *dev,
> bool use) {}
> --
Applied as 5.13 material, thanks!
Re: [PATCH -next] freezer: Remove unused inline function try_to_freeze_nowarn()
On Thu, Apr 1, 2021 at 7:58 PM YueHaibing wrote:
>
> There is no caller in tree, so can remove it.
>
> Signed-off-by: YueHaibing
> ---
> include/linux/freezer.h | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/include/linux/freezer.h b/include/linux/freezer.h
> index 27828145ca09..0621c5f86c39 100644
> --- a/include/linux/freezer.h
> +++ b/include/linux/freezer.h
> @@ -279,7 +279,6 @@ static inline int freeze_kernel_threads(void) { return
> -ENOSYS; }
> static inline void thaw_processes(void) {}
> static inline void thaw_kernel_threads(void) {}
>
> -static inline bool try_to_freeze_nowarn(void) { return false; }
> static inline bool try_to_freeze(void) { return false; }
>
> static inline void freezer_do_not_count(void) {}
> --
Applied as 5.13 material, thanks!
Re: [PATCH] linux/intel_rapl.h: Modify struct declaration
On Tue, Mar 30, 2021 at 8:40 AM Wan Jiabing wrote:
>
> struct rapl_package is declared twice. One has been declared
> at 80th line.
> By reviewing the code, it should declare struct rapl_domain
> rather than rapl_package. Modify it.
>
> Signed-off-by: Wan Jiabing
> ---
> include/linux/intel_rapl.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/intel_rapl.h b/include/linux/intel_rapl.h
> index 50b8398ffd21..93780834fc8f 100644
> --- a/include/linux/intel_rapl.h
> +++ b/include/linux/intel_rapl.h
> @@ -33,7 +33,7 @@ enum rapl_domain_reg_id {
> RAPL_DOMAIN_REG_MAX,
> };
>
> -struct rapl_package;
> +struct rapl_domain;
>
> enum rapl_primitives {
> ENERGY_COUNTER,
> --
Applied as 5.13 material with edited subject and changelog, thanks!
Re: [PATCH v5 1/1] use crc32 instead of md5 for hibernation e820 integrity check
On Thu, Apr 8, 2021 at 5:26 PM Eric Biggers wrote:
>
> On Thu, Apr 08, 2021 at 03:32:38PM +0200, Rafael J. Wysocki wrote:
> > On Thu, Apr 8, 2021 at 3:15 PM Chris von Recklinghausen
> > wrote:
> > >
> > > Suspend fails on a system in fips mode because md5 is used for the e820
> > > integrity check and is not available. Use crc32 instead.
> > >
> > > This patch changes the integrity check algorithm from md5 to
> > > crc32. This integrity check is used only to verify accidental
> > > corruption of the hybernation data
> >
> > It isn't used for that.
> >
> > In fact, it is used to detect differences between the memory map used
> > before hibernation and the one made available by the BIOS during the
> > subsequent resume. And the check is there, because it is generally
> > unsafe to load the hibernation image into memory if the current memory
> > map doesn't match the one used when the image was created.
>
> So what types of "differences" are you trying to detect? If you need to
> detect
> differences caused by someone who maliciously made changes ("malicious"
> implies
> they may try to avoid detection), then you need to use a cryptographic hash
> function (or a cryptographic MAC if the hash value isn't stored separately).
> If
> you only need to detect non-malicious changes (normally these would be called
> "accidental" changes, but sure, it could be changes that are "intentionally"
> made provided that the other side can be trusted to not try to avoid
> detection...)
That's the case here.
> then a non-cryptographic checksum would be sufficient.
Re: [PATCH v2 1/6] software node: Free resources explicitly when swnode_register() fails
On Thu, Apr 8, 2021 at 5:18 PM Andy Shevchenko wrote: > > On Thu, Apr 08, 2021 at 05:04:32PM +0200, Rafael J. Wysocki wrote: > > On Thu, Apr 8, 2021 at 4:50 PM Andy Shevchenko > > wrote: > > > On Thu, Apr 08, 2021 at 04:15:37PM +0200, Rafael J. Wysocki wrote: > > > > On Wed, Mar 31, 2021 at 1:06 PM Heikki Krogerus > > > > wrote: > > > > > > > > > > On Mon, Mar 29, 2021 at 06:12:02PM +0300, Andy Shevchenko wrote: > > > > > > Currently we have a slightly twisted logic in swnode_register(). > > > > > > It frees resources that it doesn't allocate on error path and > > > > > > in once case it relies on the ->release() implementation. > > > > > > > > > > > > Untwist the logic by freeing resources explicitly when > > > > > > swnode_register() > > > > > > fails. Currently it happens only in fwnode_create_software_node(). > > > > > > > > > > > > Signed-off-by: Andy Shevchenko > > > > > > > > > > It all looks OK to me. FWIW, for the whole series: > > > > > > > > > > Reviewed-by: Heikki Krogerus > > > > > > > > Whole series applied (with some minor changelog edits) as 5.13 > > > > material, thanks! > > > > > > It seems Greg applied it already. Was it dropped there? > > > > Did he? > > > > OK, so please let me know if it's still there in the Greg's tree. > > Here [1] what I see. Seems still there. > > [1]: > https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git/commit/?h=driver-core-next=6e11b376fd74356e32d842be588e12dc9bf6e197 I will not be applying it then, sorry for the confusion.
Re: [PATCH v2] ACPI / hotplug / PCI: fix memory leak in enable_slot()
On Thu, Mar 25, 2021 at 8:27 AM Zhiqiang Liu wrote: > > From: Feilong Lin > > In enable_slot() in drivers/pci/hotplug/acpiphp_glue.c, if pci_get_slot() > will return NULL, we will do not set SLOT_ENABLED flag of slot. if one > device is found by calling pci_get_slot(), its reference count will be > increased. In this case, we did not call pci_dev_put() to decrement the > its reference count, the memory of the device (struct pci_dev type) will > leak. > > Fix it by calling pci_dev_put() to decrement its reference count after that > pci_get_slot() returns a PCI device. > > Signed-off-by: Feilong Lin > Signed-off-by: Zhiqiang Liu > -- > v2: rewrite subject and commit log as suggested by Bjorn Helgaas. The fix is correct AFAICS, so Reviewed-by: Rafael J. Wysocki Bjorn, has this been applied already? If not, do you want me to take it or are you going to queue it up yourself? > --- > drivers/pci/hotplug/acpiphp_glue.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/pci/hotplug/acpiphp_glue.c > b/drivers/pci/hotplug/acpiphp_glue.c > index 3365c93abf0e..f031302ad401 100644 > --- a/drivers/pci/hotplug/acpiphp_glue.c > +++ b/drivers/pci/hotplug/acpiphp_glue.c > @@ -533,6 +533,7 @@ static void enable_slot(struct acpiphp_slot *slot, bool > bridge) > slot->flags &= ~SLOT_ENABLED; > continue; > } > + pci_dev_put(dev); > } > } > > -- > 2.19.1 >
Re: [PATCH v2 1/6] software node: Free resources explicitly when swnode_register() fails
On Thu, Apr 8, 2021 at 4:50 PM Andy Shevchenko wrote: > > On Thu, Apr 08, 2021 at 04:15:37PM +0200, Rafael J. Wysocki wrote: > > On Wed, Mar 31, 2021 at 1:06 PM Heikki Krogerus > > wrote: > > > > > > On Mon, Mar 29, 2021 at 06:12:02PM +0300, Andy Shevchenko wrote: > > > > Currently we have a slightly twisted logic in swnode_register(). > > > > It frees resources that it doesn't allocate on error path and > > > > in once case it relies on the ->release() implementation. > > > > > > > > Untwist the logic by freeing resources explicitly when swnode_register() > > > > fails. Currently it happens only in fwnode_create_software_node(). > > > > > > > > Signed-off-by: Andy Shevchenko > > > > > > It all looks OK to me. FWIW, for the whole series: > > > > > > Reviewed-by: Heikki Krogerus > > > > Whole series applied (with some minor changelog edits) as 5.13 material, > > thanks! > > It seems Greg applied it already. Was it dropped there? Did he? OK, so please let me know if it's still there in the Greg's tree.
Re: [PATCH v3 00/12] acpi: fix some coding style issues
On Sat, Mar 27, 2021 at 1:11 PM Xiaofei Tan wrote: > > Fix some coding style issues reported by checkpatch.pl. > Only cleanup and no function changes. > > Differences from v2 to v3: > - Remove the modifications that may cause function change. > > Differences from v1 to v2: > - Add subsystem and module name in the name of patch 05/15. > - Change to use more proper module name for some patch names. > > Xiaofei Tan (12): > ACPI: APD: fix a block comment align issue > ACPI: processor: fix some coding style issues > ACPI: ipmi: remove useless return statement for void function > ACPI: LPSS: add a missed blank line after declarations > ACPI: acpi_pad: add a missed blank line after declarations > ACPI: battery: fix some coding style issues > ACPI: button: fix some coding style issues > ACPI: CPPC: fix some coding style issues > ACPI: custom_method: fix a coding style issue > ACPI: PM: add a missed blank line after declarations > ACPI: sysfs: fix some coding style issues > ACPI: dock: fix some coding style issues All applied as 5.13 material, thanks!
Re: [PATCH v2] ACPI: AC: fix some coding style issues
On Sat, Mar 27, 2021 at 4:50 AM Xiaofei Tan wrote:
>
> Fix some coding style issues reported by checkpatch.pl, including
> following types:
>
> ERROR: "foo * bar" should be "foo *bar"
> ERROR: code indent should use tabs where possible
> WARNING: Block comments use a trailing */ on a separate line
> WARNING: braces {} are not necessary for single statement blocks
> WARNING: void function return statements are not generally useful
> WARNING: CVS style keyword markers, these will _not_ be updated
>
> Signed-off-by: Xiaofei Tan
> ---
> drivers/acpi/ac.c | 28
> 1 file changed, 8 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/acpi/ac.c b/drivers/acpi/ac.c
> index b86ee6e..b0cb662 100644
> --- a/drivers/acpi/ac.c
> +++ b/drivers/acpi/ac.c
> @@ -1,6 +1,6 @@
> // SPDX-License-Identifier: GPL-2.0-or-later
> /*
> - * acpi_ac.c - ACPI AC Adapter Driver ($Revision: 27 $)
> + * acpi_ac.c - ACPI AC Adapter Driver (Revision: 27)
> *
> * Copyright (C) 2001, 2002 Andy Grover
> * Copyright (C) 2001, 2002 Paul Diefenbaugh
> @@ -78,17 +78,14 @@ static struct acpi_driver acpi_ac_driver = {
> struct acpi_ac {
> struct power_supply *charger;
> struct power_supply_desc charger_desc;
> - struct acpi_device * device;
> + struct acpi_device *device;
> unsigned long long state;
> struct notifier_block battery_nb;
> };
>
> #define to_acpi_ac(x) power_supply_get_drvdata(x)
>
> -/* --
> - AC Adapter Management
> -
> -- */
> -
> +/* AC Adapter Management */
> static int acpi_ac_get_state(struct acpi_ac *ac)
> {
> acpi_status status = AE_OK;
> @@ -109,9 +106,7 @@ static int acpi_ac_get_state(struct acpi_ac *ac)
> return 0;
> }
>
> -/* --
> -sysfs I/F
> -
> -- */
> +/* sysfs I/F */
> static int get_ac_property(struct power_supply *psy,
>enum power_supply_property psp,
>union power_supply_propval *val)
> @@ -138,10 +133,7 @@ static enum power_supply_property ac_props[] = {
> POWER_SUPPLY_PROP_ONLINE,
> };
>
> -/* --
> - Driver Model
> -
> -- */
> -
> +/* Driver Model */
> static void acpi_ac_notify(struct acpi_device *device, u32 event)
> {
> struct acpi_ac *ac = acpi_driver_data(device);
> @@ -174,8 +166,6 @@ static void acpi_ac_notify(struct acpi_device *device,
> u32 event)
> acpi_notifier_call_chain(device, event, (u32) ac->state);
> kobject_uevent(>charger->dev.kobj, KOBJ_CHANGE);
> }
> -
> - return;
> }
>
> static int acpi_ac_battery_notify(struct notifier_block *nb,
> @@ -282,9 +272,8 @@ static int acpi_ac_add(struct acpi_device *device)
> ac->battery_nb.notifier_call = acpi_ac_battery_notify;
> register_acpi_notifier(>battery_nb);
> end:
> - if (result) {
> + if (result)
> kfree(ac);
> - }
>
> return result;
> }
> @@ -293,7 +282,7 @@ static int acpi_ac_add(struct acpi_device *device)
> static int acpi_ac_resume(struct device *dev)
> {
> struct acpi_ac *ac;
> - unsigned old_state;
> + unsigned int old_state;
>
> if (!dev)
> return -EINVAL;
> @@ -352,9 +341,8 @@ static int __init acpi_ac_init(void)
> }
>
> result = acpi_bus_register_driver(_ac_driver);
> - if (result < 0) {
> + if (result < 0)
> return -ENODEV;
> - }
>
> return 0;
> }
> --
Applied as 5.13 material, thanks!
Re: [PATCH v2 1/6] software node: Free resources explicitly when swnode_register() fails
On Wed, Mar 31, 2021 at 1:06 PM Heikki Krogerus wrote: > > On Mon, Mar 29, 2021 at 06:12:02PM +0300, Andy Shevchenko wrote: > > Currently we have a slightly twisted logic in swnode_register(). > > It frees resources that it doesn't allocate on error path and > > in once case it relies on the ->release() implementation. > > > > Untwist the logic by freeing resources explicitly when swnode_register() > > fails. Currently it happens only in fwnode_create_software_node(). > > > > Signed-off-by: Andy Shevchenko > > It all looks OK to me. FWIW, for the whole series: > > Reviewed-by: Heikki Krogerus Whole series applied (with some minor changelog edits) as 5.13 material, thanks!
Re: [PATCH v5 1/1] use crc32 instead of md5 for hibernation e820 integrity check
On Thu, Apr 8, 2021 at 3:15 PM Chris von Recklinghausen
wrote:
>
> Suspend fails on a system in fips mode because md5 is used for the e820
> integrity check and is not available. Use crc32 instead.
>
> This patch changes the integrity check algorithm from md5 to
> crc32. This integrity check is used only to verify accidental
> corruption of the hybernation data
It isn't used for that.
In fact, it is used to detect differences between the memory map used
before hibernation and the one made available by the BIOS during the
subsequent resume. And the check is there, because it is generally
unsafe to load the hibernation image into memory if the current memory
map doesn't match the one used when the image was created.
> and is not intended as a cryptographic integrity check.
But this is true nevertheless, so I would write:
"The purpose of the integrity check is to detect possible differences
between the memory map used at the time when the hibernation image is
about to be loaded into memory and the memory map used at the image
creation time, because it is generally unsafe to load the image if the
current memory map doesn't match the one used when it was created. so
it is not intended as a cryptographic integrity check."
And please make the md5 spelling consistent.
> Md5 is overkill in this case and also disabled in FIPS mode because it
> is known to be broken for cryptographic purposes.
>
> Fixes: 62a03defeabd ("PM / hibernate: Verify the consistent of e820 memory map
>by md5 digest")
>
> Tested-by: Dexuan Cui
> Reviewed-by: Dexuan Cui
> Signed-off-by: Chris von Recklinghausen
> ---
> v1 -> v2
>bump up RESTORE_MAGIC
> v2 -> v3
>move embelishment from cover letter to commit comments (no code change)
> v3 -> v4
>add note to comments that md5 isn't used for encryption here.
> v4 -> v5
>reword comment per Simo's suggestion
>
> arch/x86/power/hibernate.c | 35 +++
> 1 file changed, 19 insertions(+), 16 deletions(-)
>
> diff --git a/arch/x86/power/hibernate.c b/arch/x86/power/hibernate.c
> index cd3914fc9f3d..b56172553275 100644
> --- a/arch/x86/power/hibernate.c
> +++ b/arch/x86/power/hibernate.c
> @@ -55,31 +55,31 @@ int pfn_is_nosave(unsigned long pfn)
> }
>
>
> -#define MD5_DIGEST_SIZE 16
> +#define CRC32_DIGEST_SIZE 16
>
> struct restore_data_record {
> unsigned long jump_address;
> unsigned long jump_address_phys;
> unsigned long cr3;
> unsigned long magic;
> - u8 e820_digest[MD5_DIGEST_SIZE];
> + u8 e820_digest[CRC32_DIGEST_SIZE];
> };
>
> -#if IS_BUILTIN(CONFIG_CRYPTO_MD5)
> +#if IS_BUILTIN(CONFIG_CRYPTO_CRC32)
> /**
> - * get_e820_md5 - calculate md5 according to given e820 table
> + * get_e820_crc32 - calculate crc32 according to given e820 table
> *
> * @table: the e820 table to be calculated
> - * @buf: the md5 result to be stored to
> + * @buf: the crc32 result to be stored to
> */
> -static int get_e820_md5(struct e820_table *table, void *buf)
> +static int get_e820_crc32(struct e820_table *table, void *buf)
> {
> struct crypto_shash *tfm;
> struct shash_desc *desc;
> int size;
> int ret = 0;
>
> - tfm = crypto_alloc_shash("md5", 0, 0);
> + tfm = crypto_alloc_shash("crc32", 0, 0);
> if (IS_ERR(tfm))
> return -ENOMEM;
>
> @@ -107,24 +107,24 @@ static int get_e820_md5(struct e820_table *table, void
> *buf)
>
> static int hibernation_e820_save(void *buf)
> {
> - return get_e820_md5(e820_table_firmware, buf);
> + return get_e820_crc32(e820_table_firmware, buf);
> }
>
> static bool hibernation_e820_mismatch(void *buf)
> {
> int ret;
> - u8 result[MD5_DIGEST_SIZE];
> + u8 result[CRC32_DIGEST_SIZE];
>
> - memset(result, 0, MD5_DIGEST_SIZE);
> + memset(result, 0, CRC32_DIGEST_SIZE);
> /* If there is no digest in suspend kernel, let it go. */
> - if (!memcmp(result, buf, MD5_DIGEST_SIZE))
> + if (!memcmp(result, buf, CRC32_DIGEST_SIZE))
> return false;
>
> - ret = get_e820_md5(e820_table_firmware, result);
> + ret = get_e820_crc32(e820_table_firmware, result);
> if (ret)
> return true;
>
> - return memcmp(result, buf, MD5_DIGEST_SIZE) ? true : false;
> + return memcmp(result, buf, CRC32_DIGEST_SIZE) ? true : false;
> }
> #else
> static int hibernation_e820_save(void *buf)
> @@ -134,15 +134,15 @@ static int hibernation_e820_save(void *buf)
>
> static bool hibernation_e820_mismatch(void *buf)
> {
> - /* If md5 is not builtin for restore kernel, let it go. */
> + /* If crc32 is not builtin for restore kernel, let it go. */
> return false;
> }
> #endif
>
> #ifdef CONFIG_X86_64
> -#define RESTORE_MAGIC 0x23456789ABCDEF01UL
> +#define RESTORE_MAGIC 0x23456789ABCDEF02UL
> #else
> -#define RESTORE_MAGIC 0x12345678UL
> +#define RESTORE_MAGIC 0x12345679UL
> #endif
>
Re: [PATCH v1 0/5] cpuidle: Take possible negative "sleep length" values into account
On Mon, Mar 29, 2021 at 8:38 PM Rafael J. Wysocki wrote: > > Hi All, > > As follows from the discussion triggered by the patch at > > https://lore.kernel.org/lkml/20210311123708.23501-2-frede...@kernel.org/ > > the cpuidle governors using tick_nohz_get_sleep_length() assume it to always > return positive values which is not correct in general. > > To address this issues, first document the fact that negative values can > be returned by tick_nohz_get_sleep_length() (patch [1/5]). Then, in > preparation for more substantial changes, change the data type of two > fields in struct cpuidle_state to s64 so they can be used in computations > involving negative numbers safely (patch [2/5]). > > Next, adjust the teo governor a bit so that negative "sleep length" values > are counted like zero by it (patch [3/5]) and modify it so as to avoid > mishandling negative "sleep length" values (patch [4/5]). > > Finally, make the menu governor take negative "sleep length" values into > account properly (patch [5/5]). > > Please see the changelogs of the patches for details. Given no objections or concerns regarding this lot, let me queue it up. Thanks!
Re: [PATCH 3/7] PM: runtime: remove kernel-doc warnings
On Thu, Apr 1, 2021 at 4:13 PM Pierre-Louis Bossart wrote: > > > > On 4/1/21 8:40 AM, Rafael J. Wysocki wrote: > > On Thu, Apr 1, 2021 at 1:26 AM Pierre-Louis Bossart > > wrote: > >> > >> remove make W=1 warnings > >> > >> drivers/base/power/runtime.c:926: warning: Function parameter or > >> member 'timer' not described in 'pm_suspend_timer_fn' > >> > >> drivers/base/power/runtime.c:926: warning: Excess function parameter > >> 'data' description in 'pm_suspend_timer_fn' > >> > >> Signed-off-by: Pierre-Louis Bossart > >> --- > >> drivers/base/power/runtime.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c > >> index fe1dad68aee4..1fc1a992f90c 100644 > >> --- a/drivers/base/power/runtime.c > >> +++ b/drivers/base/power/runtime.c > >> @@ -951,7 +951,7 @@ static void pm_runtime_work(struct work_struct *work) > >> > >> /** > >>* pm_suspend_timer_fn - Timer function for pm_schedule_suspend(). > >> - * @data: Device pointer passed by pm_schedule_suspend(). > >> + * @timer: hrtimer used by pm_schedule_suspend(). > >>* > >>* Check if the time is right and queue a suspend request. > >>*/ > >> -- > > > > I can apply this along with the [4-5/7]. Do you want me to do that? > > Works for me. I wasn't sure by looking at the MAINTAINERS file which > files in drivers/base/ are maintained by whom, so sent the patches as a > single set. All three applied now as 5.13 material, thanks!
Re: [PATCH] include: linux: pm: Remove duplicate declaration
On Wed, Mar 24, 2021 at 8:30 AM Wan Jiabing wrote: > > struct device is declared twice.So remove the duplicate. > > Signed-off-by: Wan Jiabing > --- > include/linux/pm.h | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/include/linux/pm.h b/include/linux/pm.h > index 482313a8ccfc..c9657408fee1 100644 > --- a/include/linux/pm.h > +++ b/include/linux/pm.h > @@ -39,7 +39,6 @@ static inline void pm_vt_switch_unregister(struct device > *dev) > * Device power management > */ > > -struct device; > > #ifdef CONFIG_PM > extern const char power_group_name[]; /* = "power" */ > -- Applied as 5.13 material, thanks!
Re: [PATCH v3] ACPI: processor: Fix build when CONFIG_ACPI_PROCESSOR=m
On Tue, Apr 6, 2021 at 5:56 PM Vitaly Kuznetsov wrote:
>
> Commit 8c182bd7 ("ACPI: processor: Fix CPU0 wakeup in
> acpi_idle_play_dead()") tried to fix CPU0 hotplug breakage by copying
> wakeup_cpu0() + start_cpu0() logic from hlt_play_dead()//mwait_play_dead()
> into acpi_idle_play_dead(). The problem is that these functions are not
> exported to modules so when CONFIG_ACPI_PROCESSOR=m build fails.
>
> The issue could've been fixed by exporting both wakeup_cpu0()/start_cpu0()
> (the later from assembly) but it seems putting the whole pattern into a
> new function and exporting it instead is better.
>
> Reported-by: kernel test robot
> Fixes: 8c182bd7 ("CPI: processor: Fix CPU0 wakeup in
> acpi_idle_play_dead()")
> Cc: # 5.10+
> Signed-off-by: Vitaly Kuznetsov
Applied as 5.12-rc material, thanks!
> ---
> Changes since v2:
> - Use proper kerneldoc format [Rafael J. Wysocki]
> ---
> arch/x86/include/asm/smp.h| 2 +-
> arch/x86/kernel/smpboot.c | 26 --
> drivers/acpi/processor_idle.c | 4 +---
> 3 files changed, 14 insertions(+), 18 deletions(-)
>
> diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
> index 57ef2094af93..630ff08532be 100644
> --- a/arch/x86/include/asm/smp.h
> +++ b/arch/x86/include/asm/smp.h
> @@ -132,7 +132,7 @@ void native_play_dead(void);
> void play_dead_common(void);
> void wbinvd_on_cpu(int cpu);
> int wbinvd_on_all_cpus(void);
> -bool wakeup_cpu0(void);
> +void cond_wakeup_cpu0(void);
>
> void native_smp_send_reschedule(int cpu);
> void native_send_call_func_ipi(const struct cpumask *mask);
> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> index f877150a91da..16703c35a944 100644
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -1659,13 +1659,17 @@ void play_dead_common(void)
> local_irq_disable();
> }
>
> -bool wakeup_cpu0(void)
> +/**
> + * cond_wakeup_cpu0 - Wake up CPU0 if needed.
> + *
> + * If NMI wants to wake up CPU0, start CPU0.
> + */
> +void cond_wakeup_cpu0(void)
> {
> if (smp_processor_id() == 0 && enable_start_cpu0)
> - return true;
> -
> - return false;
> + start_cpu0();
> }
> +EXPORT_SYMBOL_GPL(cond_wakeup_cpu0);
>
> /*
> * We need to flush the caches before going to sleep, lest we have
> @@ -1734,11 +1738,8 @@ static inline void mwait_play_dead(void)
> __monitor(mwait_ptr, 0, 0);
> mb();
> __mwait(eax, 0);
> - /*
> -* If NMI wants to wake up CPU0, start CPU0.
> -*/
> - if (wakeup_cpu0())
> - start_cpu0();
> +
> + cond_wakeup_cpu0();
> }
> }
>
> @@ -1749,11 +1750,8 @@ void hlt_play_dead(void)
>
> while (1) {
> native_halt();
> - /*
> -* If NMI wants to wake up CPU0, start CPU0.
> -*/
> - if (wakeup_cpu0())
> - start_cpu0();
> +
> + cond_wakeup_cpu0();
> }
> }
>
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index 768a6b4d2368..4e2d76b8b697 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -544,9 +544,7 @@ static int acpi_idle_play_dead(struct cpuidle_device
> *dev, int index)
> return -ENODEV;
>
> #if defined(CONFIG_X86) && defined(CONFIG_HOTPLUG_CPU)
> - /* If NMI wants to wake up CPU0, start CPU0. */
> - if (wakeup_cpu0())
> - start_cpu0();
> + cond_wakeup_cpu0();
> #endif
> }
>
> --
> 2.30.2
>
[PATCH v1 0/5] ACPI: scan: acpi_bus_check_add() simplifications
Hi, This series simplifies acpi_bus_check_add() and related code. It mostly is not expected to alter functionality, except for patch [4/5] that unifies the handling of device and processor objects. Please refer to the patch changelogs for details. Thanks!
[PATCH v1 1/5] ACPI: scan: Fold acpi_bus_type_and_status() into its caller
From: Rafael J. Wysocki
There is only one caller of acpi_bus_type_and_status() which is
acpi_bus_check_add(), so fold the former into the latter and use
the observation that the initial status of the device is
ACPI_STA_DEFAULT in all cases except for ACPI_BUS_TYPE_PROCESSOR
to simplify the code.
No intentional functional impact.
Signed-off-by: Rafael J. Wysocki
---
drivers/acpi/scan.c | 80
1 file changed, 32 insertions(+), 48 deletions(-)
Index: linux-pm/drivers/acpi/scan.c
===
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -1763,50 +1763,6 @@ static bool acpi_device_should_be_hidden
return true;
}
-static int acpi_bus_type_and_status(acpi_handle handle, int *type,
- unsigned long long *sta)
-{
- acpi_status status;
- acpi_object_type acpi_type;
-
- status = acpi_get_type(handle, _type);
- if (ACPI_FAILURE(status))
- return -ENODEV;
-
- switch (acpi_type) {
- case ACPI_TYPE_ANY: /* for ACPI_ROOT_OBJECT */
- case ACPI_TYPE_DEVICE:
- if (acpi_device_should_be_hidden(handle))
- return -ENODEV;
-
- *type = ACPI_BUS_TYPE_DEVICE;
- /*
-* acpi_add_single_object updates this once we've an acpi_device
-* so that acpi_bus_get_status' quirk handling can be used.
-*/
- *sta = ACPI_STA_DEFAULT;
- break;
- case ACPI_TYPE_PROCESSOR:
- *type = ACPI_BUS_TYPE_PROCESSOR;
- status = acpi_bus_get_status_handle(handle, sta);
- if (ACPI_FAILURE(status))
- return -ENODEV;
- break;
- case ACPI_TYPE_THERMAL:
- *type = ACPI_BUS_TYPE_THERMAL;
- *sta = ACPI_STA_DEFAULT;
- break;
- case ACPI_TYPE_POWER:
- *type = ACPI_BUS_TYPE_POWER;
- *sta = ACPI_STA_DEFAULT;
- break;
- default:
- return -ENODEV;
- }
-
- return 0;
-}
-
bool acpi_device_is_present(const struct acpi_device *adev)
{
return adev->status.present || adev->status.functional;
@@ -1953,18 +1909,46 @@ static acpi_status acpi_bus_check_add(ac
struct acpi_device **adev_p)
{
struct acpi_device *device = NULL;
- unsigned long long sta;
+ unsigned long long sta = ACPI_STA_DEFAULT;
+ acpi_object_type acpi_type;
int type;
- int result;
acpi_bus_get_device(handle, );
if (device)
goto out;
- result = acpi_bus_type_and_status(handle, , );
- if (result)
+ if (ACPI_FAILURE(acpi_get_type(handle, _type)))
return AE_OK;
+ switch (acpi_type) {
+ case ACPI_TYPE_DEVICE:
+ if (acpi_device_should_be_hidden(handle))
+ return AE_OK;
+
+ fallthrough;
+ case ACPI_TYPE_ANY: /* for ACPI_ROOT_OBJECT */
+ type = ACPI_BUS_TYPE_DEVICE;
+ break;
+
+ case ACPI_TYPE_PROCESSOR:
+ if (ACPI_FAILURE(acpi_bus_get_status_handle(handle, )))
+ return AE_OK;
+
+ type = ACPI_BUS_TYPE_PROCESSOR;
+ break;
+
+ case ACPI_TYPE_THERMAL:
+ type = ACPI_BUS_TYPE_THERMAL;
+ break;
+
+ case ACPI_TYPE_POWER:
+ type = ACPI_BUS_TYPE_POWER;
+ break;
+
+ default:
+ return AE_OK;
+ }
+
if (type == ACPI_BUS_TYPE_POWER) {
acpi_add_power_resource(handle);
return AE_OK;
[PATCH v1 2/5] ACPI: scan: Rearrange checks in acpi_bus_check_add()
From: Rafael J. Wysocki
Rearrange the checks in acpi_bus_check_add() to avoid checking
the "type" twice and take "check_dep" into account only for
ACPI_TYPE_DEVICE objects.
No intentional functional impact.
Signed-off-by: Rafael J. Wysocki
---
drivers/acpi/scan.c | 30 +++---
1 file changed, 11 insertions(+), 19 deletions(-)
Index: linux-pm/drivers/acpi/scan.c
===
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -1831,7 +1831,7 @@ static void acpi_scan_init_hotplug(struc
}
}
-static u32 acpi_scan_check_dep(acpi_handle handle)
+static u32 acpi_scan_check_dep(acpi_handle handle, bool check_dep)
{
struct acpi_handle_list dep_devices;
acpi_status status;
@@ -1844,7 +1844,8 @@ static u32 acpi_scan_check_dep(acpi_hand
* 2. ACPI nodes describing USB ports.
* Still, checking for _HID catches more then just these cases ...
*/
- if (!acpi_has_method(handle, "_DEP") || !acpi_has_method(handle,
"_HID"))
+ if (!check_dep || !acpi_has_method(handle, "_DEP") ||
+ !acpi_has_method(handle, "_HID"))
return 0;
status = acpi_evaluate_reference(handle, "_DEP", NULL, _devices);
@@ -1925,6 +1926,12 @@ static acpi_status acpi_bus_check_add(ac
if (acpi_device_should_be_hidden(handle))
return AE_OK;
+ /* Bail out if there are dependencies. */
+ if (acpi_scan_check_dep(handle, check_dep) > 0) {
+ acpi_bus_scan_second_pass = true;
+ return AE_CTRL_DEPTH;
+ }
+
fallthrough;
case ACPI_TYPE_ANY: /* for ACPI_ROOT_OBJECT */
type = ACPI_BUS_TYPE_DEVICE;
@@ -1942,27 +1949,12 @@ static acpi_status acpi_bus_check_add(ac
break;
case ACPI_TYPE_POWER:
- type = ACPI_BUS_TYPE_POWER;
- break;
-
- default:
- return AE_OK;
- }
-
- if (type == ACPI_BUS_TYPE_POWER) {
acpi_add_power_resource(handle);
+ fallthrough;
+ default:
return AE_OK;
}
- if (type == ACPI_BUS_TYPE_DEVICE && check_dep) {
- u32 count = acpi_scan_check_dep(handle);
- /* Bail out if the number of recorded dependencies is not 0. */
- if (count > 0) {
- acpi_bus_scan_second_pass = true;
- return AE_CTRL_DEPTH;
- }
- }
-
acpi_add_single_object(, handle, type, sta);
if (!device)
return AE_CTRL_DEPTH;
[PATCH v1 3/5] ACPI: scan: Drop sta argument from acpi_add_single_object()
From: Rafael J. Wysocki
Move the initial status check for ACPI_BUS_TYPE_PROCESSOR objects
into acpi_add_single_object() so it is not necessary to pass the
"sta" argument to it, get rid of that argument from there and update
the callers of that function accordingly.
No intentional functional impact.
Signed-off-by: Rafael J. Wysocki
---
drivers/acpi/scan.c | 24 ++--
1 file changed, 10 insertions(+), 14 deletions(-)
Index: linux-pm/drivers/acpi/scan.c
===
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -1681,15 +1681,18 @@ void acpi_device_add_finalize(struct acp
}
static int acpi_add_single_object(struct acpi_device **child,
- acpi_handle handle, int type,
- unsigned long long sta)
+ acpi_handle handle, int type)
{
struct acpi_device_info *info = NULL;
+ unsigned long long sta = ACPI_STA_DEFAULT;
struct acpi_device *device;
int result;
- if (handle != ACPI_ROOT_OBJECT && type == ACPI_BUS_TYPE_DEVICE)
+ if (type == ACPI_BUS_TYPE_DEVICE && handle != ACPI_ROOT_OBJECT)
acpi_get_object_info(handle, );
+ else if (type == ACPI_BUS_TYPE_PROCESSOR &&
+ACPI_FAILURE(acpi_bus_get_status_handle(handle, )))
+ return -ENODEV;
device = kzalloc(sizeof(struct acpi_device), GFP_KERNEL);
if (!device) {
@@ -1910,7 +1913,6 @@ static acpi_status acpi_bus_check_add(ac
struct acpi_device **adev_p)
{
struct acpi_device *device = NULL;
- unsigned long long sta = ACPI_STA_DEFAULT;
acpi_object_type acpi_type;
int type;
@@ -1938,9 +1940,6 @@ static acpi_status acpi_bus_check_add(ac
break;
case ACPI_TYPE_PROCESSOR:
- if (ACPI_FAILURE(acpi_bus_get_status_handle(handle, )))
- return AE_OK;
-
type = ACPI_BUS_TYPE_PROCESSOR;
break;
@@ -1955,7 +1954,7 @@ static acpi_status acpi_bus_check_add(ac
return AE_OK;
}
- acpi_add_single_object(, handle, type, sta);
+ acpi_add_single_object(, handle, type);
if (!device)
return AE_CTRL_DEPTH;
@@ -2229,8 +2228,7 @@ int acpi_bus_register_early_device(int t
struct acpi_device *device = NULL;
int result;
- result = acpi_add_single_object(, NULL,
- type, ACPI_STA_DEFAULT);
+ result = acpi_add_single_object(, NULL, type);
if (result)
return result;
@@ -2250,8 +2248,7 @@ static int acpi_bus_scan_fixed(void)
struct acpi_device *device = NULL;
result = acpi_add_single_object(, NULL,
- ACPI_BUS_TYPE_POWER_BUTTON,
- ACPI_STA_DEFAULT);
+ ACPI_BUS_TYPE_POWER_BUTTON);
if (result)
return result;
@@ -2267,8 +2264,7 @@ static int acpi_bus_scan_fixed(void)
struct acpi_device *device = NULL;
result = acpi_add_single_object(, NULL,
- ACPI_BUS_TYPE_SLEEP_BUTTON,
- ACPI_STA_DEFAULT);
+ ACPI_BUS_TYPE_SLEEP_BUTTON);
if (result)
return result;
[PATCH v1 4/5] ACPI: scan: Drop sta argument from acpi_init_device_object()
From: Rafael J. Wysocki
Use the observation that the initial status check for
ACPI_BUS_TYPE_PROCESSOR objects can be carried out in the same way
as for ACPI_BUS_TYPE_DEVICE objects and it is not necessary to fail
acpi_add_single_object() if acpi_bus_get_status_handle() returns an
error for a processor (its status can be set to 0 instead) to
simplify acpi_add_single_object().
Accordingly, drop the "sta" argument from acpi_init_device_object()
as it can always set the initial status to ACPI_STA_DEFAULT and let
its caller correct that later on.
Signed-off-by: Rafael J. Wysocki
---
drivers/acpi/internal.h |3 +--
drivers/acpi/power.c|3 +--
drivers/acpi/scan.c | 28 ++--
3 files changed, 16 insertions(+), 18 deletions(-)
Index: linux-pm/drivers/acpi/scan.c
===
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -1649,15 +1649,14 @@ static bool acpi_device_enumeration_by_p
}
void acpi_init_device_object(struct acpi_device *device, acpi_handle handle,
-int type, unsigned long long sta,
-struct acpi_device_info *info)
+int type, struct acpi_device_info *info)
{
INIT_LIST_HEAD(>pnp.ids);
device->device_type = type;
device->handle = handle;
device->parent = acpi_bus_get_parent(handle);
fwnode_init(>fwnode, _device_fwnode_ops);
- acpi_set_device_status(device, sta);
+ acpi_set_device_status(device, ACPI_STA_DEFAULT);
acpi_device_get_busid(device);
acpi_set_pnp_ids(handle, >pnp, type, info);
acpi_init_properties(device);
@@ -1680,19 +1679,21 @@ void acpi_device_add_finalize(struct acp
kobject_uevent(>dev.kobj, KOBJ_ADD);
}
+static void acpi_scan_init_status(struct acpi_device *adev)
+{
+ if (acpi_bus_get_status(adev))
+ acpi_set_device_status(adev, 0);
+}
+
static int acpi_add_single_object(struct acpi_device **child,
acpi_handle handle, int type)
{
struct acpi_device_info *info = NULL;
- unsigned long long sta = ACPI_STA_DEFAULT;
struct acpi_device *device;
int result;
if (type == ACPI_BUS_TYPE_DEVICE && handle != ACPI_ROOT_OBJECT)
acpi_get_object_info(handle, );
- else if (type == ACPI_BUS_TYPE_PROCESSOR &&
-ACPI_FAILURE(acpi_bus_get_status_handle(handle, )))
- return -ENODEV;
device = kzalloc(sizeof(struct acpi_device), GFP_KERNEL);
if (!device) {
@@ -1700,16 +1701,15 @@ static int acpi_add_single_object(struct
return -ENOMEM;
}
- acpi_init_device_object(device, handle, type, sta, info);
+ acpi_init_device_object(device, handle, type, info);
kfree(info);
/*
-* For ACPI_BUS_TYPE_DEVICE getting the status is delayed till here so
-* that we can call acpi_bus_get_status() and use its quirk handling.
-* Note this must be done before the get power-/wakeup_dev-flags calls.
+* Getting the status is delayed till here so that we can call
+* acpi_bus_get_status() and use its quirk handling. Note that
+* this must be done before the get power-/wakeup_dev-flags calls.
*/
- if (type == ACPI_BUS_TYPE_DEVICE)
- if (acpi_bus_get_status(device) < 0)
- acpi_set_device_status(device, 0);
+ if (type == ACPI_BUS_TYPE_DEVICE || type == ACPI_BUS_TYPE_PROCESSOR)
+ acpi_scan_init_status(device);
acpi_bus_get_power_flags(device);
acpi_bus_get_wakeup_device_flags(device);
Index: linux-pm/drivers/acpi/internal.h
===
--- linux-pm.orig/drivers/acpi/internal.h
+++ linux-pm/drivers/acpi/internal.h
@@ -109,8 +109,7 @@ struct acpi_device_bus_id {
int acpi_device_add(struct acpi_device *device,
void (*release)(struct device *));
void acpi_init_device_object(struct acpi_device *device, acpi_handle handle,
-int type, unsigned long long sta,
-struct acpi_device_info *info);
+int type, struct acpi_device_info *info);
int acpi_device_setup_files(struct acpi_device *dev);
void acpi_device_remove_files(struct acpi_device *dev);
void acpi_device_add_finalize(struct acpi_device *device);
Index: linux-pm/drivers/acpi/power.c
===
--- linux-pm.orig/drivers/acpi/power.c
+++ linux-pm/drivers/acpi/power.c
@@ -925,8 +925,7 @@ int acpi_add_power_resource(acpi_handle
return -ENOMEM;
device = >device;
- acpi_init_device_object(device, handle, ACPI_BUS_TYPE_POWER,
-
[PATCH v1 5/5] ACPI: scan: Call acpi_get_object_info() from acpi_set_pnp_ids()
From: Rafael J. Wysocki
Notice that it is not necessary to call acpi_get_object_info() from
acpi_add_single_object() in order to pass the pointer returned by it
to acpi_init_device_object() and from there to acpi_set_pnp_ids().
It is more straightforward to call acpi_get_object_info() from
acpi_set_pnp_ids() and avoid unnecessary pointer passing, so change
the code accordingly.
No intentional functional impact.
Signed-off-by: Rafael J. Wysocki
---
drivers/acpi/internal.h |2 +-
drivers/acpi/power.c|2 +-
drivers/acpi/scan.c | 21 +
3 files changed, 11 insertions(+), 14 deletions(-)
Index: linux-pm/drivers/acpi/scan.c
===
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -1307,8 +1307,9 @@ static bool acpi_object_is_system_bus(ac
}
static void acpi_set_pnp_ids(acpi_handle handle, struct acpi_device_pnp *pnp,
- int device_type, struct acpi_device_info *info)
+int device_type)
{
+ struct acpi_device_info *info = NULL;
struct acpi_pnp_device_id_list *cid_list;
int i;
@@ -1319,6 +1320,7 @@ static void acpi_set_pnp_ids(acpi_handle
break;
}
+ acpi_get_object_info(handle, );
if (!info) {
pr_err(PREFIX "%s: Error reading device info\n",
__func__);
@@ -1344,6 +1346,8 @@ static void acpi_set_pnp_ids(acpi_handle
if (info->valid & ACPI_VALID_CLS)
acpi_add_id(pnp, info->class_code.string);
+ kfree(info);
+
/*
* Some devices don't reliably have _HIDs & _CIDs, so add
* synthetic HIDs to make sure drivers can find them.
@@ -1649,7 +1653,7 @@ static bool acpi_device_enumeration_by_p
}
void acpi_init_device_object(struct acpi_device *device, acpi_handle handle,
-int type, struct acpi_device_info *info)
+int type)
{
INIT_LIST_HEAD(>pnp.ids);
device->device_type = type;
@@ -1658,7 +1662,7 @@ void acpi_init_device_object(struct acpi
fwnode_init(>fwnode, _device_fwnode_ops);
acpi_set_device_status(device, ACPI_STA_DEFAULT);
acpi_device_get_busid(device);
- acpi_set_pnp_ids(handle, >pnp, type, info);
+ acpi_set_pnp_ids(handle, >pnp, type);
acpi_init_properties(device);
acpi_bus_get_flags(device);
device->flags.match_driver = false;
@@ -1688,21 +1692,14 @@ static void acpi_scan_init_status(struct
static int acpi_add_single_object(struct acpi_device **child,
acpi_handle handle, int type)
{
- struct acpi_device_info *info = NULL;
struct acpi_device *device;
int result;
- if (type == ACPI_BUS_TYPE_DEVICE && handle != ACPI_ROOT_OBJECT)
- acpi_get_object_info(handle, );
-
device = kzalloc(sizeof(struct acpi_device), GFP_KERNEL);
- if (!device) {
- kfree(info);
+ if (!device)
return -ENOMEM;
- }
- acpi_init_device_object(device, handle, type, info);
- kfree(info);
+ acpi_init_device_object(device, handle, type);
/*
* Getting the status is delayed till here so that we can call
* acpi_bus_get_status() and use its quirk handling. Note that
Index: linux-pm/drivers/acpi/internal.h
===
--- linux-pm.orig/drivers/acpi/internal.h
+++ linux-pm/drivers/acpi/internal.h
@@ -109,7 +109,7 @@ struct acpi_device_bus_id {
int acpi_device_add(struct acpi_device *device,
void (*release)(struct device *));
void acpi_init_device_object(struct acpi_device *device, acpi_handle handle,
-int type, struct acpi_device_info *info);
+int type);
int acpi_device_setup_files(struct acpi_device *dev);
void acpi_device_remove_files(struct acpi_device *dev);
void acpi_device_add_finalize(struct acpi_device *device);
Index: linux-pm/drivers/acpi/power.c
===
--- linux-pm.orig/drivers/acpi/power.c
+++ linux-pm/drivers/acpi/power.c
@@ -925,7 +925,7 @@ int acpi_add_power_resource(acpi_handle
return -ENOMEM;
device = >device;
- acpi_init_device_object(device, handle, ACPI_BUS_TYPE_POWER, NULL);
+ acpi_init_device_object(device, handle, ACPI_BUS_TYPE_POWER);
mutex_init(>resource_lock);
INIT_LIST_HEAD(>list_node);
INIT_LIST_HEAD(>dependents);
[PATCH] cpufreq: intel_pstate: Simplify intel_pstate_update_perf_limits()
From: Rafael J. Wysocki
Because pstate.max_freq is always equal to the product of
pstate.max_pstate and pstate.scaling and, analogously,
pstate.turbo_freq is always equal to the product of
pstate.turbo_pstate and pstate.scaling, the result of the
max_policy_perf computation in intel_pstate_update_perf_limits() is
always equal to the quotient of policy_max and pstate.scaling,
regardless of whether or not turbo is disabled. Analogously, the
result of min_policy_perf in intel_pstate_update_perf_limits() is
always equal to the quotient of policy_min and pstate.scaling.
Accordingly, intel_pstate_update_perf_limits() need not check
whether or not turbo is enabled at all and in order to compute
max_policy_perf and min_policy_perf it can always divide policy_max
and policy_min, respectively, by pstate.scaling. Make it do so.
While at it, move the definition and initialization of the
turbo_max local variable to the code branch using it.
No intentional functional impact.
Signed-off-by: Rafael J. Wysocki
---
On top of linux-next.
---
drivers/cpufreq/intel_pstate.c | 22 ++
1 file changed, 6 insertions(+), 16 deletions(-)
Index: linux-pm/drivers/cpufreq/intel_pstate.c
===
--- linux-pm.orig/drivers/cpufreq/intel_pstate.c
+++ linux-pm/drivers/cpufreq/intel_pstate.c
@@ -2195,9 +2195,8 @@ static void intel_pstate_update_perf_lim
unsigned int policy_min,
unsigned int policy_max)
{
+ int scaling = cpu->pstate.scaling;
int32_t max_policy_perf, min_policy_perf;
- int max_state, turbo_max;
- int max_freq;
/*
* HWP needs some special consideration, because HWP_REQUEST uses
@@ -2206,33 +2205,24 @@ static void intel_pstate_update_perf_lim
if (hwp_active)
intel_pstate_get_hwp_cap(cpu);
- if (global.no_turbo || global.turbo_disabled) {
- max_state = cpu->pstate.max_pstate;
- max_freq = cpu->pstate.max_freq;
- } else {
- max_state = cpu->pstate.turbo_pstate;
- max_freq = cpu->pstate.turbo_freq;
- }
-
- turbo_max = cpu->pstate.turbo_pstate;
-
- max_policy_perf = max_state * policy_max / max_freq;
+ max_policy_perf = policy_max / scaling;
if (policy_max == policy_min) {
min_policy_perf = max_policy_perf;
} else {
- min_policy_perf = max_state * policy_min / max_freq;
+ min_policy_perf = policy_min / scaling;
min_policy_perf = clamp_t(int32_t, min_policy_perf,
0, max_policy_perf);
}
- pr_debug("cpu:%d max_state %d min_policy_perf:%d max_policy_perf:%d\n",
-cpu->cpu, max_state, min_policy_perf, max_policy_perf);
+ pr_debug("cpu:%d min_policy_perf:%d max_policy_perf:%d\n",
+cpu->cpu, min_policy_perf, max_policy_perf);
/* Normalize user input to [min_perf, max_perf] */
if (per_cpu_limits) {
cpu->min_perf_ratio = min_policy_perf;
cpu->max_perf_ratio = max_policy_perf;
} else {
+ int turbo_max = cpu->pstate.turbo_pstate;
int32_t global_min, global_max;
/* Global limits are in percent of the maximum turbo P-state. */
Re: [PATCH] resource: Prevent irqresource_disabled() from erasing flags
On Wed, Apr 7, 2021 at 1:01 PM Angela Czubak wrote:
>
> On Tue, Mar 30, 2021 at 5:50 PM Mika Westerberg
> wrote:
> >
> > Hi,
> >
> > On Tue, Mar 30, 2021 at 05:09:42PM +0200, Rafael J. Wysocki wrote:
> > > On 3/29/2021 9:52 PM, Angela Czubak wrote:
> > > > Do not overwrite flags as it leads to erasing triggering and polarity
> > > > information which might be useful in case of hard-coded interrupts.
> > > > This way the information can be read later on even though mapping to
> > > > APIC domain failed.
> > > >
> > > > Signed-off-by: Angela Czubak
> > > > ---
> > > > Some Chromebooks use hard-coded interrupts in their ACPI tables.
> > > > This is an excerpt as dumped on Relm:
> > > >
> > > > ...
> > > > Name (_HID, "ELAN0001") // _HID: Hardware ID
> > > > Name (_DDN, "Elan Touchscreen ") // _DDN: DOS Device Name
> > > > Name (_UID, 0x05) // _UID: Unique ID
> > > > Name (ISTP, Zero)
> > > > Method (_CRS, 0, NotSerialized) // _CRS: Current Resource
> > > > Settings
> > > > {
> > > > Name (BUF0, ResourceTemplate ()
> > > > {
> > > > I2cSerialBusV2 (0x0010, ControllerInitiated,
> > > > 0x00061A80,
> > > > AddressingMode7Bit, "\\_SB.I2C1",
> > > > 0x00, ResourceConsumer, , Exclusive,
> > > > )
> > > > Interrupt (ResourceConsumer, Edge, ActiveLow,
> > > > Exclusive, ,, )
> > > > {
> > > > 0x00B8,
> > > > }
> > > > })
> > > > Return (BUF0) /* \_SB_.I2C1.ETSA._CRS.BUF0 */
> > > > }
> > > > ...
> > > >
> > > > This interrupt is hard-coded to 0xB8 = 184 which is too high to be
> > > > mapped
> > > > to IO-APIC, so no triggering information is propagated as
> > > > acpi_register_gsi()
> > > > fails and irqresource_disabled() is issued, which leads to erasing
> > > > triggering
> > > > and polarity information.
> > > > If that function added its flags instead of overwriting them the
> > > > correct IRQ
> > > > type would be set even for the hard-coded interrupts, which allows
> > > > device driver
> > > > to retrieve it.
> > > > Please, let me know if this kind of modification is acceptable.
> > >
> > > From the quick look it should not be problematic, but it needs to be
> > > checked
> > > more carefully.
> > >
> > > Mika, what do you think?
> >
> > I think it makes sense. We still set IORESOURCE_DISABLED unconditionally
> > so this should not cause issues. In theory at least :)
> >
> Is there anything else you would need me to do regarding the patch?
Yes, please resend it with a CC to linux-a...@vger.kernel.org for more
visibility.
> I suppose there are more platforms that could benefit from not erasing
> the flags, so if this patch is fit for upstream, can we continue the
> process?
>
> > > > include/linux/ioport.h | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/include/linux/ioport.h b/include/linux/ioport.h
> > > > index 55de385c839cf..647744d8514e0 100644
> > > > --- a/include/linux/ioport.h
> > > > +++ b/include/linux/ioport.h
> > > > @@ -331,7 +331,7 @@ static inline void irqresource_disabled(struct
> > > > resource *res, u32 irq)
> > > > {
> > > > res->start = irq;
> > > > res->end = irq;
> > > > - res->flags = IORESOURCE_IRQ | IORESOURCE_DISABLED |
> > > > IORESOURCE_UNSET;
> > > > + res->flags |= IORESOURCE_IRQ | IORESOURCE_DISABLED |
> > > > IORESOURCE_UNSET;
> > > > }
> > > > extern struct address_space *iomem_get_mapping(void);
> > >
Re: [bug report] Memory leak from acpi_ev_install_space_handler()
On Tue, Apr 6, 2021 at 5:51 PM John Garry wrote: > > Hi guys, > > On next-20210406, I enabled CONFIG_DEBUG_KMEMLEAK and > CONFIG_DEBUG_TEST_DRIVER_REMOVE for my arm64 system, and see this: Why exactly do you think that acpi_ev_install_space_handler() leaks memory? > root@debian:/home/john# more /sys/kernel/debug/kmemleak > unreferenced object 0x202803c11f00 (size 128): > comm "swapper/0", pid 1, jiffies 4294894325 (age 337.524s) > hex dump (first 32 bytes): > 00 00 00 00 02 00 00 00 08 1f c1 03 28 20 ff ff( .. > 08 1f c1 03 28 20 ff ff 00 00 00 00 00 00 00 00( .. > backtrace: > [<670a0938>] slab_post_alloc_hook+0x9c/0x2f8 > [] kmem_cache_alloc+0x198/0x2a8 > [<2bdba864>] acpi_os_create_semaphore+0x54/0xe0 > [ ] acpi_ev_install_space_handler+0x24c/0x300 > [<02e116e2>] acpi_install_address_space_handler+0x64/0xb0 > [ ] i2c_acpi_install_space_handler+0xd4/0x138 > [<8da42058>] i2c_register_adapter+0x368/0x910 > [ ] i2c_add_adapter+0x9c/0x100 > [<00ba2fcf>] i2c_add_numbered_adapter+0x44/0x58 > [<7df22d67>] i2c_dw_probe_master+0x68c/0x900 > [<682dfc98>] dw_i2c_plat_probe+0x460/0x640 > [ ] platform_probe+0x8c/0x108 > [ ] really_probe+0x190/0x670 > [<66017341>] driver_probe_device+0x8c/0xf8 > [ ] device_driver_attach+0x9c/0xa8 > [ ] __driver_attach+0x88/0x138 > unreferenced object 0x00280452c100 (size 128): > comm "swapper/0", pid 1, jiffies 4294894558 (age 336.604s) > hex dump (first 32 bytes): > 00 00 00 00 02 00 00 00 08 c1 52 04 28 00 ff ff..R.(... > 08 c1 52 04 28 00 ff ff 00 00 00 00 00 00 00 00..R.(... > backtrace: > [<670a0938>] slab_post_alloc_hook+0x9c/0x2f8 > [ ] kmem_cache_alloc+0x198/0x2a8 > [<2bdba864>] acpi_os_create_semaphore+0x54/0xe0 > [ ] acpi_ev_install_space_handler+0x24c/0x300 > [<02e116e2>] acpi_install_address_space_handler+0x64/0xb0 > [<988d4f61>] acpi_gpiochip_add+0x20c/0x4a0 > [<73d4faab>] gpiochip_add_data_with_key+0xd10/0x1680 > [<1d50b98a>] devm_gpiochip_add_data_with_key+0x30/0x78 > [ ] dwapb_gpio_probe+0x828/0xb28 > [ ] platform_probe+0x8c/0x108 > [ ] really_probe+0x190/0x670 > [<66017341>] driver_probe_device+0x8c/0xf8 > [ ] device_driver_attach+0x9c/0xa8 > [ ] __driver_attach+0x88/0x138 > [ ] bus_for_each_dev+0xec/0x160 > [ ] driver_attach+0x34/0x48 > root@debian:/home/john# > > Thanks, > John
Re: [PATCH v2] ACPI: processor: Fix build when CONFIG_ACPI_PROCESSOR=m
On Tue, Apr 6, 2021 at 5:39 PM Vitaly Kuznetsov wrote:
>
> "Rafael J. Wysocki" writes:
>
> > On Tue, Apr 6, 2021 at 4:01 PM Vitaly Kuznetsov wrote:
> >>
> >> Commit 8c182bd7 ("ACPI: processor: Fix CPU0 wakeup in
> >> acpi_idle_play_dead()") tried to fix CPU0 hotplug breakage by copying
> >> wakeup_cpu0() + start_cpu0() logic from hlt_play_dead()//mwait_play_dead()
> >> into acpi_idle_play_dead(). The problem is that these functions are not
> >> exported to modules so when CONFIG_ACPI_PROCESSOR=m build fails.
> >>
> >> The issue could've been fixed by exporting both wakeup_cpu0()/start_cpu0()
> >> (the later from assembly) but it seems putting the whole pattern into a
> >> new function and exporting it instead is better.
> >>
> >> Reported-by: kernel test robot
> >> Fixes: 8c182bd7 ("CPI: processor: Fix CPU0 wakeup in
> >> acpi_idle_play_dead()")
> >> Cc: # 5.10+
> >> Signed-off-by: Vitaly Kuznetsov
> >> ---
> >> Changes since v1:
> >> - Rename wakeup_cpu0() to cond_wakeup_cpu0() and fold wakeup_cpu0() in
> >> as it has no other users [Rafael J. Wysocki]
> >> ---
> >> arch/x86/include/asm/smp.h| 2 +-
> >> arch/x86/kernel/smpboot.c | 24 ++--
> >> drivers/acpi/processor_idle.c | 4 +---
> >> 3 files changed, 12 insertions(+), 18 deletions(-)
> >>
> >> diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
> >> index 57ef2094af93..630ff08532be 100644
> >> --- a/arch/x86/include/asm/smp.h
> >> +++ b/arch/x86/include/asm/smp.h
> >> @@ -132,7 +132,7 @@ void native_play_dead(void);
> >> void play_dead_common(void);
> >> void wbinvd_on_cpu(int cpu);
> >> int wbinvd_on_all_cpus(void);
> >> -bool wakeup_cpu0(void);
> >> +void cond_wakeup_cpu0(void);
> >>
> >> void native_smp_send_reschedule(int cpu);
> >> void native_send_call_func_ipi(const struct cpumask *mask);
> >> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> >> index f877150a91da..147f1bba9736 100644
> >> --- a/arch/x86/kernel/smpboot.c
> >> +++ b/arch/x86/kernel/smpboot.c
> >> @@ -1659,13 +1659,15 @@ void play_dead_common(void)
> >> local_irq_disable();
> >> }
> >>
> >> -bool wakeup_cpu0(void)
> >> +/*
> >> + * If NMI wants to wake up CPU0, start CPU0.
> >> + */
> >
> > Hasn't checkpatch.pl complained about this?
> >
>
> No, it didn't.
>
> > A proper kerneldoc would be something like:
> >
> > /**
> > * cond_wakeup_cpu0 - Wake up CPU0 if needed.
> > *
> > * If NMI wants to wake up CPU0, start CPU0.
> > */
>
> Yea, I didn't do that partly because of my laziness but partly because
> I don't see much usage of this format in arch/x86/kernel/[smpboot.c]. I
> can certainly do v3 if it's prefered.
Yes, please.
Exported functions generally need proper kerneldoc comments.
Re: [PATCH v2] ACPI: processor: Fix build when CONFIG_ACPI_PROCESSOR=m
On Tue, Apr 6, 2021 at 4:01 PM Vitaly Kuznetsov wrote:
>
> Commit 8c182bd7 ("ACPI: processor: Fix CPU0 wakeup in
> acpi_idle_play_dead()") tried to fix CPU0 hotplug breakage by copying
> wakeup_cpu0() + start_cpu0() logic from hlt_play_dead()//mwait_play_dead()
> into acpi_idle_play_dead(). The problem is that these functions are not
> exported to modules so when CONFIG_ACPI_PROCESSOR=m build fails.
>
> The issue could've been fixed by exporting both wakeup_cpu0()/start_cpu0()
> (the later from assembly) but it seems putting the whole pattern into a
> new function and exporting it instead is better.
>
> Reported-by: kernel test robot
> Fixes: 8c182bd7 ("CPI: processor: Fix CPU0 wakeup in
> acpi_idle_play_dead()")
> Cc: # 5.10+
> Signed-off-by: Vitaly Kuznetsov
> ---
> Changes since v1:
> - Rename wakeup_cpu0() to cond_wakeup_cpu0() and fold wakeup_cpu0() in
> as it has no other users [Rafael J. Wysocki]
> ---
> arch/x86/include/asm/smp.h| 2 +-
> arch/x86/kernel/smpboot.c | 24 ++--
> drivers/acpi/processor_idle.c | 4 +---
> 3 files changed, 12 insertions(+), 18 deletions(-)
>
> diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
> index 57ef2094af93..630ff08532be 100644
> --- a/arch/x86/include/asm/smp.h
> +++ b/arch/x86/include/asm/smp.h
> @@ -132,7 +132,7 @@ void native_play_dead(void);
> void play_dead_common(void);
> void wbinvd_on_cpu(int cpu);
> int wbinvd_on_all_cpus(void);
> -bool wakeup_cpu0(void);
> +void cond_wakeup_cpu0(void);
>
> void native_smp_send_reschedule(int cpu);
> void native_send_call_func_ipi(const struct cpumask *mask);
> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> index f877150a91da..147f1bba9736 100644
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -1659,13 +1659,15 @@ void play_dead_common(void)
> local_irq_disable();
> }
>
> -bool wakeup_cpu0(void)
> +/*
> + * If NMI wants to wake up CPU0, start CPU0.
> + */
Hasn't checkpatch.pl complained about this?
A proper kerneldoc would be something like:
/**
* cond_wakeup_cpu0 - Wake up CPU0 if needed.
*
* If NMI wants to wake up CPU0, start CPU0.
*/
> +void cond_wakeup_cpu0(void)
> {
> if (smp_processor_id() == 0 && enable_start_cpu0)
> - return true;
> -
> - return false;
> + start_cpu0();
> }
> +EXPORT_SYMBOL_GPL(cond_wakeup_cpu0);
>
> /*
> * We need to flush the caches before going to sleep, lest we have
> @@ -1734,11 +1736,8 @@ static inline void mwait_play_dead(void)
> __monitor(mwait_ptr, 0, 0);
> mb();
> __mwait(eax, 0);
> - /*
> -* If NMI wants to wake up CPU0, start CPU0.
> -*/
> - if (wakeup_cpu0())
> - start_cpu0();
> +
> + cond_wakeup_cpu0();
> }
> }
>
> @@ -1749,11 +1748,8 @@ void hlt_play_dead(void)
>
> while (1) {
> native_halt();
> - /*
> -* If NMI wants to wake up CPU0, start CPU0.
> -*/
> - if (wakeup_cpu0())
> - start_cpu0();
> +
> + cond_wakeup_cpu0();
> }
> }
>
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index 768a6b4d2368..4e2d76b8b697 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -544,9 +544,7 @@ static int acpi_idle_play_dead(struct cpuidle_device
> *dev, int index)
> return -ENODEV;
>
> #if defined(CONFIG_X86) && defined(CONFIG_HOTPLUG_CPU)
> - /* If NMI wants to wake up CPU0, start CPU0. */
> - if (wakeup_cpu0())
> - start_cpu0();
> + cond_wakeup_cpu0();
> #endif
> }
>
> --
> 2.30.2
>
Re: [PATCH] ACPI: processor: Fix build when CONFIG_ACPI_PROCESSOR=m
On Tue, Apr 6, 2021 at 2:50 PM Vitaly Kuznetsov wrote:
>
> Commit 8c182bd7 ("ACPI: processor: Fix CPU0 wakeup in
> acpi_idle_play_dead()") tried to fix CPU0 hotplug breakage by copying
> wakeup_cpu0() + start_cpu0() logic from hlt_play_dead()//mwait_play_dead()
> into acpi_idle_play_dead(). The problem is that these functions are not
> exported to modules so when CONFIG_ACPI_PROCESSOR=m build fails.
>
> The issue could've been fixed by exporting both wakeup_cpu0()/start_cpu0()
> (the later from assembly) but it seems putting the whole pattern into a
> new function and exporting it instead is better.
>
> Reported-by: kernel test robot
> Fixes: 8c182bd7 ("CPI: processor: Fix CPU0 wakeup in
> acpi_idle_play_dead()")
> Cc: # 5.10+
> Signed-off-by: Vitaly Kuznetsov
> ---
> arch/x86/include/asm/smp.h| 2 +-
> arch/x86/kernel/smpboot.c | 15 ++-
> drivers/acpi/processor_idle.c | 3 +--
> 3 files changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
> index 57ef2094af93..6f79deb1f970 100644
> --- a/arch/x86/include/asm/smp.h
> +++ b/arch/x86/include/asm/smp.h
> @@ -132,7 +132,7 @@ void native_play_dead(void);
> void play_dead_common(void);
> void wbinvd_on_cpu(int cpu);
> int wbinvd_on_all_cpus(void);
> -bool wakeup_cpu0(void);
> +void wakeup_cpu0_if_needed(void);
>
> void native_smp_send_reschedule(int cpu);
> void native_send_call_func_ipi(const struct cpumask *mask);
> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> index f877150a91da..9547d870ee27 100644
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -1659,7 +1659,7 @@ void play_dead_common(void)
> local_irq_disable();
> }
>
> -bool wakeup_cpu0(void)
> +static bool wakeup_cpu0(void)
> {
> if (smp_processor_id() == 0 && enable_start_cpu0)
> return true;
> @@ -1667,6 +1667,13 @@ bool wakeup_cpu0(void)
> return false;
> }
>
> +void wakeup_cpu0_if_needed(void)
> +{
> + if (wakeup_cpu0())
> + start_cpu0();
Note that all of the callers of wakeup_cpu0 do the above, so maybe
make them all use the new function?
In which case it can be rewritten in the following way
void cond_wakeup_cpu0(void)
{
if (smp_processor_id() == 0 && enable_start_cpu0)
start_cpu0();
}
EXPORT_SYMBOL_GPL(cond_wakeup_cpu0);
Also please add a proper kerneldoc comment to it and maybe drop the
comments at the call sites?
> +}
> +EXPORT_SYMBOL_GPL(wakeup_cpu0_if_needed);
> +
> /*
> * We need to flush the caches before going to sleep, lest we have
> * dirty data in our caches when we come back up.
> @@ -1737,8 +1744,7 @@ static inline void mwait_play_dead(void)
> /*
> * If NMI wants to wake up CPU0, start CPU0.
> */
> - if (wakeup_cpu0())
> - start_cpu0();
> + wakeup_cpu0_if_needed();
> }
> }
>
> @@ -1752,8 +1758,7 @@ void hlt_play_dead(void)
> /*
> * If NMI wants to wake up CPU0, start CPU0.
> */
> - if (wakeup_cpu0())
> - start_cpu0();
> + wakeup_cpu0_if_needed();
> }
> }
>
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index 768a6b4d2368..de15116b754a 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -545,8 +545,7 @@ static int acpi_idle_play_dead(struct cpuidle_device
> *dev, int index)
>
> #if defined(CONFIG_X86) && defined(CONFIG_HOTPLUG_CPU)
> /* If NMI wants to wake up CPU0, start CPU0. */
> - if (wakeup_cpu0())
> - start_cpu0();
> + wakeup_cpu0_if_needed();
> #endif
> }
>
> --
> 2.30.2
>
Re: linux-next: build warning after merge of the pm tree
On Wednesday, March 31, 2021 8:22:54 AM CEST Vitaly Kuznetsov wrote: > Stephen Rothwell writes: > > > Hi all, > > > > After merging the pm tree, today's linux-next build (x86_64 allmodconfig) > > produced this warning: > > > > drivers/acpi/processor_idle.c: In function 'acpi_idle_play_dead': > > drivers/acpi/processor_idle.c:542:15: warning: extra tokens at end of > > #ifdef directive > > 542 | #ifdef defined(CONFIG_X86) && defined(CONFIG_HOTPLUG_CPU) > > | ^ > > > > Sigh, smaller the patch, more iterations it will take to make it > right > > Rafael, > > something went wrong when you folded in my "[PATCH] ACPI: processor: Fix > build when !CONFIG_HOTPLUG_CPU" here. This line should be: > > #if defined(CONFIG_X86) && defined(CONFIG_HOTPLUG_CPU) Right, my mistake, sorry. Fixed yesterday. Thanks!
