On Tuesday, February 24, 2009 5:00 PM, Grant Grundler wrote:
> > +static void
> > +_base_display_reply_info(struct MPT2SAS_ADAPTER *ioc, u16
> smid, u8 VF_ID,
> > + u32 reply)
> > +{
> > + MPI2DefaultReply_t *mpi_reply;
> > + u16 ioc_status;
> > +
> > + mpi_reply = base_get_reply_virt_addr(ioc, reply);
> > + ioc_status = le16_to_cpu(mpi_reply->IOCStatus);
> > +#ifdef CONFIG_SCSI_MPT2SAS_LOGGING
> > + if ((ioc_status & MPI2_IOCSTATUS_MASK) &&
> > + (ioc->logging_level & MPT_DEBUG_REPLY)) {
> > + _base_sas_ioc_info(ioc , mpi_reply,
> > + base_get_msg_frame(ioc, smid));
> > + }
> > +#endif
>
> Can all the "if (ioc_status...)" gunk get wrapped into a macro that is
> conditionally defined?
> e.g. the logging level could become a compile time option instead of a
> runtime option.
>
> I was able to cut 1% CPU utilization from another driver by allowing
> the compiler to get
> rid of all the debug messages that would never get printed because
> they were below
> a particular threshold.
>
> Maybe it doesn't matter in this case. But I've no clue yet how
> often/when _base_display_reply_info()
> is called.
I have already implemented macros for all the debug prints in the driver. In addition, these debug prints are designed to be compile time options, as you have suggested. There are logging levels to designate different type of debug that one would be interested in. There is an option in the Kconfig via CONFIG_SCSI_MPT2SAS_LOGGING for this. Please refer to mpt2sas_debug.h and Kconfig.
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.
> > +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()?
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.
> > + completed_cmds = 0;
> > + do {
> > + if (ioc->reply_post_free[post_index].Words ==
> > + 0xFFFFFFFFFFFFFFFFULL)
>
> Would this be better written as "~0ULL" or as a predefined constant?
>
I guess your right, that would be better becuase its shorter.
> > + /* reply post descriptor handling */
> > + post_index_next = ioc->reply_post_host_index;
> > + for (i = 0 ; i < completed_cmds; i++) {
> > + post_index = post_index_next;
> > + /* poison the reply post descriptor */
> > + ioc->reply_post_free[post_index_next].Words =
> > + 0xFFFFFFFFFFFFFFFFULL;
> > + post_index_next = (post_index ==
> > + (ioc->reply_post_queue_depth - 1))
> > + ? 0 : post_index + 1;
> > + }
> > + 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.
> > +static int
> > +_base_config_dma_addressing(struct MPT2SAS_ADAPTER *ioc,
> struct pci_dev *pdev)
> > +{
> > + struct sysinfo s;
> > + char *desc = NULL;
> > +
> > + if (sizeof(dma_addr_t) > 4) {
> > + const uint64_t required_mask =
> > + dma_get_required_mask(&pdev->dev);
> > + if ((required_mask > DMA_32BIT_MASK) &&
> !pci_set_dma_mask(pdev,
> > + DMA_64BIT_MASK) &&
> !pci_set_consistent_dma_mask(pdev,
> > + DMA_64BIT_MASK)) {
> > + ioc->base_add_sg_single =
> &base_add_sg_single_64;
> > + ioc->sge_size = sizeof(Mpi2SGESimple64_t);
> > + desc = "64";
> > + goto out;
> > + }
> > + }
> > +
> > + if (!pci_set_dma_mask(pdev, DMA_32BIT_MASK)
> > + && !pci_set_consistent_dma_mask(pdev, DMA_32BIT_MASK)) {
> > + ioc->base_add_sg_single = &base_add_sg_single_32;
> > + ioc->sge_size = sizeof(Mpi2SGESimple32_t);
> > + desc = "32";
> > + } else
> > + return -ENODEV;
> > +
> > + out:
> > + si_meminfo(&s);
> > + printk(MPT2SAS_INFO_FMT "%s BIT PCI BUS DMA
> ADDRESSING SUPPORTED, "
> > + "total mem (%ld kB)\n", ioc->name, desc,
> convert_to_kb(s.totalram));
> > +
> > + return 0;
> > +}
>
>
> 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.
> > +/**
> > + * base_check_enable_msix - checks MSIX capabable.
> > + * @ioc: per adapter object
> > + *
> > + * Check to see if card is capable of MSIX, and set number
> > + * of avaliable msix vectors
> > + */
> > +static int
> > +base_check_enable_msix(struct MPT2SAS_ADAPTER *ioc)
> > +{
> > + int base;
> > + u16 message_control;
> > + u32 msix_table_offset;
> > +
> > + base = pci_find_capability(ioc->pdev, PCI_CAP_ID_MSIX);
> > + if (!base) {
> > + dfailprintk(ioc, printk(MPT2SAS_INFO_FMT "msix not "
> > + "supported\n", ioc->name));
> > + return -EINVAL;
> > + }
> > +
> > + /* 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.
>
> 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 architechs 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().
> > +
> > + /* 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 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.
>
> > + r = request_irq(entries[0].vector, base_interrupt,
> IRQF_SHARED,
> > + ioc->name, ioc);
>
> MSI is never shared. IRQF_SHARED flag is still correct?
>
> I have to stop looking now...back to work. :(
>
There are two drivers that currently implement MSIX in the scsi tree. I found the emulex driver doing IRQF_SHARED, and qlogic is zero. Since we suport shared interrupts, I set it up for that. I didn't know if it mattered, does it?
--
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
