Thanks James, for your feeback, I will add your comments and repost the patch again. -Sathya
-----Original Message----- From: James Bottomley [mailto:[EMAIL PROTECTED] Sent: Tuesday, February 12, 2008 4:24 AM To: Prakash, Sathya Cc: linux-scsi@vger.kernel.org; Moore, Eric; [EMAIL PROTECTED] Subject: Re: [PATCH] mpt fusion: Power Management fixes for MPT SAS PCI-Econtrollers On Mon, 2008-02-11 at 18:36 +0530, Prakash, Sathya wrote: > This patch was submitted as [PATCH 2/3] yesterday, since it did not > reach the list due to CC errors sending this again. > It is regenerated against new git tree. > > The system power state changes like hibernation and standby are not > happening properly with 106XE controllers, this patch modifies the > driver to free resources and allocate resources in power management > entry points and to enable MSI interrupts for SAS controllers I really wish you hadn't done this. Both MSI and suspend/resume are somewhat bug inducing at the moment. If you combine both the msi default change and suspend/resume changes in a single patch, you'll make it very hard for us to tell which is causing the problem by simple bisection. > Signed-off-by: Sathya Prakash <[EMAIL PROTECTED]> > --- > > diff --git a/drivers/message/fusion/mptbase.c > b/drivers/message/fusion/mptbase.c > index bfda731..b3b80bf 100644 > --- a/drivers/message/fusion/mptbase.c > +++ b/drivers/message/fusion/mptbase.c > @@ -1429,6 +1429,100 @@ mpt_get_product_name(u16 vendor, u16 device, u8 revision, char *prod_name) > sprintf(prod_name, "%s", product_str); } > > +/** > + * mpt_mapresources - map in memory mapped io > + * @ioc: Pointer to pointer to IOC adapter > + * > + **/ > +static int > +mpt_mapresources(MPT_ADAPTER *ioc) > +{ > + u8 __iomem *mem; > + int ii; > + unsigned long mem_phys; > + unsigned long port; > + u32 msize; > + u32 psize; > + u8 revision; > + int r = -ENODEV; > + struct pci_dev *pdev; > + > + pdev = ioc->pcidev; > + ioc->bars = pci_select_bars(pdev, IORESOURCE_MEM); > + if (pci_enable_device_mem(pdev)) { > + printk(MYIOC_s_ERR_FMT "pci_enable_device_mem() " > + "failed\n", ioc->name); > + return r; > + } > + if (pci_request_selected_regions(pdev, ioc->bars, "mpt")) { > + printk(MYIOC_s_ERR_FMT "pci_request_selected_regions() with " > + "MEM failed\n", ioc->name); > + return r; > + } I can't help noticing that any failure after this point leaves the driver with the selected regions requested, meaning it can never be attached again. > + pci_read_config_byte(pdev, PCI_CLASS_REVISION, &revision); > + > + if (!pci_set_dma_mask(pdev, DMA_64BIT_MASK) > + && !pci_set_consistent_dma_mask(pdev, DMA_64BIT_MASK)) { > + dinitprintk(ioc, printk(MYIOC_s_INFO_FMT > + ": 64 BIT PCI BUS DMA ADDRESSING SUPPORTED\n", > + ioc->name)); > + } else if (!pci_set_dma_mask(pdev, DMA_32BIT_MASK) > + && !pci_set_consistent_dma_mask(pdev, DMA_32BIT_MASK)) { > + dinitprintk(ioc, printk(MYIOC_s_INFO_FMT > + ": 32 BIT PCI BUS DMA ADDRESSING SUPPORTED\n", > + ioc->name)); > + } else { > + printk(MYIOC_s_WARN_FMT "no suitable DMA mask for %s\n", > + ioc->name, pci_name(pdev)); > + return r; > + } > + > + mem_phys = msize = 0; > + port = psize = 0; > + for (ii = 0; ii < DEVICE_COUNT_RESOURCE; ii++) { > + if (pci_resource_flags(pdev, ii) & PCI_BASE_ADDRESS_SPACE_IO) { > + if (psize) > + continue; > + /* Get I/O space! */ > + port = pci_resource_start(pdev, ii); > + psize = pci_resource_len(pdev, ii); > + } else { > + if (msize) > + continue; > + /* Get memmap */ > + mem_phys = pci_resource_start(pdev, ii); > + msize = pci_resource_len(pdev, ii); > + } > + } > + ioc->mem_size = msize; > + > + mem = NULL; > + /* Get logical ptr for PciMem0 space */ > + /*mem = ioremap(mem_phys, msize);*/ > + mem = ioremap(mem_phys, msize); > + if (mem == NULL) { > + printk(MYIOC_s_ERR_FMT ": ERROR - Unable to map adapter" > + " memory!\n", ioc->name); > + return -EINVAL; > + } > + ioc->memmap = mem; > + dinitprintk(ioc, printk(MYIOC_s_INFO_FMT "mem = %p, mem_phys = %lx\n", > + ioc->name, mem, mem_phys)); > + > + ioc->mem_phys = mem_phys; > + ioc->chip = (SYSIF_REGS __iomem *)mem; > + > + /* Save Port IO values in case we need to do downloadboot */ > + { > + u8 *pmem = (u8 *)port; > + ioc->pio_mem_phys = port; > + ioc->pio_chip = (SYSIF_REGS __iomem *)pmem; > + } Why on earth are you doing this; just do ioc->pio_chip = SYSIF_REGS __iomem *)port; port is an unsigned long, so it can be promoted to a pointer. > + return 0; > +} > + > > /*=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=- > =-=-=-=*/ > /** > * mpt_attach - Install a PCI intelligent MPT adapter. > @@ -1451,13 +1545,6 @@ int > mpt_attach(struct pci_dev *pdev, const struct pci_device_id *id) { > MPT_ADAPTER *ioc; > - u8 __iomem *mem; > - u8 __iomem *pmem; > - unsigned long mem_phys; > - unsigned long port; > - u32 msize; > - u32 psize; > - int ii; > u8 cb_idx; > int r = -ENODEV; > u8 revision; > @@ -1467,52 +1554,33 @@ mpt_attach(struct pci_dev *pdev, const struct pci_device_id *id) > struct proc_dir_entry *dent, *ent; > #endif > > - if (mpt_debug_level) > - printk(KERN_INFO MYNAM ": mpt_debug_level=%xh\n", mpt_debug_level); > - > ioc = kzalloc(sizeof(MPT_ADAPTER), GFP_ATOMIC); > if (ioc == NULL) { > printk(KERN_ERR MYNAM ": ERROR - Insufficient memory to add adapter!\n"); > return -ENOMEM; > } > - ioc->debug_level = mpt_debug_level; > + > ioc->id = mpt_ids++; > sprintf(ioc->name, "ioc%d", ioc->id); > + dinitprintk(ioc, printk(KERN_WARNING MYNAM ": > +mpt_adapter_install\n")); > + > + /* > + * set initial debug level > + * (refer to mptdebug.h) > + */ > + ioc->debug_level = mpt_debug_level; > + if (mpt_debug_level) > + printk(KERN_INFO "mpt_debug_level=%xh\n", mpt_debug_level); > > - ioc->bars = pci_select_bars(pdev, IORESOURCE_MEM); > - if (pci_enable_device_mem(pdev)) { > - kfree(ioc); > - printk(MYIOC_s_ERR_FMT "pci_enable_device_mem() " > - "failed\n", ioc->name); > - return r; > - } > - if (pci_request_selected_regions(pdev, ioc->bars, "mpt")) { > - kfree(ioc); > - printk(MYIOC_s_ERR_FMT "pci_request_selected_regions() with " > - "MEM failed\n", ioc->name); > - return r; > - } > > dinitprintk(ioc, printk(MYIOC_s_INFO_FMT ": mpt_adapter_install\n", > ioc->name)); > > - if (!pci_set_dma_mask(pdev, DMA_64BIT_MASK)) { > - dprintk(ioc, printk(MYIOC_s_INFO_FMT > - ": 64 BIT PCI BUS DMA ADDRESSING SUPPORTED\n", ioc->name)); > - } else if (pci_set_dma_mask(pdev, DMA_32BIT_MASK)) { > - printk(MYIOC_s_WARN_FMT ": 32 BIT PCI BUS DMA ADDRESSING NOT SUPPORTED\n", > - ioc->name); > + ioc->pcidev = pdev; > + if (mpt_mapresources(ioc)) { > kfree(ioc); > return r; > } > > - if (!pci_set_consistent_dma_mask(pdev, DMA_64BIT_MASK)) { > - dprintk(ioc, printk(MYIOC_s_INFO_FMT > - ": Using 64 bit consistent mask\n", ioc->name)); > - } else { > - dprintk(ioc, printk(MYIOC_s_INFO_FMT > - ": Not using 64 bit consistent mask\n", ioc->name)); > - } > - > ioc->alloc_total = sizeof(MPT_ADAPTER); > ioc->req_sz = MPT_DEFAULT_FRAME_SIZE; /* avoid div by zero! */ > ioc->reply_sz = MPT_REPLY_FRAME_SIZE; @@ -1550,48 +1618,9 @@ > mpt_attach(struct pci_dev *pdev, const struct pci_device_id *id) > /* Find lookup slot. */ > INIT_LIST_HEAD(&ioc->list); > > - mem_phys = msize = 0; > - port = psize = 0; > - for (ii=0; ii < DEVICE_COUNT_RESOURCE; ii++) { > - if (pci_resource_flags(pdev, ii) & PCI_BASE_ADDRESS_SPACE_IO) { > - if (psize) > - continue; > - /* Get I/O space! */ > - port = pci_resource_start(pdev, ii); > - psize = pci_resource_len(pdev,ii); > - } else { > - if (msize) > - continue; > - /* Get memmap */ > - mem_phys = pci_resource_start(pdev, ii); > - msize = pci_resource_len(pdev,ii); > - } > - } > - ioc->mem_size = msize; > - > - mem = NULL; > - /* Get logical ptr for PciMem0 space */ > - /*mem = ioremap(mem_phys, msize);*/ > - mem = ioremap(mem_phys, msize); > - if (mem == NULL) { > - printk(MYIOC_s_ERR_FMT "Unable to map adapter memory!\n", ioc->name); > - kfree(ioc); > - return -EINVAL; > - } > - ioc->memmap = mem; > - dinitprintk(ioc, printk(MYIOC_s_INFO_FMT "mem = %p, mem_phys = %lx\n", ioc->name, mem, mem_phys)); > - > dinitprintk(ioc, printk(MYIOC_s_INFO_FMT "facts @ %p, pfacts[0] @ %p\n", > ioc->name, &ioc->facts, &ioc->pfacts[0])); > > - ioc->mem_phys = mem_phys; > - ioc->chip = (SYSIF_REGS __iomem *)mem; > - > - /* Save Port IO values in case we need to do downloadboot */ > - ioc->pio_mem_phys = port; > - pmem = (u8 __iomem *)port; > - ioc->pio_chip = (SYSIF_REGS __iomem *)pmem; > - > pci_read_config_byte(pdev, PCI_CLASS_REVISION, &revision); > mpt_get_product_name(pdev->vendor, pdev->device, revision, > ioc->prod_name); > > @@ -1652,6 +1681,8 @@ mpt_attach(struct pci_dev *pdev, const struct pci_device_id *id) > case MPI_MANUFACTPAGE_DEVID_SAS1064: > case MPI_MANUFACTPAGE_DEVID_SAS1068: > ioc->errata_flag_1064 = 1; > + ioc->bus_type = SAS; > + break; This is a bit pointless, isn't it? The old code just dropped through the case to set the bus_type anyway. > case MPI_MANUFACTPAGE_DEVID_SAS1064E: > case MPI_MANUFACTPAGE_DEVID_SAS1068E: > @@ -1659,6 +1690,11 @@ mpt_attach(struct pci_dev *pdev, const struct pci_device_id *id) > ioc->bus_type = SAS; > } > > + if (ioc->bus_type == SAS) > + ioc->msi_enable = 1; > + else > + ioc->msi_enable = mpt_msi_enable; > + This would give no way to disable msi for a SAS chip ... You could do better by making mpt_msi_enable initialised to -1; then if it's still -1 you set the default (0 for non-SAS, 1 for SAS) otherwise, you know the user must have specified it, so you always take the override. > if (ioc->errata_flag_1064) > pci_disable_io_access(pdev); > > @@ -1687,7 +1723,7 @@ mpt_attach(struct pci_dev *pdev, const struct pci_device_id *id) > list_del(&ioc->list); > if (ioc->alt_ioc) > ioc->alt_ioc->alt_ioc = NULL; > - iounmap(mem); > + iounmap(ioc->memmap); > kfree(ioc); > pci_set_drvdata(pdev, NULL); > return r; > @@ -1776,20 +1812,17 @@ mpt_detach(struct pci_dev *pdev) > * mpt_suspend - Fusion MPT base driver suspend routine. > * @pdev: Pointer to pci_dev structure > * @state: new state to enter > - */ > + **/ Actually, no; docbook tags end with */ not **/ > int > mpt_suspend(struct pci_dev *pdev, pm_message_t state) { > u32 device_state; > MPT_ADAPTER *ioc = pci_get_drvdata(pdev); > > - device_state=pci_choose_state(pdev, state); > - > - printk(MYIOC_s_INFO_FMT > - "pci-suspend: pdev=0x%p, slot=%s, Entering operating state [D%d]\n", > - ioc->name, pdev, pci_name(pdev), device_state); > - > - pci_save_state(pdev); > + device_state = pci_choose_state(pdev, state); > + printk(MYIOC_s_INFO_FMT "pci-suspend: pdev=0x%p, slot=%s, Entering " > + "operating state [D%d]\n", ioc->name, pdev, pci_name(pdev), > + device_state); > > /* put ioc into READY_STATE */ > if(SendIocReset(ioc, MPI_FUNCTION_IOC_MESSAGE_UNIT_RESET, > CAN_SLEEP)) { @@ -1804,10 +1837,14 @@ mpt_suspend(struct pci_dev *pdev, pm_message_t state) > /* Clear any lingering interrupt */ > CHIPREG_WRITE32(&ioc->chip->IntStatus, 0); > > + free_irq(ioc->pci_irq, ioc); > + if (ioc->msi_enable) > + pci_disable_msi(ioc->pcidev); > + ioc->pci_irq = -1; > + pci_save_state(pdev); > pci_disable_device(pdev); > pci_release_selected_regions(pdev, ioc->bars); > pci_set_power_state(pdev, device_state); > - > return 0; > } > > @@ -1822,48 +1859,54 @@ mpt_resume(struct pci_dev *pdev) > MPT_ADAPTER *ioc = pci_get_drvdata(pdev); > u32 device_state = pdev->current_state; > int recovery_state; > + int err; > > - printk(MYIOC_s_INFO_FMT > - "pci-resume: pdev=0x%p, slot=%s, Previous operating state [D%d]\n", > - ioc->name, pdev, pci_name(pdev), device_state); > + printk(MYIOC_s_INFO_FMT "pci-resume: pdev=0x%p, slot=%s, Previous " > + "operating state [D%d]\n", ioc->name, pdev, pci_name(pdev), > + device_state); > > - pci_set_power_state(pdev, 0); > + pci_set_power_state(pdev, PCI_D0); > + pci_enable_wake(pdev, PCI_D0, 0); > pci_restore_state(pdev); > - if (ioc->facts.Flags & MPI_IOCFACTS_FLAGS_FW_DOWNLOAD_BOOT) { > - ioc->bars = pci_select_bars(ioc->pcidev, IORESOURCE_MEM | > - IORESOURCE_IO); > - if (pci_enable_device(pdev)) > - return 0; > - } else { > - ioc->bars = pci_select_bars(pdev, IORESOURCE_MEM); > - if (pci_enable_device_mem(pdev)) > - return 0; > - } > - if (pci_request_selected_regions(pdev, ioc->bars, "mpt")) > - return 0; > + ioc->pcidev = pdev; > + err = mpt_mapresources(ioc); > + if (err) > + return err; > > - /* enable interrupts */ > - CHIPREG_WRITE32(&ioc->chip->IntMask, MPI_HIM_DIM); > - ioc->active = 1; > + printk(MYIOC_s_INFO_FMT "pci-resume: ioc-state=0x%x,doorbell=0x%x\n", > + ioc->name, (mpt_GetIocState(ioc, 1) >> MPI_IOC_STATE_SHIFT), > + CHIPREG_READ32(&ioc->chip->Doorbell)); > > - printk(MYIOC_s_INFO_FMT > - "pci-resume: ioc-state=0x%x,doorbell=0x%x\n", > - ioc->name, > - (mpt_GetIocState(ioc, 1) >> MPI_IOC_STATE_SHIFT), > - CHIPREG_READ32(&ioc->chip->Doorbell)); > + /* > + * Errata workaround for SAS pci express: > + * Upon returning to the D0 state, the contents of the doorbell will be > + * stale data, and this will incorrectly signal to the host driver that > + * the firmware is ready to process mpt commands. The workaround is > + * to issue a diagnostic reset. > + */ > + if (ioc->bus_type == SAS && (pdev->device == > + MPI_MANUFACTPAGE_DEVID_SAS1068E || pdev->device == > + MPI_MANUFACTPAGE_DEVID_SAS1064E)) { > + if (KickStart(ioc, 1, CAN_SLEEP) < 0) { > + printk(MYIOC_s_WARN_FMT "pci-resume: Cannot recover\n", > + ioc->name); > + goto out; > + } > + } > > /* bring ioc to operational state */ > - if ((recovery_state = mpt_do_ioc_recovery(ioc, > - MPT_HOSTEVENT_IOC_RECOVER, CAN_SLEEP)) != 0) { > - printk(MYIOC_s_INFO_FMT > - "pci-resume: Cannot recover, error:[%x]\n", > - ioc->name, recovery_state); > - } else { > + printk(MYIOC_s_INFO_FMT "Sending mpt_do_ioc_recovery\n", ioc->name); > + recovery_state = mpt_do_ioc_recovery(ioc, MPT_HOSTEVENT_IOC_BRINGUP, > + CAN_SLEEP); > + if (recovery_state != 0) > + printk(MYIOC_s_WARN_FMT "pci-resume: Cannot recover, " > + "error:[%x]\n", ioc->name, recovery_state); > + else > printk(MYIOC_s_INFO_FMT > - "pci-resume: success\n", ioc->name); > - } > - > + "pci-resume: success\n", ioc->name); > + out: > return 0; > + > } > #endif > > @@ -2020,15 +2063,17 @@ mpt_do_ioc_recovery(MPT_ADAPTER *ioc, u32 reason, int sleepFlag) > if ((ret == 0) && (reason == MPT_HOSTEVENT_IOC_BRINGUP)) { > ioc->pci_irq = -1; > if (ioc->pcidev->irq) { > - if (mpt_msi_enable && !pci_enable_msi(ioc->pcidev)) > + if (ioc->msi_enable && !pci_enable_msi(ioc->pcidev)) > printk(MYIOC_s_INFO_FMT "PCI-MSI enabled\n", > ioc->name); > + else > + ioc->msi_enable = 0; > rc = request_irq(ioc->pcidev->irq, mpt_interrupt, > IRQF_SHARED, ioc->name, ioc); > if (rc < 0) { > printk(MYIOC_s_ERR_FMT "Unable to allocate " > "interrupt %d!\n", ioc->name, ioc->pcidev->irq); > - if (mpt_msi_enable) > + if (ioc->msi_enable) > pci_disable_msi(ioc->pcidev); > return -EBUSY; > } > @@ -2224,7 +2269,7 @@ mpt_do_ioc_recovery(MPT_ADAPTER *ioc, u32 reason, int sleepFlag) > out: > if ((ret != 0) && irq_allocated) { > free_irq(ioc->pci_irq, ioc); > - if (mpt_msi_enable) > + if (ioc->msi_enable) > pci_disable_msi(ioc->pcidev); > } > return ret; > @@ -2406,7 +2451,7 @@ mpt_adapter_dispose(MPT_ADAPTER *ioc) > > if (ioc->pci_irq != -1) { > free_irq(ioc->pci_irq, ioc); > - if (mpt_msi_enable) > + if (ioc->msi_enable) > pci_disable_msi(ioc->pcidev); > ioc->pci_irq = -1; > } > diff --git a/drivers/message/fusion/mptbase.h > b/drivers/message/fusion/mptbase.h > index d83ea96..52ef673 100644 > --- a/drivers/message/fusion/mptbase.h > +++ b/drivers/message/fusion/mptbase.h > @@ -629,7 +629,9 @@ typedef struct _MPT_ADAPTER > dma_addr_t HostPageBuffer_dma; > int mtrr_reg; > struct pci_dev *pcidev; /* struct pci_dev pointer */ > - int bars; /* bitmask of BAR's that must be configured */ > + int bars; /* bitmask of BAR's that must > + be configured */ > + int msi_enable; > u8 __iomem *memmap; /* mmap address */ > struct Scsi_Host *sh; /* Scsi Host pointer */ > SpiCfgData spi_data; /* Scsi config. data */ > diff --git a/drivers/message/fusion/mptscsih.c > b/drivers/message/fusion/mptscsih.c > index af1de0c..e509788 100644 > --- a/drivers/message/fusion/mptscsih.c > +++ b/drivers/message/fusion/mptscsih.c > @@ -1162,30 +1162,38 @@ mptscsih_shutdown(struct pci_dev *pdev) > > #ifdef CONFIG_PM > > /*=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=- > =-=-=-=*/ > -/* > +/** This isn't a docbook comment, so it shouldn't have a docbook tag (unless you want to update the comment to be a docbook conforming one). > * mptscsih_suspend - Fusion MPT scsi driver suspend routine. > * > * > - */ > + **/ And likewise, even if you update the comment to be docbook, the end tag is */ > int > mptscsih_suspend(struct pci_dev *pdev, pm_message_t state) { > + MPT_ADAPTER *ioc = pci_get_drvdata(pdev); > + > + scsi_block_requests(ioc->sh); Is this done because of some suspend problem that causes SCSI commands to be seen after suspension? If so it looks like it would be global rather than fusion specific > + flush_scheduled_work(); > mptscsih_shutdown(pdev); > return mpt_suspend(pdev,state); > } > > > /*=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=- > =-=-=-=*/ > -/* > +/** > * mptscsih_resume - Fusion MPT scsi driver resume routine. > * > * > - */ > + **/ > int > mptscsih_resume(struct pci_dev *pdev) { > - return mpt_resume(pdev); > -} > + MPT_ADAPTER *ioc = pci_get_drvdata(pdev); > + int rc; > > + rc = mpt_resume(pdev); > + scsi_unblock_requests(ioc->sh); > + return rc; > +} > #endif James - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
