Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Sherry Sun <sherry.sun@nxp.com>
To:     Abhishek Pandit-Subedi <abhishekpandit@chromium.org>,
        "marcel@holtmann.org" <marcel@holtmann.org>,
        "johan.hedberg@gmail.com" <johan.hedberg@gmail.com>,
        "luiz.dentz@gmail.com" <luiz.dentz@gmail.com>
CC:     "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>
Subject: RE: BT large file transfer failed when do suspend/resume test
Thread-Topic: BT large file transfer failed when do suspend/resume test
Thread-Index: AdbkLWygZS16CF8vSlqlxcAFRAOtGQAMP8gAAA9HdQAA9mSRgAAT6+qQAVbVnIAAAeYKEA==
Date:   Tue, 19 Jan 2021 06:34:19 +0000
Message-ID: <VI1PR04MB4960710ABF79A6569E66107792A30@VI1PR04MB4960.eurprd04.prod.outlook.com>
References: <VI1PR04MB496003E449FB10B12959ADC292D00@VI1PR04MB4960.eurprd04.prod.outlook.com>
 <CANFp7mXtX4H=tQ5usRiOXRBzfuQORc6-MCznbjmsMBRAQMo36w@mail.gmail.com>
 <VI1PR04MB4960E51607240EC6B681526492AF0@VI1PR04MB4960.eurprd04.prod.outlook.com>
 <CANFp7mUWv1=SW_CL8YZgBP-emXC9aRXTeteE1+Ds7qrP3jWcBw@mail.gmail.com>
 <VI1PR04MB4960FFA7AB29E413CF7C8A3292AA0@VI1PR04MB4960.eurprd04.prod.outlook.com>
 <CANFp7mXeFOSGWLqPRoCGcH95u=m_W=HVn8Ey34u9SOqi31Xj7Q@mail.gmail.com>
In-Reply-To: <CANFp7mXeFOSGWLqPRoCGcH95u=m_W=HVn8Ey34u9SOqi31Xj7Q@mail.gmail.com>
Accept-Language: zh-CN, en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: a112224f-2c06-452f-3878-08d8bc44403d
X-MS-Exchange-CrossTenant-originalarrivaltime: 19 Jan 2021 06:34:19.3387
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: Ajgojl4kG6QVprg7mrfws3vveV5RSEEaOO0rszG8fKqfMJl/KO2fKQ1kGdXpE7xyv4p0uDbqnwSN7mhwgsVp4Q==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1PR04MB5917
Precedence: bulk
List-ID: <linux-bluetooth.vger.kernel.org>
X-Mailing-List: linux-bluetooth@vger.kernel.org

Hi Abhishek,

