Re: [PATCH v3 3/7] ocxl: Create a clear delineation between ocxl backend & frontend

[Frederic Barrat]

This is a huge patch, there are probably ways to split it in smaller 
pieces to make the review easier. However, considering how much time 
we've already invested discussing and reviewing it, it's with by me to 
keep it as is.
The ref-counting and device management look good to me now. A few 
details below.


> --- a/drivers/misc/ocxl/file.c
> +++ b/drivers/misc/ocxl/file.c
> @@ -17,12 +17,10 @@ static struct class *ocxl_class;
>   static struct mutex minors_idr_lock;
>   static struct idr minors_idr;
>   
> -static struct ocxl_afu *find_and_get_afu(dev_t devno)
> +static struct ocxl_file_info *find_file_info(dev_t devno)
>   {
> -	struct ocxl_afu *afu;
> -	int afu_minor;
> +	struct ocxl_file_info *info;
>   
> -	afu_minor = MINOR(devno);
>   	/*
>   	 * We don't declare an RCU critical section here, as our AFU
>   	 * is protected by a reference counter on the device. By the time the
> @@ -30,56 +28,52 @@ static struct ocxl_afu *find_and_get_afu(dev_t devno)
>   	 * the device is already at 0, so no user API will access that AFU and
>   	 * this function can't return it.
>   	 */


The comment is still true, but needs tuning. Something like:
"We don't declare an RCU critical section here, as our info structure is 
protected by a reference counter on the device. By the time the info 
reference is removed from the idr, the ref count of the device is 
already at 0, so no user API will access the corresponding AFU and this 
function can't return it."


> -	afu = idr_find(&minors_idr, afu_minor);
> -	if (afu)
> -		ocxl_afu_get(afu);
> -	return afu;
> +	info = idr_find(&minors_idr, MINOR(devno));
> +	return info;
>   }
>   
> -static int allocate_afu_minor(struct ocxl_afu *afu)
> +static int allocate_minor(struct ocxl_file_info *info)
>   {
>   	int minor;
>   
>   	mutex_lock(&minors_idr_lock);
> -	minor = idr_alloc(&minors_idr, afu, 0, OCXL_NUM_MINORS, GFP_KERNEL);
> +	minor = idr_alloc(&minors_idr, info, 0, OCXL_NUM_MINORS, GFP_KERNEL);
>   	mutex_unlock(&minors_idr_lock);
>   	return minor;
>   }
>   
> -static void free_afu_minor(struct ocxl_afu *afu)
> +static void free_minor(struct ocxl_file_info *info)
>   {
>   	mutex_lock(&minors_idr_lock);
> -	idr_remove(&minors_idr, MINOR(afu->dev.devt));
> +	idr_remove(&minors_idr, MINOR(info->dev.devt));
>   	mutex_unlock(&minors_idr_lock);
>   }
>   
>   static int afu_open(struct inode *inode, struct file *file)
>   {
> -	struct ocxl_afu *afu;
> +	struct ocxl_file_info *info;
>   	struct ocxl_context *ctx;
>   	int rc;
>   
>   	pr_debug("%s for device %x\n", __func__, inode->i_rdev);
>   
> -	afu = find_and_get_afu(inode->i_rdev);
> -	if (!afu)
> +	info = find_file_info(inode->i_rdev);
> +	if (!info)
>   		return -ENODEV;
>   
>   	ctx = ocxl_context_alloc();
>   	if (!ctx) {
>   		rc = -ENOMEM;
> -		goto put_afu;
> +		goto err;
>   	}
>   
> -	rc = ocxl_context_init(ctx, afu, inode->i_mapping);
> +	rc = ocxl_context_init(ctx, info->afu, inode->i_mapping);
>   	if (rc)
> -		goto put_afu;
> +		goto err;
>   	file->private_data = ctx;
> -	ocxl_afu_put(afu);
>   	return 0;
>   
> -put_afu:
> -	ocxl_afu_put(afu);
> +err:
>   	return rc;


The error path with goto is here useless. However, if 
ocxl_context_init() fails, the memory for the context is never released.
You may consider either getting rid of ocxl_context_alloc(), which is 
just a simple wrapper around kzalloc(), or merging the allocation in 
ocxl_context_init(). It would impact the external API, but having 2 
calls (alloc and init) feels like there's one too many.



> +static int ocxl_file_make_visible(struct ocxl_afu *afu)
>   {
>   	int rc;
> +	struct ocxl_file_info *info = ocxl_afu_get_private(afu);
>   
> -	cdev_init(&afu->cdev, &ocxl_afu_fops);
> -	rc = cdev_add(&afu->cdev, afu->dev.devt, 1);
> +	cdev_init(&info->cdev, &ocxl_afu_fops);
> +	rc = cdev_add(&info->cdev, info->dev.devt, 1);
>   	if (rc) {
> -		dev_err(&afu->dev, "Unable to add afu char device: %d\n", rc);
> +		dev_err(&info->dev, "Unable to add afu char device: %d\n", rc);
>   		return rc;
>   	}
> +
>   	return 0;
>   }
>   
> -void ocxl_destroy_cdev(struct ocxl_afu *afu)
> +void ocxl_file_make_invisible(struct ocxl_afu *afu)


This function is not called anywhere?



> -void ocxl_unregister_afu(struct ocxl_afu *afu)
> +void ocxl_file_unregister_afu(struct ocxl_afu *afu)
>   {
> -	free_afu_minor(afu);
> +	struct ocxl_file_info *info = ocxl_afu_get_private(afu);
> +
> +	if (!info)
> +		return;
> +


So that's likely where we miss the "make invisible" call. However, is it 
enough to rely on the private data to be set on the AFU?



> diff --git a/drivers/misc/ocxl/ocxl_internal.h b/drivers/misc/ocxl/ocxl_internal.h
> index 81086534dab5..e04e547df29e 100644
> --- a/drivers/misc/ocxl/ocxl_internal.h
> +++ b/drivers/misc/ocxl/ocxl_internal.h


> +/**
> + * Free an AFU
> + *
> + * afu: The AFU to free
> + */
> +void ocxl_free_afu(struct ocxl_afu *afu);


This is obsolete and should go away.

  Fred