Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Received-SPF: Pass (protection.outlook.com: domain of xilinx.com designates
 149.199.62.198 as permitted sender) receiver=protection.outlook.com;
 client-ip=149.199.62.198; helo=xsj-pvapexch02.xlnx.xilinx.com;
Envelope-to: matthew.gerlach@intel.com,
 hao.wu@intel.com,
 yilun.xu@intel.com,
 lgoncalv@redhat.com,
 trix@redhat.com,
 linux-kernel@vger.kernel.org,
 linux-fpga@vger.kernel.org,
 mdf@kernel.org,
 russell.h.weight@intel.com
Subject: Re: [PATCH v17 4/5] fpga: image-load: add status ioctl
To:     Russ Weight <russell.h.weight@intel.com>,
        Lizhi Hou <lizhi.hou@xilinx.com>, <mdf@kernel.org>,
        <linux-fpga@vger.kernel.org>, <linux-kernel@vger.kernel.org>
CC:     <trix@redhat.com>, <lgoncalv@redhat.com>, <yilun.xu@intel.com>,
        <hao.wu@intel.com>, <matthew.gerlach@intel.com>
References: <20210929230025.68961-1-russell.h.weight@intel.com>
 <20210929230025.68961-5-russell.h.weight@intel.com>
 <97bb3fb5-3807-386d-d2c8-fec9c077a255@xilinx.com>
 <1e94cff5-ed53-bab7-1bb1-321ba833ea1e@intel.com>
