mbox series

pull request: bluetooth-next 2022-07-22

Message ID 20220722205400.847019-1-luiz.dentz@gmail.com (mailing list archive)
State Superseded
Headers show
Series pull request: bluetooth-next 2022-07-22 | expand

Pull-request

git://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git tags/for-net-next-2022-07-22

Message

Luiz Augusto von Dentz July 22, 2022, 8:54 p.m. UTC
The following changes since commit 6e0e846ee2ab01bc44254e6a0a6a6a0db1cba16d:

  Merge git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net (2022-07-21 13:03:39 -0700)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git tags/for-net-next-2022-07-22

for you to fetch changes up to 768677808478ee7ffabf9c9128f345b7ec62b5f3:

  Bluetooth: btusb: Detect if an ACL packet is in fact an ISO packet (2022-07-22 13:24:55 -0700)

----------------------------------------------------------------
bluetooth-next pull request for net-next:

 - Add support for IM Networks PID 0x3568
 - Add support for BCM4349B1
 - Add support for CYW55572
 - Add support for MT7922 VID/PID 0489/e0e2
 - Add support for Realtek RTL8852C
 - Initial support for Isochronous Channels/ISO sockets
 - Remove HCI_QUIRK_BROKEN_ERR_DATA_REPORTING quirk

----------------------------------------------------------------
Aaron Ma (1):
      Bluetooth: btusb: Add support of IMC Networks PID 0x3568

Abhishek Pandit-Subedi (2):
      Bluetooth: Fix index added after unregister
      Bluetooth: Unregister suspend with userchannel

Ahmad Fatoum (2):
      dt-bindings: bluetooth: broadcom: Add BCM4349B1 DT binding
      Bluetooth: hci_bcm: Add BCM4349B1 variant

Alain Michaud (1):
      Bluetooth: clear the temporary linkkey in hci_conn_cleanup

Brian Gix (3):
      Bluetooth: Remove dead code from hci_request.c
      Bluetooth: Remove update_scan hci_request dependancy
      Bluetooth: Convert delayed discov_off to hci_sync

Dan Carpenter (2):
      Bluetooth: fix an error code in hci_register_dev()
      Bluetooth: clean up error pointer checking

Hakan Jansson (7):
      dt-bindings: net: broadcom-bluetooth: Add property for autobaud mode
      Bluetooth: hci_bcm: Add support for FW loading in autobaud mode
      dt-bindings: net: broadcom-bluetooth: Add CYW55572 DT binding
      dt-bindings: net: broadcom-bluetooth: Add conditional constraints
      Bluetooth: hci_bcm: Add DT compatible for CYW55572
      Bluetooth: hci_bcm: Prevent early baudrate setting in autobaud mode
      Bluetooth: hci_bcm: Increase host baudrate for CYW55572 in autobaud mode

He Wang (1):
      Bluetooth: btusb: Add a new VID/PID 0489/e0e2 for MT7922

Hilda Wu (5):
      Bluetooth: btusb: Add Realtek RTL8852C support ID 0x04CA:0x4007
      Bluetooth: btusb: Add Realtek RTL8852C support ID 0x04C5:0x1675
      Bluetooth: btusb: Add Realtek RTL8852C support ID 0x0CB8:0xC558
      Bluetooth: btusb: Add Realtek RTL8852C support ID 0x13D3:0x3587
      Bluetooth: btusb: Add Realtek RTL8852C support ID 0x13D3:0x3586

Jiasheng Jiang (1):
      Bluetooth: hci_intel: Add check for platform_driver_register

Luiz Augusto von Dentz (16):
      Bluetooth: eir: Fix using strlen with hdev->{dev_name,short_name}
      Bluetooth: HCI: Fix not always setting Scan Response/Advertising Data
      Bluetooth: hci_sync: Fix not updating privacy_mode
      Bluetooth: hci_sync: Don't remove connected devices from accept list
      Bluetooth: hci_sync: Split hci_dev_open_sync
      Bluetooth: Add bt_status
      Bluetooth: Use bt_status to convert from errno
      Bluetooth: mgmt: Fix using hci_conn_abort
      Bluetooth: MGMT: Fix holding hci_conn reference while command is queued
      Bluetooth: hci_core: Introduce hci_recv_event_data
      Bluetooth: Add initial implementation of CIS connections
      Bluetooth: Add BTPROTO_ISO socket type
      Bluetooth: Add initial implementation of BIS connections
      Bluetooth: ISO: Add broadcast support
      Bluetooth: btusb: Add support for ISO packets
      Bluetooth: btusb: Detect if an ACL packet is in fact an ISO packet

