Re: [PATCH v7 03/12] powerpc/vas: Define vas_init() and vas_exit()

Michael Ellerman [mpe@ellerman.id.au] wrote:

Hi Suka,

Comments inline ...

Sukadev Bhattiprolu [off-list ref] writes:

quoted

diff --git a/Documentation/devicetree/bindings/powerpc/ibm,vas.txt b/Documentation/devicetree/bindings/powerpc/ibm,vas.txt
new file mode 100644
index 0000000..0e3111d
--- /dev/null
+++ b/Documentation/devicetree/bindings/powerpc/ibm,vas.txt

@@ -0,0 +1,24 @@
+* IBM Powerpc Virtual Accelerator Switchboard (VAS)
+
+VAS is a hardware mechanism that allows kernel subsystems and user processes
+to directly submit compression and other requests to Nest accelerators (NX)
+or other coprocessors functions.
+
+Required properties:
+- compatible : should be "ibm,vas" or "ibm,power9-vas"

The driver doesn't look for the latter.

Ok. I have removed it from this list of required properties

quoted

+- ibm,vas-id : A unique identifier for each instance of VAS in the system

What is this?

Like the ibm,chip-id, but in the future, there could be more than one instance
of VAS per chip, so firmware assigns a unique id to each instance of VAS.

quoted

+- reg : Should contain 4 pairs of 64-bit fields specifying the Hypervisor
+  window context start and length, OS/User window context start and length,
+  "Paste address" start and length, "Paste window id" start bit and number
+  of bits)
+- name : "vas"

I don't think the name is normally included in the binding, and in fact
there's no reason why the name is important, so I'd be inclined to drop that.

Ok. I dropped it.

quoted

diff --git a/MAINTAINERS b/MAINTAINERS
index 3c41902..abc235f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS

@@ -6425,6 +6425,14 @@ F:	drivers/crypto/nx/nx.*
 F:	drivers/crypto/nx/nx_csbcpb.h
 F:	drivers/crypto/nx/nx_debugfs.h
 
+IBM Power Virtual Accelerator Switchboard
+M:	Sukadev Bhattiprolu
+L:	linuxppc-dev@lists.ozlabs.org
+S:	Supported
+F:	arch/powerpc/platforms/powernv/vas*
+F:	arch/powerpc/include/asm/vas.h
+F:	arch/powerpc/include/uapi/asm/vas.h

That's not in the right place, the file is sorted alphabetically.

ah, fixed.

V comes after L.

quoted

diff --git a/arch/powerpc/platforms/powernv/Kconfig b/arch/powerpc/platforms/powernv/Kconfig
index 6a6f4ef..f565454 100644
--- a/arch/powerpc/platforms/powernv/Kconfig
+++ b/arch/powerpc/platforms/powernv/Kconfig

@@ -30,3 +30,17 @@ config OPAL_PRD
 	help
 	  This enables the opal-prd driver, a facility to run processor
 	  recovery diagnostics on OpenPower machines
+
+config PPC_VAS
+	bool "IBM Virtual Accelerator Switchboard (VAS)"

^ bool, so never a module.

yes, it should be built in.

quoted

+	depends on PPC_POWERNV && PPC_64K_PAGES
+	default n

It should be default y.

I know the usual advice is to make things 'default n', but this has
fairly tight depends already, so y is OK IMO.

Ok.

quoted

diff --git a/arch/powerpc/platforms/powernv/vas.c b/arch/powerpc/platforms/powernv/vas.c
new file mode 100644
index 0000000..556156b
--- /dev/null
+++ b/arch/powerpc/platforms/powernv/vas.c

@@ -0,0 +1,183 @@
+/*
+ * Copyright 2016 IBM Corp.

2016-2017.

Ok.

quoted

+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */

#define pr_fmt(fmt) "vas: " fmt

Ok

quoted

