RE: [PATCH 3/5] rapidio: run discovery as an asynchronous process

On Wed, October 03, 2012 6:30 PM
Andrew Morton [off-list ref] wrote:

Thank you for the fix. Will avoid that ugliness in the future.

quoted

+	pr_debug("RIO: discovery work for mport %d %s\n",
+		 work->mport->id, work->mport->name);
+	rio_disc_mport(work->mport);
+
+	kfree(work);
+}
+
 int __devinit rio_init_mports(void)
 {
 	struct rio_mport *port;
+	struct rio_disc_work *work;
+	int no_disc =3D 0;

 	list_for_each_entry(port, &rio_mports, node) {
 		if (port->host_deviceid >=3D 0)
 			rio_enum_mport(port);
-		else
-			rio_disc_mport(port);
+		else if (!no_disc) {
+			if (!rio_wq) {
+				rio_wq =3D alloc_workqueue("riodisc", 0, 0);
+				if (!rio_wq) {
+					pr_err("RIO: unable allocate rio_wq\n");
+					no_disc =3D 1;
+					continue;
+				}
+			}
+
+			work =3D kzalloc(sizeof *work, GFP_KERNEL);
+			if (!work) {
+				pr_err("RIO: no memory for work struct\n");
+				no_disc =3D 1;
+				continue;
+			}
+
+			work->mport =3D port;
+			INIT_WORK(&work->work, disc_work_handler);
+			queue_work(rio_wq, &work->work);
+		}
+	}

=20
I'm having a lot of trouble with `no_disc'.  afacit what it does is to
cease running async discovery for any remaining devices if the
workqueue
allocation failed (vaguely reasonable) or if the allocation of a single
work item failed (incomprehensible).
=20
But if we don't run discovery, the subsystem is permanently busted for
at least some devices, isn't it?

This is correct. We are considering ways to restart discovery
process later but it is not applicable now.

=20
And this code is basically untestable unless the programmer does
deliberate fault injection, which makes it pretty much unmaintainable.
=20
So...  if I haven't totally misunderstood, I suggest a rethink is in
order?

I will review and simplify. Probably, just try to allocate all required
resources ahead of port list scan. Simple and safe.
=20

quoted

+	if (rio_wq) {
+		pr_debug("RIO: flush discovery workqueue\n");
+		flush_workqueue(rio_wq);
+		pr_debug("RIO: flush discovery workqueue finished\n");
+		destroy_workqueue(rio_wq);
 	}