Hi Luiz,
Thank you for reviewing this patch.
> On Thu, Jul 3, 2025 at 9:17 AM Neeraj Sanjay Kale
> <neeraj.sanjaykale@xxxxxxx> 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@xxxxxxx>
> > ---
> > 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
