From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (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 F0170238C15 for ; Mon, 26 Jan 2026 11:01:22 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1769425283; cv=none; b=f+UPIJuikOhXzpkPLaExWGrjDfapMqrd0UJSTo8KZKQSeWOjUotQ88heYTxRAFhW/PnDCgoLyeR84kyhk3BJimF2A+N7EqyHWK00IWqv/yIMcFVo6NMWZUZeLSoeRyv+G6SQtqWUtSSXCjmhLHF+LuTn9nyZU/1NWRh7Z62u9b4= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1769425283; c=relaxed/simple; bh=63TN7QkZFEfpjv3JCIQCeLZhAYXlS3k1CkWWAp/J7Ko=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=W45aIADWnPRgLodUiHCkNxALNMHPSgarofGuj5w2CVVdMrzHGeJaWBRX3fIjVyIg179JduO/pwBjOPuO9aBpzODpRny+U4qyRccErqeQMd00fs37pZ3UliO9tnDy5xNA0XJ3AVVWQYSM6XllSTIZY+B7Udqcb0fi68LW3rjogok= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=NXh8D1bI; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="NXh8D1bI" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 354F8C116C6; Mon, 26 Jan 2026 11:01:21 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1769425282; bh=63TN7QkZFEfpjv3JCIQCeLZhAYXlS3k1CkWWAp/J7Ko=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=NXh8D1bI1mTXEW0Y0Eoa5tzXUgVWZsB9ygK+gp76mzHVBTYVx2AkipxEeCsdzGkLz r7C87jtd8YGiWP160roI8zhB0GQCtAnkHFEevyq/GFXXFT0nOqhGV4aMTDzUL/p7jo 3WPr44xRNljALRQbOK+6GBjP0vnlW+ny/WOlw57UUGcZKILMu7eZLzWoP1ROG0uptN AMtSCQ5mFAdf8VZfjJD/0enoSNKq5fOHUd9whUmaFyNFWs4ZuPdlZQnB592ogufBF2 J9aXoMOLWp9PI102j6Ht5V97DBbT/XOAifA3hnYRSEoRyo3FA9t/SAcEi1BvM2qKyx hXRugYFKcHDfw== Date: Mon, 26 Jan 2026 11:01:18 +0000 From: Simon Horman To: "Keller, Jacob E" Cc: "Kwapulinski, Piotr" , "intel-wired-lan@lists.osuosl.org" , "netdev@vger.kernel.org" , "dan.carpenter@linaro.org" , "pmenzel@molgen.mpg.de" , "Loktionov, Aleksandr" Subject: Re: [PATCH iwl-next v2 1/2] ixgbe: e610: Convert ACI descriptor buffer to little endian Message-ID: References: <20260122164632.112607-1-piotr.kwapulinski@intel.com> 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: On Fri, Jan 23, 2026 at 11:41:26PM +0000, Keller, Jacob E wrote: > > > > -----Original Message----- > > From: Simon Horman > > Sent: Friday, January 23, 2026 12:07 PM > > To: Kwapulinski, Piotr > > Cc: intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org; > > dan.carpenter@linaro.org; pmenzel@molgen.mpg.de; Loktionov, Aleksandr > > > > Subject: Re: [PATCH iwl-next v2 1/2] ixgbe: e610: Convert ACI descriptor > > buffer to little endian > > > > On Thu, Jan 22, 2026 at 05:46:32PM +0100, Piotr Kwapulinski wrote: > > > The ixgbe device registers/descriptors expect little-endian ordering. Make > > > the code aware that the e610 adapter operates on data with little endian > > > ordering. The extra conversion is required on big-endian hosts. In most > > > scenarios this conversion is not required. > > > > > > Fixes: 46761fd52a88 ("ixgbe: Add support for E610 FW Admin Command > > Interface") > > > Reviewed-by: Aleksandr Loktionov > > > Signed-off-by: Piotr Kwapulinski > > > --- > > > drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c | 7 ++++--- > > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c > > b/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c > > > index c2f8189..f494e90 100644 > > > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c > > > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c > > > @@ -113,7 +113,8 @@ static int ixgbe_aci_send_cmd_execute(struct > > ixgbe_hw *hw, > > > > > > /* Descriptor is written to specific registers */ > > > for (i = 0; i < IXGBE_ACI_DESC_SIZE_IN_DWORDS; i++) > > > - IXGBE_WRITE_REG(hw, IXGBE_PF_HIDA(i), raw_desc[i]); > > > + IXGBE_WRITE_REG(hw, IXGBE_PF_HIDA(i), > > > + le32_to_cpu(raw_desc[i])); > > > > IXGBE_WRITE_REG is backed by writel. And my understanding is that writel > > converts values from host byte order to little endian. So I'm confused > > about where this is going. > > > > Yes, it should. In this case, the raw_desc value is being converted *to* CPU order to work with writel... Yes, sorry. I seem to have completely confused myself there. > > > /* SW has to set PF_HICR.C bit and clear PF_HICR.SV and > > > * PF_HICR_EV > > > @@ -145,7 +146,7 @@ static int ixgbe_aci_send_cmd_execute(struct > > ixgbe_hw *hw, > > > if ((hicr & IXGBE_PF_HICR_SV)) { > > > for (i = 0; i < IXGBE_ACI_DESC_SIZE_IN_DWORDS; i++) { > > > raw_desc[i] = IXGBE_READ_REG(hw, > > IXGBE_PF_HIDA(i)); > > > - raw_desc[i] = raw_desc[i]; > > > > I'm also curious to know what the intent (if any) of the line above was/is. > > > > > + raw_desc[i] = cpu_to_le32(raw_desc[i]); > > > It's being converted to LE32 order here. But if nothing else touches raw_desc is there any reason to convert?? > > > > > Please don't use the same variable to store both host byte order and little > > endian values. In this case I'd use another local variable, say scoped to > > within this block, to store the intermediate value. > > > > And if raw_desc will be used to hold __le32 values, it's type should be > > updated. > > > > If I understand Simon's comments correctly, this whole change is a no-op, and unnecessary. Writel and readl already handle conversion to CPU format, so unless you have some issue because raw_desc is assumed to be LE32 somewhere else, I think this patch should be dropped. If you do have a real case where something was wrong, can you please provide details? Sorry for not being clearer. I'm suspecting that the issue that this patch tries to address is that the backing structure, struct libie_aq_desc, is described in terms of __le integers. I haven't dug deep enough to be able to say if that is a good approach or not. But if that data is __le, then I expect that some sort of conversion along the lines of this patch makes sense.