Re: [PATCH] input: imon driver for SoundGraph iMON/Antec Veris IR devices

2009-12-30 Thread Jarod Wilson
On Dec 29, 2009, at 5:30 PM, Dmitry Torokhov wrote:

 On Tue, Dec 29, 2009 at 12:04:00AM -0500, Jarod Wilson wrote:
 On Dec 28, 2009, at 4:31 AM, Dmitry Torokhov wrote:
 
 Hm, will this work on big-endian?
 
 Good question. Not sure offhand. Probably not. Unfortunately, the only 
 devices I have to test with at the moment are integrated into cases with x86 
 boards in them, so testing isn't particularly straight-forward. I should 
 probably get ahold of one of the plain external usb devices to play with... 
 Mind if I just add a TODO marker near that for now?
 
 
 How about just do le32_to_cpu() instead of memcpy()ing and that's
 probably it?

Hrm. My endian-fu sucks. Not sure what the right way to do it without the 
memcpy is. I thought I had something together than would work, using 2 lines 
(memcpy, then le32_to_cpu), but I just realized that'll go south on the keys 
where we're only looking at half the buffer (i.e., the wrong half on 
big-endian)... Will see what I can do to fix that up in the morning, was hoping 
to get this out tonight, but its nearly 3am (again)...

Also, forgot to reply to this bit last time:

 +init_completion(context-tx.finished);
 +atomic_set((context-tx.busy), 1);
 
 What does this atomic give you? Atomic operations do not imply memory
 barriers IIRC...

Code is borrowed from lirc_imon from before my time, honestly hadn't really 
looked into that much until now. I'm guessing it was *supposed* to provide an 
assurance that later readers saw the correct value for busy, but indeed, I 
don't think its actually guaranteeing that. I've changed busy to a bool and 
inserted smp_rmb()'s after each change to it, which I think *will* provide the 
desired guarantee. (In practice, its been working just fine either way for me 
so far).

 We have pretty different behavior depending on the interface, maybe the
 driver should be split further?
 
 This is what we'll call a fun topic... These devices expose two 
 interfaces, and a while back in the lirc_imon days, they actually loaded up 
 as two separate lirc devices. But there's a catch: they can't operate 
 independently. Some keys come in via intf0, some via intf1, even from the 
 very same remote. And the interfaces share a hardware-internal buffer (or 
 something), and if you're only listening to one of the two devices, and a 
 key is decoded and sent via the interface you're not listening to, it wedges 
 the entire device until you flush the other interface. Horribly bad hardware 
 design at play there, imo, but meh. What exactly did you have in mind as far 
 as a split? (And/or does it still apply with the above info taken into 
 consideration? ;).
 
 Ok, fair enough. I'd still want to see larger functions split up a bit though.

I've hacked imon_probe() up into 6 different functions now:

imon_probe()
 - imon_init_intf0()
  - imon_init_idev()
  - imon_init_display()
 - imon_init_intf1()
  - imon_init_touch()

(and there was an existing imon_set_ir_protocol() in the mix)

Quite a bit more manageable and readable now, I think. Haven't looked yet for 
other candidates for similar refactoring. Were there others you had in mind? At 
a glance, imon_incoming_packet() seems to be the only remaining function that 
is particularly hefty. Well, imon_probe() is still ~180 lines, but it used to 
be north of 400, I think... So perhaps I try trimming imon_probe() a bit more, 
and see what can be done to make imon_incoming_packet() less chunky.

Current work-in-progress pushed to git.kernel.org again.

Thanks much,

-- 
Jarod Wilson
ja...@wilsonet.com



--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] input: imon driver for SoundGraph iMON/Antec Veris IR devices