Manish Mandlik (2):
      Bluetooth: hci_sync: Refactor add Adv Monitor
      Bluetooth: hci_sync: Refactor remove Adv Monitor

Sai Teja Aluvala (1):
      Bluetooth: hci_qca: Return wakeup for qca_wakeup

Schspa Shi (1):
      Bluetooth: When HCI work queue is drained, only queue chained work

Sean Wang (1):
      Bluetooth: btmtksdio: Add in-band wakeup support

Tamas Koczka (1):
      Bluetooth: Collect kcov coverage from hci_rx_work

Xiaohui Zhang (1):
      Bluetooth: use memset avoid memory leaks

Ying Hsu (1):
      Bluetooth: Add default wakeup callback for HCI UART driver

Yuri D'Elia (1):
      Bluetooth: btusb: Set HCI_QUIRK_BROKEN_ENHANCED_SETUP_SYNC_CONN for MTK

Zhengping Jiang (2):
      Bluetooth: mgmt: Fix refresh cached connection info
      Bluetooth: hci_sync: Fix resuming scan after suspend resume

Zijun Hu (5):
      Bluetooth: hci_sync: Correct hci_set_event_mask_page_2_sync() event mask
      Bluetooth: hci_sync: Check LMP feature bit instead of quirk
      Bluetooth: btusb: Remove HCI_QUIRK_BROKEN_ERR_DATA_REPORTING for QCA
      Bluetooth: btusb: Remove HCI_QUIRK_BROKEN_ERR_DATA_REPORTING for fake CSR
      Bluetooth: hci_sync: Remove HCI_QUIRK_BROKEN_ERR_DATA_REPORTING

shaomin Deng (1):
      Bluetooth: btrtl: Fix typo in comment

 .../bindings/net/broadcom-bluetooth.yaml           |   25 +
 drivers/bluetooth/btbcm.c                          |   33 +-
 drivers/bluetooth/btbcm.h                          |    8 +-
 drivers/bluetooth/btmtksdio.c                      |   15 +
 drivers/bluetooth/btrtl.c                          |    2 +-
 drivers/bluetooth/btusb.c                          |   45 +-
 drivers/bluetooth/hci_bcm.c                        |   35 +-
 drivers/bluetooth/hci_intel.c                      |    6 +-
 drivers/bluetooth/hci_qca.c                        |    2 +-
 drivers/bluetooth/hci_serdev.c                     |   11 +
 include/net/bluetooth/bluetooth.h                  |   71 +-
 include/net/bluetooth/hci.h                        |  203 ++-
 include/net/bluetooth/hci_core.h                   |  234 ++-
 include/net/bluetooth/hci_sock.h                   |    2 +
 include/net/bluetooth/hci_sync.h                   |   16 +
 include/net/bluetooth/iso.h                        |   32 +
 net/bluetooth/Kconfig                              |    1 +
 net/bluetooth/Makefile                             |    1 +
 net/bluetooth/af_bluetooth.c                       |    4 +-
 net/bluetooth/eir.c                                |   62 +-
 net/bluetooth/eir.h                                |    1 +
 net/bluetooth/hci_conn.c                           |  900 +++++++++-
 net/bluetooth/hci_core.c                           |  569 ++++--
 net/bluetooth/hci_event.c                          |  529 +++++-
 net/bluetooth/hci_request.c                        |  429 +----
 net/bluetooth/hci_request.h                        |   16 +-
 net/bluetooth/hci_sock.c                           |   11 +-
 net/bluetooth/hci_sync.c                           |  628 +++++--
 net/bluetooth/iso.c                                | 1824 ++++++++++++++++++++
 net/bluetooth/l2cap_core.c                         |    1 +
 net/bluetooth/lib.c                                |   71 +
 net/bluetooth/mgmt.c                               |  338 ++--
 net/bluetooth/msft.c                               |  269 +--
 net/bluetooth/msft.h                               |    6 +-
 34 files changed, 5224 insertions(+), 1176 deletions(-)
 create mode 100644 include/net/bluetooth/iso.h
 create mode 100644 net/bluetooth/iso.c

