[Intel-wired-lan] [PATCH] i40e: Add error message when MTU on device is out of the range

[Rybak, Eryk Roch]

Guys,

Friendy reminder.

IMHO, double the log message is not the best solution here. However, I suggest to prepare string on stack earlier and send it as extrack then.
What do you think guys, Maciej/Paul?

Thanks,
-Eryk

>-----Original Message-----
>From: Rybak, Eryk Roch 
>Sent: Friday, January 8, 2021 12:19 PM
>To: Fijalkowski, Maciej <maciej.fijalkowski at intel.com>; Paul Menzel <pmenzel at molgen.mpg.de>
>Cc: intel-wired-lan at lists.osuosl.org
>Subject: RE: [Intel-wired-lan] [PATCH] i40e: Add error message when MTU on device is out of the range
>
>>-----Original Message-----
>>From: Fijalkowski, Maciej <maciej.fijalkowski at intel.com>
>>Sent: Thursday, January 7, 2021 10:28 AM
>>To: Paul Menzel <pmenzel at molgen.mpg.de>
>>Cc: Rybak, Eryk Roch <eryk.roch.rybak at intel.com>; 
>>intel-wired-lan at lists.osuosl.org
>>Subject: Re: [Intel-wired-lan] [PATCH] i40e: Add error message when MTU 
>>on device is out of the range
>>
>>On Wed, Jan 06, 2021 at 10:36:23AM +0100, Paul Menzel wrote:
>>> Dear Eryk,
>>
>>Eryk, you still haven't specified the tree that this patch is supposed 
>>to land (nit - it will be net-next :))
>>
>Sure I will add to the second version of patch, thanks for hint.
>>> 
>>> 
>>> Am 18.12.20 um 17:52 schrieb Eryk Rybak:
>>> 
>>> Maybe a shorter summary:
>>> 
>>> > i40e: Log error for oversized MTU on device
>>> 
>>> > When attempting to link XDP prog with MTU larger than supported, 
>>> > user is not informed why XDP linking fails. Adding proper
>>> 
>>> Nit: Imperative mood could be used instead: Add proper …
>>
>>+1, thanks Paul!
>Thanks guys, expect better commit message in next version of this patch.
>>
>>> 
>>> > error message.
>>> 
>>> Personally, when adding new log message, I find it nice to see them 
>>> in the commit message too.
>>> 
>>> > Signed-off-by: Arkadiusz Kubalewski 
>>> > <arkadiusz.kubalewski at intel.com>
>>> > Signed-off-by: Eryk Rybak <eryk.roch.rybak at intel.com>
>>> > ---
>>> >   drivers/net/ethernet/intel/i40e/i40e_main.c | 11 +++++++----
>>> >   1 file changed, 7 insertions(+), 4 deletions(-)
>>> > 
>>> > diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c
>>> > b/drivers/net/ethernet/intel/i40e/i40e_main.c
>>> > index 630258e..4fdef00 100644
>>> > --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
>>> > +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
>>> > @@ -12933,9 +12933,10 @@ static netdev_features_t i40e_features_check(struct sk_buff *skb,
>>> >    * i40e_xdp_setup - add/remove an XDP program
>>> >    * @vsi: VSI to changed
>>> >    * @prog: XDP program
>>> > + * @extack: netlink extended ack
>>> >    **/
>>> > -static int i40e_xdp_setup(struct i40e_vsi *vsi,
>>> > -			  struct bpf_prog *prog)
>>> > +static int i40e_xdp_setup(struct i40e_vsi *vsi, struct bpf_prog *prog,
>>> > +			  struct netlink_ext_ack *extack)
>>> >   {
>>> >   	int frame_size = vsi->netdev->mtu + ETH_HLEN + ETH_FCS_LEN + VLAN_HLEN;
>>> >   	struct i40e_pf *pf = vsi->back;
>>> > @@ -12944,8 +12945,10 @@ static int i40e_xdp_setup(struct i40e_vsi *vsi,
>>> >   	int i;
>>> >   	/* Don't allow frames that span over multiple buffers */
>>> > -	if (frame_size > vsi->rx_buf_len)
>>> > +	if (frame_size > vsi->rx_buf_len) {
>>> > +		NL_SET_ERR_MSG_MOD(extack, "MTU too large to enable XDP");
>>> 
>>> Could you please also print out both values? Maybe (unsure about the units):
>>> 
>>>    MTU of %u bytes(?) too large to enable XDP (maximum: %u bytes)
>>
>>No, extack does not support printf-like format specifiers, 
>>unfortunately :<
>>
>>Maybe we can double the message and have a separate dev_info() in here?
>
>IMHO, double the log message is not the best solution here. However, I suggest to prepare string on stack earlier and send it as extrack then.
>What do you think guys, Maciej/Paul?
>
>>
>>> 
>>> 
>>> Kind regards,
>>> 
>>> Paul
>>> 
>>> 
>>> >   		return -EINVAL;
>>> > +	}
>>> >   	/* When turning XDP on->off/off->on we reset and rebuild the rings. */
>>> >   	need_reset = (i40e_enabled_xdp_vsi(vsi) != !!prog); @@ -13254,7
>>> > +13257,7 @@ static int i40e_xdp(struct net_device *dev,
>>> >   	switch (xdp->command) {
>>> >   	case XDP_SETUP_PROG:
>>> > -		return i40e_xdp_setup(vsi, xdp->prog);
>>> > +		return i40e_xdp_setup(vsi, xdp->prog, xdp->extack);
>>> >   	case XDP_SETUP_XSK_POOL:
>>> >   		return i40e_xsk_pool_setup(vsi, xdp->xsk.pool,
>>> >   					   xdp->xsk.queue_id);
>>> > 
>>> > base-commit: d2c2bbad93b818020657a82121a147e9ace1230d
>>> _______________________________________________
>>> Intel-wired-lan mailing list
>>> Intel-wired-lan at osuosl.org
>>> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
---------------------------------------------------------------------
Intel Technology Poland sp. z o.o.
ul. Sowackiego 173 | 80-298 Gdask | Sd Rejonowy Gdask Pnoc | VII Wydzia Gospodarczy Krajowego Rejestru Sdowego - KRS 101882 | NIP 957-07-52-316 | Kapita zakadowy 200.000 PLN.
Ta wiadomo wraz z zacznikami jest przeznaczona dla okrelonego adresata i moe zawiera informacje poufne. W razie przypadkowego otrzymania tej wiadomoci, prosimy o powiadomienie nadawcy oraz trwae jej usunicie; jakiekolwiek przegldanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by others is strictly prohibited.