[PATCH net-next 0/3][pull request] Intel Wired LAN Driver Updates 2023-06-02 (iavf)

[PATCH net-next 0/3][pull request] Intel Wired LAN Driver Updates 2023-06-02 (iavf)

[Tony Nguyen]

This series contains updates to iavf driver only.

Piotr avoids sending request for MAC address if it is the current MAC
address.

Przemek defers removing, previous, primary MAC address until after
getting result of adding its replacement.

Ahmed removes unnecessary masking when setting up interrupts.

The following are changes since commit 3f06760c00f56c5fe6c7f3361c2cf64becee1174:
  ipv4: Drop tos parameter from flowi4_update_output()
and are available in the git repository at:
  git://git.kernel.org/pub/scm/linux/kernel/git/tnguy/next-queue 40GbE

Ahmed Zaki (1):
  iavf: remove mask from iavf_irq_enable_queues()

Piotr Gardocki (1):
  iavf: add check for current MAC address in set_mac callback

Przemek Kitszel (1):
  iavf: fix err handling for MAC replace

 drivers/net/ethernet/intel/iavf/iavf.h        |  2 +-
 drivers/net/ethernet/intel/iavf/iavf_main.c   | 63 +++++++++----------
 .../net/ethernet/intel/iavf/iavf_register.h   |  2 +-
 3 files changed, 33 insertions(+), 34 deletions(-)

-- 
2.38.1

[PATCH net-next 1/3] iavf: add check for current MAC address in set_mac callback

[Tony Nguyen]

From: Piotr Gardocki <piotrx.gardocki@intel.com>

In some cases it is possible for kernel to come with request
to change primary MAC address to the address that is actually
already set on the given interface.

If the old and new MAC addresses are equal there is no need
for going through entire routine, including AdminQ and
waitqueue.

This patch adds proper check to return fast from the function
in these cases. The same check can also be found in i40e and
ice drivers.

An example of such case is adding an interface to bonding
channel in balance-alb mode:
modprobe bonding mode=balance-alb miimon=100 max_bonds=1
ip link set bond0 up
ifenslave bond0 <eth>

Signed-off-by: Piotr Gardocki <piotrx.gardocki@intel.com>
Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
Tested-by: Rafal Romanowski <rafal.romanowski@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/iavf/iavf_main.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
index 2de4baff4c20..420aaca548a0 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_main.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
@@ -1088,6 +1088,12 @@ static int iavf_set_mac(struct net_device *netdev, void *p)
 	if (!is_valid_ether_addr(addr->sa_data))
 		return -EADDRNOTAVAIL;
 
+	if (ether_addr_equal(netdev->dev_addr, addr->sa_data)) {
+		netdev_dbg(netdev, "already using mac address %pM\n",
+			   addr->sa_data);
+		return 0;
+	}
+
 	ret = iavf_replace_primary_mac(adapter, addr->sa_data);
 
 	if (ret)
-- 
2.38.1

Re: [PATCH net-next 1/3] iavf: add check for current MAC address in set_mac callback

[Simon Horman]

On Fri, Jun 02, 2023 at 10:13:00AM -0700, Tony Nguyen wrote:
> From: Piotr Gardocki <piotrx.gardocki@intel.com>
> 
> In some cases it is possible for kernel to come with request
> to change primary MAC address to the address that is actually
> already set on the given interface.
> 
> If the old and new MAC addresses are equal there is no need
> for going through entire routine, including AdminQ and
> waitqueue.
> 
> This patch adds proper check to return fast from the function
> in these cases. The same check can also be found in i40e and
> ice drivers.
> 
> An example of such case is adding an interface to bonding
> channel in balance-alb mode:
> modprobe bonding mode=balance-alb miimon=100 max_bonds=1
> ip link set bond0 up
> ifenslave bond0 <eth>
> 
> Signed-off-by: Piotr Gardocki <piotrx.gardocki@intel.com>
> Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
> Tested-by: Rafal Romanowski <rafal.romanowski@intel.com>
> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>

Reviewed-by: Simon Horman <simon.horman@corigine.com>

Re: [PATCH net-next 1/3] iavf: add check for current MAC address in set_mac callback

[Maciej Fijalkowski]

On Fri, Jun 02, 2023 at 10:13:00AM -0700, Tony Nguyen wrote:
> From: Piotr Gardocki <piotrx.gardocki@intel.com>
> 
> In some cases it is possible for kernel to come with request
> to change primary MAC address to the address that is actually
> already set on the given interface.
> 
> If the old and new MAC addresses are equal there is no need
> for going through entire routine, including AdminQ and
> waitqueue.
> 
> This patch adds proper check to return fast from the function
> in these cases. The same check can also be found in i40e and
> ice drivers.

couldn't this be checked the layer above then? and pulled out of drivers?

> 
> An example of such case is adding an interface to bonding
> channel in balance-alb mode:
> modprobe bonding mode=balance-alb miimon=100 max_bonds=1
> ip link set bond0 up
> ifenslave bond0 <eth>
> 
> Signed-off-by: Piotr Gardocki <piotrx.gardocki@intel.com>
> Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
> Tested-by: Rafal Romanowski <rafal.romanowski@intel.com>
> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> ---
>  drivers/net/ethernet/intel/iavf/iavf_main.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
> index 2de4baff4c20..420aaca548a0 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
> @@ -1088,6 +1088,12 @@ static int iavf_set_mac(struct net_device *netdev, void *p)
>  	if (!is_valid_ether_addr(addr->sa_data))
>  		return -EADDRNOTAVAIL;
>  
> +	if (ether_addr_equal(netdev->dev_addr, addr->sa_data)) {
> +		netdev_dbg(netdev, "already using mac address %pM\n",
> +			   addr->sa_data);

i am not sure if this is helpful message, you end up with an address that
you requested, why would you care that it was already same us you wanted?

> +		return 0;
> +	}
> +
>  	ret = iavf_replace_primary_mac(adapter, addr->sa_data);
>  
>  	if (ret)
> -- 
> 2.38.1
> 
> 

Re: [PATCH net-next 1/3] iavf: add check for current MAC address in set_mac callback

[Piotr Gardocki]

On 05.06.2023 21:02, Maciej Fijalkowski wrote:
> On Fri, Jun 02, 2023 at 10:13:00AM -0700, Tony Nguyen wrote:
>> From: Piotr Gardocki <piotrx.gardocki@intel.com>
>>
>> In some cases it is possible for kernel to come with request
>> to change primary MAC address to the address that is actually
>> already set on the given interface.
>>
>> If the old and new MAC addresses are equal there is no need
>> for going through entire routine, including AdminQ and
>> waitqueue.
>>
>> This patch adds proper check to return fast from the function
>> in these cases. The same check can also be found in i40e and
>> ice drivers.
> 
> couldn't this be checked the layer above then? and pulled out of drivers?
> 

Probably it could, but I can't tell for all drivers if such request should
always be ignored. I'm not aware of all possible use cases for this callback
to be called and I can imagine designs where such request should be
always handled.

>>
>> An example of such case is adding an interface to bonding
>> channel in balance-alb mode:
>> modprobe bonding mode=balance-alb miimon=100 max_bonds=1
>> ip link set bond0 up
>> ifenslave bond0 <eth>
>>
>> Signed-off-by: Piotr Gardocki <piotrx.gardocki@intel.com>
>> Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
>> Tested-by: Rafal Romanowski <rafal.romanowski@intel.com>
>> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
>> ---
>>  drivers/net/ethernet/intel/iavf/iavf_main.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
>> index 2de4baff4c20..420aaca548a0 100644
>> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
>> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
>> @@ -1088,6 +1088,12 @@ static int iavf_set_mac(struct net_device *netdev, void *p)
>>  	if (!is_valid_ether_addr(addr->sa_data))
>>  		return -EADDRNOTAVAIL;
>>  
>> +	if (ether_addr_equal(netdev->dev_addr, addr->sa_data)) {
>> +		netdev_dbg(netdev, "already using mac address %pM\n",
>> +			   addr->sa_data);
> 
> i am not sure if this is helpful message, you end up with an address that
> you requested, why would you care that it was already same us you wanted?
> 

You can find similar message in i40e and ice drivers. Please note that this
is a debug message, so it won't print by default. I would leave it this way,
it might be useful in a future for debugging.

>> +		return 0;
>> +	}
>> +
>>  	ret = iavf_replace_primary_mac(adapter, addr->sa_data);
>>  
>>  	if (ret)
>> -- 
>> 2.38.1
>>
>>

Regards,
Piotr

Re: [PATCH net-next 1/3] iavf: add check for current MAC address in set_mac callback

[Maciej Fijalkowski]

On Tue, Jun 06, 2023 at 11:22:55AM +0200, Piotr Gardocki wrote:
> On 05.06.2023 21:02, Maciej Fijalkowski wrote:
> > On Fri, Jun 02, 2023 at 10:13:00AM -0700, Tony Nguyen wrote:
> >> From: Piotr Gardocki <piotrx.gardocki@intel.com>
> >>
> >> In some cases it is possible for kernel to come with request
> >> to change primary MAC address to the address that is actually
> >> already set on the given interface.
> >>
> >> If the old and new MAC addresses are equal there is no need
> >> for going through entire routine, including AdminQ and
> >> waitqueue.
> >>
> >> This patch adds proper check to return fast from the function
> >> in these cases. The same check can also be found in i40e and
> >> ice drivers.
> > 
> > couldn't this be checked the layer above then? and pulled out of drivers?
> > 
> 
> Probably it could, but I can't tell for all drivers if such request should
> always be ignored. I'm not aware of all possible use cases for this callback
> to be called and I can imagine designs where such request should be
> always handled.

if you can imagine a case where such request should be handled then i'm
all ears. it feels like this is in an optimization where everyone could
benefit from (no expert in this scope though), but yeah this callback went
into the wild and it's implemented all over the place.

