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

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

[Oliver Neukum]

Am Sunday 15 February 2009 05:08:46 schrieb Mike Murphy:
> Greetings,
> 
> The attached patch is a result of a few days of hacking to get better
> Linux support for Xbox 360 wireless controllers. Major functional
> improvements include support for the LEDs on the wireless controllers
> (which are auto-set to the controller number as they are on the
> official console), added support for rumble effects on wireless
> controllers, added support for dead zones on all supported
> controllers, and added support for a square axis mapping for the left
> stick on all supported controllers. A large amount of the bulk in this
> patch is from the addition of a sysfs interface to retrieve
> information and adjust the dead zone/square axis settings.
[..]
> Patch generated against 2.6.28.2 (no changes in relevant in-kernel
> driver from 2.6.28.2 to 2.6.28.5, so should apply against latest
> stable).
> 
> Comments are appreciated. I am _NOT_ subscribed to the LKML, so please
> CC me directly.

1. You need to check the returns of sscanf
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);
+}

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;
+	}

4. Why the cpup variety?

+	coords[0] = (__s16) le16_to_cpup((__le16 *)(data + x_offset));

5. What happens if this work is already scheduled?

 	if (data[0] & 0x08) {
+		padnum = xpad->controller_data->controller_number;
 		if (data[1] & 0x80) {
-			xpad->pad_present = 1;
-			usb_submit_urb(xpad->bulk_out, GFP_ATOMIC);
-		} else
-			xpad->pad_present = 0;
+			printk(KERN_INFO "Wireless Xbox 360 pad #%d present\n", padnum);
+			xpad->controller_data->pad_present = 1;
+			
+			INIT_WORK(&xpad->work, &xpad_work_controller);
+			schedule_work(&xpad->work);

6. No GFP_ATOMIC. If you can take a mutex you can sleep.
+		usb_submit_urb(xpad->irq_out, GFP_ATOMIC);

	Regards
		Oliver

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

[Mike Murphy]

On Mon, Feb 16, 2009 at 3:31 AM, Oliver Neukum <oliver-GvhC2dPhHPQdnm+yROfE0A@public.gmane.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. Is
this not correct? I find the sysfs docs to be fairly thin... and sysfs
seems to be substantially more complex than procfs or ioctls would be
for the same purpose. However, everything I read suggested that sysfs
is the "best" way to go in a modern kernel.

> 4. Why the cpup variety?
>
> +       coords[0] = (__s16) le16_to_cpup((__le16 *)(data + x_offset));
>

The cpup cast is in the original stable driver
(drivers/input/joystick/xpad.c), and I didn't question it.

> 5. What happens if this work is already scheduled?
>
>        if (data[0] & 0x08) {
> +               padnum = xpad->controller_data->controller_number;
>                if (data[1] & 0x80) {
> -                       xpad->pad_present = 1;
> -                       usb_submit_urb(xpad->bulk_out, GFP_ATOMIC);
> -               } else
> -                       xpad->pad_present = 0;
> +                       printk(KERN_INFO "Wireless Xbox 360 pad #%d present\n", padnum);
> +                       xpad->controller_data->pad_present = 1;
> +
> +                       INIT_WORK(&xpad->work, &xpad_work_controller);
> +                       schedule_work(&xpad->work);
>

I'm still a little fuzzy on this... in theory, I could see that
INIT_WORK would clobber the existing work structures while they wait
in the queue (thought about changing to PREPARE_WORK).

However, in practice, this work queue trick is only used when a
wireless 360 controller connects to the receiver. There is 1 instance
of struct usb_xpad per wireless controller (4 total, since the
receiver exposes 4 controller slots), and each instance has a separate
struct work_struct. So two things have to happen to reschedule the
work before it completes:

1. The user has to remove the battery pack from the controller,
reinstall the battery pack, and re-activate the controller by pushing
and holding the center button for at least 1 second.

2. The kernel has to be busy enough not to have completed the work in
the ~2 seconds a human could have done (1).

I need a bit of guidance from someone who has a better understanding
of the work queues to have a good solution to this one. Is switching
to PREPARE_WORK sufficient (with an INIT_WORK somewhere in
xpad_probe)? Or is a more involved solution needed?

> 6. No GFP_ATOMIC. If you can take a mutex you can sleep.
> +               usb_submit_urb(xpad->irq_out, GFP_ATOMIC);
>

Per the "Linux Device Drivers" book (O'Reilly, 3rd ed), the claim is
made that submissions while holding a mutex should be GFP_ATOMIC. My
tests seemed to verify this claim... as sending LED commands
GFP_KERNEL while holding the mutex resulted in BUGs (scheduling while
atomic) in dmesg. Switching those GFP_KERNELs to GFP_ATOMICs
eliminated that particular BUG.

>        Regards
>                Oliver

Thanks for your reply... I will keep working on the driver as time
allows. This is really the first driver on which I've done any
substantial hacking, and my formal kernel-level programming training
was on an older version of the FreeBSD kernel, so I'm having to learn
things as I go. I'm trying to develop based off the latest stable
sources, so the outdated nature of most of the reference material I
have is not helping matters.

Thanks,
Mike
-- 
Mike Murphy
Ph.D. Candidate and NSF Graduate Research Fellow
Clemson University School of Computing
120 McAdams Hall
Clemson, SC 29634-0974 USA
Tel: +1 864.656.2838   Fax: +1 864.656.0145
http://cirg.cs.clemson.edu/~mamurph
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Oliver Neukum]

Am Monday 16 February 2009 14:22:05 schrieb Mike Murphy:
> >
> > 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. Is
> this not correct? I find the sysfs docs to be fairly thin... and sysfs

I don't know. I also find that documentation thin.
Please add that the memory is freed elsewhere.

> seems to be substantially more complex than procfs or ioctls would be
> for the same purpose. However, everything I read suggested that sysfs
> is the "best" way to go in a modern kernel.
> 
> > 4. Why the cpup variety?
> >
> > +       coords[0] = (__s16) le16_to_cpup((__le16 *)(data + x_offset));
> >
> 
> The cpup cast is in the original stable driver
> (drivers/input/joystick/xpad.c), and I didn't question it.
> 
> > 5. What happens if this work is already scheduled?
> >
> >        if (data[0] & 0x08) {
> > +               padnum = xpad->controller_data->controller_number;
> >                if (data[1] & 0x80) {
> > -                       xpad->pad_present = 1;
> > -                       usb_submit_urb(xpad->bulk_out, GFP_ATOMIC);
> > -               } else
> > -                       xpad->pad_present = 0;
> > +                       printk(KERN_INFO "Wireless Xbox 360 pad #%d present\n", padnum);
> > +                       xpad->controller_data->pad_present = 1;
> > +
> > +                       INIT_WORK(&xpad->work, &xpad_work_controller);
> > +                       schedule_work(&xpad->work);
> >

> 1. The user has to remove the battery pack from the controller,
> reinstall the battery pack, and re-activate the controller by pushing
> and holding the center button for at least 1 second.
> 
> 2. The kernel has to be busy enough not to have completed the work in
> the ~2 seconds a human could have done (1).

Also what happens if the work is scheduled  and you unplug?

> I need a bit of guidance from someone who has a better understanding
> of the work queues to have a good solution to this one. Is switching
> to PREPARE_WORK sufficient (with an INIT_WORK somewhere in
> xpad_probe)? Or is a more involved solution needed?
> 
> > 6. No GFP_ATOMIC. If you can take a mutex you can sleep.
> > +               usb_submit_urb(xpad->irq_out, GFP_ATOMIC);
> >
> 
> Per the "Linux Device Drivers" book (O'Reilly, 3rd ed), the claim is
> made that submissions while holding a mutex should be GFP_ATOMIC. My

That is not correct. It is true while holding a spinlock, not a mutex.

> tests seemed to verify this claim... as sending LED commands
> GFP_KERNEL while holding the mutex resulted in BUGs (scheduling while
> atomic) in dmesg. Switching those GFP_KERNELs to GFP_ATOMICs
> eliminated that particular BUG.

Please post that BUG.

	Regards
		Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Greg KH]

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-GvhC2dPhHPQdnm+yROfE0A@public.gmane.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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Mike Murphy]

On Mon, Feb 16, 2009 at 11:13 AM, Greg KH <greg@kroah.com> wrote:
...
>
> 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
>

The purpose of the struct xpad_data (which contains the struct
kobject) is to expose parameters that can be read and/or tweaked at
runtime. At the moment, the adjustable parameters are the size of the
dead zone around the center of the stick, and the choice of whether to
map to a square axis. Read-only data include a unique identifier for
each pad (wireless) and an indication as to whether the wireless pad
is actually connected.

The idea behind this interface is to enable a future userspace
application, likely using HAL and D-BUS, to receive events whenever a
wireless pad connects, and to identify the particular pad in question
(specifically, the unique ID of the pad). The user could then, for
example, have a different dead zone size for each pad, based upon the
actual manufacturing variations in the sticks on that pad (some appear
more precise at centering than others). Ideally, this functionality
could be designed in a "common" way so that the userspace code could
eventually work with other, non-xpad gaming devices.

So I really just wanted a way to get some parameters to userspace. The
consensus opinion of everything I read was that /proc additions were
frowned upon for cluttering up /proc with kernel stuff, ioctls were
viewed negatively because of their inconsistent nature from device to
device, and that sysfs was the best solution for this problem.

I didn't use a struct device because of the existing struct usb_xpad
that was created on a per-gamepad basis (not necessarily per device,
as the wireless 360 adapter is one physical device but exposes 4
gamepads by exposing 4 logical devices [and then each gamepad, in
turn, is technically a USB hub that can have other stuff
attached...]). I tried not to break existing functionality.
Additionally, struct usb_xpad contains two device pointers: one to the
actual USB device, and one to an input device (see source of the
in-tree xpad.c). So I followed your kobject.txt documentation and
samples to create a new object whose sole purpose in life is to expose
the sysfs interface, without interfering with the existing device
entries in the driver. I'm not sure I see a clean way to use a single
struct device here....

Thanks,
Mike
-- 
Mike Murphy
Ph.D. Candidate and NSF Graduate Research Fellow
Clemson University School of Computing
120 McAdams Hall
Clemson, SC 29634-0974 USA
Tel: +1 864.656.2838   Fax: +1 864.656.0145
http://cirg.cs.clemson.edu/~mamurph

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

[Greg KH]

On Mon, Feb 16, 2009 at 01:09:00PM -0500, Mike Murphy wrote:
> On Mon, Feb 16, 2009 at 11:13 AM, Greg KH <greg-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org> wrote:
> ...
> >
> > 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
> >
> 
> The purpose of the struct xpad_data (which contains the struct
> kobject) is to expose parameters that can be read and/or tweaked at
> runtime. At the moment, the adjustable parameters are the size of the
> dead zone around the center of the stick, and the choice of whether to
> map to a square axis. Read-only data include a unique identifier for
> each pad (wireless) and an indication as to whether the wireless pad
> is actually connected.

Then hang those parameters off the struct device you already have.

> The idea behind this interface is to enable a future userspace
> application, likely using HAL and D-BUS, to receive events whenever a
> wireless pad connects, and to identify the particular pad in question
> (specifically, the unique ID of the pad). The user could then, for
> example, have a different dead zone size for each pad, based upon the
> actual manufacturing variations in the sticks on that pad (some appear
> more precise at centering than others). Ideally, this functionality
> could be designed in a "common" way so that the userspace code could
> eventually work with other, non-xpad gaming devices.

Have you documented them in Documentation/ABI?

> I didn't use a struct device because of the existing struct usb_xpad
> that was created on a per-gamepad basis (not necessarily per device,
> as the wireless 360 adapter is one physical device but exposes 4
> gamepads by exposing 4 logical devices [and then each gamepad, in
> turn, is technically a USB hub that can have other stuff
> attached...]).

Put it on the logical device, as given to you.

> I tried not to break existing functionality.  Additionally, struct
> usb_xpad contains two device pointers: one to the actual USB device,
> and one to an input device (see source of the in-tree xpad.c). So I
> followed your kobject.txt documentation and samples to create a new
> object whose sole purpose in life is to expose the sysfs interface,
> without interfering with the existing device entries in the driver.
> I'm not sure I see a clean way to use a single struct device here....

Put it on the input device, which is what is the per-device thing.  It's
much simpler than creating a new struct kobject.  You can even create a
subdirectory for your attributes if you use an attribute group (which
you should be doing anyway, it's much simpler that way.)

And document the attributes please.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Mike Murphy]

On Mon, Feb 16, 2009 at 1:59 PM, Greg KH <greg-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org> wrote:
>
> Put it on the logical device, as given to you.
>
>> I tried not to break existing functionality.  Additionally, struct
>> usb_xpad contains two device pointers: one to the actual USB device,
>> and one to an input device (see source of the in-tree xpad.c). So I
>> followed your kobject.txt documentation and samples to create a new
>> object whose sole purpose in life is to expose the sysfs interface,
>> without interfering with the existing device entries in the driver.
>> I'm not sure I see a clean way to use a single struct device here....
>
> Put it on the input device, which is what is the per-device thing.  It's
> much simpler than creating a new struct kobject.  You can even create a
> subdirectory for your attributes if you use an attribute group (which
> you should be doing anyway, it's much simpler that way.)
>

OK, one thing I'm not clear on: is there a clean API for adding
attributes to an existing struct device, or do I need to "subclass" it
(the C containment and delegation approach)?

This may take me a few days or a week or so, depending on how things
go. It's dissertation proposal season....

> And document the attributes please.
>

Will do... still need to nail down what the interface should look like.

> thanks,
>
> greg k-h
>

Thanks,
Mike
-- 
Mike Murphy
Ph.D. Candidate and NSF Graduate Research Fellow
Clemson University School of Computing
120 McAdams Hall
Clemson, SC 29634-0974 USA
Tel: +1 864.656.2838   Fax: +1 864.656.0145
http://cirg.cs.clemson.edu/~mamurph
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Greg KH]

On Mon, Feb 16, 2009 at 02:30:01PM -0500, Mike Murphy wrote:
> On Mon, Feb 16, 2009 at 1:59 PM, Greg KH <greg@kroah.com> wrote:
> >
> > Put it on the logical device, as given to you.
> >
> >> I tried not to break existing functionality.  Additionally, struct
> >> usb_xpad contains two device pointers: one to the actual USB device,
> >> and one to an input device (see source of the in-tree xpad.c). So I
> >> followed your kobject.txt documentation and samples to create a new
> >> object whose sole purpose in life is to expose the sysfs interface,
> >> without interfering with the existing device entries in the driver.
> >> I'm not sure I see a clean way to use a single struct device here....
> >
> > Put it on the input device, which is what is the per-device thing.  It's
> > much simpler than creating a new struct kobject.  You can even create a
> > subdirectory for your attributes if you use an attribute group (which
> > you should be doing anyway, it's much simpler that way.)
> >
> 
> OK, one thing I'm not clear on: is there a clean API for adding
> attributes to an existing struct device, or do I need to "subclass" it
> (the C containment and delegation approach)?

device_create_file()

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

[Mike Murphy]

On Mon, Feb 16, 2009 at 3:22 PM, Greg KH <greg@kroah.com> wrote:
>> >
>> > Put it on the input device, which is what is the per-device thing.  It's
>> > much simpler than creating a new struct kobject.  You can even create a
>> > subdirectory for your attributes if you use an attribute group (which
>> > you should be doing anyway, it's much simpler that way.)
>> >
>>
>> OK, one thing I'm not clear on: is there a clean API for adding
>> attributes to an existing struct device, or do I need to "subclass" it
>> (the C containment and delegation approach)?
>
> device_create_file()
>

In the process of trying to re-factor the code for the above changes,
I ran into the problem that the show and store functions for sysfs
expect to be passed a struct device *. I can get to struct input_dev *
without problems (since struct input_dev contains the struct device,
and I can simply use container_of indirectly via to_input_dev).
However, I can't seem to get back to struct usb_xpad, because that
structure defines 2 pointers to devices, rather than simply embedding
the devices:

struct usb_xpad {
	struct input_dev *dev;		/* input device interface */
	struct usb_device *udev;	/* usb device */
...
};

This is not my code... it was set up this way in the stable xpad driver.

So it looks like I'm stuck with a struct input_dev * pointer to the
input device, a struct device * pointer in the show/store handlers,
and no way to get back to struct usb_xpad * with the container_of
macro. Unless, of course, there is something I don't know about
container_of (or another macro I can use in this instance).

Worse, I can't simply change the struct input_dev *dev to struct
input_dev dev, because the input subsystem's input_register_device
expects to have a struct input_dev * pointer allocated with
input_allocate_device, which does both a kzalloc AND initialization.
Trying to hack my driver by incorporating the initialization logic in
input_allocate_device would be stupid, since the result would be
unmaintainable. So the route out of this mess, going in the direction
of device attributes, would be to modify drivers/input/input.c to
factor the initialization away from the allocation.

So which is the lesser of two evils? Allocating a new kobject, or
mucking around with more of the input subsystem? Or am I missing
something?

Thanks,
Mike
-- 
Mike Murphy
Ph.D. Candidate and NSF Graduate Research Fellow
Clemson University School of Computing
120 McAdams Hall
Clemson, SC 29634-0974 USA
Tel: +1 864.656.2838   Fax: +1 864.656.0145
http://cirg.cs.clemson.edu/~mamurph

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

[Greg KH]

On Mon, Feb 16, 2009 at 09:53:54PM -0500, Mike Murphy wrote:
> struct usb_xpad {
> 	struct input_dev *dev;		/* input device interface */
> 	struct usb_device *udev;	/* usb device */
> ...
> };
> 
> This is not my code... it was set up this way in the stable xpad driver.

And it is correct :)

> So it looks like I'm stuck with a struct input_dev * pointer to the
> input device, a struct device * pointer in the show/store handlers,
> and no way to get back to struct usb_xpad * with the container_of
> macro. Unless, of course, there is something I don't know about
> container_of (or another macro I can use in this instance).

input_set_drvdata() and input_get_drvdata() is what you are looking for.

Hope this helps,

greg k-h

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

[Mike Murphy]

On Mon, Feb 16, 2009 at 10:18 PM, Greg KH <greg@kroah.com> wrote:
>
> input_set_drvdata() and input_get_drvdata() is what you are looking for.
>

Thanks! That solved the problem at least as far as getting the entries
to show up in sysfs... though somehow now I'm having problems with the
I/O buffers, and a cat of the sysfs file shows no output (not even a
blank line). I added a couple printk's to one of the routines, and the
values are being read correctly from the data structure, but no
working I/O (cannot set the values either, per printk results). The
dmesg output shows the printk results, so the functions are getting
called. Even the count from sprintf is correct, and the resulting
buffer looks like it should... though with the extra blank line I
added in the last printk:

static ssize_t xpad_show_dead_zone(struct device *dev, char *buf)
{
	struct usb_xpad *xpad = to_xpad(dev);
	int count;
	printk(KERN_INFO "Dead zone is %d\n", xpad->dead_zone);
	count = sprintf(buf, "%d\n", xpad->dead_zone);
	printk(KERN_INFO "Count is %d\n", count);
	printk(KERN_INFO "Buffer is %s\n", buf);
	return count;
}

Unless that is some common error that is obvious from its description,
I will have to chase the bug down tomorrow or Wednesday. It's getting
a bit late here.

Thanks,
Mike
-- 
Mike Murphy
Ph.D. Candidate and NSF Graduate Research Fellow
Clemson University School of Computing
120 McAdams Hall
Clemson, SC 29634-0974 USA
Tel: +1 864.656.2838   Fax: +1 864.656.0145
http://cirg.cs.clemson.edu/~mamurph

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

[Mike Murphy]

On Mon, Feb 16, 2009 at 11:57 PM, Mike Murphy <mamurph@cs.clemson.edu> wrote:
> Unless that is some common error that is obvious from its description,
> I will have to chase the bug down tomorrow or Wednesday. It's getting
> a bit late here.
>

Fixed... the function signature was wrong. Should have been:

static ssize_t xpad_show_dead_zone(struct device *dev, struct
device_attribute *attr, char *buf)

Incidentally, both Documentation/driver-model/device.txt and
Documentation/filesystems/sysfs.txt appear to be wrong. The former
defines the device attribute structure as:

struct device_attribute {
        struct attribute        attr;
        ssize_t (*show)(struct device * dev, char * buf, size_t count,
loff_t off);
        ssize_t (*store)(struct device * dev, const char * buf, size_t
count, loff_t off);
};

while the latter (that I followed last night) defines it as:

struct device_attribute {
        struct attribute        attr;
        ssize_t (*show)(struct device * dev, char * buf);
        ssize_t (*store)(struct device * dev, const char * buf);
};

I finally got the correct one out of include/linux/device.h:

struct device_attribute {
	struct attribute	attr;
	ssize_t (*show)(struct device *dev, struct device_attribute *attr,
			char *buf);
	ssize_t (*store)(struct device *dev, struct device_attribute *attr,
			 const char *buf, size_t count);
};

Thanks,
Mike
-- 
Mike Murphy
Ph.D. Candidate and NSF Graduate Research Fellow
Clemson University School of Computing
120 McAdams Hall
Clemson, SC 29634-0974 USA
Tel: +1 864.656.2838   Fax: +1 864.656.0145
http://cirg.cs.clemson.edu/~mamurph

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

[Greg KH]

On Tue, Feb 17, 2009 at 01:27:48PM -0500, Mike Murphy wrote:
> On Mon, Feb 16, 2009 at 11:57 PM, Mike Murphy <mamurph-ZCNwHF9ErpZ2icitjWtXSw@public.gmane.org> wrote:
> > Unless that is some common error that is obvious from its description,
> > I will have to chase the bug down tomorrow or Wednesday. It's getting
> > a bit late here.
> >
> 
> Fixed... the function signature was wrong. Should have been:

Ah, always pay attention to complier warnings :)

> static ssize_t xpad_show_dead_zone(struct device *dev, struct
> device_attribute *attr, char *buf)
> 
> Incidentally, both Documentation/driver-model/device.txt and
> Documentation/filesystems/sysfs.txt appear to be wrong. The former
> defines the device attribute structure as:

<snip>

Care to send a patch to fix up the documentation?

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Mike Murphy]

On Mon, Feb 16, 2009 at 10:21 AM, Oliver Neukum <oliver@neukum.org> wrote:
>> tests seemed to verify this claim... as sending LED commands
>> GFP_KERNEL while holding the mutex resulted in BUGs (scheduling while
>> atomic) in dmesg. Switching those GFP_KERNELs to GFP_ATOMICs
>> eliminated that particular BUG.
>
> Please post that BUG.
>

I switched back to GFP_KERNEL on the functions in question.

After further testing, I have determined that the BUG only occurs on
my Ubuntu (2.6.27) system, and only when using the rumble function. I
am unable to reproduce the BUG on my Arch (2.6.28) system, so it is
either an artifact of some patch to this kernel, or it got fixed
somewhere between 2.6.27 and 2.6.28.

Based on this analysis, and (more importantly) an inability to
reproduce the behavior on a more vanilla kernel, I have removed the
comments about it from the TODO section of the driver. It isn't a true
kernel bug at least with a more recent stable kernel.

Mike
-- 
Mike Murphy
Ph.D. Candidate and NSF Graduate Research Fellow
Clemson University School of Computing
120 McAdams Hall
Clemson, SC 29634-0974 USA
Tel: +1 864.656.2838   Fax: +1 864.656.0145
http://cirg.cs.clemson.edu/~mamurph