[PATCH v1 1/2] Bluetooth: coredump: Add hci_devcd_unregister() for cleanup

[PATCH v1 1/2] Bluetooth: coredump: Add hci_devcd_unregister() for cleanup

[Neeraj Sanjay Kale]

This adds hci_devcd_unregister() which can be called when driver is
removed, which will cleanup the devcoredump data and cancel delayed
dump_timeout work.

With BTNXPUART driver, it is observed that after FW dump, if driver is
removed and re-loaded, it creates hci1 interface instead of hci0
interface.

But after DEVCD_TIMEOUT (5 minutes) if driver is re-loaded, hci0 is
created. This is because after FW dump, hci0 is not unregistered
properly for DEVCD_TIMEOUT.

With this patch, BTNXPUART is able to create hci0 after every FW dump
and driver reload.

Signed-off-by: Neeraj Sanjay Kale <neeraj.sanjaykale@nxp.com>
---
 include/net/bluetooth/coredump.h | 3 +++
 net/bluetooth/coredump.c         | 8 ++++++++
 2 files changed, 11 insertions(+)

diff --git a/include/net/bluetooth/coredump.h b/include/net/bluetooth/coredump.h
index 72f51b587a04..bc8856e4bfe7 100644
--- a/include/net/bluetooth/coredump.h
+++ b/include/net/bluetooth/coredump.h
@@ -66,6 +66,7 @@ void hci_devcd_timeout(struct work_struct *work);
 
 int hci_devcd_register(struct hci_dev *hdev, coredump_t coredump,
 		       dmp_hdr_t dmp_hdr, notify_change_t notify_change);
+void hci_devcd_unregister(struct hci_dev *hdev);
 int hci_devcd_init(struct hci_dev *hdev, u32 dump_size);
 int hci_devcd_append(struct hci_dev *hdev, struct sk_buff *skb);
 int hci_devcd_append_pattern(struct hci_dev *hdev, u8 pattern, u32 len);
@@ -85,6 +86,8 @@ static inline int hci_devcd_register(struct hci_dev *hdev, coredump_t coredump,
 	return -EOPNOTSUPP;
 }
 
+static inline void hci_devcd_unregister(struct hci_dev *hdev) {}
+
 static inline int hci_devcd_init(struct hci_dev *hdev, u32 dump_size)
 {
 	return -EOPNOTSUPP;
diff --git a/net/bluetooth/coredump.c b/net/bluetooth/coredump.c
index 819eacb38762..dd7bd40e3eba 100644
--- a/net/bluetooth/coredump.c
+++ b/net/bluetooth/coredump.c
@@ -442,6 +442,14 @@ int hci_devcd_register(struct hci_dev *hdev, coredump_t coredump,
 }
 EXPORT_SYMBOL(hci_devcd_register);
 
+void hci_devcd_unregister(struct hci_dev *hdev)
+{
+	cancel_delayed_work(&hdev->dump.dump_timeout);
+	skb_queue_purge(&hdev->dump.dump_q);
+	dev_coredump_put(&hdev->dev);
+}
+EXPORT_SYMBOL_GPL(hci_devcd_unregister);
+
 static inline bool hci_devcd_enabled(struct hci_dev *hdev)
 {
 	return hdev->dump.supported;
-- 
2.34.1

[PATCH v1 2/2] Bluetooth: btnxpuart: Call hci_devcd_unregister() in driver exit

[Neeraj Sanjay Kale]

This adds a call to the new hci_devcd_unregister() API during driver
exit to cleanup devcoredump data and cancel delayed work, allowing hci
interface to be unregistered properly.

Signed-off-by: Neeraj Sanjay Kale <neeraj.sanjaykale@nxp.com>
---
 drivers/bluetooth/btnxpuart.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/bluetooth/btnxpuart.c b/drivers/bluetooth/btnxpuart.c
index c56b52bd8d98..91bf9812c8df 100644
--- a/drivers/bluetooth/btnxpuart.c
+++ b/drivers/bluetooth/btnxpuart.c
@@ -1866,6 +1866,7 @@ static void nxp_serdev_remove(struct serdev_device *serdev)
 	}
 
 	ps_cleanup(nxpdev);
+	hci_devcd_unregister(hdev);
 	hci_unregister_dev(hdev);
 	hci_free_dev(hdev);
 }