> 
> >>
> >> An example of such case is adding an interface to bonding
> >> channel in balance-alb mode:
> >> modprobe bonding mode=balance-alb miimon=100 max_bonds=1
> >> ip link set bond0 up
> >> ifenslave bond0 <eth>
> >>
> >> Signed-off-by: Piotr Gardocki <piotrx.gardocki@intel.com>
> >> Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
> >> Tested-by: Rafal Romanowski <rafal.romanowski@intel.com>
> >> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> >> ---
> >>  drivers/net/ethernet/intel/iavf/iavf_main.c | 6 ++++++
> >>  1 file changed, 6 insertions(+)
> >>
> >> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
> >> index 2de4baff4c20..420aaca548a0 100644
> >> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
> >> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
> >> @@ -1088,6 +1088,12 @@ static int iavf_set_mac(struct net_device *netdev, void *p)
> >>  	if (!is_valid_ether_addr(addr->sa_data))
> >>  		return -EADDRNOTAVAIL;
> >>  
> >> +	if (ether_addr_equal(netdev->dev_addr, addr->sa_data)) {
> >> +		netdev_dbg(netdev, "already using mac address %pM\n",
> >> +			   addr->sa_data);
> > 
> > i am not sure if this is helpful message, you end up with an address that
> > you requested, why would you care that it was already same us you wanted?
> > 
> 
> You can find similar message in i40e and ice drivers. Please note that this
> is a debug message, so it won't print by default. I would leave it this way,
> it might be useful in a future for debugging.

hmm fair enough :) :
Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>

CC: Olek
do you think libie could implement common ndo callbacks?

> 
> >> +		return 0;
> >> +	}
> >> +
> >>  	ret = iavf_replace_primary_mac(adapter, addr->sa_data);
> >>  
> >>  	if (ret)
> >> -- 
> >> 2.38.1
> >>
> >>
> 
> Regards,
> Piotr

Re: [PATCH net-next 1/3] iavf: add check for current MAC address in set_mac callback

[Alexander Lobakin]

From: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
Date: Tue, 6 Jun 2023 12:21:07 +0200

[...]

>>>> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
>>>> index 2de4baff4c20..420aaca548a0 100644
>>>> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
>>>> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
>>>> @@ -1088,6 +1088,12 @@ static int iavf_set_mac(struct net_device *netdev, void *p)
>>>>  	if (!is_valid_ether_addr(addr->sa_data))
>>>>  		return -EADDRNOTAVAIL;
>>>>  
>>>> +	if (ether_addr_equal(netdev->dev_addr, addr->sa_data)) {
>>>> +		netdev_dbg(netdev, "already using mac address %pM\n",
>>>> +			   addr->sa_data);
>>>
>>> i am not sure if this is helpful message, you end up with an address that
>>> you requested, why would you care that it was already same us you wanted?
>>>
>>
>> You can find similar message in i40e and ice drivers. Please note that this
>> is a debug message, so it won't print by default. I would leave it this way,
>> it might be useful in a future for debugging.

I'm not a fan of neither this debug print nor the argument that it's
already present in driver A :D When we're developing things, we always
add a bunch of "useful for debugging" stuff to the code.
I also don't really get what this message can help with.

> 
> hmm fair enough :) :
> Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> 
> CC: Olek
> do you think libie could implement common ndo callbacks?

For sure it will. OTOH .ndo_set_mac_address() have a piece of
HW-specific code per each our driver. Configuration path is more
difficult to merge/share than hotpath in general, but will see later.

> 
>>
>>>> +		return 0;
>>>> +	}
>>>> +
>>>>  	ret = iavf_replace_primary_mac(adapter, addr->sa_data);
>>>>  
>>>>  	if (ret)
>>>> -- 
>>>> 2.38.1
>>>>
>>>>
>>
>> Regards,
>> Piotr

Thanks,
Olek

Re: [PATCH net-next 1/3] iavf: add check for current MAC address in set_mac callback

[Jakub Kicinski]

On Tue, 6 Jun 2023 12:21:07 +0200 Maciej Fijalkowski wrote:
> > > couldn't this be checked the layer above then? and pulled out of drivers?
> > 
> > Probably it could, but I can't tell for all drivers if such request should
> > always be ignored. I'm not aware of all possible use cases for this callback
> > to be called and I can imagine designs where such request should be
> > always handled.  
> 
> if you can imagine a case where such request should be handled then i'm
> all ears. it feels like this is in an optimization where everyone could
> benefit from (no expert in this scope though), but yeah this callback went
> into the wild and it's implemented all over the place.

+1, FWIW, this is a net-next change, let's try to put it in the core
unless we see a clear enough reason not to.

Re: [PATCH net-next 1/3] iavf: add check for current MAC address in set_mac callback

[Piotr Gardocki]

On 06.06.2023 19:24, Jakub Kicinski wrote:
> On Tue, 6 Jun 2023 12:21:07 +0200 Maciej Fijalkowski wrote:
>>>> couldn't this be checked the layer above then? and pulled out of drivers?
>>>
>>> Probably it could, but I can't tell for all drivers if such request should
>>> always be ignored. I'm not aware of all possible use cases for this callback
>>> to be called and I can imagine designs where such request should be
>>> always handled.  
>>
>> if you can imagine a case where such request should be handled then i'm
>> all ears. it feels like this is in an optimization where everyone could
>> benefit from (no expert in this scope though), but yeah this callback went
>> into the wild and it's implemented all over the place.
> 
> +1, FWIW, this is a net-next change, let's try to put it in the core
> unless we see a clear enough reason not to.

The reason I was not keen to move it to core is that a lot of drivers actually
perform some actions even when the MAC address doesn't change, like writing
to HW registers, performing link down, link up routine, notifying physical
function drivers, etc. I'm not aware if any of them really need this kind of logic,
so I can't stand strongly against new proposal. If the community is fine with it
we can move it to core code.

I need a piece of advice though:
1) Should I fix it in this patch set, or treat it as a separate thread?
2) I suppose the change is required only in dev_set_mac_address function, but
am I right assuming we should do it before call to dev_pre_changeaddr_notify
and return from function early? What about call to add_device_randomness?

Re: [PATCH net-next 1/3] iavf: add check for current MAC address in set_mac callback

[Jakub Kicinski]

On Wed, 7 Jun 2023 12:29:36 +0200 Piotr Gardocki wrote:
> I need a piece of advice though:
> 1) Should I fix it in this patch set, or treat it as a separate thread?

Separate is probably better, you can post such a change directly 
to netdev, without going via the Intel tree.

> 2) I suppose the change is required only in dev_set_mac_address function, but
> am I right assuming we should do it before call to dev_pre_changeaddr_notify
> and return from function early? What about call to add_device_randomness?

I'd add the check right after the netif_device_present() check and not
worry about notifier or randomness. The address isn't changing so
nothing to notify about and no real randomness to be gained.

Re: [PATCH net-next 1/3] iavf: add check for current MAC address in set_mac callback

[Maciej Fijalkowski]

On Wed, Jun 07, 2023 at 09:38:10AM -0700, Jakub Kicinski wrote:
> On Wed, 7 Jun 2023 12:29:36 +0200 Piotr Gardocki wrote:
> > I need a piece of advice though:
> > 1) Should I fix it in this patch set, or treat it as a separate thread?
> 
> Separate is probably better, you can post such a change directly 
> to netdev, without going via the Intel tree.

That's what we like:D

> 
> > 2) I suppose the change is required only in dev_set_mac_address function, but
> > am I right assuming we should do it before call to dev_pre_changeaddr_notify
> > and return from function early? What about call to add_device_randomness?
> 
> I'd add the check right after the netif_device_present() check and not
> worry about notifier or randomness. The address isn't changing so
> nothing to notify about and no real randomness to be gained.

I find this as positive side effect - why would i want to notify anyone
that my addr has changed if it was not in fact changed? This is happening
in the current approach where you exit with success code from
ndo_set_mac_address().

[PATCH net-next 2/3] iavf: fix err handling for MAC replace

[Tony Nguyen]

From: Przemek Kitszel <przemyslaw.kitszel@intel.com>

Defer removal of current primary MAC until a replacement is successfully added.
Previous implementation would left filter list with no primary MAC.
This was found while reading the code.

The patch takes advantage of the fact that there can only be a single primary
MAC filter at any time.

Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Signed-off-by: Piotr Gardocki <piotrx.gardocki@intel.com>
Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
Tested-by: Rafal Romanowski <rafal.romanowski@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/iavf/iavf_main.c | 42 ++++++++++-----------
 1 file changed, 19 insertions(+), 23 deletions(-)

diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
index 420aaca548a0..3a78f86ba4f9 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_main.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
@@ -1010,40 +1010,36 @@ int iavf_replace_primary_mac(struct iavf_adapter *adapter,
 			     const u8 *new_mac)
 {
 	struct iavf_hw *hw = &adapter->hw;
-	struct iavf_mac_filter *f;
+	struct iavf_mac_filter *new_f;
+	struct iavf_mac_filter *old_f;
 
 	spin_lock_bh(&adapter->mac_vlan_list_lock);
 
-	list_for_each_entry(f, &adapter->mac_filter_list, list) {
-		f->is_primary = false;
+	new_f = iavf_add_filter(adapter, new_mac);
+	if (!new_f) {
+		spin_unlock_bh(&adapter->mac_vlan_list_lock);
+		return -ENOMEM;
 	}
 
-	f = iavf_find_filter(adapter, hw->mac.addr);
-	if (f) {
-		f->remove = true;
+	old_f = iavf_find_filter(adapter, hw->mac.addr);
+	if (old_f) {
+		old_f->is_primary = false;
+		old_f->remove = true;
 		adapter->aq_required |= IAVF_FLAG_AQ_DEL_MAC_FILTER;
 	}
-
-	f = iavf_add_filter(adapter, new_mac);
-
-	if (f) {
-		/* Always send the request to add if changing primary MAC
-		 * even if filter is already present on the list
-		 */
-		f->is_primary = true;
-		f->add = true;
-		adapter->aq_required |= IAVF_FLAG_AQ_ADD_MAC_FILTER;
-		ether_addr_copy(hw->mac.addr, new_mac);
-	}
+	/* Always send the request to add if changing primary MAC,
+	 * even if filter is already present on the list
+	 */
+	new_f->is_primary = true;
+	new_f->add = true;
+	adapter->aq_required |= IAVF_FLAG_AQ_ADD_MAC_FILTER;
+	ether_addr_copy(hw->mac.addr, new_mac);
 
 	spin_unlock_bh(&adapter->mac_vlan_list_lock);
 
 	/* schedule the watchdog task to immediately process the request */
-	if (f) {
-		mod_delayed_work(adapter->wq, &adapter->watchdog_task, 0);
-		return 0;
-	}
-	return -ENOMEM;
+	mod_delayed_work(adapter->wq, &adapter->watchdog_task, 0);
+	return 0;
 }
 
 /**
-- 
2.38.1

Re: [PATCH net-next 2/3] iavf: fix err handling for MAC replace

[Simon Horman]

On Fri, Jun 02, 2023 at 10:13:01AM -0700, Tony Nguyen wrote:
> From: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> 
> Defer removal of current primary MAC until a replacement is successfully added.
> Previous implementation would left filter list with no primary MAC.
> This was found while reading the code.
> 
> The patch takes advantage of the fact that there can only be a single primary
> MAC filter at any time.
> 
> Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> Signed-off-by: Piotr Gardocki <piotrx.gardocki@intel.com>
> Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
> Tested-by: Rafal Romanowski <rafal.romanowski@intel.com>
> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>

Reviewed-by: Simon Horman <simon.horman@corigine.com>

Re: [PATCH net-next 2/3] iavf: fix err handling for MAC replace

[Maciej Fijalkowski]

On Fri, Jun 02, 2023 at 10:13:01AM -0700, Tony Nguyen wrote:
> From: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> 
> Defer removal of current primary MAC until a replacement is successfully added.
> Previous implementation would left filter list with no primary MAC.

and this opens up for what kind of issues? do you mean that
iavf_add_filter() could break and existing primary filter has been marked
for removal?

> This was found while reading the code.
> 
> The patch takes advantage of the fact that there can only be a single primary
> MAC filter at any time.
> 
> Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> Signed-off-by: Piotr Gardocki <piotrx.gardocki@intel.com>
> Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
> Tested-by: Rafal Romanowski <rafal.romanowski@intel.com>
> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> ---
>  drivers/net/ethernet/intel/iavf/iavf_main.c | 42 ++++++++++-----------
>  1 file changed, 19 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
> index 420aaca548a0..3a78f86ba4f9 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
> @@ -1010,40 +1010,36 @@ int iavf_replace_primary_mac(struct iavf_adapter *adapter,

from what i'm looking at, iavf_replace_primary_mac() could be scoped only
to iavf_main.c and become static func.

>  			     const u8 *new_mac)
>  {
>  	struct iavf_hw *hw = &adapter->hw;
> -	struct iavf_mac_filter *f;
> +	struct iavf_mac_filter *new_f;
> +	struct iavf_mac_filter *old_f;
>  
>  	spin_lock_bh(&adapter->mac_vlan_list_lock);
>  
> -	list_for_each_entry(f, &adapter->mac_filter_list, list) {
> -		f->is_primary = false;
> +	new_f = iavf_add_filter(adapter, new_mac);
> +	if (!new_f) {
> +		spin_unlock_bh(&adapter->mac_vlan_list_lock);
> +		return -ENOMEM;
>  	}
>  
> -	f = iavf_find_filter(adapter, hw->mac.addr);
> -	if (f) {
> -		f->remove = true;
> +	old_f = iavf_find_filter(adapter, hw->mac.addr);
> +	if (old_f) {
> +		old_f->is_primary = false;
> +		old_f->remove = true;
>  		adapter->aq_required |= IAVF_FLAG_AQ_DEL_MAC_FILTER;
>  	}
> -
> -	f = iavf_add_filter(adapter, new_mac);
> -
> -	if (f) {
> -		/* Always send the request to add if changing primary MAC
> -		 * even if filter is already present on the list
> -		 */
> -		f->is_primary = true;
> -		f->add = true;
> -		adapter->aq_required |= IAVF_FLAG_AQ_ADD_MAC_FILTER;
> -		ether_addr_copy(hw->mac.addr, new_mac);
> -	}
> +	/* Always send the request to add if changing primary MAC,
> +	 * even if filter is already present on the list
> +	 */
> +	new_f->is_primary = true;
> +	new_f->add = true;
> +	adapter->aq_required |= IAVF_FLAG_AQ_ADD_MAC_FILTER;
> +	ether_addr_copy(hw->mac.addr, new_mac);
>  
>  	spin_unlock_bh(&adapter->mac_vlan_list_lock);
>  
>  	/* schedule the watchdog task to immediately process the request */
> -	if (f) {
> -		mod_delayed_work(adapter->wq, &adapter->watchdog_task, 0);
> -		return 0;
> -	}
> -	return -ENOMEM;
> +	mod_delayed_work(adapter->wq, &adapter->watchdog_task, 0);
> +	return 0;
>  }
>  
>  /**
> -- 
> 2.38.1
> 
> 

Re: [PATCH net-next 2/3] iavf: fix err handling for MAC replace

[Przemek Kitszel]

On 6/5/23 21:17, Maciej Fijalkowski wrote:
> On Fri, Jun 02, 2023 at 10:13:01AM -0700, Tony Nguyen wrote:
>> From: Przemek Kitszel <przemyslaw.kitszel@intel.com>
>>
>> Defer removal of current primary MAC until a replacement is successfully added.
>> Previous implementation would left filter list with no primary MAC.
> 
> and this opens up for what kind of issues? do you mean that
> iavf_add_filter() could break and existing primary filter has been marked
> for removal?

Yes, prior to the patch the flow was:
1. mark all MACs non-primary;
2. mark current HW MAC for removal;
3. try to add new MAC, say it fails, so that's an end with -ENOMEM;
4. ::is_primary and ::remove fields for the ::mac_filter_list, alongside 
with ::aq_required are left modified, to be finalized next time 
user/watchdog processes that.

For me it was enough to treat it as a bug, and for sure a "bad smell".


> 
>> This was found while reading the code.
>>
>> The patch takes advantage of the fact that there can only be a single primary
>> MAC filter at any time.
>>
>> Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
>> Signed-off-by: Piotr Gardocki <piotrx.gardocki@intel.com>
>> Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
>> Tested-by: Rafal Romanowski <rafal.romanowski@intel.com>
>> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
>> ---
>>   drivers/net/ethernet/intel/iavf/iavf_main.c | 42 ++++++++++-----------
>>   1 file changed, 19 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
>> index 420aaca548a0..3a78f86ba4f9 100644
>> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
>> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
>> @@ -1010,40 +1010,36 @@ int iavf_replace_primary_mac(struct iavf_adapter *adapter,
> 
> from what i'm looking at, iavf_replace_primary_mac() could be scoped only
> to iavf_main.c and become static func.
> 

makes sense, thanks

>>   			     const u8 *new_mac)
>>   {
>>   	struct iavf_hw *hw = &adapter->hw;
>> -	struct iavf_mac_filter *f;
>> +	struct iavf_mac_filter *new_f;
>> +	struct iavf_mac_filter *old_f;
>>   
>>   	spin_lock_bh(&adapter->mac_vlan_list_lock);
>>   
>> -	list_for_each_entry(f, &adapter->mac_filter_list, list) {
>> -		f->is_primary = false;
>> +	new_f = iavf_add_filter(adapter, new_mac);
>> +	if (!new_f) {
>> +		spin_unlock_bh(&adapter->mac_vlan_list_lock);
>> +		return -ENOMEM;
>>   	}
>>   
>> -	f = iavf_find_filter(adapter, hw->mac.addr);
>> -	if (f) {
>> -		f->remove = true;
>> +	old_f = iavf_find_filter(adapter, hw->mac.addr);
>> +	if (old_f) {
>> +		old_f->is_primary = false;
>> +		old_f->remove = true;
>>   		adapter->aq_required |= IAVF_FLAG_AQ_DEL_MAC_FILTER;
>>   	}
>> -
>> -	f = iavf_add_filter(adapter, new_mac);
>> -
>> -	if (f) {
>> -		/* Always send the request to add if changing primary MAC
>> -		 * even if filter is already present on the list
>> -		 */
>> -		f->is_primary = true;
>> -		f->add = true;
>> -		adapter->aq_required |= IAVF_FLAG_AQ_ADD_MAC_FILTER;
>> -		ether_addr_copy(hw->mac.addr, new_mac);
>> -	}
>> +	/* Always send the request to add if changing primary MAC,
>> +	 * even if filter is already present on the list
>> +	 */
>> +	new_f->is_primary = true;
>> +	new_f->add = true;
>> +	adapter->aq_required |= IAVF_FLAG_AQ_ADD_MAC_FILTER;
>> +	ether_addr_copy(hw->mac.addr, new_mac);
>>   
>>   	spin_unlock_bh(&adapter->mac_vlan_list_lock);
>>   
>>   	/* schedule the watchdog task to immediately process the request */
>> -	if (f) {
>> -		mod_delayed_work(adapter->wq, &adapter->watchdog_task, 0);
>> -		return 0;
>> -	}
>> -	return -ENOMEM;
>> +	mod_delayed_work(adapter->wq, &adapter->watchdog_task, 0);
>> +	return 0;
>>   }
>>   
>>   /**
>> -- 
>> 2.38.1
>>
>>

Re: [PATCH net-next 2/3] iavf: fix err handling for MAC replace

[Maciej Fijalkowski]

On Tue, Jun 06, 2023 at 12:14:49PM +0200, Przemek Kitszel wrote:
> On 6/5/23 21:17, Maciej Fijalkowski wrote:
> > On Fri, Jun 02, 2023 at 10:13:01AM -0700, Tony Nguyen wrote:
> > > From: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> > > 
> > > Defer removal of current primary MAC until a replacement is successfully added.
> > > Previous implementation would left filter list with no primary MAC.
> > 
> > and this opens up for what kind of issues? do you mean that
> > iavf_add_filter() could break and existing primary filter has been marked
> > for removal?
> 
> Yes, prior to the patch the flow was:
> 1. mark all MACs non-primary;
> 2. mark current HW MAC for removal;
> 3. try to add new MAC, say it fails, so that's an end with -ENOMEM;
> 4. ::is_primary and ::remove fields for the ::mac_filter_list, alongside
> with ::aq_required are left modified, to be finalized next time
> user/watchdog processes that.
> 
> For me it was enough to treat it as a bug, and for sure a "bad smell".

Thanks,
Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>

> 
> 
> > 
> > > This was found while reading the code.
> > > 
> > > The patch takes advantage of the fact that there can only be a single primary
> > > MAC filter at any time.
> > > 
> > > Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> > > Signed-off-by: Piotr Gardocki <piotrx.gardocki@intel.com>
> > > Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
> > > Tested-by: Rafal Romanowski <rafal.romanowski@intel.com>
> > > Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> > > ---
> > >   drivers/net/ethernet/intel/iavf/iavf_main.c | 42 ++++++++++-----------
> > >   1 file changed, 19 insertions(+), 23 deletions(-)
> > > 
> > > diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
> > > index 420aaca548a0..3a78f86ba4f9 100644
> > > --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
> > > +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
> > > @@ -1010,40 +1010,36 @@ int iavf_replace_primary_mac(struct iavf_adapter *adapter,
> > 
> > from what i'm looking at, iavf_replace_primary_mac() could be scoped only
> > to iavf_main.c and become static func.
> > 
> 
> makes sense, thanks

are you going to followup on this? probably there are some more low
hanging fruits out in iavf such as this one.

> 
> > >   			     const u8 *new_mac)
> > >   {
> > >   	struct iavf_hw *hw = &adapter->hw;
> > > -	struct iavf_mac_filter *f;
> > > +	struct iavf_mac_filter *new_f;
> > > +	struct iavf_mac_filter *old_f;
> > >   	spin_lock_bh(&adapter->mac_vlan_list_lock);
> > > -	list_for_each_entry(f, &adapter->mac_filter_list, list) {
> > > -		f->is_primary = false;
> > > +	new_f = iavf_add_filter(adapter, new_mac);
> > > +	if (!new_f) {
> > > +		spin_unlock_bh(&adapter->mac_vlan_list_lock);
> > > +		return -ENOMEM;
> > >   	}
> > > -	f = iavf_find_filter(adapter, hw->mac.addr);
> > > -	if (f) {
> > > -		f->remove = true;
> > > +	old_f = iavf_find_filter(adapter, hw->mac.addr);
> > > +	if (old_f) {
> > > +		old_f->is_primary = false;
> > > +		old_f->remove = true;
> > >   		adapter->aq_required |= IAVF_FLAG_AQ_DEL_MAC_FILTER;
> > >   	}
> > > -
> > > -	f = iavf_add_filter(adapter, new_mac);
> > > -
> > > -	if (f) {
> > > -		/* Always send the request to add if changing primary MAC
> > > -		 * even if filter is already present on the list
> > > -		 */
> > > -		f->is_primary = true;
> > > -		f->add = true;
> > > -		adapter->aq_required |= IAVF_FLAG_AQ_ADD_MAC_FILTER;
> > > -		ether_addr_copy(hw->mac.addr, new_mac);
> > > -	}
> > > +	/* Always send the request to add if changing primary MAC,
> > > +	 * even if filter is already present on the list
> > > +	 */
> > > +	new_f->is_primary = true;
> > > +	new_f->add = true;
> > > +	adapter->aq_required |= IAVF_FLAG_AQ_ADD_MAC_FILTER;
> > > +	ether_addr_copy(hw->mac.addr, new_mac);
> > >   	spin_unlock_bh(&adapter->mac_vlan_list_lock);
> > >   	/* schedule the watchdog task to immediately process the request */
> > > -	if (f) {
> > > -		mod_delayed_work(adapter->wq, &adapter->watchdog_task, 0);
> > > -		return 0;
> > > -	}
> > > -	return -ENOMEM;
> > > +	mod_delayed_work(adapter->wq, &adapter->watchdog_task, 0);
> > > +	return 0;
> > >   }
> > >   /**
> > > -- 
> > > 2.38.1
> > > 
> > > 
> 

Re: [PATCH net-next 2/3] iavf: fix err handling for MAC replace

[Przemek Kitszel]

On 6/6/23 12:23, Maciej Fijalkowski wrote:
> On Tue, Jun 06, 2023 at 12:14:49PM +0200, Przemek Kitszel wrote:
>> On 6/5/23 21:17, Maciej Fijalkowski wrote:
>>> On Fri, Jun 02, 2023 at 10:13:01AM -0700, Tony Nguyen wrote:
>>>> From: Przemek Kitszel <przemyslaw.kitszel@intel.com>
>>>>
>>>> Defer removal of current primary MAC until a replacement is successfully added.
>>>> Previous implementation would left filter list with no primary MAC.
>>>
>>> and this opens up for what kind of issues? do you mean that
>>> iavf_add_filter() could break and existing primary filter has been marked
>>> for removal?
>>
>> Yes, prior to the patch the flow was:
>> 1. mark all MACs non-primary;
>> 2. mark current HW MAC for removal;
>> 3. try to add new MAC, say it fails, so that's an end with -ENOMEM;
>> 4. ::is_primary and ::remove fields for the ::mac_filter_list, alongside
>> with ::aq_required are left modified, to be finalized next time
>> user/watchdog processes that.
>>
>> For me it was enough to treat it as a bug, and for sure a "bad smell".
> 
> Thanks,
> Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> 
>>
>>
>>>
>>>> This was found while reading the code.
>>>>
>>>> The patch takes advantage of the fact that there can only be a single primary
>>>> MAC filter at any time.
>>>>
>>>> Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
>>>> Signed-off-by: Piotr Gardocki <piotrx.gardocki@intel.com>
>>>> Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
>>>> Tested-by: Rafal Romanowski <rafal.romanowski@intel.com>
>>>> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
>>>> ---
>>>>    drivers/net/ethernet/intel/iavf/iavf_main.c | 42 ++++++++++-----------
>>>>    1 file changed, 19 insertions(+), 23 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
>>>> index 420aaca548a0..3a78f86ba4f9 100644
>>>> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
>>>> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
>>>> @@ -1010,40 +1010,36 @@ int iavf_replace_primary_mac(struct iavf_adapter *adapter,
>>>
>>> from what i'm looking at, iavf_replace_primary_mac() could be scoped only
>>> to iavf_main.c and become static func.
>>>
>>
>> makes sense, thanks
> 
> are you going to followup on this? probably there are some more low
> hanging fruits out in iavf such as this one.

Sure, after some digging it looks like static-whats-possible warrants 
another patch ;) (will send soon as separate series)

> 
>>
>>>>    			     const u8 *new_mac)
>>>>    {
>>>>    	struct iavf_hw *hw = &adapter->hw;
>>>> -	struct iavf_mac_filter *f;
>>>> +	struct iavf_mac_filter *new_f;
>>>> +	struct iavf_mac_filter *old_f;
>>>>    	spin_lock_bh(&adapter->mac_vlan_list_lock);
>>>> -	list_for_each_entry(f, &adapter->mac_filter_list, list) {
>>>> -		f->is_primary = false;
>>>> +	new_f = iavf_add_filter(adapter, new_mac);
>>>> +	if (!new_f) {
>>>> +		spin_unlock_bh(&adapter->mac_vlan_list_lock);
>>>> +		return -ENOMEM;
>>>>    	}
>>>> -	f = iavf_find_filter(adapter, hw->mac.addr);
>>>> -	if (f) {
>>>> -		f->remove = true;
>>>> +	old_f = iavf_find_filter(adapter, hw->mac.addr);
>>>> +	if (old_f) {
>>>> +		old_f->is_primary = false;
>>>> +		old_f->remove = true;
>>>>    		adapter->aq_required |= IAVF_FLAG_AQ_DEL_MAC_FILTER;
>>>>    	}
>>>> -
>>>> -	f = iavf_add_filter(adapter, new_mac);
>>>> -
>>>> -	if (f) {
>>>> -		/* Always send the request to add if changing primary MAC
>>>> -		 * even if filter is already present on the list
>>>> -		 */
>>>> -		f->is_primary = true;
>>>> -		f->add = true;
>>>> -		adapter->aq_required |= IAVF_FLAG_AQ_ADD_MAC_FILTER;
>>>> -		ether_addr_copy(hw->mac.addr, new_mac);
>>>> -	}
>>>> +	/* Always send the request to add if changing primary MAC,
>>>> +	 * even if filter is already present on the list
>>>> +	 */
>>>> +	new_f->is_primary = true;
>>>> +	new_f->add = true;
>>>> +	adapter->aq_required |= IAVF_FLAG_AQ_ADD_MAC_FILTER;
>>>> +	ether_addr_copy(hw->mac.addr, new_mac);
>>>>    	spin_unlock_bh(&adapter->mac_vlan_list_lock);
>>>>    	/* schedule the watchdog task to immediately process the request */
>>>> -	if (f) {
>>>> -		mod_delayed_work(adapter->wq, &adapter->watchdog_task, 0);
>>>> -		return 0;
>>>> -	}
>>>> -	return -ENOMEM;
>>>> +	mod_delayed_work(adapter->wq, &adapter->watchdog_task, 0);
>>>> +	return 0;
>>>>    }
>>>>    /**
>>>> -- 
>>>> 2.38.1
>>>>
>>>>
>>

Re: [PATCH net-next 2/3] iavf: fix err handling for MAC replace

[Przemek Kitszel]

On 6/6/23 12:14, Przemek Kitszel wrote:
> On 6/5/23 21:17, Maciej Fijalkowski wrote:
>> On Fri, Jun 02, 2023 at 10:13:01AM -0700, Tony Nguyen wrote:
>>> From: Przemek Kitszel <przemyslaw.kitszel@intel.com>
>>>
>>> Defer removal of current primary MAC until a replacement is 
>>> successfully added.
>>> Previous implementation would left filter list with no primary MAC.
>>

[...]

Tony, without Piotr's patch for short-cutting new Mac == old Mac case, 
supposedly my patch would not work (we have to either re-test via our 
VAL or just wait for Piotr's next version).

Przemek

RE: [PATCH net-next 2/3] iavf: fix err handling for MAC replace

[Fijalkowski, Maciej]

> 
> On 6/6/23 12:14, Przemek Kitszel wrote:
> > On 6/5/23 21:17, Maciej Fijalkowski wrote:
> >> On Fri, Jun 02, 2023 at 10:13:01AM -0700, Tony Nguyen wrote:
> >>> From: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> >>>
> >>> Defer removal of current primary MAC until a replacement is
> >>> successfully added.
> >>> Previous implementation would left filter list with no primary MAC.
> >>
> 
> [...]
> 
> Tony, without Piotr's patch for short-cutting new Mac == old Mac case,
> supposedly my patch would not work (we have to either re-test via our
> VAL or just wait for Piotr's next version).

Would be good to share a link to patch you refer to + short explanation
why this would not work (I know which patch you had on mind but not
every other reader would do so).

> 
> Przemek

Re: [PATCH net-next 2/3] iavf: fix err handling for MAC replace

[Przemek Kitszel]

On 6/7/23 21:08, Fijalkowski, Maciej wrote:
>>
>> On 6/6/23 12:14, Przemek Kitszel wrote:
>>> On 6/5/23 21:17, Maciej Fijalkowski wrote:
>>>> On Fri, Jun 02, 2023 at 10:13:01AM -0700, Tony Nguyen wrote:
>>>>> From: Przemek Kitszel <przemyslaw.kitszel@intel.com>
>>>>>
>>>>> Defer removal of current primary MAC until a replacement is
>>>>> successfully added.
>>>>> Previous implementation would left filter list with no primary MAC.
>>>>
>>
>> [...]
>>
>> Tony, without Piotr's patch for short-cutting new Mac == old Mac case,
>> supposedly my patch would not work (we have to either re-test via our
>> VAL or just wait for Piotr's next version).
> 
> Would be good to share a link to patch you refer to + short explanation
> why this would not work (I know which patch you had on mind but not
> every other reader would do so).

Right :)

> 
>>
>> Przemek


Final version of Piotr's patch [1] has been merged into net-next,
so @Tony, you could re-apply this patch (Subject line here) of mine to 
your queue.

[1] 
https://lore.kernel.org/netdev/20230614145302.902301-2-piotrx.gardocki@intel.com/

Re: [PATCH net-next 2/3] iavf: fix err handling for MAC replace

[Tony Nguyen]

On 6/16/2023 12:09 AM, Przemek Kitszel wrote:
> Final version of Piotr's patch [1] has been merged into net-next,
> so @Tony, you could re-apply this patch (Subject line here) of mine to 
> your queue.

Please re-send the patch; it'll work better that way.

Thanks,
Tony

> [1] 
> https://lore.kernel.org/netdev/20230614145302.902301-2-piotrx.gardocki@intel.com/

[PATCH net-next 3/3] iavf: remove mask from iavf_irq_enable_queues()

[Tony Nguyen]

From: Ahmed Zaki <ahmed.zaki@intel.com>

Enable more than 32 IRQs by removing the u32 bit mask in
iavf_irq_enable_queues(). There is no need for the mask as there are no
callers that select individual IRQs through the bitmask. Also, if the PF
allocates more than 32 IRQs, this mask will prevent us from using all of
them.

The comment in iavf_register.h is modified to show that the maximum
number allowed for the IRQ index is 63 as per the iAVF standard 1.0 [1].

link: [1] https://www.intel.com/content/dam/www/public/us/en/documents/product-specifications/ethernet-adaptive-virtual-function-hardware-spec.pdf
Signed-off-by: Ahmed Zaki <ahmed.zaki@intel.com>
Tested-by: Rafal Romanowski <rafal.romanowski@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/iavf/iavf.h          |  2 +-
 drivers/net/ethernet/intel/iavf/iavf_main.c     | 15 ++++++---------
 drivers/net/ethernet/intel/iavf/iavf_register.h |  2 +-
 3 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/intel/iavf/iavf.h b/drivers/net/ethernet/intel/iavf/iavf.h
index 9abaff1f2aff..39d0fe76a38f 100644
--- a/drivers/net/ethernet/intel/iavf/iavf.h
+++ b/drivers/net/ethernet/intel/iavf/iavf.h
@@ -525,7 +525,7 @@ void iavf_set_ethtool_ops(struct net_device *netdev);
 void iavf_update_stats(struct iavf_adapter *adapter);
 void iavf_reset_interrupt_capability(struct iavf_adapter *adapter);
 int iavf_init_interrupt_scheme(struct iavf_adapter *adapter);
-void iavf_irq_enable_queues(struct iavf_adapter *adapter, u32 mask);
+void iavf_irq_enable_queues(struct iavf_adapter *adapter);
 void iavf_free_all_tx_resources(struct iavf_adapter *adapter);
 void iavf_free_all_rx_resources(struct iavf_adapter *adapter);
 
diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
index 3a78f86ba4f9..1332633f0ca5 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_main.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
@@ -359,21 +359,18 @@ static void iavf_irq_disable(struct iavf_adapter *adapter)
 }
 
 /**
- * iavf_irq_enable_queues - Enable interrupt for specified queues
+ * iavf_irq_enable_queues - Enable interrupt for all queues
  * @adapter: board private structure
- * @mask: bitmap of queues to enable
  **/
