* [PATCH iwl-net v1 1/4] idpf: Change function argument
2024-11-05 18:48 [PATCH iwl-net v1 0/4] fix locking issue Tarun K Singh
@ 2024-11-05 18:48 ` Tarun K Singh
2025-01-14 6:53 ` Singh, Krishneil K
2024-11-05 18:48 ` [PATCH iwl-net v1 2/4] idpf: rename vport_ctrl_lock Tarun K Singh
` (3 subsequent siblings)
4 siblings, 1 reply; 13+ messages in thread
From: Tarun K Singh @ 2024-11-05 18:48 UTC (permalink / raw)
To: intel-wired-lan; +Cc: netdev
Change idpf_vport_ctrl_lock's argument from netdev to adapter.
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Signed-off-by: Tarun K Singh <tarun.k.singh@intel.com>
---
drivers/net/ethernet/intel/idpf/idpf.h | 16 ++---
.../net/ethernet/intel/idpf/idpf_ethtool.c | 59 ++++++++++---------
drivers/net/ethernet/intel/idpf/idpf_lib.c | 39 ++++++------
3 files changed, 58 insertions(+), 56 deletions(-)
diff --git a/drivers/net/ethernet/intel/idpf/idpf.h b/drivers/net/ethernet/intel/idpf/idpf.h
index 66544faab710..d87ed50af681 100644
--- a/drivers/net/ethernet/intel/idpf/idpf.h
+++ b/drivers/net/ethernet/intel/idpf/idpf.h
@@ -788,27 +788,23 @@ static inline u16 idpf_get_max_tx_hdr_size(struct idpf_adapter *adapter)
/**
* idpf_vport_ctrl_lock - Acquire the vport control lock
- * @netdev: Network interface device structure
+ * @adapter: private data struct
*
* This lock should be used by non-datapath code to protect against vport
* destruction.
*/
-static inline void idpf_vport_ctrl_lock(struct net_device *netdev)
+static inline void idpf_vport_ctrl_lock(struct idpf_adapter *adapter)
{
- struct idpf_netdev_priv *np = netdev_priv(netdev);
-
- mutex_lock(&np->adapter->vport_ctrl_lock);
+ mutex_lock(&adapter->vport_ctrl_lock);
}
/**
* idpf_vport_ctrl_unlock - Release the vport control lock
- * @netdev: Network interface device structure
+ * @adapter: private data struct
*/
-static inline void idpf_vport_ctrl_unlock(struct net_device *netdev)
+static inline void idpf_vport_ctrl_unlock(struct idpf_adapter *adapter)
{
- struct idpf_netdev_priv *np = netdev_priv(netdev);
-
- mutex_unlock(&np->adapter->vport_ctrl_lock);
+ mutex_unlock(&adapter->vport_ctrl_lock);
}
void idpf_statistics_task(struct work_struct *work);
diff --git a/drivers/net/ethernet/intel/idpf/idpf_ethtool.c b/drivers/net/ethernet/intel/idpf/idpf_ethtool.c
index 59b1a1a09996..e5ac3e5a50ce 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_ethtool.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_ethtool.c
@@ -14,22 +14,23 @@
static int idpf_get_rxnfc(struct net_device *netdev, struct ethtool_rxnfc *cmd,
u32 __always_unused *rule_locs)
{
+ struct idpf_adapter *adapter = idpf_netdev_to_adapter(netdev);
struct idpf_vport *vport;
- idpf_vport_ctrl_lock(netdev);
+ idpf_vport_ctrl_lock(adapter);
vport = idpf_netdev_to_vport(netdev);
switch (cmd->cmd) {
case ETHTOOL_GRXRINGS:
cmd->data = vport->num_rxq;
- idpf_vport_ctrl_unlock(netdev);
+ idpf_vport_ctrl_unlock(adapter);
return 0;
default:
break;
}
- idpf_vport_ctrl_unlock(netdev);
+ idpf_vport_ctrl_unlock(adapter);
return -EOPNOTSUPP;
}
@@ -88,9 +89,8 @@ static int idpf_get_rxfh(struct net_device *netdev,
int err = 0;
u16 i;
- idpf_vport_ctrl_lock(netdev);
-
adapter = np->adapter;
+ idpf_vport_ctrl_lock(adapter);
if (!idpf_is_cap_ena_all(adapter, IDPF_RSS_CAPS, IDPF_CAP_RSS)) {
err = -EOPNOTSUPP;
@@ -112,7 +112,7 @@ static int idpf_get_rxfh(struct net_device *netdev,
}
unlock_mutex:
- idpf_vport_ctrl_unlock(netdev);
+ idpf_vport_ctrl_unlock(adapter);
return err;
}
@@ -131,17 +131,15 @@ static int idpf_set_rxfh(struct net_device *netdev,
struct netlink_ext_ack *extack)
{
struct idpf_netdev_priv *np = netdev_priv(netdev);
+ struct idpf_adapter *adapter = np->adapter;
struct idpf_rss_data *rss_data;
- struct idpf_adapter *adapter;
struct idpf_vport *vport;
int err = 0;
u16 lut;
- idpf_vport_ctrl_lock(netdev);
+ idpf_vport_ctrl_lock(adapter);
vport = idpf_netdev_to_vport(netdev);
- adapter = vport->adapter;
-
if (!idpf_is_cap_ena_all(adapter, IDPF_RSS_CAPS, IDPF_CAP_RSS)) {
err = -EOPNOTSUPP;
goto unlock_mutex;
@@ -168,7 +166,7 @@ static int idpf_set_rxfh(struct net_device *netdev,
err = idpf_config_rss(vport);
unlock_mutex:
- idpf_vport_ctrl_unlock(netdev);
+ idpf_vport_ctrl_unlock(adapter);
return err;
}
@@ -221,6 +219,7 @@ static void idpf_get_channels(struct net_device *netdev,
static int idpf_set_channels(struct net_device *netdev,
struct ethtool_channels *ch)
{
+ struct idpf_adapter *adapter = idpf_netdev_to_adapter(netdev);
struct idpf_vport_config *vport_config;
unsigned int num_req_tx_q;
unsigned int num_req_rx_q;
@@ -235,7 +234,7 @@ static int idpf_set_channels(struct net_device *netdev,
return -EINVAL;
}
- idpf_vport_ctrl_lock(netdev);
+ idpf_vport_ctrl_lock(adapter);
vport = idpf_netdev_to_vport(netdev);
idx = vport->idx;
@@ -279,7 +278,7 @@ static int idpf_set_channels(struct net_device *netdev,
}
unlock_mutex:
- idpf_vport_ctrl_unlock(netdev);
+ idpf_vport_ctrl_unlock(adapter);
return err;
}
@@ -299,9 +298,10 @@ static void idpf_get_ringparam(struct net_device *netdev,
struct kernel_ethtool_ringparam *kring,
struct netlink_ext_ack *ext_ack)
{
+ struct idpf_adapter *adapter = idpf_netdev_to_adapter(netdev);
struct idpf_vport *vport;
- idpf_vport_ctrl_lock(netdev);
+ idpf_vport_ctrl_lock(adapter);
vport = idpf_netdev_to_vport(netdev);
ring->rx_max_pending = IDPF_MAX_RXQ_DESC;
@@ -311,7 +311,7 @@ static void idpf_get_ringparam(struct net_device *netdev,
kring->tcp_data_split = idpf_vport_get_hsplit(vport);
- idpf_vport_ctrl_unlock(netdev);
+ idpf_vport_ctrl_unlock(adapter);
}
/**
@@ -329,13 +329,14 @@ static int idpf_set_ringparam(struct net_device *netdev,
struct kernel_ethtool_ringparam *kring,
struct netlink_ext_ack *ext_ack)
{
+ struct idpf_adapter *adapter = idpf_netdev_to_adapter(netdev);
struct idpf_vport_user_config_data *config_data;
u32 new_rx_count, new_tx_count;
struct idpf_vport *vport;
int i, err = 0;
u16 idx;
- idpf_vport_ctrl_lock(netdev);
+ idpf_vport_ctrl_lock(adapter);
vport = idpf_netdev_to_vport(netdev);
idx = vport->idx;
@@ -394,7 +395,7 @@ static int idpf_set_ringparam(struct net_device *netdev,
err = idpf_initiate_soft_reset(vport, IDPF_SR_Q_DESC_CHANGE);
unlock_mutex:
- idpf_vport_ctrl_unlock(netdev);
+ idpf_vport_ctrl_unlock(adapter);
return err;
}
@@ -869,6 +870,7 @@ static void idpf_get_ethtool_stats(struct net_device *netdev,
u64 *data)
{
struct idpf_netdev_priv *np = netdev_priv(netdev);
+ struct idpf_adapter *adapter = np->adapter;
struct idpf_vport_config *vport_config;
struct idpf_vport *vport;
unsigned int total = 0;
@@ -876,11 +878,11 @@ static void idpf_get_ethtool_stats(struct net_device *netdev,
bool is_splitq;
u16 qtype;
- idpf_vport_ctrl_lock(netdev);
+ idpf_vport_ctrl_lock(adapter);
vport = idpf_netdev_to_vport(netdev);
if (np->state != __IDPF_VPORT_UP) {
- idpf_vport_ctrl_unlock(netdev);
+ idpf_vport_ctrl_unlock(adapter);
return;
}
@@ -947,7 +949,7 @@ static void idpf_get_ethtool_stats(struct net_device *netdev,
rcu_read_unlock();
- idpf_vport_ctrl_unlock(netdev);
+ idpf_vport_ctrl_unlock(adapter);
}
/**
@@ -1025,10 +1027,11 @@ static int idpf_get_q_coalesce(struct net_device *netdev,
u32 q_num)
{
const struct idpf_netdev_priv *np = netdev_priv(netdev);
+ struct idpf_adapter *adapter = np->adapter;
const struct idpf_vport *vport;
int err = 0;
- idpf_vport_ctrl_lock(netdev);
+ idpf_vport_ctrl_lock(adapter);
vport = idpf_netdev_to_vport(netdev);
if (np->state != __IDPF_VPORT_UP)
@@ -1048,7 +1051,7 @@ static int idpf_get_q_coalesce(struct net_device *netdev,
VIRTCHNL2_QUEUE_TYPE_TX);
unlock_mutex:
- idpf_vport_ctrl_unlock(netdev);
+ idpf_vport_ctrl_unlock(adapter);
return err;
}
@@ -1200,10 +1203,11 @@ static int idpf_set_coalesce(struct net_device *netdev,
struct netlink_ext_ack *extack)
{
struct idpf_netdev_priv *np = netdev_priv(netdev);
+ struct idpf_adapter *adapter = np->adapter;
struct idpf_vport *vport;
int i, err = 0;
- idpf_vport_ctrl_lock(netdev);
+ idpf_vport_ctrl_lock(adapter);
vport = idpf_netdev_to_vport(netdev);
if (np->state != __IDPF_VPORT_UP)
@@ -1222,7 +1226,7 @@ static int idpf_set_coalesce(struct net_device *netdev,
}
unlock_mutex:
- idpf_vport_ctrl_unlock(netdev);
+ idpf_vport_ctrl_unlock(adapter);
return err;
}
@@ -1238,22 +1242,23 @@ static int idpf_set_coalesce(struct net_device *netdev,
static int idpf_set_per_q_coalesce(struct net_device *netdev, u32 q_num,
struct ethtool_coalesce *ec)
{
+ struct idpf_adapter *adapter = idpf_netdev_to_adapter(netdev);
struct idpf_vport *vport;
int err;
- idpf_vport_ctrl_lock(netdev);
+ idpf_vport_ctrl_lock(adapter);
vport = idpf_netdev_to_vport(netdev);
err = idpf_set_q_coalesce(vport, ec, q_num, false);
if (err) {
- idpf_vport_ctrl_unlock(netdev);
+ idpf_vport_ctrl_unlock(adapter);
return err;
}
err = idpf_set_q_coalesce(vport, ec, q_num, true);
- idpf_vport_ctrl_unlock(netdev);
+ idpf_vport_ctrl_unlock(adapter);
return err;
}
diff --git a/drivers/net/ethernet/intel/idpf/idpf_lib.c b/drivers/net/ethernet/intel/idpf/idpf_lib.c
index b4fbb99bfad2..a870748a8be7 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_lib.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_lib.c
@@ -904,18 +904,18 @@ static void idpf_vport_stop(struct idpf_vport *vport)
*/
static int idpf_stop(struct net_device *netdev)
{
- struct idpf_netdev_priv *np = netdev_priv(netdev);
+ struct idpf_adapter *adapter = idpf_netdev_to_adapter(netdev);
struct idpf_vport *vport;
- if (test_bit(IDPF_REMOVE_IN_PROG, np->adapter->flags))
+ if (test_bit(IDPF_REMOVE_IN_PROG, adapter->flags))
return 0;
- idpf_vport_ctrl_lock(netdev);
+ idpf_vport_ctrl_lock(adapter);
vport = idpf_netdev_to_vport(netdev);
idpf_vport_stop(vport);
- idpf_vport_ctrl_unlock(netdev);
+ idpf_vport_ctrl_unlock(adapter);
return 0;
}
@@ -2098,16 +2098,14 @@ static int idpf_vport_manage_rss_lut(struct idpf_vport *vport)
static int idpf_set_features(struct net_device *netdev,
netdev_features_t features)
{
+ struct idpf_adapter *adapter = idpf_netdev_to_adapter(netdev);
netdev_features_t changed = netdev->features ^ features;
- struct idpf_adapter *adapter;
struct idpf_vport *vport;
int err = 0;
- idpf_vport_ctrl_lock(netdev);
+ idpf_vport_ctrl_lock(adapter);
vport = idpf_netdev_to_vport(netdev);
- adapter = vport->adapter;
-
if (idpf_is_reset_in_prog(adapter)) {
dev_err(&adapter->pdev->dev, "Device is resetting, changing netdev features temporarily unavailable.\n");
err = -EBUSY;
@@ -2134,7 +2132,7 @@ static int idpf_set_features(struct net_device *netdev,
}
unlock_mutex:
- idpf_vport_ctrl_unlock(netdev);
+ idpf_vport_ctrl_unlock(adapter);
return err;
}
@@ -2153,15 +2151,16 @@ static int idpf_set_features(struct net_device *netdev,
*/
static int idpf_open(struct net_device *netdev)
{
+ struct idpf_adapter *adapter = idpf_netdev_to_adapter(netdev);
struct idpf_vport *vport;
int err;
- idpf_vport_ctrl_lock(netdev);
+ idpf_vport_ctrl_lock(adapter);
vport = idpf_netdev_to_vport(netdev);
err = idpf_vport_open(vport);
- idpf_vport_ctrl_unlock(netdev);
+ idpf_vport_ctrl_unlock(adapter);
return err;
}
@@ -2175,17 +2174,18 @@ static int idpf_open(struct net_device *netdev)
*/
static int idpf_change_mtu(struct net_device *netdev, int new_mtu)
{
+ struct idpf_adapter *adapter = idpf_netdev_to_adapter(netdev);
struct idpf_vport *vport;
int err;
- idpf_vport_ctrl_lock(netdev);
+ idpf_vport_ctrl_lock(adapter);
vport = idpf_netdev_to_vport(netdev);
WRITE_ONCE(netdev->mtu, new_mtu);
err = idpf_initiate_soft_reset(vport, IDPF_SR_MTU_CHANGE);
- idpf_vport_ctrl_unlock(netdev);
+ idpf_vport_ctrl_unlock(adapter);
return err;
}
@@ -2261,23 +2261,24 @@ static netdev_features_t idpf_features_check(struct sk_buff *skb,
static int idpf_set_mac(struct net_device *netdev, void *p)
{
struct idpf_netdev_priv *np = netdev_priv(netdev);
+ struct idpf_adapter *adapter = np->adapter;
struct idpf_vport_config *vport_config;
struct sockaddr *addr = p;
struct idpf_vport *vport;
int err = 0;
- idpf_vport_ctrl_lock(netdev);
+ idpf_vport_ctrl_lock(adapter);
vport = idpf_netdev_to_vport(netdev);
- if (!idpf_is_cap_ena(vport->adapter, IDPF_OTHER_CAPS,
+ if (!idpf_is_cap_ena(adapter, IDPF_OTHER_CAPS,
VIRTCHNL2_CAP_MACFILTER)) {
- dev_info(&vport->adapter->pdev->dev, "Setting MAC address is not supported\n");
+ dev_info(&adapter->pdev->dev, "Setting MAC address is not supported\n");
err = -EOPNOTSUPP;
goto unlock_mutex;
}
if (!is_valid_ether_addr(addr->sa_data)) {
- dev_info(&vport->adapter->pdev->dev, "Invalid MAC address: %pM\n",
+ dev_info(&adapter->pdev->dev, "Invalid MAC address: %pM\n",
addr->sa_data);
err = -EADDRNOTAVAIL;
goto unlock_mutex;
@@ -2286,7 +2287,7 @@ static int idpf_set_mac(struct net_device *netdev, void *p)
if (ether_addr_equal(netdev->dev_addr, addr->sa_data))
goto unlock_mutex;
- vport_config = vport->adapter->vport_config[vport->idx];
+ vport_config = adapter->vport_config[vport->idx];
err = idpf_add_mac_filter(vport, np, addr->sa_data, false);
if (err) {
__idpf_del_mac_filter(vport_config, addr->sa_data);
@@ -2300,7 +2301,7 @@ static int idpf_set_mac(struct net_device *netdev, void *p)
eth_hw_addr_set(netdev, addr->sa_data);
unlock_mutex:
- idpf_vport_ctrl_unlock(netdev);
+ idpf_vport_ctrl_unlock(adapter);
return err;
}
--
2.46.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* RE: [PATCH iwl-net v1 1/4] idpf: Change function argument
2024-11-05 18:48 ` [PATCH iwl-net v1 1/4] idpf: Change function argument Tarun K Singh
@ 2025-01-14 6:53 ` Singh, Krishneil K
0 siblings, 0 replies; 13+ messages in thread
From: Singh, Krishneil K @ 2025-01-14 6:53 UTC (permalink / raw)
To: Tarun K Singh, intel-wired-lan@lists.osuosl.org; +Cc: netdev@vger.kernel.org
> -----Original Message-----
> From: Tarun K Singh <tarun.k.singh@intel.com>
> Sent: Tuesday, November 5, 2024 10:49 AM
> To: intel-wired-lan@lists.osuosl.org
> Cc: netdev@vger.kernel.org
> Subject: [PATCH iwl-net v1 1/4] idpf: Change function argument
>
> Change idpf_vport_ctrl_lock's argument from netdev to adapter.
>
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> Signed-off-by: Tarun K Singh <tarun.k.singh@intel.com>
> ---
> drivers/net/ethernet/intel/idpf/idpf.h | 16 ++---
> .../net/ethernet/intel/idpf/idpf_ethtool.c | 59 ++++++++++---------
> drivers/net/ethernet/intel/idpf/idpf_lib.c | 39 ++++++------
> 3 files changed, 58 insertions(+), 56 deletions(-)
>
Tested-by: Krishneil Singh <krishneil.k.singh@intel.com>
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH iwl-net v1 2/4] idpf: rename vport_ctrl_lock
2024-11-05 18:48 [PATCH iwl-net v1 0/4] fix locking issue Tarun K Singh
2024-11-05 18:48 ` [PATCH iwl-net v1 1/4] idpf: Change function argument Tarun K Singh
@ 2024-11-05 18:48 ` Tarun K Singh
2025-01-14 6:52 ` Singh, Krishneil K
2024-11-05 18:48 ` [PATCH iwl-net v1 3/4] idpf: Add init, reinit, and deinit control lock Tarun K Singh
` (2 subsequent siblings)
4 siblings, 1 reply; 13+ messages in thread
From: Tarun K Singh @ 2024-11-05 18:48 UTC (permalink / raw)
To: intel-wired-lan; +Cc: netdev
Rename 'vport_ctrl_lock' to 'vport_cfg_lock'. Rename related
functions name for lock and unlock.
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Signed-off-by: Tarun K Singh <tarun.k.singh@intel.com>
---
drivers/net/ethernet/intel/idpf/idpf.h | 16 +++----
.../net/ethernet/intel/idpf/idpf_ethtool.c | 46 +++++++++----------
drivers/net/ethernet/intel/idpf/idpf_lib.c | 24 +++++-----
drivers/net/ethernet/intel/idpf/idpf_main.c | 4 +-
4 files changed, 45 insertions(+), 45 deletions(-)
diff --git a/drivers/net/ethernet/intel/idpf/idpf.h b/drivers/net/ethernet/intel/idpf/idpf.h
index d87ed50af681..8dea2dd784af 100644
--- a/drivers/net/ethernet/intel/idpf/idpf.h
+++ b/drivers/net/ethernet/intel/idpf/idpf.h
@@ -526,7 +526,7 @@ struct idpf_vc_xn_manager;
* @crc_enable: Enable CRC insertion offload
* @req_tx_splitq: TX split or single queue model to request
* @req_rx_splitq: RX split or single queue model to request
- * @vport_ctrl_lock: Lock to protect the vport control flow
+ * @vport_cfg_lock: Lock to protect the vport config flow
* @vector_lock: Lock to protect vector distribution
* @queue_lock: Lock to protect queue distribution
* @vc_buf_lock: Lock to protect virtchnl buffer
@@ -583,7 +583,7 @@ struct idpf_adapter {
bool req_tx_splitq;
bool req_rx_splitq;
- struct mutex vport_ctrl_lock;
+ struct mutex vport_cfg_lock;
struct mutex vector_lock;
struct mutex queue_lock;
struct mutex vc_buf_lock;
@@ -787,24 +787,24 @@ static inline u16 idpf_get_max_tx_hdr_size(struct idpf_adapter *adapter)
}
/**
- * idpf_vport_ctrl_lock - Acquire the vport control lock
+ * idpf_vport_cfg_lock - Acquire the vport config lock
* @adapter: private data struct
*
* This lock should be used by non-datapath code to protect against vport
* destruction.
*/
-static inline void idpf_vport_ctrl_lock(struct idpf_adapter *adapter)
+static inline void idpf_vport_cfg_lock(struct idpf_adapter *adapter)
{
- mutex_lock(&adapter->vport_ctrl_lock);
+ mutex_lock(&adapter->vport_cfg_lock);
}
/**
- * idpf_vport_ctrl_unlock - Release the vport control lock
+ * idpf_vport_cfg_unlock - Release the vport config lock
* @adapter: private data struct
*/
-static inline void idpf_vport_ctrl_unlock(struct idpf_adapter *adapter)
+static inline void idpf_vport_cfg_unlock(struct idpf_adapter *adapter)
{
- mutex_unlock(&adapter->vport_ctrl_lock);
+ mutex_unlock(&adapter->vport_cfg_lock);
}
void idpf_statistics_task(struct work_struct *work);
diff --git a/drivers/net/ethernet/intel/idpf/idpf_ethtool.c b/drivers/net/ethernet/intel/idpf/idpf_ethtool.c
index e5ac3e5a50ce..b3ed1d9a80ae 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_ethtool.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_ethtool.c
@@ -17,20 +17,20 @@ static int idpf_get_rxnfc(struct net_device *netdev, struct ethtool_rxnfc *cmd,
struct idpf_adapter *adapter = idpf_netdev_to_adapter(netdev);
struct idpf_vport *vport;
- idpf_vport_ctrl_lock(adapter);
+ idpf_vport_cfg_lock(adapter);
vport = idpf_netdev_to_vport(netdev);
switch (cmd->cmd) {
case ETHTOOL_GRXRINGS:
cmd->data = vport->num_rxq;
- idpf_vport_ctrl_unlock(adapter);
+ idpf_vport_cfg_unlock(adapter);
return 0;
default:
break;
}
- idpf_vport_ctrl_unlock(adapter);
+ idpf_vport_cfg_unlock(adapter);
return -EOPNOTSUPP;
}
@@ -90,7 +90,7 @@ static int idpf_get_rxfh(struct net_device *netdev,
u16 i;
adapter = np->adapter;
- idpf_vport_ctrl_lock(adapter);
+ idpf_vport_cfg_lock(adapter);
if (!idpf_is_cap_ena_all(adapter, IDPF_RSS_CAPS, IDPF_CAP_RSS)) {
err = -EOPNOTSUPP;
@@ -112,7 +112,7 @@ static int idpf_get_rxfh(struct net_device *netdev,
}
unlock_mutex:
- idpf_vport_ctrl_unlock(adapter);
+ idpf_vport_cfg_unlock(adapter);
return err;
}
@@ -137,7 +137,7 @@ static int idpf_set_rxfh(struct net_device *netdev,
int err = 0;
u16 lut;
- idpf_vport_ctrl_lock(adapter);
+ idpf_vport_cfg_lock(adapter);
vport = idpf_netdev_to_vport(netdev);
if (!idpf_is_cap_ena_all(adapter, IDPF_RSS_CAPS, IDPF_CAP_RSS)) {
@@ -166,7 +166,7 @@ static int idpf_set_rxfh(struct net_device *netdev,
err = idpf_config_rss(vport);
unlock_mutex:
- idpf_vport_ctrl_unlock(adapter);
+ idpf_vport_cfg_unlock(adapter);
return err;
}
@@ -234,7 +234,7 @@ static int idpf_set_channels(struct net_device *netdev,
return -EINVAL;
}
- idpf_vport_ctrl_lock(adapter);
+ idpf_vport_cfg_lock(adapter);
vport = idpf_netdev_to_vport(netdev);
idx = vport->idx;
@@ -278,7 +278,7 @@ static int idpf_set_channels(struct net_device *netdev,
}
unlock_mutex:
- idpf_vport_ctrl_unlock(adapter);
+ idpf_vport_cfg_unlock(adapter);
return err;
}
@@ -301,7 +301,7 @@ static void idpf_get_ringparam(struct net_device *netdev,
struct idpf_adapter *adapter = idpf_netdev_to_adapter(netdev);
struct idpf_vport *vport;
- idpf_vport_ctrl_lock(adapter);
+ idpf_vport_cfg_lock(adapter);
vport = idpf_netdev_to_vport(netdev);
ring->rx_max_pending = IDPF_MAX_RXQ_DESC;
@@ -311,7 +311,7 @@ static void idpf_get_ringparam(struct net_device *netdev,
kring->tcp_data_split = idpf_vport_get_hsplit(vport);
- idpf_vport_ctrl_unlock(adapter);
+ idpf_vport_cfg_unlock(adapter);
}
/**
@@ -336,7 +336,7 @@ static int idpf_set_ringparam(struct net_device *netdev,
int i, err = 0;
u16 idx;
- idpf_vport_ctrl_lock(adapter);
+ idpf_vport_cfg_lock(adapter);
vport = idpf_netdev_to_vport(netdev);
idx = vport->idx;
@@ -395,7 +395,7 @@ static int idpf_set_ringparam(struct net_device *netdev,
err = idpf_initiate_soft_reset(vport, IDPF_SR_Q_DESC_CHANGE);
unlock_mutex:
- idpf_vport_ctrl_unlock(adapter);
+ idpf_vport_cfg_unlock(adapter);
return err;
}
@@ -878,11 +878,11 @@ static void idpf_get_ethtool_stats(struct net_device *netdev,
bool is_splitq;
u16 qtype;
- idpf_vport_ctrl_lock(adapter);
+ idpf_vport_cfg_lock(adapter);
vport = idpf_netdev_to_vport(netdev);
if (np->state != __IDPF_VPORT_UP) {
- idpf_vport_ctrl_unlock(adapter);
+ idpf_vport_cfg_unlock(adapter);
return;
}
@@ -949,7 +949,7 @@ static void idpf_get_ethtool_stats(struct net_device *netdev,
rcu_read_unlock();
- idpf_vport_ctrl_unlock(adapter);
+ idpf_vport_cfg_unlock(adapter);
}
/**
@@ -1031,7 +1031,7 @@ static int idpf_get_q_coalesce(struct net_device *netdev,
const struct idpf_vport *vport;
int err = 0;
- idpf_vport_ctrl_lock(adapter);
+ idpf_vport_cfg_lock(adapter);
vport = idpf_netdev_to_vport(netdev);
if (np->state != __IDPF_VPORT_UP)
@@ -1051,7 +1051,7 @@ static int idpf_get_q_coalesce(struct net_device *netdev,
VIRTCHNL2_QUEUE_TYPE_TX);
unlock_mutex:
- idpf_vport_ctrl_unlock(adapter);
+ idpf_vport_cfg_unlock(adapter);
return err;
}
@@ -1207,7 +1207,7 @@ static int idpf_set_coalesce(struct net_device *netdev,
struct idpf_vport *vport;
int i, err = 0;
- idpf_vport_ctrl_lock(adapter);
+ idpf_vport_cfg_lock(adapter);
vport = idpf_netdev_to_vport(netdev);
if (np->state != __IDPF_VPORT_UP)
@@ -1226,7 +1226,7 @@ static int idpf_set_coalesce(struct net_device *netdev,
}
unlock_mutex:
- idpf_vport_ctrl_unlock(adapter);
+ idpf_vport_cfg_unlock(adapter);
return err;
}
@@ -1246,19 +1246,19 @@ static int idpf_set_per_q_coalesce(struct net_device *netdev, u32 q_num,
struct idpf_vport *vport;
int err;
- idpf_vport_ctrl_lock(adapter);
+ idpf_vport_cfg_lock(adapter);
vport = idpf_netdev_to_vport(netdev);
err = idpf_set_q_coalesce(vport, ec, q_num, false);
if (err) {
- idpf_vport_ctrl_unlock(adapter);
+ idpf_vport_cfg_unlock(adapter);
return err;
}
err = idpf_set_q_coalesce(vport, ec, q_num, true);
- idpf_vport_ctrl_unlock(adapter);
+ idpf_vport_cfg_unlock(adapter);
return err;
}
diff --git a/drivers/net/ethernet/intel/idpf/idpf_lib.c b/drivers/net/ethernet/intel/idpf/idpf_lib.c
index a870748a8be7..778dc71fbf4a 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_lib.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_lib.c
@@ -910,12 +910,12 @@ static int idpf_stop(struct net_device *netdev)
if (test_bit(IDPF_REMOVE_IN_PROG, adapter->flags))
return 0;
- idpf_vport_ctrl_lock(adapter);
+ idpf_vport_cfg_lock(adapter);
vport = idpf_netdev_to_vport(netdev);
idpf_vport_stop(vport);
- idpf_vport_ctrl_unlock(adapter);
+ idpf_vport_cfg_unlock(adapter);
return 0;
}
@@ -1733,7 +1733,7 @@ static int idpf_init_hard_reset(struct idpf_adapter *adapter)
int err;
u16 i;
- mutex_lock(&adapter->vport_ctrl_lock);
+ mutex_lock(&adapter->vport_cfg_lock);
dev_info(dev, "Device HW Reset initiated\n");
@@ -1798,7 +1798,7 @@ static int idpf_init_hard_reset(struct idpf_adapter *adapter)
msleep(100);
unlock_mutex:
- mutex_unlock(&adapter->vport_ctrl_lock);
+ mutex_unlock(&adapter->vport_cfg_lock);
return err;
}
@@ -2103,7 +2103,7 @@ static int idpf_set_features(struct net_device *netdev,
struct idpf_vport *vport;
int err = 0;
- idpf_vport_ctrl_lock(adapter);
+ idpf_vport_cfg_lock(adapter);
vport = idpf_netdev_to_vport(netdev);
if (idpf_is_reset_in_prog(adapter)) {
@@ -2132,7 +2132,7 @@ static int idpf_set_features(struct net_device *netdev,
}
unlock_mutex:
- idpf_vport_ctrl_unlock(adapter);
+ idpf_vport_cfg_unlock(adapter);
return err;
}
@@ -2155,12 +2155,12 @@ static int idpf_open(struct net_device *netdev)
struct idpf_vport *vport;
int err;
- idpf_vport_ctrl_lock(adapter);
+ idpf_vport_cfg_lock(adapter);
vport = idpf_netdev_to_vport(netdev);
err = idpf_vport_open(vport);
- idpf_vport_ctrl_unlock(adapter);
+ idpf_vport_cfg_unlock(adapter);
return err;
}
@@ -2178,14 +2178,14 @@ static int idpf_change_mtu(struct net_device *netdev, int new_mtu)
struct idpf_vport *vport;
int err;
- idpf_vport_ctrl_lock(adapter);
+ idpf_vport_cfg_lock(adapter);
vport = idpf_netdev_to_vport(netdev);
WRITE_ONCE(netdev->mtu, new_mtu);
err = idpf_initiate_soft_reset(vport, IDPF_SR_MTU_CHANGE);
- idpf_vport_ctrl_unlock(adapter);
+ idpf_vport_cfg_unlock(adapter);
return err;
}
@@ -2267,7 +2267,7 @@ static int idpf_set_mac(struct net_device *netdev, void *p)
struct idpf_vport *vport;
int err = 0;
- idpf_vport_ctrl_lock(adapter);
+ idpf_vport_cfg_lock(adapter);
vport = idpf_netdev_to_vport(netdev);
if (!idpf_is_cap_ena(adapter, IDPF_OTHER_CAPS,
@@ -2301,7 +2301,7 @@ static int idpf_set_mac(struct net_device *netdev, void *p)
eth_hw_addr_set(netdev, addr->sa_data);
unlock_mutex:
- idpf_vport_ctrl_unlock(adapter);
+ idpf_vport_cfg_unlock(adapter);
return err;
}
diff --git a/drivers/net/ethernet/intel/idpf/idpf_main.c b/drivers/net/ethernet/intel/idpf/idpf_main.c
index db476b3314c8..0522b3a6f42c 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_main.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_main.c
@@ -72,7 +72,7 @@ static void idpf_remove(struct pci_dev *pdev)
kfree(adapter->vcxn_mngr);
adapter->vcxn_mngr = NULL;
- mutex_destroy(&adapter->vport_ctrl_lock);
+ mutex_destroy(&adapter->vport_cfg_lock);
mutex_destroy(&adapter->vector_lock);
mutex_destroy(&adapter->queue_lock);
mutex_destroy(&adapter->vc_buf_lock);
@@ -229,7 +229,7 @@ static int idpf_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
goto err_cfg_hw;
}
- mutex_init(&adapter->vport_ctrl_lock);
+ mutex_init(&adapter->vport_cfg_lock);
mutex_init(&adapter->vector_lock);
mutex_init(&adapter->queue_lock);
mutex_init(&adapter->vc_buf_lock);
--
2.46.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* RE: [PATCH iwl-net v1 2/4] idpf: rename vport_ctrl_lock
2024-11-05 18:48 ` [PATCH iwl-net v1 2/4] idpf: rename vport_ctrl_lock Tarun K Singh
@ 2025-01-14 6:52 ` Singh, Krishneil K
0 siblings, 0 replies; 13+ messages in thread
From: Singh, Krishneil K @ 2025-01-14 6:52 UTC (permalink / raw)
To: Tarun K Singh, intel-wired-lan@lists.osuosl.org; +Cc: netdev@vger.kernel.org
> -----Original Message-----
> From: Tarun K Singh <tarun.k.singh@intel.com>
> Sent: Tuesday, November 5, 2024 10:49 AM
> To: intel-wired-lan@lists.osuosl.org
> Cc: netdev@vger.kernel.org
> Subject: [PATCH iwl-net v1 2/4] idpf: rename vport_ctrl_lock
>
> Rename 'vport_ctrl_lock' to 'vport_cfg_lock'. Rename related
> functions name for lock and unlock.
>
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> Signed-off-by: Tarun K Singh <tarun.k.singh@intel.com>
> ---
> drivers/net/ethernet/intel/idpf/idpf.h | 16 +++----
> .../net/ethernet/intel/idpf/idpf_ethtool.c | 46 +++++++++----------
> drivers/net/ethernet/intel/idpf/idpf_lib.c | 24 +++++-----
> drivers/net/ethernet/intel/idpf/idpf_main.c | 4 +-
> 4 files changed, 45 insertions(+), 45 deletions(-)
>
Tested-by: Krishneil Singh <krishneil.k.singh@intel.com>
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH iwl-net v1 3/4] idpf: Add init, reinit, and deinit control lock
2024-11-05 18:48 [PATCH iwl-net v1 0/4] fix locking issue Tarun K Singh
2024-11-05 18:48 ` [PATCH iwl-net v1 1/4] idpf: Change function argument Tarun K Singh
2024-11-05 18:48 ` [PATCH iwl-net v1 2/4] idpf: rename vport_ctrl_lock Tarun K Singh
@ 2024-11-05 18:48 ` Tarun K Singh
2025-01-14 6:50 ` [Intel-wired-lan] " Singh, Krishneil K
2024-11-05 18:48 ` [PATCH iwl-net v1 4/4] idpf: add lock class key Tarun K Singh
2025-01-27 6:06 ` [PATCH iwl-net v1 0/4] fix locking issue Singh, Krishneil K
4 siblings, 1 reply; 13+ messages in thread
From: Tarun K Singh @ 2024-11-05 18:48 UTC (permalink / raw)
To: intel-wired-lan; +Cc: netdev
Add new 'vport_init_lock' to prevent locking issue.
The existing 'vport_cfg_lock' was controlling the vport initialization,
re-initialization due to reset, and de-initialization of code flow.
In addition to controlling the above behavior it was also controlling
the parallel netdevice calls such as open/close from Linux OS, which
can happen independent of re-init or de-init of the vport(s). If first
one such as re-init or de-init is going on then the second operation
to config the netdevice with OS should not take place. The first
operation (init or de-init) takes the precedence if both are to happen
simultaneously.
Use of single lock cause deadlock and inconsistent behavior of code
flow. E.g. when driver undergoes reset via 'idpf_init_hard_reset', it
acquires the 'vport_cfg_lock', and during this process it tries to
unregister netdevice which may call 'idpf_stop' which tries to acquire
same lock causing it to deadlock.
To address above, add new lock 'vport_init_lock' which control the
initialization, re-initialization, and de-initialization flow.
The 'vport_cfg_lock' now exclusively controls the vport config
operations.
Add vport config lock around 'idpf_vport_stop()' and 'idpf_vport_open()'
to protect close and open operation at the same time.
Add vport init lock around 'idpf_sriv_configure()' to protect it from
load and removal path. To accomplish it, use existing function
as wrapper and introduce another function 'idpf_sriov_config_vfs'
which is used inside the lock.
In idpf_remove, change 'idpf_sriov_configure' to
'idpf_sriov_config_vfs', and move inside the init lock.
Use these two locks in the following precedence:
'vport_init_lock' first, then 'vport_cfg_lock'.
Fixes: 8077c727561a ("idpf: add controlq init and reset checks")
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Reviewed-by: Madhu Chittim <madhu.chittim@intel.com>
Signed-off-by: Tarun K Singh <tarun.k.singh@intel.com>
---
drivers/net/ethernet/intel/idpf/idpf.h | 25 +++++++++++++
drivers/net/ethernet/intel/idpf/idpf_lib.c | 41 ++++++++++++++++++---
drivers/net/ethernet/intel/idpf/idpf_main.c | 7 +++-
3 files changed, 67 insertions(+), 6 deletions(-)
diff --git a/drivers/net/ethernet/intel/idpf/idpf.h b/drivers/net/ethernet/intel/idpf/idpf.h
index 8dea2dd784af..34dbdc7d6c88 100644
--- a/drivers/net/ethernet/intel/idpf/idpf.h
+++ b/drivers/net/ethernet/intel/idpf/idpf.h
@@ -526,6 +526,7 @@ struct idpf_vc_xn_manager;
* @crc_enable: Enable CRC insertion offload
* @req_tx_splitq: TX split or single queue model to request
* @req_rx_splitq: RX split or single queue model to request
+ * @vport_init_lock: Lock to protect vport init, re-init, and deinit flow
* @vport_cfg_lock: Lock to protect the vport config flow
* @vector_lock: Lock to protect vector distribution
* @queue_lock: Lock to protect queue distribution
@@ -583,6 +584,7 @@ struct idpf_adapter {
bool req_tx_splitq;
bool req_rx_splitq;
+ struct mutex vport_init_lock;
struct mutex vport_cfg_lock;
struct mutex vector_lock;
struct mutex queue_lock;
@@ -786,6 +788,28 @@ static inline u16 idpf_get_max_tx_hdr_size(struct idpf_adapter *adapter)
return le16_to_cpu(adapter->caps.max_tx_hdr_size);
}
+/**
+ * idpf_vport_init_lock -acquire init/deinit control lock
+ * @adapter: private data struct
+ *
+ * It controls and protect vport initialization, re-initialization,
+ * and deinitialization code flow and its resources. This
+ * lock is only used by non-datapath code.
+ */
+static inline void idpf_vport_init_lock(struct idpf_adapter *adapter)
+{
+ mutex_lock(&adapter->vport_init_lock);
+}
+
+/**
+ * idpf_vport_init_unlock - release vport init lock
+ * @adapter: private data struct
+ */
+static inline void idpf_vport_init_unlock(struct idpf_adapter *adapter)
+{
+ mutex_unlock(&adapter->vport_init_lock);
+}
+
/**
* idpf_vport_cfg_lock - Acquire the vport config lock
* @adapter: private data struct
@@ -827,6 +851,7 @@ void idpf_set_ethtool_ops(struct net_device *netdev);
void idpf_vport_intr_write_itr(struct idpf_q_vector *q_vector,
u16 itr, bool tx);
int idpf_sriov_configure(struct pci_dev *pdev, int num_vfs);
+int idpf_sriov_config_vfs(struct pci_dev *pdev, int num_vfs);
u8 idpf_vport_get_hsplit(const struct idpf_vport *vport);
bool idpf_vport_set_hsplit(const struct idpf_vport *vport, u8 val);
diff --git a/drivers/net/ethernet/intel/idpf/idpf_lib.c b/drivers/net/ethernet/intel/idpf/idpf_lib.c
index 778dc71fbf4a..931d0f988c95 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_lib.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_lib.c
@@ -1000,7 +1000,10 @@ static void idpf_vport_dealloc(struct idpf_vport *vport)
unsigned int i = vport->idx;
idpf_deinit_mac_addr(vport);
+
+ idpf_vport_cfg_lock(adapter);
idpf_vport_stop(vport);
+ idpf_vport_cfg_unlock(adapter);
if (!test_bit(IDPF_HR_RESET_IN_PROG, adapter->flags))
idpf_decfg_netdev(vport);
@@ -1522,8 +1525,11 @@ void idpf_init_task(struct work_struct *work)
/* Once state is put into DOWN, driver is ready for dev_open */
np = netdev_priv(vport->netdev);
np->state = __IDPF_VPORT_DOWN;
- if (test_and_clear_bit(IDPF_VPORT_UP_REQUESTED, vport_config->flags))
+ if (test_and_clear_bit(IDPF_VPORT_UP_REQUESTED, vport_config->flags)) {
+ idpf_vport_cfg_lock(adapter);
idpf_vport_open(vport);
+ idpf_vport_cfg_unlock(adapter);
+ }
/* Spawn and return 'idpf_init_task' work queue until all the
* default vports are created
@@ -1601,17 +1607,19 @@ static int idpf_sriov_ena(struct idpf_adapter *adapter, int num_vfs)
}
/**
- * idpf_sriov_configure - Configure the requested VFs
+ * idpf_sriov_config_vfs - Configure the requested VFs
* @pdev: pointer to a pci_dev structure
* @num_vfs: number of vfs to allocate
*
* Enable or change the number of VFs. Called when the user updates the number
* of VFs in sysfs.
**/
-int idpf_sriov_configure(struct pci_dev *pdev, int num_vfs)
+int idpf_sriov_config_vfs(struct pci_dev *pdev, int num_vfs)
{
struct idpf_adapter *adapter = pci_get_drvdata(pdev);
+ lockdep_assert_held(&adapter->vport_init_lock);
+
if (!idpf_is_cap_ena(adapter, IDPF_OTHER_CAPS, VIRTCHNL2_CAP_SRIOV)) {
dev_info(&pdev->dev, "SR-IOV is not supported on this device\n");
@@ -1634,6 +1642,26 @@ int idpf_sriov_configure(struct pci_dev *pdev, int num_vfs)
return 0;
}
+/**
+ * idpf_sriov_configure - Call idpf_sriov_config_vfs to configure
+ * @pdev: pointer to a pci_dev structure
+ * @num_vfs: number of vfs to allocate
+ *
+ * Enable or change the number of VFs. Called when the user updates the number
+ * of VFs in sysfs.
+ **/
+int idpf_sriov_configure(struct pci_dev *pdev, int num_vfs)
+{
+ struct idpf_adapter *adapter = pci_get_drvdata(pdev);
+ int ret;
+
+ idpf_vport_init_lock(adapter);
+ ret = idpf_sriov_config_vfs(pdev, num_vfs);
+ idpf_vport_init_unlock(adapter);
+
+ return ret;
+}
+
/**
* idpf_deinit_task - Device deinit routine
* @adapter: Driver specific private structure
@@ -1733,7 +1761,7 @@ static int idpf_init_hard_reset(struct idpf_adapter *adapter)
int err;
u16 i;
- mutex_lock(&adapter->vport_cfg_lock);
+ idpf_vport_init_lock(adapter);
dev_info(dev, "Device HW Reset initiated\n");
@@ -1798,7 +1826,7 @@ static int idpf_init_hard_reset(struct idpf_adapter *adapter)
msleep(100);
unlock_mutex:
- mutex_unlock(&adapter->vport_cfg_lock);
+ idpf_vport_init_unlock(adapter);
return err;
}
@@ -2155,6 +2183,9 @@ static int idpf_open(struct net_device *netdev)
struct idpf_vport *vport;
int err;
+ if (test_bit(IDPF_REMOVE_IN_PROG, adapter->flags))
+ return 0;
+
idpf_vport_cfg_lock(adapter);
vport = idpf_netdev_to_vport(netdev);
diff --git a/drivers/net/ethernet/intel/idpf/idpf_main.c b/drivers/net/ethernet/intel/idpf/idpf_main.c
index 0522b3a6f42c..04bbc048c829 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_main.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_main.c
@@ -28,10 +28,13 @@ static void idpf_remove(struct pci_dev *pdev)
* end up in bad state.
*/
cancel_delayed_work_sync(&adapter->vc_event_task);
+
+ idpf_vport_init_lock(adapter);
if (adapter->num_vfs)
- idpf_sriov_configure(pdev, 0);
+ idpf_sriov_config_vfs(pdev, 0);
idpf_vc_core_deinit(adapter);
+ idpf_vport_init_unlock(adapter);
/* Be a good citizen and leave the device clean on exit */
adapter->dev_ops.reg_ops.trigger_reset(adapter, IDPF_HR_FUNC_RESET);
@@ -72,6 +75,7 @@ static void idpf_remove(struct pci_dev *pdev)
kfree(adapter->vcxn_mngr);
adapter->vcxn_mngr = NULL;
+ mutex_destroy(&adapter->vport_init_lock);
mutex_destroy(&adapter->vport_cfg_lock);
mutex_destroy(&adapter->vector_lock);
mutex_destroy(&adapter->queue_lock);
@@ -229,6 +233,7 @@ static int idpf_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
goto err_cfg_hw;
}
+ mutex_init(&adapter->vport_init_lock);
mutex_init(&adapter->vport_cfg_lock);
mutex_init(&adapter->vector_lock);
mutex_init(&adapter->queue_lock);
--
2.46.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* RE: [Intel-wired-lan] [PATCH iwl-net v1 3/4] idpf: Add init, reinit, and deinit control lock
2024-11-05 18:48 ` [PATCH iwl-net v1 3/4] idpf: Add init, reinit, and deinit control lock Tarun K Singh
@ 2025-01-14 6:50 ` Singh, Krishneil K
0 siblings, 0 replies; 13+ messages in thread
From: Singh, Krishneil K @ 2025-01-14 6:50 UTC (permalink / raw)
To: Tarun K Singh, intel-wired-lan@lists.osuosl.org; +Cc: netdev@vger.kernel.org
> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Tarun K Singh
> Sent: Tuesday, November 5, 2024 10:49 AM
> To: intel-wired-lan@lists.osuosl.org
> Cc: netdev@vger.kernel.org
> Subject: [Intel-wired-lan] [PATCH iwl-net v1 3/4] idpf: Add init, reinit, and
> deinit control lock
>
> Add new 'vport_init_lock' to prevent locking issue.
>
> The existing 'vport_cfg_lock' was controlling the vport initialization,
> re-initialization due to reset, and de-initialization of code flow.
> In addition to controlling the above behavior it was also controlling
> the parallel netdevice calls such as open/close from Linux OS, which
> can happen independent of re-init or de-init of the vport(s). If first
> one such as re-init or de-init is going on then the second operation
> to config the netdevice with OS should not take place. The first
> operation (init or de-init) takes the precedence if both are to happen
> simultaneously.
>
> Use of single lock cause deadlock and inconsistent behavior of code
> flow. E.g. when driver undergoes reset via 'idpf_init_hard_reset', it
> acquires the 'vport_cfg_lock', and during this process it tries to
> unregister netdevice which may call 'idpf_stop' which tries to acquire
> same lock causing it to deadlock.
>
> To address above, add new lock 'vport_init_lock' which control the
> initialization, re-initialization, and de-initialization flow.
> The 'vport_cfg_lock' now exclusively controls the vport config
> operations.
>
> Add vport config lock around 'idpf_vport_stop()' and 'idpf_vport_open()'
> to protect close and open operation at the same time.
>
> Add vport init lock around 'idpf_sriv_configure()' to protect it from
> load and removal path. To accomplish it, use existing function
> as wrapper and introduce another function 'idpf_sriov_config_vfs'
> which is used inside the lock.
>
> In idpf_remove, change 'idpf_sriov_configure' to
> 'idpf_sriov_config_vfs', and move inside the init lock.
>
> Use these two locks in the following precedence:
> 'vport_init_lock' first, then 'vport_cfg_lock'.
>
> Fixes: 8077c727561a ("idpf: add controlq init and reset checks")
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> Reviewed-by: Madhu Chittim <madhu.chittim@intel.com>
> Signed-off-by: Tarun K Singh <tarun.k.singh@intel.com>
> ---
> drivers/net/ethernet/intel/idpf/idpf.h | 25 +++++++++++++
> drivers/net/ethernet/intel/idpf/idpf_lib.c | 41 ++++++++++++++++++---
> drivers/net/ethernet/intel/idpf/idpf_main.c | 7 +++-
> 3 files changed, 67 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/idpf/idpf.h
> b/drivers/net/ethernet/intel/idpf/idpf.h
> index 8dea2dd784af..34dbdc7d6c88 100644
> --- a/drivers/net/ethernet/intel/idpf/idpf.h
> +++ b/drivers/net/ethernet/intel/idpf/idpf.h
Tested-by: Krishneil Singh <krishneil.k.singh@intel.com>
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH iwl-net v1 4/4] idpf: add lock class key
2024-11-05 18:48 [PATCH iwl-net v1 0/4] fix locking issue Tarun K Singh
` (2 preceding siblings ...)
2024-11-05 18:48 ` [PATCH iwl-net v1 3/4] idpf: Add init, reinit, and deinit control lock Tarun K Singh
@ 2024-11-05 18:48 ` Tarun K Singh
2025-01-14 6:50 ` [Intel-wired-lan] " Singh, Krishneil K
2025-01-27 6:06 ` [PATCH iwl-net v1 0/4] fix locking issue Singh, Krishneil K
4 siblings, 1 reply; 13+ messages in thread
From: Tarun K Singh @ 2024-11-05 18:48 UTC (permalink / raw)
To: intel-wired-lan; +Cc: netdev
From: Ahmed Zaki <ahmed.zaki@intel.com>
Add lock class key changes to prevent lockdep from complaining
when PF reset the VFs.
Signed-off-by: Ahmed Zaki <ahmed.zaki@intel.com>
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Reviewed-by: Madhu Chittim <madhu.chittim@intel.com>
Signed-off-by: Tarun K Singh <tarun.k.singh@intel.com>
---
drivers/net/ethernet/intel/idpf/idpf_main.c | 32 +++++++++++++--------
1 file changed, 20 insertions(+), 12 deletions(-)
diff --git a/drivers/net/ethernet/intel/idpf/idpf_main.c b/drivers/net/ethernet/intel/idpf/idpf_main.c
index 04bbc048c829..082026c2a7ab 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_main.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_main.c
@@ -11,6 +11,10 @@ MODULE_DESCRIPTION(DRV_SUMMARY);
MODULE_IMPORT_NS(LIBETH);
MODULE_LICENSE("GPL");
+/* Prevent lockdep from complaining when PF reset the VFs */
+static struct lock_class_key idpf_pf_vport_init_lock_key;
+static struct lock_class_key idpf_pf_work_lock_key;
+
/**
* idpf_remove - Device removal routine
* @pdev: PCI device information struct
@@ -140,9 +144,25 @@ static int idpf_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
adapter->req_tx_splitq = true;
adapter->req_rx_splitq = true;
+ mutex_init(&adapter->vport_init_lock);
+ mutex_init(&adapter->vport_cfg_lock);
+ mutex_init(&adapter->vector_lock);
+ mutex_init(&adapter->queue_lock);
+ mutex_init(&adapter->vc_buf_lock);
+
+ INIT_DELAYED_WORK(&adapter->init_task, idpf_init_task);
+ INIT_DELAYED_WORK(&adapter->serv_task, idpf_service_task);
+ INIT_DELAYED_WORK(&adapter->mbx_task, idpf_mbx_task);
+ INIT_DELAYED_WORK(&adapter->stats_task, idpf_statistics_task);
+ INIT_DELAYED_WORK(&adapter->vc_event_task, idpf_vc_event_task);
+
switch (ent->device) {
case IDPF_DEV_ID_PF:
idpf_dev_ops_init(adapter);
+ lockdep_set_class(&adapter->vport_init_lock,
+ &idpf_pf_vport_init_lock_key);
+ lockdep_init_map(&adapter->vc_event_task.work.lockdep_map,
+ "idpf-PF-vc-work", &idpf_pf_work_lock_key, 0);
break;
case IDPF_DEV_ID_VF:
idpf_vf_dev_ops_init(adapter);
@@ -233,18 +253,6 @@ static int idpf_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
goto err_cfg_hw;
}
- mutex_init(&adapter->vport_init_lock);
- mutex_init(&adapter->vport_cfg_lock);
- mutex_init(&adapter->vector_lock);
- mutex_init(&adapter->queue_lock);
- mutex_init(&adapter->vc_buf_lock);
-
- INIT_DELAYED_WORK(&adapter->init_task, idpf_init_task);
- INIT_DELAYED_WORK(&adapter->serv_task, idpf_service_task);
- INIT_DELAYED_WORK(&adapter->mbx_task, idpf_mbx_task);
- INIT_DELAYED_WORK(&adapter->stats_task, idpf_statistics_task);
- INIT_DELAYED_WORK(&adapter->vc_event_task, idpf_vc_event_task);
-
adapter->dev_ops.reg_ops.reset_reg_init(adapter);
set_bit(IDPF_HR_DRV_LOAD, adapter->flags);
queue_delayed_work(adapter->vc_event_wq, &adapter->vc_event_task,
--
2.46.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* RE: [Intel-wired-lan] [PATCH iwl-net v1 4/4] idpf: add lock class key
2024-11-05 18:48 ` [PATCH iwl-net v1 4/4] idpf: add lock class key Tarun K Singh
@ 2025-01-14 6:50 ` Singh, Krishneil K
0 siblings, 0 replies; 13+ messages in thread
From: Singh, Krishneil K @ 2025-01-14 6:50 UTC (permalink / raw)
To: Tarun K Singh, intel-wired-lan@lists.osuosl.org; +Cc: netdev@vger.kernel.org
> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Tarun K Singh
> Sent: Tuesday, November 5, 2024 10:49 AM
> To: intel-wired-lan@lists.osuosl.org
> Cc: netdev@vger.kernel.org
> Subject: [Intel-wired-lan] [PATCH iwl-net v1 4/4] idpf: add lock class key
>
> From: Ahmed Zaki <ahmed.zaki@intel.com>
>
> Add lock class key changes to prevent lockdep from complaining
> when PF reset the VFs.
>
> Signed-off-by: Ahmed Zaki <ahmed.zaki@intel.com>
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> Reviewed-by: Madhu Chittim <madhu.chittim@intel.com>
> Signed-off-by: Tarun K Singh <tarun.k.singh@intel.com>
> ---
> drivers/net/ethernet/intel/idpf/idpf_main.c | 32 +++++++++++++--------
> 1 file changed, 20 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/idpf/idpf_main.c
> b/drivers/net/ethernet/intel/idpf/idpf_main.c
> index 04bbc048c829..082026c2a7ab 100644
> --- a/drivers/net/ethernet/intel/idpf/idpf_main.c
> +++ b/drivers/net/ethernet/intel/idpf/idpf_main.c
Tested-by: Krishneil Singh <krishneil.k.singh@intel.com>
^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH iwl-net v1 0/4] fix locking issue
2024-11-05 18:48 [PATCH iwl-net v1 0/4] fix locking issue Tarun K Singh
` (3 preceding siblings ...)
2024-11-05 18:48 ` [PATCH iwl-net v1 4/4] idpf: add lock class key Tarun K Singh
@ 2025-01-27 6:06 ` Singh, Krishneil K
2025-01-27 8:11 ` Przemek Kitszel
4 siblings, 1 reply; 13+ messages in thread
From: Singh, Krishneil K @ 2025-01-27 6:06 UTC (permalink / raw)
To: Tarun K Singh, intel-wired-lan@lists.osuosl.org; +Cc: netdev@vger.kernel.org
> -----Original Message-----
> From: Tarun K Singh <tarun.k.singh@intel.com>
> Sent: Tuesday, November 5, 2024 10:49 AM
> To: intel-wired-lan@lists.osuosl.org
> Cc: netdev@vger.kernel.org
> Subject: [PATCH iwl-net v1 0/4] fix locking issue
>
> This series fix deadlock issues in the driver. The first patch changes
> argument of function 'idpf_vport_ctrl_lock' to adapter. The second patch
> renames 'vport_ctrl_lock' to 'vport_cfg_lock'. The first 2 patches make the
> third patch easier to review. The third patch fixes the locking issue,
> and the fourth patch prevents lockdep from complaining.
>
> Ahmed Zaki (1):
> idpf: add lock class key
>
> Tarun K Singh (3):
> idpf: Change function argument
> idpf: rename vport_ctrl_lock
> idpf: Add init, reinit, and deinit control lock
>
> drivers/net/ethernet/intel/idpf/idpf.h | 49 ++++++++----
> .../net/ethernet/intel/idpf/idpf_ethtool.c | 59 +++++++-------
> drivers/net/ethernet/intel/idpf/idpf_lib.c | 80 +++++++++++++------
> drivers/net/ethernet/intel/idpf/idpf_main.c | 39 ++++++---
> 4 files changed, 149 insertions(+), 78 deletions(-)
>
> --
> 2.46.0
>
After additional testing on this [series|patch] we discovered an issue after doing ifdown while device is in a reset, which can result in a <null pointer dereference(?)>.
kernel: idpf 0000:af:00.0: resetting
kernel: idpf 0000:af:00.0: reset done
kernel: idpf 0000:af:00.0: HW reset detected
kernel: idpf 0000:af:00.0: Device HW Reset initiated
kernel: BUG: kernel NULL pointer dereference, address: 00000000000001a0
kernel: #PF: supervisor write access in kernel mode
kernel: #PF: error_code(0x0002) - not-present page
kernel: PGD 0 P4D 0
kernel: Oops: Oops: 0002 [#1] PREEMPT SMP NOPTI
kernel: CPU: 77 UID: 0 PID: 303278 Comm: ip Not tainted 6.13.0-rc7-p18.g5879fe0+ #1
kernel: Hardware name: Intel Corporation S2600WFT/S2600WFT, BIOS SE5C620.86B.02.01.0015.032120220358 03/21/2022
kernel: RIP: 0010:mutex_lock+0x1c/0x30
kernel: Code: 90 90 90 90 90 90 90 90 90 90 90 90 90 90 f3 0f 1e fa 0f 1f 44 00 00 55 48 89 fd e8 ce e7 ff ff 65 48 8b 15 c6 7b 20 6f 31 c0 <f0> 48 0f b1 55 00 75 06 5d c3 cc cc cc cc 48 89 ef 5d eb b0 90 90
kernel: RSP: 0018:ffffb73e43907768 EFLAGS: 00010246
kernel: RAX: 0000000000000000 RBX: ffff8df786a37000 RCX: 0000000000000000
kernel: RDX: ffff8de876922f80 RSI: 00000000fffffe00 RDI: 00000000000001a0
kernel: RBP: 00000000000001a0 R08: ffffb73e439076f8 R09: 0000000000000000
kernel: R10: ffff8de823b5e000 R11: 0000000000000100 R12: 0000000000000000
kernel: R13: ffff8df786a37188 R14: 0000000000000001 R15: 0000000000000001
kernel: FS: 00007fe6f6a30740(0000) GS:ffff8df73fe80000(0000) knlGS:0000000000000000
kernel: CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
kernel: CR2: 00000000000001a0 CR3: 000000011bb58006 CR4: 00000000007726f0
kernel: DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
kernel: DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
kernel: PKRU: 55555554
kernel: Call Trace:
kernel: <TASK>
kernel: ? __die+0x24/0x70
kernel: ? page_fault_oops+0x158/0x4f0
kernel: ? exc_page_fault+0x77/0x190
kernel: ? asm_exc_page_fault+0x26/0x30
kernel: ? mutex_lock+0x1c/0x30
kernel: ? mutex_lock+0x12/0x30
kernel: idpf_stop+0x39/0x60 [idpf]
kernel: __dev_close_many+0xa9/0x150
kernel: __dev_change_flags+0x114/0x250
kernel: dev_change_flags+0x25/0x60
kernel: do_setlink.constprop.0+0x215/0x1030
kernel: ? avc_has_perm_noaudit+0x6b/0xf0
kernel: ? cred_has_capability.isra.0+0x78/0x120
kernel: ? security_capable+0x58/0x90
kernel: rtnl_newlink+0x7c2/0xb90
kernel: ? avc_has_perm_noaudit+0x6b/0xf0
kernel: ? cred_has_capability.isra.0+0x78/0x120
kernel: ? rtnl_lock_killable+0x3/0x20
kernel: ? netdev_run_todo+0x6f/0x590
kernel: ? __pfx_rtnl_newlink+0x10/0x10
kernel: rtnetlink_rcv_msg+0x361/0x410
kernel: ? __pfx_rtnetlink_rcv_msg+0x10/0x10
kernel: netlink_rcv_skb+0x58/0x110
kernel: netlink_unicast+0x247/0x370
kernel: netlink_sendmsg+0x1fa/0x440
kernel: ____sys_sendmsg+0x3ac/0x3e0
kernel: ? import_iovec+0x1a/0x20
kernel: ? copy_msghdr_from_user+0x6d/0xa0
kernel: ___sys_sendmsg+0x88/0xd0
kernel: ? set_ptes.constprop.0+0x28/0x90
kernel: ? __memcg_slab_free_hook+0xd9/0x120
kernel: __sys_sendmsg+0x6c/0xc0
kernel: do_syscall_64+0x64/0x170
kernel: entry_SYSCALL_64_after_hwframe+0x76/0x7e
kernel: RIP: 0033:0x7fe6f6b5cc94
kernel: Code: 15 a1 41 0c 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b7 0f 1f 00 f3 0f 1e fa 80 3d 6d c9 0c 00 00 74 13 b8 2e 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 54 c3 0f 1f 00 48 83 ec 28 89 54 24 1c 48 89
kernel: RSP: 002b:00007ffe89733418 EFLAGS: 00000202 ORIG_RAX: 000000000000002e
kernel: RAX: ffffffffffffffda RBX: 0000000000000003 RCX: 00007fe6f6b5cc94
kernel: RDX: 0000000000000000 RSI: 00007ffe89733480 RDI: 0000000000000003
kernel: RBP: 0000000000000003 R08: 0000000067941450 R09: 00005560018342a0
kernel: R10: 00007fe6f6c21ce0 R11: 0000000000000202 R12: 0000000067941451
kernel: R13: 0000555fd0b8d040 R14: 0000000000000001 R15: 0000000000000000
kernel: </TASK>
kernel: Modules linked in: kheaders idpf libeth cmac nls_utf8 cifs cifs_arc4 nls_ucs2_utils cifs_md4 dns_resolver netfs xt_conntrack nft_chain_nat xt_MASQUERADE nf_nat nf_conntrack_netlink nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 xt_addrtype nft_compat nf_tables nfnetlink br_netfilter bridge stp llc rpcrdma intel_rapl_msr intel_rapl_common skx_edac skx_edac_common nfit rfkill libnvdimm overlay x86_pkg_temp_thermal intel_powerclamp coretemp kvm_intel kvm rapl intel_cstate iTCO_wdt ipmi_ssif iTCO_vendor_support vfat fat acpi_power_meter i2c_i801 intel_uncore joydev ipmi_si acpi_ipmi lpc_ich pcspkr i2c_smbus ipmi_devintf ioatdma ipmi_msghandler mei_me acpi_pad mei intel_pch_thermal dca nfsd auth_rpcgss nfs_acl lockd grace sunrpc zram ast drm_client_lib crct10dif_pclmul i2c_algo_bit crc32_pclmul drm_shmem_helper drm_kms_helper ghash_clmulni_intel nvme sha512_ssse3 i40e sha256_ssse3 drm nvme_core sha1_ssse3 libie wmi sctp libcrc32c crc32c_intel ip6_udp_tunnel udp_tunnel fuse
kernel: CR2: 00000000000001a0
kernel: ---[ end trace 0000000000000000 ]---
Thank You
Krishneil Singh
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH iwl-net v1 0/4] fix locking issue
2025-01-27 6:06 ` [PATCH iwl-net v1 0/4] fix locking issue Singh, Krishneil K
@ 2025-01-27 8:11 ` Przemek Kitszel
2025-01-27 17:42 ` Jakub Kicinski
0 siblings, 1 reply; 13+ messages in thread
From: Przemek Kitszel @ 2025-01-27 8:11 UTC (permalink / raw)
To: Singh, Krishneil K, intel-wired-lan@lists.osuosl.org,
Emil Tantilov, Jakub Kicinski
Cc: netdev@vger.kernel.org, Ahmed Zaki
On 1/27/25 07:06, Singh, Krishneil K wrote:
>
>> -----Original Message-----
>> From: Tarun K Singh <tarun.k.singh@intel.com>
>> Sent: Tuesday, November 5, 2024 10:49 AM
>> To: intel-wired-lan@lists.osuosl.org
>> Cc: netdev@vger.kernel.org
>> Subject: [PATCH iwl-net v1 0/4] fix locking issue
>>
>> This series fix deadlock issues in the driver. The first patch changes
>> argument of function 'idpf_vport_ctrl_lock' to adapter. The second patch
>> renames 'vport_ctrl_lock' to 'vport_cfg_lock'. The first 2 patches make the
>> third patch easier to review. The third patch fixes the locking issue,
>> and the fourth patch prevents lockdep from complaining.
>>
>> Ahmed Zaki (1):
>> idpf: add lock class key
>>
>> Tarun K Singh (3):
>> idpf: Change function argument
>> idpf: rename vport_ctrl_lock
>> idpf: Add init, reinit, and deinit control lock
>>
>> drivers/net/ethernet/intel/idpf/idpf.h | 49 ++++++++----
>> .../net/ethernet/intel/idpf/idpf_ethtool.c | 59 +++++++-------
>> drivers/net/ethernet/intel/idpf/idpf_lib.c | 80 +++++++++++++------
>> drivers/net/ethernet/intel/idpf/idpf_main.c | 39 ++++++---
>> 4 files changed, 149 insertions(+), 78 deletions(-)
>>
>> --
>> 2.46.0
>>
>
> After additional testing on this [series|patch] we discovered an issue after doing ifdown while device is in a reset, which can result in a <null pointer dereference(?)>.
Thank you for the report,
Could you please check if this issue could be reproduced without the
series?
We have yet another patch by Emil, that aims to fix such issues by
detaching the device prior to the reset.
@netdev
I would like to consider adding "in reset" state for the netdev,
to prevent such behavior in more civilized way though.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH iwl-net v1 0/4] fix locking issue
2025-01-27 8:11 ` Przemek Kitszel
@ 2025-01-27 17:42 ` Jakub Kicinski
2025-01-27 20:55 ` Przemek Kitszel
0 siblings, 1 reply; 13+ messages in thread
From: Jakub Kicinski @ 2025-01-27 17:42 UTC (permalink / raw)
To: Przemek Kitszel
Cc: Singh, Krishneil K, intel-wired-lan@lists.osuosl.org,
Emil Tantilov, netdev@vger.kernel.org, Ahmed Zaki
On Mon, 27 Jan 2025 09:11:41 +0100 Przemek Kitszel wrote:
> @netdev
> I would like to consider adding "in reset" state for the netdev,
> to prevent such behavior in more civilized way though.
Can you provide more context? We already have netif_device_detach()
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH iwl-net v1 0/4] fix locking issue
2025-01-27 17:42 ` Jakub Kicinski
@ 2025-01-27 20:55 ` Przemek Kitszel
0 siblings, 0 replies; 13+ messages in thread
From: Przemek Kitszel @ 2025-01-27 20:55 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Singh, Krishneil K, intel-wired-lan@lists.osuosl.org,
Emil Tantilov, netdev@vger.kernel.org, Ahmed Zaki
On 1/27/25 18:42, Jakub Kicinski wrote:
> On Mon, 27 Jan 2025 09:11:41 +0100 Przemek Kitszel wrote:
>> @netdev
>> I would like to consider adding "in reset" state for the netdev,
>> to prevent such behavior in more civilized way though.
>
> Can you provide more context? We already have netif_device_detach()
>
getting -ENODEV on the device/interface that will soon be up again feels
unoptimal, and with all the risks of "poor driver undoing user config"
during the disablement I would like to see better.
perhaps with Emil's patch there will be also more context
^ permalink raw reply [flat|nested] 13+ messages in thread