Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Sat, 4 Jun 2022 23:47:13 +0800
From:   Xu Yilun <yilun.xu@intel.com>
To:     Ivan Bornyakov <i.bornyakov@metrotek.ru>
Cc:     mdf@kernel.org, hao.wu@intel.com, trix@redhat.com,
        Conor.Dooley@microchip.com, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, linux-fpga@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        system@metrotek.ru
Subject: Re: [PATCH v14 1/3] fpga: fpga-mgr: support bitstream offset in
 image buffer
Message-ID: <20220604154713.GA243326@yilunxu-OptiPlex-7050>
References: <20220531192344.23038-1-i.bornyakov@metrotek.ru>
 <20220531192344.23038-2-i.bornyakov@metrotek.ru>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220531192344.23038-2-i.bornyakov@metrotek.ru>
Precedence: bulk

On Tue, May 31, 2022 at 10:23:42PM +0300, Ivan Bornyakov wrote:
> At the moment FPGA manager core loads to the device entire image
> provided to fpga_mgr_load(). But it is not always whole FPGA image
> buffer meant to be written to the device. In particular, .dat formatted
> image for Microchip MPF contains meta info in the header that is not
> meant to be written to the device. This is issue for those low level
> drivers that loads data to the device with write() fpga_manager_ops
> callback, since write() can be called in iterator over scatter-gather
> table, not only linear image buffer. On the other hand, write_sg()
> callback is provided with whole image in scatter-gather form and can
> decide itself which part should be sent to the device.
> 
> Add header_size and data_size to the fpga_image_info struct and adjust
> fpga_mgr_write() callers with respect to them.
> 
>   * info->header_size indicates part at the beginning of image buffer
>     that is *not* meant to be written to the device. It is optional and
>     can be 0.
> 
>   * info->data_size is the size of actual bitstream data that *is* meant
>     to be written to the device, starting at info->header_size from the
>     beginning of image buffer. It is also optional and can be 0, which
>     means bitstream data is up to the end of image buffer.
> 
> Also add parse_header() callback to fpga_manager_ops, which purpose is
> to set info->header_size and info->data_size. At least
> initial_header_size bytes of image buffer will be passed into
> parse_header() first time. If it is not enough, parse_header() should
> set desired size into info->header_size and return -EAGAIN, then it will
> be called again with greater part of image buffer on the input.
> 
> Signed-off-by: Ivan Bornyakov <i.bornyakov@metrotek.ru>

Only one concern left, The XX_PARSE_HEAD state transition is a little messy,
see my comments inline.


