Re: [PATCH v3 06/10] VAS: Define helpers to alloc/free windows

Michael Neuling [michael.neuling@au1.ibm.com] wrote:

On Thu, 2017-03-16 at 20:33 -0700, Sukadev Bhattiprolu wrote:

quoted

Define helpers to allocate/free VAS window objects. These will
be used in follow-on patches when opening/closing windows.
=20
Signed-off-by: Sukadev Bhattiprolu <redacted>
---
=A0drivers/misc/vas/vas-window.c | 74 +++++++++++++++++++++++++++++++++=

++++++++-

quoted

-
=A01 file changed, 72 insertions(+), 2 deletions(-)
=20

diff --git a/drivers/misc/vas/vas-window.c b/drivers/misc/vas/vas-windo=

w.c

quoted

index edf5c9f..9233bf5 100644

--- a/drivers/misc/vas/vas-window.c
+++ b/drivers/misc/vas/vas-window.c

@@ -119,7 +119,7 @@ static void unmap_wc_mmio_bars(struct vas_window *w=

indow)

quoted

=A0 * OS/User Window Context (UWC) MMIO Base Address Region for the giv=

en

quoted

window.
=A0 * Map these bus addresses and save the mapped kernel addresses in @=

window.

quoted

=A0 */
-int map_wc_mmio_bars(struct vas_window *window)
+static int map_wc_mmio_bars(struct vas_window *window)
=A0{
=A0	int len;
=A0	uint64_t start;

@@ -472,8 +472,78 @@ int init_winctx_regs(struct vas_window *window, st=

ruct

quoted

vas_winctx *winctx)
=A0	return 0;
=A0}
=A0
-/* stub for now */
+DEFINE_SPINLOCK(vas_ida_lock);
+
+void vas_release_window_id(struct ida *ida, int winid)
+{
+	spin_lock(&vas_ida_lock);
+	ida_remove(ida, winid);
+	spin_unlock(&vas_ida_lock);
+}
+
+int vas_assign_window_id(struct ida *ida)
+{
+	int rc, winid;
+
+	rc =3D ida_pre_get(ida, GFP_KERNEL);
+	if (!rc)
+		return -EAGAIN;
+
+	spin_lock(&vas_ida_lock);
+	rc =3D ida_get_new_above(ida, 0, &winid);
+	spin_unlock(&vas_ida_lock);
+
+	if (rc)
+		return rc;
+
+	if (winid > VAS_MAX_WINDOWS_PER_CHIP) {
+		pr_err("VAS: Too many (%d) open windows\n", winid);
+		vas_release_window_id(ida, winid);
+		return -EAGAIN;
+	}
+
+	return winid;
+}
+
+static void vas_window_free(struct vas_window *window)
+{
+	unmap_wc_mmio_bars(window);
+	kfree(window->paste_addr_name);
+	kfree(window);
+}
+
+static struct vas_window *vas_window_alloc(struct vas_instance *vinst,=

 int

quoted

id)
+{
+	struct vas_window *window;
+
+	window =3D kzalloc(sizeof(*window), GFP_KERNEL);
+	if (!window)
+		return NULL;
+
+	window->vinst =3D vinst;
+	window->winid =3D id;
+
+	if (map_wc_mmio_bars(window))
+		goto out_free;
+
+	return window;
+
+out_free:
+	kfree(window);
+	return NULL;
+}
+
=A0int vas_window_reset(struct vas_instance *vinst, int winid)
=20

=20
This interface seems a little weird to me. Needing an alloc in a hardware=

 reset

path seems a bit strange.

Yeah, the name alloc in this interface is awkward.

I probably can drop this interface. Its used only during start up to clear
the window contexts. But since we must and do clear each window context
before using, we don't to do this during start up.

=20
Maybe the data structures are the issue.  A window is a hardware construc=

t.=20

Something that uses it should probably be called something else like a co=

ntext.=20

Something that references a window should just be the vas_instance + wini=

d.=20

=20
You should be able to reset this hardware window by referencing structures
already allocated.  Something associated with the struct vas_instance.
=20

'struct vas_winctx' is the window context (register fields associated
with the window) 'struct vas_window' is a container for the kernel state
associated with a window.

Mikey

Thanks for the review.

Sukadev