-void iavf_irq_enable_queues(struct iavf_adapter *adapter, u32 mask)
+void iavf_irq_enable_queues(struct iavf_adapter *adapter)
 {
 	struct iavf_hw *hw = &adapter->hw;
 	int i;
 
 	for (i = 1; i < adapter->num_msix_vectors; i++) {
-		if (mask & BIT(i - 1)) {
-			wr32(hw, IAVF_VFINT_DYN_CTLN1(i - 1),
-			     IAVF_VFINT_DYN_CTLN1_INTENA_MASK |
-			     IAVF_VFINT_DYN_CTLN1_ITR_INDX_MASK);
-		}
+		wr32(hw, IAVF_VFINT_DYN_CTLN1(i - 1),
+		     IAVF_VFINT_DYN_CTLN1_INTENA_MASK |
+		     IAVF_VFINT_DYN_CTLN1_ITR_INDX_MASK);
 	}
 }
 
@@ -387,7 +384,7 @@ void iavf_irq_enable(struct iavf_adapter *adapter, bool flush)
 	struct iavf_hw *hw = &adapter->hw;
 
 	iavf_misc_irq_enable(adapter);
-	iavf_irq_enable_queues(adapter, ~0);
+	iavf_irq_enable_queues(adapter);
 
 	if (flush)
 		iavf_flush(hw);
