bug in usbback

bug in usbback

[James Harper]

I think there is a bug in pvusb when the DomU does not respond to the
XenbusStateReconfiguring state change and is then rebooted. This caused
the following Dom0 (after a long freeze):

BUG: soft lockup detected on CPU#0!

Call Trace:
 <IRQ> [<ffffffff802a938c>] softlockup_tick+0xe1/0xef
 [<ffffffff80268f82>] timer_interrupt+0x408/0x466
 [<ffffffff8020fb41>] handle_IRQ_event+0x4d/0x95
 [<ffffffff802a9870>] __do_IRQ+0xb4/0x127
 [<ffffffff8026729c>] do_IRQ+0x44/0x4d
 [<ffffffff80375f6b>] evtchn_do_upcall+0x1b3/0x26e
 [<ffffffff8025c8d2>] do_hypervisor_callback+0x1e/0x2c
 <EOI> [<ffffffff88355b7e>] :usbbk:usbbk_schedule+0x1e0/0xa3f
 [<ffffffff8029482f>] autoremove_wake_function+0x0/0x2e
 [<ffffffff802947ba>] keventd_create_kthread+0x0/0x62
 [<ffffffff8835599e>] :usbbk:usbbk_schedule+0x0/0xa3f
 [<ffffffff802947ba>] keventd_create_kthread+0x0/0x62
 [<ffffffff802325e4>] kthread+0xd4/0x108
 [<ffffffff8025cb24>] child_rip+0xa/0x12
 [<ffffffff802947ba>] keventd_create_kthread+0x0/0x62
 [<ffffffff8025c827>] error_exit+0x0/0x71
 [<ffffffff80232510>] kthread+0x0/0x108
 [<ffffffff8025cb1a>] child_rip+0x0/0x12
 
FYI, I am trying to get pvusb working under GPLPV and I had never seen
the Reconfiguring/Reconfigured states before and so I wasn't doing
anything with them...

James

Re: bug in usbback

[Keir Fraser]

Cc'ing the author...

 K.

On 25/07/2009 11:01, "James Harper" <james.harper@bendigoit.com.au> wrote:

> I think there is a bug in pvusb when the DomU does not respond to the
> XenbusStateReconfiguring state change and is then rebooted. This caused
> the following Dom0 (after a long freeze):
> 
> BUG: soft lockup detected on CPU#0!
> 
> Call Trace:
>  <IRQ> [<ffffffff802a938c>] softlockup_tick+0xe1/0xef
>  [<ffffffff80268f82>] timer_interrupt+0x408/0x466
>  [<ffffffff8020fb41>] handle_IRQ_event+0x4d/0x95
>  [<ffffffff802a9870>] __do_IRQ+0xb4/0x127
>  [<ffffffff8026729c>] do_IRQ+0x44/0x4d
>  [<ffffffff80375f6b>] evtchn_do_upcall+0x1b3/0x26e
>  [<ffffffff8025c8d2>] do_hypervisor_callback+0x1e/0x2c
>  <EOI> [<ffffffff88355b7e>] :usbbk:usbbk_schedule+0x1e0/0xa3f
>  [<ffffffff8029482f>] autoremove_wake_function+0x0/0x2e
>  [<ffffffff802947ba>] keventd_create_kthread+0x0/0x62
>  [<ffffffff8835599e>] :usbbk:usbbk_schedule+0x0/0xa3f
>  [<ffffffff802947ba>] keventd_create_kthread+0x0/0x62
>  [<ffffffff802325e4>] kthread+0xd4/0x108
>  [<ffffffff8025cb24>] child_rip+0xa/0x12
>  [<ffffffff802947ba>] keventd_create_kthread+0x0/0x62
>  [<ffffffff8025c827>] error_exit+0x0/0x71
>  [<ffffffff80232510>] kthread+0x0/0x108
>  [<ffffffff8025cb1a>] child_rip+0x0/0x12
>  
> FYI, I am trying to get pvusb working under GPLPV and I had never seen
> the Reconfiguring/Reconfigured states before and so I wasn't doing
> anything with them...
> 
> James
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

RE: bug in usbback

[James Harper]

Also, a question on behaviour:

When we receive a reconfigure event, I assume that we should then read
'num-ports' and scan through each of the port-n values in xenstore. Is
there any guarantee that a device hasn't been unplugged and replugged
from the same port during the reconfiguration window? How should I
detect such a thing if the value of port-n hasn't changed? Should I then
probe each device? I'm doing this under Windows which is a bit more
picky and much less flexible...

Thanks

James

