From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from pidgin.makrotopia.org (pidgin.makrotopia.org [185.142.180.65]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 461B72FE04D; Fri, 6 Feb 2026 03:14:50 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=185.142.180.65 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1770347690; cv=none; b=KCYk5PGB9xYuJhhuEDYUF0NRh6tQdynaAcNWfD9RvmvzrI80ARGccQR7L2bNKdFMChykCtuWRrTinc9w0N6QkgQ2BKs4WsmlhBIZacm76dusvgdkPqYg85r5+0hZ8o7Hl/AneWcO43QdoXCWmBKxBj2SqAhSqxUd1bA7atUh5bo= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1770347690; c=relaxed/simple; bh=Jc3IZcYW2rB1qgV1vPDAlqvxWXgP++0p0Sj1wrvla9Q=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=BWLT25zFh7QwFv380MqWZBfPkwen5uKVx1TSCsHvPTKQWVJfS+0q+eJcRtd30XdCeIaBThv6J1dv43UY27mlgG/iMwRMU7IjyRzHvi4TidnjNaAze/l4AI3c/BGWEVkxrV4oP++fG2e+x9ypfRjki0naFN+xDDfYYrctuXBfd6w= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=makrotopia.org; spf=pass smtp.mailfrom=makrotopia.org; arc=none smtp.client-ip=185.142.180.65 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=makrotopia.org Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=makrotopia.org Received: from local by pidgin.makrotopia.org with esmtpsa (TLS1.3:TLS_AES_256_GCM_SHA384:256) (Exim 4.99) (envelope-from ) id 1voCIk-000000005tg-3Mqr; Fri, 06 Feb 2026 03:14:34 +0000 Date: Fri, 6 Feb 2026 03:14:26 +0000 From: Daniel Golle To: Jakub Kicinski Cc: Andrew Lunn , Vladimir Oltean , "David S. Miller" , Eric Dumazet , Paolo Abeni , Rob Herring , Krzysztof Kozlowski , Conor Dooley , Heiner Kallweit , Russell King , Simon Horman , netdev@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, Frank Wunderlich , Chad Monroe , Cezary Wilmanski , Liang Xu , John Crispin Subject: Re: [PATCH net-next v13 4/4] net: dsa: add basic initial driver for MxL862xx switches Message-ID: References: <2da8267175bfe7b8ff92d67ba5aa88755fab1710.1770211259.git.daniel@makrotopia.org> <20260205182117.41618f8d@kernel.org> Precedence: bulk X-Mailing-List: netdev@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20260205182117.41618f8d@kernel.org> Hi Jakub, thank you for looking into this driver another time. On Thu, Feb 05, 2026 at 06:21:17PM -0800, Jakub Kicinski wrote: > On Wed, 4 Feb 2026 13:33:19 +0000 Daniel Golle wrote: > > +/* The switch firmware expects all structs to be byte-aligned */ > > +#pragma pack(push, 1) > > "Byte-aligned" means..? Generally aligned means that it starts > at an address which is multiple of X. All addresses are multiple of 1 In case fo the firmware running on this switch it means that data types used in structures used as input and output parameters for firmware functions should be aligned to 8 bits, without any additional padding in between. struct foo { u8 var1; __le16 var2; __le32 var3; } __packed; It's size is 7 bytes and it looks like this: .||||||||.||||||||.||||||||.||||||||.||||||||.||||||||.||||||||. | var1 | var2 | var3 | . . LSB . MSB . LSB MSB . This is what the firmware on the other end expects, from all data sent to it and what the Linux host has to expect from all data received from it. > We used you push back against blanket __packed because it's forcing > all *host* accesses to also assume that the structures are unaligned. Understand that in general, and of course know that using packed structs without a hardware requirement of doing so needlessly wastes CPU cycles on each access to struct members. However, in this case this is a header file which exclusively defines structs which are only used to communicate with the firmware running on the switch. Using them for anything else, such as storing or processing data the driver deals with internally is, very inconvenient because all types are also defined as little-endian, so not only unaligned access, but also endian conversion burdens every access (in the sense that it burdens the programmer on little-endian machines, but the CPU as well on big-endian machines). tl;dr: This whole file is only API definition. And this is how the firmware API is defined, and that's the only way to deal with that switch. (I would have preferred if they just exposed the internal 16-bit registers of the switch via MDIO, and have asked MxL for that several times, without success) > The best practice is to pack only specific structs which need it > and add compile_assert()s to make sure that the compiler doesn't add > any padding. Imho checking whether each of these structs is naturally packed (ie. 8-bit aligned without padding between 8-bit aligned members) is prone to human errors which only become visible when testing on the real hardware, and hence complicates maintainance. Other drivers which operate on similar APIs (many GPU drivers, for example) also use #pragma pack(push, 1) in header files defining external API. Also there all external API definitions are kept in a separate file, away from any of the datastructures used by the driver internally at runtime. Anyway, if you really really want me to set individual __packed for each struct which isn't naturally packed in this whole file, please tell me clearly that this is what you would like, and I will of course do it despite disagreeing with the reasoning. Cheers Daniel