Re: [PATCH] fpga: mgr: Update the state to provide the exact error code

[Marco Pagani <marpagan@redhat.com>]

On 2023-02-08 12:01, Manne, Nava kishore wrote:
> Hi Marco,
> 
> 	Thanks for providing the review comments.
> Please find my response inline below.
> 
>> -----Original Message-----
>> From: Marco Pagani <marpagan@redhat.com>
>> Sent: Wednesday, February 8, 2023 12:04 AM
>> To: Nava kishore Manne <nava.manne@xilinx.com>
>> Cc: Manne, Nava kishore <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
>> Subject: Re: [PATCH] fpga: mgr: Update the state to provide the exact error
>> code
>>
>>
>> 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?
>>
> Agree, the low-level error code is already returned to the caller but the user application
> will not have any access to read this error info. So, I feel this patch provides that flexibility 
> to the user application to get the exact error info. 
> please let me know if you have any other thoughts will implement that.
> 
> Regards,
> Navakishore.


Hi Nava,

Thanks for your quick reply. I understand the need to access the
low-level error code from userspace if the configuration goes wrong.

However, in my understanding, the low-level driver is supposed to
export reconfiguration errors by implementing the status op and
returning a bit field set using the macros defined in fpga-mgr.h +189.
The fpga manager will, in turn, make the errors visible to userspace
through the status attribute. If the available error bits aren't
descriptive enough, wouldn't it be better to add more error macros
instead of "overloading" the state attribute?

Moreover, it seems to me that if the reconfiguration is done by
loading a device tree overlay from userspace, the error code gets
propagated back through the notifier in of-fpga-region. Am I correct?

Thanks,
Marco