Re: [bug] uboot 2022.07 hangs on rpi 2 with attached usb storage

[Jan Palus <jpalus@fastmail.com>]

On 25.07.2022 11:25, AKASHI Takahiro wrote:
> On Sat, Jul 23, 2022 at 05:03:39PM +0200, Jan Palus wrote:
> > On 23.07.2022 16:43, Jan Palus wrote:
> > > On 23.07.2022 16:19, Jan Palus wrote:
> > > > On 22.07.2022 02:59, Simon Glass wrote:
> > > > > Hi Jan,
> > > > > 
> > > > > On Mon, 18 Jul 2022 at 11:48, Jan Palus <jpalus@fastmail.com> wrote:
> > > > > >
> > > > > > u-boot 2022.07 boots fine without any USB devices attached to
> > > > > > RaspberryPi 2 however it hangs early on if external USB drive is
> > > > > > connected, right after:
> > > > > >
> > > > > >    Request Sense returned 02 04 01
> > > > > >
> > > > > > git bisect indicates first commit to cause regression is:
> > > > > >
> > > > > >   8c9812a5d557c4eacf164147d7380b3af1b222ec is the first bad commit
> > > > > >   commit 8c9812a5d557c4eacf164147d7380b3af1b222ec
> > > > > >   Author: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > > > > >   Date:   Tue Mar 8 20:36:40 2022 +0900
> > > > > >
> > > > > >       usb: storage: call device_probe() after scanning
> > > > > >
> > > > > >       Every time a usb bus/port is scanned and a new device is detected,
> > > > > >       we want to call device_probe() as it will give us a chance to run
> > > > > >       additional post-processings for some purposes.
> > > > > >
> > > > > >       In particular, support for creating partitions on a device will be added.
> > > > > >
> > > > > >       Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > > > > >       Reviewed-by: Simon Glass <sjg@chromium.org>
> > > > > >
> > > > > > Reverting this commit fixes the issue.
> > > > > >
> > > > > > Note that USB drive is TOSHIBA MQ04UBD200 and it's not used for booting.
> > > > > > Also note that without this change 0 storage devices are detected even
> > > > > > when drive is attached.
> > > > > 
> > > > > I am not sure what is going on here. Can you provide the full console
> > > > > trace of the boot? Any idea where it is hanging?
> > > > 
> > > ...
> > > > 
> > > > Now the place where it hangs is:
> > > > 
> > > >   part_efi.c:
> > > > 
> > > >   static int part_test_efi(struct blk_desc *dev_desc)
> > > >   {
> > > >           ALLOC_CACHE_ALIGN_BUFFER_PAD(legacy_mbr, legacymbr, 1, dev_desc->blksz);
> > > > 
> > > > where dev_desc->blksz is 3782209548.
> > > > 
> > > 
> > > So it appears block size is read incorrectly? Should be 512 but not sure
> > > where this value 3782209548 is coming from, it's not block capacity
> > > either. After plugging in Linux reports:
> > > 
> > >   sd 0:0:0:0: [sda] 3907029168 512-byte logical blocks: (2.00 TB/1.82 TiB)
> > 
> > Since this was logged:
> > 
> >   Device NOT ready
> >      Request Sense returned 02 04 01
> > 
> > then it seems capacity/block size were never determined (happens right
> > after these log messages if they *don't* occur) so I guess they are
> > random values and this usb storage should never have been probed? I'll
> > stop here due to my cluelessness.
> 
> The code looks like:
>   usb_stor_probe_device()
>       ...
>       blk_create_devicef();
>       ret = usb_stor_get_info(udev, data, blkdev);
>       if (ret == 1) {
>           usb_max_devs++;
>           debug("%s: Found device %p\n", __func__, udev);
>       } else {
>           debug("usb_stor_get_info: Invalid device\n");
>           ret = device_unbind(dev);
>           if (ret)
>               return ret;
> 						<== (A)
>       }
> 
>       blk_probe_or_unbind(dev);
>       ...
> 
> 
> usb_stor_get_info() returns 0 when it generates "Device NOT ready" message.
> Then blk_probe_or_unbind(), hence part_test_efi(), is accidentally called
> although blkdev is not fully initialised/populated.
> 
> I think we should skip the subsequent processing by adding "continue" at (A).

I see there is a change proposed in ml in the same place:

https://lists.denx.de/pipermail/u-boot/2022-July/489531.html

Though I'm also wondering "Request Sense returned 02 04 01" from a quick
search appears to indeed mean "Device NOT ready", but "it's becoming
ready" so in my case that's likely about spinning up HDD plates. I
wonder if it shouldn't be treated like -EAGAIN with some timeout as in
second or two the drive would likely report it is ready.