Re: [PATCH v2 3/5] fpga manager: xilinx-spi: rework write_complete loop implementation

[Luca Ceresoli]

Hi Tom,

On 27/08/20 21:34, Tom Rix wrote:
> 
> On 8/27/20 12:26 PM, Luca Ceresoli wrote:
>> Hi Tom,
>>
>> thanks for the prompt feedback!
>>
>> On 27/08/20 20:59, Tom Rix wrote:
>>> On 8/27/20 7:32 AM, Luca Ceresoli wrote:
>>>> In preparation to add error checking for gpiod_get_value(), rework
>>>> the loop to avoid the duplication of these lines:
>>>>
>>>> 	if (gpiod_get_value(conf->done))
>>>> 		return xilinx_spi_apply_cclk_cycles(conf);
>>>>
>>>> There is little advantage in this rework with current code. However
>>>> error checking will expand these two lines to five, making code
>>>> duplication more annoying.
>>>>
>>>> Signed-off-by: Luca Ceresoli <luca@xxxxxxxxxxxxxxxx>
>>>>
>>>> ---
>>>>
>>>> This patch is new in v2
>>>> ---
>>>>  drivers/fpga/xilinx-spi.c | 15 ++++++---------
>>>>  1 file changed, 6 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/drivers/fpga/xilinx-spi.c b/drivers/fpga/xilinx-spi.c
>>>> index 01f494172379..cfc933d70f52 100644
>>>> --- a/drivers/fpga/xilinx-spi.c
>>>> +++ b/drivers/fpga/xilinx-spi.c
>>>> @@ -151,22 +151,19 @@ static int xilinx_spi_write_complete(struct fpga_manager *mgr,
>>>>  				     struct fpga_image_info *info)
>>>>  {
>>>>  	struct xilinx_spi_conf *conf = mgr->priv;
>>>> -	unsigned long timeout;
>>>> +	unsigned long timeout = jiffies + usecs_to_jiffies(info->config_complete_timeout_us);
>>>>  	int ret;
>>>>  
>>>> -	if (gpiod_get_value(conf->done))
>>>> -		return xilinx_spi_apply_cclk_cycles(conf);
>>>> -
>>>> -	timeout = jiffies + usecs_to_jiffies(info->config_complete_timeout_us);
>>>> +	while (true) {
>>>> +		if (gpiod_get_value(conf->done))
>>>> +			return xilinx_spi_apply_cclk_cycles(conf);
>>>>  
>>>> -	while (time_before(jiffies, timeout)) {
>>>> +		if (time_after(jiffies, timeout))
>>>> +			break;
>>>>  
>>>>  		ret = xilinx_spi_apply_cclk_cycles(conf);
>>>>  		if (ret)
>>>>  			return ret;
>>>> -
>>>> -		if (gpiod_get_value(conf->done))
>>>> -			return xilinx_spi_apply_cclk_cycles(conf);
>>>>  	} 
>>> Do you need another
>>>
>>> 	if (gpiod_get_value(conf->done))
>>> 		return xilinx_spi_apply_cclk_cycles(conf);
>>>
>>> here to cover the chance of sleeping in the loop ?
>> If I got your question correctly: if we get here it's because of a
>> timeout, thus programming has failed (DONE didn't come up after some
>> time), and checking it one more here seems pointless.
> 
> It may not be pointless, if this routine sleeps because it was scheduled out, when it wakes up a lot of time  happened. You will see this as a timeout but the state may be good.  Another, final check at the end will cover this case.

Oh, now I got your point! Yes, there is this risk, and it exists in
current code as well but with a smaller risk window. Unrolling the
current and new loop code they behave the same except for the position
of the timeout computation (after vs before the first 'if (done) return'
group).

I think this reimplementation is sleep-safe, check for GPIO errors and
also avoid code duplication:

static int xilinx_spi_write_complete(struct fpga_manager *mgr,
				     struct fpga_image_info *info)
{
	struct xilinx_spi_conf *conf = mgr->priv;
	unsigned long timeout = jiffies +
		usecs_to_jiffies(info->config_complete_timeout_us);
	bool expired;
	int done;
	int ret;

	while (!expired) {
		expired = time_after(jiffies, timeout);

		done = get_done_gpio(mgr);
		if (done < 0)
			return done;

		ret = xilinx_spi_apply_cclk_cycles(conf);
		if (ret)
			return ret;

		if (done)
			return 0;
	}

	dev_err(&mgr->dev, "Timeout after config data transfer\n");

	return -ETIMEDOUT;
}

A key point is to assess all the status (expired and done variables)
before taking any action based on it. Then we can unconditionally apply
8 cclk cycles before even checking the actual DONE value, so that we
always do that after DONE has been seen asserted.

Does it look good?

-- 
Luca