Comments

Jakub Kicinski July 22, 2022, 11:55 p.m. UTC | #1
On Fri, 22 Jul 2022 13:54:00 -0700 Luiz Augusto von Dentz wrote:
> The following changes since commit 6e0e846ee2ab01bc44254e6a0a6a6a0db1cba16d:
> 
>   Merge git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net (2022-07-21 13:03:39 -0700)
> 
> are available in the Git repository at:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git tags/for-net-next-2022-07-22
> 
> for you to fetch changes up to 768677808478ee7ffabf9c9128f345b7ec62b5f3:
> 
>   Bluetooth: btusb: Detect if an ACL packet is in fact an ISO packet (2022-07-22 13:24:55 -0700)
> 
> ----------------------------------------------------------------
> bluetooth-next pull request for net-next:
> 
>  - Add support for IM Networks PID 0x3568
>  - Add support for BCM4349B1
>  - Add support for CYW55572
>  - Add support for MT7922 VID/PID 0489/e0e2
>  - Add support for Realtek RTL8852C
>  - Initial support for Isochronous Channels/ISO sockets
>  - Remove HCI_QUIRK_BROKEN_ERR_DATA_REPORTING quirk

I see two new sparse warnings (for a follow up):

net/bluetooth/hci_event.c:3789:26: warning: cast to restricted __le16
net/bluetooth/hci_event.c:3791:26: warning: cast to restricted __le16

Two bad Fixes tags:

Commit: 68253f3cd715 ("Bluetooth: hci_sync: Fix resuming scan after suspend resume")
	Fixes tag: Fixes: 3b42055388c30 (Bluetooth: hci_sync: Fix attempting to suspend with
	Has these problem(s):
		- Subject has leading but no trailing parentheses
Commit: 9111786492f1 ("Bluetooth: fix an error code in hci_register_dev()")
	Fixes tag: Fixes: d6bb2a91f95b ("Bluetooth: Unregister suspend with userchannel")
	Has these problem(s):
		- Target SHA1 does not exist

And a whole bunch of patches committed by you but signed off by Marcel.
Last time we tried to fix that it ended up making things worse.
So I guess it is what it is :) Pulling...
Luiz Augusto von Dentz July 23, 2022, 12:09 a.m. UTC | #2
Hi Jakub,

On Fri, Jul 22, 2022 at 4:55 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Fri, 22 Jul 2022 13:54:00 -0700 Luiz Augusto von Dentz wrote:
> > The following changes since commit 6e0e846ee2ab01bc44254e6a0a6a6a0db1cba16d:
> >
> >   Merge git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net (2022-07-21 13:03:39 -0700)
> >
> > are available in the Git repository at:
> >
> >   git://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git tags/for-net-next-2022-07-22
> >
> > for you to fetch changes up to 768677808478ee7ffabf9c9128f345b7ec62b5f3:
> >
> >   Bluetooth: btusb: Detect if an ACL packet is in fact an ISO packet (2022-07-22 13:24:55 -0700)
> >
> > ----------------------------------------------------------------
> > bluetooth-next pull request for net-next:
> >
> >  - Add support for IM Networks PID 0x3568
> >  - Add support for BCM4349B1
> >  - Add support for CYW55572
> >  - Add support for MT7922 VID/PID 0489/e0e2
> >  - Add support for Realtek RTL8852C
> >  - Initial support for Isochronous Channels/ISO sockets
> >  - Remove HCI_QUIRK_BROKEN_ERR_DATA_REPORTING quirk
>
> I see two new sparse warnings (for a follow up):
>
> net/bluetooth/hci_event.c:3789:26: warning: cast to restricted __le16
> net/bluetooth/hci_event.c:3791:26: warning: cast to restricted __le16