diff --git a/drivers/net/ethernet/intel/iavf/iavf_register.h b/drivers/net/ethernet/intel/iavf/iavf_register.h
index bf793332fc9d..a19e88898a0b 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_register.h
+++ b/drivers/net/ethernet/intel/iavf/iavf_register.h
@@ -40,7 +40,7 @@
 #define IAVF_VFINT_DYN_CTL01_INTENA_MASK IAVF_MASK(0x1, IAVF_VFINT_DYN_CTL01_INTENA_SHIFT)
 #define IAVF_VFINT_DYN_CTL01_ITR_INDX_SHIFT 3
 #define IAVF_VFINT_DYN_CTL01_ITR_INDX_MASK IAVF_MASK(0x3, IAVF_VFINT_DYN_CTL01_ITR_INDX_SHIFT)
-#define IAVF_VFINT_DYN_CTLN1(_INTVF) (0x00003800 + ((_INTVF) * 4)) /* _i=0...15 */ /* Reset: VFR */
+#define IAVF_VFINT_DYN_CTLN1(_INTVF) (0x00003800 + ((_INTVF) * 4)) /* _i=0...63 */ /* Reset: VFR */
 #define IAVF_VFINT_DYN_CTLN1_INTENA_SHIFT 0
 #define IAVF_VFINT_DYN_CTLN1_INTENA_MASK IAVF_MASK(0x1, IAVF_VFINT_DYN_CTLN1_INTENA_SHIFT)
 #define IAVF_VFINT_DYN_CTLN1_SWINT_TRIG_SHIFT 2
-- 
2.38.1

Re: [PATCH net-next 3/3] iavf: remove mask from iavf_irq_enable_queues()

[Simon Horman]

On Fri, Jun 02, 2023 at 10:13:02AM -0700, Tony Nguyen wrote:
> From: Ahmed Zaki <ahmed.zaki@intel.com>
> 
> Enable more than 32 IRQs by removing the u32 bit mask in
> iavf_irq_enable_queues(). There is no need for the mask as there are no
> callers that select individual IRQs through the bitmask. Also, if the PF
> allocates more than 32 IRQs, this mask will prevent us from using all of
> them.
> 
> The comment in iavf_register.h is modified to show that the maximum
> number allowed for the IRQ index is 63 as per the iAVF standard 1.0 [1].
> 
> link: [1] https://www.intel.com/content/dam/www/public/us/en/documents/product-specifications/ethernet-adaptive-virtual-function-hardware-spec.pdf
> Signed-off-by: Ahmed Zaki <ahmed.zaki@intel.com>
> Tested-by: Rafal Romanowski <rafal.romanowski@intel.com>
> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>

Reviewed-by: Simon Horman <simon.horman@corigine.com>

Re: [PATCH net-next 3/3] iavf: remove mask from iavf_irq_enable_queues()

[Maciej Fijalkowski]

On Fri, Jun 02, 2023 at 10:13:02AM -0700, Tony Nguyen wrote:
> From: Ahmed Zaki <ahmed.zaki@intel.com>
> 
> Enable more than 32 IRQs by removing the u32 bit mask in
> iavf_irq_enable_queues(). There is no need for the mask as there are no
> callers that select individual IRQs through the bitmask. Also, if the PF
> allocates more than 32 IRQs, this mask will prevent us from using all of
> them.
> 
> The comment in iavf_register.h is modified to show that the maximum
> number allowed for the IRQ index is 63 as per the iAVF standard 1.0 [1].

please use imperative mood:
"modify the comment in..."

besides, it sounds to me like a bug, we were not following the spec, no?

> 
> link: [1] https://www.intel.com/content/dam/www/public/us/en/documents/product-specifications/ethernet-adaptive-virtual-function-hardware-spec.pdf
> Signed-off-by: Ahmed Zaki <ahmed.zaki@intel.com>
> Tested-by: Rafal Romanowski <rafal.romanowski@intel.com>
> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>

Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>

