Re: [PATCH] scsi: hisi_sas: optimise DMA slot memory

[Frans Klaver]

On Wed, Jun 28, 2017 at 5:25 PM, John Garry  wrote:
> From: Xiaofei Tan 
>
> Currently we allocate 3 sets of DMA memories from separate
> pools for each slot. This is inefficient in terms of memory usage
> (buffers are less than 1 page in size, so we lose due to alignment),
> and also time spend in doing 3 allocations + de-allocations per slot,
> instead of 1.
>
> To optimise, combine the 3 DMA buffers into a single buffer from a
> single pool.
>
> Signed-off-by: John Garry 
> Signed-off-by: Xiaofei Tan 

I'm not sure how strictly this is done, but typically the author signs
off first.

Frans

Re: [PATCH] [SCSI] FlashPoint: optimize string comparison

[Frans Klaver]

On Wed, Jul 8, 2015 at 7:45 AM, Christophe JAILLET
christophe.jail...@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.jail...@wanadoo.fr wrote:

 Stop comparing the strings as soon as we know that they don't match.

 Signed-off-by: Christophe JAILLET christophe.jail...@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
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] [SCSI] FlashPoint: optimize string comparison

[Frans Klaver]

On Tue, Jul 7, 2015 at 7:39 AM, Christophe JAILLET
christophe.jail...@wanadoo.fr wrote:
 Stop comparing the strings as soon as we know that they don't match.

 Signed-off-by: Christophe JAILLET christophe.jail...@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
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] megaraid:Remove no longer required variable ret from the function megasas_sync_map_info

[Frans Klaver]

Can't seem to find the original, so here's a reply to the ack mail.

On Tue, Jul 7, 2015 at 10:49 AM, Sumit Saxena
sumit.sax...@avagotech.com wrote:
 -Original Message-
 From: Nicholas Krause [mailto:xerofo...@gmail.com]
 Sent: Monday, July 06, 2015 9:43 PM
 To: kashyap.de...@avagotech.com
 Cc: sumit.sax...@avagotech.com; uday.ling...@avagotech.com;
 jbottom...@odin.com; megaraidlinux@avagotech.com;
 linux-scsi@vger.kernel.org; linux-ker...@vger.kernel.org
 Subject: [PATCH] megaraid:Remove no longer required variable ret from the
 function megasas_sync_map_info

Isn't something shorter like

  [PATCH] megaraid: remove pointless variable

much more readable?


 This removes the no longer required variable ret due to this variable only
 ever being used at the end of the function megasas_sync_map_info without
 changing it's value from the orginal setting of its value to zero due to
 this just remove the variable ret and just return the value of zero
 directly here in order to indicate to the caller the call to this function
 has run successfully without any non recoverable issues.

No interpunction?


 Signed-off-by: Nicholas Krause xerofo...@gmail.com
 ---
  drivers/scsi/megaraid/megaraid_sas_fusion.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

 diff --git a/drivers/scsi/megaraid/megaraid_sas_fusion.c
 b/drivers/scsi/megaraid/megaraid_sas_fusion.c
 index 46a0f8f..b5a8c65 100644
 --- a/drivers/scsi/megaraid/megaraid_sas_fusion.c
 +++ b/drivers/scsi/megaraid/megaraid_sas_fusion.c
 @@ -836,7 +836,7 @@ megasas_get_map_info(struct megasas_instance
 *instance)  int  megasas_sync_map_info(struct megasas_instance *instance)
 {
 -   int ret = 0, i;
 +   int i;
 struct megasas_cmd *cmd;
 struct megasas_dcmd_frame *dcmd;
 u32 size_sync_info, num_lds;
 @@ -906,7 +906,7 @@ megasas_sync_map_info(struct megasas_instance
 *instance)

 instance-instancet-issue_dcmd(instance, cmd);

 -   return ret;
 +   return 0;
  }

 Acked-by: Sumit Saxena sumit.sax...@avagotech.com

This ack in an outlook-style response confused the hell out of me ;).

Thanks,
Frans
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html