Crap, let me fix them.

> Two bad Fixes tags:
>
> Commit: 68253f3cd715 ("Bluetooth: hci_sync: Fix resuming scan after suspend resume")
>         Fixes tag: Fixes: 3b42055388c30 (Bluetooth: hci_sync: Fix attempting to suspend with
>         Has these problem(s):
>                 - Subject has leading but no trailing parentheses
> Commit: 9111786492f1 ("Bluetooth: fix an error code in hci_register_dev()")
>         Fixes tag: Fixes: d6bb2a91f95b ("Bluetooth: Unregister suspend with userchannel")
>         Has these problem(s):
>                 - Target SHA1 does not exist
>
> And a whole bunch of patches committed by you but signed off by Marcel.
> Last time we tried to fix that it ended up making things worse.
> So I guess it is what it is :) Pulling...

Yep, that happens when I rebase on top of net-next so I would have to
redo all the Signed-off-by lines if the patches were originally
applied by Marcel, at least I don't know of any option to keep the
original committer while rebasing?
Jakub Kicinski July 23, 2022, 12:19 a.m. UTC | #3
On Fri, 22 Jul 2022 17:09:33 -0700 Luiz Augusto von Dentz wrote:
> > I see two new sparse warnings (for a follow up):
> >
> > net/bluetooth/hci_event.c:3789:26: warning: cast to restricted __le16
> > net/bluetooth/hci_event.c:3791:26: warning: cast to restricted __le16  
> 
> Crap, let me fix them.

Do you mean i should hold off with pushing or you'll follow up?

> > Two bad Fixes tags:
> >
> > Commit: 68253f3cd715 ("Bluetooth: hci_sync: Fix resuming scan after suspend resume")
> >         Fixes tag: Fixes: 3b42055388c30 (Bluetooth: hci_sync: Fix attempting to suspend with
> >         Has these problem(s):
> >                 - Subject has leading but no trailing parentheses
> > Commit: 9111786492f1 ("Bluetooth: fix an error code in hci_register_dev()")
> >         Fixes tag: Fixes: d6bb2a91f95b ("Bluetooth: Unregister suspend with userchannel")
> >         Has these problem(s):
> >                 - Target SHA1 does not exist
> >
> > And a whole bunch of patches committed by you but signed off by Marcel.
> > Last time we tried to fix that it ended up making things worse.
> > So I guess it is what it is :) Pulling...  
> 
> Yep, that happens when I rebase on top of net-next so I would have to
> redo all the Signed-off-by lines if the patches were originally
> applied by Marcel, at least I don't know of any option to keep the
> original committer while rebasing?

I think the most common way is to avoid rebasing. Do you rebase to get
rid of revised patches or such?
Luiz Augusto von Dentz July 23, 2022, 12:25 a.m. UTC | #4
Hi Jakub,

On Fri, Jul 22, 2022 at 5:19 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Fri, 22 Jul 2022 17:09:33 -0700 Luiz Augusto von Dentz wrote:
> > > I see two new sparse warnings (for a follow up):
> > >
> > > net/bluetooth/hci_event.c:3789:26: warning: cast to restricted __le16
> > > net/bluetooth/hci_event.c:3791:26: warning: cast to restricted __le16
> >
> > Crap, let me fix them.
>
> Do you mean i should hold off with pushing or you'll follow up?

Ive just fixup the original patch that introduced it, btw how do you
run sparse to capture such errors?

