HG pull http://jusst.de/hg/stv090x
Mauro, Please pull from http://jusst.de/hg/stv090x for the following changes changeset 14413 stv090x: Add some notes about the internal tuner I/O control http://jusst.de/hg/stv090x/rev/6d166305382b changeset 14412 Budget/STV090x/STV6110x: Initialize the demodulator immediately after the tuner is attahced http://jusst.de/hg/stv090x/rev/27a8b3a8415f changeset 14411 [STV090x] Use gate control, while tuner is being accessed. http://jusst.de/hg/stv090x/rev/5ff2bc2dc92c changeset 14410 [STV090x, STV6110x] Use tuner sleep within the demodulator control http://jusst.de/hg/stv090x/rev/0b84056090e3 changeset 14409 [STV090x] Code simplification http://jusst.de/hg/stv090x/rev/f8a4b7a4eb77 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: PULL http://jusst.de/hg/stv090x
Hi, Am Samstag, den 23.01.2010, 15:25 +0400 schrieb Manu Abraham: > On Sat, Jan 23, 2010 at 5:08 AM, hermann pitton > wrote: > > Hi. > > > > Am Samstag, den 23.01.2010, 01:23 +0400 schrieb Manu Abraham: > >> On Sat, Jan 23, 2010 at 1:14 AM, Mauro Carvalho Chehab > >> wrote: > >> > Devin Heitmueller wrote: > >> >> On Fri, Jan 22, 2010 at 3:22 PM, Manu Abraham > >> >> wrote: > >> >>> On Fri, Jan 22, 2010 at 11:40 PM, Devin Heitmueller > >> >>> wrote: > >> Also, the dvb_frontend.c makes calls to i2c_gate_ctrl() at various > >> points, so you would need to ensure that none of those occur before > >> calling into your driver as there could potentially be a deadlock > >> there too. > >> >>> Ok, thanks for the pointer. The gate control is never called > >> >>> externally in reality. I will wait a little while for this patch to be > >> >>> applied. It removes the exported function and thereby an unnecessary > >> >>> dereference. > >> >>> > >> >>> http://jusst.de/hg/stv090x/rev/b3d28f5b2b53 > >> >> > >> >> If it never needs to be called externally, then removing it from the > >> >> dvb_frontend_ops does eliminate the risk entirely. The case I > >> >> frequently see it called from dvb_frontend.c is for powering down the > >> >> tuner when the dvb frontend thread shuts down. > >> > > >> > The removal of the external call to the gate function removes the risk > >> > that > >> > an external call, at dvb core, to keep it locked. Yet, the code there is > >> > completely > >> > symetric nowadays. > >> > > >> > However, the proper documentation of the lock is needed to avoid the risk > >> > of some patch to keep the mutex locked. > >> > > >> > As, even the initial lock changeset has this problem (later fixed by > >> > http://jusst.de/hg/stv090x/rev/3a8f35abc0f2), it shows that a good > >> > documentation > >> > is required. > >> > > >> > As you've talked about a FSM, the lock itself is a FSM with 2 states. > >> > All I'm > >> > asking is that you document that the lock FSM has changed its state in > >> > the name > >> > of the function that alters the state of the lock. > >> > > >> > >> Sure, of course, i will add some comments into the patch that I have > >> queued up, before pull request. Don't worry, be at peace. :-) > >> > >> Regards, > >> Manu > > > > Good, seems there is some progress. > > > > But 1080p was first from satellites only too ;) > > > Eventually, broadcasters will restrict all good content. Only old > stuff will even be happening with FTA. Even old scrambling systems are > going away. > > Broadcasters would transmit protected content in the stream, along > with relevant keys for relevant schema; CI+ is replacing old CI > systems. > > CI+ streams would eventually be encrypted; ie, you won't get access to > those streams, The CI+ output will be paired with a CI+ capable > decoder, which will be given it's set of keys. > > The decoder will output HDMI with HDCP .. > > You can see that vicious circle that's evolving ... Broadcasters and > Media producers like to term it as a war; they don't want users to > capture the streams on a computer eventually; that's another way of > stating it; even if the media has to be Time shifted, that would be > paired with decoder, so eventually the media stays at one place and > doesn't get circulated. > > > Regards, > Manu yes, you are totally right, serious problems with lots of implications. We are also already excluded from HD content becoming available on demand over IP, because of missing DRM. All the private/commercial broadcasters switched over to HD+ and CI+ right now here in Germany. So you don't come any further even with the new S2 cards. Given that, isn't it better to buy blu(e)-ray disks for the little HD content you are really interested in and have 1080p? If I'm not outdated on it again, likely possible, you can exchange legal disks legally with others. Should be much cheaper than to have such a jukebox in your living room you have to feed again and again, even for the same content, if you ever want to see it one more time. I won't buy any such + stuff to watch soap operas in some sort of HD and some other stuff on lowest levels they have, not even be able to skip the endless commercials in between. Cheers, Hermann -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: PULL http://jusst.de/hg/stv090x
On Sat, Jan 23, 2010 at 10:46 PM, Andreas Regel wrote: > Hi Manu, > > Am 23.01.2010 19:32, schrieb Manu Abraham: >> >> On Sat, Jan 23, 2010 at 10:07 PM, Manu Abraham >> wrote: >>> >>> Hi Andreas, >>> >>> On Sat, Jan 23, 2010 at 9:50 PM, Andreas Regel >>> wrote: Hi Manu, Am 22.01.2010 21:22, schrieb Manu Abraham: > > On Fri, Jan 22, 2010 at 11:40 PM, Devin Heitmueller > wrote: >> >> Also, the dvb_frontend.c makes calls to i2c_gate_ctrl() at various >> points, so you would need to ensure that none of those occur before >> calling into your driver as there could potentially be a deadlock >> there too. > > Ok, thanks for the pointer. The gate control is never called > externally in reality. I will wait a little while for this patch to be > applied. It removes the exported function and thereby an unnecessary > dereference. > > http://jusst.de/hg/stv090x/rev/b3d28f5b2b53 There is one call to the gate control function from stv6110x_attach. This is needed to set up the clock output divider to the correct value before the demodulators clock is configured. This could be solved by calling tuner_init before setting up the master clock in stv090x_init but that only helps on single tuner devices. On dual tuner devices you can only open the adapter that works with the second tuner. Then you will have the case that the master clock is set without having the clock output divider of first tuner initialized to the correct value. >>> >>> Thinking of which, maybe it would be better to attach the tuner_attach >>> within the stv090x_attach(). That could solve the issue, AFAICT. What >>> do you say ? >> >> OR >> >> Another option will be to attach the tuner prior to the demodulator, >> without the clock configuration in the tuner attach (clk configuartion >> would be another function ptr), attach the demodulator, run clock >> configuration... >> >> I think this might be a bit more cleaner than attaching the tuner >> within the demodulator_attach() ... ? > > as there already is a function pointer interface for tuner control, I would > prefer the second approach. I started up on it, but if you would like to send a patch, I am happy that way too... > Shall I prepare a patch for it or do you want to? Either is fine with me. Regards, Manu -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: PULL http://jusst.de/hg/stv090x
Hi Manu, Am 23.01.2010 19:32, schrieb Manu Abraham: On Sat, Jan 23, 2010 at 10:07 PM, Manu Abraham wrote: Hi Andreas, On Sat, Jan 23, 2010 at 9:50 PM, Andreas Regel wrote: Hi Manu, Am 22.01.2010 21:22, schrieb Manu Abraham: On Fri, Jan 22, 2010 at 11:40 PM, Devin Heitmueller wrote: Also, the dvb_frontend.c makes calls to i2c_gate_ctrl() at various points, so you would need to ensure that none of those occur before calling into your driver as there could potentially be a deadlock there too. Ok, thanks for the pointer. The gate control is never called externally in reality. I will wait a little while for this patch to be applied. It removes the exported function and thereby an unnecessary dereference. http://jusst.de/hg/stv090x/rev/b3d28f5b2b53 There is one call to the gate control function from stv6110x_attach. This is needed to set up the clock output divider to the correct value before the demodulators clock is configured. This could be solved by calling tuner_init before setting up the master clock in stv090x_init but that only helps on single tuner devices. On dual tuner devices you can only open the adapter that works with the second tuner. Then you will have the case that the master clock is set without having the clock output divider of first tuner initialized to the correct value. Thinking of which, maybe it would be better to attach the tuner_attach within the stv090x_attach(). That could solve the issue, AFAICT. What do you say ? OR Another option will be to attach the tuner prior to the demodulator, without the clock configuration in the tuner attach (clk configuartion would be another function ptr), attach the demodulator, run clock configuration... I think this might be a bit more cleaner than attaching the tuner within the demodulator_attach() ... ? as there already is a function pointer interface for tuner control, I would prefer the second approach. Shall I prepare a patch for it or do you want to? Regards Andreas -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: PULL http://jusst.de/hg/stv090x
On Sat, Jan 23, 2010 at 10:07 PM, Manu Abraham wrote: > Hi Andreas, > > On Sat, Jan 23, 2010 at 9:50 PM, Andreas Regel wrote: >> Hi Manu, >> >> Am 22.01.2010 21:22, schrieb Manu Abraham: >>> >>> On Fri, Jan 22, 2010 at 11:40 PM, Devin Heitmueller >>> wrote: Also, the dvb_frontend.c makes calls to i2c_gate_ctrl() at various points, so you would need to ensure that none of those occur before calling into your driver as there could potentially be a deadlock there too. >>> >>> Ok, thanks for the pointer. The gate control is never called >>> externally in reality. I will wait a little while for this patch to be >>> applied. It removes the exported function and thereby an unnecessary >>> dereference. >>> >>> http://jusst.de/hg/stv090x/rev/b3d28f5b2b53 >> >> There is one call to the gate control function from stv6110x_attach. This is >> needed to set up the clock output divider to the correct value before the >> demodulators clock is configured. >> >> This could be solved by calling tuner_init before setting up the master >> clock in stv090x_init but that only helps on single tuner devices. On dual >> tuner devices you can only open the adapter that works with the second >> tuner. Then you will have the case that the master clock is set without >> having the clock output divider of first tuner initialized to the correct >> value. > > Thinking of which, maybe it would be better to attach the tuner_attach > within the stv090x_attach(). That could solve the issue, AFAICT. What > do you say ? OR Another option will be to attach the tuner prior to the demodulator, without the clock configuration in the tuner attach (clk configuartion would be another function ptr), attach the demodulator, run clock configuration... I think this might be a bit more cleaner than attaching the tuner within the demodulator_attach() ... ? Regards, Manu -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: PULL http://jusst.de/hg/stv090x
Hi Andreas, On Sat, Jan 23, 2010 at 9:50 PM, Andreas Regel wrote: > Hi Manu, > > Am 22.01.2010 21:22, schrieb Manu Abraham: >> >> On Fri, Jan 22, 2010 at 11:40 PM, Devin Heitmueller >> wrote: >>> >>> Also, the dvb_frontend.c makes calls to i2c_gate_ctrl() at various >>> points, so you would need to ensure that none of those occur before >>> calling into your driver as there could potentially be a deadlock >>> there too. >> >> Ok, thanks for the pointer. The gate control is never called >> externally in reality. I will wait a little while for this patch to be >> applied. It removes the exported function and thereby an unnecessary >> dereference. >> >> http://jusst.de/hg/stv090x/rev/b3d28f5b2b53 > > There is one call to the gate control function from stv6110x_attach. This is > needed to set up the clock output divider to the correct value before the > demodulators clock is configured. > > This could be solved by calling tuner_init before setting up the master > clock in stv090x_init but that only helps on single tuner devices. On dual > tuner devices you can only open the adapter that works with the second > tuner. Then you will have the case that the master clock is set without > having the clock output divider of first tuner initialized to the correct > value. Thinking of which, maybe it would be better to attach the tuner_attach within the stv090x_attach(). That could solve the issue, AFAICT. What do you say ? Regards, Manu -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: PULL http://jusst.de/hg/stv090x
Hi Manu, Am 22.01.2010 21:22, schrieb Manu Abraham: On Fri, Jan 22, 2010 at 11:40 PM, Devin Heitmueller wrote: Also, the dvb_frontend.c makes calls to i2c_gate_ctrl() at various points, so you would need to ensure that none of those occur before calling into your driver as there could potentially be a deadlock there too. Ok, thanks for the pointer. The gate control is never called externally in reality. I will wait a little while for this patch to be applied. It removes the exported function and thereby an unnecessary dereference. http://jusst.de/hg/stv090x/rev/b3d28f5b2b53 There is one call to the gate control function from stv6110x_attach. This is needed to set up the clock output divider to the correct value before the demodulators clock is configured. This could be solved by calling tuner_init before setting up the master clock in stv090x_init but that only helps on single tuner devices. On dual tuner devices you can only open the adapter that works with the second tuner. Then you will have the case that the master clock is set without having the clock output divider of first tuner initialized to the correct value. Regards Andreas -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: PULL http://jusst.de/hg/stv090x
On Sat, Jan 23, 2010 at 5:08 AM, hermann pitton wrote: > Hi. > > Am Samstag, den 23.01.2010, 01:23 +0400 schrieb Manu Abraham: >> On Sat, Jan 23, 2010 at 1:14 AM, Mauro Carvalho Chehab >> wrote: >> > Devin Heitmueller wrote: >> >> On Fri, Jan 22, 2010 at 3:22 PM, Manu Abraham >> >> wrote: >> >>> On Fri, Jan 22, 2010 at 11:40 PM, Devin Heitmueller >> >>> wrote: >> Also, the dvb_frontend.c makes calls to i2c_gate_ctrl() at various >> points, so you would need to ensure that none of those occur before >> calling into your driver as there could potentially be a deadlock >> there too. >> >>> Ok, thanks for the pointer. The gate control is never called >> >>> externally in reality. I will wait a little while for this patch to be >> >>> applied. It removes the exported function and thereby an unnecessary >> >>> dereference. >> >>> >> >>> http://jusst.de/hg/stv090x/rev/b3d28f5b2b53 >> >> >> >> If it never needs to be called externally, then removing it from the >> >> dvb_frontend_ops does eliminate the risk entirely. The case I >> >> frequently see it called from dvb_frontend.c is for powering down the >> >> tuner when the dvb frontend thread shuts down. >> > >> > The removal of the external call to the gate function removes the risk that >> > an external call, at dvb core, to keep it locked. Yet, the code there is >> > completely >> > symetric nowadays. >> > >> > However, the proper documentation of the lock is needed to avoid the risk >> > of some patch to keep the mutex locked. >> > >> > As, even the initial lock changeset has this problem (later fixed by >> > http://jusst.de/hg/stv090x/rev/3a8f35abc0f2), it shows that a good >> > documentation >> > is required. >> > >> > As you've talked about a FSM, the lock itself is a FSM with 2 states. All >> > I'm >> > asking is that you document that the lock FSM has changed its state in the >> > name >> > of the function that alters the state of the lock. >> > >> >> Sure, of course, i will add some comments into the patch that I have >> queued up, before pull request. Don't worry, be at peace. :-) >> >> Regards, >> Manu > > Good, seems there is some progress. > > But 1080p was first from satellites only too ;) Eventually, broadcasters will restrict all good content. Only old stuff will even be happening with FTA. Even old scrambling systems are going away. Broadcasters would transmit protected content in the stream, along with relevant keys for relevant schema; CI+ is replacing old CI systems. CI+ streams would eventually be encrypted; ie, you won't get access to those streams, The CI+ output will be paired with a CI+ capable decoder, which will be given it's set of keys. The decoder will output HDMI with HDCP .. You can see that vicious circle that's evolving ... Broadcasters and Media producers like to term it as a war; they don't want users to capture the streams on a computer eventually; that's another way of stating it; even if the media has to be Time shifted, that would be paired with decoder, so eventually the media stays at one place and doesn't get circulated. Regards, Manu -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: PULL http://jusst.de/hg/stv090x
On Sat, Jan 23, 2010 at 1:58 PM, Oliver Endriss wrote:
> Manu Abraham wrote:
>> Hi Oliver,
>>
>> On Sat, Jan 23, 2010 at 7:09 AM, Oliver Endriss wrote:
>> > Manu Abraham wrote:
>> >> On Fri, Jan 22, 2010 at 11:40 PM, Devin Heitmueller
>> >> wrote:
>> >> > Also, the dvb_frontend.c makes calls to i2c_gate_ctrl() at various
>> >> > points, so you would need to ensure that none of those occur before
>> >> > calling into your driver as there could potentially be a deadlock
>> >> > there too.
>> >>
>> >> Ok, thanks for the pointer. The gate control is never called
>> >> externally in reality. I will wait a little while for this patch to be
>> >> applied. It removes the exported function and thereby an unnecessary
>> >> dereference.
>> >>
>> >> http://jusst.de/hg/stv090x/rev/b3d28f5b2b53
>> >
>> > Imho not a good idea, as the frontend thread calls
>> > - fe->ops.tuner_ops.init
>> > - fe->ops.tuner_ops.sleep
>> >
>> > If you remove fe->ops.i2c_gate_ctrl, init and sleep will fail,
>> > because gate_ctrl was never called...
>>
>>
>> tuner Init is already called within the demodulator control loop: ie, init
>> I have moved in tuner Sleep likewise.
>>
>> http://jusst.de/hg/stv090x/rev/5699b0d87a12
>>
>> I think that would fix the issues at hand ...
>>
>>
>> Thanks for the pointer,
>
> + if (state->config->tuner_init) {
> + if (state->config->tuner_sleep(fe) < 0)
> + goto err_gateoff;
> + }
> +
>
> s/tuner_init/tuner_sleep
I noticed that immediately after writing that email, fixed in the next
commit after that, also the gate control has been added in there,
which is also necessary.
> Btw, these NULL initialisations could be removed:
> .tuner_init = NULL,
> + .tuner_sleep = NULL,
> .tuner_set_mode = NULL,
> .tuner_set_frequency = NULL,
> .tuner_get_frequency = NULL,
>
Ah, ok. I will remove them.
Regards,
Manu
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: PULL http://jusst.de/hg/stv090x
Manu Abraham wrote:
> Hi Oliver,
>
> On Sat, Jan 23, 2010 at 7:09 AM, Oliver Endriss wrote:
> > Manu Abraham wrote:
> >> On Fri, Jan 22, 2010 at 11:40 PM, Devin Heitmueller
> >> wrote:
> >> > Also, the dvb_frontend.c makes calls to i2c_gate_ctrl() at various
> >> > points, so you would need to ensure that none of those occur before
> >> > calling into your driver as there could potentially be a deadlock
> >> > there too.
> >>
> >> Ok, thanks for the pointer. The gate control is never called
> >> externally in reality. I will wait a little while for this patch to be
> >> applied. It removes the exported function and thereby an unnecessary
> >> dereference.
> >>
> >> http://jusst.de/hg/stv090x/rev/b3d28f5b2b53
> >
> > Imho not a good idea, as the frontend thread calls
> > - fe->ops.tuner_ops.init
> > - fe->ops.tuner_ops.sleep
> >
> > If you remove fe->ops.i2c_gate_ctrl, init and sleep will fail,
> > because gate_ctrl was never called...
>
>
> tuner Init is already called within the demodulator control loop: ie, init
> I have moved in tuner Sleep likewise.
>
> http://jusst.de/hg/stv090x/rev/5699b0d87a12
>
> I think that would fix the issues at hand ...
>
>
> Thanks for the pointer,
+ if (state->config->tuner_init) {
+ if (state->config->tuner_sleep(fe) < 0)
+ goto err_gateoff;
+ }
+
s/tuner_init/tuner_sleep
Btw, these NULL initialisations could be removed:
.tuner_init = NULL,
+ .tuner_sleep= NULL,
.tuner_set_mode = NULL,
.tuner_set_frequency= NULL,
.tuner_get_frequency= NULL,
(struct tt1600_stv090x_config is static anyway.)
CU
Oliver
--
VDR Remote Plugin 0.4.0: http://www.escape-edv.de/endriss/vdr/
4 MByte Mod: http://www.escape-edv.de/endriss/dvb-mem-mod/
Full-TS Mod: http://www.escape-edv.de/endriss/dvb-full-ts-mod/
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: PULL http://jusst.de/hg/stv090x
Hi Oliver, On Sat, Jan 23, 2010 at 7:09 AM, Oliver Endriss wrote: > Manu Abraham wrote: >> On Fri, Jan 22, 2010 at 11:40 PM, Devin Heitmueller >> wrote: >> > Also, the dvb_frontend.c makes calls to i2c_gate_ctrl() at various >> > points, so you would need to ensure that none of those occur before >> > calling into your driver as there could potentially be a deadlock >> > there too. >> >> Ok, thanks for the pointer. The gate control is never called >> externally in reality. I will wait a little while for this patch to be >> applied. It removes the exported function and thereby an unnecessary >> dereference. >> >> http://jusst.de/hg/stv090x/rev/b3d28f5b2b53 > > Imho not a good idea, as the frontend thread calls > - fe->ops.tuner_ops.init > - fe->ops.tuner_ops.sleep > > If you remove fe->ops.i2c_gate_ctrl, init and sleep will fail, > because gate_ctrl was never called... tuner Init is already called within the demodulator control loop: ie, init I have moved in tuner Sleep likewise. http://jusst.de/hg/stv090x/rev/5699b0d87a12 I think that would fix the issues at hand ... Thanks for the pointer, Regards, Manu -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: PULL http://jusst.de/hg/stv090x
Manu Abraham wrote: > On Fri, Jan 22, 2010 at 11:40 PM, Devin Heitmueller > wrote: > > Also, the dvb_frontend.c makes calls to i2c_gate_ctrl() at various > > points, so you would need to ensure that none of those occur before > > calling into your driver as there could potentially be a deadlock > > there too. > > Ok, thanks for the pointer. The gate control is never called > externally in reality. I will wait a little while for this patch to be > applied. It removes the exported function and thereby an unnecessary > dereference. > > http://jusst.de/hg/stv090x/rev/b3d28f5b2b53 Imho not a good idea, as the frontend thread calls - fe->ops.tuner_ops.init - fe->ops.tuner_ops.sleep If you remove fe->ops.i2c_gate_ctrl, init and sleep will fail, because gate_ctrl was never called... CU Oliver -- VDR Remote Plugin 0.4.0: http://www.escape-edv.de/endriss/vdr/ 4 MByte Mod: http://www.escape-edv.de/endriss/dvb-mem-mod/ Full-TS Mod: http://www.escape-edv.de/endriss/dvb-full-ts-mod/ -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: PULL http://jusst.de/hg/stv090x
Hi. Am Samstag, den 23.01.2010, 01:23 +0400 schrieb Manu Abraham: > On Sat, Jan 23, 2010 at 1:14 AM, Mauro Carvalho Chehab > wrote: > > Devin Heitmueller wrote: > >> On Fri, Jan 22, 2010 at 3:22 PM, Manu Abraham > >> wrote: > >>> On Fri, Jan 22, 2010 at 11:40 PM, Devin Heitmueller > >>> wrote: > Also, the dvb_frontend.c makes calls to i2c_gate_ctrl() at various > points, so you would need to ensure that none of those occur before > calling into your driver as there could potentially be a deadlock > there too. > >>> Ok, thanks for the pointer. The gate control is never called > >>> externally in reality. I will wait a little while for this patch to be > >>> applied. It removes the exported function and thereby an unnecessary > >>> dereference. > >>> > >>> http://jusst.de/hg/stv090x/rev/b3d28f5b2b53 > >> > >> If it never needs to be called externally, then removing it from the > >> dvb_frontend_ops does eliminate the risk entirely. The case I > >> frequently see it called from dvb_frontend.c is for powering down the > >> tuner when the dvb frontend thread shuts down. > > > > The removal of the external call to the gate function removes the risk that > > an external call, at dvb core, to keep it locked. Yet, the code there is > > completely > > symetric nowadays. > > > > However, the proper documentation of the lock is needed to avoid the risk > > of some patch to keep the mutex locked. > > > > As, even the initial lock changeset has this problem (later fixed by > > http://jusst.de/hg/stv090x/rev/3a8f35abc0f2), it shows that a good > > documentation > > is required. > > > > As you've talked about a FSM, the lock itself is a FSM with 2 states. All > > I'm > > asking is that you document that the lock FSM has changed its state in the > > name > > of the function that alters the state of the lock. > > > > Sure, of course, i will add some comments into the patch that I have > queued up, before pull request. Don't worry, be at peace. :-) > > Regards, > Manu Good, seems there is some progress. But 1080p was first from satellites only too ;) Cheers, Hermann -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: PULL http://jusst.de/hg/stv090x
On Sat, Jan 23, 2010 at 1:14 AM, Mauro Carvalho Chehab wrote: > Devin Heitmueller wrote: >> On Fri, Jan 22, 2010 at 3:22 PM, Manu Abraham wrote: >>> On Fri, Jan 22, 2010 at 11:40 PM, Devin Heitmueller >>> wrote: Also, the dvb_frontend.c makes calls to i2c_gate_ctrl() at various points, so you would need to ensure that none of those occur before calling into your driver as there could potentially be a deadlock there too. >>> Ok, thanks for the pointer. The gate control is never called >>> externally in reality. I will wait a little while for this patch to be >>> applied. It removes the exported function and thereby an unnecessary >>> dereference. >>> >>> http://jusst.de/hg/stv090x/rev/b3d28f5b2b53 >> >> If it never needs to be called externally, then removing it from the >> dvb_frontend_ops does eliminate the risk entirely. The case I >> frequently see it called from dvb_frontend.c is for powering down the >> tuner when the dvb frontend thread shuts down. > > The removal of the external call to the gate function removes the risk that > an external call, at dvb core, to keep it locked. Yet, the code there is > completely > symetric nowadays. > > However, the proper documentation of the lock is needed to avoid the risk > of some patch to keep the mutex locked. > > As, even the initial lock changeset has this problem (later fixed by > http://jusst.de/hg/stv090x/rev/3a8f35abc0f2), it shows that a good > documentation > is required. > > As you've talked about a FSM, the lock itself is a FSM with 2 states. All I'm > asking is that you document that the lock FSM has changed its state in the > name > of the function that alters the state of the lock. > Sure, of course, i will add some comments into the patch that I have queued up, before pull request. Don't worry, be at peace. :-) Regards, Manu -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: PULL http://jusst.de/hg/stv090x
Devin Heitmueller wrote: > On Fri, Jan 22, 2010 at 3:22 PM, Manu Abraham wrote: >> On Fri, Jan 22, 2010 at 11:40 PM, Devin Heitmueller >> wrote: >>> Also, the dvb_frontend.c makes calls to i2c_gate_ctrl() at various >>> points, so you would need to ensure that none of those occur before >>> calling into your driver as there could potentially be a deadlock >>> there too. >> Ok, thanks for the pointer. The gate control is never called >> externally in reality. I will wait a little while for this patch to be >> applied. It removes the exported function and thereby an unnecessary >> dereference. >> >> http://jusst.de/hg/stv090x/rev/b3d28f5b2b53 > > If it never needs to be called externally, then removing it from the > dvb_frontend_ops does eliminate the risk entirely. The case I > frequently see it called from dvb_frontend.c is for powering down the > tuner when the dvb frontend thread shuts down. The removal of the external call to the gate function removes the risk that an external call, at dvb core, to keep it locked. Yet, the code there is completely symetric nowadays. However, the proper documentation of the lock is needed to avoid the risk of some patch to keep the mutex locked. As, even the initial lock changeset has this problem (later fixed by http://jusst.de/hg/stv090x/rev/3a8f35abc0f2), it shows that a good documentation is required. As you've talked about a FSM, the lock itself is a FSM with 2 states. All I'm asking is that you document that the lock FSM has changed its state in the name of the function that alters the state of the lock. Cheers, Mauro. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: PULL http://jusst.de/hg/stv090x
On Fri, Jan 22, 2010 at 4:03 PM, Mauro Carvalho Chehab wrote: > I've checked the core and it does it on a balanced way. Yet, it is risky > to assume that this will always happen, and having a bad, non-interrupt > mutex there can lead to machine hangups. The core does balance the calls (I actually submitted a patch last year fixing a bug where it wasn't). However, if the tuner driver itself calls the i2c_gate_ctrl() function [and some tuners do do this] then you would hit a deadlock if it were used with Manu's frontend driver. >> Today, there is no real problem if a particular call path attempts to >> enable the gate if it is already open (or disable if it is already >> closed). With your proposed change, you will result in a deadlock. > > Precisely. Of course, both of these concerns will be no longer be relevant now that he is removing it from the dvb_frontend_ops (which means neither the tuner nor the frontend will be able to manipulate the gate). Devin -- Devin J. Heitmueller - Kernel Labs http://www.kernellabs.com -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: PULL http://jusst.de/hg/stv090x
On Fri, Jan 22, 2010 at 3:59 PM, Manu Abraham wrote: > No, rather on frontend shutdown, demodulator sleep() is called > instead. demodulator sleep() shuts down the tuner, AFAIR. This is the > original and correct behaviour for dvb frontend. Both the tuner and frontend are issued sleep calls. See dvb_frontend.c line 680. Devin -- Devin J. Heitmueller - Kernel Labs http://www.kernellabs.com -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: PULL http://jusst.de/hg/stv090x
Devin Heitmueller wrote: > On Fri, Jan 22, 2010 at 1:23 PM, Manu Abraham wrote: >> The point there is that it is a dual demod which implements a FSM for >> it's tuner flow control and hence. The demodulators in the tree are >> generally single/standalone and don't need such a FSM to handle that. >> Even if it's a dual frontend, most of them just do locking for the >> register access, while here it is not register access locking, while >> it is controlling state, similar to Finite State Machine (FSM) > > Ok, I see what is going on now. I didn't notice before that the gate > enable was keeping the "state->internal->tuner_lock" mutex set even > after the call returned. So you proofed my point: the code "quietly" changes the state of the mutex. As all other code that do that has a _lock/_unlock, it is a matter of documenting things using the standard kernel way for it, to avoid people to bad interpret the code. > My only concern here then is that I've seen cases where the > i2c_gate_ctrl() function is called in a non-symmetric fashion (where > the enable/disable calls are not necessarily properly balanced), > depending on the driver. While I am relatively confident that you > have balanced the instances where it is called within your driver, I > would be concerned about how it could possibly be called from other > places such as from the tuner driver or the dvb frontend core. I've checked the core and it does it on a balanced way. Yet, it is risky to assume that this will always happen, and having a bad, non-interrupt mutex there can lead to machine hangups. Even the patch that introduced this locking schema had this bug, showing that it is simple for someone to forget to ballance. The usage of the _lock/_unlock suffix helps to document that those calls need to be balanced. > Today, there is no real problem if a particular call path attempts to > enable the gate if it is already open (or disable if it is already > closed). With your proposed change, you will result in a deadlock. Precisely. Cheers, Mauro. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: PULL http://jusst.de/hg/stv090x
On Sat, Jan 23, 2010 at 12:42 AM, Devin Heitmueller wrote: > On Fri, Jan 22, 2010 at 3:22 PM, Manu Abraham wrote: >> On Fri, Jan 22, 2010 at 11:40 PM, Devin Heitmueller >> wrote: >>> Also, the dvb_frontend.c makes calls to i2c_gate_ctrl() at various >>> points, so you would need to ensure that none of those occur before >>> calling into your driver as there could potentially be a deadlock >>> there too. >> >> Ok, thanks for the pointer. The gate control is never called >> externally in reality. I will wait a little while for this patch to be >> applied. It removes the exported function and thereby an unnecessary >> dereference. >> >> http://jusst.de/hg/stv090x/rev/b3d28f5b2b53 > > If it never needs to be called externally, then removing it from the > dvb_frontend_ops does eliminate the risk entirely. Yup. > The case I > frequently see it called from dvb_frontend.c is for powering down the > tuner when the dvb frontend thread shuts down. No, rather on frontend shutdown, demodulator sleep() is called instead. demodulator sleep() shuts down the tuner, AFAIR. This is the original and correct behaviour for dvb frontend. I don't know if there are any instances in which tuner initial shutdown is proper, from the top of my head. I haven't looked whether the dvb frontend code changed recently. Regards, Manu -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: PULL http://jusst.de/hg/stv090x
On Fri, Jan 22, 2010 at 3:22 PM, Manu Abraham wrote: > On Fri, Jan 22, 2010 at 11:40 PM, Devin Heitmueller > wrote: >> Also, the dvb_frontend.c makes calls to i2c_gate_ctrl() at various >> points, so you would need to ensure that none of those occur before >> calling into your driver as there could potentially be a deadlock >> there too. > > Ok, thanks for the pointer. The gate control is never called > externally in reality. I will wait a little while for this patch to be > applied. It removes the exported function and thereby an unnecessary > dereference. > > http://jusst.de/hg/stv090x/rev/b3d28f5b2b53 If it never needs to be called externally, then removing it from the dvb_frontend_ops does eliminate the risk entirely. The case I frequently see it called from dvb_frontend.c is for powering down the tuner when the dvb frontend thread shuts down. Cheers, Devin -- Devin J. Heitmueller - Kernel Labs http://www.kernellabs.com -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: PULL http://jusst.de/hg/stv090x
On Fri, Jan 22, 2010 at 11:40 PM, Devin Heitmueller wrote: > Also, the dvb_frontend.c makes calls to i2c_gate_ctrl() at various > points, so you would need to ensure that none of those occur before > calling into your driver as there could potentially be a deadlock > there too. Ok, thanks for the pointer. The gate control is never called externally in reality. I will wait a little while for this patch to be applied. It removes the exported function and thereby an unnecessary dereference. http://jusst.de/hg/stv090x/rev/b3d28f5b2b53 Thanks, Manu -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: PULL http://jusst.de/hg/stv090x
On Fri, Jan 22, 2010 at 11:40 PM, Devin Heitmueller wrote: > On Fri, Jan 22, 2010 at 2:11 PM, Manu Abraham wrote: >> I think you understood incorrectly. Generally most demodulators have >> an option to detect the I2C stop bit, thereby automatically disabling >> the gate, while some don't. In those cases, you don't even need a >> i2c_gate_control(disable) >> It is that safe. Now, even if the gate happens to be open for some >> reason, it won't have a large downside, just that the i2c transactions >> might seep into the tuner silicon as RF noise. > > Yes, I was aware of this (although I might dispute whether *most* > demodulators really operate this way). > >> Can you explain your logic a bit more in detail, please ? >> >> If you meant to imply the action on a failure case, it does unlock in >> all possible cases as you can see... >> http://jusst.de/hg/stv090x/rev/3a8f35abc0f2 > > I am not talking about the case where the tuning fails. I mean that > not only is the i2c_gate_ctrl function used internally by stv090x.c, > but it is also exposed via the dvb_frontend_ops struct. Depending on > what tuner chip your frontend is being used with, I have seen cases > where the tuner driver itself takes responsibility for opening/closing > the gate. So you effectively end up with: > > frontend driver calls i2c_gate_ctrl(enable) > frontend driver tells tuner driver to tune > tuner driver calls i2c_gate_ctrl(enable) <--- Would cause deadlock > in your driver > tuner driver calls i2c_gate_ctrl(disable) > tuner driver returns to caller > frontend driver calls i2c_gate_ctrl(disable) > > With other devices, this call is typically silently succeeds (and > doesn't cause any harm). In your case though, it would end up with a > deadlock because your particular i2c_gate_ctrl() function leaves the > "state->internal->tuner_lock" in a locked state. > > Now you may not run into this issue because today your frontend is > only used with a tuner that doesn't do this. However it is something > to watch out for in the future. > > Also, the dvb_frontend.c makes calls to i2c_gate_ctrl() at various > points, so you would need to ensure that none of those occur before > calling into your driver as there could potentially be a deadlock > there too. These demodulators generally don't need to do separate tuner programming even, it happens transparently. ie, in those cases, you don't need a tuner driver even. For the stv090x, the tuner control is within the acquisition loop of the demodulator itself. Even this wouldn't have been necessary, but it is kept for some very legacy use cases. Regards, Manu -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: PULL http://jusst.de/hg/stv090x
On Fri, Jan 22, 2010 at 2:11 PM, Manu Abraham wrote: > I think you understood incorrectly. Generally most demodulators have > an option to detect the I2C stop bit, thereby automatically disabling > the gate, while some don't. In those cases, you don't even need a > i2c_gate_control(disable) > It is that safe. Now, even if the gate happens to be open for some > reason, it won't have a large downside, just that the i2c transactions > might seep into the tuner silicon as RF noise. Yes, I was aware of this (although I might dispute whether *most* demodulators really operate this way). > Can you explain your logic a bit more in detail, please ? > > If you meant to imply the action on a failure case, it does unlock in > all possible cases as you can see... > http://jusst.de/hg/stv090x/rev/3a8f35abc0f2 I am not talking about the case where the tuning fails. I mean that not only is the i2c_gate_ctrl function used internally by stv090x.c, but it is also exposed via the dvb_frontend_ops struct. Depending on what tuner chip your frontend is being used with, I have seen cases where the tuner driver itself takes responsibility for opening/closing the gate. So you effectively end up with: frontend driver calls i2c_gate_ctrl(enable) frontend driver tells tuner driver to tune tuner driver calls i2c_gate_ctrl(enable) <--- Would cause deadlock in your driver tuner driver calls i2c_gate_ctrl(disable) tuner driver returns to caller frontend driver calls i2c_gate_ctrl(disable) With other devices, this call is typically silently succeeds (and doesn't cause any harm). In your case though, it would end up with a deadlock because your particular i2c_gate_ctrl() function leaves the "state->internal->tuner_lock" in a locked state. Now you may not run into this issue because today your frontend is only used with a tuner that doesn't do this. However it is something to watch out for in the future. Also, the dvb_frontend.c makes calls to i2c_gate_ctrl() at various points, so you would need to ensure that none of those occur before calling into your driver as there could potentially be a deadlock there too. Devin -- Devin J. Heitmueller - Kernel Labs http://www.kernellabs.com -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: PULL http://jusst.de/hg/stv090x
On Fri, Jan 22, 2010 at 10:43 PM, Devin Heitmueller wrote: > On Fri, Jan 22, 2010 at 1:23 PM, Manu Abraham wrote: >> The point there is that it is a dual demod which implements a FSM for >> it's tuner flow control and hence. The demodulators in the tree are >> generally single/standalone and don't need such a FSM to handle that. >> Even if it's a dual frontend, most of them just do locking for the >> register access, while here it is not register access locking, while >> it is controlling state, similar to Finite State Machine (FSM) > > Ok, I see what is going on now. I didn't notice before that the gate > enable was keeping the "state->internal->tuner_lock" mutex set even > after the call returned. > > My only concern here then is that I've seen cases where the > i2c_gate_ctrl() function is called in a non-symmetric fashion (where > the enable/disable calls are not necessarily properly balanced), > depending on the driver. While I am relatively confident that you > have balanced the instances where it is called within your driver, I > would be concerned about how it could possibly be called from other > places such as from the tuner driver or the dvb frontend core. I think you understood incorrectly. Generally most demodulators have an option to detect the I2C stop bit, thereby automatically disabling the gate, while some don't. In those cases, you don't even need a i2c_gate_control(disable) It is that safe. Now, even if the gate happens to be open for some reason, it won't have a large downside, just that the i2c transactions might seep into the tuner silicon as RF noise. In this case, it is not trying to balance of the gate control, whereas it is a state machine to safely control 2 tuner instances on the same demodulator. > Today, there is no real problem if a particular call path attempts to > enable the gate if it is already open (or disable if it is already > closed). With your proposed change, you will result in a deadlock. Can you explain your logic a bit more in detail, please ? If you meant to imply the action on a failure case, it does unlock in all possible cases as you can see... http://jusst.de/hg/stv090x/rev/3a8f35abc0f2 Regards, Manu -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: PULL http://jusst.de/hg/stv090x
On Fri, Jan 22, 2010 at 1:23 PM, Manu Abraham wrote: > The point there is that it is a dual demod which implements a FSM for > it's tuner flow control and hence. The demodulators in the tree are > generally single/standalone and don't need such a FSM to handle that. > Even if it's a dual frontend, most of them just do locking for the > register access, while here it is not register access locking, while > it is controlling state, similar to Finite State Machine (FSM) Ok, I see what is going on now. I didn't notice before that the gate enable was keeping the "state->internal->tuner_lock" mutex set even after the call returned. My only concern here then is that I've seen cases where the i2c_gate_ctrl() function is called in a non-symmetric fashion (where the enable/disable calls are not necessarily properly balanced), depending on the driver. While I am relatively confident that you have balanced the instances where it is called within your driver, I would be concerned about how it could possibly be called from other places such as from the tuner driver or the dvb frontend core. Today, there is no real problem if a particular call path attempts to enable the gate if it is already open (or disable if it is already closed). With your proposed change, you will result in a deadlock. Also, you might want to consider replacing the calls themselves with "fe->ops.i2c_gate_ctrl()" as opposed to calling the internal function directly, since that will allow the "frontend ioctl override" framework to continue to work properly. Devin -- Devin J. Heitmueller - Kernel Labs http://www.kernellabs.com -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: PULL http://jusst.de/hg/stv090x
On Fri, Jan 22, 2010 at 9:56 PM, Devin Heitmueller wrote: > On Fri, Jan 22, 2010 at 12:48 PM, Mauro Carvalho Chehab > wrote: > if (stv090x_i2c_gate_ctrl(fe, 1) < 0) > goto err; > > tuner access > > if (stv090x_i2c_gate_ctrl(fe, 0) < 0) > goto err; Ok. It is very unusual to have a lock internally like the above, since the code becomes poorly documented. >>> >>> >>> That's how a tuner is accessed for "any" dvb device. >> >> Yes, but that's not a function is expect to behave. In general, functions >> handle >> the lock/unlock inside it, returning the mutex unlocked. > > I'm confused - isn't this how pretty much *every* frontend does it's > locking? The i2c_gate_ctrl() callback is a standard component in the > DVB API. How is what Manu is doing different than any of the other > DVB drivers? > > While I agree that the name "i2c_gate_ctrl" is not what I would have > chosen, as far as I can tell this is how every DVB frontend does it. The point there is that it is a dual demod which implements a FSM for it's tuner flow control and hence. The demodulators in the tree are generally single/standalone and don't need such a FSM to handle that. Even if it's a dual frontend, most of them just do locking for the register access, while here it is not register access locking, while it is controlling state, similar to Finite State Machine (FSM) Regards, Manu -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: PULL http://jusst.de/hg/stv090x
On Fri, Jan 22, 2010 at 9:48 PM, Mauro Carvalho Chehab wrote: > Manu Abraham wrote: >> On Sat, Jan 16, 2010 at 5:14 PM, Mauro Carvalho Chehab >> wrote: >>> Hi Andreas, >>> >>> Andreas Regel wrote: >>> you don't have to look at stv090x_i2c_gate_ctrl alone. stv090x driver contains code like this: if (stv090x_i2c_gate_ctrl(fe, 1) < 0) goto err; tuner access if (stv090x_i2c_gate_ctrl(fe, 0) < 0) goto err; >>> Ok. It is very unusual to have a lock internally like the above, since >>> the code becomes poorly documented. >> >> >> That's how a tuner is accessed for "any" dvb device. > > Yes, but that's not a function is expect to behave. In general, functions > handle > the lock/unlock inside it, returning the mutex unlocked. > > In this case, the function returns with a mutex on a different state. This > should > be documented somehow. The better is to document at the name of the > function, as on other cases along the kernel. It's in fact a Finite State Machine or FSM as popularly known ... http://en.wikipedia.org/wiki/Finite_State_Machine >> >> >>> In the cases that we want to do things like that, it is documented like: >>> >>> if (stv090x_gate_enable_lock(fe) < 0) >>> goto err; >>> >>> tuner access >>> >>> if (stv090x_gate_disable_unlock(fe, 0) < 0) >>> goto err; >> >> >> >> To me, that looks horrible and do non-intuitive. I would have to read >> those functions additionally to understand the implications of the >> same. What you suggest, will look to a user at initial glance it would >> seem that you are trying to lock register access, while it is not. > > Some examples of similar functions on kernel: > > spin_lock_irqsave/spin_unlock_irqrestore > test_and_set_bit_lock/clear_bit_unlock > ttm_read_lock/ttm_read_unlock > queue_flag_test_and_clear > ... > > >> I don't agree with your comment on this one. > > If you didn't like the name, feel free to use another one, but this needs > to be documented somehow. Converting to a function a FSM, will definitely cause a confusion. Maybe an explanation why a FSM is needed there would be a better thing. Regards, Manu -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: PULL http://jusst.de/hg/stv090x
On Fri, Jan 22, 2010 at 12:48 PM, Mauro Carvalho Chehab wrote: if (stv090x_i2c_gate_ctrl(fe, 1) < 0) goto err; tuner access if (stv090x_i2c_gate_ctrl(fe, 0) < 0) goto err; >>> Ok. It is very unusual to have a lock internally like the above, since >>> the code becomes poorly documented. >> >> >> That's how a tuner is accessed for "any" dvb device. > > Yes, but that's not a function is expect to behave. In general, functions > handle > the lock/unlock inside it, returning the mutex unlocked. I'm confused - isn't this how pretty much *every* frontend does it's locking? The i2c_gate_ctrl() callback is a standard component in the DVB API. How is what Manu is doing different than any of the other DVB drivers? While I agree that the name "i2c_gate_ctrl" is not what I would have chosen, as far as I can tell this is how every DVB frontend does it. Devin -- Devin J. Heitmueller - Kernel Labs http://www.kernellabs.com -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: PULL http://jusst.de/hg/stv090x
Oliver Endriss wrote: > Hi, > > Mauro Carvalho Chehab wrote: >> Hi Andreas, >> >> Andreas Regel wrote: >> >>> you don't have to look at stv090x_i2c_gate_ctrl alone. stv090x driver >>> contains code like this: >>> >>> if (stv090x_i2c_gate_ctrl(fe, 1) < 0) >>> goto err; >>> >>> tuner access >>> >>> if (stv090x_i2c_gate_ctrl(fe, 0) < 0) >>> goto err; >> Ok. It is very unusual to have a lock internally like the above, since >> the code becomes poorly documented. >> >> In the cases that we want to do things like that, it is documented like: >> >> if (stv090x_gate_enable_lock(fe) < 0) >> goto err; >> >> tuner access >> >> if (stv090x_gate_disable_unlock(fe, 0) < 0) >> goto err; >> >> This way, it is clear to anyone that is reviewing the code that the functions >> are returning with the lock on a different state and prevent the kernel >> janitors >> to send patches that will try to "fix" the lock. > > You cannot do that, because there is _one_ entry in > "struct dvb_frontend_ops": > .i2c_gate_ctrl = stv090x_i2c_gate_ctrl; > This function must open/close the I2C gate. Yes, I'm aware of that. Maybe the better is to patch the KABI to have two separate functions, or to have a separate ops to handle the tuner mutex. > >> ... >> After the patch, the lock is fine, but we still need to better document it, >> in order to save time from reviewers and janitors. > > I agree that some comments should be added to stv090x_i2c_gate_ctrl, > explaining why the mutex is required. Ok, thanks. Cheers, Mauro. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: PULL http://jusst.de/hg/stv090x
Manu Abraham wrote: > On Sat, Jan 16, 2010 at 5:14 PM, Mauro Carvalho Chehab > wrote: >> Hi Andreas, >> >> Andreas Regel wrote: >> >>> you don't have to look at stv090x_i2c_gate_ctrl alone. stv090x driver >>> contains code like this: >>> >>> if (stv090x_i2c_gate_ctrl(fe, 1) < 0) >>> goto err; >>> >>> tuner access >>> >>> if (stv090x_i2c_gate_ctrl(fe, 0) < 0) >>> goto err; >> Ok. It is very unusual to have a lock internally like the above, since >> the code becomes poorly documented. > > > That's how a tuner is accessed for "any" dvb device. Yes, but that's not a function is expect to behave. In general, functions handle the lock/unlock inside it, returning the mutex unlocked. In this case, the function returns with a mutex on a different state. This should be documented somehow. The better is to document at the name of the function, as on other cases along the kernel. > > >> In the cases that we want to do things like that, it is documented like: >> >> if (stv090x_gate_enable_lock(fe) < 0) >> goto err; >> >> tuner access >> >> if (stv090x_gate_disable_unlock(fe, 0) < 0) >> goto err; > > > > To me, that looks horrible and do non-intuitive. I would have to read > those functions additionally to understand the implications of the > same. What you suggest, will look to a user at initial glance it would > seem that you are trying to lock register access, while it is not. Some examples of similar functions on kernel: spin_lock_irqsave/spin_unlock_irqrestore test_and_set_bit_lock/clear_bit_unlock ttm_read_lock/ttm_read_unlock queue_flag_test_and_clear ... > I don't agree with your comment on this one. If you didn't like the name, feel free to use another one, but this needs to be documented somehow. Maybe you could use, for example: tuner_lock/tuner_unlock tuner_opengate_lock/tuner_closegate_unlock ... Cheers, Mauro. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: PULL http://jusst.de/hg/stv090x
Hi, Mauro Carvalho Chehab wrote: > Hi Andreas, > > Andreas Regel wrote: > > > you don't have to look at stv090x_i2c_gate_ctrl alone. stv090x driver > > contains code like this: > > > > if (stv090x_i2c_gate_ctrl(fe, 1) < 0) > > goto err; > > > > tuner access > > > > if (stv090x_i2c_gate_ctrl(fe, 0) < 0) > > goto err; > > Ok. It is very unusual to have a lock internally like the above, since > the code becomes poorly documented. > > In the cases that we want to do things like that, it is documented like: > > if (stv090x_gate_enable_lock(fe) < 0) > goto err; > > tuner access > > if (stv090x_gate_disable_unlock(fe, 0) < 0) > goto err; > > This way, it is clear to anyone that is reviewing the code that the functions > are returning with the lock on a different state and prevent the kernel > janitors > to send patches that will try to "fix" the lock. You cannot do that, because there is _one_ entry in "struct dvb_frontend_ops": .i2c_gate_ctrl = stv090x_i2c_gate_ctrl; This function must open/close the I2C gate. > ... > After the patch, the lock is fine, but we still need to better document it, > in order to save time from reviewers and janitors. I agree that some comments should be added to stv090x_i2c_gate_ctrl, explaining why the mutex is required. CU Oliver -- VDR Remote Plugin 0.4.0: http://www.escape-edv.de/endriss/vdr/ 4 MByte Mod: http://www.escape-edv.de/endriss/dvb-mem-mod/ Full-TS Mod: http://www.escape-edv.de/endriss/dvb-full-ts-mod/ -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: PULL http://jusst.de/hg/stv090x
On Sat, Jan 16, 2010 at 5:14 PM, Mauro Carvalho Chehab wrote: > Hi Andreas, > > Andreas Regel wrote: > >> you don't have to look at stv090x_i2c_gate_ctrl alone. stv090x driver >> contains code like this: >> >> if (stv090x_i2c_gate_ctrl(fe, 1) < 0) >> goto err; >> >> tuner access >> >> if (stv090x_i2c_gate_ctrl(fe, 0) < 0) >> goto err; > > Ok. It is very unusual to have a lock internally like the above, since > the code becomes poorly documented. That's how a tuner is accessed for "any" dvb device. > In the cases that we want to do things like that, it is documented like: > > if (stv090x_gate_enable_lock(fe) < 0) > goto err; > > tuner access > > if (stv090x_gate_disable_unlock(fe, 0) < 0) > goto err; To me, that looks horrible and do non-intuitive. I would have to read those functions additionally to understand the implications of the same. What you suggest, will look to a user at initial glance it would seem that you are trying to lock register access, while it is not. I don't agree with your comment on this one. > This way, it is clear to anyone that is reviewing the code that the functions > are returning with the lock on a different state and prevent the kernel > janitors > to send patches that will try to "fix" the lock. > > I bet that their coccinelle rules will generate some false alarms with the > current code. We will wait and see, since there is nothing wrong in the semantic construct in there. It is just that the gate control for that device is slightly different from other devices, due to the difference in the device type and the mode of operation. Regards, Manu -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: PULL http://jusst.de/hg/stv090x
Hi Andreas, Andreas Regel wrote: > you don't have to look at stv090x_i2c_gate_ctrl alone. stv090x driver > contains code like this: > > if (stv090x_i2c_gate_ctrl(fe, 1) < 0) > goto err; > > tuner access > > if (stv090x_i2c_gate_ctrl(fe, 0) < 0) > goto err; Ok. It is very unusual to have a lock internally like the above, since the code becomes poorly documented. In the cases that we want to do things like that, it is documented like: if (stv090x_gate_enable_lock(fe) < 0) goto err; tuner access if (stv090x_gate_disable_unlock(fe, 0) < 0) goto err; This way, it is clear to anyone that is reviewing the code that the functions are returning with the lock on a different state and prevent the kernel janitors to send patches that will try to "fix" the lock. I bet that their coccinelle rules will generate some false alarms with the current code. > Intention of the patch is to make the tuner access exclusive. Thats the > reason why the mutex is locked when gate is opened and unlocked when the > gate is closed. > > In case the opening fails the close call is not made und thus mutex is > not unlocked twice. > > There one problem pending with: when there is an error during "tuner > access" the gate will not be closed and thus the mutex will not be > unlocked. For this case a patch from Oliver Endriss is attached. After the patch, the lock is fine, but we still need to better document it, in order to save time from reviewers and janitors. Cheers, Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: PULL http://jusst.de/hg/stv090x
Oliver Endriss wrote: > Mauro Carvalho Chehab wrote: >> Oliver Endriss wrote: >>> Manu Abraham wrote: On Mon, Jan 11, 2010 at 12:17 AM, Manu Abraham wrote: > Mauro, > > > Following the changesets 13830 - 13894 from my earlier pull request, Correction, 13880 - 13894 inclusive of both. > Please pull the following changeset as well > > > http://jusst.de/hg/stv090x/rev/80c74966d955 > > > It fixes a nasty bug as described at > http://osdir.com/ml/linux-media/2009-11/msg00643.html > > > Regards, > Manu >>> Mauro, >>> >>> when will you pull-in these changesets? >>> You are blocking the submission of the ngene driver. >> Hi Oliver, >> >> Since I returned from vacations this week, I had a pile of tasks to >> handle, both at the subsystem and at the work. >> >> Unfortunately, having to deal with both -hg and -git eats lot of my >> time, to be sure that nothing is missed. This time, I had to re-generate >> all my local -git trees that somehow had loose some patches in the process. >> Due to that, the patch merge is taking longer than I've expected. >> >> I still have a few issues pending at the subsystem, including this stv090x >> pull request, the omap pull (that it is very complex, as it requires both >> -hg and -git patch insertions), lnbh24 fix, my own proposals to dvb-apps, >> and two upstream submissions. >> >> My intention is to finish those tasks during this weekend if time >> allows me to do everything. >> >> In the specific case of stv090x, I intend to review again the locks, >> in the light of your comments. > > I just wondered why you did not pull-in these changesets. I finished reviewing the locking schema and it seemed OK with your fixes. Still, I think that the i2c_gate_control function is poorly documented. Somehow, it should be commented that the lock is taken when enable=1 and is dropped when enable=0, warning anyone that may be analyzing the code about that, especially since the lock function is exported to dvb core. It is not common to use a locking schema like that, where the information that the code is locked is hidden. IMO, the better way would be to split the function in two, like: i2c_gate_enable_lock(struct dvb_frontend *fe); i2c_gate_disable_unlock(struct dvb_frontend *fe); And use those two functions internally. You'll still need to have an i2c_gate_control() function that has the locks inside, due to the calls done at dvb_frontend.c, but the code will be better documented. In order to not add more delay to ngene, I'll apply the stv090x patches, but please better document the i2c_gate_control on your next patch series. Btw, with respect to dvb-core, we'll need to do soon a lock review, since dvbdev.c uses the BKL, and it will be removed on 2.6.34 or 2.6.35. > > The first lnbh24 patch was pulled so fast, that I had no chance to > respond. This patch was submitted much later than the stv090x stuff. > Btw, it breaks the ngene driver, so it is important to apply the > lnbh24 fix... Yes, it were submitted latter, but I've reviewed stv090x before lnbh24. Anyway, it is the next on my review list. > > CU > Oliver > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: PULL http://jusst.de/hg/stv090x
Mauro Carvalho Chehab wrote: > Oliver Endriss wrote: > > Manu Abraham wrote: > >> On Mon, Jan 11, 2010 at 12:17 AM, Manu Abraham > >> wrote: > >>> Mauro, > >>> > >>> > >>> Following the changesets 13830 - 13894 from my earlier pull request, > >> Correction, 13880 - 13894 inclusive of both. > >> > >>> Please pull the following changeset as well > >>> > >>> > >>> http://jusst.de/hg/stv090x/rev/80c74966d955 > >>> > >>> > >>> It fixes a nasty bug as described at > >>> http://osdir.com/ml/linux-media/2009-11/msg00643.html > >>> > >>> > >>> Regards, > >>> Manu > > > > Mauro, > > > > when will you pull-in these changesets? > > You are blocking the submission of the ngene driver. > > Hi Oliver, > > Since I returned from vacations this week, I had a pile of tasks to > handle, both at the subsystem and at the work. > > Unfortunately, having to deal with both -hg and -git eats lot of my > time, to be sure that nothing is missed. This time, I had to re-generate > all my local -git trees that somehow had loose some patches in the process. > Due to that, the patch merge is taking longer than I've expected. > > I still have a few issues pending at the subsystem, including this stv090x > pull request, the omap pull (that it is very complex, as it requires both > -hg and -git patch insertions), lnbh24 fix, my own proposals to dvb-apps, > and two upstream submissions. > > My intention is to finish those tasks during this weekend if time > allows me to do everything. > > In the specific case of stv090x, I intend to review again the locks, > in the light of your comments. I just wondered why you did not pull-in these changesets. The first lnbh24 patch was pulled so fast, that I had no chance to respond. This patch was submitted much later than the stv090x stuff. Btw, it breaks the ngene driver, so it is important to apply the lnbh24 fix... CU Oliver -- VDR Remote Plugin 0.4.0: http://www.escape-edv.de/endriss/vdr/ 4 MByte Mod: http://www.escape-edv.de/endriss/dvb-mem-mod/ Full-TS Mod: http://www.escape-edv.de/endriss/dvb-full-ts-mod/ -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: PULL http://jusst.de/hg/stv090x
Manu Abraham wrote: > On Mon, Jan 11, 2010 at 12:17 AM, Manu Abraham wrote: > > Mauro, > > > > > > Following the changesets 13830 - 13894 from my earlier pull request, > > Correction, 13880 - 13894 inclusive of both. > > > > > Please pull the following changeset as well > > > > > > http://jusst.de/hg/stv090x/rev/80c74966d955 > > > > > > It fixes a nasty bug as described at > > http://osdir.com/ml/linux-media/2009-11/msg00643.html > > > > > > Regards, > > Manu Mauro, when will you pull-in these changesets? You are blocking the submission of the ngene driver. CU Oliver -- VDR Remote Plugin 0.4.0: http://www.escape-edv.de/endriss/vdr/ 4 MByte Mod: http://www.escape-edv.de/endriss/dvb-mem-mod/ Full-TS Mod: http://www.escape-edv.de/endriss/dvb-full-ts-mod/ -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: PULL http://jusst.de/hg/stv090x
Oliver Endriss wrote: > Manu Abraham wrote: >> On Mon, Jan 11, 2010 at 12:17 AM, Manu Abraham >> wrote: >>> Mauro, >>> >>> >>> Following the changesets 13830 - 13894 from my earlier pull request, >> Correction, 13880 - 13894 inclusive of both. >> >>> Please pull the following changeset as well >>> >>> >>> http://jusst.de/hg/stv090x/rev/80c74966d955 >>> >>> >>> It fixes a nasty bug as described at >>> http://osdir.com/ml/linux-media/2009-11/msg00643.html >>> >>> >>> Regards, >>> Manu > > Mauro, > > when will you pull-in these changesets? > You are blocking the submission of the ngene driver. Hi Oliver, Since I returned from vacations this week, I had a pile of tasks to handle, both at the subsystem and at the work. Unfortunately, having to deal with both -hg and -git eats lot of my time, to be sure that nothing is missed. This time, I had to re-generate all my local -git trees that somehow had loose some patches in the process. Due to that, the patch merge is taking longer than I've expected. I still have a few issues pending at the subsystem, including this stv090x pull request, the omap pull (that it is very complex, as it requires both -hg and -git patch insertions), lnbh24 fix, my own proposals to dvb-apps, and two upstream submissions. My intention is to finish those tasks during this weekend if time allows me to do everything. In the specific case of stv090x, I intend to review again the locks, in the light of your comments. Cheers, Mauro. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: PULL http://jusst.de/hg/stv090x
On Mon, Jan 11, 2010 at 12:17 AM, Manu Abraham wrote: > Mauro, > > > Following the changesets 13830 - 13894 from my earlier pull request, Correction, 13880 - 13894 inclusive of both. > > Please pull the following changeset as well > > > http://jusst.de/hg/stv090x/rev/80c74966d955 > > > It fixes a nasty bug as described at > http://osdir.com/ml/linux-media/2009-11/msg00643.html > > > Regards, > Manu > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
PULL http://jusst.de/hg/stv090x
Mauro, Following the changesets 13830 - 13894 from my earlier pull request, Please pull the following changeset as well http://jusst.de/hg/stv090x/rev/80c74966d955 It fixes a nasty bug as described at http://osdir.com/ml/linux-media/2009-11/msg00643.html Regards, Manu -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: PULL http://jusst.de/hg/stv090x
Hi,
Manu Abraham wrote:
> On Sun, Jan 10, 2010 at 4:52 PM, Mauro Carvalho Chehab
> wrote:
> > Manu Abraham wrote:
> >> Mauro,
> >>
> >> Please pull http://jusst.de/hg/stv090x changesets: from 13880 - 13891
> >> inclusive of both.
> >
> > The third changeset has some locking troubles:
> >
> > # Node ID 4dd17a6a5ecc320eab58590a8567e5ba03b9d53a
> > [STV090x] Added mutex protection around tuner I2C access.
> >
> > After the patch, the code will look like:
> >
> > static int stv090x_i2c_gate_ctrl(struct dvb_frontend *fe, int enable)
> > {
> > struct stv090x_state *state = fe->demodulator_priv;
> > u32 reg;
> >
> > if (enable)
> > mutex_lock(&state->internal->tuner_lock);
> >
> > reg = STV090x_READ_DEMOD(state, I2CRPT);
> > if (enable) {
> > dprintk(FE_DEBUG, 1, "Enable Gate");
> > STV090x_SETFIELD_Px(reg, I2CT_ON_FIELD, 1);
> > if (STV090x_WRITE_DEMOD(state, I2CRPT, reg) < 0)
> > goto err;
> >
> > } else {
> >
> > dprintk(FE_DEBUG, 1, "Disable Gate");
> > STV090x_SETFIELD_Px(reg, I2CT_ON_FIELD, 0);
> > if ((STV090x_WRITE_DEMOD(state, I2CRPT, reg)) < 0)
> > goto err;
> > }
> >
> > if (!enable)
> > mutex_unlock(&state->internal->tuner_lock);
> >
> > return 0;
> > err:
> > dprintk(FE_ERROR, 1, "I/O error");
> > mutex_unlock(&state->internal->tuner_lock);
> > return -1;
> > }
> >
> > As the err: is called on both enable and disable conditions, the error
> > code will try to unlock an already unlocked mutex, if !enable.
Nak. stv090x_i2c_gate_ctrl will only be called with enable==false,
if it has previously been called successfully with enable==true.
> > Also, it doesn't make sense to lock only if the gate is enabled,
> > since it seems that the lock is meant to protect the i2c gate bus and
> > both conditions will call STV090x_SETFIELD_Px(reg, I2CT_ON_FIELD, enable ?
> > 1 : 0);
> >
> > The better would be simply to remove the if (enable)/if (!enable) tests
> > on the above code.
>
> It looks okay to my eyes. Any other pitfalls in the tuner operations
> can you think of ?
>
>
> Oliver,
>
> any thoughts that comes to your mind on this one ?
The purpuse of tuner_lock is to allow for 2 tuners with equal I2C
address on one I2C bus. Each one is behind its own I2C gate.
The code is a bit tricky. Lets analyse what may happen:
1. no error, enable==true:
lock mutex -> return 0
-> ok
2. no error, enable==false:
unlock mutex (which has been locked by a previous call), return 0
-> ok
3. error on enable:
mutex lock, mutex unlock, return -1
-> ok
4. error on disable:
unlock mutex (which has been locked by a previous call), return -1
-> ok
The code looks ok to me.
CU
Oliver
--
VDR Remote Plugin 0.4.0: http://www.escape-edv.de/endriss/vdr/
4 MByte Mod: http://www.escape-edv.de/endriss/dvb-mem-mod/
Full-TS Mod: http://www.escape-edv.de/endriss/dvb-full-ts-mod/
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: PULL http://jusst.de/hg/stv090x
On Sun, Jan 10, 2010 at 6:43 PM, Andreas Regel wrote:
> Hi Mauro,
>
> Am 10.01.2010 15:15, schrieb Mauro Carvalho Chehab:
>>
>> Andreas Regel wrote:
>>>
>>> Hi Mauro,
>>>
>>> Am 10.01.2010 13:52, schrieb Mauro Carvalho Chehab:
>>>>
>>>> Manu Abraham wrote:
>>>>>
>>>>> Mauro,
>>>>>
>>>>> Please pull http://jusst.de/hg/stv090x changesets: from 13880 - 13891
>>>>> inclusive of both.
>>>>
>>>> The third changeset has some locking troubles:
>>>>
>>>> # Node ID 4dd17a6a5ecc320eab58590a8567e5ba03b9d53a
>>>> [STV090x] Added mutex protection around tuner I2C access.
>>>>
>>>> After the patch, the code will look like:
>>>>
>>>> static int stv090x_i2c_gate_ctrl(struct dvb_frontend *fe, int enable)
>>>> {
>>>> struct stv090x_state *state = fe->demodulator_priv;
>>>> u32 reg;
>>>>
>>>> if (enable)
>>>> mutex_lock(&state->internal->tuner_lock);
>>>>
>>>> reg = STV090x_READ_DEMOD(state, I2CRPT);
>>>> if (enable) {
>>>> dprintk(FE_DEBUG, 1, "Enable Gate");
>>>> STV090x_SETFIELD_Px(reg, I2CT_ON_FIELD, 1);
>>>> if (STV090x_WRITE_DEMOD(state, I2CRPT, reg)< 0)
>>>> goto err;
>>>>
>>>> } else {
>>>>
>>>> dprintk(FE_DEBUG, 1, "Disable Gate");
>>>> STV090x_SETFIELD_Px(reg, I2CT_ON_FIELD, 0);
>>>> if ((STV090x_WRITE_DEMOD(state, I2CRPT, reg))< 0)
>>>> goto err;
>>>> }
>>>>
>>>> if (!enable)
>>>> mutex_unlock(&state->internal->tuner_lock);
>>>>
>>>> return 0;
>>>> err:
>>>> dprintk(FE_ERROR, 1, "I/O error");
>>>> mutex_unlock(&state->internal->tuner_lock);
>>>> return -1;
>>>> }
>>>>
>>>> As the err: is called on both enable and disable conditions, the error
>>>> code will try to unlock an already unlocked mutex, if !enable.
>>>>
>>>> Also, it doesn't make sense to lock only if the gate is enabled,
>>>> since it seems that the lock is meant to protect the i2c gate bus and
>>>> both conditions will call STV090x_SETFIELD_Px(reg, I2CT_ON_FIELD,
>>>> enable ? 1 : 0);
>>>>
>>>> The better would be simply to remove the if (enable)/if (!enable) tests
>>>> on the above code.
>>>
>>> The lock shall protect the access to the tuners and not to the registers
>>> itself, so it is locked when enable is set and unlocked when enable isn't
>>
>> Ok.
>>>
>>> The code between a call to stv090x_i2c_gate_ctrl(..., 1)
>>> and stv090x_i2c_gate_ctrl(..., 0) shall be exclusive in case both tuners
>>> have the same I2C address.
>>
>> By just looking at the i2c_gate_ctrl, it is not clear how do you expect
>> that
>> the locking will work. You should add a comment better explaining it.
>>
>> Also, as I pointed, if STV090x_WRITE_DEMOD fails, the unlock code will be
>> called
>> even if the gate is not enabled.
>>
>
> you don't have to look at stv090x_i2c_gate_ctrl alone. stv090x driver
> contains code like this:
>
> if (stv090x_i2c_gate_ctrl(fe, 1) < 0)
> goto err;
>
> tuner access
>
> if (stv090x_i2c_gate_ctrl(fe, 0) < 0)
> goto err;
>
> Intention of the patch is to make the tuner access exclusive. Thats the
> reason why the mutex is locked when gate is opened and unlocked when the
> gate is closed.
>
> In case the opening fails the close call is not made und thus mutex is not
> unlocked twice.
>
> There one problem pending with: when there is an error during "tuner access"
> the gate will not be closed and thus the mutex will not be unlocked. For
> this case a patch from Oliver Endriss is attached.
Mauro, Andreas,
Ok, I will queue up the other patch as well and issue another pull request.
Mauro,
Ignore the pull request for the moment.
Regards,
Manu
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: PULL http://jusst.de/hg/stv090x
Hi Mauro,
Am 10.01.2010 15:15, schrieb Mauro Carvalho Chehab:
Andreas Regel wrote:
Hi Mauro,
Am 10.01.2010 13:52, schrieb Mauro Carvalho Chehab:
Manu Abraham wrote:
Mauro,
Please pull http://jusst.de/hg/stv090x changesets: from 13880 - 13891
inclusive of both.
The third changeset has some locking troubles:
# Node ID 4dd17a6a5ecc320eab58590a8567e5ba03b9d53a
[STV090x] Added mutex protection around tuner I2C access.
After the patch, the code will look like:
static int stv090x_i2c_gate_ctrl(struct dvb_frontend *fe, int enable)
{
struct stv090x_state *state = fe->demodulator_priv;
u32 reg;
if (enable)
mutex_lock(&state->internal->tuner_lock);
reg = STV090x_READ_DEMOD(state, I2CRPT);
if (enable) {
dprintk(FE_DEBUG, 1, "Enable Gate");
STV090x_SETFIELD_Px(reg, I2CT_ON_FIELD, 1);
if (STV090x_WRITE_DEMOD(state, I2CRPT, reg)< 0)
goto err;
} else {
dprintk(FE_DEBUG, 1, "Disable Gate");
STV090x_SETFIELD_Px(reg, I2CT_ON_FIELD, 0);
if ((STV090x_WRITE_DEMOD(state, I2CRPT, reg))< 0)
goto err;
}
if (!enable)
mutex_unlock(&state->internal->tuner_lock);
return 0;
err:
dprintk(FE_ERROR, 1, "I/O error");
mutex_unlock(&state->internal->tuner_lock);
return -1;
}
As the err: is called on both enable and disable conditions, the error
code will try to unlock an already unlocked mutex, if !enable.
Also, it doesn't make sense to lock only if the gate is enabled,
since it seems that the lock is meant to protect the i2c gate bus and
both conditions will call STV090x_SETFIELD_Px(reg, I2CT_ON_FIELD,
enable ? 1 : 0);
The better would be simply to remove the if (enable)/if (!enable) tests
on the above code.
The lock shall protect the access to the tuners and not to the registers
itself, so it is locked when enable is set and unlocked when enable isn't
Ok.
The code between a call to stv090x_i2c_gate_ctrl(..., 1)
and stv090x_i2c_gate_ctrl(..., 0) shall be exclusive in case both tuners
have the same I2C address.
By just looking at the i2c_gate_ctrl, it is not clear how do you expect that
the locking will work. You should add a comment better explaining it.
Also, as I pointed, if STV090x_WRITE_DEMOD fails, the unlock code will be called
even if the gate is not enabled.
you don't have to look at stv090x_i2c_gate_ctrl alone. stv090x driver
contains code like this:
if (stv090x_i2c_gate_ctrl(fe, 1) < 0)
goto err;
tuner access
if (stv090x_i2c_gate_ctrl(fe, 0) < 0)
goto err;
Intention of the patch is to make the tuner access exclusive. Thats the
reason why the mutex is locked when gate is opened and unlocked when the
gate is closed.
In case the opening fails the close call is not made und thus mutex is
not unlocked twice.
There one problem pending with: when there is an error during "tuner
access" the gate will not be closed and thus the mutex will not be
unlocked. For this case a patch from Oliver Endriss is attached.
Regards,
Andreas
# HG changeset patch
# User Oliver Endriss
# Date 1263097942 -3600
# Node ID fefb0eb3c442f8ab1e446c6f275c914a99495312
# Parent b1e950fefc1ac04f3ef67d274d6a72859afd734f
stv090x: Disable I2C gate on error
From: Oliver Endriss
The I2C gate must also be disabled, if a tuner command failed.
Otherwise the tuner mutex would be locked forever.
Priority: normal
Signed-off-by: Oliver Endriss
diff -r b1e950fefc1a -r fefb0eb3c442 linux/drivers/media/dvb/frontends/stv090x.c
--- a/linux/drivers/media/dvb/frontends/stv090x.c Wed Jan 06 02:24:56
2010 +0400
+++ b/linux/drivers/media/dvb/frontends/stv090x.c Sun Jan 10 05:32:22
2010 +0100
@@ -2514,12 +2514,12 @@ static u32 stv090x_srate_srch_coarse(str
if (state->config->tuner_set_frequency) {
if (state->config->tuner_set_frequency(fe,
freq) < 0)
- goto err;
+ goto err_gateoff;
}
if (state->config->tuner_set_bandwidth) {
if (state->config->tuner_set_bandwidth(fe,
state->tuner_bw) < 0)
- goto err;
+ goto err_gateoff;
}
if (stv090x_i2c_gate_ctrl(fe, 0) < 0)
@@ -2532,7 +2532,7 @@ static u32 stv090x_srate_srch_coarse(str
if (state->config->tuner_get_status) {
if (state->
Re: PULL http://jusst.de/hg/stv090x
Andreas Regel wrote:
> Hi Mauro,
>
> Am 10.01.2010 13:52, schrieb Mauro Carvalho Chehab:
>> Manu Abraham wrote:
>>> Mauro,
>>>
>>> Please pull http://jusst.de/hg/stv090x changesets: from 13880 - 13891
>>> inclusive of both.
>>
>> The third changeset has some locking troubles:
>>
>> # Node ID 4dd17a6a5ecc320eab58590a8567e5ba03b9d53a
>> [STV090x] Added mutex protection around tuner I2C access.
>>
>> After the patch, the code will look like:
>>
>> static int stv090x_i2c_gate_ctrl(struct dvb_frontend *fe, int enable)
>> {
>> struct stv090x_state *state = fe->demodulator_priv;
>> u32 reg;
>>
>> if (enable)
>> mutex_lock(&state->internal->tuner_lock);
>>
>> reg = STV090x_READ_DEMOD(state, I2CRPT);
>> if (enable) {
>> dprintk(FE_DEBUG, 1, "Enable Gate");
>> STV090x_SETFIELD_Px(reg, I2CT_ON_FIELD, 1);
>> if (STV090x_WRITE_DEMOD(state, I2CRPT, reg)< 0)
>> goto err;
>>
>> } else {
>>
>> dprintk(FE_DEBUG, 1, "Disable Gate");
>> STV090x_SETFIELD_Px(reg, I2CT_ON_FIELD, 0);
>> if ((STV090x_WRITE_DEMOD(state, I2CRPT, reg))< 0)
>> goto err;
>> }
>>
>> if (!enable)
>> mutex_unlock(&state->internal->tuner_lock);
>>
>> return 0;
>> err:
>> dprintk(FE_ERROR, 1, "I/O error");
>> mutex_unlock(&state->internal->tuner_lock);
>> return -1;
>> }
>>
>> As the err: is called on both enable and disable conditions, the error
>> code will try to unlock an already unlocked mutex, if !enable.
>>
>> Also, it doesn't make sense to lock only if the gate is enabled,
>> since it seems that the lock is meant to protect the i2c gate bus and
>> both conditions will call STV090x_SETFIELD_Px(reg, I2CT_ON_FIELD,
>> enable ? 1 : 0);
>>
>> The better would be simply to remove the if (enable)/if (!enable) tests
>> on the above code.
>
> The lock shall protect the access to the tuners and not to the registers
> itself, so it is locked when enable is set and unlocked when enable isn't
Ok.
>
> The code between a call to stv090x_i2c_gate_ctrl(..., 1)
> and stv090x_i2c_gate_ctrl(..., 0) shall be exclusive in case both tuners
> have the same I2C address.
By just looking at the i2c_gate_ctrl, it is not clear how do you expect that
the locking will work. You should add a comment better explaining it.
Also, as I pointed, if STV090x_WRITE_DEMOD fails, the unlock code will be called
even if the gate is not enabled.
As the lock should be used only if gate is enabled, it would be cleaner if you
code the routine as:
static int stv090x_i2c_gate_ctrl(struct dvb_frontend *fe, int enable)
{
struct stv090x_state *state = fe->demodulator_priv;
u32 reg;
int rc = -EINVAL;
if (enable) {
/*
* (please better explain the lock - something like:
* Tuner access should be locked to avoid
* concurrent access when reading/writing to I2CRPT,
* since those calls do tuner access)
*/
mutex_lock(&state->internal->tuner_lock);
reg = STV090x_READ_DEMOD(state, I2CRPT);
dprintk(FE_DEBUG, 1, "Enable Gate");
STV090x_SETFIELD_Px(reg, I2CT_ON_FIELD, 1);
rc = STV090x_WRITE_DEMOD(state, I2CRPT, reg)
mutex_unlock(&state->internal->tuner_lock);
if (rc < 0)
goto err;
} else {
dprintk(FE_DEBUG, 1, "Disable Gate");
reg = STV090x_READ_DEMOD(state, I2CRPT);
STV090x_SETFIELD_Px(reg, I2CT_ON_FIELD, 0);
rc = STV090x_WRITE_DEMOD(state, I2CRPT, reg);
if (rc < 0)
goto err;
}
return 0;
err:
dprintk(FE_ERROR, 1, "I/O error");
return -rc;
}
This way, you avoid the risks of having lock troubles.
A side effect of the above code is that the routine will properly
propagate the error code produced at STV090x_WRITE_DEMOD(), instead
of returning -1. We should really avoid using -1 for errors, using, instead
an error code that it is defined on errors.h headers.
Cheers,
Mauro.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: PULL http://jusst.de/hg/stv090x
Mauro,
On Sun, Jan 10, 2010 at 4:52 PM, Mauro Carvalho Chehab
wrote:
> Manu Abraham wrote:
>> Mauro,
>>
>> Please pull http://jusst.de/hg/stv090x changesets: from 13880 - 13891
>> inclusive of both.
>
> The third changeset has some locking troubles:
>
> # Node ID 4dd17a6a5ecc320eab58590a8567e5ba03b9d53a
> [STV090x] Added mutex protection around tuner I2C access.
>
> After the patch, the code will look like:
>
> static int stv090x_i2c_gate_ctrl(struct dvb_frontend *fe, int enable)
> {
> struct stv090x_state *state = fe->demodulator_priv;
> u32 reg;
>
> if (enable)
> mutex_lock(&state->internal->tuner_lock);
>
> reg = STV090x_READ_DEMOD(state, I2CRPT);
> if (enable) {
> dprintk(FE_DEBUG, 1, "Enable Gate");
> STV090x_SETFIELD_Px(reg, I2CT_ON_FIELD, 1);
> if (STV090x_WRITE_DEMOD(state, I2CRPT, reg) < 0)
> goto err;
>
> } else {
>
> dprintk(FE_DEBUG, 1, "Disable Gate");
> STV090x_SETFIELD_Px(reg, I2CT_ON_FIELD, 0);
> if ((STV090x_WRITE_DEMOD(state, I2CRPT, reg)) < 0)
> goto err;
> }
>
> if (!enable)
> mutex_unlock(&state->internal->tuner_lock);
>
> return 0;
> err:
> dprintk(FE_ERROR, 1, "I/O error");
> mutex_unlock(&state->internal->tuner_lock);
> return -1;
> }
>
> As the err: is called on both enable and disable conditions, the error
> code will try to unlock an already unlocked mutex, if !enable.
>
> Also, it doesn't make sense to lock only if the gate is enabled,
> since it seems that the lock is meant to protect the i2c gate bus and
> both conditions will call STV090x_SETFIELD_Px(reg, I2CT_ON_FIELD, enable ? 1
> : 0);
>
> The better would be simply to remove the if (enable)/if (!enable) tests
> on the above code.
It looks okay to my eyes. Any other pitfalls in the tuner operations
can you think of ?
Oliver,
any thoughts that comes to your mind on this one ?
Regards,
Manu
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: PULL http://jusst.de/hg/stv090x
Hi Mauro,
Am 10.01.2010 13:52, schrieb Mauro Carvalho Chehab:
Manu Abraham wrote:
Mauro,
Please pull http://jusst.de/hg/stv090x changesets: from 13880 - 13891
inclusive of both.
The third changeset has some locking troubles:
# Node ID 4dd17a6a5ecc320eab58590a8567e5ba03b9d53a
[STV090x] Added mutex protection around tuner I2C access.
After the patch, the code will look like:
static int stv090x_i2c_gate_ctrl(struct dvb_frontend *fe, int enable)
{
struct stv090x_state *state = fe->demodulator_priv;
u32 reg;
if (enable)
mutex_lock(&state->internal->tuner_lock);
reg = STV090x_READ_DEMOD(state, I2CRPT);
if (enable) {
dprintk(FE_DEBUG, 1, "Enable Gate");
STV090x_SETFIELD_Px(reg, I2CT_ON_FIELD, 1);
if (STV090x_WRITE_DEMOD(state, I2CRPT, reg)< 0)
goto err;
} else {
dprintk(FE_DEBUG, 1, "Disable Gate");
STV090x_SETFIELD_Px(reg, I2CT_ON_FIELD, 0);
if ((STV090x_WRITE_DEMOD(state, I2CRPT, reg))< 0)
goto err;
}
if (!enable)
mutex_unlock(&state->internal->tuner_lock);
return 0;
err:
dprintk(FE_ERROR, 1, "I/O error");
mutex_unlock(&state->internal->tuner_lock);
return -1;
}
As the err: is called on both enable and disable conditions, the error
code will try to unlock an already unlocked mutex, if !enable.
Also, it doesn't make sense to lock only if the gate is enabled,
since it seems that the lock is meant to protect the i2c gate bus and
both conditions will call STV090x_SETFIELD_Px(reg, I2CT_ON_FIELD, enable ? 1 :
0);
The better would be simply to remove the if (enable)/if (!enable) tests
on the above code.
The lock shall protect the access to the tuners and not to the registers
itself, so it is locked when enable is set and unlocked when enable isn't.
The code between a call to stv090x_i2c_gate_ctrl(..., 1)
and stv090x_i2c_gate_ctrl(..., 0) shall be exclusive in case both tuners
have the same I2C address.
Regards,
Andreas
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: PULL http://jusst.de/hg/stv090x
Manu Abraham wrote:
> Mauro,
>
> Please pull http://jusst.de/hg/stv090x changesets: from 13880 - 13891
> inclusive of both.
The third changeset has some locking troubles:
# Node ID 4dd17a6a5ecc320eab58590a8567e5ba03b9d53a
[STV090x] Added mutex protection around tuner I2C access.
After the patch, the code will look like:
static int stv090x_i2c_gate_ctrl(struct dvb_frontend *fe, int enable)
{
struct stv090x_state *state = fe->demodulator_priv;
u32 reg;
if (enable)
mutex_lock(&state->internal->tuner_lock);
reg = STV090x_READ_DEMOD(state, I2CRPT);
if (enable) {
dprintk(FE_DEBUG, 1, "Enable Gate");
STV090x_SETFIELD_Px(reg, I2CT_ON_FIELD, 1);
if (STV090x_WRITE_DEMOD(state, I2CRPT, reg) < 0)
goto err;
} else {
dprintk(FE_DEBUG, 1, "Disable Gate");
STV090x_SETFIELD_Px(reg, I2CT_ON_FIELD, 0);
if ((STV090x_WRITE_DEMOD(state, I2CRPT, reg)) < 0)
goto err;
}
if (!enable)
mutex_unlock(&state->internal->tuner_lock);
return 0;
err:
dprintk(FE_ERROR, 1, "I/O error");
mutex_unlock(&state->internal->tuner_lock);
return -1;
}
As the err: is called on both enable and disable conditions, the error
code will try to unlock an already unlocked mutex, if !enable.
Also, it doesn't make sense to lock only if the gate is enabled,
since it seems that the lock is meant to protect the i2c gate bus and
both conditions will call STV090x_SETFIELD_Px(reg, I2CT_ON_FIELD, enable ? 1 :
0);
The better would be simply to remove the if (enable)/if (!enable) tests
on the above code.
>
> Regards,
> Manu
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
PULL http://jusst.de/hg/stv090x
Mauro, Please pull http://jusst.de/hg/stv090x changesets: from 13880 - 13891 inclusive of both. Regards, Manu -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
