Matthias Dahl wrote: > Hello Oliver. > > On Saturday 23 August 2008 22:58:39 Oliver Endriss wrote: > > > Finally I had some time to review your patches. > > No problem. I am currently short on time either so I know what it's like. :-) > > > budget-av: > > Could you please elaborate why > > - ciintf_slot_ts_enable > > - ciintf_slot_shutdown > > require locking? I cannot see that. > > The thought behind that was to make it impossible that any of all the ciintf > functions could interfere with each other and thus possibly get the ci intf > in an undefined state. So to prevent that only one one of them is allowed to > be active at any given time. But you are right one could have left those out > of there... sorry, my fault. > > > ciintf_slot_reset: > > Ok, although I doubt that it will make any difference, because the > > routine will kill tuner and CI interface anyway... > > Basically, see above. :-) Nevertheless I still think we should make absolutely > certain that nothing interferes with the reset function because otherwise we > may end up with an inoperational ci system just like the issues I had. So > having the mutex in there won't hurt at the very least. Agreed. > > ciintf_poll_slot_status: > > Hm, I think my version was also correct. > > Same applies here. I guess I was just too overly cautious trying to get rid of > those problems. The only reason I saw here was that slot_status might have > gotten changed when the mutex was released early. But that clearly shouldn't > cause any harm either. Man, I guess was asleep while doing those. That's > already kinda embarrassing... > > > Btw, wouldn't it be better to remove the locking stuff from budget-av.c, > > and do all locking in dvb_ca_en50221.c? > > That was exactly what I was working on before I got your patch but as your > patch seemed to work fine initially I put that on hold and later just started > from scratch and only put the locks in [read|write]_data because then I knew > what was causing the trouble. When I wrote the budget-av patch, I assumed that dvb_ca_en50221.c was fine. I read the comments in dvb_ca_en50221.h and noticed that the locks were missing in budget-av.c. As the budget-av patch was not sufficient, it is better to handle the locking in dvb_ca_en50221.c. > > Imho this would be a far better solution (only one mutex, not two). > > Could you implement that? Note that the *_irq functions in dvb_ca_en50221.c might be called from interrupt context. (Does not apply for budget-av, but for budget-ci.) Interrupt routines cannot use the mutex stuff... > Actually I wanted to work on it today but I am dealing with some health issues > so I didn't get around to it. I try to get it done in the next few days. So > with some extra days for testing, hopefully I will have something next > weekend. No need to hurry. Bug fixes can be submitted to the kernel anytime, even when the merge window (for new features) has closed. CU Oliver -- ---------------------------------------------------------------- VDR Remote Plugin 0.4.0: http://www.escape-edv.de/endriss/vdr/ ---------------------------------------------------------------- _______________________________________________ linux-dvb mailing list linux-dvb@xxxxxxxxxxx http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
