2008-11-26 00:34:48

by Stefan Richter

[permalink] [raw]
Subject: [PATCH] ieee1394: fix list corruption (reported at module removal)

If there is more than one FireWire controller present, dummy_zero_addr
and dummy_max_addr were added multiple times to different lists, thus
corrupting the lists. Fix this by allocating them dynamically per host
instead of just once globally.

(Perhaps a better address space allocation algorithm could rid us of the
two dummy address spaces.)

Fixes http://bugzilla.kernel.org/show_bug.cgi?id=10129 .

Signed-off-by: Stefan Richter <[email protected]>
---
drivers/ieee1394/highlevel.c | 25 ++++++++++++-------------
drivers/ieee1394/hosts.h | 4 ++++
2 files changed, 16 insertions(+), 13 deletions(-)

Index: linux/drivers/ieee1394/highlevel.c
===================================================================
--- linux.orig/drivers/ieee1394/highlevel.c
+++ linux/drivers/ieee1394/highlevel.c
@@ -46,10 +46,6 @@ static DEFINE_RWLOCK(hl_irqs_lock);

static DEFINE_RWLOCK(addr_space_lock);

-/* addr_space list will have zero and max already included as bounds */
-static struct hpsb_address_ops dummy_ops = { NULL, NULL, NULL, NULL };
-static struct hpsb_address_serve dummy_zero_addr, dummy_max_addr;
-

static struct hl_host_info *hl_get_hostinfo(struct hpsb_highlevel *hl,
struct hpsb_host *host)
@@ -481,20 +477,23 @@ int hpsb_unregister_addrspace(struct hps
return retval;
}

+static struct hpsb_address_ops dummy_ops;
+
+/* dummy address spaces as lower and upper bounds of the host's a.s. list */
static void init_hpsb_highlevel(struct hpsb_host *host)
{
- INIT_LIST_HEAD(&dummy_zero_addr.host_list);
- INIT_LIST_HEAD(&dummy_zero_addr.hl_list);
- INIT_LIST_HEAD(&dummy_max_addr.host_list);
- INIT_LIST_HEAD(&dummy_max_addr.hl_list);
+ INIT_LIST_HEAD(&host->dummy_zero_addr.host_list);
+ INIT_LIST_HEAD(&host->dummy_zero_addr.hl_list);
+ INIT_LIST_HEAD(&host->dummy_max_addr.host_list);
+ INIT_LIST_HEAD(&host->dummy_max_addr.hl_list);

- dummy_zero_addr.op = dummy_max_addr.op = &dummy_ops;
+ host->dummy_zero_addr.op = host->dummy_max_addr.op = &dummy_ops;

- dummy_zero_addr.start = dummy_zero_addr.end = 0;
- dummy_max_addr.start = dummy_max_addr.end = ((u64) 1) << 48;
+ host->dummy_zero_addr.start = host->dummy_zero_addr.end = 0;
+ host->dummy_max_addr.start = host->dummy_max_addr.end = ((u64) 1) << 48;

- list_add_tail(&dummy_zero_addr.host_list, &host->addr_space);
- list_add_tail(&dummy_max_addr.host_list, &host->addr_space);
+ list_add_tail(&host->dummy_zero_addr.host_list, &host->addr_space);
+ list_add_tail(&host->dummy_max_addr.host_list, &host->addr_space);
}

void highlevel_add_host(struct hpsb_host *host)
Index: linux/drivers/ieee1394/hosts.h
===================================================================
--- linux.orig/drivers/ieee1394/hosts.h
+++ linux/drivers/ieee1394/hosts.h
@@ -13,6 +13,7 @@ struct module;

#include "ieee1394_types.h"
#include "csr.h"
+#include "highlevel.h"

struct hpsb_packet;
struct hpsb_iso;
@@ -72,6 +73,9 @@ struct hpsb_host {
struct { DECLARE_BITMAP(map, 64); } tl_pool[ALL_NODES];

struct csr_control csr;
+
+ struct hpsb_address_serve dummy_zero_addr;
+ struct hpsb_address_serve dummy_max_addr;
};