> > > Two bad Fixes tags:
> > >
> > > Commit: 68253f3cd715 ("Bluetooth: hci_sync: Fix resuming scan after suspend resume")
> > >         Fixes tag: Fixes: 3b42055388c30 (Bluetooth: hci_sync: Fix attempting to suspend with
> > >         Has these problem(s):
> > >                 - Subject has leading but no trailing parentheses
> > > Commit: 9111786492f1 ("Bluetooth: fix an error code in hci_register_dev()")
> > >         Fixes tag: Fixes: d6bb2a91f95b ("Bluetooth: Unregister suspend with userchannel")
> > >         Has these problem(s):
> > >                 - Target SHA1 does not exist
> > >
> > > And a whole bunch of patches committed by you but signed off by Marcel.
> > > Last time we tried to fix that it ended up making things worse.
> > > So I guess it is what it is :) Pulling...
> >
> > Yep, that happens when I rebase on top of net-next so I would have to
> > redo all the Signed-off-by lines if the patches were originally
> > applied by Marcel, at least I don't know of any option to keep the
> > original committer while rebasing?
>
> I think the most common way is to avoid rebasing. Do you rebase to get
> rid of revised patches or such?

So we don't need to rebase? There were some patches already applied
via bluetooth.git so at least I do it to remove them and any possible
conflicts if there were changes introduced to the bluetooth
directories that can eventually come from some other tree.
Jakub Kicinski July 23, 2022, 12:50 a.m. UTC | #5
On Fri, 22 Jul 2022 17:25:57 -0700 Luiz Augusto von Dentz wrote:
> > > Crap, let me fix them.  
> >
> > Do you mean i should hold off with pushing or you'll follow up?  
> 
> Ive just fixup the original patch that introduced it, btw how do you
> run sparse to capture such errors?

We run builds with W=1 C=1 in the CI and then diff the outputs.
That's pretty noisy so we have a regex which counts number of
warnings per file, that makes it possible to locate the exact new
warning. At least most of the time...

> > > Yep, that happens when I rebase on top of net-next so I would have to
> > > redo all the Signed-off-by lines if the patches were originally
> > > applied by Marcel, at least I don't know of any option to keep the
> > > original committer while rebasing?  
> >
> > I think the most common way is to avoid rebasing. Do you rebase to get
> > rid of revised patches or such?  
> 
> So we don't need to rebase?

No, not usually. After we pull from you, you should pull back from us 
(git pull --ff-only $net-or-net-next depending on the tree you
targeted), and that's it. The only patches that go into your tree then
are bluetooth patches, everything else is fed via pulling back from us.

> There were some patches already applied via bluetooth.git so at least
> I do it to remove them 

Normally you'd not apply bluetooth fixes to bluetooth-next, apply 
them to bluetooth and send us a PR. Then once a week we'll merge
net (containing your fixes) into net-next, at which point you can 
send a bluetooth-next PR and get the fixes into bluetooth-next.
FWIW from our perspective there's no limit on how often you send PRs.

Alternatively you could apply the fixes into bluetooth and then
merge bluetooth into bluetooth-next. If you never rebase either tree, 
git will be able to figure out that it's the same commit hash even if
it makes it to the tree twice (once thru direct merge and once via 
net). That said, I believe Linus does not like cross tree merges, i.e.
merges which are not fast forwards to the downstream tree. So it's
better to take the long road via bt ->  net -> net-next -> bt-next.

> and any possible conflicts if there were
> changes introduced to the bluetooth directories that can eventually
> come from some other tree.

Conflicts are not a worry, just let us know in the PR description how
to resolve them.
Luiz Augusto von Dentz July 26, 2022, 10:05 p.m. UTC | #6
Hi Jakub,

On Fri, Jul 22, 2022 at 5:50 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Fri, 22 Jul 2022 17:25:57 -0700 Luiz Augusto von Dentz wrote:
> > > > Crap, let me fix them.
> > >
> > > Do you mean i should hold off with pushing or you'll follow up?
> >
> > Ive just fixup the original patch that introduced it, btw how do you
> > run sparse to capture such errors?
>
> We run builds with W=1 C=1 in the CI and then diff the outputs.
> That's pretty noisy so we have a regex which counts number of
> warnings per file, that makes it possible to locate the exact new
> warning. At least most of the time...

Hmm, is there any way to trigger net CI, either that or we need to
duplicate the same test under our CI to avoid these last minute
findings when we are attempting to merge something.

