On Thu, Apr 16, 2009 at 1:37 AM, Desai, Kashyap <Kashyap.Desai@xxxxxxx> wrote: > Grant, > > For your comment #1, Print is just extra debug print. > I am agreeing with your concern. I will revert back that particular print, since it is duplicate information. > > For your comment #2, Black space will be removed in next patch set. thanks - sounds good (for #1 and 2) > > For your comment #3, it is just code readability purpose. I would like to keep that piece of code, since the new way is more readable. I can revert back that particular piece of code, if it is strongly recommended. No - either way is fine coding wise. I just wanted to know if there was a specific incident. Generally, unless there is cause, I don't like to touch code. "I clearly like it better this way" is sufficient "cause" for me. thanks, grant > > > Thanks, > Kashyap > > > -----Original Message----- > From: Grant Grundler [mailto:grundler@xxxxxxxxxx] > Sent: Thursday, April 16, 2009 12:22 PM > To: Desai, Kashyap > Cc: linux-scsi@xxxxxxxxxxxxxxx; James.Bottomley@xxxxxxxxxxxxxxxxxxxxx; Moore, Eric; Prakash, Sathya > Subject: Re: [PATCH 1/1mpt fusion:C] Code cleanup patch > > On Wed, Apr 15, 2009 at 4:10 AM, Kashyap, Desai <kashyap.desai@xxxxxxx> wrote: >> Main goal to submit this patch is code cleaup. >> 1. Better driver debug prints and code indentation. > > A few silly nits about the new printk's, etc. > > > ... >> @@ -1596,6 +1600,7 @@ mpt_mapresources(MPT_ADAPTER *ioc) >> u8 revision; >> int r = -ENODEV; >> struct pci_dev *pdev; >> + struct sysinfo s; >> >> pdev = ioc->pcidev; >> ioc->bars = pci_select_bars(pdev, IORESOURCE_MEM); >> @@ -1647,6 +1652,11 @@ mpt_mapresources(MPT_ADAPTER *ioc) >> } >> } >> >> + si_meminfo(&s); >> + printk(MYIOC_s_INFO_FMT "%s BIT PCI BUS DMA ADDRESSING SUPPORTED, " >> + "total memory = %ld kB\n", >> + ioc->name, ioc->dma_mask == DMA_64BIT_MASK ? "64" : "32", >> + convert_to_kb(s.totalram)); > > 1) Is printing one line for dma_mask really necessary? > Maybe consolidate this onto another line of output. Boot output is > already too verbose. > > 2) Isn't the "total Ram" printed elsewhere at boot time? > Will this get printed once for each MPT Adapter in the system? > > Lastly, it's not the total RAM that matters, it depends on *where* > in the physical address space that RAM is located. I'm sure there > are better ways to get that than by a device driver printing it. > > We have lots of systems with 4G of RAM and some major chunk > of that is above 4GB addresss. I forget if it's 1GB or 2GB on > x86 systems. That is to give more MMIO address space to > PCI devices that want to live below 4GB phys address and > possibly use by IOMMUs (I don't know that for x86 - it was > the case for HP IA64 systems). > > >> @@ -1768,7 +1779,7 @@ mpt_attach(struct pci_dev *pdev, const struct pci_device_id *id) >> ioc->reply_sz = MPT_REPLY_FRAME_SIZE; >> >> ioc->pcidev = pdev; >> - spin_lock_init(&ioc->initializing_hba_lock); >> + >> >> spin_lock_init(&ioc->taskmgmt_lock); >> mutex_init(&ioc->internal_cmds.mutex); > > > Extra blank line needed? Could probably be dropped. > >> @@ -1789,6 +1800,7 @@ mpt_attach(struct pci_dev *pdev, const struct pci_device_id *id) >> ioc->mfcnt = 0; >> #endif >> >> + ioc->sh = NULL; >> ioc->cached_fw = NULL; >> >> /* Initilize SCSI Config Data structure >> @@ -1805,9 +1817,8 @@ mpt_attach(struct pci_dev *pdev, const struct pci_device_id *id) >> >> /* Initialize workqueue */ >> INIT_DELAYED_WORK(&ioc->fault_reset_work, mpt_fault_reset_work); >> - spin_lock_init(&ioc->fault_reset_work_lock); >> >> - snprintf(ioc->reset_work_q_name, sizeof(ioc->reset_work_q_name), >> + snprintf(ioc->reset_work_q_name, MPT_KOBJ_NAME_LEN, >> "mpt_poll_%d", ioc->id); >> ioc->reset_work_q = >> create_singlethread_workqueue(ioc->reset_work_q_name); >> @@ -1882,11 +1893,14 @@ 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; > > Why not Just let it fall through to the next case? > Did someone have problems reading that code? > >> >> case MPI_MANUFACTPAGE_DEVID_SAS1064E: >> case MPI_MANUFACTPAGE_DEVID_SAS1068E: >> case MPI_MANUFACTPAGE_DEVID_SAS1078: >> ioc->bus_type = SAS; >> + break; >> } >> >> > > 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
