RE: [PATCH 1/1mpt fusion:C] Code cleanup patch

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.

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.


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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [SCSI Target Devel]     [Linux SCSI Target Infrastructure]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Linux IIO]     [Samba]     [Device Mapper]
  Powered by Linux