Message ID | 4EE2B7BC.9090501@linuxtv.org (mailing list archive) |
---|---|
State | Accepted, archived |
Headers |
Received: from mail.tu-berlin.de ([130.149.7.33]) by www.linuxtv.org with esmtp (Exim 4.72) (envelope-from <linux-media-owner@vger.kernel.org>) id 1RZBsJ-0003EL-6E; Sat, 10 Dec 2011 02:37:15 +0100 X-tubIT-Incoming-IP: 209.132.180.67 Received: from vger.kernel.org ([209.132.180.67]) by mail.tu-berlin.de (exim-4.75/mailfrontend-3) with esmtp id 1RZBsI-0005vo-Di; Sat, 10 Dec 2011 02:37:14 +0100 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753191Ab1LJBhH (ORCPT <rfc822;hunold@linuxtv.org> + 4 others); Fri, 9 Dec 2011 20:37:07 -0500 Received: from ffm.saftware.de ([83.141.3.46]:47963 "EHLO ffm.saftware.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751978Ab1LJBhG (ORCPT <rfc822;linux-media@vger.kernel.org>); Fri, 9 Dec 2011 20:37:06 -0500 Received: from localhost (localhost [127.0.0.1]) by ffm.saftware.de (Postfix) with ESMTP id 783EC2C171C; Sat, 10 Dec 2011 02:37:03 +0100 (CET) X-Virus-Scanned: Debian amavisd-new at ffm.saftware.de Received: from ffm.saftware.de ([127.0.0.1]) by localhost (ffm.saftware.de [127.0.0.1]) (amavisd-new, port 10024) with LMTP id qPgRsWRY5o9f; Sat, 10 Dec 2011 02:37:02 +0100 (CET) Message-ID: <4EE2B7BC.9090501@linuxtv.org> Date: Sat, 10 Dec 2011 02:37:00 +0100 From: Andreas Oberritter <obi@linuxtv.org> User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:8.0) Gecko/20111124 Thunderbird/8.0 MIME-Version: 1.0 To: Mauro Carvalho Chehab <mchehab@redhat.com> CC: Devin Heitmueller <dheitmueller@kernellabs.com>, Antti Palosaari <crope@iki.fi>, linux-media@vger.kernel.org Subject: [PATCH] DVB: dvb_frontend: fix delayed thread exit References: <1323454852-7426-1-git-send-email-mchehab@redhat.com> <4EE252E5.2050204@iki.fi> <4EE25A3C.9040404@redhat.com> <4EE25CB4.3000501@iki.fi> <4EE287A9.3000502@redhat.com> <CAGoCfiyE8JhX5fT_SYjb6_X5Mkjx1Vx34_pKYaTjXu+muWxxwg@mail.gmail.com> <4EE29BA6.1030909@redhat.com> <4EE29D1A.6010900@redhat.com> In-Reply-To: <4EE29D1A.6010900@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org X-PMX-Version: 5.6.1.2065439, Antispam-Engine: 2.7.2.376379, Antispam-Data: 2011.12.10.12715 X-PMX-Spam: Gauge=IIIIIIII, Probability=8%, Report=' MULTIPLE_RCPTS 0.1, MSGID_ADDED_BY_MTA 0.05, BODYTEXTP_SIZE_3000_LESS 0, BODY_SIZE_2000_2999 0, BODY_SIZE_5000_LESS 0, BODY_SIZE_7000_LESS 0, __ANY_URI 0, __CP_URI_IN_BODY 0, __CT 0, __CTE 0, __CT_TEXT_PLAIN 0, __HAS_MSGID 0, __HAS_X_MAILING_LIST 0, __MIME_TEXT_ONLY 0, __MIME_VERSION 0, __MOZILLA_MSGID 0, __MULTIPLE_RCPTS_CC_X2 0, __SANE_MSGID 0, __SUBJ_ALPHA_END 0, __TO_MALFORMED_2 0, __URI_NO_WWW 0, __URI_NS , __USER_AGENT 0' X-LSpam-Score: -6.9 (------) X-LSpam-Report: No, score=-6.9 required=5.0 tests=BAYES_00=-1.9, RCVD_IN_DNSWL_HI=-5 autolearn=ham |
Commit Message
Andreas Oberritter
Dec. 10, 2011, 1:37 a.m. UTC
On 10.12.2011 00:43, Mauro Carvalho Chehab wrote: > On 09-12-2011 21:37, Mauro Carvalho Chehab wrote: >> On 09-12-2011 20:33, Devin Heitmueller wrote: >>> On Fri, Dec 9, 2011 at 5:11 PM, Mauro Carvalho Chehab >>> <mchehab@redhat.com> wrote: >>>>> Could someone explain reason for that? >>>> >>>> >>>> I dunno, but I think this needs to be fixed, at least when the frontend >>>> is opened with O_NONBLOCK. >>> >>> Are you doing the drx-k firmware load on dvb_init()? That could >>> easily take 4 seconds. >> >> No. The firmware were opened previously. > > Maybe the delay is due to this part of dvb_frontend.c: > > static int dvb_mfe_wait_time = 5; > ... > int mferetry = (dvb_mfe_wait_time << 1); > > mutex_unlock (&adapter->mfe_lock); > while (mferetry-- && (mfedev->users != -1 || > mfepriv->thread != NULL)) { > if(msleep_interruptible(500)) { > if(signal_pending(current)) > return -EINTR; > } > } I haven't looked at the mfe code, but in case it's waiting for the frontend thread to exit, there's a problem that causes the thread not to exit immediately. Here's a patch that's been sitting in my queue for a while: --- Signed-off-by: Andreas Oberritter <obi@linuxtv.org> -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Comments
On Fri, Dec 9, 2011 at 8:37 PM, Andreas Oberritter <obi@linuxtv.org> wrote: > On 10.12.2011 00:43, Mauro Carvalho Chehab wrote: >> On 09-12-2011 21:37, Mauro Carvalho Chehab wrote: >>> On 09-12-2011 20:33, Devin Heitmueller wrote: >>>> On Fri, Dec 9, 2011 at 5:11 PM, Mauro Carvalho Chehab >>>> <mchehab@redhat.com> wrote: >>>>>> Could someone explain reason for that? >>>>> >>>>> >>>>> I dunno, but I think this needs to be fixed, at least when the frontend >>>>> is opened with O_NONBLOCK. >>>> >>>> Are you doing the drx-k firmware load on dvb_init()? That could >>>> easily take 4 seconds. >>> >>> No. The firmware were opened previously. >> >> Maybe the delay is due to this part of dvb_frontend.c: >> >> static int dvb_mfe_wait_time = 5; >> ... >> int mferetry = (dvb_mfe_wait_time << 1); >> >> mutex_unlock (&adapter->mfe_lock); >> while (mferetry-- && (mfedev->users != -1 || >> mfepriv->thread != NULL)) { >> if(msleep_interruptible(500)) { >> if(signal_pending(current)) >> return -EINTR; >> } >> } > > I haven't looked at the mfe code, but in case it's waiting for the > frontend thread to exit, there's a problem that causes the thread > not to exit immediately. Here's a patch that's been sitting in my > queue for a while: > > --- > > Signed-off-by: Andreas Oberritter <obi@linuxtv.org> > > diff --git a/linux/drivers/media/dvb/dvb-core/dvb_frontend.c b/linux/drivers/media/dvb/dvb-core/dvb_frontend.c > index 7784d74..6823c2b 100644 > --- a/linux/drivers/media/dvb/dvb-core/dvb_frontend.c 2011-09-07 12:32:24.000000000 +0200 > +++ a/linux/drivers/media/dvb/dvb-core/dvb_frontend.c 2011-09-13 15:55:48.865742791 +0200 > @@ -514,7 +514,7 @@ > return 1; > > if (fepriv->dvbdev->writers == 1) > - if (time_after(jiffies, fepriv->release_jiffies + > + if (time_after_eq(jiffies, fepriv->release_jiffies + > dvb_shutdown_timeout * HZ)) > return 1; > > @@ -2070,12 +2070,15 @@ > > dprintk ("%s\n", __func__); > > - if ((file->f_flags & O_ACCMODE) != O_RDONLY) > + if ((file->f_flags & O_ACCMODE) != O_RDONLY) { > fepriv->release_jiffies = jiffies; > + mb(); > + } > > ret = dvb_generic_release (inode, file); > > if (dvbdev->users == -1) { > + wake_up(&fepriv->wait_queue); > if (fepriv->exit != DVB_FE_NO_EXIT) { > fops_put(file->f_op); > file->f_op = NULL; This patch needs to have a much better explanation of exactly what it does and what problem it solves. We have a history of race conditions in dvb_frontend.c, and it's patches like this with virtually no details just makes it worse. I'm not arguing the actual merits of the code change - it *may* be correct. But without the appropriate background there is no real way of knowing... Mauro, this patch should be NACK'd and resubmitted with a detailed explanation of the current behavior, what the problem is, and how the code changes proposed solve that problem. Devin
On 10.12.2011 02:59, Devin Heitmueller wrote: > On Fri, Dec 9, 2011 at 8:37 PM, Andreas Oberritter <obi@linuxtv.org> wrote: >> On 10.12.2011 00:43, Mauro Carvalho Chehab wrote: >>> On 09-12-2011 21:37, Mauro Carvalho Chehab wrote: >>>> On 09-12-2011 20:33, Devin Heitmueller wrote: >>>>> On Fri, Dec 9, 2011 at 5:11 PM, Mauro Carvalho Chehab >>>>> <mchehab@redhat.com> wrote: >>>>>>> Could someone explain reason for that? >>>>>> >>>>>> >>>>>> I dunno, but I think this needs to be fixed, at least when the frontend >>>>>> is opened with O_NONBLOCK. >>>>> >>>>> Are you doing the drx-k firmware load on dvb_init()? That could >>>>> easily take 4 seconds. >>>> >>>> No. The firmware were opened previously. >>> >>> Maybe the delay is due to this part of dvb_frontend.c: >>> >>> static int dvb_mfe_wait_time = 5; >>> ... >>> int mferetry = (dvb_mfe_wait_time << 1); >>> >>> mutex_unlock (&adapter->mfe_lock); >>> while (mferetry-- && (mfedev->users != -1 || >>> mfepriv->thread != NULL)) { >>> if(msleep_interruptible(500)) { >>> if(signal_pending(current)) >>> return -EINTR; >>> } >>> } >> >> I haven't looked at the mfe code, but in case it's waiting for the >> frontend thread to exit, there's a problem that causes the thread >> not to exit immediately. Here's a patch that's been sitting in my >> queue for a while: >> >> --- >> >> Signed-off-by: Andreas Oberritter <obi@linuxtv.org> >> >> diff --git a/linux/drivers/media/dvb/dvb-core/dvb_frontend.c b/linux/drivers/media/dvb/dvb-core/dvb_frontend.c >> index 7784d74..6823c2b 100644 >> --- a/linux/drivers/media/dvb/dvb-core/dvb_frontend.c 2011-09-07 12:32:24.000000000 +0200 >> +++ a/linux/drivers/media/dvb/dvb-core/dvb_frontend.c 2011-09-13 15:55:48.865742791 +0200 >> @@ -514,7 +514,7 @@ >> return 1; >> >> if (fepriv->dvbdev->writers == 1) >> - if (time_after(jiffies, fepriv->release_jiffies + >> + if (time_after_eq(jiffies, fepriv->release_jiffies + >> dvb_shutdown_timeout * HZ)) >> return 1; >> >> @@ -2070,12 +2070,15 @@ >> >> dprintk ("%s\n", __func__); >> >> - if ((file->f_flags & O_ACCMODE) != O_RDONLY) >> + if ((file->f_flags & O_ACCMODE) != O_RDONLY) { >> fepriv->release_jiffies = jiffies; >> + mb(); >> + } >> >> ret = dvb_generic_release (inode, file); >> >> if (dvbdev->users == -1) { >> + wake_up(&fepriv->wait_queue); >> if (fepriv->exit != DVB_FE_NO_EXIT) { >> fops_put(file->f_op); >> file->f_op = NULL; > > This patch needs to have a much better explanation of exactly what it > does and what problem it solves. We have a history of race conditions > in dvb_frontend.c, and it's patches like this with virtually no > details just makes it worse. > > I'm not arguing the actual merits of the code change - it *may* be > correct. But without the appropriate background there is no real way > of knowing... > > Mauro, this patch should be NACK'd and resubmitted with a detailed > explanation of the current behavior, what the problem is, and how the > code changes proposed solve that problem. WTF, Devin, you again? I haven't asked anyone to upstream it. Feel free to analyze the code and resubmit it. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Andreas, On Fri, Dec 9, 2011 at 9:06 PM, Andreas Oberritter <obi@linuxtv.org> wrote: > WTF, Devin, you again? I haven't asked anyone to upstream it. Feel free > to analyze the code and resubmit it. 1. It's marked with a subject line that starts with "[PATCH]" 2. It has your SIgned-Off-By line. 3. it was sent to the mailing list. 4. It doesn't have any keywords like "RFC" or "proposed". If you don't intend for it to go upstream then don't do all of the above. I'm not sure if your "WTF, Devin, you again?" is some indication that I'm annoying you. The last patch you submitted that touches the threading in dvb_frontend.c had a host of problems and was clearly not well researched (i.e. "DVB: dvb_frontend: convert semaphore to mutex"). As in this case, there is no background indicating that this patch has been fully thought out and due diligence has been done. Maybe you have thoroughly researched the change, taken the time to fully understand its effects, and tested it with a variety of boards and scenarios. Without a good patch description, there is no way to know. I apologize if you're inconvenienced if I'm making an active effort to prevent poorly documented changes from getting merged (which often result in regressions). Oh wait, I'm not sorry at all. Nevermind. Devin
On 10-12-2011 00:25, Devin Heitmueller wrote: > Hello Andreas, > > On Fri, Dec 9, 2011 at 9:06 PM, Andreas Oberritter<obi@linuxtv.org> wrote: >> WTF, Devin, you again? I haven't asked anyone to upstream it. Feel free >> to analyze the code and resubmit it. > > 1. It's marked with a subject line that starts with "[PATCH]" > 2. It has your SIgned-Off-By line. > 3. it was sent to the mailing list. > 4. It doesn't have any keywords like "RFC" or "proposed". Devin, You're over-reacting. This patch were a reply from Andreas to a thread, and not a separate patch submission. Patches like are generally handled as RFC, especially since it doesn't contain a description. > If you don't intend for it to go upstream then don't do all of the above. > > I'm not sure if your "WTF, Devin, you again?" is some indication that > I'm annoying you. The last patch you submitted that touches the > threading in dvb_frontend.c had a host of problems and was clearly not > well researched (i.e. "DVB: dvb_frontend: convert semaphore to > mutex"). As in this case, there is no background indicating that this > patch has been fully thought out and due diligence has been done. > > Maybe you have thoroughly researched the change, taken the time to > fully understand its effects, and tested it with a variety of boards > and scenarios. Without a good patch description, there is no way to > know. > > I apologize if you're inconvenienced if I'm making an active effort to > prevent poorly documented changes from getting merged (which often > result in regressions). Oh wait, I'm not sorry at all. Nevermind. > > Devin > Regards, Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09-12-2011 23:37, Andreas Oberritter wrote: > On 10.12.2011 00:43, Mauro Carvalho Chehab wrote: >> On 09-12-2011 21:37, Mauro Carvalho Chehab wrote: >>> On 09-12-2011 20:33, Devin Heitmueller wrote: >>>> On Fri, Dec 9, 2011 at 5:11 PM, Mauro Carvalho Chehab >>>> <mchehab@redhat.com> wrote: >>>>>> Could someone explain reason for that? >>>>> >>>>> >>>>> I dunno, but I think this needs to be fixed, at least when the frontend >>>>> is opened with O_NONBLOCK. >>>> >>>> Are you doing the drx-k firmware load on dvb_init()? That could >>>> easily take 4 seconds. >>> >>> No. The firmware were opened previously. >> >> Maybe the delay is due to this part of dvb_frontend.c: >> >> static int dvb_mfe_wait_time = 5; >> ... >> int mferetry = (dvb_mfe_wait_time<< 1); >> >> mutex_unlock (&adapter->mfe_lock); >> while (mferetry--&& (mfedev->users != -1 || >> mfepriv->thread != NULL)) { >> if(msleep_interruptible(500)) { >> if(signal_pending(current)) >> return -EINTR; >> } >> } > > I haven't looked at the mfe code, but in case it's waiting for the > frontend thread to exit, there's a problem that causes the thread > not to exit immediately. Here's a patch that's been sitting in my > queue for a while: > > --- > > Signed-off-by: Andreas Oberritter<obi@linuxtv.org> Andreas, Thanks for the patch! Devin, > diff --git a/linux/drivers/media/dvb/dvb-core/dvb_frontend.c b/linux/drivers/media/dvb/dvb-core/dvb_frontend.c > index 7784d74..6823c2b 100644 > --- a/linux/drivers/media/dvb/dvb-core/dvb_frontend.c 2011-09-07 12:32:24.000000000 +0200 > +++ a/linux/drivers/media/dvb/dvb-core/dvb_frontend.c 2011-09-13 15:55:48.865742791 +0200 > @@ -514,7 +514,7 @@ > return 1; > > if (fepriv->dvbdev->writers == 1) > - if (time_after(jiffies, fepriv->release_jiffies + > + if (time_after_eq(jiffies, fepriv->release_jiffies + > dvb_shutdown_timeout * HZ)) The only change here is that it will now use dvb_shutdown_timeout instead of (dvb_shutdown_timeout * HZ + 1). This makes sense. > return 1; > > @@ -2070,12 +2070,15 @@ > > dprintk ("%s\n", __func__); > > - if ((file->f_flags& O_ACCMODE) != O_RDONLY) > + if ((file->f_flags& O_ACCMODE) != O_RDONLY) { > fepriv->release_jiffies = jiffies; > + mb(); This is just a memory barrier to warrant that all CPU's will consider the new value for release_jiffies. Probably Andreas added it because he noticed some race condition. In any case, this won't cause any regressions. > + } > > ret = dvb_generic_release (inode, file); > > if (dvbdev->users == -1) { > + wake_up(&fepriv->wait_queue); This is the only hook that changes the core behavior. > if (fepriv->exit != DVB_FE_NO_EXIT) { > fops_put(file->f_op); > file->f_op = NULL; With this change, the current code at dvb_frontend_release() wil; be: ret = dvb_generic_release (inode, file); if (dvbdev->users == -1) { wake_up(&fepriv->wait_queue); if (fepriv->exit != DVB_FE_NO_EXIT) { fops_put(file->f_op); file->f_op = NULL; wake_up(&dvbdev->wait_queue); } if (fe->ops.ts_bus_ctrl) fe->ops.ts_bus_ctrl(fe, 0); } The addition of a wake_up there is that the wake_up thread will be called also when fepriv->exit == DVB_FE_NO_EXIT. This seems to make sense, as dvb_frontend_thread() explicitly tests for it at: wait_event_interruptible_timeout(fepriv->wait_queue, dvb_frontend_should_wakeup(fe) || kthread_should_stop() || freezing(current), fepriv->delay); as dvb_frontend_should_wakeup(fe) is defined (after applying this patch) as: static int dvb_frontend_is_exiting(struct dvb_frontend *fe) { struct dvb_frontend_private *fepriv = fe->frontend_priv; if (fepriv->exit != DVB_FE_NO_EXIT) return 1; if (fepriv->dvbdev->writers == 1) if (time_after_eq(jiffies, fepriv->release_jiffies + dvb_shutdown_timeout * HZ)) return 1; return 0; } static int dvb_frontend_should_wakeup(struct dvb_frontend *fe) { struct dvb_frontend_private *fepriv = fe->frontend_priv; if (fepriv->wakeup) { fepriv->wakeup = 0; return 1; } return dvb_frontend_is_exiting(fe); } So, this code makes sense to me. Btw, a wait queue can wait even without an explicit call, since it is just something like [1]: do schedule() while (!condition); So, all this patch would hurt would be to increase the chance for us to detect a bug that it is already there. Devin, I'll do some tests here with a few devices, but, in principle, I don't see any reason why not applying this patch. So, except if I detect something wrong on my tests, of if you you point us for a regression caused by this change, I'll apply it. Of course, it would be nice if Andreas could add some comments, but if he doesn't, I can write something. It won't be the first patch that the maintainer would need to insert some description. Regards, Mauro. [1] The actual implementation is a more complex than that loop. In this specific case, as it uses the interruptible version, any signal would also wake this thread. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Mauro, On Sat, Dec 10, 2011 at 5:28 AM, Mauro Carvalho Chehab <mchehab@redhat.com> wrote: > Devin, > > You're over-reacting. This patch were a reply from Andreas to a thread, > and not a separate patch submission. > > Patches like are generally handled as RFC, especially since it doesn't > contain a description. Any email that starts with "WTF, Devin, you again?" will probably not get a very polite response. I agree there's been some overreaction, but it hasn't been on my part. He took the time to split it onto a new thread, add the subject line "PATCH", as well as adding an SOB. Even if his intent was only to get it reviewed, why should I waste half an hour of my time analyzing his patch to try to figure out his intent if he isn't willing to simply document it? You have a history of blindly accepting such patches without review. My only intent was to flag this patch to ensure that this didn't happen here. I've spent way more time than I should have to fixing obscure race conditions in dvb core. If the author of a patch cannot take the time to document his findings to provide context then the patch should be rejected without review until he does so. And why isn't this broken into a patch series? Even after you analyzed the patch you still don't understand what the changes do and why there are being made. Your explanation for why he added the "mb()" call was because "Probably Andreas added it because he noticed some race condition". What is the race condition? Did he find multiple race conditions? Is this patch multiple fixes for a race condition and some other crap at the same time? If a developer wants a patch reviewed (as Andreas suggested was the case here after-the-fact), then here's my feedback: break this into a series of small incremental patches which *in detail* describe the problem that was found and how each patch addresses the issue. Once we have that, the maintainer can do a more in-depth analysis of whether the patch should be accepted. Code whose function cannot be explicitly justified but simply 'looks better' should not be mixed in with real functional changes. Devin
On 10-12-2011 11:43, Devin Heitmueller wrote: > Hello Mauro, > > On Sat, Dec 10, 2011 at 5:28 AM, Mauro Carvalho Chehab > <mchehab@redhat.com> wrote: >> Devin, >> >> You're over-reacting. This patch were a reply from Andreas to a thread, >> and not a separate patch submission. >> >> Patches like are generally handled as RFC, especially since it doesn't >> contain a description. > > Any email that starts with "WTF, Devin, you again?" will probably not > get a very polite response. > > I agree there's been some overreaction, but it hasn't been on my part. > He took the time to split it onto a new thread, add the subject line > "PATCH", as well as adding an SOB. Even if his intent was only to get > it reviewed, why should I waste half an hour of my time analyzing his > patch to try to figure out his intent if he isn't willing to simply > document it? Both overacted, but this doesn't bring anything good. > You have a history of blindly accepting such patches without review. No. I always review all patches I receive. Yeah, I have to confess: I'm not a robot, I'm not infallible ;) (well, even a robot would hardly be infallible, anyway). > My only intent was to flag this patch to ensure that this didn't > happen here. I've spent way more time than I should have to fixing > obscure race conditions in dvb core. If the author of a patch cannot > take the time to document his findings to provide context then the > patch should be rejected without review until he does so. > > And why isn't this broken into a patch series? Even after you > analyzed the patch you still don't understand what the changes do and > why there are being made. Your explanation for why he added the > "mb()" call was because "Probably Andreas added it because he noticed > some race condition". What is the race condition? Did he find > multiple race conditions? Is this patch multiple fixes for a race > condition and some other crap at the same time? > > If a developer wants a patch reviewed (as Andreas suggested was the > case here after-the-fact), then here's my feedback: break this into a > series of small incremental patches which *in detail* describe the > problem that was found and how each patch addresses the issue. Once > we have that, the maintainer can do a more in-depth analysis of > whether the patch should be accepted. Code whose function cannot be > explicitly justified but simply 'looks better' should not be mixed in > with real functional changes. I understand that you want patches better documented, so do I, and it would be great if this patch had a better description since the beginning. Yet, I don't agree that this patch should be split. It does just one thing: it fixes the timeout handling for the dvb core frontend thread. Regards, Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/linux/drivers/media/dvb/dvb-core/dvb_frontend.c b/linux/drivers/media/dvb/dvb-core/dvb_frontend.c index 7784d74..6823c2b 100644 --- a/linux/drivers/media/dvb/dvb-core/dvb_frontend.c 2011-09-07 12:32:24.000000000 +0200 +++ a/linux/drivers/media/dvb/dvb-core/dvb_frontend.c 2011-09-13 15:55:48.865742791 +0200 @@ -514,7 +514,7 @@ return 1; if (fepriv->dvbdev->writers == 1) - if (time_after(jiffies, fepriv->release_jiffies + + if (time_after_eq(jiffies, fepriv->release_jiffies + dvb_shutdown_timeout * HZ)) return 1; @@ -2070,12 +2070,15 @@ dprintk ("%s\n", __func__); - if ((file->f_flags & O_ACCMODE) != O_RDONLY) + if ((file->f_flags & O_ACCMODE) != O_RDONLY) { fepriv->release_jiffies = jiffies; + mb(); + } ret = dvb_generic_release (inode, file); if (dvbdev->users == -1) { + wake_up(&fepriv->wait_queue); if (fepriv->exit != DVB_FE_NO_EXIT) { fops_put(file->f_op); file->f_op = NULL;