| Message ID | 20240815-const_dfc_prepare-v2-3-8316b87b8ff9@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | driver core: Prevent device_find_child() from modifying caller's match data | expand |
Hi, On Thu, Aug 15, 2024 at 10:58:04PM +0800, Zijun Hu wrote: > From: Zijun Hu <quic_zijuhu@quicinc.com> > > To prepare for constifying the following old driver core API: > > struct device *device_find_child(struct device *dev, void *data, > int (*match)(struct device *dev, void *data)); > to new: > struct device *device_find_child(struct device *dev, const void *data, > int (*match)(struct device *dev, const void *data)); > > The new API does not allow its match function (*match)() to modify > caller's match data @*data, but lookup_existing_device() as the old > API's match function indeed modifies relevant match data, so it is not > suitable for the new API any more, fixed by implementing a equivalent > fw_device_find_child() instead of the old API usage. > > Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com> > --- > drivers/firewire/core-device.c | 37 +++++++++++++++++++++++++++++++++++-- > 1 file changed, 35 insertions(+), 2 deletions(-) Thanks for the patch. > Why to constify the API ? > > (1) It normally does not make sense, also does not need to, for > such device finding operation to modify caller's match data which > is mainly used for comparison. > > (2) It will make the API's match function and match data parameter > have the same type as all other APIs (bus|class|driver)_find_device(). > > (3) It will give driver author hints about choice between this API and > the following one: > int device_for_each_child(struct device *dev, void *data, > int (*fn)(struct device *dev, void *data)); I have found another issue in respect to this subsystem. The whole procedure in 'lookup_existing_device()' in the call of 'device_find_child()' is a bit superfluous, since it includes not only finding but also updating. The helper function passed to 'device_find_child()' should do quick finding only. I think we can change the relevant codes like the following patch. It would solve your concern, too. If you prefer the change, I'm going to evaluate it. ======== 8< -------- From ceaa8a986ae07865eb3fec810de330e96b6d56e2 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto <o-takashi@sakamocchi.jp> Date: Sat, 17 Aug 2024 17:52:53 +0900 Subject: [PATCH] firewire: core: update fw_device outside of device_find_child() When detecting updates of bus topology, the data of fw_device is newly allocated and caches the content of configuration ROM from the corresponding node. Then, the tree of device is sought to find the previous data of fw_device corresponding to the node, since in IEEE 1394 specification numeric node identifier could be changed dynamically every generation of bus topology. If it is found, the previous data is updated and reused, then the newly allocated data is going to be released. The above procedure is done in the call of device_find_child(), however it is a bit abusing against the intention of the helper function, since the call would not only find but also update. This commit splits the update outside of the call. --- drivers/firewire/core-device.c | 109 ++++++++++++++++----------------- 1 file changed, 54 insertions(+), 55 deletions(-) diff --git a/drivers/firewire/core-device.c b/drivers/firewire/core-device.c index bc4c9e5a..62e8d839 100644 --- a/drivers/firewire/core-device.c +++ b/drivers/firewire/core-device.c @@ -928,56 +928,6 @@ static void fw_device_update(struct work_struct *work) device_for_each_child(&device->device, NULL, update_unit); } -/* - * If a device was pending for deletion because its node went away but its - * bus info block and root directory header matches that of a newly discovered - * device, revive the existing fw_device. - * The newly allocated fw_device becomes obsolete instead. - */ -static int lookup_existing_device(struct device *dev, void *data) -{ - struct fw_device *old = fw_device(dev); - struct fw_device *new = data; - struct fw_card *card = new->card; - int match = 0; - - if (!is_fw_device(dev)) - return 0; - - guard(rwsem_read)(&fw_device_rwsem); // serialize config_rom access - guard(spinlock_irq)(&card->lock); // serialize node access - - if (memcmp(old->config_rom, new->config_rom, 6 * 4) == 0 && - atomic_cmpxchg(&old->state, - FW_DEVICE_GONE, - FW_DEVICE_RUNNING) == FW_DEVICE_GONE) { - struct fw_node *current_node = new->node; - struct fw_node *obsolete_node = old->node; - - new->node = obsolete_node; - new->node->data = new; - old->node = current_node; - old->node->data = old; - - old->max_speed = new->max_speed; - old->node_id = current_node->node_id; - smp_wmb(); /* update node_id before generation */ - old->generation = card->generation; - old->config_rom_retries = 0; - fw_notice(card, "rediscovered device %s\n", dev_name(dev)); - - old->workfn = fw_device_update; - fw_schedule_device_work(old, 0); - - if (current_node == card->root_node) - fw_schedule_bm_work(card, 0); - - match = 1; - } - - return match; -} - enum { BC_UNKNOWN = 0, BC_UNIMPLEMENTED, BC_IMPLEMENTED, }; static void set_broadcast_channel(struct fw_device *device, int generation) @@ -1038,6 +988,17 @@ int fw_device_set_broadcast_channel(struct device *dev, void *gen) return 0; } +static int compare_configuration_rom(struct device *dev, void *data) +{ + const struct fw_device *old = fw_device(dev); + const u32 *config_rom = data; + + if (!is_fw_device(dev)) + return 0; + + return !!memcmp(old->config_rom, config_rom, 6 * 4); +} + static void fw_device_init(struct work_struct *work) { struct fw_device *device = @@ -1071,13 +1032,51 @@ static void fw_device_init(struct work_struct *work) return; } - revived_dev = device_find_child(card->device, - device, lookup_existing_device); + // If a device was pending for deletion because its node went away but its bus info block + // and root directory header matches that of a newly discovered device, revive the + // existing fw_device. The newly allocated fw_device becomes obsolete instead. + // + // serialize config_rom access. + scoped_guard(rwsem_read, &fw_device_rwsem) { + // TODO: The cast to 'void *' could be removed if Zijun Hu's work goes well. + revived_dev = device_find_child(card->device, (void *)device->config_rom, + compare_configuration_rom); + } if (revived_dev) { - put_device(revived_dev); - fw_device_release(&device->device); + struct fw_device *found = fw_device(revived_dev); - return; + // serialize node access + guard(spinlock_irq)(&card->lock); + + if (atomic_cmpxchg(&found->state, + FW_DEVICE_GONE, + FW_DEVICE_RUNNING) == FW_DEVICE_GONE) { + struct fw_node *current_node = device->node; + struct fw_node *obsolete_node = found->node; + + device->node = obsolete_node; + device->node->data = device; + found->node = current_node; + found->node->data = found; + + found->max_speed = device->max_speed; + found->node_id = current_node->node_id; + smp_wmb(); /* update node_id before generation */ + found->generation = card->generation; + found->config_rom_retries = 0; + fw_notice(card, "rediscovered device %s\n", dev_name(revived_dev)); + + found->workfn = fw_device_update; + fw_schedule_device_work(found, 0); + + if (current_node == card->root_node) + fw_schedule_bm_work(card, 0); + + put_device(revived_dev); + fw_device_release(&device->device); + + return; + } } device_initialize(&device->device);
On 2024/8/17 17:57, Takashi Sakamoto wrote: > Hi, > > On Thu, Aug 15, 2024 at 10:58:04PM +0800, Zijun Hu wrote: >> From: Zijun Hu <quic_zijuhu@quicinc.com> >> >> To prepare for constifying the following old driver core API: >> >> struct device *device_find_child(struct device *dev, void *data, >> int (*match)(struct device *dev, void *data)); >> to new: >> struct device *device_find_child(struct device *dev, const void *data, >> int (*match)(struct device *dev, const void *data)); >> >> The new API does not allow its match function (*match)() to modify >> caller's match data @*data, but lookup_existing_device() as the old >> API's match function indeed modifies relevant match data, so it is not >> suitable for the new API any more, fixed by implementing a equivalent >> fw_device_find_child() instead of the old API usage. >> >> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com> >> --- >> drivers/firewire/core-device.c | 37 +++++++++++++++++++++++++++++++++++-- >> 1 file changed, 35 insertions(+), 2 deletions(-) > > Thanks for the patch. > >> Why to constify the API ? >> >> (1) It normally does not make sense, also does not need to, for >> such device finding operation to modify caller's match data which >> is mainly used for comparison. >> >> (2) It will make the API's match function and match data parameter >> have the same type as all other APIs (bus|class|driver)_find_device(). >> >> (3) It will give driver author hints about choice between this API and >> the following one: >> int device_for_each_child(struct device *dev, void *data, >> int (*fn)(struct device *dev, void *data)); > > I have found another issue in respect to this subsystem. > > The whole procedure in 'lookup_existing_device()' in the call of > 'device_find_child()' is a bit superfluous, since it includes not only > finding but also updating. The helper function passed to > 'device_find_child()' should do quick finding only. > > I think we can change the relevant codes like the following patch. It > would solve your concern, too. If you prefer the change, I'm going to > evaluate it. > thanks for your reply. of course, i prefer your change. > ======== 8< -------- > > From ceaa8a986ae07865eb3fec810de330e96b6d56e2 Mon Sep 17 00:00:00 2001 > From: Takashi Sakamoto <o-takashi@sakamocchi.jp> > Date: Sat, 17 Aug 2024 17:52:53 +0900 > Subject: [PATCH] firewire: core: update fw_device outside of > device_find_child() > > When detecting updates of bus topology, the data of fw_device is newly > allocated and caches the content of configuration ROM from the > corresponding node. Then, the tree of device is sought to find the > previous data of fw_device corresponding to the node, since in IEEE 1394 > specification numeric node identifier could be changed dynamically every > generation of bus topology. If it is found, the previous data is updated > and reused, then the newly allocated data is going to be released. > > The above procedure is done in the call of device_find_child(), however it > is a bit abusing against the intention of the helper function, since the > call would not only find but also update. > > This commit splits the update outside of the call. > --- > drivers/firewire/core-device.c | 109 ++++++++++++++++----------------- > 1 file changed, 54 insertions(+), 55 deletions(-) > > diff --git a/drivers/firewire/core-device.c b/drivers/firewire/core-device.c > index bc4c9e5a..62e8d839 100644 > --- a/drivers/firewire/core-device.c > +++ b/drivers/firewire/core-device.c > @@ -928,56 +928,6 @@ static void fw_device_update(struct work_struct *work) > device_for_each_child(&device->device, NULL, update_unit); > } > > -/* > - * If a device was pending for deletion because its node went away but its > - * bus info block and root directory header matches that of a newly discovered > - * device, revive the existing fw_device. > - * The newly allocated fw_device becomes obsolete instead. > - */ > -static int lookup_existing_device(struct device *dev, void *data) > -{ > - struct fw_device *old = fw_device(dev); > - struct fw_device *new = data; > - struct fw_card *card = new->card; > - int match = 0; > - > - if (!is_fw_device(dev)) > - return 0; > - > - guard(rwsem_read)(&fw_device_rwsem); // serialize config_rom access > - guard(spinlock_irq)(&card->lock); // serialize node access > - > - if (memcmp(old->config_rom, new->config_rom, 6 * 4) == 0 && > - atomic_cmpxchg(&old->state, > - FW_DEVICE_GONE, > - FW_DEVICE_RUNNING) == FW_DEVICE_GONE) { > - struct fw_node *current_node = new->node; > - struct fw_node *obsolete_node = old->node; > - > - new->node = obsolete_node; > - new->node->data = new; > - old->node = current_node; > - old->node->data = old; > - > - old->max_speed = new->max_speed; > - old->node_id = current_node->node_id; > - smp_wmb(); /* update node_id before generation */ > - old->generation = card->generation; > - old->config_rom_retries = 0; > - fw_notice(card, "rediscovered device %s\n", dev_name(dev)); > - > - old->workfn = fw_device_update; > - fw_schedule_device_work(old, 0); > - > - if (current_node == card->root_node) > - fw_schedule_bm_work(card, 0); > - > - match = 1; > - } > - > - return match; > -} > - > enum { BC_UNKNOWN = 0, BC_UNIMPLEMENTED, BC_IMPLEMENTED, }; > > static void set_broadcast_channel(struct fw_device *device, int generation) > @@ -1038,6 +988,17 @@ int fw_device_set_broadcast_channel(struct device *dev, void *gen) > return 0; > } > > +static int compare_configuration_rom(struct device *dev, void *data) > +{ > + const struct fw_device *old = fw_device(dev); > + const u32 *config_rom = data; > + > + if (!is_fw_device(dev)) > + return 0; > + > + return !!memcmp(old->config_rom, config_rom, 6 * 4); !memcmp(old->config_rom, config_rom, 6 * 4) ? is this extra condition old->state == FW_DEVICE_GONE required ? namely, is it okay for below return ? return !memcmp(old->config_rom, config_rom, 6 * 4) && old->state == FW_DEVICE_GONE > +} > + > static void fw_device_init(struct work_struct *work) > { > struct fw_device *device = > @@ -1071,13 +1032,51 @@ static void fw_device_init(struct work_struct *work) > return; > } > > - revived_dev = device_find_child(card->device, > - device, lookup_existing_device); > + // If a device was pending for deletion because its node went away but its bus info block > + // and root directory header matches that of a newly discovered device, revive the > + // existing fw_device. The newly allocated fw_device becomes obsolete instead. > + // > + // serialize config_rom access. > + scoped_guard(rwsem_read, &fw_device_rwsem) { > + // TODO: The cast to 'void *' could be removed if Zijun Hu's work goes well. may remove this TODO line since i will simply remove the cast with the other patch series as shown below: https://lore.kernel.org/all/20240811-const_dfc_done-v1-0-9d85e3f943cb@quicinc.com/ > + revived_dev = device_find_child(card->device, (void *)device->config_rom, > + compare_configuration_rom); > + } > if (revived_dev) { > - put_device(revived_dev); > - fw_device_release(&device->device); > + struct fw_device *found = fw_device(revived_dev); > > - return; > + // serialize node access > + guard(spinlock_irq)(&card->lock); > + > + if (atomic_cmpxchg(&found->state, > + FW_DEVICE_GONE, > + FW_DEVICE_RUNNING) == FW_DEVICE_GONE) { > + struct fw_node *current_node = device->node; > + struct fw_node *obsolete_node = found->node; > + > + device->node = obsolete_node; > + device->node->data = device; > + found->node = current_node; > + found->node->data = found; > + > + found->max_speed = device->max_speed; > + found->node_id = current_node->node_id; > + smp_wmb(); /* update node_id before generation */ > + found->generation = card->generation; > + found->config_rom_retries = 0; > + fw_notice(card, "rediscovered device %s\n", dev_name(revived_dev)); > + > + found->workfn = fw_device_update; > + fw_schedule_device_work(found, 0); > + > + if (current_node == card->root_node) > + fw_schedule_bm_work(card, 0); > + > + put_device(revived_dev); > + fw_device_release(&device->device); > + > + return; > + } is it okay to put_device() here as well ? put_device(revived_dev); > } > > device_initialize(&device->device);
Hi, On 2024/8/18 22:34, Zijun Hu wrote: >On 2024/8/17 17:57, Takashi Sakamoto wrote: >> ======== 8< -------- >> >> From ceaa8a986ae07865eb3fec810de330e96b6d56e2 Mon Sep 17 00:00:00 2001 >> From: Takashi Sakamoto <o-takashi@sakamocchi.jp> >> Date: Sat, 17 Aug 2024 17:52:53 +0900 >> Subject: [PATCH] firewire: core: update fw_device outside of >> device_find_child() >> >> When detecting updates of bus topology, the data of fw_device is newly >> allocated and caches the content of configuration ROM from the >> corresponding node. Then, the tree of device is sought to find the >> previous data of fw_device corresponding to the node, since in IEEE 1394 >> specification numeric node identifier could be changed dynamically every >> generation of bus topology. If it is found, the previous data is updated >> and reused, then the newly allocated data is going to be released. >> >> The above procedure is done in the call of device_find_child(), however it >> is a bit abusing against the intention of the helper function, since the >> call would not only find but also update. >> >> This commit splits the update outside of the call. >> --- >> drivers/firewire/core-device.c | 109 ++++++++++++++++----------------- >> 1 file changed, 54 insertions(+), 55 deletions(-) >> >> diff --git a/drivers/firewire/core-device.c b/drivers/firewire/core-device.c >> index bc4c9e5a..62e8d839 100644 >> --- a/drivers/firewire/core-device.c >> +++ b/drivers/firewire/core-device.c >> ... >> @@ -1038,6 +988,17 @@ int fw_device_set_broadcast_channel(struct device *dev, void *gen) >> return 0; >> } >> >> +static int compare_configuration_rom(struct device *dev, void *data) >> +{ >> + const struct fw_device *old = fw_device(dev); >> + const u32 *config_rom = data; >> + >> + if (!is_fw_device(dev)) >> + return 0; >> + >> + return !!memcmp(old->config_rom, config_rom, 6 * 4); > >!memcmp(old->config_rom, config_rom, 6 * 4) ? Indeed. >is this extra condition old->state == FW_DEVICE_GONE required ? > >namely, is it okay for below return ? >return !memcmp(old->config_rom, config_rom, 6 * 4) && old->state == >FW_DEVICE_GONE If so, atomic_read() should be used, however I avoid it since the access to state member happens twice in in the path to reuse the instance. >> +} >> + >> static void fw_device_init(struct work_struct *work) >> { >> struct fw_device *device = >> @@ -1071,13 +1032,51 @@ static void fw_device_init(struct work_struct *work) >> return; >> } >> >> - revived_dev = device_find_child(card->device, >> - device, lookup_existing_device); >> + // If a device was pending for deletion because its node went away but its bus info block >> + // and root directory header matches that of a newly discovered device, revive the >> + // existing fw_device. The newly allocated fw_device becomes obsolete instead. >> + // >> + // serialize config_rom access. >> + scoped_guard(rwsem_read, &fw_device_rwsem) { >> + // TODO: The cast to 'void *' could be removed if Zijun Hu's work goes well. > >may remove this TODO line since i will simply remove the cast with the >other patch series as shown below: >https://lore.kernel.org/all/20240811-const_dfc_done-v1-0-9d85e3f943cb@quicinc.com/ Of course, I won't apply this patch as is. It is just a mark to hold your attention. >> + revived_dev = device_find_child(card->device, (void *)device->config_rom, >> + compare_configuration_rom); >> + } >> if (revived_dev) { >> - put_device(revived_dev); >> - fw_device_release(&device->device); >> + struct fw_device *found = fw_device(revived_dev); >> >> - return; >> + // serialize node access >> + guard(spinlock_irq)(&card->lock); >> + >> + if (atomic_cmpxchg(&found->state, >> + FW_DEVICE_GONE, >> + FW_DEVICE_RUNNING) == FW_DEVICE_GONE) { >> + struct fw_node *current_node = device->node; >> + struct fw_node *obsolete_node = found->node; >> + >> + device->node = obsolete_node; >> + device->node->data = device; >> + found->node = current_node; >> + found->node->data = found; >> + >> + found->max_speed = device->max_speed; >> + found->node_id = current_node->node_id; >> + smp_wmb(); /* update node_id before generation */ >> + found->generation = card->generation; >> + found->config_rom_retries = 0; >> + fw_notice(card, "rediscovered device %s\n", dev_name(revived_dev)); >> + >> + found->workfn = fw_device_update; >> + fw_schedule_device_work(found, 0); >> + >> + if (current_node == card->root_node) >> + fw_schedule_bm_work(card, 0); >> + >> + put_device(revived_dev); >> + fw_device_release(&device->device); >> + >> + return; >> + } > >is it okay to put_device() here as well ? >put_device(revived_dev); Exactly. The call of put_device() should be done when the call of device_find_child() returns non-NULL value. Additionally, I realize that the call of fw_device_release() under acquiring card->lock causes dead lock. >> } >> >> device_initialize(&device->device); Anyway, I'll post take 2 and work for its evaluation. Thanks Takashi Sakamoto
On 2024/8/19 16:58, Takashi Sakamoto wrote: > > Hi, > > On 2024/8/18 22:34, Zijun Hu wrote: >> On 2024/8/17 17:57, Takashi Sakamoto wrote: >>> ======== 8< -------- >>> >>> From ceaa8a986ae07865eb3fec810de330e96b6d56e2 Mon Sep 17 00:00:00 2001 >>> From: Takashi Sakamoto <o-takashi@sakamocchi.jp> >>> Date: Sat, 17 Aug 2024 17:52:53 +0900 >>> Subject: [PATCH] firewire: core: update fw_device outside of >>> device_find_child() >>> >>> When detecting updates of bus topology, the data of fw_device is newly >>> allocated and caches the content of configuration ROM from the >>> corresponding node. Then, the tree of device is sought to find the >>> previous data of fw_device corresponding to the node, since in IEEE 1394 >>> specification numeric node identifier could be changed dynamically every >>> generation of bus topology. If it is found, the previous data is updated >>> and reused, then the newly allocated data is going to be released. >>> >>> The above procedure is done in the call of device_find_child(), however it >>> is a bit abusing against the intention of the helper function, since the >>> call would not only find but also update. >>> >>> This commit splits the update outside of the call. >>> --- >>> drivers/firewire/core-device.c | 109 ++++++++++++++++----------------- >>> 1 file changed, 54 insertions(+), 55 deletions(-) >>> >>> diff --git a/drivers/firewire/core-device.c b/drivers/firewire/core-device.c >>> index bc4c9e5a..62e8d839 100644 >>> --- a/drivers/firewire/core-device.c >>> +++ b/drivers/firewire/core-device.c >>> ... >>> @@ -1038,6 +988,17 @@ int fw_device_set_broadcast_channel(struct device *dev, void *gen) >>> return 0; >>> } >>> >>> +static int compare_configuration_rom(struct device *dev, void *data) >>> +{ >>> + const struct fw_device *old = fw_device(dev); >>> + const u32 *config_rom = data; >>> + >>> + if (!is_fw_device(dev)) >>> + return 0; >>> + >>> + return !!memcmp(old->config_rom, config_rom, 6 * 4); >> >> !memcmp(old->config_rom, config_rom, 6 * 4) ? > > Indeed. > >> is this extra condition old->state == FW_DEVICE_GONE required ? >> >> namely, is it okay for below return ? >> return !memcmp(old->config_rom, config_rom, 6 * 4) && old->state == >> FW_DEVICE_GONE > > If so, atomic_read() should be used, however I avoid it since the access > to state member happens twice in in the path to reuse the instance. > it sounds good to not append the extra condition. >>> +} >>> + >>> static void fw_device_init(struct work_struct *work) >>> { >>> struct fw_device *device = >>> @@ -1071,13 +1032,51 @@ static void fw_device_init(struct work_struct *work) >>> return; >>> } >>> >>> - revived_dev = device_find_child(card->device, >>> - device, lookup_existing_device); >>> + // If a device was pending for deletion because its node went away but its bus info block >>> + // and root directory header matches that of a newly discovered device, revive the >>> + // existing fw_device. The newly allocated fw_device becomes obsolete instead. >>> + // >>> + // serialize config_rom access. >>> + scoped_guard(rwsem_read, &fw_device_rwsem) { >>> + // TODO: The cast to 'void *' could be removed if Zijun Hu's work goes well. >> >> may remove this TODO line since i will simply remove the cast with the >> other patch series as shown below: >> https://lore.kernel.org/all/20240811-const_dfc_done-v1-0-9d85e3f943cb@quicinc.com/ > > Of course, I won't apply this patch as is. It is just a mark to hold > your attention. > >>> + revived_dev = device_find_child(card->device, (void *)device->config_rom, >>> + compare_configuration_rom); >>> + } >>> if (revived_dev) { >>> - put_device(revived_dev); >>> - fw_device_release(&device->device); >>> + struct fw_device *found = fw_device(revived_dev); >>> >>> - return; >>> + // serialize node access >>> + guard(spinlock_irq)(&card->lock); >>> + >>> + if (atomic_cmpxchg(&found->state, >>> + FW_DEVICE_GONE, >>> + FW_DEVICE_RUNNING) == FW_DEVICE_GONE) { >>> + struct fw_node *current_node = device->node; >>> + struct fw_node *obsolete_node = found->node; >>> + >>> + device->node = obsolete_node; >>> + device->node->data = device; >>> + found->node = current_node; >>> + found->node->data = found; >>> + >>> + found->max_speed = device->max_speed; >>> + found->node_id = current_node->node_id; >>> + smp_wmb(); /* update node_id before generation */ >>> + found->generation = card->generation; >>> + found->config_rom_retries = 0; >>> + fw_notice(card, "rediscovered device %s\n", dev_name(revived_dev)); >>> + >>> + found->workfn = fw_device_update; >>> + fw_schedule_device_work(found, 0); >>> + >>> + if (current_node == card->root_node) >>> + fw_schedule_bm_work(card, 0); >>> + >>> + put_device(revived_dev); >>> + fw_device_release(&device->device); >>> + >>> + return; >>> + } >> >> is it okay to put_device() here as well ? >> put_device(revived_dev); > > Exactly. The call of put_device() should be done when the call of > device_find_child() returns non-NULL value. > > Additionally, I realize that the call of fw_device_release() under > acquiring card->lock causes dead lock. > >>> } >>> >>> device_initialize(&device->device); > > Anyway, I'll post take 2 and work for its evaluation. > great > > Thanks > > Takashi Sakamoto
diff --git a/drivers/firewire/core-device.c b/drivers/firewire/core-device.c index 00e9a13e6c45..7fbccb113d54 100644 --- a/drivers/firewire/core-device.c +++ b/drivers/firewire/core-device.c @@ -33,6 +33,39 @@ #define ROOT_DIR_OFFSET 5 +struct fw_dfc_data { + int (*match)(struct device *dev, void *data); + void *data; + struct device *target_device; +}; + +static int fw_dfc_match_modify(struct device *dev, void *data) +{ + struct fw_dfc_data *dfc_data = data; + int res; + + res = dfc_data->match(dev, dfc_data->data); + if (res && get_device(dev)) { + dfc_data->target_device = dev; + return res; + } + + return 0; +} + +/* + * I have the same function as device_find_child() but allow to modify + * caller's match data @*data. + */ +static struct device *fw_device_find_child(struct device *parent, void *data, + int (*match)(struct device *dev, void *data)) +{ + struct fw_dfc_data dfc_data = {match, data, NULL}; + + device_for_each_child(parent, &dfc_data, fw_dfc_match_modify); + return dfc_data.target_device; +} + void fw_csr_iterator_init(struct fw_csr_iterator *ci, const u32 *p) { ci->p = p + 1; @@ -1087,8 +1120,8 @@ static void fw_device_init(struct work_struct *work) return; } - revived_dev = device_find_child(card->device, - device, lookup_existing_device); + revived_dev = fw_device_find_child(card->device, device, + lookup_existing_device); if (revived_dev) { put_device(revived_dev); fw_device_release(&device->device);