Message ID | 20230411073707.19230-1-jan.sokolowski@intel.com |
---|---|
State | Changes Requested |
Headers | show |
Series | [net,v1] ice: fix undersized tx_flags field | expand |
From: Jan Sokolowski <jan.sokolowski@intel.com> Date: Tue, 11 Apr 2023 09:37:07 +0200 Please always add original authors to Ccs when you modify some code. I found this mail only by scrolling IWL, while I should've got it from the start. + Cc netdev (no idea why you didn't do that) > As not all ICE_TX_FLAGS_* fit in current 16-bit limited > tx_flags field, some flags would not properly apply. Could you give more details here? With the actual definitions and also how it was found and what's the regression is. I found that there's VLAN tag which uses upper 16 bits only by browsing the code, while I'd say you should've written it here. > > Fix that by removing 16 bit limitation. > > Signed-off-by: Jan Sokolowski <jan.sokolowski@intel.com> > Fixes: aa1d3faf71a6 ("ice: Robustify cleaning/completing XDP Tx buffers") Your SoB must go last, i.e. "Fixes:" should be placed above it. > --- > drivers/net/ethernet/intel/ice/ice_txrx.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.h b/drivers/net/ethernet/intel/ice/ice_txrx.h > index fff0efe28373..46c108cc5283 100644 > --- a/drivers/net/ethernet/intel/ice/ice_txrx.h > +++ b/drivers/net/ethernet/intel/ice/ice_txrx.h > @@ -183,7 +183,7 @@ struct ice_tx_buf { > unsigned int nr_frags; /* used for mbuf XDP */ > }; > u32 type:16; /* &ice_tx_buf_type */ > - u32 tx_flags:16; > + u32 tx_flags; Please always provide pahole output when you change fields size/structure. Here you create a 16-bit hole and increase structure size with no mentioning. I wouldn't say the fix is optimal. From what I see, we have such flags (correct me if I'm wrong): TSO BIT(0) [bits 1-7 are used] OUTER_SINGLE_VLAN BIT(8) [bits 9-15 are UNused] VLAN_S (shift) 16 [bits 16-31 are used for VLAN tag] So you have 7 free bits to reuse for &ice_tx_buf_type, but you just restored the before-commit ::tx_flags size =\ I would do the following: u32 tx_flags:12; u32 type:4; u32 vid:16; * no structure size change (even no layout change); * ::type range is 0-15 -- more than enough, as the last &ice_tx_buf_type value is 6; * ::tx_flags still has 3 free bits left (9, 10, and 11); * ::vid makes it easier to set a VLAN tag (no explicit masking-shifting, just don't forget to adjust the places where %ICE_TX_VLAN_{M,S} are used). Don't just use "first that works" approach =\ > DEFINE_DMA_UNMAP_LEN(len); > DEFINE_DMA_UNMAP_ADDR(dma); > }; Thanks, Olek
From: Lobakin, Aleksander <aleksander.lobakin@intel.com> Sent: Wednesday, April 12, 2023 6:49 PM To: Sokolowski, Jan <jan.sokolowski@intel.com> Cc: intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org >Subject: Re: [Intel-wired-lan] [PATCH net v1] ice: fix undersized tx_flags field >>From: Jan Sokolowski <jan.sokolowski@intel.com> >>Date: Tue, 11 Apr 2023 09:37:07 +0200 > >Please always add original authors to Ccs when you modify some code. I >found this mail only by scrolling IWL, while I should've got it from the >start. > >+ Cc netdev (no idea why you didn't do that) >> As not all ICE_TX_FLAGS_* fit in current 16-bit limited >> tx_flags field, some flags would not properly apply. > >Could you give more details here? With the actual definitions and also >how it was found and what's the regression is. >I found that there's VLAN tag which uses upper 16 bits only by browsing >the code, while I'd say you should've written it here. The definitions are ICE_TX_FLAGS_VLAN_* ones, for example ICE_TX_FLAGS_VLAN_M that is a 0xffff0000 mask. The regression found was with some vlan traffic no longer passing through after commit aa1d3faf71a6 ("ice: Robustify cleaning/completing XDP Tx buffers") >> >> Fix that by removing 16 bit limitation. >> >> Signed-off-by: Jan Sokolowski <jan.sokolowski@intel.com> >> Fixes: aa1d3faf71a6 ("ice: Robustify cleaning/completing XDP Tx buffers") > >Your SoB must go last, i.e. "Fixes:" should be placed above it. > >> --- >> drivers/net/ethernet/intel/ice/ice_txrx.h | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.h b/drivers/net/ethernet/intel/ice/ice_txrx.h >> index fff0efe28373..46c108cc5283 100644 >> --- a/drivers/net/ethernet/intel/ice/ice_txrx.h >> +++ b/drivers/net/ethernet/intel/ice/ice_txrx.h >> @@ -183,7 +183,7 @@ struct ice_tx_buf { >> unsigned int nr_frags; /* used for mbuf XDP */ >> }; >> u32 type:16; /* &ice_tx_buf_type */ >> - u32 tx_flags:16; >> + u32 tx_flags; > >Please always provide pahole output when you change fields >size/structure. Here you create a 16-bit hole and increase structure >size with no mentioning. Ok, will do that. >I wouldn't say the fix is optimal. From what I see, we have such flags >(correct me if I'm wrong): > >TSO BIT(0) >[bits 1-7 are used] >OUTER_SINGLE_VLAN BIT(8) >[bits 9-15 are UNused] >VLAN_S (shift) 16 >[bits 16-31 are used for VLAN tag] > >So you have 7 free bits to reuse for &ice_tx_buf_type, but you just >restored the before-commit ::tx_flags size =\ >I would do the following: > > u32 tx_flags:12; > u32 type:4; > u32 vid:16; Ok, will try to refactor it in this way and see whether it'll work. >* no structure size change (even no layout change); >* ::type range is 0-15 -- more than enough, as the last &ice_tx_buf_type > value is 6; >* ::tx_flags still has 3 free bits left (9, 10, and 11); >* ::vid makes it easier to set a VLAN tag (no explicit masking-shifting, > just don't forget to adjust the places where %ICE_TX_VLAN_{M,S} are > used). > >Don't just use "first that works" approach =\ > >> DEFINE_DMA_UNMAP_LEN(len); >> DEFINE_DMA_UNMAP_ADDR(dma); >> }; > >Thanks, >Olek
diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.h b/drivers/net/ethernet/intel/ice/ice_txrx.h index fff0efe28373..46c108cc5283 100644 --- a/drivers/net/ethernet/intel/ice/ice_txrx.h +++ b/drivers/net/ethernet/intel/ice/ice_txrx.h @@ -183,7 +183,7 @@ struct ice_tx_buf { unsigned int nr_frags; /* used for mbuf XDP */ }; u32 type:16; /* &ice_tx_buf_type */ - u32 tx_flags:16; + u32 tx_flags; DEFINE_DMA_UNMAP_LEN(len); DEFINE_DMA_UNMAP_ADDR(dma); };
As not all ICE_TX_FLAGS_* fit in current 16-bit limited tx_flags field, some flags would not properly apply. Fix that by removing 16 bit limitation. Signed-off-by: Jan Sokolowski <jan.sokolowski@intel.com> Fixes: aa1d3faf71a6 ("ice: Robustify cleaning/completing XDP Tx buffers") --- drivers/net/ethernet/intel/ice/ice_txrx.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)