linux-kernel - Re: [PATCH v4 2/2] drm/tiny: add driver for Apple Touch Bars in x86 Macs

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: <Z7yMIC1wxm0AGBCt@smile.fi.intel.com>
Date: Mon, 24 Feb 2025 17:11:28 +0200
From: "andriy.shevchenko@...ux.intel.com" <andriy.shevchenko@...ux.intel.com>
To: Aditya Garg <gargaditya08@...e.com>
Cc: "maarten.lankhorst@...ux.intel.com" <maarten.lankhorst@...ux.intel.com>,
	"mripard@...nel.org" <mripard@...nel.org>,
	"tzimmermann@...e.de" <tzimmermann@...e.de>,
	"airlied@...il.com" <airlied@...il.com>,
	"simona@...ll.ch" <simona@...ll.ch>,
	Kerem Karabay <kekrby@...il.com>,
	Atharva Tiwari <evepolonium@...il.com>,
	Aun-Ali Zaidi <admin@...eit.net>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	"dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>
Subject: Re: [PATCH v4 2/2] drm/tiny: add driver for Apple Touch Bars in x86
 Macs

On Mon, Feb 24, 2025 at 03:03:40PM +0000, Aditya Garg wrote:
> > On 24 Feb 2025, at 8:27 PM, andriy.shevchenko@...ux.intel.com wrote:
> > On Mon, Feb 24, 2025 at 02:32:37PM +0000, Aditya Garg wrote:
> >>> On 24 Feb 2025, at 7:30 PM, andriy.shevchenko@...ux.intel.com wrote:
> >>> On Mon, Feb 24, 2025 at 01:40:20PM +0000, Aditya Garg wrote:

...

> >>>> +#define __APPLETBDRM_MSG_STR4(str4) ((__le32 __force)((str4[0] << 24) | (str4[1] << 16) | (str4[2] << 8) | str4[3]))
> >>> 
> >>> As commented previously this is quite strange what's going on with endianess in
> >>> this driver. Especially the above weirdness when get_unaligned_be32() is being
> >>> open coded and force-cast to __le32.
> >> 
> >> I would assume it was also mimicked from the Windows driver, though I haven't
> >> really tried exploring this there.
> >> 
> >> I’d rather be happy if you give me code change suggestions and let me review
> >> and test them
> > 
> > For the starter I would do the following for all related constants and
> > drop that weird and ugly macros at the top (it also has an issue with
> > the str4 length as it is 5 bytes long, not 4, btw):
> > 
> >  #define APPLETBDRM_MSG_CLEAR_DISPLAY cpu_to_le32(0x434c5244) /* CLRD */
> 
> Lemme test this.

Just in case it won't work, reverse bytes in the integer. Because I was lost in
this conversion.

> > (assuming we stick with __leXX for now). This will be much less confusing.

...

> >> Alright. For some reason (a mistake on my part), some dev_err_probe were also
> >> still left in this version.
> > 
> > But those are seems to me in the correct locations, no? How do we even know
> > the DRM device before its creation? So, dev_err_probe() calls in ->probe()
> > seem logical to me. Somebody from DRM should clarify this.
> 
> Thomas asked me to do this change. Maybe you didn’t see his reply.

I saw, maybe I took it wrong, but I really don't understand how on earth
drm_err()  or whatever can be used in real ->probe() of the physical device.

Imagine the hypotetical case

probe(strict device *dev)
{
	mydrm;
	foo;
	...
	foo = devm_gpiod_get(dev, ...);
	if (IS_ERR(foo))
		return dev_err_probe(dev, ...); // how?!
	...
	mydrm = ...DRM alloc...;
	...
}

I don't even believe it will be possible to create drm_err_probe() as it most
likely will require to have an allocation to be always the first op (that may
fail) in the ->probe() which might be not the case for some device drivers.

> >>>> +  */

-- 
With Best Regards,
Andy Shevchenko