Re: [PATCH 2/7] ieee1394 : use class iteration api

["Dave Young" <hidave.darkstar@gmail.com>]

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/
>