LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Herbert Poetzl <herbert@13thfloor.at>
To: Cedric Le Goater <clg@fr.ibm.com>
Cc: "Eric W. Biederman" <ebiederm@xmission.com>,
	containers@lists.osdl.org,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	v4l-dvb-maintainer@linuxtv.org, Andrew Morton <akpm@osdl.org>,
	Andrew de Quincey <adq_dvb@lidskialf.net>
Subject: Re: [PATCH/RFC] kthread API conversion for dvb_frontend and av7110
Date: Fri, 15 Sep 2006 00:10:24 +0200	[thread overview]
Message-ID: <20060914221024.GB26916@MAIL.13thfloor.at> (raw)
In-Reply-To: <4509C4A5.5030600@fr.ibm.com>

On Thu, Sep 14, 2006 at 11:07:49PM +0200, Cedric Le Goater wrote:
> Herbert Poetzl wrote:
> > Okay, as I promised, I had a first shot at the
> > dvb kernel_thread to kthread API port, and here
> > is the result, which is running fine here since
> > yesterday, including module load/unload and
> > software suspend (which doesn't work as expected
> > with or without this patch :),
> 
> So you have such an hardware ? 

yes, I do .. that's how I tested it :)

> [ ...  ]
> 
> > @@ -600,7 +595,6 @@ static int dvb_frontend_thread(void *dat
> >  
> >  static void dvb_frontend_stop(struct dvb_frontend *fe)
> >  {
> > -	unsigned long ret;
> >  	struct dvb_frontend_private *fepriv = fe->frontend_priv;
> >  
> >  	dprintk ("%s\n", __FUNCTION__);
> > @@ -608,33 +602,17 @@ static void dvb_frontend_stop(struct dvb
> >  	fepriv->exit = 1;
> 
> do we still need the ->exit flag now that we are using kthread_stop() ? 
> same question for ->wakeup ? 

probably not, but I didn't want to change too
much on the first try, especially I'd appreciate
some feedback to this from the maintainer(s)

> >  	mb();
> >  
> > -	if (!fepriv->thread_pid)
> > -		return;
> > -
> > -	/* check if the thread is really alive */
> > -	if (kill_proc(fepriv->thread_pid, 0, 1) == -ESRCH) {
> > -		printk("dvb_frontend_stop: thread PID %d already died\n",
> > -				fepriv->thread_pid);
> > -		/* make sure the mutex was not held by the thread */
> > -		init_MUTEX (&fepriv->sem);
> > +	if (!fepriv->thread)
> >  		return;
> > -	}
> > -
> > -	/* wake up the frontend thread, so it notices that fe->exit == 1 */
> > -	dvb_frontend_wakeup(fe);
> >  
> > -	/* wait until the frontend thread has exited */
> > -	ret = wait_event_interruptible(fepriv->wait_queue,0 == fepriv->thread_pid);
> > -	if (-ERESTARTSYS != ret) {
> > -		fepriv->state = FESTATE_IDLE;
> > -		return;
> > -	}
> > +	kthread_stop(fepriv->thread);
> > +	init_MUTEX (&fepriv->sem);
> 
> the use of the semaphore to synchronise the thread is complex. It will
> require extra care to avoid deadlocks.

well, it 'works' quite fine for now, but yeah, I
thought about completely removing the additional
synchronization and 'jsut' go with the kthread
one, if that is sufficient ...

> >  	fepriv->state = FESTATE_IDLE;
> >  
> >  	/* paranoia check in case a signal arrived */
> > -	if (fepriv->thread_pid)
> > -		printk("dvb_frontend_stop: warning: thread PID %d won't exit\n",
> > -				fepriv->thread_pid);
> > +	if (fepriv->thread)
> > +		printk("dvb_frontend_stop: warning: thread %p won't exit\n",
> > +				fepriv->thread);
> 
> kthread_stop uses a completion already. so the above is real paranoia :)

again, I think this will go away soon :)

> >  }
> >  
> >  s32 timeval_usec_diff(struct timeval lasttime, struct timeval curtime)
> > @@ -684,10 +662,11 @@ static int dvb_frontend_start(struct dvb
> >  {
> >  	int ret;
> >  	struct dvb_frontend_private *fepriv = fe->frontend_priv;
> > +	struct task_struct *fe_thread;
> >  
> >  	dprintk ("%s\n", __FUNCTION__);
> >  
> > -	if (fepriv->thread_pid) {
> > +	if (fepriv->thread) {
> >  		if (!fepriv->exit)
> >  			return 0;
> >  		else
> > @@ -701,18 +680,18 @@ static int dvb_frontend_start(struct dvb
> >  
> >  	fepriv->state = FESTATE_IDLE;
> >  	fepriv->exit = 0;
> > -	fepriv->thread_pid = 0;
> > +	fepriv->thread = NULL;
> >  	mb();
> >  
> > -	ret = kernel_thread (dvb_frontend_thread, fe, 0);
> > -
> > -	if (ret < 0) {
> > -		printk("dvb_frontend_start: failed to start kernel_thread (%d)\n", ret);
> > +	fe_thread = kthread_run(dvb_frontend_thread, fe,
> > +		"kdvb-fe-%i", fe->dvb->num);
> > +	if (IS_ERR(fe_thread)) {
> > +		ret = PTR_ERR(fe_thread);
> 
> ret could be local.

correct, will fix that up in the next round

thanks for the feedback,
Herbert

> [ ... ] 
> 
> _______________________________________________
> Containers mailing list
> Containers@lists.osdl.org
> https://lists.osdl.org/mailman/listinfo/containers

  reply	other threads:[~2006-09-14 22:10 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2006-09-08 16:39 [patch -mm] update mq_notify to use a struct pid Cedric Le Goater
2006-09-09  2:39 ` Eric W. Biederman
2006-09-11 10:17   ` Cedric Le Goater
2006-09-11 11:09     ` Eric W. Biederman
2006-09-11 14:05       ` Cedric Le Goater
2006-09-11 19:01         ` Eric W. Biederman
2006-09-11 21:53           ` Cedric Le Goater
2006-09-12  1:22             ` Eric W. Biederman
2006-09-12 15:37               ` Cedric Le Goater
2006-09-12 16:03                 ` Eric W. Biederman
2006-09-12 11:05           ` Herbert Poetzl
2006-09-12 15:14             ` Eric W. Biederman
2006-09-14 20:01             ` [PATCH/RFC] kthread API conversion for dvb_frontend and av7110 Herbert Poetzl
2006-09-14 21:07               ` Cedric Le Goater
2006-09-14 22:10                 ` Herbert Poetzl [this message]
2006-11-17  1:50                   ` Andrew de Quincey
2006-11-22 14:56                     ` [Devel] " Cedric Le Goater
2006-11-22 21:32                       ` [v4l-dvb-maintainer] " Andrew de Quincey
2007-01-24 15:47                       ` Cedric Le Goater
2006-12-12 22:58                     ` Eric W. Biederman
2006-12-12 23:13                       ` Herbert Poetzl
2006-12-13 15:55                         ` [Devel] " Cedric Le Goater
2006-09-11 15:48     ` [patch -mm] update mq_notify to use a struct pid Oleg Nesterov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20060914221024.GB26916@MAIL.13thfloor.at \
    --to=herbert@13thfloor.at \
    --cc=adq_dvb@lidskialf.net \
    --cc=akpm@osdl.org \
    --cc=clg@fr.ibm.com \
    --cc=containers@lists.osdl.org \
    --cc=ebiederm@xmission.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=v4l-dvb-maintainer@linuxtv.org \
    --subject='Re: [PATCH/RFC] kthread API conversion for dvb_frontend and av7110' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).