> ---
>  drivers/net/ethernet/intel/iavf/iavf.h          |  2 +-
>  drivers/net/ethernet/intel/iavf/iavf_main.c     | 15 ++++++---------
>  drivers/net/ethernet/intel/iavf/iavf_register.h |  2 +-
>  3 files changed, 8 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/iavf/iavf.h b/drivers/net/ethernet/intel/iavf/iavf.h
> index 9abaff1f2aff..39d0fe76a38f 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf.h
> +++ b/drivers/net/ethernet/intel/iavf/iavf.h
> @@ -525,7 +525,7 @@ void iavf_set_ethtool_ops(struct net_device *netdev);
>  void iavf_update_stats(struct iavf_adapter *adapter);
>  void iavf_reset_interrupt_capability(struct iavf_adapter *adapter);
>  int iavf_init_interrupt_scheme(struct iavf_adapter *adapter);
> -void iavf_irq_enable_queues(struct iavf_adapter *adapter, u32 mask);
> +void iavf_irq_enable_queues(struct iavf_adapter *adapter);
>  void iavf_free_all_tx_resources(struct iavf_adapter *adapter);
>  void iavf_free_all_rx_resources(struct iavf_adapter *adapter);
>  
> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
> index 3a78f86ba4f9..1332633f0ca5 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
> @@ -359,21 +359,18 @@ static void iavf_irq_disable(struct iavf_adapter *adapter)
>  }
>  
>  /**
> - * iavf_irq_enable_queues - Enable interrupt for specified queues
> + * iavf_irq_enable_queues - Enable interrupt for all queues
>   * @adapter: board private structure
> - * @mask: bitmap of queues to enable
>   **/
> -void iavf_irq_enable_queues(struct iavf_adapter *adapter, u32 mask)
> +void iavf_irq_enable_queues(struct iavf_adapter *adapter)
>  {
>  	struct iavf_hw *hw = &adapter->hw;
>  	int i;
>  
>  	for (i = 1; i < adapter->num_msix_vectors; i++) {
> -		if (mask & BIT(i - 1)) {
> -			wr32(hw, IAVF_VFINT_DYN_CTLN1(i - 1),
> -			     IAVF_VFINT_DYN_CTLN1_INTENA_MASK |
> -			     IAVF_VFINT_DYN_CTLN1_ITR_INDX_MASK);
> -		}
> +		wr32(hw, IAVF_VFINT_DYN_CTLN1(i - 1),
> +		     IAVF_VFINT_DYN_CTLN1_INTENA_MASK |
> +		     IAVF_VFINT_DYN_CTLN1_ITR_INDX_MASK);
>  	}
>  }
>  
> @@ -387,7 +384,7 @@ void iavf_irq_enable(struct iavf_adapter *adapter, bool flush)
>  	struct iavf_hw *hw = &adapter->hw;
>  
>  	iavf_misc_irq_enable(adapter);
> -	iavf_irq_enable_queues(adapter, ~0);
> +	iavf_irq_enable_queues(adapter);
>  
>  	if (flush)
>  		iavf_flush(hw);
> diff --git a/drivers/net/ethernet/intel/iavf/iavf_register.h b/drivers/net/ethernet/intel/iavf/iavf_register.h
> index bf793332fc9d..a19e88898a0b 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_register.h
> +++ b/drivers/net/ethernet/intel/iavf/iavf_register.h
> @@ -40,7 +40,7 @@
>  #define IAVF_VFINT_DYN_CTL01_INTENA_MASK IAVF_MASK(0x1, IAVF_VFINT_DYN_CTL01_INTENA_SHIFT)
>  #define IAVF_VFINT_DYN_CTL01_ITR_INDX_SHIFT 3
>  #define IAVF_VFINT_DYN_CTL01_ITR_INDX_MASK IAVF_MASK(0x3, IAVF_VFINT_DYN_CTL01_ITR_INDX_SHIFT)
> -#define IAVF_VFINT_DYN_CTLN1(_INTVF) (0x00003800 + ((_INTVF) * 4)) /* _i=0...15 */ /* Reset: VFR */

so this was wrong even before as not indicating 31 as max?

> +#define IAVF_VFINT_DYN_CTLN1(_INTVF) (0x00003800 + ((_INTVF) * 4)) /* _i=0...63 */ /* Reset: VFR */
>  #define IAVF_VFINT_DYN_CTLN1_INTENA_SHIFT 0
>  #define IAVF_VFINT_DYN_CTLN1_INTENA_MASK IAVF_MASK(0x1, IAVF_VFINT_DYN_CTLN1_INTENA_SHIFT)
>  #define IAVF_VFINT_DYN_CTLN1_SWINT_TRIG_SHIFT 2
> -- 
> 2.38.1
> 
> 

Re: [PATCH net-next 3/3] iavf: remove mask from iavf_irq_enable_queues()

[Ahmed Zaki]

On 2023-06-05 13:25, Maciej Fijalkowski wrote:
> On Fri, Jun 02, 2023 at 10:13:02AM -0700, Tony Nguyen wrote:
>> From: Ahmed Zaki <ahmed.zaki@intel.com>
>>
>> Enable more than 32 IRQs by removing the u32 bit mask in
>> iavf_irq_enable_queues(). There is no need for the mask as there are no
>> callers that select individual IRQs through the bitmask. Also, if the PF
>> allocates more than 32 IRQs, this mask will prevent us from using all of
>> them.
>>
>> The comment in iavf_register.h is modified to show that the maximum
>> number allowed for the IRQ index is 63 as per the iAVF standard 1.0 [1].
> please use imperative mood:
> "modify the comment in..."
>
> besides, it sounds to me like a bug, we were not following the spec, no?

yes, but all PF's were allocating  <= 16 IRQs, so it was not causing any 
issues.


>
>> link: [1] https://www.intel.com/content/dam/www/public/us/en/documents/product-specifications/ethernet-adaptive-virtual-function-hardware-spec.pdf
>> Signed-off-by: Ahmed Zaki <ahmed.zaki@intel.com>
>> Tested-by: Rafal Romanowski <rafal.romanowski@intel.com>
>> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
>
>> ---
>>   drivers/net/ethernet/intel/iavf/iavf.h          |  2 +-
>>   drivers/net/ethernet/intel/iavf/iavf_main.c     | 15 ++++++---------
>>   drivers/net/ethernet/intel/iavf/iavf_register.h |  2 +-
>>   3 files changed, 8 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/iavf/iavf.h b/drivers/net/ethernet/intel/iavf/iavf.h
>> index 9abaff1f2aff..39d0fe76a38f 100644
>> --- a/drivers/net/ethernet/intel/iavf/iavf.h
>> +++ b/drivers/net/ethernet/intel/iavf/iavf.h
>> @@ -525,7 +525,7 @@ void iavf_set_ethtool_ops(struct net_device *netdev);
>>   void iavf_update_stats(struct iavf_adapter *adapter);
>>   void iavf_reset_interrupt_capability(struct iavf_adapter *adapter);
>>   int iavf_init_interrupt_scheme(struct iavf_adapter *adapter);
>> -void iavf_irq_enable_queues(struct iavf_adapter *adapter, u32 mask);
>> +void iavf_irq_enable_queues(struct iavf_adapter *adapter);
>>   void iavf_free_all_tx_resources(struct iavf_adapter *adapter);
>>   void iavf_free_all_rx_resources(struct iavf_adapter *adapter);
>>   
>> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
>> index 3a78f86ba4f9..1332633f0ca5 100644
>> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
>> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
>> @@ -359,21 +359,18 @@ static void iavf_irq_disable(struct iavf_adapter *adapter)
>>   }
>>   
>>   /**
>> - * iavf_irq_enable_queues - Enable interrupt for specified queues
>> + * iavf_irq_enable_queues - Enable interrupt for all queues
>>    * @adapter: board private structure
>> - * @mask: bitmap of queues to enable
>>    **/
>> -void iavf_irq_enable_queues(struct iavf_adapter *adapter, u32 mask)
>> +void iavf_irq_enable_queues(struct iavf_adapter *adapter)
>>   {
>>   	struct iavf_hw *hw = &adapter->hw;
>>   	int i;
>>   
>>   	for (i = 1; i < adapter->num_msix_vectors; i++) {
>> -		if (mask & BIT(i - 1)) {
>> -			wr32(hw, IAVF_VFINT_DYN_CTLN1(i - 1),
>> -			     IAVF_VFINT_DYN_CTLN1_INTENA_MASK |
>> -			     IAVF_VFINT_DYN_CTLN1_ITR_INDX_MASK);
>> -		}
>> +		wr32(hw, IAVF_VFINT_DYN_CTLN1(i - 1),
>> +		     IAVF_VFINT_DYN_CTLN1_INTENA_MASK |
>> +		     IAVF_VFINT_DYN_CTLN1_ITR_INDX_MASK);
>>   	}
>>   }
>>   
>> @@ -387,7 +384,7 @@ void iavf_irq_enable(struct iavf_adapter *adapter, bool flush)
>>   	struct iavf_hw *hw = &adapter->hw;
>>   
>>   	iavf_misc_irq_enable(adapter);
>> -	iavf_irq_enable_queues(adapter, ~0);
>> +	iavf_irq_enable_queues(adapter);
>>   
>>   	if (flush)
>>   		iavf_flush(hw);
>> diff --git a/drivers/net/ethernet/intel/iavf/iavf_register.h b/drivers/net/ethernet/intel/iavf/iavf_register.h
>> index bf793332fc9d..a19e88898a0b 100644
>> --- a/drivers/net/ethernet/intel/iavf/iavf_register.h
>> +++ b/drivers/net/ethernet/intel/iavf/iavf_register.h
>> @@ -40,7 +40,7 @@
>>   #define IAVF_VFINT_DYN_CTL01_INTENA_MASK IAVF_MASK(0x1, IAVF_VFINT_DYN_CTL01_INTENA_SHIFT)
>>   #define IAVF_VFINT_DYN_CTL01_ITR_INDX_SHIFT 3
>>   #define IAVF_VFINT_DYN_CTL01_ITR_INDX_MASK IAVF_MASK(0x3, IAVF_VFINT_DYN_CTL01_ITR_INDX_SHIFT)
>> -#define IAVF_VFINT_DYN_CTLN1(_INTVF) (0x00003800 + ((_INTVF) * 4)) /* _i=0...15 */ /* Reset: VFR */
> so this was wrong even before as not indicating 31 as max?

Correct, but again no issues.

Given that, should I re-send to net ?

Re: [PATCH net-next 3/3] iavf: remove mask from iavf_irq_enable_queues()

[Maciej Fijalkowski]

