From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail.marcansoft.com (marcansoft.com [212.63.210.85]) (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 B60F04416 for ; Mon, 5 Sep 2022 15:39:35 +0000 (UTC) Received: from [127.0.0.1] (localhost [127.0.0.1]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits)) (No client certificate requested) (Authenticated sender: marcan@marcan.st) by mail.marcansoft.com (Postfix) with ESMTPSA id 1BEBD41E2F; Mon, 5 Sep 2022 15:39:29 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=marcan.st; s=default; t=1662392373; bh=CPuvfrj+bg5Wc9yyPhZWo6SV0wB4yQZN+c8KRS6Vr0M=; h=Date:To:Cc:References:From:Subject:In-Reply-To; b=LT1ztgMe28WggocrB6F19ZACYhsxHVDmsk+iwtNrNvzj+lOCx5kjsgfiRpkPSzOrG nBaJVrL92+YDDJNF+BXuuJ56XLyxZMGT9l98bqF4tw8tYs9ruu90oCDcAb7vNixJsg HYJPoWne6D+uFDb3/N0SXrs9WUEasqyLYzK1c+8L/d/n9Eh7gJldvZFymICpe09nk2 aD0YmCVKpnJjNHqM5QAaIFwU66O09LVWtNHX/7zJHZSFFva/uxKzwmJYBzlZ6fYqOM ev/uZr6LEgSLKyWfOSBXeWSlfIW9lQ1P5E9Aqa2tvFUDS5TewVfbTYnt6etvY2J0dG 5haqKHo9caKfA== Message-ID: <2dbeef40-7ab8-a5e7-ce47-0c27720b947c@marcan.st> Date: Tue, 6 Sep 2022 00:39:27 +0900 Precedence: bulk X-Mailing-List: asahi@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.9.1 Content-Language: en-US To: "Russell King (Oracle)" , Andy Shevchenko Cc: Arnd Bergmann , Lee Jones , Linus Walleij , Alyssa Rosenzweig , asahi@lists.linux.dev, Bartosz Golaszewski , linux-arm Mailing List , "open list:GPIO SUBSYSTEM" , Sven Peter References: From: Hector Martin Subject: Re: [PATCH 5/6] gpio: Add new gpio-macsmc driver for Apple Macs In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit On 05/09/2022 19.20, Russell King (Oracle) wrote: > So, I'm going with my first suggestion for the hex2bin() conversion > above, and adding a comment thusly: > > /* > * The most significant nibble comes from k[0] and key bits 15..8 > * The least significant nibble comes from k[1] and key bits 7..0 > */ > k[0] = key >> 8; > k[1] = key; > > because I needed the comment to prove to myself that I wasn't breaking > this code. Maybe it's obvious to you, but it isn't obvious to everyone. Honestly, I don't see what this buys us over the original code. It's longer, no more readable, makes you think about the order that characters are stored in the string as an extra indirection step, which as as you found out with having to write the comment, is easy to get backwards. But I will say it is at least *semantically* correct, if awkward. Let's back up and talk a bit about SMC keys for a second. First, SMC is a legacy mess and plays around with endianness wrong in several places - there are values which are in wrong-endian for no reason, etc. So any discussion over "what would happen on a big-endian platform" is ultimately speculation. If this ever ends up running on some ancient PowerPC Mac (did any of those ever ship with an SMC that followed these semantics?) then we'll have to deal with the endianness issues then and correct any incorrect assumptions, because right now we just don't have the information on what Apple's *intent* was when designing this whole thing, if there was an intent at all. That said. When I designed this driver, and the way I understand the hardware, I consider SMC keys to be 32-bit integers containing packed ASCII characters in natural integer order, that is, 0xAABBCCDD representing the fourcc ABCD in that order. The SMC backend is designed with this in mind, and puts things in the right endian in the right contexts when it comes to the actual interface with the SMC coprocessor (which is, itself, a mix of shared memory - which is a bag of bytes - and 64-bit mailbox messages - which are fundamentally integers and merely represented in little-endian at the hardware level - so I'm sure how you can see how this gets interesting). In other words, at the driver level, *SMC keys are not character strings, nor integers stored in some byte order*. They are integers. Integers do not have a byte order until they are stored to memory. Therefore, using functions that operate on strings on SMC keys is wrong, and requires you to make a trip through endian-land to get it right (as you found out). Making the representation of SMC keys in the driver 32-bit integers makes manipulating them easier and ergonomic in C, and allows for things like comparisons (look at how the GPIO code uses < to compare SMC keys, which maps to ASCIIbetical sort the way the keys are naturally encoded), while basically relegating all the endian issues to the SMC core. For comparison, if the data structure were a char[4] in reading order, there would be no ergonomic way to do comparisons without some helper function/macro. And comparisons are used quite a bit as part of the self-discovery aspects of SMC (there's that binary search function to find key indices, which also took like 4 tries to get right... please don't break it! :). This is why I added a printk specifier, because V4L/etc already had a very special-purpose specifier with fancy rules just for them, and I think a generic FOURCC style format specifier that works in any context is useful (this isn't the only driver dealing with this kind of FOURCC-style construct). The printk patch in particular adds 4 variations to the existing v4l specifier that that interpret endianness differently, so it can be used in any context (in this context, the specifier is 'h' which means 'host endian' and is the correct specifier for abstract integers, which are passed by reference in this case and therefore inherit the host endianness). - Hector From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id B57B0ECAAD5 for ; Mon, 5 Sep 2022 18:20:00 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:Subject:From:References:Cc: To:MIME-Version:Date:Message-ID:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=6fP5YV92vxvEkO3qscQSsZGNUlEJgwtJsTd1qY5QhSE=; b=QV6a+o4qyICAct 3vzi4vl5hOxzzhagyKNTKlbq/kfrYKFSIRrNPZY0CgnCiYL5wZguwHqU15cFWgw+/ZYymIJZq4qs7 wUYgOKR4buB516UrO7Nbi0+aLKR9cDjj6Oi5RnaKvCk/DmJ/0Fg6LxFLovG1Ge57fpVffedIVP6b5 TDO3RSTu8bGva3XfKGjlM/em9ykMY3T92nybTDmuDt9SKCV9YM0VEk9YcCfm2OryoB0yAqLniC/dG yhoW+0YUiZPmnd415CRvuU9SokfXy8F0nzIpdeklMvgaqYlC7p4/5xDkyyny8codAQ1DFdOQxNQuI bQzw4VGvD9mpBur5iZJA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1oVGgC-008HZ9-S2; Mon, 05 Sep 2022 18:18:41 +0000 Received: from marcansoft.com ([212.63.210.85] helo=mail.marcansoft.com) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1oVECF-0066vh-Tq for linux-arm-kernel@lists.infradead.org; Mon, 05 Sep 2022 15:39:38 +0000 Received: from [127.0.0.1] (localhost [127.0.0.1]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits)) (No client certificate requested) (Authenticated sender: marcan@marcan.st) by mail.marcansoft.com (Postfix) with ESMTPSA id 1BEBD41E2F; Mon, 5 Sep 2022 15:39:29 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=marcan.st; s=default; t=1662392373; bh=CPuvfrj+bg5Wc9yyPhZWo6SV0wB4yQZN+c8KRS6Vr0M=; h=Date:To:Cc:References:From:Subject:In-Reply-To; b=LT1ztgMe28WggocrB6F19ZACYhsxHVDmsk+iwtNrNvzj+lOCx5kjsgfiRpkPSzOrG nBaJVrL92+YDDJNF+BXuuJ56XLyxZMGT9l98bqF4tw8tYs9ruu90oCDcAb7vNixJsg HYJPoWne6D+uFDb3/N0SXrs9WUEasqyLYzK1c+8L/d/n9Eh7gJldvZFymICpe09nk2 aD0YmCVKpnJjNHqM5QAaIFwU66O09LVWtNHX/7zJHZSFFva/uxKzwmJYBzlZ6fYqOM ev/uZr6LEgSLKyWfOSBXeWSlfIW9lQ1P5E9Aqa2tvFUDS5TewVfbTYnt6etvY2J0dG 5haqKHo9caKfA== Message-ID: <2dbeef40-7ab8-a5e7-ce47-0c27720b947c@marcan.st> Date: Tue, 6 Sep 2022 00:39:27 +0900 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.9.1 Content-Language: en-US To: "Russell King (Oracle)" , Andy Shevchenko Cc: Arnd Bergmann , Lee Jones , Linus Walleij , Alyssa Rosenzweig , asahi@lists.linux.dev, Bartosz Golaszewski , linux-arm Mailing List , "open list:GPIO SUBSYSTEM" , Sven Peter References: From: Hector Martin Subject: Re: [PATCH 5/6] gpio: Add new gpio-macsmc driver for Apple Macs In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220905_083936_490071_9D268277 X-CRM114-Status: GOOD ( 23.85 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On 05/09/2022 19.20, Russell King (Oracle) wrote: > So, I'm going with my first suggestion for the hex2bin() conversion > above, and adding a comment thusly: > > /* > * The most significant nibble comes from k[0] and key bits 15..8 > * The least significant nibble comes from k[1] and key bits 7..0 > */ > k[0] = key >> 8; > k[1] = key; > > because I needed the comment to prove to myself that I wasn't breaking > this code. Maybe it's obvious to you, but it isn't obvious to everyone. Honestly, I don't see what this buys us over the original code. It's longer, no more readable, makes you think about the order that characters are stored in the string as an extra indirection step, which as as you found out with having to write the comment, is easy to get backwards. But I will say it is at least *semantically* correct, if awkward. Let's back up and talk a bit about SMC keys for a second. First, SMC is a legacy mess and plays around with endianness wrong in several places - there are values which are in wrong-endian for no reason, etc. So any discussion over "what would happen on a big-endian platform" is ultimately speculation. If this ever ends up running on some ancient PowerPC Mac (did any of those ever ship with an SMC that followed these semantics?) then we'll have to deal with the endianness issues then and correct any incorrect assumptions, because right now we just don't have the information on what Apple's *intent* was when designing this whole thing, if there was an intent at all. That said. When I designed this driver, and the way I understand the hardware, I consider SMC keys to be 32-bit integers containing packed ASCII characters in natural integer order, that is, 0xAABBCCDD representing the fourcc ABCD in that order. The SMC backend is designed with this in mind, and puts things in the right endian in the right contexts when it comes to the actual interface with the SMC coprocessor (which is, itself, a mix of shared memory - which is a bag of bytes - and 64-bit mailbox messages - which are fundamentally integers and merely represented in little-endian at the hardware level - so I'm sure how you can see how this gets interesting). In other words, at the driver level, *SMC keys are not character strings, nor integers stored in some byte order*. They are integers. Integers do not have a byte order until they are stored to memory. Therefore, using functions that operate on strings on SMC keys is wrong, and requires you to make a trip through endian-land to get it right (as you found out). Making the representation of SMC keys in the driver 32-bit integers makes manipulating them easier and ergonomic in C, and allows for things like comparisons (look at how the GPIO code uses < to compare SMC keys, which maps to ASCIIbetical sort the way the keys are naturally encoded), while basically relegating all the endian issues to the SMC core. For comparison, if the data structure were a char[4] in reading order, there would be no ergonomic way to do comparisons without some helper function/macro. And comparisons are used quite a bit as part of the self-discovery aspects of SMC (there's that binary search function to find key indices, which also took like 4 tries to get right... please don't break it! :). This is why I added a printk specifier, because V4L/etc already had a very special-purpose specifier with fancy rules just for them, and I think a generic FOURCC style format specifier that works in any context is useful (this isn't the only driver dealing with this kind of FOURCC-style construct). The printk patch in particular adds 4 variations to the existing v4l specifier that that interpret endianness differently, so it can be used in any context (in this context, the specifier is 'h' which means 'host endian' and is the correct specifier for abstract integers, which are passed by reference in this case and therefore inherit the host endianness). - Hector _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel