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

["Narsimhulu Musini (nmusini)" <nmusini@cisco.com>]

Hi Christoph,

Thank you for reviewing the patch. Please find responses inline.
I will incorporate the comments and suggestions in next patch submittal.



On 02/04/15 2:51 pm, "Christoph Hellwig" <hch@infradead.org> wrote:

>> +/*
>> + * 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().
Sure, driver will use its own private structure.
 

> That should also avoid the need for the mempool
>that the driver currently creates.
Yes, agreed. For now, I will just consider making changes to replace
scratch buffer usage. And will take up the mempool creation in future
patches.


>
>> +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.
Sure, I will move it to ->slave_configure.

>
>> +	.cmd_per_lun = 3,
>
>why so low?
Thanks for pointing this, will set it to default queue depth.


>
>> +
>> +#ifndef PCI_VENDOR_ID_CISCO
>> +#define PCI_VENDOR_ID_CISCO     0x1137
>> +#endif
>
>Just use the numeric value directly.
Sure.
>
>> +
>> +#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.
Sure, I will directly use the APIs, and delete the snic_os.h file.


Thanks
Narsimhulu
>