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
