One other ask. Since we have fixed the free panic with the last commit now, I'm wondering whether tools like scanimage are smart enough to handle things like that the device returns a too short configuration descriptor temporary on wake up.
Could you please check if you still can scan only with the last commit we did? Maybe we don't even need the additional diff to workaround that problem ... On Thu, 27 Aug 2020 21:27:30 +0200 Marcus Glocker <[email protected]> wrote: > Hey Mikolaj, > > On Thu, Aug 27, 2020 at 06:49:12PM +0000, Mikolaj Kucharski wrote: > > > On Wed, Aug 26, 2020 at 11:27:21PM +0200, Marcus Glocker wrote: > > > On Wed, 26 Aug 2020 19:20:26 +0000 > > > Mikolaj Kucharski <[email protected]> wrote: > > > > > > > On Wed, Aug 26, 2020 at 06:54:59PM +0000, Mikolaj Kucharski > > > > wrote: > > > > > On Wed, Aug 26, 2020 at 06:45:28PM +0000, Mikolaj Kucharski > > > > > wrote: > > > > ... > > > > > > > > > > > > Do you have any theory why first control request is > > > > > > returning wTotalLength=32 and second is returning > > > > > > wTotalLength=55? It also seem that bNumInterface has > > > > > > different value between the runs. > > > > > > > > > > I ran more `scanimage -L` commands in a row and > > > > > wTotalLength=32 doesn't occur any more. As I'm remote to the > > > > > machine and the printer I don't see it, but I imagine that > > > > > printer/scanner goes to sleep after certain amount of time of > > > > > inactivity. > > > > > > > > > > Maybe it's related that first call wakes up the > > > > > printer/scanner and then wTotalLength=32. After that when > > > > > printer/scanner is up all wTotalLength=55. I can test it > > > > > again during a day tomorrow with hours of break in between of > > > > > `scanimage -L` runs to see am I seeing more of this behaviour. > > > > > > > > > > > > > Ok, so I waited 15+ minutes between scanimage -L, as I suspect > > > > that would be enough time to get printer to sleep and yeah, I > > > > see again wTotalLength=32: > > > > > > OK, this makes sense. > > > > > > Does this diff fix the issue? > > > I'm hoping that we don't require a delay between the requests. > > > > Yes, this diff does fix the issue. It works really well. I can > > confirm this is related to the printer going to sleep-mode as that > > printer has also web UI and there I can see status of the printer, > > for example page says: > > > > Sleeping... > > > > or, when `scanimage -L` wakes up the printer it says: > > > > Ready to Copy > > Eco Mode 01 > > > > and this relates to wTotalLength 32 vs 55. After below diff all > > works without panic and `scanimage -L` works reliably and scanning > > itself also works. > > All right. > > > I've also tested your diff against (kernel has disable ulpt > > applied): > > > > # dmesg > > ... > > ugen0 at uhub0 port 4 "Canon MG3200 series" rev 2.00/1.04 addr 2 > > > > # usbdevs -v > > Controller /dev/usb0: > > addr 01: 1022:0000 AMD, xHCI root hub > > super speed, self powered, config 1, rev 1.00 > > driver: uhub0 > > addr 02: 04a9:1762 Canon, MG3200 series > > high speed, self powered, config 1, rev 1.04, iSerial > > 860FE4 driver: ugen0 > > Controller /dev/usb1: > > addr 01: 1022:0000 AMD, EHCI root hub > > high speed, self powered, config 1, rev 1.00 > > driver: uhub1 > > addr 02: 0438:7900 Advanced Micro Devices, Hub > > high speed, self powered, config 1, rev 0.18 > > driver: uhub2 > > > > # scanimage -L > > device `pixma:04A91762_860FE4' is a CANON Canon PIXMA MG3200 Series > > multi-function peripheral > > > > That Canon MG3250 printer/scanner didn't had this issue, but I just > > wanted to check is your diff also working on a device, which was > > working before the change. All good here. > > > > Thank you very much Marcus for fixing the problem! > > Cool - Thanks for the thoroughly testing. > > As discussed with Theo before, we'll first fix the underlying issue of > the malloc(size)/free(,size) which is fundamentally handled wrong in > this case. > > I'll afterwards seek for another OK to get the diff in you've tested. > > > > > # grep -F '/bsd' /var/log/messages | cut -b35- | head -n30 > > > > /bsd: USB_DEVICE_GET_FDESC wTotalLength free = 25 > > > > /bsd: USB_DEVICE_GET_FDESC wTotalLength free = 55 > > > > /bsd: USB_DEVICE_GET_FDESC wTotalLength free = 25 > > > > /bsd: USB_DEVICE_GET_FDESC wTotalLength free = 25 > > > > /bsd: USB_DEVICE_GET_FDESC wTotalLength free = 216 > > > > /bsd: 1: > > > > /bsd: bLength=9 > > > > /bsd: bDescriptorType=2 > > > > /bsd: wTotalLength=32 > > > > /bsd: bNumInterface=1 > > > > /bsd: bConfigurationValue=1 > > > > /bsd: iConfiguration=0 > > > > /bsd: bmAttributes=0xc0 > > > > /bsd: bMaxPower=1 > > > > /bsd: 2: > > > > /bsd: bLength=9 > > > > /bsd: bDescriptorType=2 > > > > /bsd: wTotalLength=55 > > > > /bsd: bNumInterface=2 > > > > /bsd: bConfigurationValue=1 > > > > /bsd: iConfiguration=0 > > > > /bsd: bmAttributes=0xc0 > > > > /bsd: bMaxPower=1 > > > > /bsd: USB_DEVICE_GET_FDESC wTotalLength free = 55 > > > > /bsd: USB_DEVICE_GET_FDESC wTotalLength free = 55 > > > > /bsd: USB_DEVICE_GET_FDESC wTotalLength free = 25 > > > > /bsd: USB_DEVICE_GET_FDESC wTotalLength free = 25 > > > > /bsd: USB_DEVICE_GET_FDESC wTotalLength free = 216 > > > > /bsd: USB_DEVICE_GET_FDESC wTotalLength free = 216 > > > > /bsd: USB_DEVICE_GET_FDESC wTotalLength free = 55 > > > > > > > > > Index: sys/dev/usb/usb_subr.c > > > =================================================================== > > > RCS file: /cvs/src/sys/dev/usb/usb_subr.c,v > > > retrieving revision 1.151 > > > diff -u -p -u -p -r1.151 usb_subr.c > > > --- sys/dev/usb/usb_subr.c 31 Jul 2020 10:49:33 > > > -0000 1.151 +++ sys/dev/usb/usb_subr.c 26 Aug 2020 > > > 21:25:31 -0000 @@ -1366,6 +1366,7 @@ usbd_get_cdesc(struct > > > usbd_device *dev, usb_config_descriptor_t *cdesc, *tdesc, cdescr; > > > u_int len; > > > usbd_status err; > > > + int i; > > > > > > if (index == USB_CURRENT_CONFIG_INDEX) { > > > tdesc = usbd_get_config_descriptor(dev); > > > @@ -1378,11 +1379,23 @@ usbd_get_cdesc(struct usbd_device *dev, > > > memcpy(cdesc, tdesc, len); > > > DPRINTFN(5,("%s: current, len=%u\n", __func__, > > > len)); } else { > > > - err = usbd_get_desc(dev, UDESC_CONFIG, index, > > > - USB_CONFIG_DESCRIPTOR_SIZE, &cdescr); > > > - if (err || cdescr.bDescriptorType != > > > UDESC_CONFIG) > > > - return (0); > > > - len = UGETW(cdescr.wTotalLength); > > > + /* > > > + * Some devices return false configuration > > > descriptor values > > > + * on the first request, e.g. when they are in > > > sleep mode. > > > + * Therefore compare multiple request results; > > > Two for sane > > > + * values, three for initial false values. > > > + */ > > > + for (len = 0, i = 0; i < 3; i++) { > > > + err = usbd_get_desc(dev, UDESC_CONFIG, > > > index, > > > + USB_CONFIG_DESCRIPTOR_SIZE, &cdescr); > > > + if (err || cdescr.bDescriptorType != > > > UDESC_CONFIG) > > > + return (0); > > > + if (len == UGETW(cdescr.wTotalLength)) > > > + break; > > > + len = UGETW(cdescr.wTotalLength); > > > + } > > > + if (i == 3) > > > + return (NULL); > > > DPRINTFN(5,("%s: index=%d, len=%u\n", __func__, > > > index, len)); if (lenp) > > > *lenp = len; > > > > -- > > Regards, > > Mikolaj > -- [ Marcus Glocker, [email protected], http://nazgul.ch ]
