The plugin allows us to restore the previous power state on
adapters when the killswitch on them has been unblocked.
Otherwise we end up with the adapter disabled when coming back from a
soft killswitch.
Cheers
Hi Marcel,
On Thu, Jul 30, 2009, Marcel Holtmann wrote:
> > So having a btd_adapter_set_powered exported to plugins (which is what
> > Bastien's patch seems to do) makes sense to me in this case. I might
> > actually need something similar for maemo in order to handle our offline
> > mode better (maemo specific plugin to catch the MCE offline mode signal
> > and then call btd_adapter_set_powered).
>
> then I leave this with you to export that function correctly. It should
> be a separate patch anyway.
Ok. Maybe Bastien can split this out from his original patch? Note that
the function should probably have the btd_ prefix since the idea has been
that only functions like that get exported to plugins (not sure if it's
actually enforced yet though).
> I would disagree with the MCE offline handling as a plugin. First off
> all, why is it not integrated into RFKILL. Second off all, it could just
> use RFKILL or RememberPowered=false and change Powered by itself. The
> listening to offline signal is the wrong design.
You're probably right, however this is what I have to live with in the
current maemo version. I've added David to CC so he can comment on the
possibility of replacing MCE offline/normal mode signals with rfkill in
the future.
Johan
Hi Johan,
> > > > Hijacking the set_powered D-Bus command is the
> > > > wrong approach. We need a properly exported adapter_up() function for
> > > > this.
> > >
> > > I believe I tried exporting adapter_up() but it didn't work.
> >
> > Johan, Luiz, any reason why this would not work. What needs to be done
> > to bring up the adapter. Besides calling the ioctl() directly which we
> > don't wanna do anymore.
>
> adapter_up() is more of a callback that's responsible for doing the
> necessary initializations *after* adapter has just gone up, so it's not
> the right function to call when you want to bring it up (i.e. call the
> ioctl). I believe all code paths for bringing the adapter up call set_mode
> in src/adapter.c which in turn calls adapter_ops->set_powered (which calls
> the ioctl in the case of hciops).
>
> So having a btd_adapter_set_powered exported to plugins (which is what
> Bastien's patch seems to do) makes sense to me in this case. I might
> actually need something similar for maemo in order to handle our offline
> mode better (maemo specific plugin to catch the MCE offline mode signal
> and then call btd_adapter_set_powered).
then I leave this with you to export that function correctly. It should
be a separate patch anyway.
I would disagree with the MCE offline handling as a plugin. First off
all, why is it not integrated into RFKILL. Second off all, it could just
use RFKILL or RememberPowered=false and change Powered by itself. The
listening to offline signal is the wrong design.
Regards
Marcel
Hi Luiz,
> > adapter_up() is more of a callback that's responsible for doing the
> > necessary initializations *after* adapter has just gone up, so it's not
> > the right function to call when you want to bring it up (i.e. call the
> > ioctl). I believe all code paths for bringing the adapter up call set_mode
> > in src/adapter.c which in turn calls adapter_ops->set_powered (which calls
> > the ioctl in the case of hciops).
> >
> > So having a btd_adapter_set_powered exported to plugins (which is what
> > Bastien's patch seems to do) makes sense to me in this case. I might
> > actually need something similar for maemo in order to handle our offline
> > mode better (maemo specific plugin to catch the MCE offline mode signal
> > and then call btd_adapter_set_powered).
>
> Yep, sounds good to me too, plugins can have references to adapters so
> I guess this is perfectly fine. Also I don't think there is much to be
> protected here since powered property is readwrite and can be changed
> by any dbus client. This also makes me wonder what is the purpose of
> rfkill when we can anyone can set powered directly?
if it is rfkilled, then hciconfig hci0 up will return -ERFKILL. We fixed
this for the 2.6.31 kernel. WiFi behaves the same now. So you can't just
go ahead and bring it up again, you do have to unblock it. And you can
only unblock softkilled devices. If it is hardkilled via a hardware
switch for example, there is nothing you can do to bring it back up
except flipping the hardware switch.
Regards
Marcel
Hi,
On Wed, Jul 29, 2009 at 6:44 PM, Johan Hedberg<[email protected]> wro=
te:
> adapter_up() is more of a callback that's responsible for doing the
> necessary initializations *after* adapter has just gone up, so it's not
> the right function to call when you want to bring it up (i.e. call the
> ioctl). I believe all code paths for bringing the adapter up call set_mod=
e
> in src/adapter.c which in turn calls adapter_ops->set_powered (which call=
s
> the ioctl in the case of hciops).
>
> So having a btd_adapter_set_powered exported to plugins (which is what
> Bastien's patch seems to do) makes sense to me in this case. I might
> actually need something similar for maemo in order to handle our offline
> mode better (maemo specific plugin to catch the MCE offline mode signal
> and then call btd_adapter_set_powered).
Yep, sounds good to me too, plugins can have references to adapters so
I guess this is perfectly fine. Also I don't think there is much to be
protected here since powered property is readwrite and can be changed
by any dbus client. This also makes me wonder what is the purpose of
rfkill when we can anyone can set powered directly?
--=20
Luiz Augusto von Dentz
Engenheiro de Computa=E7=E3o
On Thu, 2009-07-30 at 00:44 +0300, Johan Hedberg wrote:
> Hi Marcel,
>
> On Wed, Jul 29, 2009, Marcel Holtmann wrote:
> > > > Hijacking the set_powered D-Bus command is the
> > > > wrong approach. We need a properly exported adapter_up() function for
> > > > this.
> > >
> > > I believe I tried exporting adapter_up() but it didn't work.
> >
> > Johan, Luiz, any reason why this would not work. What needs to be done
> > to bring up the adapter. Besides calling the ioctl() directly which we
> > don't wanna do anymore.
>
> adapter_up() is more of a callback that's responsible for doing the
> necessary initializations *after* adapter has just gone up, so it's not
> the right function to call when you want to bring it up (i.e. call the
> ioctl). I believe all code paths for bringing the adapter up call set_mode
> in src/adapter.c which in turn calls adapter_ops->set_powered (which calls
> the ioctl in the case of hciops).
>
> So having a btd_adapter_set_powered exported to plugins (which is what
> Bastien's patch seems to do) makes sense to me in this case. I might
> actually need something similar for maemo in order to handle our offline
> mode better (maemo specific plugin to catch the MCE offline mode signal
> and then call btd_adapter_set_powered).
Hence why it would probably be a good thing to have as a plugin :)
Hi Marcel,
On Wed, Jul 29, 2009, Marcel Holtmann wrote:
> > > Hijacking the set_powered D-Bus command is the
> > > wrong approach. We need a properly exported adapter_up() function for
> > > this.
> >
> > I believe I tried exporting adapter_up() but it didn't work.
>
> Johan, Luiz, any reason why this would not work. What needs to be done
> to bring up the adapter. Besides calling the ioctl() directly which we
> don't wanna do anymore.
adapter_up() is more of a callback that's responsible for doing the
necessary initializations *after* adapter has just gone up, so it's not
the right function to call when you want to bring it up (i.e. call the
ioctl). I believe all code paths for bringing the adapter up call set_mode
in src/adapter.c which in turn calls adapter_ops->set_powered (which calls
the ioctl in the case of hciops).
So having a btd_adapter_set_powered exported to plugins (which is what
Bastien's patch seems to do) makes sense to me in this case. I might
actually need something similar for maemo in order to handle our offline
mode better (maemo specific plugin to catch the MCE offline mode signal
and then call btd_adapter_set_powered).
Johan
On Wed, 2009-07-29 at 22:23 +0200, Marcel Holtmann wrote:
> Hi Bastien,
>
> > > > > The plugin allows us to restore the previous power state on
> > > > > adapters when the killswitch on them has been unblocked.
> > > > >
> > > > > Otherwise we end up with the adapter disabled when coming back from a
> > > > > soft killswitch.
> > > >
> > > > Just a note that __u32 causes failures with newer GCCs (not sure why),
> > > > using uint32_t instead fixes the problem.
> > > >
> > > > I'll send another patch when this one gets committed.
> > >
> > > I didn't commit this patch, because it should be part of bluetoothd and
> > > not a plugin
> >
> > You said you didn't mind:
> > http://thread.gmane.org/gmane.linux.bluez.kernel/2981/focus=2986
> > > Okay. Since we have built-in plugins, it makes no big difference.
> >
> > > and as I mentioned before honor the RememberPowered setting
> > > for system where other entities control that.
> >
> > That's what that does:
> > + if (main_opts.remember_powered == FALSE)
> > + return 0;
> >
> > > However I did push a RFKILL skeleton that that all the lifting except
> > > bringing the adapter up.
> >
> > Which is pretty much the same as the code I sent, and the code in
> > connman. What's the point of putting this in the core when it does the
> > exact same as the plugin save for a level of indirection?
>
> I changed my mind, because of the main_opts. We don't wanna export the
> options in the long term. So using src/rfkill.c seems more logical at
> this point of time.
We could have exported some of the settings through manager_get_*(), for
example manager_get_remember_powered(), for use by plugins.
The point is moot now anyway.
Hi Bastien,
> > > > The plugin allows us to restore the previous power state on
> > > > adapters when the killswitch on them has been unblocked.
> > > >
> > > > Otherwise we end up with the adapter disabled when coming back from a
> > > > soft killswitch.
> > >
> > > Just a note that __u32 causes failures with newer GCCs (not sure why),
> > > using uint32_t instead fixes the problem.
> > >
> > > I'll send another patch when this one gets committed.
> >
> > I didn't commit this patch, because it should be part of bluetoothd and
> > not a plugin
>
> You said you didn't mind:
> http://thread.gmane.org/gmane.linux.bluez.kernel/2981/focus=2986
> > Okay. Since we have built-in plugins, it makes no big difference.
>
> > and as I mentioned before honor the RememberPowered setting
> > for system where other entities control that.
>
> That's what that does:
> + if (main_opts.remember_powered == FALSE)
> + return 0;
>
> > However I did push a RFKILL skeleton that that all the lifting except
> > bringing the adapter up.
>
> Which is pretty much the same as the code I sent, and the code in
> connman. What's the point of putting this in the core when it does the
> exact same as the plugin save for a level of indirection?
I changed my mind, because of the main_opts. We don't wanna export the
options in the long term. So using src/rfkill.c seems more logical at
this point of time.
> > Hijacking the set_powered D-Bus command is the
> > wrong approach. We need a properly exported adapter_up() function for
> > this.
>
> I believe I tried exporting adapter_up() but it didn't work.
Johan, Luiz, any reason why this would not work. What needs to be done
to bring up the adapter. Besides calling the ioctl() directly which we
don't wanna do anymore.
Regards
Marcel
On Wed, 2009-07-29 at 21:49 +0200, Marcel Holtmann wrote:
> Hi Bastien,
>
> > > The plugin allows us to restore the previous power state on
> > > adapters when the killswitch on them has been unblocked.
> > >
> > > Otherwise we end up with the adapter disabled when coming back from a
> > > soft killswitch.
> >
> > Just a note that __u32 causes failures with newer GCCs (not sure why),
> > using uint32_t instead fixes the problem.
> >
> > I'll send another patch when this one gets committed.
>
> I didn't commit this patch, because it should be part of bluetoothd and
> not a plugin
You said you didn't mind:
http://thread.gmane.org/gmane.linux.bluez.kernel/2981/focus=2986
> Okay. Since we have built-in plugins, it makes no big difference.
> and as I mentioned before honor the RememberPowered setting
> for system where other entities control that.
That's what that does:
+ if (main_opts.remember_powered == FALSE)
+ return 0;
> However I did push a RFKILL skeleton that that all the lifting except
> bringing the adapter up.
Which is pretty much the same as the code I sent, and the code in
connman. What's the point of putting this in the core when it does the
exact same as the plugin save for a level of indirection?
> Hijacking the set_powered D-Bus command is the
> wrong approach. We need a properly exported adapter_up() function for
> this.
I believe I tried exporting adapter_up() but it didn't work.
Hi Bastien,
> > The plugin allows us to restore the previous power state on
> > adapters when the killswitch on them has been unblocked.
> >
> > Otherwise we end up with the adapter disabled when coming back from a
> > soft killswitch.
>
> Just a note that __u32 causes failures with newer GCCs (not sure why),
> using uint32_t instead fixes the problem.
>
> I'll send another patch when this one gets committed.
I didn't commit this patch, because it should be part of bluetoothd and
not a plugin and as I mentioned before honor the RememberPowered setting
for system where other entities control that.
However I did push a RFKILL skeleton that that all the lifting except
bringing the adapter up. Hijacking the set_powered D-Bus command is the
wrong approach. We need a properly exported adapter_up() function for
this.
Regards
Marcel
On Tue, 2009-07-28 at 17:34 +0100, Bastien Nocera wrote:
> The plugin allows us to restore the previous power state on
> adapters when the killswitch on them has been unblocked.
>
> Otherwise we end up with the adapter disabled when coming back from a
> soft killswitch.
Just a note that __u32 causes failures with newer GCCs (not sure why),
using uint32_t instead fixes the problem.
I'll send another patch when this one gets committed.
Hi Bastien,
> > > The plugin allows us to restore the previous power state on
> > > adapters when the killswitch on them has been unblocked.
> > >
> > > Otherwise we end up with the adapter disabled when coming back from a
> > > soft killswitch.
> >
> > I think that having this as a plugin is overkill. I would just integrate
> > this as src/rfkill.c. Especially since it has no dependencies.
>
> It would mean it's harder to change its behaviour. Surely, it would make
> more sense for this to be a plugin than for HCI operations to be.
the HCI operation are becoming/are a plugin, because we wanna switch to
netlink in the long term. However to do this, we need to separate them
first.
> Given that you don't use an object system with typing, it's probably one
> or 2 function calls off. To me, it makes sense to have parts of the core
> as plugins if it means that the core is leaner because of it.
Okay. Since we have built-in plugins, it makes no big difference.
Regards
Marcel
On Tue, 2009-07-28 at 22:06 +0200, Marcel Holtmann wrote:
> Hi Bastien,
>
> > The plugin allows us to restore the previous power state on
> > adapters when the killswitch on them has been unblocked.
> >
> > Otherwise we end up with the adapter disabled when coming back from a
> > soft killswitch.
>
> I think that having this as a plugin is overkill. I would just integrate
> this as src/rfkill.c. Especially since it has no dependencies.
It would mean it's harder to change its behaviour. Surely, it would make
more sense for this to be a plugin than for HCI operations to be.
Given that you don't use an object system with typing, it's probably one
or 2 function calls off. To me, it makes sense to have parts of the core
as plugins if it means that the core is leaner because of it.
Hi Bastien,
> The plugin allows us to restore the previous power state on
> adapters when the killswitch on them has been unblocked.
>
> Otherwise we end up with the adapter disabled when coming back from a
> soft killswitch.
I think that having this as a plugin is overkill. I would just integrate
this as src/rfkill.c. Especially since it has no dependencies.
Regards
Marcel