| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Z7jlORk0MiMFTmp6@smile.fi.intel.com>
Date: Fri, 21 Feb 2025 22:42:33 +0200
From: "andriy.shevchenko@...ux.intel.com" <andriy.shevchenko@...ux.intel.com>
To: Aditya Garg <gargaditya08@...e.com>
Cc: "pmladek@...e.com" <pmladek@...e.com>,
Steven Rostedt <rostedt@...dmis.org>,
"linux@...musvillemoes.dk" <linux@...musvillemoes.dk>,
"senozhatsky@...omium.org" <senozhatsky@...omium.org>,
Jonathan Corbet <corbet@....net>,
"maarten.lankhorst@...ux.intel.com" <maarten.lankhorst@...ux.intel.com>,
"mripard@...nel.org" <mripard@...nel.org>,
"tzimmermann@...e.de" <tzimmermann@...e.de>,
"airlied@...il.com" <airlied@...il.com>,
"simona@...ll.ch" <simona@...ll.ch>,
Andrew Morton <akpm@...ux-foundation.org>,
"apw@...onical.com" <apw@...onical.com>,
"joe@...ches.com" <joe@...ches.com>,
"dwaipayanray1@...il.com" <dwaipayanray1@...il.com>,
"lukas.bulwahn@...il.com" <lukas.bulwahn@...il.com>,
"sumit.semwal@...aro.org" <sumit.semwal@...aro.org>,
"christian.koenig@....com" <christian.koenig@....com>,
Kerem Karabay <kekrby@...il.com>, Aun-Ali Zaidi <admin@...eit.net>,
Orlando Chamberlain <orlandoch.dev@...il.com>,
Atharva Tiwari <evepolonium@...il.com>,
"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
"dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
"linux-media@...r.kernel.org" <linux-media@...r.kernel.org>,
"linaro-mm-sig@...ts.linaro.org" <linaro-mm-sig@...ts.linaro.org>,
Hector Martin <marcan@...can.st>,
"linux@...linux.org.uk" <linux@...linux.org.uk>,
Asahi Linux Mailing List <asahi@...ts.linux.dev>,
Sven Peter <sven@...npeter.dev>, Janne Grunau <j@...nau.net>
Subject: Re: [PATCH v3 3/3] drm/tiny: add driver for Apple Touch Bars in x86
Macs
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
Powered by blists - more mailing lists