Hi Marco,
Please find my response inline.
> -----Original Message-----
> From: Marco Pagani <marpagan@xxxxxxxxxx>
> Sent: Thursday, February 9, 2023 3:52 AM
> To: Manne, Nava kishore <nava.kishore.manne@xxxxxxx>
> Cc: Nava kishore Manne <nava.manne@xxxxxxxxxx>; mdf@xxxxxxxxxx;
> hao.wu@xxxxxxxxx; trix@xxxxxxxxxx; yilun.xu@xxxxxxxxx; linux-
> fpga@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH] fpga: mgr: Update the state to provide the exact error
> code
>
>
> 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@xxxxxxxxxx>
> >> Sent: Wednesday, February 8, 2023 12:04 AM
> >> To: Nava kishore Manne <nava.manne@xxxxxxxxxx>
> >> Cc: Manne, Nava kishore <nava.kishore.manne@xxxxxxx>;
> mdf@xxxxxxxxxx;
> >> hao.wu@xxxxxxxxx; trix@xxxxxxxxxx; yilun.xu@xxxxxxxxx;
> >> linux-fpga@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> >> 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@xxxxxxxxxx>
> >>>
> >>> 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@xxxxxxxxxx>
> >>> ---
> >>> 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?
>
AFAIK The state and status interface use cases are different. The Status interface will provide the H/W error info.
whereas the state interface provides the FPGA manager driver state(including Error strings).
Please Refer: Documentation/ABI/testing/sysfs-class-fpga-manager (for Error strings information).
With the existing implementation using DT-Overlay the Configuration/Reconfiguration lower-level
driver errors are not propagating to userspace.
Please correct me if my understanding is wrong.
Regards,
Navakishore.
