Date: Thu, 13 Mar 2008 11:24:32 +0100
From: =?UTF-8?B?SGFucy1Kw7xyZ2Vu?= Koch <hjk@linutronix.de>
To: Paul Mundt <lethal@linux-sh.org>
Cc: Ben Nizette <bn@niasdigital.com>, gregkh <gregkh@suse.de>,
       linux-kernel <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v3] UIO: Implement a UIO interface for the SMX
 Cryptengine
Message-ID: <20080313112432.578076a5@dilbert.local>
In-Reply-To: <20080313100655.GA6705@linux-sh.org>
References: <1205400940.3735.15.camel@moss.renham>
	<20080313100655.GA6705@linux-sh.org>
Organization: Linutronix
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2305
Lines: 68

Am Thu, 13 Mar 2008 19:06:55 +0900
schrieb Paul Mundt <lethal@linux-sh.org>:

> > +
> > +	info->name = "smx-ce";
> > +	info->version = "0.03";
> 
> You may as well use DRV_NAME and DRV_VERSION, then you can toss that over
> to MODULE_VERSION() if you're so inclined. It's generally nice to keep
> modinfo happy.

UIO name and version are intentionally independent of module name and
version. It's meant to be an information for the userspace part of the driver
and should be set to values that are meaningful for it.

> 
> > +	info->irq = platform_get_irq(dev, 0);
> > +	info->irq_flags = IRQF_SHARED;
> > +	info->handler = smx_handler;
> > +
> platform_get_irq() can fail, which you don't presently handle (though I
> guess uio_register_device() will error out if you hand off -ENXIO as your
> IRQ anyways, so it's not technically an issue). That's a pretty minor
> issue, though, but might be something you wish to fix up if you are
> planning on using this as an example driver for others.

uio_register_device() will silently not register an interrupt if the number
is negative. This allows us to register a driver without interrupts. Hmm.
Maybe we should explicitly check for UIO_IRQ_NONE there and fail if a
different negative irq is passed in. Thanks for pointing this out.

But I agree, Ben should check if platform_get_irq() failed.

> 
> > +	platform_set_drvdata(dev, info);
> > +
> > +	if (uio_register_device(&dev->dev, info))
> > +		goto out_unmap;
> > +
> > +	return 0;
> > +out_unmap:
> > +	iounmap(info->mem[0].internal_addr);
> > +out_free:
> > +	kfree(info);
> > +
> > +	return -ENODEV;
> > +}
> > +
> Your error paths are also pretty quiet. A dev_err() or so in the few
> failure cases you do have might not be a terrible idea.

I agree.

> 
> uio_register_device() also has quite a few different error cases, so it's
> at least worth preserving the error value and returning that back,
> rather than always returning -ENODEV.

Right. Maybe we should make uio_register_device a bit more verbose, too.

Thanks for your detailed review!

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