Date:   Thu, 17 Mar 2022 23:15:33 +0530
From:   Sriharsha Basavapatna <sriharsha.basavapatna@...adcom.com>
To:     netdev@...r.kernel.org,
        Jiří Pírko <jiri@...nulli.us>,
        leonro@...dia.com, saeedm@...dia.com, idosch@...sch.org,
        Michael Chan <michael.chan@...adcom.com>,
        simon.horman@...igine.com, kuba@...nel.org
Cc:     Sriharsha Basavapatna <sriharsha.basavapatna@...adcom.com>
Subject: Re: [PATCH net-next 1/5] bnxt: use the devlink instance lock to
 protect sriov

>
>
> 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@...nel.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

-- 
This electronic communication and the information and any files transmitted 
with it, or attached to it, are confidential and are intended solely for 
the use of the individual or entity to whom it is addressed and may contain 
information that is confidential, legally privileged, protected by privacy 
laws, or otherwise restricted from disclosure to anyone else. If you are 
not the intended recipient or the person responsible for delivering the 
e-mail to the intended recipient, you are hereby notified that any use, 
copying, distributing, dissemination, forwarding, printing, or copying of 
this e-mail is strictly prohibited. If you received this e-mail in error, 
please return the e-mail to the sender, delete it from your computer, and 
destroy any printed copy of it.

Download attachment "smime.p7s" of type "application/pkcs7-signature" (4236 bytes)

Powered by blists - more mailing lists