linux-kernel - Re: [PATCH v3 3/3] 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] [day] [month] [year] [list]
Message-ID: <99EA7DFC-E157-4589-9D24-623D4B5A37B6@live.com>
Date: Sun, 23 Feb 2025 14:58:25 +0000
From: Aditya Garg <gargaditya08@...e.com>
To: "andriy.shevchenko@...ux.intel.com" <andriy.shevchenko@...ux.intel.com>
CC: "pmladek@...e.com" <pmladek@...e.com>, Steven Rostedt
	<rostedt@...dmis.org>, "linux@...musvillemoes.dk" <linux@...musvillemoes.dk>,
	"senozhatsky@...omium.org" <senozhatsky@...omium.org>, Jonathan Corbet
	<corbet@....net>, "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>,
	Andrew Morton <akpm@...ux-foundation.org>, "apw@...onical.com"
	<apw@...onical.com>, "joe@...ches.com" <joe@...ches.com>,
	"dwaipayanray1@...il.com" <dwaipayanray1@...il.com>,
	"lukas.bulwahn@...il.com" <lukas.bulwahn@...il.com>,
	"sumit.semwal@...aro.org" <sumit.semwal@...aro.org>,
	"christian.koenig@....com" <christian.koenig@....com>, Kerem Karabay
	<kekrby@...il.com>, Aun-Ali Zaidi <admin@...eit.net>, Orlando Chamberlain
	<orlandoch.dev@...il.com>, Atharva Tiwari <evepolonium@...il.com>,
	"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>, Linux Kernel Mailing
 List <linux-kernel@...r.kernel.org>, "dri-devel@...ts.freedesktop.org"
	<dri-devel@...ts.freedesktop.org>, "linux-media@...r.kernel.org"
	<linux-media@...r.kernel.org>, "linaro-mm-sig@...ts.linaro.org"
	<linaro-mm-sig@...ts.linaro.org>, Hector Martin <marcan@...can.st>,
	"linux@...linux.org.uk" <linux@...linux.org.uk>, Asahi Linux Mailing List
	<asahi@...ts.linux.dev>, Sven Peter <sven@...npeter.dev>, Janne Grunau
	<j@...nau.net>
Subject: Re: [PATCH v3 3/3] drm/tiny: add driver for Apple Touch Bars in x86
 Macs



> On 22 Feb 2025, at 5:52 PM, Aditya Garg <gargaditya08@...e.com> wrote:
> 
> 
> 
>> On 22 Feb 2025, at 2:37 PM, Aditya Garg <gargaditya08@...e.com> wrote:
>> 
>>> 
>>> What padding, please? Why TCP UAPI headers do not have these attributes?
>>> Think about it, and think about what actually __packed does and how it affects
>>> (badly) the code generation. Otherwise it looks like a cargo cult.
>>> 
>>>> I tried removing __packed btw and driver no longer works.
>>> 
>>> So, you need to find a justification why. But definitely not due to padding in
>>> many of them. They can go without __packed as they are naturally aligned.
>> 
>> Alright, I did some debugging, basically printk sizeof(struct). Did it for both packed and unpacked with the following results:
>> 
>> Feb 22 13:02:03 MacBook kernel: size of struct appletbdrm_msg_request_header is 16
>> Feb 22 13:02:03 MacBook kernel: size of struct appletbdrm_msg_request_header_unpacked is 16
>> 
>> Feb 22 13:02:03 MacBook kernel: size of struct appletbdrm_msg_response_header is 20
>> Feb 22 13:02:03 MacBook kernel: size of struct appletbdrm_msg_response_header_unpacked is 20
>> 
>> Feb 22 13:02:03 MacBook kernel: size of struct appletbdrm_msg_simple_request is 32
>> Feb 22 13:02:03 MacBook kernel: size of struct appletbdrm_msg_simple_request_unpacked is 32
>> 
>> Feb 22 13:02:03 MacBook kernel: size of struct appletbdrm_msg_information is 65
>> Feb 22 13:02:03 MacBook kernel: size of struct appletbdrm_msg_information_unpacked is 68
>> 
>> Feb 22 13:02:03 MacBook kernel: size of struct appletbdrm_frame is 12
>> Feb 22 13:02:03 MacBook kernel: size of struct appletbdrm_frame_unpacked is 12
>> 
>> Feb 22 13:02:03 MacBook kernel: size of struct appletbdrm_fb_request_footer is 80
>> Feb 22 13:02:03 MacBook kernel: size of struct appletbdrm_fb_request_footer_unpacked is 80
>> 
>> Feb 22 13:02:03 MacBook kernel: size of struct appletbdrm_fb_request is 48
>> Feb 22 13:02:03 MacBook kernel: size of struct appletbdrm_fb_request_unpacked is 48
>> 
>> Feb 22 13:02:03 MacBook kernel: size of struct appletbdrm_fb_request_response is 40
>> Feb 22 13:02:04 MacBook kernel: size of struct appletbdrm_fb_request_response_unpacked is 40
>> 
>> So, the difference in sizeof in unpacked and packed is only in appletbdrm_msg_information. So, I kept this packed, and removed it from others. The Touch Bar still works.
>> 
>> So maybe keep just this packed?
> 
> And for justification why driver was not working, with appletbdrm_msg_information not packed is because sizeof(struct appletbdrm_msg_information) is being used in kzalloc in the driver. Similar is the case for most other __packed structs.

I tried to debug a bit more and its not really the kzalloc logic, rather its the USB bit that’s causing the issue:

Code:

static int appletbdrm_read_response(struct appletbdrm_device *adev,
				    struct appletbdrm_msg_response_header *response,
				    size_t size, __le32 expected_response)
{
	struct usb_device *udev = adev_to_udev(adev);
	struct drm_device *drm = &adev->drm;
	int ret, actual_size;
	bool readiness_signal_received = false;

retry:
	ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, adev->in_ep),
			   response, size, &actual_size, APPLETBDRM_BULK_MSG_TIMEOUT);
	if (ret) {
		drm_err(drm, "Failed to read response (%d)\n", ret);
		return ret;
	}

	/*
	 * The device responds to the first request sent in a particular
	 * timeframe after the USB device configuration is set with a readiness
	 * signal, in which case the response should be read again
	 */
	if (response->msg == APPLETBDRM_MSG_SIGNAL_READINESS) {
		if (!readiness_signal_received) {
			readiness_signal_received = true;
			goto retry;
		}

		drm_err(drm, "Encountered unexpected readiness signal\n");
		return -EIO;
	}

	if (actual_size != size) {
		drm_err(drm, "Actual size (%d) doesn't match expected size (%lu)\n",
			actual_size, size);
		return -EIO;
	}

	if (response->msg != expected_response) {
		drm_err(drm, "Unexpected response from device (expected %p4ch found %p4ch)\n",
			&expected_response, &response->msg);
		return -EIO;
	}

	return 0;
}

Error:

Feb 23 20:00:29 MacBook kernel: appletbdrm 7-6:2.1: [drm] *ERROR* Actual size (65) doesn't match expected size (68)

So IMO, we still need this struct to remain packed.

> 
> Maybe the author wanted to keep this value consistent across various compiler options? I don’t think CPU architecture really matters here though since this driver is exclusively for x86_64 Intel Macs.