| Message ID | 20220204124601.3617217-1-hkalra@marvell.com |
|---|---|
| State | Accepted |
| Commit | 4363f3d3ce8f5440dfbcd66b6a6800b42a58ba6a |
| Headers | show |
| Series | crypto: octeontx2 - add synchronization between mailbox accesses | expand |
Hi Herbert Please see inline. > -----Original Message----- > From: Herbert Xu <herbert@gondor.apana.org.au> > Sent: Friday, February 11, 2022 2:49 PM > To: Harman Kalra <hkalra@marvell.com> > Cc: Arnaud Ebalard <arno@natisbad.org>; Boris Brezillon > <bbrezillon@kernel.org>; Srujana Challa <schalla@marvell.com>; linux- > crypto@vger.kernel.org; Jerin Jacob Kollanukkaran <jerinj@marvell.com>; > Sunil Kovvuri Goutham <sgoutham@marvell.com> > Subject: [EXT] Re: [PATCH] crypto: octeontx2 - add synchronization between > mailbox accesses > > External Email > > ---------------------------------------------------------------------- > On Fri, Feb 04, 2022 at 06:16:01PM +0530, Harman Kalra wrote: > > > > offset = msg->next_msgoff; > > + /* Write barrier required for VF responses which are handled > by > > + * PF driver and not forwarded to AF. > > + */ > > + smp_wmb(); > > Who is the reader in this case? Is it also part of the kernel? This shared region is accessed by VF driver which is a DPDK driver. > Because if a device is involved then smp_wmb is not appropriate. This is the same driver which is handling multiple platforms. In older platforms this region was normal DRAM region and was mapped in DPDK driver but in recent platforms it is device memory. Thanks Harman > > Thanks, > -- > Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page: > https://urldefense.proofpoint.com/v2/url?u=http- > 3A__gondor.apana.org.au_- > 7Eherbert_&d=DwIBAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=5ESHPj7V- > 7JdkxT_Z_SU6RrS37ys4UXudBQ_rrS5LRo&m=ho3Yrv-lqiH2g1- > aPC__mENJQ5Sl-8ZiYhq1B9w7q4JIznCaE51- > HsGGwyoybXo1&s=l3Jk2Ay8mVeXEZN8mEYcUduOUgAnBZkLP2bpGEKtwL4& > e= > PGP Key: https://urldefense.proofpoint.com/v2/url?u=http- > 3A__gondor.apana.org.au_- > 7Eherbert_pubkey.txt&d=DwIBAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=5ESHPj > 7V-7JdkxT_Z_SU6RrS37ys4UXudBQ_rrS5LRo&m=ho3Yrv-lqiH2g1- > aPC__mENJQ5Sl-8ZiYhq1B9w7q4JIznCaE51-HsGGwyoybXo1&s=- > jOywGz3R15pI-vdtiom908wdVHFZVmBn7ktoqtVkYE&e=
On Fri, Feb 04, 2022 at 06:16:01PM +0530, Harman Kalra wrote: > Since there are two workqueues implemented in CPTPF driver - one > for handling mailbox requests from VFs and another for handling FLR. > In both cases PF driver will forward the request to AF driver by > writing to mailbox memory. A race condition may arise if two > simultaneous requests are written to mailbox memory. Introducing > locking mechanism to maintain synchronization between multiple > mailbox accesses. > > Signed-off-by: Harman Kalra <hkalra@marvell.com> > --- > .../marvell/octeontx2/otx2_cpt_common.h | 1 + > .../marvell/octeontx2/otx2_cpt_mbox_common.c | 14 +++++++++++ > drivers/crypto/marvell/octeontx2/otx2_cptpf.h | 1 + > .../marvell/octeontx2/otx2_cptpf_main.c | 21 ++++++++++------- > .../marvell/octeontx2/otx2_cptpf_mbox.c | 23 ++++++++++++++----- > 5 files changed, 46 insertions(+), 14 deletions(-) Patch applied. Thanks.
diff --git a/drivers/crypto/marvell/octeontx2/otx2_cpt_common.h b/drivers/crypto/marvell/octeontx2/otx2_cpt_common.h index fb56824cb0a6..5012b7e669f0 100644 --- a/drivers/crypto/marvell/octeontx2/otx2_cpt_common.h +++ b/drivers/crypto/marvell/octeontx2/otx2_cpt_common.h @@ -157,5 +157,6 @@ struct otx2_cptlfs_info; int otx2_cpt_attach_rscrs_msg(struct otx2_cptlfs_info *lfs); int otx2_cpt_detach_rsrcs_msg(struct otx2_cptlfs_info *lfs); int otx2_cpt_msix_offset_msg(struct otx2_cptlfs_info *lfs); +int otx2_cpt_sync_mbox_msg(struct otx2_mbox *mbox); #endif /* __OTX2_CPT_COMMON_H */ diff --git a/drivers/crypto/marvell/octeontx2/otx2_cpt_mbox_common.c b/drivers/crypto/marvell/octeontx2/otx2_cpt_mbox_common.c index 9074876d38e5..a317319696ef 100644 --- a/drivers/crypto/marvell/octeontx2/otx2_cpt_mbox_common.c +++ b/drivers/crypto/marvell/octeontx2/otx2_cpt_mbox_common.c @@ -202,3 +202,17 @@ int otx2_cpt_msix_offset_msg(struct otx2_cptlfs_info *lfs) } return ret; } + +int otx2_cpt_sync_mbox_msg(struct otx2_mbox *mbox) +{ + int err; + + if (!otx2_mbox_nonempty(mbox, 0)) + return 0; + otx2_mbox_msg_send(mbox, 0); + err = otx2_mbox_wait_for_rsp(mbox, 0); + if (err) + return err; + + return otx2_mbox_check_rsp_msgs(mbox, 0); +} diff --git a/drivers/crypto/marvell/octeontx2/otx2_cptpf.h b/drivers/crypto/marvell/octeontx2/otx2_cptpf.h index 05b2d9c650e1..936174b012e8 100644 --- a/drivers/crypto/marvell/octeontx2/otx2_cptpf.h +++ b/drivers/crypto/marvell/octeontx2/otx2_cptpf.h @@ -46,6 +46,7 @@ struct otx2_cptpf_dev { struct workqueue_struct *flr_wq; struct cptpf_flr_work *flr_work; + struct mutex lock; /* serialize mailbox access */ unsigned long cap_flag; u8 pf_id; /* RVU PF number */ diff --git a/drivers/crypto/marvell/octeontx2/otx2_cptpf_main.c b/drivers/crypto/marvell/octeontx2/otx2_cptpf_main.c index 1720a5bb7016..17a9dd20c8c3 100644 --- a/drivers/crypto/marvell/octeontx2/otx2_cptpf_main.c +++ b/drivers/crypto/marvell/octeontx2/otx2_cptpf_main.c @@ -140,6 +140,7 @@ static void cptpf_flr_wq_handler(struct work_struct *work) vf = flr_work - pf->flr_work; + mutex_lock(&pf->lock); req = otx2_mbox_alloc_msg_rsp(mbox, 0, sizeof(*req), sizeof(struct msg_rsp)); if (!req) @@ -151,16 +152,19 @@ static void cptpf_flr_wq_handler(struct work_struct *work) req->pcifunc |= (vf + 1) & RVU_PFVF_FUNC_MASK; otx2_cpt_send_mbox_msg(mbox, pf->pdev); + if (!otx2_cpt_sync_mbox_msg(&pf->afpf_mbox)) { - if (vf >= 64) { - reg = 1; - vf = vf - 64; + if (vf >= 64) { + reg = 1; + vf = vf - 64; + } + /* Clear transaction pending register */ + otx2_cpt_write64(pf->reg_base, BLKADDR_RVUM, 0, + RVU_PF_VFTRPENDX(reg), BIT_ULL(vf)); + otx2_cpt_write64(pf->reg_base, BLKADDR_RVUM, 0, + RVU_PF_VFFLR_INT_ENA_W1SX(reg), BIT_ULL(vf)); } - /* Clear transaction pending register */ - otx2_cpt_write64(pf->reg_base, BLKADDR_RVUM, 0, - RVU_PF_VFTRPENDX(reg), BIT_ULL(vf)); - otx2_cpt_write64(pf->reg_base, BLKADDR_RVUM, 0, - RVU_PF_VFFLR_INT_ENA_W1SX(reg), BIT_ULL(vf)); + mutex_unlock(&pf->lock); } static irqreturn_t cptpf_vf_flr_intr(int __always_unused irq, void *arg) @@ -468,6 +472,7 @@ static int cptpf_afpf_mbox_init(struct otx2_cptpf_dev *cptpf) goto error; INIT_WORK(&cptpf->afpf_mbox_work, otx2_cptpf_afpf_mbox_handler); + mutex_init(&cptpf->lock); return 0; error: diff --git a/drivers/crypto/marvell/octeontx2/otx2_cptpf_mbox.c b/drivers/crypto/marvell/octeontx2/otx2_cptpf_mbox.c index 186f1c1190c1..fee758b86d29 100644 --- a/drivers/crypto/marvell/octeontx2/otx2_cptpf_mbox.c +++ b/drivers/crypto/marvell/octeontx2/otx2_cptpf_mbox.c @@ -18,6 +18,7 @@ static int forward_to_af(struct otx2_cptpf_dev *cptpf, struct mbox_msghdr *msg; int ret; + mutex_lock(&cptpf->lock); msg = otx2_mbox_alloc_msg(&cptpf->afpf_mbox, 0, size); if (msg == NULL) return -ENOMEM; @@ -29,15 +30,19 @@ static int forward_to_af(struct otx2_cptpf_dev *cptpf, msg->sig = req->sig; msg->ver = req->ver; - otx2_mbox_msg_send(&cptpf->afpf_mbox, 0); - ret = otx2_mbox_wait_for_rsp(&cptpf->afpf_mbox, 0); + ret = otx2_cpt_sync_mbox_msg(&cptpf->afpf_mbox); + /* Error code -EIO indicate there is a communication failure + * to the AF. Rest of the error codes indicate that AF processed + * VF messages and set the error codes in response messages + * (if any) so simply forward responses to VF. + */ if (ret == -EIO) { - dev_err(&cptpf->pdev->dev, "RVU MBOX timeout.\n"); + dev_warn(&cptpf->pdev->dev, + "AF not responding to VF%d messages\n", vf->vf_id); + mutex_unlock(&cptpf->lock); return ret; - } else if (ret) { - dev_err(&cptpf->pdev->dev, "RVU MBOX error: %d.\n", ret); - return -EFAULT; } + mutex_unlock(&cptpf->lock); return 0; } @@ -204,6 +209,10 @@ void otx2_cptpf_vfpf_mbox_handler(struct work_struct *work) if (err == -ENOMEM || err == -EIO) break; offset = msg->next_msgoff; + /* Write barrier required for VF responses which are handled by + * PF driver and not forwarded to AF. + */ + smp_wmb(); } /* Send mbox responses to VF */ if (mdev->num_msgs) @@ -350,6 +359,8 @@ void otx2_cptpf_afpf_mbox_handler(struct work_struct *work) process_afpf_mbox_msg(cptpf, msg); offset = msg->next_msgoff; + /* Sync VF response ready to be sent */ + smp_wmb(); mdev->msgs_acked++; } otx2_mbox_reset(afpf_mbox, 0);
Since there are two workqueues implemented in CPTPF driver - one for handling mailbox requests from VFs and another for handling FLR. In both cases PF driver will forward the request to AF driver by writing to mailbox memory. A race condition may arise if two simultaneous requests are written to mailbox memory. Introducing locking mechanism to maintain synchronization between multiple mailbox accesses. Signed-off-by: Harman Kalra <hkalra@marvell.com> --- .../marvell/octeontx2/otx2_cpt_common.h | 1 + .../marvell/octeontx2/otx2_cpt_mbox_common.c | 14 +++++++++++ drivers/crypto/marvell/octeontx2/otx2_cptpf.h | 1 + .../marvell/octeontx2/otx2_cptpf_main.c | 21 ++++++++++------- .../marvell/octeontx2/otx2_cptpf_mbox.c | 23 ++++++++++++++----- 5 files changed, 46 insertions(+), 14 deletions(-)