| Message ID | 20220609154752.20781-2-i.bornyakov@metrotek.ru (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Microchip Polarfire FPGA manager | expand |
On Thu, Jun 09, 2022 at 06:47:49PM +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> > --- > drivers/fpga/fpga-mgr.c | 243 +++++++++++++++++++++++++++++----- > include/linux/fpga/fpga-mgr.h | 17 ++- > 2 files changed, 229 insertions(+), 31 deletions(-) > > diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c > index 08dc85fcd511..0854fbc8f11e 100644 > --- a/drivers/fpga/fpga-mgr.c > +++ b/drivers/fpga/fpga-mgr.c Should we check in fpga_mgr_create, that initial_header_size must not be 0 if parse_header() is defined. If we pass no data to parse_header(), does it make any sense? > @@ -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,145 @@ 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 with entire FPGA image > + * buffer on the input. This will set info->header_size and info->data_size. > + */ > +static int fpga_mgr_parse_header_mapped(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 (info->header_size + info->data_size > count) { > + dev_err(&mgr->dev, "Bitsream data outruns FPGA image\n"); > + ret = -EINVAL; > + } > + > + if (ret) { > + dev_err(&mgr->dev, "Error while parsing FPGA image header\n"); > + mgr->state = FPGA_MGR_STATE_PARSE_HEADER_ERR; > + } > + > + return ret; > +} > + > +/* > + * Call the low level driver's parse_header function with first fragment of > + * scattered FPGA image on the input. If header fits first fragment, > + * parse_header will set info->header_size and info->data_size. If it is not, > + * parse_header will set desired size to info->header_size and -EAGAIN will be > + * returned. > + */ > +static int fpga_mgr_parse_header_sg_first(struct fpga_manager *mgr, > + struct fpga_image_info *info, > + struct sg_table *sgt) > +{ > + size_t header_size = mgr->mops->initial_header_size; > + struct sg_mapping_iter miter; > + int ret; > + > + mgr->state = FPGA_MGR_STATE_PARSE_HEADER; > + > + sg_miter_start(&miter, sgt->sgl, sgt->nents, SG_MITER_FROM_SG); > + if (sg_miter_next(&miter) && > + miter.length >= header_size) > + ret = fpga_mgr_parse_header(mgr, info, miter.addr, miter.length); > + else > + ret = -EAGAIN; > + sg_miter_stop(&miter); > + > + if (ret && ret != -EAGAIN) { > + dev_err(&mgr->dev, "Error while parsing FPGA image header\n"); > + mgr->state = FPGA_MGR_STATE_PARSE_HEADER_ERR; > + } > + > + return ret; > +} > + > +/* > + * Copy scattered FPGA image fragments to temporary buffer and call the > + * low level driver's parse_header function. This should be called after > + * fpga_mgr_parse_header_sg_first() returned -EAGAIN. In case of success, > + * pointer to the newly allocated image header copy will be set into *ret_buf > + * and its size will be returned. *ret_buf needs to be freed by caller. > + */ > +static ssize_t fpga_mgr_parse_header_sg(struct fpga_manager *mgr, > + struct fpga_image_info *info, > + struct sg_table *sgt, char **ret_buf) Since the function allocs buffer for the user, I suggest it still returns the buffer pointer. The buffer size could be an output parameter. > +{ > + size_t len, header_size = mgr->mops->initial_header_size; > + char *buf = NULL; > + int ret; > + > + do { > + if (info->header_size) > + header_size = info->header_size; > + > + buf = krealloc(buf, header_size, GFP_KERNEL); > + if (!buf) { > + ret = -ENOMEM; If you need to return ERROR value, ERR_PTR() could be used. BTW: Have you tried to test your FPGA programming using sg buffers? I want to ensure these changes are actually tested? Thanks, Yilun > + break; > + } > + > + len = sg_copy_to_buffer(sgt->sgl, sgt->nents, buf, header_size); > + if (len != header_size) { > + ret = -EFAULT; > + break; > + } > + > + ret = fpga_mgr_parse_header(mgr, info, buf, header_size); > + if (ret == -EAGAIN && info->header_size <= header_size) { > + dev_err(&mgr->dev, "Requested invalid header size\n"); > + ret = -EFAULT; > + } > + } while (ret == -EAGAIN); > + > + if (ret) { > + dev_err(&mgr->dev, "Error while parsing FPGA image header\n"); > + mgr->state = FPGA_MGR_STATE_PARSE_HEADER_ERR; > + kfree(buf); > + buf = NULL; > + } else { > + ret = header_size; > + } > + > + *ret_buf = buf; > + > + 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,13 +294,13 @@ 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; > + ssize_t header_size; > + char *header_buf; > int ret; > > if (!mgr->mops->initial_header_size) > @@ -180,26 +310,35 @@ static int fpga_mgr_write_init_sg(struct fpga_manager *mgr, > * First try to use miter to map the first fragment to access the > * header, this is the typical path. > */ > - 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); > + ret = fpga_mgr_parse_header_sg_first(mgr, info, sgt); > + /* If 0, header fits first fragment, call write_init on it */ > + if (!ret) { > + sg_miter_start(&miter, sgt->sgl, sgt->nents, SG_MITER_FROM_SG); > + if (sg_miter_next(&miter)) { > + ret = fpga_mgr_write_init_buf(mgr, info, miter.addr, > + miter.length); > + sg_miter_stop(&miter); > + return ret; > + } > sg_miter_stop(&miter); > + /* > + * If -EAGAIN, more sg buffer is needed, > + * otherwise an error has occurred. > + */ > + } else if (ret != -EAGAIN) > 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; > + /* > + * Otherwise copy the fragments into temporary memory. > + * Copying is done inside fpga_mgr_parse_header_sg() > + */ > + header_size = fpga_mgr_parse_header_sg(mgr, info, sgt, &header_buf); > + if (header_size < 0) > + return header_size; > > - 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); > + ret = fpga_mgr_write_init_buf(mgr, info, header_buf, header_size); > > - kfree(buf); > + kfree(header_buf); > > return ret; > } > @@ -227,7 +366,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 +376,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 +430,21 @@ static int fpga_mgr_buf_load_mapped(struct fpga_manager *mgr, > { > int ret; > > + ret = fpga_mgr_parse_header_mapped(mgr, info, buf, count); > + if (ret) > + return ret; > + > 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 +603,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 >
On Fri, Jun 10, 2022 at 03:43:11PM +0800, Xu Yilun wrote: > On Thu, Jun 09, 2022 at 06:47:49PM +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> > > --- > > drivers/fpga/fpga-mgr.c | 243 +++++++++++++++++++++++++++++----- > > include/linux/fpga/fpga-mgr.h | 17 ++- > > 2 files changed, 229 insertions(+), 31 deletions(-) > > > > diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c > > index 08dc85fcd511..0854fbc8f11e 100644 > > --- a/drivers/fpga/fpga-mgr.c > > +++ b/drivers/fpga/fpga-mgr.c > > Should we check in fpga_mgr_create, that initial_header_size must not be > 0 if parse_header() is defined. If we pass no data to parse_header(), > does it make any sense? Sorry, there is no fpga_mgr_create now, should be fpga_mgr_register_full Thanks, Yilun
On Fri, Jun 10, 2022 at 03:43:11PM +0800, Xu Yilun wrote: > On Thu, Jun 09, 2022 at 06:47:49PM +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> > > --- > > drivers/fpga/fpga-mgr.c | 243 +++++++++++++++++++++++++++++----- > > include/linux/fpga/fpga-mgr.h | 17 ++- > > 2 files changed, 229 insertions(+), 31 deletions(-) > > > > diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c > > index 08dc85fcd511..0854fbc8f11e 100644 > > --- a/drivers/fpga/fpga-mgr.c > > +++ b/drivers/fpga/fpga-mgr.c > > Should we check in fpga_mgr_create, that initial_header_size must not be > 0 if parse_header() is defined. If we pass no data to parse_header(), > does it make any sense? > If FPGA image is mapped, whole buffer will be passed to parse_header(), regardless of initial_header_size. If FPGA image is sg_table and initial_header_size is 0, first sg fragment will be passed to parse_header() in fpga_mgr_parse_header_sg_first(). If low level driver's parse_header() is buggy and return -EAGAIN without setting desired size into info->header_size, we will fail in fpga_mgr_parse_header_sg() with -ENOMEM. static ssize_t fpga_mgr_parse_header_sg(...) { size_t header_size = mgr->mops->initial_header_size; /* header_size == 0 */ ... do { if (info->header_size) /* header_size is still 0 */ header_size = info->header_size; buf = krealloc(buf, header_size, GFP_KERNEL); /* returned buf will be NULL */ if (!buf) { ret = -ENOMEM; break; } ... } while (ret == -EAGAIN); ... } So, I believe it is OK to define parse_header() without defining initial_header_size. > > @@ -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,145 @@ 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 with entire FPGA image > > + * buffer on the input. This will set info->header_size and info->data_size. > > + */ > > +static int fpga_mgr_parse_header_mapped(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 (info->header_size + info->data_size > count) { > > + dev_err(&mgr->dev, "Bitsream data outruns FPGA image\n"); > > + ret = -EINVAL; > > + } > > + > > + if (ret) { > > + dev_err(&mgr->dev, "Error while parsing FPGA image header\n"); > > + mgr->state = FPGA_MGR_STATE_PARSE_HEADER_ERR; > > + } > > + > > + return ret; > > +} > > + > > +/* > > + * Call the low level driver's parse_header function with first fragment of > > + * scattered FPGA image on the input. If header fits first fragment, > > + * parse_header will set info->header_size and info->data_size. If it is not, > > + * parse_header will set desired size to info->header_size and -EAGAIN will be > > + * returned. > > + */ > > +static int fpga_mgr_parse_header_sg_first(struct fpga_manager *mgr, > > + struct fpga_image_info *info, > > + struct sg_table *sgt) > > +{ > > + size_t header_size = mgr->mops->initial_header_size; > > + struct sg_mapping_iter miter; > > + int ret; > > + > > + mgr->state = FPGA_MGR_STATE_PARSE_HEADER; > > + > > + sg_miter_start(&miter, sgt->sgl, sgt->nents, SG_MITER_FROM_SG); > > + if (sg_miter_next(&miter) && > > + miter.length >= header_size) > > + ret = fpga_mgr_parse_header(mgr, info, miter.addr, miter.length); > > + else > > + ret = -EAGAIN; > > + sg_miter_stop(&miter); > > + > > + if (ret && ret != -EAGAIN) { > > + dev_err(&mgr->dev, "Error while parsing FPGA image header\n"); > > + mgr->state = FPGA_MGR_STATE_PARSE_HEADER_ERR; > > + } > > + > > + return ret; > > +} > > + > > +/* > > + * Copy scattered FPGA image fragments to temporary buffer and call the > > + * low level driver's parse_header function. This should be called after > > + * fpga_mgr_parse_header_sg_first() returned -EAGAIN. In case of success, > > + * pointer to the newly allocated image header copy will be set into *ret_buf > > + * and its size will be returned. *ret_buf needs to be freed by caller. > > + */ > > +static ssize_t fpga_mgr_parse_header_sg(struct fpga_manager *mgr, > > + struct fpga_image_info *info, > > + struct sg_table *sgt, char **ret_buf) > > Since the function allocs buffer for the user, I suggest it still returns > the buffer pointer. The buffer size could be an output parameter. > OK, will do that. > > +{ > > + size_t len, header_size = mgr->mops->initial_header_size; > > + char *buf = NULL; > > + int ret; > > + > > + do { > > + if (info->header_size) > > + header_size = info->header_size; > > + > > + buf = krealloc(buf, header_size, GFP_KERNEL); > > + if (!buf) { > > + ret = -ENOMEM; > > If you need to return ERROR value, ERR_PTR() could be used. > > > BTW: Have you tried to test your FPGA programming using sg buffers? I want > to ensure these changes are actually tested? > Of course, but not every single error path, to be honest. Yet, I'm trying my best. > Thanks, > Yilun > > > + break; > > + } > > + > > + len = sg_copy_to_buffer(sgt->sgl, sgt->nents, buf, header_size); > > + if (len != header_size) { > > + ret = -EFAULT; > > + break; > > + } > > + > > + ret = fpga_mgr_parse_header(mgr, info, buf, header_size); > > + if (ret == -EAGAIN && info->header_size <= header_size) { > > + dev_err(&mgr->dev, "Requested invalid header size\n"); > > + ret = -EFAULT; > > + } > > + } while (ret == -EAGAIN); > > + > > + if (ret) { > > + dev_err(&mgr->dev, "Error while parsing FPGA image header\n"); > > + mgr->state = FPGA_MGR_STATE_PARSE_HEADER_ERR; > > + kfree(buf); > > + buf = NULL; > > + } else { > > + ret = header_size; > > + } > > + > > + *ret_buf = buf; > > + > > + 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,13 +294,13 @@ 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; > > + ssize_t header_size; > > + char *header_buf; > > int ret; > > > > if (!mgr->mops->initial_header_size) > > @@ -180,26 +310,35 @@ static int fpga_mgr_write_init_sg(struct fpga_manager *mgr, > > * First try to use miter to map the first fragment to access the > > * header, this is the typical path. > > */ > > - 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); > > + ret = fpga_mgr_parse_header_sg_first(mgr, info, sgt); > > + /* If 0, header fits first fragment, call write_init on it */ > > + if (!ret) { > > + sg_miter_start(&miter, sgt->sgl, sgt->nents, SG_MITER_FROM_SG); > > + if (sg_miter_next(&miter)) { > > + ret = fpga_mgr_write_init_buf(mgr, info, miter.addr, > > + miter.length); > > + sg_miter_stop(&miter); > > + return ret; > > + } > > sg_miter_stop(&miter); > > + /* > > + * If -EAGAIN, more sg buffer is needed, > > + * otherwise an error has occurred. > > + */ > > + } else if (ret != -EAGAIN) > > 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; > > + /* > > + * Otherwise copy the fragments into temporary memory. > > + * Copying is done inside fpga_mgr_parse_header_sg() > > + */ > > + header_size = fpga_mgr_parse_header_sg(mgr, info, sgt, &header_buf); > > + if (header_size < 0) > > + return header_size; > > > > - 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); > > + ret = fpga_mgr_write_init_buf(mgr, info, header_buf, header_size); > > > > - kfree(buf); > > + kfree(header_buf); > > > > return ret; > > } > > @@ -227,7 +366,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 +376,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 +430,21 @@ static int fpga_mgr_buf_load_mapped(struct fpga_manager *mgr, > > { > > int ret; > > > > + ret = fpga_mgr_parse_header_mapped(mgr, info, buf, count); > > + if (ret) > > + return ret; > > + > > 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 +603,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 > >
On Fri, Jun 10, 2022 at 11:13:42AM +0300, Ivan Bornyakov wrote: > On Fri, Jun 10, 2022 at 03:43:11PM +0800, Xu Yilun wrote: > > On Thu, Jun 09, 2022 at 06:47:49PM +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> > > > --- > > > drivers/fpga/fpga-mgr.c | 243 +++++++++++++++++++++++++++++----- > > > include/linux/fpga/fpga-mgr.h | 17 ++- > > > 2 files changed, 229 insertions(+), 31 deletions(-) > > > > > > diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c > > > index 08dc85fcd511..0854fbc8f11e 100644 > > > --- a/drivers/fpga/fpga-mgr.c > > > +++ b/drivers/fpga/fpga-mgr.c > > > > Should we check in fpga_mgr_create, that initial_header_size must not be > > 0 if parse_header() is defined. If we pass no data to parse_header(), > > does it make any sense? > > > > If FPGA image is mapped, whole buffer will be passed to parse_header(), > regardless of initial_header_size. > > If FPGA image is sg_table and initial_header_size is 0, first sg > fragment will be passed to parse_header() in > fpga_mgr_parse_header_sg_first(). > > If low level driver's parse_header() is buggy and return -EAGAIN without > setting desired size into info->header_size, we will fail in > fpga_mgr_parse_header_sg() with -ENOMEM. > > static ssize_t fpga_mgr_parse_header_sg(...) > { > size_t header_size = mgr->mops->initial_header_size; /* header_size == 0 */ > ... > > do { > if (info->header_size) /* header_size is still 0 */ > header_size = info->header_size; > > buf = krealloc(buf, header_size, GFP_KERNEL); /* returned buf will be NULL */ > if (!buf) { > ret = -ENOMEM; > break; > } > > ... > > } while (ret == -EAGAIN); > > ... > } > > So, I believe it is OK to define parse_header() without defining > initial_header_size. That's good to me. > > > > @@ -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,145 @@ 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 with entire FPGA image > > > + * buffer on the input. This will set info->header_size and info->data_size. > > > + */ > > > +static int fpga_mgr_parse_header_mapped(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 (info->header_size + info->data_size > count) { > > > + dev_err(&mgr->dev, "Bitsream data outruns FPGA image\n"); > > > + ret = -EINVAL; > > > + } > > > + > > > + if (ret) { > > > + dev_err(&mgr->dev, "Error while parsing FPGA image header\n"); > > > + mgr->state = FPGA_MGR_STATE_PARSE_HEADER_ERR; > > > + } > > > + > > > + return ret; > > > +} > > > + > > > +/* > > > + * Call the low level driver's parse_header function with first fragment of > > > + * scattered FPGA image on the input. If header fits first fragment, > > > + * parse_header will set info->header_size and info->data_size. If it is not, > > > + * parse_header will set desired size to info->header_size and -EAGAIN will be > > > + * returned. > > > + */ > > > +static int fpga_mgr_parse_header_sg_first(struct fpga_manager *mgr, > > > + struct fpga_image_info *info, > > > + struct sg_table *sgt) > > > +{ > > > + size_t header_size = mgr->mops->initial_header_size; > > > + struct sg_mapping_iter miter; > > > + int ret; > > > + > > > + mgr->state = FPGA_MGR_STATE_PARSE_HEADER; > > > + > > > + sg_miter_start(&miter, sgt->sgl, sgt->nents, SG_MITER_FROM_SG); > > > + if (sg_miter_next(&miter) && > > > + miter.length >= header_size) > > > + ret = fpga_mgr_parse_header(mgr, info, miter.addr, miter.length); > > > + else > > > + ret = -EAGAIN; > > > + sg_miter_stop(&miter); > > > + > > > + if (ret && ret != -EAGAIN) { > > > + dev_err(&mgr->dev, "Error while parsing FPGA image header\n"); > > > + mgr->state = FPGA_MGR_STATE_PARSE_HEADER_ERR; > > > + } > > > + > > > + return ret; > > > +} > > > + > > > +/* > > > + * Copy scattered FPGA image fragments to temporary buffer and call the > > > + * low level driver's parse_header function. This should be called after > > > + * fpga_mgr_parse_header_sg_first() returned -EAGAIN. In case of success, > > > + * pointer to the newly allocated image header copy will be set into *ret_buf > > > + * and its size will be returned. *ret_buf needs to be freed by caller. > > > + */ > > > +static ssize_t fpga_mgr_parse_header_sg(struct fpga_manager *mgr, > > > + struct fpga_image_info *info, > > > + struct sg_table *sgt, char **ret_buf) > > > > Since the function allocs buffer for the user, I suggest it still returns > > the buffer pointer. The buffer size could be an output parameter. > > > > OK, will do that. > > > > +{ > > > + size_t len, header_size = mgr->mops->initial_header_size; > > > + char *buf = NULL; > > > + int ret; > > > + > > > + do { > > > + if (info->header_size) > > > + header_size = info->header_size; > > > + > > > + buf = krealloc(buf, header_size, GFP_KERNEL); > > > + if (!buf) { > > > + ret = -ENOMEM; > > > > If you need to return ERROR value, ERR_PTR() could be used. > > > > > > BTW: Have you tried to test your FPGA programming using sg buffers? I want > > to ensure these changes are actually tested? > > > > Of course, but not every single error path, to be honest. Yet, I'm > trying my best. That's fine. Thanks for your support.
On Fri, Jun 10, 2022 at 11:13:42AM +0300, Ivan Bornyakov wrote: > On Fri, Jun 10, 2022 at 03:43:11PM +0800, Xu Yilun wrote: > > On Thu, Jun 09, 2022 at 06:47:49PM +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> > > > --- > > > drivers/fpga/fpga-mgr.c | 243 +++++++++++++++++++++++++++++----- > > > include/linux/fpga/fpga-mgr.h | 17 ++- > > > 2 files changed, 229 insertions(+), 31 deletions(-) > > > > > > diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c > > > index 08dc85fcd511..0854fbc8f11e 100644 > > > --- a/drivers/fpga/fpga-mgr.c > > > +++ b/drivers/fpga/fpga-mgr.c > > > > Should we check in fpga_mgr_create, that initial_header_size must not be > > 0 if parse_header() is defined. If we pass no data to parse_header(), > > does it make any sense? > > > > If FPGA image is mapped, whole buffer will be passed to parse_header(), > regardless of initial_header_size. > > If FPGA image is sg_table and initial_header_size is 0, first sg > fragment will be passed to parse_header() in > fpga_mgr_parse_header_sg_first(). > > If low level driver's parse_header() is buggy and return -EAGAIN without > setting desired size into info->header_size, we will fail in > fpga_mgr_parse_header_sg() with -ENOMEM. > > static ssize_t fpga_mgr_parse_header_sg(...) > { > size_t header_size = mgr->mops->initial_header_size; /* header_size == 0 */ > ... > > do { > if (info->header_size) /* header_size is still 0 */ > header_size = info->header_size; > > buf = krealloc(buf, header_size, GFP_KERNEL); /* returned buf will be NULL */ > if (!buf) { Actually this check should be against ZERO_OR_NULL_PTR(buf), then my words would be correct. > ret = -ENOMEM; > break; > } > > ... > > } while (ret == -EAGAIN); > > ... > } > > So, I believe it is OK to define parse_header() without defining > initial_header_size. > > > > @@ -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,145 @@ 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 with entire FPGA image > > > + * buffer on the input. This will set info->header_size and info->data_size. > > > + */ > > > +static int fpga_mgr_parse_header_mapped(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 (info->header_size + info->data_size > count) { > > > + dev_err(&mgr->dev, "Bitsream data outruns FPGA image\n"); > > > + ret = -EINVAL; > > > + } > > > + > > > + if (ret) { > > > + dev_err(&mgr->dev, "Error while parsing FPGA image header\n"); > > > + mgr->state = FPGA_MGR_STATE_PARSE_HEADER_ERR; > > > + } > > > + > > > + return ret; > > > +} > > > + > > > +/* > > > + * Call the low level driver's parse_header function with first fragment of > > > + * scattered FPGA image on the input. If header fits first fragment, > > > + * parse_header will set info->header_size and info->data_size. If it is not, > > > + * parse_header will set desired size to info->header_size and -EAGAIN will be > > > + * returned. > > > + */ > > > +static int fpga_mgr_parse_header_sg_first(struct fpga_manager *mgr, > > > + struct fpga_image_info *info, > > > + struct sg_table *sgt) > > > +{ > > > + size_t header_size = mgr->mops->initial_header_size; > > > + struct sg_mapping_iter miter; > > > + int ret; > > > + > > > + mgr->state = FPGA_MGR_STATE_PARSE_HEADER; > > > + > > > + sg_miter_start(&miter, sgt->sgl, sgt->nents, SG_MITER_FROM_SG); > > > + if (sg_miter_next(&miter) && > > > + miter.length >= header_size) > > > + ret = fpga_mgr_parse_header(mgr, info, miter.addr, miter.length); > > > + else > > > + ret = -EAGAIN; > > > + sg_miter_stop(&miter); > > > + > > > + if (ret && ret != -EAGAIN) { > > > + dev_err(&mgr->dev, "Error while parsing FPGA image header\n"); > > > + mgr->state = FPGA_MGR_STATE_PARSE_HEADER_ERR; > > > + } > > > + > > > + return ret; > > > +} > > > + > > > +/* > > > + * Copy scattered FPGA image fragments to temporary buffer and call the > > > + * low level driver's parse_header function. This should be called after > > > + * fpga_mgr_parse_header_sg_first() returned -EAGAIN. In case of success, > > > + * pointer to the newly allocated image header copy will be set into *ret_buf > > > + * and its size will be returned. *ret_buf needs to be freed by caller. > > > + */ > > > +static ssize_t fpga_mgr_parse_header_sg(struct fpga_manager *mgr, > > > + struct fpga_image_info *info, > > > + struct sg_table *sgt, char **ret_buf) > > > > Since the function allocs buffer for the user, I suggest it still returns > > the buffer pointer. The buffer size could be an output parameter. > > > > OK, will do that. > > > > +{ > > > + size_t len, header_size = mgr->mops->initial_header_size; > > > + char *buf = NULL; > > > + int ret; > > > + > > > + do { > > > + if (info->header_size) > > > + header_size = info->header_size; > > > + > > > + buf = krealloc(buf, header_size, GFP_KERNEL); > > > + if (!buf) { > > > + ret = -ENOMEM; > > > > If you need to return ERROR value, ERR_PTR() could be used. > > > > > > BTW: Have you tried to test your FPGA programming using sg buffers? I want > > to ensure these changes are actually tested? > > > > Of course, but not every single error path, to be honest. Yet, I'm > trying my best. > > > Thanks, > > Yilun > > > > > + break; > > > + } > > > + > > > + len = sg_copy_to_buffer(sgt->sgl, sgt->nents, buf, header_size); > > > + if (len != header_size) { > > > + ret = -EFAULT; > > > + break; > > > + } > > > + > > > + ret = fpga_mgr_parse_header(mgr, info, buf, header_size); > > > + if (ret == -EAGAIN && info->header_size <= header_size) { > > > + dev_err(&mgr->dev, "Requested invalid header size\n"); > > > + ret = -EFAULT; > > > + } > > > + } while (ret == -EAGAIN); > > > + > > > + if (ret) { > > > + dev_err(&mgr->dev, "Error while parsing FPGA image header\n"); > > > + mgr->state = FPGA_MGR_STATE_PARSE_HEADER_ERR; > > > + kfree(buf); > > > + buf = NULL; > > > + } else { > > > + ret = header_size; > > > + } > > > + > > > + *ret_buf = buf; > > > + > > > + 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,13 +294,13 @@ 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; > > > + ssize_t header_size; > > > + char *header_buf; > > > int ret; > > > > > > if (!mgr->mops->initial_header_size) > > > @@ -180,26 +310,35 @@ static int fpga_mgr_write_init_sg(struct fpga_manager *mgr, > > > * First try to use miter to map the first fragment to access the > > > * header, this is the typical path. > > > */ > > > - 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); > > > + ret = fpga_mgr_parse_header_sg_first(mgr, info, sgt); > > > + /* If 0, header fits first fragment, call write_init on it */ > > > + if (!ret) { > > > + sg_miter_start(&miter, sgt->sgl, sgt->nents, SG_MITER_FROM_SG); > > > + if (sg_miter_next(&miter)) { > > > + ret = fpga_mgr_write_init_buf(mgr, info, miter.addr, > > > + miter.length); > > > + sg_miter_stop(&miter); > > > + return ret; > > > + } > > > sg_miter_stop(&miter); > > > + /* > > > + * If -EAGAIN, more sg buffer is needed, > > > + * otherwise an error has occurred. > > > + */ > > > + } else if (ret != -EAGAIN) > > > 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; > > > + /* > > > + * Otherwise copy the fragments into temporary memory. > > > + * Copying is done inside fpga_mgr_parse_header_sg() > > > + */ > > > + header_size = fpga_mgr_parse_header_sg(mgr, info, sgt, &header_buf); > > > + if (header_size < 0) > > > + return header_size; > > > > > > - 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); > > > + ret = fpga_mgr_write_init_buf(mgr, info, header_buf, header_size); > > > > > > - kfree(buf); > > > + kfree(header_buf); > > > > > > return ret; > > > } > > > @@ -227,7 +366,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 +376,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 +430,21 @@ static int fpga_mgr_buf_load_mapped(struct fpga_manager *mgr, > > > { > > > int ret; > > > > > > + ret = fpga_mgr_parse_header_mapped(mgr, info, buf, count); > > > + if (ret) > > > + return ret; > > > + > > > 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 +603,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 > > >
diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c index 08dc85fcd511..0854fbc8f11e 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,145 @@ 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 with entire FPGA image + * buffer on the input. This will set info->header_size and info->data_size. + */ +static int fpga_mgr_parse_header_mapped(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 (info->header_size + info->data_size > count) { + dev_err(&mgr->dev, "Bitsream data outruns FPGA image\n"); + ret = -EINVAL; + } + + if (ret) { + dev_err(&mgr->dev, "Error while parsing FPGA image header\n"); + mgr->state = FPGA_MGR_STATE_PARSE_HEADER_ERR; + } + + return ret; +} + +/* + * Call the low level driver's parse_header function with first fragment of + * scattered FPGA image on the input. If header fits first fragment, + * parse_header will set info->header_size and info->data_size. If it is not, + * parse_header will set desired size to info->header_size and -EAGAIN will be + * returned. + */ +static int fpga_mgr_parse_header_sg_first(struct fpga_manager *mgr, + struct fpga_image_info *info, + struct sg_table *sgt) +{ + size_t header_size = mgr->mops->initial_header_size; + struct sg_mapping_iter miter; + int ret; + + mgr->state = FPGA_MGR_STATE_PARSE_HEADER; + + sg_miter_start(&miter, sgt->sgl, sgt->nents, SG_MITER_FROM_SG); + if (sg_miter_next(&miter) && + miter.length >= header_size) + ret = fpga_mgr_parse_header(mgr, info, miter.addr, miter.length); + else + ret = -EAGAIN; + sg_miter_stop(&miter); + + if (ret && ret != -EAGAIN) { + dev_err(&mgr->dev, "Error while parsing FPGA image header\n"); + mgr->state = FPGA_MGR_STATE_PARSE_HEADER_ERR; + } + + return ret; +} + +/* + * Copy scattered FPGA image fragments to temporary buffer and call the + * low level driver's parse_header function. This should be called after + * fpga_mgr_parse_header_sg_first() returned -EAGAIN. In case of success, + * pointer to the newly allocated image header copy will be set into *ret_buf + * and its size will be returned. *ret_buf needs to be freed by caller. + */ +static ssize_t fpga_mgr_parse_header_sg(struct fpga_manager *mgr, + struct fpga_image_info *info, + struct sg_table *sgt, char **ret_buf) +{ + size_t len, header_size = mgr->mops->initial_header_size; + char *buf = NULL; + int ret; + + do { + if (info->header_size) + header_size = info->header_size; + + buf = krealloc(buf, header_size, GFP_KERNEL); + if (!buf) { + ret = -ENOMEM; + break; + } + + len = sg_copy_to_buffer(sgt->sgl, sgt->nents, buf, header_size); + if (len != header_size) { + ret = -EFAULT; + break; + } + + ret = fpga_mgr_parse_header(mgr, info, buf, header_size); + if (ret == -EAGAIN && info->header_size <= header_size) { + dev_err(&mgr->dev, "Requested invalid header size\n"); + ret = -EFAULT; + } + } while (ret == -EAGAIN); + + if (ret) { + dev_err(&mgr->dev, "Error while parsing FPGA image header\n"); + mgr->state = FPGA_MGR_STATE_PARSE_HEADER_ERR; + kfree(buf); + buf = NULL; + } else { + ret = header_size; + } + + *ret_buf = buf; + + 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,13 +294,13 @@ 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; + ssize_t header_size; + char *header_buf; int ret; if (!mgr->mops->initial_header_size) @@ -180,26 +310,35 @@ static int fpga_mgr_write_init_sg(struct fpga_manager *mgr, * First try to use miter to map the first fragment to access the * header, this is the typical path. */ - 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); + ret = fpga_mgr_parse_header_sg_first(mgr, info, sgt); + /* If 0, header fits first fragment, call write_init on it */ + if (!ret) { + sg_miter_start(&miter, sgt->sgl, sgt->nents, SG_MITER_FROM_SG); + if (sg_miter_next(&miter)) { + ret = fpga_mgr_write_init_buf(mgr, info, miter.addr, + miter.length); + sg_miter_stop(&miter); + return ret; + } sg_miter_stop(&miter); + /* + * If -EAGAIN, more sg buffer is needed, + * otherwise an error has occurred. + */ + } else if (ret != -EAGAIN) 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; + /* + * Otherwise copy the fragments into temporary memory. + * Copying is done inside fpga_mgr_parse_header_sg() + */ + header_size = fpga_mgr_parse_header_sg(mgr, info, sgt, &header_buf); + if (header_size < 0) + return header_size; - 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); + ret = fpga_mgr_write_init_buf(mgr, info, header_buf, header_size); - kfree(buf); + kfree(header_buf); return ret; } @@ -227,7 +366,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 +376,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 +430,21 @@ static int fpga_mgr_buf_load_mapped(struct fpga_manager *mgr, { int ret; + ret = fpga_mgr_parse_header_mapped(mgr, info, buf, count); + if (ret) + return ret; + 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 +603,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);
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> --- drivers/fpga/fpga-mgr.c | 243 +++++++++++++++++++++++++++++----- include/linux/fpga/fpga-mgr.h | 17 ++- 2 files changed, 229 insertions(+), 31 deletions(-)