> -----Original Message----- > From: Leon Romanovsky [mailto:leon@xxxxxxxxxx] > Sent: Tuesday, December 20, 2016 11:48 PM > To: Tung, Chien Tin <chien.tin.tung@xxxxxxxxx> > Cc: Orosco, Henry <henry.orosco@xxxxxxxxx>; dledford@xxxxxxxxxx; linux- > rdma@xxxxxxxxxxxxxxx; e1000-rdma@xxxxxxxxxxxxxxxxxxxxx > Subject: Re: [PATCH] i40iw: Set 128B as the only supported RQ WQE size > > On Wed, Dec 21, 2016 at 12:04:55AM +0000, Tung, Chien Tin wrote: > > > > > > > -----Original Message----- > > > From: Leon Romanovsky [mailto:leon@xxxxxxxxxx] > > > Sent: Tuesday, December 20, 2016 2:03 PM > > > To: Tung, Chien Tin <chien.tin.tung@xxxxxxxxx> > > > Cc: Orosco, Henry <henry.orosco@xxxxxxxxx>; dledford@xxxxxxxxxx; > > > linux- rdma@xxxxxxxxxxxxxxx; e1000-rdma@xxxxxxxxxxxxxxxxxxxxx > > > Subject: Re: [PATCH] i40iw: Set 128B as the only supported RQ WQE > > > size > > > > > > On Tue, Dec 20, 2016 at 02:46:30PM +0000, Tung, Chien Tin wrote: > > > > > > > > > > > > > -----Original Message----- > > > > > From: Leon Romanovsky [mailto:leon@xxxxxxxxxx] > > > > > Sent: Tuesday, December 20, 2016 7:42 AM > > > > > To: Tung, Chien Tin <chien.tin.tung@xxxxxxxxx> > > > > > Cc: Orosco, Henry <henry.orosco@xxxxxxxxx>; dledford@xxxxxxxxxx; > > > > > linux- rdma@xxxxxxxxxxxxxxx; e1000-rdma@xxxxxxxxxxxxxxxxxxxxx > > > > > Subject: Re: [PATCH] i40iw: Set 128B as the only supported RQ > > > > > WQE size > > > > > > > > > > On Tue, Dec 20, 2016 at 12:44:04PM +0000, Tung, Chien Tin wrote: > > > > > > > > > > > > > > > > > > > -----Original Message----- > > > > > > > From: linux-rdma-owner@xxxxxxxxxxxxxxx [mailto:linux-rdma- > > > > > > > owner@xxxxxxxxxxxxxxx] On Behalf Of Leon Romanovsky > > > > > > > Sent: Tuesday, December 20, 2016 5:24 AM > > > > > > > To: Orosco, Henry <henry.orosco@xxxxxxxxx> > > > > > > > Cc: dledford@xxxxxxxxxx; linux-rdma@xxxxxxxxxxxxxxx; e1000- > > > > > > > rdma@xxxxxxxxxxxxxxxxxxxxx; Tung, Chien Tin > > > > > > > <chien.tin.tung@xxxxxxxxx> > > > > > > > Subject: Re: [PATCH] i40iw: Set 128B as the only supported > > > > > > > RQ WQE size > > > > > > > > > > > > > > > diff --git a/drivers/infiniband/hw/i40iw/i40iw_ucontext.h > > > > > > > > b/drivers/infiniband/hw/i40iw/i40iw_ucontext.h > > > > > > > > index 12acd68..57d3f1d 100644 > > > > > > > > --- a/drivers/infiniband/hw/i40iw/i40iw_ucontext.h > > > > > > > > +++ b/drivers/infiniband/hw/i40iw/i40iw_ucontext.h > > > > > > > > @@ -39,8 +39,8 @@ > > > > > > > > > > > > > > > > #include <linux/types.h> > > > > > > > > > > > > > > > > -#define I40IW_ABI_USERSPACE_VER 4 > > > > > > > > -#define I40IW_ABI_KERNEL_VER 4 > > > > > > > > +#define I40IW_ABI_VER 5 > > > > > > > > + > > > > > > > > > > > > > > Why did you remove defines and move to use constants "4" and "5" > > > > > instead? > > > > > > [Chien Tin Tung] This is the ABI version change, did you read > > > > > > the commit > > > > > message? Two defines were not necessary. > > > > > > > > > > > > > > > > Thank you Chian Tin Tung for your informative answer and yes, I > > > > > read commit message. > > > > > > > > > > If you think that these defines are not needed, so can you > > > > > please remove the code which uses hardcoded "case 4" and "case 5"? > > > > [Chien Tin Tung] Those are explicit checks against a particular > > > > ABI version and it is clearer to use the numbers rather than some define. > > > > > > Can you please stop to add [..] in front of your response? This > > > Outlook style is annoying and not needed. > > > > > > Regarding you response, you added the same check in two places and > > > from grep/ctags/cscope/lxr perspective it is easier to spot them with a > define. > > > > You have yet to provide a good argument for changing the 4 and 5 to > > defines, so I don't know why you are telling me how to find them. Very > puzzling. > > I'm not telling to you, but asking from you to provide maintainable code which > will be read more than once. Right now, you blindly copy-pasted hardcoded > values in three different places (2 kernel and 1 rdma-core). Please make your case by coming up with a define that's better than 4 or 5, making the code more readable and understandable, then I will consider using it. Until you can do that, you have no case for your comment. Blindly using a define is something you tell junior programmers, sometimes, a number is just a number, nothing wrong with using it as is. > > Feel free to ask your colleagues (Faisal L. and Shiraz S.) if they interested in > maintainable driver code or they prefer your prone to errors copy-paste > technique. Why don't you go back and read the comment made against I40IW_SUCCESS? Maybe you will learn something from that? > > > I already made my case on this. Enough said. > > > > Chien > > > > > > > [Chien] > > > > > > > > > > > > > > Or maybe they still needed and should be renamed? > > > > [Chien Tin Tung] They are not needed. Before we were checking > > > > against one version and one version only. Now we are making the > > > > provider library and driver backward compatible. All we need is > > > > one define to specify current ABI version. > > > > > > > > Chien > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-rdma" > > in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo > > info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
