linux-kernel - Re: [PATCH] drivers/misc: Altera Cyclone active serial implementation

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]

Message-ID: <20101109072200.GB29441@jasper.tkos.co.il>
Date:	Tue, 9 Nov 2010 09:22:00 +0200
From:	Baruch Siach <baruch@...s.co.il>
To:	Greg KH <greg@...ah.com>
Cc:	linux-kernel@...r.kernel.org,
	Andrew Morton <akpm@...ux-foundation.org>,
	Alex Gershgorin <agersh@...bler.ru>
Subject: Re: [PATCH] drivers/misc: Altera Cyclone active serial
 implementation

Hi Greg,

On Mon, Nov 08, 2010 at 07:58:51AM -0800, Greg KH wrote:
> On Mon, Nov 08, 2010 at 08:57:37AM +0200, Baruch Siach wrote:
> > On Sat, Nov 06, 2010 at 11:19:30AM -0700, Greg KH wrote:
> > > On Wed, Nov 03, 2010 at 04:21:35PM +0200, Baruch Siach wrote:
> > > > From: Alex Gershgorin <agersh@...bler.ru>
> > > > 
> > > > The active serial protocol can be used to program Altera Cyclone FPGA devices.
> > > > This driver uses the kernel gpio interface to implement the active serial
> > > > protocol.
> > > > 
> > > > Signed-off-by: Alex Gershgorin <agersh@...bler.ru>
> > > > Signed-off-by: Baruch Siach <baruch@...s.co.il>
> > > > ---
> > 
> > [snip]
> > 
> > > > +static struct class *cyclone_as_class;
> > > 
> > > Please don't create your own class just for a single driver.  Just use
> > > the misc class interface instead, as all you really want/need here is a
> > > character device node, right?
> > 
> > Searching for 'mist' under include/linux/ I couldn't find this "misc class 
> > interface". Can you enlighten me?
> > 
> > I did find the "miscdevices" interface in include/linux/miscdevice.h.
> 
> Yes, that is the one.
> 
> > I also tried this one before going on to create a class of my own.
> > However, this interface seems to be limited to singleton devices. Can
> > I use MISC_DYNAMIC_MINOR to create multiple device nodes?
> 
> I didn't see where your code was handling multiple device nodes, is it
> really doing that?

Of course. Quoting a .probe snippet from the original patch:

+	minor = find_first_zero_bit(cyclone_as_devs, AS_MAX_DEVS);
+	if (minor >= AS_MAX_DEVS)
+		return -EBUSY;
+	ret = cdev_add(&drvdata->cdev, MKDEV(cyclone_as_major, minor), 1);
+	if (ret < 0)
+		return ret;
+	set_bit(minor, cyclone_as_devs);
+
+	dev = device_create(cyclone_as_class, &pdev->dev,
+			MKDEV(cyclone_as_major, minor), drvdata,
+			"%s%d", "cyclone_as", minor);

> > Is there any example for this?
> 
> You can just increment the name for the miscdevice to do this based on a
> driver-supplied unique number.

OK. Will do.

> > > And as discussed at the Plumbers conference this past week, we don't
> > > want to add any new 'struct class' implementations to the kernel from
> > > now on, as it's the overall wrong thing to do.
> > > 
> > > > --- /dev/null
> > > > +++ b/include/linux/cyclone_as.h
> > > 
> > > Why do you need a .h file at all?
> > 
> > Look at the content of this file. I need to pass GPIO numbers from the 
> > platform code under arch/ to the driver. For this I use a platform_data 
> > struct, which must be visible to the platform code. Is there a better way to 
> > do this?
> 
> {sigh}
> 
> I'm getting really tired of these kinds of .h files cluttering up the
> kernel for an interface that should be easier to handle some other way.

Which easier way? Extending IORESOURCE in include/linux/ioport.h to include 
GPIOs (say, IORESOURCE_GPIO) may have solved the problem for me. Is it 
reasonable?

> I'm really tempted to create something like:
> 	include/platform_crap/
> to put all of these into.
> 
> Ok, that probably will not make people all that happy, so how about:
> 	include/platform_drivers/
> 
> instead?

OK. I'll create include/platform_drivers/ in my next patch. Quite a few header 
from include/ could move there.

baruch

-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@...s.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -
--
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/