Re: usb storage cleanup

[Matthew Dharm]

--9l24NVCWtSuIVIod
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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...
> >=20
> > You're reverting our new state machine changes... why?
> >=20
>=20
> Because the state machine doesn't work. I've degraded it into a=20
> debugging state.
> I've described it in a mail I send to you and linux-usb-devel a few=20
> 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=
=20
> then moved it from queue_srb to srb and set sm_state to RUNNING.
>=20
> But what if command_abort() is called before the worker thread is schedul=
ed?

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=20
> 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?
>=20
> Unnesessary duplication. Device disconnected is equivalent to=20
> ->pusb_dev=3D=3DNULL. 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?
>=20
> Hmm. Are you sure that the code can't cause data losses with unrelated=20
> devices?
> Suppose I have an usb hub installed, and behind that hub 2 usb disks. If=
=20
> bus_reset is called for the scsi controller that represents one disk,=20
> 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.
>=20
> I've sent you a mail on 06/02 with details about all changes.
>=20
> http://www.geocrawler.com/archives/3/2571/2002/6/600/8821396/
>=20
> You did not reply, thus I assumed that you were too busy and I fixed=20
> 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=20
> replacing it with "up(&us->ip_waitq);" from usb_stor_abort_transport.=20
> Setting sm_state and then calling usb_stor_CBI_irq() is a=20
> synchronization nightmare.
> Situation: command is completed by the hardware and aborted by the scsi=
=20
> midlayer at the same time. usb_stor_abort_transport() could run on cpu1,=
=20
> _CBI_irq() on cpu2. Now imagine you run on Alpha, where both reads and=20
> writes are reordered. Initially I tried to fix it with memory barriers,=
=20
> 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

--=20
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.=
net=20
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

--9l24NVCWtSuIVIod
Content-Type: application/pgp-signature
Content-Disposition: inline

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.0.6 (GNU/Linux)
Comment: For info see http://www.gnupg.org

iD8DBQE9I5FAIjReC7bSPZARAs5NAJ9CfxGyT7+lqXEqaWBCWBRzOBuRdACfe8xs
Sh5/UsqaFaMKUKV4MV0WvyY=
=5Ekn
-----END PGP SIGNATURE-----

--9l24NVCWtSuIVIod--