Skip to content

Commit

Permalink
Merge branch 'nic-thunderx-fix-communication-races-between-VF-PF'
Browse files Browse the repository at this point in the history 
Vadim Lomovtsev says:

====================
nic: thunderx: fix communication races between VF & PF

The ThunderX CN88XX NIC Virtual Function driver uses mailbox interface
to communicate to physical function driver. Each of VF has it's own pair
of mailbox registers to read from and write to. The mailbox registers
has no protection from possible races, so it has to be implemented
at software side.

After long term testing by loop of 'ip link set <ifname> up/down'
command it was found that there are two possible scenarios when
race condition appears:
 1. VF receives link change message from PF and VF send RX mode
configuration message to PF in the same time from separate thread.
 2. PF receives RX mode configuration from VF and in the same time,
in separate thread PF detects link status change and sends appropriate
message to particular VF.

Both cases leads to mailbox data to be rewritten, NIC VF messaging control
data to be updated incorrectly and communication sequence gets broken.

This patch series is to address race condition with VF & PF communication.

Changes:
v1 -> v2
 - 0000: correct typo in cover letter subject: 'betwen' -> 'between';
 - move link state polling request task from pf to vf
   instead of cheking status of mailbox irq;
v2 -> v3
 - 0003: change return type of nicvf_send_cfg_done() function
   from int to void;
 - 0007: update subject and remove unused variable 'netdev'
   from nicvf_link_status_check_task() function;
====================

Signed-off-by: David S. Miller <davem@davemloft.net>
  • Loading branch information
David S. Miller committed Feb 22, 2019
2 parents efcc9bc + 2e1c3ff commit aaaf598
Show file tree
Hide file tree
Showing 5 changed files with 142 additions and 153 deletions.
14 changes: 8 additions & 6 deletions drivers/net/ethernet/cavium/thunder/nic.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -271,7 +271,7 @@ struct xcast_addr_list {
};