On Mon, Jun 05, 2023 at 01:56:48PM -0600, Ahmed Zaki wrote:
> 
> On 2023-06-05 13:25, Maciej Fijalkowski wrote:
> > On Fri, Jun 02, 2023 at 10:13:02AM -0700, Tony Nguyen wrote:
> > > From: Ahmed Zaki <ahmed.zaki@intel.com>
> > > 
> > > Enable more than 32 IRQs by removing the u32 bit mask in
> > > iavf_irq_enable_queues(). There is no need for the mask as there are no
> > > callers that select individual IRQs through the bitmask. Also, if the PF
> > > allocates more than 32 IRQs, this mask will prevent us from using all of
> > > them.
> > > 
> > > The comment in iavf_register.h is modified to show that the maximum
> > > number allowed for the IRQ index is 63 as per the iAVF standard 1.0 [1].
> > please use imperative mood:
> > "modify the comment in..."
> > 
> > besides, it sounds to me like a bug, we were not following the spec, no?
> 
> yes, but all PF's were allocating  <= 16 IRQs, so it was not causing any
> issues.
> 
> 
> > 
> > > link: [1] https://www.intel.com/content/dam/www/public/us/en/documents/product-specifications/ethernet-adaptive-virtual-function-hardware-spec.pdf
> > > Signed-off-by: Ahmed Zaki <ahmed.zaki@intel.com>
> > > Tested-by: Rafal Romanowski <rafal.romanowski@intel.com>
> > > Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> > Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> > 
> > > ---
> > >   drivers/net/ethernet/intel/iavf/iavf.h          |  2 +-
> > >   drivers/net/ethernet/intel/iavf/iavf_main.c     | 15 ++++++---------
> > >   drivers/net/ethernet/intel/iavf/iavf_register.h |  2 +-
> > >   3 files changed, 8 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/drivers/net/ethernet/intel/iavf/iavf.h b/drivers/net/ethernet/intel/iavf/iavf.h
> > > index 9abaff1f2aff..39d0fe76a38f 100644
> > > --- a/drivers/net/ethernet/intel/iavf/iavf.h
> > > +++ b/drivers/net/ethernet/intel/iavf/iavf.h
> > > @@ -525,7 +525,7 @@ void iavf_set_ethtool_ops(struct net_device *netdev);
> > >   void iavf_update_stats(struct iavf_adapter *adapter);
> > >   void iavf_reset_interrupt_capability(struct iavf_adapter *adapter);
> > >   int iavf_init_interrupt_scheme(struct iavf_adapter *adapter);
> > > -void iavf_irq_enable_queues(struct iavf_adapter *adapter, u32 mask);
> > > +void iavf_irq_enable_queues(struct iavf_adapter *adapter);
> > >   void iavf_free_all_tx_resources(struct iavf_adapter *adapter);
> > >   void iavf_free_all_rx_resources(struct iavf_adapter *adapter);
> > > diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
> > > index 3a78f86ba4f9..1332633f0ca5 100644
> > > --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
> > > +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
> > > @@ -359,21 +359,18 @@ static void iavf_irq_disable(struct iavf_adapter *adapter)
> > >   }
> > >   /**
> > > - * iavf_irq_enable_queues - Enable interrupt for specified queues
> > > + * iavf_irq_enable_queues - Enable interrupt for all queues
> > >    * @adapter: board private structure
> > > - * @mask: bitmap of queues to enable
> > >    **/
> > > -void iavf_irq_enable_queues(struct iavf_adapter *adapter, u32 mask)
> > > +void iavf_irq_enable_queues(struct iavf_adapter *adapter)
> > >   {
> > >   	struct iavf_hw *hw = &adapter->hw;
> > >   	int i;
> > >   	for (i = 1; i < adapter->num_msix_vectors; i++) {
> > > -		if (mask & BIT(i - 1)) {
> > > -			wr32(hw, IAVF_VFINT_DYN_CTLN1(i - 1),
> > > -			     IAVF_VFINT_DYN_CTLN1_INTENA_MASK |
> > > -			     IAVF_VFINT_DYN_CTLN1_ITR_INDX_MASK);
> > > -		}
> > > +		wr32(hw, IAVF_VFINT_DYN_CTLN1(i - 1),
> > > +		     IAVF_VFINT_DYN_CTLN1_INTENA_MASK |
> > > +		     IAVF_VFINT_DYN_CTLN1_ITR_INDX_MASK);
> > >   	}
> > >   }
> > > @@ -387,7 +384,7 @@ void iavf_irq_enable(struct iavf_adapter *adapter, bool flush)
> > >   	struct iavf_hw *hw = &adapter->hw;
> > >   	iavf_misc_irq_enable(adapter);
> > > -	iavf_irq_enable_queues(adapter, ~0);
> > > +	iavf_irq_enable_queues(adapter);
> > >   	if (flush)
> > >   		iavf_flush(hw);
> > > diff --git a/drivers/net/ethernet/intel/iavf/iavf_register.h b/drivers/net/ethernet/intel/iavf/iavf_register.h
> > > index bf793332fc9d..a19e88898a0b 100644
> > > --- a/drivers/net/ethernet/intel/iavf/iavf_register.h
> > > +++ b/drivers/net/ethernet/intel/iavf/iavf_register.h
> > > @@ -40,7 +40,7 @@
> > >   #define IAVF_VFINT_DYN_CTL01_INTENA_MASK IAVF_MASK(0x1, IAVF_VFINT_DYN_CTL01_INTENA_SHIFT)
> > >   #define IAVF_VFINT_DYN_CTL01_ITR_INDX_SHIFT 3
> > >   #define IAVF_VFINT_DYN_CTL01_ITR_INDX_MASK IAVF_MASK(0x3, IAVF_VFINT_DYN_CTL01_ITR_INDX_SHIFT)
> > > -#define IAVF_VFINT_DYN_CTLN1(_INTVF) (0x00003800 + ((_INTVF) * 4)) /* _i=0...15 */ /* Reset: VFR */
> > so this was wrong even before as not indicating 31 as max?
> 
> Correct, but again no issues.
> 
> Given that, should I re-send to net ?

probably with older kernels PFs would still be allocating <= 16 irqs,
right? not sure if one could take a PF and hack it to request for more
than 32 irqs and then hit the wall with the mask you're removing.

> 
> 

Re: [PATCH net-next 3/3] iavf: remove mask from iavf_irq_enable_queues()

[Ahmed Zaki]

On 2023-06-06 04:26, Maciej Fijalkowski wrote:
> On Mon, Jun 05, 2023 at 01:56:48PM -0600, Ahmed Zaki wrote:
>> On 2023-06-05 13:25, Maciej Fijalkowski wrote:
>>> On Fri, Jun 02, 2023 at 10:13:02AM -0700, Tony Nguyen wrote:
>>>> From: Ahmed Zaki <ahmed.zaki@intel.com>
>>>>
>>>> Enable more than 32 IRQs by removing the u32 bit mask in
>>>> iavf_irq_enable_queues(). There is no need for the mask as there are no
>>>> callers that select individual IRQs through the bitmask. Also, if the PF
>>>> allocates more than 32 IRQs, this mask will prevent us from using all of
>>>> them.
>>>>
>>>> The comment in iavf_register.h is modified to show that the maximum
>>>> number allowed for the IRQ index is 63 as per the iAVF standard 1.0 [1].
>>> please use imperative mood:
>>> "modify the comment in..."
>>>
>>> besides, it sounds to me like a bug, we were not following the spec, no?
>> yes, but all PF's were allocating  <= 16 IRQs, so it was not causing any
>> issues.
>>
>>
>>>> link: [1] https://www.intel.com/content/dam/www/public/us/en/documents/product-specifications/ethernet-adaptive-virtual-function-hardware-spec.pdf
>>>> Signed-off-by: Ahmed Zaki <ahmed.zaki@intel.com>
>>>> Tested-by: Rafal Romanowski <rafal.romanowski@intel.com>
>>>> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
>>> Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
>>>
>>>> ---
>>>>    drivers/net/ethernet/intel/iavf/iavf.h          |  2 +-
>>>>    drivers/net/ethernet/intel/iavf/iavf_main.c     | 15 ++++++---------
>>>>    drivers/net/ethernet/intel/iavf/iavf_register.h |  2 +-
>>>>    3 files changed, 8 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/intel/iavf/iavf.h b/drivers/net/ethernet/intel/iavf/iavf.h
>>>> index 9abaff1f2aff..39d0fe76a38f 100644
>>>> --- a/drivers/net/ethernet/intel/iavf/iavf.h
>>>> +++ b/drivers/net/ethernet/intel/iavf/iavf.h
>>>> @@ -525,7 +525,7 @@ void iavf_set_ethtool_ops(struct net_device *netdev);
>>>>    void iavf_update_stats(struct iavf_adapter *adapter);
>>>>    void iavf_reset_interrupt_capability(struct iavf_adapter *adapter);
>>>>    int iavf_init_interrupt_scheme(struct iavf_adapter *adapter);
>>>> -void iavf_irq_enable_queues(struct iavf_adapter *adapter, u32 mask);
>>>> +void iavf_irq_enable_queues(struct iavf_adapter *adapter);
>>>>    void iavf_free_all_tx_resources(struct iavf_adapter *adapter);
>>>>    void iavf_free_all_rx_resources(struct iavf_adapter *adapter);
>>>> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
>>>> index 3a78f86ba4f9..1332633f0ca5 100644
>>>> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
>>>> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
>>>> @@ -359,21 +359,18 @@ static void iavf_irq_disable(struct iavf_adapter *adapter)
>>>>    }
>>>>    /**
>>>> - * iavf_irq_enable_queues - Enable interrupt for specified queues
>>>> + * iavf_irq_enable_queues - Enable interrupt for all queues
>>>>     * @adapter: board private structure
>>>> - * @mask: bitmap of queues to enable
>>>>     **/
>>>> -void iavf_irq_enable_queues(struct iavf_adapter *adapter, u32 mask)
>>>> +void iavf_irq_enable_queues(struct iavf_adapter *adapter)
>>>>    {
>>>>    	struct iavf_hw *hw = &adapter->hw;
>>>>    	int i;
>>>>    	for (i = 1; i < adapter->num_msix_vectors; i++) {
>>>> -		if (mask & BIT(i - 1)) {
>>>> -			wr32(hw, IAVF_VFINT_DYN_CTLN1(i - 1),
>>>> -			     IAVF_VFINT_DYN_CTLN1_INTENA_MASK |
>>>> -			     IAVF_VFINT_DYN_CTLN1_ITR_INDX_MASK);
>>>> -		}
>>>> +		wr32(hw, IAVF_VFINT_DYN_CTLN1(i - 1),
>>>> +		     IAVF_VFINT_DYN_CTLN1_INTENA_MASK |
>>>> +		     IAVF_VFINT_DYN_CTLN1_ITR_INDX_MASK);
>>>>    	}
>>>>    }
>>>> @@ -387,7 +384,7 @@ void iavf_irq_enable(struct iavf_adapter *adapter, bool flush)
>>>>    	struct iavf_hw *hw = &adapter->hw;
>>>>    	iavf_misc_irq_enable(adapter);
>>>> -	iavf_irq_enable_queues(adapter, ~0);
>>>> +	iavf_irq_enable_queues(adapter);
>>>>    	if (flush)
>>>>    		iavf_flush(hw);
>>>> diff --git a/drivers/net/ethernet/intel/iavf/iavf_register.h b/drivers/net/ethernet/intel/iavf/iavf_register.h
>>>> index bf793332fc9d..a19e88898a0b 100644
>>>> --- a/drivers/net/ethernet/intel/iavf/iavf_register.h
>>>> +++ b/drivers/net/ethernet/intel/iavf/iavf_register.h
>>>> @@ -40,7 +40,7 @@
>>>>    #define IAVF_VFINT_DYN_CTL01_INTENA_MASK IAVF_MASK(0x1, IAVF_VFINT_DYN_CTL01_INTENA_SHIFT)
>>>>    #define IAVF_VFINT_DYN_CTL01_ITR_INDX_SHIFT 3
>>>>    #define IAVF_VFINT_DYN_CTL01_ITR_INDX_MASK IAVF_MASK(0x3, IAVF_VFINT_DYN_CTL01_ITR_INDX_SHIFT)
>>>> -#define IAVF_VFINT_DYN_CTLN1(_INTVF) (0x00003800 + ((_INTVF) * 4)) /* _i=0...15 */ /* Reset: VFR */
>>> so this was wrong even before as not indicating 31 as max?
>> Correct, but again no issues.
>>
>> Given that, should I re-send to net ?
> probably with older kernels PFs would still be allocating <= 16 irqs,
> right? not sure if one could take a PF and hack it to request for more
> than 32 irqs and then hit the wall with the mask you're removing.
>
Unlikely since the VF currently never requests more than 16 queues, so 
any IRQs > 16 are useless.

