| Message ID | 20221004163224.1.I46e98b47be875d0b9abff2d19417c612077d1909@changeid (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Bluetooth: btusb: Introduce generic USB reset | expand |
| Context | Check | Description |
|---|---|---|
| tedd_an/pre-ci_am | success | Success |
| tedd_an/checkpatch | success | Checkpatch PASS |
| tedd_an/gitlint | success | Gitlint PASS |
| tedd_an/subjectprefix | success | PASS |
| tedd_an/buildkernel | success | Build Kernel PASS |
| tedd_an/buildkernel32 | success | Build Kernel32 PASS |
| tedd_an/incremental_build | success | Pass |
| tedd_an/testrunnersetup | success | Test Runner Setup PASS |
| tedd_an/testrunnerl2cap-tester | success | Total: 40, Passed: 40 (100.0%), Failed: 0, Not Run: 0 |
| tedd_an/testrunneriso-tester | success | Total: 55, Passed: 55 (100.0%), Failed: 0, Not Run: 0 |
| tedd_an/testrunnerbnep-tester | success | Total: 1, Passed: 1 (100.0%), Failed: 0, Not Run: 0 |
| tedd_an/testrunnermgmt-tester | success | Total: 494, Passed: 494 (100.0%), Failed: 0, Not Run: 0 |
| tedd_an/testrunnerrfcomm-tester | success | Total: 11, Passed: 11 (100.0%), Failed: 0, Not Run: 0 |
| tedd_an/testrunnersco-tester | success | Total: 12, Passed: 12 (100.0%), Failed: 0, Not Run: 0 |
| tedd_an/testrunnerioctl-tester | success | Total: 28, Passed: 28 (100.0%), Failed: 0, Not Run: 0 |
| tedd_an/testrunnersmp-tester | success | Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0 |
| tedd_an/testrunneruserchan-tester | success | Total: 4, Passed: 4 (100.0%), Failed: 0, Not Run: 0 |
Dear Archie, Thank you for the patch. Am 04.10.22 um 10:32 schrieb Archie Pusaka: > From: Archie Pusaka <apusaka@chromium.org> > > On cmd_timeout and there is no reset_gpio, reset the USB port as a Maybe: s/there is no/with no/g > last resort. Can you please document your test setup, and also mention that you change the behavior of: 1. btusb_intel_cmd_timeout 2. btusb_rtl_cmd_timeout […] Kind regards, Paul > Signed-off-by: Archie Pusaka <apusaka@chromium.org> > Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@google.com> > Reviewed-by: Ying Hsu <yinghsu@chromium.org> > > --- > > drivers/bluetooth/btusb.c | 26 ++++++++++++++++---------- > 1 file changed, 16 insertions(+), 10 deletions(-) > > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c > index 271963805a38..11040124ef79 100644 > --- a/drivers/bluetooth/btusb.c > +++ b/drivers/bluetooth/btusb.c > @@ -696,6 +696,19 @@ struct btusb_data { > unsigned cmd_timeout_cnt; > }; > > +static void generic_usb_reset(struct hci_dev *hdev, struct btusb_data *data) > +{ > + int err; > + > + bt_dev_err(hdev, "Resetting usb device."); > + /* This is not an unbalanced PM reference since the device will reset */ > + err = usb_autopm_get_interface(data->intf); > + if (!err) > + usb_queue_reset_device(data->intf); > + else > + bt_dev_err(hdev, "Failed usb_autopm_get_interface: %d", err); > +} > + > static void btusb_intel_cmd_timeout(struct hci_dev *hdev) > { > struct btusb_data *data = hci_get_drvdata(hdev); > @@ -705,7 +718,7 @@ static void btusb_intel_cmd_timeout(struct hci_dev *hdev) > return; > > if (!reset_gpio) { > - bt_dev_err(hdev, "No way to reset. Ignoring and continuing"); > + generic_usb_reset(hdev, data); > return; > } > > @@ -736,7 +749,7 @@ static void btusb_rtl_cmd_timeout(struct hci_dev *hdev) > return; > > if (!reset_gpio) { > - bt_dev_err(hdev, "No gpio to reset Realtek device, ignoring"); > + generic_usb_reset(hdev, data); > return; > } > > @@ -761,7 +774,6 @@ static void btusb_qca_cmd_timeout(struct hci_dev *hdev) > { > struct btusb_data *data = hci_get_drvdata(hdev); > struct gpio_desc *reset_gpio = data->reset_gpio; > - int err; > > if (++data->cmd_timeout_cnt < 5) > return; > @@ -787,13 +799,7 @@ static void btusb_qca_cmd_timeout(struct hci_dev *hdev) > return; > } > > - bt_dev_err(hdev, "Multiple cmd timeouts seen. Resetting usb device."); > - /* This is not an unbalanced PM reference since the device will reset */ > - err = usb_autopm_get_interface(data->intf); > - if (!err) > - usb_queue_reset_device(data->intf); > - else > - bt_dev_err(hdev, "Failed usb_autopm_get_interface with %d", err); > + generic_usb_reset(hdev, data); > } > > static inline void btusb_free_frags(struct btusb_data *data)
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=682932 ---Test result--- Test Summary: CheckPatch PASS 1.62 seconds GitLint PASS 0.74 seconds SubjectPrefix PASS 0.63 seconds BuildKernel PASS 42.93 seconds BuildKernel32 PASS 39.54 seconds Incremental Build with patchesPASS 55.77 seconds TestRunner: Setup PASS 643.76 seconds TestRunner: l2cap-tester PASS 19.50 seconds TestRunner: iso-tester PASS 19.73 seconds TestRunner: bnep-tester PASS 7.72 seconds TestRunner: mgmt-tester PASS 124.90 seconds TestRunner: rfcomm-tester PASS 12.48 seconds TestRunner: sco-tester PASS 11.55 seconds TestRunner: ioctl-tester PASS 13.47 seconds TestRunner: smp-tester PASS 11.79 seconds TestRunner: userchan-tester PASS 8.33 seconds --- Regards, Linux Bluetooth
Hi Paul, On Tue, 4 Oct 2022 at 17:18, Paul Menzel <pmenzel@molgen.mpg.de> wrote: > > Dear Archie, > > > Thank you for the patch. > > Am 04.10.22 um 10:32 schrieb Archie Pusaka: > > From: Archie Pusaka <apusaka@chromium.org> > > > > On cmd_timeout and there is no reset_gpio, reset the USB port as a > > Maybe: > > s/there is no/with no/g > > > last resort. > > Can you please document your test setup, and also mention that you > change the behavior of: > > 1. btusb_intel_cmd_timeout > 2. btusb_rtl_cmd_timeout > > […] > > > Kind regards, > > Paul Thanks for the input! All done, uploaded v2. PTAL, thanks! > > > > Signed-off-by: Archie Pusaka <apusaka@chromium.org> > > Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@google.com> > > Reviewed-by: Ying Hsu <yinghsu@chromium.org> > > > > --- > > > > drivers/bluetooth/btusb.c | 26 ++++++++++++++++---------- > > 1 file changed, 16 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c > > index 271963805a38..11040124ef79 100644 > > --- a/drivers/bluetooth/btusb.c > > +++ b/drivers/bluetooth/btusb.c > > @@ -696,6 +696,19 @@ struct btusb_data { > > unsigned cmd_timeout_cnt; > > }; > > > > +static void generic_usb_reset(struct hci_dev *hdev, struct btusb_data *data) > > +{ > > + int err; > > + > > + bt_dev_err(hdev, "Resetting usb device."); > > + /* This is not an unbalanced PM reference since the device will reset */ > > + err = usb_autopm_get_interface(data->intf); > > + if (!err) > > + usb_queue_reset_device(data->intf); > > + else > > + bt_dev_err(hdev, "Failed usb_autopm_get_interface: %d", err); > > +} > > + > > static void btusb_intel_cmd_timeout(struct hci_dev *hdev) > > { > > struct btusb_data *data = hci_get_drvdata(hdev); > > @@ -705,7 +718,7 @@ static void btusb_intel_cmd_timeout(struct hci_dev *hdev) > > return; > > > > if (!reset_gpio) { > > - bt_dev_err(hdev, "No way to reset. Ignoring and continuing"); > > + generic_usb_reset(hdev, data); > > return; > > } > > > > @@ -736,7 +749,7 @@ static void btusb_rtl_cmd_timeout(struct hci_dev *hdev) > > return; > > > > if (!reset_gpio) { > > - bt_dev_err(hdev, "No gpio to reset Realtek device, ignoring"); > > + generic_usb_reset(hdev, data); > > return; > > } > > > > @@ -761,7 +774,6 @@ static void btusb_qca_cmd_timeout(struct hci_dev *hdev) > > { > > struct btusb_data *data = hci_get_drvdata(hdev); > > struct gpio_desc *reset_gpio = data->reset_gpio; > > - int err; > > > > if (++data->cmd_timeout_cnt < 5) > > return; > > @@ -787,13 +799,7 @@ static void btusb_qca_cmd_timeout(struct hci_dev *hdev) > > return; > > } > > > > - bt_dev_err(hdev, "Multiple cmd timeouts seen. Resetting usb device."); > > - /* This is not an unbalanced PM reference since the device will reset */ > > - err = usb_autopm_get_interface(data->intf); > > - if (!err) > > - usb_queue_reset_device(data->intf); > > - else > > - bt_dev_err(hdev, "Failed usb_autopm_get_interface with %d", err); > > + generic_usb_reset(hdev, data); > > } > > > > static inline void btusb_free_frags(struct btusb_data *data) Cheers, Archie
Hi Archie, On Tue, Oct 4, 2022 at 1:33 AM Archie Pusaka <apusaka@google.com> wrote: > > From: Archie Pusaka <apusaka@chromium.org> > > On cmd_timeout and there is no reset_gpio, reset the USB port as a > last resort. > > Signed-off-by: Archie Pusaka <apusaka@chromium.org> > Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@google.com> > Reviewed-by: Ying Hsu <yinghsu@chromium.org> > > --- > > drivers/bluetooth/btusb.c | 26 ++++++++++++++++---------- > 1 file changed, 16 insertions(+), 10 deletions(-) > > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c > index 271963805a38..11040124ef79 100644 > --- a/drivers/bluetooth/btusb.c > +++ b/drivers/bluetooth/btusb.c > @@ -696,6 +696,19 @@ struct btusb_data { > unsigned cmd_timeout_cnt; > }; > > +static void generic_usb_reset(struct hci_dev *hdev, struct btusb_data *data) > +{ > + int err; > + > + bt_dev_err(hdev, "Resetting usb device."); > + /* This is not an unbalanced PM reference since the device will reset */ > + err = usb_autopm_get_interface(data->intf); > + if (!err) > + usb_queue_reset_device(data->intf); > + else > + bt_dev_err(hdev, "Failed usb_autopm_get_interface: %d", err); Lets just have one line printed if it fails: err = usb_autopm_get_interface(data->intf); if (err) { bt_dev_err(hdev, "Failed usb_autopm_get_interface: %d", err); return; } bt_dev_err(hdev, "Resetting usb device."); usb_queue_reset_device(data->intf); > +} > + > static void btusb_intel_cmd_timeout(struct hci_dev *hdev) > { > struct btusb_data *data = hci_get_drvdata(hdev); > @@ -705,7 +718,7 @@ static void btusb_intel_cmd_timeout(struct hci_dev *hdev) > return; > > if (!reset_gpio) { > - bt_dev_err(hdev, "No way to reset. Ignoring and continuing"); > + generic_usb_reset(hdev, data); Lets call it btusb_reset since this is specific for the data->intf, btw is this safe, or perhaps we want to refactor this to have it callback based so each vendor can add it own specific hdev->reset callback hardware reset with btusb_reset serving as default callback? Also the logic of btusb_intel_cmd_timeout should probably be put inside btintel.c and I don't think we need the gpio_desc reference to be inside the btusb_data since we can call gpiod_get_optional during the reset phase and immediately do gpiod_put after done using it. > return; > } > > @@ -736,7 +749,7 @@ static void btusb_rtl_cmd_timeout(struct hci_dev *hdev) > return; > > if (!reset_gpio) { > - bt_dev_err(hdev, "No gpio to reset Realtek device, ignoring"); > + generic_usb_reset(hdev, data); > return; > } > > @@ -761,7 +774,6 @@ static void btusb_qca_cmd_timeout(struct hci_dev *hdev) > { > struct btusb_data *data = hci_get_drvdata(hdev); > struct gpio_desc *reset_gpio = data->reset_gpio; > - int err; > > if (++data->cmd_timeout_cnt < 5) > return; > @@ -787,13 +799,7 @@ static void btusb_qca_cmd_timeout(struct hci_dev *hdev) > return; > } > > - bt_dev_err(hdev, "Multiple cmd timeouts seen. Resetting usb device."); > - /* This is not an unbalanced PM reference since the device will reset */ > - err = usb_autopm_get_interface(data->intf); > - if (!err) > - usb_queue_reset_device(data->intf); > - else > - bt_dev_err(hdev, "Failed usb_autopm_get_interface with %d", err); > + generic_usb_reset(hdev, data); > } > > static inline void btusb_free_frags(struct btusb_data *data) > -- > 2.38.0.rc1.362.ged0d419d3c-goog >
Hi Luiz, On Wed, 5 Oct 2022 at 04:53, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > Hi Archie, > > On Tue, Oct 4, 2022 at 1:33 AM Archie Pusaka <apusaka@google.com> wrote: > > > > From: Archie Pusaka <apusaka@chromium.org> > > > > On cmd_timeout and there is no reset_gpio, reset the USB port as a > > last resort. > > > > Signed-off-by: Archie Pusaka <apusaka@chromium.org> > > Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@google.com> > > Reviewed-by: Ying Hsu <yinghsu@chromium.org> > > > > --- > > > > drivers/bluetooth/btusb.c | 26 ++++++++++++++++---------- > > 1 file changed, 16 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c > > index 271963805a38..11040124ef79 100644 > > --- a/drivers/bluetooth/btusb.c > > +++ b/drivers/bluetooth/btusb.c > > @@ -696,6 +696,19 @@ struct btusb_data { > > unsigned cmd_timeout_cnt; > > }; > > > > +static void generic_usb_reset(struct hci_dev *hdev, struct btusb_data *data) > > +{ > > + int err; > > + > > + bt_dev_err(hdev, "Resetting usb device."); > > + /* This is not an unbalanced PM reference since the device will reset */ > > + err = usb_autopm_get_interface(data->intf); > > + if (!err) > > + usb_queue_reset_device(data->intf); > > + else > > + bt_dev_err(hdev, "Failed usb_autopm_get_interface: %d", err); > > Lets just have one line printed if it fails: > > err = usb_autopm_get_interface(data->intf); > if (err) { > bt_dev_err(hdev, "Failed usb_autopm_get_interface: %d", err); > return; > } > > bt_dev_err(hdev, "Resetting usb device."); > usb_queue_reset_device(data->intf); > OK, will update > > +} > > + > > static void btusb_intel_cmd_timeout(struct hci_dev *hdev) > > { > > struct btusb_data *data = hci_get_drvdata(hdev); > > @@ -705,7 +718,7 @@ static void btusb_intel_cmd_timeout(struct hci_dev *hdev) > > return; > > > > if (!reset_gpio) { > > - bt_dev_err(hdev, "No way to reset. Ignoring and continuing"); > > + generic_usb_reset(hdev, data); > > Lets call it btusb_reset since this is specific for the data->intf, > btw is this safe, or perhaps we want to refactor this to have it > callback based so each vendor can add it own specific hdev->reset > callback hardware reset with btusb_reset serving as default callback? OK. I think this is safe at least for now, since resetting btusb is probably better than just giving up doing nothing. But I can add hdev->reset if that is desirable. > Also the logic of btusb_intel_cmd_timeout should probably be put > inside btintel.c and I don't think we need the gpio_desc reference to > be inside the btusb_data since we can call gpiod_get_optional during > the reset phase and immediately do gpiod_put after done using it. > I am not familiar with this part. Perhaps it's better for this to be managed by another patch, since it is not the focus of this patch? > > return; > > } > > > > @@ -736,7 +749,7 @@ static void btusb_rtl_cmd_timeout(struct hci_dev *hdev) > > return; > > > > if (!reset_gpio) { > > - bt_dev_err(hdev, "No gpio to reset Realtek device, ignoring"); > > + generic_usb_reset(hdev, data); > > return; > > } > > > > @@ -761,7 +774,6 @@ static void btusb_qca_cmd_timeout(struct hci_dev *hdev) > > { > > struct btusb_data *data = hci_get_drvdata(hdev); > > struct gpio_desc *reset_gpio = data->reset_gpio; > > - int err; > > > > if (++data->cmd_timeout_cnt < 5) > > return; > > @@ -787,13 +799,7 @@ static void btusb_qca_cmd_timeout(struct hci_dev *hdev) > > return; > > } > > > > - bt_dev_err(hdev, "Multiple cmd timeouts seen. Resetting usb device."); > > - /* This is not an unbalanced PM reference since the device will reset */ > > - err = usb_autopm_get_interface(data->intf); > > - if (!err) > > - usb_queue_reset_device(data->intf); > > - else > > - bt_dev_err(hdev, "Failed usb_autopm_get_interface with %d", err); > > + generic_usb_reset(hdev, data); > > } > > > > static inline void btusb_free_frags(struct btusb_data *data) > > -- > > 2.38.0.rc1.362.ged0d419d3c-goog > > > > > -- > Luiz Augusto von Dentz Cheers, Archie
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c index 271963805a38..11040124ef79 100644 --- a/drivers/bluetooth/btusb.c +++ b/drivers/bluetooth/btusb.c @@ -696,6 +696,19 @@ struct btusb_data { unsigned cmd_timeout_cnt; }; +static void generic_usb_reset(struct hci_dev *hdev, struct btusb_data *data) +{ + int err; + + bt_dev_err(hdev, "Resetting usb device."); + /* This is not an unbalanced PM reference since the device will reset */ + err = usb_autopm_get_interface(data->intf); + if (!err) + usb_queue_reset_device(data->intf); + else + bt_dev_err(hdev, "Failed usb_autopm_get_interface: %d", err); +} + static void btusb_intel_cmd_timeout(struct hci_dev *hdev) { struct btusb_data *data = hci_get_drvdata(hdev); @@ -705,7 +718,7 @@ static void btusb_intel_cmd_timeout(struct hci_dev *hdev) return; if (!reset_gpio) { - bt_dev_err(hdev, "No way to reset. Ignoring and continuing"); + generic_usb_reset(hdev, data); return; } @@ -736,7 +749,7 @@ static void btusb_rtl_cmd_timeout(struct hci_dev *hdev) return; if (!reset_gpio) { - bt_dev_err(hdev, "No gpio to reset Realtek device, ignoring"); + generic_usb_reset(hdev, data); return; } @@ -761,7 +774,6 @@ static void btusb_qca_cmd_timeout(struct hci_dev *hdev) { struct btusb_data *data = hci_get_drvdata(hdev); struct gpio_desc *reset_gpio = data->reset_gpio; - int err; if (++data->cmd_timeout_cnt < 5) return; @@ -787,13 +799,7 @@ static void btusb_qca_cmd_timeout(struct hci_dev *hdev) return; } - bt_dev_err(hdev, "Multiple cmd timeouts seen. Resetting usb device."); - /* This is not an unbalanced PM reference since the device will reset */ - err = usb_autopm_get_interface(data->intf); - if (!err) - usb_queue_reset_device(data->intf); - else - bt_dev_err(hdev, "Failed usb_autopm_get_interface with %d", err); + generic_usb_reset(hdev, data); } static inline void btusb_free_frags(struct btusb_data *data)