Re: [PATCH 2/2] - SH/Dreamcast - fix maple bus bugs

2008-02-06 Thread Greg KH
On Wed, Feb 06, 2008 at 11:51:21PM +, Adrian McMenamin wrote:
> Replacement second-in-series patch:
> 
> This patch fixes up memory leaks and, by delaying initialisation, makes
> device detection more robust.
> 
> It also makes clearer the difference between struct maple_device and
> struct device, as well as cleaning up the interrupt request code
> (without changing its function in any way).
> 
> Also now removes redundant registration checking.

Thanks for changing this.

greg k-h
--
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/


Re: [PATCH 2/2] - SH/Dreamcast - fix maple bus bugs

2008-02-06 Thread Adrian McMenamin
Replacement second-in-series patch:

This patch fixes up memory leaks and, by delaying initialisation, makes
device detection more robust.

It also makes clearer the difference between struct maple_device and
struct device, as well as cleaning up the interrupt request code
(without changing its function in any way).

Also now removes redundant registration checking.

Signed-off by: Adrian McMenamin <[EMAIL PROTECTED]>



diff --git a/drivers/sh/maple/maple.c b/drivers/sh/maple/maple.c
index 3f341dc..fbca7f8 100644
--- a/drivers/sh/maple/maple.c
+++ b/drivers/sh/maple/maple.c
@@ -30,6 +30,7 @@
 #include 
 #include 
 #include 
+#include 
 
 MODULE_AUTHOR("Yaegshi Takeshi, Paul Mundt, M.R. Brown, Adrian McMenamin");
 MODULE_DESCRIPTION("Maple bus driver for Dreamcast");
@@ -52,7 +53,7 @@ static struct device maple_bus;
 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 {
@@ -72,7 +73,6 @@ int maple_driver_register(struct device_driver *drv)
drv->bus = &maple_bus_type;
return driver_register(drv);
 }
-
 EXPORT_SYMBOL_GPL(maple_driver_register);
 
 /* set hardware registers to enable next round of dma */
@@ -94,15 +94,14 @@ static void maplebus_dma_reset(void)
  * @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;
 }
-
 EXPORT_SYMBOL_GPL(maple_getcond_callback);
 
 static int maple_dma_done(void)
@@ -112,10 +111,19 @@ static int maple_dma_done(void)
 
 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);
 }
 
 /**
@@ -128,10 +136,9 @@ void maple_add_packet(struct mapleq *mq)
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;
 
@@ -139,7 +146,7 @@ static struct mapleq *maple_allocq(struct maple_device *dev)
if (!mq)
return NULL;
 
-   mq->dev = dev;
+   mq->dev = mdev;
mq->recvbufdcsp = kmem_cache_zalloc(maple_queue_cache, GFP_KERNEL);
mq->recvbuf = (void *) P2SEGADDR(mq->recvbufdcsp);
if (!mq->recvbuf) {
@@ -152,22 +159,24 @@ static struct mapleq *maple_allocq(struct maple_device 
*dev)
 
 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)
+   mdev = kzalloc(sizeof(*mdev), GFP_KERNEL);
+   if (!mdev)
return NULL;
 
-   dev->port = port;
-   dev->unit = unit;
-   dev->mq = maple_allocq(dev);
+   mdev->port = port;
+   mdev->unit = unit;
+   mdev->mq = maple_allocq(mdev);
 
-   if (!dev->mq) {
-   kfree(dev);
+   if (!mdev->mq) {
+   kfree(mdev);
return NULL;
}
-
-   return dev;
+   mdev->dev.bus = &maple_bus_type;
+   mdev->dev.parent = &maple_bus;
+   mdev->function = 0;
+   return mdev;
 }
 
 static void maple_free_dev(struct maple_device *mdev)
@@ -175,7 +184,9 @@ static void maple_free_dev(struct maple_device *mdev)
if (!mdev)
return;
if (mdev->mq) {
-   kmem_cache_free(maple_queue_cache, mdev->mq->recvbufdcsp);
+   if (mdev->mq->recvbufdcsp)
+   kmem_cache_free(maple_queue_cache,
+   mdev->mq->recvbufdcsp);
kfree(mdev->mq);
}
kfree(mdev);
@@ -259,80 +270,89 @@ static void maple_detach_driver(struct maple_device *mdev)
mdev->driver->disconnect(mdev);
}
mdev->driver = NULL;
-   if (mdev->registered) {
-   maple_release_device(&mdev->dev);
-   

Re: [PATCH 2/2] - SH/Dreamcast - fix maple bus bugs

2008-02-06 Thread Greg KH
On Wed, Feb 06, 2008 at 11:05:14PM +, Adrian McMenamin wrote:
> 
> On Wed, 2008-02-06 at 15:01 -0800, Greg KH wrote:
> > On Wed, Feb 06, 2008 at 10:53:51PM +, Adrian McMenamin wrote:
> > > - 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);
> > > + mdev->function = function;
> > > + 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;
> > >   }
> > > - dev->registered = 1;
> > 
> > I think you are still papering over the real problem here of trying to
> > register a device twice.  That's something you should never be doing,
> > and your bus walking logic must be incorrect...
> > 
> I just left that in as a sanity check. It should never be acted on. I
> can remove it if you prefer.

Please do, that way, and real problems will show up with that warning
you were getting that started all of this work :)

thanks,

greg k-h
--
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/


Re: [PATCH 2/2] - SH/Dreamcast - fix maple bus bugs

2008-02-06 Thread Adrian McMenamin

On Wed, 2008-02-06 at 15:01 -0800, Greg KH wrote:
> On Wed, Feb 06, 2008 at 10:53:51PM +, Adrian McMenamin wrote:
> > -   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);
> > +   mdev->function = function;
> > +   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;
> > }
> > -   dev->registered = 1;
> 
> I think you are still papering over the real problem here of trying to
> register a device twice.  That's something you should never be doing,
> and your bus walking logic must be incorrect...
> 
I just left that in as a sanity check. It should never be acted on. I
can remove it if you prefer.

--
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/


Re: [PATCH 2/2] - SH/Dreamcast - fix maple bus bugs

2008-02-06 Thread Greg KH
On Wed, Feb 06, 2008 at 10:53:51PM +, Adrian McMenamin wrote:
> - 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);
> + mdev->function = function;
> + 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;
>   }
> - dev->registered = 1;

I think you are still papering over the real problem here of trying to
register a device twice.  That's something you should never be doing,
and your bus walking logic must be incorrect...

thanks,

greg k-h
--
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/


[PATCH 2/2] - SH/Dreamcast - fix maple bus bugs

2008-02-06 Thread Adrian McMenamin
This patch fixes up memory leaks and, by delaying initialisation, makes
device detection more robust.

It also makes clearer the difference between struct maple_device and
struct device, as well as cleaning up the interrupt request code
(without changing its function in any way).

Signed-off by: Adrian McMenamin <[EMAIL PROTECTED]>


diff --git a/drivers/sh/maple/maple.c b/drivers/sh/maple/maple.c
index 3f341dc..60eeb92 100644
--- a/drivers/sh/maple/maple.c
+++ b/drivers/sh/maple/maple.c
@@ -30,6 +30,7 @@
 #include 
 #include 
 #include 
+#include 
 
 MODULE_AUTHOR("Yaegshi Takeshi, Paul Mundt, M.R. Brown, Adrian McMenamin");
 MODULE_DESCRIPTION("Maple bus driver for Dreamcast");
@@ -52,7 +53,7 @@ static struct device maple_bus;
 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 {
@@ -72,7 +73,6 @@ int maple_driver_register(struct device_driver *drv)
drv->bus = &maple_bus_type;
return driver_register(drv);
 }
-
 EXPORT_SYMBOL_GPL(maple_driver_register);
 
 /* set hardware registers to enable next round of dma */