> > > > > >
> > > > > > Platform: L5.10 + Bluez5.55 + Marvell BT chip
> > > > > >
> > > > > > Background: Our test team usually try suspend/resume test when
> > > > > > transferring large file through BT, to see if the file
> > > > > > transfer can be continued
> > > > > after suspend/resume. It can works well before L5.10 But we
> > > > > found on L5.10, the BT connection lost if we try to
> > > > > suspend/resume, so the file transfer(through OBEX Object Push)
> > > > > shows failed. Then we found your patches when debugging.
> > > > > >
> > > > > > Questions:
> > > > > > 1. Before L5.10, kernel always keep BT connected during
> > > suspend/resume.
> > > > > So why we need to disconnect all the BT devices when system
> > > > > suspend
> > > now?
> > > > >
> > > > > Bluetooth has often been a source of spurious wakes in the past.
> > > > > Using rfkill or masking the wake interrupt were used in the past
> > > > > to make this more reliable but this was resulting in instability
> > > > > on the controller (controller needs to drop traffic if host is
> > > > > asleep and there's no
> > > clean way to do that).
> > > > >
> > > > > The new suspend behavior is the following:
> > > > > - All devices get disconnected during suspend.
> > > > > - Only HID devices can wake the device from suspend (i.e. Remote
> Wake).
> > > > > The BT controller will be configured to scan (page scan and/or
> > > > > LE passive
> > > > > scan) based on currently paired devices. If the device is not
> > > > > configured for wakeup (i.e. power/wakeup in sysfs is
> > > > > "disabled"), we will not configure this scanning either (see the
> > > > > hdev->prevent_wake implementation in btusb for an
> > > > > example)
> > > >
> > > > So for HID devices, they also been disconnected when suspend and
> > > > auto
> > > reconnected when resume, right?
> > >
> > > HID will not be auto-reconnected because the peripheral is
> > > responsible for reconnecting. Most HID devices do not page scan
> > > unless they are pairing (or at least that's what I've found in practi=
ce).
> >
> > What's the peripheral mean here?
> >
>=20
> The connected Bluetooth device (i.e. mouse, keyboard, etc).
>=20
> > >
> > > >
> > > > > - On resume, devices that support a2dp-sink will be
> > > > > automatically reconnected
> > > > >
> > > >
> > > > I just tried a2dp-sink devices(BT headphones), and it wasn't
> > > > reconnected
> > > after system resume.
> > > > I checked your patches which to supported a2dp-sink auto
> > > > reconnect, and
> > > found them may didn't been included in bluez5.55,  can you help
> > > confirm that?
> > >
> > > It looks like bluez5.55 was released on Sep 06
> > > (https://eur01.safelinks.protection.outlook.com/?url=3Dhttps%3A%2F%2F=
g
> > > it.ke
> > >
> rnel.org%2Fpub%2Fscm%2Fbluetooth%2Fbluez.git%2Ftag%2F%3Fh%3D5.55&
> > >
> amp;data=3D04%7C01%7Csherry.sun%40nxp.com%7C1f3ec177fe974c90e1a508
> > >
> d8b68c4584%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637460
> > >
> 060872077285%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJ
> > >
> QIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=3DEDn
> > >
> MnLjPIN29nQpmH1%2Fk%2BBnhMq5IZ3xG7BXIEq0xs6A%3D&amp;reserved
> > > =3D0) and my change was merged Sep 14
> > > (https://eur01.safelinks.protection.outlook.com/?url=3Dhttps%3A%2F%2F=
g
> > > it.ke
> > >
> rnel.org%2Fpub%2Fscm%2Fbluetooth%2Fbluez.git%2Fcommit%2F%3Fid%3D
> > >
> 6611b72600c370ec31795ab48a222594c4afb7ee&amp;data=3D04%7C01%7Cshe
> > >
> rry.sun%40nxp.com%7C1f3ec177fe974c90e1a508d8b68c4584%7C686ea1d3b
> > >
> c2b4c6fa92cd99c5c301635%7C0%7C0%7C637460060872077285%7CUnknown
> > > %7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1
> haW
> > >
> wiLCJXVCI6Mn0%3D%7C1000&amp;sdata=3D2jYeDwzoy87GA2V2SqilgdA502jeI
> > > ZdGd4aeyXH125M%3D&amp;reserved=3D0).
> > >
> >
> > Okay, got it, so you patches don't been included in bluez5.55.
> >
> > > >
> > > > > This results in more reliable behavior from Bluetooth around
> > > > > suspend while allowing Remote Wake to work properly.
> > > > >
> > > >
> > > > Yes, it' reasonable, but I think it may need to reconnect all the
> > > > devices
> > > which are disconnected due to suspend, right?
> > > > Otherwise for the user, when we connect the BT, which means we do
> > > > want to use the BT device, but every time when system get into
> > > > suspend, we will
> > > need to reconnect the device manually, it's really inconvenient.
> > >
> > > Depending on the profile implemented, not all peer devices will page
> > > scan once disconnected. HID devices at least will not page scan and
> > > are expected to initiate the reconnect themselves (i.e. suspend,
> > > resume and then click your mouse; the mouse will reconnect).
> > >
> > > If reconnecting to arbitrary profiles is desirable, you can merge
> > > this patch
> > > (https://eur01.safelinks.protection.outlook.com/?url=3Dhttps%3A%2F%2F=
p
> > > atch
> > >
> work.kernel.org%2Fproject%2Fbluetooth%2Fpatch%2F20201207154903.blue
> > >
> z.1.I3e043a481273442748bcff0728b2f0e208017cd2%40changeid%2F&amp;d
> > >
> ata=3D04%7C01%7Csherry.sun%40nxp.com%7C1f3ec177fe974c90e1a508d8b68
> > >
> c4584%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637460060872
> > >
> 077285%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2l
> > >
> uMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=3DaNXOfCTXs%
> > > 2BISAbYBH2die47qfpqXRmqehSnW1mC8M7U%3D&amp;reserved=3D0).
> > >
> >
> > But for the most Bluetooth profile, like A2DP-sink/A2DP-
> source/OPP/HFP/HSP, I think it is necessary to re-connect it, right?
> > So I think it's better to add the auto reconnect support for all those =
profiles
> instead of the A2DP a2dp-sink only here.
> > Please correct me if I'm wrong.
> >
>=20
> I think it should be user configurable and I'm not sure what defaults mak=
e
> sense for general Linux distributions. For ChromeOS, we only want a2dp-si=
nk.

I saw your patch series below didn't been accepted till now, right?
And I believe the ChromeOS is just a use case, for the general Linux, I don=
't think it's a good choice to add these profiles one by one when users nee=
d.
Since I'm not experienced in Bluetooth, maybe we need some more experienced=
 man to provide some reasonable suggestions.

Hi Marcel & Johan & Luiz, can you help to comment it, and the wakelock ques=
tion below?
@marcel@holtmann.org,@johan.hedberg@gmail.com,@luiz.dentz@gmail.com