The "fix" is needed for another patch that will enable up to 256 queues 
though.

RE: [PATCH net-next 3/3] iavf: remove mask from iavf_irq_enable_queues()

[Romanowski, Rafal]

> -----Original Message-----
> From: Zaki, Ahmed <ahmed.zaki@intel.com>
> Sent: wtorek, 6 czerwca 2023 17:23
> To: Fijalkowski, Maciej <maciej.fijalkowski@intel.com>
> Cc: Nguyen, Anthony L <anthony.l.nguyen@intel.com>;
> davem@davemloft.net; kuba@kernel.org; pabeni@redhat.com;
> edumazet@google.com; netdev@vger.kernel.org; Romanowski, Rafal
> <rafal.romanowski@intel.com>
> Subject: Re: [PATCH net-next 3/3] iavf: remove mask from
> iavf_irq_enable_queues()
> 
> 
> On 2023-06-06 04:26, Maciej Fijalkowski wrote:
> > On Mon, Jun 05, 2023 at 01:56:48PM -0600, Ahmed Zaki wrote:
> >> On 2023-06-05 13:25, Maciej Fijalkowski wrote:
> >>> On Fri, Jun 02, 2023 at 10:13:02AM -0700, Tony Nguyen wrote:
> >>>> From: Ahmed Zaki <ahmed.zaki@intel.com>
> >>>>
> >>>> Enable more than 32 IRQs by removing the u32 bit mask in
> >>>> iavf_irq_enable_queues(). There is no need for the mask as there
> >>>> are no callers that select individual IRQs through the bitmask.
> >>>> Also, if the PF allocates more than 32 IRQs, this mask will prevent
> >>>> us from using all of them.
> >>>>
> >>>> The comment in iavf_register.h is modified to show that the maximum
> >>>> number allowed for the IRQ index is 63 as per the iAVF standard 1.0 [1].
> >>> please use imperative mood:
> >>> "modify the comment in..."
> >>>
> >>> besides, it sounds to me like a bug, we were not following the spec, no?
> >> yes, but all PF's were allocating  <= 16 IRQs, so it was not causing
> >> any issues.
> >>
> >>
> >>>> link: [1]
> >>>>
> https://www.intel.com/content/dam/www/public/us/en/documents/produ
> c
> >>>> t-specifications/ethernet-adaptive-virtual-function-hardware-spec.p
> >>>> df
> >>>> Signed-off-by: Ahmed Zaki <ahmed.zaki@intel.com>
> >>>> Tested-by: Rafal Romanowski <rafal.romanowski@intel.com>
> >>>> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> >>> Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> >>>
> >>>> ---
> >>>>    drivers/net/ethernet/intel/iavf/iavf.h          |  2 +-
> >>>>    drivers/net/ethernet/intel/iavf/iavf_main.c     | 15 ++++++---------
> >>>>    drivers/net/ethernet/intel/iavf/iavf_register.h |  2 +-
> >>>>    3 files changed, 8 insertions(+), 11 deletions(-)
> >>>>
> >>>> diff --git a/drivers/net/ethernet/intel/iavf/iavf.h
> >>>> b/drivers/net/ethernet/intel/iavf/iavf.h
> >>>> index 9abaff1f2aff..39d0fe76a38f 100644
> >>>> --- a/drivers/net/ethernet/intel/iavf/iavf.h
> >>>> +++ b/drivers/net/ethernet/intel/iavf/iavf.h
> >>>> @@ -525,7 +525,7 @@ void iavf_set_ethtool_ops(struct net_device
> *netdev);
> >>>>    void iavf_update_stats(struct iavf_adapter *adapter);
> >>>>    void iavf_reset_interrupt_capability(struct iavf_adapter *adapter);
> >>>>    int iavf_init_interrupt_scheme(struct iavf_adapter *adapter);
> >>>> -void iavf_irq_enable_queues(struct iavf_adapter *adapter, u32
> >>>> mask);
> >>>> +void iavf_irq_enable_queues(struct iavf_adapter *adapter);
> >>>>    void iavf_free_all_tx_resources(struct iavf_adapter *adapter);
> >>>>    void iavf_free_all_rx_resources(struct iavf_adapter *adapter);
> >>>> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c
> >>>> b/drivers/net/ethernet/intel/iavf/iavf_main.c
> >>>> index 3a78f86ba4f9..1332633f0ca5 100644
> >>>> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
> >>>> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
> >>>> @@ -359,21 +359,18 @@ static void iavf_irq_disable(struct
> iavf_adapter *adapter)
> >>>>    }
> >>>>    /**
> >>>> - * iavf_irq_enable_queues - Enable interrupt for specified queues
> >>>> + * iavf_irq_enable_queues - Enable interrupt for all queues
> >>>>     * @adapter: board private structure
> >>>> - * @mask: bitmap of queues to enable
> >>>>     **/
> >>>> -void iavf_irq_enable_queues(struct iavf_adapter *adapter, u32
> >>>> mask)
> >>>> +void iavf_irq_enable_queues(struct iavf_adapter *adapter)
> >>>>    {
> >>>>    	struct iavf_hw *hw = &adapter->hw;
> >>>>    	int i;
> >>>>    	for (i = 1; i < adapter->num_msix_vectors; i++) {
> >>>> -		if (mask & BIT(i - 1)) {
> >>>> -			wr32(hw, IAVF_VFINT_DYN_CTLN1(i - 1),
> >>>> -			     IAVF_VFINT_DYN_CTLN1_INTENA_MASK |
> >>>> -			     IAVF_VFINT_DYN_CTLN1_ITR_INDX_MASK);
> >>>> -		}
> >>>> +		wr32(hw, IAVF_VFINT_DYN_CTLN1(i - 1),
> >>>> +		     IAVF_VFINT_DYN_CTLN1_INTENA_MASK |
> >>>> +		     IAVF_VFINT_DYN_CTLN1_ITR_INDX_MASK);
> >>>>    	}
> >>>>    }
> >>>> @@ -387,7 +384,7 @@ void iavf_irq_enable(struct iavf_adapter
> *adapter, bool flush)
> >>>>    	struct iavf_hw *hw = &adapter->hw;
> >>>>    	iavf_misc_irq_enable(adapter);
> >>>> -	iavf_irq_enable_queues(adapter, ~0);
> >>>> +	iavf_irq_enable_queues(adapter);
> >>>>    	if (flush)
> >>>>    		iavf_flush(hw);
> >>>> diff --git a/drivers/net/ethernet/intel/iavf/iavf_register.h
> >>>> b/drivers/net/ethernet/intel/iavf/iavf_register.h
> >>>> index bf793332fc9d..a19e88898a0b 100644
> >>>> --- a/drivers/net/ethernet/intel/iavf/iavf_register.h
> >>>> +++ b/drivers/net/ethernet/intel/iavf/iavf_register.h
> >>>> @@ -40,7 +40,7 @@
> >>>>    #define IAVF_VFINT_DYN_CTL01_INTENA_MASK IAVF_MASK(0x1,
> IAVF_VFINT_DYN_CTL01_INTENA_SHIFT)
> >>>>    #define IAVF_VFINT_DYN_CTL01_ITR_INDX_SHIFT 3
> >>>>    #define IAVF_VFINT_DYN_CTL01_ITR_INDX_MASK IAVF_MASK(0x3,
> >>>> IAVF_VFINT_DYN_CTL01_ITR_INDX_SHIFT)
> >>>> -#define IAVF_VFINT_DYN_CTLN1(_INTVF) (0x00003800 + ((_INTVF) *
> 4))
> >>>> /* _i=0...15 */ /* Reset: VFR */
> >>> so this was wrong even before as not indicating 31 as max?
> >> Correct, but again no issues.
> >>
> >> Given that, should I re-send to net ?
> > probably with older kernels PFs would still be allocating <= 16 irqs,
> > right? not sure if one could take a PF and hack it to request for more
> > than 32 irqs and then hit the wall with the mask you're removing.
> >
> Unlikely since the VF currently never requests more than 16 queues, so any
> IRQs > 16 are useless.
> 
> The "fix" is needed for another patch that will enable up to 256 queues
> though.

We don't see this patch in https://patchwork.ozlabs.org/bundle/anguy11/Virtualization/


Best Regards,
Rafal Romanowski
NCNG SW