Re: [PATCH 2/2] - SH/Dreamcast - fix maple bus bugs
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
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
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
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
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
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);
- }
-
