Status Update
No update yet.
Comments
sh...@google.com
So, the end result is that usb_write() sometimes sends a ZLP when it doesn't need to
should be
So, the end result is that usb_write() sometimes sends a ZLP when it doesn't need to, and sometimes neglects to send a ZLP when it should
en...@google.com
da...@sonos.com
good bug report, thanks! i'm assuming this isn't darwin-specific either --- it looks like we have similar logic using masks in the linux and windows backends and in the libusb backend too.
I'm not sure the switch to HandleError was a good idea, since it more or less swept the real problem under the rug.
yeah, i know what you mean, but it's also hard to argue with this logic in the commit message that made that change:
These CHECKs are expected to happen if the client does the wrong thing,
so we probably shouldn't be aborting in adbd.
a CHECK in the client (as you suggested earlier) would probably have been the best idea... postel's law and all that :-)
ju...@sonos.com
Indeed, I see similar logic in usb_write() in usb_linux. maxPacketSize is 512 (a power of 2) on my Ubuntu environment, so all good there as far as runtime behavior. It sure is odd that on macOS it's 5120, which happens to be exactly 10x the linux value. Seem suspicious, doesn't it?
Yeah, on the CHECK thing... I guess I see the client and server as a closed system in that the code is developed and distributed together. That said, obviously anyone can write their own adb client/server, so in that respect, I agree with the disagreement :-)
ju...@sonos.com
5120 is likely not really the maxPacketSize on our Macs. It's probably 512. And a burst is probably 10 packets.
Note that usb_osx.cpp calculates the maxPacketSize as follows
if (maxBurst != 0)
// bMaxBurst is the number of additional packets in the burst.
maxPacketSize /= (maxBurst + 1);
If the theory about packet size truly being 512 is correct, then it means the real bug is in GetPipePropertiesV2() returning zero for maxBurst instead of 9. If it returned 9, then maxPacketSize would be 512 (again, a power of 2), and the zero_mask expression would work out just fine. All situations where we've had our adbd abort because of the unexpected stowaway amessage header in a packet are ones where the sent buffer are an integer multiple of 512. None are an integer multiple of 5120.
Still there's no denying that the unchecked assumption of maxPacketSize being a power of 2 is a bug. Changing the code to use modulus operation instead of a bit-mask check would be technically more correct, but wouldn't fix the broken behavior in our case, since ultimately, it appears the maxPacketSize adb is getting is wrong. But if you had an assert in the client/server on macPacketSize being a power of 2, then that would at least have brought the incorrect maxPacketSize issue front and center.
sh...@google.com
da...@sonos.com
From GetEndpointPropertiesV3's documentation:
... The wMaxPacketSize is the BASE maxPacketSize as found in the endpoint descriptor, and has not been multiplied to take into account burst and mult...
Whereas GetPipePropertiesV3 documentation states that it does mention is may be a multiple
> .. current FULL maxPacketSize for this pipe, which includes both the mult and the burst...
So using GetEndpointProperties would avoid the need to compute the actual max packet length which goes wrong.
en...@google.com
use GetPipePropertiesV2 ( and not GetEndpointProperties)
the opposite way round, right? we should switch to GetEndpointProperties?
do you have a pointer to the docs? my search-fu failed me :-(
en...@google.com
جهازي مخترق
sh...@google.com
ne...@gmail.com
hu...@gmail.com
ja...@gmail.com
(i've added dvander to the fastboot cl, since he's the fastboot owner.)
magicleap folks: please take a look if you have time, and note that you can download prebuilt binaries from ci.android.com if you want to test but can't easily rebuild yourselves. these changes will go out in the next platform tools release, so let us know if they don't solve the issues you've been seeing!
Description
$ adb --version
Android Debug Bridge version 1.0.41
Version 34.0.4-10411341
Installed as /usr/local/android/sdk-linux_x86/platform-tools/adb
Running on Linux 6.2.0-33-generic (x86_64)
$ adb devices
List of devices attached
<scrubbed> device
$ adb reboot
$ adb devices
List of devices attached
$ adb kill-server
$ adb devices
* daemon not running; starting now at tcp:5037
* daemon started successfully
List of devices attached
<scrubbed> device
For what it's worth, this is impacting a few of my co-workers, whom all have similar hardware / build environments.
Any information / debugging suggestions would be greatly appreciated. Thanks in advance.