Re: [PATCH] drivers/scsi/emcctd: drivers/scsi/emcctd: Client driver implementation for EMC-Symmetrix GuestOS emulated Cut-Through Device

[Johannes Thumshirn <jthumshirn@suse.de>]

On Sat, Jan 23, 2016 at 05:33:31AM +0000, Singhal, Maneesh wrote:
> Hello Thumshirn.
> Thanks for taking out time to review the patch. I appreciate that. Please find my comments inlined.
> 

[...]

> > 
> > Wouldn't it be nice to have this in the Kconfig file? No user will ever
> > look
> > at the README file in the driver directory.
> 
> [MS>] Certainly, I will keep this README as it is (for someone who really reads this) and also add these details in Kconfig as well.
> > 

OK, I can live with this.

> > > diff --git a/drivers/scsi/emcctd/emc_ctd_interface.h
> > b/drivers/scsi/emcctd/emc_ctd_interface.h
> > > new file mode 100644
> > > index 0000000..58a0276
> > > --- /dev/null
> > > +++ b/drivers/scsi/emcctd/emc_ctd_interface.h
> > > @@ -0,0 +1,386 @@

[...]

> > > +
> > > +/* a CTD v010 scatter/gather list entry: */
> > > +struct emc_ctd_v010_sgl {
> > > +
> > > +	/* the physical address of the buffer: */
> > > +	emc_ctd_uint32_t emc_ctd_v010_sgl_paddr_0_31;
> > > +	emc_ctd_uint32_t emc_ctd_v010_sgl_paddr_32_63;
> > > +
> > > +	/* the size of the buffer: */
> > > +	emc_ctd_uint32_t emc_ctd_v010_sgl_size;
> > > +};
> > > +
> > > +/* a CTD v010 header: */
> > > +struct emc_ctd_v010_header {
> > > +
> > > +	/* the other address: */
> > > +	emc_ctd_uint16_t emc_ctd_v010_header_address;
> > > +
> > > +	/* the minor version: */
> > > +	emc_ctd_uint8_t emc_ctd_v010_header_minor;
> > > +
> > > +	/* the what: */
> > > +	emc_ctd_uint8_t emc_ctd_v010_header_what;
> > > +};
> > 
> > Well this is a matter of taste but you have (and not only in this struct,
> > just
> > an example)
> > 
> > emc_ctd_v010_header.emc_ctd_v010_header_address
> > 
> > all the emc_ctd_v010_header_ stuff is totally redundant and you
> > suffer from extremely
> > long lines in your dirver anyways. Just a hint.
> [MS>] Well, didn't actually get what you meant here, header_stuff is getting used in the code, and is extremely useful as well.
> Also, I tried reducing long lines ... I don't think the left overs could be reduced in a better way.


I'd suggest the following:

/* a CTD v010 header: */
struct emc_ctd_v010_header {

	/* the other address: */
	u16 header_address;

	/* the minor version: */
	u8 header_minor;

	/* the what: */
	u8 what;
};

and then use it like:

static void
ctd_handle_response(union emc_ctd_v010_message *io_message,
				struct ctd_pci_private *ctd_private)
{
	struct emc_ctd_v010_header *msg_header;

	msg_header = &io_message->emc_ctd_v010_message_header;

	switch (msg_header->what) {

	case EMC_CTD_V010_WHAT_DETECT:
		ctd_handle_detect((struct emc_ctd_v010_detect *)io_message,
						ctd_private);

All the "emc_ctd_v010_header_" is unneeded redundant information, that doesn't
really solve a purpose in my opinion.

> > 
> > > +
> > > +/* a CTD v010 name: */
> > > +struct emc_ctd_v010_name {
> > > +
> > > +	/* the name: */
> > > +	emc_ctd_uint8_t emc_ctd_v010_name_bytes[8];
> > > +};
> > > +
> > > +/* a CTD v010 detect message: */
> > > +struct emc_ctd_v010_detect {
> > > +
> > > +	/* the header: */
> > > +	struct emc_ctd_v010_header emc_ctd_v010_detect_header;
> > > +
> > > +	/* the flags: */
> > > +	emc_ctd_uint32_t emc_ctd_v010_detect_flags;
> > > +
> > > +	/* the name: */
> > > +	struct emc_ctd_v010_name emc_ctd_v010_detect_name;
> > > +
> > > +	/* the key: */
> > > +	emc_ctd_uint64_t emc_ctd_v010_detect_key;
> > > +};
> > > +

[...]

> > > +
> > > +/* nomenclature for versioning
> > > + * MAJOR:MINOR:SUBVERSION:PATCH
> > > + */
> > > +
> > > +#define EMCCTD_MODULE_VERSION "2.0.0.24"
> > > +
> > > +MODULE_LICENSE("GPL");
> > > +MODULE_AUTHOR("EMC");
> > > +MODULE_DESCRIPTION("EMC CTD V1 - Build 18-Jan-2016");
> > 
> > This is very misleading. If I was a user I'd think the kernel was build on
> > 18-Jan-2016. Anyways The version should be enough.
> [MS>] I actually wanted to understand when this driver was last touched, and hence this description was added.
> > 

If you insist.

> > > +MODULE_VERSION(EMCCTD_MODULE_VERSION);
> > > +

[...]

> > > +#define ctd_dprintk(__m_fmt, ...)		\
> > > +do {				\
> > > +	if (ctd_debug)		\
> > > +		pr_info("%s:%d:"__m_fmt, __func__, __LINE__,
> > ##__VA_ARGS__); \
> > > +} while (0)
> > 
> > Please use pr_debug() here.
> [MS>] Sure.
> > 
> > > +
> > > +#define ctd_dprintk_crit(__m_fmt, ...)		\
> > > +		pr_crit("%s:%d:"__m_fmt, __func__, __LINE__,
> > ##__VA_ARGS__)
> > 
> > File and line information is probably not of any interest for the users
> > and
> > serves a debugging purpose only.
> [MS>] That's precisely why it is there...

Then please use the kernel's dynamic debug facility.

Thanks,
	Johannes
-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850