struct nicvf_work {
struct delayed_work work;
struct work_struct work;
u8 mode;
struct xcast_addr_list *mc;
};
Expand Down Expand Up @@ -327,7 +327,11 @@ struct nicvf {
struct nicvf_work rx_mode_work;
/* spinlock to protect workqueue arguments from concurrent access */
spinlock_t rx_mode_wq_lock;

/* workqueue for handling kernel ndo_set_rx_mode() calls */
struct workqueue_struct *nicvf_rx_mode_wq;
/* mutex to protect VF's mailbox contents from concurrent access */
struct mutex rx_mode_mtx;
struct delayed_work link_change_work;
/* PTP timestamp */
struct cavium_ptp *ptp_clock;
/* Inbound timestamping is on */
Expand Down Expand Up @@ -575,10 +579,8 @@ struct set_ptp {

struct xcast {
u8 msg;
union {
u8 mode;
u64 mac;
} data;
u8 mode;
u64 mac:48;
};

/* 128 bit shared memory between PF and each VF */
Expand Down
149 changes: 46 additions & 103 deletions drivers/net/ethernet/cavium/thunder/nic_main.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,14 +57,8 @@ struct nicpf {
#define NIC_GET_BGX_FROM_VF_LMAC_MAP(map) ((map >> 4) & 0xF)
#define NIC_GET_LMAC_FROM_VF_LMAC_MAP(map) (map & 0xF)
u8 *vf_lmac_map;
struct delayed_work dwork;
struct workqueue_struct *check_link;
u8 *link;
u8 *duplex;
u32 *speed;
u16 cpi_base[MAX_NUM_VFS_SUPPORTED];
u16 rssi_base[MAX_NUM_VFS_SUPPORTED];
bool mbx_lock[MAX_NUM_VFS_SUPPORTED];

/* MSI-X */
u8 num_vec;
Expand Down Expand Up @@ -929,6 +923,35 @@ static void nic_config_timestamp(struct nicpf *nic, int vf, struct set_ptp *ptp)
nic_reg_write(nic, NIC_PF_PKIND_0_15_CFG | (pkind_idx << 3), pkind_val);
}

/* Get BGX LMAC link status and update corresponding VF
* if there is a change, valid only if internal L2 switch
* is not present otherwise VF link is always treated as up
*/
static void nic_link_status_get(struct nicpf *nic, u8 vf)
{
union nic_mbx mbx = {};
struct bgx_link_status link;
u8 bgx, lmac;

mbx.link_status.msg = NIC_MBOX_MSG_BGX_LINK_CHANGE;

/* Get BGX, LMAC indices for the VF */
bgx = NIC_GET_BGX_FROM_VF_LMAC_MAP(nic->vf_lmac_map[vf]);
lmac = NIC_GET_LMAC_FROM_VF_LMAC_MAP(nic->vf_lmac_map[vf]);

/* Get interface link status */
bgx_get_lmac_link_state(nic->node, bgx, lmac, &link);

/* Send a mbox message to VF with current link status */
mbx.link_status.link_up = link.link_up;
mbx.link_status.duplex = link.duplex;
mbx.link_status.speed = link.speed;
mbx.link_status.mac_type = link.mac_type;

/* reply with link status */
nic_send_msg_to_vf(nic, vf, &mbx);
}

/* Interrupt handler to handle mailbox messages from VFs */
static void nic_handle_mbx_intr(struct nicpf *nic, int vf)
{
Expand All @@ -941,8 +964,6 @@ static void nic_handle_mbx_intr(struct nicpf *nic, int vf)
int i;
int ret = 0;

nic->mbx_lock[vf] = true;

mbx_addr = nic_get_mbx_addr(vf);
mbx_data = (u64 *)&mbx;

Expand All @@ -957,12 +978,7 @@ static void nic_handle_mbx_intr(struct nicpf *nic, int vf)
switch (mbx.msg.msg) {
case NIC_MBOX_MSG_READY:
nic_mbx_send_ready(nic, vf);
if (vf < nic->num_vf_en) {
nic->link[vf] = 0;
nic->duplex[vf] = 0;
nic->speed[vf] = 0;
}
goto unlock;
return;
case NIC_MBOX_MSG_QS_CFG:
reg_addr = NIC_PF_QSET_0_127_CFG |
(mbx.qs.num << NIC_QS_ID_SHIFT);
Expand Down Expand Up @@ -1031,15 +1047,15 @@ static void nic_handle_mbx_intr(struct nicpf *nic, int vf)
break;
case NIC_MBOX_MSG_RSS_SIZE:
nic_send_rss_size(nic, vf);
goto unlock;
return;
case NIC_MBOX_MSG_RSS_CFG:
case NIC_MBOX_MSG_RSS_CFG_CONT:
nic_config_rss(nic, &mbx.rss_cfg);
break;
case NIC_MBOX_MSG_CFG_DONE:
/* Last message of VF config msg sequence */
nic_enable_vf(nic, vf, true);
goto unlock;
break;
case NIC_MBOX_MSG_SHUTDOWN:
/* First msg in VF teardown sequence */
if (vf >= nic->num_vf_en)
Expand All @@ -1049,19 +1065,19 @@ static void nic_handle_mbx_intr(struct nicpf *nic, int vf)
break;
case NIC_MBOX_MSG_ALLOC_SQS:
nic_alloc_sqs(nic, &mbx.sqs_alloc);
goto unlock;
return;
case NIC_MBOX_MSG_NICVF_PTR:
nic->nicvf[vf] = mbx.nicvf.nicvf;
break;
case NIC_MBOX_MSG_PNICVF_PTR:
nic_send_pnicvf(nic, vf);
goto unlock;
return;
case NIC_MBOX_MSG_SNICVF_PTR:
nic_send_snicvf(nic, &mbx.nicvf);
goto unlock;
return;
case NIC_MBOX_MSG_BGX_STATS:
nic_get_bgx_stats(nic, &mbx.bgx_stats);
goto unlock;
return;
case NIC_MBOX_MSG_LOOPBACK:
ret = nic_config_loopback(nic, &mbx.lbk);
break;
Expand All @@ -1070,7 +1086,7 @@ static void nic_handle_mbx_intr(struct nicpf *nic, int vf)
break;
case NIC_MBOX_MSG_PFC:
nic_pause_frame(nic, vf, &mbx.pfc);
goto unlock;
return;
case NIC_MBOX_MSG_PTP_CFG:
nic_config_timestamp(nic, vf, &mbx.ptp);
break;
Expand All @@ -1094,7 +1110,7 @@ static void nic_handle_mbx_intr(struct nicpf *nic, int vf)
bgx = NIC_GET_BGX_FROM_VF_LMAC_MAP(nic->vf_lmac_map[vf]);
lmac = NIC_GET_LMAC_FROM_VF_LMAC_MAP(nic->vf_lmac_map[vf]);
bgx_set_dmac_cam_filter(nic->node, bgx, lmac,
mbx.xcast.data.mac,
mbx.xcast.mac,
vf < NIC_VF_PER_MBX_REG ? vf :
vf - NIC_VF_PER_MBX_REG);
break;
Expand All @@ -1106,8 +1122,15 @@ static void nic_handle_mbx_intr(struct nicpf *nic, int vf)
}
bgx = NIC_GET_BGX_FROM_VF_LMAC_MAP(nic->vf_lmac_map[vf]);
lmac = NIC_GET_LMAC_FROM_VF_LMAC_MAP(nic->vf_lmac_map[vf]);
bgx_set_xcast_mode(nic->node, bgx, lmac, mbx.xcast.data.mode);
bgx_set_xcast_mode(nic->node, bgx, lmac, mbx.xcast.mode);
break;
case NIC_MBOX_MSG_BGX_LINK_CHANGE:
if (vf >= nic->num_vf_en) {
ret = -1; /* NACK */
break;
}
nic_link_status_get(nic, vf);
return;
default:
dev_err(&nic->pdev->dev,
"Invalid msg from VF%d, msg 0x%x\n", vf, mbx.msg.msg);
Expand All @@ -1121,8 +1144,6 @@ static void nic_handle_mbx_intr(struct nicpf *nic, int vf)
mbx.msg.msg, vf);
nic_mbx_send_nack(nic, vf);
}
unlock:
nic->mbx_lock[vf] = false;
}

