linux-kernel - Re: [PATCH] Bluetooth: btusb: Fix suspend issue for Realtek

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-ID: <0551C926975A174EA8972327741C7889EE73768F@RS-MBS01.realsil.com.cn>
Date:   Fri, 2 Aug 2019 09:24:42 +0000
From:   陆朱伟 <alex_lu@...lsil.com.cn>
To:     Marcel Holtmann <marcel@...tmann.org>
CC:     Johan Hedberg <johan.hedberg@...il.com>,
        "linux-bluetooth@...r.kernel.org" <linux-bluetooth@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Max Chou <max.chou@...ltek.com>
Subject: Re: [PATCH] Bluetooth: btusb: Fix suspend issue for Realtek

Hi Marcel,
Got it, thanks.
I will change the patch and resubmit it.

Regards

> Subject: Re: [PATCH] Bluetooth: btusb: Fix suspend issue for Realtek
> 
> Hi Alex,
> 
> > From the perspective of controller, global suspend means there is no
> > SET_FEATURE (DEVICE_REMOTE_WAKEUP) and controller would drop the
> > firmware. It would consume less power. So we should not send this kind
> > of SET_FEATURE when host goes to suspend state.
> > Otherwise, when making device enter selective suspend, host should send
> > SET_FEATURE to make sure the firmware remains.
> >
> > Signed-off-by: Alex Lu <alex_lu@...lsil.com.cn>
> > ---
> > drivers/bluetooth/btusb.c | 40
> +++++++++++++++++++++++++++++++++++----
> > 1 file changed, 36 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > index 50aed5259c2b..69f6b4208901 100644
> > --- a/drivers/bluetooth/btusb.c
> > +++ b/drivers/bluetooth/btusb.c
> > @@ -426,6 +426,7 @@ static const struct dmi_system_id
> btusb_needs_reset_resume_table[] = {
> > #define BTUSB_DIAG_RUNNING	10
> > #define BTUSB_OOB_WAKE_ENABLED	11
> > #define BTUSB_HW_RESET_ACTIVE	12
> > +#define BTUSB_QUIRK_SUSPEND	13
> 
> I think this name is not really descriptive. What about
> BTUSB_SUSPEND_DISABLE or BTUSB_WAKEUP_DISABLE or
> BTUSB_REMOTE_WAKEUP or something that is rather clear.
> 
> >
> > struct btusb_data {
> > 	struct hci_dev       *hdev;
> > @@ -1165,6 +1166,15 @@ static int btusb_open(struct hci_dev *hdev)
> > 	 */
> > 	device_wakeup_enable(&data->udev->dev);
> >
> > +	/* Disable device remote wakeup when host is suspended
> > +	 * For Realtek chips, global suspend without
> > +	 * SET_FEATURE (DEVICE_REMOTE_WAKEUP) can save more power
> in device.
> > +	 */
> > +#ifdef CONFIG_BT_HCIBTUSB_RTL
> > +	if (test_bit(BTUSB_QUIRK_SUSPEND, &data->flags))
> > +		device_wakeup_disable(&data->udev->dev);
> > +#endif
> > +
> 
> I have no idea what the #ifdef does here. Limiting it to HCIBTUSB_RTL seems
> pointless.
> 
> > 	if (test_and_set_bit(BTUSB_INTR_RUNNING, &data->flags))
> > 		goto done;
> >
> > @@ -1227,6 +1237,13 @@ static int btusb_close(struct hci_dev *hdev)
> > 		goto failed;
> >
> > 	data->intf->needs_remote_wakeup = 0;
> > +
> > +	/* Enable remote wake up for auto-suspend */
> > +#ifdef CONFIG_BT_HCIBTUSB_RTL
> > +	if (test_bit(BTUSB_QUIRK_SUSPEND, &data->flags))
> > +		data->intf->needs_remote_wakeup = 1;
> > +#endif
> > +
> 
> Same comment as above. No need for the #ifdef.
> 
> > 	device_wakeup_disable(&data->udev->dev);
> > 	usb_autopm_put_interface(data->intf);
> >
> > @@ -3185,11 +3202,11 @@ static int btusb_probe(struct usb_interface
> *intf,
> > 	if (id->driver_info & BTUSB_REALTEK) {
> > 		hdev->setup = btrtl_setup_realtek;
> >
> > -		/* Realtek devices lose their updated firmware over suspend,
> > -		 * but the USB hub doesn't notice any status change.
> > -		 * Explicitly request a device reset on resume.
> > +		/* Realtek devices lose their updated firmware over global
> > +		 * suspend that means host doesn't send SET_FEATURE
> > +		 * (DEVICE_REMOTE_WAKEUP)
> > 		 */
> > -		interface_to_usbdev(intf)->quirks |=
> USB_QUIRK_RESET_RESUME;
> > +		set_bit(BTUSB_QUIRK_SUSPEND, &data->flags);
> > 	}
> > #endif
> >
> > @@ -3363,6 +3380,21 @@ static int btusb_suspend(struct usb_interface
> *intf, pm_message_t message)
> > 		enable_irq(data->oob_wake_irq);
> > 	}
> >
> > +#ifdef CONFIG_BT_HCIBTUSB_RTL
> > +	/* For global suspend, Realtek devices lose the loaded fw
> > +	 * in them. But for autosuspend, firmware should remain.
> > +	 * Actually, it depends on whether the usb host sends
> > +	 * set feature (enable wakeup) or not.
> > +	 */
> > +	if (test_bit(BTUSB_QUIRK_SUSPEND, &data->flags)) {
> > +		if (PMSG_IS_AUTO(message) &&
> > +		    device_can_wakeup(&data->udev->dev))
> > +			data->udev->do_remote_wakeup = 1;
> > +		else if (!PMSG_IS_AUTO(message))
> > +			data->udev->reset_resume = 1;
> > +	}
> > +#endif
> > +
> 
> The #ifdef is also not needed here.
> 
> Regards
> 
> Marcel
> 
> 
> ------Please consider the environment before printing this e-mail.