Re: [PATCH V12 3/9] PCI: Create PCIe library functions in support of DOE mailboxes.

[Ira Weiny <ira.weiny@intel.com>]

On Thu, Jun 30, 2022 at 04:25:40PM +0100, Jonathan Cameron wrote:
> On Wed, 29 Jun 2022 21:34:18 -0700
> Ira Weiny <ira.weiny@intel.com> wrote:
> 

[snip]

I've dropped the IRQ support and was polishing things up.  Without the IRQ I
don't think any 'arming' makes sense.

However, in working through the sequence again I think I found another problem.
I _think_...  :-/

> > 
> > But we are only going to see this if some other entity is using the mailbox
> > right?  And I don't think that is going to be common, is it?
> 
> BUSY on entry to doe_statemachine_work() is indeed only relevant if
> some other entity is trampling on us. It's best effort only.
> 
> BUSY during normal flow is the one I care about.
> In most cases it will go like (assuming we clear the int status in the handler as now)
> 
>              Send Object
> BUSY      ________|-----___________________
> PROC      ________|------------------______
> OBJ RDY   ___________________________-------
> Int Status______________-____________-_____

So I did not realize that BUSY could clear like this.  I thought the point of
BUSY was to indicate someone else had an exchange in flight.

What happens if another entity jumps in during the PROC time?  How does one
know that OBJ RDY is _our_ object ready and not someone else's?

For example 'entity' issues a send, we see busy clear and also start a
send.  But the device is still processing the send from 'entity':

            Send Object(entity) Send Object (Linux)
BUSY      ___|----_______________|---______________________________
PROC      ___|-----------------------------___|-----------------___
OBJ RDY   _________________________________-------______________---
Int Status________-__________________-_____-____________________-__

                                          ^^^
					  This...

... is _not_ Linux's object!?!?!?!

Can that happen?

If so this is entirely broken.  Even Polling OBJ RDY will break.  And worse yet
we will not even see BUSY being set in any 'abnormal' way.

> 
> where I've added PROC to mean the device is processing the data.
> Once it clears the input buffer on the device and hence the device can accept
> another protocol request BUSY will drop.  If device has some pipelining
> or runs multiple protocols in different threads, you can think of that busy
> period just being the time it needs to copy out the request to some protocol
> thread specific storage.

BUSY was not at all doing what I thought it did.  I'm now concerned that it is
completely broken WRT to other entities even without IRQs.  Frankly I'm
confused why pci_doe_send_req() even checks for busy because it is unlikely
that we will ever see it set.  For sure we won't from our side because the
workqueue is going to process one task at a time.

If Linux wanted to have multiple objects in flight I think we would need a much
more complex state machine than we had.  Maybe your original state machine
handled this.  If so, I apologize for missing this subtle point.

At this point I'm debating removing the check for BUSY as well because I don't
see the point.  (Other than maybe flagging some error to say that 'entity' may
be messing things up for us and bailing.)

Thoughts?
Ira

> 
> You won't see this in QEMU without extra hacks because we shorten the
> flow so that whole thing is instantaneous.
> 
> If those two interrupts per transfer occur well spread out they can result in
> your INT flag being set too often and some of the waits dropping through early.
> 
> It will 'work' I think though because you ultimately spin on Data object
> ready which won't be set until after the second interrupt.
>