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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <8bd19526-58a3-440b-a6ba-4cd123cb55e6@linux.dev>
Date: Thu, 21 Mar 2024 11:43:35 -0400
From: Sean Anderson <sean.anderson@...ux.dev>
To: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
Cc: Michal Simek <michal.simek@....com>, David Airlie <airlied@...il.com>,
 linux-kernel@...r.kernel.org, Daniel Vetter <daniel@...ll.ch>,
 linux-arm-kernel@...ts.infradead.org,
 Laurent Pinchart <laurent.pinchart@...asonboard.com>,
 Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
 Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>,
 dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH v2 4/8] drm: zynqmp_dp: Rearrange zynqmp_dp for better
 padding

On 3/20/24 02:14, Tomi Valkeinen wrote:
> On 20/03/2024 00:51, Sean Anderson wrote:
>> Sort the members of struct zynqmp_dp to reduce padding necessary for
>> alignment.
>>
>> Signed-off-by: Sean Anderson <sean.anderson@...ux.dev>
>> ---
>>
>> Changes in v2:
>> - New
>>
>>   drivers/gpu/drm/xlnx/zynqmp_dp.c | 28 ++++++++++++++--------------
>>   1 file changed, 14 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
>> index 8635b5673386..f1834c8e3c02 100644
>> --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
>> +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
>> @@ -255,10 +255,10 @@ struct zynqmp_dp_link_config {
>>    * @fmt: format identifier string
>>    */
>>   struct zynqmp_dp_mode {
>> -    u8 bw_code;
>> -    u8 lane_cnt;
>> -    int pclock;
>>       const char *fmt;
>> +    int pclock;
>> +    u8 bw_code;
>> +    u8 lane_cnt;
>>   };
>>     /**
>> @@ -295,27 +295,27 @@ struct zynqmp_dp_config {
>>    * @train_set: set of training data
>>    */
>>   struct zynqmp_dp {
>> +    struct drm_dp_aux aux;
>> +    struct drm_bridge bridge;
>> +    struct delayed_work hpd_work;
>> +
>> +    struct drm_bridge *next_bridge;
>>       struct device *dev;
>>       struct zynqmp_dpsub *dpsub;
>>       void __iomem *iomem;
>>       struct reset_control *reset;
>> -    int irq;
>> -
>> -    struct drm_bridge bridge;
>> -    struct drm_bridge *next_bridge;
>> -
>> -    struct zynqmp_dp_config config;
>> -    struct drm_dp_aux aux;
>>       struct phy *phy[ZYNQMP_DP_MAX_LANES];
>> -    u8 num_lanes;
>> -    struct delayed_work hpd_work;
>> +
>>       enum drm_connector_status status;
>> +    int irq;
>>       bool enabled;
>>   -    u8 dpcd[DP_RECEIVER_CAP_SIZE];
>> -    struct zynqmp_dp_link_config link_config;
>>       struct zynqmp_dp_mode mode;
>> +    struct zynqmp_dp_link_config link_config;
>> +    struct zynqmp_dp_config config;
>> +    u8 dpcd[DP_RECEIVER_CAP_SIZE];
>>       u8 train_set[ZYNQMP_DP_MAX_LANES];
>> +    u8 num_lanes;
>>   };
>>     static inline struct zynqmp_dp *bridge_to_dp(struct drm_bridge *bridge)
> 
> If you change the order of the fields, you should change the order in the kernel doc accordingly.

The kernel doc is documentation, so it should continue to group similar
functionality together.

> To be honest, I'm not sure if I like this patch. We have usually one instance of these structs allocated. How many bytes do we save?

Actually, the main reason is to make it easy to determine where to
insert new members. Stick the pointers with the pointers, u8s with u8s,
etc.

> I'm fine with getting easy savings by changing the field order in some cases, but I think the "human" side of the order is important too: usually the fields are grouped in some way, and ordered so that the more base or generic ones are first, and fields for some specific feature are later. And fields protected by a lock should be grouped together, with their lock being first/last in that group.
> 
> Looking at the zynqmp_dp struct with this patch, I get an urge to start moving things around: dev, dpsub, iomem, etc first, hpd somewhere later. Base config fields like config, num_lanes, irq would be grouped together. Etc.

--Sean

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