RE: [PATCHv3 4/4] crypto: inside-secure - add support for using the EIP197 without vendor firmware

[Pascal Van Leeuwen <pvanleeuwen@verimatrix.com>]

> -----Original Message-----
> From: Antoine Tenart <antoine.tenart@bootlin.com>
> Sent: Monday, August 5, 2019 2:43 PM
> To: Pascal Van Leeuwen <pvanleeuwen@verimatrix.com>
> Cc: Antoine Tenart <antoine.tenart@bootlin.com>; Pascal van Leeuwen
> <pascalvanl@gmail.com>; linux-crypto@vger.kernel.org; herbert@gondor.apana.org.au;
> davem@davemloft.net
> Subject: Re: [PATCHv3 4/4] crypto: inside-secure - add support for using the EIP197
> without vendor firmware
> 
> On Mon, Aug 05, 2019 at 10:12:07AM +0000, Pascal Van Leeuwen wrote:
> > > From: Antoine Tenart <antoine.tenart@bootlin.com>
> > > On Mon, Aug 05, 2019 at 09:48:13AM +0000, Pascal Van Leeuwen wrote:
> > > > > On Wed, Jul 31, 2019 at 05:29:19PM +0200, Pascal van Leeuwen wrote:
> > > > > >
> > > > > > -	/* Release engine from reset */
> > > > > > -	val = readl(EIP197_PE(priv) + ctrl);
> > > > > > -	val &= ~EIP197_PE_ICE_x_CTRL_SW_RESET;
> > > > > > -	writel(val, EIP197_PE(priv) + ctrl);
> > > > > > +	for (pe = 0; pe < priv->config.pes; pe++) {
> > > > > > +		base = EIP197_PE_ICE_SCRATCH_RAM(pe);
> > > > > > +		pollcnt = EIP197_FW_START_POLLCNT;
> > > > > > +		while (pollcnt &&
> > > > > > +		       (readl_relaxed(EIP197_PE(priv) + base +
> > > > > > +			      pollofs) != 1)) {
> > > > > > +			pollcnt--;
> > > > >
> > > > > You might want to use readl_relaxed_poll_timeout() here, instead of a
> > > > > busy polling.
> > > > >
> > > > Didn't know such a thing existed, but I also wonder how appropriate it
> > > > is in this case, condering it measures in whole microseconds, while the
> > > > response time I'm expecting here is in the order of a few dozen nano-
> > > > seconds internally ... i.e. 1 microsecond is already a *huge* overkill.
> > > >
> > > > The current implementation runs that loop for only 16 iterations which
> > > > should be both more than sufficient (it probably could be reduced
> > > > further, I picked 16 rather arbitrarily) and at the same time take so
> > > > few cycles on the CPU that I doubt it is worthwhile to reschedule/
> > > > preempt/whatever?
> > >
> > > Your choice, I was just making a suggestion :)
> > >
> > After reading the implementation code, I think using it with a
> > sleep_us value of 0 and a time-out value of 1 may be reasonable here,
> > as time-out means some serious error that shouldn't normally happen.
> > I just wouldn't want any microsecond delays for the "normal" case.
> 
> Using this function that is designed to sleep with a delay of 0 and
> designed to timeout with a value of 1 does not seem to follow what the
> function is designed for :) It could work but I would suggest to keep
> the polling as it is in the patch in this case.
> 
Ok, will do. Does that mean that patch 4/4 is good as is?
i.e. can I add an "Acked-by: Antoine Tenart" line when I resubmit?

> Thanks,
> Antoine
> 
> --
> Antoine Ténart, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

Regards,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
www.insidesecure.com