Return-Path: MIME-Version: 1.0 Sender: linus971@gmail.com In-Reply-To: <38429B86-0E46-4C6A-9954-3260D864B10C@holtmann.org> References: <300602D8-9DB7-4F8D-AC97-B60924554093@holtmann.org> <38429B86-0E46-4C6A-9954-3260D864B10C@holtmann.org> Date: Fri, 17 Apr 2015 16:18:20 -0400 Message-ID: Subject: Re: [V4.1] Regression: Bluetooth mouse not working. From: Linus Torvalds To: Marcel Holtmann Cc: =?UTF-8?Q?J=C3=B6rg_Otte?= , "Gustavo F. Padovan" , Johan Hedberg , "bluez mailin list (linux-bluetooth@vger.kernel.org)" , Linux Kernel Mailing List Content-Type: text/plain; charset=UTF-8 List-ID: On Fri, Apr 17, 2015 at 1:02 PM, Marcel Holtmann wrote: > > okay. I only looked at BlueZ 5.x and that might have been my mistake. Let me check this and fix this properly. Why not just revert that commit. It looks like garbage. It has odd code like + u32 valid_flags = 0; + ci->flags = session->flags & valid_flags; which is basically saying "no flags are valid, and we are silently just clearing them all when copying". The reason I think it's garbage is (a) the commit clearly breaks something, so the whole "let's check flags that we've never checked before" is already fundamentally suspicious (b) code like the above is just crap to begin with, because it makes things superficially "look" sensible when looking at individual lines of code (for example, when grepping things), and then when you look at the actual bigger picture, it turns out that the code doesn't actually care about the flags it is "copying", it just clears them all. The other code sequences do things like + u32 valid_flags = 0; + if (req->flags & ~valid_flags) + return -EINVAL; Which again is just a very unreadable way of saying "if any flags are set, return an error". This kind of thing is presumably what breaks things, because clearly people *have* set flags that you thought are invalid. Now *IF* the interfaces had had these kinds of flag validation checks from day one, that would be one thing. But adding these kinds of things after the fact, when somebody then reports that they break things, then that's just a big big flag that you shouldn't try to do this at all. It's water under the bridge. That ship has sailed. It's too late. Give up on it. So I don't think this code is "fixable". It really smells like a fundamental mistake to begin with. Just revert it, chalk it up as "ok, that was a stupid idea", and move on. Linus