diff mbox series

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

Message ID 6bed7a69367856080a62e3ee89df6a2a3d0d5f20.1623846327.git.mchehab+huawei@kernel.org
State New
Headers show
Series Address some smatch warnings | expand

Commit Message

Mauro Carvalho Chehab June 16, 2021, 12:28 p.m. UTC 
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.

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/media/i2c/adv7842.c | 33 ++++++++++++++++++++++-----------
 1 file changed, 22 insertions(+), 11 deletions(-)

Comments

Hans Verkuil June 16, 2021, 12:32 p.m. UTC | #1 
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);
>
kernel test robot June 17, 2021, 6:26 a.m. UTC | #2 
Hi Mauro,

I love your patch! Perhaps something to improve:

[auto build test WARNING on linuxtv-media/master]
[also build test WARNING on next-20210616]
[cannot apply to v5.13-rc6]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Mauro-Carvalho-Chehab/Address-some-smatch-warnings/20210617-091510
base:   git://linuxtv.org/media_tree.git master
config: arc-allyesconfig (attached as .config)
compiler: arceb-elf-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/3bdf84a7467fed26b64ffe547f5989d73060a30e
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Mauro-Carvalho-Chehab/Address-some-smatch-warnings/20210617-091510
        git checkout 3bdf84a7467fed26b64ffe547f5989d73060a30e
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   drivers/media/i2c/adv7842.c: In function 'edid_write_vga_segment':
>> drivers/media/i2c/adv7842.c:739:19: warning: comparison between pointer and integer

     739 |  for (i = 0; && i < blocks * EDID_BLOCK_SIZE; i += I2C_SMBUS_BLOCK_MAX) {
         |                   ^
   drivers/media/i2c/adv7842.c:739:2: error: label 'i' used but not defined
     739 |  for (i = 0; && i < blocks * EDID_BLOCK_SIZE; i += I2C_SMBUS_BLOCK_MAX) {
         |  ^~~


vim +739 drivers/media/i2c/adv7842.c

   715	
   716	static int edid_write_vga_segment(struct v4l2_subdev *sd)
   717	{
   718		struct i2c_client *client = v4l2_get_subdevdata(sd);
   719		struct adv7842_state *state = to_state(sd);
   720		const u8 *edid = state->vga_edid.edid;
   721		u32 blocks = state->vga_edid.blocks;
   722		int err = 0;
   723		int i;
   724	
   725		v4l2_dbg(2, debug, sd, "%s: write EDID on VGA port\n", __func__);
   726	
   727		if (!state->vga_edid.present)
   728			return 0;
   729	
   730		/* HPA disable on port A and B */
   731		io_write_and_or(sd, 0x20, 0xcf, 0x00);
   732	
   733		/* Disable I2C access to internal EDID ram from VGA DDC port */
   734		rep_write_and_or(sd, 0x7f, 0x7f, 0x00);
   735	
   736		/* edid segment pointer '1' for VGA port */
   737		rep_write_and_or(sd, 0x77, 0xef, 0x10);
   738	
 > 739		for (i = 0; && i < blocks * EDID_BLOCK_SIZE; i += I2C_SMBUS_BLOCK_MAX) {

   740			err = i2c_smbus_write_i2c_block_data(state->i2c_edid, i,
   741							     I2C_SMBUS_BLOCK_MAX,
   742							     edid + i);
   743			if (err)
   744				return err;
   745		}
   746	
   747		/* Calculates the checksums and enables I2C access
   748		 * to internal EDID ram from VGA DDC port.
   749		 */
   750		rep_write_and_or(sd, 0x7f, 0x7f, 0x80);
   751	
   752		for (i = 0; i < 1000; i++) {
   753			if (rep_read(sd, 0x79) & 0x20)
   754				break;
   755			mdelay(1);
   756		}
   757		if (i == 1000) {
   758			v4l_err(client, "error enabling edid on VGA port\n");
   759			return -EIO;
   760		}
   761	
   762		/* enable hotplug after 200 ms */
   763		schedule_delayed_work(&state->delayed_work_enable_hotplug, HZ / 5);
   764	
   765		return 0;
   766	}
   767	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
kernel test robot June 17, 2021, 8:15 a.m. UTC | #3 
Hi Mauro,

I love your patch! Yet something to improve:

[auto build test ERROR on linuxtv-media/master]
[also build test ERROR on next-20210616]
[cannot apply to v5.13-rc6]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Mauro-Carvalho-Chehab/Address-some-smatch-warnings/20210617-091510
base:   git://linuxtv.org/media_tree.git master
config: alpha-randconfig-r025-20210617 (attached as .config)
compiler: alpha-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/3bdf84a7467fed26b64ffe547f5989d73060a30e
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Mauro-Carvalho-Chehab/Address-some-smatch-warnings/20210617-091510
        git checkout 3bdf84a7467fed26b64ffe547f5989d73060a30e
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=alpha 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All error/warnings (new ones prefixed by >>):

   drivers/media/i2c/adv7842.c: In function 'edid_write_vga_segment':
>> drivers/media/i2c/adv7842.c:739:19: warning: comparison between pointer and integer

     739 |  for (i = 0; && i < blocks * EDID_BLOCK_SIZE; i += I2C_SMBUS_BLOCK_MAX) {
         |                   ^
>> drivers/media/i2c/adv7842.c:739:2: error: label 'i' used but not defined

     739 |  for (i = 0; && i < blocks * EDID_BLOCK_SIZE; i += I2C_SMBUS_BLOCK_MAX) {
         |  ^~~


vim +/i +739 drivers/media/i2c/adv7842.c

   715	
   716	static int edid_write_vga_segment(struct v4l2_subdev *sd)
   717	{
   718		struct i2c_client *client = v4l2_get_subdevdata(sd);
   719		struct adv7842_state *state = to_state(sd);
   720		const u8 *edid = state->vga_edid.edid;
   721		u32 blocks = state->vga_edid.blocks;
   722		int err = 0;
   723		int i;
   724	
   725		v4l2_dbg(2, debug, sd, "%s: write EDID on VGA port\n", __func__);
   726	
   727		if (!state->vga_edid.present)
   728			return 0;
   729	
   730		/* HPA disable on port A and B */
   731		io_write_and_or(sd, 0x20, 0xcf, 0x00);
   732	
   733		/* Disable I2C access to internal EDID ram from VGA DDC port */
   734		rep_write_and_or(sd, 0x7f, 0x7f, 0x00);
   735	
   736		/* edid segment pointer '1' for VGA port */
   737		rep_write_and_or(sd, 0x77, 0xef, 0x10);
   738	
 > 739		for (i = 0; && i < blocks * EDID_BLOCK_SIZE; i += I2C_SMBUS_BLOCK_MAX) {

   740			err = i2c_smbus_write_i2c_block_data(state->i2c_edid, i,
   741							     I2C_SMBUS_BLOCK_MAX,
   742							     edid + i);
   743			if (err)
   744				return err;
   745		}
   746	
   747		/* Calculates the checksums and enables I2C access
   748		 * to internal EDID ram from VGA DDC port.
   749		 */
   750		rep_write_and_or(sd, 0x7f, 0x7f, 0x80);
   751	
   752		for (i = 0; i < 1000; i++) {
   753			if (rep_read(sd, 0x79) & 0x20)
   754				break;
   755			mdelay(1);
   756		}
   757		if (i == 1000) {
   758			v4l_err(client, "error enabling edid on VGA port\n");
   759			return -EIO;
   760		}
   761	
   762		/* enable hotplug after 200 ms */
   763		schedule_delayed_work(&state->delayed_work_enable_hotplug, HZ / 5);
   764	
   765		return 0;
   766	}
   767	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
diff mbox series

Patch

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) {
 		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);