2008-01-12 09:46:58

by Dave Young

[permalink] [raw]
Subject: [PATCH 2/7] ieee1394 : use class iteration api

Convert to use the class iteration api.

Signed-off-by: Dave Young <[email protected]>

---
drivers/ieee1394/nodemgr.c | 319 +++++++++++++++++++++++++--------------------
1 file changed, 178 insertions(+), 141 deletions(-)

diff -upr linux/drivers/ieee1394/nodemgr.c linux.new/drivers/ieee1394/nodemgr.c
--- linux/drivers/ieee1394/nodemgr.c 2008-01-12 15:20:27.000000000 +0800
+++ linux.new/drivers/ieee1394/nodemgr.c 2008-01-12 15:20:27.000000000 +0800
@@ -727,36 +727,35 @@ static int nodemgr_bus_match(struct devi

static DEFINE_MUTEX(nodemgr_serialize_remove_uds);

+static int __match_ne(struct device *dev, void *data)
+{
+ struct unit_directory *ud;
+ struct node_entry *ne = (struct node_entry *)data;
+
+ ud = container_of(dev, struct unit_directory, unit_dev);
+ if (ud->ne == ne)
+ return 1;
+ return 0;
+}
+
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);
}

@@ -882,45 +881,64 @@ fail_alloc:
return NULL;
}

+static int __match_ne_guid(struct device *dev, void *data)
+{
+ struct node_entry *ne;
+ u64 *guid = (u64 *)data;
+
+ ne = container_of(dev, struct node_entry, node_dev);
+ if (ne->guid == *guid)
+ return 1;
+ return 0;
+}

static struct node_entry *find_entry_by_guid(u64 guid)
{
struct device *dev;
- struct node_entry *ne, *ret_ne = NULL;
-
- down(&nodemgr_ne_class.sem);
- list_for_each_entry(dev, &nodemgr_ne_class.devices, node) {
- ne = container_of(dev, struct node_entry, node_dev);
+ struct node_entry *ne;

- if (ne->guid == guid) {
- ret_ne = ne;
- break;
- }
- }
- up(&nodemgr_ne_class.sem);
+ dev = class_find_device(&nodemgr_ne_class, &guid, __match_ne_guid);
+ if (!dev)
+ return NULL;
+ ne = container_of(dev, struct node_entry, node_dev);

- return ret_ne;
+ return ne;
}

+struct match_nodeid_param {
+ struct hpsb_host *host;
+ nodeid_t nodeid;
+};
+
+static int __match_ne_nodeid(struct device *dev, void *data)
+{
+ struct node_entry *ne;
+ struct match_nodeid_param *param = (struct match_nodeid_param *)data;
+
+ if (!dev)
+ return 0;
+ ne = container_of(dev, struct node_entry, node_dev);
+ if (ne->host == param->host && ne->nodeid == param->nodeid)
+ return 1;
+ return 0;
+}

static struct node_entry *find_entry_by_nodeid(struct hpsb_host *host,
nodeid_t nodeid)
{
struct device *dev;
- struct node_entry *ne, *ret_ne = NULL;
+ struct node_entry *ne;
+ struct match_nodeid_param param;

- down(&nodemgr_ne_class.sem);
- list_for_each_entry(dev, &nodemgr_ne_class.devices, node) {
- ne = container_of(dev, struct node_entry, node_dev);
+ param.host = host;
+ param.nodeid = nodeid;

- if (ne->host == host && ne->nodeid == nodeid) {
- ret_ne = ne;
- break;
- }
- }
- up(&nodemgr_ne_class.sem);
+ dev = class_find_device(&nodemgr_ne_class, &param, __match_ne_nodeid);
+ if (!dev)
+ return NULL;
+ ne = container_of(dev, struct node_entry, node_dev);

- return ret_ne;
+ return ne;
}


@@ -1370,107 +1388,109 @@ static void nodemgr_node_scan(struct hos
}
}

-
-static void nodemgr_suspend_ne(struct node_entry *ne)
+static int __nodemgr_driver_suspend(struct device *dev, void *data)
{
- struct device *dev;
struct unit_directory *ud;
struct device_driver *drv;
+ struct node_entry *ne = (struct node_entry *)data;
int error;

- HPSB_DEBUG("Node suspended: ID:BUS[" NODE_BUS_FMT "] GUID[%016Lx]",
- NODE_BUS_ARGS(ne->host, ne->nodeid), (unsigned long long)ne->guid);
+ ud = container_of(dev, struct unit_directory, unit_dev);
+ if (ud->ne == ne) {
+ drv = get_driver(ud->device.driver);
+ if (drv) {
+ error = 1; /* release if suspend is not implemented */
+ if (drv->suspend) {
+ down(&ud->device.sem);
+ error = drv->suspend(&ud->device, PMSG_SUSPEND);
+ up(&ud->device.sem);
+ }
+ if (error)
+ device_release_driver(&ud->device);
+ put_driver(drv);
+ }
+ }

- ne->in_limbo = 1;
- WARN_ON(device_create_file(&ne->device, &dev_attr_ne_in_limbo));
+ return 0;
+}

