[ECOS] Bugs in the at91 USB driver

[ECOS] Bugs in the at91 USB driver

[Frank Pagliughi]

Hi,

I was reading through the AT91 USB driver code and spotted two bugs, but 
am unsure what to do with them.

1. The "usbs_at91_control_setup_get_status()" function needs to queue up 
a value to send back to the host, but the function sets the endpoint 
buffer variables (pbegin, etc) to the address of a local variable, 
'word', which disappears when the function returns (before the buffer is 
shipped to the host).

I assume the return value can be placed into ep0's "control_buffer" for 
return to the host. Or a tx buffer can be declared and used for such 
purposes. Which would be better?


2. The "usbs_state_notify()" function sends the wrong value to the state 
change callback function. Instead of sending a value of a 
"usbs_state_change" enumeration (like USBS_STATE_CHANGE_RESET)  for the 
third parameter, it sends the new state (like USBS_STATE_POWERED).

On one hand, this needs to be fixed so that class drivers and examples 
can work with the AT91 driver, but on the other hand, fixing it could 
break existing applications that were written to use the incorrect 
values being emitted by the driver. It's especially confusing because 
the two enumerations are very close and the "state change" values are an 
eCos invention and not that well defined.

What to do?

Frank Pagliughi

-- 
Before posting, please read the FAQ: http://ecos.sourceware.org/fom/ecos
and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss

Re: [ECOS] Bugs in the at91 USB driver

[Andrew Lunn]

On Thu, May 08, 2008 at 01:30:42AM -0400, Frank Pagliughi wrote:
> Hi,
>
> I was reading through the AT91 USB driver code and spotted two bugs, but  
> am unsure what to do with them.
>
> 1. The "usbs_at91_control_setup_get_status()" function needs to queue up  
> a value to send back to the host, but the function sets the endpoint  
> buffer variables (pbegin, etc) to the address of a local variable,  
> 'word', which disappears when the function returns (before the buffer is  
> shipped to the host).
>
> I assume the return value can be placed into ep0's "control_buffer" for  
> return to the host. Or a tx buffer can be declared and used for such  
> purposes. Which would be better?

Or just make word static so it is not a stack variable.

> 2. The "usbs_state_notify()" function sends the wrong value to the state  
> change callback function. Instead of sending a value of a  
> "usbs_state_change" enumeration (like USBS_STATE_CHANGE_RESET)  for the  
> third parameter, it sends the new state (like USBS_STATE_POWERED).
>
> On one hand, this needs to be fixed so that class drivers and examples  
> can work with the AT91 driver, but on the other hand, fixing it could  
> break existing applications that were written to use the incorrect  
> values being emitted by the driver.

Like you said, this needs to be fixed, it is wrong. It is actually not
too bad. If you compare usbs_state_change and USBS_STATE_*, most are
the same. So in many cases i think it will just work.

> It's especially confusing because the two enumerations are very
> close and the "state change" values are an eCos invention and not
> that well defined.

It is a shame the compiler did not emit a warning here. If the
USBS_STATE_* had been an enumeration, i think it would of done....

Please submit a patch.

       Andrew

-- 
Before posting, please read the FAQ: http://ecos.sourceware.org/fom/ecos
and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss