On Fri, Jun 19, 2009 at 6:52 PM, Pavel Roskin <pro...@gnu.org> 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 = usbdev;
> >
> > /* 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
>
> --
> Regards,
> Pavel Roskin
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>
--
Regards
Vladimir 'phcoder' Serbinenko
Personal git repository: http://repo.or.cz/w/grub2/phcoder.git
diff --git a/ChangeLog b/ChangeLog
index 4185723..69c8ae1 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,22 +1,22 @@
2009-06-17 Vladimir Serbinenko <phco...@gmail.com>
- Enable all targets that can be built by default
+ Fix libusb
- * configure.c: enable efiemu runtime, grub-emu, grub-emu-usb,
- grub-mkfont and grub-fstest if they can be built
+ * Makefile.in (LIBUSB): new macro
+ * genmk.rb (Utility/print_tail): new method
+ (Utility/rule): use intermediary variable #{prefix}_OBJECTS
+ (top level): call util.print_tail at the end.
2009-06-19 Vladimir Serbinenko <phco...@gmail.com>
* disk/scsi.c (grub_scsi_open): use continue instead of big if
-2009-06-17 Vladimir Serbinenko <phco...@gmail.com>
+2009-06-19 Vladimir Serbinenko <phco...@gmail.com>
- Fix libusb
+ Enable all targets that can be built by default
- * Makefile.in (LIBUSB): new macro
- * genmk.rb (Utility/print_tail): new method
- (Utility/rule): use intermediary variable #{prefix}_OBJECTS
- (top level): call util.print_tail at the end.
+ * configure.c: enable efiemu runtime, grub-emu, grub-emu-usb,
+ grub-mkfont and grub-fstest if they can be built
2009-06-18 Pavel Roskin <pro...@gnu.org>
diff --git a/disk/scsi.c b/disk/scsi.c
index 353e639..24ebdb6 100644
--- a/disk/scsi.c
+++ b/disk/scsi.c
@@ -248,6 +248,7 @@ grub_scsi_open (const char *name, grub_disk_t disk)
{
if (p->open (name, scsi))
continue;
+
disk->id = (unsigned long) "scsi"; /* XXX */
disk->data = scsi;
scsi->dev = p;
@@ -266,7 +267,7 @@ grub_scsi_open (const char *name, grub_disk_t disk)
{
grub_free (scsi);
grub_dprintf ("scsi", "inquiry failed\n");
- return grub_errno;
+ return err;
}
grub_dprintf ("scsi", "inquiry: devtype=0x%02x removable=%d\n",
@@ -292,7 +293,7 @@ grub_scsi_open (const char *name, grub_disk_t disk)
{
grub_free (scsi);
grub_dprintf ("scsi", "READ CAPACITY failed\n");
- return grub_errno;
+ return err;
}
/* SCSI blocks can be something else than 512, although GRUB
diff --git a/disk/usbms.c b/disk/usbms.c
index 3c7ebaf..403ed19 100644
--- a/disk/usbms.c
+++ b/disk/usbms.c
@@ -222,11 +222,12 @@ grub_usbms_transfer (struct grub_scsi *scsi, grub_size_t cmdsize, char *cmd,
struct grub_usbms_csw status;
static grub_uint32_t tag = 0;
grub_usb_err_t err = GRUB_USB_ERR_NONE;
- int retrycnt = 3;
+ int retrycnt = 3 + 1;
retry:
+ retrycnt--;
if (retrycnt == 0)
- return err;
+ return grub_error (GRUB_ERR_IO, "USB Mass Storage stalled");
/* Setup the request. */
grub_memset (&cbw, 0, sizeof (cbw));
@@ -305,9 +306,7 @@ grub_usbms_transfer (struct grub_scsi *scsi, grub_size_t cmdsize, char *cmd,
grub_usb_clear_halt (dev->dev, dev->in->endp_addr);
grub_usb_clear_halt (dev->dev, dev->out->endp_addr);
- retrycnt--;
- if (retrycnt)
- goto retry;
+ goto retry;
}
if (status.status)
diff --git a/include/grub/disk.h b/include/grub/disk.h
index a47e113..8b56448 100644
--- a/include/grub/disk.h
+++ b/include/grub/disk.h
@@ -173,11 +173,4 @@ struct grub_disk_ata_pass_through_parms
extern grub_err_t (* EXPORT_VAR(grub_disk_ata_pass_through)) (grub_disk_t,
struct grub_disk_ata_pass_through_parms *);
-#ifdef GRUB_UTIL
-void grub_raid_init (void);
-void grub_raid_fini (void);
-void grub_lvm_init (void);
-void grub_lvm_fini (void);
-#endif
-
#endif /* ! GRUB_DISK_HEADER */
diff --git a/util/grub-emu.c b/util/grub-emu.c
index c133dbe..badec18 100644
--- a/util/grub-emu.c
+++ b/util/grub-emu.c
@@ -192,10 +192,6 @@ main (int argc, char *argv[])
/* XXX: This is a bit unportable. */
grub_util_biosdisk_init (dev_map);
-#if HAVE_USB_H
- grub_libusb_init ();
-#endif
-
grub_init_all ();
/* Make sure that there is a root device. */
diff --git a/util/usb.c b/util/usb.c
index e1d8c71..a687eea 100644
--- 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,12 @@ grub_libusb_devices (void)
dev->data = usbdev;
/* Fill in all descriptors. */
- grub_usb_device_initialize (dev);
+ err = grub_usb_device_initialize (dev);
+ if (err)
+ {
+ grub_errno = GRUB_ERR_NONE;
+ continue;
+ }
/* Register the device. */
grub_usb_devs[last++] = dev;
@@ -72,27 +78,6 @@ grub_libusb_devices (void)
return GRUB_USB_ERR_NONE;
}
-grub_err_t
-grub_libusb_init (void)
-{
- usb_init();
- usb_find_busses();
- usb_find_devices();
-
- if (grub_libusb_devices ())
- return grub_errno;
-
- grub_usb_controller_dev_register (&usb_controller);
-
- return 0;
-}
-
-grub_err_t
-grub_libusb_fini (void)
-{
- return 0;
-}
-
int
grub_usb_iterate (int (*hook) (grub_usb_device_t dev))
@@ -189,3 +174,22 @@ grub_usb_bulk_write (grub_usb_device_t dev,
usb_close (devh);
return GRUB_USB_ERR_STALL;
}
+
+GRUB_MOD_INIT (libusb)
+{
+ usb_init();
+ usb_find_busses();
+ usb_find_devices();
+
+ if (grub_libusb_devices ())
+ return;
+
+ grub_usb_controller_dev_register (&usb_controller);
+
+ return;
+}
+
+GRUB_MOD_FINI (libusb)
+{
+ return;
+}
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel
