From: <li...@ba...> - 2007-09-30 10:04:57
|
Hi! Jarod Wilson "ja...@wi..." wrote: [...] >> 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 > things like lirc_igorplugusb as well? Certainly can be done, but we'll lose > a bit of debugging info from the printk before returning -ENOMEM (at least > in the igor code). Let's keep it like it is now. We will have to see if this alloc_status variable will go through peer review. If not we will have to change it later. > Speaking of the igor code... Upon looking at the mem_failure switch in > lirc_igorplugusb.c in greater detail, it flat-out looks wrong. You hit the > switch, then only free a tiny portion of memory allocated, and only > return -ENOMEM if the mem_failure was case 1. Otherwise, we continue on our > way... I don't see the problem. This is a fall-through switch, so it should work fine. Based on your v5 patch, I have just put a new version to CVS. I fixed some minor glitches in the patch and some bugs in the original code. Please compare your latest version with the current version in CVS. Apart from some more fixes in CVS, it should be the same now. If anything is still missing, please let me know. Christoph |
From: Jon S. <jon...@gm...> - 2007-09-30 14:23:18
|
On 30 Sep 2007 12:04:00 +0200, Christoph Bartelmus <li...@ba...> wrote: > Hi! > > Jarod Wilson "ja...@wi..." wrote: > [...] > >> 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 > > things like lirc_igorplugusb as well? Certainly can be done, but we'll lose > > a bit of debugging info from the printk before returning -ENOMEM (at least > > in the igor code). > > Let's keep it like it is now. We will have to see if this alloc_status > variable will go through peer review. If not we will have to change it > later. It might make it but I wouldn't count on it. Look at the other drivers in the kernel. I can't find any using alloc status variables, they all use goto. You can put the debug info in by using a printk before each goto. The printks are often put in a DBG_PRINT type macro that can make then disappear. The goto style is modeled after exception handling. The goto equates to a throw() and the error processing is the catch(). The idea is to move the error recovery out of the main flow of code making the main flow easier to understand. I have seen some extremely complex code made many times simpler by switching to this method. One case has about 25 nested ifs which all disappeared after switching to goto. -- Jon Smirl jon...@gm... |
From: <li...@ba...> - 2007-09-30 15:19:45
|
Hi! Jon Smirl "jon...@gm..." wrote: [getting rid of the alloc_status variables] > It might make it but I wouldn't count on it. Look at the other drivers > in the kernel. I can't find any using alloc status variables, they all > use goto. > > You can put the debug info in by using a printk before each goto. The > printks are often put in a DBG_PRINT type macro that can make then > disappear. Full ACK. If someone submits a patch, I'll be happy to include it. Christoph |
From: Jarod W. <ja...@wi...> - 2007-10-01 23:35:20
Attachments:
PGP.sig
|
On Sep 30, 2007, at 11:19, Christoph Bartelmus wrote: > Hi! > > Jon Smirl "jon...@gm..." wrote: > > [getting rid of the alloc_status variables] > >> It might make it but I wouldn't count on it. Look at the other >> drivers >> in the kernel. I can't find any using alloc status variables, they >> all >> use goto. >> >> You can put the debug info in by using a printk before each goto. The >> printks are often put in a DBG_PRINT type macro that can make then >> disappear. > > Full ACK. > If someone submits a patch, I'll be happy to include it. I'm on it, unless someone else really wants to do it, and/or has already started on it. -- Jarod Wilson ja...@wi... |
From: Jarod W. <ja...@wi...> - 2007-10-01 23:34:49
Attachments:
PGP.sig
|
On Sep 30, 2007, at 06:04, Christoph Bartelmus wrote: >> Speaking of the igor code... Upon looking at the mem_failure >> switch in >> lirc_igorplugusb.c in greater detail, it flat-out looks wrong. You >> hit the >> switch, then only free a tiny portion of memory allocated, and only >> return -ENOMEM if the mem_failure was case 1. Otherwise, we >> continue on our >> way... > > I don't see the problem. This is a fall-through switch, so it should > work fine. Blah. Once again, I wrote too soon w/o carefully re-reading. In my mind, I inserted break's all over the place in the switch. Sorry 'bout that. > Based on your v5 patch, I have just put a new version to CVS. > I fixed some minor glitches in the patch and some bugs in the original > code. > Please compare your latest version with the current version in CVS. > Apart from some more fixes in CVS, it should be the same now. If > anything is still missing, please let me know. I'll try to give it a look either tonight or tomorrow and see what I can see (hopefully, nothing!). -- Jarod Wilson ja...@wi... |
From: Eric S. <sa...@sa...> - 2007-09-02 22:09:30
|
Jarod Wilson wrote: >> 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... Why is that? I'd think that cvs could be structured so that in cvs, foo/bar/whatever/kerneldrivers/* exactly matches what's in the kernel.org tree at drivers/input/lirc/ - no? I think the dvb guys were able to do this... and with kbuild building out-of-tree modules so nicely, I think it could work. I'd be happy to do some kbuild/makefile work if needed, I've messed with that a lot for other projects... -Eric |
From: Jarod W. <ja...@wi...> - 2007-09-02 23:09:35
|
On Sep 2, 2007, at 6:09 PM, Eric Sandeen wrote: > Jarod Wilson wrote: > >>> 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... > > > Why is that? I'd think that cvs could be structured so that in cvs, > foo/bar/whatever/kerneldrivers/* exactly matches what's in the > kernel.org tree at drivers/input/lirc/ - no? > > I think the dvb guys were able to do this... and with kbuild building > out-of-tree modules so nicely, I think it could work. > > I'd be happy to do some kbuild/makefile work if needed, I've messed > with > that a lot for other projects... I talked to one of the dvb guys at length about it a few days ago. They actually do have slightly different trees for ongoing development and maintenance, maintained in mercurial, then a tree that gets published via git for linus to pull from. The hg tree maintains backwards compatibility with back to about 2.6.18, iirc, while the git tree has all the compatibility bits stripped out. The compat bits are clean enough though that as I understand it, its a simple scripting job to pull from hg, strip out the compat bits and shove them into git. The other issue is lirc user-space vs. lirc kernel-space. For the dvb case, dvb-apps was split into its own self- contained repo, apart from the kernel drivers. Could certainly do the same for lirc, though I'm not sure what the benefit would be vs. maintaining everything in cvs as the canonical upstream, but working it into a form where we can easily strip the compat bits and import just the drivers into a git tree via scripting. At least, that's the way I'd lean toward right now. So really, not so much two different trees, as one canonical upstream lirc tree in cvs, then a version stripped and imported into git. I think that's what I had in mind earlier, but wasn't so clear about -- in this version, we certainly *would* want some kbuild/makefile work done so that the same bits could be built both out of tree and in-kernel w/o having separate makefiles. Make more sense? --jarod |
From: Matthias S. <zz...@ge...> - 2007-09-03 16:20:05
|
On Montag, 3. September 2007, Jarod Wilson wrote: > On Sep 2, 2007, at 6:09 PM, Eric Sandeen wrote: > > I'd be happy to do some kbuild/makefile work if needed, I've messed > > with > > that a lot for other projects... > > I talked to one of the dvb guys at length about it a few days ago. > They actually do have slightly different trees for ongoing > development and maintenance, maintained in mercurial, then a tree > that gets published via git for linus to pull from. The hg tree > maintains backwards compatibility with back to about 2.6.18, iirc, > while the git tree has all the compatibility bits stripped out. The > compat bits are clean enough though that as I understand it, its a > simple scripting job to pull from hg, strip out the compat bits and > shove them into git. The other issue is lirc user-space vs. lirc > kernel-space. For the dvb case, dvb-apps was split into its own self- > contained repo, apart from the kernel drivers. Could certainly do the > same for lirc, though I'm not sure what the benefit would be vs. > maintaining everything in cvs as the canonical upstream, but working > it into a form where we can easily strip the compat bits and import > just the drivers into a git tree via scripting. At least, that's the Well, I guess it makes no big difference splitting the lirc source tree into two parts. But I always hated the way lirc handled the Makefiles in drivers subdir. I suggest to make configure no longer touch drivers subdir - and then just deleting all makefiles and start from scratch like kernel doc describes for external modules. A make drivers could trigger the modules stuff (simply target) .PHONY: drivers drivers: make -C drivers Then a make in lirc-dir will build just the userspace stuff. ... or having a look at kernel-sources if lirc is enabled and if not calling make drivers - but I would like to avoid that if possible. About the compat stuff: I also follow dvb development so I think their approach is sane and easy maintainable. My impression is: A script strips: * All inclusions of compat.h. * All #if 0 ... #endif sequences * All #if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,20) ... #else ... #endif sequences Example: #if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,19) static irqreturn_t flexcop_pci_isr(int irq, void *dev_id, struct pt_regs *regs) #else static irqreturn_t flexcop_pci_isr(int irq, void *dev_id) #endif That will be reduced to just static irqreturn_t flexcop_pci_isr(int irq, void *dev_id) for the kernel export git tree. Only problem for lirc was (had not browsed in detail through your kerne-patch): It had so much compile-time parameters. Matthias -- Matthias Schwarzott (zzam) |
From: <li...@ba...> - 2007-09-04 06:17:39
|
Hi! Matthias Schwarzott "zz...@ge..." wrote: [...] > I suggest to make configure no longer touch drivers subdir - and then just > deleting all makefiles and start from scratch like kernel doc describes for > external modules. > A make drivers could trigger the modules stuff (simply target) > ..PHONY: drivers > drivers: > make -C drivers > > Then a make in lirc-dir will build just the userspace stuff. I suggest not to touch the structure like it is now. Just because I think we should not waste time on changing things that will be descarded some time in the (near?) future anyway. The makefiles are working fine as they are now. Furthermore there is already the special configure --driver option "userspace" which will allow you to compile only userspace stuff. [...] > About the compat stuff: I also follow dvb development so I think their > approach is sane and easy maintainable. > My impression is: > A script strips: > * All inclusions of compat.h. > * All #if 0 ... #endif sequences > * All #if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,20) ... #else ... #endif > sequences Yes, I think this should be the way to go. With this approach there is only one source that has to be maintained. The in-kernel version should be automatically generated from that. [...] > Only problem for lirc was (had not browsed in detail through your > kerne-patch): It had so much compile-time parameters. Yes, that's right. There is still some work to do. But many of the compile-time parameters only set some default that can be overridden by module parameters anyway, so don't get scared off by them. Christoph |
From: Jarod W. <ja...@wi...> - 2007-09-07 03:09:56
Attachments:
PGP.sig
|
On Sep 04, 2007, at 02:16, Christoph Bartelmus wrote: > Matthias Schwarzott "zz...@ge..." wrote: [...] >> About the compat stuff: I also follow dvb development so I think >> their >> approach is sane and easy maintainable. >> My impression is: >> A script strips: >> * All inclusions of compat.h. >> * All #if 0 ... #endif sequences >> * All #if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,20) ... >> #else ... #endif >> sequences > > Yes, I think this should be the way to go. With this approach there is > only one source that has to be maintained. The in-kernel version > should > be automatically generated from that. Just started poking at unifdef... Its definitely a piece of cake to script up stripping out the compat bits with unifdef. I'm not seeing a direct way to immediately strip things, but if everything follows the same format '#if LINUX_VERSION_CODE < KERNEL_VERSION(2, 6, something less than latest)', then a quick sed to strip the '< KERNEL_VERSION()' portion and 'unifdef -DLINUX_VERSION_CODE=0' does the trick. For example: $ cat lirc_atiusb.c | sed -e 's/< KERNEL_VERSION.*//g' | unifdef - DLINUX_VERSION_CODE=0 Diffing the original file with the output from that string looks sane enough, anyhow. -- Jarod Wilson ja...@wi... |
From: Jon S. <jon...@gm...> - 2007-09-03 00:24:19
|
On 8/25/07, Jarod Wilson <ja...@wi...> wrote: > 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 Some random comments. MOD_IN_USE, MOD_INC_USE_COUNT and set_use_inc and not the way the current kernel does ref counting. Some of doesn't look like it is in 2.4 ifdefs. Use the compat.h concept to hide this. This is normally done with gotos, it generates much smaller code. I don't think these mem_failure variables are going to fly, but I could be wrong. + if (!(plugin = kmalloc(sizeof(struct lirc_plugin), GFP_KERNEL))) { + mem_failure = 2; + } else if (!(rbuf = kmalloc(sizeof(struct lirc_buffer), GFP_KERNEL))) { + mem_failure = 3; + } else if (lirc_buffer_init(rbuf, sizeof(lirc_t), LIRCBUF_SIZE)) { + mem_failure = 4; + } else if (!(ir->buf_in = usb_buffer_alloc(dev, maxp, GFP_ATOMIC, &ir->dma_in))) { + mem_failure = 5; + } else if (!(ir->urb_in = usb_alloc_urb(0, GFP_KERNEL))) { + mem_failure = 7; This is normally done with structure initializers + ir->p = plugin; + ir->devnum = devnum; + ir->usbdev = dev; + ir->len_in = maxp; + ir->connected = 0; + + ir->lircdata=PULSE_MASK; + ir->is_pulse=0; Have all of these drivers been checked for endian problems on non-x86 boxes? Personally I write code like this so that there are only one LOCK/UNLOCK in each routine. Then grep can be used to locate locking bugs. + IRUNLOCK; + MOD_DEC_USE_COUNT; + return -EIO; + } + } + ir->connected = 1; + } + IRUNLOCK; + + return SUCCESS; +} ret = SUCCESS + ret = -EIO; + MOD_DEC_USE_COUNT; // what to do with this? + goto exit + } + } + ir->connected = 1; + } exit: + IRUNLOCK; + + return ret; +} Defines for the magic numbers, people will definitely complain about that This is a problem is a lot of drivers. + send_packet(oep, 0x8004, init1); + send_packet(oep, 0x8007, init2); The whole bt829 driver looks like magic to me, no comments about what is going on and magic numbers used everywhere. Safer to use sizeof instead of repeating the 63 + char buf[63], name[128]=""; + if (dev->descriptor.iManufacturer + && usb_string(dev, dev->descriptor.iManufacturer, buf, 63) > 0) Structure initializers + atir_plugin.minor = -1; + atir_plugin.code_length = 8; + atir_plugin.sample_rate = 10; + atir_plugin.data = 0; + atir_plugin.add_to_buf = atir_add_to_buf; + atir_plugin.set_use_inc = atir_set_use_inc; + atir_plugin.set_use_dec = atir_set_use_dec; Structure initializer +static inline void init_irctl(struct irctl *ir) +{ + memset(&ir->p, 0, sizeof(struct lirc_plugin)); + sema_init(&ir->buffer_sem, 1); + ir->p.minor = NOPLUG; + + ir->tpid = -1; + ir->t_notify = NULL; + ir->t_notify2 = NULL; + ir->shutdown = 0; + ir->jiffies_to_wait = 0; + + ir->open = 0; + ir->attached = 0; +} Get rid of everything DEVFS Are all of the drivers using sysfs consistently? lirc_buffer_unlock, lirc_buffer_clear, etc appear to be building a SMP safe FIFO. There should be a generalized implementation of this in the kernel already, like the list macros. What about kfifo.h? Is the code clean with the sparse tool? http://www.kernel.org/pub/software/devel/sparse/ #defines for these constants, use the name to tell us what they are ir_attach + switch(addr) + { + case 0x64: They are all copied again + static const int probe[] = { + 0x1a, /* Hauppauge IR external */ + 0x18, /* Hauppauge IR internal */ + 0x71, /* Hauppauge IR (PVR150) */ + 0x4b, /* PV951 IR */ + 0x64, /* Pixelview IR */ + 0x30, /* KNC ONE IR */ + 0x6b, /* Adaptec IR */ The names of the LOCK macros vary from driver to driver lirc_it87.c, init_hardware() inb/outb are terribly slow, is this going to mess with IRQ latency? Is init_lirc_timer(void) going to loop inside the kernel for upto 1 second reading an IO port? -- Jon Smirl jon...@gm... |
From: Jarod W. <ja...@wi...> - 2007-09-05 02:55:42
Attachments:
PGP.sig
|
On Sep 02, 2007, at 20:24, Jon Smirl wrote: > On 8/25/07, Jarod Wilson <ja...@wi...> wrote: >> 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 > > Some random comments. Hey Jon, just wanted to say thank you for the notes, they'll be put to good use. At this point, we've not really even begun to do any code review, just working on getting the bits shoehorned into a kernel build and working through coding style changes to make checkpatch.pl happy. -- Jarod Wilson ja...@wi... |
From: Alec L. <lea...@gm...> - 2015-06-30 15:52:19
|
On 30/06/15 16:45, Werner Mahr wrote: > Hi, > > I would like to ask, if I can get assistance for debugging the imon-mod from > kernel sources. > Basically, this is a kernel problem and as such perhaps the kernel mailing list is an alternative way to get support (?) I havn't too much experience in this. However, you should be able to track this down by simple modifying/adding debug output. The first thing to do is to establish the build cycle. Rebuilding kernel modules is actually simple and not least fast. There is an example in the lirc sources (drivers/lirc_wpc8769l); google will bring you more. Using modprobe(1) you should be able to reload the driver for testing. There is some kernel html documentation on the input subsystem on https://www.kernel.org/doc/htmldocs/media_api/lirc_dev.html HTH Cheers! --alec |
From: Alec L. <lea...@gm...> - 2015-06-30 19:57:42
|
On 30/06/15 18:13, Werner Mahr wrote: > That stuff isn't the problem, it's more the interpretation of the output. As > imon had it's initial support in lirc, and communication with devices > shouldn't has changed since then, I hoped someone could help me with that > point. > > I added already debug-output for what comes from and goes to the device. > The kernel module has basically three interfaces. The low-level interrupt handlers was historically a lirc concern, but has since the move to the kernel moved out of sight since long. The interface to the /dev/input subsystem is new and more or less unknown to me (but not to others on this list). The interface to lirc is the data flowing in the /dev/lirc[0-9] devices. This is documented in the lirc.4 and lirc-ioctl.4. This is basically stable since "long ago". I cannot say anything about the debug output, sorry. I your situation I would consider using mode2 to get not only the debug output but also the regular data sent to /dev/lirc0 (if any). I have recently taken part in a thread on the mythtv mailing list [1]. This boiled down to a probable bug in the usb driver. Given the message, could this be something similar? Cheers! --alec [1] mhttp://lists.mythtv.org/pipermail/mythtv-users/2015-June/379616.html |