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

[Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>]

Michael Neuling [michael.neuling@au1.ibm.com] wrote:
> 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 <sukadev@linux.vnet.ibm.com>
> > ---
> > =A0drivers/misc/vas/vas-window.c | 74 +++++++++++++++++++++++++++++++++=
++++++++-
> > -
> > =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
> > 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)
> > =A0 * OS/User Window Context (UWC) MMIO Base Address Region for the giv=
en
> > window.
> > =A0 * Map these bus addresses and save the mapped kernel addresses in @=
window.
> > =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
> > 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
> > 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