linux-kernel - Re: radeonfb i2c regression post-2.6.18.

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] [thread-next>] [day] [month] [year] [list]

Message-Id: <200711231700.53103.mb@bu3sch.de>
Date:	Fri, 23 Nov 2007 17:00:52 +0100
From:	Michael Buesch <mb@...sch.de>
To:	benh@...nel.crashing.org
Cc:	Roger Leigh <rleigh@...nlatter.ukfsn.org>,
	Jean Delvare <khali@...ux-fr.org>,
	linux-kernel@...r.kernel.org, Dennis Munsie <dmunsie@...ropia.com>
Subject: Re: radeonfb i2c regression post-2.6.18.

On Monday 19 November 2007 00:16:06 Benjamin Herrenschmidt wrote:
> (Also, Michel, can you check if it fixes your other problem with this
> code ? ie. your "hot crash")

> -------- Forwarded Message --------
> From: Jean Delvare <khali@...ux-fr.org>
> To: linux-fbdev-devel@...ts.sourceforge.net
> Cc: Benjamin Herrenschmidt <benh@...nel.crashing.org>, Antonino Daplas
> <adaplas@...il.com>
> Subject: [PATCH] fb_ddc: Fix DDC lines quirk
> Date: Sun, 18 Nov 2007 14:21:41 +0100
> 
> The code in fb_ddc_read() is said to be based on the implementation
> of the radeon driver:
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=fc5891c8a3ba284f13994d7bc1f1bfa8283982de
> 
> However, comparing the old radeon driver code with the new fb_ddc code
> reveals some differences. Most notably, the I2C bus lines are held at
> the end of the function, while the original code was releasing them
> (as the comment above correctly says.)
> 
> There are a few other differences, which appear to be responsible for
> read failures on my system. While tracing low-level I2C code in
> i2c-algo-bit, I noticed that the initial attempt to read the EDID
> always failed. It takes one retry for the read to succeed. As we are
> about to remove this automatic retry property from i2c-algo-bit,
> reading the EDID would really fail.
> 
> As a summary, the I2C lines quirk which is supposedly needed to read
> EDID on some older monitors is currently breaking the (first) read on
> all other monitors (and might not even work with older ones - did
> anyone try since October 2006?)
> 
> After applying the patch below, which makes the code in fb_ddc_read()
> really similar to what the radeon driver used to have, the first EDID
> read succeeds again.
> 
> On top of that, as it appears that this code has been broken for one
> year now and nobody seems to have complained, I'm curious if it makes
> sense to keep this quirk in place. It makes the code more complex and
> slower just for the sake of monitors which I guess nobody uses
> anymore. Can't we just get rid of it?

This patch fixes my crash problem.
Thanks a lot guys!

> Signed-off-by: Jean Delvare <khali@...ux-fr.org>

Acked-by: Michael Buesch <mb@...sch.de>

> ---
>  drivers/video/fb_ddc.c |    8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> --- linux-2.6.24-rc3.orig/drivers/video/fb_ddc.c	2007-11-17 20:23:03.000000000 +0100
> +++ linux-2.6.24-rc3/drivers/video/fb_ddc.c	2007-11-18 12:49:14.000000000 +0100
> @@ -56,13 +56,12 @@ unsigned char *fb_ddc_read(struct i2c_ad
>  	int i, j;
>  
>  	algo_data->setscl(algo_data->data, 1);
> -	algo_data->setscl(algo_data->data, 0);
>  
>  	for (i = 0; i < 3; i++) {
>  		/* For some old monitors we need the
>  		 * following process to initialize/stop DDC
>  		 */
> -		algo_data->setsda(algo_data->data, 0);
> +		algo_data->setsda(algo_data->data, 1);
>  		msleep(13);
>  
>  		algo_data->setscl(algo_data->data, 1);
> @@ -97,14 +96,15 @@ unsigned char *fb_ddc_read(struct i2c_ad
>  		algo_data->setsda(algo_data->data, 1);
>  		msleep(15);
>  		algo_data->setscl(algo_data->data, 0);
> +		algo_data->setsda(algo_data->data, 0);
>  		if (edid)
>  			break;
>  	}
>  	/* Release the DDC lines when done or the Apple Cinema HD display
>  	 * will switch off
>  	 */
> -	algo_data->setsda(algo_data->data, 0);
> -	algo_data->setscl(algo_data->data, 0);
> +	algo_data->setsda(algo_data->data, 1);
> +	algo_data->setscl(algo_data->data, 1);
>  
>  	return edid;
>  }
> 
> 
> 
> 
> 



-- 
Greetings Michael.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/