Re: [PATCH 3/8] Bluetooth: Test for HCI_SETUP and HCI_USER_CHANNEL in mgmt_valid_hdev()

Hi Marcel,

On Sat, Sep 28, 2013, Marcel Holtmann wrote:

quoted

quoted

quoted

quoted

quoted

If either one of the HCI_SETUP or HCI_USER_CHANNEL flags is set the
device is not considered valid for mgmt. By having these checks inside
the mgmt_valid_hdev function the a couple of places using it can be
simplified.

I looked at doing this and decided not to. Reason was that the device
gets removed from mgmt anyway.

I'm not sure what significance you think "device gets removed from mgmt"
has. All that is is a mgmt event saying that index has been removed. In
addition to that we need to ensure that we don't send any more mgmt
events for such devices and that we don't include such devices in the
response to mgmt_read_index list.

These are the two places of the code that my patch simplifies, one is
the check for whether to send a "power on" mgmt event and the other the
response handling of read_index_list.

I have taken care of that part. It is done inside mgmt_control() and
in case of a controller in user channel mode it returns invalid index.

That's great, but it doesn't cover the read_index_list command which we
were talking about here since the command doesn't target any specific
controller (as per our mgmt-api.txt it has the value <non-controller> as
the controller index). The command handler will in this case be invoked
that that's where it'd be convenient to rely on mgmt_valid_hdev() for
constructing the response.

the read index list command has a special check for user channel. As I
said, I looked into this and opted against extending valid hdev macro
since we only need it in a few specific cases. Adding it there
resulted in simpler code then some magic macro that is used in more
places.

When I wrote the patches for user channel this felt cleaner. I had
patches for both ways, but decided to go this route.

Fair enough.

quoted

quoted

For the events, I have no idea on how we would generate events if we
are not going through hci_event.c when the user channel is active.

The "powered on" event in question doesn't come from hci_event.c but
from the hci_dev_open() function, so again this is not covered by the
existing user channel checks.

How would you trigger a power on event. When user channel is active
everything else is blocked. So every other operation will fail. That
is taken care of.

In addition you can only activate user channel on a controller that is
powered off. That was a conscious design decision. Keep it really
simple. User channel mode is exclusive and exclusive to one user at a
time.

Good point. The only hci_dev_open() call that comes to mind is the one
automatically triggered by the setup stage, but user space doesn't yet
know about the controller at that point.

Johan