Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932149AbbFBBaD (ORCPT ); Mon, 1 Jun 2015 21:30:03 -0400 Received: from senator.holtmann.net ([87.106.208.187]:56232 "EHLO mail.holtmann.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754794AbbFBB2Y convert rfc822-to-8bit (ORCPT ); Mon, 1 Jun 2015 21:28:24 -0400 Content-Type: text/plain; charset=us-ascii Mime-Version: 1.0 (Mac OS X Mail 8.2 \(2098\)) Subject: Re: [PATCH 1/2] Bluetooth: Add reset_resume function From: Marcel Holtmann In-Reply-To: <1433207682-15064-1-git-send-email-labbott@fedoraproject.org> Date: Tue, 2 Jun 2015 03:28:14 +0200 Cc: Alan Stern , Takashi Iwai , Oliver Neukum , Ming Lei , "David S. Miller" , Johan Hedberg , "Rafael J. Wysocki" , "Gustavo F. Padovan" , BlueZ development , Linux Kernel Mailing List , USB list , netdev Content-Transfer-Encoding: 8BIT Message-Id: References: <1433207682-15064-1-git-send-email-labbott@fedoraproject.org> To: Laura Abbott X-Mailer: Apple Mail (2.2098) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4291 Lines: 104 Hi Laura, > Bluetooth devices off of some buses such as USB may lose power across > suspend/resume. When this happens, drivers may need to have the setup > function called again and behave differently than a cold power on. > Add a reset_resume function for drivers to call. During the > reset_resume case, the flag HCI_RESET_RESUME will be set to allow > drivers to differentate. > > Signed-off-by: Laura Abbott > --- > This matches with what hci_reset_dev does and also ensures > the setup function gets called again. > --- > include/net/bluetooth/hci.h | 1 + > include/net/bluetooth/hci_core.h | 1 + > net/bluetooth/hci_core.c | 16 ++++++++++++++++ > 3 files changed, 18 insertions(+) > > diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h > index d95da83..6285410 100644 > --- a/include/net/bluetooth/hci.h > +++ b/include/net/bluetooth/hci.h > @@ -185,6 +185,7 @@ enum { > HCI_RAW, > > HCI_RESET, > + HCI_RESET_RESUME, > }; no more addition to this list of flags please. These are userspace exposed flags and with that ABI that we are never ever touching again. If you need flags on a per device basis, then use the second list. > /* HCI socket flags */ > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h > index a056c2b..14f9c72 100644 > --- a/include/net/bluetooth/hci_core.h > +++ b/include/net/bluetooth/hci_core.h > @@ -941,6 +941,7 @@ int hci_register_dev(struct hci_dev *hdev); > void hci_unregister_dev(struct hci_dev *hdev); > int hci_suspend_dev(struct hci_dev *hdev); > int hci_resume_dev(struct hci_dev *hdev); > +int hci_reset_resume_dev(struct hci_dev *hdev); > int hci_reset_dev(struct hci_dev *hdev); > int hci_dev_open(__u16 dev); > int hci_dev_close(__u16 dev); > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c > index c4802f3..090762b 100644 > --- a/net/bluetooth/hci_core.c > +++ b/net/bluetooth/hci_core.c > @@ -1558,6 +1558,7 @@ static int hci_dev_do_close(struct hci_dev *hdev) > BT_DBG("%s %p", hdev->name, hdev); > > if (!hci_dev_test_flag(hdev, HCI_UNREGISTER) && > + !test_bit(HCI_RESET_RESUME, &hdev->flags) && > test_bit(HCI_UP, &hdev->flags)) { > /* Execute vendor specific shutdown routine */ > if (hdev->shutdown) > @@ -2110,6 +2111,7 @@ static void hci_power_on(struct work_struct *work) > */ > mgmt_index_added(hdev); > } > + hci_dev_test_and_clear_flag(hdev, HCI_RESET_RESUME); If you do not use the result of the test, why bother testing at all. Also you realize that you are a now clearing the flag on hdev->dev_flags and not hdev->flags. It also means that this code is not tested when you actually have had a reset resume and then get a clean power down. Not running the shutdown procedure would be actually wrong in that case. > } > > static void hci_power_off(struct work_struct *work) > @@ -3298,6 +3300,20 @@ int hci_reset_dev(struct hci_dev *hdev) > } > EXPORT_SYMBOL(hci_reset_dev); > > +/* > + * For USB reset_resume callbacks > + */ > +int hci_reset_resume_dev(struct hci_dev *hdev) > +{ > + set_bit(HCI_RESET_RESUME, &hdev->flags); > + hci_dev_do_close(hdev); > + hci_dev_set_flag(hdev, HCI_SETUP); > + > + queue_work(hdev->req_workqueue, &hdev->power_on); > + return 0; > +} > +EXPORT_SYMBOL(hci_reset_resume_dev); > + When we are reacting to a hardware error, we do hci_dev_do_close followed hci_dev_do_open. Why would you queue the power on work here. It sounds more like that this should be actually similar to hci_error_reset that gets queued. And this is where I said the really tricky part comes in. Is the device keeping the firmware or not. We really need to know that one first. If it keeps its firmware, then we do not need to run through hdev->setup again. >From a driver point of view, the current guarantee is that hdev->setup is only executed once. And this means really only once. It does not need to protect itself against being run again. So it should only be run again if the device looses all its states. Regards Marcel -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/