2009-12-30 Thread Jarod Wilson
On Dec 30, 2009, at 3:02 AM, Jarod Wilson wrote:

 On Dec 29, 2009, at 5:30 PM, Dmitry Torokhov wrote:
 
 On Tue, Dec 29, 2009 at 12:04:00AM -0500, Jarod Wilson wrote:
 On Dec 28, 2009, at 4:31 AM, Dmitry Torokhov wrote:
 
 Hm, will this work on big-endian?
 
 Good question. Not sure offhand. Probably not. Unfortunately, the only 
 devices I have to test with at the moment are integrated into cases with 
 x86 boards in them, so testing isn't particularly straight-forward. I 
 should probably get ahold of one of the plain external usb devices to play 
 with... Mind if I just add a TODO marker near that for now?
 
 
 How about just do le32_to_cpu() instead of memcpy()ing and that's
 probably it?
 
 Hrm. My endian-fu sucks. Not sure what the right way to do it without the 
 memcpy is. I thought I had something together than would work, using 2 lines 
 (memcpy, then le32_to_cpu), but I just realized that'll go south on the keys 
 where we're only looking at half the buffer (i.e., the wrong half on 
 big-endian)... Will see what I can do to fix that up in the morning, was 
 hoping to get this out tonight, but its nearly 3am (again)...

Got something that works in place now. Well, definitely works on x86, works in 
theory on big-endian, haven't got a way to test it on the latter just yet.


 [...] I'd still want to see larger functions split up a bit though.
 
 I've hacked imon_probe() up into 6 different functions now:
 
 imon_probe()
 - imon_init_intf0()
  - imon_init_idev()
  - imon_init_display()
 - imon_init_intf1()
  - imon_init_touch()
 
 (and there was an existing imon_set_ir_protocol() in the mix)
 
 Quite a bit more manageable and readable now, I think. Haven't looked yet for 
 other candidates for similar refactoring. Were there others you had in mind? 
 At a glance, imon_incoming_packet() seems to be the only remaining function 
 that is particularly hefty. Well, imon_probe() is still ~180 lines, but it 
 used to be north of 400, I think... So perhaps I try trimming imon_probe() a 
 bit more, and see what can be done to make imon_incoming_packet() less chunky.

Making good headway on imon_incoming_packet(), in-progress bits pushed again. 
Gotta take my son and a buddy to a movie. Life keeps getting in the way of me 
finishing this as quickly as I'd like... :)

-- 
Jarod Wilson
ja...@wilsonet.com



--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] input: imon driver for SoundGraph iMON/Antec Veris IR devices

2009-12-29 Thread Dan Carpenter

I ran smatch (http://repo.or.cz/w/smatch.git) on it and there are
some bugs worth fixing.

drivers/input/misc/imon.c +331 free_imon_context(7) error: dereferencing freed 
memory 'context'
Move the debug line earlier.

drivers/input/misc/imon.c +1812 imon_probe(216) error: dereferencing undefined: 
 'context-idev'
drivers/input/misc/imon.c +1876 imon_probe(280) error: dereferencing undefined: 
 'context-touch'
The allocation func can return NULL.  They probably won't fail in real 
life, but it will slightly annoy every person checking running smatch 
over the entire kernel (me).

drivers/input/misc/imon.c +1979 imon_probe(383) error: double unlock 
'mutex:context-lock'
drivers/input/misc/imon.c +1983 imon_probe(387) error: double unlock 
'mutex:context-lock'
It sometimes unlocks both before and after the goto.

regards,
dan carpenter
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] input: imon driver for SoundGraph iMON/Antec Veris IR devices

2009-12-29 Thread Jarod Wilson

On 12/29/2009 12:01 PM, Dan Carpenter wrote:


I ran smatch (http://repo.or.cz/w/smatch.git) on it and there are
some bugs worth fixing.

drivers/input/misc/imon.c +331 free_imon_context(7) error: dereferencing freed 
memory 'context'
Move the debug line earlier.

drivers/input/misc/imon.c +1812 imon_probe(216) error: dereferencing undefined:  
'context-idev'
drivers/input/misc/imon.c +1876 imon_probe(280) error: dereferencing undefined:  
'context-touch'
The allocation func can return NULL.  They probably won't fail in real
life, but it will slightly annoy every person checking running smatch
over the entire kernel (me).

drivers/input/misc/imon.c +1979 imon_probe(383) error: double unlock 
'mutex:context-lock'
drivers/input/misc/imon.c +1983 imon_probe(387) error: double unlock 
'mutex:context-lock'
It sometimes unlocks both before and after the goto.


Yeah, I think I've actually already fixed every one of these problems in 
the past 24 hours (a few just a few minutes ago), stay tuned for a 
repost, hopefully later today. :)


Thanks much,

--
Jarod Wilson
ja...@wilsonet.com
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] input: imon driver for SoundGraph iMON/Antec Veris IR devices

