Re: [PATCH v3 3/3] drm/tiny: add driver for Apple Touch Bars in x86 Macs

[Aditya Garg]

> On 22 Feb 2025, at 5:52 PM, Aditya Garg <gargaditya08@xxxxxxxx> wrote:
> 
> 
> 
>> On 22 Feb 2025, at 2:37 PM, Aditya Garg <gargaditya08@xxxxxxxx> 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.