Date: Fri, 10 Nov 2023 09:10:31 +0100
From: Ivan Vecera <ivecera@...hat.com>
To: Jesse Brandeburg <jesse.brandeburg@...el.com>,
 intel-wired-lan@...ts.osuosl.org
Cc: Jacob Keller <jacob.e.keller@...el.com>,
 Wojciech Drewek <wojciech.drewek@...el.com>,
 Tony Nguyen <anthony.l.nguyen@...el.com>,
 "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 "open list:NETWORKING DRIVERS" <netdev@...r.kernel.org>
Subject: Re: [PATCH iwl-net] i40e: Fix max frame size check

On 08. 11. 23 21:38, Jesse Brandeburg wrote:
> On 11/8/2023 7:10 AM, Ivan Vecera wrote:
>> Commit 3a2c6ced90e1 ("i40e: Add a check to see if MFS is set") added
>> a check for port's MFS (max frame size) value. The value is stored
>> in PRTGL_SAH register for each physical port. According datasheet this
>> register is defined as:
>>
>> PRTGL_SAH[PRT]: (0x001E2140 + 0x4*PRT, PRT=0...3)
>>
>> where PRT is physical port number.
> 
> <trimmed lkml, and a couple of non-existent intel addresses>
> 
> Was there an actual problem here? I suspect if you read all the
> registers for each PF's BAR, you'll find that all 4 report the same,
> correct value, for the perspective of the BAR they're being read from.
> 
> The i40e hardware does this (somewhat non-obvious) for *lots* of port
> specific registers, and what happens is no matter which of the 4 you
> read the value from, you'll get the right "port specific" value. This is
> because the hardware designers decided to make a different "view" on the
> register set depending on which PF you access it from. The only time
> these offsets matter is when the part is in debug mode or when the
> firmware is reading the internal registers (from the internal firmware
> register space - which has no aliasing) that rely on the correct offset.
> 
> In this case, I think your change won't make any functional difference,
> but I can see why you want to make the change as the code doesn't match
> the datasheet's definition of the register.
> 
> That all said, unless you can prove a problem, I'm relatively sure that
> nothing is wrong here in functionality or code. And if you go this
> route, there might be a lot of other registers to fix that have the same
> aliasing.
> 
> I apologize for the confusing manuals and header file, it's complicated
> but in practice works really well. Effectively you can't read other
> port's registers by accident.
> 
> Here was my experiment showing the aliasing on X722. You'll note that
> the lower 16 bits are a MAC address that doesn't change no matter which
> register you read.
> 
> device 20:0.0
> 0x1e2140 == 0x26008245
> 0x1e2144 == 0x26008245
> 0x1e2148 == 0x26008245
> 0x1e214c == 0x26008245
> device 20:0.1
> 0x1e2140 == 0x26008345
> 0x1e2144 == 0x26008345
> 0x1e2148 == 0x26008345
> 0x1e214c == 0x26008345
> device 20:0.2
> 0x1e2140 == 0x26008445
> 0x1e2144 == 0x26008445
> 0x1e2148 == 0x26008445
> 0x1e214c == 0x26008445
> device 20:0.3
> 0x1e2140 == 0x26008545
> 0x1e2144 == 0x26008545
> 0x1e2148 == 0x26008545
> 0x1e214c == 0x26008545
> 
> lspci -d ::0200
> 20:00.0 Ethernet controller: Intel Corporation Ethernet Connection X722
> for 10GBASE-T (rev 04)
> 20:00.1 Ethernet controller: Intel Corporation Ethernet Connection X722
> for 10GBASE-T (rev 04)
> 20:00.2 Ethernet controller: Intel Corporation Ethernet Connection X722
> for 10GbE SFP+ (rev 04)
> 20:00.3 Ethernet controller: Intel Corporation Ethernet Connection X722
> for 10GbE SFP+ (rev 04)
> 
> Hope this helps!

Hi Jesse,
thanks a lot for the explanation. I found this during preparation of my 
iwl-next stuff and found that variable 'i' is used inappropriately so I 
checked also the datasheet and found the definition of PRTGL_SAH 
register that is defined per port but I didn't know there is such 
aliasing for registers in PF BAR space.
I will send a new patch that at least fix the wrong usage of 'i' variable.

Thanks,
Ivan

Powered by blists - more mailing lists