> Are we talking about the same thing here?  In my original message I was
> describing the usb_device_altered() function, which we expect a driver to
> call after it has called usb_device_reset().  The reset() call rebinds all
> the drivers to the interfaces they had before the reset, explicitly
> assuming that the descriptors have _not_ changed.  (That will be the
> common case for things like error recovery.)  Altered() reads the new
> descriptors, unbinds the drivers, and binds the new interfaces.

OK, yes I see. You're right. Nevertheless I'd like a version that does
a full reset with enumeration for error recovery.

> Okay, so what about errors like a failure to accept firmware?  As you say,
> the device is screwed.  I don't know what you mean by a "full reset" --
> usb_device_reset() really does reset the device and usb_device_altered()
> will re-read the descriptors and do reprobing.  If reading the descriptors
> fails, altered() will disable the device, just as in the case of a
> newly-connected device.  What else remains to be done?

Reset again. If a device has a failure somewhere during firmware load,
it's probably really screwed.

[..]
> > > another way, if a firmware download is needed for the device to work
> > > properly, why wouldn't that download be done first thing, during
> > > probe()?
> >
> > Right, but there it's not the core's lock, but the driver model's lock.
> > And neither is the problem specific to USB. Wouldn't it be more elegant
> > to just return -ERESTARTSYS and let the driver model reprobe?
>
> No.  We need a usbcore-specific lock.  It's not enough to rely on the
> driver model's locking.  Doing that would leave open the possibility, for

That's not a question of reliance. The generic lock is there. We need
to deal with it. Furthermore we can always live with coarser locking.

> example, of a driver newly bound to one interface doing a
> set_configuration() at the same time that a driver for another interface
> is being probed.  To avoid this, set_config and probing must use the
> _same_ lock.

Yes, but it need not be per device. Per bus would do as well.
Per interface is wrong.

> The real problem is that the driver model is only aware of separate
> individual interfaces, whereas usbcore realizes that several interfaces
> are part of the same device.  We need to lock the whole device, not just
> a single interface.
>
> > Sorry, I was unclear. Shouldn't robust error handling look like:
> > disable port -> reset hub -- retry disable -> powerdown port ->
> > reset hub -- retry power down port -> power down hub -> recursion
>
> Would that really work?  That is, if you reset the hub, won't that disable
> _all_ its ports?  We don't want to do that if powering down a single port
> is possible.

Powering down means that the port will no longer work. From disable we can
recover.

> > > I agree partially.  Probing while the device is not in sync wouldn't be
> > > a disaster, but it would be nice to avoid it.  Perhaps the
> > > usb_device_altered() routine could set some flag which would tell the
> > > core to abort further probing until after the new descriptors have been
> > > read. This would not require exporting the locking to the drivers.
> >
> > Too late. Submitting URBs to nonexistant interfaces is a very bad idea.
>
> Let's be more precise.  URBs aren't submitted to interfaces, they're
> submitted to endpoints.  Submitting an URB to a nonexistent endpoint isn't
> very bad, it merely causes an error code to be sent to the URB's

Nonexistent is survivable. Endpoints going away while the URB is processed
are another matter.

> completion routine.  Besides, who's going to be sending these URBs?  Not
> the driver that just altered the device's state.  And if further probing
> is aborted, no later drivers will get the chance.  Maybe a driver that was
> bound earlier.

Right.

> Anyway, how will allowing the drivers to lock out further probing prevent
> these bad URBs from being sent?

Lock against probe -> Kick off other drivers -> reset/config change

> One way around this might be to have a combined reset_altered()
> function.  That would make sense; it's hard to think of a case where you
> would want to call device_altered() without calling device_reset()
> immediately beforehand.  Doing things this way would avoid having any
> period in which the core is out-of-sync with the device.

Firmware download.

> > > 2. I don't see how we can have a version without locking.  Setting a
> > > new configuration necessarily involves unbinding the existing
> > > interfaces and binding the new ones, and that requires holding the
> > > device lock.
> >
> > Which is held during probe, where these cases arise.
>
> I don't like that idea.  It might lead to unbounded stack usage.  You're
> saying that a driver's probe() function could tell the core to set a
> configuration and restart probing from the beginning, without waiting for
> the original probe() to return.  It goes against my grain.

No, by returning an error. There's no recursion. The loop is justed started
anew.

> > > helper to do the job.  But the existing code would probably still need
> > > to be rewritten.  The correct way for a driver to handle this in its
> > > probe() routine is:  See if the proper configuration is already loaded.
> > >  If yes then proceed normally; if no then call usb_set_configuration()
> > > and return a failure code (i.e., don't bind).
> >
> > Then you can fail regardless. Could you explain? I don't see the logic?
>
> I don't understand what you mean by "you can fail regardless"... but I'll
> explain the earlier statement.  First, it's clear that if the correct
> configuration is already loaded then the driver doesn't need to ask for it
> to be loaded again.  The driver can simply go on about its job.
>
> Second, if the wrong configuration is loaded, the driver _does_ need to
> call set_configuration().  Doing so will ultimately cause a new set of
> interfaces to be bound, and it will also cause the driver to be unbound
> from the interface it is currently probing should the probe() return
> success.  So returning success is pointless; the driver might as well just
> return a failure code.  Even more strongly: if the configuration is wrong
> then the driver has no business binding to this interface in the first
> place, so it should return a failure code regardless, even if
> set_configuration() fails.

Probe() should have three possible returns.
1. can deal with the device
2. cannot deal with the device
3. can deal in a new form with firmware now in place

> But assuming everything goes well and set_configuration() succeeds, the
> core will at some point probe the interfaces in the new configuration.
> One of them presumably will match the driver, so probe() will be called
> again, this time with the correct configuration in place.  Then the driver
> will be happy and things will work as expected.

Right.

        Regards
                Oliver



-------------------------------------------------------
This SF.net email is sponsored by: eBay
Get office equipment for less on eBay!
http://adfarm.mediaplex.com/ad/ck/711-11697-6916-5
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Reply via email to