[prev in list] [next in list] [prev in thread] [next in thread]
List: linux-fpga
Subject: Re: [PATCH] fpga: mgr: Update the state to provide the exact error code
From: Marco Pagani <marpagan () redhat ! com>
Date: 2023-02-17 12:05:04
Message-ID: ddd17066-ce40-118b-28d8-33e14a3de876 () redhat ! com
[Download RAW message or body]
On 2023-02-13 06:50, Manne, Nava kishore wrote:
> Hi Marco,
>
> Please find my response inline.
>
> > -----Original Message-----
> > From: Marco Pagani <marpagan@redhat.com>
> > Sent: Thursday, February 9, 2023 3:52 AM
> > To: Manne, Nava kishore <nava.kishore.manne@amd.com>
> > Cc: Nava kishore Manne <nava.manne@xilinx.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-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?
> >
>
> 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). \
>
I didn't say that state and status interfaces have the same use case.
Instead, I suggested that the state interface shouldn't be used to
report low-lever errors since this "responsibility" belongs to the
status interface.
The ABI documentation you cited states: "the intent [of the status
interface] is to provide more detailed information for FPGA
programming errors to userspace".
> 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.
Out of curiosity, I ran a couple of tests on the QEMU arm "virt"
platform using linux-xlnx and linux-socfpga kernels and the fake
FPGA manager from the KUnit RFC. I modified the fake manager to
fail the write op returning a specific error code.
static int op_write(struct fpga_manager ...)
{
[...]
return -202302;
}
The op error code is visible from the message log on both kernels when
the overlay application fails:
[ 23.958015] fpga_manager fpga0: writing fake.bin to Fake FPGA Manager
[ 23.959119] fpga_manager fpga0: Error while writing image data to FPGA
[ 23.959341] fpga_region region0: failed to load FPGA image
[ 23.959472] OF: overlay: overlay changeset pre-apply notifier error -202302, \
target: /test_region [ 23.959622] create_overlay: Failed to create overlay \
(err=-202302)
However, I feel this part of the discussion is more speculative since
the DT overlay configFS interface is not mainline.
> Regards,
> Navakishore.
>
Thanks,
Marco
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic