linux-kernel - Re: [PATCH 2/2] drm/panel: novatek-nt35560: Fix bug and clean up

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: <DBNGMHS14LUB.3PDFCB3DI1789@brighamcampbell.com>
Date: Mon, 28 Jul 2025 00:04:32 -0600
From: "Brigham Campbell" <me@...ghamcampbell.com>
To: "Doug Anderson" <dianders@...omium.org>
Cc: <maarten.lankhorst@...ux.intel.com>, <mripard@...nel.org>,
 <tzimmermann@...e.de>, <airlied@...il.com>, <simona@...ll.ch>,
 <linus.walleij@...aro.org>, <neil.armstrong@...aro.org>,
 <jessica.zhang@....qualcomm.com>, <sam@...nborg.org>,
 <skhan@...uxfoundation.org>, <linux-kernel-mentees@...ts.linux.dev>,
 <dri-devel@...ts.freedesktop.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/2] drm/panel: novatek-nt35560: Fix bug and clean up

On Fri Jul 25, 2025 at 3:17 PM MDT, Doug Anderson wrote:
> Hi,
>
> On Thu, Jul 24, 2025 at 1:23 PM Brigham Campbell <me@...ghamcampbell.com> wrote:
>>
>> Fix bug in nt35560_set_brightness() which causes the function to
>> erroneously report an error. mipi_dsi_dcs_write() returns either a
>> negative value when an error occurred or a positive number of bytes
>> written when no error occurred. The buggy code reports and error under
>> either condition.
>
> My personal preference would be to code up the fix itself (without the
> multi transition) as patch #1. That will make everyone's lives easier
> with stable backports. You'll touch the same code twice in your
> series, but it will keep it cleaner...

Oh, this is good to know. It makes sense to me that a lazer-focused bug
fix would be less likely to conflict with other changes in stable
branches and would be easier to resolve in the case of conflict. I'll
just fix the bug in patch 1/3 of v2.

>> The usage of the u8 array, mipi_buf_out, in nt35560_set_brightness() may
>> be a little curious. It's useful here because pwm_ratio and pwm_div
>> aren't constant, therefore we must store them in a buffer at runtime.
>>
>> Using mipi_dsi_dcs_write_{seq,buffer}_multi() in place of
>> mipi_dsi_dcs_write() gives the added benefit that kmalloc() isn't used
>> to write mipi commands.
>
> Ah, this makes sense. We've seen this before, but I keep forgetting
> about it. Thanks for mentioning it. I wonder if it makes sense to have
> variants of mipi_dsi_generic_write_seq_multi() and
> mipi_dsi_dcs_write_seq_multi() that take non-const data. The only
> difference would be that the array they declare on the stack would be
> a "const" array instead of a "static const" array...

Ok, I've thought about this one for a while. The problem with my patch
as it is now is that it uses a u8 array, mipi_buf_out, to construct MIPI
messages and send them out. My patch reuses mipi_buf_out because it
happens to be the right size for both messages which need to be
constructed at runtime. Not a super clean solution, perhaps.

The Novatek NT35950 has a better solution. See the following function
from drivers/gpu/drm/panel/panel-novatek-nt35950.c:107:

static void nt35950_set_cmd2_page(struct mipi_dsi_multi_context *dsi_ctx,
				  struct nt35950 *nt, u8 page)
{
	const u8 mauc_cmd2_page[] = { MCS_CMD_MAUCCTR, 0x55, 0xaa, 0x52,
				      0x08, page };

	mipi_dsi_dcs_write_buffer_multi(dsi_ctx, mauc_cmd2_page,
					ARRAY_SIZE(mauc_cmd2_page));
	if (!dsi_ctx->accum_err)
		nt->last_page = page;
}

The driver has a couple different functions like this and they're all
for the express purpose of writing out a single MIPI buffer which is
constructed at runtime.

Arguably, a more readable solution would involve the definition of a new
non-static macro like you suggest. The macro's `do {} while 0;` block
would achieve effectively the exact same effect as the functions in the
NT35950 driver, causing the buffer to be popped off the stack as soon as
the code inside the macro completed.

We could call it mipi_dsi_dcs_write_var_seq_multi(), perhaps. Or
mipi_dsi_dcs_write_sequence_of_bytes_determined_at_runtime_multi()? ...
(Help! I genuinely don't know what I would call it...)

Please let me know if you'd prefer that in v2 I adopt the approach that
the NT35950 driver uses or that I instead introduce a new macro for
non-static data.

I'll address the rest of your comments in v2.

Thanks again for another thorough review,
Brigham