Re: [PATCH 1/6] RFC: beiscsi : handles core routines

Rolf Eike Beer
2009-08-13T10:09:52Z

[-- Attachment #1: Type: Text/Plain, Size: 1779 bytes --]

Jayamohan Kallickal wrote:
> This file handles initiallization/teardown, allocation/free as well
> as IO/Management flows.
>
> Signed-off-by: Jayamohan Kallickal <jayamohank@serverengines.com>

> +struct beiscsi_hba *beiscsi_hba_alloc(struct pci_dev *pcidev)
> +{
[...]
> + phba = iscsi_host_priv(shost);
> + memset(phba, 0x0, sizeof(*phba));
> + phba->shost = shost;
> + pci_dev_get(pcidev);
> +
> + phba->shost = shost;
> + phba->pcidev = pcidev;

You initialized shost 3 lines before. Additionally you can simply say

phba->pcidev = pci_dev_get(pcidev);

> +int beiscsi_enable_pci(struct pci_dev *pcidev)
> +{
> + if (pci_enable_device(pcidev)) {
> + dev_err(&pcidev->dev, "beiscsi_enable_pci - enable device "
> + "failed. Returning -ENODEV\n");
> + return -ENODEV;
> + }

You should forward the error code of pci_enable_device() here.

> + if (pci_set_consistent_dma_mask(pcidev, DMA_BIT_MASK(64))) {
> + if (pci_set_consistent_dma_mask(pcidev, DMA_BIT_MASK(32))) {
> + dev_err(&pcidev->dev, "Could not set PCI DMA Mask\n");
> + return -ENODEV;
> + }
> + }
> + return 0;
> +}

This function is only called from one place. You should move it before that
caller, remove it from the header file and mark it static.

> +static int beiscsi_alloc_mem(struct beiscsi_hba *phba)
> +{
> + struct be_mem_descriptor *mem_descr;
> + dma_addr_t bus_add;
> + unsigned int num_size, i, j;
> + phba->phwi_ctrlr = kmalloc(phba->params.hwi_ws_sz, GFP_KERNEL);
> + if (!phba->phwi_ctrlr)
> + return -ENOMEM;
> + phba->init_mem =
> + kzalloc(sizeof(struct be_mem_descriptor) * SE_MEM_MAX, GFP_KERNEL);

kcalloc()

There is also a typo (found at least 5 places): Initiallize has one 'l' too
much.

Greetings,

Eike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]