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.025 / U:153.020 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site