From: Werner M. <we...@vo...> - 2015-06-30 15:08:19
|
Hi, I would like to ask, if I can get assistance for debugging the imon-mod from kernel sources. I had always problems with the module and while it has been in lirc in the last years, but now as I have a new TV, it's even worse and I have hangs in there almost twice a day. I got some coding exp, but never touched the kernel, so I think I need some advice how to do it. I think the problem is in the handling of unexpected messages, because it works for days when I don't touch the TV. More precise it seems that in some situations something is sent to the device which fucks it up. I didn't setup anything to use the vfd, and it is black when everything works, but in the situation when it hangs, I have some weird symbols on the display. In the logs I get: [81641.310168] imon 5-1:1.0: imon usb_rx_callback_intf0: status(-32): ignored [81641.318168] imon 5-1:1.0: imon usb_rx_callback_intf0: status(-32): ignored [81641.326168] imon 5-1:1.0: imon usb_rx_callback_intf0: status(-32): ignored You see the rapid rate, so it's flodding all the logs. When I reboot the machine, I get in every second try: [ 184.118947] intf0 decoded packet: 80 0f 84 22 00 00 9e 55 [ 184.126948] intf0 decoded packet: 80 0f 84 22 00 00 9e d6 [ 184.134948] intf0 decoded packet: b6 db 6d b6 00 00 9e ae The output is only visible when I activate the debug-param of the module, but it the only debug output in there. -- MfG usw. Werner Mahr |
From: Jarod W. <ja...@wi...> - 2007-08-25 04:58:42
|
Hey all, I've not been on this particular list for long, but I'm a long-time lirc user (via mythtv), who now works at Red Hat. For a while now, I've been wanting to get lirc included in Fedora kernels, and tonight, I finally got around to doing something significant about it, and would like people's feedback. I know Christoph has basically said "go for it, I don't have the time", and Mario started working on this a bit ago. I'd been working on it a bit myself silently in my spare time, and hooked up with Mario and merged his work in with mine. The result is a patch that now applies on top of the latest fedora development kernel, which is 2.6.23-rc3-git6-based as I write this, and builds quite nicely. Said patch can be found here: http://people.redhat.com/jwilson/lirc/linux-2.6-lirc.patch It includes all the latest bits from lirc cvs, save the lirc_gpio driver, but restructures the code in a few ways: 1) header include paths have been altered to match the file layout as patched into the kernel tree 2) in theory, all '#if LINUX_VERSION_CODE' and similar bits have been eliminated, along with all code sections for older kernels dropped, which trims down the patch by over 1000 lines and definitely makes it more manageable for upstream. 3) a few minor changes from the upstream code base to compile with the latest rawhide gcc (a few "error: void value not ignored as it ought to be" messages, which I've not yet looked into properly fixing). -- I've done a local x86_64 test build with this patch against 2.6.23- rc3-git6, which upon initial inspection, looks good, though I have yet to try it out on actual hardware (on my todo list this weekend). See resulting build file listing below. If this patch meets with folks approval, I'd like to get it broken up into manageable chunks and sent off to lkml for review so we can hopefully get everything included in the kernel proper. $ rpm -qpl RPMS/x86_64/ kernel-2.6.23-0.133.rc3.git6.lirc1.fc8.x86_64.rpm | grep -e 'lirc_.*.ko' /lib/modules/2.6.23-0.133.rc3.git6.lirc1.fc8/kernel/drivers/input/ lirc/lirc_atiusb/lirc_atiusb.ko /lib/modules/2.6.23-0.133.rc3.git6.lirc1.fc8/kernel/drivers/input/ lirc/lirc_bt829/lirc_bt829.ko /lib/modules/2.6.23-0.133.rc3.git6.lirc1.fc8/kernel/drivers/input/ lirc/lirc_cmdir/commandir.ko /lib/modules/2.6.23-0.133.rc3.git6.lirc1.fc8/kernel/drivers/input/ lirc/lirc_cmdir/lirc_cmdir.ko /lib/modules/2.6.23-0.133.rc3.git6.lirc1.fc8/kernel/drivers/input/ lirc/lirc_dev/lirc_dev.ko /lib/modules/2.6.23-0.133.rc3.git6.lirc1.fc8/kernel/drivers/input/ lirc/lirc_i2c/lirc_i2c.ko /lib/modules/2.6.23-0.133.rc3.git6.lirc1.fc8/kernel/drivers/input/ lirc/lirc_igorplugusb/lirc_igorplugusb.ko /lib/modules/2.6.23-0.133.rc3.git6.lirc1.fc8/kernel/drivers/input/ lirc/lirc_imon/lirc_imon.ko /lib/modules/2.6.23-0.133.rc3.git6.lirc1.fc8/kernel/drivers/input/ lirc/lirc_it87/lirc_it87.ko /lib/modules/2.6.23-0.133.rc3.git6.lirc1.fc8/kernel/drivers/input/ lirc/lirc_mceusb/lirc_mceusb.ko /lib/modules/2.6.23-0.133.rc3.git6.lirc1.fc8/kernel/drivers/input/ lirc/lirc_mceusb2/lirc_mceusb2.ko /lib/modules/2.6.23-0.133.rc3.git6.lirc1.fc8/kernel/drivers/input/ lirc/lirc_pvr150/lirc_pvr150.ko /lib/modules/2.6.23-0.133.rc3.git6.lirc1.fc8/kernel/drivers/input/ lirc/lirc_serial/lirc_serial.ko /lib/modules/2.6.23-0.133.rc3.git6.lirc1.fc8/kernel/drivers/input/ lirc/lirc_sir/lirc_sir.ko/lib/modules/2.6.23-0.133.rc3.git6.lirc1.fc7/ kernel/drivers/input/lirc/lirc_streamzap/lirc_streamzap.ko /lib/modules/2.6.23-0.133.rc3.git6.lirc1.fc8/kernel/drivers/input/ lirc/lirc_ttusbir/lirc_ttusbir.ko Thanks for listening, time for z's... -- Jarod Wilson ja...@wi... |
From: <li...@ba...> - 2007-08-25 16:44:43
|
Hi! Jarod Wilson "ja...@wi..." wrote: > I've been wanting to get lirc included in Fedora kernels, and > tonight, I finally got around to doing something significant about > it, and would like people's feedback. Great! If there is anything that you think should also go into LIRC CVS (e.g. getting rid of 2.4 compatibility), please send patches. Christoph |
From: Jarod W. <ja...@wi...> - 2007-08-27 20:13:00
|
Hi Christoph (and list), On Saturday 25 August 2007 12:25:00 pm Christoph Bartelmus wrote: > Jarod Wilson "ja...@wi..." wrote: > > I've been wanting to get lirc included in Fedora kernels, and > > tonight, I finally got around to doing something significant about > > it, and would like people's feedback. > > Great! > If there is anything that you think should also go into LIRC CVS (e.g. > getting rid of 2.4 compatibility), please send patches. I think there's actually quite a bit that should probably go into LIRC CVS, so things don't diverge too much. I'd actually like to open some discussion about how this would best work. Things we need to do for kernel inclusion: 1) run <kernel source>/scripts/checkpatch.pl over any patches to be applied to the kernel, reduce checkpatch output as much as possible. A lot of this is coding style changes, whitespace clean-ups, etc. 2) fixes for compile errors and warnings with latest linus kernel 3) flatten source tree so we have all bits in <kernel source>/drivers/input/lirc/, rather than each driver in its own sub-directory 4) drop as much stuff in '#if LINUX_VERSION_CODE' wrappers as possible. I presume the first two items would be easily merged into current LIRC CVS. Not so sure about whether or not flattening the source tree would be problematic, or how much of the '#if LINUX_VERSION_CODE' bits you'd want to drop, since we'd quickly lose compatibility with anything older than 2.6.19, iirc. What are your thoughts on all this? To get an idea of the work done thus far, check out: http://git.wilsonet.com/linux-2.6-lirc.git/ -- Jarod Wilson ja...@wi... |
From: Jarod W. <ja...@wi...> - 2007-08-30 20:54:27
|
On Monday 27 August 2007 04:12:54 pm Jarod Wilson wrote: > Hi Christoph (and list), > > On Saturday 25 August 2007 12:25:00 pm Christoph Bartelmus wrote: > > Jarod Wilson "ja...@wi..." wrote: > > > I've been wanting to get lirc included in Fedora kernels, and > > > tonight, I finally got around to doing something significant about > > > it, and would like people's feedback. > > > > Great! > > If there is anything that you think should also go into LIRC CVS (e.g. > > getting rid of 2.4 compatibility), please send patches. > > I think there's actually quite a bit that should probably go into LIRC CVS, > so things don't diverge too much. I'd actually like to open some discussion > about how this would best work. Things we need to do for kernel inclusion: > > 1) run <kernel source>/scripts/checkpatch.pl over any patches to be applied > to the kernel, reduce checkpatch output as much as possible. A lot of this > is coding style changes, whitespace clean-ups, etc. > > 2) fixes for compile errors and warnings with latest linus kernel > > 3) flatten source tree so we have all bits in <kernel > source>/drivers/input/lirc/, rather than each driver in its own > sub-directory > > 4) drop as much stuff in '#if LINUX_VERSION_CODE' wrappers as possible. > > > I presume the first two items would be easily merged into current LIRC CVS. > Not so sure about whether or not flattening the source tree would be > problematic, or how much of the '#if LINUX_VERSION_CODE' bits you'd want to > drop, since we'd quickly lose compatibility with anything older than > 2.6.19, iirc. What are your thoughts on all this? > > To get an idea of the work done thus far, check out: > > http://git.wilsonet.com/linux-2.6-lirc.git/ FWIW, I'm reworking all the patches to apply to both the git tree and lirc cvs right now, the idea being to push as much as possible back to lirc cvs before I start doing things like stripping out the compatibility for older kernels. I talked with a buddy of mine over in v4l-dvb land, who had a few suggestions for how they do things for older kernels. The v4l-dvb hg tree still builds back to about 2.6.18, but has most of the compatibility stuff abstracted out into a compat header (which looks like it was sort of partially done with lirc's kcompat.h), and then before committing to the v4l-dvb git tree for Linus to pull, the compat stuff is stripped out in a scripted fashion. I think trying to achieve something similar would be optimal to help prevent diverging code. A bit more clean-up work, then I'll get this new-and-improved git tree published, and it should be possible to directly apply most of the changes in it to lirc cvs (so far, almost solely coding style and whitespace changes to match the kernel, but also a few build fixes along the way). -- Jarod Wilson ja...@wi... |
From: Jarod W. <ja...@wi...> - 2007-09-01 05:48:10
Attachments:
PGP.sig
|
On Aug 30, 2007, at 16:54, Jarod Wilson wrote: > On Monday 27 August 2007 04:12:54 pm Jarod Wilson wrote: >> Hi Christoph (and list), >> >> On Saturday 25 August 2007 12:25:00 pm Christoph Bartelmus wrote: >>> Jarod Wilson "ja...@wi..." wrote: >>>> I've been wanting to get lirc included in Fedora kernels, and >>>> tonight, I finally got around to doing something significant about >>>> it, and would like people's feedback. >>> >>> Great! >>> If there is anything that you think should also go into LIRC CVS >>> (e.g. >>> getting rid of 2.4 compatibility), please send patches. >> >> I think there's actually quite a bit that should probably go into >> LIRC CVS, >> so things don't diverge too much. I'd actually like to open some >> discussion >> about how this would best work. Things we need to do for kernel >> inclusion: >> >> 1) run <kernel source>/scripts/checkpatch.pl over any patches to >> be applied >> to the kernel, reduce checkpatch output as much as possible. A lot >> of this >> is coding style changes, whitespace clean-ups, etc. >> >> 2) fixes for compile errors and warnings with latest linus kernel >> >> 3) flatten source tree so we have all bits in <kernel >> source>/drivers/input/lirc/, rather than each driver in its own >> sub-directory >> >> 4) drop as much stuff in '#if LINUX_VERSION_CODE' wrappers as >> possible. >> >> >> I presume the first two items would be easily merged into current >> LIRC CVS. >> Not so sure about whether or not flattening the source tree would be >> problematic, or how much of the '#if LINUX_VERSION_CODE' bits >> you'd want to >> drop, since we'd quickly lose compatibility with anything older than >> 2.6.19, iirc. What are your thoughts on all this? >> >> To get an idea of the work done thus far, check out: >> >> http://git.wilsonet.com/linux-2.6-lirc.git/ > > FWIW, I'm reworking all the patches to apply to both the git tree > and lirc cvs > right now, the idea being to push as much as possible back to lirc > cvs before > I start doing things like stripping out the compatibility for older > kernels. > > I talked with a buddy of mine over in v4l-dvb land, who had a few > suggestions > for how they do things for older kernels. The v4l-dvb hg tree still > builds > back to about 2.6.18, but has most of the compatibility stuff > abstracted out > into a compat header (which looks like it was sort of partially > done with > lirc's kcompat.h), and then before committing to the v4l-dvb git > tree for > Linus to pull, the compat stuff is stripped out in a scripted > fashion. I > think trying to achieve something similar would be optimal to help > prevent > diverging code. > > A bit more clean-up work, then I'll get this new-and-improved git tree > published, and it should be possible to directly apply most of the > changes in > it to lirc cvs (so far, almost solely coding style and whitespace > changes to > match the kernel, but also a few build fixes along the way). Okay, almost all the checkpatch cleanup work is done, all against a tree that still pretty much resembles lirc cvs (only drivers/* are included though, and they're under drivers/input/lirc/). So far as I know, it should be possible (and worthwhile) to apply each and every one of these patches to lirc cvs, though I should probably scrape them together into a more easily digestible format. http://git.wilsonet.com/lirc-cvs.git/ Note that the above does not yet build into the kernel, as I want to finish up any and all changes that can be easily fed back to lirc cvs first. This link more accurately depicts the bits that are for lirc, as the changelog is cluttered up with all sorts of other stuff, as I try to keep the tree in sync with the upstream kernel. http://git.wilsonet.com/lirc-cvs.git/?a=history;f=drivers/ input;h=24295f6d0a01304adb33c4103ae6a454477e2959;hb=7b86841b29bb2e22ebee 580ed4e1135dced2b328 -- Jarod Wilson ja...@wi... |
From: <li...@ba...> - 2007-09-02 08:05:25
|
Hi! Jarod Wilson "ja...@wi..." wrote: [...] > I think there's actually quite a bit that should probably go into LIRC CVS, > so things don't diverge too much. I'd actually like to open some discussion > about how this would best work. Things we need to do for kernel inclusion: > > 1) run <kernel source>/scripts/checkpatch.pl over any patches to be applied > to the kernel, reduce checkpatch output as much as possible. A lot of this > is coding style changes, whitespace clean-ups, etc. Is there a way to collect all these changes and get it as one patch file? This requires a careful review. E.g. in the lirc_parallel driver I noticed that the initialisation of tx_mask was removed, breaking the transmit functionality. > 2) fixes for compile errors and warnings with latest linus kernel This will have to go into LIRC CVS sooner or later anyway. > 3) flatten source tree so we have all bits in <kernel > source>/drivers/input/lirc/, rather than each driver in its own > sub-directory I think it should be easy to keep the current structure in LIRC CVS and convert to the other directory structure using a simple script. Makefiles will be different anyway. > 4) drop as much stuff in '#if LINUX_VERSION_CODE' wrappers as possible. Yes, 2.4 stuff can be dropped anyway. The idea of kcompat.h was to always use the latest kernel API and provide compatibility wrappers in kcompat.h for older kernels. Unfortunately this is not always possible. So the idea to remove the LINUX_VERSION_CODE stuff later by a script sounds like the way to go. Christoph |
From: Eric S. <sa...@sa...> - 2007-09-02 16:02:09
|
Christoph Bartelmus wrote: > Hi! > > Jarod Wilson "ja...@wi..." wrote: > [...] >> I think there's actually quite a bit that should probably go into LIRC CVS, >> so things don't diverge too much. I'd actually like to open some discussion >> about how this would best work. Things we need to do for kernel inclusion: >> >> 1) run <kernel source>/scripts/checkpatch.pl over any patches to be applied >> to the kernel, reduce checkpatch output as much as possible. A lot of this >> is coding style changes, whitespace clean-ups, etc. > > Is there a way to collect all these changes and get it as one patch > file? That would be easy, but why? Seems like somewhat smaller changes would be easier to manage & review... but hey, it's your CVS :) > This requires a careful review. E.g. in the lirc_parallel driver I > noticed that the initialisation of tx_mask was removed, breaking the > transmit functionality. Ugh, did I do that... yeah, you're right. That was not intentional. I wonder... it'd be more work on our part, but perhaps each patch could be split into *purely* cosmetic changes (whitespace) and then slightly more functional changes (don't initialize statics to 0, don't make assignments inside if statements...) - that way the binaries after the cosmetic-only changes could be verified to be the unchanged, and the resulting semi-function changes would be smaller, perhaps? >> 2) fixes for compile errors and warnings with latest linus kernel > > This will have to go into LIRC CVS sooner or later anyway. > >> 3) flatten source tree so we have all bits in <kernel >> source>/drivers/input/lirc/, rather than each driver in its own >> sub-directory > > I think it should be easy to keep the current structure in LIRC CVS and > convert to the other directory structure using a simple script. > Makefiles will be different anyway. > >> 4) drop as much stuff in '#if LINUX_VERSION_CODE' wrappers as possible. > > Yes, 2.4 stuff can be dropped anyway. > The idea of kcompat.h was to always use the latest kernel API and > provide compatibility wrappers in kcompat.h for older kernels. > Unfortunately this is not always possible. So the idea to remove the > LINUX_VERSION_CODE stuff later by a script sounds like the way to go. the unifdef tool can probably do this pretty easily. This probably depends more on how you plan to continue source control & development if/when there is an upstream version to keep in sync... -Eric |
From: Jarod W. <ja...@wi...> - 2007-09-02 20:25:23
|
On Sep 2, 2007, at 12:01 PM, Eric Sandeen wrote: > Christoph Bartelmus wrote: >> Hi! >> >> Jarod Wilson "ja...@wi..." wrote: >> [...] >>> I think there's actually quite a bit that should probably go into >>> LIRC CVS, >>> so things don't diverge too much. I'd actually like to open some >>> discussion >>> about how this would best work. Things we need to do for kernel >>> inclusion: >>> >>> 1) run <kernel source>/scripts/checkpatch.pl over any patches to >>> be applied >>> to the kernel, reduce checkpatch output as much as possible. A >>> lot of this >>> is coding style changes, whitespace clean-ups, etc. >> >> Is there a way to collect all these changes and get it as one patch >> file? > > That would be easy, but why? Seems like somewhat smaller changes > would > be easier to manage & review... but hey, it's your CVS :) I'll throw together both one big patch and a series of patches, basically one per driver + one for lirc_dev and the common headers. >> This requires a careful review. E.g. in the lirc_parallel driver I >> noticed that the initialisation of tx_mask was removed, breaking the >> transmit functionality. > > Ugh, did I do that... yeah, you're right. That was not intentional. Fixed in my git tree. I think that was the only one where initializations were removed, based upon a quick cursory inspection... > I wonder... it'd be more work on our part, but perhaps each patch > could be split into *purely* cosmetic changes (whitespace) and then > slightly more functional changes (don't initialize statics to 0, don't > make assignments inside if statements...) - that way the binaries > after > the cosmetic-only changes could be verified to be the unchanged, > and the > resulting semi-function changes would be smaller, perhaps? Ew, I'd rather not make that split, but... >>> 2) fixes for compile errors and warnings with latest linus kernel >> >> This will have to go into LIRC CVS sooner or later anyway. >> >>> 3) flatten source tree so we have all bits in <kernel >>> source>/drivers/input/lirc/, rather than each driver in its own >>> sub-directory >> >> I think it should be easy to keep the current structure in LIRC >> CVS and >> convert to the other directory structure using a simple script. >> Makefiles will be different anyway. >> >>> 4) drop as much stuff in '#if LINUX_VERSION_CODE' wrappers as >>> possible. >> >> Yes, 2.4 stuff can be dropped anyway. Okay, I'll work on stripping out all the 2.4 and earlier bits soonish. >> The idea of kcompat.h was to always use the latest kernel API and >> provide compatibility wrappers in kcompat.h for older kernels. >> Unfortunately this is not always possible. So the idea to remove the >> LINUX_VERSION_CODE stuff later by a script sounds like the way to go. > > the unifdef tool can probably do this pretty easily. Been meaning to poke at unifdef to figure out if it would indeed do all the stripping for us... > This probably > depends more on how you plan to continue source control & development > if/when there is an upstream version to keep in sync... That's one of the biggest questions I'd like to figure out. Not really sure what the best way about this is. If nothing else, I'm perfectly willing to be the in-kernel maintainer for lirc and do the leg work to keep the in-kernel and cvs lirc trees in sync. One way or another, it does seem like there's going to have to be two slightly different trees... --jarod |
From: <li...@ba...> - 2007-09-04 06:17:37
|
Hi! Eric Sandeen "sa...@sa..." wrote: [...] >>> 1) run <kernel source>/scripts/checkpatch.pl over any patches to be >>> applied to the kernel, reduce checkpatch output as much as possible. A lot >>> of this is coding style changes, whitespace clean-ups, etc. >> >> Is there a way to collect all these changes and get it as one patch >> file? > That would be easy, but why? Just because I don't want to download all 20 checkpatch patches for each driver file individually and apply it seperately. >> This requires a careful review. E.g. in the lirc_parallel driver I >> noticed that the initialisation of tx_mask was removed, breaking the >> transmit functionality. > Ugh, did I do that... yeah, you're right. That was not intentional. > > I wonder... it'd be more work on our part, but perhaps each patch > could be split into *purely* cosmetic changes (whitespace) and then > slightly more functional changes (don't initialize statics to 0, don't > make assignments inside if statements...) - that way the binaries after > the cosmetic-only changes could be verified to be the unchanged, and the > resulting semi-function changes would be smaller, perhaps? If somebody can do this work, great. If not, there is still diff -w. Christoph |
From: Jarod W. <ja...@wi...> - 2007-09-05 02:10:19
Attachments:
PGP.sig
|
On Sep 04, 2007, at 02:10, Christoph Bartelmus wrote: > Hi! > > Eric Sandeen "sa...@sa..." wrote: > [...] >>>> 1) run <kernel source>/scripts/checkpatch.pl over any patches to be >>>> applied to the kernel, reduce checkpatch output as much as >>>> possible. A lot >>>> of this is coding style changes, whitespace clean-ups, etc. >>> >>> Is there a way to collect all these changes and get it as one patch >>> file? > >> That would be easy, but why? > > Just because I don't want to download all 20 checkpatch patches for > each > driver file individually and apply it seperately. Here it is: http://wilsonet.com/jarod/lirc-checkpatch-cleanup.patch And on second thought, I'll just hold off on the one-for-each-driver rendition version for now... >>> This requires a careful review. E.g. in the lirc_parallel driver I >>> noticed that the initialisation of tx_mask was removed, breaking the >>> transmit functionality. > >> Ugh, did I do that... yeah, you're right. That was not intentional. >> >> I wonder... it'd be more work on our part, but perhaps each patch >> could be split into *purely* cosmetic changes (whitespace) and then >> slightly more functional changes (don't initialize statics to 0, >> don't >> make assignments inside if statements...) - that way the binaries >> after >> the cosmetic-only changes could be verified to be the unchanged, >> and the >> resulting semi-function changes would be smaller, perhaps? > > If somebody can do this work, great. If not, there is still diff -w. Hm... I'm actually on vacation this week, that feels an awful lot like work... ;) However, it its really desired after taking a look at the patch above, we can get it done. Not sure how much -w is going to help with the volume of lines that were broken up to fit 80 chars... Ah, also, there are two code changes also in the patch: 1) convert from using strcpy to using strncpy when copying i2c driver names: http://git.wilsonet.com/lirc-cvs.git/? a=commitdiff;h=a8270f175eb5da01e527a12eacbb18f437c7c588 2) in commandir.c, there was a line with an assignment in an if that looked to me more like it should be a test. Excerpt below: ---- break; } } - - if ((changeType!=cNothing)&&(equalsign=0)) err("No device specified"); - if (changeType==cNothing) err("Unknown command"); + + /* that used to be an assignment of equalsign, which I *think* was + * in error... -jarod */ + if ((changeType != cNothing) && (equalsign == 0)) + err("No device specified"); + if (changeType == cNothing) + err("Unknown command"); exit: kfree(local_buffer); ---- HTH, -- Jarod Wilson ja...@wi... |
From: <li...@ba...> - 2007-09-09 20:46:09
|
Hi! Jarod Wilson "ja...@wi..." wrote: [...] > Here it is: > > http://wilsonet.com/jarod/lirc-checkpatch-cleanup.patch After a review I have made a second version of this patch (against current CVS): http://lirc.sourceforge.net/software/snapshots/lirc-checkpatch-cleanup.patch2 I hope I have spotted all errors. At least it compiles for me now. As there are a lot of changes and I probably have overseen some problems I'd like someone else to make a second review before I put it into CVS. I think the code that uses alloc_status, mem_faiure, etc. should be rewritten to use goto or similar. At least the changes to avoid assignments in if conditions were not vaild, so I had to undo some of them. [...] > However, it its really desired after taking a look at the patch > above, we can get it done. Not sure how much -w is going to help with > the volume of lines that were broken up to fit 80 chars... > > Ah, also, there are two code changes also in the patch: > > 1) convert from using strcpy to using strncpy when copying i2c driver > names: I changed that to use strlcpy. It's in CVS already. > 2) in commandir.c, there was a line with an assignment in an if that > looked to me more like it should be a test. Excerpt below: In CVS. Christoph |
From: Jarod W. <ja...@wi...> - 2007-09-11 03:28:47
Attachments:
PGP.sig
|
On Sep 09, 2007, at 16:45, Christoph Bartelmus wrote: > Jarod Wilson "ja...@wi..." wrote: > [...] >> Here it is: >> >> http://wilsonet.com/jarod/lirc-checkpatch-cleanup.patch > > After a review I have made a second version of this patch (against > current CVS): > http://lirc.sourceforge.net/software/snapshots/lirc-checkpatch- > cleanup.patch2 > > I hope I have spotted all errors. At least it compiles for me now. Crap, my apologies, I really shoulda done at least that much first... (everything is compiling in the Fedora kernel right now, but that patch looks a lot different than lirc cvs plus my first patch there now). > As there are a lot of changes and I probably have overseen some > problems > I'd like someone else to make a second review before I put it into > CVS. I haven't yet had time to go over it in detail, but will try to do so as soon as possible. > I think the code that uses alloc_status, mem_faiure, etc. should be > rewritten to use goto or similar. At least the changes to avoid > assignments in if conditions were not vaild, so I had to undo some of > them. Yeah, it actually crossed my mind that the way I'd done it there wasn't so hot, since there's potential to keep trying allocs after one has failed. Just didn't get around to it -- I was actually out of town on vacation last week. That's my excuse, anyhow. ;) I'll take a crack at re-writing those bits in a saner fashion after I go over your rendition of the checkpatch cleanups, unless someone else beats me to it. > [...] >> However, it its really desired after taking a look at the patch >> above, we can get it done. Not sure how much -w is going to help with >> the volume of lines that were broken up to fit 80 chars... >> >> Ah, also, there are two code changes also in the patch: >> >> 1) convert from using strcpy to using strncpy when copying i2c driver >> names: > > I changed that to use strlcpy. It's in CVS already. I suppose that works too. Was there any particular objection to strncpy? >> 2) in commandir.c, there was a line with an assignment in an if that >> looked to me more like it should be a test. Excerpt below: > > In CVS. Cool. -- Jarod Wilson ja...@wi... |
From: Eric S. <sa...@sa...> - 2007-09-11 03:33:52
|
Jarod Wilson wrote: >>> Ah, also, there are two code changes also in the patch: >>> >>> 1) convert from using strcpy to using strncpy when copying i2c driver >>> names: >> I changed that to use strlcpy. It's in CVS already. > > I suppose that works too. Was there any particular objection to strncpy? makes sure it's null-terminated even if it's too long to fit in the destination, right... good plan. -Eric |
From: Jarod W. <ja...@wi...> - 2007-09-12 04:50:07
Attachments:
PGP.sig
|
On Sep 10, 2007, at 23:28, Jarod Wilson wrote: > On Sep 09, 2007, at 16:45, Christoph Bartelmus wrote: >> Jarod Wilson "ja...@wi..." wrote: >> [...] >>> Here it is: >>> >>> http://wilsonet.com/jarod/lirc-checkpatch-cleanup.patch >> >> After a review I have made a second version of this patch (against >> current CVS): >> http://lirc.sourceforge.net/software/snapshots/lirc-checkpatch- >> cleanup.patch2 >> >> I hope I have spotted all errors. At least it compiles for me now. > > Crap, my apologies, I really shoulda done at least that much > first... (everything is compiling in the Fedora kernel right now, > but that patch looks a lot different than lirc cvs plus my first > patch there now). > >> As there are a lot of changes and I probably have overseen some >> problems >> I'd like someone else to make a second review before I put it into >> CVS. > > I haven't yet had time to go over it in detail, but will try to do > so as soon as possible. Done. http://wilsonet.com/jarod/lirc-checkpatch-cleanup-v3.patch The differences from your version 2: 1) rediffed against latest cvs head (minor conflicts due to addition of FINTEK devices) 2) minor tweaks to #if LINUX_VERSION_CODE bits so they all use < instead of mixing <, >, <=, etc. 3) remove extraneous ')' from lirc_igorplususb.c (line 336) 4) remove extra leading space from 'error:' label in lirc_mceusb.c (line 883) 5) trim lines to under 80 char in lirc_mceusb2.c (lines 80 & 81) I think that's all I saw, save reworking the bits where there are still assignments in if statements, which I'll try to get to soonish. -- Jarod Wilson ja...@wi... |
From: <li...@ba...> - 2007-09-16 22:34:39
|
Hi! Jarod Wilson "ja...@wi..." wrote: [...] >>> As there are a lot of changes and I probably have overseen some >>> problems >>> I'd like someone else to make a second review before I put it into >>> CVS. >> >> I haven't yet had time to go over it in detail, but will try to do >> so as soon as possible. > Done. > > http://wilsonet.com/jarod/lirc-checkpatch-cleanup-v3.patch I put a new version online: http://lirc.sourceforge.net/software/snapshots/lirc-checkpatch-cleanup-v4.patch I found one more point where the original code was broken by the clean- up, so maybe we should have one more review round. Christoph |
From: Jarod W. <ja...@wi...> - 2007-09-17 20:44:44
|
On Sunday 16 September 2007 06:33:00 pm Christoph Bartelmus wrote: > Hi! > > Jarod Wilson "ja...@wi..." wrote: > [...] > > >>> As there are a lot of changes and I probably have overseen some > >>> problems > >>> I'd like someone else to make a second review before I put it into > >>> CVS. > >> > >> I haven't yet had time to go over it in detail, but will try to do > >> so as soon as possible. > > > > Done. > > > > http://wilsonet.com/jarod/lirc-checkpatch-cleanup-v3.patch > > I put a new version online: > http://lirc.sourceforge.net/software/snapshots/lirc-checkpatch-cleanup-v4.p >atch > > I found one more point where the original code was broken by the clean- > up, so maybe we should have one more review round. Gah, sorry, I actually saw that bit after the fact, and in my local tree (and what I just put in the latest fedora kernel last week), I rewrote that section and most of the similar ones to use goto instead of all the nested ifs. I'd posted another email here asking if you thought a sample snippet using goto looked sane, but hadn't heard back yet, so I'd not posted the full rendition. I'll just post a -v5 cleanup patch carrying those changes later tonight. Regardless, I agree one more review round, ideally from someone who isn't you or me, would be good. -- Jarod Wilson ja...@wi... |
From: Jarod W. <ja...@wi...> - 2007-09-19 03:44:44
Attachments:
PGP.sig
|
On Sep 17, 2007, at 16:44, Jarod Wilson wrote: > On Sunday 16 September 2007 06:33:00 pm Christoph Bartelmus wrote: >> I put a new version online: >> http://lirc.sourceforge.net/software/snapshots/lirc-checkpatch- >> cleanup-v4.p >> atch >> >> I found one more point where the original code was broken by the >> clean- >> up, so maybe we should have one more review round. > > Gah, sorry, I actually saw that bit after the fact, and in my local > tree (and > what I just put in the latest fedora kernel last week), I rewrote that > section and most of the similar ones to use goto instead of all the > nested > ifs. I'd posted another email here asking if you thought a sample > snippet > using goto looked sane, but hadn't heard back yet, so I'd not > posted the full > rendition. I'll just post a -v5 cleanup patch carrying those > changes later > tonight. A bit tardy, wound up having a busy evening last night... But here's a 5th version, complete with converting all the remaining assignments in tests over to use goto's. http://wilsonet.com/jarod/lirc-checkpatch-cleanup-v5.patch -- Jarod Wilson ja...@wi... |
From: <li...@ba...> - 2007-09-27 20:33:15
|
Hi! Jarod Wilson "ja...@wi..." wrote: [...] > A bit tardy, wound up having a busy evening last night... But here's > a 5th version, complete with converting all the remaining assignments > in tests over to use goto's. > > http://wilsonet.com/jarod/lirc-checkpatch-cleanup-v5.patch I have now checked in v4 of the patch in the hope that all issues have been discovered already. Looking at v5 is the next thing on my todo list. Christoph |
From: <li...@ba...> - 2007-09-20 19:13:43
|
Hi! Jarod Wilson "ja...@wi..." wrote: [...] >> I found one more point where the original code was broken by the clean- >> up, so maybe we should have one more review round. [...] > later tonight. Regardless, I agree one more review round, ideally from > someone who isn't you or me, would be good. Any volunteers? Christoph |
From: Jarod W. <ja...@wi...> - 2007-09-12 19:21:37
Attachments:
lirc_imon-no-assignments-in-tests.patch
|
On Sunday 09 September 2007 04:45:00 pm Christoph Bartelmus wrote: > I think the code that uses alloc_status, mem_faiure, etc. should be =A0 > rewritten to use goto or similar. At least the changes to avoid =A0 > assignments in if conditions were not vaild, so I had to undo some of =A0 > them. Does the attached patch snippet make sense as a route to go? If so, I'll ap= ply=20 the same treatment to other remaining similar sections. =2D-=20 Jarod Wilson ja...@wi... |
From: Jarod W. <ja...@wi...> - 2007-09-12 19:31:36
|
On Wednesday 12 September 2007 03:21:31 pm Jarod Wilson wrote: > On Sunday 09 September 2007 04:45:00 pm Christoph Bartelmus wrote: > > I think the code that uses alloc_status, mem_faiure, etc. should be =C2= =A0 > > rewritten to use goto or similar. At least the changes to avoid =C2=A0 > > assignments in if conditions were not vaild, so I had to undo some of = =C2=A0 > > them. > > Does the attached patch snippet make sense as a route to go? If so, I'll > apply the same treatment to other remaining similar sections. Ugh, scratch that version... Instead of a goto inside every if, after all t= he=20 ifs, test for alloc_status !=3D SUCCESS, and if so, then goto. Much cleaner. =2D-=20 Jarod Wilson ja...@wi... |
From: Jarod W. <ja...@wi...> - 2007-09-12 19:35:16
|
On Wednesday 12 September 2007 03:31:31 pm Jarod Wilson wrote: > On Wednesday 12 September 2007 03:21:31 pm Jarod Wilson wrote: > > On Sunday 09 September 2007 04:45:00 pm Christoph Bartelmus wrote: > > > I think the code that uses alloc_status, mem_faiure, etc. should be = =C2=A0 > > > rewritten to use goto or similar. At least the changes to avoid =C2=A0 > > > assignments in if conditions were not vaild, so I had to undo some of= =C2=A0 > > > them. > > > > Does the attached patch snippet make sense as a route to go? If so, I'll > > apply the same treatment to other remaining similar sections. > > Ugh, scratch that version... Instead of a goto inside every if, after all > the ifs, test for alloc_status !=3D SUCCESS, and if so, then goto. Much > cleaner. Erm, un-scratch that. Then we'd try doing all the extra allocs even after o= ne=20 failed. Bah. =2D-=20 Jarod Wilson ja...@wi... |
From: <li...@ba...> - 2007-09-20 19:13:43
|
Hi! Jarod Wilson "ja...@wi..." wrote: > On Sunday 09 September 2007 04:45:00 pm Christoph Bartelmus wrote: >> I think the code that uses alloc_status, mem_faiure, etc. should be =A0 >> rewritten to use goto or similar. At least the changes to avoid =A0 >> assignments in if conditions were not vaild, so I had to undo some of =A0 >> them. > Does the attached patch snippet make sense as a route to go? If so, I'll > apply the same treatment to other remaining similar sections. Sorry for the late reply. I think we should get rid of the alloc_status variable altogether. Instead you can add some labels with proper names and use a goto with =20 the according label, e.g. goto kmalloc_imon_context_failed;. Christoph |
From: Jarod W. <ja...@wi...> - 2007-09-20 21:46:55
|
On Thursday 20 September 2007 03:07:00 pm Christoph Bartelmus wrote: > Hi! > > Jarod Wilson "ja...@wi..." wrote: > > On Sunday 09 September 2007 04:45:00 pm Christoph Bartelmus wrote: > >> I think the code that uses alloc_status, mem_faiure, etc. should be =A0 > >> rewritten to use goto or similar. At least the changes to avoid =A0 > >> assignments in if conditions were not vaild, so I had to undo some of = =A0 > >> them. > > > > Does the attached patch snippet make sense as a route to go? If so, I'll > > apply the same treatment to other remaining similar sections. > > Sorry for the late reply. > I think we should get rid of the alloc_status variable altogether. > Instead you can add some labels with proper names and use a goto with > the according label, e.g. goto kmalloc_imon_context_failed;. I presume that means you'd also like to drop the mem_failure variable in=20 things like lirc_igorplugusb as well? Certainly can be done, but we'll lose= a=20 bit of debugging info from the printk before returning -ENOMEM (at least in= =20 the igor code). Speaking of the igor code... Upon looking at the mem_failure switch in=20 lirc_igorplugusb.c in greater detail, it flat-out looks wrong. You hit the= =20 switch, then only free a tiny portion of memory allocated, and only=20 return -ENOMEM if the mem_failure was case 1. Otherwise, we continue on our= =20 way... Here's a patched up implementation of the igor code that still uses the=20 mem_failure variable to track where we failed, but does so without a switch= ,=20 and I believe correctly cleans up memory: http://wilsonet.com/jarod/lirc_igorplugusb.c If you do indeed want to drop mem_failure altogether, simple enough to rip = it=20 out of that version. Either way, let me know your thoughts, and I can give= =20 the same treatment to the other drivers with similar code. =2D-=20 Jarod Wilson ja...@wi... |