[net-next,v2] net: phy: aquantia: drop wrong endianness conversion for addr and CRC
Message ID | 20231127002924.22384-1-ansuelsmth@gmail.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:ce62:0:b0:403:3b70:6f57 with SMTP id o2csp2761749vqx; Sun, 26 Nov 2023 16:29:49 -0800 (PST) X-Google-Smtp-Source: AGHT+IHu3g+XqE1fRIF+jhwcSR4+QMxsw6Jn+XF8GzQkmpBvzNEFVa49RaviCg7aUqQsyfUeP0Am X-Received: by 2002:a17:903:1108:b0:1bc:edd:e891 with SMTP id n8-20020a170903110800b001bc0edde891mr18857369plh.1.1701044989450; Sun, 26 Nov 2023 16:29:49 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1701044989; cv=none; d=google.com; s=arc-20160816; b=gCDvVbDbyqk1QInXU1AKW0OqRM9TNKuZU+ZbUjhRYdfgGeP6GA2TT2S5W2QNAqzM/y c77H5MZwPApC5e3NOjSQZDbCjPsbaBJOG1SQbp9LPQ+YQEtGtB2O33LzduzNbCNfAyYv b/7InTXRc4u4kgc9c1pJdBFafi3+M4fGpyxc8TIP8L/LmQcWbXdRbe3pOPuhgihZgUa6 RY9JNLGwSQ1S/v8Dr2N+0JRXwTpnyFVc8nQwOo+sv0Pai+tLfAcHpZV7LXuva5sU8M19 gnvZ5u1r8No5gp7A3ilv5XSQzBXafUrZP3Tq/Fay26WEfs2n+ofkyLVy7KbVvE3XWDuk D0tw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:dkim-signature; bh=DcFK3Yx/QJzJGb4Zmy6oTVxw1trZEcqb4ZIR/f9ub9Q=; fh=TLP31TsvNjXfybFHjyS0ptdGep1co6SEP/v6lxPO++U=; b=p8yKmIh28lxHd3tngcPS727KFTF3s1L4AdCAnBOHE8idQegsdBASakYSAyo+ehSlFv Y8KIgm4/bf9TYar6q4zJoGVlRg2+PFYDRb03J3sGIpPVy6PXI3NSboQdfWySuS3pLd0Q Nvnq84eFjCcNJTSLgZ6B5i03G6+5RJ1Bple0qY+ocSW/iUjfId27wBFTMOkTTfMajvFC 0qGGGxZ1N+/EcT1bZDe6pnwHOanwfLx8ixJ86ZxRdaiT5FZkyo0bWx+70R16xHDDIddA 82YLHLemLIEa9opLQUdCB9T/NcUT8CIzihjkrYuJpmV694P8rlOrWZ8mhhZOw2HAy3Pi ueqA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gmail.com header.s=20230601 header.b=hKQFC30U; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: from fry.vger.email (fry.vger.email. [2620:137:e000::3:8]) by mx.google.com with ESMTPS id i1-20020a1709026ac100b001ce5b8cfe7dsi8248341plt.230.2023.11.26.16.29.49 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 26 Nov 2023 16:29:49 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) client-ip=2620:137:e000::3:8; Authentication-Results: mx.google.com; dkim=pass header.i=@gmail.com header.s=20230601 header.b=hKQFC30U; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by fry.vger.email (Postfix) with ESMTP id AE329808726F; Sun, 26 Nov 2023 16:29:43 -0800 (PST) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.11 at fry.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229514AbjK0A3f (ORCPT <rfc822;kernel.ruili@gmail.com> + 99 others); Sun, 26 Nov 2023 19:29:35 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:35850 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229472AbjK0A3e (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Sun, 26 Nov 2023 19:29:34 -0500 Received: from mail-wm1-x32a.google.com (mail-wm1-x32a.google.com [IPv6:2a00:1450:4864:20::32a]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 61C21F0; Sun, 26 Nov 2023 16:29:40 -0800 (PST) Received: by mail-wm1-x32a.google.com with SMTP id 5b1f17b1804b1-40b3e2f8ec5so13275655e9.1; Sun, 26 Nov 2023 16:29:40 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1701044979; x=1701649779; darn=vger.kernel.org; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:from:to:cc:subject:date:message-id:reply-to; bh=DcFK3Yx/QJzJGb4Zmy6oTVxw1trZEcqb4ZIR/f9ub9Q=; b=hKQFC30UIayOeZXvEVmqCFwKlHxR6t5H5FgNSDtOlDbIlohCM7ensc4jWJOWvZ5Vh5 5z6vr9Mzl6dJL3rT19Hd2BUCKPXyTtg7GjkhvPoh6Zt7XuguPGNJWrZy5vm5nRpf0hS2 xs6kY8sj6304tNaAkAwD43+JdLfZvXFWwORvyMQzbJTQepK8kI9Yt7BNyU6u2E6qX352 6La0wlzbraFspunXwMS9U1Rz0fZLcOW8PQROPVIz0QjTHJS918VsFY3LlSqHDUi9b+Gc 5DGUh7zsBiMF95IFjvtWdv5sTP4AnRT7ISpn4SdtCP+I8DBOunc+EFUtQj6tPbM8ebNs QqgQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1701044979; x=1701649779; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=DcFK3Yx/QJzJGb4Zmy6oTVxw1trZEcqb4ZIR/f9ub9Q=; b=YrZ/E/FGGU9Gs6/sMW60KmpiU171w5kdMsOmdbZXujrptotjrw1ALgKHSLRvac4QLR j8hRX5mXZjTjpDW8W9jlycwIuLDzlWo83wgjchIsRak0UKbqhSdkRxcHhj+cusomeUFe suqGJXgYK0UZN829BnwqxjZJ0MG4e7JUDEV+lFF6zbhfWknk7N3anjEptkWzVv3Q/P4C g/yn8EvNQaaE7jzZYuYJm9OAlzEVd3PWXxTC3TrRtoKHsDllldT+gF/uCM/oqPn/xWsW oirmZWxxNn2+6fzjV1kXkbb9qV1As6mm6RcHtlANW77G0PF5J11Ds/zXHvyRW2UgmlVs 1K6g== X-Gm-Message-State: AOJu0YzjI++T8Re8iDbAt9XGA+Dxpgurt+wJ9i5MX5hWIgq7MOXEryuT UETQUb3dLHeQjkZb+zLBDbA= X-Received: by 2002:a05:600c:1d24:b0:408:3696:3d51 with SMTP id l36-20020a05600c1d2400b0040836963d51mr7524526wms.4.1701044978625; Sun, 26 Nov 2023 16:29:38 -0800 (PST) Received: from localhost.localdomain (93-34-89-13.ip49.fastwebnet.it. [93.34.89.13]) by smtp.googlemail.com with ESMTPSA id i12-20020a05600c354c00b004060f0a0fd5sm11738625wmq.13.2023.11.26.16.29.37 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 26 Nov 2023 16:29:38 -0800 (PST) From: Christian Marangi <ansuelsmth@gmail.com> To: Andrew Lunn <andrew@lunn.ch>, Heiner Kallweit <hkallweit1@gmail.com>, Russell King <linux@armlinux.org.uk>, "David S. Miller" <davem@davemloft.net>, Eric Dumazet <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>, Christian Marangi <ansuelsmth@gmail.com>, Robert Marko <robimarko@gmail.com>, netdev@vger.kernel.org, linux-kernel@vger.kernel.org Cc: kernel test robot <lkp@intel.com> Subject: [net-next PATCH v2] net: phy: aquantia: drop wrong endianness conversion for addr and CRC Date: Mon, 27 Nov 2023 01:29:24 +0100 Message-Id: <20231127002924.22384-1-ansuelsmth@gmail.com> X-Mailer: git-send-email 2.40.1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-0.6 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on fry.vger.email Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (fry.vger.email [0.0.0.0]); Sun, 26 Nov 2023 16:29:43 -0800 (PST) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1783674950501626883 X-GMAIL-MSGID: 1783674950501626883 |
Series |
[net-next,v2] net: phy: aquantia: drop wrong endianness conversion for addr and CRC
|
|
Commit Message
Christian Marangi
Nov. 27, 2023, 12:29 a.m. UTC
On further testing on BE target with kernel test robot, it was notice that the endianness conversion for addr and CRC in fw_load_memory was wrong and actually not needed. Values in define doesn't get converted and are passed as is and hardcoded values are already in what the PHY require, that is LE. Use get_unaligned_le32 instead of get_unaligned for FW data word load to correctly convert data in the correct order to follow system endian. Also drop the cpu_to_be32 for CRC calculation as it's wrong and use get_unaligned_be32 instead. The word is taken from firmware and is always LE, the mailbox will emit a BE CRC from BE word hence the word needs to be swapped on u8 to u32 cast on LE system. This is needed as crc_ccitt_false will recast u32 to u8 and read order changes between BE and LE system. By using get_unaligned_be32, word is swapped only when needed resulting in the correct CRC calculated. Reported-by: kernel test robot <lkp@intel.com> Closes: https://lore.kernel.org/oe-kbuild-all/202311210414.sEJZjlcD-lkp@intel.com/ Fixes: e93984ebc1c8 ("net: phy: aquantia: add firmware load support") Tested-by: Robert Marko <robimarko@gmail.com> # ipq8072 LE device Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> --- Changes v2: - Add further explaination in commit description - Fix wrong CRC conversion and swap only when needed drivers/net/phy/aquantia/aquantia_firmware.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
Comments
On Mon, Nov 27, 2023 at 01:29:24AM +0100, Christian Marangi wrote: > On further testing on BE target with kernel test robot, it was notice > that the endianness conversion for addr and CRC in fw_load_memory was > wrong and actually not needed. Values in define doesn't get converted > and are passed as is and hardcoded values are already in what the PHY > require, that is LE. > > Use get_unaligned_le32 instead of get_unaligned for FW data word load to > correctly convert data in the correct order to follow system endian. > > Also drop the cpu_to_be32 for CRC calculation as it's wrong and use > get_unaligned_be32 instead. The word is taken from firmware and is > always LE, the mailbox will emit a BE CRC from BE word hence the > word needs to be swapped on u8 to u32 cast on LE system. > This is needed as crc_ccitt_false will recast u32 to u8 and read order > changes between BE and LE system. By using get_unaligned_be32, word is > swapped only when needed resulting in the correct CRC calculated. > > Reported-by: kernel test robot <lkp@intel.com> > Closes: https://lore.kernel.org/oe-kbuild-all/202311210414.sEJZjlcD-lkp@intel.com/ > Fixes: e93984ebc1c8 ("net: phy: aquantia: add firmware load support") > Tested-by: Robert Marko <robimarko@gmail.com> # ipq8072 LE device > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> > --- > Changes v2: > - Add further explaination in commit description > - Fix wrong CRC conversion and swap only when needed > > drivers/net/phy/aquantia/aquantia_firmware.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/drivers/net/phy/aquantia/aquantia_firmware.c b/drivers/net/phy/aquantia/aquantia_firmware.c > index c5f292b1c4c8..c12e8a3acb77 100644 > --- a/drivers/net/phy/aquantia/aquantia_firmware.c > +++ b/drivers/net/phy/aquantia/aquantia_firmware.c > @@ -93,9 +93,9 @@ static int aqr_fw_load_memory(struct phy_device *phydev, u32 addr, > u16 crc = 0, up_crc; > size_t pos; > > - /* PHY expect addr in LE */ > - addr = (__force u32)cpu_to_le32(addr); > - > + /* PHY expect addr in LE. Hardcoded addr in defines are > + * already in this format. > + */ Please fix this comment. No, the address is not in LE. You program the address into a register in the PHY. Bit 0 of the register is bit 0 of the data field on the MDIO bus, and bit 0 of the value passed to phy_write_mmd() which is in CPU endian. Bit 15 of the register is bit 15 of the data field on the MDIO bus, which is bit 15 of the value passed to phy_write_mmd(). So the talk of "LE" here is meaningless. Please stop over-complicating this. > phy_write_mmd(phydev, MDIO_MMD_VEND1, > VEND1_GLOBAL_MAILBOX_INTERFACE1, > VEND1_GLOBAL_MAILBOX_INTERFACE1_CRC_RESET); > @@ -113,7 +113,7 @@ static int aqr_fw_load_memory(struct phy_device *phydev, u32 addr, > u32 word; > > /* FW data is always stored in little-endian */ > - word = get_unaligned((const u32 *)(data + pos)); > + word = get_unaligned_le32((const u32 *)(data + pos)); This comment is appropriate, and get_unaligned_le32() is correct. > > phy_write_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_MAILBOX_INTERFACE5, > VEND1_GLOBAL_MAILBOX_INTERFACE5_MSW_DATA(word)); > @@ -125,10 +125,10 @@ static int aqr_fw_load_memory(struct phy_device *phydev, u32 addr, > VEND1_GLOBAL_MAILBOX_INTERFACE1_WRITE); > > /* calculate CRC as we load data to the mailbox. > - * We convert word to big-endian as PHY is BE and mailbox will > + * We read word as big-endian as PHY is BE and mailbox will > * return a BE CRC. Is that true (about returning a BE CRC) ? > */ > - word = (__force u32)cpu_to_be32(word); > + word = get_unaligned_be32((const u32 *)(data + pos)); > crc = crc_ccitt_false(crc, (u8 *)&word, sizeof(word)); > } And you _still_ haven't taken on board my issue with this, sigh. No, this will be subject to variation in result from CPU endianness. For the sake of your education on the first point, do this: u16 le_crc = 0, be_crc = 0, up_crc; ... le_crc = crc_ccitt_false(le_crc, (u8 *)&word, sizeof(word)); word = get_unaligned_be32((const u32 *)(data + pos)); be_crc = crc_ccitt_false(be_crc, (u8 *)&word, sizeof(word)); } printk("le_crc=0x%04x be_crc=0x%04x\n", le_crc, be_crc); What you will find is that the two CRCs are _totally_ different - not just different in endian, but different in value as well. The endianness of the input data does not simply change the endianness of the resulting CRC, it will change the value. So, returning a "BE CRC" has no bearing on whether the data passed to the CRC function needs to be in big endian order or not. On the second point, as I have stated numerous times, and it seems I'm not getting anywhere with: u32 word; int i; u8 *p; word = 0x01020304; p = (u8 *)&word; for (i = 0; i < sizeof(word); i++) printf("p[%d] = %02x\n", i, p[i]); on little endian machines will print: p[0] = 0x01 p[1] = 0x02 p[2] = 0x03 p[3] = 0x04 but on big endian machines will print: p[0] = 0x04 p[1] = 0x03 p[2] = 0x02 p[3] = 0x01 The order in which you provide the bytes to a CRC function will change the CRC value. So, the endianness of the machine/CPU your code will be running on will change the resulting CRC. Your code is buggy. I've given you solutions to it, but you are very resistant to these suggestions. I know endianness can be difficult to those who don't understand it, but I've described it numerous times, complete with code examples to show how things change - including for the above issue. It seems I'm demonstrably waiting my time because it's having very little effect. Therefore, quite simply, NAK to this patch.
On Mon, Nov 27, 2023 at 10:02:51AM +0000, Russell King (Oracle) wrote: > On Mon, Nov 27, 2023 at 01:29:24AM +0100, Christian Marangi wrote: > > On further testing on BE target with kernel test robot, it was notice > > that the endianness conversion for addr and CRC in fw_load_memory was > > wrong and actually not needed. Values in define doesn't get converted > > and are passed as is and hardcoded values are already in what the PHY > > require, that is LE. > > > > Use get_unaligned_le32 instead of get_unaligned for FW data word load to > > correctly convert data in the correct order to follow system endian. > > > > Also drop the cpu_to_be32 for CRC calculation as it's wrong and use > > get_unaligned_be32 instead. The word is taken from firmware and is > > always LE, the mailbox will emit a BE CRC from BE word hence the > > word needs to be swapped on u8 to u32 cast on LE system. > > This is needed as crc_ccitt_false will recast u32 to u8 and read order > > changes between BE and LE system. By using get_unaligned_be32, word is > > swapped only when needed resulting in the correct CRC calculated. > > > > Reported-by: kernel test robot <lkp@intel.com> > > Closes: https://lore.kernel.org/oe-kbuild-all/202311210414.sEJZjlcD-lkp@intel.com/ > > Fixes: e93984ebc1c8 ("net: phy: aquantia: add firmware load support") > > Tested-by: Robert Marko <robimarko@gmail.com> # ipq8072 LE device > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> > > --- > > Changes v2: > > - Add further explaination in commit description > > - Fix wrong CRC conversion and swap only when needed > > > > drivers/net/phy/aquantia/aquantia_firmware.c | 12 ++++++------ > > 1 file changed, 6 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/net/phy/aquantia/aquantia_firmware.c b/drivers/net/phy/aquantia/aquantia_firmware.c > > index c5f292b1c4c8..c12e8a3acb77 100644 > > --- a/drivers/net/phy/aquantia/aquantia_firmware.c > > +++ b/drivers/net/phy/aquantia/aquantia_firmware.c > > @@ -93,9 +93,9 @@ static int aqr_fw_load_memory(struct phy_device *phydev, u32 addr, > > u16 crc = 0, up_crc; > > size_t pos; > > > > - /* PHY expect addr in LE */ > > - addr = (__force u32)cpu_to_le32(addr); > > - > > + /* PHY expect addr in LE. Hardcoded addr in defines are > > + * already in this format. > > + */ > > Please fix this comment. No, the address is not in LE. You program the > address into a register in the PHY. Bit 0 of the register is bit 0 of > the data field on the MDIO bus, and bit 0 of the value passed to > phy_write_mmd() which is in CPU endian. Bit 15 of the register is bit > 15 of the data field on the MDIO bus, which is bit 15 of the value > passed to phy_write_mmd(). > > So the talk of "LE" here is meaningless. Please stop over-complicating > this. > Will drop sorry. > > phy_write_mmd(phydev, MDIO_MMD_VEND1, > > VEND1_GLOBAL_MAILBOX_INTERFACE1, > > VEND1_GLOBAL_MAILBOX_INTERFACE1_CRC_RESET); > > @@ -113,7 +113,7 @@ static int aqr_fw_load_memory(struct phy_device *phydev, u32 addr, > > u32 word; > > > > /* FW data is always stored in little-endian */ > > - word = get_unaligned((const u32 *)(data + pos)); > > + word = get_unaligned_le32((const u32 *)(data + pos)); > > This comment is appropriate, and get_unaligned_le32() is correct. > > > > > phy_write_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_MAILBOX_INTERFACE5, > > VEND1_GLOBAL_MAILBOX_INTERFACE5_MSW_DATA(word)); > > @@ -125,10 +125,10 @@ static int aqr_fw_load_memory(struct phy_device *phydev, u32 addr, > > VEND1_GLOBAL_MAILBOX_INTERFACE1_WRITE); > > > > /* calculate CRC as we load data to the mailbox. > > - * We convert word to big-endian as PHY is BE and mailbox will > > + * We read word as big-endian as PHY is BE and mailbox will > > * return a BE CRC. > > Is that true (about returning a BE CRC) ? > More info at the bottom. > > */ > > - word = (__force u32)cpu_to_be32(word); > > + word = get_unaligned_be32((const u32 *)(data + pos)); > > crc = crc_ccitt_false(crc, (u8 *)&word, sizeof(word)); > > } > > And you _still_ haven't taken on board my issue with this, sigh. No, > this will be subject to variation in result from CPU endianness. > > For the sake of your education on the first point, do this: > > u16 le_crc = 0, be_crc = 0, up_crc; > > ... > le_crc = crc_ccitt_false(le_crc, (u8 *)&word, sizeof(word)); > word = get_unaligned_be32((const u32 *)(data + pos)); > be_crc = crc_ccitt_false(be_crc, (u8 *)&word, sizeof(word)); > } > > printk("le_crc=0x%04x be_crc=0x%04x\n", le_crc, be_crc); > > What you will find is that the two CRCs are _totally_ different - not > just different in endian, but different in value as well. The endianness > of the input data does not simply change the endianness of the resulting > CRC, it will change the value. > > So, returning a "BE CRC" has no bearing on whether the data passed to > the CRC function needs to be in big endian order or not. > > On the second point, as I have stated numerous times, and it seems I'm > not getting anywhere with: > > > u32 word; > int i; > u8 *p; > > word = 0x01020304; > > p = (u8 *)&word; > > for (i = 0; i < sizeof(word); i++) > printf("p[%d] = %02x\n", i, p[i]); > > on little endian machines will print: > > p[0] = 0x01 > p[1] = 0x02 > p[2] = 0x03 > p[3] = 0x04 > > but on big endian machines will print: > > p[0] = 0x04 > p[1] = 0x03 > p[2] = 0x02 > p[3] = 0x01 > > The order in which you provide the bytes to a CRC function will change > the CRC value. So, the endianness of the machine/CPU your code will be > running on will change the resulting CRC. > > Your code is buggy. > > I've given you solutions to it, but you are very resistant to these > suggestions. > > I know endianness can be difficult to those who don't understand it, > but I've described it numerous times, complete with code examples to > show how things change - including for the above issue. It seems I'm > demonstrably waiting my time because it's having very little effect. > > Therefore, quite simply, NAK to this patch. > Sorry for getting you annoyed, wasn't my intention at all. Also wasn't reistantant but more of trying to find a solution without falling back to the raw shift, trying to keep them out of the function code. Anyway on the the explaination on how this majestic thing works. We verified and it's almost everything correct, just badly worded I guess...(not referring to your explaination but what is currently implemented and written in the driver comments) The thing works this way: 1. Things are loaded to mailbox with the data regs. (LE order expected) 2. Internally the word is converted to BE 3. CRC is calculated in the just converted data (CRC calculated on BE word) 4. CRC is provided via the mailbox CRC regs in LE order. We verified this by loading a word and comparing what was the CRC returned and we observed it's bit match the CRC of the word with the bytes swapped. Given this sorted out and having a clear idea of what happens internally. Yes your suggested way of an u8 struct it's the only way to make the crc function to work given the cast... An alternative might be: crc = crc_ccitt_false_byte(crc, data + pos + 3); crc = crc_ccitt_false_byte(crc, data + pos + 2); crc = crc_ccitt_false_byte(crc, data + pos + 1); crc = crc_ccitt_false_byte(crc, data + pos); but I think this is unreadable. Also the CRC returned from the mailbox CRC has to be converted with le16_to_cpu since it's LE and won't match on BE system. Am I wrong?
On Tue, Nov 28, 2023 at 01:44:39PM +0100, Christian Marangi wrote: > Also the CRC returned from the mailbox CRC has to be converted with > le16_to_cpu since it's LE and won't match on BE system. Am I wrong? I think you are. As I've said before, everything transferred over the MDIO bus is totally independent of the CPU endianness. Bit 0 in the registers on the PHY will appear in bit 0 of the CPU register, and bit 15 in the registers on the PHY will appear in bit 15 of the CPU register. If this weren't the case, then if we access the BMCR register, and the BMCR contains 0x1140 (indicating AN is enabled, full duplex, 1000Mbps) then if these were CPU endian-dependent, we'd end up reading 0x4011, and that will break phylib _and_ user applications (these register definitions are exported to userspace.)
diff --git a/drivers/net/phy/aquantia/aquantia_firmware.c b/drivers/net/phy/aquantia/aquantia_firmware.c index c5f292b1c4c8..c12e8a3acb77 100644 --- a/drivers/net/phy/aquantia/aquantia_firmware.c +++ b/drivers/net/phy/aquantia/aquantia_firmware.c @@ -93,9 +93,9 @@ static int aqr_fw_load_memory(struct phy_device *phydev, u32 addr, u16 crc = 0, up_crc; size_t pos; - /* PHY expect addr in LE */ - addr = (__force u32)cpu_to_le32(addr); - + /* PHY expect addr in LE. Hardcoded addr in defines are + * already in this format. + */ phy_write_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_MAILBOX_INTERFACE1, VEND1_GLOBAL_MAILBOX_INTERFACE1_CRC_RESET); @@ -113,7 +113,7 @@ static int aqr_fw_load_memory(struct phy_device *phydev, u32 addr, u32 word; /* FW data is always stored in little-endian */ - word = get_unaligned((const u32 *)(data + pos)); + word = get_unaligned_le32((const u32 *)(data + pos)); phy_write_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_MAILBOX_INTERFACE5, VEND1_GLOBAL_MAILBOX_INTERFACE5_MSW_DATA(word)); @@ -125,10 +125,10 @@ static int aqr_fw_load_memory(struct phy_device *phydev, u32 addr, VEND1_GLOBAL_MAILBOX_INTERFACE1_WRITE); /* calculate CRC as we load data to the mailbox. - * We convert word to big-endian as PHY is BE and mailbox will + * We read word as big-endian as PHY is BE and mailbox will * return a BE CRC. */ - word = (__force u32)cpu_to_be32(word); + word = get_unaligned_be32((const u32 *)(data + pos)); crc = crc_ccitt_false(crc, (u8 *)&word, sizeof(word)); }