Patchwork [4/4] mISDN: misc timerdev fixes

login
register
mail settings
Submitter Andrew Morton
Date Sept. 23, 2008, 11:30 a.m.
Message ID <20080923043005.358e19e7.akpm@linux-foundation.org>
Download mbox | patch
Permalink /patch/1049/
State Rejected
Delegated to: David Miller
Headers show

Comments

Andrew Morton - Sept. 23, 2008, 11:30 a.m.
On Tue, 23 Sep 2008 06:40:26 -0400 Christoph Hellwig <hch@infradead.org> wrote:

> On Mon, Sep 22, 2008 at 02:51:03PM -0700, akpm@linux-foundation.org wrote:
> > From: Andi Kleen <andi@firstfloor.org>
> > 
> > - Remove noop VFS stubs. The VFS does that on a NULL pointer anyways.
> 
> > -static loff_t
> > -mISDN_llseek(struct file *filep, loff_t offset, int orig)
> > -{
> > -	return -ESPIPE;
> > -}
> 
> >  static struct file_operations mISDN_fops = {
> > -	.llseek		= mISDN_llseek,
> 
> This is wrong.  no llseek means we use default_llseek, which is
> different from returning -ESPIPE.

so.. this?
Christoph Hellwig - Sept. 23, 2008, 11:45 a.m.
On Tue, Sep 23, 2008 at 04:30:05AM -0700, Andrew Morton wrote:
> so.. this?

Much better.

> --- a/drivers/isdn/mISDN/timerdev.c~misdn-misc-timerdev-fixes-fix
> +++ a/drivers/isdn/mISDN/timerdev.c
> @@ -61,7 +61,7 @@ mISDN_open(struct inode *ino, struct fil
>  	init_waitqueue_head(&dev->wait);
>  	filep->private_data = dev;
>  	__module_get(THIS_MODULE);
> -	return 0;
> +	return nonseekable_open(ino, filep);

But this also shows that mISDN is kinda stuck in a different century.
Doing __module_get(THIS_MODULE) at the end of ->open is utterly racy,
it really needs to set a owner field in file_operations and rip this
cruft out.

Btw, can anyone explain WTF this timerdev module is doing?  It's not
using any functionality from the rest of mISDN, it's not exporting
any functionality to it either but just provides a really awkward way
to expose dumb timers to userspace.  What does it provide that the
normal timer syscalls can't provide?
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Karsten Keil - Sept. 23, 2008, noon
On Tue, Sep 23, 2008 at 07:45:34AM -0400, Christoph Hellwig wrote:
> On Tue, Sep 23, 2008 at 04:30:05AM -0700, Andrew Morton wrote:
> > so.. this?
> 
> Much better.
> 
> > --- a/drivers/isdn/mISDN/timerdev.c~misdn-misc-timerdev-fixes-fix
> > +++ a/drivers/isdn/mISDN/timerdev.c
> > @@ -61,7 +61,7 @@ mISDN_open(struct inode *ino, struct fil
> >  	init_waitqueue_head(&dev->wait);
> >  	filep->private_data = dev;
> >  	__module_get(THIS_MODULE);
> > -	return 0;
> > +	return nonseekable_open(ino, filep);
> 
> But this also shows that mISDN is kinda stuck in a different century.
> Doing __module_get(THIS_MODULE) at the end of ->open is utterly racy,
> it really needs to set a owner field in file_operations and rip this
> cruft out.
> 
> Btw, can anyone explain WTF this timerdev module is doing?  It's not
> using any functionality from the rest of mISDN, it's not exporting
> any functionality to it either but just provides a really awkward way
> to expose dumb timers to userspace.  What does it provide that the
> normal timer syscalls can't provide?

This version only makes the programing of upper ISDN layers easier,
you only need to watch /dev/mISDNtimer together with the sockets in one
select call.
The next version will have a option to synchronise the timer with
the ISDN hardware clock which would avoid additional jitter if you need to
bridge channels in software.
Andi Kleen - Sept. 23, 2008, 2:14 p.m.
On Tue, Sep 23, 2008 at 04:30:05AM -0700, Andrew Morton wrote:
> On Tue, 23 Sep 2008 06:40:26 -0400 Christoph Hellwig <hch@infradead.org> wrote:
> 
> > On Mon, Sep 22, 2008 at 02:51:03PM -0700, akpm@linux-foundation.org wrote:
> > > From: Andi Kleen <andi@firstfloor.org>
> > > 
> > > - Remove noop VFS stubs. The VFS does that on a NULL pointer anyways.
> > 
> > > -static loff_t
> > > -mISDN_llseek(struct file *filep, loff_t offset, int orig)
> > > -{
> > > -	return -ESPIPE;
> > > -}
> > 
> > >  static struct file_operations mISDN_fops = {
> > > -	.llseek		= mISDN_llseek,
> > 
> > This is wrong.  no llseek means we use default_llseek, which is
> > different from returning -ESPIPE.
> 
> so.. this?

No, Christoph was wrong on that one. The original patch is ok.
He would have been right if that was a file system, but it isn't.

-Andi
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andi Kleen - Sept. 23, 2008, 2:17 p.m.
On Tue, Sep 23, 2008 at 04:30:05AM -0700, Andrew Morton wrote:
> On Tue, 23 Sep 2008 06:40:26 -0400 Christoph Hellwig <hch@infradead.org> wrote:
> 
> > On Mon, Sep 22, 2008 at 02:51:03PM -0700, akpm@linux-foundation.org wrote:
> > > From: Andi Kleen <andi@firstfloor.org>
> > > 
> > > - Remove noop VFS stubs. The VFS does that on a NULL pointer anyways.
> > 
> > > -static loff_t
> > > -mISDN_llseek(struct file *filep, loff_t offset, int orig)
> > > -{
> > > -	return -ESPIPE;
> > > -}
> > 
> > >  static struct file_operations mISDN_fops = {
> > > -	.llseek		= mISDN_llseek,
> > 
> > This is wrong.  no llseek means we use default_llseek, which is
> > different from returning -ESPIPE.
> 
> so.. this?

Hmm actually on double checking it's really needed, sorry.

-Andi
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Christoph Hellwig - Sept. 23, 2008, 3:58 p.m.
On Tue, Sep 23, 2008 at 04:17:40PM +0200, Andi Kleen wrote:
> > > >  static struct file_operations mISDN_fops = {
> > > > -	.llseek		= mISDN_llseek,
> > > 
> > > This is wrong.  no llseek means we use default_llseek, which is
> > > different from returning -ESPIPE.
> > 
> > so.. this?
> 
> Hmm actually on double checking it's really needed, sorry.

Yes, unfortunately the only way it's cleared currently is through using
nonseekable_open, but I've started preparing a war plan to sort this whole
mess out.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andi Kleen - Sept. 23, 2008, 4:46 p.m.
On Tue, Sep 23, 2008 at 11:58:44AM -0400, Christoph Hellwig wrote:
> On Tue, Sep 23, 2008 at 04:17:40PM +0200, Andi Kleen wrote:
> > > > >  static struct file_operations mISDN_fops = {
> > > > > -	.llseek		= mISDN_llseek,
> > > > 
> > > > This is wrong.  no llseek means we use default_llseek, which is
> > > > different from returning -ESPIPE.
> > > 
> > > so.. this?
> > 
> > Hmm actually on double checking it's really needed, sorry.
> 
> Yes, unfortunately the only way it's cleared currently is through using
> nonseekable_open, but I've started preparing a war plan to sort this whole
> mess out.

Yes it would be much more logical if FMODE_SEEK was cleared on character devices
by default. That is what I assumed with the original patch.

-Andi
Christoph Hellwig - Sept. 23, 2008, 4:48 p.m.
On Tue, Sep 23, 2008 at 06:46:52PM +0200, Andi Kleen wrote:
> > Yes, unfortunately the only way it's cleared currently is through using
> > nonseekable_open, but I've started preparing a war plan to sort this whole
> > mess out.
> 
> Yes it would be much more logical if FMODE_SEEK was cleared on character devices
> by default. That is what I assumed with the original patch.

Actually I'd prefer to not have by default at all.  While all
filesystems should support seeking they also can easily set a .llseek
instead of the current horrible default.  llseek is the only file
operation with a default (and a really bad one), and I'd prefer it not
to have for consistency.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andrew Morton - Sept. 23, 2008, 5:31 p.m.
On Tue, 23 Sep 2008 14:00:30 +0200 Karsten Keil <kkeil@suse.de> wrote:

> On Tue, Sep 23, 2008 at 07:45:34AM -0400, Christoph Hellwig wrote:
> > On Tue, Sep 23, 2008 at 04:30:05AM -0700, Andrew Morton wrote:
> > > so.. this?
> > 
> > Much better.
> > 
> > > --- a/drivers/isdn/mISDN/timerdev.c~misdn-misc-timerdev-fixes-fix
> > > +++ a/drivers/isdn/mISDN/timerdev.c
> > > @@ -61,7 +61,7 @@ mISDN_open(struct inode *ino, struct fil
> > >  	init_waitqueue_head(&dev->wait);
> > >  	filep->private_data = dev;
> > >  	__module_get(THIS_MODULE);
> > > -	return 0;
> > > +	return nonseekable_open(ino, filep);
> > 
> > But this also shows that mISDN is kinda stuck in a different century.
> > Doing __module_get(THIS_MODULE) at the end of ->open is utterly racy,
> > it really needs to set a owner field in file_operations and rip this
> > cruft out.
> > 
> > Btw, can anyone explain WTF this timerdev module is doing?  It's not
> > using any functionality from the rest of mISDN, it's not exporting
> > any functionality to it either but just provides a really awkward way
> > to expose dumb timers to userspace.  What does it provide that the
> > normal timer syscalls can't provide?
> 
> This version only makes the programing of upper ISDN layers easier,
> you only need to watch /dev/mISDNtimer together with the sockets in one
> select call.

sys_timerfd_create() can do this?

> The next version will have a option to synchronise the timer with
> the ISDN hardware clock which would avoid additional jitter if you need to
> bridge channels in software.

hrm.  If that's really really useful and actually works then I guess it
might then be justifiable.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Christoph Hellwig - Oct. 12, 2008, 11:05 a.m.
The bogus version which removes ->llseek just got into Linus' tree..
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

--- a/drivers/isdn/mISDN/timerdev.c~misdn-misc-timerdev-fixes-fix
+++ a/drivers/isdn/mISDN/timerdev.c
@@ -61,7 +61,7 @@  mISDN_open(struct inode *ino, struct fil
 	init_waitqueue_head(&dev->wait);
 	filep->private_data = dev;
 	__module_get(THIS_MODULE);
-	return 0;
+	return nonseekable_open(ino, filep);
 }
 
 static int