Re: [PATCH 08/11] media: adv7842: better document EDID block size

[Hans Verkuil <hverkuil-cisco@xs4all.nl>]

On 16/06/2021 14:28, Mauro Carvalho Chehab wrote:
> While the logic there is correct, it leads to smatch warnings:
> 	/home/hans/work/build/media-git/drivers/media/i2c/adv7842.c:2538 adv7842_set_edid() error: memcpy() '&state->vga_edid.edid' too small (128 vs 512)
> 
> Because the code tricks static analyzers by doing:
> 	memcpy(&state->hdmi_edid.edid, e->edid, 128 * e->blocks);
> 
> for ADV7842_EDID_PORT_VGA, where a logic before that makes
> e->blocks being either 0 or 1.
> 
> Yet, it is ugly to see the "128" magic number all spread about the
> EDID code. So, while here, replace 128 (and 4 x 128) by macros:
> 
> And ensure that the logic which copy into the VGA block
> will use EDID_MAX_VGA_BLOCKS.

Please drop this patch, there is already another patch from me for adv7842 in a pending PR.

> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> ---
>  drivers/media/i2c/adv7842.c | 33 ++++++++++++++++++++++-----------
>  1 file changed, 22 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/media/i2c/adv7842.c b/drivers/media/i2c/adv7842.c
> index 78e61fe6f2f0..30bddab320b9 100644
> --- a/drivers/media/i2c/adv7842.c
> +++ b/drivers/media/i2c/adv7842.c
> @@ -85,6 +85,10 @@ struct adv7842_format_info {
>  	u8 op_format_sel;
>  };
>  
> +#define EDID_BLOCK_SIZE		128
> +#define EDID_MAX_HDMI_BLOCKS	4
> +#define EDID_MAX_VGA_BLOCKS	1
> +
>  struct adv7842_state {
>  	struct adv7842_platform_data pdata;
>  	struct v4l2_subdev sd;
> @@ -98,12 +102,12 @@ struct adv7842_state {
>  
>  	v4l2_std_id norm;
>  	struct {
> -		u8 edid[512];
> +		u8 edid[EDID_BLOCK_SIZE * EDID_MAX_HDMI_BLOCKS];
>  		u32 blocks;
>  		u32 present;
>  	} hdmi_edid;
>  	struct {
> -		u8 edid[128];
> +		u8 edid[EDID_MAX_VGA_BLOCKS * EDID_MAX_VGA_BLOCKS];
>  		u32 blocks;
>  		u32 present;
>  	} vga_edid;
> @@ -732,12 +736,13 @@ static int edid_write_vga_segment(struct v4l2_subdev *sd)
>  	/* edid segment pointer '1' for VGA port */
>  	rep_write_and_or(sd, 0x77, 0xef, 0x10);
>  
> -	for (i = 0; !err && i < blocks * 128; i += I2C_SMBUS_BLOCK_MAX)
> +	for (i = 0; && i < blocks * EDID_BLOCK_SIZE; i += I2C_SMBUS_BLOCK_MAX) {

Besides, this change won't compile, so:

Nacked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

Regards,

	Hans

>  		err = i2c_smbus_write_i2c_block_data(state->i2c_edid, i,
>  						     I2C_SMBUS_BLOCK_MAX,
>  						     edid + i);
> -	if (err)
> -		return err;
> +		if (err)
> +			return err;
> +	}
>  
>  	/* Calculates the checksums and enables I2C access
>  	 * to internal EDID ram from VGA DDC port.
> @@ -785,7 +790,7 @@ static int edid_write_hdmi_segment(struct v4l2_subdev *sd, u8 port)
>  		return 0;
>  	}
>  
> -	pa = v4l2_get_edid_phys_addr(edid, blocks * 128, &spa_loc);
> +	pa = v4l2_get_edid_phys_addr(edid, blocks * EDID_BLOCK_SIZE, &spa_loc);
>  	err = v4l2_phys_addr_validate(pa, &parent_pa, NULL);
>  	if (err)
>  		return err;
> @@ -800,7 +805,7 @@ static int edid_write_hdmi_segment(struct v4l2_subdev *sd, u8 port)
>  	}
>  
>  
> -	for (i = 0; !err && i < blocks * 128; i += I2C_SMBUS_BLOCK_MAX) {
> +	for (i = 0; !err && i < blocks * EDID_BLOCK_SIZE; i += I2C_SMBUS_BLOCK_MAX) {
>  		/* set edid segment pointer for HDMI ports */
>  		if (i % 256 == 0)
>  			rep_write_and_or(sd, 0x77, 0xef, i >= 256 ? 0x10 : 0x00);
> @@ -2491,7 +2496,9 @@ static int adv7842_get_edid(struct v4l2_subdev *sd, struct v4l2_edid *edid)
>  	if (edid->start_block + edid->blocks > blocks)
>  		edid->blocks = blocks - edid->start_block;
>  
> -	memcpy(edid->edid, data + edid->start_block * 128, edid->blocks * 128);
> +	memcpy(edid->edid,
> +	       data + edid->start_block * EDID_BLOCK_SIZE,
> +	       edid->blocks * EDID_BLOCK_SIZE);
>  
>  	return 0;
>  }
> @@ -2506,9 +2513,12 @@ static int adv7842_get_edid(struct v4l2_subdev *sd, struct v4l2_edid *edid)
>  static int adv7842_set_edid(struct v4l2_subdev *sd, struct v4l2_edid *e)
>  {
>  	struct adv7842_state *state = to_state(sd);
> -	unsigned int max_blocks = e->pad == ADV7842_EDID_PORT_VGA ? 1 : 4;
> +	unsigned int max_blocks;
>  	int err = 0;
>  
> +	max_blocks = e->pad == ADV7842_EDID_PORT_VGA ?
> +		     EDID_MAX_VGA_BLOCKS  : EDID_MAX_HDMI_BLOCKS;
> +
>  	memset(e->reserved, 0, sizeof(e->reserved));
>  
>  	if (e->pad > ADV7842_EDID_PORT_VGA)
> @@ -2535,7 +2545,7 @@ static int adv7842_set_edid(struct v4l2_subdev *sd, struct v4l2_edid *e)
>  		state->vga_edid.blocks = e->blocks;
>  		state->vga_edid.present = e->blocks ? 0x1 : 0x0;
>  		if (e->blocks)
> -			memcpy(&state->vga_edid.edid, e->edid, 128 * e->blocks);
> +			memcpy(&state->vga_edid.edid, e->edid, EDID_BLOCK_SIZE);
>  		err = edid_write_vga_segment(sd);
>  		break;
>  	case ADV7842_EDID_PORT_A:
> @@ -2544,7 +2554,8 @@ static int adv7842_set_edid(struct v4l2_subdev *sd, struct v4l2_edid *e)
>  		state->hdmi_edid.blocks = e->blocks;
>  		if (e->blocks) {
>  			state->hdmi_edid.present |= 0x04 << e->pad;
> -			memcpy(&state->hdmi_edid.edid, e->edid, 128 * e->blocks);
> +			memcpy(&state->hdmi_edid.edid, e->edid,
> +			       EDID_BLOCK_SIZE * e->blocks);
>  		} else {
>  			state->hdmi_edid.present &= ~(0x04 << e->pad);
>  			adv7842_s_detect_tx_5v_ctrl(sd);
>