-- 
2.34.1

Re: [PATCH v1 1/2] Bluetooth: coredump: Add hci_devcd_unregister() for cleanup

[Luiz Augusto von Dentz]

Hi Neeraj,

On Thu, Jul 3, 2025 at 9:17 AM Neeraj Sanjay Kale
<neeraj.sanjaykale@nxp.com> wrote:
>
> This adds hci_devcd_unregister() which can be called when driver is
> removed, which will cleanup the devcoredump data and cancel delayed
> dump_timeout work.
>
> With BTNXPUART driver, it is observed that after FW dump, if driver is
> removed and re-loaded, it creates hci1 interface instead of hci0
> interface.
>
> But after DEVCD_TIMEOUT (5 minutes) if driver is re-loaded, hci0 is
> created. This is because after FW dump, hci0 is not unregistered
> properly for DEVCD_TIMEOUT.
>
> With this patch, BTNXPUART is able to create hci0 after every FW dump
> and driver reload.
>
> Signed-off-by: Neeraj Sanjay Kale <neeraj.sanjaykale@nxp.com>
> ---
>  include/net/bluetooth/coredump.h | 3 +++
>  net/bluetooth/coredump.c         | 8 ++++++++
>  2 files changed, 11 insertions(+)
>
> diff --git a/include/net/bluetooth/coredump.h b/include/net/bluetooth/coredump.h
> index 72f51b587a04..bc8856e4bfe7 100644
> --- a/include/net/bluetooth/coredump.h
> +++ b/include/net/bluetooth/coredump.h
> @@ -66,6 +66,7 @@ void hci_devcd_timeout(struct work_struct *work);
>
>  int hci_devcd_register(struct hci_dev *hdev, coredump_t coredump,
>                        dmp_hdr_t dmp_hdr, notify_change_t notify_change);
> +void hci_devcd_unregister(struct hci_dev *hdev);
>  int hci_devcd_init(struct hci_dev *hdev, u32 dump_size);
>  int hci_devcd_append(struct hci_dev *hdev, struct sk_buff *skb);
>  int hci_devcd_append_pattern(struct hci_dev *hdev, u8 pattern, u32 len);
> @@ -85,6 +86,8 @@ static inline int hci_devcd_register(struct hci_dev *hdev, coredump_t coredump,
>         return -EOPNOTSUPP;
>  }
>
> +static inline void hci_devcd_unregister(struct hci_dev *hdev) {}
> +
>  static inline int hci_devcd_init(struct hci_dev *hdev, u32 dump_size)
>  {
>         return -EOPNOTSUPP;
> diff --git a/net/bluetooth/coredump.c b/net/bluetooth/coredump.c
> index 819eacb38762..dd7bd40e3eba 100644
> --- a/net/bluetooth/coredump.c
> +++ b/net/bluetooth/coredump.c
> @@ -442,6 +442,14 @@ int hci_devcd_register(struct hci_dev *hdev, coredump_t coredump,
>  }
>  EXPORT_SYMBOL(hci_devcd_register);
>
> +void hci_devcd_unregister(struct hci_dev *hdev)
> +{
> +       cancel_delayed_work(&hdev->dump.dump_timeout);
> +       skb_queue_purge(&hdev->dump.dump_q);
> +       dev_coredump_put(&hdev->dev);
> +}
> +EXPORT_SYMBOL_GPL(hci_devcd_unregister);

The fact that the dump lives inside hdev is sort of the source of
these problems, specially if the dumps are not HCI traffic it might be
better off having the driver control its lifetime and not use
hdev->workqueue to schedule it.

>  static inline bool hci_devcd_enabled(struct hci_dev *hdev)
>  {
>         return hdev->dump.supported;
> --
> 2.34.1
>


-- 
Luiz Augusto von Dentz

RE: [v1,1/2] Bluetooth: coredump: Add hci_devcd_unregister() for cleanup

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 2028 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=978641

---Test result---

Test Summary:
CheckPatch                    PENDING   0.44 seconds
GitLint                       PENDING   0.39 seconds
SubjectPrefix                 PASS      0.24 seconds
BuildKernel                   PASS      24.12 seconds
CheckAllWarning               PASS      26.78 seconds
CheckSparse                   PASS      30.23 seconds
BuildKernel32                 PASS      24.15 seconds
TestRunnerSetup               PASS      471.77 seconds
TestRunner_l2cap-tester       PASS      25.05 seconds
TestRunner_iso-tester         PASS      46.34 seconds
TestRunner_bnep-tester        PASS      5.85 seconds
TestRunner_mgmt-tester        PASS      129.71 seconds
TestRunner_rfcomm-tester      PASS      9.32 seconds
TestRunner_sco-tester         PASS      14.72 seconds
TestRunner_ioctl-tester       PASS      10.01 seconds
TestRunner_mesh-tester        FAIL      11.80 seconds
TestRunner_smp-tester         PASS      8.53 seconds
TestRunner_userchan-tester    PASS      6.19 seconds
IncrementalBuild              PENDING   0.79 seconds

Details
##############################
Test: CheckPatch - PENDING
Desc: Run checkpatch.pl script
Output:

##############################
Test: GitLint - PENDING
Desc: Run gitlint
Output:

##############################
Test: TestRunner_mesh-tester - FAIL
Desc: Run mesh-tester with test-runner
Output:
Total: 10, Passed: 8 (80.0%), Failed: 2, Not Run: 0

Failed Test Cases
Mesh - Send cancel - 1                               Timed out    2.128 seconds
Mesh - Send cancel - 2                               Timed out    1.999 seconds
##############################
Test: IncrementalBuild - PENDING
Desc: Incremental build with the patches in the series
Output:



---
Regards,
Linux Bluetooth

Re: [PATCH v1 1/2] Bluetooth: coredump: Add hci_devcd_unregister() for cleanup

[Neeraj Sanjay Kale]

Hi Luiz,

Thank you for reviewing this patch.

> On Thu, Jul 3, 2025 at 9:17 AM Neeraj Sanjay Kale
> <neeraj.sanjaykale@nxp.com> wrote:
> >
> > This adds hci_devcd_unregister() which can be called when driver is
> > removed, which will cleanup the devcoredump data and cancel delayed
> > dump_timeout work.
> >
> > With BTNXPUART driver, it is observed that after FW dump, if driver is
> > removed and re-loaded, it creates hci1 interface instead of hci0
> > interface.
> >
> > But after DEVCD_TIMEOUT (5 minutes) if driver is re-loaded, hci0 is
> > created. This is because after FW dump, hci0 is not unregistered
> > properly for DEVCD_TIMEOUT.
> >
> > With this patch, BTNXPUART is able to create hci0 after every FW dump
> > and driver reload.
> >
> > Signed-off-by: Neeraj Sanjay Kale <neeraj.sanjaykale@nxp.com>
> > ---
> >  include/net/bluetooth/coredump.h | 3 +++
> >  net/bluetooth/coredump.c         | 8 ++++++++
> >  2 files changed, 11 insertions(+)
> >
> > diff --git a/include/net/bluetooth/coredump.h
> > b/include/net/bluetooth/coredump.h
> > index 72f51b587a04..bc8856e4bfe7 100644
> > --- a/include/net/bluetooth/coredump.h
> > +++ b/include/net/bluetooth/coredump.h
> > @@ -66,6 +66,7 @@ void hci_devcd_timeout(struct work_struct *work);
> >
> >  int hci_devcd_register(struct hci_dev *hdev, coredump_t coredump,
> >                        dmp_hdr_t dmp_hdr, notify_change_t
> > notify_change);
> > +void hci_devcd_unregister(struct hci_dev *hdev);
> >  int hci_devcd_init(struct hci_dev *hdev, u32 dump_size);  int
> > hci_devcd_append(struct hci_dev *hdev, struct sk_buff *skb);  int
> > hci_devcd_append_pattern(struct hci_dev *hdev, u8 pattern, u32 len);
> > @@ -85,6 +86,8 @@ static inline int hci_devcd_register(struct hci_dev
> *hdev, coredump_t coredump,
> >         return -EOPNOTSUPP;
> >  }
> >
> > +static inline void hci_devcd_unregister(struct hci_dev *hdev) {}
> > +
> >  static inline int hci_devcd_init(struct hci_dev *hdev, u32 dump_size)
> > {
> >         return -EOPNOTSUPP;
> > diff --git a/net/bluetooth/coredump.c b/net/bluetooth/coredump.c index
> > 819eacb38762..dd7bd40e3eba 100644
> > --- a/net/bluetooth/coredump.c
> > +++ b/net/bluetooth/coredump.c
> > @@ -442,6 +442,14 @@ int hci_devcd_register(struct hci_dev *hdev,
> > coredump_t coredump,  }  EXPORT_SYMBOL(hci_devcd_register);
> >
> > +void hci_devcd_unregister(struct hci_dev *hdev) {
> > +       cancel_delayed_work(&hdev->dump.dump_timeout);
> > +       skb_queue_purge(&hdev->dump.dump_q);
> > +       dev_coredump_put(&hdev->dev);
> > +}
> > +EXPORT_SYMBOL_GPL(hci_devcd_unregister);
> 
> The fact that the dump lives inside hdev is sort of the source of these
> problems, specially if the dumps are not HCI traffic it might be better off
> having the driver control its lifetime and not use
> hdev->workqueue to schedule it.
Are you are talking about "hdev->dump.dump_timeout"? it does not control the dump lifetime.
It simply makes sure that once FW dump is started, it should be complete within "dump_timeout" seconds.

The actual cleaning up of dump data is done by the " devcd->del_wk" which is delay-scheduled by 5 minutes in dev_coredumpm_timeout(), which is part of the devcoredump base.

Sure, with some modification, the driver can control the dump lifetime instead of hardcode DEVCD_TIMEOUT, but during driver exit, there is a need for "dev_coredump_put()" API to be called anyway.

Please let me know your thoughts on this.

> 
> >  static inline bool hci_devcd_enabled(struct hci_dev *hdev)  {
> >         return hdev->dump.supported;
> > --

Thanks,
Neeraj

RE: [PATCH v1 1/2] Bluetooth: coredump: Add hci_devcd_unregister() for cleanup

[Neeraj Sanjay Kale]

Hi Luiz,

A follow-up on my previous reply.

Looks like the dump lifetime is fixed/hardcoded to 5 minutes by the devcoredump base framework on purpose. Any change to this framework, to allow btnxpuart driver control the dump lifetime may not get approved by devcoredump maintainers.
https://github.com/bluez/bluetooth-next/commit/3b9c181bcde8555ca81b2394c2dc2201cefc2dd4#diff-dd2971a4abc5dbad2b03c42f25b2d8ef9d9417189070012500f315ee22fd17f9

Anyways, for Bluetooth drivers that relay on hci_devcd layer, calling the new hci_devcd_unregister() API fixes the issue mentioned in the commit message, properly cleaning up the dump data if driver is removed before the dump lifetime is complete, no matter who controls the dump lifetime.

Please let me know if I'm missing something.

Thanks,
Neeraj
 
> Hi Luiz,
> 
> Thank you for reviewing this patch.
> 
> > On Thu, Jul 3, 2025 at 9:17 AM Neeraj Sanjay Kale
> > <neeraj.sanjaykale@nxp.com> wrote:
> > >
> > > This adds hci_devcd_unregister() which can be called when driver is
> > > removed, which will cleanup the devcoredump data and cancel delayed
> > > dump_timeout work.
> > >
> > > With BTNXPUART driver, it is observed that after FW dump, if driver
> > > is removed and re-loaded, it creates hci1 interface instead of hci0
> > > interface.
> > >
> > > But after DEVCD_TIMEOUT (5 minutes) if driver is re-loaded, hci0 is
> > > created. This is because after FW dump, hci0 is not unregistered
> > > properly for DEVCD_TIMEOUT.
> > >
> > > With this patch, BTNXPUART is able to create hci0 after every FW
> > > dump and driver reload.
> > >
> > > Signed-off-by: Neeraj Sanjay Kale <neeraj.sanjaykale@nxp.com>
> > > ---
> > >  include/net/bluetooth/coredump.h | 3 +++
> > >  net/bluetooth/coredump.c         | 8 ++++++++
> > >  2 files changed, 11 insertions(+)
> > >
> > > diff --git a/include/net/bluetooth/coredump.h
> > > b/include/net/bluetooth/coredump.h
> > > index 72f51b587a04..bc8856e4bfe7 100644
> > > --- a/include/net/bluetooth/coredump.h
> > > +++ b/include/net/bluetooth/coredump.h
> > > @@ -66,6 +66,7 @@ void hci_devcd_timeout(struct work_struct *work);
> > >
> > >  int hci_devcd_register(struct hci_dev *hdev, coredump_t coredump,
> > >                        dmp_hdr_t dmp_hdr, notify_change_t
> > > notify_change);
> > > +void hci_devcd_unregister(struct hci_dev *hdev);
> > >  int hci_devcd_init(struct hci_dev *hdev, u32 dump_size);  int
> > > hci_devcd_append(struct hci_dev *hdev, struct sk_buff *skb);  int
> > > hci_devcd_append_pattern(struct hci_dev *hdev, u8 pattern, u32 len);
> > > @@ -85,6 +86,8 @@ static inline int hci_devcd_register(struct
> > > hci_dev
> > *hdev, coredump_t coredump,
> > >         return -EOPNOTSUPP;
> > >  }
> > >
> > > +static inline void hci_devcd_unregister(struct hci_dev *hdev) {}
> > > +
> > >  static inline int hci_devcd_init(struct hci_dev *hdev, u32
> > > dump_size) {
> > >         return -EOPNOTSUPP;
> > > diff --git a/net/bluetooth/coredump.c b/net/bluetooth/coredump.c
> > > index 819eacb38762..dd7bd40e3eba 100644
> > > --- a/net/bluetooth/coredump.c
> > > +++ b/net/bluetooth/coredump.c
> > > @@ -442,6 +442,14 @@ int hci_devcd_register(struct hci_dev *hdev,
> > > coredump_t coredump,  }  EXPORT_SYMBOL(hci_devcd_register);
> > >
> > > +void hci_devcd_unregister(struct hci_dev *hdev) {
> > > +       cancel_delayed_work(&hdev->dump.dump_timeout);
> > > +       skb_queue_purge(&hdev->dump.dump_q);
> > > +       dev_coredump_put(&hdev->dev); }
> > > +EXPORT_SYMBOL_GPL(hci_devcd_unregister);
> >
> > The fact that the dump lives inside hdev is sort of the source of
> > these problems, specially if the dumps are not HCI traffic it might be
> > better off having the driver control its lifetime and not use
> > hdev->workqueue to schedule it.
> Are you are talking about "hdev->dump.dump_timeout"? it does not control
> the dump lifetime.
> It simply makes sure that once FW dump is started, it should be complete
> within "dump_timeout" seconds.
> 
> The actual cleaning up of dump data is done by the " devcd->del_wk" which
> is delay-scheduled by 5 minutes in dev_coredumpm_timeout(), which is part
> of the devcoredump base.
> 
> Sure, with some modification, the driver can control the dump lifetime
> instead of hardcode DEVCD_TIMEOUT, but during driver exit, there is a need
> for "dev_coredump_put()" API to be called anyway.
> 
> Please let me know your thoughts on this.
> 
> >
> > >  static inline bool hci_devcd_enabled(struct hci_dev *hdev)  {
> > >         return hdev->dump.supported;
> > > --
> 
> Thanks,
> Neeraj