Message ID | 20230414082047.1320947-1-horatiu.vultur@microchip.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:b0ea:0:b0:3b6:4342:cba0 with SMTP id b10csp222305vqo; Fri, 14 Apr 2023 01:34:13 -0700 (PDT) X-Google-Smtp-Source: AKy350b5LMt6be5oY4e8F6cXt7N7fE8+iR15MzlYKw6Q+YrLf3gB4ocU+Sklg6uxlU6Md6+cZ3fr X-Received: by 2002:a05:6a20:6d05:b0:eb:cef8:70e0 with SMTP id fv5-20020a056a206d0500b000ebcef870e0mr4856591pzb.16.1681461253236; Fri, 14 Apr 2023 01:34:13 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1681461253; cv=none; d=google.com; s=arc-20160816; b=O8OCrfI821GG3eR479AqSR5K+ZF9R4yXVTvxzIK4UrnmlY1QDLtIPhFT/SqdxJhznu BdUkeYqbdqQbecD5XQSlnUi0kM5fCIh1Lu6vX+0t/81M7nH6guH/dO6LeOm4RLPLDdS7 nOvZHkheTdRQ4Gx0BoDSsLiTCY28Loo6eo2+Bn7B6hC2DFEU0rEyK+fRFThm8FFEPMrk nxs9d5V/n0YEEHtQfEXItwBuw3czMgxoW7X+7XbsoknNZFezNqQAGjZzP+prRSARRXXR q3OvVlfVAvwYZe689nW0I3p4mcc3kEEsO9b8wp1FPgWtnAmoLgprtc7qFFz/bzd7CsYu Y0/w== 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=8MJwVVvwYCR27e9eNHpHI4xE2ysR73I/Gr/3qYH/+RI=; b=oPR1raW1oS3tSOsNrfddZlQzFP/sQ5BFzKIQ5fLNTZ4sfGW4kteU+xy+XymqLMszJ2 Qx42bu9GcS2FvHyP6IYj5eoP+Injxsr/olJ/BBoUoy4QxzsoJ90oLRbBbyBaoI5pMOqZ h9IqhuPTGNAFdq1S1YjtkoffnwtTNFHMsLCcCIFjB44ANb27UC9H+Q+8amVK2027niYO HGu/VE4axLdTE9KtQdQzKT8FkSF2Yfr40SGYCjO3U0ts9IkAj3ktTbcYEhoox7Qwykj2 KaxKJNfc72ZpgeY515FdkEr3aeLF/JGGpax8I0a3AbgaW47NhbTUA8gxvZHejn98afIm VAcg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@microchip.com header.s=mchp header.b=vZxv27T7; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=microchip.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id r4-20020a63ce44000000b0051b1fe0b7besi4161475pgi.580.2023.04.14.01.34.01; Fri, 14 Apr 2023 01:34:13 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@microchip.com header.s=mchp header.b=vZxv27T7; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=microchip.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230092AbjDNIVE (ORCPT <rfc822;leviz.kernel.dev@gmail.com> + 99 others); Fri, 14 Apr 2023 04:21:04 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:58654 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230096AbjDNIVB (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Fri, 14 Apr 2023 04:21:01 -0400 Received: from esa.microchip.iphmx.com (esa.microchip.iphmx.com [68.232.154.123]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 84F3830C4; Fri, 14 Apr 2023 01:20:56 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=microchip.com; i=@microchip.com; q=dns/txt; s=mchp; t=1681460457; x=1712996457; h=from:to:cc:subject:date:message-id:mime-version: content-transfer-encoding; bh=rtLuS5HdkkGf6K1pvoN/8HXKoYmlRrjzY3AWTl9DpU8=; b=vZxv27T7W4bK5nlitQzwDF5X6zK27TwiWKgxlWMNOLsG/qLXZTwxnEWl wXJI79CKFFCRfHTyYxqvwVMoOGMwnyhoGoXYdJPLu5fEG+cKW3k+6z1t9 RVx2ao1QDSOimY105W7iYKPhSsTCu1FIq5Rg686/KPMPVPh3fseJzPrHB Ey2nkShEbben+2y0tRDqBVFboH+AgEcz0MaUBrIjrvHkXMWRQ3rBvh4Nl wC/sI14BnJ4ILhPLRQN6RKSNofGhq9Fnvfl66HApgmByfkDGwRN0ugELD rrm06xzF65n0MUgsSQGGJXTniw3eBbqKa2V2pMYdsPxnEklz22He6YT9D w==; X-IronPort-AV: E=Sophos;i="5.99,195,1677567600"; d="scan'208,223";a="210429983" Received: from unknown (HELO email.microchip.com) ([170.129.1.10]) by esa2.microchip.iphmx.com with ESMTP/TLS/AES256-SHA256; 14 Apr 2023 01:20:55 -0700 Received: from chn-vm-ex02.mchp-main.com (10.10.87.72) by chn-vm-ex02.mchp-main.com (10.10.87.72) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.21; Fri, 14 Apr 2023 01:20:52 -0700 Received: from soft-dev3-1.microsemi.net (10.10.115.15) by chn-vm-ex02.mchp-main.com (10.10.85.144) with Microsoft SMTP Server id 15.1.2507.21 via Frontend Transport; Fri, 14 Apr 2023 01:20:51 -0700 From: Horatiu Vultur <horatiu.vultur@microchip.com> To: <netdev@vger.kernel.org>, <linux-kernel@vger.kernel.org> CC: <davem@davemloft.net>, <edumazet@google.com>, <kuba@kernel.org>, <pabeni@redhat.com>, <UNGLinuxDriver@microchip.com>, Horatiu Vultur <horatiu.vultur@microchip.com> Subject: [PATCH net-next] net: lan966x: Fix lan966x_ifh_get Date: Fri, 14 Apr 2023 10:20:47 +0200 Message-ID: <20230414082047.1320947-1-horatiu.vultur@microchip.com> X-Mailer: git-send-email 2.38.0 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Content-Type: text/plain X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED, RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1763139915327819162?= X-GMAIL-MSGID: =?utf-8?q?1763139915327819162?= |
Series |
[net-next] net: lan966x: Fix lan966x_ifh_get
|
|
Commit Message
Horatiu Vultur
April 14, 2023, 8:20 a.m. UTC
From time to time, it was observed that the nanosecond part of the
received timestamp, which is extracted from the IFH, it was actually
bigger than 1 second. So then when actually calculating the full
received timestamp, based on the nanosecond part from IFH and the second
part which is read from HW, it was actually wrong.
The issue seems to be inside the function lan966x_ifh_get, which
extracts information from an IFH(which is an byte array) and returns the
value in a u64. When extracting the timestamp value from the IFH, which
starts at bit 192 and have the size of 32 bits, then if the most
significant bit was set in the timestamp, then this bit was extended
then the return value became 0xffffffff... . To fix this, make sure to
clear all the other bits before returning the value.
Fixes: fd7627833ddf ("net: lan966x: Stop using packing library")
Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
drivers/net/ethernet/microchip/lan966x/lan966x_main.c | 1 +
1 file changed, 1 insertion(+)
Comments
From: Horatiu Vultur <horatiu.vultur@microchip.com> Date: Fri, 14 Apr 2023 10:20:47 +0200 >>From time to time, it was observed that the nanosecond part of the > received timestamp, which is extracted from the IFH, it was actually > bigger than 1 second. So then when actually calculating the full > received timestamp, based on the nanosecond part from IFH and the second > part which is read from HW, it was actually wrong. > > The issue seems to be inside the function lan966x_ifh_get, which > extracts information from an IFH(which is an byte array) and returns the > value in a u64. When extracting the timestamp value from the IFH, which > starts at bit 192 and have the size of 32 bits, then if the most > significant bit was set in the timestamp, then this bit was extended > then the return value became 0xffffffff... . To fix this, make sure to > clear all the other bits before returning the value. Ooooh, I remember I was having the same issue with sign extension :s Pls see below. > > Fixes: fd7627833ddf ("net: lan966x: Stop using packing library") > Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com> > --- > drivers/net/ethernet/microchip/lan966x/lan966x_main.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c > index 80e2ea7e6ce8a..508e494dcc342 100644 > --- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c > @@ -608,6 +608,7 @@ static u64 lan966x_ifh_get(u8 *ifh, size_t pos, size_t length) > val |= (1 << i); Alternatively, you can change that to (pick one that you like the most): val |= 1ULL << i; // or val |= BIT_ULL(i); The thing is that constants without any postfix (U, UL etc.) are treated as signed longs, that's why `1 << 31` becomes 0xffffffff80000000. 1U / 1UL / 1ULL don't. Adding unsigned postfix may also make it better for 32-bit systems, as `1 << i` there is 32-bit value, so `1 << 48` may go wrong and/or even trigger compilers. > } > > + val &= GENMASK(length, 0); > return val; > } > (now blah not directly related to the fix) I'm wondering a bit if lan966x_ifh_get() can be improved in general to work with words rather than bits. You read one byte per each bit each iteration there. For example, byte arrays could be casted to __be{32,64} and you'd get native byteorder for 32/64 bits via one __be*_to_cpu*() call. Thanks, Olek
On Fri, 14 Apr 2023 19:00:20 +0200 Alexander Lobakin wrote: > > @@ -608,6 +608,7 @@ static u64 lan966x_ifh_get(u8 *ifh, size_t pos, size_t length) > > val |= (1 << i); > > Alternatively, you can change that to (pick one that you like the most): > > val |= 1ULL << i; > // or > val |= BIT_ULL(i); // or (u64)1 << i // or, since you're only concerned about sign extension, even 1U << i having the correct type of the lval seems cleaner than masking, indeed.
The 04/14/2023 19:00, Alexander Lobakin wrote: > > From: Horatiu Vultur <horatiu.vultur@microchip.com> > Date: Fri, 14 Apr 2023 10:20:47 +0200 Hi Olek, > > >>From time to time, it was observed that the nanosecond part of the > > received timestamp, which is extracted from the IFH, it was actually > > bigger than 1 second. So then when actually calculating the full > > received timestamp, based on the nanosecond part from IFH and the second > > part which is read from HW, it was actually wrong. > > > > The issue seems to be inside the function lan966x_ifh_get, which > > extracts information from an IFH(which is an byte array) and returns the > > value in a u64. When extracting the timestamp value from the IFH, which > > starts at bit 192 and have the size of 32 bits, then if the most > > significant bit was set in the timestamp, then this bit was extended > > then the return value became 0xffffffff... . To fix this, make sure to > > clear all the other bits before returning the value. > > Ooooh, I remember I was having the same issue with sign extension :s > Pls see below. > > > > > Fixes: fd7627833ddf ("net: lan966x: Stop using packing library") > > Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com> > > --- > > drivers/net/ethernet/microchip/lan966x/lan966x_main.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c > > index 80e2ea7e6ce8a..508e494dcc342 100644 > > --- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c > > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c > > @@ -608,6 +608,7 @@ static u64 lan966x_ifh_get(u8 *ifh, size_t pos, size_t length) > > val |= (1 << i); > > Alternatively, you can change that to (pick one that you like the most): > > val |= 1ULL << i; > // or > val |= BIT_ULL(i); > > The thing is that constants without any postfix (U, UL etc.) are treated > as signed longs, that's why `1 << 31` becomes 0xffffffff80000000. 1U / > 1UL / 1ULL don't. > > Adding unsigned postfix may also make it better for 32-bit systems, as > `1 << i` there is 32-bit value, so `1 << 48` may go wrong and/or even > trigger compilers. Thanks for suggestion and the explanation, it was really helpful. I will update this in the next version. > > > } > > > > + val &= GENMASK(length, 0); > > return val; > > } > > > > (now blah not directly related to the fix) I think this change regarding the improvement of the lan966x_ifh_get should not be in the next version of this patch, as there are 2 different things. But I would still like to know how to do this! > > I'm wondering a bit if lan966x_ifh_get() can be improved in general to > work with words rather than bits. You read one byte per each bit each > iteration there. Actually, I am not reading 1 byte per each bit iteration. I am reading 1 byte first time when entering in the loop or each time when the bit iteration (j variable) is modulo 8. > For example, byte arrays could be casted to __be{32,64} and you'd get > native byteorder for 32/64 bits via one __be*_to_cpu*() call. > > Thanks, > Olek
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c index 80e2ea7e6ce8a..508e494dcc342 100644 --- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c @@ -608,6 +608,7 @@ static u64 lan966x_ifh_get(u8 *ifh, size_t pos, size_t length) val |= (1 << i); } + val &= GENMASK(length, 0); return val; }