enum devctl_cmd {

--
Stefan Richter
-=====-==--- =-== ==-=-
http://arcgraph.de/sr/


2008-11-26 00:35:22

by Stefan Richter

[permalink] [raw]
Subject: [PATCH] ieee1394: replace a GFP_ATOMIC by GFP_KERNEL allocation

All callers of hpsb_register_addrspace() can sleep.

Signed-off-by: Stefan Richter <[email protected]>
---
drivers/ieee1394/highlevel.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux/drivers/ieee1394/highlevel.c
===================================================================
--- linux.orig/drivers/ieee1394/highlevel.c
+++ linux/drivers/ieee1394/highlevel.c
@@ -420,7 +420,7 @@ int hpsb_register_addrspace(struct hpsb_
return 0;
}

- as = kmalloc(sizeof(*as), GFP_ATOMIC);
+ as = kmalloc(sizeof(*as), GFP_KERNEL);
if (!as)
return 0;


--
Stefan Richter
-=====-==--- =-== ==-=-
http://arcgraph.de/sr/

2008-11-26 00:35:44

by Stefan Richter

[permalink] [raw]
Subject: [PATCH] ieee1394: mark all hpsb_address_ops instances as const

These are never modified.

Signed-off-by: Stefan Richter <[email protected]>
---
drivers/ieee1394/csr.c | 8 ++++----
drivers/ieee1394/eth1394.c | 2 +-
drivers/ieee1394/highlevel.c | 7 ++++---
drivers/ieee1394/highlevel.h | 7 ++++---
drivers/ieee1394/raw1394.c | 2 +-
drivers/ieee1394/sbp2.c | 4 ++--
6 files changed, 16 insertions(+), 14 deletions(-)

Index: linux/drivers/ieee1394/csr.c
===================================================================
--- linux.orig/drivers/ieee1394/csr.c
+++ linux/drivers/ieee1394/csr.c
@@ -68,22 +68,22 @@ static struct hpsb_highlevel csr_highlev
.host_reset = host_reset,
};

