From: Moritz Fischer <mdf@kernel.org>
To: Luca Ceresoli <luca@lucaceresoli.net>
Cc: linux-fpga@vger.kernel.org, Moritz Fischer <mdf@kernel.org>,
Tom Rix <trix@redhat.com>, Michal Simek <michal.simek@xilinx.com>,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, Anatolij Gustschin <agust@denx.de>
Subject: Re: [PATCH v4 3/5] fpga manager: xilinx-spi: fix write_complete timeout handling
Date: Sun, 30 Aug 2020 17:07:42 -0700 [thread overview]
Message-ID: <20200831000742.GC7421@epycbox.lan> (raw)
In-Reply-To: <20200830163850.8380-3-luca@lucaceresoli.net>
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
next prev parent reply other threads:[~2020-08-31 0:07 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-08-30 16:38 [PATCH v4 1/5] fpga manager: xilinx-spi: remove stray comment Luca Ceresoli
2020-08-30 16:38 ` [PATCH v4 2/5] fpga manager: xilinx-spi: remove final dot from dev_err() strings Luca Ceresoli
2020-08-31 0:05 ` Moritz Fischer
2020-08-30 16:38 ` [PATCH v4 3/5] fpga manager: xilinx-spi: fix write_complete timeout handling Luca Ceresoli
2020-08-31 0:07 ` Moritz Fischer [this message]
2020-08-30 16:38 ` [PATCH v4 4/5] fpga manager: xilinx-spi: add error checking after gpiod_get_value() Luca Ceresoli
2020-08-31 0:09 ` Moritz Fischer
2020-08-30 16:38 ` [PATCH v4 5/5] fpga manager: xilinx-spi: provide better diagnostics on programming failure Luca Ceresoli
2020-08-31 0:09 ` Moritz Fischer
2020-08-31 0:05 ` [PATCH v4 1/5] fpga manager: xilinx-spi: remove stray comment Moritz Fischer
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200831000742.GC7421@epycbox.lan \
--to=mdf@kernel.org \
--cc=agust@denx.de \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-fpga@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=luca@lucaceresoli.net \
--cc=michal.simek@xilinx.com \
--cc=trix@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).