From:   Lizhi Hou <lizhi.hou@xilinx.com>
Message-ID: <bbcb902c-49e4-75e1-6e8e-4e791d8c6537@xilinx.com>
Date:   Mon, 25 Oct 2021 17:07:02 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.9.1
MIME-Version: 1.0
In-Reply-To: <1e94cff5-ed53-bab7-1bb1-321ba833ea1e@intel.com>
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 26 Oct 2021 00:07:04.4587
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: e0625bae-3b98-421f-9fe8-08d998148ada
X-MS-Exchange-CrossTenant-Id: 657af505-d5df-48d0-8300-c31994686c5c
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=657af505-d5df-48d0-8300-c31994686c5c;Ip=[149.199.62.198];Helo=[xsj-pvapexch02.xlnx.xilinx.com]
X-MS-Exchange-CrossTenant-AuthSource: BN1NAM02FT026.eop-nam02.prod.protection.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL0PR02MB3889
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 10/20/21 2:42 PM, Russ Weight wrote:
> On 10/15/21 1:22 PM, Lizhi Hou wrote:
>> On 9/29/21 4:00 PM, Russ Weight wrote:
>>> Extend the FPGA Image Load framework to include an FPGA_IMAGE_LOAD_STATUS
>>> IOCTL that can be used to monitor the progress of an ongoing image upload.
>>> The status returned includes how much data remains to be transferred, the
>>> progress of the image upload, and error information in the case of a
>>> failure.
>>>
>>> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
>>> ---
>>> v17:
>>>    - Rebased for changes to earlier patches.
>>> v16:
>>>    - Minor changes to adapt in changes in prevoius patches.
>>> v15:
>>>    - This patch is new to the patchset and provides an FPGA_IMAGE_LOAD_STATUS
>>>      IOCTL to return the current values for: remaining_size, progress,
>>>      err_progress, and err_code.
>>>    - This patch has elements of the following three patches from the previous
>>>      patch-set:
>>>        [PATCH v14 3/6] fpga: sec-mgr: expose sec-mgr update status
>>>        [PATCH v14 4/6] fpga: sec-mgr: expose sec-mgr update errors
>>>        [PATCH v14 5/6] fpga: sec-mgr: expose sec-mgr update size
>>>    - Changed file, symbol, and config names to reflect the new driver name
>>>    - There are some minor changes to locking to enable this ioctl to return
>>>      coherent data.
>>> ---
>>>    Documentation/fpga/fpga-image-load.rst |  6 +++
>>>    drivers/fpga/fpga-image-load.c         | 58 +++++++++++++++++++++-----
>>>    include/linux/fpga/fpga-image-load.h   |  1 +
>>>    include/uapi/linux/fpga-image-load.h   | 18 ++++++++
>>>    4 files changed, 73 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/Documentation/fpga/fpga-image-load.rst b/Documentation/fpga/fpga-image-load.rst
>>> index 487b5466f67c..f64f5ee473b8 100644
>>> --- a/Documentation/fpga/fpga-image-load.rst
>>> +++ b/Documentation/fpga/fpga-image-load.rst
>>> @@ -33,3 +33,9 @@ being updated. This is an exclusive operation; an attempt to start
>>>    concurrent image uploads for the same device will fail with EBUSY. An
>>>    eventfd file descriptor parameter is provided to this IOCTL. It will be
>>>    signalled at the completion of the image upload.
>>> +
>>> +FPGA_IMAGE_LOAD_STATUS:
>>> +
>>> +Collect status for an on-going image upload. The status returned includes
>>> +how much data remains to be transferred, the progress of the image upload,
>>> +and error information in the case of a failure.
>>> diff --git a/drivers/fpga/fpga-image-load.c b/drivers/fpga/fpga-image-load.c
>>> index f04dfc71c190..58373b9e8c02 100644
>>> --- a/drivers/fpga/fpga-image-load.c
>>> +++ b/drivers/fpga/fpga-image-load.c
>>> @@ -22,6 +22,22 @@ static dev_t fpga_image_devt;
>>>
>>>    #define to_image_load(d) container_of(d, struct fpga_image_load, dev)
>>>
>>> +static void fpga_image_update_progress(struct fpga_image_load *imgld,
>>> +                                      u32 new_progress)
>>> +{
>>> +       mutex_lock(&imgld->lock);
>>> +       imgld->progress = new_progress;
>>> +       mutex_unlock(&imgld->lock);
>>> +}
>>> +
>>> +static void fpga_image_set_error(struct fpga_image_load *imgld, u32 err_code)
>>> +{
>>> +       mutex_lock(&imgld->lock);
>>> +       imgld->err_progress = imgld->progress;
>>> +       imgld->err_code = err_code;
>>> +       mutex_unlock(&imgld->lock);
>>> +}
>>> +
>>>    static void fpga_image_prog_complete(struct fpga_image_load *imgld)
>>>    {
>>>           mutex_lock(&imgld->lock);
>>> @@ -38,24 +54,24 @@ static void fpga_image_do_load(struct work_struct *work)
>>>           imgld = container_of(work, struct fpga_image_load, work);
>>>
>>>           if (imgld->driver_unload) {
>>> -               imgld->err_code = FPGA_IMAGE_ERR_CANCELED;
>>> +               fpga_image_set_error(imgld, FPGA_IMAGE_ERR_CANCELED);
>>>                   goto idle_exit;
>>>           }
>>>
>>>           get_device(&imgld->dev);
>>>           if (!try_module_get(imgld->dev.parent->driver->owner)) {
>>> -               imgld->err_code = FPGA_IMAGE_ERR_BUSY;
>>> +               fpga_image_set_error(imgld, FPGA_IMAGE_ERR_BUSY);
>>>                   goto putdev_exit;
>>>           }
>>>
>>> -       imgld->progress = FPGA_IMAGE_PROG_PREPARING;
>>> +       fpga_image_update_progress(imgld, FPGA_IMAGE_PROG_PREPARING);
>>>           ret = imgld->ops->prepare(imgld, imgld->data, imgld->remaining_size);
>>>           if (ret) {
>>> -               imgld->err_code = ret;
>>> +               fpga_image_set_error(imgld, ret);
>>>                   goto modput_exit;
>>>           }
>>>
>>> -       imgld->progress = FPGA_IMAGE_PROG_WRITING;
>>> +       fpga_image_update_progress(imgld, FPGA_IMAGE_PROG_WRITING);
>>>           while (imgld->remaining_size) {
>>>                   ret = imgld->ops->write(imgld, imgld->data, offset,
>>>                                           imgld->remaining_size);
>>> @@ -65,7 +81,7 @@ static void fpga_image_do_load(struct work_struct *work)
>>>                                            "write-op wrote zero data\n");
>>>                                   ret = -FPGA_IMAGE_ERR_RW_ERROR;
>>>                           }
>>> -                       imgld->err_code = -ret;
>>> +                       fpga_image_set_error(imgld, -ret);
>>>                           goto done;
>>>                   }
>>>
>>> @@ -73,10 +89,10 @@ static void fpga_image_do_load(struct work_struct *work)
>>>                   offset += ret;
>>>           }
>>>
>>> -       imgld->progress = FPGA_IMAGE_PROG_PROGRAMMING;
>>> +       fpga_image_update_progress(imgld, FPGA_IMAGE_PROG_PROGRAMMING);
>>>           ret = imgld->ops->poll_complete(imgld);
>>>           if (ret)
>>> -               imgld->err_code = ret;
>>> +               fpga_image_set_error(imgld, ret);
>>>
>>>    done:
>>>           if (imgld->ops->cleanup)
>>> @@ -151,20 +167,42 @@ static int fpga_image_load_ioctl_write(struct fpga_image_load *imgld,
>>>           return ret;
>>>    }
>>>
>>> +static int fpga_image_load_ioctl_status(struct fpga_image_load *imgld,
>>> +                                       unsigned long arg)
>>> +{
>>> +       struct fpga_image_status status;
>>> +
>>> +       memset(&status, 0, sizeof(status));
>>> +       status.progress = imgld->progress;
>>> +       status.remaining_size = imgld->remaining_size;
>>> +       status.err_progress = imgld->err_progress;
>>> +       status.err_code = imgld->err_code;
>>> +
>>> +       if (copy_to_user((void __user *)arg, &status, sizeof(status)))
>>> +               return -EFAULT;
>>> +
>>> +       return 0;
>>> +}
>>> +
>>>    static long fpga_image_load_ioctl(struct file *filp, unsigned int cmd,
>>>                                     unsigned long arg)
>>>    {
>>>           struct fpga_image_load *imgld = filp->private_data;
>>>           int ret = -ENOTTY;
>>>
>>> +       mutex_lock(&imgld->lock);
>>> +
>>>           switch (cmd) {
>>>           case FPGA_IMAGE_LOAD_WRITE:
>>> -               mutex_lock(&imgld->lock);
>>>                   ret = fpga_image_load_ioctl_write(imgld, arg);
>>> -               mutex_unlock(&imgld->lock);
>>> +               break;
>>> +       case FPGA_IMAGE_LOAD_STATUS:
>>> +               ret = fpga_image_load_ioctl_status(imgld, arg);
>>>                   break;
>>>           }
>>>
>>> +       mutex_unlock(&imgld->lock);
>>> +
>>>           return ret;
>>>    }
>>>
>>> diff --git a/include/linux/fpga/fpga-image-load.h b/include/linux/fpga/fpga-image-load.h
>>> index 77b3c91ce073..366111d090fb 100644
>>> --- a/include/linux/fpga/fpga-image-load.h
>>> +++ b/include/linux/fpga/fpga-image-load.h
>>> @@ -49,6 +49,7 @@ struct fpga_image_load {
>>>           const u8 *data;                 /* pointer to update data */
>>>           u32 remaining_size;             /* size remaining to transfer */
>>>           u32 progress;
>>> +       u32 err_progress;               /* progress at time of error */
>>>           u32 err_code;                   /* image load error code */
>>>           bool driver_unload;
>>>           struct eventfd_ctx *finished;
>>> diff --git a/include/uapi/linux/fpga-image-load.h b/include/uapi/linux/fpga-image-load.h
>>> index 8d2d3db92e87..1b91343961df 100644
>>> --- a/include/uapi/linux/fpga-image-load.h
>>> +++ b/include/uapi/linux/fpga-image-load.h
>>> @@ -51,4 +51,22 @@ struct fpga_image_write {
>>>
>>>    #define FPGA_IMAGE_LOAD_WRITE  _IOW(FPGA_IMAGE_LOAD_MAGIC, 0, struct fpga_image_write)
>>>
>>> +/**
>>> + * FPGA_IMAGE_LOAD_STATUS - _IOR(FPGA_IMAGE_LOAD_MAGIC, 1,
>>> + *                              struct fpga_image_status)
>>> + *
>>> + * Request status information for an ongoing update.
>>> + *
>>> + * Return: 0 on success, -errno on failure.
>>> + */
>>> +struct fpga_image_status {
>>> +       /* Output */
>>> +       __u32 remaining_size;   /* size remaining to transfer */
>>> +       __u32 progress;         /* current progress of image load */
>>> +       __u32 err_progress;     /* progress at time of error */
>>> +       __u32 err_code;         /* error code */
>>> +};
>> Could this be extended to also collect the image detail?
>>
>>          Image version, name, etc been successfully written to device (flash).
>>
>>          Image version, name, etc is currently running on the device.
>>
>> Or maybe add another query command to do these?
>>
>> So the userland utility will be able to show what image is running and what image is going to run with next cold reboot.
> This status call is intended specifically to show the progress and
> status of the image load. Yilun's comments on patch 5 suggest that
> some of these types of operations could be handled as part of the
> fpga-region class driver
>
> I have patches that have not been submitted yet that contain some
> of these types of features, although not in the class driver (yet).
> Here is a link to documentation for what I have now. Take a look
> at the sysfs node descriptions that are in the control subdirectory.
> These are listed at the bottom of the file.
>
> https://github.com/OPAE/linux-dfl/blob/fpga-ofs-dev/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc-sec-update

Thanks a lot. This is very helpful.

Lizhi

>
> - Russ
>
>
>>
>> Thanks,
>>
>> Lizhi
>>
>>> +
>>> +#define FPGA_IMAGE_LOAD_STATUS _IOR(FPGA_IMAGE_LOAD_MAGIC, 1, struct fpga_image_status)
>>> +
>>>    #endif /* _UAPI_LINUX_FPGA_IMAGE_LOAD_H */
>>> --
>>> 2.25.1
>>>