+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/export.h>
+#include <linux/types.h>
+#include <linux/slab.h>
+#include <linux/platform_device.h>
+#include <linux/of_platform.h>
+#include <linux/of_address.h>
+#include <linux/of.h>
+
+#include "vas.h"
+
+static bool init_done;
+LIST_HEAD(vas_instances);

Can be static.

Yes

quoted

+
+static int init_vas_instance(struct platform_device *pdev)
+{
+	int rc, vasid;
+	struct vas_instance *vinst;
+	struct device_node *dn = pdev->dev.of_node;
+	struct resource *res;

	struct device_node *dn = pdev->dev.of_node;
	struct vas_instance *vinst;
	struct resource *res;
	int rc, vasid;

Petty I know, but much prettier :)

I usually go the opposite way (shortest first) so I have done that here also.
For newer files I will invert the tree.

quoted

+
+	rc = of_property_read_u32(dn, "ibm,vas-id", &vasid);
+	if (rc) {
+		pr_err("VAS: No ibm,vas-id property for %s?\n", pdev->name);

With the pr_fmt() above you don't need VAS: on the front of all these.

Ok

quoted

+		return -ENODEV;
+	}
+
+	if (pdev->num_resources != 4) {
+		pr_err("VAS: Unexpected DT configuration for [%s, %d]\n",
+				pdev->name, vasid);
+		return -ENODEV;
+	}
+
+	vinst = kcalloc(1, sizeof(*vinst), GFP_KERNEL);

kzalloc() would be more normal given there's only 1.

Yes.

quoted

+	if (!vinst)
+		return -ENOMEM;
+
+	INIT_LIST_HEAD(&vinst->node);
+	ida_init(&vinst->ida);
+	mutex_init(&vinst->mutex);
+	vinst->vas_id = vasid;
+	vinst->pdev = pdev;
+
+	res = &pdev->resource[0];
+	vinst->hvwc_bar_start = res->start;
+	vinst->hvwc_bar_len = res->end - res->start + 1;
+
+	res = &pdev->resource[1];
+	vinst->uwc_bar_start = res->start;
+	vinst->uwc_bar_len = res->end - res->start + 1;

You have vinst->pdev, why do you need to copy all those?

I don't see the lens even used.

I have dropped the len fields. Kept the starts for now as it might
be easier to understand what the field is.

quoted

+	res = &pdev->resource[2];
+	vinst->paste_base_addr = res->start;
+
+	res = &pdev->resource[3];
+	vinst->paste_win_id_shift = 63 - res->end;

????

What if res->end is INT_MAX ?

Good question. I have added a check for res->end exceeding 62 but
am not sure how else to error check this or, for that matter, the
res->start fields that we get from skiboot.

quoted

+	pr_devel("VAS: Initialized instance [%s, %d], paste_base 0x%llx, "
+			"paste_win_id_shift 0x%llx\n", pdev->name, vasid,
+			vinst->paste_base_addr, vinst->paste_win_id_shift);
+
+	vinst->ready = true;

I don't see ready used.

Yes, we don't need it now. I have dropped it.

It also shouldn't be necessary, it's not ready unless it's in the list,
and if it's in the list then it's ready.

If you're actually concerned about concurrent usage then you need a
memory barrier here to order the stores to the vinst struct vs the
addition to the list. And you need a matching barrier on the read side.

quoted