>=20
> I've linked the patch series making it more configurable in previous emai=
ls
> (https://eur01.safelinks.protection.outlook.com/?url=3Dhttps%3A%2F%2Fpatc=
h
> work.kernel.org%2Fproject%2Fbluetooth%2Fpatch%2F20201207154903.blue
> z.1.I3e043a481273442748bcff0728b2f0e208017cd2%40changeid%2F&amp;d
> ata=3D04%7C01%7Csherry.sun%40nxp.com%7Cb06b00a1abc5495a3d2508d8bc
> 374c07%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63746629297
> 2854206%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV
> 2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=3Db5gZJ7bbD
> e51F0YfGdFQSjUAjLNwhh95jYJuGhsvhjo%3D&amp;reserved=3D0).
> If you agree that behavior is necessary, please make your case to the blu=
ez
> maintainer (Luiz).
>=20
> > > >
> > > > > > 2. I found that the device been disconnected due to suspend
> > > > > > won't been
> > > > > auto-connected after resume, shouldn't we get the BT device
> > > > > auto-connected after resume like wifi devices done?
> > > > >
> > > > > We do this currently only for Bluetooth headphones (reconnect on
> > > > > a2dp-sink service). I'm not familiar with Obex so I don't know
> > > > > if this would work for that as well. I did send up a patch
> > > > > making reconnect on resume configurable based on service uuid
> > > > > that may be relevant to
> > > > > this:
> > > > > https://eur01.safelinks.protection.outlook.com/?url=3Dhttps%3A%2F=
%
> > > > > 2Fpa
> > > > > tch
> > > > >
> > >
> work.kernel.org%2Fproject%2Fbluetooth%2Fpatch%2F20201207154903.blue
> > > > >
> > >
> z.1.I3e043a481273442748bcff0728b2f0e208017cd2%40changeid%2F&amp;d
> > > > >
> > >
> ata=3D04%7C01%7Csherry.sun%40nxp.com%7C9d41a6725ac1432355c408d8b2
> > > > >
> > >
> 759700%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6374555654
> > > > >
> > >
> 22685536%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoi
> > > > >
> > >
> V2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=3D6zZda8Yu
> > > > > RRbAMkwOxiSwjbJIpvi%2ByMSDWCzVkFkkpzs%3D&amp;reserved=3D0
> > > > >
> > > >
> > > > Why here only add a2dp-sink device auto reconnect support?
> > > > Why not add all the devices auto reconnect support which are
> > > disconnected due to suspend?
> > > >
> > > > > > 3. For the large file transfer, if the BT been disconnected
> > > > > > during suspend,
> > > > > the transfer will fail, do we have any methods to avoid this issu=
e?
> > > > >
> > > > > If you have an active transfer, does it make sense to be suspendi=
ng?
> > > > > Perhaps you should hold a wakelock while a transfer is ongoing.
> > > > > I am not sure how Obex and other services should behave here so
> > > > > I will defer to Luiz and Marcel's opinion on this topic.
> > > >
> > > > I'm not sure whether it's reasonable, our drivers seems can be
> > > > suspended
> > > during transfer file and A2DP playing.
> > > > Maybe we really need a wakelock here.
> > > >
> >
> > I checked the wakelock related code in kernel suspend steps, but it see=
ms
> that the wakelock cannot prevent the Bluetooth disconnect behavior.
> > Because when we run echo mem > /sys/power/state command to put the
> > system get into suspend mode, it will call pm_notifier_call_chain_robus=
t()
> in suspend_prepare(), the Bluetooth will be disconnected here as you add
> the pm_notifier callback.
> > In the next few steps, pm_wakeup_pending will be called to determine
> whether the suspend operation should be continued, if we registered the
> wakelock, we will not enter the suspend state.
> > But the Bluetooth has been disconnected already. Therefore, even if a
> wakelock is added, we will also meet this BT large file transfer fail iss=
ue. Am I
> right?
> >
>=20
> From what you've described, that seems to be the case. I would expect
> wakelocks to prevent suspend notifiers as well so this seems like a bug i=
n the
> suspend code. You should submit a patch to check that there isn't a pendi=
ng
> wakeup in enter_state since that's the case:
> https://eur01.safelinks.protection.outlook.com/?url=3Dhttps%3A%2F%2Fgithu=
b
> .com%2Ftorvalds%2Flinux%2Fblob%2Fmaster%2Fkernel%2Fpower%2Fsuspe
> nd.c%23L562&amp;data=3D04%7C01%7Csherry.sun%40nxp.com%7Cb06b00a1
> abc5495a3d2508d8bc374c07%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0
> %7C0%7C637466292972854206%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC
> 4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&
> amp;sdata=3DAhNB3i17HgSy9JoAqnD47oA5up19LY9fF8Zone90aqo%3D&amp;r
> eserved=3D0
>=20

I don't think it is a bug in the suspend code, it may need to call suspend =
notifier before wakelock checking due to some limitation.
I just wonder why we must do Bluetooth disconnect in suspend notifier, can =
we put the BT disconnect behavior later than wakelock checking?
Hi Marcel & Johan & Luiz, can you give some advices about this?

Best regards
Sherry