| Message ID | 20231108151018.72670-1-ivecera@redhat.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [iwl-net] i40e: Fix max frame size check | expand |
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!
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
diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c index 4f445f6835de..6a2907674583 100644 --- a/drivers/net/ethernet/intel/i40e/i40e_main.c +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c @@ -16219,11 +16219,11 @@ static int i40e_probe(struct pci_dev *pdev, const struct pci_device_id *ent) /* make sure the MFS hasn't been set lower than the default */ #define MAX_FRAME_SIZE_DEFAULT 0x2600 - val = (rd32(&pf->hw, I40E_PRTGL_SAH) & + val = (rd32(&pf->hw, I40E_PRTGL_SAH(pf->hw.port)) & I40E_PRTGL_SAH_MFS_MASK) >> I40E_PRTGL_SAH_MFS_SHIFT; if (val < MAX_FRAME_SIZE_DEFAULT) dev_warn(&pdev->dev, "MFS for port %x has been set below the default: %x\n", - i, val); + pf->hw.port, val); /* Add a filter to drop all Flow control frames from any VSI from being * transmitted. By doing so we stop a malicious VF from sending out diff --git a/drivers/net/ethernet/intel/i40e/i40e_register.h b/drivers/net/ethernet/intel/i40e/i40e_register.h index f408fcf23ce8..75edfe3d43f7 100644 --- a/drivers/net/ethernet/intel/i40e/i40e_register.h +++ b/drivers/net/ethernet/intel/i40e/i40e_register.h @@ -498,7 +498,7 @@ #define I40E_VSILAN_QBASE_VSIQTABLE_ENA_SHIFT 11 #define I40E_VSILAN_QBASE_VSIQTABLE_ENA_MASK I40E_MASK(0x1, I40E_VSILAN_QBASE_VSIQTABLE_ENA_SHIFT) #define I40E_VSILAN_QTABLE(_i, _VSI) (0x00200000 + ((_i) * 2048 + (_VSI) * 4)) /* _i=0...7, _VSI=0...383 */ /* Reset: PFR */ -#define I40E_PRTGL_SAH 0x001E2140 /* Reset: GLOBR */ +#define I40E_PRTGL_SAH(_PRT) (0x001E2140 + ((_PRT) * 4)) /* _PRT=0...3 */ /* Reset: GLOBR */ #define I40E_PRTGL_SAH_FC_SAH_SHIFT 0 #define I40E_PRTGL_SAH_FC_SAH_MASK I40E_MASK(0xFFFF, I40E_PRTGL_SAH_FC_SAH_SHIFT) #define I40E_PRTGL_SAH_MFS_SHIFT 16
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. The existing check does not take port number into account and reads actually MFS value always for port 0 that is correct for PF 0 but not for other PFs. Update PRTGL_SAH register definition so it takes a port number as an argument and fix the check by passing the port number. Also fix the warning message that use for a port number a local variable 'i' that really does not contain such information. Fixes: 3a2c6ced90e1 ("i40e: Add a check to see if MFS is set") Cc: Todd Fujinaka <todd.fujinaka@intel.com> Signed-off-by: Ivan Vecera <ivecera@redhat.com> --- drivers/net/ethernet/intel/i40e/i40e_main.c | 4 ++-- drivers/net/ethernet/intel/i40e/i40e_register.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-)