> -----Original Message-----
> From: Keir Fraser [mailto:keir.fraser@eu.citrix.com]
> Sent: Saturday, 25 July 2009 20:13
> To: James Harper; xen-devel@lists.xensource.com
> Cc: n_iwamatsu@jp.fujitsu.com
> Subject: Re: [Xen-devel] bug in usbback
> 
> Cc'ing the author...
> 
>  K.
> 
> On 25/07/2009 11:01, "James Harper" <james.harper@bendigoit.com.au>
wrote:
> 
> > I think there is a bug in pvusb when the DomU does not respond to
the
> > XenbusStateReconfiguring state change and is then rebooted. This
caused
> > the following Dom0 (after a long freeze):
> >
> > BUG: soft lockup detected on CPU#0!
> >
> > Call Trace:
> >  <IRQ> [<ffffffff802a938c>] softlockup_tick+0xe1/0xef
> >  [<ffffffff80268f82>] timer_interrupt+0x408/0x466
> >  [<ffffffff8020fb41>] handle_IRQ_event+0x4d/0x95
> >  [<ffffffff802a9870>] __do_IRQ+0xb4/0x127
> >  [<ffffffff8026729c>] do_IRQ+0x44/0x4d
> >  [<ffffffff80375f6b>] evtchn_do_upcall+0x1b3/0x26e
> >  [<ffffffff8025c8d2>] do_hypervisor_callback+0x1e/0x2c
> >  <EOI> [<ffffffff88355b7e>] :usbbk:usbbk_schedule+0x1e0/0xa3f
> >  [<ffffffff8029482f>] autoremove_wake_function+0x0/0x2e
> >  [<ffffffff802947ba>] keventd_create_kthread+0x0/0x62
> >  [<ffffffff8835599e>] :usbbk:usbbk_schedule+0x0/0xa3f
> >  [<ffffffff802947ba>] keventd_create_kthread+0x0/0x62
> >  [<ffffffff802325e4>] kthread+0xd4/0x108
> >  [<ffffffff8025cb24>] child_rip+0xa/0x12
> >  [<ffffffff802947ba>] keventd_create_kthread+0x0/0x62
> >  [<ffffffff8025c827>] error_exit+0x0/0x71
> >  [<ffffffff80232510>] kthread+0x0/0x108
> >  [<ffffffff8025cb1a>] child_rip+0x0/0x12
> >
> > FYI, I am trying to get pvusb working under GPLPV and I had never
seen
> > the Reconfiguring/Reconfigured states before and so I wasn't doing
> > anything with them...
> >
> > James
> >
> >
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xensource.com
> > http://lists.xensource.com/xen-devel
> 

Re: bug in usbback

[Noboru Iwamatsu]

Hi James,

Reconfiguring/Resconfigured states are used to notify the hotplug
event, as follows.


usb device    usbback                     usbfront
---------     -------                     --------

             Connected                    Connected
                  |                           |
---plug&probe--->|                           |
                  |                           |
            Reconfiguring-------------------->|
                  |                           |
                  | <---------------------Reconfiguring
                  |                           |
                  |                           |
          usbback_do_hotplug()                |
                  |                           |
                  |                           |
             Reconfigured-------------------->|
                  |                           |
                  |                           |
                  |                  usbfront_do_hotplug()
                  |                           |
                  |                           |
                  |<----------------------Connected
                  |                           |
              Connected                       |
                  |                           |


You mean:
- Your version of usbfront NOT respond to Reconfiguring state.
   It's not bug of original usbfront.
- When usbback is in Reconfiguring state and usbfront NOT respond to
   that, rebooting domU causes the BUG.

Is my understanding right?

Regards,
Noboru

James Harper wrote:
> I think there is a bug in pvusb when the DomU does not respond to the
> XenbusStateReconfiguring state change and is then rebooted. This caused
> the following Dom0 (after a long freeze):
> 
> BUG: soft lockup detected on CPU#0!
> 
> Call Trace:
>  <IRQ> [<ffffffff802a938c>] softlockup_tick+0xe1/0xef
>  [<ffffffff80268f82>] timer_interrupt+0x408/0x466
>  [<ffffffff8020fb41>] handle_IRQ_event+0x4d/0x95
>  [<ffffffff802a9870>] __do_IRQ+0xb4/0x127
>  [<ffffffff8026729c>] do_IRQ+0x44/0x4d
>  [<ffffffff80375f6b>] evtchn_do_upcall+0x1b3/0x26e
>  [<ffffffff8025c8d2>] do_hypervisor_callback+0x1e/0x2c
>  <EOI> [<ffffffff88355b7e>] :usbbk:usbbk_schedule+0x1e0/0xa3f
>  [<ffffffff8029482f>] autoremove_wake_function+0x0/0x2e
>  [<ffffffff802947ba>] keventd_create_kthread+0x0/0x62
>  [<ffffffff8835599e>] :usbbk:usbbk_schedule+0x0/0xa3f
>  [<ffffffff802947ba>] keventd_create_kthread+0x0/0x62
>  [<ffffffff802325e4>] kthread+0xd4/0x108
>  [<ffffffff8025cb24>] child_rip+0xa/0x12
>  [<ffffffff802947ba>] keventd_create_kthread+0x0/0x62
>  [<ffffffff8025c827>] error_exit+0x0/0x71
>  [<ffffffff80232510>] kthread+0x0/0x108
>  [<ffffffff8025cb1a>] child_rip+0x0/0x12
>  
> FYI, I am trying to get pvusb working under GPLPV and I had never seen
> the Reconfiguring/Reconfigured states before and so I wasn't doing
> anything with them...
> 
> James
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

