2008-02-03 20:01:40

by Adrian McMenamin

[permalink] [raw]
Subject: [PATCH] SH/Dreamcast - fix regressions, whitespace and memory leaks in Maple Bus driver

From: Adrian McMenamin

This patch fixes the regression noted here:
http://lkml.org/lkml/2008/1/26/189 as well as whitespace issues in the
previous commit of this driver and the memory leaks noted here:
http://lkml.org/lkml/2008/2/2/143 (as well as one or two other minor
cleanups).

Signed off by: Adrian McMenamin <[email protected]>

==============



diff -ruN a/drivers/sh/maple/maple.c b/drivers/sh/maple/maple.c
--- a/drivers/sh/maple/maple.c 2008-02-03 19:32:23.000000000 +0000
+++ b/drivers/sh/maple/maple.c 2008-02-03 19:45:41.000000000 +0000
@@ -18,7 +18,6 @@
#include <linux/init.h>
#include <linux/kernel.h>
#include <linux/device.h>
-#include <linux/module.h>
#include <linux/interrupt.h>
#include <linux/list.h>
#include <linux/io.h>
@@ -53,12 +52,12 @@
static int subdevice_map[MAPLE_PORTS];
static unsigned long *maple_sendbuf, *maple_sendptr, *maple_lastptr;
static unsigned long maple_pnp_time;
-static int started, scanning, liststatus;
+static int started, scanning, liststatus, realscan;
static struct kmem_cache *maple_queue_cache;

struct maple_device_specify {
- int port;
- int unit;
+ int port;
+ int unit;
};

/**
@@ -68,22 +67,22 @@
*/
int maple_driver_register(struct device_driver *drv)
{
- if (!drv)
- return -EINVAL;
- drv->bus = &maple_bus_type;
- return driver_register(drv);
+ if (!drv)
+ return -EINVAL;
+ drv->bus = &maple_bus_type;
+ return driver_register(drv);
}
EXPORT_SYMBOL_GPL(maple_driver_register);

/* set hardware registers to enable next round of dma */
static void maplebus_dma_reset(void)
{
- ctrl_outl(MAPLE_MAGIC, MAPLE_RESET);
- /* set trig type to 0 for software trigger, 1 for hardware (VBLANK) */
- ctrl_outl(1, MAPLE_TRIGTYPE);
- ctrl_outl(MAPLE_2MBPS | MAPLE_TIMEOUT(50000), MAPLE_SPEED);
- ctrl_outl(PHYSADDR(maple_sendbuf), MAPLE_DMAADDR);
- ctrl_outl(1, MAPLE_ENABLE);
+ ctrl_outl(MAPLE_MAGIC, MAPLE_RESET);
+ /* set trig type to 0 for software trigger, 1 for hardware (VBLANK) */
+ ctrl_outl(1, MAPLE_TRIGTYPE);
+ ctrl_outl(MAPLE_2MBPS | MAPLE_TIMEOUT(50000), MAPLE_SPEED);
+ ctrl_outl(PHYSADDR(maple_sendbuf), MAPLE_DMAADDR);
+ ctrl_outl(1, MAPLE_ENABLE);
}

/**
@@ -94,27 +93,36 @@
* @function: the function code for the device
*/
void maple_getcond_callback(struct maple_device *dev,
- void (*callback) (struct mapleq * mq),
- unsigned long interval, unsigned long function)
+ void (*callback) (struct mapleq *mq),
+ unsigned long interval, unsigned long function)
{
- dev->callback = callback;
- dev->interval = interval;
- dev->function = cpu_to_be32(function);
- dev->when = jiffies;
+ dev->callback = callback;
+ dev->interval = interval;
+ dev->function = cpu_to_be32(function);
+ dev->when = jiffies;
}
EXPORT_SYMBOL_GPL(maple_getcond_callback);

static int maple_dma_done(void)
{
- return (ctrl_inl(MAPLE_STATE) & 1) == 0;
+ return (ctrl_inl(MAPLE_STATE) & 1) == 0;
}

static void maple_release_device(struct device *dev)
{
- if (dev->type) {
- kfree(dev->type->name);
- kfree(dev->type);
- }
+ struct maple_device *mdev;
+ struct mapleq *mq;
+ if (!dev)
+ return;
+ mdev = to_maple_dev(dev);
+ mq = mdev->mq;
+ if (mq) {
+ if (mq->recvbufdcsp)
+ kmem_cache_free(maple_queue_cache, mq->recvbufdcsp);
+ kfree(mq);
+ mq = NULL;
+ }
+ kfree(mdev);
}

/**
@@ -123,60 +131,62 @@
*/
void maple_add_packet(struct mapleq *mq)
{
- mutex_lock(&maple_list_lock);
- list_add(&mq->list, &maple_waitq);
- mutex_unlock(&maple_list_lock);
+ mutex_lock(&maple_list_lock);
+ list_add(&mq->list, &maple_waitq);
+ mutex_unlock(&maple_list_lock);
}
EXPORT_SYMBOL_GPL(maple_add_packet);

-static struct mapleq *maple_allocq(struct maple_device *dev)
+static struct mapleq *maple_allocq(struct maple_device *mdev)
{
- struct mapleq *mq;
+ struct mapleq *mq;

- mq = kmalloc(sizeof(*mq), GFP_KERNEL);
- if (!mq)
- return NULL;
-
- mq->dev = dev;
- mq->recvbufdcsp = kmem_cache_zalloc(maple_queue_cache, GFP_KERNEL);
- mq->recvbuf = (void *) P2SEGADDR(mq->recvbufdcsp);
- if (!mq->recvbuf) {
- kfree(mq);
- return NULL;
- }
+ mq = kmalloc(sizeof(*mq), GFP_KERNEL);
+ if (!mq)
+ return NULL;
+
+ mq->dev = mdev;
+ mq->recvbufdcsp = kmem_cache_zalloc(maple_queue_cache, GFP_KERNEL);
+ mq->recvbuf = (void *) P2SEGADDR(mq->recvbufdcsp);
+ if (!mq->recvbuf) {
+ kfree(mq);
+ return NULL;
+ }

- return mq;
+ return mq;
}

static struct maple_device *maple_alloc_dev(int port, int unit)
{
- struct maple_device *dev;
+ struct maple_device *mdev;

- dev = kzalloc(sizeof(*dev), GFP_KERNEL);
- if (!dev)
- return NULL;
-
- dev->port = port;
- dev->unit = unit;
- dev->mq = maple_allocq(dev);
-
- if (!dev->mq) {
- kfree(dev);
- return NULL;
- }
+ mdev = kzalloc(sizeof(*mdev), GFP_KERNEL);
+ if (!mdev)
+ return NULL;
+
+ mdev->port = port;
+ mdev->unit = unit;
+ mdev->mq = maple_allocq(mdev);
+
+ if (!mdev->mq) {
+ kfree(mdev);
+ return NULL;
+ }

- return dev;
+ return mdev;
}

static void maple_free_dev(struct maple_device *mdev)
{
- if (!mdev)
- return;
- if (mdev->mq) {
- kmem_cache_free(maple_queue_cache, mdev->mq->recvbufdcsp);
- kfree(mdev->mq);
- }
- kfree(mdev);
+ if (!mdev)
+ return;
+ if (mdev->mq) {
+ if (mdev->mq->recvbufdcsp)
+ kmem_cache_free(maple_queue_cache,
+ mdev->mq->recvbufdcsp);
+ kfree(mdev->mq);
+ }
+ kfree(mdev);
}

/* process the command queue into a maple command block
@@ -184,153 +194,167 @@
*/
static void maple_build_block(struct mapleq *mq)
{
- int port, unit, from, to, len;
- unsigned long *lsendbuf = mq->sendbuf;
+ int port, unit, from, to, len;
+ unsigned long *lsendbuf = mq->sendbuf;

- port = mq->dev->port & 3;
- unit = mq->dev->unit;
- len = mq->length;
- from = port << 6;
- to = (port << 6) | (unit > 0 ? (1 << (unit - 1)) & 0x1f : 0x20);
-
- *maple_lastptr &= 0x7fffffff;
- maple_lastptr = maple_sendptr;
-
- *maple_sendptr++ = (port << 16) | len | 0x80000000;
- *maple_sendptr++ = PHYSADDR(mq->recvbuf);
- *maple_sendptr++ =
- mq->command | (to << 8) | (from << 16) | (len << 24);
+ port = mq->dev->port & 3;
+ unit = mq->dev->unit;
+ len = mq->length;
+ from = port << 6;
+ to = (port << 6) | (unit > 0 ? (1 << (unit - 1)) & 0x1f : 0x20);
+
+ *maple_lastptr &= 0x7fffffff;
+ maple_lastptr = maple_sendptr;
+
+ *maple_sendptr++ = (port << 16) | len | 0x80000000;
+ *maple_sendptr++ = PHYSADDR(mq->recvbuf);
+ *maple_sendptr++ =
+ mq->command | (to << 8) | (from << 16) | (len << 24);

- while (len-- > 0)
- *maple_sendptr++ = *lsendbuf++;
+ while (len-- > 0)
+ *maple_sendptr++ = *lsendbuf++;
}

/* build up command queue */
static void maple_send(void)
{
- int i;
- int maple_packets;
- struct mapleq *mq, *nmq;
-
- if (!list_empty(&maple_sentq))
- return;
- if (list_empty(&maple_waitq) || !maple_dma_done())
- return;
- maple_packets = 0;
- maple_sendptr = maple_lastptr = maple_sendbuf;
- list_for_each_entry_safe(mq, nmq, &maple_waitq, list) {
- maple_build_block(mq);
- list_move(&mq->list, &maple_sentq);
- if (maple_packets++ > MAPLE_MAXPACKETS)
- break;
- }
- if (maple_packets > 0) {
- for (i = 0; i < (1 << MAPLE_DMA_PAGES); i++)
- dma_cache_sync(0, maple_sendbuf + i * PAGE_SIZE,
- PAGE_SIZE, DMA_BIDIRECTIONAL);
- }
+ int i;
+ int maple_packets;
+ struct mapleq *mq, *nmq;
+
+ if (!list_empty(&maple_sentq))
+ return;
+ if (list_empty(&maple_waitq) || !maple_dma_done())
+ return;
+ maple_packets = 0;
+ maple_sendptr = maple_lastptr = maple_sendbuf;
+ list_for_each_entry_safe(mq, nmq, &maple_waitq, list) {
+ maple_build_block(mq);
+ list_move(&mq->list, &maple_sentq);
+ if (maple_packets++ > MAPLE_MAXPACKETS)
+ break;
+ }
+ if (maple_packets > 0) {
+ for (i = 0; i < (1 << MAPLE_DMA_PAGES); i++)
+ dma_cache_sync(0, maple_sendbuf + i * PAGE_SIZE,
+ PAGE_SIZE, DMA_BIDIRECTIONAL);
+ }
}

