diff mbox series

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

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

Commit Message

Luca Ceresoli Aug. 30, 2020, 4:38 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>
Reviewed-by: Tom Rix <trix@redhat.com>
Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>

---

Changes in v4:
 - add Reviewed-by Tom Rix

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

Moritz Fischer Aug. 31, 2020, 12:09 a.m. UTC | #1 
On Sun, Aug 30, 2020 at 06:38:49PM +0200, Luca Ceresoli wrote:
> 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>
> Reviewed-by: Tom Rix <trix@redhat.com>
> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
> 
> ---
> 
> Changes in v4:
>  - add Reviewed-by Tom Rix
> 
> 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(-)
> 
> diff --git a/drivers/fpga/xilinx-spi.c b/drivers/fpga/xilinx-spi.c
> index fba8eb4866a7..52aab5a1f0ba 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;
>  	}
> -- 
> 2.28.0
> 
Applied to for-next,

Thanks
diff mbox series

Patch

diff --git a/drivers/fpga/xilinx-spi.c b/drivers/fpga/xilinx-spi.c
index fba8eb4866a7..52aab5a1f0ba 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;
 	}