Re: [PATCH v1 02/19] cxl: Add skeletal features driver

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

On Wed, 22 Jan 2025 16:50:33 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> Add the basic bits of a features driver to handle all CXL feature related
> services. The driver is expected to handle all CXL mailbox feature command
> related operations.
> 
> Suggested-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
I've tried not to duplicate Dan's feedback but might well
have done in places.

I have more or less completely forgotten earlier discussions
so may well repeat comments long addressed.

Jonathan


> diff --git a/drivers/cxl/core/features.c b/drivers/cxl/core/features.c
> new file mode 100644
> index 000000000000..eb6eb191a32e
> --- /dev/null
> +++ b/drivers/cxl/core/features.c
> @@ -0,0 +1,71 @@

> +struct cxl_features *cxl_features_alloc(struct cxl_mailbox *cxl_mbox,
> +					struct device *parent)
> +{
> +	struct device *dev;
> +	int rc;
> +
> +	struct cxl_features *features __free(kfree) =
> +		kzalloc(sizeof(*features), GFP_KERNEL);
> +	if (!features)
> +		return ERR_PTR(-ENOMEM);
> +
> +	rc = ida_alloc_max(&cxl_features_ida, CXL_FEATURE_MAX_DEVS - 1,
> +			   GFP_KERNEL);
> +	if (rc < 0)
> +		return ERR_PTR(rc);
> +
> +	features->id = rc;
> +	features->cxl_mbox = cxl_mbox;
> +	dev = &features->dev;
> +	device_initialize(dev);
> +	device_set_pm_not_required(dev);
> +	dev->parent = parent;
> +	dev->bus = &cxl_bus_type;
> +	dev->type = &cxl_features_type;
> +	rc = dev_set_name(dev, "features%d", features->id);
> +	if (rc)
> +		goto err;
> +
> +	rc = device_add(dev);
> +	if (rc)
> +		goto err;
> +
> +	rc = devm_add_action_or_reset(parent, remove_features_dev, dev);
> +	if (rc)
		return rc;

If this fails the or_reset() means it has called device_unregister()
so put_device() should not be needed I think (though I may have missed
a reference counter increase somewhere).

On top of the release freeing features that Dan called out.


> +		goto err;
> +
> +	return no_free_ptr(features);
> +
> +err:
> +	put_device(dev);
> +	return ERR_PTR(rc);
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_features_alloc, "CXL");

> diff --git a/drivers/cxl/features.c b/drivers/cxl/features.c
> new file mode 100644
> index 000000000000..644add26975f
> --- /dev/null
> +++ b/drivers/cxl/features.c
> @@ -0,0 +1,44 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Copyright(c) 2024,2025 Intel Corporation. All rights reserved. */
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <cxl/features.h>
> +
> +#include "cxl.h"
> +
> +static int cxl_features_probe(struct device *dev)
> +{
> +	struct cxl_features *features = to_cxl_features(dev);
> +	struct cxl_features_state *cfs __free(kfree) =
> +		kzalloc(sizeof(*cfs), GFP_KERNEL);

Maybe devm_ and no need for the scoped free handling
or for now the remove function?


> +
> +	if (!cfs)
> +		return -ENOMEM;
> +
> +	cfs->features = features;
> +	dev_set_drvdata(dev, no_free_ptr(cfs));
> +
> +	return 0;
> +}
> +
> +static void cxl_features_remove(struct device *dev)
> +{
> +	struct cxl_features_state *cfs = dev_get_drvdata(dev);
> +
> +	kfree(cfs);
> +}
> +
> +static struct cxl_driver cxl_features_driver = {
> +	.name = "cxl_features",
> +	.probe = cxl_features_probe,
> +	.remove = cxl_features_remove,
> +	.id = CXL_DEVICE_FEATURES,
> +};
> +
> +module_cxl_driver(cxl_features_driver);
> +
> +MODULE_DESCRIPTION("CXL: Features");
> +MODULE_LICENSE("GPL");
> +MODULE_IMPORT_NS("CXL");
> +MODULE_ALIAS_CXL(CXL_DEVICE_FEATURES);

> diff --git a/include/cxl/features.h b/include/cxl/features.h
> new file mode 100644
> index 000000000000..b92da1e92780
> --- /dev/null
> +++ b/include/cxl/features.h
> @@ -0,0 +1,23 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/* Copyright(c) 2024-2025 Intel Corporation. */
> +#ifndef __CXL_FEATURES_H__
> +#define __CXL_FEATURES_H__

#include <linux/device.h> for struct device definition.

> +
> +struct cxl_mailbox;
> +
> +struct cxl_features {
> +	int id;
> +	struct device dev;

Trivial thing but I'd put the device first as we tend
to end up with maths like to_cxl_features() that can be simpler
if it is there.


> +	struct cxl_mailbox *cxl_mbox;
> +};
> +#define to_cxl_features(dev) container_of(dev, struct cxl_features, dev)
> +
> +struct cxl_features_state {
> +	struct cxl_features *features;
> +	int num_features;
> +};
> +
> +struct cxl_features *cxl_features_alloc(struct cxl_mailbox *cxl_mbox,
> +					struct device *parent);
> +
> +#endif