static int attach_matching_maple_driver(struct device_driver *driver,
- void *devptr)
+ void *devptr)
{
- struct maple_driver *maple_drv;
- struct maple_device *mdev;
+ struct maple_driver *maple_drv;
+ struct maple_device *mdev;

- mdev = devptr;
- maple_drv = to_maple_driver(driver);
- if (mdev->devinfo.function & be32_to_cpu(maple_drv->function)) {
- if (maple_drv->connect(mdev) == 0) {
- mdev->driver = maple_drv;
- return 1;
- }
- }
- return 0;
+ mdev = devptr;
+ maple_drv = to_maple_driver(driver);
+ if (mdev->devinfo.function & be32_to_cpu(maple_drv->function)) {
+ if (maple_drv->connect(mdev) == 0) {
+ mdev->driver = maple_drv;
+ return 1;
+ }
+ }
+ return 0;
}

static void maple_detach_driver(struct maple_device *mdev)
{
- if (!mdev)
- return;
- if (mdev->driver) {
- if (mdev->driver->disconnect)
- mdev->driver->disconnect(mdev);
- }
- mdev->driver = NULL;
- if (mdev->registered) {
- maple_release_device(&mdev->dev);
- device_unregister(&mdev->dev);
- }
- mdev->registered = 0;
- maple_free_dev(mdev);
+ if (!mdev)
+ return;
+ if (mdev->driver) {
+ if (mdev->driver->disconnect)
+ mdev->driver->disconnect(mdev);
+ }
+ mdev->driver = NULL;
+ if (mdev->registered)
+ device_unregister(&mdev->dev);
+ mdev = NULL;
}

/* process initial MAPLE_COMMAND_DEVINFO for each device or port */
-static void maple_attach_driver(struct maple_device *dev)
+static void maple_attach_driver(struct maple_device *mdev)
{
- char *p;
-
- char *recvbuf;
- unsigned long function;
- int matched, retval;
-
- recvbuf = dev->mq->recvbuf;
- memcpy(&dev->devinfo, recvbuf + 4, sizeof(dev->devinfo));
- memcpy(dev->product_name, dev->devinfo.product_name, 30);
- memcpy(dev->product_licence, dev->devinfo.product_licence, 60);
- dev->product_name[30] = '\0';
- dev->product_licence[60] = '\0';
-
- for (p = dev->product_name + 29; dev->product_name <= p; p--)
- if (*p == ' ')
- *p = '\0';
- else
- break;
-
- for (p = dev->product_licence + 59; dev->product_licence <= p; p--)
- if (*p == ' ')
- *p = '\0';
- else
- break;
-
- function = be32_to_cpu(dev->devinfo.function);
-
- if (function > 0x200) {
- /* Do this silently - as not a real device */
- function = 0;
- dev->driver = &maple_dummy_driver;
- sprintf(dev->dev.bus_id, "%d:0.port", dev->port);
- } else {
- printk(KERN_INFO
- "Maple bus at (%d, %d): Connected function 0x%lX\n",
- dev->port, dev->unit, function);
-
- matched =
- bus_for_each_drv(&maple_bus_type, NULL, dev,
- attach_matching_maple_driver);
-
- if (matched == 0) {
- /* Driver does not exist yet */
- printk(KERN_INFO
- "No maple driver found for this device\n");
- dev->driver = &maple_dummy_driver;
- }
-
- sprintf(dev->dev.bus_id, "%d:0%d.%lX", dev->port,
- dev->unit, function);
- }
- dev->function = function;
- dev->dev.bus = &maple_bus_type;
- dev->dev.parent = &maple_bus;
- dev->dev.release = &maple_release_device;
- retval = device_register(&dev->dev);
- if (retval) {
- printk(KERN_INFO
- "Maple bus: Attempt to register device (%x, %x) failed.\n",
- dev->port, dev->unit);
- maple_free_dev(dev);
- }
- dev->registered = 1;
+ char *p, *recvbuf;
+ unsigned long function;
+ int matched, retval;
+
+ recvbuf = mdev->mq->recvbuf;
+ /* copy the data as individual elements in
+ * case of memory optimisation */
+ memcpy(&mdev->devinfo.function, recvbuf + 4, 4);
+ memcpy(&mdev->devinfo.function_data[0], recvbuf + 8, 12);
+ memcpy(&mdev->devinfo.area_code, recvbuf + 20, 1);
+ memcpy(&mdev->devinfo.connector_direction, recvbuf + 21, 1);
+ memcpy(&mdev->devinfo.product_name[0], recvbuf + 22, 30);
+ memcpy(&mdev->devinfo.product_licence[0], recvbuf + 52, 60);
+ memcpy(&mdev->devinfo.standby_power, recvbuf + 112, 2);
+ memcpy(&mdev->devinfo.max_power, recvbuf + 114, 2);
+ memcpy(mdev->product_name, mdev->devinfo.product_name, 30);
+ mdev->product_name[30] = '\0';
+ memcpy(mdev->product_licence, mdev->devinfo.product_licence, 60);
+ mdev->product_licence[60] = '\0';
+ for (p = mdev->product_name + 29; mdev->product_name <= p; p--)
+ if (*p == ' ')
+ *p = '\0';
+ else
+ break;
+
+ for (p = mdev->product_licence + 59; mdev->product_licence <= p; p--)
+ if (*p == ' ')
+ *p = '\0';
+ else
+ break;
+ if (realscan) {
+ printk(KERN_INFO "Maple device detected: %s\n",
+ mdev->product_name);
+ printk(KERN_INFO "Maple device: %s\n", mdev->product_licence);
+ }
+
+ function = be32_to_cpu(mdev->devinfo.function);
+
+ if (function > 0x200) {
+ /* Do this silently - as not a real device */
+ function = 0;
+ mdev->driver = &maple_dummy_driver;
+ sprintf(mdev->dev.bus_id, "%d:0.port", mdev->port);
+ } else {
+ if (realscan)
+ printk(KERN_INFO
+ "Maple bus at (%d, %d): Function 0x%lX\n",
+ mdev->port, mdev->unit, function);
+
+ matched =
+ bus_for_each_drv(&maple_bus_type, NULL, mdev,
+ attach_matching_maple_driver);
+
+ if (matched == 0) {
+ /* Driver does not exist yet */
+ if (realscan)
+ printk(KERN_INFO
+ "No maple driver found.\n");
+ mdev->driver = &maple_dummy_driver;
+ }
+ sprintf(mdev->dev.bus_id, "%d:0%d.%lX", mdev->port,
+ mdev->unit, function);
+ }
+ mdev->function = function;
+ mdev->dev.bus = &maple_bus_type;
+ mdev->dev.parent = &maple_bus;
+ mdev->dev.release = &maple_release_device;
+ if (mdev->registered == 0) {
+ retval = device_register(&mdev->dev);
+ if (retval) {
+ printk(KERN_INFO
+ "Maple bus: Attempt to register device"
+ " (%x, %x) failed.\n",
+ mdev->port, mdev->unit);
+ maple_free_dev(mdev);
+ mdev = NULL;
+ return;
+ }
+ mdev->registered = 1;
+ }
}

/*
@@ -340,270 +364,262 @@
*/
static int detach_maple_device(struct device *device, void *portptr)
{
- struct maple_device_specify *ds;
- struct maple_device *mdev;
+ struct maple_device_specify *ds;
+ struct maple_device *mdev;

- ds = portptr;
- mdev = to_maple_dev(device);
- if (mdev->port == ds->port && mdev->unit == ds->unit)
- return 1;
- return 0;
+ ds = portptr;
+ mdev = to_maple_dev(device);
+ if (mdev->port == ds->port && mdev->unit == ds->unit)
+ return 1;
+ return 0;
}

static int setup_maple_commands(struct device *device, void *ignored)
{
- struct maple_device *maple_dev = to_maple_dev(device);
+ struct maple_device *maple_dev = to_maple_dev(device);

- if ((maple_dev->interval > 0)
- && time_after(jiffies, maple_dev->when)) {
- maple_dev->when = jiffies + maple_dev->interval;
- maple_dev->mq->command = MAPLE_COMMAND_GETCOND;
- maple_dev->mq->sendbuf = &maple_dev->function;
- maple_dev->mq->length = 1;
- maple_add_packet(maple_dev->mq);
- liststatus++;
- } else {
- if (time_after(jiffies, maple_pnp_time)) {
- maple_dev->mq->command = MAPLE_COMMAND_DEVINFO;
- maple_dev->mq->length = 0;
- maple_add_packet(maple_dev->mq);
- liststatus++;
- }
- }
+ if ((maple_dev->interval > 0)
+ && time_after(jiffies, maple_dev->when)) {
+ maple_dev->when = jiffies + maple_dev->interval;
+ maple_dev->mq->command = MAPLE_COMMAND_GETCOND;
+ maple_dev->mq->sendbuf = &maple_dev->function;
+ maple_dev->mq->length = 1;
+ maple_add_packet(maple_dev->mq);
+ liststatus++;
+ } else {
+ if (time_after(jiffies, maple_pnp_time)) {
+ maple_dev->mq->command = MAPLE_COMMAND_DEVINFO;
+ maple_dev->mq->length = 0;
+ maple_add_packet(maple_dev->mq);
+ liststatus++;
+ }
+ }

- return 0;
+ return 0;
}

/* VBLANK bottom half - implemented via workqueue */
static void maple_vblank_handler(struct work_struct *work)
{
- if (!maple_dma_done())
- return;
- if (!list_empty(&maple_sentq))
- return;
- ctrl_outl(0, MAPLE_ENABLE);
- liststatus = 0;
- bus_for_each_dev(&maple_bus_type, NULL, NULL,
- setup_maple_commands);
- if (time_after(jiffies, maple_pnp_time))
- maple_pnp_time = jiffies + MAPLE_PNP_INTERVAL;
- if (liststatus && list_empty(&maple_sentq)) {
- INIT_LIST_HEAD(&maple_sentq);
- maple_send();
- }
- maplebus_dma_reset();
+ if (!maple_dma_done())
+ return;
+ if (!list_empty(&maple_sentq))
+ return;
+ ctrl_outl(0, MAPLE_ENABLE);
+ liststatus = 0;
+ bus_for_each_dev(&maple_bus_type, NULL, NULL,
+ setup_maple_commands);
+ if (time_after(jiffies, maple_pnp_time))
+ maple_pnp_time = jiffies + MAPLE_PNP_INTERVAL;
+ if (liststatus && list_empty(&maple_sentq)) {
+ INIT_LIST_HEAD(&maple_sentq);
+ maple_send();
+ }
+ maplebus_dma_reset();
}

/* handle devices added via hotplugs - placing them on queue for DEVINFO*/
static void maple_map_subunits(struct maple_device *mdev, int submask)
{
- int retval, k, devcheck;
- struct maple_device *mdev_add;
- struct maple_device_specify ds;
-
- for (k = 0; k < 5; k++) {
- ds.port = mdev->port;
- ds.unit = k + 1;
- retval =
- bus_for_each_dev(&maple_bus_type, NULL, &ds,
- detach_maple_device);
- if (retval) {
- submask = submask >> 1;
- continue;
- }
- devcheck = submask & 0x01;
- if (devcheck) {
- mdev_add = maple_alloc_dev(mdev->port, k + 1);
- if (!mdev_add)
- return;
- mdev_add->mq->command = MAPLE_COMMAND_DEVINFO;
- mdev_add->mq->length = 0;
- maple_add_packet(mdev_add->mq);
- scanning = 1;
- }
- submask = submask >> 1;
- }
+ int retval, k, devcheck;
+ struct maple_device *mdev_add;
+ struct maple_device_specify ds;
+
+ for (k = 0; k < 5; k++) {
+ ds.port = mdev->port;
+ ds.unit = k + 1;
+ retval =
+ bus_for_each_dev(&maple_bus_type, NULL, &ds,
+ detach_maple_device);
+ if (retval) {
+ submask = submask >> 1;
+ continue;
+ }
+ devcheck = submask & 0x01;
+ if (devcheck) {
+ mdev_add = maple_alloc_dev(mdev->port, k + 1);
+ if (!mdev_add)
+ return;
+ mdev_add->mq->command = MAPLE_COMMAND_DEVINFO;
+ mdev_add->mq->length = 0;
+ maple_add_packet(mdev_add->mq);
+ scanning = 1;
+ }
+ submask = submask >> 1;
+ }
}

