Message ID | 20221207180842.1096243-1-anthony.l.nguyen@intel.com (mailing list archive) |
---|---|
State | Superseded |
Delegated to: | Netdev Maintainers |
Headers | show |
Series | [net,v2,1/1] i40e: Fix the inability to attach XDP program on downed interface | expand |
On 07 Dec 10:08, Tony Nguyen wrote: >From: Bartosz Staszewski <bartoszx.staszewski@intel.com> > >Whenever trying to load XDP prog on downed interface, function i40e_xdp >was passing vsi->rx_buf_len field to i40e_xdp_setup() which was equal 0. >i40e_open() calls i40e_vsi_configure_rx() which configures that field, >but that only happens when interface is up. When it is down, i40e_open() >is not being called, thus vsi->rx_buf_len is not set. > >Solution for this is calculate buffer length in newly created >function - i40e_calculate_vsi_rx_buf_len() that return actual buffer >length. Buffer length is being calculated based on the same rules >applied previously in i40e_vsi_configure_rx() function. > >Fixes: 613142b0bb88 ("i40e: Log error for oversized MTU on device") >Fixes: 0c8493d90b6b ("i40e: add XDP support for pass and drop actions") >Signed-off-by: Bartosz Staszewski <bartoszx.staszewski@intel.com> >Signed-off-by: Mateusz Palczewski <mateusz.palczewski@intel.com> >Tested-by: Shwetha Nagaraju <Shwetha.nagaraju@intel.com> >Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com> >Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com> >--- >v2: >- Change title and rework commit message >- Dropped, previous, patch 1 > >v1: https://lore.kernel.org/netdev/20221115000324.3040207-1-anthony.l.nguyen@intel.com/ > > drivers/net/ethernet/intel/i40e/i40e_main.c | 42 +++++++++++++++------ > 1 file changed, 30 insertions(+), 12 deletions(-) > >diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c >index 6416322d7c18..b8a8098110eb 100644 >--- a/drivers/net/ethernet/intel/i40e/i40e_main.c >+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c >@@ -3693,6 +3693,30 @@ static int i40e_vsi_configure_tx(struct i40e_vsi *vsi) > return err; > } > >+/** >+ * i40e_calculate_vsi_rx_buf_len - Calculates buffer length >+ * >+ * @vsi: VSI to calculate rx_buf_len from >+ */ >+static u16 i40e_calculate_vsi_rx_buf_len(struct i40e_vsi *vsi) >+{ >+ u16 ret; >+ >+ if (!vsi->netdev || (vsi->back->flags & I40E_FLAG_LEGACY_RX)) { >+ ret = I40E_RXBUFFER_2048; >+#if (PAGE_SIZE < 8192) >+ } else if (!I40E_2K_TOO_SMALL_WITH_PADDING && >+ (vsi->netdev->mtu <= ETH_DATA_LEN)) { >+ ret = I40E_RXBUFFER_1536 - NET_IP_ALIGN; >+#endif >+ } else { >+ ret = (PAGE_SIZE < 8192) ? I40E_RXBUFFER_3072 : >+ I40E_RXBUFFER_2048; >+ } >+ >+ return ret; >+} nit: linux coding style states: "Do not unnecessarily use braces where a single statement will do" I think this applies here. Also you could simplify this function to do early returns and drop the u16 ret variable and all the else statements. if (condition 1) return val1; if (condition 2) return val2; return val3; Other than that LGTM.
On 12/7/2022 10:25 AM, Saeed Mahameed wrote: > On 07 Dec 10:08, Tony Nguyen wrote: >> From: Bartosz Staszewski <bartoszx.staszewski@intel.com> >> >> Whenever trying to load XDP prog on downed interface, function i40e_xdp >> was passing vsi->rx_buf_len field to i40e_xdp_setup() which was equal 0. >> i40e_open() calls i40e_vsi_configure_rx() which configures that field, >> but that only happens when interface is up. When it is down, i40e_open() >> is not being called, thus vsi->rx_buf_len is not set. >> >> Solution for this is calculate buffer length in newly created >> function - i40e_calculate_vsi_rx_buf_len() that return actual buffer >> length. Buffer length is being calculated based on the same rules >> applied previously in i40e_vsi_configure_rx() function. >> >> Fixes: 613142b0bb88 ("i40e: Log error for oversized MTU on device") >> Fixes: 0c8493d90b6b ("i40e: add XDP support for pass and drop actions") >> Signed-off-by: Bartosz Staszewski <bartoszx.staszewski@intel.com> >> Signed-off-by: Mateusz Palczewski <mateusz.palczewski@intel.com> >> Tested-by: Shwetha Nagaraju <Shwetha.nagaraju@intel.com> >> Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com> >> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com> >> --- >> v2: >> - Change title and rework commit message >> - Dropped, previous, patch 1 >> >> v1: >> https://lore.kernel.org/netdev/20221115000324.3040207-1-anthony.l.nguyen@intel.com/ >> >> drivers/net/ethernet/intel/i40e/i40e_main.c | 42 +++++++++++++++------ >> 1 file changed, 30 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c >> b/drivers/net/ethernet/intel/i40e/i40e_main.c >> index 6416322d7c18..b8a8098110eb 100644 >> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c >> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c >> @@ -3693,6 +3693,30 @@ static int i40e_vsi_configure_tx(struct >> i40e_vsi *vsi) >> return err; >> } >> >> +/** >> + * i40e_calculate_vsi_rx_buf_len - Calculates buffer length >> + * >> + * @vsi: VSI to calculate rx_buf_len from >> + */ >> +static u16 i40e_calculate_vsi_rx_buf_len(struct i40e_vsi *vsi) >> +{ >> + u16 ret; >> + >> + if (!vsi->netdev || (vsi->back->flags & I40E_FLAG_LEGACY_RX)) { >> + ret = I40E_RXBUFFER_2048; >> +#if (PAGE_SIZE < 8192) >> + } else if (!I40E_2K_TOO_SMALL_WITH_PADDING && >> + (vsi->netdev->mtu <= ETH_DATA_LEN)) { >> + ret = I40E_RXBUFFER_1536 - NET_IP_ALIGN; >> +#endif >> + } else { >> + ret = (PAGE_SIZE < 8192) ? I40E_RXBUFFER_3072 : >> + I40E_RXBUFFER_2048; >> + } >> + >> + return ret; >> +} > > nit: linux coding style states: > "Do not unnecessarily use braces where a single statement will do" > > I think this applies here. > > Also you could simplify this function to do early returns and drop the u16 > ret variable and all the else statements. > > if (condition 1) > return val1; > > if (condition 2) > return val2; > > return val3; > > > Other than that LGTM. Thanks for the review. Will make these adjustments in next version.
diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c index 6416322d7c18..b8a8098110eb 100644 --- a/drivers/net/ethernet/intel/i40e/i40e_main.c +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c @@ -3693,6 +3693,30 @@ static int i40e_vsi_configure_tx(struct i40e_vsi *vsi) return err; } +/** + * i40e_calculate_vsi_rx_buf_len - Calculates buffer length + * + * @vsi: VSI to calculate rx_buf_len from + */ +static u16 i40e_calculate_vsi_rx_buf_len(struct i40e_vsi *vsi) +{ + u16 ret; + + if (!vsi->netdev || (vsi->back->flags & I40E_FLAG_LEGACY_RX)) { + ret = I40E_RXBUFFER_2048; +#if (PAGE_SIZE < 8192) + } else if (!I40E_2K_TOO_SMALL_WITH_PADDING && + (vsi->netdev->mtu <= ETH_DATA_LEN)) { + ret = I40E_RXBUFFER_1536 - NET_IP_ALIGN; +#endif + } else { + ret = (PAGE_SIZE < 8192) ? I40E_RXBUFFER_3072 : + I40E_RXBUFFER_2048; + } + + return ret; +} + /** * i40e_vsi_configure_rx - Configure the VSI for Rx * @vsi: the VSI being configured @@ -3704,20 +3728,14 @@ static int i40e_vsi_configure_rx(struct i40e_vsi *vsi) int err = 0; u16 i; - if (!vsi->netdev || (vsi->back->flags & I40E_FLAG_LEGACY_RX)) { - vsi->max_frame = I40E_MAX_RXBUFFER; - vsi->rx_buf_len = I40E_RXBUFFER_2048; + vsi->max_frame = I40E_MAX_RXBUFFER; + vsi->rx_buf_len = i40e_calculate_vsi_rx_buf_len(vsi); + #if (PAGE_SIZE < 8192) - } else if (!I40E_2K_TOO_SMALL_WITH_PADDING && - (vsi->netdev->mtu <= ETH_DATA_LEN)) { + if (vsi->netdev && !I40E_2K_TOO_SMALL_WITH_PADDING && + vsi->netdev->mtu <= ETH_DATA_LEN) vsi->max_frame = I40E_RXBUFFER_1536 - NET_IP_ALIGN; - vsi->rx_buf_len = I40E_RXBUFFER_1536 - NET_IP_ALIGN; #endif - } else { - vsi->max_frame = I40E_MAX_RXBUFFER; - vsi->rx_buf_len = (PAGE_SIZE < 8192) ? I40E_RXBUFFER_3072 : - I40E_RXBUFFER_2048; - } /* set up individual rings */ for (i = 0; i < vsi->num_queue_pairs && !err; i++) @@ -13282,7 +13300,7 @@ static int i40e_xdp_setup(struct i40e_vsi *vsi, struct bpf_prog *prog, int i; /* Don't allow frames that span over multiple buffers */ - if (frame_size > vsi->rx_buf_len) { + if (frame_size > i40e_calculate_vsi_rx_buf_len(vsi)) { NL_SET_ERR_MSG_MOD(extack, "MTU too large to enable XDP"); return -EINVAL; }