[PATCH next] wifi: rtw89: unlock on error path in rtw89_ops_unassign_vif_chanctx()

[PATCH next] wifi: rtw89: unlock on error path in rtw89_ops_unassign_vif_chanctx()

[Dan Carpenter]

We need to call mutex_unlock() on this error path.

Fixes: aad0394e7a02 ("wifi: rtw89: tweak driver architecture for impending MLO support")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
 drivers/net/wireless/realtek/rtw89/mac80211.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/wireless/realtek/rtw89/mac80211.c b/drivers/net/wireless/realtek/rtw89/mac80211.c
index 1ee63a85308f..565347a6e1e6 100644
--- a/drivers/net/wireless/realtek/rtw89/mac80211.c
+++ b/drivers/net/wireless/realtek/rtw89/mac80211.c
@@ -1373,6 +1373,7 @@ static void rtw89_ops_unassign_vif_chanctx(struct ieee80211_hw *hw,
 
 	rtwvif_link = rtwvif->links[link_conf->link_id];
 	if (unlikely(!rtwvif_link)) {
+		mutex_unlock(&rtwdev->mutex);
 		rtw89_err(rtwdev,
 			  "%s: rtwvif link (link_id %u) is not active\n",
 			  __func__, link_conf->link_id);
-- 
2.45.2

RE: [PATCH next] wifi: rtw89: unlock on error path in rtw89_ops_unassign_vif_chanctx()

[Zong-Zhe Yang]

Dan Carpenter <dan.carpenter@linaro.org> wrote:
> 
> [...]
>
> @@ -1373,6 +1373,7 @@ static void rtw89_ops_unassign_vif_chanctx(struct ieee80211_hw
> *hw,
> 
>         rtwvif_link = rtwvif->links[link_conf->link_id];
>         if (unlikely(!rtwvif_link)) {
> +               mutex_unlock(&rtwdev->mutex);
>                 rtw89_err(rtwdev,
>                           "%s: rtwvif link (link_id %u) is not active\n",
>                           __func__, link_conf->link_id);
>

Acked-by: Zong-Zhe Yang <kevin_yang@realtek.com>

Thanks

RE: [PATCH next] wifi: rtw89: unlock on error path in rtw89_ops_unassign_vif_chanctx()

[Ping-Ke Shih]

Zong-Zhe Yang <kevin_yang@realtek.com> wrote:
> Dan Carpenter <dan.carpenter@linaro.org> wrote:
> >
> > [...]
> >
> > @@ -1373,6 +1373,7 @@ static void rtw89_ops_unassign_vif_chanctx(struct ieee80211_hw
> > *hw,
> >
> >         rtwvif_link = rtwvif->links[link_conf->link_id];
> >         if (unlikely(!rtwvif_link)) {
> > +               mutex_unlock(&rtwdev->mutex);
> >                 rtw89_err(rtwdev,
> >                           "%s: rtwvif link (link_id %u) is not active\n",
> >                           __func__, link_conf->link_id);
> >
> 
> Acked-by: Zong-Zhe Yang <kevin_yang@realtek.com>
> 

Thanks for the ack. 

Acked-by is often used by the maintainer, so I will change it to Reviewed-by
during committing. 

Re: [PATCH next] wifi: rtw89: unlock on error path in rtw89_ops_unassign_vif_chanctx()

[Dan Carpenter]

On Tue, Oct 22, 2024 at 03:32:23AM +0000, Ping-Ke Shih wrote:
> Zong-Zhe Yang <kevin_yang@realtek.com> wrote:
> > Dan Carpenter <dan.carpenter@linaro.org> wrote:
> > >
> > > [...]
> > >
> > > @@ -1373,6 +1373,7 @@ static void rtw89_ops_unassign_vif_chanctx(struct ieee80211_hw
> > > *hw,
> > >
> > >         rtwvif_link = rtwvif->links[link_conf->link_id];
> > >         if (unlikely(!rtwvif_link)) {
> > > +               mutex_unlock(&rtwdev->mutex);
> > >                 rtw89_err(rtwdev,
> > >                           "%s: rtwvif link (link_id %u) is not active\n",
> > >                           __func__, link_conf->link_id);
> > >
> > 
> > Acked-by: Zong-Zhe Yang <kevin_yang@realtek.com>
> > 
> 
> Thanks for the ack. 
> 
> Acked-by is often used by the maintainer, so I will change it to Reviewed-by
> during committing. 

To me Acked by just means you're okay with the patch.  When I use it, it means I
don't feel qualified or interested enough to do a full review.  For example, if
I complain about a v1 patch and they fix my issue in v2 then I like to say that
I'm okay with it.  In that case I'll use Reviewed-by for a full review or Acked
by if the bits that I care about are okay.  I don't like to complain and then
just go silent.

In the end, it doesn't make any difference.  You'll get CC'd on bug reports to
do with the patch and you'll potentially feel bad for not spotting the bug, I
guess.

regards,
dan carpenter

Re: [PATCH next] wifi: rtw89: unlock on error path in rtw89_ops_unassign_vif_chanctx()

[Kalle Valo]

Dan Carpenter <dan.carpenter@linaro.org> writes:

> On Tue, Oct 22, 2024 at 03:32:23AM +0000, Ping-Ke Shih wrote:
>
>> Zong-Zhe Yang <kevin_yang@realtek.com> wrote:
>> > Dan Carpenter <dan.carpenter@linaro.org> wrote:
>> > >
>> > > [...]
>> > >
>> > > @@ -1373,6 +1373,7 @@ static void rtw89_ops_unassign_vif_chanctx(struct ieee80211_hw
>> > > *hw,
>> > >
>> > >         rtwvif_link = rtwvif->links[link_conf->link_id];
>> > >         if (unlikely(!rtwvif_link)) {
>> > > +               mutex_unlock(&rtwdev->mutex);
>> > >                 rtw89_err(rtwdev,
>> > >                           "%s: rtwvif link (link_id %u) is not active\n",
>> > >                           __func__, link_conf->link_id);
>> > >
>> > 
>> > Acked-by: Zong-Zhe Yang <kevin_yang@realtek.com>
>> > 
>> 
>> Thanks for the ack. 
>> 
>> Acked-by is often used by the maintainer, so I will change it to Reviewed-by
>> during committing. 
>
> To me Acked by just means you're okay with the patch.  When I use it, it means I
> don't feel qualified or interested enough to do a full review.  For example, if
> I complain about a v1 patch and they fix my issue in v2 then I like to say that
> I'm okay with it.  In that case I'll use Reviewed-by for a full review or Acked
> by if the bits that I care about are okay.  I don't like to complain and then
> just go silent.
>
> In the end, it doesn't make any difference.  You'll get CC'd on bug reports to
> do with the patch and you'll potentially feel bad for not spotting the bug, I
> guess.

I have understood that Acked-by should be only used by the corresponding
maintainers and the documentation seems to say the same:

https://docs.kernel.org/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by

The reason I ask non-maintainers avoid using Acked-by is that it messes
our patchwork listings (it counts both Acked-by and Reviewed-by tags).

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH next] wifi: rtw89: unlock on error path in rtw89_ops_unassign_vif_chanctx()

[Dan Carpenter]

On Wed, Oct 23, 2024 at 12:38:38PM +0300, Kalle Valo wrote:
> Dan Carpenter <dan.carpenter@linaro.org> writes:
> 
> > On Tue, Oct 22, 2024 at 03:32:23AM +0000, Ping-Ke Shih wrote:
> >
> >> Zong-Zhe Yang <kevin_yang@realtek.com> wrote:
> >> > Dan Carpenter <dan.carpenter@linaro.org> wrote:
> >> > >
> >> > > [...]
> >> > >
> >> > > @@ -1373,6 +1373,7 @@ static void rtw89_ops_unassign_vif_chanctx(struct ieee80211_hw
> >> > > *hw,
> >> > >
> >> > >         rtwvif_link = rtwvif->links[link_conf->link_id];
> >> > >         if (unlikely(!rtwvif_link)) {
> >> > > +               mutex_unlock(&rtwdev->mutex);
> >> > >                 rtw89_err(rtwdev,
> >> > >                           "%s: rtwvif link (link_id %u) is not active\n",
> >> > >                           __func__, link_conf->link_id);
> >> > >
> >> > 
> >> > Acked-by: Zong-Zhe Yang <kevin_yang@realtek.com>
> >> > 
> >> 
> >> Thanks for the ack. 
> >> 
> >> Acked-by is often used by the maintainer, so I will change it to Reviewed-by
> >> during committing. 
> >
> > To me Acked by just means you're okay with the patch.  When I use it, it means I
> > don't feel qualified or interested enough to do a full review.  For example, if
> > I complain about a v1 patch and they fix my issue in v2 then I like to say that
> > I'm okay with it.  In that case I'll use Reviewed-by for a full review or Acked
> > by if the bits that I care about are okay.  I don't like to complain and then
> > just go silent.
> >
> > In the end, it doesn't make any difference.  You'll get CC'd on bug reports to
> > do with the patch and you'll potentially feel bad for not spotting the bug, I
> > guess.
> 
> I have understood that Acked-by should be only used by the corresponding
> maintainers and the documentation seems to say the same:
> 
> https://docs.kernel.org/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by

"If a person was not directly involved in the preparation or handling of a patch
but wishes to signify and record their approval of it then they can ask to have
an Acked-by: line added to the patch’s changelog."

The documentation does say that it's also often used by maintainers for
approving part of a patchset.  But to me, it's the "partial" which is the more
important word in that sentence.  I haven't reviewed the whole patch.

> 
> The reason I ask non-maintainers avoid using Acked-by is that it messes
> our patchwork listings (it counts both Acked-by and Reviewed-by tags).
> 
> -- 
> https://patchwork.kernel.org/project/linux-wireless/list/

Huh.  I wasn't aware that anything really differentiated between Acks and
Reviews.  That does make things more complicated.

I rarely do Acks, but when I do it's because I'm outside of a subsystem I'm
familiar with.  I would say LGTM and leave it at that, except other people want
proper tags.  Probably they won't insist on proper tags from me though so it's
fine.

regards,
dan carpenter

Re: [PATCH next] wifi: rtw89: unlock on error path in rtw89_ops_unassign_vif_chanctx()

[Ping-Ke Shih]

Dan Carpenter <dan.carpenter@linaro.org> wrote:

> We need to call mutex_unlock() on this error path.
> 
> Fixes: aad0394e7a02 ("wifi: rtw89: tweak driver architecture for impending MLO support")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> Acked-by: Zong-Zhe Yang <kevin_yang@realtek.com>

1 patch(es) applied to rtw-next branch of rtw.git, thanks.

ac4f4e5a2039 wifi: rtw89: unlock on error path in rtw89_ops_unassign_vif_chanctx()

---
https://github.com/pkshih/rtw.git