linux-kernel - Re: [PATCH 2.6.33] s3c-fb: Fix handling of missing refresh in platform data

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: <20100114073617.GT3738@trinity.fluff.org>
Date:	Thu, 14 Jan 2010 07:36:17 +0000
From:	Ben Dooks <ben-linux@...ff.org>
To:	Mark Brown <broonie@...nsource.wolfsonmicro.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Ben Dooks <ben-linux@...ff.org>, linux-kernel@...r.kernel.org,
	stable@...nel.org
Subject: Re: [PATCH 2.6.33] s3c-fb: Fix handling of missing refresh in
	platform data

On Wed, Jan 13, 2010 at 04:16:42PM +0000, Mark Brown wrote:
> Commit 600ce1a0faafeed1ce6bcfd421bc040b941cbbc1 ("fix clock setting
> for Samsung SoC Framebuffer") introduced a mandatory refresh parameter
> to the platform data for the S3C framebuffer but did not introduce any
> validation code, causing existing platforms to divide by zero whenever
> the framebuffer is configured, generating warnings and unusable output.

Hmm, having just read 600ce1a0faafeed1ce6bcfd421bc040b941cbbc1 it seems
the the original authour has has missed the point of what is happening
and thus possibly broken the driver.

We get passed the pixclk in the fb settings to produce the required
refresh rate for the given parameters. We do not need to think about
the refresh rate in the driver itself, the caller should have deal with
it!.

Secondly, s3c_fb_calc_pixclk() can only be called from the root fb,
as there is only one divider in the block used to generate the output
pixel clock for the LCD bus. Adding a window id to the s3c_fb_calc_pixclk()
is useless as the subordinate framebuffers for the overlay windows do not
have their own dividers. as noted in the code it only ever uses this
in the case of win_no == 0, so it was unecessarey.

Thirdly he's changed the code to calculate the divider to assume pixclk
is a frequency, and not a time-period. It clearly stats in the docs
that pixclk is in picoseconds. Our clock rate is in Hz.

It seems I wasn't cc'd on the original submission or I'd have said no.

I'd rather see this one reverted.

> Add a WARN_ON(), in line with similar parameters, and check that we
> have a non-zero refresh before we dividing by it. This gets usable
> output on at least the SMDK6410, though without specifying a refresh
> we see visible issues on some boots.
> 
> Signed-off-by: Mark Brown <broonie@...nsource.wolfsonmicro.com>
> Cc: stable@...nel.org
> ---
> 
> All mainline users of the s3c-fb driver are affected by this, none
> provide refresh.
> 
>  drivers/video/s3c-fb.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/video/s3c-fb.c b/drivers/video/s3c-fb.c
> index adf9632..0f2e8f6 100644
> --- a/drivers/video/s3c-fb.c
> +++ b/drivers/video/s3c-fb.c
> @@ -224,7 +224,8 @@ static int s3c_fb_calc_pixclk(unsigned char id, struct s3c_fb *sfb, unsigned int
>  	unsigned long clk = clk_get_rate(sfb->bus_clk);
>  	unsigned int result;
>  
> -	pixclk *= win->win_mode.refresh;
> +	if (win->win_mode.refresh)
> +		pixclk *= win->win_mode.refresh;
>  	result = clk / pixclk;
>  
>  	dev_dbg(sfb->dev, "pixclk=%u, clk=%lu, div=%d (%lu)\n",
> @@ -766,6 +767,7 @@ static int __devinit s3c_fb_probe_win(struct s3c_fb *sfb, unsigned int win_no,
>  	WARN_ON(windata->max_bpp == 0);
>  	WARN_ON(windata->win_mode.xres == 0);
>  	WARN_ON(windata->win_mode.yres == 0);
> +	WARN_ON(windata->win_mode.refresh == 0);
>  
>  	win = fbinfo->par;
>  	var = &fbinfo->var;
> -- 
> 1.6.6
> 

-- 
-- 
Ben

Q:      What's a light-year?
A:      One-third less calories than a regular year.

--
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/