I think of the issues was clarified...a few more are still open
(below) but shouldn't
preclude inclusion in scsi-misc and continue the dialog around that code.
On Wed, Feb 25, 2009 at 12:27 PM, Moore, Eric <Eric.Moore@xxxxxxx> wrote:
...
[ re _base_display_reply_info() function ]
> Also, this particular function needs to be called all the time becuase at the end of it we are printing out the loginfos returned by controller firmware. The loginfos are needed as a way to be informed when problems resonating below the driver in controller firmware.
Ok. The frequency this gets called will dictate if it's a problem or
not. The design choice makes sense though.
Diagnostic logging is something linux still sucks at but is getting
better (dev_printk is a good step).
>
>
>> > +static void
>> > +_base_mask_interrupts(struct MPT2SAS_ADAPTER *ioc)
>> > +{
>> > + u32 him_register;
>> > +
>> > + ioc->mask_interrupts = 1;
>> > + him_register = readl(&ioc->chip->HostInterruptMask);
>> > + him_register |= MPI2_HIM_DIM + MPI2_HIM_RIM +
>> MPI2_HIM_RESET_IRQ_MASK;
>> > + writel(him_register, &ioc->chip->HostInterruptMask);
>>
>> This is a posted write. Does it need to be flushed?
>> ie do other parts of the driver require this take effect immediately
>> or will they tolerate some late arriving interrupt?
>
>
> What are you suggesting? Calling wmb()?
No. "Flushing" meant "readl()".
> I thought I was handling this case. I'm setting the flag "ioc->mask_interrupts". If there are any spurious interrupts that occur following this calling this function, I will ignore them from the interrupt routine -> base_interrupt() where I have this sanity check.
No change required then. My preference would be to flush the writel in
base_mask_interrupts and avoid adding code to the interrupt routine.
But this will work too.
...
>> > + ioc->reply_post_host_index = post_index_next;
>> > + writel(post_index_next, &ioc->chip->ReplyPostHostIndex);
>> > + wmb();
>>
>> What is the wmb() protecting here? Add a comment?
>>
>> My impression is the wmb() belongs in front of the writel()
>> and not behind it.
>> That is, we want to make sure the updates to ioc->reply_post_free[]
>> are visible before sending out the writel().
>>
>
> I'm doing is a 64bit write to pcie memory. On some systems this is broken out to two 32bit writes. So I put the barrier there to insure the entire 64bits was written out to the ReplyPostHostIndex register before another write was done.
Can you add a comment what the wmb() is protecting against?
The above explanation wasn't quite right and I still have the feeling
this wmb() is misplaced. FWIW, I expect this would only cause problems
on alpha/ia64 (machines which have weak memory ordering).
>> > +static int
>> > +_base_config_dma_addressing(struct MPT2SAS_ADAPTER *ioc,
...
>> Interesting. I would have expected the dma_mask and
>> consistent_dma_mask to not depend on each other.
>> ie set dma_mask to 64, if that fails, set dma_mask to 32-bit.
>> Rinse and repeat for consistent_dma_mask.
>
> I'm not clear what your point is. My algorithm is to find out if the system has addressable memory above 32bit boundary, then set that mask for 64bit, else go for 32bit addressing.
Yeah, this ok. It's just a little more convoluted than other drivers
I've looked at in the past.
My preference would be the driver not try to optimize in this case
since the pci_set_dma_mask(DMA_64_MASK) could fail in the case where
32-BIT DMA mask would be sufficient. However, most drivers won't
change API because of this.
Again, this is an issue for future discussion and not something that
would block adoption of this driver.
...
>> > +base_check_enable_msix(struct MPT2SAS_ADAPTER *ioc)
...
>> > + /* get msix vector count */
>> > + pci_read_config_word(ioc->pdev, base + 2, &message_control);
>> > + ioc->msix_vector_count = (message_control & 0x3FF) + 1;
>>
>>
>> Any comments on why the return value from pci_enable_msix()
>> isn't used here?
>
> I'm calling pci_enable_msix() from base_enable_msix(), and from that function I'm checking the return value. I'm not clear what your point is.
My point is device drivers in general shouldn't need to frob MSI
config space. ioc->msix_vector_count should be set in the same
function that is calling pci_enable_msix(). But I haven't looked at
all of the code (including base_save_msix_table() mentioned below) and
will drop this part of the thread until I do.
>>
>> I wasn't expecting the drivers would need to read the MSIX
>> table directly.
>
> There is a bug in the hardware, where the msix table is cleared out during
> host reset. So the architects here at LSI decided to address this errata by
> having all the device driver writters back and restore this table during host reset.
> I made reference to this errata from the comments in base_save_msix_table().
Ah ok - I didn't see those (yet). Sounds good.
>
>> > +
>> > + /* current HW implemention uses only one interrupt
>> handler */
>>
>> Which implementation?
>> This comment sounds like it expects to be obsolete soon.
>> Specifying the implementation makes it clear even when other
>> implementations become available.
>>
>
> The explanation is currently we only map the first MSIX vector for all interrupts
The "current implementation" has a name or revision. I'm just asking a specific
name or version be used instead of "current".
> In future products we may have more expand to use more vectors, however
> that has not been decided upon yet. What I mean by more vectors is we might
> map the 1st vector for initiator mode, and the 2nd for target mode, or maybe in
> virtualization, each VF port is assigned its own handler.
Yup - reserving those for future use is fine. I think willy already pointed out
if the driver knows only one is used, the driver should only allocate
one vector.
...
>> MSI is never shared. IRQF_SHARED flag is still correct?
...
> There are two drivers that currently implement MSIX in the scsi tree. I found
> the emulex driver doing IRQF_SHARED, and qlogic is zero.
There are more in drivers/net:
grundler <2034>find -name \*.c | xargs fgrep -l pci_enable_msix |
xargs fgrep -l request_irq
./bnx2.c
./bnx2x_main.c
./cxgb3/cxgb3_main.c
./e1000e/netdev.c
./enic/enic_main.c
./forcedeth.c
./igb/igb_main.c
./ixgbe/ixgbe_main.c
./myri10ge/myri10ge.c
./netxen/netxen_nic_main.c
./niu.c
./qlge/qlge_main.c
./s2io.c
But one has to manually check IRQF_SHARED usage. I don't know how to script
something that verifies request_irq() is called in the MSI-X enabled case.
> Since we support shared interrupts, I set it up for that. I didn't know if it mattered, does it?
I don't think it's correct but I don't know if it matters either. I
doubt it does.
I was hoping someone else would "just know".
I hope that clarifies my concerns and apologies for not responding sooner.
thanks,
grant
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