static irqreturn_t nic_mbx_intr_handler(int irq, void *nic_irq)
Expand Down Expand Up @@ -1270,52 +1291,6 @@ static int nic_sriov_init(struct pci_dev *pdev, struct nicpf *nic)
return 0;
}

/* Poll for BGX LMAC link status and update corresponding VF
* if there is a change, valid only if internal L2 switch
* is not present otherwise VF link is always treated as up
*/
static void nic_poll_for_link(struct work_struct *work)
{
union nic_mbx mbx = {};
struct nicpf *nic;
struct bgx_link_status link;
u8 vf, bgx, lmac;

nic = container_of(work, struct nicpf, dwork.work);

mbx.link_status.msg = NIC_MBOX_MSG_BGX_LINK_CHANGE;

for (vf = 0; vf < nic->num_vf_en; vf++) {
/* Poll only if VF is UP */
if (!nic->vf_enabled[vf])
continue;

/* Get BGX, LMAC indices for the VF */
bgx = NIC_GET_BGX_FROM_VF_LMAC_MAP(nic->vf_lmac_map[vf]);
lmac = NIC_GET_LMAC_FROM_VF_LMAC_MAP(nic->vf_lmac_map[vf]);
/* Get interface link status */
bgx_get_lmac_link_state(nic->node, bgx, lmac, &link);

/* Inform VF only if link status changed */
if (nic->link[vf] == link.link_up)
continue;

if (!nic->mbx_lock[vf]) {
nic->link[vf] = link.link_up;
nic->duplex[vf] = link.duplex;
nic->speed[vf] = link.speed;

/* Send a mbox message to VF with current link status */
mbx.link_status.link_up = link.link_up;
mbx.link_status.duplex = link.duplex;
mbx.link_status.speed = link.speed;
mbx.link_status.mac_type = link.mac_type;
nic_send_msg_to_vf(nic, vf, &mbx);
}
}
queue_delayed_work(nic->check_link, &nic->dwork, HZ * 2);
}

static int nic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
{
struct device *dev = &pdev->dev;
Expand Down Expand Up @@ -1384,18 +1359,6 @@ static int nic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
if (!nic->vf_lmac_map)
goto err_release_regions;

nic->link = devm_kmalloc_array(dev, max_lmac, sizeof(u8), GFP_KERNEL);
if (!nic->link)
goto err_release_regions;

nic->duplex = devm_kmalloc_array(dev, max_lmac, sizeof(u8), GFP_KERNEL);
if (!nic->duplex)
goto err_release_regions;

nic->speed = devm_kmalloc_array(dev, max_lmac, sizeof(u32), GFP_KERNEL);
if (!nic->speed)
goto err_release_regions;

/* Initialize hardware */
nic_init_hw(nic);

Expand All @@ -1411,22 +1374,8 @@ static int nic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
if (err)
goto err_unregister_interrupts;

/* Register a physical link status poll fn() */
nic->check_link = alloc_workqueue("check_link_status",
WQ_UNBOUND | WQ_MEM_RECLAIM, 1);
if (!nic->check_link) {
err = -ENOMEM;
goto err_disable_sriov;
}

INIT_DELAYED_WORK(&nic->dwork, nic_poll_for_link);
queue_delayed_work(nic->check_link, &nic->dwork, 0);

return 0;

err_disable_sriov:
if (nic->flags & NIC_SRIOV_ENABLED)
pci_disable_sriov(pdev);
err_unregister_interrupts:
nic_unregister_interrupts(nic);
err_release_regions:
Expand All @@ -1447,12 +1396,6 @@ static void nic_remove(struct pci_dev *pdev)
if (nic->flags & NIC_SRIOV_ENABLED)
pci_disable_sriov(pdev);

if (nic->check_link) {
/* Destroy work Queue */
cancel_delayed_work_sync(&nic->dwork);
destroy_workqueue(nic->check_link);
}

nic_unregister_interrupts(nic);
pci_release_regions(pdev);

Expand Down
Loading

0 comments on commit aaaf598

Please sign in to comment.