From: Satish Kharat <satishkh@cisco.com>

Both the data path (ndo_open/ndo_stop) and the upcoming admin channel
need to enable and disable the vNIC device independently. Without
reference counting, closing the admin channel while the netdev is up
would inadvertently disable the entire device.

Add an enable_count to struct enic, protected by the existing
devcmd_lock. enic_dev_enable() issues CMD_ENABLE_WAIT only on the
first caller (0 -> 1 transition), and enic_dev_disable() issues
CMD_DISABLE only when the last caller releases (1 -> 0 transition).

No functional change for the current single-caller data path.

Signed-off-by: Satish Kharat <satishkh@cisco.com>
---
 drivers/net/ethernet/cisco/enic/enic.h     |  1 +
 drivers/net/ethernet/cisco/enic/enic_dev.c | 17 +++++++++++++----
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/cisco/enic/enic.h b/drivers/net/ethernet/cisco/enic/enic.h
index 0fd9cd917132..67fd780b1fa1 100644
--- a/drivers/net/ethernet/cisco/enic/enic.h
+++ b/drivers/net/ethernet/cisco/enic/enic.h
@@ -260,6 +260,7 @@ struct enic {
 	u16 num_vfs;
 #endif
 	enum enic_vf_type vf_type;
+	unsigned int enable_count;
 	spinlock_t enic_api_lock;
 	bool enic_api_busy;
 	struct enic_port_profile *pp;
diff --git a/drivers/net/ethernet/cisco/enic/enic_dev.c b/drivers/net/ethernet/cisco/enic/enic_dev.c
index 2cbae7c6cc3d..659787f73cf1 100644
--- a/drivers/net/ethernet/cisco/enic/enic_dev.c
+++ b/drivers/net/ethernet/cisco/enic/enic_dev.c
@@ -131,10 +131,13 @@ int enic_dev_set_ig_vlan_rewrite_mode(struct enic *enic)
 
 int enic_dev_enable(struct enic *enic)
 {
-	int err;
+	int err = 0;
 
 	spin_lock_bh(&enic->devcmd_lock);
-	err = vnic_dev_enable_wait(enic->vdev);
+	if (enic->enable_count == 0)
+		err = vnic_dev_enable_wait(enic->vdev);
+	if (!err)
+		enic->enable_count++;
 	spin_unlock_bh(&enic->devcmd_lock);
 
 	return err;
@@ -142,10 +145,16 @@ int enic_dev_enable(struct enic *enic)
 
 int enic_dev_disable(struct enic *enic)
 {
-	int err;
+	int err = 0;
 
 	spin_lock_bh(&enic->devcmd_lock);
-	err = vnic_dev_disable(enic->vdev);
+	if (enic->enable_count == 0) {
+		spin_unlock_bh(&enic->devcmd_lock);
+		return 0;
+	}
+	enic->enable_count--;
+	if (enic->enable_count == 0)
+		err = vnic_dev_disable(enic->vdev);
 	spin_unlock_bh(&enic->devcmd_lock);
 
 	return err;

-- 
2.43.0

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
enic: make enic_dev_enable/disable ref-counted

Both the data path (ndo_open/ndo_stop) and the upcoming admin channel
need to enable and disable the vNIC device independently. Without
reference counting, closing the admin channel while the netdev is up
would inadvertently disable the entire device.

[ ... ]
> diff --git a/drivers/net/ethernet/cisco/enic/enic_dev.c b/drivers/net/ethernet/cisco/enic/enic_dev.c
> index 2cbae7c6cc3d6..659787f73cf18 100644
> --- a/drivers/net/ethernet/cisco/enic/enic_dev.c
> +++ b/drivers/net/ethernet/cisco/enic/enic_dev.c
> @@ -131,10 +131,13 @@ int enic_dev_set_ig_vlan_rewrite_mode(struct enic *enic)
>  
>  int enic_dev_enable(struct enic *enic)
>  {
> -	int err;
> +	int err = 0;
>  
>  	spin_lock_bh(&enic->devcmd_lock);
> -	err = vnic_dev_enable_wait(enic->vdev);
> +	if (enic->enable_count == 0)
> +		err = vnic_dev_enable_wait(enic->vdev);
> +	if (!err)
> +		enic->enable_count++;
>  	spin_unlock_bh(&enic->devcmd_lock);
>  
>  	return err;
> @@ -142,10 +145,16 @@ int enic_dev_enable(struct enic *enic)
>  
>  int enic_dev_disable(struct enic *enic)
>  {
> -	int err;
> +	int err = 0;
>  
>  	spin_lock_bh(&enic->devcmd_lock);
> -	err = vnic_dev_disable(enic->vdev);
> +	if (enic->enable_count == 0) {
> +		spin_unlock_bh(&enic->devcmd_lock);
> +		return 0;
> +	}
> +	enic->enable_count--;
> +	if (enic->enable_count == 0)
> +		err = vnic_dev_disable(enic->vdev);
>  	spin_unlock_bh(&enic->devcmd_lock);
>  
>  	return err;

If vnic_dev_enable_wait() fails, enable_count remains 0. However, the data
path caller enic_open() ignores the return value of enic_dev_enable() and
unconditionally returns 0:

drivers/net/ethernet/cisco/enic/enic_main.c:enic_open() {
    ...
	enic_dev_enable(enic);

	for (i = 0; i < enic->intr_count; i++)
		vnic_intr_unmask(&enic->intr[i]);
    ...
	return 0;
}

Can this cause the device reference count to become unbalanced?

If enic_dev_enable() fails during enic_open(), the interface is marked
logically up but enable_count remains 0. If enic_stop() is called later,
enic_dev_disable() will return early since the count is 0, skipping the
hardware disable command.

Additionally, if the new admin channel calls enic_dev_enable() while
the device is in this state, it will increment the count from 0 to 1.
When the admin channel later calls enic_dev_disable(), the count will
drop back to 0 and issue vnic_dev_disable(), incorrectly disabling the
hardware while the netdev data path is still up.

Should enic_open() check the return value of enic_dev_enable() and
handle failures appropriately, or should the enable_count be incremented
regardless of the hardware command success to keep the logical states
aligned?
-- 
pw-bot: cr