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

2016-01-25 Thread Singhal, Maneesh
Thanks a lot for detailed information. Appreciate it !

Regards
Maneesh

> -Original Message-
> From: Johannes Thumshirn [mailto:jthumsh...@suse.de]
> Sent: Monday, January 25, 2016 3:39 PM
> To: Singhal, Maneesh
> Cc: linux-s...@vger.kernel.org; linux-kernel@vger.kernel.org;
> jbottom...@odin.com; martin.peter...@oracle.com; linux-
> a...@vger.kernel.org
> Subject: Re: [PATCH] drivers/scsi/emcctd: drivers/scsi/emcctd: Client
> driver implementation for EMC-Symmetrix GuestOS emulated Cut-
> Through Device
> 
> On Mon, Jan 25, 2016 at 09:30:47AM +, Singhal, Maneesh wrote:
> > Thanks Johannes for generously reviewing my patch. It indeed is
> going to improve the thing a lot. I agree with all your comments so far,
> and will submit a newer patch soon.
> > Since this is my first patch, I have couple of questions regarding
> submitting the patch:
> >
> > 1.  Is there a page/place where I can see the review comments
> rather neatly ?, finding out all the comments and infact reading to
> entire code on text file is cumbersome. If there is no way, then its
> fine,... I can live with it.
> 
> Nope unfortunately not. That's why it's nice when people trim their e-
> mails
> ;-)
> 
> > 2. When I address the review comments and want to resubmit the
> patch, do I need to submit the incremental patch or the entire code
> patch again ? git-format-patch gives me incremental patch only.
> 
> Ususally you change your code and do a git commit --amend. This
> modifies your commit instead of creating a new one. If you already
> have a new commit than you can use git rebase -i  commit to amend> and squash the commit's into one (git gives you a
> nice readme when doing rebase -i). Afterwards you do a git format-
> patch -1 -v 2, this creates a v2-0001-your-patch-name.patch for git
> send-email which has an updated [PATCH V2] in the subject.
> 
> At least, the above is the way I do it (i.e. there are more than one ways
> to do it).
> 
> 
> --
> Johannes Thumshirn  Storage
> jthumsh...@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


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

2016-01-25 Thread Johannes Thumshirn
On Mon, Jan 25, 2016 at 09:30:47AM +, Singhal, Maneesh wrote:
> Thanks Johannes for generously reviewing my patch. It indeed is going to 
> improve the thing a lot. I agree with all your comments so far, and will 
> submit a newer patch soon. 
> Since this is my first patch, I have couple of questions regarding submitting 
> the patch:
> 
> 1.  Is there a page/place where I can see the review comments rather neatly 
> ?, finding out all the comments and infact reading to entire code on text 
> file is cumbersome. If there is no way, then its fine,... I can live with it.

Nope unfortunately not. That's why it's nice when people trim their e-mails
;-)

> 2. When I address the review comments and want to resubmit the patch, do I 
> need to submit the incremental patch or the entire code patch again ? 
> git-format-patch gives me incremental patch only.

Ususally you change your code and do a git commit --amend. This modifies your
commit instead of creating a new one. If you already have a new commit than
you can use git rebase -i  and squash the
commit's into one (git gives you a nice readme when doing rebase -i). Afterwards
you do a git format-patch -1 -v 2, this creates a v2-0001-your-patch-name.patch
for git send-email which has an updated [PATCH V2] in the subject.

At least, the above is the way I do it (i.e. there are more than one ways to
do it).


-- 
Johannes Thumshirn  Storage
jthumsh...@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


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

2016-01-25 Thread Singhal, Maneesh
Thanks Johannes for generously reviewing my patch. It indeed is going to 
improve the thing a lot. I agree with all your comments so far, and will submit 
a newer patch soon. 
Since this is my first patch, I have couple of questions regarding submitting 
the patch:

1.  Is there a page/place where I can see the review comments rather neatly ?, 
finding out all the comments and infact reading to entire code on text file is 
cumbersome. If there is no way, then its fine,... I can live with it.
2. When I address the review comments and want to resubmit the patch, do I need 
to submit the incremental patch or the entire code patch again ? 
git-format-patch gives me incremental patch only.

Thanks
Maneesh