RE: bug in usbback

[James Harper]

> You mean:
> - Your version of usbfront NOT respond to Reconfiguring state.
>    It's not bug of original usbfront.

Correct. I was not responding at all to 'Reconfiguring' (I didn't even
know they existed at the time - I was just plugging my generic xenbus
routines into a skeleton usb driver).

> - When usbback is in Reconfiguring state and usbfront NOT respond to
>    that, rebooting domU causes the BUG.

Yes. Actually just stopping DomU is enough to cause the bug. It doesn't
appear to matter if it restarts or not. I was able to work around the
problem by writing 'Reconfiguring' and then 'Connected' into the
frontend xenstore to complete my testing.

> 
> Is my understanding right?
> 

Yes. Obviously with a correctly functioning frontend (mine works
properly now) the bug in the backend should not become apparent, but if
the DomU ever crashed during the transition from backend Reconfiguring
to frontend Reconfiguring, then Dom0 would be affected so I think it is
worth fixing this bug. Also a malicious DomU could exploit this.

Thanks

James

Re: bug in usbback

[Noboru Iwamatsu]

Hi,

James Harper wrote:
> Also, a question on behaviour:
> 
> When we receive a reconfigure event, I assume that we should then read
> 'num-ports' and scan through each of the port-n values in xenstore. Is
> there any guarantee that a device hasn't been unplugged and replugged
> from the same port during the reconfiguration window? How should I
> detect such a thing if the value of port-n hasn't changed? Should I then
> probe each device? I'm doing this under Windows which is a bit more
> picky and much less flexible...

"num-ports" is only read in creating the hcd. This value is unchanged
during reconfiguring state.

Reconfiguring is called from usb probe and disconnect callbacks.
So, reconfiguring is called every time unplugging and plugging are
detected.

However, connecting and disconnecting the devices at the same time
or frequent operations were not tested enough.

