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

[Greg KH <gregkh@linuxfoundation.org>]

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