From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from mail.gmx.net ([213.165.64.20]) by www.linuxtv.org with smtp (Exim 4.63) (envelope-from ) id 1KXLpT-0001D8-Bk for linux-dvb@linuxtv.org; Sun, 24 Aug 2008 22:04:55 +0200 From: Oliver Endriss To: Matthias Dahl Date: Sun, 24 Aug 2008 22:03:53 +0200 References: <200808221555.26507.mldvb@mortal-soul.de> <200808232258.40112@orion.escape-edv.de> <200808242030.24060.mldvb@mortal-soul.de> In-Reply-To: <200808242030.24060.mldvb@mortal-soul.de> MIME-Version: 1.0 Content-Disposition: inline Message-Id: <200808242203.53675@orion.escape-edv.de> Cc: linux-dvb@linuxtv.org Subject: Re: [linux-dvb] [PATCH] budget_av / dvb_ca_en50221: fixes ci/cam handling especially on SMP machines Reply-To: linux-dvb@linuxtv.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: linux-dvb-bounces@linuxtv.org Errors-To: linux-dvb-bounces+mchehab=infradead.org@linuxtv.org List-ID: 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@linuxtv.org http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb