linux-kernel - Re: [PATCH] drm/panel: samsung: s6e8aa0: Add backlight control support

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: <20200405142715.GA28291@ravnborg.org>
Date:   Sun, 5 Apr 2020 16:27:15 +0200
From:   Sam Ravnborg <sam@...nborg.org>
To:     Joonas Kylmälä <joonas.kylmala@....fi>
Cc:     Andrzej Hajda <a.hajda@...sung.com>,
        Thierry Reding <thierry.reding@...il.com>,
        David Airlie <airlied@...ux.ie>,
        Daniel Vetter <daniel@...ll.ch>,
        dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
        paul.kocialkowski@...tlin.com, GNUtoo@...erdimension.org
Subject: Re: [PATCH] drm/panel: samsung: s6e8aa0: Add backlight control
 support

Hi Joonas.

On Sat, Apr 04, 2020 at 04:27:02PM +0300, Joonas Kylmälä wrote:
> Hi,
> 
> addressing this email to you all since there might be widespread race
> condition issue in the DRM panel drivers that are using MIPI DSI. See
> below for my message.
> 
> Andrzej Hajda:
> >> +static int s6e8aa0_set_brightness(struct backlight_device *bd)
> >> +{
> >> +	struct s6e8aa0 *ctx = bl_get_data(bd);
> >> +	const u8 *gamma;
> >> +
> >> +	if (ctx->error)
> >> +		return;
> >> +
> >> +	gamma = ctx->variant->gamma_tables[bd->props.brightness];
> >> +
> >> +	if (ctx->version >= 142)
> >> +		s6e8aa0_elvss_nvm_set(ctx);
> >> +
> >> +	s6e8aa0_dcs_write(ctx, gamma, GAMMA_TABLE_LEN);
> >> +
> >> +	/* update gamma table. */
> >> +	s6e8aa0_dcs_write_seq_static(ctx, 0xf7, 0x03);
> >> +
> >> +	return s6e8aa0_clear_error(ctx);
> >> +}
> >> +
> >> +static const struct backlight_ops s6e8aa0_backlight_ops = {
> >> +	.update_status	= s6e8aa0_set_brightness,
> > 
> > 
> > This is racy, update_status can be called in any time between probe and
> > remove, particularly:
> > 
> > a) before panel enable,
> > 
> > b) during panel enable,
> > 
> > c) when panel is enabled,
> > 
> > d) during panel disable,
> > 
> > e) after panel disable,
> > 
> > 
> > b and d are racy for sure - backlight and drm callbacks are async.
> > 
> > IMO the best solution would be to register backlight after attaching
> > panel to drm, but for this drm_panel_funcs should have attach/detach
> > callbacks (like drm_bridge_funcs),
> > 
> > then update_status callback should take some drm_connector lock to
> > synchronize with drm, and write to hw only when pipe is enabled.
> 
> I have done now research and if I understand right one issue here might
> be with setting the backlight brightness if the DSI device is not
> attached before calling update_status() since calling it would call
> subsequently s6e8aa0_set_brightness() -> s6e8aa0_dcs_write() ->
> mipi_dsi_dcs_write_buffer(), which then requires DSI to be attached.

Not directly related to your comments above.
But I have looked at the backlight support for the various
samsung panels.

None of them are good examples to follow.
Please have a look at for example panel-novatek-nt35510.c
which is a good example how to have a local backligth
and tie it into the general way it is used by drm_panel.

I have typed patches to fix all three samsung panels, will
post patches later when I get more time.

If we are concerned with set_brightness() being called
while not ready, this can be checked in the
set_brightness() function and return error if not OK.

As the the race concerns see Daniel's reply.

	Sam