Re: [PATCH 4.4 095/108] Bluetooth: btusb: Restore QCA Rome suspend/resume fix with a "rewritten" version
On Thu, Mar 22, 2018 at 11:56:35AM -0700, Guenter Roeck wrote: > On Thu, Mar 22, 2018 at 06:52:51PM +0100, Greg Kroah-Hartman wrote: > > [ ... ] > > > > And that't the point to drive home here. If you stay away from updating > > to stable patches, you have a huge boatload of KNOWN SECURITY HOLES in > > your product. If you take them, you have the _possiblity_ of some bugs > > added, but overall, the rate is _VERY_ small. Guenter has numbers of > > 2-4 patches per year cause problems. That's lower than ANY other > > development model I have ever seen anywhere. > > > Unfortunately, people tend to be irrational. Yes, the regression rate I have > observed is in the 0.1..0.15% range for v4.4.y and v4.14.y. Yet, there are > still people who believe that we should not merge stable releases due to the > regressions it causes (though they are much less vocal nowadays). > > So, stick with known buggy/insecure devices? Or take the updates and > > handle the 1-2 problems a year they provide you. I think the > > cost-analysis is easy to make here :) > > > > Agreed, on an objective basis. Unfortunately, one does not get credit for > fixing bugs which have never been observed in the field because they have > been fixed before they showed up. But one _does_ get blame for regressions. Someone has half-way joked that they were going to turn an intern on the stable releases and get a CVE assigned for every patch in them. Just to highlight just how many "real" things we are fixing before anyone notices. Some days I think that is going to be the only way people pay attention :( > Even though there have been very few regressions in absolute numbers, the > default reaction to newly observed problems is "it must be due to a stable > release merge", even though it almost always turns out to be incorrect. > > The only way to deal with that is to reduce regressions to 0, or as close > to 0 as possible. 0.1% is good, but not good enough. For some platforms, it is 0%. Facebook has published numbers showing this for a 2 year run of stable kernel releases. When you start dealing with crazy embedded/odd hardware platforms, the numbers does go up, just because no one is testing those platforms before I do a release. Hence the push to do the testing on the real hardware, which is why kernel.ci and Linaro are now doing this. If you note, we also have people doing merges on their phones, and I get private emails from a number of SoC companies showing that their merge/test cycle worked as well. And one note from that SoC testing, in the past 6 months since it has started, I have _NO_ reported regressions on any stable release so far. Not bad... > Also, while I agree that we are much better off in respect to security, > the verdict is still out if stable release merges actually improve release > stability; I don't see a clear trend even with chromeos-4.4. Of course, > it is all but impossible to say if this is due to 4.4.y or due to the > 13,000+ patches we have on top of v4.4.y in chromeos-4.4. Yeah, _THATS_ the major issue here. The interaction of the 3+million lines of out-of-tree crazyness in device trees still scares me. But, as the SoCs are now reporting, so far it's going well, but it's only been 6 months. But it has been an "interesting" 6 months :) As for "improve" stability, well, given that we are fixing known-root-holes, yes, that does increase stability. Again, I can crash any phone shipping today except for 2 of them, because those 2 updated to newer kernel versions. Do I need to start publishing reproducers? Actually, along those lines, I have seen people start putting tests for reported kernel bugs into some regression tests. When those start being more popular (i.e. people start running them on devices that are not updated), then you will start to see the reports of "instability". Oh well, back to patch reviewing, I'm preaching to the choir here... thanks, greg k-h
Re: [PATCH 4.4 095/108] Bluetooth: btusb: Restore QCA Rome suspend/resume fix with a "rewritten" version
On Thu, Mar 22, 2018 at 06:52:51PM +0100, Greg Kroah-Hartman wrote: [ ... ] > > And that't the point to drive home here. If you stay away from updating > to stable patches, you have a huge boatload of KNOWN SECURITY HOLES in > your product. If you take them, you have the _possiblity_ of some bugs > added, but overall, the rate is _VERY_ small. Guenter has numbers of > 2-4 patches per year cause problems. That's lower than ANY other > development model I have ever seen anywhere. > Unfortunately, people tend to be irrational. Yes, the regression rate I have observed is in the 0.1..0.15% range for v4.4.y and v4.14.y. Yet, there are still people who believe that we should not merge stable releases due to the regressions it causes (though they are much less vocal nowadays). > So, stick with known buggy/insecure devices? Or take the updates and > handle the 1-2 problems a year they provide you. I think the > cost-analysis is easy to make here :) > Agreed, on an objective basis. Unfortunately, one does not get credit for fixing bugs which have never been observed in the field because they have been fixed before they showed up. But one _does_ get blame for regressions. Even though there have been very few regressions in absolute numbers, the default reaction to newly observed problems is "it must be due to a stable release merge", even though it almost always turns out to be incorrect. The only way to deal with that is to reduce regressions to 0, or as close to 0 as possible. 0.1% is good, but not good enough. Also, while I agree that we are much better off in respect to security, the verdict is still out if stable release merges actually improve release stability; I don't see a clear trend even with chromeos-4.4. Of course, it is all but impossible to say if this is due to 4.4.y or due to the 13,000+ patches we have on top of v4.4.y in chromeos-4.4. Guenter
Re: [PATCH 4.4 095/108] Bluetooth: btusb: Restore QCA Rome suspend/resume fix with a "rewritten" version
On Wed, Feb 28, 2018 at 11:39:56AM -0800, Brian Norris wrote:
> Hi Greg,
Hi,
Sorry for the delay, wanted to think about this one for a while...
> ...
> >> > Thanks, but I've now released all of these with this patch committed, so
> >> > we are now "bug compatible" :)
>
> So, is that to say that the boilerplate above about objections is
> meaningless? This is the second time that this same "feature" has been
> pushed (degrading the quality of my systems) despite my objections,
> under the banner of "bug compatibility" [1]. The first attempt to
> revert was back around Dec 20 of last year, but I see that there were
> 10 "stable" 4.4 kernels released in the meantime [2] where that
> original bug was still present. (Commit fd865802c66b "Bluetooth:
> btusb: fix QCA Rome suspend/resume" was proven undeniably buggy.)
Sorry, I know you were frustrated, but for some subsystems/minor devices
like this, regressions happen and getting them fixed properly can take a
few weeks.
And yes, something doesn't feel "minor" when it affects your devices but
really you have the control here to revert the change on your side (more
on that below...)
> Next: we see this current valiant attempt at a less buggy fix, by
> Hans. It's an OK solution, but it still wastes power for me. I
> objected above, but instead of delaying applying it, you applied it in
> the same release as you finally fixed the original crap (v4.4.116). So
> all-in-all, my system (if using 4.4.x directly) hasn't had decent
> Bluetooth since v4.4.99.
I'm amazed bluetooth works at all at times, given the mess of the
hardware involved, and the horrid spec and all of the intermediate
pieces. Luckily 4.15+ seems really good for me now, but I know you
can't upgrade :)
That being said, some subsystems have problems with stuff like this due
to crazy hardware that one fix breaks another and the like. There is
also the issue of maintainers that don't work on the subsystem "full" or
even "part" time. From my side, I submitted a known-security-bugfix and
it was ignored by the bluetooth maintainers for weeks, so I had to route
around them and push it directly to Linus just to get it fixed. So I
feel your pain, but we are dealing with people with different
priorities, none of which we directly control, so we have to handle it
the best we can.
In the end, it's amazing any of this works at all, but it does, it just
sometimes takes longer than we all like :)
> > It's a tough trade-off. If I dropped this patch, the normal mode of
> > operation would be for it to get merged into device kernels and then
> > forgotten about. Only if/when the user with the problem moves to a
> > newer release a long time later would the regression normally appear
> > again, and everyone would have to remember what happened and try to
> > piece it all together again as to what commit caused the issue.
>
> Note that I didn't suggest we have to completely drop the patch. And I
> also don't suspect you need to delay all -rc1 bugfixes. I'd just
> suggest delaying the patch for a few weeks, when there are objections
> raised. (Or, reverting and scheduling to re-queue in a few weeks if no
> progress...or something like that.) Is that not something that could
> work, in order to keep "stable" releases *actually* stable? In most
> software release processes, buggy patches are reverted as quickly as
> possible while alternatives are worked out. Not all fixes are security
> fixes that need to be out the door as soon as they see the light of
> day...
Having the "bug compatible" stable kernels is controversial. And I
don't always follow that rule, depending on the subsystem/bug involved
(see a recent btrfs bug for one such example.) That being said, I have
found that it is the best thing to do overall, as it provides the needed
pressure on the developer/maintainer/user to get the bug fixed and
pushed to Linus as soon as possible.
And in the meantime, if you, as a user, knows the patch in problem, you
can always revert it on your own. We all have local patches, you more
than me, but that's just part of dealing with open source projects. Not
a big deal at all, add the revert to your stack, when the bug gets
fixed you drop your patch and all is good/fine.
If you aren't using tools to make this easier, well that can be fixed (I
strongly recommend quilt, not git, to work with a device kernel, but
that's another long rant/email for another time...)
> > By you adding the revert to your device kernel now, you have a record of
> > this being a problem, how upstream isn't fixing the issue, and when/if
> > you do move to a newer kernel, that bugfix will still be there in your
> > patch stack to forward port.
>
> So, you rely entirely on device kernels to manage the pain that your
> release process causes? We're actively trying to stay much closer to
> upstream these days, and would essentially like to eliminate the
> concept of "device" kernels, at least for Chrom{e,ium} OS, if
> possible. But it's c
Re: [PATCH 4.4 095/108] Bluetooth: btusb: Restore QCA Rome suspend/resume fix with a "rewritten" version
Hi Greg,
On Sat, Feb 17, 2018 at 7:24 AM, Greg Kroah-Hartman
wrote:
> On Sat, Feb 17, 2018 at 07:12:17AM -0800, Guenter Roeck wrote:
>> On 02/17/2018 05:43 AM, Greg Kroah-Hartman wrote:
>> > On Fri, Feb 16, 2018 at 10:52:20AM -0800, Guenter Roeck wrote:
>> > > On Fri, Feb 16, 2018 at 10:10:44AM -0800, Brian Norris wrote:
>> > > > On Fri, Feb 16, 2018 at 07:48:50AM +0100, Greg Kroah-Hartman wrote:
>> > > > > On Thu, Feb 15, 2018 at 06:31:48PM -0800, Brian Norris wrote:
>> > > > > > On Thu, Feb 15, 2018 at 04:17:32PM +0100, Greg Kroah-Hartman wrote:
>> > > > > > > 4.4-stable review patch. If anyone has any objections, please
>> > > > > > > let me know.
>> > > > > >
>> > > > > > Consider this an objection:
>> > > > > >
>> > > > > > I'm currently arguing that this is unnecessarily regressing power
>> > > > > > consumption here:
>> > > > > >
>> > > > > > https://patchwork.kernel.org/patch/10149195/
>> > > > > >
>> > > > > > I'll leave it up to you what to do with this, but if this ends up
>> > > > > > in
>> > > > > > Chromium OS kernels, I'm likely to revert it there...
...
>> > Thanks, but I've now released all of these with this patch committed, so
>> > we are now "bug compatible" :)
So, is that to say that the boilerplate above about objections is
meaningless? This is the second time that this same "feature" has been
pushed (degrading the quality of my systems) despite my objections,
under the banner of "bug compatibility" [1]. The first attempt to
revert was back around Dec 20 of last year, but I see that there were
10 "stable" 4.4 kernels released in the meantime [2] where that
original bug was still present. (Commit fd865802c66b "Bluetooth:
btusb: fix QCA Rome suspend/resume" was proven undeniably buggy.)
Next: we see this current valiant attempt at a less buggy fix, by
Hans. It's an OK solution, but it still wastes power for me. I
objected above, but instead of delaying applying it, you applied it in
the same release as you finally fixed the original crap (v4.4.116). So
all-in-all, my system (if using 4.4.x directly) hasn't had decent
Bluetooth since v4.4.99.
At least things are still moving forward here, and maybe in another
month, I can expect a v4.4.x stable kernel that works well. But the
hilarious current state of things is that we're basically going back
to a no-op for the time being:
https://marc.info/?l=linux-bluetooth&m=151981547905651&w=2
https://marc.info/?l=linux-bluetooth&m=151981548105654&w=2
[PATCH] Bluetooth: btusb: Remove Yoga 920 from the
btusb_needs_reset_resume_table
(I know others are looking at properly identifying a DMI match list
still, so this won't stay a no-op.)
>> FWIW, seems to me that trying to be "bug compatible" with -rc1 upstream
>> kernels may not really be a good idea for stable releases.
I couldn't agree more.
> It's a tough trade-off. If I dropped this patch, the normal mode of
> operation would be for it to get merged into device kernels and then
> forgotten about. Only if/when the user with the problem moves to a
> newer release a long time later would the regression normally appear
> again, and everyone would have to remember what happened and try to
> piece it all together again as to what commit caused the issue.
Note that I didn't suggest we have to completely drop the patch. And I
also don't suspect you need to delay all -rc1 bugfixes. I'd just
suggest delaying the patch for a few weeks, when there are objections
raised. (Or, reverting and scheduling to re-queue in a few weeks if no
progress...or something like that.) Is that not something that could
work, in order to keep "stable" releases *actually* stable? In most
software release processes, buggy patches are reverted as quickly as
possible while alternatives are worked out. Not all fixes are security
fixes that need to be out the door as soon as they see the light of
day...
> By you adding the revert to your device kernel now, you have a record of
> this being a problem, how upstream isn't fixing the issue, and when/if
> you do move to a newer kernel, that bugfix will still be there in your
> patch stack to forward port.
So, you rely entirely on device kernels to manage the pain that your
release process causes? We're actively trying to stay much closer to
upstream these days, and would essentially like to eliminate the
concept of "device" kernels, at least for Chrom{e,ium} OS, if
possible. But it's crap like this that proves that we can't.
> Yeah, you all are normally better than that, and I trust that you will
> push to get this resolved, hopefully soon. But for the most part, this
> method works best overall for the majority of the cases like this as not
> all bug reporters are persistent, and if not, the maintainer usually
> forgets about it as no one is saying anything and they have other things
> to work on.
>
> Well, bluetooth is known to not have responsive maintainers, so who am I
> kidding here, odds are it's only going to get fixed as Hans is
> involved, despite the bluetooth
Re: [PATCH 4.4 095/108] Bluetooth: btusb: Restore QCA Rome suspend/resume fix with a "rewritten" version
On Sat, Feb 17, 2018 at 07:12:17AM -0800, Guenter Roeck wrote: > On 02/17/2018 05:43 AM, Greg Kroah-Hartman wrote: > > On Fri, Feb 16, 2018 at 10:52:20AM -0800, Guenter Roeck wrote: > > > On Fri, Feb 16, 2018 at 10:10:44AM -0800, Brian Norris wrote: > > > > On Fri, Feb 16, 2018 at 07:48:50AM +0100, Greg Kroah-Hartman wrote: > > > > > On Thu, Feb 15, 2018 at 06:31:48PM -0800, Brian Norris wrote: > > > > > > On Thu, Feb 15, 2018 at 04:17:32PM +0100, Greg Kroah-Hartman wrote: > > > > > > > 4.4-stable review patch. If anyone has any objections, please > > > > > > > let me know. > > > > > > > > > > > > Consider this an objection: > > > > > > > > > > > > I'm currently arguing that this is unnecessarily regressing power > > > > > > consumption here: > > > > > > > > > > > > https://patchwork.kernel.org/patch/10149195/ > > > > > > > > > > > > I'll leave it up to you what to do with this, but if this ends up in > > > > > > Chromium OS kernels, I'm likely to revert it there... > > > > > > > > > > Is that patch in Linus's tree yet? If so, I'll be glad to also apply > > > > > it > > > > > here. > > > > > > > > The link is the original patch, where I'm (too late?) complaining about > > > > its side effects. Hans and Marcel are discussing potential alternatives. > > > > This stuff happens in -rc kernels. But you're already ready to push it > > > > out to -stable users? I can try to push another few reverts into Linus's > > > > tree if that really helps, or else you can wait on pushing these to > > > > -stable until 4.16 settles down. > > > > > > FWIW, here are the various commit SHAs. > > > > > > Upstream: 61f5acea8737 > > > v4.15 (queued for v4.15.4): e766a2d7f7c2 > > > v4.14 (queued for v4.14.20): 736385472dfa > > > v4.9 (queued for v4.9.82):1c6fc2167678 > > > v4.4 (queued for v4.4.116): 575538a5371d > > > > > > I didn't check older stable kernels. > > > > Thanks, but I've now released all of these with this patch committed, so > > we are now "bug compatible" :) > > > > FWIW, seems to me that trying to be "bug compatible" with -rc1 upstream > kernels may not really be a good idea for stable releases. It's a tough trade-off. If I dropped this patch, the normal mode of operation would be for it to get merged into device kernels and then forgotten about. Only if/when the user with the problem moves to a newer release a long time later would the regression normally appear again, and everyone would have to remember what happened and try to piece it all together again as to what commit caused the issue. By you adding the revert to your device kernel now, you have a record of this being a problem, how upstream isn't fixing the issue, and when/if you do move to a newer kernel, that bugfix will still be there in your patch stack to forward port. Yeah, you all are normally better than that, and I trust that you will push to get this resolved, hopefully soon. But for the most part, this method works best overall for the majority of the cases like this as not all bug reporters are persistent, and if not, the maintainer usually forgets about it as no one is saying anything and they have other things to work on. Well, bluetooth is known to not have responsive maintainers, so who am I kidding here, odds are it's only going to get fixed as Hans is involved, despite the bluetooth maintainers :) thanks, greg k-h
Re: [PATCH 4.4 095/108] Bluetooth: btusb: Restore QCA Rome suspend/resume fix with a "rewritten" version
On 02/17/2018 05:43 AM, Greg Kroah-Hartman wrote: On Fri, Feb 16, 2018 at 10:52:20AM -0800, Guenter Roeck wrote: On Fri, Feb 16, 2018 at 10:10:44AM -0800, Brian Norris wrote: On Fri, Feb 16, 2018 at 07:48:50AM +0100, Greg Kroah-Hartman wrote: On Thu, Feb 15, 2018 at 06:31:48PM -0800, Brian Norris wrote: On Thu, Feb 15, 2018 at 04:17:32PM +0100, Greg Kroah-Hartman wrote: 4.4-stable review patch. If anyone has any objections, please let me know. Consider this an objection: I'm currently arguing that this is unnecessarily regressing power consumption here: https://patchwork.kernel.org/patch/10149195/ I'll leave it up to you what to do with this, but if this ends up in Chromium OS kernels, I'm likely to revert it there... Is that patch in Linus's tree yet? If so, I'll be glad to also apply it here. The link is the original patch, where I'm (too late?) complaining about its side effects. Hans and Marcel are discussing potential alternatives. This stuff happens in -rc kernels. But you're already ready to push it out to -stable users? I can try to push another few reverts into Linus's tree if that really helps, or else you can wait on pushing these to -stable until 4.16 settles down. FWIW, here are the various commit SHAs. Upstream: 61f5acea8737 v4.15 (queued for v4.15.4): e766a2d7f7c2 v4.14 (queued for v4.14.20):736385472dfa v4.9 (queued for v4.9.82): 1c6fc2167678 v4.4 (queued for v4.4.116): 575538a5371d I didn't check older stable kernels. Thanks, but I've now released all of these with this patch committed, so we are now "bug compatible" :) FWIW, seems to me that trying to be "bug compatible" with -rc1 upstream kernels may not really be a good idea for stable releases. Guenter
Re: [PATCH 4.4 095/108] Bluetooth: btusb: Restore QCA Rome suspend/resume fix with a "rewritten" version
On Fri, Feb 16, 2018 at 10:52:20AM -0800, Guenter Roeck wrote: > On Fri, Feb 16, 2018 at 10:10:44AM -0800, Brian Norris wrote: > > On Fri, Feb 16, 2018 at 07:48:50AM +0100, Greg Kroah-Hartman wrote: > > > On Thu, Feb 15, 2018 at 06:31:48PM -0800, Brian Norris wrote: > > > > On Thu, Feb 15, 2018 at 04:17:32PM +0100, Greg Kroah-Hartman wrote: > > > > > 4.4-stable review patch. If anyone has any objections, please let me > > > > > know. > > > > > > > > Consider this an objection: > > > > > > > > I'm currently arguing that this is unnecessarily regressing power > > > > consumption here: > > > > > > > > https://patchwork.kernel.org/patch/10149195/ > > > > > > > > I'll leave it up to you what to do with this, but if this ends up in > > > > Chromium OS kernels, I'm likely to revert it there... > > > > > > Is that patch in Linus's tree yet? If so, I'll be glad to also apply it > > > here. > > > > The link is the original patch, where I'm (too late?) complaining about > > its side effects. Hans and Marcel are discussing potential alternatives. > > This stuff happens in -rc kernels. But you're already ready to push it > > out to -stable users? I can try to push another few reverts into Linus's > > tree if that really helps, or else you can wait on pushing these to > > -stable until 4.16 settles down. > > FWIW, here are the various commit SHAs. > > Upstream: 61f5acea8737 > v4.15 (queued for v4.15.4): e766a2d7f7c2 > v4.14 (queued for v4.14.20): 736385472dfa > v4.9 (queued for v4.9.82):1c6fc2167678 > v4.4 (queued for v4.4.116): 575538a5371d > > I didn't check older stable kernels. Thanks, but I've now released all of these with this patch committed, so we are now "bug compatible" :) Please work to get this resolved in Linus's tree and I will be glad to backport the result. thanks, greg k-h
Re: [PATCH 4.4 095/108] Bluetooth: btusb: Restore QCA Rome suspend/resume fix with a "rewritten" version
On Fri, Feb 16, 2018 at 10:10:44AM -0800, Brian Norris wrote: > On Fri, Feb 16, 2018 at 07:48:50AM +0100, Greg Kroah-Hartman wrote: > > On Thu, Feb 15, 2018 at 06:31:48PM -0800, Brian Norris wrote: > > > On Thu, Feb 15, 2018 at 04:17:32PM +0100, Greg Kroah-Hartman wrote: > > > > 4.4-stable review patch. If anyone has any objections, please let me > > > > know. > > > > > > Consider this an objection: > > > > > > I'm currently arguing that this is unnecessarily regressing power > > > consumption here: > > > > > > https://patchwork.kernel.org/patch/10149195/ > > > > > > I'll leave it up to you what to do with this, but if this ends up in > > > Chromium OS kernels, I'm likely to revert it there... > > > > Is that patch in Linus's tree yet? If so, I'll be glad to also apply it > > here. > > The link is the original patch, where I'm (too late?) complaining about > its side effects. Hans and Marcel are discussing potential alternatives. > This stuff happens in -rc kernels. But you're already ready to push it > out to -stable users? I can try to push another few reverts into Linus's > tree if that really helps, or else you can wait on pushing these to > -stable until 4.16 settles down. I can drop this for now, but I really like to be "bug compatible" with Linus's tree if at all possible. That keeps the pressure on people to get Linus's tree fixed :) I'll drop this if the maintainer tells me to do so... thanks, greg k-h
Re: [PATCH 4.4 095/108] Bluetooth: btusb: Restore QCA Rome suspend/resume fix with a "rewritten" version
On Fri, Feb 16, 2018 at 10:10:44AM -0800, Brian Norris wrote: > On Fri, Feb 16, 2018 at 07:48:50AM +0100, Greg Kroah-Hartman wrote: > > On Thu, Feb 15, 2018 at 06:31:48PM -0800, Brian Norris wrote: > > > On Thu, Feb 15, 2018 at 04:17:32PM +0100, Greg Kroah-Hartman wrote: > > > > 4.4-stable review patch. If anyone has any objections, please let me > > > > know. > > > > > > Consider this an objection: > > > > > > I'm currently arguing that this is unnecessarily regressing power > > > consumption here: > > > > > > https://patchwork.kernel.org/patch/10149195/ > > > > > > I'll leave it up to you what to do with this, but if this ends up in > > > Chromium OS kernels, I'm likely to revert it there... > > > > Is that patch in Linus's tree yet? If so, I'll be glad to also apply it > > here. > > The link is the original patch, where I'm (too late?) complaining about > its side effects. Hans and Marcel are discussing potential alternatives. > This stuff happens in -rc kernels. But you're already ready to push it > out to -stable users? I can try to push another few reverts into Linus's > tree if that really helps, or else you can wait on pushing these to > -stable until 4.16 settles down. FWIW, here are the various commit SHAs. Upstream: 61f5acea8737 v4.15 (queued for v4.15.4): e766a2d7f7c2 v4.14 (queued for v4.14.20):736385472dfa v4.9 (queued for v4.9.82): 1c6fc2167678 v4.4 (queued for v4.4.116): 575538a5371d I didn't check older stable kernels. Guenter
Re: [PATCH 4.4 095/108] Bluetooth: btusb: Restore QCA Rome suspend/resume fix with a "rewritten" version
On Fri, Feb 16, 2018 at 07:48:50AM +0100, Greg Kroah-Hartman wrote: > On Thu, Feb 15, 2018 at 06:31:48PM -0800, Brian Norris wrote: > > On Thu, Feb 15, 2018 at 04:17:32PM +0100, Greg Kroah-Hartman wrote: > > > 4.4-stable review patch. If anyone has any objections, please let me > > > know. > > > > Consider this an objection: > > > > I'm currently arguing that this is unnecessarily regressing power > > consumption here: > > > > https://patchwork.kernel.org/patch/10149195/ > > > > I'll leave it up to you what to do with this, but if this ends up in > > Chromium OS kernels, I'm likely to revert it there... > > Is that patch in Linus's tree yet? If so, I'll be glad to also apply it > here. The link is the original patch, where I'm (too late?) complaining about its side effects. Hans and Marcel are discussing potential alternatives. This stuff happens in -rc kernels. But you're already ready to push it out to -stable users? I can try to push another few reverts into Linus's tree if that really helps, or else you can wait on pushing these to -stable until 4.16 settles down. Or you can ignore my objection. But I don't really like that option ;) Brian
Re: [PATCH 4.4 095/108] Bluetooth: btusb: Restore QCA Rome suspend/resume fix with a "rewritten" version
On Thu, Feb 15, 2018 at 06:31:48PM -0800, Brian Norris wrote: > On Thu, Feb 15, 2018 at 04:17:32PM +0100, Greg Kroah-Hartman wrote: > > 4.4-stable review patch. If anyone has any objections, please let me know. > > Consider this an objection: > > I'm currently arguing that this is unnecessarily regressing power > consumption here: > > https://patchwork.kernel.org/patch/10149195/ > > I'll leave it up to you what to do with this, but if this ends up in > Chromium OS kernels, I'm likely to revert it there... Is that patch in Linus's tree yet? If so, I'll be glad to also apply it here. thanks, greg k-h
Re: [PATCH 4.4 095/108] Bluetooth: btusb: Restore QCA Rome suspend/resume fix with a "rewritten" version
On Thu, Feb 15, 2018 at 04:17:32PM +0100, Greg Kroah-Hartman wrote:
> 4.4-stable review patch. If anyone has any objections, please let me know.
Consider this an objection:
I'm currently arguing that this is unnecessarily regressing power
consumption here:
https://patchwork.kernel.org/patch/10149195/
I'll leave it up to you what to do with this, but if this ends up in
Chromium OS kernels, I'm likely to revert it there...
Brian
> --
>
> From: Hans de Goede
>
> commit 61f5acea8737d9b717fcc22bb6679924f3c82b98 upstream.
>
> Commit 7d06d5895c15 ("Revert "Bluetooth: btusb: fix QCA...suspend/resume"")
> removed the setting of the BTUSB_RESET_RESUME quirk for QCA Rome devices,
> instead favoring adding USB_QUIRK_RESET_RESUME quirks in usb/core/quirks.c.
>
> This was done because the DIY BTUSB_RESET_RESUME reset-resume handling
> has several issues (see the original commit message). An added advantage
> of moving over to the USB-core reset-resume handling is that it also
> disables autosuspend for these devices, which is similarly broken on these.
>
> But there are 2 issues with this approach:
> 1) It leaves the broken DIY BTUSB_RESET_RESUME code in place for Realtek
>devices.
> 2) Sofar only 2 of the 10 QCA devices known to the btusb code have been
>added to usb/core/quirks.c and if we fix the Realtek case the same way
>we need to add an additional 14 entries. So in essence we need to
>duplicate a large part of the usb_device_id table in btusb.c in
>usb/core/quirks.c and manually keep them in sync.
>
> This commit instead restores setting a reset-resume quirk for QCA devices
> in the btusb.c code, avoiding the duplicate usb_device_id table problem.
>
> This commit avoids the problems with the original DIY BTUSB_RESET_RESUME
> code by simply setting the USB_QUIRK_RESET_RESUME quirk directly on the
> usb_device.
>
> This commit also moves the BTUSB_REALTEK case over to directly setting the
> USB_QUIRK_RESET_RESUME on the usb_device and removes the now unused
> BTUSB_RESET_RESUME code.
>
> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1514836
> Fixes: 7d06d5895c15 ("Revert "Bluetooth: btusb: fix QCA...suspend/resume"")
> Cc: Leif Liddy
> Cc: Matthias Kaehlcke
> Cc: Brian Norris
> Cc: Daniel Drake
> Cc: Kai-Heng Feng
> Signed-off-by: Hans de Goede
> Signed-off-by: Marcel Holtmann
> Signed-off-by: Greg Kroah-Hartman
>
> ---
> drivers/bluetooth/btusb.c | 21 ++---
> 1 file changed, 10 insertions(+), 11 deletions(-)
>
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -23,6 +23,7 @@
>
> #include
> #include
> +#include
> #include
> #include
>
> @@ -360,8 +361,8 @@ static const struct usb_device_id blackl
> #define BTUSB_FIRMWARE_LOADED7
> #define BTUSB_FIRMWARE_FAILED8
> #define BTUSB_BOOTING9
> -#define BTUSB_RESET_RESUME 10
> -#define BTUSB_DIAG_RUNNING 11
> +#define BTUSB_DIAG_RUNNING 10
> +#define BTUSB_OOB_WAKE_ENABLED 11
>
> struct btusb_data {
> struct hci_dev *hdev;
> @@ -2969,6 +2970,12 @@ static int btusb_probe(struct usb_interf
> if (id->driver_info & BTUSB_QCA_ROME) {
> data->setup_on_usb = btusb_setup_qca;
> hdev->set_bdaddr = btusb_set_bdaddr_ath3012;
> +
> + /* QCA Rome devices lose their updated firmware over suspend,
> + * but the USB hub doesn't notice any status change.
> + * explicitly request a device reset on resume.
> + */
> + interface_to_usbdev(intf)->quirks |= USB_QUIRK_RESET_RESUME;
> }
>
> #ifdef CONFIG_BT_HCIBTUSB_RTL
> @@ -2979,7 +2986,7 @@ static int btusb_probe(struct usb_interf
>* but the USB hub doesn't notice any status change.
>* Explicitly request a device reset on resume.
>*/
> - set_bit(BTUSB_RESET_RESUME, &data->flags);
> + interface_to_usbdev(intf)->quirks |= USB_QUIRK_RESET_RESUME;
> }
> #endif
>
> @@ -3136,14 +3143,6 @@ static int btusb_suspend(struct usb_inte
> btusb_stop_traffic(data);
> usb_kill_anchored_urbs(&data->tx_anchor);
>
> - /* Optionally request a device reset on resume, but only when
> - * wakeups are disabled. If wakeups are enabled we assume the
> - * device will stay powered up throughout suspend.
> - */
> - if (test_bit(BTUSB_RESET_RESUME, &data->flags) &&
> - !device_may_wakeup(&data->udev->dev))
> - data->udev->reset_resume = 1;
> -
> return 0;
> }
>
>
>
[PATCH 4.4 095/108] Bluetooth: btusb: Restore QCA Rome suspend/resume fix with a "rewritten" version
4.4-stable review patch. If anyone has any objections, please let me know.
--
From: Hans de Goede
commit 61f5acea8737d9b717fcc22bb6679924f3c82b98 upstream.
Commit 7d06d5895c15 ("Revert "Bluetooth: btusb: fix QCA...suspend/resume"")
removed the setting of the BTUSB_RESET_RESUME quirk for QCA Rome devices,
instead favoring adding USB_QUIRK_RESET_RESUME quirks in usb/core/quirks.c.
This was done because the DIY BTUSB_RESET_RESUME reset-resume handling
has several issues (see the original commit message). An added advantage
of moving over to the USB-core reset-resume handling is that it also
disables autosuspend for these devices, which is similarly broken on these.
But there are 2 issues with this approach:
1) It leaves the broken DIY BTUSB_RESET_RESUME code in place for Realtek
devices.
2) Sofar only 2 of the 10 QCA devices known to the btusb code have been
added to usb/core/quirks.c and if we fix the Realtek case the same way
we need to add an additional 14 entries. So in essence we need to
duplicate a large part of the usb_device_id table in btusb.c in
usb/core/quirks.c and manually keep them in sync.
This commit instead restores setting a reset-resume quirk for QCA devices
in the btusb.c code, avoiding the duplicate usb_device_id table problem.
This commit avoids the problems with the original DIY BTUSB_RESET_RESUME
code by simply setting the USB_QUIRK_RESET_RESUME quirk directly on the
usb_device.
This commit also moves the BTUSB_REALTEK case over to directly setting the
USB_QUIRK_RESET_RESUME on the usb_device and removes the now unused
BTUSB_RESET_RESUME code.
BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1514836
Fixes: 7d06d5895c15 ("Revert "Bluetooth: btusb: fix QCA...suspend/resume"")
Cc: Leif Liddy
Cc: Matthias Kaehlcke
Cc: Brian Norris
Cc: Daniel Drake
Cc: Kai-Heng Feng
Signed-off-by: Hans de Goede
Signed-off-by: Marcel Holtmann
Signed-off-by: Greg Kroah-Hartman
---
drivers/bluetooth/btusb.c | 21 ++---
1 file changed, 10 insertions(+), 11 deletions(-)
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -23,6 +23,7 @@
#include
#include
+#include
#include
#include
@@ -360,8 +361,8 @@ static const struct usb_device_id blackl
#define BTUSB_FIRMWARE_LOADED 7
#define BTUSB_FIRMWARE_FAILED 8
#define BTUSB_BOOTING 9
-#define BTUSB_RESET_RESUME 10
-#define BTUSB_DIAG_RUNNING 11
+#define BTUSB_DIAG_RUNNING 10
+#define BTUSB_OOB_WAKE_ENABLED 11
struct btusb_data {
struct hci_dev *hdev;
@@ -2969,6 +2970,12 @@ static int btusb_probe(struct usb_interf
if (id->driver_info & BTUSB_QCA_ROME) {
data->setup_on_usb = btusb_setup_qca;
hdev->set_bdaddr = btusb_set_bdaddr_ath3012;
+
+ /* QCA Rome devices lose their updated firmware over suspend,
+* but the USB hub doesn't notice any status change.
+* explicitly request a device reset on resume.
+*/
+ interface_to_usbdev(intf)->quirks |= USB_QUIRK_RESET_RESUME;
}
#ifdef CONFIG_BT_HCIBTUSB_RTL
@@ -2979,7 +2986,7 @@ static int btusb_probe(struct usb_interf
* but the USB hub doesn't notice any status change.
* Explicitly request a device reset on resume.
*/
- set_bit(BTUSB_RESET_RESUME, &data->flags);
+ interface_to_usbdev(intf)->quirks |= USB_QUIRK_RESET_RESUME;
}
#endif
@@ -3136,14 +3143,6 @@ static int btusb_suspend(struct usb_inte
btusb_stop_traffic(data);
usb_kill_anchored_urbs(&data->tx_anchor);
- /* Optionally request a device reset on resume, but only when
-* wakeups are disabled. If wakeups are enabled we assume the
-* device will stay powered up throughout suspend.
-*/
- if (test_bit(BTUSB_RESET_RESUME, &data->flags) &&
- !device_may_wakeup(&data->udev->dev))
- data->udev->reset_resume = 1;
-
return 0;
}
