On Jan 12, 2008 7:10 PM, Stefan Richter <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]> 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/
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