Do you think it's good idea that the method for the notification
of hotplug is switched from xenbus to RING (like pv network's rx_ring)?

Regards,
Noboru

RE: bug in usbback

[James Harper]

> James Harper wrote:
> > Also, a question on behaviour:
> >
> > When we receive a reconfigure event, I assume that we should then
read
> > 'num-ports' and scan through each of the port-n values in xenstore.
Is
> > there any guarantee that a device hasn't been unplugged and
replugged
> > from the same port during the reconfiguration window? How should I
> > detect such a thing if the value of port-n hasn't changed? Should I
then
> > probe each device? I'm doing this under Windows which is a bit more
> > picky and much less flexible...
> 
> "num-ports" is only read in creating the hcd. This value is unchanged
> during reconfiguring state.
> 
> Reconfiguring is called from usb probe and disconnect callbacks.
> So, reconfiguring is called every time unplugging and plugging are
> detected.
> 
> However, connecting and disconnecting the devices at the same time
> or frequent operations were not tested enough.
> 
> Do you think it's good idea that the method for the notification
> of hotplug is switched from xenbus to RING (like pv network's
rx_ring)?
> 

I like the idea of having the port states in the xenstore. It is a low
speed and (mostly) relatively infrequently changing thing so I think
keeping it in xenstore is appropriate.

I think the problem could be solved by simply including a port change
counter for each port, either as part of the existing port-N value or as
an additional 'port-N-generation' field. The backend would just
increment that every time there is a plug or unplug event, and the
frontend could compare the value to what it was last time to see if the
port had changed.


So maybe "port-N=S" would become "port-N=GGS", where S is the current
'speed' value, and "GG" is a change counter. As an example:

1. port-1=000
2. <hot plug>
3. port-1=013
4. <hot unplug>
5. port-1=020
6. <hot plug>
7. port-1=033

It would unfortunately break backwards compatibility, but it's still in
the staging area, for exactly this reason right?

Thanks

James

Re: bug in usbback

[Noboru Iwamatsu]

Hi,

>> Do you think it's good idea that the method for the notification
>> of hotplug is switched from xenbus to RING (like pv network's
> rx_ring)?
> 
> I like the idea of having the port states in the xenstore. It is a low
> speed and (mostly) relatively infrequently changing thing so I think
> keeping it in xenstore is appropriate.
> 
> I think the problem could be solved by simply including a port change
> counter for each port, either as part of the existing port-N value or as
> an additional 'port-N-generation' field. The backend would just
> increment that every time there is a plug or unplug event, and the
> frontend could compare the value to what it was last time to see if the
> port had changed.
> 
> 
> So maybe "port-N=S" would become "port-N=GGS", where S is the current
> 'speed' value, and "GG" is a change counter. As an example:
> 
> 1. port-1=000
> 2. <hot plug>
> 3. port-1=013
> 4. <hot unplug>
> 5. port-1=020
> 6. <hot plug>
> 7. port-1=033

I don't think it's a good idea.

If disconnection happened, the urb transfer is sure to make an error, 
and usbfront can detect this error without counter.

Furthermore, usbfront only emulates the roothub ports' virtual status
by the value of port-N's speed, and, even if the counter value is wrong,
this value cannot be associated to the rootport's status.

As I mentioned, if notifications by xenbus has the problem, I prefer 
using RING.


Regards,
Noboru

RE: bug in usbback

[James Harper]

> >
> > So maybe "port-N=S" would become "port-N=GGS", where S is the
current
> > 'speed' value, and "GG" is a change counter. As an example:
> >
> > 1. port-1=000
> > 2. <hot plug>
> > 3. port-1=013
> > 4. <hot unplug>
> > 5. port-1=020
> > 6. <hot plug>
> > 7. port-1=033
> 
> I don't think it's a good idea.
> 
> If disconnection happened, the urb transfer is sure to make an error,
> and usbfront can detect this error without counter.

Is it a good idea for the usb device driver running in the frontend to
detect an unplug->replug event by getting error codes? It could report a
device failure in that case rather than figuring out that the device has
been unplugged and replugged.

> 
> Furthermore, usbfront only emulates the roothub ports' virtual status
> by the value of port-N's speed, and, even if the counter value is
wrong,
> this value cannot be associated to the rootport's status.

The last digit of the counter still represents the port virtual status.
Only the first two digits increment. As per my previous email, the
format of the port-N xenstore entry would become 'GGS' where GG is the
counter value and 'S' is the status, as it is now.

> 
> As I mentioned, if notifications by xenbus has the problem, I prefer
> using RING.
> 

The problem with the ring method is that there has to be an outstanding
ring request sent by the frontend in order for the backend to be able to
notify the frontend. I think that would add unnecessary overhead.

James

Re: bug in usbback

[Noboru Iwamatsu]

Hi,

James Harper wrote:
>>> So maybe "port-N=S" would become "port-N=GGS", where S is the
> current
>>> 'speed' value, and "GG" is a change counter. As an example:
>>>
>>> 1. port-1=000
>>> 2. <hot plug>
>>> 3. port-1=013
>>> 4. <hot unplug>
>>> 5. port-1=020
>>> 6. <hot plug>
>>> 7. port-1=033
>> I don't think it's a good idea.
>>
>> If disconnection happened, the urb transfer is sure to make an error,
>> and usbfront can detect this error without counter.
> 
> Is it a good idea for the usb device driver running in the frontend to
> detect an unplug->replug event by getting error codes? It could report a
> device failure in that case rather than figuring out that the device has
> been unplugged and replugged.

USB device driver (aka function driver) detects plug/unplug event by
probe/disconnect callbacks. These callbacks and URB transfer error
callback are different.

Disconnection causes the transfer error, or disconnection is detected
and the transfer is canceled. Normally, after canceling the transfer,
the device is re-initialized (if the device is still present).

In any case, it might be another problem that reports the disconnection
event as an error.

>> Furthermore, usbfront only emulates the roothub ports' virtual status
>> by the value of port-N's speed, and, even if the counter value is
> wrong,
>> this value cannot be associated to the rootport's status.
> 
> The last digit of the counter still represents the port virtual status.
> Only the first two digits increment. As per my previous email, the
> format of the port-N xenstore entry would become 'GGS' where GG is the
> counter value and 'S' is the status, as it is now.

I mean "roothub ports' virtual status" as port status that is emulated
in usbfront roothub part. How do you associate the error of the counter
to wPortChange bit field?
  (See struct rhport_status in usbfront.h and usbfront_hub.c)

>> As I mentioned, if notifications by xenbus has the problem, I prefer
>> using RING.
>>
> 
> The problem with the ring method is that there has to be an outstanding
> ring request sent by the frontend in order for the backend to be able to
> notify the frontend. I think that would add unnecessary overhead.
> 

I believe the method of no "reconfiguring mismatch" happening is better
than correcting the mismatch. It's necessary overhead.

Regards,
Noboru