[PATCH] firewire: replace static ROM cache by allocated cache

[PATCH] firewire: replace static ROM cache by allocated cache

[Stefan Richter]

read_bus_info_block() is repeatedly called by workqueue jobs.
These will step on each others toes eventually if there are multiple
workqueue threads, and we end up with corrupt config ROM images.

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
 drivers/firewire/fw-device.c |   41 +++++++++++++++++++++++------------
 1 file changed, 27 insertions(+), 14 deletions(-)

Index: linux/drivers/firewire/fw-device.c
===================================================================
--- linux.orig/drivers/firewire/fw-device.c
+++ linux/drivers/firewire/fw-device.c
@@ -400,6 +400,9 @@ read_rom(struct fw_device *device, int g
 	return callback_data.rcode;
 }
 
+#define READ_BIB_ROM_SIZE	256
+#define READ_BIB_STACK_SIZE	16
+
 /*
  * Read the bus info block, perform a speed probe, and read all of the rest of
  * the config ROM.  We do all this with a cached bus generation.  If the bus
@@ -409,16 +412,23 @@ read_rom(struct fw_device *device, int g
  */
 static int read_bus_info_block(struct fw_device *device, int generation)
 {
-	static u32 rom[256];
-	u32 stack[16], sp, key;
-	int i, end, length;
+	u32 *rom, *stack;
+	u32 sp, key;
+	int i, end, length, ret = -1;
+
+	rom = kmalloc(sizeof(*rom) * READ_BIB_ROM_SIZE +
+		      sizeof(*stack) * READ_BIB_STACK_SIZE, GFP_KERNEL);
+	if (rom == NULL)
+		return -ENOMEM;
+
+	stack = &rom[READ_BIB_ROM_SIZE];
 
 	device->max_speed = SCODE_100;
 
 	/* First read the bus info block. */
 	for (i = 0; i < 5; i++) {
 		if (read_rom(device, generation, i, &rom[i]) != RCODE_COMPLETE)
-			return -1;
+			goto out;
 		/*
 		 * As per IEEE1212 7.2, during power-up, devices can
 		 * reply with a 0 for the first quadlet of the config
@@ -428,7 +438,7 @@ static int read_bus_info_block(struct fw
 		 * retry mechanism will try again later.
 		 */
 		if (i == 0 && rom[i] == 0)
-			return -1;
+			goto out;
 	}
 
 	device->max_speed = device->node->max_speed;
@@ -478,26 +488,26 @@ static int read_bus_info_block(struct fw
 		 */
 		key = stack[--sp];
 		i = key & 0xffffff;
-		if (i >= ARRAY_SIZE(rom))
+		if (i >= READ_BIB_ROM_SIZE)
 			/*
 			 * The reference points outside the standard
 			 * config rom area, something's fishy.
 			 */
-			return -1;
+			goto out;
 
 		/* Read header quadlet for the block to get the length. */
 		if (read_rom(device, generation, i, &rom[i]) != RCODE_COMPLETE)
-			return -1;
+			goto out;
 		end = i + (rom[i] >> 16) + 1;
 		i++;
-		if (end > ARRAY_SIZE(rom))
+		if (end > READ_BIB_ROM_SIZE)
 			/*
 			 * This block extends outside standard config
 			 * area (and the array we're reading it
 			 * into).  That's broken, so ignore this
 			 * device.
 			 */
-			return -1;
+			goto out;
 
 		/*
 		 * Now read in the block.  If this is a directory
@@ -507,9 +517,9 @@ static int read_bus_info_block(struct fw
 		while (i < end) {
 			if (read_rom(device, generation, i, &rom[i]) !=
 			    RCODE_COMPLETE)
-				return -1;
+				goto out;
 			if ((key >> 30) == 3 && (rom[i] >> 30) > 1 &&
-			    sp < ARRAY_SIZE(stack))
+			    sp < READ_BIB_STACK_SIZE)
 				stack[sp++] = i + rom[i];
 			i++;
 		}
@@ -519,11 +529,14 @@ static int read_bus_info_block(struct fw
 
 	device->config_rom = kmalloc(length * 4, GFP_KERNEL);
 	if (device->config_rom == NULL)
-		return -1;
+		goto out;
 	memcpy(device->config_rom, rom, length * 4);
 	device->config_rom_length = length;
+	ret = 0;
+ out:
+	kfree(rom);
 
-	return 0;
+	return ret;
 }
 
 static void fw_unit_release(struct device *dev)

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

[PATCH] firewire: reread config ROM when device reset the bus

[Stefan Richter]

When a device changes its configuration ROM, it announces this with a
bus reset.  firewire-core has to check which node initiated a bus reset
and whether any unit directories went away or were added on this node.

Tested with an IOI FWB-IDE01AB which has its link-on bit set if bus
power is available but does not respond to ROM read requests if self
power is off.  This implements
  - recognition of the units if self power is switched on after fw-core
    gave up the initial attempt to read the config ROM,
  - shutdown of the units when self power is switched off.

Also tested with a second PC running Linux/ieee1394.  When the eth1394
driver is inserted and removed on that node, fw-core now notices the
addition and removal of the IPv4 unit on the ieee1394 node.

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---

Applies after "firewire: replace static ROM cache by allocated cache".

 drivers/firewire/fw-cdev.c     |   18 ++--
 drivers/firewire/fw-device.c   |  147 ++++++++++++++++++++++++++++++---
 drivers/firewire/fw-topology.c |    3 
 drivers/firewire/fw-topology.h |   11 +-
 4 files changed, 158 insertions(+), 21 deletions(-)

Index: linux/drivers/firewire/fw-cdev.c
===================================================================
--- linux.orig/drivers/firewire/fw-cdev.c
+++ linux/drivers/firewire/fw-cdev.c
@@ -32,6 +32,7 @@
 #include <linux/idr.h>
 #include <linux/compat.h>
 #include <linux/firewire-cdev.h>
+#include <asm/semaphore.h>
 #include <asm/system.h>
 #include <asm/uaccess.h>
 #include "fw-transaction.h"
@@ -269,20 +270,25 @@ static int ioctl_get_info(struct client 
 {
 	struct fw_cdev_get_info *get_info = buffer;
 	struct fw_cdev_event_bus_reset bus_reset;
+	struct fw_device *device = client->device;
+	unsigned long ret = 0;
 
 	client->version = get_info->version;
 	get_info->version = FW_CDEV_VERSION;
 
+	down(&device->device.sem);
 	if (get_info->rom != 0) {
 		void __user *uptr = u64_to_uptr(get_info->rom);
 		size_t want = get_info->rom_length;
-		size_t have = client->device->config_rom_length * 4;
+		size_t have;
 
-		if (copy_to_user(uptr, client->device->config_rom,
-				 min(want, have)))
-			return -EFAULT;
+		have = device->config_rom_length * 4;
+		ret = copy_to_user(uptr, device->config_rom, min(want, have));
 	}
-	get_info->rom_length = client->device->config_rom_length * 4;
+	get_info->rom_length = device->config_rom_length * 4;
+	up(&device->device.sem);
+	if (ret != 0)
+		return -EFAULT;
 
 	client->bus_reset_closure = get_info->bus_reset_closure;
 	if (get_info->bus_reset != 0) {
@@ -293,7 +299,7 @@ static int ioctl_get_info(struct client 
 			return -EFAULT;
 	}
 
-	get_info->card = client->device->card->index;
+	get_info->card = device->card->index;
 
 	return 0;
 }
Index: linux/drivers/firewire/fw-device.c
===================================================================
--- linux.orig/drivers/firewire/fw-device.c
+++ linux/drivers/firewire/fw-device.c
@@ -26,6 +26,7 @@
 #include <linux/delay.h>
 #include <linux/idr.h>
 #include <linux/rwsem.h>
+#include <linux/string.h>
 #include <asm/semaphore.h>
 #include <asm/system.h>
 #include <linux/ctype.h>
@@ -160,9 +161,9 @@ static void fw_device_release(struct dev
 	 * Take the card lock so we don't set this to NULL while a
 	 * FW_NODE_UPDATED callback is being handled.
 	 */
-	spin_lock_irqsave(&device->card->lock, flags);
+	spin_lock_irqsave(&card->lock, flags);
 	device->node->data = NULL;
-	spin_unlock_irqrestore(&device->card->lock, flags);
+	spin_unlock_irqrestore(&card->lock, flags);
 
 	fw_node_put(device->node);
 	kfree(device->config_rom);
@@ -337,10 +338,14 @@ static ssize_t
 config_rom_show(struct device *dev, struct device_attribute *attr, char *buf)
 {
 	struct fw_device *device = fw_device(dev);
+	size_t length;
 
-	memcpy(buf, device->config_rom, device->config_rom_length * 4);
+	down(&dev->sem);
+	length = device->config_rom_length * 4;
+	memcpy(buf, device->config_rom, length);
+	up(&dev->sem);
 
-	return device->config_rom_length * 4;
+	return length;
 }
 
 static ssize_t
@@ -412,7 +417,7 @@ read_rom(struct fw_device *device, int g
  */
 static int read_bus_info_block(struct fw_device *device, int generation)
 {
-	u32 *rom, *stack;
+	u32 *rom, *stack, *old_rom, *new_rom;
 	u32 sp, key;
 	int i, end, length, ret = -1;
 
@@ -527,11 +532,18 @@ static int read_bus_info_block(struct fw
 			length = i;
 	}
 
-	device->config_rom = kmalloc(length * 4, GFP_KERNEL);
-	if (device->config_rom == NULL)
+	old_rom = device->config_rom;
+	new_rom = kmemdup(rom, length * 4, GFP_KERNEL);
+	if (new_rom == NULL)
 		goto out;
-	memcpy(device->config_rom, rom, length * 4);
+
+	/* serialize with readers via sysfs or ioctl */
+	down(&device->device.sem);
+	device->config_rom = new_rom;
 	device->config_rom_length = length;
+	up(&device->device.sem);
+
+	kfree(old_rom);
 	ret = 0;
  out:
 	kfree(rom);
@@ -724,7 +736,7 @@ static void fw_device_init(struct work_s
 	if (atomic_cmpxchg(&device->state,
 		    FW_DEVICE_INITIALIZING,
 		    FW_DEVICE_RUNNING) == FW_DEVICE_SHUTDOWN) {
-		fw_device_shutdown(&device->work.work);
+		fw_device_shutdown(work);
 	} else {
 		if (device->config_rom_retries)
 			fw_notify("created device %s: GUID %08x%08x, S%d00, "
@@ -738,6 +750,7 @@ static void fw_device_init(struct work_s
 				  device->device.bus_id,
 				  device->config_rom[3], device->config_rom[4],
 				  1 << device->max_speed);
+		device->config_rom_retries = 0;
 	}
 
 	/*
@@ -784,6 +797,104 @@ static void fw_device_update(struct work
 	device_for_each_child(&device->device, NULL, update_unit);
 }
 
+enum {
+	REREAD_BIB_ERROR,
+	REREAD_BIB_GONE,
+	REREAD_BIB_UNCHANGED,
+	REREAD_BIB_CHANGED,
+};
+
+/* Reread and compare bus info block and header of root directory */
+static int reread_bus_info_block(struct fw_device *device, int generation)
+{
+	u32 q;
+	int i;
+
+	for (i = 0; i < 6; i++) {
+		if (read_rom(device, generation, i, &q) != RCODE_COMPLETE)
+			return REREAD_BIB_ERROR;
+
+		if (i == 0 && q == 0)
+			return REREAD_BIB_GONE;
+
+		if (i > device->config_rom_length || q != device->config_rom[i])
+			return REREAD_BIB_CHANGED;
+	}
+
+	return REREAD_BIB_UNCHANGED;
+}
+
+static void fw_device_refresh(struct work_struct *work)
+{
+	struct fw_device *device =
+		container_of(work, struct fw_device, work.work);
+	struct fw_card *card = device->card;
+	int node_id = device->node_id;
+
+	switch (reread_bus_info_block(device, device->generation)) {
+	case REREAD_BIB_ERROR:
+		if (device->config_rom_retries < MAX_RETRIES / 2 &&
+		    atomic_read(&device->state) == FW_DEVICE_INITIALIZING) {
+			device->config_rom_retries++;
+			schedule_delayed_work(&device->work, RETRY_DELAY / 2);
+
+			return;
+		}
+		goto give_up;
+
+	case REREAD_BIB_GONE:
+		goto gone;
+
+	case REREAD_BIB_UNCHANGED:
+		if (atomic_cmpxchg(&device->state,
+			    FW_DEVICE_INITIALIZING,
+			    FW_DEVICE_RUNNING) == FW_DEVICE_SHUTDOWN)
+			goto gone;
+
+		fw_device_update(work);
+		device->config_rom_retries = 0;
+
+		return;
+	}
+
+	/*
+	 * Something changed.  We keep things simple and don't investigate
+	 * further.  We just destroy all previous units and create new ones.
+	 */
+	device_for_each_child(&device->device, NULL, shutdown_unit);
+
+	if (read_bus_info_block(device, device->generation) < 0) {
+		if (device->config_rom_retries < MAX_RETRIES &&
+		    atomic_read(&device->state) == FW_DEVICE_INITIALIZING) {
+			device->config_rom_retries++;
+			schedule_delayed_work(&device->work, RETRY_DELAY);
+
+			return;
+		}
+		goto give_up;
+	}
+
+	create_units(device);
+
+	if (atomic_cmpxchg(&device->state,
+		    FW_DEVICE_INITIALIZING,
+		    FW_DEVICE_RUNNING) == FW_DEVICE_SHUTDOWN)
+		goto gone;
+
+	fw_notify("refreshed device %s\n", device->device.bus_id);
+	device->config_rom_retries = 0;
+	goto out;
+
+ give_up:
+	fw_notify("giving up on refresh of device %s\n", device->device.bus_id);
+ gone:
+	atomic_set(&device->state, FW_DEVICE_SHUTDOWN);
+	fw_device_shutdown(work);
+ out:
+	if (node_id == card->root_node->node_id)
+		schedule_delayed_work(&card->work, 0);
+}
+
 void fw_node_event(struct fw_card *card, struct fw_node *node, int event)
 {
 	struct fw_device *device;
@@ -793,7 +904,7 @@ void fw_node_event(struct fw_card *card,
 	case FW_NODE_LINK_ON:
 		if (!node->link_on)
 			break;
-
+ create:
 		device = kzalloc(sizeof(*device), GFP_ATOMIC);
 		if (device == NULL)
 			break;
@@ -832,6 +943,22 @@ void fw_node_event(struct fw_card *card,
 		schedule_delayed_work(&device->work, INITIAL_DELAY);
 		break;
 
+	case FW_NODE_INITIATED_RESET:
+		device = node->data;
+		if (device == NULL)
+			goto create;
+
+		device->node_id = node->node_id;
+		smp_wmb();  /* update node_id before generation */
+		device->generation = card->generation;
+		if (atomic_cmpxchg(&device->state,
+			    FW_DEVICE_RUNNING,
+			    FW_DEVICE_INITIALIZING) == FW_DEVICE_RUNNING) {
+			PREPARE_DELAYED_WORK(&device->work, fw_device_refresh);
+			schedule_delayed_work(&device->work, INITIAL_DELAY);
+		}
+		break;
+
 	case FW_NODE_UPDATED:
 		if (!node->link_on || node->data == NULL)
 			break;
Index: linux/drivers/firewire/fw-topology.c
===================================================================
--- linux.orig/drivers/firewire/fw-topology.c
+++ linux/drivers/firewire/fw-topology.c
@@ -107,6 +107,7 @@ static struct fw_node *fw_node_create(u3
 	node->node_id = LOCAL_BUS | SELF_ID_PHY_ID(sid);
 	node->link_on = SELF_ID_LINK_ON(sid);
 	node->phy_speed = SELF_ID_PHY_SPEED(sid);
+	node->initiated_reset = SELF_ID_PHY_INITIATOR(sid);
 	node->port_count = port_count;
 
 	atomic_set(&node->ref_count, 1);
@@ -430,6 +431,8 @@ update_tree(struct fw_card *card, struct
 			event = FW_NODE_LINK_OFF;
 		else if (!node0->link_on && node1->link_on)
 			event = FW_NODE_LINK_ON;
+		else if (node1->initiated_reset && node1->link_on)
+			event = FW_NODE_INITIATED_RESET;
 		else
 			event = FW_NODE_UPDATED;
 
Index: linux/drivers/firewire/fw-topology.h
===================================================================
--- linux.orig/drivers/firewire/fw-topology.h
+++ linux/drivers/firewire/fw-topology.h
@@ -20,11 +20,12 @@
 #define __fw_topology_h
 
 enum {
-	FW_NODE_CREATED =   0x00,
-	FW_NODE_UPDATED =   0x01,
-	FW_NODE_DESTROYED = 0x02,
-	FW_NODE_LINK_ON =   0x03,
-	FW_NODE_LINK_OFF =  0x04,
+	FW_NODE_CREATED,
+	FW_NODE_UPDATED,
+	FW_NODE_DESTROYED,
+	FW_NODE_LINK_ON,
+	FW_NODE_LINK_OFF,
+	FW_NODE_INITIATED_RESET,
 };
 
 struct fw_node {

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

Re: [PATCH] firewire: reread config ROM when device reset the bus

[Kristian Høgsberg]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=UTF-8, Size: 14006 bytes --]

On Sun, Mar 2, 2008 at 7:48 PM, Stefan Richter<stefanr@s5r6.in-berlin.de> wrote:> When a device changes its configuration ROM, it announces this with a>  bus reset.  firewire-core has to check which node initiated a bus reset>  and whether any unit directories went away or were added on this node.
Nicely done, looks good to me.  I like how simple this turned out tobe.  I would just add a
  case REREAD_BIB_CHANGED:    break;
to the switch to make that clear, but otherwise
Signed-off-by: Kristian Høgsberg <krh@redhat.com>
>  Tested with an IOI FWB-IDE01AB which has its link-on bit set if bus>  power is available but does not respond to ROM read requests if self>  power is off.  This implements>   - recognition of the units if self power is switched on after fw-core>     gave up the initial attempt to read the config ROM,>   - shutdown of the units when self power is switched off.>>  Also tested with a second PC running Linux/ieee1394.  When the eth1394>  driver is inserted and removed on that node, fw-core now notices the>  addition and removal of the IPv4 unit on the ieee1394 node.>>  Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>>  --->>  Applies after "firewire: replace static ROM cache by allocated cache".>>   drivers/firewire/fw-cdev.c     |   18 ++-->   drivers/firewire/fw-device.c   |  147 ++++++++++++++++++++++++++++++--->   drivers/firewire/fw-topology.c |    3>   drivers/firewire/fw-topology.h |   11 +->   4 files changed, 158 insertions(+), 21 deletions(-)>>  Index: linux/drivers/firewire/fw-cdev.c>  ===================================================================>  --- linux.orig/drivers/firewire/fw-cdev.c>  +++ linux/drivers/firewire/fw-cdev.c>  @@ -32,6 +32,7 @@>   #include <linux/idr.h>>   #include <linux/compat.h>>   #include <linux/firewire-cdev.h>>  +#include <asm/semaphore.h>>   #include <asm/system.h>>   #include <asm/uaccess.h>>   #include "fw-transaction.h">  @@ -269,20 +270,25 @@ static int ioctl_get_info(struct client>   {>         struct fw_cdev_get_info *get_info = buffer;>         struct fw_cdev_event_bus_reset bus_reset;>  +       struct fw_device *device = client->device;>  +       unsigned long ret = 0;>>         client->version = get_info->version;>         get_info->version = FW_CDEV_VERSION;>>  +       down(&device->device.sem);>         if (get_info->rom != 0) {>                 void __user *uptr = u64_to_uptr(get_info->rom);>                 size_t want = get_info->rom_length;>  -               size_t have = client->device->config_rom_length * 4;>  +               size_t have;>>  -               if (copy_to_user(uptr, client->device->config_rom,>  -                                min(want, have)))>  -                       return -EFAULT;>  +               have = device->config_rom_length * 4;>  +               ret = copy_to_user(uptr, device->config_rom, min(want, have));>         }>  -       get_info->rom_length = client->device->config_rom_length * 4;>  +       get_info->rom_length = device->config_rom_length * 4;>  +       up(&device->device.sem);>  +       if (ret != 0)>  +               return -EFAULT;>>         client->bus_reset_closure = get_info->bus_reset_closure;>         if (get_info->bus_reset != 0) {>  @@ -293,7 +299,7 @@ static int ioctl_get_info(struct client>                         return -EFAULT;>         }>>  -       get_info->card = client->device->card->index;>  +       get_info->card = device->card->index;>>         return 0;>   }>  Index: linux/drivers/firewire/fw-device.c>  ===================================================================>  --- linux.orig/drivers/firewire/fw-device.c>  +++ linux/drivers/firewire/fw-device.c>  @@ -26,6 +26,7 @@>   #include <linux/delay.h>>   #include <linux/idr.h>>   #include <linux/rwsem.h>>  +#include <linux/string.h>>   #include <asm/semaphore.h>>   #include <asm/system.h>>   #include <linux/ctype.h>>  @@ -160,9 +161,9 @@ static void fw_device_release(struct dev>          * Take the card lock so we don't set this to NULL while a>          * FW_NODE_UPDATED callback is being handled.>          */>  -       spin_lock_irqsave(&device->card->lock, flags);>  +       spin_lock_irqsave(&card->lock, flags);>         device->node->data = NULL;>  -       spin_unlock_irqrestore(&device->card->lock, flags);>  +       spin_unlock_irqrestore(&card->lock, flags);>>         fw_node_put(device->node);>         kfree(device->config_rom);>  @@ -337,10 +338,14 @@ static ssize_t>   config_rom_show(struct device *dev, struct device_attribute *attr, char *buf)>   {>         struct fw_device *device = fw_device(dev);>  +       size_t length;>>  -       memcpy(buf, device->config_rom, device->config_rom_length * 4);>  +       down(&dev->sem);>  +       length = device->config_rom_length * 4;>  +       memcpy(buf, device->config_rom, length);>  +       up(&dev->sem);>>  -       return device->config_rom_length * 4;>  +       return length;>   }>>   static ssize_t>  @@ -412,7 +417,7 @@ read_rom(struct fw_device *device, int g>   */>   static int read_bus_info_block(struct fw_device *device, int generation)>   {>  -       u32 *rom, *stack;>  +       u32 *rom, *stack, *old_rom, *new_rom;>         u32 sp, key;>         int i, end, length, ret = -1;>>  @@ -527,11 +532,18 @@ static int read_bus_info_block(struct fw>                         length = i;>         }>>  -       device->config_rom = kmalloc(length * 4, GFP_KERNEL);>  -       if (device->config_rom == NULL)>  +       old_rom = device->config_rom;>  +       new_rom = kmemdup(rom, length * 4, GFP_KERNEL);>  +       if (new_rom == NULL)>                 goto out;>  -       memcpy(device->config_rom, rom, length * 4);>  +>  +       /* serialize with readers via sysfs or ioctl */>  +       down(&device->device.sem);>  +       device->config_rom = new_rom;>         device->config_rom_length = length;>  +       up(&device->device.sem);>  +>  +       kfree(old_rom);>         ret = 0;>   out:>         kfree(rom);>  @@ -724,7 +736,7 @@ static void fw_device_init(struct work_s>         if (atomic_cmpxchg(&device->state,>                     FW_DEVICE_INITIALIZING,>                     FW_DEVICE_RUNNING) == FW_DEVICE_SHUTDOWN) {>  -               fw_device_shutdown(&device->work.work);>  +               fw_device_shutdown(work);>         } else {>                 if (device->config_rom_retries)>                         fw_notify("created device %s: GUID %08x%08x, S%d00, ">  @@ -738,6 +750,7 @@ static void fw_device_init(struct work_s>                                   device->device.bus_id,>                                   device->config_rom[3], device->config_rom[4],>                                   1 << device->max_speed);>  +               device->config_rom_retries = 0;>         }>>         /*>  @@ -784,6 +797,104 @@ static void fw_device_update(struct work>         device_for_each_child(&device->device, NULL, update_unit);>   }>>  +enum {>  +       REREAD_BIB_ERROR,>  +       REREAD_BIB_GONE,>  +       REREAD_BIB_UNCHANGED,>  +       REREAD_BIB_CHANGED,>  +};>  +>  +/* Reread and compare bus info block and header of root directory */>  +static int reread_bus_info_block(struct fw_device *device, int generation)>  +{>  +       u32 q;>  +       int i;>  +>  +       for (i = 0; i < 6; i++) {>  +               if (read_rom(device, generation, i, &q) != RCODE_COMPLETE)>  +                       return REREAD_BIB_ERROR;>  +>  +               if (i == 0 && q == 0)>  +                       return REREAD_BIB_GONE;>  +>  +               if (i > device->config_rom_length || q != device->config_rom[i])>  +                       return REREAD_BIB_CHANGED;>  +       }>  +>  +       return REREAD_BIB_UNCHANGED;>  +}>  +>  +static void fw_device_refresh(struct work_struct *work)>  +{>  +       struct fw_device *device =>  +               container_of(work, struct fw_device, work.work);>  +       struct fw_card *card = device->card;>  +       int node_id = device->node_id;>  +>  +       switch (reread_bus_info_block(device, device->generation)) {>  +       case REREAD_BIB_ERROR:>  +               if (device->config_rom_retries < MAX_RETRIES / 2 &&>  +                   atomic_read(&device->state) == FW_DEVICE_INITIALIZING) {>  +                       device->config_rom_retries++;>  +                       schedule_delayed_work(&device->work, RETRY_DELAY / 2);>  +>  +                       return;>  +               }>  +               goto give_up;>  +>  +       case REREAD_BIB_GONE:>  +               goto gone;>  +>  +       case REREAD_BIB_UNCHANGED:>  +               if (atomic_cmpxchg(&device->state,>  +                           FW_DEVICE_INITIALIZING,>  +                           FW_DEVICE_RUNNING) == FW_DEVICE_SHUTDOWN)>  +                       goto gone;>  +>  +               fw_device_update(work);>  +               device->config_rom_retries = 0;>  +>  +               return;>  +       }>  +>  +       /*>  +        * Something changed.  We keep things simple and don't investigate>  +        * further.  We just destroy all previous units and create new ones.>  +        */>  +       device_for_each_child(&device->device, NULL, shutdown_unit);>  +>  +       if (read_bus_info_block(device, device->generation) < 0) {>  +               if (device->config_rom_retries < MAX_RETRIES &&>  +                   atomic_read(&device->state) == FW_DEVICE_INITIALIZING) {>  +                       device->config_rom_retries++;>  +                       schedule_delayed_work(&device->work, RETRY_DELAY);>  +>  +                       return;>  +               }>  +               goto give_up;>  +       }>  +>  +       create_units(device);>  +>  +       if (atomic_cmpxchg(&device->state,>  +                   FW_DEVICE_INITIALIZING,>  +                   FW_DEVICE_RUNNING) == FW_DEVICE_SHUTDOWN)>  +               goto gone;>  +>  +       fw_notify("refreshed device %s\n", device->device.bus_id);>  +       device->config_rom_retries = 0;>  +       goto out;>  +>  + give_up:>  +       fw_notify("giving up on refresh of device %s\n", device->device.bus_id);>  + gone:>  +       atomic_set(&device->state, FW_DEVICE_SHUTDOWN);>  +       fw_device_shutdown(work);>  + out:>  +       if (node_id == card->root_node->node_id)>  +               schedule_delayed_work(&card->work, 0);>  +}>  +>   void fw_node_event(struct fw_card *card, struct fw_node *node, int event)>   {>         struct fw_device *device;>  @@ -793,7 +904,7 @@ void fw_node_event(struct fw_card *card,>         case FW_NODE_LINK_ON:>                 if (!node->link_on)>                         break;>  ->  + create:>                 device = kzalloc(sizeof(*device), GFP_ATOMIC);>                 if (device == NULL)>                         break;>  @@ -832,6 +943,22 @@ void fw_node_event(struct fw_card *card,>                 schedule_delayed_work(&device->work, INITIAL_DELAY);>                 break;>>  +       case FW_NODE_INITIATED_RESET:>  +               device = node->data;>  +               if (device == NULL)>  +                       goto create;>  +>  +               device->node_id = node->node_id;>  +               smp_wmb();  /* update node_id before generation */>  +               device->generation = card->generation;>  +               if (atomic_cmpxchg(&device->state,>  +                           FW_DEVICE_RUNNING,>  +                           FW_DEVICE_INITIALIZING) == FW_DEVICE_RUNNING) {>  +                       PREPARE_DELAYED_WORK(&device->work, fw_device_refresh);>  +                       schedule_delayed_work(&device->work, INITIAL_DELAY);>  +               }>  +               break;>  +>         case FW_NODE_UPDATED:>                 if (!node->link_on || node->data == NULL)>                         break;>  Index: linux/drivers/firewire/fw-topology.c>  ===================================================================>  --- linux.orig/drivers/firewire/fw-topology.c>  +++ linux/drivers/firewire/fw-topology.c>  @@ -107,6 +107,7 @@ static struct fw_node *fw_node_create(u3>         node->node_id = LOCAL_BUS | SELF_ID_PHY_ID(sid);>         node->link_on = SELF_ID_LINK_ON(sid);>         node->phy_speed = SELF_ID_PHY_SPEED(sid);>  +       node->initiated_reset = SELF_ID_PHY_INITIATOR(sid);>         node->port_count = port_count;>>         atomic_set(&node->ref_count, 1);>  @@ -430,6 +431,8 @@ update_tree(struct fw_card *card, struct>                         event = FW_NODE_LINK_OFF;>                 else if (!node0->link_on && node1->link_on)>                         event = FW_NODE_LINK_ON;>  +               else if (node1->initiated_reset && node1->link_on)>  +                       event = FW_NODE_INITIATED_RESET;>                 else>                         event = FW_NODE_UPDATED;>>  Index: linux/drivers/firewire/fw-topology.h>  ===================================================================>  --- linux.orig/drivers/firewire/fw-topology.h>  +++ linux/drivers/firewire/fw-topology.h>  @@ -20,11 +20,12 @@>   #define __fw_topology_h>>   enum {>  -       FW_NODE_CREATED =   0x00,>  -       FW_NODE_UPDATED =   0x01,>  -       FW_NODE_DESTROYED = 0x02,>  -       FW_NODE_LINK_ON =   0x03,>  -       FW_NODE_LINK_OFF =  0x04,>  +       FW_NODE_CREATED,>  +       FW_NODE_UPDATED,>  +       FW_NODE_DESTROYED,>  +       FW_NODE_LINK_ON,>  +       FW_NODE_LINK_OFF,>  +       FW_NODE_INITIATED_RESET,>   };>>   struct fw_node {>>  -->  Stefan Richter>  -=====-==--- --== ---==>  http://arcgraph.de/sr/>>>  ------------------------------------------------------------------------->  This SF.net email is sponsored by: Microsoft>  Defy all challenges. Microsoft(R) Visual Studio 2008.>  http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/>  _______________________________________________>  mailing list linux1394-devel@lists.sourceforge.net>  https://lists.sourceforge.net/lists/listinfo/linux1394-devel>ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] firewire: reread config ROM when device reset the bus

[Stefan Richter]

Kristian Høgsberg wrote:
> I would just add a
> 
>   case REREAD_BIB_CHANGED:
>     break;
> 
> to the switch to make that clear,

Yes, that's nicer.

> but otherwise
> 
> Signed-off-by: Kristian Høgsberg <krh@redhat.com>

Thanks.

...
>>  --- linux.orig/drivers/firewire/fw-cdev.c
>>  +++ linux/drivers/firewire/fw-cdev.c
>>  @@ -269,20 +270,25 @@ static int ioctl_get_info(struct client
>>   {
>>         struct fw_cdev_get_info *get_info = buffer;
>>         struct fw_cdev_event_bus_reset bus_reset;
>>  +       struct fw_device *device = client->device;
>>  +       unsigned long ret = 0;
>>
>>         client->version = get_info->version;
>>         get_info->version = FW_CDEV_VERSION;
>>
>>  +       down(&device->device.sem);
>>         if (get_info->rom != 0) {
>>                 void __user *uptr = u64_to_uptr(get_info->rom);
>>                 size_t want = get_info->rom_length;
>>  -               size_t have = client->device->config_rom_length * 4;
>>  +               size_t have;
>>
>>  -               if (copy_to_user(uptr, client->device->config_rom,
>>  -                                min(want, have)))
>>  -                       return -EFAULT;
>>  +               have = device->config_rom_length * 4;
>>  +               ret = copy_to_user(uptr, device->config_rom, min(want, have));
>>         }
>>  -       get_info->rom_length = client->device->config_rom_length * 4;
>>  +       get_info->rom_length = device->config_rom_length * 4;
>>  +       up(&device->device.sem);
>>  +       if (ret != 0)
>>  +               return -EFAULT;
>>
>>         client->bus_reset_closure = get_info->bus_reset_closure;
>>         if (get_info->bus_reset != 0) {

Maybe I should rather use fw-device.c::idr_rwsem instead of device.sem, 
to have better control over who takes the mutex when.  Could also be a 
new dedicated mutex but we don't want to end up with too many of them... 
Do you have an opinion?
-- 
Stefan Richter
-=====-==--- --== ---==
http://arcgraph.de/sr/

Re: [PATCH] firewire: reread config ROM when device reset the bus

[Kristian Høgsberg]

On Mon, Mar 3, 2008 at 11:51 AM, Stefan Richter
<stefanr@s5r6.in-berlin.de> wrote:
...
>  Maybe I should rather use fw-device.c::idr_rwsem instead of device.sem,
>  to have better control over who takes the mutex when.  Could also be a
>  new dedicated mutex but we don't want to end up with too many of them...
>  Do you have an opinion?

Using the struct device mutex is fine, and it parallelizes better than
the global idr_mutex (FWIW).  The only concern I have there is that
the device core structs seem to change now and then, and it's not
clear what is implementation details and what is exported for drivers
to use (eg the subsystem sem).

cheers,
Kristian

Re: [PATCH] firewire: reread config ROM when device reset the bus

[Stefan Richter]

Kristian Høgsberg wrote:
> On Mon, Mar 3, 2008 at 11:51 AM, Stefan Richter
> <stefanr@s5r6.in-berlin.de> wrote:
> ...
>>  Maybe I should rather use fw-device.c::idr_rwsem instead of device.sem,
>>  to have better control over who takes the mutex when.  Could also be a
>>  new dedicated mutex but we don't want to end up with too many of them...
>>  Do you have an opinion?
> 
> Using the struct device mutex is fine, and it parallelizes better than
> the global idr_mutex (FWIW).  The only concern I have there is that
> the device core structs seem to change now and then, and it's not
> clear what is implementation details and what is exported for drivers
> to use (eg the subsystem sem).

This may be more of a concern to anybody who wanted to change the driver 
core internals and might be unsure what to do with those three dev->sem 
taking sites which I added; not so much a concern from the firewire 
driver maintenance POV.

OTOH, contention for idr_rwsem is low and there can be multiple readers 
of course.  The most time consuming thing that could happen would be 
waiting for GFP_KERNEL allocations of new IDR tree leaves.  And maybe 
having the dev->sem in a cacheline but idr_rwsem not is probably not a 
concern for the stuff that the writer and the two readers of the ROM 
cache do.

Ah, wait, there is a 3rd reader: sbp2_probe's sbp2_scan_unit_dir.  So, 
using dev->sem is actually the nicest way for now.
-- 
Stefan Richter
-=====-==--- --== ---==
http://arcgraph.de/sr/

Re: [PATCH] firewire: reread config ROM when device reset the bus

[Stefan Richter]

I wrote:
>> On Mon, Mar 3, 2008 at 11:51 AM, Stefan Richter
>> <stefanr@s5r6.in-berlin.de> wrote:
[...rewriting data of a device with children devices whose driver probe 
accesses these data...]
>>>  Maybe I should rather use fw-device.c::idr_rwsem instead of device.sem,
>>>  to have better control over who takes the mutex when.  Could also be a
>>>  new dedicated mutex but we don't want to end up with too many of 
>>>  them...
[...]
> Ah, wait, there is a 3rd reader: sbp2_probe's sbp2_scan_unit_dir.  So, 
> using dev->sem is actually the nicest way for now.

Or not.  The necessary protection for this and other driver->probe()s 
would be the device->parent.sem, not the device->sem itself.  There seem 
to be several ways how a driver probe may be entered (adding a device 
when the driver is already there; attaching a driver when the device is 
already there...) and I am not sure whether all these paths take the 
device->parent.sem around the probe.  It doesn't seem to be always the case.

Greg, can you comment on this?
-- 
Stefan Richter
-=====-==--- --== ---==
http://arcgraph.de/sr/

Re: [PATCH] firewire: reread config ROM when device reset the bus

[Greg KH]

On Mon, Mar 03, 2008 at 07:35:07PM +0100, Stefan Richter wrote:
> I wrote:
>>> On Mon, Mar 3, 2008 at 11:51 AM, Stefan Richter
>>> <stefanr@s5r6.in-berlin.de> wrote:
> [...rewriting data of a device with children devices whose driver probe 
> accesses these data...]
>>>>  Maybe I should rather use fw-device.c::idr_rwsem instead of device.sem,
>>>>  to have better control over who takes the mutex when.  Could also be a
>>>>  new dedicated mutex but we don't want to end up with too many of  
>>>> them...
> [...]
>> Ah, wait, there is a 3rd reader: sbp2_probe's sbp2_scan_unit_dir.  So, 
>> using dev->sem is actually the nicest way for now.
>
> Or not.  The necessary protection for this and other driver->probe()s would 
> be the device->parent.sem, not the device->sem itself.  There seem to be 
> several ways how a driver probe may be entered (adding a device when the 
> driver is already there; attaching a driver when the device is already 
> there...) and I am not sure whether all these paths take the 
> device->parent.sem around the probe.  It doesn't seem to be always the 
> case.
>
> Greg, can you comment on this?

Hm, comment on what?  Ah, semaphore fun in the device.  If at all
possible, use your own, not the driver core as the locking there is a
bit "odd" as you have seen.  I think Alan Stern has some patches pending
to mess with it all to try to make it sane during the suspend/resume
path.

thanks,

greg k-h

Re: [PATCH] firewire: reread config ROM when device reset the bus

[Stefan Richter]

Greg KH wrote:
> Ah, semaphore fun in the device.  If at all
> possible, use your own, not the driver core as the locking there is a
> bit "odd" as you have seen.  I think Alan Stern has some patches pending
> to mess with it all to try to make it sane during the suspend/resume
> path.

OK, I will redo it so that I don't have to assume anything about 
driver-core internal locking.  Thanks,
-- 
Stefan Richter
-=====-==--- --== --=--
http://arcgraph.de/sr/

[PATCH update] firewire: reread config ROM when device reset the bus

[Stefan Richter]

Date: 
From: Stefan Richter <stefanr@s5r6.in-berlin.de>
Subject: firewire: reread config ROM when device reset the bus

When a device changes its configuration ROM, it announces this with a
bus reset.  firewire-core has to check which node initiated a bus reset
and whether any unit directories went away or were added on this node.

Tested with an IOI FWB-IDE01AB which has its link-on bit set if bus
power is available but does not respond to ROM read requests if self
power is off.  This implements
  - recognition of the units if self power is switched on after fw-core
    gave up the initial attempt to read the config ROM,
  - shutdown of the units when self power is switched off.

Also tested with a second PC running Linux/ieee1394.  When the eth1394
driver is inserted and removed on that node, fw-core now notices the
addition and removal of the IPv4 unit on the ieee1394 node.

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---

Update:
  - added explicit "case REREAD_BIB_CHANGED"
  - moved from dev->dem to idr_rwsem
  - extended note about semaphore protection of .config_rom and
    .config_rom_length
  - secure a few more places which access the config_rom...
    should have covered all of them now

 drivers/firewire/fw-card.c     |    2 
 drivers/firewire/fw-cdev.c     |   13 +
 drivers/firewire/fw-device.c   |  222 +++++++++++++++++++++++++++------
 drivers/firewire/fw-device.h   |   11 +
 drivers/firewire/fw-sbp2.c     |    8 -
 drivers/firewire/fw-topology.c |    3 
 drivers/firewire/fw-topology.h |   11 -
 7 files changed, 223 insertions(+), 47 deletions(-)

Index: linux/drivers/firewire/fw-card.c
===================================================================
--- linux.orig/drivers/firewire/fw-card.c
+++ linux/drivers/firewire/fw-card.c
@@ -331,7 +331,7 @@ fw_card_bm_work(struct work_struct *work
 		 */
 		spin_unlock_irqrestore(&card->lock, flags);
 		goto out;
-	} else if (root_device->config_rom[2] & BIB_CMC) {
+	} else if (root_device->cmc) {
 		/*
 		 * FIXME: I suppose we should set the cmstr bit in the
 		 * STATE_CLEAR register of this node, as described in
Index: linux/drivers/firewire/fw-cdev.c
===================================================================
--- linux.orig/drivers/firewire/fw-cdev.c
+++ linux/drivers/firewire/fw-cdev.c
@@ -269,21 +269,28 @@ static int ioctl_get_info(struct client 
 {
 	struct fw_cdev_get_info *get_info = buffer;
 	struct fw_cdev_event_bus_reset bus_reset;
+	unsigned long ret = 0;
 
 	client->version = get_info->version;
 	get_info->version = FW_CDEV_VERSION;
 
+	down_read(&fw_device_rwsem);
+
 	if (get_info->rom != 0) {
 		void __user *uptr = u64_to_uptr(get_info->rom);
 		size_t want = get_info->rom_length;
 		size_t have = client->device->config_rom_length * 4;
 
-		if (copy_to_user(uptr, client->device->config_rom,
-				 min(want, have)))
-			return -EFAULT;
+		ret = copy_to_user(uptr, client->device->config_rom,
+				   min(want, have));
 	}
 	get_info->rom_length = client->device->config_rom_length * 4;
 
+	up_read(&fw_device_rwsem);
+
+	if (ret != 0)
+		return -EFAULT;
+
 	client->bus_reset_closure = get_info->bus_reset_closure;
 	if (get_info->bus_reset != 0) {
 		void __user *uptr = u64_to_uptr(get_info->bus_reset);
Index: linux/drivers/firewire/fw-device.c
===================================================================
--- linux.orig/drivers/firewire/fw-device.c
+++ linux/drivers/firewire/fw-device.c
@@ -25,7 +25,7 @@
 #include <linux/device.h>
 #include <linux/delay.h>
 #include <linux/idr.h>
-#include <linux/rwsem.h>
+#include <linux/string.h>
 #include <asm/semaphore.h>
 #include <asm/system.h>
 #include <linux/ctype.h>
@@ -160,9 +160,9 @@ static void fw_device_release(struct dev
 	 * Take the card lock so we don't set this to NULL while a
 	 * FW_NODE_UPDATED callback is being handled.
 	 */
-	spin_lock_irqsave(&device->card->lock, flags);
+	spin_lock_irqsave(&card->lock, flags);
 	device->node->data = NULL;
-	spin_unlock_irqrestore(&device->card->lock, flags);
+	spin_unlock_irqrestore(&card->lock, flags);
 
 	fw_node_put(device->node);
 	kfree(device->config_rom);
@@ -195,7 +195,9 @@ show_immediate(struct device *dev, struc
 		container_of(dattr, struct config_rom_attribute, attr);
 	struct fw_csr_iterator ci;
 	u32 *dir;
-	int key, value;
+	int key, value, ret = -ENOENT;
+
+	down_read(&fw_device_rwsem);
 
 	if (is_fw_unit(dev))
 		dir = fw_unit(dev)->directory;
@@ -204,11 +206,15 @@ show_immediate(struct device *dev, struc
 
 	fw_csr_iterator_init(&ci, dir);
 	while (fw_csr_iterator_next(&ci, &key, &value))
-		if (attr->key == key)
-			return snprintf(buf, buf ? PAGE_SIZE : 0,
-					"0x%06x\n", value);
+		if (attr->key == key) {
+			ret = snprintf(buf, buf ? PAGE_SIZE : 0,
+				       "0x%06x\n", value);
+			break;
+		}
+
+	up_read(&fw_device_rwsem);
 
-	return -ENOENT;
+	return ret;
 }
 
 #define IMMEDIATE_ATTR(name, key)				\
@@ -221,9 +227,11 @@ show_text_leaf(struct device *dev, struc
 		container_of(dattr, struct config_rom_attribute, attr);
 	struct fw_csr_iterator ci;
 	u32 *dir, *block = NULL, *p, *end;
-	int length, key, value, last_key = 0;
+	int length, key, value, last_key = 0, ret = -ENOENT;
 	char *b;
 
+	down_read(&fw_device_rwsem);
+
 	if (is_fw_unit(dev))
 		dir = fw_unit(dev)->directory;
 	else
@@ -238,18 +246,20 @@ show_text_leaf(struct device *dev, struc
 	}
 
 	if (block == NULL)
-		return -ENOENT;
+		goto out;
 
 	length = min(block[0] >> 16, 256U);
 	if (length < 3)
-		return -ENOENT;
+		goto out;
 
 	if (block[1] != 0 || block[2] != 0)
 		/* Unknown encoding. */
-		return -ENOENT;
+		goto out;
 
-	if (buf == NULL)
-		return length * 4;
+	if (buf == NULL) {
+		ret = length * 4;
+		goto out;
+	}
 
 	b = buf;
 	end = &block[length + 1];
@@ -259,8 +269,11 @@ show_text_leaf(struct device *dev, struc
 	/* Strip trailing whitespace and add newline. */
 	while (b--, (isspace(*b) || *b == '\0') && b > buf);
 	strcpy(b + 1, "\n");
+	ret = b + 2 - buf;
+ out:
+	up_read(&fw_device_rwsem);
 
-	return b + 2 - buf;
+	return ret;
 }
 
 #define TEXT_LEAF_ATTR(name, key)				\
@@ -337,19 +350,28 @@ static ssize_t
 config_rom_show(struct device *dev, struct device_attribute *attr, char *buf)
 {
 	struct fw_device *device = fw_device(dev);
+	size_t length;
 
-	memcpy(buf, device->config_rom, device->config_rom_length * 4);
+	down_read(&fw_device_rwsem);
+	length = device->config_rom_length * 4;
+	memcpy(buf, device->config_rom, length);
+	up_read(&fw_device_rwsem);
 
-	return device->config_rom_length * 4;
+	return length;
 }
 
 static ssize_t
 guid_show(struct device *dev, struct device_attribute *attr, char *buf)
 {
 	struct fw_device *device = fw_device(dev);
+	int ret;
 
-	return snprintf(buf, PAGE_SIZE, "0x%08x%08x\n",
-			device->config_rom[3], device->config_rom[4]);
+	down_read(&fw_device_rwsem);
+	ret = snprintf(buf, PAGE_SIZE, "0x%08x%08x\n",
+		       device->config_rom[3], device->config_rom[4]);
+	up_read(&fw_device_rwsem);
+
+	return ret;
 }
 
 static struct device_attribute fw_device_attributes[] = {
@@ -412,7 +434,7 @@ read_rom(struct fw_device *device, int g
  */
 static int read_bus_info_block(struct fw_device *device, int generation)
 {
-	u32 *rom, *stack;
+	u32 *rom, *stack, *old_rom, *new_rom;
 	u32 sp, key;
 	int i, end, length, ret = -1;
 
@@ -527,12 +549,19 @@ static int read_bus_info_block(struct fw
 			length = i;
 	}
 
-	device->config_rom = kmalloc(length * 4, GFP_KERNEL);
-	if (device->config_rom == NULL)
+	old_rom = device->config_rom;
+	new_rom = kmemdup(rom, length * 4, GFP_KERNEL);
+	if (new_rom == NULL)
 		goto out;
-	memcpy(device->config_rom, rom, length * 4);
+
+	down_write(&fw_device_rwsem);
+	device->config_rom = new_rom;
 	device->config_rom_length = length;
+	up_write(&fw_device_rwsem);
+
+	kfree(old_rom);
 	ret = 0;
+	device->cmc = rom[2] & 1 << 30;
  out:
 	kfree(rom);
 
@@ -605,7 +634,14 @@ static int shutdown_unit(struct device *
 	return 0;
 }
 
-static DECLARE_RWSEM(idr_rwsem);
+/*
+ * fw_device_rwsem acts as dual purpose mutex:
+ *   - serializes accesses to fw_device_idr,
+ *   - serializes accesses to fw_device.config_rom/.config_rom_length and
+ *     fw_unit.directory, unless those accesses happen at safe occasions
+ */
+DECLARE_RWSEM(fw_device_rwsem);
+
 static DEFINE_IDR(fw_device_idr);
 int fw_cdev_major;
 
@@ -613,11 +649,11 @@ struct fw_device *fw_device_get_by_devt(
 {
 	struct fw_device *device;
 
-	down_read(&idr_rwsem);
+	down_read(&fw_device_rwsem);
 	device = idr_find(&fw_device_idr, MINOR(devt));
 	if (device)
 		fw_device_get(device);
-	up_read(&idr_rwsem);
+	up_read(&fw_device_rwsem);
 
 	return device;
 }
@@ -632,9 +668,9 @@ static void fw_device_shutdown(struct wo
 	device_for_each_child(&device->device, NULL, shutdown_unit);
 	device_unregister(&device->device);
 
-	down_write(&idr_rwsem);
+	down_write(&fw_device_rwsem);
 	idr_remove(&fw_device_idr, minor);
-	up_write(&idr_rwsem);
+	up_write(&fw_device_rwsem);
 	fw_device_put(device);
 }
 
@@ -687,10 +723,10 @@ static void fw_device_init(struct work_s
 	err = -ENOMEM;
 
 	fw_device_get(device);
-	down_write(&idr_rwsem);
+	down_write(&fw_device_rwsem);
 	if (idr_pre_get(&fw_device_idr, GFP_KERNEL))
 		err = idr_get_new(&fw_device_idr, device, &minor);
-	up_write(&idr_rwsem);
+	up_write(&fw_device_rwsem);
 
 	if (err < 0)
 		goto error;
@@ -724,7 +760,7 @@ static void fw_device_init(struct work_s
 	if (atomic_cmpxchg(&device->state,
 		    FW_DEVICE_INITIALIZING,
 		    FW_DEVICE_RUNNING) == FW_DEVICE_SHUTDOWN) {
-		fw_device_shutdown(&device->work.work);
+		fw_device_shutdown(work);
 	} else {
 		if (device->config_rom_retries)
 			fw_notify("created device %s: GUID %08x%08x, S%d00, "
@@ -738,6 +774,7 @@ static void fw_device_init(struct work_s
 				  device->device.bus_id,
 				  device->config_rom[3], device->config_rom[4],
 				  1 << device->max_speed);
+		device->config_rom_retries = 0;
 	}
 
 	/*
@@ -752,9 +789,9 @@ static void fw_device_init(struct work_s
 	return;
 
  error_with_cdev:
-	down_write(&idr_rwsem);
+	down_write(&fw_device_rwsem);
 	idr_remove(&fw_device_idr, minor);
-	up_write(&idr_rwsem);
+	up_write(&fw_device_rwsem);
  error:
 	fw_device_put(device);		/* fw_device_idr's reference */
 
@@ -784,6 +821,107 @@ static void fw_device_update(struct work
 	device_for_each_child(&device->device, NULL, update_unit);
 }
 
+enum {
+	REREAD_BIB_ERROR,
+	REREAD_BIB_GONE,
+	REREAD_BIB_UNCHANGED,
+	REREAD_BIB_CHANGED,
+};
+
+/* Reread and compare bus info block and header of root directory */
+static int reread_bus_info_block(struct fw_device *device, int generation)
+{
+	u32 q;
+	int i;
+
+	for (i = 0; i < 6; i++) {
+		if (read_rom(device, generation, i, &q) != RCODE_COMPLETE)
+			return REREAD_BIB_ERROR;
+
+		if (i == 0 && q == 0)
+			return REREAD_BIB_GONE;
+
+		if (i > device->config_rom_length || q != device->config_rom[i])
+			return REREAD_BIB_CHANGED;
+	}
+
+	return REREAD_BIB_UNCHANGED;
+}
+
+static void fw_device_refresh(struct work_struct *work)
+{
+	struct fw_device *device =
+		container_of(work, struct fw_device, work.work);
+	struct fw_card *card = device->card;
+	int node_id = device->node_id;
+
+	switch (reread_bus_info_block(device, device->generation)) {
+	case REREAD_BIB_ERROR:
+		if (device->config_rom_retries < MAX_RETRIES / 2 &&
+		    atomic_read(&device->state) == FW_DEVICE_INITIALIZING) {
+			device->config_rom_retries++;
+			schedule_delayed_work(&device->work, RETRY_DELAY / 2);
+
+			return;
+		}
+		goto give_up;
+
+	case REREAD_BIB_GONE:
+		goto gone;
+
+	case REREAD_BIB_UNCHANGED:
+		if (atomic_cmpxchg(&device->state,
+			    FW_DEVICE_INITIALIZING,
+			    FW_DEVICE_RUNNING) == FW_DEVICE_SHUTDOWN)
+			goto gone;
+
+		fw_device_update(work);
+		device->config_rom_retries = 0;
+
+		return;
+
+	case REREAD_BIB_CHANGED:
+		break;
+	}
+
+	/*
+	 * Something changed.  We keep things simple and don't investigate
+	 * further.  We just destroy all previous units and create new ones.
+	 */
+	device_for_each_child(&device->device, NULL, shutdown_unit);
+
+	if (read_bus_info_block(device, device->generation) < 0) {
+		if (device->config_rom_retries < MAX_RETRIES &&
+		    atomic_read(&device->state) == FW_DEVICE_INITIALIZING) {
+			device->config_rom_retries++;
+			schedule_delayed_work(&device->work, RETRY_DELAY);
+
+			return;
+		}
+		goto give_up;
+	}
+
+	create_units(device);
+
+	if (atomic_cmpxchg(&device->state,
+		    FW_DEVICE_INITIALIZING,
+		    FW_DEVICE_RUNNING) == FW_DEVICE_SHUTDOWN)
+		goto gone;
+
+	fw_notify("refreshed device %s\n", device->device.bus_id);
+	device->config_rom_retries = 0;
+	goto out;
+
+ give_up:
+	fw_notify("giving up on refresh of device %s\n", device->device.bus_id);
+ gone:
+	atomic_set(&device->state, FW_DEVICE_SHUTDOWN);
+	fw_device_shutdown(work);
+ out:
+	if (node_id == card->root_node->node_id)
+		schedule_delayed_work(&card->work, 0);
+}
+
 void fw_node_event(struct fw_card *card, struct fw_node *node, int event)
 {
 	struct fw_device *device;
@@ -793,7 +931,7 @@ void fw_node_event(struct fw_card *card,
 	case FW_NODE_LINK_ON:
 		if (!node->link_on)
 			break;
-
+ create:
 		device = kzalloc(sizeof(*device), GFP_ATOMIC);
 		if (device == NULL)
 			break;
@@ -832,6 +970,22 @@ void fw_node_event(struct fw_card *card,
 		schedule_delayed_work(&device->work, INITIAL_DELAY);
 		break;
 
+	case FW_NODE_INITIATED_RESET:
+		device = node->data;
+		if (device == NULL)
+			goto create;
+
+		device->node_id = node->node_id;
+		smp_wmb();  /* update node_id before generation */
+		device->generation = card->generation;
+		if (atomic_cmpxchg(&device->state,
+			    FW_DEVICE_RUNNING,
+			    FW_DEVICE_INITIALIZING) == FW_DEVICE_RUNNING) {
+			PREPARE_DELAYED_WORK(&device->work, fw_device_refresh);
+			schedule_delayed_work(&device->work, INITIAL_DELAY);
+		}
+		break;
+
 	case FW_NODE_UPDATED:
 		if (!node->link_on || node->data == NULL)
 			break;
Index: linux/drivers/firewire/fw-device.h
===================================================================
--- linux.orig/drivers/firewire/fw-device.h
+++ linux/drivers/firewire/fw-device.h
@@ -21,6 +21,7 @@
 
 #include <linux/fs.h>
 #include <linux/cdev.h>
+#include <linux/rwsem.h>
 #include <asm/atomic.h>
 
 enum fw_device_state {
@@ -46,6 +47,11 @@ struct fw_attribute_group {
  * fw_device.node_id is guaranteed to be current too.
  *
  * The same applies to fw_device.card->node_id vs. fw_device.generation.
+ *
+ * fw_device.config_rom and fw_device.config_rom_length may be accessed during
+ * the lifetime of any fw_unit belonging to the fw_device, before device_del()
+ * was called on the last fw_unit.  Alternatively, they may be accessed while
+ * holding fw_device_rwsem.
  */
 struct fw_device {
 	atomic_t state;
@@ -53,6 +59,7 @@ struct fw_device {
 	int node_id;
 	int generation;
 	unsigned max_speed;
+	bool cmc;
 	struct fw_card *card;
 	struct device device;
 	struct list_head link;
@@ -92,8 +99,12 @@ int fw_device_enable_phys_dma(struct fw_
 void fw_device_cdev_update(struct fw_device *device);
 void fw_device_cdev_remove(struct fw_device *device);
 
+extern struct rw_semaphore fw_device_rwsem;
 extern int fw_cdev_major;
 
+/*
+ * fw_unit.directory must not be accessed after device_del(&fw_unit.device).
+ */
 struct fw_unit {
 	struct device device;
 	u32 *directory;
Index: linux/drivers/firewire/fw-sbp2.c
===================================================================
--- linux.orig/drivers/firewire/fw-sbp2.c
+++ linux/drivers/firewire/fw-sbp2.c
@@ -153,6 +153,7 @@ struct sbp2_target {
 	struct list_head lu_list;
 
 	u64 management_agent_address;
+	u64 guid;
 	int directory_id;
 	int node_id;
 	int address_high;
@@ -1070,6 +1071,7 @@ static int sbp2_probe(struct device *dev
 	kref_init(&tgt->kref);
 	INIT_LIST_HEAD(&tgt->lu_list);
 	tgt->bus_id = unit->device.bus_id;
+	tgt->guid = (u64)device->config_rom[3] << 32 | device->config_rom[4];
 
 	if (fw_device_enable_phys_dma(device) < 0)
 		goto fail_shost_put;
@@ -1527,16 +1529,14 @@ sbp2_sysfs_ieee1394_id_show(struct devic
 {
 	struct scsi_device *sdev = to_scsi_device(dev);
 	struct sbp2_logical_unit *lu;
-	struct fw_device *device;
 
 	if (!sdev)
 		return 0;
 
 	lu = sdev->hostdata;
-	device = fw_device(lu->tgt->unit->device.parent);
 
-	return sprintf(buf, "%08x%08x:%06x:%04x\n",
-			device->config_rom[3], device->config_rom[4],
+	return sprintf(buf, "%016llx:%06x:%04x\n",
+			(unsigned long long)lu->tgt->guid,
 			lu->tgt->directory_id, lu->lun);
 }
 
Index: linux/drivers/firewire/fw-topology.c
===================================================================
--- linux.orig/drivers/firewire/fw-topology.c
+++ linux/drivers/firewire/fw-topology.c
@@ -107,6 +107,7 @@ static struct fw_node *fw_node_create(u3
 	node->node_id = LOCAL_BUS | SELF_ID_PHY_ID(sid);
 	node->link_on = SELF_ID_LINK_ON(sid);
 	node->phy_speed = SELF_ID_PHY_SPEED(sid);
+	node->initiated_reset = SELF_ID_PHY_INITIATOR(sid);
 	node->port_count = port_count;
 
 	atomic_set(&node->ref_count, 1);
@@ -430,6 +431,8 @@ update_tree(struct fw_card *card, struct
 			event = FW_NODE_LINK_OFF;
 		else if (!node0->link_on && node1->link_on)
 			event = FW_NODE_LINK_ON;
+		else if (node1->initiated_reset && node1->link_on)
+			event = FW_NODE_INITIATED_RESET;
 		else
 			event = FW_NODE_UPDATED;
 
Index: linux/drivers/firewire/fw-topology.h
===================================================================
--- linux.orig/drivers/firewire/fw-topology.h
+++ linux/drivers/firewire/fw-topology.h
@@ -20,11 +20,12 @@
 #define __fw_topology_h
 
 enum {
-	FW_NODE_CREATED =   0x00,
-	FW_NODE_UPDATED =   0x01,
-	FW_NODE_DESTROYED = 0x02,
-	FW_NODE_LINK_ON =   0x03,
-	FW_NODE_LINK_OFF =  0x04,
+	FW_NODE_CREATED,
+	FW_NODE_UPDATED,
+	FW_NODE_DESTROYED,
+	FW_NODE_LINK_ON,
+	FW_NODE_LINK_OFF,
+	FW_NODE_INITIATED_RESET,
 };
 
 struct fw_node {

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

Re: [PATCH update] firewire: reread config ROM when device reset the bus

[Stefan Richter]

I wrote:
>   - moved from dev->dem to idr_rwsem
>   - extended note about semaphore protection of .config_rom and
>     .config_rom_length
>   - secure a few more places which access the config_rom...
>     should have covered all of them now

Hmm.  When I power the PC down there are lots of messages scrolling by
which look somewhat like lockdep spew.  I can't reproduce this merely by
module unloading though.  So don't put this patch into production yet.
-- 
Stefan Richter
-=====-==--- --== --=-=
http://arcgraph.de/sr/

Re: [PATCH update] firewire: reread config ROM when device reset the bus

[Stefan Richter]

I wrote:
> I wrote:
>>   - moved from dev->dem to idr_rwsem
>>   - extended note about semaphore protection of .config_rom and
>>     .config_rom_length
>>   - secure a few more places which access the config_rom...
>>     should have covered all of them now
> 
> Hmm.  When I power the PC down there are lots of messages scrolling by
> which look somewhat like lockdep spew.  I can't reproduce this merely by
> module unloading though.  So don't put this patch into production yet.

/...a few hundreds reboots later.../

No, it is not this patch.  It is something else.  And whatever it is, it 
is already present in 2.6.25-rc3.

To reproduce it, I need to plug an SBP-2 device in and out, then shut 
the machine down (e.g. shutdown -h now, whereas shutdown -r now does not 
seem to trigger the bug).

Since it happens after all filesystems were unmounted or r/o-mounted, I 
can't capture the log output easily (perhaps with a netconsole or so) 
but it also can't do any damage at that point anymore.

It does not happen with ohci1394 + sbp2, so it is obviously located in 
the firewire stack.

I am now gradually removing debug options from the kernel to see which 
debug facility is making the fuzz...
-- 
Stefan Richter
-=====-==--- --== --=-=
http://arcgraph.de/sr/

Re: [PATCH update] firewire: reread config ROM when device reset the bus

[Stefan Richter]

I wrote:
> I wrote:
>> When I power the PC down there are lots of messages scrolling by
>> which look somewhat like lockdep spew.  I can't reproduce this merely by
>> module unloading though.  So don't put this patch into production yet.
> 
> /...a few hundreds reboots later.../
> 
> No, it is not this patch.  It is something else.  And whatever it is, it 
> is already present in 2.6.25-rc3.
> 
> To reproduce it, I need to plug an SBP-2 device in and out, then shut 
> the machine down (e.g. shutdown -h now, whereas shutdown -r now does not 
> seem to trigger the bug).
...
> I am now gradually removing debug options from the kernel to see which 
> debug facility is making the fuzz...

It is CONFIG_PROVE_LOCKING (Lock debugging: prove locking correctness).

At the moment when the machine powers itself off.

What a waste of time.
-- 
Stefan Richter
-=====-==--- --== --==-
http://arcgraph.de/sr/

Re: [PATCH] firewire: reread config ROM when device reset the bus

[Jarod Wilson]

On Sunday 02 March 2008 07:48:30 pm Stefan Richter wrote:
> When a device changes its configuration ROM, it announces this with a
> bus reset.  firewire-core has to check which node initiated a bus reset
> and whether any unit directories went away or were added on this node.
>
> Tested with an IOI FWB-IDE01AB which has its link-on bit set if bus
> power is available but does not respond to ROM read requests if self
> power is off.  This implements
>   - recognition of the units if self power is switched on after fw-core
>     gave up the initial attempt to read the config ROM,
>   - shutdown of the units when self power is switched off.
>
> Also tested with a second PC running Linux/ieee1394.  When the eth1394
> driver is inserted and removed on that node, fw-core now notices the
> addition and removal of the IPv4 unit on the ieee1394 node.
>
> Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
> ---
>
> Applies after "firewire: replace static ROM cache by allocated cache".

I've also tested and verified proper disk suspend (and resume) functionality 
with a FW800 Western Digital My Book Pro and a FW400 Western Digital My Book, 
both of which were previously unable to power down their disks.

Signed-off-by: Jarod Wilson <jwilson@redhat.com>


-- 
Jarod Wilson
jwilson@redhat.com