Re: [PATCH 10/16] gdth: gdth_get_status() return pointer to host not its index

[Boaz Harrosh <bharrosh@panasas.com>]

On Sun, Sep 30 2007 at 23:26 +0200, Christoph Hellwig <hch@infradead.org> wrote:
> On Sun, Sep 30, 2007 at 10:09:20PM +0200, Boaz Harrosh wrote:
>>   - Return the interrupting host and not it's index.
>>     NULL if intr is not by gdth.
>>   - fix calling site
>>
>>   FIXME:
>>    Why are we looping on all cards? the passed dev_id
>>    can be used for pinpointing the exact interrupting
>>    card. The kernel is already looping on all sharing
>>    cards so this code makes it O(n^2).
> 
> This is just pre-historic code that makes no sense whatover in a modern
> kernel.  I'd much prefer to stop doing this and use dev_id properly.
> 
OK I'm attempting just that but please look in with me on the code
I have some questions.

>>    What is that polling mode? can we pass dev_id to
>>    gdth_get_status() and do a poll only if dev_id is
>>    NULL?
> 
> Just loop over all host adapters in gdth_wait instead and pass in the
> ha pointer.
> 

[Issue 1]
  In gdth_get_status() the first check is if(ha->irq == passed_in_irq)
  if not than no status read is attempted and 0 is returned. In this
  case gdth_interrupt() will return with out doing any advancement of states.

  Now gdth_wait() calls with gdth_interrupt(ha->irq, ha) so it means that in
  effect current code will loop on all devices but will actually read the status
  of only the waiting on card.

  [Q1] So am I safe in just dropping the loop even from gdth_wait() ?
  [Q2] When gdth_interrupt() is called by the kernel am I guaranteed that irq will
  match the ha->irq that is passed as dev_id? (Since that is what I registered
  as my interrupt routine)

[Issue 2]
  In gdth_wait() and gdth_interrupt() the globals 
  "gdth_from_wait", "wait_hanum" and "wait_index" give me the creeps.
  We are talking about hotplug API and proper lucking of adapter's list
  and this thing is just plain don't work with more than one card.

  [Q1] Do you understand all that business with gdth_polling and gdth_wait()?
       It looks like if you do gdth_polling==true than you can have only one
       card. Is that true?
  [Q2] Should I fix that? I thought of doing a __gdth_interrupt() that is called
       by gdth_interrupt() and passes all these members on the stack as parameters
       to __gdth_interrupt().
       (Actually most of them will be dropped they will not be needed)

Boaz