/* mark a device as removed */
static void maple_clean_submap(struct maple_device *mdev)
{
- int killbit;
+ int killbit;

- killbit = (mdev->unit > 0 ? (1 << (mdev->unit - 1)) & 0x1f : 0x20);
- killbit = ~killbit;
- killbit &= 0xFF;
- subdevice_map[mdev->port] = subdevice_map[mdev->port] & killbit;
+ killbit = (mdev->unit > 0 ? (1 << (mdev->unit - 1)) & 0x1f : 0x20);
+ killbit = ~killbit;
+ killbit &= 0xFF;
+ subdevice_map[mdev->port] = subdevice_map[mdev->port] & killbit;
}

/* handle empty port or hotplug removal */
static void maple_response_none(struct maple_device *mdev,
- struct mapleq *mq)
+ struct mapleq *mq)
{
- if (mdev->unit != 0) {
- list_del(&mq->list);
- maple_clean_submap(mdev);
- printk(KERN_INFO
- "Maple bus device detaching at (%d, %d)\n",
- mdev->port, mdev->unit);
- maple_detach_driver(mdev);
- return;
- }
- if (!started) {
- printk(KERN_INFO "No maple devices attached to port %d\n",
- mdev->port);
- return;
- }
- maple_clean_submap(mdev);
+ if (mdev->unit != 0) {
+ list_del(&mq->list);
+ maple_clean_submap(mdev);
+ printk(KERN_INFO
+ "Maple bus device detaching at (%d, %d)\n",
+ mdev->port, mdev->unit);
+ maple_detach_driver(mdev);
+ return;
+ }
+ if (!started) {
+ printk(KERN_INFO "No maple devices attached to port %d\n",
+ mdev->port);
+ return;
+ }
+ maple_clean_submap(mdev);
}

/* preprocess hotplugs or scans */
static void maple_response_devinfo(struct maple_device *mdev,
- char *recvbuf)
+ char *recvbuf)
{
- char submask;
- if ((!started) || (scanning == 2)) {
- maple_attach_driver(mdev);
- return;
- }
- if (mdev->unit == 0) {
- submask = recvbuf[2] & 0x1F;
- if (submask ^ subdevice_map[mdev->port]) {
- maple_map_subunits(mdev, submask);
- subdevice_map[mdev->port] = submask;
- }
- }
+ char submask;
+ if ((!started) || (scanning == 2)) {
+ maple_attach_driver(mdev);
+ return;
+ }
+ if (mdev->unit == 0) {
+ submask = recvbuf[2] & 0x1F;
+ if (submask ^ subdevice_map[mdev->port]) {
+ maple_map_subunits(mdev, submask);
+ subdevice_map[mdev->port] = submask;
+ }
+ }
}

/* maple dma end bottom half - implemented via workqueue */
static void maple_dma_handler(struct work_struct *work)
{
- struct mapleq *mq, *nmq;
- struct maple_device *dev;
- char *recvbuf;
- enum maple_code code;
-
- if (!maple_dma_done())
- return;
- ctrl_outl(0, MAPLE_ENABLE);
- if (!list_empty(&maple_sentq)) {
- list_for_each_entry_safe(mq, nmq, &maple_sentq, list) {
- recvbuf = mq->recvbuf;
- code = recvbuf[0];
- dev = mq->dev;
- switch (code) {
- case MAPLE_RESPONSE_NONE:
- maple_response_none(dev, mq);
- break;
-
- case MAPLE_RESPONSE_DEVINFO:
- maple_response_devinfo(dev, recvbuf);
- break;
-
- case MAPLE_RESPONSE_DATATRF:
- if (dev->callback)
- dev->callback(mq);
- break;
-
- case MAPLE_RESPONSE_FILEERR:
- case MAPLE_RESPONSE_AGAIN:
- case MAPLE_RESPONSE_BADCMD:
- case MAPLE_RESPONSE_BADFUNC:
- printk(KERN_DEBUG
- "Maple non-fatal error 0x%X\n",
- code);
- break;
-
- case MAPLE_RESPONSE_ALLINFO:
- printk(KERN_DEBUG
- "Maple - extended device information not supported\n");
- break;
-
- case MAPLE_RESPONSE_OK:
- break;
-
- default:
- break;
- }
- }
- INIT_LIST_HEAD(&maple_sentq);
- if (scanning == 1) {
- maple_send();
- scanning = 2;
- } else
- scanning = 0;
-
- if (started == 0)
- started = 1;
- }
- maplebus_dma_reset();
+ struct mapleq *mq, *nmq;
+ struct maple_device *dev;
+ char *recvbuf;
+ enum maple_code code;
+
+ if (!maple_dma_done())
+ return;
+ ctrl_outl(0, MAPLE_ENABLE);
+ if (!list_empty(&maple_sentq)) {
+ list_for_each_entry_safe(mq, nmq, &maple_sentq, list) {
+ recvbuf = mq->recvbuf;
+ code = recvbuf[0];
+ dev = mq->dev;
+ switch (code) {
+ case MAPLE_RESPONSE_NONE:
+ maple_response_none(dev, mq);
+ break;
+
+ case MAPLE_RESPONSE_DEVINFO:
+ maple_response_devinfo(dev, recvbuf);
+ break;
+
+ case MAPLE_RESPONSE_DATATRF:
+ if (dev->callback)
+ dev->callback(mq);
+ break;
+
+ case MAPLE_RESPONSE_FILEERR:
+ case MAPLE_RESPONSE_AGAIN:
+ case MAPLE_RESPONSE_BADCMD:
+ case MAPLE_RESPONSE_BADFUNC:
+ printk(KERN_DEBUG
+ "Maple non-fatal error 0x%X\n",
+ code);
+ break;
+
+ case MAPLE_RESPONSE_ALLINFO:
+ printk(KERN_DEBUG
+ "Maple - extended device information"
+ " not supported\n");
+ break;
+
+ case MAPLE_RESPONSE_OK:
+ break;
+
+ default:
+ break;
+ }
+ }
+ INIT_LIST_HEAD(&maple_sentq);
+ if (scanning == 1) {
+ maple_send();
+ scanning = 2;
+ } else
+ scanning = 0;
+
+ if (started == 0)
+ started = 1;
+ }
+ maplebus_dma_reset();
}

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;
+ /* Load everything into the bottom half */
+ schedule_work(&maple_dma_process);
+ return IRQ_HANDLED;
}

static irqreturn_t maplebus_vblank_interrupt(int irq, void *dev_id)
{
- schedule_work(&maple_vblank_process);
- return IRQ_HANDLED;
+ schedule_work(&maple_vblank_process);
+ return IRQ_HANDLED;
}

-static struct irqaction maple_dma_irq = {
- .name = "maple bus DMA handler",
- .handler = maplebus_dma_interrupt,
- .flags = IRQF_SHARED,
-};
-
-static struct irqaction maple_vblank_irq = {
- .name = "maple bus VBLANK handler",
- .handler = maplebus_vblank_interrupt,
- .flags = IRQF_SHARED,
-};
-
static int maple_set_dma_interrupt_handler(void)
{
- return setup_irq(HW_EVENT_MAPLE_DMA, &maple_dma_irq);
+ return request_irq(HW_EVENT_MAPLE_DMA, maplebus_dma_interrupt,
+ IRQF_SHARED, "maple bus DMA", &maple_dummy_driver);
}

static int maple_set_vblank_interrupt_handler(void)
{
- return setup_irq(HW_EVENT_VSYNC, &maple_vblank_irq);
+ return request_irq(HW_EVENT_VSYNC, maplebus_vblank_interrupt,
+ IRQF_SHARED, "maple bus VBLANK", &maple_dummy_driver);
}

static int maple_get_dma_buffer(void)
{
- maple_sendbuf =
- (void *) __get_free_pages(GFP_KERNEL | __GFP_ZERO,
- MAPLE_DMA_PAGES);
- if (!maple_sendbuf)
- return -ENOMEM;
- return 0;
+ maple_sendbuf =
+ (void *) __get_free_pages(GFP_KERNEL | __GFP_ZERO,
+ MAPLE_DMA_PAGES);
+ if (!maple_sendbuf)
+ return -ENOMEM;
+ return 0;
}

static int match_maple_bus_driver(struct device *devptr,
- struct device_driver *drvptr)
+ struct device_driver *drvptr)
{
- struct maple_driver *maple_drv;
- struct maple_device *maple_dev;
+ struct maple_driver *maple_drv;
+ struct maple_device *maple_dev;

- maple_drv = container_of(drvptr, struct maple_driver, drv);
- maple_dev = container_of(devptr, struct maple_device, dev);
- /* Trap empty port case */
- if (maple_dev->devinfo.function == 0xFFFFFFFF)
- return 0;
- else if (maple_dev->devinfo.function &
- be32_to_cpu(maple_drv->function))
- return 1;
- return 0;
+ maple_drv = container_of(drvptr, struct maple_driver, drv);
+ maple_dev = container_of(devptr, struct maple_device, dev);
+ /* Trap empty port case */
+ if (maple_dev->devinfo.function == 0xFFFFFFFF)
+ return 0;
+ else if (maple_dev->devinfo.function &
+ be32_to_cpu(maple_drv->function))
+ return 1;
+ return 0;
}

-static int maple_bus_uevent(struct device *dev, struct kobj_uevent_env *env)
+static int maple_bus_uevent(struct device *dev,
+ struct kobj_uevent_env *env)
{
- return 0;
+ return 0;
}

static void maple_bus_release(struct device *dev)
@@ -611,124 +627,124 @@
}

static struct maple_driver maple_dummy_driver = {
- .drv = {
- .name = "maple_dummy_driver",
- .bus = &maple_bus_type,
- },
+ .drv = {
+ .name = "maple_dummy_driver",
+ .bus = &maple_bus_type,
+ },
};

struct bus_type maple_bus_type = {
- .name = "maple",
- .match = match_maple_bus_driver,
- .uevent = maple_bus_uevent,
+ .name = "maple",
+ .match = match_maple_bus_driver,
+ .uevent = maple_bus_uevent,
};
EXPORT_SYMBOL_GPL(maple_bus_type);

static struct device maple_bus = {
- .bus_id = "maple",
- .release = maple_bus_release,
+ .bus_id = "maple",
+ .release = maple_bus_release,
};