> ---
>  drivers/fpga/fpga-mgr.c       | 176 ++++++++++++++++++++++++++++------
>  include/linux/fpga/fpga-mgr.h |  17 +++-
>  2 files changed, 165 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
> index a3595ecc3f79..34473d482c21 100644
> --- a/drivers/fpga/fpga-mgr.c
> +++ b/drivers/fpga/fpga-mgr.c
> @@ -74,6 +74,15 @@ static inline int fpga_mgr_write_complete(struct fpga_manager *mgr,
>  	return 0;
>  }
>  
> +static inline int fpga_mgr_parse_header(struct fpga_manager *mgr,
> +					struct fpga_image_info *info,
> +					const char *buf, size_t count)
> +{
> +	if (mgr->mops->parse_header)
> +		return mgr->mops->parse_header(mgr, info, buf, count);
> +	return 0;
> +}
> +
>  static inline int fpga_mgr_write_init(struct fpga_manager *mgr,
>  				      struct fpga_image_info *info,
>  				      const char *buf, size_t count)
> @@ -136,24 +145,57 @@ void fpga_image_info_free(struct fpga_image_info *info)
>  EXPORT_SYMBOL_GPL(fpga_image_info_free);
>  
>  /*
> - * Call the low level driver's write_init function.  This will do the
> + * Call the low level driver's parse_header function. This will set
> + * info->header_size and info->data_size. The low level driver gets entire
> + * buffer provided. If it is not enough, driver should set desired size into
> + * info->header_size and return -EAGAIN, then parse_header() will be called
> + * again with greater part of image buffer on the input.
> + */
> +static int fpga_mgr_parse_header_buf(struct fpga_manager *mgr,
> +				     struct fpga_image_info *info,
> +				     const char *buf, size_t count)
> +{
> +	int ret;
> +
> +	mgr->state = FPGA_MGR_STATE_PARSE_HEADER;
> +	ret = fpga_mgr_parse_header(mgr, info, buf, count);
> +	if (ret && ret != -EAGAIN) {
> +		dev_err(&mgr->dev, "Error while parsing FPGA image header\n");
> +		mgr->state = FPGA_MGR_STATE_PARSE_HEADER_ERR;

Try not to assign the state again and again, or outside the
xx_parse_header_xxx() function.

If a single function cannot handle all cases, how about having more
functions, like fpga_mgr_parse_header_mapped(),
fpga_mgr_parse_header_sg_first(), fpga_mgr_parse_header_sg(),
and just toggle the state in these functions.

> +	}
> +
> +	return ret;
> +}
> +
> +/*
> + * Call the low level driver's write_init function. This will do the
>   * device-specific things to get the FPGA into the state where it is ready to
> - * receive an FPGA image. The low level driver only gets to see the first
> - * initial_header_size bytes in the buffer.
> + * receive an FPGA image. If info->header_size is defined, the low level
> + * driver gets to see at least first info->header_size bytes in the buffer,
> + * mgr->mops->initial_header_size otherwise. If neither initial_header_size
> + * nor header_size are not set, write_init will not get any bytes of image
> + * buffer.
>   */
>  static int fpga_mgr_write_init_buf(struct fpga_manager *mgr,
>  				   struct fpga_image_info *info,
>  				   const char *buf, size_t count)
>  {
> +	size_t header_size;
>  	int ret;
>  
>  	mgr->state = FPGA_MGR_STATE_WRITE_INIT;
> -	if (!mgr->mops->initial_header_size) {
> +
> +	if (info->header_size)
> +		header_size = info->header_size;
> +	else
> +		header_size = mgr->mops->initial_header_size;
> +
> +	if (header_size > count)
> +		ret = -EINVAL;
> +	else if (!header_size)
>  		ret = fpga_mgr_write_init(mgr, info, NULL, 0);
> -	} else {
> -		count = min(mgr->mops->initial_header_size, count);
> +	else
>  		ret = fpga_mgr_write_init(mgr, info, buf, count);
> -	}
>  
>  	if (ret) {
>  		dev_err(&mgr->dev, "Error preparing FPGA for writing\n");
> @@ -164,16 +206,17 @@ static int fpga_mgr_write_init_buf(struct fpga_manager *mgr,
>  	return 0;
>  }
>  
> -static int fpga_mgr_write_init_sg(struct fpga_manager *mgr,
> -				  struct fpga_image_info *info,
> -				  struct sg_table *sgt)
> +static int fpga_mgr_prepare_sg(struct fpga_manager *mgr,
> +			       struct fpga_image_info *info,
> +			       struct sg_table *sgt)
>  {
>  	struct sg_mapping_iter miter;
> -	size_t len;
> -	char *buf;
> +	size_t header_size, len;
> +	char *buf = NULL;
>  	int ret;
>  
> -	if (!mgr->mops->initial_header_size)
> +	header_size = mgr->mops->initial_header_size;
> +	if (!header_size)
>  		return fpga_mgr_write_init_buf(mgr, info, NULL, 0);
>  
>  	/*
> @@ -182,22 +225,44 @@ static int fpga_mgr_write_init_sg(struct fpga_manager *mgr,
>  	 */
>  	sg_miter_start(&miter, sgt->sgl, sgt->nents, SG_MITER_FROM_SG);
>  	if (sg_miter_next(&miter) &&
> -	    miter.length >= mgr->mops->initial_header_size) {
> -		ret = fpga_mgr_write_init_buf(mgr, info, miter.addr,
> -					      miter.length);
> -		sg_miter_stop(&miter);
> -		return ret;
> +	    miter.length >= header_size) {
> +		ret = fpga_mgr_parse_header_buf(mgr, info, miter.addr,
> +						miter.length);

Maybe fpga_mgr_parse_header_sg_first()

> +		/*
> +		 * If -EAGAIN, more sg buffer is needed, otherwise the flow
> +		 * would always end up in this branch.
> +		 */
> +		if (ret != -EAGAIN) {
> +			if (!ret)
> +				ret = fpga_mgr_write_init_buf(mgr, info, miter.addr,
> +							      miter.length);
> +
> +			sg_miter_stop(&miter);
> +			return ret;
> +		}
>  	}
>  	sg_miter_stop(&miter);
>  
>  	/* Otherwise copy the fragments into temporary memory. */
> -	buf = kmalloc(mgr->mops->initial_header_size, GFP_KERNEL);
> -	if (!buf)
> -		return -ENOMEM;
> +	do {
> +		if (info->header_size)
> +			header_size = info->header_size;
> +
> +		buf = krealloc(buf, header_size, GFP_KERNEL);
> +		if (!buf)
> +			return -ENOMEM;
> +
> +		len = sg_copy_to_buffer(sgt->sgl, sgt->nents, buf, header_size);
> +		if (len != header_size) {
> +			kfree(buf);
> +			return -EFAULT;
> +		}
> +
> +		ret = fpga_mgr_parse_header_buf(mgr, info, buf, header_size);
> +	} while (ret == -EAGAIN);

Maybe put the operations in fpga_mgr_parse_header_sg().

>  
> -	len = sg_copy_to_buffer(sgt->sgl, sgt->nents, buf,
> -				mgr->mops->initial_header_size);
> -	ret = fpga_mgr_write_init_buf(mgr, info, buf, len);
> +	if (!ret)
> +		ret = fpga_mgr_write_init_buf(mgr, info, buf, header_size);
>  
>  	kfree(buf);
>  
> @@ -227,7 +292,7 @@ static int fpga_mgr_buf_load_sg(struct fpga_manager *mgr,
>  {
>  	int ret;
>  
> -	ret = fpga_mgr_write_init_sg(mgr, info, sgt);
> +	ret = fpga_mgr_prepare_sg(mgr, info, sgt);
>  	if (ret)
>  		return ret;
>  
> @@ -237,11 +302,40 @@ static int fpga_mgr_buf_load_sg(struct fpga_manager *mgr,
>  		ret = fpga_mgr_write_sg(mgr, sgt);
>  	} else {
>  		struct sg_mapping_iter miter;
> +		size_t length, data_size;
> +		bool last = false;
> +		ssize_t count;
> +		char *addr;
> +
> +		data_size = info->data_size;
> +		count = -info->header_size;
>  
>  		sg_miter_start(&miter, sgt->sgl, sgt->nents, SG_MITER_FROM_SG);
>  		while (sg_miter_next(&miter)) {
> -			ret = fpga_mgr_write(mgr, miter.addr, miter.length);
> -			if (ret)
> +			count += miter.length;
> +
> +			/* sg block contains only header, no data */
> +			if (count <= 0)
> +				continue;
> +
> +			if (count < miter.length) {
> +				/* sg block contains both header and data */
> +				addr = miter.addr + miter.length - count;
> +				length = count;
> +			} else {
> +				/* sg block contains pure data */
> +				addr = miter.addr;
> +				length = miter.length;
> +			}
> +
> +			/* truncate last block to data_size, if needed */
> +			if (data_size && count > data_size) {
> +				length -= count - data_size;
> +				last = true;
> +			}
> +
> +			ret = fpga_mgr_write(mgr, addr, length);
> +			if (ret || last)
>  				break;
>  		}
>  		sg_miter_stop(&miter);
> @@ -262,10 +356,34 @@ static int fpga_mgr_buf_load_mapped(struct fpga_manager *mgr,
>  {
>  	int ret;
>  
> +	ret = fpga_mgr_parse_header_buf(mgr, info, buf, count);
> +	if (ret) {
> +		if (ret == -EAGAIN) {
> +			dev_err(&mgr->dev, "Header outruns FPGA image\n");
> +			mgr->state = FPGA_MGR_STATE_PARSE_HEADER_ERR;
> +			ret = -EINVAL;
> +		}
> +
> +		return ret;
> +	}
> +
> +	if (info->header_size + info->data_size > count) {
> +		dev_err(&mgr->dev, "Bitsream data outruns FPGA image\n");
> +		mgr->state = FPGA_MGR_STATE_PARSE_HEADER_ERR;
> +		return -EINVAL;
> +	}

Maybe put the checks in fpga_mgr_parse_header_mapped()

Thanks,
Yilun

> +
>  	ret = fpga_mgr_write_init_buf(mgr, info, buf, count);
>  	if (ret)
>  		return ret;
>  
> +	if (info->data_size)
> +		count = info->data_size;
> +	else
> +		count -= info->header_size;
> +
> +	buf += info->header_size;
> +
>  	/*
>  	 * Write the FPGA image to the FPGA.
>  	 */
> @@ -424,6 +542,10 @@ static const char * const state_str[] = {
>  	[FPGA_MGR_STATE_FIRMWARE_REQ] =		"firmware request",
>  	[FPGA_MGR_STATE_FIRMWARE_REQ_ERR] =	"firmware request error",
>  
> +	/* Parse FPGA image header */
> +	[FPGA_MGR_STATE_PARSE_HEADER] =		"parse header",
> +	[FPGA_MGR_STATE_PARSE_HEADER_ERR] =	"parse header error",
> +
>  	/* Preparing FPGA to receive image */
>  	[FPGA_MGR_STATE_WRITE_INIT] =		"write init",
>  	[FPGA_MGR_STATE_WRITE_INIT_ERR] =	"write init error",
> diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
> index 0f9468771bb9..cba8bb7827a5 100644
> --- a/include/linux/fpga/fpga-mgr.h
> +++ b/include/linux/fpga/fpga-mgr.h
> @@ -22,6 +22,8 @@ struct sg_table;
>   * @FPGA_MGR_STATE_RESET: FPGA in reset state
>   * @FPGA_MGR_STATE_FIRMWARE_REQ: firmware request in progress
>   * @FPGA_MGR_STATE_FIRMWARE_REQ_ERR: firmware request failed
> + * @FPGA_MGR_STATE_PARSE_HEADER: parse FPGA image header
> + * @FPGA_MGR_STATE_PARSE_HEADER_ERR: Error during PARSE_HEADER stage
>   * @FPGA_MGR_STATE_WRITE_INIT: preparing FPGA for programming
>   * @FPGA_MGR_STATE_WRITE_INIT_ERR: Error during WRITE_INIT stage
>   * @FPGA_MGR_STATE_WRITE: writing image to FPGA
> @@ -42,6 +44,8 @@ enum fpga_mgr_states {
>  	FPGA_MGR_STATE_FIRMWARE_REQ_ERR,
>  
>  	/* write sequence: init, write, complete */
> +	FPGA_MGR_STATE_PARSE_HEADER,
> +	FPGA_MGR_STATE_PARSE_HEADER_ERR,
>  	FPGA_MGR_STATE_WRITE_INIT,
>  	FPGA_MGR_STATE_WRITE_INIT_ERR,
>  	FPGA_MGR_STATE_WRITE,
> @@ -85,6 +89,8 @@ enum fpga_mgr_states {
>   * @sgt: scatter/gather table containing FPGA image
>   * @buf: contiguous buffer containing FPGA image
>   * @count: size of buf
> + * @header_size: offset in image buffer where bitstream data starts
> + * @data_size: size of bitstream. If 0, (count - header_size) will be used.
>   * @region_id: id of target region
>   * @dev: device that owns this
>   * @overlay: Device Tree overlay
> @@ -98,6 +104,8 @@ struct fpga_image_info {
>  	struct sg_table *sgt;
>  	const char *buf;
>  	size_t count;
> +	size_t header_size;
> +	size_t data_size;
>  	int region_id;
>  	struct device *dev;
>  #ifdef CONFIG_OF
> @@ -137,9 +145,13 @@ struct fpga_manager_info {
>  
>  /**
>   * struct fpga_manager_ops - ops for low level fpga manager drivers
> - * @initial_header_size: Maximum number of bytes that should be passed into write_init
> + * @initial_header_size: minimum number of bytes that should be passed into
> + *	parse_header and write_init.
>   * @state: returns an enum value of the FPGA's state
>   * @status: returns status of the FPGA, including reconfiguration error code
> + * @parse_header: parse FPGA image header to set info->header_size and
> + *	info->data_size. In case the input buffer is not large enough, set
> + *	required size to info->header_size and return -EAGAIN.
>   * @write_init: prepare the FPGA to receive configuration data
>   * @write: write count bytes of configuration data to the FPGA
>   * @write_sg: write the scatter list of configuration data to the FPGA
> @@ -155,6 +167,9 @@ struct fpga_manager_ops {
>  	size_t initial_header_size;
>  	enum fpga_mgr_states (*state)(struct fpga_manager *mgr);
>  	u64 (*status)(struct fpga_manager *mgr);
> +	int (*parse_header)(struct fpga_manager *mgr,
> +			    struct fpga_image_info *info,
> +			    const char *buf, size_t count);
>  	int (*write_init)(struct fpga_manager *mgr,
>  			  struct fpga_image_info *info,
>  			  const char *buf, size_t count);
> -- 
> 2.35.1
>