> > > > Yep, that happens when I rebase on top of net-next so I would have to
> > > > redo all the Signed-off-by lines if the patches were originally
> > > > applied by Marcel, at least I don't know of any option to keep the
> > > > original committer while rebasing?
> > >
> > > I think the most common way is to avoid rebasing. Do you rebase to get
> > > rid of revised patches or such?
> >
> > So we don't need to rebase?
>
> No, not usually. After we pull from you, you should pull back from us
> (git pull --ff-only $net-or-net-next depending on the tree you
> targeted), and that's it. The only patches that go into your tree then
> are bluetooth patches, everything else is fed via pulling back from us.
>
> > There were some patches already applied via bluetooth.git so at least
> > I do it to remove them
>
> Normally you'd not apply bluetooth fixes to bluetooth-next, apply
> them to bluetooth and send us a PR. Then once a week we'll merge
> net (containing your fixes) into net-next, at which point you can
> send a bluetooth-next PR and get the fixes into bluetooth-next.
> FWIW from our perspective there's no limit on how often you send PRs.

Are you saying we should be using merge commits instead of rebase then?

> Alternatively you could apply the fixes into bluetooth and then
> merge bluetooth into bluetooth-next. If you never rebase either tree,
> git will be able to figure out that it's the same commit hash even if
> it makes it to the tree twice (once thru direct merge and once via
> net). That said, I believe Linus does not like cross tree merges, i.e.
> merges which are not fast forwards to the downstream tree. So it's
> better to take the long road via bt ->  net -> net-next -> bt-next.

Well I got the impression that merge commits shall be avoided, but
rebase overwrites the committer, so the two option seem to have
drawbacks, well we can just resign on rebase as well provided git
doesn't duplicate Signed-off-by if I use something like exec="git
commit -s --amend".

> > and any possible conflicts if there were
> > changes introduced to the bluetooth directories that can eventually
> > come from some other tree.
>
> Conflicts are not a worry, just let us know in the PR description how
> to resolve them.

Not really following, how can we anticipate a merge conflict if we
don't rebase? With merge strategy it seem that the one pulling needs
to resolve the conflicts rather than the submitter which I think would
lead to bad interaction between subsystems, expect if we do a merge
[-> resolve conflict] -> pull request -> [resolve conflicts ->] merge
which sounds a little too complicated since we have to resolve
conflicts in both directions.

In my opinion rebase strategy is cleaner and is what we recommend for
possible clones of bluetooth-next and bluetooth trees including CI so
possible conflicts are fixed in place rather on the time the trees are
merged.
Jakub Kicinski July 26, 2022, 10:31 p.m. UTC | #7
On Tue, 26 Jul 2022 15:05:17 -0700 Luiz Augusto von Dentz wrote:
> > > Ive just fixup the original patch that introduced it, btw how do you
> > > run sparse to capture such errors?  
> >
> > We run builds with W=1 C=1 in the CI and then diff the outputs.
> > That's pretty noisy so we have a regex which counts number of
> > warnings per file, that makes it possible to locate the exact new
> > warning. At least most of the time...  
> 
> Hmm, is there any way to trigger net CI, either that or we need to
> duplicate the same test under our CI to avoid these last minute
> findings when we are attempting to merge something.

The code is at:

https://github.com/kuba-moo/nipa

But it hardcodes net and bpf tree maching in places. You may want
to steal just the build script, its in bash.

> > > So we don't need to rebase?  
> >
> > No, not usually. After we pull from you, you should pull back from us
> > (git pull --ff-only $net-or-net-next depending on the tree you
> > targeted), and that's it. The only patches that go into your tree then
> > are bluetooth patches, everything else is fed via pulling back from us.
> >  
> > > There were some patches already applied via bluetooth.git so at least
> > > I do it to remove them  
> >
> > Normally you'd not apply bluetooth fixes to bluetooth-next, apply
> > them to bluetooth and send us a PR. Then once a week we'll merge
> > net (containing your fixes) into net-next, at which point you can
> > send a bluetooth-next PR and get the fixes into bluetooth-next.
> > FWIW from our perspective there's no limit on how often you send PRs.  
> 
> Are you saying we should be using merge commits instead of rebase then?

