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

[Rolf Eike Beer <eike-kernel@sf-tec.de>]

[-- 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 --]