Re: [PATCH 4/6] cxl/mem: Convert devm_cxl_add_memdev() to scope-based-cleanup

[Cheatham, Benjamin]

[snip]

> +}
> +
> +DEFINE_FREE(put_cxlmd, struct cxl_memdev *,
> +	    if (!IS_ERR_OR_NULL(_T)) put_device(&_T->dev);)
> +
>  /*
>   * Core helper for devm_cxl_add_memdev() that wants to both create a device and
>   * assert to the caller that upon return cxl_mem::probe() has been invoked.
> @@ -1057,45 +1084,28 @@ static const struct file_operations cxl_memdev_fops = {
>  struct cxl_memdev *__devm_cxl_add_memdev(struct device *host,
>  					 struct cxl_dev_state *cxlds)
>  {
> -	struct cxl_memdev *cxlmd;
>  	struct device *dev;
> -	struct cdev *cdev;
>  	int rc;
>  
> -	cxlmd = cxl_memdev_alloc(cxlds, &cxl_memdev_fops);
> +	struct cxl_memdev *cxlmd __free(put_cxlmd) =
> +		cxl_memdev_alloc(cxlds, &cxl_memdev_fops);
>  	if (IS_ERR(cxlmd))
>  		return cxlmd;
>  
>  	dev = &cxlmd->dev;
>  	rc = dev_set_name(dev, "mem%d", cxlmd->id);
>  	if (rc)
> -		goto err;
> -
> -	/*
> -	 * Activate ioctl operations, no cxl_memdev_rwsem manipulation
> -	 * needed as this is ordered with cdev_add() publishing the device.
> -	 */
> -	cxlmd->cxlds = cxlds;
> -	cxlds->cxlmd = cxlmd;
> +		return ERR_PTR(rc);
>  
> -	cdev = &cxlmd->cdev;
> -	rc = cdev_device_add(cdev, dev);
> +	rc = cxlmd_add(cxlmd, cxlds);
>  	if (rc)
> -		goto err;
> +		return ERR_PTR(rc);
>  
> -	rc = devm_add_action_or_reset(host, cxl_memdev_unregister, cxlmd);
> +	rc = devm_add_action_or_reset(host, cxl_memdev_unregister,
> +				      no_free_ptr(cxlmd));
>  	if (rc)
>  		return ERR_PTR(rc);
>  	return cxlmd;

Isn't cxlmd zeroed out by no_free_ptr() above? I think what needs to happen here is:

	rc = devm_add_action_or_reset(host, cxl_memdev_unregister, cxlmd);
	if (rc) {
		no_free_ptr(cxlmd);
		return ERR_PTR(rc);
	}

	return_ptr(cxlmd);

Looking ahead, this gets nullified in patch 6/6 so I guess it's only an issue if
someone is in the middle of a bisect (or doesn't pick up patch 6/6 for some reason).

> -
> -err:
> -	/*
> -	 * The cdev was briefly live, shutdown any ioctl operations that
> -	 * saw that state.
> -	 */
> -	cxl_memdev_shutdown(dev);
> -	put_device(dev);
> -	return ERR_PTR(rc);
>  }
>  EXPORT_SYMBOL_FOR_MODULES(__devm_cxl_add_memdev, "cxl_mem");
>