Re: [PATCH 1/3] Add BT3 AMP device support, by Atheros Linux BT3 team.

[Marcel Holtmann <marcel@holtmann.org>]

Hi Dan,

> > > Could you split all your patches in little chunks? +1882 is very hard to
> > > review. And please be more verbose on the commit message, it can help us
> > > figure out what's happening.
> > 
> > This used to be 1 large patch, it was recently split up to 3, I havent't
> > had a chance yet to review further but my understanding is this this was
> > split up as much as possible in the last try. If you can provide suggestions
> > how to split it up more that might be useful, but agreed, if possible more
> > splitup would help.
> 
> But it is still too big. I looked to patch 3/3, it does a lot different
> things in the same patch that make the patch impossible to be tracked.
> We need atomic patches for at least each change in the ERTM code, each
> new state added in the L2CAP state machine, each new HS feature, etc,
> i.e., we need proper patches with proper commit messages explaining
> what's happening. The same should apply to 1/3 and 2/3, I didn't looked
> to them.

please split these in smaller chunks. You can provide the full blown
patch on kernel.org as Luis did, but we need small chunks to get this
merged. It touches too many areas and I won't be able to do the full
review otherwise.

Patch 1/3 can be split properly. It doesn't have to be this big in the
first place.

 include/net/bluetooth/hci.h      |  342 +++++++++++++++++++-
 include/net/bluetooth/hci_core.h |  264 ++++++++++++++-
 net/bluetooth/cmtp/core.c        |    1 +
 net/bluetooth/hci_conn.c         |  201 +++++++++++-
 net/bluetooth/hci_core.c         |  349 ++++++++++++++++++--
 net/bluetooth/hci_event.c        |  698 +++++++++++++++++++++++++++++++++++++-
 6 files changed, 1822 insertions(+), 33 deletions(-)

Why are you touching CMTP. If that is needed, then send a patch up-front
that prepares this change. This just clutters it.

I see also thinks like this:

-#define HCI_OP_WRITE_SCAN_ENABLE       0x0c1a
+#define HCI_OP_WRITE_SCAN_ENABLE       0x0c1a

If you wanna do cleanups of already existing code, then please send
these cleanup patches first. Otherwise they just clutter the rest and I
will refuse to review them.

Same here:

-void hci_send_acl(struct hci_conn *conn, struct sk_buff *skb, __u16 flags);
-void hci_send_sco(struct hci_conn *conn, struct sk_buff *skb);
+int hci_send_acl(struct hci_conn *conn, struct sk_buff *skb, __u16 flags);
+int hci_send_sco(struct hci_conn *conn, struct sk_buff *skb);

Separate patch with proper commit message explaining why this is needed
etc. And then ensuring that it doesn't affect current code.

And for this you better have a good explanation:

 LIST_HEAD(hci_dev_list);
+EXPORT_SYMBOL(hci_dev_list);
+
 DEFINE_RWLOCK(hci_dev_list_lock);
+EXPORT_SYMBOL(hci_dev_list_lock);

 /* HCI callback list */
 LIST_HEAD(hci_cb_list);
+EXPORT_SYMBOL(hci_cb_list);
+
 DEFINE_RWLOCK(hci_cb_list_lock);
+EXPORT_SYMBOL(hci_cb_list_lock);

For every single symbol that you wanna export I expect at least a proper
paragraph in the commit message explaining why that is needed.

And then please one patch that just adds the new HCI definitions. And
form there on single steps with changing init behavior for BR/EDR and
AMP, adding the new flow control, adding physical link establishment.
All in single patches with proper commit messages please.

And this is only for patch 1/3.

After that I can have a look at the AMP Manager and the L2CAP changes.

> > Dan, can you also please drop the "by Atheros Linux BT3 team." on the subject
> > of the patches. This is already implied by the From and the Signed-off-by.
> 
> Dan, your patches don't apply cleanly upstream, please rebase them
> against Marcel's bluetooth-next tree and do the proper split of the
> patches.

I can update bluetooth-testing, but right it would be way better to base
them against the bluetooth-next tree. That will be the one, I ask John
to pull into his wireless-next tree in a bit.

Regards

Marcel