diff mbox series

[v3,4/5] fpga manager: xilinx-spi: add error checking after gpiod_get_value()

Message ID 20200828195808.27975-4-luca@lucaceresoli.net (mailing list archive)
State New, archived
Headers show
Series [v3,1/5] fpga manager: xilinx-spi: remove stray comment | expand

Commit Message

Luca Ceresoli Aug. 28, 2020, 7:58 p.m. UTC 
Current code calls gpiod_get_value() without error checking. Should the
GPIO controller fail, execution would continue without any error message.

Fix by checking for negative error values.

Reported-by: Tom Rix <trix@redhat.com>
Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>

---

Changes in v3:
 - rebase on previous patches

This patch is new in v2
---
 drivers/fpga/xilinx-spi.c | 35 +++++++++++++++++++++++++++--------
 1 file changed, 27 insertions(+), 8 deletions(-)

Comments

kernel test robot Aug. 29, 2020, 3:30 a.m. UTC | #1 
Hi Luca,

I love your patch! Perhaps something to improve:

[auto build test WARNING on v5.9-rc2]
[also build test WARNING on next-20200828]
[cannot apply to xlnx/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Luca-Ceresoli/fpga-manager-xilinx-spi-remove-stray-comment/20200829-040041
base:    d012a7190fc1fd72ed48911e77ca97ba4521bccd
compiler: nds32le-linux-gcc (GCC) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


cppcheck warnings: (new ones prefixed by >>)

>> drivers/fpga/xilinx-spi.c:183:10: warning: Uninitialized variable: expired [uninitvar]
    while (!expired) {
            ^

# https://github.com/0day-ci/linux/commit/5ae295c0b82631de73665c58df85ec3ed8567a8e
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Luca-Ceresoli/fpga-manager-xilinx-spi-remove-stray-comment/20200829-040041
git checkout 5ae295c0b82631de73665c58df85ec3ed8567a8e
vim +183 drivers/fpga/xilinx-spi.c

061c97d13f1a69 Anatolij Gustschin 2017-03-23  168  
061c97d13f1a69 Anatolij Gustschin 2017-03-23  169  static int xilinx_spi_write_complete(struct fpga_manager *mgr,
061c97d13f1a69 Anatolij Gustschin 2017-03-23  170  				     struct fpga_image_info *info)
061c97d13f1a69 Anatolij Gustschin 2017-03-23  171  {
061c97d13f1a69 Anatolij Gustschin 2017-03-23  172  	struct xilinx_spi_conf *conf = mgr->priv;
629463d1acc532 Luca Ceresoli      2020-08-28  173  	unsigned long timeout = jiffies + usecs_to_jiffies(info->config_complete_timeout_us);
629463d1acc532 Luca Ceresoli      2020-08-28  174  	bool expired;
629463d1acc532 Luca Ceresoli      2020-08-28  175  	int done;
061c97d13f1a69 Anatolij Gustschin 2017-03-23  176  	int ret;
061c97d13f1a69 Anatolij Gustschin 2017-03-23  177  
629463d1acc532 Luca Ceresoli      2020-08-28  178  	/*
629463d1acc532 Luca Ceresoli      2020-08-28  179  	 * This loop is carefully written such that if the driver is
629463d1acc532 Luca Ceresoli      2020-08-28  180  	 * scheduled out for more than 'timeout', we still check for DONE
629463d1acc532 Luca Ceresoli      2020-08-28  181  	 * before giving up and we apply 8 extra CCLK cycles in all cases.
629463d1acc532 Luca Ceresoli      2020-08-28  182  	 */
629463d1acc532 Luca Ceresoli      2020-08-28 @183  	while (!expired) {
629463d1acc532 Luca Ceresoli      2020-08-28  184  		expired = time_after(jiffies, timeout);
061c97d13f1a69 Anatolij Gustschin 2017-03-23  185  
629463d1acc532 Luca Ceresoli      2020-08-28  186  		done = get_done_gpio(mgr);
629463d1acc532 Luca Ceresoli      2020-08-28  187  		if (done < 0)
629463d1acc532 Luca Ceresoli      2020-08-28  188  			return done;
061c97d13f1a69 Anatolij Gustschin 2017-03-23  189  
061c97d13f1a69 Anatolij Gustschin 2017-03-23  190  		ret = xilinx_spi_apply_cclk_cycles(conf);
061c97d13f1a69 Anatolij Gustschin 2017-03-23  191  		if (ret)
061c97d13f1a69 Anatolij Gustschin 2017-03-23  192  			return ret;
061c97d13f1a69 Anatolij Gustschin 2017-03-23  193  
629463d1acc532 Luca Ceresoli      2020-08-28  194  		if (done)
629463d1acc532 Luca Ceresoli      2020-08-28  195  			return 0;
061c97d13f1a69 Anatolij Gustschin 2017-03-23  196  	}
061c97d13f1a69 Anatolij Gustschin 2017-03-23  197  
70cac0e8c9ec83 Luca Ceresoli      2020-08-28  198  	dev_err(&mgr->dev, "Timeout after config data transfer\n");
061c97d13f1a69 Anatolij Gustschin 2017-03-23  199  	return -ETIMEDOUT;
061c97d13f1a69 Anatolij Gustschin 2017-03-23  200  }
061c97d13f1a69 Anatolij Gustschin 2017-03-23  201  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Luca Ceresoli Aug. 29, 2020, 9:08 p.m. UTC | #2 
Hi,

On 29/08/20 05:30, kernel test robot wrote:
> Hi Luca,
> 
> I love your patch! Perhaps something to improve:
> 
> [auto build test WARNING on v5.9-rc2]
> [also build test WARNING on next-20200828]
> [cannot apply to xlnx/master]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
> 
> url:    https://github.com/0day-ci/linux/commits/Luca-Ceresoli/fpga-manager-xilinx-spi-remove-stray-comment/20200829-040041
> base:    d012a7190fc1fd72ed48911e77ca97ba4521bccd
> compiler: nds32le-linux-gcc (GCC) 9.3.0
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> 
> 
> cppcheck warnings: (new ones prefixed by >>)
> 
>>> drivers/fpga/xilinx-spi.c:183:10: warning: Uninitialized variable: expired [uninitvar]
>     while (!expired) {
>             ^

Oh dear, Please ignore this patch, v4 will be coming with this fixed.
diff mbox series

Patch

diff --git a/drivers/fpga/xilinx-spi.c b/drivers/fpga/xilinx-spi.c
index a7b919eb0b2a..9488c8fbaefd 100644
--- a/drivers/fpga/xilinx-spi.c
+++ b/drivers/fpga/xilinx-spi.c
@@ -27,11 +27,22 @@  struct xilinx_spi_conf {
 	struct gpio_desc *done;
 };
 
-static enum fpga_mgr_states xilinx_spi_state(struct fpga_manager *mgr)
+static int get_done_gpio(struct fpga_manager *mgr)
 {
 	struct xilinx_spi_conf *conf = mgr->priv;
+	int ret;
+
+	ret = gpiod_get_value(conf->done);
+
+	if (ret < 0)
+		dev_err(&mgr->dev, "Error reading DONE (%d)\n", ret);
 
-	if (!gpiod_get_value(conf->done))
+	return ret;
+}
+
+static enum fpga_mgr_states xilinx_spi_state(struct fpga_manager *mgr)
+{
+	if (!get_done_gpio(mgr))
 		return FPGA_MGR_STATE_RESET;
 
 	return FPGA_MGR_STATE_UNKNOWN;
@@ -57,10 +68,21 @@  static int wait_for_init_b(struct fpga_manager *mgr, int value,
 
 	if (conf->init_b) {
 		while (time_before(jiffies, timeout)) {
-			if (gpiod_get_value(conf->init_b) == value)
+			int ret = gpiod_get_value(conf->init_b);
+
+			if (ret == value)
 				return 0;
+
+			if (ret < 0) {
+				dev_err(&mgr->dev, "Error reading INIT_B (%d)\n", ret);
+				return ret;
+			}
+
 			usleep_range(100, 400);
 		}
+
+		dev_err(&mgr->dev, "Timeout waiting for INIT_B to %s\n",
+			value ? "assert" : "deassert");
 		return -ETIMEDOUT;
 	}
 
@@ -85,7 +107,6 @@  static int xilinx_spi_write_init(struct fpga_manager *mgr,
 
 	err = wait_for_init_b(mgr, 1, 1); /* min is 500 ns */
 	if (err) {
-		dev_err(&mgr->dev, "INIT_B pin did not go low\n");
 		gpiod_set_value(conf->prog_b, 0);
 		return err;
 	}
@@ -93,12 +114,10 @@  static int xilinx_spi_write_init(struct fpga_manager *mgr,
 	gpiod_set_value(conf->prog_b, 0);
 
 	err = wait_for_init_b(mgr, 0, 0);
-	if (err) {
-		dev_err(&mgr->dev, "INIT_B pin did not go high\n");
+	if (err)
 		return err;
-	}
 
-	if (gpiod_get_value(conf->done)) {
+	if (get_done_gpio(mgr)) {
 		dev_err(&mgr->dev, "Unexpected DONE pin state...\n");
 		return -EIO;
 	}