> -Original Message-
> From: Johannes Thumshirn [mailto:jthumsh...@suse.de]
> Sent: Monday, January 25, 2016 2:56 PM
> To: Singhal, Maneesh
> Cc: linux-s...@vger.kernel.org; linux-kernel@vger.kernel.org;
> jbottom...@odin.com; martin.peter...@oracle.com; linux-
> a...@vger.kernel.org
> Subject: Re: [PATCH] drivers/scsi/emcctd: drivers/scsi/emcctd: Client
> driver implementation for EMC-Symmetrix GuestOS emulated Cut-
> Through Device
> 
> On Sat, Jan 23, 2016 at 05:51:50AM +, Singhal, Maneesh wrote:
> > Thanks for your time. My replies inlined...
> >
> 
> [...]
> 
> > > > +   }
> > > > +
> > >
> > > You don't do any cleanup work at
> > > ctd_scsi_response_sanity_check_complete. You could just reutrn 0
> > > here as well.
> > [MS>] Just avoiding multiple exit points from the function
> 
> Please have a look at Documentation/CodingStyle Chapter 7:
> Centralized exiting of functions. Especially this part:
> 
> 
> The goto statement comes in handy when a function exits from
> multiple locations and some common work such as cleanup has to be
> done.  If there is no cleanup needed then just return directly.
> 
> 
> and the example below that section.
> 
> > >
> > > > +
> > > > +   ctd_dprintk_crit(
> 
> [...]
> 
> > > > +   if (request && request->io_timeout < EMCCTD_MAX_RETRY) {
> > >
> > > The following block is a bit long and therefore it's hard to spot an
> > > eventual error of handling the io_mgmt_lock. Can't it be factored
> > > out in a helper function?
> > >
> > [MS>] I know its little longer, but actually fits logically together... Will
> see if it can be factored. Not sure though.
> 
> Yes please. It's not uber important but short functions and paths are
> generally favoured when debugging and reviewing code.
> 
> 
> Thanks,
>   Johannes
> 
> --
> Johannes Thumshirn  Storage
> jthumsh...@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


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

2016-01-25 Thread Johannes Thumshirn
On Sat, Jan 23, 2016 at 05:51:50AM +, Singhal, Maneesh wrote:
> Thanks for your time. My replies inlined...
> 

[...]

> > > + }
> > > +
> > 
> > You don't do any cleanup work at
> > ctd_scsi_response_sanity_check_complete. You
> > could just reutrn 0 here as well. 
> [MS>] Just avoiding multiple exit points from the function

Please have a look at Documentation/CodingStyle Chapter 7: Centralized exiting 
of functions. Especially this part:


The goto statement comes in handy when a function exits from multiple
locations and some common work such as cleanup has to be done.  If there is no
cleanup needed then just return directly.


and the example below that section.

> > 
> > > +
> > > + ctd_dprintk_crit(

[...]

> > > + if (request && request->io_timeout < EMCCTD_MAX_RETRY) {
> > 
> > The following block is a bit long and therefore it's hard to spot an
> > eventual
> > error of handling the io_mgmt_lock. Can't it be factored out in a helper
> > function?
> > 
> [MS>] I know its little longer, but actually fits logically together... Will 
> see if it can be factored. Not sure though.

Yes please. It's not uber important but short functions and paths are
generally favoured when debugging and reviewing code.


Thanks,
Johannes

-- 
Johannes Thumshirn  Storage
jthumsh...@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


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

2016-01-25 Thread Johannes Thumshirn
On Sat, Jan 23, 2016 at 05:33:31AM +, 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 000..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 = _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
jthumsh...@suse.de+49 911 74053 689
SUSE 

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

2016-01-25 Thread Johannes Thumshirn
On Sat, Jan 23, 2016 at 05:33:31AM +, 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 000..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 = _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
jthumsh...@suse.de+49 911 74053 689
SUSE 

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

2016-01-25 Thread Johannes Thumshirn
On Sat, Jan 23, 2016 at 05:51:50AM +, Singhal, Maneesh wrote:
> Thanks for your time. My replies inlined...
> 

[...]

> > > + }
> > > +
> > 
> > You don't do any cleanup work at
> > ctd_scsi_response_sanity_check_complete. You
> > could just reutrn 0 here as well. 
> [MS>] Just avoiding multiple exit points from the function

Please have a look at Documentation/CodingStyle Chapter 7: Centralized exiting 
of functions. Especially this part:


The goto statement comes in handy when a function exits from multiple
locations and some common work such as cleanup has to be done.  If there is no
cleanup needed then just return directly.


and the example below that section.

> > 
> > > +
> > > + ctd_dprintk_crit(

[...]

> > > + if (request && request->io_timeout < EMCCTD_MAX_RETRY) {
> > 
> > The following block is a bit long and therefore it's hard to spot an
> > eventual
> > error of handling the io_mgmt_lock. Can't it be factored out in a helper
> > function?
> > 
> [MS>] I know its little longer, but actually fits logically together... Will 
> see if it can be factored. Not sure though.

Yes please. It's not uber important but short functions and paths are
generally favoured when debugging and reviewing code.


Thanks,
Johannes

-- 
Johannes Thumshirn  Storage
jthumsh...@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


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

2016-01-25 Thread Singhal, Maneesh
Thanks Johannes for generously reviewing my patch. It indeed is going to 
improve the thing a lot. I agree with all your comments so far, and will submit 
a newer patch soon. 
Since this is my first patch, I have couple of questions regarding submitting 
the patch:

1.  Is there a page/place where I can see the review comments rather neatly ?, 
finding out all the comments and infact reading to entire code on text file is 
cumbersome. If there is no way, then its fine,... I can live with it.
2. When I address the review comments and want to resubmit the patch, do I need 
to submit the incremental patch or the entire code patch again ? 
git-format-patch gives me incremental patch only.

Thanks
Maneesh

> -Original Message-
> From: Johannes Thumshirn [mailto:jthumsh...@suse.de]
> Sent: Monday, January 25, 2016 2:56 PM
> To: Singhal, Maneesh
> Cc: linux-s...@vger.kernel.org; linux-kernel@vger.kernel.org;
> jbottom...@odin.com; martin.peter...@oracle.com; linux-
> a...@vger.kernel.org
> Subject: Re: [PATCH] drivers/scsi/emcctd: drivers/scsi/emcctd: Client
> driver implementation for EMC-Symmetrix GuestOS emulated Cut-
> Through Device
> 
> On Sat, Jan 23, 2016 at 05:51:50AM +, Singhal, Maneesh wrote:
> > Thanks for your time. My replies inlined...
> >
> 
> [...]
> 
> > > > +   }
> > > > +
> > >
> > > You don't do any cleanup work at
> > > ctd_scsi_response_sanity_check_complete. You could just reutrn 0
> > > here as well.
> > [MS>] Just avoiding multiple exit points from the function
> 
> Please have a look at Documentation/CodingStyle Chapter 7:
> Centralized exiting of functions. Especially this part:
> 
> 
> The goto statement comes in handy when a function exits from
> multiple locations and some common work such as cleanup has to be
> done.  If there is no cleanup needed then just return directly.
> 
> 
> and the example below that section.
> 
> > >
> > > > +
> > > > +   ctd_dprintk_crit(
> 
> [...]
> 
> > > > +   if (request && request->io_timeout < EMCCTD_MAX_RETRY) {
> > >
> > > The following block is a bit long and therefore it's hard to spot an
> > > eventual error of handling the io_mgmt_lock. Can't it be factored
> > > out in a helper function?
> > >
> > [MS>] I know its little longer, but actually fits logically together... Will
> see if it can be factored. Not sure though.
> 
> Yes please. It's not uber important but short functions and paths are
> generally favoured when debugging and reviewing code.
> 
> 
> Thanks,
>   Johannes
> 
> --
> Johannes Thumshirn  Storage
> jthumsh...@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


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

2016-01-25 Thread Johannes Thumshirn
On Mon, Jan 25, 2016 at 09:30:47AM +, Singhal, Maneesh wrote:
> Thanks Johannes for generously reviewing my patch. It indeed is going to 
> improve the thing a lot. I agree with all your comments so far, and will 
> submit a newer patch soon. 
> Since this is my first patch, I have couple of questions regarding submitting 
> the patch:
> 
> 1.  Is there a page/place where I can see the review comments rather neatly 
> ?, finding out all the comments and infact reading to entire code on text 
> file is cumbersome. If there is no way, then its fine,... I can live with it.

Nope unfortunately not. That's why it's nice when people trim their e-mails
;-)

> 2. When I address the review comments and want to resubmit the patch, do I 
> need to submit the incremental patch or the entire code patch again ? 
> git-format-patch gives me incremental patch only.

Ususally you change your code and do a git commit --amend. This modifies your
commit instead of creating a new one. If you already have a new commit than
you can use git rebase -i  and squash the
commit's into one (git gives you a nice readme when doing rebase -i). Afterwards
you do a git format-patch -1 -v 2, this creates a v2-0001-your-patch-name.patch
for git send-email which has an updated [PATCH V2] in the subject.

At least, the above is the way I do it (i.e. there are more than one ways to
do it).


-- 
Johannes Thumshirn  Storage
jthumsh...@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


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

2016-01-25 Thread Singhal, Maneesh
Thanks a lot for detailed information. Appreciate it !

Regards
Maneesh

> -Original Message-
> From: Johannes Thumshirn [mailto:jthumsh...@suse.de]
> Sent: Monday, January 25, 2016 3:39 PM
> To: Singhal, Maneesh
> Cc: linux-s...@vger.kernel.org; linux-kernel@vger.kernel.org;
> jbottom...@odin.com; martin.peter...@oracle.com; linux-
> a...@vger.kernel.org
> Subject: Re: [PATCH] drivers/scsi/emcctd: drivers/scsi/emcctd: Client
> driver implementation for EMC-Symmetrix GuestOS emulated Cut-
> Through Device
> 
> On Mon, Jan 25, 2016 at 09:30:47AM +, Singhal, Maneesh wrote:
> > Thanks Johannes for generously reviewing my patch. It indeed is
> going to improve the thing a lot. I agree with all your comments so far,
> and will submit a newer patch soon.
> > Since this is my first patch, I have couple of questions regarding
> submitting the patch:
> >
> > 1.  Is there a page/place where I can see the review comments
> rather neatly ?, finding out all the comments and infact reading to
> entire code on text file is cumbersome. If there is no way, then its
> fine,... I can live with it.
> 
> Nope unfortunately not. That's why it's nice when people trim their e-
> mails
> ;-)
> 
> > 2. When I address the review comments and want to resubmit the
> patch, do I need to submit the incremental patch or the entire code
> patch again ? git-format-patch gives me incremental patch only.
> 
> Ususally you change your code and do a git commit --amend. This
> modifies your commit instead of creating a new one. If you already
> have a new commit than you can use git rebase -i  commit to amend> and squash the commit's into one (git gives you a
> nice readme when doing rebase -i). Afterwards you do a git format-
> patch -1 -v 2, this creates a v2-0001-your-patch-name.patch for git
> send-email which has an updated [PATCH V2] in the subject.
> 
> At least, the above is the way I do it (i.e. there are more than one ways
> to do it).
> 
> 
> --
> Johannes Thumshirn  Storage
> jthumsh...@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


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

2016-01-22 Thread Singhal, Maneesh
Hello Greg,

Thanks for taking out time to review the patch. Please find my replies 
inlined...
Will post the next patch for modifications soon.

> -Original Message-
> From: Greg KH [mailto:g...@kroah.com]
> Sent: Tuesday, January 19, 2016 11:42 PM
> To: Singhal, Maneesh
> Cc: linux-s...@vger.kernel.org; linux-kernel@vger.kernel.org;
> jbottom...@odin.com; martin.peter...@oracle.com; linux-
> a...@vger.kernel.org
> Subject: Re: [PATCH] drivers/scsi/emcctd: drivers/scsi/emcctd: Client
> driver implementation for EMC-Symmetrix GuestOS emulated Cut-
> Through Device
> 
> On Tue, Jan 19, 2016 at 11:58:06AM +, Singhal, Maneesh wrote:
> > Hello,
> > Kindly review the following patch for the following driver to be
> added
> > in SCSI subsystem -
> >
> > Regards
> > Maneesh
> >
> > --
> > --
> > >From f3c4b836d6f130b1d7ded618002c8164f8f4a06d Mon Sep 17
> 00:00:00
> > >2001
> > From: "maneesh.singhal" 
> > Date: Tue, 19 Jan 2016 06:39:35 -0500
> > Subject: [PATCH] [PATCH] drivers/scsi/emcctd: Client driver
> > implementation for  EMC-Symmetrix GuestOS emulated Cut-
> Through Device.
> >
> > The patch is a driver implementation  EMC-Symmetrix GuestOS
> emulated
> > Cut-Through Device. The Cut-Through Device PCI emulation is
> > implemented for GuestOS environments in the HyperMax OS.
> GuestOS
> > environments allows loading of any x86 compliant operating systems
> like Linux/FreeBSD etc.
> >
> > The client driver is a SCSI HBA implementation which interfaces with
> > SCSI midlayer in the north-bound interfaces and connects with the
> > emulated PCI device on the south side.
> >
> > The PCI vendor ID:product ID for emulated Cut-Through Device is
> 0x1120:0x1B00.
> >
> > Signed-off-by: maneesh.singhal 
> > ---
> >  Documentation/scsi/emcctd.txt   |   57 +
> >  MAINTAINERS |9 +
> >  drivers/scsi/Kconfig|1 +
> >  drivers/scsi/Makefile   |1 +
> >  drivers/scsi/emcctd/Kconfig |7 +
> >  drivers/scsi/emcctd/Makefile|1 +
> >  drivers/scsi/emcctd/README  |   10 +
> >  drivers/scsi/emcctd/emc_ctd_interface.h |  386 +
> >  drivers/scsi/emcctd/emcctd.c| 2840
> +++
> >  drivers/scsi/emcctd/emcctd.h|  232 +++
> >  10 files changed, 3544 insertions(+)
> >  create mode 100644 Documentation/scsi/emcctd.txt  create mode
> 100644
> > drivers/scsi/emcctd/Kconfig  create mode 100644
> > drivers/scsi/emcctd/Makefile  create mode 100644
> > drivers/scsi/emcctd/README  create mode 100644
> > drivers/scsi/emcctd/emc_ctd_interface.h
> >  create mode 100644 drivers/scsi/emcctd/emcctd.c  create mode
> 100644
> > drivers/scsi/emcctd/emcctd.h
> >
> > diff --git a/Documentation/scsi/emcctd.txt
> > b/Documentation/scsi/emcctd.txt new file mode 100644 index
> > 000..bcafc87
> > --- /dev/null
> > +++ b/Documentation/scsi/emcctd.txt
> > @@ -0,0 +1,56 @@
> > +This file contains brief information about the EMC Cut-Through
> Driver (emcctd).
> > +The driver is currently maintained by Singhal, Maneesh
> > +(maneesh.sing...@emc.com)
> > +
> > +Last modified: Mon Jan 18 2016 by Maneesh Singhal
> > +
> > +BASICS
> > +
> > +Its a client driver implementation for EMC-Symmetrix GuestOS
> emulated
> > +Cut-Through Device. The Cut-Through Device PCI emulation is
> > +implemented for GuestOS environments in the HyperMax OS.
> GuestOS
> > +environments allows loading of any x86 compliant operating
> systems like Linux/FreeBSD etc.
> > +
> > +The client driver is a SCSI HBA implementation which interfaces with
> > +SCSI midlayer in the north-bound interfaces and connects with the
> > +emulated PCI device on the south side.
> > +
> > +The PCI vendor ID:product ID for emulated Cut-Through Device is
> 0x1120:0x1B00.
> > +
> > +VERSIONING
> > +
> > +The Version of the driver is maintained as 2.0.0.X, where 2 refers to
> > +the CTD protocol in use, and X refers to the ongoing version of the
> driver.
> > +
> > +
> > +SYSFS SUPPORT
> > +
> > +The driver creates the directory /sys/module/emcctd and
> populates it
> > +with version file and a directory for various parameters as described
> > +in MODULE PARAMETERS section.
> > +
> > +PROCFS SUPPORT
> > +
> > +The driver creates the d

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

2016-01-22 Thread Singhal, Maneesh
Thanks for your time. My replies inlined...

> -Original Message-
> From: Johannes Thumshirn [mailto:jthumsh...@suse.de]
> Sent: Tuesday, January 19, 2016 9:34 PM
> To: Singhal, Maneesh
> Cc: linux-s...@vger.kernel.org; linux-kernel@vger.kernel.org;
> jbottom...@odin.com; martin.peter...@oracle.com; linux-
> a...@vger.kernel.org
> Subject: Re: [PATCH] drivers/scsi/emcctd: drivers/scsi/emcctd: Client
> driver implementation for EMC-Symmetrix GuestOS emulated Cut-
> Through Device
> 
> On Tue, Jan 19, 2016 at 11:58:06AM +, Singhal, Maneesh wrote:
> > Hello,
> > Kindly review the following patch for the following driver to be
> added in SCSI subsystem -
> >
> > Regards
> > Maneesh
> >
> > 
> > From f3c4b836d6f130b1d7ded618002c8164f8f4a06d Mon Sep 17
> 00:00:00 2001
> > From: "maneesh.singhal" 
> > Date: Tue, 19 Jan 2016 06:39:35 -0500
> > Subject: [PATCH] [PATCH] drivers/scsi/emcctd: Client driver
> implementation for
> >  EMC-Symmetrix GuestOS emulated Cut-Through Device.
> >
> > The patch is a driver implementation  EMC-Symmetrix GuestOS
> emulated Cut-Through
> > Device. The Cut-Through Device PCI emulation is implemented for
> GuestOS
> > environments in the HyperMax OS. GuestOS environments allows
> loading of
> > any x86 compliant operating systems like Linux/FreeBSD etc.
> >
> > The client driver is a SCSI HBA implementation which interfaces with
> SCSI
> > midlayer in the north-bound interfaces and connects with the
> emulated PCI device
> > on the south side.
> >
> > The PCI vendor ID:product ID for emulated Cut-Through Device is
> 0x1120:0x1B00.
> >
> > Signed-off-by: maneesh.singhal 
> > ---
> >  Documentation/scsi/emcctd.txt   |   57 +
> >  MAINTAINERS |9 +
> >  drivers/scsi/Kconfig|1 +
> >  drivers/scsi/Makefile   |1 +
> >  drivers/scsi/emcctd/Kconfig |7 +
> >  drivers/scsi/emcctd/Makefile|1 +
> >  drivers/scsi/emcctd/README  |   10 +
> >  drivers/scsi/emcctd/emc_ctd_interface.h |  386 +
> >  drivers/scsi/emcctd/emcctd.c| 2840
> +++
> >  drivers/scsi/emcctd/emcctd.h|  232 +++
> >  10 files changed, 3544 insertions(+)
> >  create mode 100644 Documentation/scsi/emcctd.txt
> >  create mode 100644 drivers/scsi/emcctd/Kconfig
> >  create mode 100644 drivers/scsi/emcctd/Makefile
> >  create mode 100644 drivers/scsi/emcctd/README
> >  create mode 100644 drivers/scsi/emcctd/emc_ctd_interface.h
> >  create mode 100644 drivers/scsi/emcctd/emcctd.c
> >  create mode 100644 drivers/scsi/emcctd/emcctd.h
> >
> > diff --git a/Documentation/scsi/emcctd.txt
> b/Documentation/scsi/emcctd.txt
> > new file mode 100644
> > index 000..bcafc87
> > --- /dev/null
> > +++ b/Documentation/scsi/emcctd.txt
> > @@ -0,0 +1,56 @@
> > +This file contains brief information about the EMC Cut-Through
> Driver (emcctd).
> > +The driver is currently maintained by Singhal, Maneesh
> (maneesh.sing...@emc.com)
> > +
> > +Last modified: Mon Jan 18 2016 by Maneesh Singhal
> > +
> > +BASICS
> > +
> > +Its a client driver implementation for EMC-Symmetrix GuestOS
> emulated
> > +Cut-Through Device. The Cut-Through Device PCI emulation is
> implemented for
> > +GuestOS environments in the HyperMax OS. GuestOS
> environments allows loading of
> > +any x86 compliant operating systems like Linux/FreeBSD etc.
> > +
> > +The client driver is a SCSI HBA implementation which interfaces with
> SCSI
> > +midlayer in the north-bound interfaces and connects with the
> emulated PCI device
> > +on the south side.
> > +
> > +The PCI vendor ID:product ID for emulated Cut-Through Device is
> 0x1120:0x1B00.
> > +
> > +VERSIONING
> > +
> > +The Version of the driver is maintained as 2.0.0.X, where 2 refers to
> the CTD
> > +protocol in use, and X refers to the ongoing version of the driver.
> > +
> > +
> > +SYSFS SUPPORT
> > +
> > +The driver creates the directory /sys/module/emcctd and
> populates it with
> > +version file and a directory for various parameters as described in
> MODULE
> > +PARAMETERS section.
> > +
> > +PROCFS SUPPORT
> > +
> > +The driver creates the directory /proc/emc and creates files
> emcctd_stats_x
> > +where 'x' refers to the PCI emulation num

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

2016-01-22 Thread Singhal, Maneesh
Hello Thumshirn.
Thanks for taking out time to review the patch. I appreciate that. Please find 
my comments inlined.

> -Original Message-
> From: Johannes Thumshirn [mailto:jthumsh...@suse.de]
> Sent: Tuesday, January 19, 2016 7:20 PM
> To: Singhal, Maneesh
> Cc: linux-s...@vger.kernel.org; linux-kernel@vger.kernel.org;
> jbottom...@odin.com; martin.peter...@oracle.com; linux-
> a...@vger.kernel.org
> Subject: Re: [PATCH] drivers/scsi/emcctd: drivers/scsi/emcctd: Client
> driver implementation for EMC-Symmetrix GuestOS emulated Cut-
> Through Device
> 
> On Tue, Jan 19, 2016 at 11:58:06AM +, Singhal, Maneesh wrote:
> > Hello,
> > Kindly review the following patch for the following driver to be
> added in SCSI subsystem -
> >
> > Regards
> > Maneesh
> >
> 
> Hi Maneesh,
> 
> Fery first round of review. No real functionallity yet just a bit on
> readablility.
> 
> I'll do a more in depth review later.
> 
> > 
>  From f3c4b836d6f130b1d7ded618002c8164f8f4a06d Mon Sep 17
> 00:00:00 2001
> > From: "maneesh.singhal" 
> > Date: Tue, 19 Jan 2016 06:39:35 -0500
> > Subject: [PATCH] [PATCH] drivers/scsi/emcctd: Client driver
> implementation for
> >  EMC-Symmetrix GuestOS emulated Cut-Through Device.
> >
> > The patch is a driver implementation  EMC-Symmetrix GuestOS
> emulated Cut-Through
> > Device. The Cut-Through Device PCI emulation is implemented for
> GuestOS
> > environments in the HyperMax OS. GuestOS environments allows
> loading of
> > any x86 compliant operating systems like Linux/FreeBSD etc.
> >
> > The client driver is a SCSI HBA implementation which interfaces with
> SCSI
> > midlayer in the north-bound interfaces and connects with the
> emulated PCI device
> > on the south side.
> >
> > The PCI vendor ID:product ID for emulated Cut-Through Device is
> 0x1120:0x1B00.
> >
> > Signed-off-by: maneesh.singhal 
> > ---
> >  Documentation/scsi/emcctd.txt   |   57 +
> >  MAINTAINERS |9 +
> >  drivers/scsi/Kconfig|1 +
> >  drivers/scsi/Makefile   |1 +
> >  drivers/scsi/emcctd/Kconfig |7 +
> >  drivers/scsi/emcctd/Makefile|1 +
> >  drivers/scsi/emcctd/README  |   10 +
> >  drivers/scsi/emcctd/emc_ctd_interface.h |  386 +
> >  drivers/scsi/emcctd/emcctd.c| 2840
> +++
> >  drivers/scsi/emcctd/emcctd.h|  232 +++
> >  10 files changed, 3544 insertions(+)
> >  create mode 100644 Documentation/scsi/emcctd.txt
> >  create mode 100644 drivers/scsi/emcctd/Kconfig
> >  create mode 100644 drivers/scsi/emcctd/Makefile
> >  create mode 100644 drivers/scsi/emcctd/README
> >  create mode 100644 drivers/scsi/emcctd/emc_ctd_interface.h
> >  create mode 100644 drivers/scsi/emcctd/emcctd.c
> >  create mode 100644 drivers/scsi/emcctd/emcctd.h
> >
> > diff --git a/Documentation/scsi/emcctd.txt
> b/Documentation/scsi/emcctd.txt
> > new file mode 100644
> > index 000..bcafc87
> > --- /dev/null
> > +++ b/Documentation/scsi/emcctd.txt
> > @@ -0,0 +1,56 @@
> > +This file contains brief information about the EMC Cut-Through
> Driver (emcctd).
> > +The driver is currently maintained by Singhal, Maneesh
> (maneesh.sing...@emc.com)
> > +
> > +Last modified: Mon Jan 18 2016 by Maneesh Singhal
> > +
> > +BASICS
> > +
> > +Its a client driver implementation for EMC-Symmetrix GuestOS
> emulated
> > +Cut-Through Device. The Cut-Through Device PCI emulation is
> implemented for
> > +GuestOS environments in the HyperMax OS. GuestOS
> environments allows loading of
> > +any x86 compliant operating systems like Linux/FreeBSD etc.
> > +
> > +The client driver is a SCSI HBA implementation which interfaces with
> SCSI
> > +midlayer in the north-bound interfaces and connects with the
> emulated PCI device
> > +on the south side.
> > +
> > +The PCI vendor ID:product ID for emulated Cut-Through Device is
> 0x1120:0x1B00.
> > +
> > +VERSIONING
> > +
> > +The Version of the driver is maintained as 2.0.0.X, where 2 refers to
> the CTD
> > +protocol in use, and X refers to the ongoing version of the driver.
> > +
> > +
> > +SYSFS SUPPORT
> > +
> > +The driver creates the directory /sys/module/emcctd and
> populates it with
> > +version file and a directory for various parameters as described

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

2016-01-22 Thread Singhal, Maneesh
Hello Greg,

Thanks for taking out time to review the patch. Please find my replies 
inlined...
Will post the next patch for modifications soon.

> -Original Message-
> From: Greg KH [mailto:g...@kroah.com]
> Sent: Tuesday, January 19, 2016 11:42 PM
> To: Singhal, Maneesh
> Cc: linux-s...@vger.kernel.org; linux-kernel@vger.kernel.org;
> jbottom...@odin.com; martin.peter...@oracle.com; linux-
> a...@vger.kernel.org
> Subject: Re: [PATCH] drivers/scsi/emcctd: drivers/scsi/emcctd: Client
> driver implementation for EMC-Symmetrix GuestOS emulated Cut-
> Through Device
> 
> On Tue, Jan 19, 2016 at 11:58:06AM +, Singhal, Maneesh wrote:
> > Hello,
> > Kindly review the following patch for the following driver to be
> added
> > in SCSI subsystem -
> >
> > Regards
> > Maneesh
> >
> > --
> > --
> > >From f3c4b836d6f130b1d7ded618002c8164f8f4a06d Mon Sep 17
> 00:00:00
> > >2001
> > From: "maneesh.singhal" <maneesh.sing...@emc.com>
> > Date: Tue, 19 Jan 2016 06:39:35 -0500
> > Subject: [PATCH] [PATCH] drivers/scsi/emcctd: Client driver
> > implementation for  EMC-Symmetrix GuestOS emulated Cut-
> Through Device.
> >
> > The patch is a driver implementation  EMC-Symmetrix GuestOS
> emulated
> > Cut-Through Device. The Cut-Through Device PCI emulation is
> > implemented for GuestOS environments in the HyperMax OS.
> GuestOS
> > environments allows loading of any x86 compliant operating systems
> like Linux/FreeBSD etc.
> >
> > The client driver is a SCSI HBA implementation which interfaces with
> > SCSI midlayer in the north-bound interfaces and connects with the
> > emulated PCI device on the south side.
> >
> > The PCI vendor ID:product ID for emulated Cut-Through Device is
> 0x1120:0x1B00.
> >
> > Signed-off-by: maneesh.singhal <maneesh.sing...@emc.com>
> > ---
> >  Documentation/scsi/emcctd.txt   |   57 +
> >  MAINTAINERS |9 +
> >  drivers/scsi/Kconfig|1 +
> >  drivers/scsi/Makefile   |1 +
> >  drivers/scsi/emcctd/Kconfig |7 +
> >  drivers/scsi/emcctd/Makefile|1 +
> >  drivers/scsi/emcctd/README  |   10 +
> >  drivers/scsi/emcctd/emc_ctd_interface.h |  386 +
> >  drivers/scsi/emcctd/emcctd.c| 2840
> +++
> >  drivers/scsi/emcctd/emcctd.h|  232 +++
> >  10 files changed, 3544 insertions(+)
> >  create mode 100644 Documentation/scsi/emcctd.txt  create mode
> 100644
> > drivers/scsi/emcctd/Kconfig  create mode 100644
> > drivers/scsi/emcctd/Makefile  create mode 100644
> > drivers/scsi/emcctd/README  create mode 100644
> > drivers/scsi/emcctd/emc_ctd_interface.h
> >  create mode 100644 drivers/scsi/emcctd/emcctd.c  create mode
> 100644
> > drivers/scsi/emcctd/emcctd.h
> >
> > diff --git a/Documentation/scsi/emcctd.txt
> > b/Documentation/scsi/emcctd.txt new file mode 100644 index
> > 000..bcafc87
> > --- /dev/null
> > +++ b/Documentation/scsi/emcctd.txt
> > @@ -0,0 +1,56 @@
> > +This file contains brief information about the EMC Cut-Through
> Driver (emcctd).
> > +The driver is currently maintained by Singhal, Maneesh
> > +(maneesh.sing...@emc.com)
> > +
> > +Last modified: Mon Jan 18 2016 by Maneesh Singhal
> > +
> > +BASICS
> > +
> > +Its a client driver implementation for EMC-Symmetrix GuestOS
> emulated
> > +Cut-Through Device. The Cut-Through Device PCI emulation is
> > +implemented for GuestOS environments in the HyperMax OS.
> GuestOS
> > +environments allows loading of any x86 compliant operating
> systems like Linux/FreeBSD etc.
> > +
> > +The client driver is a SCSI HBA implementation which interfaces with
> > +SCSI midlayer in the north-bound interfaces and connects with the
> > +emulated PCI device on the south side.
> > +
> > +The PCI vendor ID:product ID for emulated Cut-Through Device is
> 0x1120:0x1B00.
> > +
> > +VERSIONING
> > +
> > +The Version of the driver is maintained as 2.0.0.X, where 2 refers to
> > +the CTD protocol in use, and X refers to the ongoing version of the
> driver.
> > +
> > +
> > +SYSFS SUPPORT
> > +
> > +The driver creates the directory /sys/module/emcctd and
> populates it
> > +with version file and a directory for various parameters as described
> > +in MODULE PARAMETERS section.
> > +
&g

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

2016-01-22 Thread Singhal, Maneesh
Hello Thumshirn.
Thanks for taking out time to review the patch. I appreciate that. Please find 
my comments inlined.

> -Original Message-
> From: Johannes Thumshirn [mailto:jthumsh...@suse.de]
> Sent: Tuesday, January 19, 2016 7:20 PM
> To: Singhal, Maneesh
> Cc: linux-s...@vger.kernel.org; linux-kernel@vger.kernel.org;
> jbottom...@odin.com; martin.peter...@oracle.com; linux-
> a...@vger.kernel.org
> Subject: Re: [PATCH] drivers/scsi/emcctd: drivers/scsi/emcctd: Client
> driver implementation for EMC-Symmetrix GuestOS emulated Cut-
> Through Device
> 
> On Tue, Jan 19, 2016 at 11:58:06AM +, Singhal, Maneesh wrote:
> > Hello,
> > Kindly review the following patch for the following driver to be
> added in SCSI subsystem -
> >
> > Regards
> > Maneesh
> >
> 
> Hi Maneesh,
> 
> Fery first round of review. No real functionallity yet just a bit on
> readablility.
> 
> I'll do a more in depth review later.
> 
> > 
>  From f3c4b836d6f130b1d7ded618002c8164f8f4a06d Mon Sep 17
> 00:00:00 2001
> > From: "maneesh.singhal" <maneesh.sing...@emc.com>
> > Date: Tue, 19 Jan 2016 06:39:35 -0500
> > Subject: [PATCH] [PATCH] drivers/scsi/emcctd: Client driver
> implementation for
> >  EMC-Symmetrix GuestOS emulated Cut-Through Device.
> >
> > The patch is a driver implementation  EMC-Symmetrix GuestOS
> emulated Cut-Through
> > Device. The Cut-Through Device PCI emulation is implemented for
> GuestOS
> > environments in the HyperMax OS. GuestOS environments allows
> loading of
> > any x86 compliant operating systems like Linux/FreeBSD etc.
> >
> > The client driver is a SCSI HBA implementation which interfaces with
> SCSI
> > midlayer in the north-bound interfaces and connects with the
> emulated PCI device
> > on the south side.
> >
> > The PCI vendor ID:product ID for emulated Cut-Through Device is
> 0x1120:0x1B00.
> >
> > Signed-off-by: maneesh.singhal <maneesh.sing...@emc.com>
> > ---
> >  Documentation/scsi/emcctd.txt   |   57 +
> >  MAINTAINERS |9 +
> >  drivers/scsi/Kconfig|1 +
> >  drivers/scsi/Makefile   |1 +
> >  drivers/scsi/emcctd/Kconfig |7 +
> >  drivers/scsi/emcctd/Makefile|1 +
> >  drivers/scsi/emcctd/README  |   10 +
> >  drivers/scsi/emcctd/emc_ctd_interface.h |  386 +
> >  drivers/scsi/emcctd/emcctd.c| 2840
> +++
> >  drivers/scsi/emcctd/emcctd.h|  232 +++
> >  10 files changed, 3544 insertions(+)
> >  create mode 100644 Documentation/scsi/emcctd.txt
> >  create mode 100644 drivers/scsi/emcctd/Kconfig
> >  create mode 100644 drivers/scsi/emcctd/Makefile
> >  create mode 100644 drivers/scsi/emcctd/README
> >  create mode 100644 drivers/scsi/emcctd/emc_ctd_interface.h
> >  create mode 100644 drivers/scsi/emcctd/emcctd.c
> >  create mode 100644 drivers/scsi/emcctd/emcctd.h
> >
> > diff --git a/Documentation/scsi/emcctd.txt
> b/Documentation/scsi/emcctd.txt
> > new file mode 100644
> > index 000..bcafc87
> > --- /dev/null
> > +++ b/Documentation/scsi/emcctd.txt
> > @@ -0,0 +1,56 @@
> > +This file contains brief information about the EMC Cut-Through
> Driver (emcctd).
> > +The driver is currently maintained by Singhal, Maneesh
> (maneesh.sing...@emc.com)
> > +
> > +Last modified: Mon Jan 18 2016 by Maneesh Singhal
> > +
> > +BASICS
> > +
> > +Its a client driver implementation for EMC-Symmetrix GuestOS
> emulated
> > +Cut-Through Device. The Cut-Through Device PCI emulation is
> implemented for
> > +GuestOS environments in the HyperMax OS. GuestOS
> environments allows loading of
> > +any x86 compliant operating systems like Linux/FreeBSD etc.
> > +
> > +The client driver is a SCSI HBA implementation which interfaces with
> SCSI
> > +midlayer in the north-bound interfaces and connects with the
> emulated PCI device
> > +on the south side.
> > +
> > +The PCI vendor ID:product ID for emulated Cut-Through Device is
> 0x1120:0x1B00.
> > +
> > +VERSIONING
> > +
> > +The Version of the driver is maintained as 2.0.0.X, where 2 refers to
> the CTD
> > +protocol in use, and X refers to the ongoing version of the driver.
> > +
> > +
> > +SYSFS SUPPORT
> > +
> > +The driver creates the directory /sys/module/emcctd and
> populates it with
> > +ve

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

2016-01-22 Thread Singhal, Maneesh
Thanks for your time. My replies inlined...

> -Original Message-
> From: Johannes Thumshirn [mailto:jthumsh...@suse.de]
> Sent: Tuesday, January 19, 2016 9:34 PM
> To: Singhal, Maneesh
> Cc: linux-s...@vger.kernel.org; linux-kernel@vger.kernel.org;
> jbottom...@odin.com; martin.peter...@oracle.com; linux-
> a...@vger.kernel.org
> Subject: Re: [PATCH] drivers/scsi/emcctd: drivers/scsi/emcctd: Client
> driver implementation for EMC-Symmetrix GuestOS emulated Cut-
> Through Device
> 
> On Tue, Jan 19, 2016 at 11:58:06AM +, Singhal, Maneesh wrote:
> > Hello,
> > Kindly review the following patch for the following driver to be
> added in SCSI subsystem -
> >
> > Regards
> > Maneesh
> >
> > 
> > From f3c4b836d6f130b1d7ded618002c8164f8f4a06d Mon Sep 17
> 00:00:00 2001
> > From: "maneesh.singhal" <maneesh.sing...@emc.com>
> > Date: Tue, 19 Jan 2016 06:39:35 -0500
> > Subject: [PATCH] [PATCH] drivers/scsi/emcctd: Client driver
> implementation for
> >  EMC-Symmetrix GuestOS emulated Cut-Through Device.
> >
> > The patch is a driver implementation  EMC-Symmetrix GuestOS
> emulated Cut-Through
> > Device. The Cut-Through Device PCI emulation is implemented for
> GuestOS
> > environments in the HyperMax OS. GuestOS environments allows
> loading of
> > any x86 compliant operating systems like Linux/FreeBSD etc.
> >
> > The client driver is a SCSI HBA implementation which interfaces with
> SCSI
> > midlayer in the north-bound interfaces and connects with the
> emulated PCI device
> > on the south side.
> >
> > The PCI vendor ID:product ID for emulated Cut-Through Device is
> 0x1120:0x1B00.
> >
> > Signed-off-by: maneesh.singhal <maneesh.sing...@emc.com>
> > ---
> >  Documentation/scsi/emcctd.txt   |   57 +
> >  MAINTAINERS |9 +
> >  drivers/scsi/Kconfig|1 +
> >  drivers/scsi/Makefile   |1 +
> >  drivers/scsi/emcctd/Kconfig |7 +
> >  drivers/scsi/emcctd/Makefile|1 +
> >  drivers/scsi/emcctd/README  |   10 +
> >  drivers/scsi/emcctd/emc_ctd_interface.h |  386 +
> >  drivers/scsi/emcctd/emcctd.c| 2840
> +++
> >  drivers/scsi/emcctd/emcctd.h|  232 +++
> >  10 files changed, 3544 insertions(+)
> >  create mode 100644 Documentation/scsi/emcctd.txt
> >  create mode 100644 drivers/scsi/emcctd/Kconfig
> >  create mode 100644 drivers/scsi/emcctd/Makefile
> >  create mode 100644 drivers/scsi/emcctd/README
> >  create mode 100644 drivers/scsi/emcctd/emc_ctd_interface.h
> >  create mode 100644 drivers/scsi/emcctd/emcctd.c
> >  create mode 100644 drivers/scsi/emcctd/emcctd.h
> >
> > diff --git a/Documentation/scsi/emcctd.txt
> b/Documentation/scsi/emcctd.txt
> > new file mode 100644
> > index 000..bcafc87
> > --- /dev/null
> > +++ b/Documentation/scsi/emcctd.txt
> > @@ -0,0 +1,56 @@
> > +This file contains brief information about the EMC Cut-Through
> Driver (emcctd).
> > +The driver is currently maintained by Singhal, Maneesh
> (maneesh.sing...@emc.com)
> > +
> > +Last modified: Mon Jan 18 2016 by Maneesh Singhal
> > +
> > +BASICS
> > +
> > +Its a client driver implementation for EMC-Symmetrix GuestOS
> emulated
> > +Cut-Through Device. The Cut-Through Device PCI emulation is
> implemented for
> > +GuestOS environments in the HyperMax OS. GuestOS
> environments allows loading of
> > +any x86 compliant operating systems like Linux/FreeBSD etc.
> > +
> > +The client driver is a SCSI HBA implementation which interfaces with
> SCSI
> > +midlayer in the north-bound interfaces and connects with the
> emulated PCI device
> > +on the south side.
> > +
> > +The PCI vendor ID:product ID for emulated Cut-Through Device is
> 0x1120:0x1B00.
> > +
> > +VERSIONING
> > +
> > +The Version of the driver is maintained as 2.0.0.X, where 2 refers to
> the CTD
> > +protocol in use, and X refers to the ongoing version of the driver.
> > +
> > +
> > +SYSFS SUPPORT
> > +
> > +The driver creates the directory /sys/module/emcctd and
> populates it with
> > +version file and a directory for various parameters as described in
> MODULE
> > +PARAMETERS section.
> > +
> > +PROCFS SUPPORT
> > +
> > +The driver creates the directory /proc/emc and creates files
> emcctd_sta