lkml.org 
[lkml]   [2014]   [Feb]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH V4 1/1] Drivers: hv: Implement the file copy service
Date


> -----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


\
 
 \ /
  Last update: 2014-02-09 03:21    [W:0.076 / U:0.040 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site