linux-kernel - Re: [PATCH 2/3] media: i2c: adv7511: remove open coded version of SMBus block read

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20210127101814.GB928@ninjato>
Date:   Wed, 27 Jan 2021 11:18:14 +0100
From:   Wolfram Sang <wsa+renesas@...g-engineering.com>
To:     Hans Verkuil <hverkuil-cisco@...all.nl>
Cc:     linux-i2c@...r.kernel.org, linux-renesas-soc@...r.kernel.org,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        linux-media@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/3] media: i2c: adv7511: remove open coded version of
 SMBus block read

Hi Hans,

> > -		adv7511_edid_rd(sd, 256, &state->edid.data[segment * 256]);
> > +		err = adv7511_edid_rd(sd, 256, &state->edid.data[segment * 256]);
> >  		adv7511_dbg_dump_edid(2, debug, sd, segment, &state->edid.data[segment * 256]);
> 
> Only call adv7511_dbg_dump_edid if err >= 0.

Yes.

> 
> >  		if (segment == 0) {
> 
> Change condition to: err >= 0 && segment == 0

Yes.

> 
> >  			state->edid.blocks = state->edid.data[0x7e] + 1;
> >  			v4l2_dbg(1, debug, sd, "%s: %d blocks in total\n", __func__, state->edid.blocks);
> >  		}

So I guarded the above block with an 'if (!err)' clause.
adv7511_edid_rd() returns either 0 or errno, so we can take the above
simpler condition.



> > -		if (!edid_verify_crc(sd, segment) ||
> > -		    !edid_verify_header(sd, segment)) {
> > +		if (err < 0 || !edid_verify_crc(sd, segment) || !edid_verify_header(sd, segment)) {
> >  			/* edid crc error, force reread of edid segment */
> 
> Hmm, this comment is a bit out of date. Change to:
> 
> 			/*
> 			 * Couldn't read EDID or EDID has invalid content.
> 			 * Force reread of EDID segment.
> 			 */

I updated the comment but kept it a oneliner.

> 
> >  			v4l2_err(sd, "%s: edid crc or header error\n", __func__);
> 
> Only show this message if err >= 0. For err < 0 the adv7511_edid_rd() already
> logs an error.

Yes. I used 'if (!err)' again here.

Will resend in a minute, thanks for the review!

All the best,

   Wolfram


Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)