On Sun, 9 Sep 2007 17:46:54 +0100
"Adrian McMenamin" <[EMAIL PROTECTED]> wrote:
> This patch adds support for Sega's proprietary Maple bus - which is
> required to support the Dreamcast's peripherals.
Hi,
in general the code looks clean; great job on that.
A few suggestions and comments to hopefully help this driver to become
even better:
First of all, I'm a little concerned about the lack of locking that
this driver seems to have; what guarantees the integrity of the lists
used in the driver?
Second, you have a lot of use of likely() and unlikely(); so much that I think
it's waaaay overkill. The code it's in generally isn't hotpath, and gcc also
does a pretty decent job without giving these hints in general.
> +
> +void maple_add_packet(struct mapleq *mq)
> +{
> + list_add((struct list_head *) mq, &maple_waitq);
> +}
for example this list.. what makes sure that no 2 pieces of code muck with it
at the same time?
> +static struct maple_device *maple_alloc_dev(int port, int unit)
> +{
> + struct maple_device *dev;
> +
> + dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> + if (unlikely(!dev))
> + return NULL;
> +
> + dev->port = port;
> + dev->unit = unit;
> + dev->mq = maple_allocq(dev);
> +
> + if (unlikely(!dev->mq)) {
this unlikely isn't needed; gcc basically assumes that NULL pointer checks are
unlikely anyway...
> +
> +static int setup_maple_commands(struct device *device, void *ignored)
> +{
> + struct maple_device *maple_dev = to_maple_dev(device);
> +
> + if (likely(maple_dev->interval > 0)) {
> + if (likely(jiffies > maple_dev->when)) {
this is I think a small bug; it is for sure unsafe against jiffies wrapping...
please consider using the time_before() and time_after() helpers which will
make this comparison safe wrt jiffies wrapping. The same is true for all other
jiffies use in the driver...
> +static irqreturn_t maplebus_dma_interrupt(int irq, void *dev_id)
> +{
> + /* Load everything into the bottom half */
> + schedule_work(&maple_dma_process);
> + return IRQ_HANDLED;
> +}
I wonder if you want to at least check if the work was really for you before
returning IRQ_HANDLED....
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
