Messages in this thread | | | Date | Mon, 14 Jan 2008 09:57:33 +0800 | From | "Dave Young" <> | Subject | Re: [PATCH 2/7] ieee1394 : use class iteration api |
| |
On Jan 12, 2008 7:10 PM, Stefan Richter <stefanr@s5r6.in-berlin.de> wrote: > Dave Young wrote: > > +++ linux.new/drivers/ieee1394/nodemgr.c 2008-01-12 15:20:27.000000000 +0800 > ... > > > static void nodemgr_remove_uds(struct node_entry *ne) > > { > > struct device *dev; > > - struct unit_directory *tmp, *ud; > > + struct unit_directory *ud; > > > > - /* Iteration over nodemgr_ud_class.devices has to be protected by > > - * nodemgr_ud_class.sem, but device_unregister() will eventually > > - * take nodemgr_ud_class.sem too. Therefore pick out one ud at a time, > > - * release the semaphore, and then unregister the ud. Since this code > > - * may be called from other contexts besides the knodemgrds, protect the > > - * gap after release of the semaphore by nodemgr_serialize_remove_uds. > > + /* Use class_find device to iterate the devices. Since this code > > + * may be called from other contexts besides the knodemgrds, > > + * protect it by nodemgr_serialize_remove_uds. > > */ > > mutex_lock(&nodemgr_serialize_remove_uds); > > - for (;;) { > > - ud = NULL; > > - down(&nodemgr_ud_class.sem); > > - list_for_each_entry(dev, &nodemgr_ud_class.devices, node) { > > - tmp = container_of(dev, struct unit_directory, > > - unit_dev); > > - if (tmp->ne == ne) { > > - ud = tmp; > > - break; > > - } > > - } > > - up(&nodemgr_ud_class.sem); > > - if (ud == NULL) > > - break; > > - device_unregister(&ud->unit_dev); > > - device_unregister(&ud->device); > > + dev = class_find_device(&nodemgr_ud_class, ne, __match_ne); > > + if (!dev) { > > + mutex_unlock(&nodemgr_serialize_remove_uds); > > + return; > > } > > + ud = container_of(dev, struct unit_directory, unit_dev); > > + device_unregister(&ud->unit_dev); > > + device_unregister(&ud->device); > > mutex_unlock(&nodemgr_serialize_remove_uds); > > } > > A quick response on this change, without having checked the rest yet: > > This doesn't work. Each "ne" may have zero or more "ud". The purpose > of nodemgr_remove_uds is to kill all of the uds of one ne. After your > change, only the first ud of a ne would be gone. > > You need to keep the loop which takes care that all of the uds of the ne > are removed
My wrong, will fix.
> > Furthermore, I usually try to use "goto" or "break" constructs with > single unlock + return path instead of multiple unlock + return paths. > However, if these unlock + return paths are as visually close together > as they are here, it doesn't really matter (to me) which of the styles > is used.
I will update as your style in this patch, thanks for review.
> > BTW, you don't need to CC <krh@redhat.com> on drivers/ieee1394/ patches > (CONFIG_IEEE1394). He only looks after drivers/firewire/ > (CONFIG_FIREWIRE). I know, these are details, and everybody confuses > them. :-) I should try to clarify this in MAINTAINERS.
Ok.
> -- > Stefan Richter > -=====-==--- ---= -==-- > http://arcgraph.de/sr/ >
| |