re: [SCSI] arcmsr: Support Areca new SATA Raid Adapter ARC1214/1224/1264/1284

re: [SCSI] arcmsr: Support Areca new SATA Raid Adapter ARC1214/1224/1264/1284

[Dan Carpenter]

Hello 黃清隆,

The patch 17628f3a062b: "[SCSI] arcmsr: Support Areca new SATA Raid
Adapter ARC1214/1224/1264/1284" from Aug 26, 2013, leads to the
following Smatch warning:
"drivers/scsi/arcmsr/arcmsr_hba.c:3580 arcmsr_hbaD_get_config()
	 warn: signedness bug returning '(-12)'"

drivers/scsi/arcmsr/arcmsr_hba.c
  3576          dma_coherent = dma_alloc_coherent(&pdev->dev, acb->uncache_size,
  3577          &dma_coherent_handle, GFP_KERNEL);
  3578          if (!dma_coherent) {
  3579                  pr_notice("DMA allocation failed...\n");
  3580                  return -ENOMEM;
                        ^^^^^^^^^^^^^^
This should be returning false.

  3581          }

Line 3577 has messed up indenting.

Also this patch says it adds support for new hardware but almost 900
lines out of this 3605 line patch are white space changes.  Do the
unrelated white space changes in a separate patch.

This patch also re-introduces a bug which I fixed in the mainline kernel
a year ago.

drivers/scsi/arcmsr/arcmsr_hba.c
  4525                          writel(0xD, &pmuC->write_sequence);
  4526                  } while ((((temp = readl(&pmuC->host_diagnostic)) |
                                                                          ^
This should be a '&' not a '|'.  Please fix this again back to the way
it was.

  4527                  ARCMSR_ARC1880_DiagWrite_ENABLE) == 0) &&
  4528                  (count < 5));

The indenting here is messed up as well.  This is a very low quality
patch.

I think you are not using git internally in your company and that is why
you are messing up so badly.  Please learn to use it.  Keep track of the
fixes which go into the mainline kernel.  Separate the white space
cleanups from the new features.

regards,
dan carpenter

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

re: [SCSI] arcmsr: Support Areca new SATA Raid Adapter ARC1214/1224/1264/1284

[James Bottomley]

Dan Carpenter <dan.carpenter@oracle.com> wrote:
>Hello 黃清隆,
>
>The patch 17628f3a062b: "[SCSI] arcmsr: Support Areca new SATA Raid
>Adapter ARC1214/1224/1264/1284" from Aug 26, 2013, leads to the
>following Smatch warning:
>"drivers/scsi/arcmsr/arcmsr_hba.c:3580 arcmsr_hbaD_get_config()
>	 warn: signedness bug returning '(-12)'"
>
>drivers/scsi/arcmsr/arcmsr_hba.c
>3576          dma_coherent = dma_alloc_coherent(&pdev->dev,
>acb->uncache_size,
>  3577          &dma_coherent_handle, GFP_KERNEL);
>  3578          if (!dma_coherent) {
>  3579                  pr_notice("DMA allocation failed...\n");
>  3580                  return -ENOMEM;
>                        ^^^^^^^^^^^^^^
>This should be returning false.
>
>  3581          }
>
>Line 3577 has messed up indenting.
>
>Also this patch says it adds support for new hardware but almost 900
>lines out of this 3605 line patch are white space changes.  Do the
>unrelated white space changes in a separate patch.
>
>This patch also re-introduces a bug which I fixed in the mainline
>kernel
>a year ago.
>
>drivers/scsi/arcmsr/arcmsr_hba.c
>  4525                          writel(0xD, &pmuC->write_sequence);
>4526                  } while ((((temp = readl(&pmuC->host_diagnostic))
>|
>                                                                      ^
>This should be a '&' not a '|'.  Please fix this again back to the way
>it was.
>
>  4527                  ARCMSR_ARC1880_DiagWrite_ENABLE) == 0) &&
>  4528                  (count < 5));
>
>The indenting here is messed up as well.  This is a very low quality
>patch.
>
>I think you are not using git internally in your company and that is
>why
>you are messing up so badly.  Please learn to use it.  Keep track of
>the
>fixes which go into the mainline kernel.  Separate the white space
>cleanups from the new features.

OK based on this I'll drop the arcmsr updates pending a rewrite.

James
-- 
Sent from my Android phone with K-9 Mail. Please excuse my brevity.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [SCSI] arcmsr: Support Areca new SATA Raid Adapter ARC1214/1224/1264/1284

[Dan Carpenter]

One tool for breaking patches up is "git citool" highlight what you want
to add to the patch and then right click and select "stage lines for
commit".

So for example let's go through the changes to
drivers/scsi/arcmsr/arcmsr.h.  These should be broken up into multiple
patches with the easiest whitespace patches first.

The original patch adds 520 lines of code (I'm looking at the plus marks
in git show 17628f3a062b | diffstat).  Out of those 392 line are pure
white space changes such as tabs or indents.  9 lines are adding
parenthesis around macros.  4 lines are changing int32_t to uint32_t.
It's not clear if there was a signedness bug in the original code or if
this is purely cosmetic.  That should be mentioned in the changelog.
The remaining 115 lines are related to the new feature.  So this should
be a series of patches.

[patch 1/4] whitespace (this is the largest patch in the series).
[patch 2/4] add parenthesis
[patch 3/4] change signed to unsigned
[patch 4/4] add feature (this would touch other the other files as well)

The first 3 patches are trivial to review and then the last one is much
smaller and easier to review than when everything was mixed together.

regards,
dan carpenter