linux-kernel - Re: [PATCH] fix usb skeleton driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 6 Jun 2012 14:16:05 -0400 (EDT)
From:	Alan Stern <stern@...land.harvard.edu>
To:	Stefani Seibold <stefani@...bold.net>
cc:	linux-kernel@...r.kernel.org, <gregkh@...uxfoundation.org>,
	<oneukum@...e.de>, <alan@...rguk.ukuu.org.uk>,
	<linux-usb@...r.kernel.org>
Subject: Re: [PATCH] fix usb skeleton driver

On Wed, 6 Jun 2012, Stefani Seibold wrote:

> Am Mittwoch, den 06.06.2012, 12:55 -0400 schrieb Alan Stern:
> > On Wed, 6 Jun 2012 stefani@...bold.net wrote:
> > 
> > > From: Stefani Seibold <stefani@...bold.net>
> > > 
> > > This is a fix for the USB skeleton driver to bring it in shape.
> > > 
> > > - The usb_interface structure pointer will be no longer stored 
> > > - Every access to the USB will be handled trought the usb_interface pointer
> > 
> 
> Sorry, missed to fix. Should be:
> 
> Every access to the USB will be handled through the usb_device pointer

But that's wrong -- the accesses should go through the interface 
pointer.  After all, the driver is bound to the interface, not to the 
device.

> > Those two changes sound contradictory.
> > 
> > > - Add a new bool 'connected' for signaling a disconnect (== false)
> > > - Handle a non blocking read without blocking
> > > - Code cleanup
> > > - Synchronize disconnect() handler with open() and release(), to fix races
> > > - Introduced fsync
> > > - Single user mode
> > > - Eliminated dead code
> > > - Save some bytes in the dev structure
> > 
> > How about simplifying the code so that it can be read by somebody who's 
> > not already an expert?
> > 
> > Alan Stern
> > 
> 
> Hey, i thought i get a little thank you for the voluntary work, what a
> nice job. Not a demand for more work to do.

Have you submitted many kernel patches in the past?  :-)  This is the 
way it usually works out...

Seriously, what seems like an improvement to one person might not seem 
so great to somebody else.  Often even the most trivial changes can't 
get accepted without a lot of back-and-forth arguing^Wdiscussion.

In this case, I really think it's worthwhile to look for ways to
simplify usb-skeleton.c.  For example, does supporting fsync really
help somebody who's trying to learn how to write a USB device driver?  
I suspect it doesn't.

Going even farther, I'm not so sure it's a good idea for usb-skeleton
to try supporting both synchronous and asynchronous accesses.  This
adds a layer of complexity that people just don't need.  IMO it would 
be better to have two separate example drivers, an easy one that is 
purely synchronous and a more advanced one that is purely async.

Now, things like the race between disconnect and open are good for
teaching, because they crop up in every driver and have to be handled.  
Other things aren't so clear (such as the autosuspend support).

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/