Re: [PATCH 1/4] platform/nvidia: Introduce nvgrace-egm driver and enumerate EGM regions

[Jason Gunthorpe]

> +static int nvgrace_egm_open(struct inode *inode, struct file *file)
> +{
> +	return 0;
> +}
> +
> +static int nvgrace_egm_release(struct inode *inode, struct file *file)
> +{
> +	return 0;
> +}

You don't have anything here yet, but based on how this is working later you
should technically hold a device reference while the file is open to prevent
the file->private_data from becoming freed while the file is still open.

That is impossible for this particular design since the freeing only happens
on module unload which the open file directly prevents, but it is a nice thing
to have to be tidy.

> +static struct nvgrace_egm_dev *setup_egm_chardev(u64 egmphys, u64 egmlength,
> +						 u64 egmpxm)
> +{
> +	struct nvgrace_egm_dev *egm_chardev;
> +	unsigned int baseminor = MINOR(dev);
> +	int ret;
> +
> +	/*
> +	 * Only MAX_EGM_NODES minors from baseminor were reserved. Reject a PXM
> +	 * outside that window.
> +	 */
> +	if (egmpxm < baseminor || egmpxm - baseminor >= MAX_EGM_NODES) {
> +		pr_err("nvgrace-egm: EGM proximity domain %llu outside reserved minor window [%u, %u)\n",
> +		       egmpxm, baseminor, baseminor + MAX_EGM_NODES);
> +		goto create_err;
> +	}
> +
> +	egm_chardev = kzalloc_obj(*egm_chardev, GFP_KERNEL);
> +	if (!egm_chardev)
> +		goto create_err;
> +
> +	device_initialize(&egm_chardev->device);
> +
> +	/*
> +	 * Use the proximity domain number as the device minor number.
> +	 * So the EGM corresponding to node X would be /dev/egmX.
> +	 */
> +	egm_chardev->egmphys = egmphys;
> +	egm_chardev->egmlength = egmlength;
> +	egm_chardev->egmpxm = egmpxm;
> +	INIT_LIST_HEAD(&egm_chardev->gpus);
> +
> +	egm_chardev->device.devt = MKDEV(MAJOR(dev), egm_chardev->egmpxm);
> +	egm_chardev->device.class = class;
> +	egm_chardev->device.release = egm_chardev_release;
> +	cdev_init(&egm_chardev->cdev, &file_ops);
> +	egm_chardev->cdev.owner = THIS_MODULE;
> +
> +	ret = dev_set_name(&egm_chardev->device, "egm%llu", egm_chardev->egmpxm);
> +	if (ret)
> +		goto error_exit;
> +
> +	ret = cdev_device_add(&egm_chardev->cdev, &egm_chardev->device);
> +	if (ret)
> +		goto error_exit;

This use of cdev_device_add() looks broadly right to me.

Though I usually put the attrs on the device not the class, I guess the effect
is the same?

-- 
Jason