netdev - Re: Subject: [PATCH 2/6] bna: Brocade 10Gb Ethernet device driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Sat, 19 Dec 2009 02:19:09 +0000
From:	Ben Hutchings <bhutchings@...arflare.com>
To:	Debashis Dutt <ddutt@...cade.com>
Cc:	netdev@...r.kernel.org, adapter_linux_open_src_team@...cade.com
Subject: Re: Subject: [PATCH 2/6] bna: Brocade 10Gb Ethernet device driver

On Fri, 2009-12-18 at 17:28 -0800, Debashis Dutt wrote:
[...]
> diff -ruP net-next-2.6-orig/drivers/net/bna/bfad_fwimg.c net-next-2.6-mod/drivers/net/bna/bfad_fwimg.c
> --- net-next-2.6-orig/drivers/net/bna/bfad_fwimg.c      1969-12-31 16:00:00.000000000 -0800
> +++ net-next-2.6-mod/drivers/net/bna/bfad_fwimg.c       2009-12-18 16:53:40.000000000 -0800
[...]
> +u32 bfi_image_ct_size;
> +u32 bfi_image_cb_size;
> +u32 *bfi_image_ct;
> +u32 *bfi_image_cb;
> +
> +#define	BFAD_FW_FILE_CT	"ctfw.bin"
> +#define	BFAD_FW_FILE_CB	"cbfw.bin"

You should also use the MODULE_FIRMWARE() macro to add references to
these files in the module.  That enables distribution scripts to tell
which firmware files need to be installed on the system or included in
an initramfs.

> +u32 *
> +bfad_read_firmware(struct pci_dev *pdev, u32 **bfi_image,
> +			u32 *bfi_image_size, char *fw_name)
> +{
> +	const struct firmware *fw;
> +
> +	if (request_firmware(&fw, fw_name, &pdev->dev)) {
> +		printk(KERN_ALERT "Can't locate firmware %s\n", fw_name);
> +		goto error;
> +	}
> +
> +	*bfi_image = vmalloc(fw->size);
> +	if (NULL == *bfi_image) {
> +		printk(KERN_ALERT "Fail to allocate buffer for fw image "
> +			"size=%x!\n", (u32) fw->size);
> +		goto error;
> +	}
> +
> +	memcpy(*bfi_image, fw->data, fw->size);

What is the point of making a copy?

> +	*bfi_image_size = fw->size/sizeof(u32);
> +
> +	return *bfi_image;
> +
> +error:
> +	return NULL;
> +}

Are you ever going to free any of these firmware images?

> +u32 *
> +bfad_get_firmware_buf(struct pci_dev *pdev)
> +{
> +	if (pdev->device == BFA_PCI_DEVICE_ID_CT) {
> +		if (bfi_image_ct_size == 0)
> +			bfad_read_firmware(pdev, &bfi_image_ct,
> +				&bfi_image_ct_size, BFAD_FW_FILE_CT);
> +		return bfi_image_ct;
> +	} else {
> +		if (bfi_image_cb_size == 0)
> +			bfad_read_firmware(pdev, &bfi_image_cb,
> +				&bfi_image_cb_size, BFAD_FW_FILE_CB);
> +		return bfi_image_cb;
> +	}
> +}
[...]

PCI devices can be probed in parallel, so this must use a mutex.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html