Re: [PATCH 01/10] stm32mp: stm32prog: remove all the header check for UART download

[Patrice CHOTARD <patrice.chotard@foss.st.com>]

Hi Patrick

On 5/18/21 3:12 PM, Patrick Delaunay wrote:
> This patch removes the header check for UART download;
> the check of checksum is not mandatory with even parity and chuck
> checksum for each 256 received bytes and it is only done for
> STM32 image (FSBL = TF-A BL2), not for FIT image.
> 
> This patch solve issue of duplicated 0x100 byte written with FIP header.
> 
> Fixes: 4fb7b3e10891 ("stm32mp: stm32prog: add FIP header support")
> Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
> ---
> 
>  .../mach-stm32mp/cmd_stm32prog/stm32prog.c    |  14 +-
>  .../mach-stm32mp/cmd_stm32prog/stm32prog.h    |   5 -
>  .../cmd_stm32prog/stm32prog_serial.c          | 151 ++----------------
>  3 files changed, 22 insertions(+), 148 deletions(-)
> 
> diff --git a/arch/arm/mach-stm32mp/cmd_stm32prog/stm32prog.c b/arch/arm/mach-stm32mp/cmd_stm32prog/stm32prog.c
> index 4c4d8a7a69..84b880261a 100644
> --- a/arch/arm/mach-stm32mp/cmd_stm32prog/stm32prog.c
> +++ b/arch/arm/mach-stm32mp/cmd_stm32prog/stm32prog.c
> @@ -369,23 +369,24 @@ static int parse_flash_layout(struct stm32prog_data *data,
>  	bool end_of_line, eof;
>  	char *p, *start, *last, *col;
>  	struct stm32prog_part_t *part;
> +	struct image_header_s header;
>  	int part_list_size;
>  	int i;
>  
>  	data->part_nb = 0;
>  
>  	/* check if STM32image is detected */
> -	stm32prog_header_check((struct raw_header_s *)addr, &data->header);
> -	if (data->header.type == HEADER_STM32IMAGE) {
> +	stm32prog_header_check((struct raw_header_s *)addr, &header);
> +	if (header.type == HEADER_STM32IMAGE) {
>  		u32 checksum;
>  
>  		addr = addr + BL_HEADER_SIZE;
> -		size = data->header.image_length;
> +		size = header.image_length;
>  
> -		checksum = stm32prog_header_checksum(addr, &data->header);
> -		if (checksum != data->header.image_checksum) {
> +		checksum = stm32prog_header_checksum(addr, &header);
> +		if (checksum != header.image_checksum) {
>  			stm32prog_err("Layout: invalid checksum : 0x%x expected 0x%x",
> -				      checksum, data->header.image_checksum);
> +				      checksum, header.image_checksum);
>  			return -EIO;
>  		}
>  	}
> @@ -1727,7 +1728,6 @@ void stm32prog_clean(struct stm32prog_data *data)
>  	free(data->part_array);
>  	free(data->otp_part);
>  	free(data->buffer);
> -	free(data->header_data);
>  }
>  
>  /* DFU callback: used after serial and direct DFU USB access */
> diff --git a/arch/arm/mach-stm32mp/cmd_stm32prog/stm32prog.h b/arch/arm/mach-stm32mp/cmd_stm32prog/stm32prog.h
> index 581b10d0ac..ad404879a7 100644
> --- a/arch/arm/mach-stm32mp/cmd_stm32prog/stm32prog.h
> +++ b/arch/arm/mach-stm32mp/cmd_stm32prog/stm32prog.h
> @@ -132,14 +132,9 @@ struct stm32prog_data {
>  	u32			*otp_part;
>  	u8			pmic_part[PMIC_SIZE];
>  
> -	/* STM32 header information */
> -	struct raw_header_s	*header_data;
> -	struct image_header_s	header;
> -
>  	/* SERIAL information */
>  	u32	cursor;
>  	u32	packet_number;
> -	u32	checksum;
>  	u8	*buffer; /* size = USART_RAM_BUFFER_SIZE*/
>  	int	dfu_seq;
>  	u8	read_phase;
> diff --git a/arch/arm/mach-stm32mp/cmd_stm32prog/stm32prog_serial.c b/arch/arm/mach-stm32mp/cmd_stm32prog/stm32prog_serial.c
> index 2b92e3b149..7eca86c11b 100644
> --- a/arch/arm/mach-stm32mp/cmd_stm32prog/stm32prog_serial.c
> +++ b/arch/arm/mach-stm32mp/cmd_stm32prog/stm32prog_serial.c
> @@ -292,56 +292,6 @@ static void stm32prog_serial_putc(u8 w_byte)
>  }
>  
>  /* Helper function ************************************************/
> -
> -static u8 stm32prog_header(struct stm32prog_data *data)
> -{
> -	u8 ret;
> -	u8 boot = 0;
> -	struct dfu_entity *dfu_entity;
> -	u64 size = 0;
> -
> -	dfu_entity = stm32prog_get_entity(data);
> -	if (!dfu_entity)
> -		return -ENODEV;
> -
> -	printf("\nSTM32 download write %s\n", dfu_entity->name);
> -
> -	/* force cleanup to avoid issue with previous read */
> -	dfu_transaction_cleanup(dfu_entity);
> -
> -	stm32prog_header_check(data->header_data, &data->header);
> -
> -	/* no stm32 image header : max size is partition size */
> -	if (data->header.type != HEADER_STM32IMAGE) {
> -		dfu_entity->get_medium_size(dfu_entity, &size);
> -		data->header.image_length = size;
> -	}
> -
> -	/**** Flash the header if necessary for boot partition */
> -	if (data->phase < PHASE_FIRST_USER)
> -		boot = 1;
> -
> -	/* write header if boot partition */
> -	if (boot) {
> -		if (ret) {
> -			stm32prog_err("invalid header (error %d)", ret);
> -		} else {
> -			ret = stm32prog_write(data,
> -					      (u8 *)data->header_data,
> -					      BL_HEADER_SIZE);
> -		}
> -	} else {
> -		if (ret)
> -			printf("  partition without checksum\n");
> -		ret = 0;
> -	}
> -
> -	free(data->header_data);
> -	data->header_data = NULL;
> -
> -	return ret;
> -}
> -
>  static u8 stm32prog_start(struct stm32prog_data *data, u32 address)
>  {
>  	u8 ret = 0;
> @@ -388,23 +338,6 @@ static u8 stm32prog_start(struct stm32prog_data *data, u32 address)
>  		data->dfu_seq = 0;
>  
>  		printf("\n  received length = 0x%x\n", data->cursor);
> -		if (data->header.type == HEADER_STM32IMAGE) {
> -			if (data->cursor !=
> -			    (data->header.image_length + BL_HEADER_SIZE)) {
> -				stm32prog_err("transmission interrupted (length=0x%x expected=0x%x)",
> -					      data->cursor,
> -					      data->header.image_length +
> -					      BL_HEADER_SIZE);
> -				return -EIO;
> -			}
> -			if (data->header.image_checksum != data->checksum) {
> -				stm32prog_err("invalid checksum received (0x%x expected 0x%x)",
> -					      data->checksum,
> -					      data->header.image_checksum);
> -				return -EIO;
> -			}
> -			printf("\n  checksum OK (0x%x)\n", data->checksum);
> -		}
>  
>  		/* update DFU with received flashlayout */
>  		if (data->phase == PHASE_FLASHLAYOUT)
> @@ -627,14 +560,12 @@ static void download_command(struct stm32prog_data *data)
>  	u32 counter = 0x0, codesize = 0x0;
>  	u8 *ramaddress = 0;
>  	u8 rcv_data = 0x0;
> -	struct image_header_s *image_header = &data->header;
>  	u32 cursor = data->cursor;
>  	long size = 0;
>  	u8 operation;
>  	u32 packet_number;
>  	u32 result = ACK_BYTE;
>  	u8 ret;
> -	unsigned int i;
>  	bool error;
>  	int rcv;
>  
> @@ -668,13 +599,8 @@ static void download_command(struct stm32prog_data *data)
>  	if (packet_number == 0) {
>  		/* erase: re-initialize the image_header struct */
>  		data->packet_number = 0;
> -		if (data->header_data)
> -			memset(data->header_data, 0, BL_HEADER_SIZE);
> -		else
> -			data->header_data = calloc(1, BL_HEADER_SIZE);
>  		cursor = 0;
>  		data->cursor = 0;
> -		data->checksum = 0;
>  		/*idx = cursor;*/
>  	} else {
>  		data->packet_number++;
> @@ -746,74 +672,27 @@ static void download_command(struct stm32prog_data *data)
>  		goto end;
>  	}
>  
> -	/* Update current position in buffer */
> -	data->cursor += codesize;
> -
> -	if (operation == PHASE_OTP) {
> -		size = data->cursor - cursor;
> -		/* no header for OTP */
> -		if (stm32prog_otp_write(data, cursor,
> -					data->buffer, &size))
> -			result = ABORT_BYTE;
> -		goto end;
> -	}
> +	switch (operation) {
> +	case PHASE_OTP:
> +		size = codesize;
> +		ret = stm32prog_otp_write(data, cursor, data->buffer, &size);
> +		break;
>  
> -	if (operation == PHASE_PMIC) {
> -		size = data->cursor - cursor;
> -		/* no header for PMIC */
> -		if (stm32prog_pmic_write(data, cursor,
> -					 data->buffer, &size))
> -			result = ABORT_BYTE;
> -		goto end;
> -	}
> +	case PHASE_PMIC:
> +		size = codesize;
> +		ret = stm32prog_pmic_write(data, cursor, data->buffer, &size);
> +		break;
>  
> -	if (cursor < BL_HEADER_SIZE) {
> -		/* size = portion of header in this chunck */
> -		if (data->cursor >= BL_HEADER_SIZE)
> -			size = BL_HEADER_SIZE - cursor;
> -		else
> -			size = data->cursor - cursor;
> -		memcpy((void *)((u32)(data->header_data) + cursor),
> -		       data->buffer, size);
> -		cursor += size;
> -
> -		if (cursor == BL_HEADER_SIZE) {
> -			/* Check and Write the header */
> -			if (stm32prog_header(data)) {
> -				result = ABORT_BYTE;
> -				goto end;
> -			}
> -		} else {
> -			goto end;
> -		}
> +	default:
> +		ret = stm32prog_write(data, data->buffer, codesize);
> +		break;
>  	}
>  
> -	if (data->header.type == HEADER_STM32IMAGE) {
> -		if (data->cursor <= BL_HEADER_SIZE)
> -			goto end;
> -		/* compute checksum on payload */
> -		for (i = (unsigned long)size; i < codesize; i++)
> -			data->checksum += data->buffer[i];
> -
> -		if (data->cursor >
> -		    image_header->image_length + BL_HEADER_SIZE) {
> -			log_err("expected size exceeded\n");
> -			result = ABORT_BYTE;
> -			goto end;
> -		}
> -
> -		/* write data (payload) */
> -		ret = stm32prog_write(data,
> -				      &data->buffer[size],
> -				      codesize - size);
> -	} else {
> -		/* write all */
> -		ret = stm32prog_write(data,
> -				      data->buffer,
> -				      codesize);
> -	}
>  	if (ret)
>  		result = ABORT_BYTE;
> +	else
> +		/* Update current position in buffer */
> +		data->cursor += codesize;
>  
>  end:
>  	stm32prog_serial_result(result);
> 

Reviewed-by: Patrice Chotard <patrice.chotard@foss.st.com>

Thanks
Patrice