static int __init maple_bus_init(void)
{
- int retval, i;
- struct maple_device *mdev[MAPLE_PORTS];
- ctrl_outl(0, MAPLE_STATE);
-
- retval = device_register(&maple_bus);
- if (retval)
- goto cleanup;
-
- retval = bus_register(&maple_bus_type);
- if (retval)
- goto cleanup_device;
-
- retval = driver_register(&maple_dummy_driver.drv);
-
- if (retval)
- goto cleanup_bus;
-
- /* allocate memory for maple bus dma */
- retval = maple_get_dma_buffer();
- if (retval) {
- printk(KERN_INFO
- "Maple bus: Failed to allocate Maple DMA buffers\n");
- goto cleanup_basic;
- }
-
- /* set up DMA interrupt handler */
- retval = maple_set_dma_interrupt_handler();
- if (retval) {
- printk(KERN_INFO
- "Maple bus: Failed to grab maple DMA IRQ\n");
- goto cleanup_dma;
- }
-
- /* set up VBLANK interrupt handler */
- retval = maple_set_vblank_interrupt_handler();
- if (retval) {
- printk(KERN_INFO "Maple bus: Failed to grab VBLANK IRQ\n");
- goto cleanup_irq;
- }
-
- maple_queue_cache =
- kmem_cache_create("maple_queue_cache", 0x400, 0,
- SLAB_HWCACHE_ALIGN, NULL);
-
- if (!maple_queue_cache)
- goto cleanup_bothirqs;
-
- /* setup maple ports */
- for (i = 0; i < MAPLE_PORTS; i++) {
- mdev[i] = maple_alloc_dev(i, 0);
- if (!mdev[i]) {
- while (i-- > 0)
- maple_free_dev(mdev[i]);
- goto cleanup_cache;
- }
- mdev[i]->registered = 0;
- mdev[i]->mq->command = MAPLE_COMMAND_DEVINFO;
- mdev[i]->mq->length = 0;
- maple_attach_driver(mdev[i]);
- maple_add_packet(mdev[i]->mq);
- subdevice_map[i] = 0;
- }
-
- /* setup maplebus hardware */
- maplebus_dma_reset();
+ int retval, i;
+ struct maple_device *mdev[MAPLE_PORTS];
+ ctrl_outl(0, MAPLE_STATE);
+
+ retval = device_register(&maple_bus);
+ if (retval)
+ goto cleanup;
+
+ retval = bus_register(&maple_bus_type);
+ if (retval)
+ goto cleanup_device;
+
+ retval = driver_register(&maple_dummy_driver.drv);
+
+ if (retval)
+ goto cleanup_bus;
+
+ /* allocate memory for maple bus dma */
+ retval = maple_get_dma_buffer();
+ if (retval) {
+ printk(KERN_INFO
+ "Maple bus: Failed to allocate Maple DMA buffers\n");
+ goto cleanup_basic;
+ }
+
+ /* set up DMA interrupt handler */
+ retval = maple_set_dma_interrupt_handler();
+ if (retval) {
+ printk(KERN_INFO
+ "Maple bus: Failed to grab maple DMA IRQ\n");
+ goto cleanup_dma;
+ }
+
+ /* set up VBLANK interrupt handler */
+ retval = maple_set_vblank_interrupt_handler();
+ if (retval) {
+ printk(KERN_INFO "Maple bus: Failed to grab VBLANK IRQ\n");
+ goto cleanup_irq;
+ }
+
+ maple_queue_cache =
+ kmem_cache_create("maple_queue_cache", 0x400, 0,
+ SLAB_POISON|SLAB_HWCACHE_ALIGN, NULL);
+
+ if (!maple_queue_cache)
+ goto cleanup_bothirqs;
+
+ /* setup maple ports */
+ for (i = 0; i < MAPLE_PORTS; i++) {
+ mdev[i] = maple_alloc_dev(i, 0);
+ if (!mdev[i]) {
+ while (i-- > 0)
+ maple_free_dev(mdev[i]);
+ goto cleanup_cache;
+ }
+ mdev[i]->registered = 0;
+ mdev[i]->mq->command = MAPLE_COMMAND_DEVINFO;
+ mdev[i]->mq->length = 0;
+ /* register an empty device for each port */
+ maple_attach_driver(mdev[i]);
+ maple_add_packet(mdev[i]->mq);
+ subdevice_map[i] = 0;
+ }
+
+ realscan = 1;
+ /* setup maplebus hardware */
+ maplebus_dma_reset();
+ /* initial detection */
+ maple_send();
+ maple_pnp_time = jiffies;
+ printk(KERN_INFO "Maple bus core now registered.\n");

- /* initial detection */
- maple_send();
-
- maple_pnp_time = jiffies;
-
- printk(KERN_INFO "Maple bus core now registered.\n");
-
- return 0;
+ return 0;

cleanup_cache:
- kmem_cache_destroy(maple_queue_cache);
+ kmem_cache_destroy(maple_queue_cache);

cleanup_bothirqs:
- free_irq(HW_EVENT_VSYNC, 0);
+ free_irq(HW_EVENT_VSYNC, 0);

cleanup_irq:
- free_irq(HW_EVENT_MAPLE_DMA, 0);
+ free_irq(HW_EVENT_MAPLE_DMA, 0);

cleanup_dma:
- free_pages((unsigned long) maple_sendbuf, MAPLE_DMA_PAGES);
+ free_pages((unsigned long) maple_sendbuf, MAPLE_DMA_PAGES);

cleanup_basic:
- driver_unregister(&maple_dummy_driver.drv);
+ driver_unregister(&maple_dummy_driver.drv);

cleanup_bus:
- bus_unregister(&maple_bus_type);
+ bus_unregister(&maple_bus_type);

cleanup_device:
- device_unregister(&maple_bus);
+ device_unregister(&maple_bus);

cleanup:
- printk(KERN_INFO "Maple bus registration failed\n");
- return retval;
+ printk(KERN_INFO "Maple bus registration failed\n");
+ return retval;
}
+
subsys_initcall(maple_bus_init);
diff -ruN a/include/linux/maple.h b/include/linux/maple.h
--- a/include/linux/maple.h 2008-02-03 19:35:12.000000000 +0000
+++ b/include/linux/maple.h 2008-02-03 19:43:48.000000000 +0000
@@ -7,74 +7,75 @@

/* Maple Bus command and response codes */
enum maple_code {
- MAPLE_RESPONSE_FILEERR = -5,
- MAPLE_RESPONSE_AGAIN = -4, /* request should be retransmitted */
- MAPLE_RESPONSE_BADCMD = -3,
- MAPLE_RESPONSE_BADFUNC = -2,
- MAPLE_RESPONSE_NONE = -1, /* unit didn't respond at all */
- MAPLE_COMMAND_DEVINFO = 1,
- MAPLE_COMMAND_ALLINFO = 2,
- MAPLE_COMMAND_RESET = 3,
- MAPLE_COMMAND_KILL = 4,
- MAPLE_RESPONSE_DEVINFO = 5,
- MAPLE_RESPONSE_ALLINFO = 6,
- MAPLE_RESPONSE_OK = 7,
- MAPLE_RESPONSE_DATATRF = 8,
- MAPLE_COMMAND_GETCOND = 9,
- MAPLE_COMMAND_GETMINFO = 10,
- MAPLE_COMMAND_BREAD = 11,
- MAPLE_COMMAND_BWRITE = 12,
- MAPLE_COMMAND_SETCOND = 14
+ MAPLE_RESPONSE_FILEERR = -5,
+ MAPLE_RESPONSE_AGAIN = -4, /* request should be retransmitted */
+ MAPLE_RESPONSE_BADCMD = -3,
+ MAPLE_RESPONSE_BADFUNC = -2,
+ MAPLE_RESPONSE_NONE = -1, /* unit didn't respond at all */
+ MAPLE_COMMAND_DEVINFO = 1,
+ MAPLE_COMMAND_ALLINFO = 2,
+ MAPLE_COMMAND_RESET = 3,
+ MAPLE_COMMAND_KILL = 4,
+ MAPLE_RESPONSE_DEVINFO = 5,
+ MAPLE_RESPONSE_ALLINFO = 6,
+ MAPLE_RESPONSE_OK = 7,
+ MAPLE_RESPONSE_DATATRF = 8,
+ MAPLE_COMMAND_GETCOND = 9,
+ MAPLE_COMMAND_GETMINFO = 10,
+ MAPLE_COMMAND_BREAD = 11,
+ MAPLE_COMMAND_BWRITE = 12,
+ MAPLE_COMMAND_SETCOND = 14
};

struct mapleq {
- struct list_head list;
- struct maple_device *dev;
- void *sendbuf, *recvbuf, *recvbufdcsp;
- unsigned char length;
- enum maple_code command;
+ struct list_head list;
+ struct maple_device *dev;
+ void *sendbuf, *recvbuf, *recvbufdcsp;
+ unsigned char length;
+ enum maple_code command;
};

struct maple_devinfo {
- unsigned long function;
- unsigned long function_data[3];
- unsigned char area_code;
- unsigned char connector_directon;
- char product_name[31];
- char product_licence[61];
- unsigned short standby_power;
- unsigned short max_power;
+ unsigned long function;
+ unsigned long function_data[3];
+ unsigned char area_code;
+ unsigned char connector_direction;
+ char product_name[31];
+ char product_licence[61];
+ unsigned short standby_power;
+ unsigned short max_power;
};

struct maple_device {
- struct maple_driver *driver;
- struct mapleq *mq;
- void *private_data;
- void (*callback) (struct mapleq * mq);
- unsigned long when, interval, function;
- struct maple_devinfo devinfo;
- unsigned char port, unit;
- char product_name[32];
- char product_licence[64];
- int registered;
- struct device dev;
+ struct maple_driver *driver;
+ struct mapleq *mq;
+ void *private_data;
+ void (*callback) (struct mapleq *mq);
+ unsigned long when, interval, function;
+ struct maple_devinfo devinfo;
+ unsigned char port, unit;
+ char product_name[32];
+ char product_licence[64];
+ int registered;
+ struct device dev;
};

struct maple_driver {
- unsigned long function;
- int (*connect) (struct maple_device * dev);
- void (*disconnect) (struct maple_device * dev);
- struct device_driver drv;
+ unsigned long function;
+ int (*connect) (struct maple_device *dev);
+ void (*disconnect) (struct maple_device *dev);
+ struct device_driver drv;
+ int registered;
};

void maple_getcond_callback(struct maple_device *dev,
- void (*callback) (struct mapleq * mq),
- unsigned long interval,
- unsigned long function);
+ void (*callback) (struct mapleq *mq),
+ unsigned long interval,
+ unsigned long function);
int maple_driver_register(struct device_driver *drv);
void maple_add_packet(struct mapleq *mq);

#define to_maple_dev(n) container_of(n, struct maple_device, dev)
#define to_maple_driver(n) container_of(n, struct maple_driver, drv)

-#endif /* __LINUX_MAPLE_H */
+#endif /* __LINUX_MAPLE_H */


2008-02-04 01:11:42

by Paul Mundt

[permalink] [raw]
Subject: Re: [PATCH] SH/Dreamcast - fix regressions, whitespace and memory leaks in Maple Bus driver

