> 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.
