Message-ID: <e9828f6a-3a2a-4128-bb2c-5dc75d013a9e@intel.com>
Date: Tue, 10 Sep 2024 13:10:35 +0200
From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
To: "Loktionov, Aleksandr" <aleksandr.loktionov@...el.com>, "Fijalkowski,
 Maciej" <maciej.fijalkowski@...el.com>
CC: "intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
	"Nguyen, Anthony L" <anthony.l.nguyen@...el.com>, "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>, "Sokolowski, Jan" <jan.sokolowski@...el.com>,
	"Connolly, Padraig J" <padraig.j.connolly@...el.com>
Subject: Re: [PATCH iwl-next v3] i40e: add ability to reset vf for tx and rx
 mdd events

On 9/10/24 10:29, Loktionov, Aleksandr wrote:
>> From: Fijalkowski, Maciej <maciej.fijalkowski@...el.com>
>> Sent: Tuesday, September 3, 2024 9:59 PM
>> To: Loktionov, Aleksandr <aleksandr.loktionov@...el.com>
>> Cc: intel-wired-lan@...ts.osuosl.org; Nguyen, Anthony L
>> <anthony.l.nguyen@...el.com>; netdev@...r.kernel.org; Sokolowski, Jan
>> <jan.sokolowski@...el.com>; Connolly, Padraig J
>> <padraig.j.connolly@...el.com>
>> Subject: Re: [PATCH iwl-next v3] i40e: add ability to reset vf for tx
>> and rx mdd events

please capitalize acronyms (Tx, Rx, VF, MDD, PF)
(also in the subject line, but sent next version as v4).

>>
>> On Fri, Aug 30, 2024 at 09:28:07PM +0200, Aleksandr Loktionov wrote:
>>> In cases when vf sends malformed packets that are classified as
>>> malicious, sometimes it causes tx queue to freeze. This frozen queue
>>> can be stuck for several minutes being unusable. When mdd event
>>> occurs, there is a posibility to perform a graceful vf reset to
>>> quickly bring vf back to operational state.
>>>
>>> Currently vf iqueues are being disabled if mdd event occurs.
>>> Add the ability to reset vf if tx or rx mdd occurs.
>>> Add mdd events logging throttling /* avoid dmesg polution */.
>>> Unify tx rx mdd messages formats.
>>>
>>> Co-developed-by: Jan Sokolowski <jan.sokolowski@...el.com>
>>> Signed-off-by: Jan Sokolowski <jan.sokolowski@...el.com>
>>> Co-developed-by: Padraig J Connolly <padraig.j.connolly@...el.com>
>>> Signed-off-by:  Padraig J Connolly <padraig.j.connolly@...el.com>
>>> Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@...el.com>

Next time please wait for review on our internal e1000-mailing-list.
Feel free to ping me directly if there will be no one engaged in any
future series of yours.

>>> ---
>>> v2->v3 fix compilation issue
>>> v1->v2 fix compilation issue
>>> ---
>>>   drivers/net/ethernet/intel/i40e/i40e.h        |   4 +-
>>>   .../net/ethernet/intel/i40e/i40e_debugfs.c    |   2 +-
>>>   .../net/ethernet/intel/i40e/i40e_ethtool.c    |   2 +
>>>   drivers/net/ethernet/intel/i40e/i40e_main.c   | 116
>> ++++++++++++++++--
>>>   .../ethernet/intel/i40e/i40e_virtchnl_pf.c    |   2 +-
>>>   .../ethernet/intel/i40e/i40e_virtchnl_pf.h    |  11 +-
>>>   6 files changed, 122 insertions(+), 15 deletions(-)
>>>


>>> --- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
>>> +++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
>>> @@ -459,6 +459,8 @@ static const struct i40e_priv_flags
>> i40e_gstrings_priv_flags[] = {
>>>   	I40E_PRIV_FLAG("base-r-fec", I40E_FLAG_BASE_R_FEC, 0),
>>>   	I40E_PRIV_FLAG("vf-vlan-pruning",
>>>   		       I40E_FLAG_VF_VLAN_PRUNING_ENA, 0),
>>> +	I40E_PRIV_FLAG("mdd-auto-reset-vf",
>>> +		       I40E_FLAG_MDD_AUTO_RESET_VF, 0),
>>
>> you don't tell us that this is implemented via priv-flag in the commit
>> message, would be good to include info about it.
> This flag is implemented for other network adapters like ice, we thought it's kind of standard.
> Can you suggest what exact part to change? Please be concrete.
> Thank you

priv-flag is not a standard, by definition
what we do in intel drivers is also not necessarily a standard

keeping the code quality as-is should be rather seen as an allowance
for legacy drivers, instead of something that should be copy-pasted yet
again. But commit messages are different, you need to obey the current
standard, which is simply: describe non-obvious things, describe more
if asked during review. Please do so :)

>>> +static void i40e_print_vf_rx_mdd_event(struct i40e_pf *pf, struct
>>> +i40e_vf *vf) {
>>> +	dev_err(&pf->pdev->dev, "%lld Rx Malicious Driver Detection
>> events detected on PF %d VF %d MAC %pm. mdd-auto-reset-vfs=%s\n",
>>> +		vf->mdd_rx_events.count,
>>> +		pf->hw.pf_id,
>>> +		vf->vf_id,
>>> +		vf->default_lan_addr.addr,
>>> +		test_bit(I40E_FLAG_MDD_AUTO_RESET_VF, pf->flags) ? "on" :
>> "off"); }
>>> +
>>> +/**
>>> + * i40e_print_vf_tx_mdd_event - print VF Tx malicious driver detect
>>> +event
>>> + * @pf: board private structure
>>> + * @vf: pointer to the VF structure
>>> + */
>>> +static void i40e_print_vf_tx_mdd_event(struct i40e_pf *pf, struct
>>> +i40e_vf *vf) {
>>> +	dev_err(&pf->pdev->dev, "%lld Tx Malicious Driver Detection
>> events detected on PF %d VF %d MAC %pm. mdd-auto-reset-vfs=%s\n",
>>> +		vf->mdd_tx_events.count,
>>> +		pf->hw.pf_id,
>>> +		vf->vf_id,
>>> +		vf->default_lan_addr.addr,
>>> +		test_bit(I40E_FLAG_MDD_AUTO_RESET_VF, pf->flags) ? "on" :
>> "off"); }
>>
>> Unnecesary code duplication, two functions with printing the very same
>> statement with a single different letter.
> But it's easy to grep and find as required by linux coding standards.

You could reword to have it still obvious what to grep for, like:

Malicious Driver Detected an Event, PF: %d, VF: %d, MAC: %pm, dir: %s...

with the last %s being "Tx" or "Rx"
(note: I didn't copied all your stuff as this is just an example)

>>> +
>>> +	/* VF MDD event logs are rate limited to one second intervals */
>>> +	if (time_is_after_jiffies(pf->last_printed_mdd_jiffies + HZ *
>> 1))
>>> +		return;
>>> +
>>> +	pf->last_printed_mdd_jiffies = jiffies;
>>
>> why homegrown rate limiting?
> Because it works! And other ideas probably didn't.
> What is your suggestion exactly? Please be concrete.

dev_info_ratelimited()

Powered by blists - more mailing lists