> 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?
>
>
>
> ...
>
>>>> + 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;
>>>
>>> For three different cases the same error code, can it be adjusted more to the
>>> situation?
>>
>> All these are I/O errors, you got any suggestion?
>
> Your email client mangled the code so badly that it's hard to read. But I would
> suggest to use -EINTR in the first case, and -EBADMSG. But also you may consider
> -EPROTO.
Thanks
>
>>>> + }
>
> ...
>
>>>> + if (ret)
>>>> + return ret;
>>>
>>>> + else if (!new_plane_state->visible)
>>>
>>> Why 'else'? It's redundant.
>>
>> I’ve just followed what other drm drivers are doing here:
>>
>> https://elixir.bootlin.com/linux/v6.13.3/source/drivers/gpu/drm/tiny/bochs.c#L436
>> https://elixir.bootlin.com/linux/v6.13.3/source/drivers/gpu/drm/tiny/cirrus.c#L363
>>
>> And plenty more
>
> A bad example is still a bad example. 'else' is simply redundant in this
> case and add a noisy to the code.
>
>> I won’t mind removing else. You want that?
>
> Sure.
>
> ...
>
>>>> + request_size = ALIGN(sizeof(struct appletbdrm_fb_request) +
>>>> + frames_size +
>>>> + sizeof(struct appletbdrm_fb_request_footer), 16);
>>>
>>> Missing header for ALIGN().
>>>
>>> But have you checked overflow.h for the possibility of using some helper macros
>>> from there? This is what should be usually done for k*alloc() in the kernel.
>>
>> I don’t really think we need a macro here.
>
> Hmm... is frames_size known to be in a guaranteed range to make sure no
> potential overflow happens?
I don’t really find any cause of potential overflow.
>
>>>> + appletbdrm_state->request = kzalloc(request_size, GFP_KERNEL);
>>>> +
>>>> + if (!appletbdrm_state->request)
>>>> + return -ENOMEM;
>
> ...
>
>>>> + request->msg_id = timestamp & 0xff;
>>>
>>> Why ' & 0xff'?
>>
>> https://github.com/imbushuo/DFRDisplayKm/blob/master/src/DFRDisplayKm/DfrDisplay.c#L147
>
> This is not an answer.
> Why do you need this here? Isn't the type of msg_id enough?
Hmm, I double checked this. msg_id is u8 in the Linux port so would anyways never exceed 0xff. I’ll remove this.
Its different in the Windows driver.
>
> ...
>
>>>> + adev->mode = (struct drm_display_mode) {
>>>
>>> Why do you need a compound literal here? Perhaps you want to have that to be
>>> done directly in DRM_MODE_INIT()?
>>
>> I really don’t find this as an issue. You want me to declare another structure, basically this?:
>
> Nope, I'm asking if the DRM_MODE_INIT() is done in a way that it only can be
> used for the static data. Seems like the case. Have you tried to convert
> DRM_MODE_INIT() to be always a compound literal? Does it break things?
Seems to be breaking things.
>
>> struct drm_display_mode mode = {
>> DRM_MODE_INIT(60, adev->height, adev->width,
>> DRM_MODE_RES_MM(adev->height, 218),
>> DRM_MODE_RES_MM(adev->width, 218))
>> };
>> adev->mode = mode;
>>
>>>> + DRM_MODE_INIT(60, adev->height, adev->width,
>>>> + DRM_MODE_RES_MM(adev->height, 218),
>>>> + DRM_MODE_RES_MM(adev->width, 218))
>>>> + };
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
