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: [EMAIL PROTECTED] 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
msg07568/pgp00000.pgp
Description: PGP signature
