On Fri, Jun 19, 2009 at 6:52 PM, Pavel Roskin
<address@hidden> wrote:
OK, I understand you tried USB mass storage devices.
I believe the paramount here is consistency. There are several places
in the code where grub_errno is returned. In one place, grub_error() is
returned. It's important to fix all places at once.
Also, please check other .open functions in other disk drivers. In
disk/fs_uuid.c, grub_error() is used. The same is in disk/host.c.
I see the standard is grub_error(). Let's do it for SCSI as well.
I don't understand what do you mean. grub_error () which don't come from previous function
Something is wrong with the logic in that function. retrycnt is only
changed in one place, and if it hits zero, we don't go to the retry
label. I think the condition can be removed. I don't see how your
change could have fixed anything in the behavior.
Wel it didn't fixed the logic completely just one case when it was wrong. Sorry that patch was low-quality. My goal was to enable everything by default and the bugs in long-unmaintained libusb code weren't something I wanted to spend time on.
> @@ -246,6 +246,7 @@ grub_usbms_transfer (struct grub_scsi *scsi,
> grub_size_t cmdsize, char *cmd,
> if (err == GRUB_USB_ERR_STALL)
> {
> grub_usb_clear_halt (dev->dev, dev->out->endp_addr);
> + retrycnt--;
> goto retry;
> }
> return grub_error (GRUB_ERR_IO, "USB Mass Storage request
> failed");;
> retrycnt wasn't decreased which caused grub2 to retry infinitely hence
> a hang.
There are many instances of "goto retry" where you don't decrement
retrycnt. Then let's decrement retrycnt in the beginning.
Generally, when making a change, please have a look if it needs to be
done elsewhere.
> --- a/util/usb.c
> +++ b/util/usb.c
> @@ -51,6 +51,7 @@ grub_libusb_devices (void)
> for (usbdev = bus->devices; usbdev; usbdev = usbdev->next)
> {
> struct usb_device_descriptor *desc = &usbdev->descriptor;
> + grub_err_t err;
>
> if (! desc->bcdUSB)
> continue;
> @@ -62,7 +63,9 @@ grub_libusb_devices (void)
> dev->data = "">
>
> /* Fill in all descriptors. */
> - grub_usb_device_initialize (dev);
> + err = grub_usb_device_initialize (dev);
> + if (err)
> + continue;
It was clearing grub_errno
> Regarding the compile warning fix, I would try to make
> grub_libusb_init() and grub_libusb_fini() appear in
> grub_emu_init.h
> rather than declare them elsewhere.
> I was inspired by previous example of disk subsystems:
> #ifdef GRUB_UTIL
> void grub_raid_init (void);
> void grub_raid_fini (void);
> void grub_lvm_init (void);
> void grub_lvm_fini (void);
> #endif
> file: include/grub/disk.h
I would check why it was needed.
It seems it's unnecessary. I removed them and it didn't generate any warnings. Now I followed your recommendation and they build system with my previous fixes picked it right
--
usbfix.diff