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?
> }
>
> 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
