On Sun, Jul 13, 2014 at 10:01:16AM +0200, Mateusz Guzik wrote:
> On Sat, Jul 12, 2014 at 08:10:15PM +0300, Konstantin Belousov wrote:
> > On Sat, Jul 12, 2014 at 06:53:47PM +0200, Mateusz Guzik wrote:
> > > There can be only one 'struct file' for devctl and devclose is only
> > > called when it is about to be destroyed.
> > >
> > > fd = open("/dev/devctl");
> > > close(dup(fd));
> > >
> > > does not result in calling devclose.
> > >
> > > If devclose is indeed reachable whlie fds are active this code needs
> > > serious help since devsoftc.inuse is of no use whatsoever.
> > >
> > > There is no support for multiple readers in the sense that each event
> > > can be read only once, hence the restriction on open.
> > >
> > > On the other hand it is indeed possible to obtain multiple fds for
> > > devctl which is harmless as far as consistency in the kernel goes.
> > > Concurrent reads are serialized with a mutex and closes are invisible to
> > > the device, except for the last one which destroys fp.
> > Well, I argue that devsoftc.inuse is broken too. It was introduced in
> > time when the only way of tracking the shared use of cdev was cloning.
> > Note that it does not prevent multiple threads from simultaneously
> > fall into the cdevsw methods; e.g. cv_wait_sig() in devread() drops
> > devsoftc.mtx etc.
> >
>
> But this is again harmless as far as kernel consistency goes.
>
> > IMO the right thing to do is to allow multiple opens and to keep
> > non-blocking attribute and async bindings in the per-file structure.
> > Then your change would be real nop.
> >
>
> This would be an option, but then what to do with events? Whoever
> happens to read one consumes it? Current behaviour of denying further
> opens seems safer since the process which opened can be sure nobody
> suddenly steals any. If it decides to 'share' device fd, well, it is its
> own problem.
>
> Assignments in devclose are not going to be executed as long as
> there is an active fp, thus this is a nop from perspective of devctl
> users.
>
> > BTW, another, this time really big, user of the private (yes) cloning
> > implementation is snd(4). The conversion of it to devfs_cdevpriv(9)
> > would be also highly desirable.
>
>
> This was meant to be a cosmetic change, I'm not interested in working on
> this, sorry. I can revert the change if you want.
I do not see a point in reverting it.
I am more interested in (somebody) converting such places to use cdevpriv.> > I plan to merge the following to stable/10: > - r264114 Fix SIGIO delivery. Use fsetown() to handle file descriptor owner > ioctl and use pgsigio() to send SIGIO. > - r264310 Add kqueue support for devctl. > > Do you have any objections? > > If you don't want a revert of this patch, I'll MFC it as well. I have no objections.
pgpu56865QSi7.pgp
Description: PGP signature