Not sure what merge commits would mean in this case.

> > Alternatively you could apply the fixes into bluetooth and then
> > merge bluetooth into bluetooth-next. If you never rebase either tree,
> > git will be able to figure out that it's the same commit hash even if
> > it makes it to the tree twice (once thru direct merge and once via
> > net). That said, I believe Linus does not like cross tree merges, i.e.
> > merges which are not fast forwards to the downstream tree. So it's
> > better to take the long road via bt ->  net -> net-next -> bt-next.  
> 
> Well I got the impression that merge commits shall be avoided, but

There's many schools of thought, but upstream there's very little
rebasing of "official" branches (i.e. main/master branches, not 
testing or other unstable branches) AFAIK.

> rebase overwrites the committer, so the two option seem to have
> drawbacks, well we can just resign on rebase as well provided git
> doesn't duplicate Signed-off-by if I use something like exec="git
> commit -s --amend".

Sure, be careful tho because I think it doesn't check the signoff
history, IIRC just the most recent tag. So you may end up with multiple
signoffs from yourself and Marcel.

> > > and any possible conflicts if there were
> > > changes introduced to the bluetooth directories that can eventually
> > > come from some other tree.  
> >
> > Conflicts are not a worry, just let us know in the PR description how
> > to resolve them.  
> 
> Not really following, how can we anticipate a merge conflict if we
> don't rebase?

If your trees are hooked up to linux-next (I presume not 'cause Stephen
would probably scream at you for rebasing?) - Stephen will tell you
there's a conflict within a day or two.

Obviously sometimes you'll notice right away when applying patches that
two patches touch the same function.

> With merge strategy it seem that the one pulling needs
> to resolve the conflicts rather than the submitter which I think would
> lead to bad interaction between subsystems, expect if we do a merge
> [-> resolve conflict] -> pull request -> [resolve conflicts ->] merge
> which sounds a little too complicated since we have to resolve
> conflicts in both directions.

The pulling back should always be a fast-forward so there's no merge
commit or conflicts (git pull --ff-only). Only the actual downstream
tree (netdev) has to resolve conflicts, which is not all that bad
thanks for Stephen's advanced notices.

> In my opinion rebase strategy is cleaner and is what we recommend for
> possible clones of bluetooth-next and bluetooth trees including CI so
> possible conflicts are fixed in place rather on the time the trees are
> merged.

No strong preference here as long as we can keep the sign-offs etc in
control. Note that I'm not aware of any other tree we pull rebasing, 
tho, so you may run into unique issues.
Luiz Augusto von Dentz July 27, 2022, 1:06 a.m. UTC | #8
Hi Jakub,

On Tue, Jul 26, 2022 at 3:31 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Tue, 26 Jul 2022 15:05:17 -0700 Luiz Augusto von Dentz wrote:
> > > > Ive just fixup the original patch that introduced it, btw how do you
> > > > run sparse to capture such errors?
> > >
> > > We run builds with W=1 C=1 in the CI and then diff the outputs.
> > > That's pretty noisy so we have a regex which counts number of
> > > warnings per file, that makes it possible to locate the exact new
> > > warning. At least most of the time...
> >
> > Hmm, is there any way to trigger net CI, either that or we need to
> > duplicate the same test under our CI to avoid these last minute
> > findings when we are attempting to merge something.
>
> The code is at:
>
> https://github.com/kuba-moo/nipa
>
> But it hardcodes net and bpf tree maching in places. You may want
> to steal just the build script, its in bash.

We can either incorporate it on our own CI or start to consolidate
everything in one place since there are quite a few tests that apply
tree wide, though we would need to add support for subsystem specific
tests as well, anyway I leave for @Tedd Ho-Jeong An to comment on this
when he is back from vacation.

