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

[Aditya Garg]

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