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@xxxxxxxxxxxxx> 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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
