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

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

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>
---
=C2=A0drivers/misc/vas/vas-window.c | 74 ++++++++++++++++++++++++++++++++=

+++++++++-

c

dow)

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

ven

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

@window.

ct

vas_winctx *winctx)
=C2=A0	return 0;
=C2=A0}
=C2=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, i=

nt

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;
+}
+
=C2=A0int vas_window_reset(struct vas_instance *vinst, int winid)
=20

This interface seems a little weird to me. Needing an alloc in a hardware r=
eset
path seems a bit strange.

Maybe the data structures are the issue.  A window is a hardware construct.=
=20
Something that uses it should probably be called something else like a cont=
ext.=20
Something that references a window should just be the vas_instance + winid.=
=20

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

Mikey

=C2=A0{
+	struct vas_window *window;
+
+	window =3D vas_window_alloc(vinst, winid);
+	if (!window)
+		return -ENOMEM;
+
+	reset_window_regs(window);
+
+	vas_window_free(window);
+
=C2=A0	return 0;
=C2=A0}