[PATCH] [v4] ieee802154: ca8210: Fix a potential UAF in ca8210_probe
If of_clk_add_provider() fails in ca8210_register_ext_clock(),
it calls clk_unregister() to release priv->clk and returns an
error. However, the caller ca8210_probe() then calls ca8210_remove(),
where priv->clk is freed again in ca8210_unregister_ext_clock(). In
this case, a use-after-free may happen in the second time we call
clk_unregister().
Fix this by removing the first clk_unregister(). Also, priv->clk could
be an error code on failure of clk_register_fixed_rate(). Use
IS_ERR_OR_NULL to catch this case in ca8210_unregister_ext_clock().
Fixes: ded845a781a5 ("ieee802154: Add CA8210 IEEE 802.15.4 device driver")
Signed-off-by: Dinghao Liu
---
Changelog:
v2: -Remove the first clk_unregister() instead of nulling priv->clk.
v3: -Simplify ca8210_register_ext_clock().
-Add a ';' after return in ca8210_unregister_ext_clock().
v4: -Remove an unused variable 'ret'.
---
drivers/net/ieee802154/ca8210.c | 17 +++--
1 file changed, 3 insertions(+), 14 deletions(-)
diff --git a/drivers/net/ieee802154/ca8210.c b/drivers/net/ieee802154/ca8210.c
index aebb19f1b3a4..4ec0dab38872 100644
--- a/drivers/net/ieee802154/ca8210.c
+++ b/drivers/net/ieee802154/ca8210.c
@@ -2740,7 +2740,6 @@ static int ca8210_register_ext_clock(struct spi_device
*spi)
struct device_node *np = spi->dev.of_node;
struct ca8210_priv *priv = spi_get_drvdata(spi);
struct ca8210_platform_data *pdata = spi->dev.platform_data;
- int ret = 0;
if (!np)
return -EFAULT;
@@ -2757,18 +2756,8 @@ static int ca8210_register_ext_clock(struct spi_device
*spi)
dev_crit(>dev, "Failed to register external clk\n");
return PTR_ERR(priv->clk);
}
- ret = of_clk_add_provider(np, of_clk_src_simple_get, priv->clk);
- if (ret) {
- clk_unregister(priv->clk);
- dev_crit(
- >dev,
- "Failed to register external clock as clock provider\n"
- );
- } else {
- dev_info(>dev, "External clock set as clock provider\n");
- }
- return ret;
+ return of_clk_add_provider(np, of_clk_src_simple_get, priv->clk);
}
/**
@@ -2780,8 +2769,8 @@ static void ca8210_unregister_ext_clock(struct spi_device
*spi)
{
struct ca8210_priv *priv = spi_get_drvdata(spi);
- if (!priv->clk)
- return
+ if (IS_ERR_OR_NULL(priv->clk))
+ return;
of_clk_del_provider(spi->dev.of_node);
clk_unregister(priv->clk);
--
2.17.1
Re: [PATCH v9 3/5] kprobes: kretprobe scalability improvement with objpool
On Tue, 5 Sep 2023 09:52:53 +0800
"wuqiang.matt" wrote:
> kretprobe is using freelist to manage return-instances, but freelist,
> as LIFO queue based on singly linked list, scales badly and reduces
> the overall throughput of kretprobed routines, especially for high
> contention scenarios.
>
> Here's a typical throughput test of sys_prctl (counts in 10 seconds,
> measured with perf stat -a -I 1 -e syscalls:sys_enter_prctl):
>
> OS: Debian 10 X86_64, Linux 6.5rc7 with freelist
> HW: XEON 8336C x 2, 64 cores/128 threads, DDR4 3200MT/s
>
> 1T 2T 4T 8T 16T 24T
> 24150045 29317964 15446741 12494489 18287272 18287272
> 32T 48T 64T 72T 96T 128T
> 16200682 1620068 11645677 11269858 10470118 9931051
>
> This patch introduces objpool to kretprobe and rethook, with orginal
> freelist replaced and brings near-linear scalability to kretprobed
> routines. Tests of kretprobe throughput show the biggest ratio as
> 166.7x of the original freelist. Here's the comparison:
>
> 1T 2T 4T 8T16T
> native: 41186213 82336866 164250978 328662645 658810299
> freelist: 24150045 29317964 15446741 12494489 18287272
> objpool:24663700 49410571 98544977 197725940 396294399
> 32T48T64T96T 128T
> native: 1330338351 1969957941 2512291791 1514690434 2671040914
> freelist: 16200682 13737658 11645677 104701189931051
> objpool:78673470 1177354156 1514690434 1604472348 1655086824
>
> Tests on 96-core ARM64 system output similarly, but with the biggest
> ratio up to 454.8x:
>
> OS: Debian 10 AARCH64, Linux 6.5rc7
> HW: Kunpeng-920 96 cores/2 sockets/4 NUMA nodes, DDR4 2933 MT/s
>
> 1T 2T 4T 8T16T
> native: . 30066096 13733813 126194076 257447289 505800181
> freelist: 16152090 11064397 1112406872157685663013
> objpool:13733813 27749031 56540679 112291770 223482778
> 24T32T48T64T96T
> native:763305277 1015925192 1521075123 2033009392 3021013752
> freelist:50158104602893376679233824782945292
> objpool: 334605663 448310646 675018951 903449904 1339693418
>
This looks good to me (and I have tested with updated objpool)
Acked-by: Masami Hiramatsu (Google)
Wuqiang, can you update the above number with the simplified
objpool? I got better number (always 80% of the native performance)
with 128 node/probe.
(*)
https://lore.kernel.org/all/20231003003923.eabc33bb3f4ffb8eac71f...@kernel.org/
Thank you,
> Signed-off-by: wuqiang.matt
> ---
> include/linux/kprobes.h | 11 ++---
> include/linux/rethook.h | 16 ++-
> kernel/kprobes.c| 93 +
> kernel/trace/fprobe.c | 32 ++
> kernel/trace/rethook.c | 90 ++-
> 5 files changed, 98 insertions(+), 144 deletions(-)
>
> diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
> index 85a64cb95d75..365eb092e9c4 100644
> --- a/include/linux/kprobes.h
> +++ b/include/linux/kprobes.h
> @@ -26,8 +26,7 @@
> #include
> #include
> #include
> -#include
> -#include
> +#include
> #include
> #include
>
> @@ -141,7 +140,7 @@ static inline bool kprobe_ftrace(struct kprobe *p)
> */
> struct kretprobe_holder {
> struct kretprobe*rp;
> - refcount_t ref;
> + struct objpool_head pool;
> };
>
> struct kretprobe {
> @@ -154,7 +153,6 @@ struct kretprobe {
> #ifdef CONFIG_KRETPROBE_ON_RETHOOK
> struct rethook *rh;
> #else
> - struct freelist_head freelist;
> struct kretprobe_holder *rph;
> #endif
> };
> @@ -165,10 +163,7 @@ struct kretprobe_instance {
> #ifdef CONFIG_KRETPROBE_ON_RETHOOK
> struct rethook_node node;
> #else
> - union {
> - struct freelist_node freelist;
> - struct rcu_head rcu;
> - };
> + struct rcu_head rcu;
> struct llist_node llist;
> struct kretprobe_holder *rph;
> kprobe_opcode_t *ret_addr;
> diff --git a/include/linux/rethook.h b/include/linux/rethook.h
> index 26b6f3c81a76..ce69b2b7bc35 100644
> --- a/include/linux/rethook.h
> +++ b/include/linux/rethook.h
> @@ -6,11 +6,10 @@
> #define _LINUX_RETHOOK_H
>
> #include
> -#include
> +#include
> #include
> #include
> #include
> -#include
>
> struct rethook_node;
>
> @@ -30,14 +29,12 @@ typedef void (*rethook_handler_t) (struct rethook_node *,
> void *, unsigned long,
> struct rethook {
> void*data;
> rethook_handler_t handler;
> - struct freelist_headpool;
> - refcount_t ref;
> + struct objpool_head pool;
> struct rcu_head rcu;
> };
>
> /**
> * struct rethook_node - The rethook shadow-stack entry node.
> - * @freelist: The freelist,
Re: [PATCH] [v3] ieee802154: ca8210: Fix a potential UAF in ca8210_probe
> Hello Dinghao, > > > On 01.10.23 09:19, kernel test robot wrote: > > Hi Dinghao, > > > > kernel test robot noticed the following build warnings: > > > > [auto build test WARNING on linus/master] > > [also build test WARNING on v6.6-rc3 next-20230929] > > [If your patch is applied to the wrong git tree, kindly drop us a note. > > And when submitting patch, we suggest to use '--base' as documented in > > https://git-scm.com/docs/git-format-patch#_base_tree_information] > > > > url: > > https://github.com/intel-lab-lkp/linux/commits/Dinghao-Liu/ieee802154-ca8210-Fix-a-potential-UAF-in-ca8210_probe/20231001-135130 > > base: linus/master > > patch link: > > https://lore.kernel.org/r/20231001054949.14624-1-dinghao.liu%40zju.edu.cn > > patch subject: [PATCH] [v3] ieee802154: ca8210: Fix a potential UAF in > > ca8210_probe > > config: m68k-allyesconfig > > (https://download.01.org/0day-ci/archive/20231001/202310011548.qyqmuodi-...@intel.com/config) > > compiler: m68k-linux-gcc (GCC) 13.2.0 > > reproduce (this is a W=1 build): > > (https://download.01.org/0day-ci/archive/20231001/202310011548.qyqmuodi-...@intel.com/reproduce) > > > > If you fix the issue in a separate patch/commit (i.e. not just a new > > version of > > the same patch/commit), kindly add following tags > > | Reported-by: kernel test robot > > | Closes: > > https://lore.kernel.org/oe-kbuild-all/202310011548.qyqmuodi-...@intel.com/ > > > > All warnings (new ones prefixed by >>): > > > > drivers/net/ieee802154/ca8210.c: In function > > 'ca8210_register_ext_clock': > >>> drivers/net/ieee802154/ca8210.c:2743:13: warning: unused variable 'ret' > >>> [-Wunused-variable] > > 2743 | int ret = 0; > > | ^~~ > > > > Please take care of this now unused variable after your re-factor. > With this fixed and send out as v4 I am happy to get this applied to the > wpan tree. I will resend the v4 patch soon, thanks! Regards, Dinghao
Re: [PATCH v5 1/2] mm/memory_hotplug: split memmap_on_memory requests across memblocks
On Fri, 2023-10-06 at 14:52 +0200, David Hildenbrand wrote:
> On 05.10.23 20:31, Vishal Verma wrote:
> >
<..>
> > @@ -2167,47 +2221,28 @@ static int __ref try_remove_memory(u64 start, u64
> > size)
> > if (rc)
> > return rc;
> >
> > + mem_hotplug_begin();
> > +
> > /*
> > - * We only support removing memory added with MHP_MEMMAP_ON_MEMORY
> > in
> > - * the same granularity it was added - a single memory block.
> > + * For memmap_on_memory, the altmaps could have been added on
> > + * a per-memblock basis. Loop through the entire range if so,
> > + * and remove each memblock and its altmap.
> > */
> > if (mhp_memmap_on_memory()) {
> > - rc = walk_memory_blocks(start, size, ,
> > test_has_altmap_cb);
> > - if (rc) {
> > - if (size != memory_block_size_bytes()) {
> > - pr_warn("Refuse to remove %#llx - %#llx,"
> > - "wrong granularity\n",
> > - start, start + size);
> > - return -EINVAL;
> > - }
> > - altmap = mem->altmap;
> > - /*
> > - * Mark altmap NULL so that we can add a debug
> > - * check on memblock free.
> > - */
> > - mem->altmap = NULL;
> > - }
> > + unsigned long memblock_size = memory_block_size_bytes();
> > + u64 cur_start;
> > +
> > + for (cur_start = start; cur_start < start + size;
> > + cur_start += memblock_size)
> > + remove_memory_block_and_altmap(nid, cur_start,
> > + memblock_size);
> > + } else {
> > + remove_memory_block_and_altmap(nid, start, size);
>
> Better call remove_memory_block_devices() and arch_remove_memory(start,
> size, altmap) here explicitly instead of using
> remove_memory_block_and_altmap() that really can only handle a single
> memory block with any inputs.
>
I'm not sure I follow. Even in the non memmap_on_memory case, we'd have
to walk_memory_blocks() to get to the memory_block->altmap, right?
Or is there a more direct way? If we have to walk_memory_blocks, what's
the advantage of calling those directly instead of calling the helper
created above?
Agreed with and fixed up all the other comments.
[PATCH] KVM: deprecate KVM_WERROR in favor of general WERROR
Setting WERROR for random subsystems make life really hard for subsystems which want to build-test their stuff with W=1. WERROR for the entire kernel now exists and can be used instead. W=1 people probably know how to deal with the global W=1 already, tracking all per-subsystem WERRORs is too much... Link: https://lore.kernel.org/all/0da9874b6e9fcbaaa5edeb345d7e2a7c859fc818.1696271334.git.thomas.lenda...@amd.com/ Signed-off-by: Jakub Kicinski --- Documentation/process/maintainer-kvm-x86.rst | 2 +- arch/x86/kvm/Kconfig | 14 -- arch/x86/kvm/Makefile| 1 - 3 files changed, 1 insertion(+), 16 deletions(-) diff --git a/Documentation/process/maintainer-kvm-x86.rst b/Documentation/process/maintainer-kvm-x86.rst index 9183bd449762..cd70c0351108 100644 --- a/Documentation/process/maintainer-kvm-x86.rst +++ b/Documentation/process/maintainer-kvm-x86.rst @@ -243,7 +243,7 @@ context and disambiguate the reference. Testing --- At a bare minimum, *all* patches in a series must build cleanly for KVM_INTEL=m -KVM_AMD=m, and KVM_WERROR=y. Building every possible combination of Kconfigs +KVM_AMD=m, and WERROR=y. Building every possible combination of Kconfigs isn't feasible, but the more the merrier. KVM_SMM, KVM_XEN, PROVE_LOCKING, and X86_64 are particularly interesting knobs to turn. diff --git a/arch/x86/kvm/Kconfig b/arch/x86/kvm/Kconfig index ed90f148140d..12929324ac3e 100644 --- a/arch/x86/kvm/Kconfig +++ b/arch/x86/kvm/Kconfig @@ -63,20 +63,6 @@ config KVM If unsure, say N. -config KVM_WERROR - bool "Compile KVM with -Werror" - # KASAN may cause the build to fail due to larger frames - default y if X86_64 && !KASAN - # We use the dependency on !COMPILE_TEST to not be enabled - # blindly in allmodconfig or allyesconfig configurations - depends on KVM - depends on (X86_64 && !KASAN) || !COMPILE_TEST - depends on EXPERT - help - Add -Werror to the build flags for KVM. - - If in doubt, say "N". - config KVM_INTEL tristate "KVM for Intel (and compatible) processors support" depends on KVM && IA32_FEAT_CTL diff --git a/arch/x86/kvm/Makefile b/arch/x86/kvm/Makefile index 80e3fe184d17..8e6afde7c680 100644 --- a/arch/x86/kvm/Makefile +++ b/arch/x86/kvm/Makefile @@ -1,7 +1,6 @@ # SPDX-License-Identifier: GPL-2.0 ccflags-y += -I $(srctree)/arch/x86/kvm -ccflags-$(CONFIG_KVM_WERROR) += -Werror ifeq ($(CONFIG_FRAME_POINTER),y) OBJECT_FILES_NON_STANDARD_vmenter.o := y -- 2.41.0
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 v2 3/6] ACPI: AC: Replace acpi_driver with platform_driver
On Fri, Oct 06, 2023 at 08:30:52PM +0300, Michal Wilczynski wrote:
> AC driver uses struct acpi_driver incorrectly to register itself. This
> is wrong as the instances of the ACPI devices are not meant to
> be literal devices, they're supposed to describe ACPI entry of a
> particular device.
>
> Use platform_driver instead of acpi_driver. In relevant places call
> platform devices instances pdev to make a distinction with ACPI
> devices instances.
>
> Drop unnecessary casts from acpi_bus_generate_netlink_event() and
> acpi_notifier_call_chain().
>
> Add a blank line to distinguish pdev API vs local ACPI notify function.
...
> struct acpi_ac {
> struct power_supply *charger;
> struct power_supply_desc charger_desc;
> - struct acpi_device *device;
> + struct device *dev;
> unsigned long long state;
> struct notifier_block battery_nb;
> };
When changing this, also makes sense just to check if the moving a member in
the data structure makes code shorter, but it's not a show stopper.
...
> - status = acpi_evaluate_integer(ac->device->handle, "_PSR", NULL,
> + status = acpi_evaluate_integer(ACPI_HANDLE(ac->dev), "_PSR", NULL,
> >state);
> if (ACPI_FAILURE(status)) {
> - acpi_handle_info(ac->device->handle,
> + acpi_handle_info(ACPI_HANDLE(ac->dev),
Can we call ACPI_HANDLE() only once and cache that in a local variable and use
in all places?
...
> - struct acpi_ac *ac = acpi_driver_data(device);
> + struct acpi_ac *ac = data;
> + struct acpi_device *device = ACPI_COMPANION(ac->dev);
>
> switch (event) {
> default:
> - acpi_handle_debug(device->handle, "Unsupported event [0x%x]\n",
> + acpi_handle_debug(ACPI_HANDLE(ac->dev), "Unsupported event
> [0x%x]\n",
> event);
Does it makes any sense now? Basically it duplicates the ACPI_COMPANION() call
as Rafael pointed out in previous version discussion.
> fallthrough;
--
With Best Regards,
Andy Shevchenko
[PATCH v2 6/6] ACPI: NFIT: Remove redundant call to to_acpi_dev()
In acpi_nfit_ctl() ACPI handle is extracted using to_acpi_dev()
function and accessing handle field in ACPI device. After transformation
from ACPI driver to platform driver this is not optimal anymore. To get
a handle it's enough to just use ACPI_HANDLE() macro to extract the
handle.
Suggested-by: Andy Shevchenko
Signed-off-by: Michal Wilczynski
---
drivers/acpi/nfit/core.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index fb0bc16fa186..3d254b2cf2e7 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -475,8 +475,6 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc,
struct nvdimm *nvdimm,
guid = to_nfit_uuid(nfit_mem->family);
handle = adev->handle;
} else {
- struct acpi_device *adev = to_acpi_dev(acpi_desc);
-
cmd_name = nvdimm_bus_cmd_name(cmd);
cmd_mask = nd_desc->cmd_mask;
if (cmd == ND_CMD_CALL && call_pkg->nd_family) {
@@ -493,7 +491,7 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc,
struct nvdimm *nvdimm,
guid = to_nfit_uuid(NFIT_DEV_BUS);
}
desc = nd_cmd_bus_desc(cmd);
- handle = adev->handle;
+ handle = ACPI_HANDLE(dev);
dimm_name = "bus";
}
--
2.41.0
[PATCH v2 4/6] ACPI: AC: Rename ACPI device from device to adev
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 | 20 ++--
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/acpi/ac.c b/drivers/acpi/ac.c
index 298defeb5301..bb02e7f5d65a 100644
--- a/drivers/acpi/ac.c
+++ b/drivers/acpi/ac.c
@@ -120,7 +120,7 @@ static enum power_supply_property ac_props[] = {
static void acpi_ac_notify(acpi_handle handle, u32 event, void *data)
{
struct acpi_ac *ac = data;
- struct acpi_device *device = ACPI_COMPANION(ac->dev);
+ struct acpi_device *adev = ACPI_COMPANION(ac->dev);
switch (event) {
default:
@@ -141,11 +141,11 @@ static void acpi_ac_notify(acpi_handle handle, u32 event,
void *data)
msleep(ac_sleep_before_get_state_ms);
acpi_ac_get_state(ac);
- acpi_bus_generate_netlink_event(device->pnp.device_class,
+ acpi_bus_generate_netlink_event(adev->pnp.device_class,
dev_name(ac->dev),
event,
ac->state);
- acpi_notifier_call_chain(device, event, ac->state);
+ acpi_notifier_call_chain(adev, event, ac->state);
kobject_uevent(>charger->dev.kobj, KOBJ_CHANGE);
}
}
@@ -204,7 +204,7 @@ static const struct dmi_system_id ac_dmi_table[]
__initconst = {
static int acpi_ac_probe(struct platform_device *pdev)
{
- struct acpi_device *device = ACPI_COMPANION(>dev);
+ struct acpi_device *adev = ACPI_COMPANION(>dev);
struct power_supply_config psy_cfg = {};
struct acpi_ac *ac;
int result;
@@ -214,8 +214,8 @@ static int acpi_ac_probe(struct platform_device *pdev)
return -ENOMEM;
ac->dev = >dev;
- strcpy(acpi_device_name(device), ACPI_AC_DEVICE_NAME);
- strcpy(acpi_device_class(device), ACPI_AC_CLASS);
+ strcpy(acpi_device_name(adev), ACPI_AC_DEVICE_NAME);
+ strcpy(acpi_device_class(adev), ACPI_AC_CLASS);
platform_set_drvdata(pdev, ac);
@@ -225,7 +225,7 @@ static int acpi_ac_probe(struct platform_device *pdev)
psy_cfg.drv_data = ac;
- ac->charger_desc.name = acpi_device_bid(device);
+ ac->charger_desc.name = acpi_device_bid(adev);
ac->charger_desc.type = POWER_SUPPLY_TYPE_MAINS;
ac->charger_desc.properties = ac_props;
ac->charger_desc.num_properties = ARRAY_SIZE(ac_props);
@@ -237,13 +237,13 @@ static int acpi_ac_probe(struct platform_device *pdev)
goto err_release_ac;
}
- pr_info("%s [%s] (%s-line)\n", acpi_device_name(device),
- acpi_device_bid(device), str_on_off(ac->state));
+ pr_info("%s [%s] (%s-line)\n", acpi_device_name(adev),
+ acpi_device_bid(adev), str_on_off(ac->state));
ac->battery_nb.notifier_call = acpi_ac_battery_notify;
register_acpi_notifier(>battery_nb);
- result = acpi_dev_install_notify_handler(device, ACPI_ALL_NOTIFY,
+ result = acpi_dev_install_notify_handler(adev, ACPI_ALL_NOTIFY,
acpi_ac_notify, ac);
if (result)
goto err_unregister;
--
2.41.0
[PATCH v2 5/6] ACPI: NFIT: Replace acpi_driver with platform_driver
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);
}
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);
/* Save the acpi header for exporting the revision via sysfs */
acpi_desc->acpi_header = *tbl;
@@ -3391,7 +3393,7 @@ static int acpi_nfit_add(struct acpi_device *adev)
return rc;
rc = acpi_dev_install_notify_handler(adev, ACPI_DEVICE_NOTIFY,
-acpi_nfit_notify, adev);
+acpi_nfit_notify, dev);
if (rc)
return rc;
@@ -3475,11 +3477,11 @@ static const struct acpi_device_id acpi_nfit_ids[] = {
};
MODULE_DEVICE_TABLE(acpi, acpi_nfit_ids);
-static struct acpi_driver acpi_nfit_driver = {
- .name = KBUILD_MODNAME,
- .ids = acpi_nfit_ids,
- .ops = {
- .add = acpi_nfit_add,
+static struct platform_driver acpi_nfit_driver = {
+ .probe = acpi_nfit_probe,
+ .driver = {
+ .name = KBUILD_MODNAME,
+ .acpi_match_table = acpi_nfit_ids,
},
};
@@ -3517,7 +3519,7 @@ static __init int nfit_init(void)
return -ENOMEM;
nfit_mce_register();
- ret = acpi_bus_register_driver(_nfit_driver);
+ ret = platform_driver_register(_nfit_driver);
if (ret) {
nfit_mce_unregister();
destroy_workqueue(nfit_wq);
@@ -3530,7 +3532,7 @@ static __init int nfit_init(void)
static __exit void nfit_exit(void)
{
nfit_mce_unregister();
- acpi_bus_unregister_driver(_nfit_driver);
+ platform_driver_unregister(_nfit_driver);
destroy_workqueue(nfit_wq);
WARN_ON(!list_empty(_descs));
}
--
2.41.0
[PATCH v2 3/6] ACPI: AC: Replace acpi_driver with platform_driver
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;
unsigned long long state;
struct notifier_block battery_nb;
};
@@ -85,10 +75,10 @@ static int acpi_ac_get_state(struct acpi_ac *ac)
return 0;
}
- status = acpi_evaluate_integer(ac->device->handle, "_PSR", NULL,
+ status = acpi_evaluate_integer(ACPI_HANDLE(ac->dev), "_PSR", NULL,
>state);
if (ACPI_FAILURE(status)) {
- acpi_handle_info(ac->device->handle,
+ acpi_handle_info(ACPI_HANDLE(ac->dev),
"Error reading AC Adapter state: %s\n",
acpi_format_exception(status));
ac->state = ACPI_AC_STATUS_UNKNOWN;
@@ -129,12 +119,12 @@ static enum power_supply_property ac_props[] = {
/* Driver Model */
static void acpi_ac_notify(acpi_handle handle, u32 event, void *data)
{
- struct acpi_device *device = data;
- struct acpi_ac *ac = acpi_driver_data(device);
+ struct acpi_ac *ac = data;
+ struct acpi_device *device = ACPI_COMPANION(ac->dev);
switch (event) {
default:
- acpi_handle_debug(device->handle, "Unsupported event [0x%x]\n",
+ acpi_handle_debug(ACPI_HANDLE(ac->dev), "Unsupported event
[0x%x]\n",
event);
fallthrough;
case ACPI_AC_NOTIFY_STATUS:
@@ -152,9 +142,10 @@ static void acpi_ac_notify(acpi_handle handle, u32 event,
void *data)
acpi_ac_get_state(ac);
acpi_bus_generate_netlink_event(device->pnp.device_class,
- dev_name(>dev), event,
- (u32) ac->state);
- acpi_notifier_call_chain(device, event, (u32) ac->state);
+ dev_name(ac->dev),
+ event,
+ ac->state);
+ acpi_notifier_call_chain(device, event, ac->state);
kobject_uevent(>charger->dev.kobj, KOBJ_CHANGE);
}
}
@@ -211,8 +202,9 @@ static const struct dmi_system_id ac_dmi_table[]
__initconst = {
{},
};
-static int acpi_ac_add(struct acpi_device *device)
+static int acpi_ac_probe(struct platform_device *pdev)
{
+ struct acpi_device *device = ACPI_COMPANION(>dev);
struct power_supply_config psy_cfg = {};
struct acpi_ac *ac;
int result;
@@ -221,10 +213,11 @@ static int acpi_ac_add(struct acpi_device *device)
if (!ac)
return -ENOMEM;
- ac->device = device;
+ ac->dev = >dev;
strcpy(acpi_device_name(device), ACPI_AC_DEVICE_NAME);
strcpy(acpi_device_class(device), ACPI_AC_CLASS);
- device->driver_data = ac;
+
+ platform_set_drvdata(pdev, ac);
result = acpi_ac_get_state(ac);
if (result)
@@ -237,7 +230,7 @@ static int acpi_ac_add(struct acpi_device *device)
[PATCH v2 2/6] ACPI: AC: Use string_choices API instead of ternary operator
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);
--
2.41.0
[PATCH v2 1/6] ACPI: AC: Remove unnecessary checks
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);
--
2.41.0
[PATCH v2 0/6] Replace acpi_driver with platform_driver
Currently there is a number of drivers that somewhat incorrectly register themselves as acpi_driver, while they should be registering as platform_drivers. acpi_device was never meant to represent actual device, it only represents an entry in the ACPI table and ACPI driver should be treated as a driver for the ACPI entry of the particular device, not the device itself. Drivers of the devices itself should register as platform_drivers. Replace acpi_driver with platform_driver for all relevant drivers in drivers/acpi directory. This is just the first part of the change, as many acpi_drivers are present in many files outside of drivers/acpi directory. Additionally during internal review we've decided that it's best to send the relevant patches sequentially, in order not to overwhelm the reviewers. So I'm starting with NFIT and AC drivers. This change is possible thanks to recent .notify callback removal in drivers/acpi [1]. So flow for the remaining acpi_drivers will look like this: 1) Remove .notify callback with separate patchset for drivers/platform and other outstanding places like drivers/hwmon, drivers/iio etc. 2) Replace acpi_driver with platform_driver, as aside for .notify callback they're mostly functionally compatible. Most of the patches in this series could be considered independent, and could be merged separately, with a notable exception of the very first patch in this series that changes notify installer wrappers to be more generic. I decided it would be best not to force the user of this wrappers to pass any specific structure, like acpi_device or platform_device. It makes sense as it is very often the case that drivers declare their own private structure which gets allocated during the .probe() callback, and become the heart of the driver. In that case it makes much more sense to pass this structure to notify handler. Additionally some drivers, like acpi_video that already have custom notify handlers do that already. Link: https://lore.kernel.org/linux-acpi/20230703080252.2899090-1-michal.wilczyn...@intel.com/ [1] v2: - dropped first, second and last commit. Last commit was deemed pointless, first and second are already merged. - rebased on top of modified first commit (changes in the wrappers) Michal Wilczynski (6): ACPI: AC: Remove unnecessary checks ACPI: AC: Use string_choices API instead of ternary operator ACPI: AC: Replace acpi_driver with platform_driver ACPI: AC: Rename ACPI device from device to adev ACPI: NFIT: Replace acpi_driver with platform_driver ACPI: NFIT: Remove redundant call to to_acpi_dev() drivers/acpi/ac.c| 108 +-- drivers/acpi/nfit/core.c | 38 +++--- 2 files changed, 66 insertions(+), 80 deletions(-) -- 2.41.0
Re: [PATCH v1 2/9] docs: firmware-guide: ACPI: Clarify ACPI bus concepts
On 10/6/2023 5:36 PM, Rafael J. Wysocki wrote: > 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 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
Re: [PATCH rebased] kbuild: rpm-pkg: Fix build with non-default MODLIB
On Thu, Oct 05, 2023 at 05:07:28PM +0200, Michal Suchanek wrote:
> The default MODLIB value is composed of two variables and the hardcoded
> string '/lib/modules/'.
>
> MODLIB = $(INSTALL_MOD_PATH)/lib/modules/$(KERNELRELEASE)
>
> Defining this middle part as a variable was rejected on the basis that
> users can pass the whole MODLIB to make, such as
>
> make 'MODLIB=$(INSTALL_MOD_PATH)/usr/lib/modules/$(KERNELRELEASE)'
>
> However, this middle part of MODLIB is independently hardcoded by
> rpm-pkg, and when the user alters MODLIB this is not reflected when
> building the package.
>
> Given that $(INSTALL_MOD_PATH) is overridden during the rpm package build
> it is likely going to be empty. Then MODLIB can be passed to the rpm
> package, and used in place of the whole
> /usr/lib/modules/$(KERNELRELEASE) part.
>
> Signed-off-by: Michal Suchanek
This appears to work for me.
Reviewed-by: Nathan Chancellor
> ---
> scripts/package/kernel.spec | 8
> scripts/package/mkspec | 1 +
> 2 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/scripts/package/kernel.spec b/scripts/package/kernel.spec
> index 3eee0143e0c5..15f49c5077db 100644
> --- a/scripts/package/kernel.spec
> +++ b/scripts/package/kernel.spec
> @@ -67,7 +67,7 @@ cp $(%{make} %{makeflags} -s image_name)
> %{buildroot}/boot/vmlinuz-%{KERNELRELEA
> %{make} %{makeflags} INSTALL_HDR_PATH=%{buildroot}/usr headers_install
> cp System.map %{buildroot}/boot/System.map-%{KERNELRELEASE}
> cp .config %{buildroot}/boot/config-%{KERNELRELEASE}
> -ln -fns /usr/src/kernels/%{KERNELRELEASE}
> %{buildroot}/lib/modules/%{KERNELRELEASE}/build
> +ln -fns /usr/src/kernels/%{KERNELRELEASE} %{buildroot}%{MODLIB}/build
> %if %{with_devel}
> %{make} %{makeflags} run-command
> KBUILD_RUN_COMMAND='${srctree}/scripts/package/install-extmod-build
> %{buildroot}/usr/src/kernels/%{KERNELRELEASE}'
> %endif
> @@ -98,8 +98,8 @@ fi
>
> %files
> %defattr (-, root, root)
> -/lib/modules/%{KERNELRELEASE}
> -%exclude /lib/modules/%{KERNELRELEASE}/build
> +%{MODLIB}
> +%exclude %{MODLIB}/build
> /boot/*
>
> %files headers
> @@ -110,5 +110,5 @@ fi
> %files devel
> %defattr (-, root, root)
> /usr/src/kernels/%{KERNELRELEASE}
> -/lib/modules/%{KERNELRELEASE}/build
> +%{MODLIB}/build
> %endif
> diff --git a/scripts/package/mkspec b/scripts/package/mkspec
> index d41608efb747..d41b2e5304ac 100755
> --- a/scripts/package/mkspec
> +++ b/scripts/package/mkspec
> @@ -18,6 +18,7 @@ fi
> cat< %define ARCH ${ARCH}
> %define KERNELRELEASE ${KERNELRELEASE}
> +%define MODLIB ${MODLIB}
> %define pkg_release $("${srctree}/init/build-version")
> EOF
>
> --
> 2.42.0
>
Re: [PATCH v5 1/2] mm/memory_hotplug: split memmap_on_memory requests across memblocks
On Thu, 2023-10-05 at 14:20 -0700, Dan Williams wrote:
> Vishal Verma wrote:
<..>
> >
> > --- a/mm/memory_hotplug.c
> > +++ b/mm/memory_hotplug.c
> > @@ -1380,6 +1380,44 @@ static bool mhp_supports_memmap_on_memory(unsigned
> > long size)
> > return arch_supports_memmap_on_memory(vmemmap_size);
> > }
> >
> > +static int add_memory_create_devices(int nid, struct memory_group *group,
> > + u64 start, u64 size, mhp_t mhp_flags)
> > +{
> > + struct mhp_params params = { .pgprot = pgprot_mhp(PAGE_KERNEL) };
> > + struct vmem_altmap mhp_altmap = {
> > + .base_pfn = PHYS_PFN(start),
> > + .end_pfn = PHYS_PFN(start + size - 1),
> > + };
> > + int ret;
> > +
> > + if ((mhp_flags & MHP_MEMMAP_ON_MEMORY)) {
> > + mhp_altmap.free = memory_block_memmap_on_memory_pages();
> > + params.altmap = kmalloc(sizeof(struct vmem_altmap),
> > GFP_KERNEL);
> > + if (!params.altmap)
> > + return -ENOMEM;
> > +
> > + memcpy(params.altmap, _altmap, sizeof(mhp_altmap));
>
> Isn't this just open coded kmemdup()?
Ah yes - it was existing code that I just moved, but I can add a
precursor cleanup patch to change it.
>
> Other than that, I am not seeing anything else to comment on, you can add:
>
> Reviewed-by: Dan Williams
Thanks Dan!
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 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
[PATCH] testing: nvdimm: make struct class structures constant
Now that the driver core allows for struct class to be in read-only
memory, we should make all 'class' structures declared at build time
placing them into read-only memory, instead of having to be dynamically
allocated at runtime.
Cc: Dan Williams
Cc: Vishal Verma
Cc: Dave Jiang
Cc: Ira Weiny
Signed-off-by: Greg Kroah-Hartman
---
tools/testing/nvdimm/test/ndtest.c | 17 +
tools/testing/nvdimm/test/nfit.c | 14 +++---
2 files changed, 16 insertions(+), 15 deletions(-)
diff --git a/tools/testing/nvdimm/test/ndtest.c
b/tools/testing/nvdimm/test/ndtest.c
index 3eba10c1e3e8..fd26189d53be 100644
--- a/tools/testing/nvdimm/test/ndtest.c
+++ b/tools/testing/nvdimm/test/ndtest.c
@@ -38,7 +38,11 @@ enum {
static DEFINE_SPINLOCK(ndtest_lock);
static struct ndtest_priv *instances[NUM_INSTANCES];
-static struct class *ndtest_dimm_class;
+
+static const struct class ndtest_dimm_class = {
+ .name = "nfit_test_dimm",
+};
+
static struct gen_pool *ndtest_pool;
static struct ndtest_dimm dimm_group1[] = {
@@ -737,7 +741,7 @@ static int ndtest_dimm_register(struct ndtest_priv *priv,
return -ENXIO;
}
- dimm->dev = device_create_with_groups(ndtest_dimm_class,
+ dimm->dev = device_create_with_groups(_dimm_class,
>pdev.dev,
0, dimm, dimm_attribute_groups,
"test_dimm%d", id);
@@ -906,8 +910,7 @@ static void cleanup_devices(void)
gen_pool_destroy(ndtest_pool);
- if (ndtest_dimm_class)
- class_destroy(ndtest_dimm_class);
+ class_unregister(_dimm_class);
}
static __init int ndtest_init(void)
@@ -921,11 +924,9 @@ static __init int ndtest_init(void)
nfit_test_setup(ndtest_resource_lookup, NULL);
- ndtest_dimm_class = class_create("nfit_test_dimm");
- if (IS_ERR(ndtest_dimm_class)) {
- rc = PTR_ERR(ndtest_dimm_class);
+ rc = class_regster(_dimm_class);
+ if (rc)
goto err_register;
- }
ndtest_pool = gen_pool_create(ilog2(SZ_4M), NUMA_NO_NODE);
if (!ndtest_pool) {
diff --git a/tools/testing/nvdimm/test/nfit.c b/tools/testing/nvdimm/test/nfit.c
index 005043bd9623..a61df347a33d 100644
--- a/tools/testing/nvdimm/test/nfit.c
+++ b/tools/testing/nvdimm/test/nfit.c
@@ -1712,7 +1712,9 @@ static void put_dimms(void *data)
device_unregister(t->dimm_dev[i]);
}
-static struct class *nfit_test_dimm;
+static const struct class nfit_test_dimm = {
+ .name = "nfit_test_dimm",
+};
static int dimm_name_to_id(struct device *dev)
{
@@ -1830,7 +1832,7 @@ static int nfit_test_dimm_init(struct nfit_test *t)
if (devm_add_action_or_reset(>pdev.dev, put_dimms, t))
return -ENOMEM;
for (i = 0; i < t->num_dcr; i++) {
- t->dimm_dev[i] = device_create_with_groups(nfit_test_dimm,
+ t->dimm_dev[i] = device_create_with_groups(_test_dimm,
>pdev.dev, 0, NULL,
nfit_test_dimm_attribute_groups,
"test_dimm%d", i + t->dcr_idx);
@@ -3276,11 +3278,9 @@ static __init int nfit_test_init(void)
if (!nfit_wq)
return -ENOMEM;
- nfit_test_dimm = class_create("nfit_test_dimm");
- if (IS_ERR(nfit_test_dimm)) {
- rc = PTR_ERR(nfit_test_dimm);
+ rc = class_register(_test_dimm);
+ if (rc)
goto err_register;
- }
nfit_pool = gen_pool_create(ilog2(SZ_4M), NUMA_NO_NODE);
if (!nfit_pool) {
@@ -3377,7 +3377,7 @@ static __exit void nfit_test_exit(void)
for (i = 0; i < NUM_NFITS; i++)
put_device([i]->pdev.dev);
- class_destroy(nfit_test_dimm);
+ class_unregister(_test_dimm);
}
module_init(nfit_test_init);
--
2.42.0
Re: [PATCH v2 7/9] sched: define TIF_ALLOW_RESCHED
Hi Thomas,
On Tue, Sep 19, 2023 at 9:57 PM Thomas Gleixner wrote:
> Though it just occured to me that there are dragons lurking:
>
> arch/alpha/Kconfig: select ARCH_NO_PREEMPT
> arch/hexagon/Kconfig: select ARCH_NO_PREEMPT
> arch/m68k/Kconfig: select ARCH_NO_PREEMPT if !COLDFIRE
> arch/um/Kconfig:select ARCH_NO_PREEMPT
>
> So we have four architectures which refuse to enable preemption points,
> i.e. the only model they allow is NONE and they rely on cond_resched()
> for breaking large computations.
Looks like there is a fifth one hidden: although openrisc does not
select ARCH_NO_PREEMPT, it does not call preempt_schedule_irq() or
select GENERIC_ENTRY?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH v5 1/2] mm/memory_hotplug: split memmap_on_memory requests across memblocks
On 05.10.23 20:31, Vishal Verma wrote:
The MHP_MEMMAP_ON_MEMORY flag for hotplugged memory is restricted to
'memblock_size' chunks of memory being added. Adding a larger span of
memory precludes memmap_on_memory semantics.
For users of hotplug such as kmem, large amounts of memory might get
added from the CXL subsystem. In some cases, this amount may exceed the
available 'main memory' to store the memmap for the memory being added.
In this case, it is useful to have a way to place the memmap on the
memory being added, even if it means splitting the addition into
memblock-sized chunks.
Change add_memory_resource() to loop over memblock-sized chunks of
memory if caller requested memmap_on_memory, and if other conditions for
it are met. Teach try_remove_memory() to also expect that a memory
range being removed might have been split up into memblock sized chunks,
and to loop through those as needed.
Maybe add that this implies that we're not making use of PUD mappings in
the direct map yet, and link to the proposal on how we could optimize
that eventually in the future.
[...]
-static int __ref try_remove_memory(u64 start, u64 size)
+static void __ref remove_memory_block_and_altmap(int nid, u64 start, u64 size)
You shouldn't need the nid, right?
{
+ int rc = 0;
struct memory_block *mem;
- int rc = 0, nid = NUMA_NO_NODE;
struct vmem_altmap *altmap = NULL;
+ rc = walk_memory_blocks(start, size, , test_has_altmap_cb);
+ if (rc) {
+ altmap = mem->altmap;
+ /*
+* Mark altmap NULL so that we can add a debug
+* check on memblock free.
+*/
+ mem->altmap = NULL;
+ }
+
+ /*
+* Memory block device removal under the device_hotplug_lock is
+* a barrier against racing online attempts.
+*/
+ remove_memory_block_devices(start, size);
We're now calling that under the memory hotplug lock. I assume this is
fine, but I remember some ugly lockdep details ...should be alright I guess.
+
+ arch_remove_memory(start, size, altmap);
+
+ /* Verify that all vmemmap pages have actually been freed. */
+ if (altmap) {
+ WARN(altmap->alloc, "Altmap not fully unmapped");
+ kfree(altmap);
+ }
+}
+
+static int __ref try_remove_memory(u64 start, u64 size)
+{
+ int rc, nid = NUMA_NO_NODE;
+
BUG_ON(check_hotplug_memory_range(start, size));
/*
@@ -2167,47 +2221,28 @@ static int __ref try_remove_memory(u64 start, u64 size)
if (rc)
return rc;
+ mem_hotplug_begin();
+
/*
-* We only support removing memory added with MHP_MEMMAP_ON_MEMORY in
-* the same granularity it was added - a single memory block.
+* For memmap_on_memory, the altmaps could have been added on
+* a per-memblock basis. Loop through the entire range if so,
+* and remove each memblock and its altmap.
*/
if (mhp_memmap_on_memory()) {
- rc = walk_memory_blocks(start, size, , test_has_altmap_cb);
- if (rc) {
- if (size != memory_block_size_bytes()) {
- pr_warn("Refuse to remove %#llx - %#llx,"
- "wrong granularity\n",
- start, start + size);
- return -EINVAL;
- }
- altmap = mem->altmap;
- /*
-* Mark altmap NULL so that we can add a debug
-* check on memblock free.
-*/
- mem->altmap = NULL;
- }
+ unsigned long memblock_size = memory_block_size_bytes();
+ u64 cur_start;
+
+ for (cur_start = start; cur_start < start + size;
+cur_start += memblock_size)
+ remove_memory_block_and_altmap(nid, cur_start,
+ memblock_size);
+ } else {
+ remove_memory_block_and_altmap(nid, start, size);
Better call remove_memory_block_devices() and arch_remove_memory(start,
size, altmap) here explicitly instead of using
remove_memory_block_and_altmap() that really can only handle a single
memory block with any inputs.
}
/* remove memmap entry */
firmware_map_remove(start, start + size, "System RAM");
Can we continue doing that in the old order? (IOW before taking the lock?).
- /*
-* Memory block device removal under the device_hotplug_lock is
-* a barrier against racing online attempts.
-*/
- remove_memory_block_devices(start, size);
-
- mem_hotplug_begin();
-
- arch_remove_memory(start, size, altmap);
-
- /* Verify that all vmemmap pages have actually been
Re: [PATCH v4 1/2] mm/memory_hotplug: split memmap_on_memory requests across memblocks
On 03.10.23 22:03, Verma, Vishal L wrote:
On Mon, 2023-10-02 at 11:28 +0200, David Hildenbrand wrote:
+
+static int __ref try_remove_memory(u64 start, u64 size)
+{
+ int rc, nid = NUMA_NO_NODE;
+
+ BUG_ON(check_hotplug_memory_range(start, size));
+
+ /*
+ * All memory blocks must be offlined before removing memory. Check
+ * whether all memory blocks in question are offline and return error
+ * if this is not the case.
+ *
+ * While at it, determine the nid. Note that if we'd have mixed nodes,
+ * we'd only try to offline the last determined one -- which is good
+ * enough for the cases we care about.
+ */
+ rc = walk_memory_blocks(start, size, , check_memblock_offlined_cb);
+ if (rc)
+ return rc;
+
+ /*
+ * For memmap_on_memory, the altmaps could have been added on
+ * a per-memblock basis. Loop through the entire range if so,
+ * and remove each memblock and its altmap.
+ */
+ if (mhp_memmap_on_memory()) {
+ unsigned long memblock_size = memory_block_size_bytes();
+ u64 cur_start;
+
+ for (cur_start = start; cur_start < start + size;
+ cur_start += memblock_size)
+ __try_remove_memory(nid, cur_start, memblock_size);
+ } else {
+ __try_remove_memory(nid, start, size);
+ }
+
return 0;
}
Why is the firmware, memblock and nid handling not kept in this outer
function?
We really shouldn't be doing per memory block what needs to be done per
memblock: remove_memory_block_devices() and arch_remove_memory().
Ah yes makes sense since we only do create_memory_block_devices() and
arch_add_memory() in the per memory block inner loop during addition.
How should the locking work in this case though?
Sorry, I had to process a family NMI the last couple of days.
The original code holds the mem_hotplug_begin() lock over
arch_remove_memory() and all of the nid and memblock stuff. Should I
just hold the lock and release it in the inner loop for each memory
block, and then also acquire and release it separately for the memblock
and nid stuff in the outer function?
I think we have to hold it over the whole operation.
I saw that you sent a v5, I'll comment there.
--
Cheers,
David / dhildenb
Re: [PATCH v7 00/13] selftests/sgx: Fix compilation errors
Thank you, Kai! I'm not familiar with any next steps to get this merged
upstream, but atm all commits in this series have been reviewed by at
least Jarkko. Let me know if anything further is needed from my side!
Best,
Jo
On 05.10.23 23:25, Huang, Kai wrote:
Hi Jo,
Just FYI I won't review the rest patches in this series. One of the reasons is
I am not that familiar with the rest. Jarkko has reviewed anyway :-).
On Thu, 2023-10-05 at 17:38 +0200, Jo Van Bulck wrote:
Hi,
This patch series ensures that all SGX selftests succeed when compiling with
optimizations (as tested with -O{0,1,2,3,s} for both gcc 11.3.0 and clang
14.0.0). The aim of the patches is to avoid reliance on undefined,
compiler-specific behavior that can make the test results fragile.
As far as I see, all commits in this series now have an explicit reviewed-by
tag, so hopefully this can get merged upstream? Please let me know if any
concerns remain and I'd happily address them.
Reference output below:
.. Testing gcc -O0[OK]
.. Testing gcc -O1[OK]
.. Testing gcc -O2[OK]
.. Testing gcc -O3[OK]
.. Testing gcc -Os[OK]
.. Testing gcc -Ofast [OK]
.. Testing gcc -Og[OK]
.. Testing clang -O0[OK]
.. Testing clang -O1[OK]
.. Testing clang -O2[OK]
.. Testing clang -O3[OK]
.. Testing clang -Os[OK]
.. Testing clang -Ofast [OK]
.. Testing clang -Og[OK]
Changelog
-
v7
- Add reviewed-by tag (Jarkko)
v6
- Collect final ack/reviewed-by tags (Jarkko, Kai)
v5
- Reorder patches (Jarkko, Kai)
- Include fixes tag for inline asm memory clobber patch (Kai)
- Include linker error in static-pie commit message (Kai)
- Include generated assembly in relocations commit (Kai)
v4
- Remove redundant -nostartfiles compiler flag (Jarkko)
- Split dynamic symbol table removal in separate commit (Kai)
- Split redundant push/pop elimination in separate commit (Kai)
- Remove (incomplete) register cleansing on enclave exit
- Fix possibly uninitialized pointer dereferences in load.c
v3
- Refactor encl_op_array declaration and indexing (Jarkko)
- Annotate encl_buffer with "used" attribute (Kai)
- Split encl_buffer size and placement commits (Kai)
v2
- Add additional check for NULL pointer (Kai)
- Refine to produce proper static-pie executable
- Fix linker script assertions
- Specify memory clobber for inline asm instead of volatile (Kai)
- Clarify why encl_buffer non-static (Jarkko, Kai)
- Clarify -ffreestanding (Jarkko)
Best,
Jo
Jo Van Bulck (13):
selftests/sgx: Fix uninitialized pointer dereference in error path
selftests/sgx: Fix uninitialized pointer dereferences in
encl_get_entry
selftests/sgx: Include memory clobber for inline asm in test enclave
selftests/sgx: Separate linker options
selftests/sgx: Specify freestanding environment for enclave
compilation
selftests/sgx: Remove redundant enclave base address save/restore
selftests/sgx: Produce static-pie executable for test enclave
selftests/sgx: Handle relocations in test enclave
selftests/sgx: Fix linker script asserts
selftests/sgx: Ensure test enclave buffer is entirely preserved
selftests/sgx: Ensure expected location of test enclave buffer
selftests/sgx: Discard unsupported ELF sections
selftests/sgx: Remove incomplete ABI sanitization code in test enclave
tools/testing/selftests/sgx/Makefile | 12 ++--
tools/testing/selftests/sgx/defines.h | 2 +
tools/testing/selftests/sgx/load.c| 9 ++-
tools/testing/selftests/sgx/sigstruct.c | 5 +-
tools/testing/selftests/sgx/test_encl.c | 67 +--
tools/testing/selftests/sgx/test_encl.lds | 10 +--
.../selftests/sgx/test_encl_bootstrap.S | 28 +++-
7 files changed, 77 insertions(+), 56 deletions(-)
