Re: [PATCH v7 10/12] powerpc/vas: Define vas_win_close() interface

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

Michael Ellerman [mpe@ellerman.id.au] wrote:
> Hi Suka,
> 
> More comments :)

Thanks!

> 
> Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com> writes:
> 
> > diff --git a/arch/powerpc/platforms/powernv/vas-window.c b/arch/powerpc/platforms/powernv/vas-window.c
> > index 2dd4b63..24288dd 100644
> > --- a/arch/powerpc/platforms/powernv/vas-window.c
> > +++ b/arch/powerpc/platforms/powernv/vas-window.c
> > @@ -879,11 +887,92 @@ struct vas_window *vas_rx_win_open(int vasid, enum vas_cop_type cop,
> >  }
> >  EXPORT_SYMBOL_GPL(vas_rx_win_open);
> >  
> > -/* stub for now */
> > +static void poll_window_busy_state(struct vas_window *window)
> > +{
> > +	int busy;
> > +	uint64_t val;
> > +
> > +retry:
> > +	/*
> > +	 * Poll Window Busy flag
> > +	 */
> > +	val = read_hvwc_reg(window, VREG(WIN_STATUS));
> > +	busy = GET_FIELD(VAS_WIN_BUSY, val);
> > +	if (busy) {
> > +		val = 0;
> > +		schedule_timeout(2000);
> 
> What's 2000?
> 
> That's in jiffies, so it's not a fixed amount of time.
> 
> But on a typical config that will be 20 _seconds_ ?!

Ok. Should I change to that just HZ and

> 
> But you haven't set the task state, so AFAIK it will just return
> instantly.

call set_current_state(TASK_UNINTERRUPTIBLE) before the schedule_timeout()?

> 
> And if there's a software/hardware bug and it never stops being busy,
> then we have a softlockup. The other option would be print a big fat
> warning and just not free the window. But maybe that doesn't work for
> other reasons.
> 
> > +		goto retry;
> > +	}
> > +}
> > +
> > +static void poll_window_castout(struct vas_window *window)
> > +{
> > +	int cached;
> > +	uint64_t val;
> > +
> > +	/* Cast window context out of the cache */
> > +retry:
> > +	val = read_hvwc_reg(window, VREG(WIN_CTX_CACHING_CTL));
> > +	cached = GET_FIELD(VAS_WIN_CACHE_STATUS, val);
> > +	if (cached) {
> > +		val = 0ULL;
> > +		val = SET_FIELD(VAS_CASTOUT_REQ, val, 1);
> > +		val = SET_FIELD(VAS_PUSH_TO_MEM, val, 0);
> > +		write_hvwc_reg(window, VREG(WIN_CTX_CACHING_CTL), val);
> 
> Sigh, I still don't like that macro :)

:-) For one thing, I have used it a lot now and secondly isn't it easier
to know that VAS_CASTOUT_REQ bit is set to 1 without worrying about its
bit position? When debugging, yes we have to ensure VAS_CASTOUT_REQ is
properly defined and we have to work out value in "val".

> 
> or:
> 		write_hvwc_reg(window, VREG(WIN_CTX_CACHING_CTL), 1ull << 63);
> 
> > +
> > +		schedule_timeout(2000);
> > +		goto retry;
> > +	}
> > +}
> > +
> > +/*
> > + * Close a window.
> > + *
> > + * See Section 1.12.1 of VAS workbook v1.05 for details on closing window:
> > + *	- Disable new paste operations (unmap paste address)
> > + *	- Poll for the "Window Busy" bit to be cleared
> > + *	- Clear the Open/Enable bit for the Window.
> > + *	- Poll for return of window Credits (implies FIFO empty for Rx win?)
> > + *	- Unpin and cast window context out of cache
> > + *
> > + * Besides the hardware, kernel has some bookkeeping of course.
> > + */
> >  int vas_win_close(struct vas_window *window)
> >  {
> > -	return -1;
> > +	uint64_t val;
> > +
> > +	if (!window)
> > +		return 0;
> > +
> > +	if (!window->tx_win && atomic_read(&window->num_txwins) != 0) {
> > +		pr_devel("VAS: Attempting to close an active Rx window!\n");
> > +		WARN_ON_ONCE(1);
> > +		return -EAGAIN;
> 
> EAGAIN means "if you do the same thing again it might work".
> 
> I don't think that's right here. The window is not in a state where it
> can be freed, the caller needs to do something to fix that.
> 
> EBUSY would probably be more appropriate.

Ok. Should not happen now (or even with the fast thread-wake up code)
since only the kernel should be closing the windows - so its really a
bug.  Will change to EBUSY though.
> 
> 
> cheers