| Message ID | 20200827143249.10973-3-luca@lucaceresoli.net (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [v2,1/5] fpga manager: xilinx-spi: remove stray comment | expand |
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@lucaceresoli.net> > > --- > > 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 ? Tom > > dev_err(&mgr->dev, "Timeout after config data transfer\n");
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@lucaceresoli.net> >> >> --- >> >> 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. Does this reply your question?
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@lucaceresoli.net> >>>> >>>> --- >>>> >>>> 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?
On 8/27/20 11:38 PM, Luca Ceresoli wrote: > 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@lucaceresoli.net> >>>>> >>>>> --- >>>>> >>>>> 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? Yes. Thanks for the extra work. Tom >
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); } dev_err(&mgr->dev, "Timeout after config data transfer\n");
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@lucaceresoli.net> --- This patch is new in v2 --- drivers/fpga/xilinx-spi.c | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-)