[PATCH] ice: Add get/set hw address for VF representor ports

[PATCH] ice: Add get/set hw address for VF representor ports

[karthiksundaravel]

Changing the mac address of the VF representor ports are not
available via devlink. Add the function handlers to set and get
the HW address for the VF representor ports.

Signed-off-by: karthiksundaravel <ksundara@redhat.com>
---
 drivers/net/ethernet/intel/ice/ice_devlink.c | 134 ++++++++++++++++++-
 1 file changed, 132 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.c b/drivers/net/ethernet/intel/ice/ice_devlink.c
index 80dc5445b50d..56d81836c469 100644
--- a/drivers/net/ethernet/intel/ice/ice_devlink.c
+++ b/drivers/net/ethernet/intel/ice/ice_devlink.c
@@ -9,6 +9,8 @@
 #include "ice_eswitch.h"
 #include "ice_fw_update.h"
 #include "ice_dcb_lib.h"
+#include "ice_fltr.h"
+#include "ice_tc_lib.h"
 
 static int ice_active_port_option = -1;
 
@@ -1576,6 +1578,134 @@ void ice_devlink_destroy_pf_port(struct ice_pf *pf)
 	devlink_port_unregister(&pf->devlink_port);
 }
 
+/**
+ * ice_devlink_port_get_vf_mac_address - .port_fn_hw_addr_get devlink handler
+ * @port: devlink port structure
+ * @hw_addr: Mac address of the port
+ * @hw_addr_len: length of mac address
+ * @extack: extended netdev ack structure
+ *
+ * Callback for the devlink .port_fn_hw_addr_get operation
+ * Return: zero on success or an error code on failure.
+ */
+
+static int ice_devlink_port_get_vf_mac_address(struct devlink_port *port,
+					       u8 *hw_addr, int *hw_addr_len,
+					       struct netlink_ext_ack *extack)
+{
+	struct net_device *netdev = port->type_eth.netdev;
+
+	if (!netdev || !netdev->dev_addr)
+		return -EADDRNOTAVAIL;
+
+	ether_addr_copy(hw_addr, netdev->dev_addr);
+	*hw_addr_len = ETH_ALEN;
+	return 0;
+}
+
+/**
+ * ice_devlink_port_set_vf_mac_address - .port_fn_hw_addr_set devlink handler
+ * @port: devlink port structure
+ * @hw_addr: Mac address of the port
+ * @hw_addr_len: length of mac address
+ * @extack: extended netdev ack structure
+ *
+ * Callback for the devlink .port_fn_hw_addr_set operation
+ * Return: zero on success or an error code on failure.
+ */
+static int ice_devlink_port_set_vf_mac_address(struct devlink_port *port,
+					       const u8 *hw_addr,
+					       int hw_addr_len,
+					       struct netlink_ext_ack *extack)
+{
+	struct devlink *devlink = port->devlink;
+	struct net_device *netdev = port->type_eth.netdev;
+	struct ice_pf *pf = devlink_priv(devlink);
+	struct ice_vsi *vsi = *pf->vsi;
+	struct ice_hw *hw = &pf->hw;
+	struct device *dev = ice_pf_to_dev(pf);
+	u8 old_mac[ETH_ALEN];
+	u8 flags = 0;
+	const u8 *mac = hw_addr;
+	int err;
+
+	if (!netdev)
+		return -EADDRNOTAVAIL;
+
+	if (!is_valid_ether_addr(mac))
+		return -EADDRNOTAVAIL;
+
+	if (ether_addr_equal(netdev->dev_addr, mac)) {
+		dev_dbg(dev, "already using mac %pM\n", mac);
+		return 0;
+	}
+
+	if (test_bit(ICE_DOWN, pf->state) ||
+	    ice_is_reset_in_progress(pf->state)) {
+		dev_err(dev, "can't set mac %pM. device not ready\n", mac);
+		return -EBUSY;
+	}
+
+	if (ice_chnl_dmac_fltr_cnt(pf)) {
+		dev_err(dev, "can't set mac %pM. Device has tc-flower filters, delete all of them and try again\n",
+			mac);
+		return -EAGAIN;
+	}
+
+	netif_addr_lock_bh(netdev);
+	ether_addr_copy(old_mac, netdev->dev_addr);
+	/* change the netdev's MAC address */
+	eth_hw_addr_set(netdev, mac);
+	netif_addr_unlock_bh(netdev);
+
+	/* Clean up old MAC filter. Not an error if old filter doesn't exist */
+	err = ice_fltr_remove_mac(vsi, old_mac, ICE_FWD_TO_VSI);
+	if (err && err != -ENOENT) {
+		err = -EADDRNOTAVAIL;
+		goto err_update_filters;
+	}
+
+	/* Add filter for new MAC. If filter exists, return success */
+	err = ice_fltr_add_mac(vsi, mac, ICE_FWD_TO_VSI);
+	if (err == -EEXIST) {
+		/* Although this MAC filter is already present in hardware it's
+		 * possible in some cases (e.g. bonding) that dev_addr was
+		 * modified outside of the driver and needs to be restored back
+		 * to this value.
+		 */
+		dev_dbg(dev, "filter for MAC %pM already exists\n", mac);
+		return 0;
+	} else if (err) {
+		/* error if the new filter addition failed */
+		err = -EADDRNOTAVAIL;
+	}
+
+err_update_filters:
+	if (err) {
+		dev_err(dev, "can't set MAC %pM. filter update failed\n", mac);
+		netif_addr_lock_bh(netdev);
+		eth_hw_addr_set(netdev, old_mac);
+		netif_addr_unlock_bh(netdev);
+		return err;
+	}
+
+	dev_dbg(dev, "updated MAC address to %pM\n", netdev->dev_addr);
+
+	/* write new MAC address to the firmware */
+	flags = ICE_AQC_MAN_MAC_UPDATE_LAA_WOL;
+	err = ice_aq_manage_mac_write(hw, mac, flags, NULL);
+	if (err) {
+		dev_err(dev, "can't set MAC %pM. write to firmware failed error %d\n",
+			mac, err);
+	}
+	return 0;
+}
+
+static const struct devlink_port_ops ice_devlink_vf_port_ops = {
+	.port_fn_hw_addr_get = ice_devlink_port_get_vf_mac_address,
+	.port_fn_hw_addr_set = ice_devlink_port_set_vf_mac_address,
+};
+
 /**
  * ice_devlink_create_vf_port - Create a devlink port for this VF
  * @vf: the VF to create a port for
@@ -1611,7 +1741,8 @@ int ice_devlink_create_vf_port(struct ice_vf *vf)
 	devlink_port_attrs_set(devlink_port, &attrs);
 	devlink = priv_to_devlink(pf);
 
-	err = devlink_port_register(devlink, devlink_port, vsi->idx);
+	err = devlink_port_register_with_ops(devlink, devlink_port,
+					     vsi->idx, &ice_devlink_vf_port_ops);
 	if (err) {
 		dev_err(dev, "Failed to create devlink port for VF %d, error %d\n",
 			vf->vf_id, err);
@@ -1620,7 +1751,6 @@ int ice_devlink_create_vf_port(struct ice_vf *vf)
 
 	return 0;
 }
-
 /**
  * ice_devlink_destroy_vf_port - Destroy the devlink_port for this VF
  * @vf: the VF to cleanup
-- 
2.39.3 (Apple Git-145)

Re: [PATCH] ice: Add get/set hw address for VF representor ports

[Jiri Pirko]

Wed, Jan 31, 2024 at 09:08:47AM CET, ksundara@redhat.com wrote:
>Changing the mac address of the VF representor ports are not
>available via devlink. Add the function handlers to set and get
>the HW address for the VF representor ports.

Wait a sec. The API is there to change the mac of the actual VF. Not the
representor. Apparently your patch is not doing that, changing mac of
the representor.

NAK.

Fix this to change the VF mac address or drop the patch entirely.
Thanks!

pw-bot: cr

Re: [PATCH] ice: Add get/set hw address for VF representor ports

[Michal Swiatkowski]

On Wed, Jan 31, 2024 at 01:38:47PM +0530, karthiksundaravel wrote:
> Changing the mac address of the VF representor ports are not
> available via devlink. Add the function handlers to set and get
> the HW address for the VF representor ports.
> 
> Signed-off-by: karthiksundaravel <ksundara@redhat.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_devlink.c | 134 ++++++++++++++++++-
>  1 file changed, 132 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.c b/drivers/net/ethernet/intel/ice/ice_devlink.c
> index 80dc5445b50d..56d81836c469 100644
> --- a/drivers/net/ethernet/intel/ice/ice_devlink.c
> +++ b/drivers/net/ethernet/intel/ice/ice_devlink.c
> @@ -9,6 +9,8 @@

As Jiri already wrote, you are not changing MAC of VF in your code. Try
to look at ice_set_vf_mac in ice_sriov.c. In current implementation you
nedd to set new MAC value for VF and reset it. You shouldn't use PF VSI.

Pointer to VF you can get from representor struct (through parent VSI).

You shouldn't manage the rules during MAC changing, as in switchdev
slow-path there shouldn't be VF MAC rules. It can be problematic as user
already can have MAC + sth rule (which also needs to be change). I will
leave it to user (most probably the MAC change happens before adding any
rules).

In few days we will send patchset for subfunction support where the
subfunction MAC chaning is implementing from devlink API. I will add you
to the CC.

Thanks for working on it, it is a gap in our solution.

Thanks,
Michal

Re: [PATCH] ice: Add get/set hw address for VF representor ports

[Jiri Pirko]

Wed, Jan 31, 2024 at 11:43:44AM CET, michal.swiatkowski@linux.intel.com wrote:
>On Wed, Jan 31, 2024 at 01:38:47PM +0530, karthiksundaravel wrote:
>> Changing the mac address of the VF representor ports are not
>> available via devlink. Add the function handlers to set and get
>> the HW address for the VF representor ports.
>> 
>> Signed-off-by: karthiksundaravel <ksundara@redhat.com>
>> ---
>>  drivers/net/ethernet/intel/ice/ice_devlink.c | 134 ++++++++++++++++++-
>>  1 file changed, 132 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.c b/drivers/net/ethernet/intel/ice/ice_devlink.c
>> index 80dc5445b50d..56d81836c469 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_devlink.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_devlink.c
>> @@ -9,6 +9,8 @@
>
>As Jiri already wrote, you are not changing MAC of VF in your code. Try
>to look at ice_set_vf_mac in ice_sriov.c. In current implementation you
>nedd to set new MAC value for VF and reset it. You shouldn't use PF VSI.
>
>Pointer to VF you can get from representor struct (through parent VSI).

What if it is in a different host? Would you still be able to change the
mac?


>
>You shouldn't manage the rules during MAC changing, as in switchdev
>slow-path there shouldn't be VF MAC rules. It can be problematic as user
>already can have MAC + sth rule (which also needs to be change). I will
>leave it to user (most probably the MAC change happens before adding any
>rules).

Rules are on the representor, not the VF, correct? Seems unrelated to
me.


>
>In few days we will send patchset for subfunction support where the
>subfunction MAC chaning is implementing from devlink API. I will add you
>to the CC.
>
>Thanks for working on it, it is a gap in our solution.
>
>Thanks,
>Michal
>

Re: [Intel-wired-lan] [PATCH] ice: Add get/set hw address for VF representor ports

[Paul Menzel]

Dear karthiksundaravel,


Thank you for your patch.


Am 31.01.24 um 09:08 schrieb karthiksundaravel:
> Changing the mac address of the VF representor ports are not

Do you mean “is not possible”?

> available via devlink. Add the function handlers to set and get
> the HW address for the VF representor ports.

How did you test this? It’d be great if you documented it.

> Signed-off-by: karthiksundaravel <ksundara@redhat.com>

Is “karthiksundaravel” the official spelling of your name. If not, you 
can change it with

     git config --global user.name "Your Name"
     git commit -s --amend --author="Your Name <ksundara@redhat.com>"

> ---
>   drivers/net/ethernet/intel/ice/ice_devlink.c | 134 ++++++++++++++++++-
>   1 file changed, 132 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.c b/drivers/net/ethernet/intel/ice/ice_devlink.c
> index 80dc5445b50d..56d81836c469 100644
> --- a/drivers/net/ethernet/intel/ice/ice_devlink.c
> +++ b/drivers/net/ethernet/intel/ice/ice_devlink.c
> @@ -9,6 +9,8 @@
>   #include "ice_eswitch.h"
>   #include "ice_fw_update.h"
>   #include "ice_dcb_lib.h"
> +#include "ice_fltr.h"
> +#include "ice_tc_lib.h"
>   
>   static int ice_active_port_option = -1;
>   
> @@ -1576,6 +1578,134 @@ void ice_devlink_destroy_pf_port(struct ice_pf *pf)
>   	devlink_port_unregister(&pf->devlink_port);
>   }
>   
> +/**
> + * ice_devlink_port_get_vf_mac_address - .port_fn_hw_addr_get devlink handler
> + * @port: devlink port structure
> + * @hw_addr: Mac address of the port
> + * @hw_addr_len: length of mac address

Mac/mac is spelled differently. (Also below.)

> + * @extack: extended netdev ack structure
> + *
> + * Callback for the devlink .port_fn_hw_addr_get operation
> + * Return: zero on success or an error code on failure.
> + */
> +
> +static int ice_devlink_port_get_vf_mac_address(struct devlink_port *port,
> +					       u8 *hw_addr, int *hw_addr_len,
> +					       struct netlink_ext_ack *extack)
> +{
> +	struct net_device *netdev = port->type_eth.netdev;
> +
> +	if (!netdev || !netdev->dev_addr)
> +		return -EADDRNOTAVAIL;
> +
> +	ether_addr_copy(hw_addr, netdev->dev_addr);
> +	*hw_addr_len = ETH_ALEN;
> +	return 0;
> +}
> +
> +/**
> + * ice_devlink_port_set_vf_mac_address - .port_fn_hw_addr_set devlink handler
> + * @port: devlink port structure
> + * @hw_addr: Mac address of the port
> + * @hw_addr_len: length of mac address
> + * @extack: extended netdev ack structure
> + *
> + * Callback for the devlink .port_fn_hw_addr_set operation
> + * Return: zero on success or an error code on failure.
> + */
> +static int ice_devlink_port_set_vf_mac_address(struct devlink_port *port,
> +					       const u8 *hw_addr,
> +					       int hw_addr_len,
> +					       struct netlink_ext_ack *extack)
> +{
> +	struct devlink *devlink = port->devlink;
> +	struct net_device *netdev = port->type_eth.netdev;
> +	struct ice_pf *pf = devlink_priv(devlink);
> +	struct ice_vsi *vsi = *pf->vsi;
> +	struct ice_hw *hw = &pf->hw;
> +	struct device *dev = ice_pf_to_dev(pf);
> +	u8 old_mac[ETH_ALEN];
> +	u8 flags = 0;
> +	const u8 *mac = hw_addr;
> +	int err;
> +
> +	if (!netdev)
> +		return -EADDRNOTAVAIL;
> +
> +	if (!is_valid_ether_addr(mac))
> +		return -EADDRNOTAVAIL;
> +
> +	if (ether_addr_equal(netdev->dev_addr, mac)) {
> +		dev_dbg(dev, "already using mac %pM\n", mac);
> +		return 0;
> +	}
> +
> +	if (test_bit(ICE_DOWN, pf->state) ||
> +	    ice_is_reset_in_progress(pf->state)) {
> +		dev_err(dev, "can't set mac %pM. device not ready\n", mac);
> +		return -EBUSY;
> +	}
> +
> +	if (ice_chnl_dmac_fltr_cnt(pf)) {
> +		dev_err(dev, "can't set mac %pM. Device has tc-flower filters, delete all of them and try again\n",
> +			mac);
> +		return -EAGAIN;
> +	}
> +
> +	netif_addr_lock_bh(netdev);
> +	ether_addr_copy(old_mac, netdev->dev_addr);
> +	/* change the netdev's MAC address */

The comment seems redundant.

> +	eth_hw_addr_set(netdev, mac);
> +	netif_addr_unlock_bh(netdev);
> +
> +	/* Clean up old MAC filter. Not an error if old filter doesn't exist */
> +	err = ice_fltr_remove_mac(vsi, old_mac, ICE_FWD_TO_VSI);
> +	if (err && err != -ENOENT) {
> +		err = -EADDRNOTAVAIL;
> +		goto err_update_filters;
> +	}
> +
> +	/* Add filter for new MAC. If filter exists, return success */
> +	err = ice_fltr_add_mac(vsi, mac, ICE_FWD_TO_VSI);
> +	if (err == -EEXIST) {
> +		/* Although this MAC filter is already present in hardware it's
> +		 * possible in some cases (e.g. bonding) that dev_addr was
> +		 * modified outside of the driver and needs to be restored back
> +		 * to this value.
> +		 */
> +		dev_dbg(dev, "filter for MAC %pM already exists\n", mac);
> +		return 0;
> +	} else if (err) {
> +		/* error if the new filter addition failed */

The comment seems redundant.

> +		err = -EADDRNOTAVAIL;
> +	}
> +
> +err_update_filters:
> +	if (err) {
> +		dev_err(dev, "can't set MAC %pM. filter update failed\n", mac);
> +		netif_addr_lock_bh(netdev);
> +		eth_hw_addr_set(netdev, old_mac);
> +		netif_addr_unlock_bh(netdev);
> +		return err;
> +	}
> +
> +	dev_dbg(dev, "updated MAC address to %pM\n", netdev->dev_addr);

Should it be level info?

> +
> +	/* write new MAC address to the firmware */
> +	flags = ICE_AQC_MAN_MAC_UPDATE_LAA_WOL;
> +	err = ice_aq_manage_mac_write(hw, mac, flags, NULL);
> +	if (err) {
> +		dev_err(dev, "can't set MAC %pM. write to firmware failed error %d\n",
> +			mac, err);
> +	}
> +	return 0;
> +}
> +
> +static const struct devlink_port_ops ice_devlink_vf_port_ops = {
> +	.port_fn_hw_addr_get = ice_devlink_port_get_vf_mac_address,
> +	.port_fn_hw_addr_set = ice_devlink_port_set_vf_mac_address,
> +};
> +
>   /**
>    * ice_devlink_create_vf_port - Create a devlink port for this VF
>    * @vf: the VF to create a port for
> @@ -1611,7 +1741,8 @@ int ice_devlink_create_vf_port(struct ice_vf *vf)
>   	devlink_port_attrs_set(devlink_port, &attrs);
>   	devlink = priv_to_devlink(pf);
>   
> -	err = devlink_port_register(devlink, devlink_port, vsi->idx);
> +	err = devlink_port_register_with_ops(devlink, devlink_port,
> +					     vsi->idx, &ice_devlink_vf_port_ops);
>   	if (err) {
>   		dev_err(dev, "Failed to create devlink port for VF %d, error %d\n",
>   			vf->vf_id, err);
> @@ -1620,7 +1751,6 @@ int ice_devlink_create_vf_port(struct ice_vf *vf)
>   
>   	return 0;
>   }
> -

Unrelated whitespace change.

>   /**
>    * ice_devlink_destroy_vf_port - Destroy the devlink_port for this VF
>    * @vf: the VF to cleanup

Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>


Kind regards,

Paul

Re: [PATCH] ice: Add get/set hw address for VF representor ports

[Michal Swiatkowski]

On Wed, Jan 31, 2024 at 01:00:04PM +0100, Jiri Pirko wrote:
> Wed, Jan 31, 2024 at 11:43:44AM CET, michal.swiatkowski@linux.intel.com wrote:
> >On Wed, Jan 31, 2024 at 01:38:47PM +0530, karthiksundaravel wrote:
> >> Changing the mac address of the VF representor ports are not
> >> available via devlink. Add the function handlers to set and get
> >> the HW address for the VF representor ports.
> >> 
> >> Signed-off-by: karthiksundaravel <ksundara@redhat.com>
> >> ---
> >>  drivers/net/ethernet/intel/ice/ice_devlink.c | 134 ++++++++++++++++++-
> >>  1 file changed, 132 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.c b/drivers/net/ethernet/intel/ice/ice_devlink.c
> >> index 80dc5445b50d..56d81836c469 100644
> >> --- a/drivers/net/ethernet/intel/ice/ice_devlink.c
> >> +++ b/drivers/net/ethernet/intel/ice/ice_devlink.c
> >> @@ -9,6 +9,8 @@
> >
> >As Jiri already wrote, you are not changing MAC of VF in your code. Try
> >to look at ice_set_vf_mac in ice_sriov.c. In current implementation you
> >nedd to set new MAC value for VF and reset it. You shouldn't use PF VSI.
> >
> >Pointer to VF you can get from representor struct (through parent VSI).
> 
> What if it is in a different host? Would you still be able to change the
> mac?
> 

In current VF MAC changing implementation yes, because it is done by
resetting the VF. After the reset new MAC will be sent via virtchnl.
But I think resetting VF may be incorrect here, as it leads to reset
also port representor.

> 
> >
> >You shouldn't manage the rules during MAC changing, as in switchdev
> >slow-path there shouldn't be VF MAC rules. It can be problematic as user
> >already can have MAC + sth rule (which also needs to be change). I will
> >leave it to user (most probably the MAC change happens before adding any
> >rules).
> 
> Rules are on the representor, not the VF, correct? Seems unrelated to
> me.
> 

I pointed it out because it was in the code. Rules added on representor
points to corresponding VF. My point was that there shouldn't be any
changes to rules after changing MAC.

> 
> >
> >In few days we will send patchset for subfunction support where the
> >subfunction MAC chaning is implementing from devlink API. I will add you
> >to the CC.
> >
> >Thanks for working on it, it is a gap in our solution.
> >
> >Thanks,
> >Michal
> >

Re: [Intel-wired-lan] [PATCH] ice: Add get/set hw address for VF representor ports

[Jiri Pirko]

Wed, Jan 31, 2024 at 05:15:46PM CET, pmenzel@molgen.mpg.de wrote:
>Dear karthiksundaravel,
>
>
>Thank you for your patch.
>
>
>Am 31.01.24 um 09:08 schrieb karthiksundaravel:
>> Changing the mac address of the VF representor ports are not
>
>Do you mean “is not possible”?
>
>> available via devlink. Add the function handlers to set and get
>> the HW address for the VF representor ports.
>
>How did you test this? It’d be great if you documented it.
>
>> Signed-off-by: karthiksundaravel <ksundara@redhat.com>
>
>Is “karthiksundaravel” the official spelling of your name. If not, you can
>change it with
>
>    git config --global user.name "Your Name"
>    git commit -s --amend --author="Your Name <ksundara@redhat.com>"
>
>> ---
>>   drivers/net/ethernet/intel/ice/ice_devlink.c | 134 ++++++++++++++++++-
>>   1 file changed, 132 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.c b/drivers/net/ethernet/intel/ice/ice_devlink.c
>> index 80dc5445b50d..56d81836c469 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_devlink.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_devlink.c
>> @@ -9,6 +9,8 @@
>>   #include "ice_eswitch.h"
>>   #include "ice_fw_update.h"
>>   #include "ice_dcb_lib.h"
>> +#include "ice_fltr.h"
>> +#include "ice_tc_lib.h"
>>   static int ice_active_port_option = -1;
>> @@ -1576,6 +1578,134 @@ void ice_devlink_destroy_pf_port(struct ice_pf *pf)
>>   	devlink_port_unregister(&pf->devlink_port);
>>   }
>> +/**
>> + * ice_devlink_port_get_vf_mac_address - .port_fn_hw_addr_get devlink handler
>> + * @port: devlink port structure
>> + * @hw_addr: Mac address of the port
>> + * @hw_addr_len: length of mac address
>
>Mac/mac is spelled differently. (Also below.)
>
>> + * @extack: extended netdev ack structure
>> + *
>> + * Callback for the devlink .port_fn_hw_addr_get operation
>> + * Return: zero on success or an error code on failure.
>> + */
>> +
>> +static int ice_devlink_port_get_vf_mac_address(struct devlink_port *port,
>> +					       u8 *hw_addr, int *hw_addr_len,
>> +					       struct netlink_ext_ack *extack)
>> +{
>> +	struct net_device *netdev = port->type_eth.netdev;
>> +
>> +	if (!netdev || !netdev->dev_addr)
>> +		return -EADDRNOTAVAIL;
>> +
>> +	ether_addr_copy(hw_addr, netdev->dev_addr);
>> +	*hw_addr_len = ETH_ALEN;
>> +	return 0;
>> +}
>> +
>> +/**
>> + * ice_devlink_port_set_vf_mac_address - .port_fn_hw_addr_set devlink handler
>> + * @port: devlink port structure
>> + * @hw_addr: Mac address of the port
>> + * @hw_addr_len: length of mac address
>> + * @extack: extended netdev ack structure
>> + *
>> + * Callback for the devlink .port_fn_hw_addr_set operation
>> + * Return: zero on success or an error code on failure.
>> + */
>> +static int ice_devlink_port_set_vf_mac_address(struct devlink_port *port,
>> +					       const u8 *hw_addr,
>> +					       int hw_addr_len,
>> +					       struct netlink_ext_ack *extack)
>> +{
>> +	struct devlink *devlink = port->devlink;
>> +	struct net_device *netdev = port->type_eth.netdev;
>> +	struct ice_pf *pf = devlink_priv(devlink);
>> +	struct ice_vsi *vsi = *pf->vsi;
>> +	struct ice_hw *hw = &pf->hw;
>> +	struct device *dev = ice_pf_to_dev(pf);
>> +	u8 old_mac[ETH_ALEN];
>> +	u8 flags = 0;
>> +	const u8 *mac = hw_addr;
>> +	int err;
>> +
>> +	if (!netdev)
>> +		return -EADDRNOTAVAIL;
>> +
>> +	if (!is_valid_ether_addr(mac))
>> +		return -EADDRNOTAVAIL;
>> +
>> +	if (ether_addr_equal(netdev->dev_addr, mac)) {
>> +		dev_dbg(dev, "already using mac %pM\n", mac);
>> +		return 0;
>> +	}
>> +
>> +	if (test_bit(ICE_DOWN, pf->state) ||
>> +	    ice_is_reset_in_progress(pf->state)) {
>> +		dev_err(dev, "can't set mac %pM. device not ready\n", mac);
>> +		return -EBUSY;
>> +	}
>> +
>> +	if (ice_chnl_dmac_fltr_cnt(pf)) {
>> +		dev_err(dev, "can't set mac %pM. Device has tc-flower filters, delete all of them and try again\n",
>> +			mac);
>> +		return -EAGAIN;
>> +	}
>> +
>> +	netif_addr_lock_bh(netdev);
>> +	ether_addr_copy(old_mac, netdev->dev_addr);
>> +	/* change the netdev's MAC address */
>
>The comment seems redundant.
>
>> +	eth_hw_addr_set(netdev, mac);
>> +	netif_addr_unlock_bh(netdev);
>> +
>> +	/* Clean up old MAC filter. Not an error if old filter doesn't exist */
>> +	err = ice_fltr_remove_mac(vsi, old_mac, ICE_FWD_TO_VSI);
>> +	if (err && err != -ENOENT) {
>> +		err = -EADDRNOTAVAIL;
>> +		goto err_update_filters;
>> +	}
>> +
>> +	/* Add filter for new MAC. If filter exists, return success */
>> +	err = ice_fltr_add_mac(vsi, mac, ICE_FWD_TO_VSI);
>> +	if (err == -EEXIST) {
>> +		/* Although this MAC filter is already present in hardware it's
>> +		 * possible in some cases (e.g. bonding) that dev_addr was
>> +		 * modified outside of the driver and needs to be restored back
>> +		 * to this value.
>> +		 */
>> +		dev_dbg(dev, "filter for MAC %pM already exists\n", mac);
>> +		return 0;
>> +	} else if (err) {
>> +		/* error if the new filter addition failed */
>
>The comment seems redundant.
>
>> +		err = -EADDRNOTAVAIL;
>> +	}
>> +
>> +err_update_filters:
>> +	if (err) {
>> +		dev_err(dev, "can't set MAC %pM. filter update failed\n", mac);
>> +		netif_addr_lock_bh(netdev);
>> +		eth_hw_addr_set(netdev, old_mac);
>> +		netif_addr_unlock_bh(netdev);
>> +		return err;
>> +	}
>> +
>> +	dev_dbg(dev, "updated MAC address to %pM\n", netdev->dev_addr);
>
>Should it be level info?
>
>> +
>> +	/* write new MAC address to the firmware */
>> +	flags = ICE_AQC_MAN_MAC_UPDATE_LAA_WOL;
>> +	err = ice_aq_manage_mac_write(hw, mac, flags, NULL);
>> +	if (err) {
>> +		dev_err(dev, "can't set MAC %pM. write to firmware failed error %d\n",
>> +			mac, err);
>> +	}
>> +	return 0;
>> +}
>> +
>> +static const struct devlink_port_ops ice_devlink_vf_port_ops = {
>> +	.port_fn_hw_addr_get = ice_devlink_port_get_vf_mac_address,
>> +	.port_fn_hw_addr_set = ice_devlink_port_set_vf_mac_address,
>> +};
>> +
>>   /**
>>    * ice_devlink_create_vf_port - Create a devlink port for this VF
>>    * @vf: the VF to create a port for
>> @@ -1611,7 +1741,8 @@ int ice_devlink_create_vf_port(struct ice_vf *vf)
>>   	devlink_port_attrs_set(devlink_port, &attrs);
>>   	devlink = priv_to_devlink(pf);
>> -	err = devlink_port_register(devlink, devlink_port, vsi->idx);
>> +	err = devlink_port_register_with_ops(devlink, devlink_port,
>> +					     vsi->idx, &ice_devlink_vf_port_ops);
>>   	if (err) {
>>   		dev_err(dev, "Failed to create devlink port for VF %d, error %d\n",
>>   			vf->vf_id, err);
>> @@ -1620,7 +1751,6 @@ int ice_devlink_create_vf_port(struct ice_vf *vf)
>>   	return 0;
>>   }
>> -
>
>Unrelated whitespace change.
>
>>   /**
>>    * ice_devlink_destroy_vf_port - Destroy the devlink_port for this VF
>>    * @vf: the VF to cleanup
>
>Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>

Paul. It looks a bit weird you put in multiple comments that require
changes and then the Reviewed-by tag. Usually, you put the tag only if
you are 100% happy with the patch as it is.

It is also weird you put in a tag in this case when the patch
is completely wrong, as I pointed out in my first reply. Did you miss
it?


>
>
>Kind regards,
>
>Paul
>

Re: [Intel-wired-lan] [PATCH] ice: Add get/set hw address for VF representor ports

[Paul Menzel]

Dear Jiri,


Am 01.02.24 um 08:40 schrieb Jiri Pirko:
> Wed, Jan 31, 2024 at 05:15:46PM CET, pmenzel@molgen.mpg.de wrote:

[…]

>> Am 31.01.24 um 09:08 schrieb karthiksundaravel:
>>> Changing the mac address of the VF representor ports are not
>>
>> Do you mean “is not possible”?
>>
>>> available via devlink. Add the function handlers to set and get
>>> the HW address for the VF representor ports.
>>
>> How did you test this? It’d be great if you documented it.
>>
>>> Signed-off-by: karthiksundaravel <ksundara@redhat.com>
>>
>> Is “karthiksundaravel” the official spelling of your name. If not, you can
>> change it with
>>
>>     git config --global user.name "Your Name"
>>     git commit -s --amend --author="Your Name <ksundara@redhat.com>"
>>
>>> ---
>>>    drivers/net/ethernet/intel/ice/ice_devlink.c | 134 ++++++++++++++++++-
>>>    1 file changed, 132 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.c b/drivers/net/ethernet/intel/ice/ice_devlink.c
>>> index 80dc5445b50d..56d81836c469 100644
>>> --- a/drivers/net/ethernet/intel/ice/ice_devlink.c
>>> +++ b/drivers/net/ethernet/intel/ice/ice_devlink.c
>>> @@ -9,6 +9,8 @@
>>>    #include "ice_eswitch.h"
>>>    #include "ice_fw_update.h"
>>>    #include "ice_dcb_lib.h"
>>> +#include "ice_fltr.h"
>>> +#include "ice_tc_lib.h"
>>>    static int ice_active_port_option = -1;
>>> @@ -1576,6 +1578,134 @@ void ice_devlink_destroy_pf_port(struct ice_pf *pf)
>>>    	devlink_port_unregister(&pf->devlink_port);
>>>    }
>>> +/**
>>> + * ice_devlink_port_get_vf_mac_address - .port_fn_hw_addr_get devlink handler
>>> + * @port: devlink port structure
>>> + * @hw_addr: Mac address of the port
>>> + * @hw_addr_len: length of mac address
>>
>> Mac/mac is spelled differently. (Also below.)
>>
>>> + * @extack: extended netdev ack structure
>>> + *
>>> + * Callback for the devlink .port_fn_hw_addr_get operation
>>> + * Return: zero on success or an error code on failure.
>>> + */
>>> +
>>> +static int ice_devlink_port_get_vf_mac_address(struct devlink_port *port,
>>> +					       u8 *hw_addr, int *hw_addr_len,
>>> +					       struct netlink_ext_ack *extack)
>>> +{
>>> +	struct net_device *netdev = port->type_eth.netdev;
>>> +
>>> +	if (!netdev || !netdev->dev_addr)
>>> +		return -EADDRNOTAVAIL;
>>> +
>>> +	ether_addr_copy(hw_addr, netdev->dev_addr);
>>> +	*hw_addr_len = ETH_ALEN;
>>> +	return 0;
>>> +}
>>> +
>>> +/**
>>> + * ice_devlink_port_set_vf_mac_address - .port_fn_hw_addr_set devlink handler
>>> + * @port: devlink port structure
>>> + * @hw_addr: Mac address of the port
>>> + * @hw_addr_len: length of mac address
>>> + * @extack: extended netdev ack structure
>>> + *
>>> + * Callback for the devlink .port_fn_hw_addr_set operation
>>> + * Return: zero on success or an error code on failure.
>>> + */
>>> +static int ice_devlink_port_set_vf_mac_address(struct devlink_port *port,
>>> +					       const u8 *hw_addr,
>>> +					       int hw_addr_len,
>>> +					       struct netlink_ext_ack *extack)
>>> +{
>>> +	struct devlink *devlink = port->devlink;
>>> +	struct net_device *netdev = port->type_eth.netdev;
>>> +	struct ice_pf *pf = devlink_priv(devlink);
>>> +	struct ice_vsi *vsi = *pf->vsi;
>>> +	struct ice_hw *hw = &pf->hw;
>>> +	struct device *dev = ice_pf_to_dev(pf);
>>> +	u8 old_mac[ETH_ALEN];
>>> +	u8 flags = 0;
>>> +	const u8 *mac = hw_addr;
>>> +	int err;
>>> +
>>> +	if (!netdev)
>>> +		return -EADDRNOTAVAIL;
>>> +
>>> +	if (!is_valid_ether_addr(mac))
>>> +		return -EADDRNOTAVAIL;
>>> +
>>> +	if (ether_addr_equal(netdev->dev_addr, mac)) {
>>> +		dev_dbg(dev, "already using mac %pM\n", mac);
>>> +		return 0;
>>> +	}
>>> +
>>> +	if (test_bit(ICE_DOWN, pf->state) ||
>>> +	    ice_is_reset_in_progress(pf->state)) {
>>> +		dev_err(dev, "can't set mac %pM. device not ready\n", mac);
>>> +		return -EBUSY;
>>> +	}
>>> +
>>> +	if (ice_chnl_dmac_fltr_cnt(pf)) {
>>> +		dev_err(dev, "can't set mac %pM. Device has tc-flower filters, delete all of them and try again\n",
>>> +			mac);
>>> +		return -EAGAIN;
>>> +	}
>>> +
>>> +	netif_addr_lock_bh(netdev);
>>> +	ether_addr_copy(old_mac, netdev->dev_addr);
>>> +	/* change the netdev's MAC address */
>>
>> The comment seems redundant.
>>
>>> +	eth_hw_addr_set(netdev, mac);
>>> +	netif_addr_unlock_bh(netdev);
>>> +
>>> +	/* Clean up old MAC filter. Not an error if old filter doesn't exist */
>>> +	err = ice_fltr_remove_mac(vsi, old_mac, ICE_FWD_TO_VSI);
>>> +	if (err && err != -ENOENT) {
>>> +		err = -EADDRNOTAVAIL;
>>> +		goto err_update_filters;
>>> +	}
>>> +
>>> +	/* Add filter for new MAC. If filter exists, return success */
>>> +	err = ice_fltr_add_mac(vsi, mac, ICE_FWD_TO_VSI);
>>> +	if (err == -EEXIST) {
>>> +		/* Although this MAC filter is already present in hardware it's
>>> +		 * possible in some cases (e.g. bonding) that dev_addr was
>>> +		 * modified outside of the driver and needs to be restored back
>>> +		 * to this value.
>>> +		 */
>>> +		dev_dbg(dev, "filter for MAC %pM already exists\n", mac);
>>> +		return 0;
>>> +	} else if (err) {
>>> +		/* error if the new filter addition failed */
>>
>> The comment seems redundant.
>>
>>> +		err = -EADDRNOTAVAIL;
>>> +	}
>>> +
>>> +err_update_filters:
>>> +	if (err) {
>>> +		dev_err(dev, "can't set MAC %pM. filter update failed\n", mac);
>>> +		netif_addr_lock_bh(netdev);
>>> +		eth_hw_addr_set(netdev, old_mac);
>>> +		netif_addr_unlock_bh(netdev);
>>> +		return err;
>>> +	}
>>> +
>>> +	dev_dbg(dev, "updated MAC address to %pM\n", netdev->dev_addr);
>>
>> Should it be level info?
>>
>>> +
>>> +	/* write new MAC address to the firmware */
>>> +	flags = ICE_AQC_MAN_MAC_UPDATE_LAA_WOL;
>>> +	err = ice_aq_manage_mac_write(hw, mac, flags, NULL);
>>> +	if (err) {
>>> +		dev_err(dev, "can't set MAC %pM. write to firmware failed error %d\n",
>>> +			mac, err);
>>> +	}
>>> +	return 0;
>>> +}
>>> +
>>> +static const struct devlink_port_ops ice_devlink_vf_port_ops = {
>>> +	.port_fn_hw_addr_get = ice_devlink_port_get_vf_mac_address,
>>> +	.port_fn_hw_addr_set = ice_devlink_port_set_vf_mac_address,
>>> +};
>>> +
>>>    /**
>>>     * ice_devlink_create_vf_port - Create a devlink port for this VF
>>>     * @vf: the VF to create a port for
>>> @@ -1611,7 +1741,8 @@ int ice_devlink_create_vf_port(struct ice_vf *vf)
>>>    	devlink_port_attrs_set(devlink_port, &attrs);
>>>    	devlink = priv_to_devlink(pf);
>>> -	err = devlink_port_register(devlink, devlink_port, vsi->idx);
>>> +	err = devlink_port_register_with_ops(devlink, devlink_port,
>>> +					     vsi->idx, &ice_devlink_vf_port_ops);
>>>    	if (err) {
>>>    		dev_err(dev, "Failed to create devlink port for VF %d, error %d\n",
>>>    			vf->vf_id, err);
>>> @@ -1620,7 +1751,6 @@ int ice_devlink_create_vf_port(struct ice_vf *vf)
>>>    	return 0;
>>>    }
>>> -
>>
>> Unrelated whitespace change.
>>
>>>    /**
>>>     * ice_devlink_destroy_vf_port - Destroy the devlink_port for this VF
>>>     * @vf: the VF to cleanup
>>
>> Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>
> 
> Paul. It looks a bit weird you put in multiple comments that require
> changes and then the Reviewed-by tag. Usually, you put the tag only if
> you are 100% happy with the patch as it is.

Sorry about that. I will keep this in mind.

> It is also weird you put in a tag in this case when the patch
> is completely wrong, as I pointed out in my first reply. Did you miss
> it?

Yes, I missed it. (I disabled threading in Mozilla Thunderbird. I am 
going to change that again. Sorry about that.)


Kind regards,

Paul