Message-ID: <8d34bc43-deb4-4166-83ad-34561ee5ac33@redhat.com>
Date:   Tue, 7 Feb 2023 19:33:40 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.1
Subject: Re: [PATCH] fpga: mgr: Update the state to provide the exact error
 code
Content-Language: en-US
To:     Nava kishore Manne <nava.manne@xilinx.com>
Cc:     Nava kishore Manne <nava.kishore.manne@amd.com>, mdf@kernel.org,
        hao.wu@intel.com, trix@redhat.com, yilun.xu@intel.com,
        linux-fpga@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20230207095915.169146-1-nava.kishore.manne@amd.com>
From:   Marco Pagani <marpagan@redhat.com>
In-Reply-To: <20230207095915.169146-1-nava.kishore.manne@amd.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 2023-02-07 10:59, Nava kishore Manne wrote:
> From: Nava kishore Manne <nava.manne@xilinx.com>
> 
> Up on fpga configuration failure, the existing sysfs state interface
> is just providing the generic error message rather than providing the
> exact error code. This patch extends sysfs state interface to provide
> the exact error received from the lower layer along with the existing
> generic error message.
> 
> Signed-off-by: Nava kishore Manne <nava.manne@xilinx.com>
> ---
>  drivers/fpga/fpga-mgr.c       | 20 +++++++++++++++++++-
>  include/linux/fpga/fpga-mgr.h |  2 ++
>  2 files changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
> index 8efa67620e21..b2d74705a5a2 100644
> --- a/drivers/fpga/fpga-mgr.c
> +++ b/drivers/fpga/fpga-mgr.c
> @@ -61,12 +61,14 @@ static inline int fpga_mgr_write_complete(struct fpga_manager *mgr,
>  {
>  	int ret = 0;
>  
> +	mgr->err = 0;
>  	mgr->state = FPGA_MGR_STATE_WRITE_COMPLETE;
>  	if (mgr->mops->write_complete)
>  		ret = mgr->mops->write_complete(mgr, info);
>  	if (ret) {
>  		dev_err(&mgr->dev, "Error after writing image data to FPGA\n");
>  		mgr->state = FPGA_MGR_STATE_WRITE_COMPLETE_ERR;
> +		mgr->err = ret;
>  		return ret;
>  	}
>  	mgr->state = FPGA_MGR_STATE_OPERATING;
> @@ -154,6 +156,7 @@ static int fpga_mgr_parse_header_mapped(struct fpga_manager *mgr,
>  {
>  	int ret;
>  
> +	mgr->err = 0;
>  	mgr->state = FPGA_MGR_STATE_PARSE_HEADER;
>  	ret = fpga_mgr_parse_header(mgr, info, buf, count);
>  
> @@ -165,6 +168,7 @@ static int fpga_mgr_parse_header_mapped(struct fpga_manager *mgr,
>  	if (ret) {
>  		dev_err(&mgr->dev, "Error while parsing FPGA image header\n");
>  		mgr->state = FPGA_MGR_STATE_PARSE_HEADER_ERR;
> +		mgr->err = ret;
>  	}
>  
>  	return ret;
> @@ -185,6 +189,7 @@ static int fpga_mgr_parse_header_sg_first(struct fpga_manager *mgr,
>  	int ret;
>  
>  	mgr->state = FPGA_MGR_STATE_PARSE_HEADER;
> +	mgr->err = 0;
>  
>  	sg_miter_start(&miter, sgt->sgl, sgt->nents, SG_MITER_FROM_SG);
>  	if (sg_miter_next(&miter) &&
> @@ -197,6 +202,7 @@ static int fpga_mgr_parse_header_sg_first(struct fpga_manager *mgr,
>  	if (ret && ret != -EAGAIN) {
>  		dev_err(&mgr->dev, "Error while parsing FPGA image header\n");
>  		mgr->state = FPGA_MGR_STATE_PARSE_HEADER_ERR;
> +		mgr->err = ret;
>  	}
>  
>  	return ret;
> @@ -249,6 +255,7 @@ static void *fpga_mgr_parse_header_sg(struct fpga_manager *mgr,
>  	if (ret) {
>  		dev_err(&mgr->dev, "Error while parsing FPGA image header\n");
>  		mgr->state = FPGA_MGR_STATE_PARSE_HEADER_ERR;
> +		mgr->err = ret;
>  		kfree(buf);
>  		buf = ERR_PTR(ret);
>  	}
> @@ -272,6 +279,7 @@ static int fpga_mgr_write_init_buf(struct fpga_manager *mgr,
>  	size_t header_size = info->header_size;
>  	int ret;
>  
> +	mgr->err = 0;
>  	mgr->state = FPGA_MGR_STATE_WRITE_INIT;
>  
>  	if (header_size > count)
> @@ -284,6 +292,7 @@ static int fpga_mgr_write_init_buf(struct fpga_manager *mgr,
>  	if (ret) {
>  		dev_err(&mgr->dev, "Error preparing FPGA for writing\n");
>  		mgr->state = FPGA_MGR_STATE_WRITE_INIT_ERR;
> +		mgr->err = ret;
>  		return ret;
>  	}
>  
> @@ -370,6 +379,7 @@ static int fpga_mgr_buf_load_sg(struct fpga_manager *mgr,
>  
>  	/* Write the FPGA image to the FPGA. */
>  	mgr->state = FPGA_MGR_STATE_WRITE;
> +	mgr->err = 0;
>  	if (mgr->mops->write_sg) {
>  		ret = fpga_mgr_write_sg(mgr, sgt);
>  	} else {
> @@ -405,6 +415,7 @@ static int fpga_mgr_buf_load_sg(struct fpga_manager *mgr,
>  	if (ret) {
>  		dev_err(&mgr->dev, "Error while writing image data to FPGA\n");
>  		mgr->state = FPGA_MGR_STATE_WRITE_ERR;
> +		mgr->err = ret;
>  		return ret;
>  	}
>  
> @@ -437,10 +448,12 @@ static int fpga_mgr_buf_load_mapped(struct fpga_manager *mgr,
>  	 * Write the FPGA image to the FPGA.
>  	 */
>  	mgr->state = FPGA_MGR_STATE_WRITE;
> +	mgr->err = 0;
>  	ret = fpga_mgr_write(mgr, buf, count);
>  	if (ret) {
>  		dev_err(&mgr->dev, "Error while writing image data to FPGA\n");
>  		mgr->state = FPGA_MGR_STATE_WRITE_ERR;
> +		mgr->err = ret;
>  		return ret;
>  	}
>  
> @@ -544,10 +557,11 @@ static int fpga_mgr_firmware_load(struct fpga_manager *mgr,
>  	dev_info(dev, "writing %s to %s\n", image_name, mgr->name);
>  
>  	mgr->state = FPGA_MGR_STATE_FIRMWARE_REQ;
> -
> +	mgr->err = 0;
>  	ret = request_firmware(&fw, image_name, dev);
>  	if (ret) {
>  		mgr->state = FPGA_MGR_STATE_FIRMWARE_REQ_ERR;
> +		mgr->err = ret;
>  		dev_err(dev, "Error requesting firmware %s\n", image_name);
>  		return ret;
>  	}
> @@ -626,6 +640,10 @@ static ssize_t state_show(struct device *dev,
>  {
>  	struct fpga_manager *mgr = to_fpga_manager(dev);
>  
> +	if (mgr->err)
> +		return sprintf(buf, "%s: 0x%x\n",
> +			       state_str[mgr->state], mgr->err);
> +
>  	return sprintf(buf, "%s\n", state_str[mgr->state]);


If one of the fpga manager ops fails, the low-level error code is already
returned to the caller. Wouldn't it be better to rely on this instead of
printing the low-level error code in a sysfs attribute and sending it to
the userspace?


>  }
>  
> diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
> index 54f63459efd6..be3a426ef903 100644
> --- a/include/linux/fpga/fpga-mgr.h
> +++ b/include/linux/fpga/fpga-mgr.h
> @@ -202,6 +202,7 @@ struct fpga_manager_ops {
>   * @compat_id: FPGA manager id for compatibility check.
>   * @mops: pointer to struct of fpga manager ops
>   * @priv: low level driver private date
> + * @err: low level driver error code
>   */
>  struct fpga_manager {
>  	const char *name;
> @@ -211,6 +212,7 @@ struct fpga_manager {
>  	struct fpga_compat_id *compat_id;
>  	const struct fpga_manager_ops *mops;
>  	void *priv;
> +	int err;
>  };
>  
>  #define to_fpga_manager(d) container_of(d, struct fpga_manager, dev)


Thanks,
Marco