@@ -94,15 +94,14 @@ static void maplebus_dma_reset(void)
  * @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;
 }
-
 EXPORT_SYMBOL_GPL(maple_getcond_callback);
 
 static int maple_dma_done(void)
@@ -112,10 +111,19 @@ static int maple_dma_done(void)
 
 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);
 }
 
 /**
@@ -128,10 +136,9 @@ void maple_add_packet(struct mapleq *mq)
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;
 
@@ -139,7 +146,7 @@ static struct mapleq *maple_allocq(struct maple_device *dev)
if (!mq)
return NULL;
 
-   mq->dev = dev;
+   mq->dev = mdev;
mq->recvbufdcsp = kmem_cache_zalloc(maple_queue_cache, GFP_KERNEL);
mq->recvbuf = (void *) P2SEGADDR(mq->recvbufdcsp);
if (!mq->recvbuf) {
@@ -152,22 +159,24 @@ static struct mapleq *maple_allocq(struct maple_device 
*dev)
 
 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)
+   mdev = kzalloc(sizeof(*mdev), GFP_KERNEL);
+   if (!mdev)
return NULL;
 
-   dev->port = port;
-   dev->unit = unit;
-   dev->mq = maple_allocq(dev);
+   mdev->port = port;
+   mdev->unit = unit;
+   mdev->mq = maple_allocq(mdev);
 
-   if (!dev->mq) {
-   kfree(dev);
+   if (!mdev->mq) {
+   kfree(mdev);
return NULL;
}
-
-   return dev;
+   mdev->dev.bus = &maple_bus_type;
+   mdev->dev.parent = &maple_bus;
+   mdev->function = 0;
+   return mdev;
 }
 
 static void maple_free_dev(struct maple_device *mdev)
@@ -175,7 +184,9 @@ static void maple_free_dev(struct maple_device *mdev)
if (!mdev)
return;
if (mdev->mq) {
-   kmem_cache_free(maple_queue_cache, mdev->mq->recvbufdcsp);
+   if (mdev->mq->recvbufdcsp)
+   kmem_cache_free(maple_queue_cache,
+   mdev->mq->recvbufdcsp);
kfree(mdev->mq);
}
kfree(mdev);
@@ -259,80 +270,93 @@ static void maple_detach_driver(struct maple_device *mdev)
mdev->driver->disconnect(mdev);
}
mdev->driver = NULL;
-   if (mdev->registered) {
-   maple_release_device(&mdev->dev);
+   if (mdev->registered)
device_unregister(&mdev->dev);
-   }
-