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

["Kristian Høgsberg" <krh@bitplanet.net>]

[-- 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¥