RE: [PATCH V4 1/1] Drivers: hv: Implement the file copy service

[KY Srinivasan <kys@microsoft.com>]

> -----Original Message-----
> From: Greg KH [mailto:gregkh@linuxfoundation.org]
> Sent: Friday, February 07, 2014 3:17 PM
> To: KY Srinivasan
> Cc: linux-kernel@vger.kernel.org; devel@linuxdriverproject.org; olaf@aepfle.de;
> apw@canonical.com; jasowang@redhat.com
> Subject: Re: [PATCH V4 1/1] Drivers: hv: Implement the file copy service
> 
> On Tue, Jan 21, 2014 at 05:43:53PM -0800, K. Y. Srinivasan wrote:
> > +/*
> > + * Create a char device that can support read/write for passing
> > + * the payload.
> > + */
> > +static struct cdev fcopy_cdev;
> > +static struct class *cl;
> > +static struct device *sysfs_dev;
> 
> Why not just be a misc device, you only want 1 minor number for a char
> device:
> 
> > +static int fcopy_dev_init(void)
> > +{
> > +	int result;
> > +
> > +	result = alloc_chrdev_region(&fcopy_dev, 1, 1, "hv_fcopy");
> 
> See, one minor.
> 
> > +	if (result < 0) {
> > +		pr_err("Cannot get major number\n");
> > +		return result;
> > +	}
> > +
> > +	cl = class_create(THIS_MODULE, "chardev");
> 
> That's a _really_ generic name, come on, you know better than that.
> 
> > +	if (IS_ERR(cl)) {
> > +		pr_err("Error creating fcopy class.\n");
> 
> Your error string is wrong :(
> 
> > +		result = PTR_ERR(cl);
> > +		goto err_unregister;
> > +	}
> > +
> > +	sysfs_dev = device_create(cl, NULL, fcopy_dev, "%s", "hv_fcopy");
> 
> A device at the root of sysfs?  No, you have a bus to hang devices off
> of, use that.  What do you need this device for anyway?
> 
> > +	if (IS_ERR(sysfs_dev)) {
> > +		pr_err("Device creation failed\n");
> > +		result = PTR_ERR(cl);
> > +		goto err_destroy_class;
> > +	}
> > +
> > +	cdev_init(&fcopy_cdev, &fcopy_fops);
> > +	fcopy_cdev.owner = THIS_MODULE;
> > +	fcopy_cdev.ops = &fcopy_fops;
> > +
> > +	result = cdev_add(&fcopy_cdev, fcopy_dev, 1);
> 
> Ah, to get udev to pay attention to the char device, no, just use a misc
> device, should make this whole code a lot simpler and more "obvious" as
> to what you want/need.
> 
> > +	if (result) {
> > +		pr_err("Cannot cdev_add\n");
> > +		goto err_destroy_device;
> > +	}
> > +	return result;
> > +
> > +err_destroy_device:
> > +	device_destroy(cl, fcopy_dev);
> > +err_destroy_class:
> > +	class_destroy(cl);
> > +err_unregister:
> > +	unregister_chrdev_region(fcopy_dev, 1);
> > +	return result;
> 
> 
> Ugh, I hate the cdev interface, one of these days I'll fix it up, it's
> so unwieldy...
> 
> > +static void fcopy_dev_deinit(void)
> > +{
> > +	/*
> > +	 * first kill the daemon.
> > +	 */
> > +	if (dtp != NULL)
> > +		send_sig(SIGKILL, dtp, 0);
> 
> We kill userspace daemon's from the kernel?  That's a recipe for
> disaster...
> 
> Why?  What does it matter here if the daemon keeps running, it should
> fail gracefully if the character device is removed, right?  If not, that
> needs to be fixed anyway.

Greg,

Thanks for the detailed comments; I will address these in the next version.


Regards,

K. Y