2009-12-29 Thread Dmitry Torokhov

On Tue, Dec 29, 2009 at 12:04:00AM -0500, Jarod Wilson wrote:

On Dec 28, 2009, at 4:31 AM, Dmitry Torokhov wrote:


Hm, will this work on big-endian?


Good question. Not sure offhand. Probably not. Unfortunately, the  
only devices I have to test with at the moment are integrated into  
cases with x86 boards in them, so testing isn't particularly  
straight-forward. I should probably get ahold of one of the plain  
external usb devices to play with... Mind if I just add a TODO  
marker near that for now?




How about just do le32_to_cpu() instead of memcpy()ing and that's
probably it?


+
+printk(KERN_INFO %s: iMON device (intf%d) disconnected\n,
+   __func__, ifnum);


dev_dbg().


Ah. I think I was thinking it might not be safe to use at this point  
in time. Which is what led me to look back at free_imon_context to  
see what it was doing. Looks like both here and to fix  
free_imon_context's use-after-free, I'll need to create a local  
struct device to pass over to dev_dbg().




+static int imon_resume(struct usb_interface *intf)
+{
+int rc = 0;
+struct imon_context *context = usb_get_intfdata(intf);
+int ifnum = intf-cur_altsetting-desc.bInterfaceNumber;
+
+if (ifnum == 0) {
+usb_fill_int_urb(context-rx_urb_intf0, context- 
usbdev_intf0,

+usb_rcvintpipe(context-usbdev_intf0,
+context-rx_endpoint_intf0-bEndpointAddress),
+context-usb_rx_buf, sizeof(context-usb_rx_buf),
+usb_rx_callback_intf0, context,
+context-rx_endpoint_intf0-bInterval);
+
+rc = usb_submit_urb(context-rx_urb_intf0, GFP_ATOMIC);
+
+} else {
+usb_fill_int_urb(context-rx_urb_intf1, context- 
usbdev_intf1,

+usb_rcvintpipe(context-usbdev_intf1,
+context-rx_endpoint_intf1-bEndpointAddress),
+context-usb_rx_buf, sizeof(context-usb_rx_buf),
+usb_rx_callback_intf1, context,
+context-rx_endpoint_intf1-bInterval);
+
+rc = usb_submit_urb(context-rx_urb_intf1, GFP_ATOMIC);
+}


We have pretty different behavior depending on the interface, maybe  
the

driver should be split further?


This is what we'll call a fun topic... These devices expose two  
interfaces, and a while back in the lirc_imon days, they actually  
loaded up as two separate lirc devices. But there's a catch: they  
can't operate independently. Some keys come in via intf0, some via  
intf1, even from the very same remote. And the interfaces share a  
hardware-internal buffer (or something), and if you're only  
listening to one of the two devices, and a key is decoded and sent  
via the interface you're not listening to, it wedges the entire  
device until you flush the other interface. Horribly bad hardware  
design at play there, imo, but meh. What exactly did you have in  
mind as far as a split? (And/or does it still apply with the above  
info taken into consideration? ;).


Ok, fair enough. I'd still want to see larger functions split up a bit  
though.


Thanks.

--
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] input: imon driver for SoundGraph iMON/Antec Veris IR devices

2009-12-28 Thread Jarod Wilson
Hey Dmitry,

Thanks much for the review, comments inline below...