On Sun, Feb 03, 2008 at 08:00:47PM +0000, Adrian McMenamin wrote:
> From: Adrian McMenamin
>
This is useless if you are submitting the patch, especially if you're
missing a mail address.

> This patch fixes the regression noted here:
> http://lkml.org/lkml/2008/1/26/189 as well as whitespace issues in the
> previous commit of this driver and the memory leaks noted here:
> http://lkml.org/lkml/2008/2/2/143 (as well as one or two other minor
> cleanups).
>
The subject notes 3 specific things that are being addressed, but you've
rolled this all in to one patch which makes it utterly impossible to
figure out what you're actually fixing. At the very least, split this in
to 3 different patches, each dealing with one of the things noted in the
subject. The fact that regressions is plural also suggests you may want
to split this down in to smaller patches that deal with specific
regressions if they are not directly related.

> Signed off by: Adrian McMenamin <[email protected]>
>
Do not invent new sign-off tags, see Documentation/SubmittingPatches.
Scripts do end up having to parse this stuff.

2008-02-04 05:31:26

by Greg KH

[permalink] [raw]
Subject: Re: [PATCH] SH/Dreamcast - fix regressions, whitespace and memory leaks in Maple Bus driver

On Sun, Feb 03, 2008 at 08:00:47PM +0000, Adrian McMenamin wrote:
> From: Adrian McMenamin
>
> This patch fixes the regression noted here:
> http://lkml.org/lkml/2008/1/26/189 as well as whitespace issues in the
> previous commit of this driver and the memory leaks noted here:
> http://lkml.org/lkml/2008/2/2/143 (as well as one or two other minor
> cleanups).

Which portion of the patch fixes the kobject WARN_ON()?

thanks,

greg k-h

2008-02-04 08:24:21

by Adrian McMenamin

[permalink] [raw]
Subject: Re: [PATCH] SH/Dreamcast - fix regressions, whitespace and memory leaks in Maple Bus driver


On Mon, 2008-02-04 at 10:10 +0900, Paul Mundt wrote:
> On Sun, Feb 03, 2008 at 08:00:47PM +0000, Adrian McMenamin wrote:
> > From: Adrian McMenamin
> >
> This is useless if you are submitting the patch, especially if you're
> missing a mail address.
>

>From Documentation/SubmittingPatches

The canonical patch message body contains the following:
*
* - A "from" line specifying the patch author.
*


> > This patch fixes the regression noted here:
> > http://lkml.org/lkml/2008/1/26/189 as well as whitespace issues in the
> > previous commit of this driver and the memory leaks noted here:
> > http://lkml.org/lkml/2008/2/2/143 (as well as one or two other minor
> > cleanups).
> >
> The subject notes 3 specific things that are being addressed, but you've
> rolled this all in to one patch which makes it utterly impossible to
> figure out what you're actually fixing. At the very least, split this in
> to 3 different patches, each dealing with one of the things noted in the
> subject. The fact that regressions is plural also suggests you may want
> to split this down in to smaller patches that deal with specific
> regressions if they are not directly related.
>

What would be the point of submitting patches of broken code just to
remove the whitespace your previous commit added to all the lines?


> > Signed off by: Adrian McMenamin <[email protected]>
> >
> Do not invent new sign-off tags, see Documentation/SubmittingPatches.
> Scripts do end up having to parse this stuff.


Yes, sorry for that.

2008-02-04 08:28:33

by Adrian McMenamin

[permalink] [raw]
Subject: Re: [PATCH] SH/Dreamcast - fix regressions, whitespace and memory leaks in Maple Bus driver


On Sun, 2008-02-03 at 21:29 -0800, Greg KH wrote:
> On Sun, Feb 03, 2008 at 08:00:47PM +0000, Adrian McMenamin wrote:
> > From: Adrian McMenamin
> >
> > This patch fixes the regression noted here:
> > http://lkml.org/lkml/2008/1/26/189 as well as whitespace issues in the
> > previous commit of this driver and the memory leaks noted here:
> > http://lkml.org/lkml/2008/2/2/143 (as well as one or two other minor
> > cleanups).
>
> Which portion of the patch fixes the kobject WARN_ON()?



+ if (mdev->registered == 0) {
+ retval = device_register(&mdev->dev);
+ if (retval) {
+ printk(KERN_INFO
+ "Maple bus: Attempt to register device"
+ " (%x, %x) failed.\n",
+ mdev->port, mdev->unit);
+ maple_free_dev(mdev);
+ mdev = NULL;
+ return;
+ }
+ mdev->registered = 1;
+ }
}


Specifically the check on mdev->registered

Unfortunately the previous commit was completely corrupted by whitespace
everywhere so the patch essentially covers the whole dirver (I had a
choice of submitting broken code with whitespace removed or working code
with whitespace removed)

2008-02-04 09:03:19

by Paul Mundt

[permalink] [raw]
Subject: Re: [PATCH] SH/Dreamcast - fix regressions, whitespace and memory leaks in Maple Bus driver

On Mon, Feb 04, 2008 at 08:23:44AM +0000, Adrian McMenamin wrote:
>
> On Mon, 2008-02-04 at 10:10 +0900, Paul Mundt wrote:
> > On Sun, Feb 03, 2008 at 08:00:47PM +0000, Adrian McMenamin wrote:
> > > From: Adrian McMenamin
> > >
> > This is useless if you are submitting the patch, especially if you're
> > missing a mail address.
> >
>
> >From Documentation/SubmittingPatches
>
> The canonical patch message body contains the following:
> *
> * - A "from" line specifying the patch author.
> *
>
Which doesn't invalidate the missing address problem, and the fact that
you are _already_ in the from line.

> > > This patch fixes the regression noted here:
> > > http://lkml.org/lkml/2008/1/26/189 as well as whitespace issues in the
> > > previous commit of this driver and the memory leaks noted here:
> > > http://lkml.org/lkml/2008/2/2/143 (as well as one or two other minor
> > > cleanups).
> > >
> > The subject notes 3 specific things that are being addressed, but you've
> > rolled this all in to one patch which makes it utterly impossible to
> > figure out what you're actually fixing. At the very least, split this in
> > to 3 different patches, each dealing with one of the things noted in the
> > subject. The fact that regressions is plural also suggests you may want
> > to split this down in to smaller patches that deal with specific
> > regressions if they are not directly related.
>
> What would be the point of submitting patches of broken code just to
> remove the whitespace your previous commit added to all the lines?
>
My previous commit was directly from _your_ patch, given that your
patches have a history of whitespace damage, this doesn't seem like much
of a stretch. It's true I neglected to run it through checkpatch, I'll be
more careful with that in the future when applying patches from certain
parties.

The point of submitting a series of patches is so that it's obvious
_what_ you are changing. Lumping it in with the whitespace changes just
makes it impossible to read, as GregKH also hinted at when trying to
figure out specifically what you were fixing. Since your patch splits up
logically in to different components, it makes sense to split the patch
up in to a series that makes it obvious. I'm not sure why this needs to
be spelled out for you.

2008-02-04 09:35:42

by Adrian McMenamin

[permalink] [raw]
Subject: Re: [PATCH] SH/Dreamcast - fix regressions, whitespace and memory leaks in Maple Bus driver

On Mon, February 4, 2008 9:02 am, Paul Mundt wrote:

> My previous commit was directly from _your_ patch, given that your
> patches have a history of whitespace damage, this doesn't seem like much
> of a stretch. It's true I neglected to run it through checkpatch, I'll be
> more careful with that in the future when applying patches from certain
> parties.
>

I'm sorry. But that's garbage.

I'm prepared to admit I made a mistake. Clearly you aren't.

2008-02-04 10:01:10

by Paul Mundt

[permalink] [raw]
Subject: Re: [PATCH] SH/Dreamcast - fix regressions, whitespace and memory leaks in Maple Bus driver

On Mon, Feb 04, 2008 at 09:35:11AM -0000, Adrian McMenamin wrote:
> On Mon, February 4, 2008 9:02 am, Paul Mundt wrote:
> > My previous commit was directly from _your_ patch, given that your
> > patches have a history of whitespace damage, this doesn't seem like much
> > of a stretch. It's true I neglected to run it through checkpatch, I'll be
> > more careful with that in the future when applying patches from certain
> > parties.
> >
>
> I'm sorry. But that's garbage.
>
http://lkml.org/lkml/2007/9/20/179

Also in my archives, and piped through checkpatch:

ERROR: use tabs not spaces
#118: FILE: drivers/sh/maple/maplebus.c:66:
+ return -EINVAL;$

WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable
#123: FILE: drivers/sh/maple/maplebus.c:71:
+EXPORT_SYMBOL_GPL(maple_driver_register);

ERROR: use tabs not spaces
#144: FILE: drivers/sh/maple/maplebus.c:92:
+ void (*callback) (struct mapleq * mq),$

ERROR: "foo * bar" should be "foo *bar"
#144: FILE: drivers/sh/maple/maplebus.c:92:
+ void (*callback) (struct mapleq * mq),

ERROR: use tabs not spaces
#145: FILE: drivers/sh/maple/maplebus.c:93:
+ unsigned long interval, unsigned long function)$

ERROR: use tabs not spaces
#162: FILE: drivers/sh/maple/maplebus.c:110:
+ kfree(dev->type->name);$

ERROR: use tabs not spaces
#163: FILE: drivers/sh/maple/maplebus.c:111:
+ kfree(dev->type);$

ERROR: use tabs not spaces
#185: FILE: drivers/sh/maple/maplebus.c:133:
+ return NULL;$

ERROR: use tabs not spaces
#191: FILE: drivers/sh/maple/maplebus.c:139:
+ kfree(mq);$

ERROR: use tabs not spaces
#192: FILE: drivers/sh/maple/maplebus.c:140:
+ return NULL;$

ERROR: use tabs not spaces
#204: FILE: drivers/sh/maple/maplebus.c:152:
+ return NULL;$

ERROR: use tabs not spaces
#211: FILE: drivers/sh/maple/maplebus.c:159:
+ kfree(dev);$

ERROR: use tabs not spaces
#212: FILE: drivers/sh/maple/maplebus.c:160:
+ return NULL;$

ERROR: use tabs not spaces
#221: FILE: drivers/sh/maple/maplebus.c:169:
+ return;$

ERROR: use tabs not spaces
#223: FILE: drivers/sh/maple/maplebus.c:171:
+ kmem_cache_free(maple_queue_cache, mdev->mq->recvbufdcsp);$

ERROR: use tabs not spaces
#224: FILE: drivers/sh/maple/maplebus.c:172:
+ kfree(mdev->mq);$

ERROR: use tabs not spaces
#249: FILE: drivers/sh/maple/maplebus.c:197:
+ mq->command | (to << 8) | (from << 16) | (len << 24);$

ERROR: use tabs not spaces
#252: FILE: drivers/sh/maple/maplebus.c:200:
+ *maple_sendptr++ = *lsendbuf++;$

ERROR: use tabs not spaces
#263: FILE: drivers/sh/maple/maplebus.c:211:
+ return;$

ERROR: use tabs not spaces
#265: FILE: drivers/sh/maple/maplebus.c:213:
+ return;$

ERROR: use tabs not spaces
#269: FILE: drivers/sh/maple/maplebus.c:217:
+ maple_build_block(mq);$

