Re: usb storage cleanup

[Matthew Dharm <mdharm-kernel@one-eyed-alien.net>]

[-- Attachment #1: Type: text/plain, Size: 3752 bytes --]

On Thu, Jul 04, 2002 at 12:19:28AM +0200, Manfred Spraul wrote:
> Matthew Dharm wrote:
> > I don't understand what this patch is trying to do...
> > 
> > You're reverting our new state machine changes... why?
> > 
> 
> Because the state machine doesn't work. I've degraded it into a 
> debugging state.
> I've described it in a mail I send to you and linux-usb-devel a few 
> weeks ago, without any reply.

I've got that mail, and it's on my todo list.

> E.g. queue_command stored new commands in ->queue_srb. The worker thread 
> then moved it from queue_srb to srb and set sm_state to RUNNING.
> 
> But what if command_abort() is called before the worker thread is scheduled?

Then we have a serious problem, because the aborts are on the order of
several seconds.  If the thread hasn't gotten scheduled by then it _should_
cause a BUG_ON.

> State machines and asynchroneous command aborts are incompatible, that 
> why I've moved command abortion out of sm_state.

I disagree here.  I think the clear state machine is the -only- way to get
this right.  We tried it without the state machine, and all we did was find
more and more corner cases which are not handled.

> > You're reverting the new mechanism to determine device state... why?
> 
> Unnesessary duplication. Device disconnected is equivalent to 
> ->pusb_dev==NULL. Why do you need a special variable?

Because relying on a pointer has caused problems in the past, especially
when there are concerns that the pointer might be invalid.

> > You're removing the entire bus_reset() logic... why?
> >
> You are right, that change is not correct.
> Do you remember the reasons that lead to the current implementation?
> 
> Hmm. Are you sure that the code can't cause data losses with unrelated 
> devices?
> Suppose I have an usb hub installed, and behind that hub 2 usb disks. If 
> bus_reset is called for the scsi controller that represents one disk, 
> won't that affect the data transfer that go to the other disk?

The hub isn't reset, only the target device is.

> > This patch undoes most of the work done in the last few months.  I
> > _strongly_ oppose the patch without some better explanations.
> 
> I've sent you a mail on 06/02 with details about all changes.
> 
> http://www.geocrawler.com/archives/3/2571/2002/6/600/8821396/
> 
> You did not reply, thus I assumed that you were too busy and I fixed 
> everything myself.

I see.. thus skipping the 4 patches which address most of these issues
which are in my queue.

Look, I might not be that speedy on this, but did it at least occur to you
to contact _any_ of the other usb-storage people?  Bjorn?  Stern?

> The only new change is removing the call to usb_stor_CBI_irq() and 
> replacing it with "up(&us->ip_waitq);" from usb_stor_abort_transport. 
> Setting sm_state and then calling usb_stor_CBI_irq() is a 
> synchronization nightmare.
> Situation: command is completed by the hardware and aborted by the scsi 
> midlayer at the same time. usb_stor_abort_transport() could run on cpu1, 
> _CBI_irq() on cpu2. Now imagine you run on Alpha, where both reads and 
> writes are reordered. Initially I tried to fix it with memory barriers, 
> but the new version is much simpler.

The only requirement in this condition is that the command state be
consistent at the end -- either completed or aborted.  I don't see how the
current code fails this requirement...

Matt

-- 
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
Maintainer, Linux USB Mass Storage Driver

A:  The most ironic oxymoron wins ...
DP: "Microsoft Works"
A:  Uh, okay, you win.
					-- A.J. & Dust Puppy
User Friendly, 1/18/1998

[-- Attachment #2: Type: application/pgp-signature, Size: 232 bytes --]