+	list_add(&vinst->node, &vas_instances);
+
+	dev_set_drvdata(&pdev->dev, vinst);
+
+	return 0;
+}
+
+/*
+ * Although this is read/used multiple times, it is written to only
+ * during initialization.
+ */
+struct vas_instance *find_vas_instance(int vasid)
+{
+	struct list_head *ent;
+	struct vas_instance *vinst;
+
+	list_for_each(ent, &vas_instances) {
+		vinst = list_entry(ent, struct vas_instance, node);
+		if (vinst->vas_id == vasid)
+			return vinst;
+	}

There's no locking here, or any reference counting of the instances. see 

The vas_instances list is populated at startup and never really modified
(besides, the vas_exit() code which never gets called and has now been
dropped). I was trying to use vas_initialized() and avoid locking in
find_vas_instance(). 

I have now dropped vas_initialized() and added a lock here - this is
used in the window setup but not in copy/paste path, so the lock should
not matter much.

quoted

+	pr_devel("VAS: Instance %d not found\n", vasid);
+	return NULL;
+}
+
+bool vas_initialized(void)
+{
+	return init_done;
+}
+
+static int vas_probe(struct platform_device *pdev)
+{
+	if (!pdev || !pdev->dev.of_node)
+		return -ENODEV;

Neither of those can happen, or if they did it would be a BUG.

Ok. Changed to BUG_ON.

quoted

+	return init_vas_instance(pdev);
+}
+
+static void free_inst(struct vas_instance *vinst)
+{
+	list_del(&vinst->node);
+	kfree(vinst);

And here you just delete and the free the instance, even though you have
handed out pointers to the instance above in find_vas_instance().

So that needs work.

Is there any hardware cleanup required before we delete the instance? Or
do we just leave any windows there?

Seems like to begin with you should probably just not support remove.

Yes, I have dropped support for the remove()

quoted

+static int vas_remove(struct platform_device *pdev)
+{
+	struct vas_instance *vinst;
+
+	vinst = dev_get_drvdata(&pdev->dev);
+
+	pr_devel("VAS: Removed instance [%s, %d]\n", pdev->name,
+				vinst->vas_id);
+	free_inst(vinst);
+
+	return 0;
+}
+static const struct of_device_id powernv_vas_match[] = {
+	{ .compatible = "ibm,vas",},
+	{},
+};
+
+static struct platform_driver vas_driver = {
+	.driver = {
+		.name = "vas",
+		.of_match_table = powernv_vas_match,
+	},
+	.probe = vas_probe,
+	.remove = vas_remove,
+};
+
+module_platform_driver(vas_driver);

Can't be a module.

Yes, its now built-in and not a module anymore.

quoted

+
+int vas_init(void)
+{
+	int found = 0;
+	struct device_node *dn;
+
+	for_each_compatible_node(dn, NULL, "ibm,vas") {
+		of_platform_device_create(dn, NULL, NULL);
+		found++;
+	}
+
+	if (!found)
+		return -ENODEV;
+
+	pr_devel("VAS: Found %d instances\n", found);
+	init_done = true;

What does init_done mean?

The way it's currently written it just means there were some ibm,vas
nodes in the device tree. But it doesn't say that we actually probed
them correctly and created vas instances for them.

So it doesn't really tell you much.

Two of the callers of vas_initialized() immediately do a
find_instance(), so they don't really need to call it at all, the lack
of any instances will suffice.

The other two callers are vas_copy_crb() and vas_paste_crb(). The only
use of the former is in nx which doesn't check the return code.

But both should never be called unless we allocated a window anyway.

In short it seems unecessary.

Yes, I have dropped vas_initialized().

quoted

+
+	return 0;
+}
+
+void vas_exit(void)
+{
+	struct list_head *ent;
+	struct vas_instance *vinst;
+
+	list_for_each(ent, &vas_instances) {
+		vinst = list_entry(ent, struct vas_instance, node);
+		of_platform_depopulate(&vinst->pdev->dev);
+	}
+
+	init_done = false;
+}
+
+module_init(vas_init);
+module_exit(vas_exit);
+MODULE_DESCRIPTION("Bare metal IBM Virtual Accelerator Switchboard");
+MODULE_AUTHOR("Sukadev Bhattiprolu [off-list ref]");
+MODULE_LICENSE("GPL");

Can't be a module.

Just a device_initcall() should work for init, you shouldn't need
vas_exit() or any of the other bits.

Yes, fixed.

cheers