Re: [PATCH v7 06/12] powerpc/vas: Define helpers to alloc/free windows

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

Sukadev Bhattiprolu [off-list ref] writes:

quoted

diff --git a/arch/powerpc/platforms/powernv/vas-window.c b/arch/powerpc/platforms/powernv/vas-window.c

quoted

+	rc = ida_pre_get(ida, GFP_KERNEL);
+	if (!rc)
+		return -EAGAIN;
+
+	spin_lock(&vas_ida_lock);
+	rc = ida_get_new_above(ida, 0, &winid);

If you're passing 0 you can just use ida_get_new().

Ok.

Or did you actually want to exclude 0? In which case you should pass 1.

quoted

+	spin_unlock(&vas_ida_lock);
+
+	if (rc)
+		return rc;

You're supposed to handle EAGAIN I thought.

Yes, I will retry the pre_get()

quoted

+
+	if (winid > VAS_WINDOWS_PER_CHIP) {
+		pr_err("VAS: Too many (%d) open windows\n", winid);
+		vas_release_window_id(ida, winid);
+		return -EAGAIN;
+	}
+
+	return winid;
+}
+
+void vas_window_free(struct vas_window *window)

static.

Ok

quoted

+{
+	int winid = window->winid;
+	struct vas_instance *vinst = window->vinst;
+
+	unmap_winctx_mmio_bars(window);
+	kfree(window);
+
+	vas_release_window_id(&vinst->ida, winid);
+}
+
+struct vas_window *vas_window_alloc(struct vas_instance *vinst)
+{
+	int winid;
+	struct vas_window *window;
+
+	winid = vas_assign_window_id(&vinst->ida);
+	if (winid < 0)
+		return ERR_PTR(winid);
+
+	window = kzalloc(sizeof(*window), GFP_KERNEL);
+	if (!window)
+		return ERR_PTR(-ENOMEM);

You leak an id here.

Argh. Yes.

The error handling would be easier in here if the caller did the alloc,
or if you split alloc and init, and alloc just did the kzalloc().

I was trying to simplify error handling in the callers where they have
to only deal with one failure now.

One of the callers even prints "unable to allocate memory" if this
function fails, but that's not accurate, there's several failure modes.

Yes, will fix that message and the leaks.

Thanks,

Suka