* [PATCH] [SCSI] FlashPoint: optimize string comparison @ 2015-07-07 5:39 Christophe JAILLET 2015-07-07 8:45 ` Frans Klaver 0 siblings, 1 reply; 6+ messages in thread From: Christophe JAILLET @ 2015-07-07 5:39 UTC (permalink / raw) To: khalid, JBottomley Cc: linux-scsi, linux-kernel, kernel-janitors, Christophe JAILLET Stop comparing the strings as soon as we know that they don't match. Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> --- drivers/scsi/FlashPoint.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/scsi/FlashPoint.c b/drivers/scsi/FlashPoint.c index 5c74e4c..24a4d1a 100644 --- a/drivers/scsi/FlashPoint.c +++ b/drivers/scsi/FlashPoint.c @@ -6280,8 +6280,10 @@ static unsigned char FPT_scmachid(unsigned char p_card, match = 1; for (k = 0; k < ID_STRING_LENGTH; k++) { - if (p_id_string[k] != FPT_scamInfo[i].id_string[k]) + if (p_id_string[k] != FPT_scamInfo[i].id_string[k]) { match = 0; + break; + } } if (match) { -- 2.1.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] [SCSI] FlashPoint: optimize string comparison 2015-07-07 5:39 [PATCH] [SCSI] FlashPoint: optimize string comparison Christophe JAILLET @ 2015-07-07 8:45 ` Frans Klaver 2015-07-07 17:04 ` Khalid Aziz 0 siblings, 1 reply; 6+ messages in thread From: Frans Klaver @ 2015-07-07 8:45 UTC (permalink / raw) To: Christophe JAILLET Cc: khalid, JBottomley, linux-scsi, linux-kernel@vger.kernel.org, kernel-janitors On Tue, Jul 7, 2015 at 7:39 AM, Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote: > Stop comparing the strings as soon as we know that they don't match. > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > --- > drivers/scsi/FlashPoint.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/scsi/FlashPoint.c b/drivers/scsi/FlashPoint.c > index 5c74e4c..24a4d1a 100644 > --- a/drivers/scsi/FlashPoint.c > +++ b/drivers/scsi/FlashPoint.c > @@ -6280,8 +6280,10 @@ static unsigned char FPT_scmachid(unsigned char p_card, > match = 1; > > for (k = 0; k < ID_STRING_LENGTH; k++) { > - if (p_id_string[k] != FPT_scamInfo[i].id_string[k]) > + if (p_id_string[k] != FPT_scamInfo[i].id_string[k]) { > match = 0; > + break; > + } > } > > if (match) { Why doesn't this use strncmp? Thanks, Frans ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] [SCSI] FlashPoint: optimize string comparison 2015-07-07 8:45 ` Frans Klaver @ 2015-07-07 17:04 ` Khalid Aziz 2015-07-08 5:45 ` Christophe JAILLET 0 siblings, 1 reply; 6+ messages in thread From: Khalid Aziz @ 2015-07-07 17:04 UTC (permalink / raw) To: Frans Klaver, Christophe JAILLET Cc: JBottomley, linux-scsi, linux-kernel@vger.kernel.org, kernel-janitors On 07/07/2015 02:45 AM, Frans Klaver wrote: > On Tue, Jul 7, 2015 at 7:39 AM, Christophe JAILLET > <christophe.jaillet@wanadoo.fr> wrote: >> Stop comparing the strings as soon as we know that they don't match. >> >> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> >> --- >> drivers/scsi/FlashPoint.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/scsi/FlashPoint.c b/drivers/scsi/FlashPoint.c >> index 5c74e4c..24a4d1a 100644 >> --- a/drivers/scsi/FlashPoint.c >> +++ b/drivers/scsi/FlashPoint.c >> @@ -6280,8 +6280,10 @@ static unsigned char FPT_scmachid(unsigned char p_card, >> match = 1; >> >> for (k = 0; k < ID_STRING_LENGTH; k++) { >> - if (p_id_string[k] != FPT_scamInfo[i].id_string[k]) >> + if (p_id_string[k] != FPT_scamInfo[i].id_string[k]) { >> match = 0; >> + break; >> + } >> } >> >> if (match) { > > Why doesn't this use strncmp? > > Thanks, > Frans > I suspect that is how this code came from Mylex many years ago. Using strncmp would indeed be a better way to clean this up. Also, further down in the same routine: if (FPT_scamInfo[match].state == ID_UNUSED) { for (k = 0; k < ID_STRING_LENGTH; k++) { FPT_scamInfo[match].id_string[k] = p_id_string[k]; } This should use strncpy instead. There is another similar spot further down. Christophe, if you can send a new patch with these clean-ups, that would be great. Thanks, Khalid ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] [SCSI] FlashPoint: optimize string comparison 2015-07-07 17:04 ` Khalid Aziz @ 2015-07-08 5:45 ` Christophe JAILLET 2015-07-08 7:12 ` Frans Klaver 0 siblings, 1 reply; 6+ messages in thread From: Christophe JAILLET @ 2015-07-08 5:45 UTC (permalink / raw) To: Khalid Aziz, Frans Klaver Cc: JBottomley, linux-scsi, linux-kernel@vger.kernel.org, kernel-janitors Le 07/07/2015 19:04, Khalid Aziz a écrit : > On 07/07/2015 02:45 AM, Frans Klaver wrote: >> On Tue, Jul 7, 2015 at 7:39 AM, Christophe JAILLET >> <christophe.jaillet@wanadoo.fr> wrote: >>> Stop comparing the strings as soon as we know that they don't match. >>> >>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> >>> --- >>> drivers/scsi/FlashPoint.c | 4 +++- >>> 1 file changed, 3 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/scsi/FlashPoint.c b/drivers/scsi/FlashPoint.c >>> index 5c74e4c..24a4d1a 100644 >>> --- a/drivers/scsi/FlashPoint.c >>> +++ b/drivers/scsi/FlashPoint.c >>> @@ -6280,8 +6280,10 @@ static unsigned char FPT_scmachid(unsigned >>> char p_card, >>> match = 1; >>> >>> for (k = 0; k < ID_STRING_LENGTH; k++) { >>> - if (p_id_string[k] != >>> FPT_scamInfo[i].id_string[k]) >>> + if (p_id_string[k] != >>> FPT_scamInfo[i].id_string[k]) { >>> match = 0; >>> + break; >>> + } >>> } >>> >>> if (match) { >> >> Why doesn't this use strncmp? >> >> Thanks, >> Frans >> > > I suspect that is how this code came from Mylex many years ago. Using > strncmp would indeed be a better way to clean this up. Also, further > down in the same routine: > > if (FPT_scamInfo[match].state == ID_UNUSED) { > for (k = 0; k < ID_STRING_LENGTH; k++) { > FPT_scamInfo[match].id_string[k] = > p_id_string[k]; > } > > > This should use strncpy instead. There is another similar spot further > down. > > Christophe, if you can send a new patch with these clean-ups, that > would be great. > > Thanks, > Khalid Hi, I'm sorry but I won't propose a new patch for that. I had the same reaction at first (why not use strncmp?) but it seems to be the way this driver is coded. Should we want to introduce strncmp here, then, as you have noticed, strcpy should be used to. memset could be also used in many places. Then looking elsewhere in the code, many things should, IMHO, also be fixed. (use consistently empty lines before/after code ; use consistently { }...) Another concern to me is "the use of carriage return". In the following examples, things could be much more readable if not limited to 50 chars per line, or so. 1357 <http://lxr.free-electrons.com/source/drivers/scsi/FlashPoint.c#L1357> FPT_sccbMgrTbl <http://lxr.free-electrons.com/ident?i=FPT_sccbMgrTbl>[thisCard][id <http://lxr.free-electrons.com/ident?i=id> * 2 + 1358 <http://lxr.free-electrons.com/source/drivers/scsi/FlashPoint.c#L1358> i <http://lxr.free-electrons.com/ident?i=i>].TarStatus |= 1359 <http://lxr.free-electrons.com/source/drivers/scsi/FlashPoint.c#L1359> SYNC_SUPPORTED <http://lxr.free-electrons.com/ident?i=SYNC_SUPPORTED>; or 4850 <http://lxr.free-electrons.com/source/drivers/scsi/FlashPoint.c#L4850> FPT_sccbMgrTbl <http://lxr.free-electrons.com/ident?i=FPT_sccbMgrTbl>[p_card] 4851 <http://lxr.free-electrons.com/source/drivers/scsi/FlashPoint.c#L4851> [currSCCB->TargID]. 4852 <http://lxr.free-electrons.com/source/drivers/scsi/FlashPoint.c#L4852> TarLUNBusy[0] = 1; 4853 <http://lxr.free-electrons.com/source/drivers/scsi/FlashPoint.c#L4853> if (currSCCB->Sccb_tag) { 4854 <http://lxr.free-electrons.com/source/drivers/scsi/FlashPoint.c#L4854> if (FPT_BL_Card <http://lxr.free-electrons.com/ident?i=FPT_BL_Card>[p_card]. 4855 <http://lxr.free-electrons.com/source/drivers/scsi/FlashPoint.c#L4855> discQCount != 0) 4856 <http://lxr.free-electrons.com/source/drivers/scsi/FlashPoint.c#L4856> FPT_BL_Card <http://lxr.free-electrons.com/ident?i=FPT_BL_Card> 4857 <http://lxr.free-electrons.com/source/drivers/scsi/FlashPoint.c#L4857> [p_card]. 4858 <http://lxr.free-electrons.com/source/drivers/scsi/FlashPoint.c#L4858> discQCount--; 4859 <http://lxr.free-electrons.com/source/drivers/scsi/FlashPoint.c#L4859> FPT_BL_Card <http://lxr.free-electrons.com/ident?i=FPT_BL_Card>[p_card]. 4860 <http://lxr.free-electrons.com/source/drivers/scsi/FlashPoint.c#L4860> discQ_Tbl[currSCCB-> 4861 <http://lxr.free-electrons.com/source/drivers/scsi/FlashPoint.c#L4861> Sccb_tag] 4862 <http://lxr.free-electrons.com/source/drivers/scsi/FlashPoint.c#L4862> =NULL <http://lxr.free-electrons.com/ident?i=NULL>; 4863 <http://lxr.free-electrons.com/source/drivers/scsi/FlashPoint.c#L4863> } else { 4864 <http://lxr.free-electrons.com/source/drivers/scsi/FlashPoint.c#L4864> if (FPT_BL_Card <http://lxr.free-electrons.com/ident?i=FPT_BL_Card>[p_card]. 4865 <http://lxr.free-electrons.com/source/drivers/scsi/FlashPoint.c#L4865> discQCount != 0) 4866 <http://lxr.free-electrons.com/source/drivers/scsi/FlashPoint.c#L4866> FPT_BL_Card <http://lxr.free-electrons.com/ident?i=FPT_BL_Card> 4867 <http://lxr.free-electrons.com/source/drivers/scsi/FlashPoint.c#L4867> [p_card]. 4868 <http://lxr.free-electrons.com/source/drivers/scsi/FlashPoint.c#L4868> discQCount--; 4869 <http://lxr.free-electrons.com/source/drivers/scsi/FlashPoint.c#L4869> FPT_BL_Card <http://lxr.free-electrons.com/ident?i=FPT_BL_Card>[p_card]. 4870 <http://lxr.free-electrons.com/source/drivers/scsi/FlashPoint.c#L4870> discQ_Tbl 4871 <http://lxr.free-electrons.com/source/drivers/scsi/FlashPoint.c#L4871> [FPT_sccbMgrTbl <http://lxr.free-electrons.com/ident?i=FPT_sccbMgrTbl> 4872 <http://lxr.free-electrons.com/source/drivers/scsi/FlashPoint.c#L4872> [p_card][currSCCB-> 4873 <http://lxr.free-electrons.com/source/drivers/scsi/FlashPoint.c#L4873> TargID]. 4874 <http://lxr.free-electrons.com/source/drivers/scsi/FlashPoint.c#L4874> LunDiscQ_Idx[0]] = 4875 <http://lxr.free-electrons.com/source/drivers/scsi/FlashPoint.c#L4875> NULL <http://lxr.free-electrons.com/ident?i=NULL>; 4876 <http://lxr.free-electrons.com/source/drivers/scsi/FlashPoint.c#L4876> } I only reported an easy fix based on a semi-automatic detection of code that could be improved. I don't want to make some much more heavy changes that could break the code. Best regards, CJ ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] [SCSI] FlashPoint: optimize string comparison 2015-07-08 5:45 ` Christophe JAILLET @ 2015-07-08 7:12 ` Frans Klaver 2015-07-08 13:59 ` Khalid Aziz 0 siblings, 1 reply; 6+ messages in thread From: Frans Klaver @ 2015-07-08 7:12 UTC (permalink / raw) To: Christophe JAILLET Cc: Khalid Aziz, JBottomley, linux-scsi, linux-kernel@vger.kernel.org, kernel-janitors On Wed, Jul 8, 2015 at 7:45 AM, Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote: > Le 07/07/2015 19:04, Khalid Aziz a écrit : >> >> On 07/07/2015 02:45 AM, Frans Klaver wrote: >>> >>> On Tue, Jul 7, 2015 at 7:39 AM, Christophe JAILLET >>> <christophe.jaillet@wanadoo.fr> wrote: >>>> >>>> Stop comparing the strings as soon as we know that they don't match. >>>> >>>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> >>>> --- >>>> drivers/scsi/FlashPoint.c | 4 +++- >>>> 1 file changed, 3 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/scsi/FlashPoint.c b/drivers/scsi/FlashPoint.c >>>> index 5c74e4c..24a4d1a 100644 >>>> --- a/drivers/scsi/FlashPoint.c >>>> +++ b/drivers/scsi/FlashPoint.c >>>> @@ -6280,8 +6280,10 @@ static unsigned char FPT_scmachid(unsigned char >>>> p_card, >>>> match = 1; >>>> >>>> for (k = 0; k < ID_STRING_LENGTH; k++) { >>>> - if (p_id_string[k] != >>>> FPT_scamInfo[i].id_string[k]) >>>> + if (p_id_string[k] != >>>> FPT_scamInfo[i].id_string[k]) { >>>> match = 0; >>>> + break; >>>> + } >>>> } >>>> >>>> if (match) { >>> >>> >>> Why doesn't this use strncmp? >>> >>> Thanks, >>> Frans >>> >> >> I suspect that is how this code came from Mylex many years ago. Using >> strncmp would indeed be a better way to clean this up. Also, further down in >> the same routine: >> >> if (FPT_scamInfo[match].state == ID_UNUSED) { >> for (k = 0; k < ID_STRING_LENGTH; k++) { >> FPT_scamInfo[match].id_string[k] = >> p_id_string[k]; >> } >> >> >> This should use strncpy instead. There is another similar spot further >> down. >> >> Christophe, if you can send a new patch with these clean-ups, that would >> be great. >> >> Thanks, >> Khalid > > > Hi, > > I'm sorry but I won't propose a new patch for that. > > I had the same reaction at first (why not use strncmp?) but it seems to be > the way this driver is coded. Should we want to introduce strncmp here, > then, as you have noticed, strcpy should be used to. memset could be also > used in many places. Then looking elsewhere in the code, many things should, > IMHO, also be fixed. (use consistently empty lines before/after code ; use > consistently { }...) > > Another concern to me is "the use of carriage return". In the following > examples, things could be much more readable if not limited to 50 chars per > line, or so. 80. Parts of the code are indented way too much, resulting in these unreadable lines. I have to say that this entire driver looks like something that (sh|w)ould be in staging right now. Frans ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] [SCSI] FlashPoint: optimize string comparison 2015-07-08 7:12 ` Frans Klaver @ 2015-07-08 13:59 ` Khalid Aziz 0 siblings, 0 replies; 6+ messages in thread From: Khalid Aziz @ 2015-07-08 13:59 UTC (permalink / raw) To: Frans Klaver, Christophe JAILLET Cc: JBottomley, linux-scsi, linux-kernel@vger.kernel.org, kernel-janitors On 07/08/2015 01:12 AM, Frans Klaver wrote: > On Wed, Jul 8, 2015 at 7:45 AM, Christophe JAILLET > <christophe.jaillet@wanadoo.fr> wrote: >> Le 07/07/2015 19:04, Khalid Aziz a écrit : >>> >>> On 07/07/2015 02:45 AM, Frans Klaver wrote: >>>> >>>> On Tue, Jul 7, 2015 at 7:39 AM, Christophe JAILLET >>>> <christophe.jaillet@wanadoo.fr> wrote: >>>>> >>>>> Stop comparing the strings as soon as we know that they don't match. >>>>> >>>>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> >>>>> --- >>>>> drivers/scsi/FlashPoint.c | 4 +++- >>>>> 1 file changed, 3 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/scsi/FlashPoint.c b/drivers/scsi/FlashPoint.c >>>>> index 5c74e4c..24a4d1a 100644 >>>>> --- a/drivers/scsi/FlashPoint.c >>>>> +++ b/drivers/scsi/FlashPoint.c >>>>> @@ -6280,8 +6280,10 @@ static unsigned char FPT_scmachid(unsigned char >>>>> p_card, >>>>> match = 1; >>>>> >>>>> for (k = 0; k < ID_STRING_LENGTH; k++) { >>>>> - if (p_id_string[k] != >>>>> FPT_scamInfo[i].id_string[k]) >>>>> + if (p_id_string[k] != >>>>> FPT_scamInfo[i].id_string[k]) { >>>>> match = 0; >>>>> + break; >>>>> + } >>>>> } >>>>> >>>>> if (match) { >>>> >>>> >>>> Why doesn't this use strncmp? >>>> >>>> Thanks, >>>> Frans >>>> >>> >>> I suspect that is how this code came from Mylex many years ago. Using >>> strncmp would indeed be a better way to clean this up. Also, further down in >>> the same routine: >>> >>> if (FPT_scamInfo[match].state == ID_UNUSED) { >>> for (k = 0; k < ID_STRING_LENGTH; k++) { >>> FPT_scamInfo[match].id_string[k] = >>> p_id_string[k]; >>> } >>> >>> >>> This should use strncpy instead. There is another similar spot further >>> down. >>> >>> Christophe, if you can send a new patch with these clean-ups, that would >>> be great. >>> >>> Thanks, >>> Khalid >> >> >> Hi, >> >> I'm sorry but I won't propose a new patch for that. >> >> I had the same reaction at first (why not use strncmp?) but it seems to be >> the way this driver is coded. Should we want to introduce strncmp here, >> then, as you have noticed, strcpy should be used to. memset could be also >> used in many places. Then looking elsewhere in the code, many things should, >> IMHO, also be fixed. (use consistently empty lines before/after code ; use >> consistently { }...) >> >> Another concern to me is "the use of carriage return". In the following >> examples, things could be much more readable if not limited to 50 chars per >> line, or so. > > 80. Parts of the code are indented way too much, resulting in these > unreadable lines. I have to say that this entire driver looks like > something that (sh|w)ould be in staging right now. FlashPoint.c and BusLogic.c together make up the buslogic driver. I went through a massive clean up of BusLogic.c (addressing issues like the ones you guys are bringing up) when I took over maintenance of buslogic driver and made it 64-bit clean. I am very much in agreement with suggested clean ups, but this code has been in the kernel for a very long time and cleaning it up at this point for the sake of clean up is not a reason enough to destabilize a very stable driver. staging is for drivers under development and in experimental state. buslogic driver is most definitely not experimental. For now we either leave FlashPoint.c as it is or clean it up as part of a bigger effort to add new functionality or fix a major bug. Christophe's original patch fixes an inefficiency in the code, so my take on it is to either accept the very specific fix without modifying rest of the code or combine this fix with clean up of at least the entire FPT_scmachid() routine. Thanks, Khalid ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-07-08 15:50 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-07-07 5:39 [PATCH] [SCSI] FlashPoint: optimize string comparison Christophe JAILLET 2015-07-07 8:45 ` Frans Klaver 2015-07-07 17:04 ` Khalid Aziz 2015-07-08 5:45 ` Christophe JAILLET 2015-07-08 7:12 ` Frans Klaver 2015-07-08 13:59 ` Khalid Aziz
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox