[PATCH 1/2] PATCH: i40e Improve trusted VF MAC addresses logging when limit is reached

[PATCH 1/2] PATCH: i40e Improve trusted VF MAC addresses logging when limit is reached

[David Hill]

When a VF reaches the limit introduced in this commit [1], the host reports
an error in the syslog but doesn't mention which VF reached its limit and
what the limit is actually is which makes troubleshooting of networking
issue a bit tedious.   This commit simply improves this error reporting
by adding which VF number has reached a limit and what that limit is.

[1] commit cfb1d572c986 ("i40e: Add ensurance of MacVlan resources for every
trusted VF")

Signed-off-by: David Hill <dhill@redhat.com>
---
 drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
index 9b8efdeafbcf..c66c8bbc3993 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
@@ -2932,13 +2932,14 @@ static inline int i40e_check_vf_permission(struct i40e_vf *vf,
 			++mac2add_cnt;
 	}
 
+	int new_count = i40e_count_filters(vsi) + mac2add_cnt;
+	int max_macvlan = I40E_VC_MAX_MACVLAN_PER_TRUSTED_VF(pf->num_alloc_vfs, hw->num_ports);
 	/* If this VF is not privileged, then we can't add more than a limited
 	 * number of addresses. Check to make sure that the additions do not
 	 * push us over the limit.
 	 */
 	if (!test_bit(I40E_VIRTCHNL_VF_CAP_PRIVILEGE, &vf->vf_caps)) {
-		if ((i40e_count_filters(vsi) + mac2add_cnt) >
-		    I40E_VC_MAX_MAC_ADDR_PER_VF) {
+		if ( new_count > I40E_VC_MAX_MAC_ADDR_PER_VF) {
 			dev_err(&pf->pdev->dev,
 				"Cannot add more MAC addresses, VF is not trusted, switch the VF to trusted to add more functionality\n");
 			return -EPERM;
@@ -2949,11 +2950,10 @@ static inline int i40e_check_vf_permission(struct i40e_vf *vf,
 	 * all VFs.
 	 */
 	} else {
-		if ((i40e_count_filters(vsi) + mac2add_cnt) >
-		    I40E_VC_MAX_MACVLAN_PER_TRUSTED_VF(pf->num_alloc_vfs,
-						       hw->num_ports)) {
+		if (new_count > max_macvlan) {
 			dev_err(&pf->pdev->dev,
-				"Cannot add more MAC addresses, trusted VF exhausted it's resources\n");
+				"Cannot add more MAC addresses, trusted VF %d uses (%d/%d) MAC addresses\n",
+				vf->vf_id, new_count, max_macvlan);
 			return -EPERM;
 		}
 	}
-- 
2.50.1


-- 
This message has been scanned for viruses and
dangerous content by MailScanner, and is
believed to be clean.

[PATCH 2/2] PATCH: i40e Add module option to disable max VF limit

[David Hill]

When a VF reaches the limit introduced in this commit [1], the driver
refuses to add any more MACs to the filter which changes the behavior
from previous releases and might break some NFVs which sometimes add
more VFs than the hardcoded limit of 18 and variable limit depending
on the number of VFs created on a given PF.   Disabling limit_mac_per_vf
would revert to previous behavior.

[1] commit cfb1d572c986 ("i40e: Add ensurance of MacVlan resources for every
trusted VF")

Signed-off-by: David Hill <dhill@redhat.com>
---
 drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
index c66c8bbc3993..fb9eb4a80069 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
@@ -4,6 +4,10 @@
 #include "i40e.h"
 #include "i40e_lan_hmc.h"
 #include "i40e_virtchnl_pf.h"
+#include <linux/moduleparam.h>
+
+bool __read_mostly limit_mac_per_vf = 1;
+module_param_named(limit_mac_per_vf, limit_mac_per_vf, bool, 0444);
 
 /*********************notification routines***********************/
 
@@ -2950,7 +2954,7 @@ static inline int i40e_check_vf_permission(struct i40e_vf *vf,
 	 * all VFs.
 	 */
 	} else {
-		if (new_count > max_macvlan) {
+		if (new_count > max_macvlan && limit_mac_per_vf) {
 			dev_err(&pf->pdev->dev,
 				"Cannot add more MAC addresses, trusted VF %d uses (%d/%d) MAC addresses\n",
 				vf->vf_id, new_count, max_macvlan);
-- 
2.50.1


-- 
This message has been scanned for viruses and
dangerous content by MailScanner, and is
believed to be clean.

Re: [PATCH 2/2] PATCH: i40e Add module option to disable max VF limit

[Simon Horman]

On Tue, Aug 05, 2025 at 09:40:42AM -0400, David Hill wrote:
> When a VF reaches the limit introduced in this commit [1], the driver
> refuses to add any more MACs to the filter which changes the behavior
> from previous releases and might break some NFVs which sometimes add
> more VFs than the hardcoded limit of 18 and variable limit depending
> on the number of VFs created on a given PF.   Disabling limit_mac_per_vf
> would revert to previous behavior.
> 
> [1] commit cfb1d572c986 ("i40e: Add ensurance of MacVlan resources for every
> trusted VF")
> 
> Signed-off-by: David Hill <dhill@redhat.com>

Hi David,

Unfortunately adding new module parameters to Ethernet drivers is discouraged.
I would suggest that devlink is an appropriate mechanism.

-- 
pw-bot: changes-requested

Re: [PATCH 2/2] PATCH: i40e Add module option to disable max VF limit

[Jacob Keller]

[-- Attachment #1.1: Type: text/plain, Size: 1454 bytes --]



On 8/5/2025 12:52 PM, Simon Horman wrote:
> On Tue, Aug 05, 2025 at 09:40:42AM -0400, David Hill wrote:
>> When a VF reaches the limit introduced in this commit [1], the driver
>> refuses to add any more MACs to the filter which changes the behavior
>> from previous releases and might break some NFVs which sometimes add
>> more VFs than the hardcoded limit of 18 and variable limit depending
>> on the number of VFs created on a given PF.   Disabling limit_mac_per_vf
>> would revert to previous behavior.
>>
>> [1] commit cfb1d572c986 ("i40e: Add ensurance of MacVlan resources for every
>> trusted VF")
>>
>> Signed-off-by: David Hill <dhill@redhat.com>
> 
> Hi David,
> 
> Unfortunately adding new module parameters to Ethernet drivers is discouraged.
> I would suggest that devlink is an appropriate mechanism.
> 

At a glance, my initial suggestion would be modeling this with devlink
resources?

If each VF has a devlink port in the host, we could create a resource
that is the number of allowed filters for each VF, and the host could
control this through the resource...

I think this might even let us nest a resource so we have one for the
parent which is the total amount available and each VF port could have
its amount available.

A devlink parameter could work but is a bit less flexible and doesn't
show you the hierarchy with the total available filters within the PF vs
what each VF is consuming.

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

Re: [PATCH 2/2] PATCH: i40e Add module option to disable max VF limit

[Simon Horman]

On Thu, Aug 07, 2025 at 09:17:30AM -0700, Jacob Keller wrote:
> 
> 
> On 8/5/2025 12:52 PM, Simon Horman wrote:
> > On Tue, Aug 05, 2025 at 09:40:42AM -0400, David Hill wrote:
> >> When a VF reaches the limit introduced in this commit [1], the driver
> >> refuses to add any more MACs to the filter which changes the behavior
> >> from previous releases and might break some NFVs which sometimes add
> >> more VFs than the hardcoded limit of 18 and variable limit depending
> >> on the number of VFs created on a given PF.   Disabling limit_mac_per_vf
> >> would revert to previous behavior.
> >>
> >> [1] commit cfb1d572c986 ("i40e: Add ensurance of MacVlan resources for every
> >> trusted VF")
> >>
> >> Signed-off-by: David Hill <dhill@redhat.com>
> > 
> > Hi David,
> > 
> > Unfortunately adding new module parameters to Ethernet drivers is discouraged.
> > I would suggest that devlink is an appropriate mechanism.
> > 
> 
> At a glance, my initial suggestion would be modeling this with devlink
> resources?
> 
> If each VF has a devlink port in the host, we could create a resource
> that is the number of allowed filters for each VF, and the host could
> control this through the resource...
> 
> I think this might even let us nest a resource so we have one for the
> parent which is the total amount available and each VF port could have
> its amount available.
> 
> A devlink parameter could work but is a bit less flexible and doesn't
> show you the hierarchy with the total available filters within the PF vs
> what each VF is consuming.

Thanks Jacob,

For some reason I had not thought of modelling filters as a resource.
But I do agree that is is a promising approach.

Re: [PATCH 2/2] PATCH: i40e Add module option to disable max VF limit

[mohammad heib]

Hi Simon, Jacob,

I’ve also been examining this issue, as it’s affecting us.
I agree that handling the number of allowed filters per VF as a devlink 
resource is the best long-term approach.
However, currently in i40e, we only create a devlink port per PF and no 
devlink ports per VF.
Implementing the resource-per-VF approach would therefore require some 
extra work.
For now, could we adopt Simon’s devlink parameter suggestion as a 
temporary solution and consider adding the resource-based approach in 
the future?

On 8/20/25 2:33 PM, Mohammad Heib wrote:
> Hi Simon, Jacob,
>
> I’ve also been examining this issue, as it’s affecting us.
> I agree that handling the number of allowed filters per VF as a 
> devlink resource is the best long-term approach.
> However, currently in i40e, we only create a devlink port per PF and 
> no devlink ports per VF.
> Implementing the resource-per-VF approach would therefore require some 
> extra work.
> For now, could we adopt Simon’s devlink parameter suggestion as a 
> temporary solution and consider adding the resource-based approach in 
> the future?

Re: [PATCH 2/2] PATCH: i40e Add module option to disable max VF limit

[Jacob Keller]

[-- Attachment #1.1: Type: text/plain, Size: 1274 bytes --]



On 8/20/2025 6:09 AM, mohammad heib wrote:
> Hi Simon, Jacob,
> 
> I’ve also been examining this issue, as it’s affecting us.
> I agree that handling the number of allowed filters per VF as a devlink 
> resource is the best long-term approach.
> However, currently in i40e, we only create a devlink port per PF and no 
> devlink ports per VF.

Cross-PF interaction of a global resource is also tricky. Hm.

> Implementing the resource-per-VF approach would therefore require some 
> extra work.
> For now, could we adopt Simon’s devlink parameter suggestion as a 
> temporary solution and consider adding the resource-based approach in 
> the future?

I do agree that its a much larger ask to implement ports and such in
addition.My only concern is we would then likely want to support the
parameter in perpetuity. It is generally preferred that we don't remove
things in the future since it is a pain for software compatibility.
Technically, I don't know if that truly falls under "user space API
changing" if a single driver changes behavior.. but its definitely
frowned on if done without a very good reason. Perhaps there is a way to
make sure parameter works ok even once resources get added in the
future, to mitigate this concern?

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

Re: [PATCH 2/2] PATCH: i40e Add module option to disable max VF limit

[Jacob Keller]

[-- Attachment #1.1: Type: text/plain, Size: 1213 bytes --]



On 8/21/2025 2:58 AM, mohammad heib wrote:
> Hi
> i defiantly agree with you Jacob
> 
> maybe to avoid removing the params in the future we can implement it 
> this way:
> 
> Instead of a simple boolean parameter, we can introduce a *numeric 
> |max_mac_per_vf| parameter*.
> 
>   *
> 
>     If set to |0|, the driver behaves as legacy (no per-VF limit).
> 
>   *
> 
>     Any non-zero value enforces that maximum number of MACs per VF.
> 
> 
> 
> This design addresses concerns about removing a parameter in the future 
> because:
> 
>   * The parameter can *coexist with a future devlink resource-per-VF
>     implementation*.
>   * if a hierarchy of resources is added later, |max_mac_per_vf = 0|
>     could automatically defer to the resource values, while a non-zero
>     value can act as an override.
>   * This numeric parameter is more flexible than a boolean and easier to
>     extend or adjust at runtime (via devlink in the future).
> 
> I think this is a reasonable *short-term solution* that preserves 
> backward compatibility while allowing for a *long-term devlink 
> resource-based design*.
> 

This seems like a good approach to me.

Thanks,
Jake

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

Re: [PATCH 1/2] PATCH: i40e Improve trusted VF MAC addresses logging when limit is reached

[Simon Horman]

On Tue, Aug 05, 2025 at 09:40:41AM -0400, David Hill wrote:
> When a VF reaches the limit introduced in this commit [1], the host reports
> an error in the syslog but doesn't mention which VF reached its limit and
> what the limit is actually is which makes troubleshooting of networking
> issue a bit tedious.   This commit simply improves this error reporting
> by adding which VF number has reached a limit and what that limit is.
> 
> [1] commit cfb1d572c986 ("i40e: Add ensurance of MacVlan resources for every
> trusted VF")
> 
> Signed-off-by: David Hill <dhill@redhat.com>
> ---
>  drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)

Reviewed-by: Simon Horman <horms@kernel.org>

For future reference: please observe the rule that there should
be a delay of 24h between posting versions of a patch to netdev.

https://docs.kernel.org/process/maintainer-netdev.html

...