| Message ID | 20200830163850.8380-3-luca@lucaceresoli.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4,1/5] fpga manager: xilinx-spi: remove stray comment | expand |
On Sun, Aug 30, 2020 at 06:38:48PM +0200, Luca Ceresoli wrote: > If this routine sleeps because it was scheduled out, it might miss DONE > going asserted and consider it a timeout. This would potentially make the > code return an error even when programming succeeded. Rewrite the loop to > always check DONE after checking if timeout expired so this cannot happen > anymore. > > While there, also add error checking for gpiod_get_value(). Also avoid > checking the DONE GPIO in two places, which would make the error-checking > code duplicated and more annoying. > > The new loop it written to still guarantee that we apply 8 extra CCLK > cycles after DONE has gone asserted, which is required by the hardware. > > Reported-by: Tom Rix <trix@redhat.com> > Reviewed-by: Tom Rix <trix@redhat.com> > Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net> > > --- > > Changes in v4: > - add Reviewed-by Tom Rix > - fix uninitialized variable > (Reported-by: kernel test robot <lkp@intel.com>) > > Changes in v3: > - completely rewrite the loop after Tom pointed out the 'sleep' bug > > This patch is new in v2 > --- > drivers/fpga/xilinx-spi.c | 23 +++++++++++++++-------- > 1 file changed, 15 insertions(+), 8 deletions(-) > > diff --git a/drivers/fpga/xilinx-spi.c b/drivers/fpga/xilinx-spi.c > index 01f494172379..fba8eb4866a7 100644 > --- a/drivers/fpga/xilinx-spi.c > +++ b/drivers/fpga/xilinx-spi.c > @@ -151,22 +151,29 @@ 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); > + bool expired = false; > + int done; > int ret; > > - if (gpiod_get_value(conf->done)) > - return xilinx_spi_apply_cclk_cycles(conf); > + /* > + * This loop is carefully written such that if the driver is > + * scheduled out for more than 'timeout', we still check for DONE > + * before giving up and we apply 8 extra CCLK cycles in all cases. > + */ > + while (!expired) { > + expired = time_after(jiffies, timeout); > > - timeout = jiffies + usecs_to_jiffies(info->config_complete_timeout_us); > - > - while (time_before(jiffies, timeout)) { > + done = get_done_gpio(mgr); > + if (done < 0) > + return done; > > ret = xilinx_spi_apply_cclk_cycles(conf); > if (ret) > return ret; > > - if (gpiod_get_value(conf->done)) > - return xilinx_spi_apply_cclk_cycles(conf); > + if (done) > + return 0; > } > > dev_err(&mgr->dev, "Timeout after config data transfer\n"); > -- > 2.28.0 > Applied to for-next, Thanks
diff --git a/drivers/fpga/xilinx-spi.c b/drivers/fpga/xilinx-spi.c index 01f494172379..fba8eb4866a7 100644 --- a/drivers/fpga/xilinx-spi.c +++ b/drivers/fpga/xilinx-spi.c @@ -151,22 +151,29 @@ 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); + bool expired = false; + int done; int ret; - if (gpiod_get_value(conf->done)) - return xilinx_spi_apply_cclk_cycles(conf); + /* + * This loop is carefully written such that if the driver is + * scheduled out for more than 'timeout', we still check for DONE + * before giving up and we apply 8 extra CCLK cycles in all cases. + */ + while (!expired) { + expired = time_after(jiffies, timeout); - timeout = jiffies + usecs_to_jiffies(info->config_complete_timeout_us); - - while (time_before(jiffies, timeout)) { + done = get_done_gpio(mgr); + if (done < 0) + return done; ret = xilinx_spi_apply_cclk_cycles(conf); if (ret) return ret; - if (gpiod_get_value(conf->done)) - return xilinx_spi_apply_cclk_cycles(conf); + if (done) + return 0; } dev_err(&mgr->dev, "Timeout after config data transfer\n");