lkml.org 
[lkml]   [2008]   [Jan]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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/
>


\
 
 \ /
  Last update: 2008-01-14 03:01    [W:0.120 / U:0.588 seconds]
©2003-2014 Jasper Spaans. Advertise on this site