Re: [RFC PATCH 3/6] cache: coherency device class

Greg KH
2025-03-20T21:15:34Z

On Thu, Mar 20, 2025 at 05:41:15PM +0000, Jonathan Cameron wrote:
> --- a/drivers/cache/Kconfig
> +++ b/drivers/cache/Kconfig
> @@ -1,6 +1,12 @@
> # SPDX-License-Identifier: GPL-2.0
> menu "Cache Drivers"
>
> +config CACHE_COHERENCY_CLASS
> + bool "Cache coherency control class"

Why can't this be a module? And why would anyone want to turn it off?

> + help
> + Class to which coherency control drivers register allowing core kernel
> + subsystems to issue invalidations and similar coherency operations.

What "core kernel subsystems"?

> +
> config AX45MP_L2_CACHE
> bool "Andes Technology AX45MP L2 Cache controller"
> depends on RISCV

Shouldn't all of these now depend on CACHE_COHERENCY_CLASS?

> diff --git a/drivers/cache/Makefile b/drivers/cache/Makefile
> index 55c5e851034d..b72b20f4248f 100644
> --- a/drivers/cache/Makefile
> +++ b/drivers/cache/Makefile
> @@ -3,3 +3,5 @@
> obj-$(CONFIG_AX45MP_L2_CACHE) += ax45mp_cache.o
> obj-$(CONFIG_SIFIVE_CCACHE) += sifive_ccache.o
> obj-$(CONFIG_STARFIVE_STARLINK_CACHE) += starfive_starlink_cache.o
> +
> +obj-$(CONFIG_CACHE_COHERENCY_CLASS) += coherency_core.o

Why the blank line?

> diff --git a/drivers/cache/coherency_core.c b/drivers/cache/coherency_core.c
> new file mode 100644
> index 000000000000..52cb4ceae00c
> --- /dev/null
> +++ b/drivers/cache/coherency_core.c
> @@ -0,0 +1,130 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Class to manage OS controlled coherency agents within the system.
> + * Specifically to enable operations such as write back and invalidate.
> + *
> + * Copyright: Huawei 2025
> + * Some elements based on fwctl class as an example of a modern
> + * lightweight class.
> + */
> +
> +#include <linux/cache_coherency.h>
> +#include <linux/container_of.h>
> +#include <linux/idr.h>
> +#include <linux/fs.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +
> +#include <asm/cacheflush.h>
> +
> +static DEFINE_IDA(cache_coherency_ida);
> +
> +static void cache_coherency_device_release(struct device *device)
> +{
> + struct cache_coherency_device *ccd =
> + container_of(device, struct cache_coherency_device, dev);
> +
> + ida_free(&cache_coherency_ida, ccd->id);
> +}
> +
> +static struct class cache_coherency_class = {
> + .name = "cache_coherency",
> + .dev_release = cache_coherency_device_release,
> +};
> +
> +static int cache_inval_one(struct device *dev, void *data)
> +{
> + struct cache_coherency_device *ccd =
> + container_of(dev, struct cache_coherency_device, dev);
> +
> + if (!ccd->ops)
> + return -EINVAL;
> +
> + return ccd->ops->wbinv(ccd, data);
> +}
> +
> +static int cache_inval_done_one(struct device *dev, void *data)
> +{
> + struct cache_coherency_device *ccd =
> + container_of(dev, struct cache_coherency_device, dev);
> + if (!ccd->ops)
> + return -EINVAL;
> +
> + return ccd->ops->done(ccd);
> +}
> +
> +static int cache_invalidate_memregion(int res_desc,
> + phys_addr_t addr, size_t size)
> +{
> + int ret;
> + struct cc_inval_params params = {
> + .addr = addr,
> + .size = size,
> + };
> +
> + ret = class_for_each_device(&cache_coherency_class, NULL, &params,
> + cache_inval_one);
> + if (ret)
> + return ret;
> +
> + return class_for_each_device(&cache_coherency_class, NULL, NULL,
> + cache_inval_done_one);
> +}
> +
> +static const struct system_cache_flush_method cache_flush_method = {
> + .invalidate_memregion = cache_invalidate_memregion,
> +};
> +
> +struct cache_coherency_device *
> +_cache_coherency_alloc_device(struct device *parent,
> + const struct coherency_ops *ops, size_t size)
> +{
> +
> + if (!ops || !ops->wbinv)
> + return NULL;
> +
> + struct cache_coherency_device *ccd __free(kfree) = kzalloc(size, GFP_KERNEL);
> +
> + if (!ccd)
> + return NULL;
> +
> + ccd->dev.class = &cache_coherency_class;
> + ccd->dev.parent = parent;
> + ccd->ops = ops;
> + ccd->id = ida_alloc(&cache_coherency_ida, GFP_KERNEL);
> +
> + if (dev_set_name(&ccd->dev, "cache_coherency%d", ccd->id))
> + return NULL;
> +
> + device_initialize(&ccd->dev);
> +
> + return_ptr(ccd);
> +}
> +EXPORT_SYMBOL_NS_GPL(_cache_coherency_alloc_device, "CACHE_COHERENCY");
> +
> +int cache_coherency_device_register(struct cache_coherency_device *ccd)
> +{
> + return device_add(&ccd->dev);
> +}
> +EXPORT_SYMBOL_NS_GPL(cache_coherency_device_register, "CACHE_COHERENCY");
> +
> +void cache_coherency_device_unregister(struct cache_coherency_device *ccd)
> +{
> + device_del(&ccd->dev);
> +}
> +EXPORT_SYMBOL_NS_GPL(cache_coherency_device_unregister, "CACHE_COHERENCY");
> +
> +static int __init cache_coherency_init(void)
> +{
> + int ret;
> +
> + ret = class_register(&cache_coherency_class);
> + if (ret)
> + return ret;
> +
> + //TODO: generalize
> + arm64_set_sys_cache_flush_method(&cache_flush_method);

I'm guessing this will blow up the build on non-x86 builds :)

> +struct cache_coherency_device {
> + struct device dev;
> + const struct coherency_ops *ops;
> + int id;
> +};

Classes are normally for user/kernel apis, what is this going to be used
for? I don't see any new user/kernel apis happening, so why do you need
a struct device to be created?

thanks,

greg k-h