-static struct hpsb_address_ops map_ops = {
+const static struct hpsb_address_ops map_ops = {
.read = read_maps,
};

-static struct hpsb_address_ops fcp_ops = {
+const static struct hpsb_address_ops fcp_ops = {
.write = write_fcp,
};

-static struct hpsb_address_ops reg_ops = {
+const static struct hpsb_address_ops reg_ops = {
.read = read_regs,
.write = write_regs,
.lock = lock_regs,
.lock64 = lock64_regs,
};

-static struct hpsb_address_ops config_rom_ops = {
+const static struct hpsb_address_ops config_rom_ops = {
.read = read_config_rom,
};

Index: linux/drivers/ieee1394/eth1394.c
===================================================================
--- linux.orig/drivers/ieee1394/eth1394.c
+++ linux/drivers/ieee1394/eth1394.c
@@ -181,7 +181,7 @@ static void ether1394_remove_host(struct
static void ether1394_host_reset(struct hpsb_host *host);

/* Function for incoming 1394 packets */
-static struct hpsb_address_ops addr_ops = {
+const static struct hpsb_address_ops addr_ops = {
.write = ether1394_write,
};

Index: linux/drivers/ieee1394/highlevel.c
===================================================================
--- linux.orig/drivers/ieee1394/highlevel.c
+++ linux/drivers/ieee1394/highlevel.c
@@ -320,7 +320,7 @@ void hpsb_unregister_highlevel(struct hp
*/
u64 hpsb_allocate_and_register_addrspace(struct hpsb_highlevel *hl,
struct hpsb_host *host,
- struct hpsb_address_ops *ops,
+ const struct hpsb_address_ops *ops,
u64 size, u64 alignment,
u64 start, u64 end)
{
@@ -407,7 +407,8 @@ u64 hpsb_allocate_and_register_addrspace
* are automatically deallocated together with the hpsb_highlevel @hl.
*/
int hpsb_register_addrspace(struct hpsb_highlevel *hl, struct hpsb_host *host,
- struct hpsb_address_ops *ops, u64 start, u64 end)
+ const struct hpsb_address_ops *ops,
+ u64 start, u64 end)
{
struct hpsb_address_serve *as;
struct list_head *lh;
@@ -477,7 +478,7 @@ int hpsb_unregister_addrspace(struct hps
return retval;
}

-static struct hpsb_address_ops dummy_ops;
+const static struct hpsb_address_ops dummy_ops;

/* dummy address spaces as lower and upper bounds of the host's a.s. list */
static void init_hpsb_highlevel(struct hpsb_host *host)
Index: linux/drivers/ieee1394/highlevel.h
===================================================================
--- linux.orig/drivers/ieee1394/highlevel.h
+++ linux/drivers/ieee1394/highlevel.h
@@ -15,7 +15,7 @@ struct hpsb_host;
struct hpsb_address_serve {
struct list_head host_list; /* per host list */
struct list_head hl_list; /* hpsb_highlevel list */
- struct hpsb_address_ops *op;
+ const struct hpsb_address_ops *op;
struct hpsb_host *host;
u64 start; /* first address handled, quadlet aligned */
u64 end; /* first address behind, quadlet aligned */
@@ -119,11 +119,12 @@ void hpsb_unregister_highlevel(struct hp

u64 hpsb_allocate_and_register_addrspace(struct hpsb_highlevel *hl,
struct hpsb_host *host,
- struct hpsb_address_ops *ops,
+ const struct hpsb_address_ops *ops,
u64 size, u64 alignment,
u64 start, u64 end);
int hpsb_register_addrspace(struct hpsb_highlevel *hl, struct hpsb_host *host,
- struct hpsb_address_ops *ops, u64 start, u64 end);
+ const struct hpsb_address_ops *ops,
+ u64 start, u64 end);
int hpsb_unregister_addrspace(struct hpsb_highlevel *hl, struct hpsb_host *host,
u64 start);

Index: linux/drivers/ieee1394/raw1394.c
===================================================================
--- linux.orig/drivers/ieee1394/raw1394.c
+++ linux/drivers/ieee1394/raw1394.c
@@ -90,7 +90,7 @@ static int arm_lock(struct hpsb_host *ho
static int arm_lock64(struct hpsb_host *host, int nodeid, octlet_t * store,
u64 addr, octlet_t data, octlet_t arg, int ext_tcode,
u16 flags);
-static struct hpsb_address_ops arm_ops = {
+const static struct hpsb_address_ops arm_ops = {
.read = arm_read,
.write = arm_write,
.lock = arm_lock,
Index: linux/drivers/ieee1394/sbp2.c
===================================================================
--- linux.orig/drivers/ieee1394/sbp2.c
+++ linux/drivers/ieee1394/sbp2.c
@@ -265,7 +265,7 @@ static struct hpsb_highlevel sbp2_highle
.host_reset = sbp2_host_reset,
};

-static struct hpsb_address_ops sbp2_ops = {
+const static struct hpsb_address_ops sbp2_ops = {
.write = sbp2_handle_status_write
};

@@ -275,7 +275,7 @@ static int sbp2_handle_physdma_write(str
static int sbp2_handle_physdma_read(struct hpsb_host *, int, quadlet_t *, u64,
size_t, u16);

-static struct hpsb_address_ops sbp2_physdma_ops = {
+const static struct hpsb_address_ops sbp2_physdma_ops = {
.read = sbp2_handle_physdma_read,
.write = sbp2_handle_physdma_write,
};

--
Stefan Richter
-=====-==--- =-== ==-=-
http://arcgraph.de/sr/