ERROR: use tabs not spaces
#270: FILE: drivers/sh/maple/maplebus.c:218:
+ list_move(&mq->list, &maple_sentq);$

ERROR: use tabs not spaces
#271: FILE: drivers/sh/maple/maplebus.c:219:
+ if (maple_packets++ > MAPLE_MAXPACKETS)$

ERROR: use tabs not spaces
#272: FILE: drivers/sh/maple/maplebus.c:220:
+ break;$

ERROR: use tabs not spaces
#275: FILE: drivers/sh/maple/maplebus.c:223:
+ for (i = 0; i < (1 << MAPLE_DMA_PAGES); i++)$

ERROR: use tabs not spaces
#276: FILE: drivers/sh/maple/maplebus.c:224:
+ dma_cache_sync(0, maple_sendbuf + i * PAGE_SIZE,$

ERROR: use tabs not spaces
#277: FILE: drivers/sh/maple/maplebus.c:225:
+ PAGE_SIZE, DMA_BIDIRECTIONAL);$

ERROR: use tabs not spaces
#282: FILE: drivers/sh/maple/maplebus.c:230:
+ void *devptr)$

ERROR: use tabs not spaces
#290: FILE: drivers/sh/maple/maplebus.c:238:
+ if (maple_drv->connect(mdev) == 0) {$

ERROR: use tabs not spaces
#291: FILE: drivers/sh/maple/maplebus.c:239:
+ mdev->driver = maple_drv;$

ERROR: use tabs not spaces
#292: FILE: drivers/sh/maple/maplebus.c:240:
+ return 1;$

ERROR: use tabs not spaces
#293: FILE: drivers/sh/maple/maplebus.c:241:
+ }$

ERROR: use tabs not spaces
#301: FILE: drivers/sh/maple/maplebus.c:249:
+ return;$

ERROR: use tabs not spaces
#303: FILE: drivers/sh/maple/maplebus.c:251:
+ if (mdev->driver->disconnect)$

ERROR: use tabs not spaces
#304: FILE: drivers/sh/maple/maplebus.c:252:
+ mdev->driver->disconnect(mdev);$

ERROR: use tabs not spaces
#308: FILE: drivers/sh/maple/maplebus.c:256:
+ maple_release_device(&mdev->dev);$

ERROR: use tabs not spaces
#309: FILE: drivers/sh/maple/maplebus.c:257:
+ device_unregister(&mdev->dev);$

ERROR: use tabs not spaces
#332: FILE: drivers/sh/maple/maplebus.c:280:
+ if (*p == ' ')$

ERROR: use tabs not spaces
#333: FILE: drivers/sh/maple/maplebus.c:281:
+ *p = '\0';$

ERROR: use tabs not spaces
#334: FILE: drivers/sh/maple/maplebus.c:282:
+ else$

ERROR: use tabs not spaces
#335: FILE: drivers/sh/maple/maplebus.c:283:
+ break;$

ERROR: use tabs not spaces
#338: FILE: drivers/sh/maple/maplebus.c:286:
+ if (*p == ' ')$

ERROR: use tabs not spaces
#339: FILE: drivers/sh/maple/maplebus.c:287:
+ *p = '\0';$

ERROR: use tabs not spaces
#340: FILE: drivers/sh/maple/maplebus.c:288:
+ else$

ERROR: use tabs not spaces
#341: FILE: drivers/sh/maple/maplebus.c:289:
+ break;$

ERROR: use tabs not spaces
#346: FILE: drivers/sh/maple/maplebus.c:294:
+ /* Do this silently - as not a real device */$

ERROR: use tabs not spaces
#347: FILE: drivers/sh/maple/maplebus.c:295:
+ function = 0;$

ERROR: use tabs not spaces
#348: FILE: drivers/sh/maple/maplebus.c:296:
+ dev->driver = &maple_dummy_driver;$

ERROR: use tabs not spaces
#349: FILE: drivers/sh/maple/maplebus.c:297:
+ sprintf(dev->dev.bus_id, "%d:0.port", dev->port);$

ERROR: use tabs not spaces
#351: FILE: drivers/sh/maple/maplebus.c:299:
+ printk(KERN_INFO$

ERROR: use tabs not spaces
#352: FILE: drivers/sh/maple/maplebus.c:300:
+ "Maple bus at (%d, %d): Connected function 0x%lX\n",$

ERROR: use tabs not spaces
#353: FILE: drivers/sh/maple/maplebus.c:301:
+ dev->port, dev->unit, function);$

ERROR: use tabs not spaces
#355: FILE: drivers/sh/maple/maplebus.c:303:
+ matched =$

ERROR: use tabs not spaces
#356: FILE: drivers/sh/maple/maplebus.c:304:
+ bus_for_each_drv(&maple_bus_type, NULL, dev,$

ERROR: use tabs not spaces
#357: FILE: drivers/sh/maple/maplebus.c:305:
+ attach_matching_maple_driver);$

ERROR: use tabs not spaces
#359: FILE: drivers/sh/maple/maplebus.c:307:
+ if (matched == 0) {$

ERROR: use tabs not spaces
#360: FILE: drivers/sh/maple/maplebus.c:308:
+ /* Driver does not exist yet */$

ERROR: use tabs not spaces
#361: FILE: drivers/sh/maple/maplebus.c:309:
+ printk(KERN_INFO$

ERROR: use tabs not spaces
#362: FILE: drivers/sh/maple/maplebus.c:310:
+ "No maple driver found for this device\n");$

ERROR: use tabs not spaces
#363: FILE: drivers/sh/maple/maplebus.c:311:
+ dev->driver = &maple_dummy_driver;$

ERROR: use tabs not spaces
#364: FILE: drivers/sh/maple/maplebus.c:312:
+ }$

ERROR: use tabs not spaces
#366: FILE: drivers/sh/maple/maplebus.c:314:
+ sprintf(dev->dev.bus_id, "%d:0%d.%lX", dev->port,$

ERROR: use tabs not spaces
#367: FILE: drivers/sh/maple/maplebus.c:315:
+ dev->unit, function);$

ERROR: use tabs not spaces
#375: FILE: drivers/sh/maple/maplebus.c:323:
+ printk(KERN_INFO$

ERROR: use tabs not spaces
#376: FILE: drivers/sh/maple/maplebus.c:324:
+ "Maple bus: Attempt to register device (%x, %x)$

ERROR: patch seems to be corrupt (line wrapped?)
#377: FILE: drivers/sh/maple/maplebus.c:324:
failed.\n",

ERROR: use tabs not spaces
#378: FILE: drivers/sh/maple/maplebus.c:325:
+ dev->port, dev->unit);$

ERROR: use tabs not spaces
#379: FILE: drivers/sh/maple/maplebus.c:326:
+ maple_free_dev(dev);$

ERROR: use tabs not spaces
#396: FILE: drivers/sh/maple/maplebus.c:343:
+ return 1;$

ERROR: use tabs not spaces
#405: FILE: drivers/sh/maple/maplebus.c:352:
+ && time_after(jiffies, maple_dev->when)) {$

ERROR: use tabs not spaces
#406: FILE: drivers/sh/maple/maplebus.c:353:
+ maple_dev->when = jiffies + maple_dev->interval;$

ERROR: use tabs not spaces
#407: FILE: drivers/sh/maple/maplebus.c:354:
+ maple_dev->mq->command = MAPLE_COMMAND_GETCOND;$

ERROR: use tabs not spaces
#408: FILE: drivers/sh/maple/maplebus.c:355:
+ maple_dev->mq->sendbuf = &maple_dev->function;$

ERROR: use tabs not spaces
#409: FILE: drivers/sh/maple/maplebus.c:356:
+ maple_dev->mq->length = 1;$

ERROR: use tabs not spaces
#410: FILE: drivers/sh/maple/maplebus.c:357:
+ maple_add_packet(maple_dev->mq);$

ERROR: use tabs not spaces
#411: FILE: drivers/sh/maple/maplebus.c:358:
+ liststatus++;$

ERROR: use tabs not spaces
#413: FILE: drivers/sh/maple/maplebus.c:360:
+ if (time_after(jiffies, maple_pnp_time)) {$

ERROR: use tabs not spaces
#414: FILE: drivers/sh/maple/maplebus.c:361:
+ maple_dev->mq->command = MAPLE_COMMAND_DEVINFO;$

ERROR: use tabs not spaces
#415: FILE: drivers/sh/maple/maplebus.c:362:
+ maple_dev->mq->length = 0;$

ERROR: use tabs not spaces
#416: FILE: drivers/sh/maple/maplebus.c:363:
+ maple_add_packet(maple_dev->mq);$

ERROR: use tabs not spaces
#417: FILE: drivers/sh/maple/maplebus.c:364:
+ liststatus++;$

ERROR: use tabs not spaces
#418: FILE: drivers/sh/maple/maplebus.c:365:
+ }$

ERROR: use tabs not spaces
#428: FILE: drivers/sh/maple/maplebus.c:375:
+ return;$

ERROR: use tabs not spaces
#430: FILE: drivers/sh/maple/maplebus.c:377:
+ return;$

ERROR: use tabs not spaces
#434: FILE: drivers/sh/maple/maplebus.c:381:
+ setup_maple_commands);$

ERROR: use tabs not spaces
#436: FILE: drivers/sh/maple/maplebus.c:383:
+ maple_pnp_time = jiffies + MAPLE_PNP_INTERVAL;$

ERROR: use tabs not spaces
#438: FILE: drivers/sh/maple/maplebus.c:385:
+ INIT_LIST_HEAD(&maple_sentq);$

ERROR: use tabs not spaces
#439: FILE: drivers/sh/maple/maplebus.c:386:
+ maple_send();$

ERROR: use tabs not spaces
#452: FILE: drivers/sh/maple/maplebus.c:399:
+ ds.port = mdev->port;$

ERROR: use tabs not spaces
#453: FILE: drivers/sh/maple/maplebus.c:400:
+ ds.unit = k + 1;$

ERROR: use tabs not spaces
#454: FILE: drivers/sh/maple/maplebus.c:401:
+ retval =$

ERROR: use tabs not spaces
#455: FILE: drivers/sh/maple/maplebus.c:402:
+ bus_for_each_dev(&maple_bus_type, NULL, &ds,$

ERROR: use tabs not spaces
#456: FILE: drivers/sh/maple/maplebus.c:403:
+ detach_maple_device);$

ERROR: use tabs not spaces
#457: FILE: drivers/sh/maple/maplebus.c:404:
+ if (retval) {$

ERROR: use tabs not spaces
#458: FILE: drivers/sh/maple/maplebus.c:405:
+ submask = submask >> 1;$

ERROR: use tabs not spaces
#459: FILE: drivers/sh/maple/maplebus.c:406:
+ continue;$

ERROR: use tabs not spaces
#460: FILE: drivers/sh/maple/maplebus.c:407:
+ }$

ERROR: use tabs not spaces
#461: FILE: drivers/sh/maple/maplebus.c:408:
+ devcheck = submask & 0x01;$