- down(&nodemgr_ud_class.sem);
- list_for_each_entry(dev, &nodemgr_ud_class.devices, node) {
- ud = container_of(dev, struct unit_directory, unit_dev);
- if (ud->ne != ne)
- continue;
+static int __nodemgr_driver_resume(struct device *dev, void *data)
+{
+ struct unit_directory *ud;
+ struct device_driver *drv;
+ struct node_entry *ne = (struct node_entry *)data;

+ ud = container_of(dev, struct unit_directory, unit_dev);
+ if (ud->ne == ne) {
drv = get_driver(ud->device.driver);
- if (!drv)
- continue;
-
- error = 1; /* release if suspend is not implemented */
- if (drv->suspend) {
- down(&ud->device.sem);
- error = drv->suspend(&ud->device, PMSG_SUSPEND);
- up(&ud->device.sem);
+ if (drv) {
+ if (drv->resume) {
+ down(&ud->device.sem);
+ drv->resume(&ud->device);
+ up(&ud->device.sem);
+ }
+ put_driver(drv);
}
- if (error)
- device_release_driver(&ud->device);
- put_driver(drv);
}
- up(&nodemgr_ud_class.sem);
-}

+ return 0;
+}

-static void nodemgr_resume_ne(struct node_entry *ne)
+static void nodemgr_suspend_ne(struct node_entry *ne)
{
- struct device *dev;
- struct unit_directory *ud;
- struct device_driver *drv;
+ HPSB_DEBUG("Node suspended: ID:BUS[" NODE_BUS_FMT "] GUID[%016Lx]",
+ NODE_BUS_ARGS(ne->host, ne->nodeid),
+ (unsigned long long)ne->guid);

- ne->in_limbo = 0;
- device_remove_file(&ne->device, &dev_attr_ne_in_limbo);
+ ne->in_limbo = 1;
+ WARN_ON(device_create_file(&ne->device, &dev_attr_ne_in_limbo));

- down(&nodemgr_ud_class.sem);
- list_for_each_entry(dev, &nodemgr_ud_class.devices, node) {
- ud = container_of(dev, struct unit_directory, unit_dev);
- if (ud->ne != ne)
- continue;
+ class_for_each_device(&nodemgr_ud_class, ne, __nodemgr_driver_suspend);
+}

- drv = get_driver(ud->device.driver);
- if (!drv)
- continue;

- if (drv->resume) {
- down(&ud->device.sem);
- drv->resume(&ud->device);
- up(&ud->device.sem);
- }
- put_driver(drv);
- }
- up(&nodemgr_ud_class.sem);
+static void nodemgr_resume_ne(struct node_entry *ne)
+{
+ ne->in_limbo = 0;
+ device_remove_file(&ne->device, &dev_attr_ne_in_limbo);

+ class_for_each_device(&nodemgr_ud_class, ne, __nodemgr_driver_resume);
HPSB_DEBUG("Node resumed: ID:BUS[" NODE_BUS_FMT "] GUID[%016Lx]",
NODE_BUS_ARGS(ne->host, ne->nodeid), (unsigned long long)ne->guid);
}

-
-static void nodemgr_update_pdrv(struct node_entry *ne)
+static int __nodemgr_update_pdrv(struct device *dev, void *data)
{
- struct device *dev;
struct unit_directory *ud;
struct device_driver *drv;
struct hpsb_protocol_driver *pdrv;
+ struct node_entry *ne = (struct node_entry *)data;
int error;

- down(&nodemgr_ud_class.sem);
- list_for_each_entry(dev, &nodemgr_ud_class.devices, node) {
- ud = container_of(dev, struct unit_directory, unit_dev);
- if (ud->ne != ne)
- continue;
-
+ ud = container_of(dev, struct unit_directory, unit_dev);
+ if (ud->ne == ne) {
drv = get_driver(ud->device.driver);
- if (!drv)
- continue;
-
- error = 0;
- pdrv = container_of(drv, struct hpsb_protocol_driver, driver);
- if (pdrv->update) {
- down(&ud->device.sem);
- error = pdrv->update(ud);
- up(&ud->device.sem);
+ if (drv) {
+ error = 0;
+ pdrv = container_of(drv, struct hpsb_protocol_driver,
+ driver);
+ if (pdrv->update) {
+ down(&ud->device.sem);
+ error = pdrv->update(ud);
+ up(&ud->device.sem);
+ }
+ if (error)
+ device_release_driver(&ud->device);
+ put_driver(drv);
}
- if (error)
- device_release_driver(&ud->device);
- put_driver(drv);
}
- up(&nodemgr_ud_class.sem);
+
+ return 0;
+}
+
+static void nodemgr_update_pdrv(struct node_entry *ne)
+{
+ class_for_each_device(&nodemgr_ud_class, ne, __nodemgr_update_pdrv);
}


@@ -1529,13 +1549,31 @@ static void nodemgr_probe_ne(struct host
put_device(dev);
}

+struct probe_param {
+ struct host_info *hi;
+ int generation;
+};
+
+static int __nodemgr_node_probe(struct device *dev, void *data)
+{
+ struct probe_param *param = (struct probe_param *)data;
+ struct node_entry *ne;
+
+ ne = container_of(dev, struct node_entry, node_dev);
+ if (!ne->needs_probe)
+ nodemgr_probe_ne(param->hi, ne, param->generation);
+ if (ne->needs_probe)
+ nodemgr_probe_ne(param->hi, ne, param->generation);
+ return 0;
+}

static void nodemgr_node_probe(struct host_info *hi, int generation)
{
struct hpsb_host *host = hi->host;
- struct device *dev;
- struct node_entry *ne;
+ struct probe_param param;

+ param.hi = hi;
+ param.generation = generation;
/* Do some processing of the nodes we've probed. This pulls them
* into the sysfs layer if needed, and can result in processing of
* unit-directories, or just updating the node and it's
@@ -1545,19 +1583,7 @@ static void nodemgr_node_probe(struct ho
* while probes are time-consuming. (Well, those probes need some
* improvement...) */

- down(&nodemgr_ne_class.sem);
- list_for_each_entry(dev, &nodemgr_ne_class.devices, node) {
- ne = container_of(dev, struct node_entry, node_dev);
- if (!ne->needs_probe)
- nodemgr_probe_ne(hi, ne, generation);
- }
- list_for_each_entry(dev, &nodemgr_ne_class.devices, node) {
- ne = container_of(dev, struct node_entry, node_dev);
- if (ne->needs_probe)
- nodemgr_probe_ne(hi, ne, generation);
- }
- up(&nodemgr_ne_class.sem);
-
+ class_for_each_device(&nodemgr_ne_class, &param, __nodemgr_node_probe);

/* If we had a bus reset while we were scanning the bus, it is
* possible that we did not probe all nodes. In that case, we
@@ -1757,6 +1783,22 @@ exit:
return 0;
}

+struct host_iter_param {
+ void *data;
+ int (*cb)(struct hpsb_host *, void *);
+};
+
+static int __nodemgr_for_each_host(struct device *dev, void *data)
+{
+ struct hpsb_host *host;
+ struct host_iter_param *hip = (struct host_iter_param *)data;
+ int error = 0;
+
+ host = container_of(dev, struct hpsb_host, host_dev);
+ error = hip->cb(host, hip->data);
+
+ return error;
+}
/**
* nodemgr_for_each_host - call a function for each IEEE 1394 host
* @data: an address to supply to the callback
@@ -1771,18 +1813,13 @@ exit:
*/
int nodemgr_for_each_host(void *data, int (*cb)(struct hpsb_host *, void *))
{
- struct device *dev;
- struct hpsb_host *host;
- int error = 0;
-
- down(&hpsb_host_class.sem);
- list_for_each_entry(dev, &hpsb_host_class.devices, node) {
- host = container_of(dev, struct hpsb_host, host_dev);
+ struct host_iter_param hip;
+ int error;

- if ((error = cb(host, data)))
- break;
- }
- up(&hpsb_host_class.sem);
+ hip.cb = cb;
+ hip.data = data;
+ error = class_for_each_device(&hpsb_host_class, &hip,
+ __nodemgr_for_each_host);

return error;
}


2008-01-12 11:11:21

by Stefan Richter

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

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.

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.

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.
--
Stefan Richter
-=====-==--- ---= -==--
http://arcgraph.de/sr/

2008-01-12 11:14:05

by Stefan Richter

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

> 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);

Hmm, maybe we actually don't need that special mutex after all. I shall
check that sometime later, independently of your patch series.
--
Stefan Richter
-=====-==--- ---= -==--
http://arcgraph.de/sr/

2008-01-14 01:57:49

by Dave Young

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

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

2008-01-15 08:39:41

by Dave Young

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

Is it right for me to put_device after class_find_device at following point?

On Jan 12, 2008 5:50 PM, Dave Young <[email protected]> wrote:
> Convert to use the class iteration api.
>
> Signed-off-by: Dave Young <[email protected]>
>
> ---
> drivers/ieee1394/nodemgr.c | 319 +++++++++++++++++++++++++--------------------
> 1 file changed, 178 insertions(+), 141 deletions(-)
>
> diff -upr linux/drivers/ieee1394/nodemgr.c linux.new/drivers/ieee1394/nodemgr.c
> --- linux/drivers/ieee1394/nodemgr.c 2008-01-12 15:20:27.000000000 +0800
> +++ linux.new/drivers/ieee1394/nodemgr.c 2008-01-12 15:20:27.000000000 +0800
> @@ -727,36 +727,35 @@ static int nodemgr_bus_match(struct devi
>
> static DEFINE_MUTEX(nodemgr_serialize_remove_uds);
>
> +static int __match_ne(struct device *dev, void *data)
> +{
> + struct unit_directory *ud;
> + struct node_entry *ne = (struct node_entry *)data;
> +
> + ud = container_of(dev, struct unit_directory, unit_dev);
> + if (ud->ne == ne)
> + return 1;
> + return 0;
> +}
> +
> 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);

============
Here.
============

> + device_unregister(&ud->unit_dev);
> + device_unregister(&ud->device);
> mutex_unlock(&nodemgr_serialize_remove_uds);
> }
>
> @@ -882,45 +881,64 @@ fail_alloc:
> return NULL;
> }
>
> +static int __match_ne_guid(struct device *dev, void *data)
> +{
> + struct node_entry *ne;
> + u64 *guid = (u64 *)data;
> +
> + ne = container_of(dev, struct node_entry, node_dev);
> + if (ne->guid == *guid)
> + return 1;
> + return 0;
> +}
>
> static struct node_entry *find_entry_by_guid(u64 guid)
> {
> struct device *dev;
> - struct node_entry *ne, *ret_ne = NULL;
> -
> - down(&nodemgr_ne_class.sem);
> - list_for_each_entry(dev, &nodemgr_ne_class.devices, node) {
> - ne = container_of(dev, struct node_entry, node_dev);
> + struct node_entry *ne;
>
> - if (ne->guid == guid) {
> - ret_ne = ne;
> - break;
> - }
> - }
> - up(&nodemgr_ne_class.sem);
> + dev = class_find_device(&nodemgr_ne_class, &guid, __match_ne_guid);
> + if (!dev)
> + return NULL;
> + ne = container_of(dev, struct node_entry, node_dev);

============
And here
============

>
> - return ret_ne;
> + return ne;
> }
>
> +struct match_nodeid_param {
> + struct hpsb_host *host;
> + nodeid_t nodeid;
> +};
> +
> +static int __match_ne_nodeid(struct device *dev, void *data)
> +{
> + struct node_entry *ne;
> + struct match_nodeid_param *param = (struct match_nodeid_param *)data;
> +
> + if (!dev)
> + return 0;
> + ne = container_of(dev, struct node_entry, node_dev);
> + if (ne->host == param->host && ne->nodeid == param->nodeid)
> + return 1;
> + return 0;
> +}
>
> static struct node_entry *find_entry_by_nodeid(struct hpsb_host *host,
> nodeid_t nodeid)
> {
> struct device *dev;
> - struct node_entry *ne, *ret_ne = NULL;
> + struct node_entry *ne;
> + struct match_nodeid_param param;
>
> - down(&nodemgr_ne_class.sem);
> - list_for_each_entry(dev, &nodemgr_ne_class.devices, node) {
> - ne = container_of(dev, struct node_entry, node_dev);
> + param.host = host;
> + param.nodeid = nodeid;
>
> - if (ne->host == host && ne->nodeid == nodeid) {
> - ret_ne = ne;
> - break;
> - }
> - }
> - up(&nodemgr_ne_class.sem);
> + dev = class_find_device(&nodemgr_ne_class, &param, __match_ne_nodeid);
> + if (!dev)
> + return NULL;
> + ne = container_of(dev, struct node_entry, node_dev);

============
And here
============

>
> - return ret_ne;
> + return ne;
> }
>
>
> @@ -1370,107 +1388,109 @@ static void nodemgr_node_scan(struct hos
> }
> }
>
> -
> -static void nodemgr_suspend_ne(struct node_entry *ne)
> +static int __nodemgr_driver_suspend(struct device *dev, void *data)
> {
> - struct device *dev;
> struct unit_directory *ud;
> struct device_driver *drv;
> + struct node_entry *ne = (struct node_entry *)data;
> int error;
>
> - HPSB_DEBUG("Node suspended: ID:BUS[" NODE_BUS_FMT "] GUID[%016Lx]",
> - NODE_BUS_ARGS(ne->host, ne->nodeid), (unsigned long long)ne->guid);
> + ud = container_of(dev, struct unit_directory, unit_dev);
> + if (ud->ne == ne) {
> + drv = get_driver(ud->device.driver);
> + if (drv) {
> + error = 1; /* release if suspend is not implemented */
> + if (drv->suspend) {
> + down(&ud->device.sem);
> + error = drv->suspend(&ud->device, PMSG_SUSPEND);
> + up(&ud->device.sem);
> + }
> + if (error)
> + device_release_driver(&ud->device);
> + put_driver(drv);
> + }
> + }
>
> - ne->in_limbo = 1;
> - WARN_ON(device_create_file(&ne->device, &dev_attr_ne_in_limbo));
> + return 0;
> +}
>
> - down(&nodemgr_ud_class.sem);
> - list_for_each_entry(dev, &nodemgr_ud_class.devices, node) {
> - ud = container_of(dev, struct unit_directory, unit_dev);
> - if (ud->ne != ne)
> - continue;
> +static int __nodemgr_driver_resume(struct device *dev, void *data)
> +{
> + struct unit_directory *ud;
> + struct device_driver *drv;
> + struct node_entry *ne = (struct node_entry *)data;
>
> + ud = container_of(dev, struct unit_directory, unit_dev);
> + if (ud->ne == ne) {
> drv = get_driver(ud->device.driver);
> - if (!drv)
> - continue;
> -
> - error = 1; /* release if suspend is not implemented */
> - if (drv->suspend) {
> - down(&ud->device.sem);
> - error = drv->suspend(&ud->device, PMSG_SUSPEND);
> - up(&ud->device.sem);
> + if (drv) {
> + if (drv->resume) {
> + down(&ud->device.sem);
> + drv->resume(&ud->device);
> + up(&ud->device.sem);
> + }
> + put_driver(drv);
> }
> - if (error)
> - device_release_driver(&ud->device);
> - put_driver(drv);
> }
> - up(&nodemgr_ud_class.sem);
> -}
>
> + return 0;
> +}
>
> -static void nodemgr_resume_ne(struct node_entry *ne)
> +static void nodemgr_suspend_ne(struct node_entry *ne)
> {
> - struct device *dev;
> - struct unit_directory *ud;
> - struct device_driver *drv;
> + HPSB_DEBUG("Node suspended: ID:BUS[" NODE_BUS_FMT "] GUID[%016Lx]",
> + NODE_BUS_ARGS(ne->host, ne->nodeid),
> + (unsigned long long)ne->guid);
>
> - ne->in_limbo = 0;
> - device_remove_file(&ne->device, &dev_attr_ne_in_limbo);
> + ne->in_limbo = 1;
> + WARN_ON(device_create_file(&ne->device, &dev_attr_ne_in_limbo));
>
> - down(&nodemgr_ud_class.sem);
> - list_for_each_entry(dev, &nodemgr_ud_class.devices, node) {
> - ud = container_of(dev, struct unit_directory, unit_dev);
> - if (ud->ne != ne)
> - continue;
> + class_for_each_device(&nodemgr_ud_class, ne, __nodemgr_driver_suspend);
> +}
>
> - drv = get_driver(ud->device.driver);
> - if (!drv)
> - continue;
>
> - if (drv->resume) {
> - down(&ud->device.sem);
> - drv->resume(&ud->device);
> - up(&ud->device.sem);
> - }
> - put_driver(drv);
> - }
> - up(&nodemgr_ud_class.sem);
> +static void nodemgr_resume_ne(struct node_entry *ne)
> +{
> + ne->in_limbo = 0;
> + device_remove_file(&ne->device, &dev_attr_ne_in_limbo);
>
> + class_for_each_device(&nodemgr_ud_class, ne, __nodemgr_driver_resume);
> HPSB_DEBUG("Node resumed: ID:BUS[" NODE_BUS_FMT "] GUID[%016Lx]",
> NODE_BUS_ARGS(ne->host, ne->nodeid), (unsigned long long)ne->guid);
> }
>
> -
> -static void nodemgr_update_pdrv(struct node_entry *ne)
> +static int __nodemgr_update_pdrv(struct device *dev, void *data)
> {
> - struct device *dev;
> struct unit_directory *ud;
> struct device_driver *drv;
> struct hpsb_protocol_driver *pdrv;
> + struct node_entry *ne = (struct node_entry *)data;
> int error;
>
> - down(&nodemgr_ud_class.sem);
> - list_for_each_entry(dev, &nodemgr_ud_class.devices, node) {
> - ud = container_of(dev, struct unit_directory, unit_dev);
> - if (ud->ne != ne)
> - continue;
> -
> + ud = container_of(dev, struct unit_directory, unit_dev);
> + if (ud->ne == ne) {
> drv = get_driver(ud->device.driver);
> - if (!drv)
> - continue;
> -
> - error = 0;
> - pdrv = container_of(drv, struct hpsb_protocol_driver, driver);
> - if (pdrv->update) {
> - down(&ud->device.sem);
> - error = pdrv->update(ud);
> - up(&ud->device.sem);
> + if (drv) {
> + error = 0;
> + pdrv = container_of(drv, struct hpsb_protocol_driver,
> + driver);
> + if (pdrv->update) {
> + down(&ud->device.sem);
> + error = pdrv->update(ud);
> + up(&ud->device.sem);
> + }
> + if (error)
> + device_release_driver(&ud->device);
> + put_driver(drv);
> }
> - if (error)
> - device_release_driver(&ud->device);
> - put_driver(drv);
> }
> - up(&nodemgr_ud_class.sem);
> +
> + return 0;
> +}
> +
> +static void nodemgr_update_pdrv(struct node_entry *ne)
> +{
> + class_for_each_device(&nodemgr_ud_class, ne, __nodemgr_update_pdrv);
> }
>
>
> @@ -1529,13 +1549,31 @@ static void nodemgr_probe_ne(struct host
> put_device(dev);
> }
>
> +struct probe_param {
> + struct host_info *hi;
> + int generation;
> +};
> +
> +static int __nodemgr_node_probe(struct device *dev, void *data)
> +{
> + struct probe_param *param = (struct probe_param *)data;
> + struct node_entry *ne;
> +
> + ne = container_of(dev, struct node_entry, node_dev);
> + if (!ne->needs_probe)
> + nodemgr_probe_ne(param->hi, ne, param->generation);
> + if (ne->needs_probe)
> + nodemgr_probe_ne(param->hi, ne, param->generation);
> + return 0;
> +}
>
> static void nodemgr_node_probe(struct host_info *hi, int generation)
> {
> struct hpsb_host *host = hi->host;
> - struct device *dev;
> - struct node_entry *ne;
> + struct probe_param param;
>
> + param.hi = hi;
> + param.generation = generation;
> /* Do some processing of the nodes we've probed. This pulls them
> * into the sysfs layer if needed, and can result in processing of
> * unit-directories, or just updating the node and it's
> @@ -1545,19 +1583,7 @@ static void nodemgr_node_probe(struct ho
> * while probes are time-consuming. (Well, those probes need some
> * improvement...) */
>
> - down(&nodemgr_ne_class.sem);
> - list_for_each_entry(dev, &nodemgr_ne_class.devices, node) {
> - ne = container_of(dev, struct node_entry, node_dev);
> - if (!ne->needs_probe)
> - nodemgr_probe_ne(hi, ne, generation);
> - }
> - list_for_each_entry(dev, &nodemgr_ne_class.devices, node) {
> - ne = container_of(dev, struct node_entry, node_dev);
> - if (ne->needs_probe)
> - nodemgr_probe_ne(hi, ne, generation);
> - }
> - up(&nodemgr_ne_class.sem);
> -
> + class_for_each_device(&nodemgr_ne_class, &param, __nodemgr_node_probe);
>
> /* If we had a bus reset while we were scanning the bus, it is
> * possible that we did not probe all nodes. In that case, we
> @@ -1757,6 +1783,22 @@ exit:
> return 0;
> }
>
> +struct host_iter_param {
> + void *data;
> + int (*cb)(struct hpsb_host *, void *);
> +};
> +
> +static int __nodemgr_for_each_host(struct device *dev, void *data)
> +{
> + struct hpsb_host *host;
> + struct host_iter_param *hip = (struct host_iter_param *)data;
> + int error = 0;
> +
> + host = container_of(dev, struct hpsb_host, host_dev);
> + error = hip->cb(host, hip->data);
> +
> + return error;
> +}
> /**
> * nodemgr_for_each_host - call a function for each IEEE 1394 host
> * @data: an address to supply to the callback
> @@ -1771,18 +1813,13 @@ exit:
> */
> int nodemgr_for_each_host(void *data, int (*cb)(struct hpsb_host *, void *))
> {
> - struct device *dev;
> - struct hpsb_host *host;
> - int error = 0;
> -
> - down(&hpsb_host_class.sem);
> - list_for_each_entry(dev, &hpsb_host_class.devices, node) {
> - host = container_of(dev, struct hpsb_host, host_dev);
> + struct host_iter_param hip;
> + int error;
>
> - if ((error = cb(host, data)))
> - break;
> - }
> - up(&hpsb_host_class.sem);
> + hip.cb = cb;
> + hip.data = data;
> + error = class_for_each_device(&hpsb_host_class, &hip,
> + __nodemgr_for_each_host);
>
> return error;
> }
>

2008-01-15 18:11:40

by Stefan Richter

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

Dave Young wrote:
> Is it right for me to put_device after class_find_device at following point?

Yes, that's right.
--
Stefan Richter
-=====-==--- ---= -====
http://arcgraph.de/sr/

2008-01-16 00:47:19

by Dave Young

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

Thanks for stefan's help to finish this patch, now updated one:

Convert to use the class iteration api.

Signed-off-by: Dave Young <[email protected]>

---
drivers/ieee1394/nodemgr.c | 312 +++++++++++++++++++++++++--------------------
1 file changed, 175 insertions(+), 137 deletions(-)

diff -upr linux/drivers/ieee1394/nodemgr.c linux.new/drivers/ieee1394/nodemgr.c
--- linux/drivers/ieee1394/nodemgr.c 2008-01-16 08:43:35.000000000 +0800
+++ linux.new/drivers/ieee1394/nodemgr.c 2008-01-16 08:43:35.000000000 +0800
@@ -727,33 +727,31 @@ static int nodemgr_bus_match(struct devi

static DEFINE_MUTEX(nodemgr_serialize_remove_uds);

+static int __match_ne(struct device *dev, void *data)
+{
+ struct unit_directory *ud;
+ struct node_entry *ne = (struct node_entry *)data;
+
+ ud = container_of(dev, struct unit_directory, unit_dev);
+ return ud->ne == ne;
+}
+
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)
+ dev = class_find_device(&nodemgr_ud_class, ne, __match_ne);
+ if (!dev)
break;
+ ud = container_of(dev, struct unit_directory, unit_dev);
+ put_device(dev);
device_unregister(&ud->unit_dev);
device_unregister(&ud->device);
}
@@ -882,45 +880,66 @@ fail_alloc:
return NULL;
}

+static int __match_ne_guid(struct device *dev, void *data)
+{
+ struct node_entry *ne;
+ u64 *guid = (u64 *)data;
+
+ ne = container_of(dev, struct node_entry, node_dev);
+ return ne->guid == *guid;
+}

static struct node_entry *find_entry_by_guid(u64 guid)
{
struct device *dev;
- struct node_entry *ne, *ret_ne = NULL;
-
- down(&nodemgr_ne_class.sem);
- list_for_each_entry(dev, &nodemgr_ne_class.devices, node) {
- ne = container_of(dev, struct node_entry, node_dev);
+ struct node_entry *ne;

- if (ne->guid == guid) {
- ret_ne = ne;
- break;
- }
- }
- up(&nodemgr_ne_class.sem);
+ dev = class_find_device(&nodemgr_ne_class, &guid, __match_ne_guid);
+ if (!dev)
+ return NULL;
+ ne = container_of(dev, struct node_entry, node_dev);
+ put_device(dev);

- return ret_ne;
+ return ne;
}

+struct match_nodeid_param {
+ struct hpsb_host *host;
+ nodeid_t nodeid;
+};
+
+static int __match_ne_nodeid(struct device *dev, void *data)
+{
+ int found = 0;
+ struct node_entry *ne;
+ struct match_nodeid_param *param = (struct match_nodeid_param *)data;
+
+ if (!dev)
+ goto ret;
+ ne = container_of(dev, struct node_entry, node_dev);
+ if (ne->host == param->host && ne->nodeid == param->nodeid)
+ found = 1;
+ret:
+ return found;
+}

static struct node_entry *find_entry_by_nodeid(struct hpsb_host *host,
nodeid_t nodeid)
{
struct device *dev;
- struct node_entry *ne, *ret_ne = NULL;
+ struct node_entry *ne;
+ struct match_nodeid_param param;

- down(&nodemgr_ne_class.sem);
- list_for_each_entry(dev, &nodemgr_ne_class.devices, node) {
- ne = container_of(dev, struct node_entry, node_dev);
+ param.host = host;
+ param.nodeid = nodeid;

- if (ne->host == host && ne->nodeid == nodeid) {
- ret_ne = ne;
- break;
- }
- }
- up(&nodemgr_ne_class.sem);
+ dev = class_find_device(&nodemgr_ne_class, &param, __match_ne_nodeid);
+ if (!dev)
+ return NULL;
+ ne = container_of(dev, struct node_entry, node_dev);
+ put_device(dev);

- return ret_ne;
+ return ne;
}


@@ -1370,107 +1389,109 @@ static void nodemgr_node_scan(struct hos
}
}

-
-static void nodemgr_suspend_ne(struct node_entry *ne)
+static int __nodemgr_driver_suspend(struct device *dev, void *data)
{
- struct device *dev;
struct unit_directory *ud;
struct device_driver *drv;
+ struct node_entry *ne = (struct node_entry *)data;
int error;

- HPSB_DEBUG("Node suspended: ID:BUS[" NODE_BUS_FMT "] GUID[%016Lx]",
- NODE_BUS_ARGS(ne->host, ne->nodeid), (unsigned long long)ne->guid);
+ ud = container_of(dev, struct unit_directory, unit_dev);
+ if (ud->ne == ne) {
+ drv = get_driver(ud->device.driver);
+ if (drv) {
+ error = 1; /* release if suspend is not implemented */
+ if (drv->suspend) {
+ down(&ud->device.sem);
+ error = drv->suspend(&ud->device, PMSG_SUSPEND);
+ up(&ud->device.sem);
+ }
+ if (error)
+ device_release_driver(&ud->device);
+ put_driver(drv);
+ }
+ }

- ne->in_limbo = 1;
- WARN_ON(device_create_file(&ne->device, &dev_attr_ne_in_limbo));
+ return 0;
+}

- down(&nodemgr_ud_class.sem);
- list_for_each_entry(dev, &nodemgr_ud_class.devices, node) {
- ud = container_of(dev, struct unit_directory, unit_dev);
- if (ud->ne != ne)
- continue;
+static int __nodemgr_driver_resume(struct device *dev, void *data)
+{
+ struct unit_directory *ud;
+ struct device_driver *drv;
+ struct node_entry *ne = (struct node_entry *)data;

+ ud = container_of(dev, struct unit_directory, unit_dev);
+ if (ud->ne == ne) {
drv = get_driver(ud->device.driver);
- if (!drv)
- continue;
-
- error = 1; /* release if suspend is not implemented */
- if (drv->suspend) {
- down(&ud->device.sem);
- error = drv->suspend(&ud->device, PMSG_SUSPEND);
- up(&ud->device.sem);
+ if (drv) {
+ if (drv->resume) {
+ down(&ud->device.sem);
+ drv->resume(&ud->device);
+ up(&ud->device.sem);
+ }
+ put_driver(drv);
}
- if (error)
- device_release_driver(&ud->device);
- put_driver(drv);
}
- up(&nodemgr_ud_class.sem);
-}

+ return 0;
+}

-static void nodemgr_resume_ne(struct node_entry *ne)
+static void nodemgr_suspend_ne(struct node_entry *ne)
{
- struct device *dev;
- struct unit_directory *ud;
- struct device_driver *drv;
+ HPSB_DEBUG("Node suspended: ID:BUS[" NODE_BUS_FMT "] GUID[%016Lx]",
+ NODE_BUS_ARGS(ne->host, ne->nodeid),
+ (unsigned long long)ne->guid);

- ne->in_limbo = 0;
- device_remove_file(&ne->device, &dev_attr_ne_in_limbo);
+ ne->in_limbo = 1;
+ WARN_ON(device_create_file(&ne->device, &dev_attr_ne_in_limbo));

- down(&nodemgr_ud_class.sem);
- list_for_each_entry(dev, &nodemgr_ud_class.devices, node) {
- ud = container_of(dev, struct unit_directory, unit_dev);
- if (ud->ne != ne)
- continue;
+ class_for_each_device(&nodemgr_ud_class, ne, __nodemgr_driver_suspend);
+}

- drv = get_driver(ud->device.driver);
- if (!drv)
- continue;

- if (drv->resume) {
- down(&ud->device.sem);
- drv->resume(&ud->device);
- up(&ud->device.sem);
- }
- put_driver(drv);
- }
- up(&nodemgr_ud_class.sem);
+static void nodemgr_resume_ne(struct node_entry *ne)
+{
+ ne->in_limbo = 0;
+ device_remove_file(&ne->device, &dev_attr_ne_in_limbo);

+ class_for_each_device(&nodemgr_ud_class, ne, __nodemgr_driver_resume);
HPSB_DEBUG("Node resumed: ID:BUS[" NODE_BUS_FMT "] GUID[%016Lx]",
NODE_BUS_ARGS(ne->host, ne->nodeid), (unsigned long long)ne->guid);
}

-
-static void nodemgr_update_pdrv(struct node_entry *ne)
+static int __nodemgr_update_pdrv(struct device *dev, void *data)
{
- struct device *dev;
struct unit_directory *ud;
struct device_driver *drv;
struct hpsb_protocol_driver *pdrv;
+ struct node_entry *ne = (struct node_entry *)data;
int error;

- down(&nodemgr_ud_class.sem);
- list_for_each_entry(dev, &nodemgr_ud_class.devices, node) {
- ud = container_of(dev, struct unit_directory, unit_dev);
- if (ud->ne != ne)
- continue;
-
+ ud = container_of(dev, struct unit_directory, unit_dev);
+ if (ud->ne == ne) {
drv = get_driver(ud->device.driver);
- if (!drv)
- continue;
-
- error = 0;
- pdrv = container_of(drv, struct hpsb_protocol_driver, driver);
- if (pdrv->update) {
- down(&ud->device.sem);
- error = pdrv->update(ud);
- up(&ud->device.sem);
+ if (drv) {
+ error = 0;
+ pdrv = container_of(drv, struct hpsb_protocol_driver,
+ driver);
+ if (pdrv->update) {
+ down(&ud->device.sem);
+ error = pdrv->update(ud);
+ up(&ud->device.sem);
+ }
+ if (error)
+ device_release_driver(&ud->device);
+ put_driver(drv);
}
- if (error)
- device_release_driver(&ud->device);
- put_driver(drv);
}
- up(&nodemgr_ud_class.sem);
+
+ return 0;
+}
+
+static void nodemgr_update_pdrv(struct node_entry *ne)
+{
+ class_for_each_device(&nodemgr_ud_class, ne, __nodemgr_update_pdrv);
}


@@ -1529,13 +1550,31 @@ static void nodemgr_probe_ne(struct host
put_device(dev);
}

+struct probe_param {
+ struct host_info *hi;
+ int generation;
+};
+
+static int __nodemgr_node_probe(struct device *dev, void *data)
+{
+ struct probe_param *param = (struct probe_param *)data;
+ struct node_entry *ne;
+
+ ne = container_of(dev, struct node_entry, node_dev);
+ if (!ne->needs_probe)
+ nodemgr_probe_ne(param->hi, ne, param->generation);
+ if (ne->needs_probe)
+ nodemgr_probe_ne(param->hi, ne, param->generation);
+ return 0;
+}

static void nodemgr_node_probe(struct host_info *hi, int generation)
{
struct hpsb_host *host = hi->host;
- struct device *dev;
- struct node_entry *ne;
+ struct probe_param param;

+ param.hi = hi;
+ param.generation = generation;
/* Do some processing of the nodes we've probed. This pulls them
* into the sysfs layer if needed, and can result in processing of
* unit-directories, or just updating the node and it's
@@ -1545,19 +1584,7 @@ static void nodemgr_node_probe(struct ho
* while probes are time-consuming. (Well, those probes need some
* improvement...) */

- down(&nodemgr_ne_class.sem);
- list_for_each_entry(dev, &nodemgr_ne_class.devices, node) {
- ne = container_of(dev, struct node_entry, node_dev);
- if (!ne->needs_probe)
- nodemgr_probe_ne(hi, ne, generation);
- }
- list_for_each_entry(dev, &nodemgr_ne_class.devices, node) {
- ne = container_of(dev, struct node_entry, node_dev);
- if (ne->needs_probe)
- nodemgr_probe_ne(hi, ne, generation);
- }
- up(&nodemgr_ne_class.sem);
-
+ class_for_each_device(&nodemgr_ne_class, &param, __nodemgr_node_probe);

/* If we had a bus reset while we were scanning the bus, it is
* possible that we did not probe all nodes. In that case, we
@@ -1757,6 +1784,22 @@ exit:
return 0;
}

+struct host_iter_param {
+ void *data;
+ int (*cb)(struct hpsb_host *, void *);
+};
+
+static int __nodemgr_for_each_host(struct device *dev, void *data)
+{
+ struct hpsb_host *host;
+ struct host_iter_param *hip = (struct host_iter_param *)data;
+ int error = 0;
+
+ host = container_of(dev, struct hpsb_host, host_dev);
+ error = hip->cb(host, hip->data);
+
+ return error;
+}
/**
* nodemgr_for_each_host - call a function for each IEEE 1394 host
* @data: an address to supply to the callback
@@ -1771,18 +1814,13 @@ exit:
*/
int nodemgr_for_each_host(void *data, int (*cb)(struct hpsb_host *, void *))
{
- struct device *dev;
- struct hpsb_host *host;
- int error = 0;
-
- down(&hpsb_host_class.sem);
- list_for_each_entry(dev, &hpsb_host_class.devices, node) {
- host = container_of(dev, struct hpsb_host, host_dev);
+ struct host_iter_param hip;
+ int error;

- if ((error = cb(host, data)))
- break;
- }
- up(&hpsb_host_class.sem);
+ hip.cb = cb;
+ hip.data = data;
+ error = class_for_each_device(&hpsb_host_class, &hip,
+ __nodemgr_for_each_host);

return error;
}

2008-01-17 00:19:42

by Stefan Richter

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

Dave Young wrote:
> Convert to use the class iteration api.

I'll try to review and test it at the weekend. Thanks for patiently
going through all the patch iterations.
--
Stefan Richter
-=====-==--- ---= =---=
http://arcgraph.de/sr/

2008-01-22 05:55:58

by Dave Young

[permalink] [raw]
Subject: [PATCH 2/6] ieee1394 : use class iteration api

Convert to use the class iteration api.

Signed-off-by: Dave Young <[email protected]>

---
drivers/ieee1394/nodemgr.c | 312 +++++++++++++++++++++++++--------------------
1 file changed, 175 insertions(+), 137 deletions(-)

diff -upr linux/drivers/ieee1394/nodemgr.c linux.new/drivers/ieee1394/nodemgr.c
--- linux/drivers/ieee1394/nodemgr.c 2008-01-16 08:43:35.000000000 +0800
+++ linux.new/drivers/ieee1394/nodemgr.c 2008-01-16 08:43:35.000000000 +0800
@@ -727,33 +727,31 @@ static int nodemgr_bus_match(struct devi

static DEFINE_MUTEX(nodemgr_serialize_remove_uds);

+static int __match_ne(struct device *dev, void *data)
+{
+ struct unit_directory *ud;
+ struct node_entry *ne = (struct node_entry *)data;
+
+ ud = container_of(dev, struct unit_directory, unit_dev);
+ return ud->ne == ne;
+}
+
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)
+ dev = class_find_device(&nodemgr_ud_class, ne, __match_ne);
+ if (!dev)
break;
+ ud = container_of(dev, struct unit_directory, unit_dev);
+ put_device(dev);
device_unregister(&ud->unit_dev);
device_unregister(&ud->device);
}
@@ -882,45 +880,66 @@ fail_alloc:
return NULL;
}

+static int __match_ne_guid(struct device *dev, void *data)
+{
+ struct node_entry *ne;
+ u64 *guid = (u64 *)data;
+
+ ne = container_of(dev, struct node_entry, node_dev);
+ return ne->guid == *guid;
+}

static struct node_entry *find_entry_by_guid(u64 guid)
{
struct device *dev;
- struct node_entry *ne, *ret_ne = NULL;
-
- down(&nodemgr_ne_class.sem);
- list_for_each_entry(dev, &nodemgr_ne_class.devices, node) {
- ne = container_of(dev, struct node_entry, node_dev);
+ struct node_entry *ne;

- if (ne->guid == guid) {
- ret_ne = ne;
- break;
- }
- }
- up(&nodemgr_ne_class.sem);
+ dev = class_find_device(&nodemgr_ne_class, &guid, __match_ne_guid);
+ if (!dev)
+ return NULL;
+ ne = container_of(dev, struct node_entry, node_dev);
+ put_device(dev);

- return ret_ne;
+ return ne;
}

+struct match_nodeid_param {
+ struct hpsb_host *host;
+ nodeid_t nodeid;
+};
+
+static int __match_ne_nodeid(struct device *dev, void *data)
+{
+ int found = 0;
+ struct node_entry *ne;
+ struct match_nodeid_param *param = (struct match_nodeid_param *)data;
+
+ if (!dev)
+ goto ret;
+ ne = container_of(dev, struct node_entry, node_dev);
+ if (ne->host == param->host && ne->nodeid == param->nodeid)
+ found = 1;
+ret:
+ return found;
+}

static struct node_entry *find_entry_by_nodeid(struct hpsb_host *host,
nodeid_t nodeid)
{
struct device *dev;
- struct node_entry *ne, *ret_ne = NULL;
+ struct node_entry *ne;
+ struct match_nodeid_param param;

- down(&nodemgr_ne_class.sem);
- list_for_each_entry(dev, &nodemgr_ne_class.devices, node) {
- ne = container_of(dev, struct node_entry, node_dev);
+ param.host = host;
+ param.nodeid = nodeid;

- if (ne->host == host && ne->nodeid == nodeid) {
- ret_ne = ne;
- break;
- }
- }
- up(&nodemgr_ne_class.sem);
+ dev = class_find_device(&nodemgr_ne_class, &param, __match_ne_nodeid);
+ if (!dev)
+ return NULL;
+ ne = container_of(dev, struct node_entry, node_dev);
+ put_device(dev);

- return ret_ne;
+ return ne;
}


@@ -1370,107 +1389,109 @@ static void nodemgr_node_scan(struct hos
}
}

-
-static void nodemgr_suspend_ne(struct node_entry *ne)
+static int __nodemgr_driver_suspend(struct device *dev, void *data)
{
- struct device *dev;
struct unit_directory *ud;
struct device_driver *drv;
+ struct node_entry *ne = (struct node_entry *)data;
int error;

- HPSB_DEBUG("Node suspended: ID:BUS[" NODE_BUS_FMT "] GUID[%016Lx]",
- NODE_BUS_ARGS(ne->host, ne->nodeid), (unsigned long long)ne->guid);
+ ud = container_of(dev, struct unit_directory, unit_dev);
+ if (ud->ne == ne) {
+ drv = get_driver(ud->device.driver);
+ if (drv) {
+ error = 1; /* release if suspend is not implemented */
+ if (drv->suspend) {
+ down(&ud->device.sem);
+ error = drv->suspend(&ud->device, PMSG_SUSPEND);
+ up(&ud->device.sem);
+ }
+ if (error)
+ device_release_driver(&ud->device);
+ put_driver(drv);
+ }
+ }

- ne->in_limbo = 1;
- WARN_ON(device_create_file(&ne->device, &dev_attr_ne_in_limbo));
+ return 0;
+}

- down(&nodemgr_ud_class.sem);
- list_for_each_entry(dev, &nodemgr_ud_class.devices, node) {
- ud = container_of(dev, struct unit_directory, unit_dev);
- if (ud->ne != ne)
- continue;
+static int __nodemgr_driver_resume(struct device *dev, void *data)
+{
+ struct unit_directory *ud;
+ struct device_driver *drv;
+ struct node_entry *ne = (struct node_entry *)data;

+ ud = container_of(dev, struct unit_directory, unit_dev);
+ if (ud->ne == ne) {
drv = get_driver(ud->device.driver);
- if (!drv)
- continue;
-
- error = 1; /* release if suspend is not implemented */
- if (drv->suspend) {
- down(&ud->device.sem);
- error = drv->suspend(&ud->device, PMSG_SUSPEND);
- up(&ud->device.sem);
+ if (drv) {
+ if (drv->resume) {
+ down(&ud->device.sem);
+ drv->resume(&ud->device);
+ up(&ud->device.sem);
+ }
+ put_driver(drv);
}
- if (error)
- device_release_driver(&ud->device);
- put_driver(drv);
}
- up(&nodemgr_ud_class.sem);
-}

+ return 0;
+}

-static void nodemgr_resume_ne(struct node_entry *ne)
+static void nodemgr_suspend_ne(struct node_entry *ne)
{
- struct device *dev;
- struct unit_directory *ud;
- struct device_driver *drv;
+ HPSB_DEBUG("Node suspended: ID:BUS[" NODE_BUS_FMT "] GUID[%016Lx]",
+ NODE_BUS_ARGS(ne->host, ne->nodeid),
+ (unsigned long long)ne->guid);

- ne->in_limbo = 0;
- device_remove_file(&ne->device, &dev_attr_ne_in_limbo);
+ ne->in_limbo = 1;
+ WARN_ON(device_create_file(&ne->device, &dev_attr_ne_in_limbo));

- down(&nodemgr_ud_class.sem);
- list_for_each_entry(dev, &nodemgr_ud_class.devices, node) {
- ud = container_of(dev, struct unit_directory, unit_dev);
- if (ud->ne != ne)
- continue;
+ class_for_each_device(&nodemgr_ud_class, ne, __nodemgr_driver_suspend);
+}

- drv = get_driver(ud->device.driver);
- if (!drv)
- continue;

- if (drv->resume) {
- down(&ud->device.sem);
- drv->resume(&ud->device);
- up(&ud->device.sem);
- }
- put_driver(drv);
- }
- up(&nodemgr_ud_class.sem);
+static void nodemgr_resume_ne(struct node_entry *ne)
+{
+ ne->in_limbo = 0;
+ device_remove_file(&ne->device, &dev_attr_ne_in_limbo);

+ class_for_each_device(&nodemgr_ud_class, ne, __nodemgr_driver_resume);
HPSB_DEBUG("Node resumed: ID:BUS[" NODE_BUS_FMT "] GUID[%016Lx]",
NODE_BUS_ARGS(ne->host, ne->nodeid), (unsigned long long)ne->guid);
}

-
-static void nodemgr_update_pdrv(struct node_entry *ne)
+static int __nodemgr_update_pdrv(struct device *dev, void *data)
{
- struct device *dev;
struct unit_directory *ud;
struct device_driver *drv;
struct hpsb_protocol_driver *pdrv;
+ struct node_entry *ne = (struct node_entry *)data;
int error;

- down(&nodemgr_ud_class.sem);
- list_for_each_entry(dev, &nodemgr_ud_class.devices, node) {
- ud = container_of(dev, struct unit_directory, unit_dev);
- if (ud->ne != ne)
- continue;
-
+ ud = container_of(dev, struct unit_directory, unit_dev);
+ if (ud->ne == ne) {
drv = get_driver(ud->device.driver);
- if (!drv)
- continue;
-
- error = 0;
- pdrv = container_of(drv, struct hpsb_protocol_driver, driver);
- if (pdrv->update) {
- down(&ud->device.sem);
- error = pdrv->update(ud);
- up(&ud->device.sem);
+ if (drv) {
+ error = 0;
+ pdrv = container_of(drv, struct hpsb_protocol_driver,
+ driver);
+ if (pdrv->update) {
+ down(&ud->device.sem);
+ error = pdrv->update(ud);
+ up(&ud->device.sem);
+ }
+ if (error)
+ device_release_driver(&ud->device);
+ put_driver(drv);
}
- if (error)
- device_release_driver(&ud->device);
- put_driver(drv);
}
- up(&nodemgr_ud_class.sem);
+
+ return 0;
+}
+
+static void nodemgr_update_pdrv(struct node_entry *ne)
+{
+ class_for_each_device(&nodemgr_ud_class, ne, __nodemgr_update_pdrv);
}


@@ -1529,13 +1550,31 @@ static void nodemgr_probe_ne(struct host
put_device(dev);
}

+struct probe_param {
+ struct host_info *hi;
+ int generation;
+};
+
+static int __nodemgr_node_probe(struct device *dev, void *data)
+{
+ struct probe_param *param = (struct probe_param *)data;
+ struct node_entry *ne;
+
+ ne = container_of(dev, struct node_entry, node_dev);
+ if (!ne->needs_probe)
+ nodemgr_probe_ne(param->hi, ne, param->generation);
+ if (ne->needs_probe)
+ nodemgr_probe_ne(param->hi, ne, param->generation);
+ return 0;
+}

static void nodemgr_node_probe(struct host_info *hi, int generation)
{
struct hpsb_host *host = hi->host;
- struct device *dev;
- struct node_entry *ne;
+ struct probe_param param;

+ param.hi = hi;
+ param.generation = generation;
/* Do some processing of the nodes we've probed. This pulls them
* into the sysfs layer if needed, and can result in processing of
* unit-directories, or just updating the node and it's
@@ -1545,19 +1584,7 @@ static void nodemgr_node_probe(struct ho
* while probes are time-consuming. (Well, those probes need some
* improvement...) */

- down(&nodemgr_ne_class.sem);
- list_for_each_entry(dev, &nodemgr_ne_class.devices, node) {
- ne = container_of(dev, struct node_entry, node_dev);
- if (!ne->needs_probe)
- nodemgr_probe_ne(hi, ne, generation);
- }
- list_for_each_entry(dev, &nodemgr_ne_class.devices, node) {
- ne = container_of(dev, struct node_entry, node_dev);
- if (ne->needs_probe)
- nodemgr_probe_ne(hi, ne, generation);
- }
- up(&nodemgr_ne_class.sem);
-
+ class_for_each_device(&nodemgr_ne_class, &param, __nodemgr_node_probe);

/* If we had a bus reset while we were scanning the bus, it is
* possible that we did not probe all nodes. In that case, we
@@ -1757,6 +1784,22 @@ exit:
return 0;
}

+struct host_iter_param {
+ void *data;
+ int (*cb)(struct hpsb_host *, void *);
+};
+
+static int __nodemgr_for_each_host(struct device *dev, void *data)
+{
+ struct hpsb_host *host;
+ struct host_iter_param *hip = (struct host_iter_param *)data;
+ int error = 0;
+
+ host = container_of(dev, struct hpsb_host, host_dev);
+ error = hip->cb(host, hip->data);
+
+ return error;
+}
/**
* nodemgr_for_each_host - call a function for each IEEE 1394 host
* @data: an address to supply to the callback
@@ -1771,18 +1814,13 @@ exit:
*/
int nodemgr_for_each_host(void *data, int (*cb)(struct hpsb_host *, void *))
{
- struct device *dev;
- struct hpsb_host *host;
- int error = 0;
-
- down(&hpsb_host_class.sem);
- list_for_each_entry(dev, &hpsb_host_class.devices, node) {
- host = container_of(dev, struct hpsb_host, host_dev);
+ struct host_iter_param hip;
+ int error;

- if ((error = cb(host, data)))
- break;
- }
- up(&hpsb_host_class.sem);
+ hip.cb = cb;
+ hip.data = data;
+ error = class_for_each_device(&hpsb_host_class, &hip,
+ __nodemgr_for_each_host);

return error;
}