On Dec 28, 2009, at 4:31 AM, Dmitry Torokhov wrote:

 Hi Jarod,
 
 On Mon, Dec 28, 2009 at 12:11:55AM -0500, Jarod Wilson wrote:
 This is an input layer driver for the SoundGraph iMON and Antec
 Veris IR and/or Display (LCD/VFD/VGA touchscreen) devices that
 do onboard signal decoding.
 
 This driver has been baking for quite a while in the lirc tree, as
 lirc_imon, which supported both all the current onboard decoding
 imon devices, as well as some older raw IR ones. I've split support
 into two different drivers now, this one, a pure input driver with
 no ties to lirc at all (device can be used with lircd via its
 userspace devinput driver though) and a pure lirc driver for the
 older devices that don't do onboard decoding. There's a bit of
 code duplication between the two, but not much anymore...
 
 I did start using *some* of the new sparse keymap code for this
 driver, but I quickly found I really needed more access to the
 raw data (as well as 64-bit hw codes), so I'm really only using
 sparse_keymap_{setup,free} right now, and I've not had cause to
 implement {s,g}etkeycode to date. Work for the future.
 
 Heavily tested with an Antec Veris Elite (IR + VFD), lightly tested
 with an Antec Veris Premiere (IR + LCD), works quite well on both,
 using both the stock Antec RM-200 remote and a Windows MCE remote
 (as well as with a Logitech Harmony 880 programmed to emulate the
 Antec remote).
 
 nb: checkpatch.pl has one warning on this patch:
  WARNING: struct file_operations should normally be const
  #200: FILE: drivers/input/misc/imon.c:146:
  +static struct file_operations display_fops = {
 
  total: 0 errors, 1 warnings, 2360 lines checked
 
 We don't make the struct const, because we swap in a new write fop
 if the device has an LCD character display instead of a VFD one.
 
 Why don't you set up separate fops for LCD and mark both const?

Was mainly to keep things simple, and that's just the way its been for some 
time... Creating separate fops didn't turn out to be too painful though, was 
only a 14-line increase, so I've gone ahead and done that.

 +/* Driver init/exit prototypes */
 +static int __init imon_init(void);
 +static void __exit imon_exit(void);
 
 Why are they needed?

Bah. Legacy crud from lirc_imon. Gone.

 +
 +/*** G L O B A L S ***/
 +
 +struct imon_context {
 +struct device *dev;
 +struct usb_device *usbdev_intf0;
 +/* Newer devices have two interfaces */
 +struct usb_device *usbdev_intf1;
 +int display_supported;  /* not all controllers do */
 +int display_isopen; /* display port has been opened */
 +int ir_isopen;  /* IR port open */
 +int ir_isassociating;   /* IR port open for association */
 
 bools for these?

Crap, yeah, been meaning to get around to changing those for some time, forgot 
about them. Done. (As well as for all other cases I could find where an int was 
being used where a bool would suffice).

 +/* to prevent races between open() and disconnect(), probing, etc */
 +static DEFINE_MUTEX(driver_lock);
 +
 +static int debug;
 +
 +/* lcd, vfd, vga or none? should be auto-detected, but can be overridden... 
 */
 +static int display_type;
 +
 +/* IR protocol: native iMON, Windows MCE (RC-6), or iMON w/o PAD stabilize 
 */
 +static int ir_protocol;
 +
 +/*
 + * In certain use cases, mouse mode isn't really helpful, and could actually
 + * cause confusion, so allow disabling it when the IR device is open.
 + */
 +static int nomouse;
 +
 +/* threshold at which a pad push registers as an arrow key in kbd mode */
 +static int pad_thresh;
 +
 +/***  M O D U L E   C O D E ***/
 +
 +MODULE_AUTHOR(MOD_AUTHOR);
 +MODULE_DESCRIPTION(MOD_DESC);
 +MODULE_VERSION(MOD_VERSION);
 +MODULE_LICENSE(GPL);
 +MODULE_DEVICE_TABLE(usb, imon_usb_id_table);
 +module_param(debug, int, S_IRUGO | S_IWUSR);
 +MODULE_PARM_DESC(debug, Debug messages: 0=no, 1=yes(default: no));
 
 Bool here. I also prefer keeping module_param() and MODULE_PARM_DESC()
 with the definition of the variable.

Done.

 +
 +static void free_imon_context(struct imon_context *context)
 +{
 +usb_free_urb(context-tx_urb);
 +usb_free_urb(context-rx_urb_intf0);
 +usb_free_urb(context-rx_urb_intf1);
 +kfree(context);
 +
 +dev_dbg(context-dev, %s: iMON context freed\n, __func__);
 +}

Eew. Just noticed the above has a nasty little use-after-free possibility 
with debugging enabled...

 +/**
 + * Process the incoming packet
 + */
 +static void imon_incoming_packet(struct imon_context *context,
 + struct urb *urb, int intf)
 +{
 +int len = urb-actual_length;
 +unsigned char *buf = urb-transfer_buffer;
 +struct device *dev = context-dev;
 +char rel_x = 0x00, rel_y = 0x00;
 +int ts_input = 0;
 +int dir = 0;
 +u16 timeout, threshold;
 +u16 kc;
 +u16 norelease = 0;
 +int i, k;
 +int offset =