On Thu, Sep 20, 2012 at 1:47 PM, Michael Krufky <mkrufky@xxxxxxxxxxx> wrote: > On Mon, Aug 6, 2012 at 3:13 PM, Antti Palosaari <crope@xxxxxx> wrote: >> On 08/06/2012 09:57 PM, Michael Krufky wrote: >>> >>> On Mon, Aug 6, 2012 at 2:35 PM, Devin Heitmueller >>> <dheitmueller@xxxxxxxxxxxxxx> wrote: >>>> >>>> On Mon, Aug 6, 2012 at 2:19 PM, Antti Palosaari <crope@xxxxxx> wrote: >>>>> >>>>> You should understand from DVB driver model: >>>>> * attach() called only once when driver is loaded >>>>> * init() called to wake-up device >>>>> * sleep() called to sleep device >>>>> >>>>> What I would like to say is that there is very big risk to shoot own leg >>>>> when doing some initialization on attach(). For example resuming from >>>>> the >>>>> suspend could cause device reset and if you rely some settings that are >>>>> done >>>>> during attach() you will likely fail as Kernel / USB-host controller has >>>>> reset your device. >>>>> >>>>> See reset_resume from Kernel documentation: >>>>> Documentation/usb/power-management.txt >>>> >>>> >>>> Be forewarned: there is a very high likelihood that this patch will >>>> cause regressions on hybrid devices due to known race conditions >>>> related to dvb_frontend sleeping the tuner asynchronously on close. >>>> This is a hybrid tuner, and unless code is specifically added to the >>>> bridge or tuner driver, going from digital to analog mode too quickly >>>> will cause the tuner to be shutdown while it's actively in analog >>>> mode. >>>> >>>> (I discovered this the hard way when working on problems MythTV users >>>> reported against the HVR-950q). >>>> >>>> Description of race: >>>> >>>> 1. User opens DVB frontend tunes >>>> 2. User closes DVB frontend >>>> 3. User *immediately* opens V4L device using same tuner >>>> 4. User performs tuning request for analog >>>> 5. DVB frontend issues sleep() call to tuner, causing analog tuning to >>>> fail. >>>> >>>> This class of problem isn't seen on DVB only devices or those that >>>> have dedicated digital tuners not shared for analog usage. And in >>>> some cases it isn't noticed because a delay between closing the DVB >>>> device and opening the analog devices causes the sleep() call to >>>> happen before the analog tune (in which case you don't hit the race). >>>> >>>> I'm certainly not against improved power management, but it will >>>> require the race conditions to be fixed first in order to avoid >>>> regressions. >>>> >>>> Devin >>>> >>>> -- >>>> Devin J. Heitmueller - Kernel Labs >>>> http://www.kernellabs.com >>> >>> >>> Devin's right. I'm sorry, please *don't* merge the patch, Mauro. >>> Antti, you should just call sleep from your driver after attach(), as >>> I had previously suggested. We can revisit this some time in the >>> future after we can be sure that these race conditions wont occur. >> >> >> OK, maybe it is safer then. I have no any hybrid hardware to test... >> >> Anyhow, I wonder how many years it will take to resolve that V4L2/DVB API >> hybrid usage pŕoblem. I ran thinking that recently when looked how to >> implement DVB SDR for V4L2 API... I could guess problem will not disappear >> near future even analog TV disappears, because there is surely coming new >> nasty features which spreads over both V4L2 and DVB APIs. > > Guys, > > Please take another look at this branch and test if possible -- I > pushed an additional patch that takes Devin's concerns into account. > After applying these patches, the tda18271 driver will behave as it > always has, but it will sleep the tuner after attaching the first > instance. If there is only one instance, then this works exactly as > Antti desires. If there are more instances, then the tuner will only > be woken up again during attach if the tda18271_need_cal_on_startup() > returns non-zero. The driver does not attempt to re-sleep the > hardware again during successive attach() calls. > > I have not tested this yet myself, but I believe it resolves the > matter -- please comment. > > Regards, > > Mike Krufky ...in case the URL got lost: The following changes since commit 0c7d5a6da75caecc677be1fda207b7578936770d: Linux 3.5-rc5 (2012-07-03 22:57:41 +0300) are available in the git repository at: git://linuxtv.org/mkrufky/tuners tda18271 for you to fetch changes up to 4e46c5d1bbb920165fecfe7de18b2c01d9787230: tda18271: make 'low-power standby mode after attach' multi-instance safe (2012-09-20 13:34:29 -0400) ---------------------------------------------------------------- Michael Krufky (2): tda18271: enter low-power standby mode at the end of tda18271_attach() tda18271: make 'low-power standby mode after attach' multi-instance safe drivers/media/common/tuners/tda18271-fe.c | 4 ++++ 1 file changed, 4 insertions(+) Best regards, Mike -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
