Re: [PATCHv1 11/12] Bluetooth: AMP: Use l2cap_physical_cfm in phylink complete evt

Hi Mat,

On Tue, Nov 13, 2012 at 09:29:38AM -0800, Mat Martineau wrote:

quoted

quoted

quoted

quoted

quoted

+void amp_physical_cfm(struct hci_conn *bredr_hcon, struct hci_conn *hs_hcon)
+{
+	struct hci_dev *bredr_hdev = hci_dev_hold(bredr_hcon->hdev);
+	struct amp_mgr *mgr = hs_hcon->amp_mgr;
+	struct l2cap_chan *bredr_chan;
+
+	BT_DBG("bredr_hcon %p hs_hcon %p mgr %p", bredr_hcon, hs_hcon, mgr);
+
+	if (!bredr_hdev || !mgr || !mgr->bredr_chan)
+		return;
+
+	bredr_chan = mgr->bredr_chan;
+
+	set_bit(FLAG_EFS_ENABLE, &bredr_chan->flags);
+	bredr_chan->ctrl_id = hs_hcon->remote_id;
+	bredr_chan->hs_hcon = hs_hcon;
+	bredr_chan->conn->mtu = hs_hcon->hdev->block_mtu;
+	bredr_chan->fcs = L2CAP_FCS_NONE;

Changing FCS requires L2CAP reconfiguration for the channel, and
chan->fcs shouldn't be modified until reconfiguration happens.
While it doesn't make much sense to do so, the remote device may
want to keep FCS enabled.  The move may also fail and you don't want
to forget the original FCS setting in that case.

So we agree that FCS shall not be used for High Speed channels. I was
thinking more about the case where we start sending data right over HS
channel. The configuration should be just started.

No matter what the BlueZ implementation prefers, it must be able to
handle a remote device that requests FCS during L2CAP config.  If
one side requests FCS, then it must be used.

The idea here is to disable FCS on our side, we would still respect FCS
requests from remote side. This code needs to be moved to l2cap_do_create.

Do we want BlueZ to always ignore the L2CAP FCS socket option on AMP
controllers?  (Marcel?)  This checksum is always redundant with
802.11 AMP controllers.

quoted

What would be the better place to init FCS? l2cap_physical_cfm after
checking channel moving status?

I think it should be as late as possible before L2CAP configuration
begins in order to be sure the channel is really on AMP.  For the
"create channel" case, set_default_fcs could see if an AMP link is
being used and turn the FCS off.

set_default_fcs sets FCS when configuration is finished, this way we
cannot respect other side FCS settings.

Best regards 
Andrei Emeltchenko