Re: [PATCH] input: xpad.c - Xbox 360 wireless and sysfs support

[Greg KH <greg@kroah.com>]

On Mon, Feb 16, 2009 at 08:22:05AM -0500, Mike Murphy wrote:
> On Mon, Feb 16, 2009 at 3:31 AM, Oliver Neukum <oliver@neukum.org> wrote:
> ...
> >
> > 1. You need to check the returns of sscanf
> 
> Will add... this is currently preliminary and not very well tested.
> 
> > 2. This is very ugly:
> >
> > +/* read-only attrs */
> > +static ssize_t xpad_show_int(struct xpad_data *xd, struct xpad_attribute *attr,
> > +       char *buf)
> > +{
> > +       int value;
> > +       if (!strcmp(attr->attr.name, "controller_number"))
> > +               value = xd->controller_number;
> > +       else if (!strcmp(attr->attr.name, "pad_present"))
> > +               value = xd->pad_present;
> > +       else if (!strcmp(attr->attr.name, "controller_type"))
> > +               value = xd->controller_type;
> > +       else
> > +               value = 0;
> > +       return sprintf(buf, "%d\n", value);
> > +}
> 
> The above code is basically following the example in
> samples/kobject/kset-example.c. I broke the rest of the sysfs stuff
> out such that it uses separate functions for show/store, which
> definitely looks cleaner. However, given the large amount of code that
> results, I'm starting to think that re-factoring and pulling the sysfs
> code out to a separate file might be useful.
> 
> >
> > 3. Possible memory leak in error case:
> >
> > +static struct xpad_data *xpad_create_data(const char *name, struct kobject *parent) {
> > +       struct xpad_data *data = NULL;
> > +       int check;
> > +
> > +       data = kzalloc(sizeof(*data), GFP_KERNEL);
> > +       if (!data)
> > +               return NULL;
> > +
> > +       check = kobject_init_and_add(&data->kobj, &xpad_ktype, parent, "%s", name);
> > +       if (check) {
> > +               kobject_put(&data->kobj);
> > +               return NULL;
> > +       }
> >
> 
> My understanding from Documentation/kobject.txt is that the
> kobject_put in the 2nd error check will set the kobj's reference
> counter to zero, eventually causing the kobject core to call my
> cleanup function for the ktype (xpad_release) and free the memory.

That is correct.

The bigger question is why are you using a struct kobject in the first
place?  As you are a device, just use a struct device.  What are you
trying to do in sysfs here to make you want to use a "raw" kobject in a
driver?

thanks,

greg k-h