Re: [PATCH] SCSI driver for VMware's virtual HBA - V3.

[Rolf Eike Beer]

Alok Kataria wrote:
> VMware PVSCSI driver - v3.
> 
> +static void pvscsi_write_cmd_desc(const struct pvscsi_adapter *adapter,
> +				  u32 cmd, const void *desc, size_t len)
> +{
> +	const u32 *ptr = desc;
> +	unsigned i;
> +
> +	len /= sizeof(*ptr);
> +	pvscsi_reg_write(adapter, PVSCSI_REG_OFFSET_COMMAND, cmd);
> +	for (i = 0; i < len; i++)

This would limit len to 16 GiB as "unsigned" (looks like "unsigned int" is 
preferred in kernel) is only 32 bit long. Instead i should probably be size_t, 
too.

> +static int __devinit pvscsi_allocate_rings(struct pvscsi_adapter *adapter)
> +{

...

> +	BUG_ON(adapter->ringStatePA & ~PAGE_MASK);
> +	BUG_ON(adapter->reqRingPA   & ~PAGE_MASK);
> +	BUG_ON(adapter->cmpRingPA   & ~PAGE_MASK);

Ok, that took me a moment. How about BUG_ON(!IS_ALIGNED(foo, PAGE_SIZE))?

> +static const char *pvscsi_info(struct Scsi_Host *host)
> +{
> +	struct pvscsi_adapter *adapter = shost_priv(host);
> +	static char buf[512];
> +
> +	sprintf(buf, "VMware PVSCSI storage adapter rev %d, req/cmp/msg rings: "
> +		"%u/%u/%u pages, cmd_per_lun=%u", adapter->rev,
> +		adapter->req_pages, adapter->cmp_pages, adapter->msg_pages,
> +		pvscsi_template.cmd_per_lun);

The buffer looks rather huge here. I'm too lazy to calculate an exact size but 
256 should still be more than enough, no?

> +static void pvscsi_shutdown_intr(struct pvscsi_adapter *adapter)
> +{
> +	if (adapter->irq) {
> +		free_irq(adapter->irq, adapter);
> +		adapter->irq = 0;
> +	}
> +	if (adapter->use_msi) {
> +		pci_disable_msi(adapter->dev);
> +		adapter->use_msi = 0;
> +	}
> +
> +	if (adapter->use_msix) {
> +		pci_disable_msix(adapter->dev);
> +		adapter->use_msix = 0;
> +	}
> +}

If MSI and MSI-X are exclusive the last one could be "else if".

Greetings,

Eike

Attachment: signature.asc
Description: This is a digitally signed message part.