Andy Pugh wrote: > On 5 May 2010 03:34, Sebastian Kuzminsky <[email protected]> wrote: > >> it's lacking documentation (in the hostmot2.9 man page), >> > > Does that go in the .patch too, or is there another submission path? >
In the same patch is definitely preferred. It's awkward to have functionality without documentation (or documentation without the described functionality!) in the repository, even for a short while. We prefer each commit to be able to stand on its own as much as possible. >> In hm2_tp_pwmgen_t, I think there's no need to make the >> "hm2_tp_pwmgen_module_global_t *hal" dynamically allocated, or even a >> separate struct. I'd just put the struct right inside the hm2_tp_pwmgen_t. >> > > At that point I think I was blindly following pwmgen, which presumably > is the way it is to support PDM too. > You're right, I did do that for the original hm2 pwmgen. And I have the resulting memory leak too. Maybe I should have reviewed my own patch before committing it... Hold on, i'll fix it now. Ok here's the commit: <http://git.linuxcnc.org/gitweb?p=emc2.git;a=commitdiff;h=2fa0df4ddf37651b6a303ca0ba66c38fcbe5c2d2> >> hm2_tp_pwmgen_parse_md() leaks memory in the error paths. >> > > Can you elaborate? I have never needed to consider memory leaks > before. I guess that you are saying that memory is malloc-ed but not > freed in the case of an error? > That's it exactly. Imagine you are an angry and spiteful god, and you're designing the universe to foil the mortal Sebastian Kuzminsky - how could you inconvenience him the most? You could let the hal_malloc at pwmgen.c:385 succeed, and then make the one on 393 fail, so that the function jumps to fail0 and returns without freeing the memory allocated on line 385. >> hm2->tp_pwmgen.enable_reg is used both as a TRAM register and as a >> non-TRAM register, which seems like a bug. >> > > It's a bit of an odd animal that register. As far as I can make out > bit0 is write-only, bit 1 is read-only and bits 2-31 are unused. I > think I have it as TRAM read (for the fault bit) and conventional > write. On reflection that probably doesn't make much sense. > The problem is that you first kmalloc memory for enable_reg, then pass enable_reg to hm2_register_tram_write_region(). That function allocates more memory and assigns it to the passed-in pointer-pointer. So the kmalloced buffer is lost, and later when you kfree enable_reg in hm2_tp_pwmgen_cleanup() it'll segfault. But note that the tram memory is not actually allocated until later, in hm2_allocate_tram_regions(), because we need all sub-drivers (like pwmgen, and tp_pwmgen, etc) to register how much memory they want first. So initialization of tram regions can't happen in the parse_md function along with all the other initialization, it needs to be delayed until hm2_*_tram_init(). Also beware that TRAM memory gets read by hm2_read() each time it runs, so it'll overwrite anything your driver writes to that register-buffer at that time. I think that's probably ok in your case. >> The hm2_led_parse_md() function requires there to be exactly one LED >> instance. It might be better to make the LEDs optional, so it accepts 0 >> or 1 instances. >> > > I took the view that there is always exactly one physical instance of > the LEDs, but perhaps there should be the option to not load the > module to save overhead. > I'm not so much worried about the overhead of this, I think that's down in the noise. This was just a reflexive reaction on my part, and feel free to ignore it: "there are exactly 8 LEDs per board" seems like a coincidence, and that limitation doesn't belong in the driver. -- Sebastian Kuzminsky ------------------------------------------------------------------------------ _______________________________________________ Emc-developers mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/emc-developers