ERROR: use tabs not spaces
#462: FILE: drivers/sh/maple/maplebus.c:409:
+ if (devcheck) {$

ERROR: use tabs not spaces
#463: FILE: drivers/sh/maple/maplebus.c:410:
+ mdev_add = maple_alloc_dev(mdev->port, k + 1);$

ERROR: use tabs not spaces
#464: FILE: drivers/sh/maple/maplebus.c:411:
+ if (!mdev_add)$

ERROR: use tabs not spaces
#465: FILE: drivers/sh/maple/maplebus.c:412:
+ return;$

ERROR: use tabs not spaces
#466: FILE: drivers/sh/maple/maplebus.c:413:
+ mdev_add->mq->command = MAPLE_COMMAND_DEVINFO;$

ERROR: use tabs not spaces
#467: FILE: drivers/sh/maple/maplebus.c:414:
+ mdev_add->mq->length = 0;$

ERROR: use tabs not spaces
#468: FILE: drivers/sh/maple/maplebus.c:415:
+ maple_add_packet(mdev_add->mq);$

ERROR: use tabs not spaces
#469: FILE: drivers/sh/maple/maplebus.c:416:
+ scanning = 1;$

ERROR: use tabs not spaces
#470: FILE: drivers/sh/maple/maplebus.c:417:
+ }$

ERROR: use tabs not spaces
#471: FILE: drivers/sh/maple/maplebus.c:418:
+ submask = submask >> 1;$

ERROR: use tabs not spaces
#488: FILE: drivers/sh/maple/maplebus.c:435:
+ struct mapleq *mq)$

ERROR: use tabs not spaces
#491: FILE: drivers/sh/maple/maplebus.c:438:
+ list_del(&mq->list);$

ERROR: use tabs not spaces
#492: FILE: drivers/sh/maple/maplebus.c:439:
+ maple_clean_submap(mdev);$

ERROR: use tabs not spaces
#493: FILE: drivers/sh/maple/maplebus.c:440:
+ printk(KERN_INFO$

ERROR: use tabs not spaces
#494: FILE: drivers/sh/maple/maplebus.c:441:
+ "Maple bus device detaching at (%d, %d)\n",$

ERROR: use tabs not spaces
#495: FILE: drivers/sh/maple/maplebus.c:442:
+ mdev->port, mdev->unit);$

ERROR: use tabs not spaces
#496: FILE: drivers/sh/maple/maplebus.c:443:
+ maple_detach_driver(mdev);$

ERROR: use tabs not spaces
#497: FILE: drivers/sh/maple/maplebus.c:444:
+ return;$

ERROR: use tabs not spaces
#500: FILE: drivers/sh/maple/maplebus.c:447:
+ printk(KERN_INFO "No maple devices attached to port %d\n",$

ERROR: use tabs not spaces
#501: FILE: drivers/sh/maple/maplebus.c:448:
+ mdev->port);$

ERROR: use tabs not spaces
#502: FILE: drivers/sh/maple/maplebus.c:449:
+ return;$

ERROR: use tabs not spaces
#509: FILE: drivers/sh/maple/maplebus.c:456:
+ char *recvbuf)$

ERROR: use tabs not spaces
#513: FILE: drivers/sh/maple/maplebus.c:460:
+ maple_attach_driver(mdev);$

ERROR: use tabs not spaces
#514: FILE: drivers/sh/maple/maplebus.c:461:
+ return;$

ERROR: use tabs not spaces
#517: FILE: drivers/sh/maple/maplebus.c:464:
+ submask = recvbuf[2] & 0x1F;$

ERROR: use tabs not spaces
#518: FILE: drivers/sh/maple/maplebus.c:465:
+ if (submask ^ subdevice_map[mdev->port]) {$

ERROR: use tabs not spaces
#519: FILE: drivers/sh/maple/maplebus.c:466:
+ maple_map_subunits(mdev, submask);$

ERROR: use tabs not spaces
#520: FILE: drivers/sh/maple/maplebus.c:467:
+ subdevice_map[mdev->port] = submask;$

ERROR: use tabs not spaces
#521: FILE: drivers/sh/maple/maplebus.c:468:
+ }$

ERROR: use tabs not spaces
#534: FILE: drivers/sh/maple/maplebus.c:481:
+ return;$

ERROR: use tabs not spaces
#537: FILE: drivers/sh/maple/maplebus.c:484:
+ list_for_each_entry_safe(mq, nmq, &maple_sentq, list) {$

ERROR: use tabs not spaces
#538: FILE: drivers/sh/maple/maplebus.c:485:
+ recvbuf = mq->recvbuf;$

ERROR: use tabs not spaces
#539: FILE: drivers/sh/maple/maplebus.c:486:
+ code = recvbuf[0];$

ERROR: use tabs not spaces
#540: FILE: drivers/sh/maple/maplebus.c:487:
+ dev = mq->dev;$

ERROR: use tabs not spaces
#541: FILE: drivers/sh/maple/maplebus.c:488:
+ switch (code) {$

ERROR: use tabs not spaces
#542: FILE: drivers/sh/maple/maplebus.c:489:
+ case MAPLE_RESPONSE_NONE:$

ERROR: use tabs not spaces
#543: FILE: drivers/sh/maple/maplebus.c:490:
+ maple_response_none(dev, mq);$

ERROR: use tabs not spaces
#544: FILE: drivers/sh/maple/maplebus.c:491:
+ break;$

ERROR: use tabs not spaces
#546: FILE: drivers/sh/maple/maplebus.c:493:
+ case MAPLE_RESPONSE_DEVINFO:$

ERROR: use tabs not spaces
#547: FILE: drivers/sh/maple/maplebus.c:494:
+ maple_response_devinfo(dev, recvbuf);$

ERROR: use tabs not spaces
#548: FILE: drivers/sh/maple/maplebus.c:495:
+ break;$

ERROR: use tabs not spaces
#550: FILE: drivers/sh/maple/maplebus.c:497:
+ case MAPLE_RESPONSE_DATATRF:$

ERROR: use tabs not spaces
#551: FILE: drivers/sh/maple/maplebus.c:498:
+ if (dev->callback)$

ERROR: use tabs not spaces
#552: FILE: drivers/sh/maple/maplebus.c:499:
+ dev->callback(mq);$

ERROR: use tabs not spaces
#553: FILE: drivers/sh/maple/maplebus.c:500:
+ break;$

ERROR: use tabs not spaces
#555: FILE: drivers/sh/maple/maplebus.c:502:
+ case MAPLE_RESPONSE_FILEERR:$

ERROR: use tabs not spaces
#556: FILE: drivers/sh/maple/maplebus.c:503:
+ case MAPLE_RESPONSE_AGAIN:$

ERROR: use tabs not spaces
#557: FILE: drivers/sh/maple/maplebus.c:504:
+ case MAPLE_RESPONSE_BADCMD:$

ERROR: use tabs not spaces
#558: FILE: drivers/sh/maple/maplebus.c:505:
+ case MAPLE_RESPONSE_BADFUNC:$

ERROR: use tabs not spaces
#559: FILE: drivers/sh/maple/maplebus.c:506:
+ printk(KERN_DEBUG$

ERROR: use tabs not spaces
#560: FILE: drivers/sh/maple/maplebus.c:507:
+ "Maple non-fatal error 0x%X\n",$

ERROR: use tabs not spaces
#561: FILE: drivers/sh/maple/maplebus.c:508:
+ code);$

ERROR: use tabs not spaces
#562: FILE: drivers/sh/maple/maplebus.c:509:
+ break;$

ERROR: use tabs not spaces
#564: FILE: drivers/sh/maple/maplebus.c:511:
+ case MAPLE_RESPONSE_ALLINFO:$

ERROR: use tabs not spaces
#565: FILE: drivers/sh/maple/maplebus.c:512:
+ printk(KERN_DEBUG$

ERROR: use tabs not spaces
#566: FILE: drivers/sh/maple/maplebus.c:513:
+ "Maple - extended device$

ERROR: use tabs not spaces
#568: FILE: drivers/sh/maple/maplebus.c:514:
+ break;$

ERROR: use tabs not spaces
#570: FILE: drivers/sh/maple/maplebus.c:516:
+ case MAPLE_RESPONSE_OK:$

ERROR: use tabs not spaces
#571: FILE: drivers/sh/maple/maplebus.c:517:
+ break;$

ERROR: use tabs not spaces
#573: FILE: drivers/sh/maple/maplebus.c:519:
+ default:$

ERROR: use tabs not spaces
#574: FILE: drivers/sh/maple/maplebus.c:520:
+ break;$

ERROR: use tabs not spaces
#575: FILE: drivers/sh/maple/maplebus.c:521:
+ }$

ERROR: use tabs not spaces
#576: FILE: drivers/sh/maple/maplebus.c:522:
+ }$

ERROR: use tabs not spaces
#577: FILE: drivers/sh/maple/maplebus.c:523:
+ INIT_LIST_HEAD(&maple_sentq);$

ERROR: use tabs not spaces
#578: FILE: drivers/sh/maple/maplebus.c:524:
+ if (scanning == 1) {$

ERROR: use tabs not spaces
#579: FILE: drivers/sh/maple/maplebus.c:525:
+ maple_send();$

ERROR: use tabs not spaces
#580: FILE: drivers/sh/maple/maplebus.c:526:
+ scanning = 2;$

ERROR: use tabs not spaces
#581: FILE: drivers/sh/maple/maplebus.c:527:
+ } else$

ERROR: use tabs not spaces
#582: FILE: drivers/sh/maple/maplebus.c:528:
+ scanning = 0;$

ERROR: use tabs not spaces
#584: FILE: drivers/sh/maple/maplebus.c:530:
+ if (started == 0)$

ERROR: use tabs not spaces
#585: FILE: drivers/sh/maple/maplebus.c:531:
+ started = 1;$

ERROR: use tabs not spaces
#628: FILE: drivers/sh/maple/maplebus.c:574:
+ (void *) __get_free_pages(GFP_KERNEL | __GFP_ZERO,$

ERROR: use tabs not spaces
#629: FILE: drivers/sh/maple/maplebus.c:575:
+ MAPLE_DMA_PAGES);$

ERROR: use tabs not spaces
#631: FILE: drivers/sh/maple/maplebus.c:577:
+ return -ENOMEM;$

ERROR: use tabs not spaces
#636: FILE: drivers/sh/maple/maplebus.c:582:
+ struct device_driver *drvptr)$

ERROR: use tabs not spaces
#645: FILE: drivers/sh/maple/maplebus.c:591:
+ return 0;$

ERROR: use tabs not spaces
#647: FILE: drivers/sh/maple/maplebus.c:593:
+ be32_to_cpu(maple_drv->function))$

ERROR: use tabs not spaces
#648: FILE: drivers/sh/maple/maplebus.c:594:
+ return 1;$

ERROR: use tabs not spaces
#653: FILE: drivers/sh/maple/maplebus.c:599:
+ int num_envp, char *buffer, int buffer_size)$

ERROR: use tabs not spaces
#664: FILE: drivers/sh/maple/maplebus.c:610:
+ .name = "maple_dummy_driver",$

ERROR: use tabs not spaces
#665: FILE: drivers/sh/maple/maplebus.c:611:
+ .bus = &maple_bus_type,$

WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable
#675: FILE: drivers/sh/maple/maplebus.c:621:
+EXPORT_SYMBOL_GPL(maple_bus_type);

