[WIP PATCH v2] iwd: Fix disabling tethering not working for brcmfmac

[WIP PATCH v2] iwd: Fix disabling tethering not working for brcmfmac

[Jonathan Liu]

For Broadcom BCM4356 chipset with brcmfmac driver, changing from AP mode
to station mode returns -EBUSY if the wireless interface is a member of
a bridge.

To resolve the issue, the wireless interface is removed from the tether
bridge before changing the mode rather than after.

Fixes: 648ed549f0ac ("iwd: Add support for tethering")
---
 plugins/iwd.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/plugins/iwd.c b/plugins/iwd.c
index ac3d1e17..25b9544d 100644
--- a/plugins/iwd.c
+++ b/plugins/iwd.c
@@ -745,6 +745,7 @@ static void tech_disable_tethering_cb(const DBusError *error, void *user_data)
 		goto out;
 	}
 
+	connman_technology_tethering_notify(cbd->tech, false);
 	iwdap = g_hash_table_lookup(access_points, iwdd->path);
 	if (!iwdap) {
 		DBG("%s no ap object found", iwdd->path);
@@ -756,11 +757,6 @@ static void tech_disable_tethering_cb(const DBusError *error, void *user_data)
 	iwdap->bridge = NULL;
 	iwdap->tech = NULL;
 
-	if (!connman_inet_remove_from_bridge(cbd->index, cbd->bridge))
-		goto out;
-
-	connman_technology_tethering_notify(cbd->tech, false);
-
 	if (!g_dbus_proxy_method_call(iwdap->proxy, "Stop",
 					NULL, tech_ap_stop_cb, cbd, NULL)) {
 		connman_warn("iwd ap %s could not stop AccessPoint mode: %s",
@@ -787,6 +783,9 @@ static int cm_change_tethering(struct iwd_device *iwdd,
 	if (index < 0)
 		return -ENODEV;
 
+	if (!enabled && connman_inet_remove_from_bridge(index, bridge))
+		return -EIO;
+
 	cbd = g_new(struct tech_cb_data, 1);
 	cbd->iwdd = iwdd;
 	cbd->path = g_strdup(iwdd->path);
-- 
2.35.1

Re: [WIP PATCH v2] iwd: Fix disabling tethering not working for brcmfmac

[Jonathan Liu]

Hi,

On Tue, 15 Feb 2022 at 21:27, Jonathan Liu <net147@gmail.com> wrote:
>
> For Broadcom BCM4356 chipset with brcmfmac driver, changing from AP mode
> to station mode returns -EBUSY if the wireless interface is a member of
> a bridge.
>
> To resolve the issue, the wireless interface is removed from the tether
> bridge before changing the mode rather than after.
>
> Fixes: 648ed549f0ac ("iwd: Add support for tethering")
> ---
>  plugins/iwd.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)

This is a work in progress patch. I needed to make these changes
otherwise disabling tethering would not work for my brcmfmac wireless.
This patch seems to result in a "no ap object found" debug message
when disabling tethering. It seems by the time it tries to lookup the
ap object in the access_points hash table in
tech_disable_tethering_cb() function, it has already been removed.
The connman_technology_tethering_notify(cbd->tech, false) line had to
be moved up before the ap object lookup as otherwise the lookup
failing would cause the tethering property to not be updated from true
to false when disabling tethering.

I guess that means Stop is not called on the iwd ap object as it was
already removed.

Any suggestions on how to improve this?

Thanks.

Regards,
Jonathan

Re: [WIP PATCH v2] iwd: Fix disabling tethering not working for brcmfmac

[Daniel Wagner]

Hi Jonathan,

On Tue, Feb 15, 2022 at 09:36:49PM +1100, Jonathan Liu wrote:
> Hi,
> 
> On Tue, 15 Feb 2022 at 21:27, Jonathan Liu <net147@gmail.com> wrote:
> >
> > For Broadcom BCM4356 chipset with brcmfmac driver, changing from AP mode
> > to station mode returns -EBUSY if the wireless interface is a member of
> > a bridge.
> >
> > To resolve the issue, the wireless interface is removed from the tether
> > bridge before changing the mode rather than after.
> >
> > Fixes: 648ed549f0ac ("iwd: Add support for tethering")
> > ---
> >  plugins/iwd.c | 9 ++++-----
> >  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> This is a work in progress patch. I needed to make these changes
> otherwise disabling tethering would not work for my brcmfmac wireless.
> This patch seems to result in a "no ap object found" debug message
> when disabling tethering. It seems by the time it tries to lookup the
> ap object in the access_points hash table in
> tech_disable_tethering_cb() function, it has already been removed.
> The connman_technology_tethering_notify(cbd->tech, false) line had to
> be moved up before the ap object lookup as otherwise the lookup
> failing would cause the tethering property to not be updated from true
> to false when disabling tethering.
> 
> I guess that means Stop is not called on the iwd ap object as it was
> already removed.
> 
> Any suggestions on how to improve this?

I think your approach is correct. The order of cleanup should be in the
in reverse to the setup steps. We add the interface to the bridge when
we get the Started signal. If I am not completely mistaken we should
first undo this operation and then call Stop.

The only thing I need to double check if we do not leak any resources
due to the -EIO I saw there when I quickly looked at.

Thanks,
Daniel

Re: [WIP PATCH v2] iwd: Fix disabling tethering not working for brcmfmac

[Jonathan Liu]

Hi Daniel,

On Mon, 21 Feb 2022 at 19:33, Daniel Wagner <wagi@monom.org> wrote:
>
> Hi Jonathan,
>
> On Tue, Feb 15, 2022 at 09:36:49PM +1100, Jonathan Liu wrote:
> > Hi,
> >
> > On Tue, 15 Feb 2022 at 21:27, Jonathan Liu <net147@gmail.com> wrote:
> > >
> > > For Broadcom BCM4356 chipset with brcmfmac driver, changing from AP mode
> > > to station mode returns -EBUSY if the wireless interface is a member of
> > > a bridge.
> > >
> > > To resolve the issue, the wireless interface is removed from the tether
> > > bridge before changing the mode rather than after.
> > >
> > > Fixes: 648ed549f0ac ("iwd: Add support for tethering")
> > > ---
> > >  plugins/iwd.c | 9 ++++-----
> > >  1 file changed, 4 insertions(+), 5 deletions(-)
> >
> > This is a work in progress patch. I needed to make these changes
> > otherwise disabling tethering would not work for my brcmfmac wireless.
> > This patch seems to result in a "no ap object found" debug message
> > when disabling tethering. It seems by the time it tries to lookup the
> > ap object in the access_points hash table in
> > tech_disable_tethering_cb() function, it has already been removed.
> > The connman_technology_tethering_notify(cbd->tech, false) line had to
> > be moved up before the ap object lookup as otherwise the lookup
> > failing would cause the tethering property to not be updated from true
> > to false when disabling tethering.
> >
> > I guess that means Stop is not called on the iwd ap object as it was
> > already removed.
> >
> > Any suggestions on how to improve this?
>
> I think your approach is correct. The order of cleanup should be in the
> in reverse to the setup steps. We add the interface to the bridge when
> we get the Started signal. If I am not completely mistaken we should
> first undo this operation and then call Stop.
>
> The only thing I need to double check if we do not leak any resources
> due to the -EIO I saw there when I quickly looked at.

Would you be willing to have an attempt at this as you are more
familiar with connman+iwd?
brcmfmac is quite common - it is used for Raspberry Pi for example.

Thanks.

Regards,
Jonathan

Re: [WIP PATCH v2] iwd: Fix disabling tethering not working for brcmfmac

[Daniel Wagner]

Hi Jonathan,

On Fri, Feb 25, 2022 at 07:33:22PM +1100, Jonathan Liu wrote:
> > I think your approach is correct. The order of cleanup should be in the
> > in reverse to the setup steps. We add the interface to the bridge when
> > we get the Started signal. If I am not completely mistaken we should
> > first undo this operation and then call Stop.
> >
> > The only thing I need to double check if we do not leak any resources
> > due to the -EIO I saw there when I quickly looked at.
> 
> Would you be willing to have an attempt at this as you are more
> familiar with connman+iwd?
> brcmfmac is quite common - it is used for Raspberry Pi for example.

I gave it a quick test run on my setup and guess what I was also getting
EBUSY without your patch. So this definitely fixes stuff. Let's see if my
worries of -EIO might cause issues. But we can deal with this later.

Thanks,
Daniel

Re: [WIP PATCH v2] iwd: Fix disabling tethering not working for brcmfmac

[Daniel Wagner]

On Tue, 15 Feb 2022 21:26:38 +1100, Jonathan Liu wrote:
> For Broadcom BCM4356 chipset with brcmfmac driver, changing from AP mode
> to station mode returns -EBUSY if the wireless interface is a member of
> a bridge.
> 
> To resolve the issue, the wireless interface is removed from the tether
> bridge before changing the mode rather than after.
> 
> [...]

Applied, thanks!

[1/1] iwd: Fix disabling tethering not working for brcmfmac
      commit: 5d1eebad7cffeae7d7b6b11d03d04f73bdd503bc

Best regards,
-- 
Daniel Wagner <wagi@monom.org>