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

From: Xu Yilun
Date: Fri Feb 10 2023 - 04:45:59 EST

On 2023-02-08 at 11:01:17 +0000, 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.

Why a user application need to look into these driver implementation
details. If just for debug, there are many existing methods for a
developer to trace the code.

Thanks,
Yilun

> please let me know if you have any other thoughts will implement that.
>
> Regards,
> Navakishore.