> -----Original Message----- > From: Greg KH [mailto:gregkh@xxxxxxxxxxxxxxxxxxx] > Sent: Friday, February 07, 2014 3:17 PM > To: KY Srinivasan > Cc: linux-kernel@xxxxxxxxxxxxxxx; devel@xxxxxxxxxxxxxxxxxxxxxx; olaf@xxxxxxxxx; > apw@xxxxxxxxxxxxx; jasowang@xxxxxxxxxx > 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 _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