ERROR: use tabs not spaces
#690: FILE: drivers/sh/maple/maplebus.c:636:
+ goto cleanup;$

ERROR: use tabs not spaces
#694: FILE: drivers/sh/maple/maplebus.c:640:
+ goto cleanup_device;$

ERROR: use tabs not spaces
#699: FILE: drivers/sh/maple/maplebus.c:645:
+ goto cleanup_bus;$

ERROR: use tabs not spaces
#704: FILE: drivers/sh/maple/maplebus.c:650:
+ printk(KERN_INFO$

ERROR: use tabs not spaces
#705: FILE: drivers/sh/maple/maplebus.c:651:
+ "Maple bus: Failed to allocate Maple DMA buffers\n");$

ERROR: use tabs not spaces
#706: FILE: drivers/sh/maple/maplebus.c:652:
+ goto cleanup_basic;$

ERROR: use tabs not spaces
#712: FILE: drivers/sh/maple/maplebus.c:658:
+ printk(KERN_INFO$

ERROR: use tabs not spaces
#713: FILE: drivers/sh/maple/maplebus.c:659:
+ "Maple bus: Failed to grab maple DMA IRQ\n");$

ERROR: use tabs not spaces
#714: FILE: drivers/sh/maple/maplebus.c:660:
+ goto cleanup_dma;$

ERROR: use tabs not spaces
#720: FILE: drivers/sh/maple/maplebus.c:666:
+ printk(KERN_INFO "Maple bus: Failed to grab VBLANK IRQ\n");$

ERROR: use tabs not spaces
#721: FILE: drivers/sh/maple/maplebus.c:667:
+ goto cleanup_irq;$

ERROR: use tabs not spaces
#725: FILE: drivers/sh/maple/maplebus.c:671:
+ kmem_cache_create("maple_queue_cache", 0x400, 0,$

ERROR: use tabs not spaces
#726: FILE: drivers/sh/maple/maplebus.c:672:
+ SLAB_HWCACHE_ALIGN, NULL);$

ERROR: use tabs not spaces
#729: FILE: drivers/sh/maple/maplebus.c:675:
+ goto cleanup_bothirqs;$

ERROR: use tabs not spaces
#733: FILE: drivers/sh/maple/maplebus.c:679:
+ mdev[i] = maple_alloc_dev(i, 0);$

ERROR: use tabs not spaces
#734: FILE: drivers/sh/maple/maplebus.c:680:
+ if (!mdev[i]) {$

ERROR: use tabs not spaces
#735: FILE: drivers/sh/maple/maplebus.c:681:
+ while (i-- > 0)$

ERROR: use tabs not spaces
#736: FILE: drivers/sh/maple/maplebus.c:682:
+ maple_free_dev(mdev[i]);$

ERROR: use tabs not spaces
#737: FILE: drivers/sh/maple/maplebus.c:683:
+ goto cleanup_cache;$

ERROR: use tabs not spaces
#738: FILE: drivers/sh/maple/maplebus.c:684:
+ }$

ERROR: use tabs not spaces
#739: FILE: drivers/sh/maple/maplebus.c:685:
+ mdev[i]->registered = 0;$

ERROR: use tabs not spaces
#740: FILE: drivers/sh/maple/maplebus.c:686:
+ mdev[i]->mq->command = MAPLE_COMMAND_DEVINFO;$

ERROR: use tabs not spaces
#741: FILE: drivers/sh/maple/maplebus.c:687:
+ mdev[i]->mq->length = 0;$

ERROR: use tabs not spaces
#742: FILE: drivers/sh/maple/maplebus.c:688:
+ maple_attach_driver(mdev[i]);$

ERROR: use tabs not spaces
#743: FILE: drivers/sh/maple/maplebus.c:689:
+ maple_add_packet(mdev[i]->mq);$

ERROR: use tabs not spaces
#744: FILE: drivers/sh/maple/maplebus.c:690:
+ subdevice_map[i] = 0;$

ERROR: Missing Signed-off-by: line(s)

total: 205 errors, 2 warnings, 758 lines checked

Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Thanks for playing, try again.

2008-02-04 10:19:31

by Adrian McMenamin

[permalink] [raw]
Subject: Re: [PATCH] SH/Dreamcast - fix regressions, whitespace and memory leaks in Maple Bus driver

On Mon, February 4, 2008 9:59 am, Paul Mundt wrote:
> On Mon, Feb 04, 2008 at 09:35:11AM -0000, Adrian McMenamin wrote:
>> On Mon, February 4, 2008 9:02 am, Paul Mundt wrote:
>> > My previous commit was directly from _your_ patch, given that your
>> > patches have a history of whitespace damage, this doesn't seem like
>> much
>> > of a stretch. It's true I neglected to run it through checkpatch, I'll
>> be
>> > more careful with that in the future when applying patches from
>> certain
>> > parties.
>> >
>>
>> I'm sorry. But that's garbage.
>>
>
> Thanks for playing, try again.
>

It's a fair cop. My apologies.

2008-02-04 16:17:40

by Greg KH

[permalink] [raw]
Subject: Re: [PATCH] SH/Dreamcast - fix regressions, whitespace and memory leaks in Maple Bus driver

On Mon, Feb 04, 2008 at 08:27:55AM +0000, Adrian McMenamin wrote:
>
> On Sun, 2008-02-03 at 21:29 -0800, Greg KH wrote:
> > On Sun, Feb 03, 2008 at 08:00:47PM +0000, Adrian McMenamin wrote:
> > > From: Adrian McMenamin
> > >
> > > This patch fixes the regression noted here:
> > > http://lkml.org/lkml/2008/1/26/189 as well as whitespace issues in the
> > > previous commit of this driver and the memory leaks noted here:
> > > http://lkml.org/lkml/2008/2/2/143 (as well as one or two other minor
> > > cleanups).
> >
> > Which portion of the patch fixes the kobject WARN_ON()?
>
>
>
> + if (mdev->registered == 0) {
> + retval = device_register(&mdev->dev);
> + if (retval) {
> + printk(KERN_INFO
> + "Maple bus: Attempt to register device"
> + " (%x, %x) failed.\n",
> + mdev->port, mdev->unit);
> + maple_free_dev(mdev);
> + mdev = NULL;
> + return;
> + }
> + mdev->registered = 1;
> + }
> }
>
>
> Specifically the check on mdev->registered

So the code path could cause devices to be registered more than once?
That seems broken, as no other bus that I know of needs such a check :(

Is there a way to fix the root problem here, instead of this type of
change?

thanks,

greg k-h

2008-02-04 22:38:18

by Adrian McMenamin

[permalink] [raw]
Subject: Re: [PATCH] SH/Dreamcast - fix regressions, whitespace and memory leaks in Maple Bus driver


On Mon, 2008-02-04 at 08:14 -0800, Greg KH wrote:
> On Mon, Feb 04, 2008 at 08:27:55AM +0000, Adrian McMenamin wrote:
> >
> > On Sun, 2008-02-03 at 21:29 -0800, Greg KH wrote:
> > > On Sun, Feb 03, 2008 at 08:00:47PM +0000, Adrian McMenamin wrote:
> > > > From: Adrian McMenamin
> > > >
> > > > This patch fixes the regression noted here:
> > > > http://lkml.org/lkml/2008/1/26/189 as well as whitespace issues in the
> > > > previous commit of this driver and the memory leaks noted here:
> > > > http://lkml.org/lkml/2008/2/2/143 (as well as one or two other minor
> > > > cleanups).
> > >
> > > Which portion of the patch fixes the kobject WARN_ON()?
> >
> >
> >
> > + if (mdev->registered == 0) {
> > + retval = device_register(&mdev->dev);
> > + if (retval) {
> > + printk(KERN_INFO
> > + "Maple bus: Attempt to register device"
> > + " (%x, %x) failed.\n",
> > + mdev->port, mdev->unit);
> > + maple_free_dev(mdev);
> > + mdev = NULL;
> > + return;
> > + }
> > + mdev->registered = 1;
> > + }
> > }
> >
> >
> > Specifically the check on mdev->registered
>
> So the code path could cause devices to be registered more than once?
> That seems broken, as no other bus that I know of needs such a check :(
>
> Is there a way to fix the root problem here, instead of this type of
> change?
>

The hardware is very flaky. If I add in delays to the bus start, it will
detect the devices, but it's not brilliant. Registering an empty device
got round that problem, at the price of testing for the earlier
registration.

2008-02-04 23:09:57

by Adrian McMenamin

[permalink] [raw]
Subject: Re: [PATCH] SH/Dreamcast - fix regressions, whitespace and memory leaks in Maple Bus driver


On Mon, 2008-02-04 at 22:37 +0000, Adrian McMenamin wrote:

>
> The hardware is very flaky. If I add in delays to the bus start, it will
> detect the devices, but it's not brilliant. Registering an empty device
> got round that problem, at the price of testing for the earlier
> registration.
>


I have found if I push it (and drivers) further down the initcall
hierarchy that problem is less significant and the hardware seems to be
properly detected.

I'll look to post a clean up series of patches in the next few days.

2008-02-05 04:59:41

by Greg KH

[permalink] [raw]
Subject: Re: [PATCH] SH/Dreamcast - fix regressions, whitespace and memory leaks in Maple Bus driver

On Mon, Feb 04, 2008 at 10:37:37PM +0000, Adrian McMenamin wrote:
>
> On Mon, 2008-02-04 at 08:14 -0800, Greg KH wrote:
> > On Mon, Feb 04, 2008 at 08:27:55AM +0000, Adrian McMenamin wrote:
> > >
> > > On Sun, 2008-02-03 at 21:29 -0800, Greg KH wrote:
> > > > On Sun, Feb 03, 2008 at 08:00:47PM +0000, Adrian McMenamin wrote:
> > > > > From: Adrian McMenamin
> > > > >
> > > > > This patch fixes the regression noted here:
> > > > > http://lkml.org/lkml/2008/1/26/189 as well as whitespace issues in the
> > > > > previous commit of this driver and the memory leaks noted here:
> > > > > http://lkml.org/lkml/2008/2/2/143 (as well as one or two other minor
> > > > > cleanups).
> > > >
> > > > Which portion of the patch fixes the kobject WARN_ON()?
> > >
> > >
> > >
> > > + if (mdev->registered == 0) {
> > > + retval = device_register(&mdev->dev);
> > > + if (retval) {
> > > + printk(KERN_INFO
> > > + "Maple bus: Attempt to register device"
> > > + " (%x, %x) failed.\n",
> > > + mdev->port, mdev->unit);
> > > + maple_free_dev(mdev);
> > > + mdev = NULL;
> > > + return;
> > > + }
> > > + mdev->registered = 1;
> > > + }
> > > }
> > >
> > >
> > > Specifically the check on mdev->registered
> >
> > So the code path could cause devices to be registered more than once?
> > That seems broken, as no other bus that I know of needs such a check :(
> >
> > Is there a way to fix the root problem here, instead of this type of
> > change?
> >
>
> The hardware is very flaky. If I add in delays to the bus start, it will
> detect the devices, but it's not brilliant. Registering an empty device
> got round that problem, at the price of testing for the earlier
> registration.

That sounds like you are just papering over the problem. Just delay
and let the hardware settle down if needed :)

thanks,

greg k-h