Re: [Patch] [bug #26237] multiple problems with usb devices

[Aleš Nesrsta]

Vladimir 'φ-coder/phcoder' Serbinenko wrote:
> Aleš Nesrsta wrote:
> > Hello,
> >
> > according to e-mail from Vladimir Serbinenko I am sending patch related
> > to (mostly) OHCI problem (bug #26237).
> > Workaround related to grub_memalign problem is removed from patch
> > because Vladimir Serbinenko wrotes that it will be corrected.
> >   
> First of all big thanks for debugging this important aspect. And sorry
> for being busy lately.
> 
> -  GRUB_OHCI_REG_RHUBA = 18,
> -  GRUB_OHCI_REG_RHUBPORT = 21
> +  GRUB_OHCI_REG_FRAME_REMAINING,
> +  GRUB_OHCI_REG_FRAME_NUMBER,
> +  GRUB_OHCI_REG_PERIODIC_START,
> +  GRUB_OHCI_REG_LS_TRESHOLD,
> +  GRUB_OHCI_REG_RHUB_DESC_A,
> 
> Please avoid register renaming unless original ones are misnomers. If
> they are I would prefer renaming to be a separate patch.

No, problem. It is the rest of lot of my experiments. I have added names
for some previously missing registers and I also renamed something to be
more closed to OHCI specification and understand it, but later I decided
that I don't need additional registers and I did not cleanup.

> 
> -  grub_ohci_td_t td_list;
> +  grub_ohci_td_t td_list ;
> This change is spurious

Hmmm, it is also rest of some experiments which I was not properly
cleaned up. I made more changes on this place but I deleted them
later...
As I wrote in previous e-mail, there can be more such curious changes...


> 
> +   * So we should:
> +   * - allow WritebackDoneHead interrupt (WDH) by proper setting of last TD
> +   *   token - it is done above in transaction settings
> +   * - detect setting of WDH bit in HcInterruptStatus register
> +   * - compare HccaDoneHead value with address of last-1 TD. If it is not
> +   *   equal, check ED for halt and if not so, reset WDH bit and wait again
> +   *   - but it should not happen - debug it!
> Are the comments from you or any part copied from spec. We need no copy from 
> spec as spec is available anyway and sentences copied from it may cause 
> copyright problems

It is not copy from specification, it is only inspired by OHCI
specification. I wrote it mainly for myself (and possibly others) to
explain what is the main change against previous algorithm, which is not
safe (from my point of view). It can be removed if it is not
interesting.

"Copied" are "official" names of OHCI registers, bits etc., i.e.
HccaDoneHead, WDH bit, HcInterruptStatus etc. - but from my point of
view it could be no problem - it is "keyword" which is necessary to
"speak with the same language". And it should be aim of such
specifications, specially of such "free" specifications as USB
specifications are. Or not?

I think it is far of any OHCI specification texts - but I have no
experiences with code licensing...


> +/* Self commenting... */
> Such comments should be removed, but an empty line at this point would be nice

OK

> +  err_sense = grub_scsi_request_sense (scsi);
> +  /* err_sense is ignored for now and Request Sense Data also... */
> When you ignore an error you need to clean grub_errno.
> Sth like
> if (!err)
>   grub_errno = GRUB_ERR_NONE;
>      grub_scsi_cmd_inquiry = 0x12,
> +    grub_scsi_cmd_test_unit_ready = 0x00,
> 

I will check it.


> Please keep this list sorted.
> -#define GRUB_USB_FEATURE_ENDP_HALT   0x01
> -#define GRUB_USB_FEATURE_DEV_REMOTE_WU       0x02
> -#define GRUB_USB_FEATURE_TEST_MODE   0x04
> +#define GRUB_USB_FEATURE_ENDP_HALT   0x00
> +#define GRUB_USB_FEATURE_DEV_REMOTE_WU       0x01
> Why do you need to change these constants? Were they wrong?
> Tomorrow I'll test your patch on both UHCI and OHCI.

Yes, I found they are wrong. But I cannot remember the source of such
information (maybe it was USB Mass Storage Class Compliance Test
Specification but I am not sure).


> 
> +#define GRUB_USB_FEATURE_TEST_MODE   0x02
>      grub_scsi_cmd_read_capacity = 0x25,
> +
> 
> > Patch was done by:
> >
> > diff -urB  grub2-1.98~experimental.20100120 grub2-1.98~my_patched.20100312 
> > > grub2-1.98~my_patched.20100312.patch
> >
> > As I shortly read the patch file, there sometimes remains some few
> > things which can be optimized - sorry, it is mostly the rest from
> > debugging experiments (even I make cleaning of code today)...
> >
> > Ales
> >
> >   
> >> Od: Vladimir Serbinenko <address@hidden>
> >> Komu: Oliver Henshaw <address@hidden>, Vladimir Serbinenko
> >> <address@hidden>, Ales Nesrsta <address@hidden>, address@hidden
> >> Předmět: [bug #26237] multiple problems with usb devices
> >> Datum: Sat, 13 Mar 2010 22:08:44 +0000
> >>
> >> Update of bug #26237 (project grub):
> >>
> >>                  Release:                    None => Bazaar - trunk        
> >>  
> >>
> >>     _______________________________________________________
> >>
> >> Follow-up Comment #6:
> >>
> >> Hello. I found out the problem with grub_memalign. I'll apply the patch
> >> tomorrow. Some of the fixes can go directly in while other require
> >> investigation and perhaps even copyright assignment. Can you remove all
> >> gratuituous changes (like commenting out dprintfs, adding commented out 
> >> code
> >> and change register names) and send patch in unified format (-u option in
> >> diff) to address@hidden Porting code from linux is appropriate only if
> >> license is compatible and porting was approved by maintainer. Are perhaps
> >> interested in coding EHCI?
> >>
> >>     _______________________________________________________
> >>
> >> Reply to this item at:
> >>
> >>   <http://savannah.gnu.org/bugs/?26237>
> >>
> >> _______________________________________________
> >>   Message sent via/by Savannah
> >>   http://savannah.gnu.org/
> >>
> >>
> >>     
> >> ------------------------------------------------------------------------
> >>
> >> _______________________________________________
> >> Grub-devel mailing list
> >> address@hidden
> >> http://lists.gnu.org/mailman/listinfo/grub-devel
> 
> 
> _______________________________________________
> Grub-devel mailing list
> address@hidden
> http://lists.gnu.org/mailman/listinfo/grub-devel

Some questions and others:

I am not so far oriented in such kind of development, so I am asking:
Will You correct my patch yourself (as maintainer of code) or should I
prepare new corrected code and related patch? And against which version
- I noted that in meantime new official version was released...

Did You tests on OHCI/UHCI and what was the result?
What HW are You using for testing - some real PC or qemu ?

It was little bit surprise for me that OHCI and UHCI drivers are not
using IRQ (and maybe also some other drivers). Is it intention of GRUB?
Nothing against such philosophy, some things are easier in this way.

I shortly read the UHCI specification and I was thought about my
comments of TD buffer size optimalization included in patch - such
optimization cannot be done simply because TD preparation code is common
for OHCI and UHCI and:
- UHCI needs (as far as I understand) TD buffer size length up to packet
size only (<=64 bytes).
- In contradiction, OHCI allows up to 8Kbytes TD buffer size (if buffer
is aligned to page).
So there is the question if it make sense to do any optimization for
OHCI - probably not. In fact slow data transfer and wasting of memory
caused by not optimized TD buffer is probably not so big problem.

I read also part of EHCI specification. There is question if GRUB 2
needs support of EHCI because:
1. According to EHCI specification, each EHCI interface should normaly
have also OHCI (or UHCI ?) interface. So, any USB 2.0 interface should
work (slowly) via GRUB2 UHCI/OHCI support.
2. EHCI is relative new HW - so it was usually used on PCs with BIOS
which supports booting from USB - so, probably, there is not necessary
to prepare special support in GRUB for such HW. But maybe I am wrong.

Best regards
Ales