Re: [PATCH v3 1/9] snic: snic module infrastructure

[Christoph Hellwig <hch@infradead.org>]

> +/*
> + * Usage of the scsi_cmnd scratchpad.
> + * These fields are locked by the hashed req_lock.
> + */
> +#define CMD_SP(Cmnd)		((Cmnd)->SCp.ptr)
> +#define CMD_STATE(Cmnd)		((Cmnd)->SCp.phase)
> +#define CMD_ABTS_STATUS(Cmnd)	((Cmnd)->SCp.Message)
> +#define CMD_LR_STATUS(Cmnd)	((Cmnd)->SCp.have_data_in)
> +#define CMD_TAG(Cmnd)		((Cmnd)->SCp.sent_command)
> +#define CMD_FLAGS(Cmnd)		((Cmnd)->SCp.Status)

Please don't abuse ->SCp for new drivers.  Declare your own structure
and set .cmd_size in the host template to it's size, then access it
using scsi_cmd_priv().  That should also avoid the need for the mempool
that the driver currently creates.

> +static int
> +snic_slave_alloc(struct scsi_device *sdev)
> +{
> +	u32 qdepth = 0, max_ios = 0;
> +	struct snic_tgt *tgt = starget_to_tgt(scsi_target(sdev));
> +
> +	if (!tgt || snic_tgt_chkready(tgt))
> +		return -ENXIO;
> +
> +	max_ios = snic_max_qdepth;
> +	max_ios = snic_max_qdepth;
> +	qdepth = min_t(u32, max_ios, SNIC_MAX_QUEUE_DEPTH);
> +	scsi_change_queue_depth(sdev, qdepth);

Plase set the queue_depth in ->slave_configure like all other drivers.

> +	.cmd_per_lun = 3,

why so low?

> +
> +#ifndef PCI_VENDOR_ID_CISCO
> +#define PCI_VENDOR_ID_CISCO     0x1137
> +#endif

Just use the numeric value directly.

> +
> +#define SNIC_OS_TYPE	SNIC_OS_LINUX
> +#define snic_cmd_tag(sc)	(((struct scsi_cmnd *) sc)->request->tag)
> +
> +extern struct device_attribute *snic_attrs[];
> +
> +static inline struct kmem_cache *
> +snic_cache_create(const char *cache_name, const int sz)
> +{
> +	void *cp;
> +
> +	cp = kmem_cache_create(cache_name, sz, SNIC_SG_DESC_ALIGN,
> +			       SLAB_HWCACHE_ALIGN, NULL);
> +
> +	return (struct kmem_cache *) cp;
> +}

Please remove all the wrappers in the snic_os.h file and use the Linux
APIs directly.