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

["andriy.shevchenko@linux.intel.com" <andriy.shevchenko@linux.intel.com>]

On Fri, Feb 21, 2025 at 07:13:06PM +0000, Aditya Garg wrote:
> > On Fri, Feb 21, 2025 at 11:37:57AM +0000, Aditya Garg wrote:

...

> >> +} __packed;
> > 
> > Why __packed? Please explain and justify for all the data types that are marked
> > with this attribute.
> 
> Just following the original Windows driver here (#pragma pack) :
> 
> https://github.com/imbushuo/DFRDisplayKm/blob/master/src/DFRDisplayKm/include/DFRHostIo.h
> 
> IMO these structures are used for communication with the Touch Bar over USB.
> The hardware expects a very specific layout for the data it receives and
> sends. If the compiler were to insert padding for alignment, it would break
> the communication protocol because the fields would not be in the expected
> positions.

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.

...

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

> >> + }

...

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

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

...

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

> 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