Re: [PATCH v5 07/15] cxl: Add FWCTL support to CXL

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

On Wed, 12 Feb 2025 17:44:40 +0000
Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:

> On Tue, 11 Feb 2025 11:28:01 -0700
> Dave Jiang <dave.jiang@intel.com> wrote:
> 
> > Add fwctl support code to allow sending of CXL feature commands from
> > userspace through as ioctls via FWCTL. Provide initial setup bits. The
> > CXL PCI probe function will call cxl_setup_fwctl() after the cxl_memdev
> > has been enumerated in order to setup FWCTL char device under the
> > cxl_memdev like the existing memdev char device for issuing CXL raw
> > mailbox commands from userspace via ioctls.
> > 
> > Signed-off-by: Dave Jiang <dave.jiang@intel.com>  
> 
> A comment inline, but not a request to change anything for this
> series.  More a suspicion that the simplification Dan suggested
> will bite later :)
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> > diff --git a/drivers/cxl/core/features.c b/drivers/cxl/core/features.c
> > index 02558f46883d..db48a8fbde81 100644
> > --- a/drivers/cxl/core/features.c
> > +++ b/drivers/cxl/core/features.c
> > @@ -1,5 +1,6 @@  
> 
> > +
> > +int devm_cxl_setup_fwctl(struct cxl_memdev *cxlmd)  
> 
> I slightly wonder if we would be better passing
> struct device *parent, struct cxl_dev_state *cxlds.
> 
> However, that can happen if this gets reuse I guess
> rather than now.  We might have to put back a specific
> fwctl structure to enable that given we could know
> where to find cxlds in the structure containing the parent
> device though.
> 
> Meh, might be my problem if I try to reuse this for
> switch-cci which is my plan if I get time to do an RFC / PoC.

Actually no, that's garbage. I'll need to do a custom
fwctl device anyway even if it shares userspace interface with
this because it makes no real sense to have a cxlds either.

Definitely keep it as is!

Jonathan

> 
> 
> 
> > +{
> > +	struct cxl_dev_state *cxlds = cxlmd->cxlds;
> > +	struct cxl_features_state *cxlfs;
> > +	int rc;
> > +
> > +	cxlfs = to_cxlfs(cxlds);
> > +	if (!cxlfs)
> > +		return -ENODEV;
> > +
> > +	/* No need to setup FWCTL if there are no user allowed features found */
> > +	if (!cxlfs->entries->num_user_features)
> > +		return -ENODEV;
> > +
> > +	struct fwctl_device *fwctl_dev __free(free_fwctl_dev) =
> > +		_fwctl_alloc_device(&cxlmd->dev, &cxlctl_ops, sizeof(*fwctl_dev));
> > +	if (!fwctl_dev)
> > +		return -ENOMEM;
> > +
> > +	rc = fwctl_register(fwctl_dev);
> > +	if (rc)
> > +		return rc;
> > +
> > +	cxlfs->fwctl_dev = fwctl_dev;
> > +
> > +	return devm_add_action_or_reset(&cxlmd->dev, free_memdev_fwctl,
> > +					no_free_ptr(fwctl_dev));
> > +}
> > +EXPORT_SYMBOL_NS_GPL(devm_cxl_setup_fwctl, "CXL");
> > +
> > +MODULE_IMPORT_NS("FWCTL");  
>