[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID:
<SJ0PR18MB521639933FF3226216EFC982DB8DA@SJ0PR18MB5216.namprd18.prod.outlook.com>
Date: Wed, 13 Dec 2023 04:52:17 +0000
From: Suman Ghosh <sumang@...vell.com>
To: Simon Horman <horms@...nel.org>
CC: "davem@...emloft.net" <davem@...emloft.net>,
"edumazet@...gle.com"
<edumazet@...gle.com>,
"kuba@...nel.org" <kuba@...nel.org>,
"pabeni@...hat.com" <pabeni@...hat.com>,
Sunil Kovvuri Goutham
<sgoutham@...vell.com>,
Subbaraya Sundeep Bhatta <sbhatta@...vell.com>,
Jerin
Jacob Kollanukkaran <jerinj@...vell.com>,
Geethasowjanya Akula
<gakula@...vell.com>,
Hariprasad Kelam <hkelam@...vell.com>,
Linu Cherian
<lcherian@...vell.com>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [EXT] Re: [net-next PATCH] octeontx2-af: Fix multicast/mirror
group lock/unlock issue
>
>> @@ -6306,6 +6310,13 @@ int
>rvu_mbox_handler_nix_mcast_grp_destroy(struct rvu *rvu,
>> return err;
>>
>> mcast_grp = &nix_hw->mcast_grp;
>> +
>> + /* If AF is requesting for the deletion,
>> + * then AF is already taking the lock
>> + */
>> + if (!req->is_af)
>> + mutex_lock(&mcast_grp->mcast_grp_lock);
>> +
>> elem = rvu_nix_mcast_find_grp_elem(mcast_grp, req->mcast_grp_idx);
>> if (!elem)
>
>Hi Suman,
>
>Does mcast_grp_lock need to be released here?
>If so, I would suggest a goto label, say unlock_grp.
[Suman] ack, will update in v2
>
>> return NIX_AF_ERR_INVALID_MCAST_GRP; @@ -6333,12 +6344,6 @@
>int
>> rvu_mbox_handler_nix_mcast_grp_destroy(struct rvu *rvu,
>> mutex_unlock(&mcast->mce_lock);
>>
>> delete_grp:
>> - /* If AF is requesting for the deletion,
>> - * then AF is already taking the lock
>> - */
>> - if (!req->is_af)
>> - mutex_lock(&mcast_grp->mcast_grp_lock);
>> -
>> list_del(&elem->list);
>> kfree(elem);
>> mcast_grp->count--;
>> @@ -6370,9 +6375,20 @@ int
>rvu_mbox_handler_nix_mcast_grp_update(struct rvu *rvu,
>> return err;
>>
>> mcast_grp = &nix_hw->mcast_grp;
>> +
>> + /* If AF is requesting for the updation,
>> + * then AF is already taking the lock
>> + */
>> + if (!req->is_af)
>> + mutex_lock(&mcast_grp->mcast_grp_lock);
>> +
>> elem = rvu_nix_mcast_find_grp_elem(mcast_grp, req->mcast_grp_idx);
>> - if (!elem)
>> + if (!elem) {
>> + if (!req->is_af)
>> + mutex_unlock(&mcast_grp->mcast_grp_lock);
>> +
>> return NIX_AF_ERR_INVALID_MCAST_GRP;
>> + }
>>
>> /* If any pcifunc matches the group's pcifunc, then we can
>> * delete the entire group.
>> @@ -6383,8 +6399,11 @@ int
>rvu_mbox_handler_nix_mcast_grp_update(struct rvu *rvu,
>> /* Delete group */
>> dreq.hdr.pcifunc = elem->pcifunc;
>> dreq.mcast_grp_idx = elem->mcast_grp_idx;
>> - dreq.is_af = req->is_af;
>> + dreq.is_af = 1;
>> rvu_mbox_handler_nix_mcast_grp_destroy(rvu, &dreq,
>NULL);
>> + if (!req->is_af)
>> + mutex_unlock(&mcast_grp->mcast_grp_lock);
>> +
>> return 0;
>> }
>> }
>> @@ -6467,5 +6486,8 @@ int rvu_mbox_handler_nix_mcast_grp_update(struct
>> rvu *rvu,
>>
>> done:
>
>I think it would be good to rename this label, say unlock_mce;
[Suman] ack, will update in v2
>
>> mutex_unlock(&mcast->mce_lock);
>
>Add a new label here, say unlock_grp;
>And jump to this label whenever there is a need for the mutex_unlock()
>below.
[Suman] ack, will update in v2
>
>> + if (!req->is_af)
>> + mutex_unlock(&mcast_grp->mcast_grp_lock);
>> +
>> return ret;
>> }
>> --
>> 2.25.1
>>
Powered by blists - more mailing lists