| Message ID | 20220317042023.1470039-2-kuba@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | devlink: hold the instance lock in eswitch callbacks | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 2 this patch: 2 |
| netdev/cc_maintainers | success | CCed 4 of 4 maintainers |
| netdev/build_clang | success | Errors and warnings before: 0 this patch: 0 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 2 this patch: 2 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 51 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
> > > In prep for .eswitch_mode_set being called with the devlink instance > lock held use that lock explicitly instead of creating a local mutex > just for the sriov reconfig. > > Signed-off-by: Jakub Kicinski <kuba@kernel.org> > --- > drivers/net/ethernet/broadcom/bnxt/bnxt.c | 1 - > drivers/net/ethernet/broadcom/bnxt/bnxt.h | 6 ------ > drivers/net/ethernet/broadcom/bnxt/bnxt_sriov.c | 4 ++-- > drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c | 4 ++-- > 4 files changed, 4 insertions(+), 11 deletions(-) > > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c > b/drivers/net/ethernet/broadcom/bnxt/bnxt.c > index 92a1a43b3bee..1c28495875cf 100644 > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c > @@ -13470,7 +13470,6 @@ static int bnxt_init_one(struct pci_dev *pdev, > const struct pci_device_id *ent) > > #ifdef CONFIG_BNXT_SRIOV > init_waitqueue_head(&bp->sriov_cfg_wait); > - mutex_init(&bp->sriov_lock); > #endif > if (BNXT_SUPPORTS_TPA(bp)) { > bp->gro_func = bnxt_gro_func_5730x; > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h > b/drivers/net/ethernet/broadcom/bnxt/bnxt.h > index 447a9406b8a2..61aa3e8c5952 100644 > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h > @@ -2072,12 +2072,6 @@ struct bnxt { > wait_queue_head_t sriov_cfg_wait; > bool sriov_cfg; > #define BNXT_SRIOV_CFG_WAIT_TMO msecs_to_jiffies(10000) > - > - /* lock to protect VF-rep creation/cleanup via > - * multiple paths such as ->sriov_configure() and > - * devlink ->eswitch_mode_set() > - */ > - struct mutex sriov_lock; > #endif > > #if BITS_PER_LONG == 32 > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_sriov.c > b/drivers/net/ethernet/broadcom/bnxt/bnxt_sriov.c > index 1d177fed44a6..ddf2f3963abe 100644 > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_sriov.c > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_sriov.c > @@ -846,7 +846,7 @@ void bnxt_sriov_disable(struct bnxt *bp) > return; > > /* synchronize VF and VF-rep create and destroy */ > - mutex_lock(&bp->sriov_lock); > + devl_lock(bp->dl); > bnxt_vf_reps_destroy(bp); > > if (pci_vfs_assigned(bp->pdev)) { > @@ -859,7 +859,7 @@ void bnxt_sriov_disable(struct bnxt *bp) > /* Free the HW resources reserved for various VF's */ > bnxt_hwrm_func_vf_resource_free(bp, num_vfs); > } > - mutex_unlock(&bp->sriov_lock); > + devl_unlock(bp->dl); > > bnxt_free_vf_resources(bp); > > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c > b/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c > index 8eb28e088582..b2a9528b456b 100644 > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c > @@ -561,7 +561,7 @@ int bnxt_dl_eswitch_mode_set(struct devlink > *devlink, u16 mode, > struct bnxt *bp = bnxt_get_bp_from_dl(devlink); > int rc = 0; > > - mutex_lock(&bp->sriov_lock); > + devl_lock(devlink); > if (bp->eswitch_mode == mode) { > netdev_info(bp->dev, "already in %s eswitch mode\n", > mode == DEVLINK_ESWITCH_MODE_LEGACY ? > @@ -595,7 +595,7 @@ int bnxt_dl_eswitch_mode_set(struct devlink > *devlink, u16 mode, > goto done; > } > done: > - mutex_unlock(&bp->sriov_lock); > + devl_unlock(devlink); > return rc; > } > > -- > 2.34.1 Hi Jakub, The changes look good to me overall. But I have a few concerns. This change introduces a lock that is held across modules and if there's any upcall from the driver into devlink that might potentially acquire the same lock, then it could result in a deadlock. I'm not familiar with the internals of devlink, but just want to make sure this point is considered. Also, the driver needs to be aware of this lock and use it in new code paths within the driver to synchronize with switchdev operations. This may not be so obvious when compared to a driver private lock. Thanks, -Harsha
On Thu, 17 Mar 2022 23:15:33 +0530 Sriharsha Basavapatna wrote: > The changes look good to me overall. But I have a few concerns. This > change introduces a lock that is held across modules and if there's > any upcall from the driver into devlink that might potentially acquire > the same lock, then it could result in a deadlock. I'm not familiar > with the internals of devlink, but just want to make sure this point > is considered. Also, the driver needs to be aware of this lock and use > it in new code paths within the driver to synchronize with switchdev > operations. This may not be so obvious when compared to a driver > private lock. That's true, that's why we're adding the new "unlocked" devl_* API. I'm switching the drivers accordingly, I didn't see any upcalls in the relevant parts in bnxt, LMK if I missed something!
On Thu, Mar 17, 2022 at 11:50 PM Jakub Kicinski <kuba@kernel.org> wrote: > > On Thu, 17 Mar 2022 23:15:33 +0530 Sriharsha Basavapatna wrote: > > The changes look good to me overall. But I have a few concerns. This > > change introduces a lock that is held across modules and if there's > > any upcall from the driver into devlink that might potentially acquire > > the same lock, then it could result in a deadlock. I'm not familiar > > with the internals of devlink, but just want to make sure this point > > is considered. Also, the driver needs to be aware of this lock and use > > it in new code paths within the driver to synchronize with switchdev > > operations. This may not be so obvious when compared to a driver > > private lock. > > That's true, that's why we're adding the new "unlocked" devl_* API. > I'm switching the drivers accordingly, I didn't see any upcalls in > the relevant parts in bnxt, LMK if I missed something! There's no upcall currently at least in switchdev related code in bnxt.
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c index 92a1a43b3bee..1c28495875cf 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c @@ -13470,7 +13470,6 @@ static int bnxt_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) #ifdef CONFIG_BNXT_SRIOV init_waitqueue_head(&bp->sriov_cfg_wait); - mutex_init(&bp->sriov_lock); #endif if (BNXT_SUPPORTS_TPA(bp)) { bp->gro_func = bnxt_gro_func_5730x; diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h index 447a9406b8a2..61aa3e8c5952 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h @@ -2072,12 +2072,6 @@ struct bnxt { wait_queue_head_t sriov_cfg_wait; bool sriov_cfg; #define BNXT_SRIOV_CFG_WAIT_TMO msecs_to_jiffies(10000) - - /* lock to protect VF-rep creation/cleanup via - * multiple paths such as ->sriov_configure() and - * devlink ->eswitch_mode_set() - */ - struct mutex sriov_lock; #endif #if BITS_PER_LONG == 32 diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_sriov.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_sriov.c index 1d177fed44a6..ddf2f3963abe 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_sriov.c +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_sriov.c @@ -846,7 +846,7 @@ void bnxt_sriov_disable(struct bnxt *bp) return; /* synchronize VF and VF-rep create and destroy */ - mutex_lock(&bp->sriov_lock); + devl_lock(bp->dl); bnxt_vf_reps_destroy(bp); if (pci_vfs_assigned(bp->pdev)) { @@ -859,7 +859,7 @@ void bnxt_sriov_disable(struct bnxt *bp) /* Free the HW resources reserved for various VF's */ bnxt_hwrm_func_vf_resource_free(bp, num_vfs); } - mutex_unlock(&bp->sriov_lock); + devl_unlock(bp->dl); bnxt_free_vf_resources(bp); diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c index 8eb28e088582..b2a9528b456b 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c @@ -561,7 +561,7 @@ int bnxt_dl_eswitch_mode_set(struct devlink *devlink, u16 mode, struct bnxt *bp = bnxt_get_bp_from_dl(devlink); int rc = 0; - mutex_lock(&bp->sriov_lock); + devl_lock(devlink); if (bp->eswitch_mode == mode) { netdev_info(bp->dev, "already in %s eswitch mode\n", mode == DEVLINK_ESWITCH_MODE_LEGACY ? @@ -595,7 +595,7 @@ int bnxt_dl_eswitch_mode_set(struct devlink *devlink, u16 mode, goto done; } done: - mutex_unlock(&bp->sriov_lock); + devl_unlock(devlink); return rc; }
In prep for .eswitch_mode_set being called with the devlink instance lock held use that lock explicitly instead of creating a local mutex just for the sriov reconfig. Signed-off-by: Jakub Kicinski <kuba@kernel.org> --- drivers/net/ethernet/broadcom/bnxt/bnxt.c | 1 - drivers/net/ethernet/broadcom/bnxt/bnxt.h | 6 ------ drivers/net/ethernet/broadcom/bnxt/bnxt_sriov.c | 4 ++-- drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c | 4 ++-- 4 files changed, 4 insertions(+), 11 deletions(-)