> > > > So we don't need to rebase?
> > >
> > > No, not usually. After we pull from you, you should pull back from us
> > > (git pull --ff-only $net-or-net-next depending on the tree you
> > > targeted), and that's it. The only patches that go into your tree then
> > > are bluetooth patches, everything else is fed via pulling back from us.
> > >
> > > > There were some patches already applied via bluetooth.git so at least
> > > > I do it to remove them
> > >
> > > Normally you'd not apply bluetooth fixes to bluetooth-next, apply
> > > them to bluetooth and send us a PR. Then once a week we'll merge
> > > net (containing your fixes) into net-next, at which point you can
> > > send a bluetooth-next PR and get the fixes into bluetooth-next.
> > > FWIW from our perspective there's no limit on how often you send PRs.
> >
> > Are you saying we should be using merge commits instead of rebase then?
>
> Not sure what merge commits would mean in this case.
>
> > > Alternatively you could apply the fixes into bluetooth and then
> > > merge bluetooth into bluetooth-next. If you never rebase either tree,
> > > git will be able to figure out that it's the same commit hash even if
> > > it makes it to the tree twice (once thru direct merge and once via
> > > net). That said, I believe Linus does not like cross tree merges, i.e.
> > > merges which are not fast forwards to the downstream tree. So it's
> > > better to take the long road via bt ->  net -> net-next -> bt-next.
> >
> > Well I got the impression that merge commits shall be avoided, but
>
> There's many schools of thought, but upstream there's very little
> rebasing of "official" branches (i.e. main/master branches, not
> testing or other unstable branches) AFAIK.
>
> > rebase overwrites the committer, so the two option seem to have
> > drawbacks, well we can just resign on rebase as well provided git
> > doesn't duplicate Signed-off-by if I use something like exec="git
> > commit -s --amend".
>
> Sure, be careful tho because I think it doesn't check the signoff
> history, IIRC just the most recent tag. So you may end up with multiple
> signoffs from yourself and Marcel.

Yep, I thought that was a bug though since I doubt there is any use
for duplicated signoffs.

> > > > and any possible conflicts if there were
> > > > changes introduced to the bluetooth directories that can eventually
> > > > come from some other tree.
> > >
> > > Conflicts are not a worry, just let us know in the PR description how
> > > to resolve them.
> >
> > Not really following, how can we anticipate a merge conflict if we
> > don't rebase?
>
> If your trees are hooked up to linux-next (I presume not 'cause Stephen
> would probably scream at you for rebasing?) - Stephen will tell you
> there's a conflict within a day or two.
>
> Obviously sometimes you'll notice right away when applying patches that
> two patches touch the same function.
>
> > With merge strategy it seem that the one pulling needs
> > to resolve the conflicts rather than the submitter which I think would
> > lead to bad interaction between subsystems, expect if we do a merge
> > [-> resolve conflict] -> pull request -> [resolve conflicts ->] merge
> > which sounds a little too complicated since we have to resolve
> > conflicts in both directions.
>
> The pulling back should always be a fast-forward so there's no merge
> commit or conflicts (git pull --ff-only). Only the actual downstream
> tree (netdev) has to resolve conflicts, which is not all that bad
> thanks for Stephen's advanced notices.
>
> > In my opinion rebase strategy is cleaner and is what we recommend for
> > possible clones of bluetooth-next and bluetooth trees including CI so
> > possible conflicts are fixed in place rather on the time the trees are
> > merged.
>
> No strong preference here as long as we can keep the sign-offs etc in
> control. Note that I'm not aware of any other tree we pull rebasing,
> tho, so you may run into unique issues.

Maybe I need to get in touch with other maintainers to know what they
are doing, but how about net-next, how does it gets updated? Is that
done via git merge or git pull alone is enough?
Jakub Kicinski July 27, 2022, 2:47 a.m. UTC | #9
On Tue, 26 Jul 2022 18:06:47 -0700 Luiz Augusto von Dentz wrote:
> > No strong preference here as long as we can keep the sign-offs etc in
> > control. Note that I'm not aware of any other tree we pull rebasing,
> > tho, so you may run into unique issues.  
> 
> Maybe I need to get in touch with other maintainers to know what they
> are doing, but how about net-next, how does it gets updated? Is that
> done via git merge or git pull alone is enough?

git pull, if you mean upstream trees making their wait into net-next.
And afterwards submitter does git pull --ff-only to update their tree.