Re: [PATCH] scsi: eata: drop VLA in reorder()
Adding kernel newbies to CC because I pose a few noob questions :) Adding Linus to CC because I quoted him. On Sun, Mar 11, 2018 at 10:06:58PM +0100, Salvatore Mesoraca wrote: > n_ready will always be less than or equal to MAX_MAILBOXES. > So we avoid a VLA[1] and use fixed-length arrays instead. > > [1] https://lkml.org/lkml/2018/3/7/621 > > Signed-off-by: Salvatore Mesoraca > --- > drivers/scsi/eata.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/scsi/eata.c b/drivers/scsi/eata.c > index 6501c33..202cd17 100644 > --- a/drivers/scsi/eata.c > +++ b/drivers/scsi/eata.c > @@ -2096,7 +2096,7 @@ static int reorder(struct hostdata *ha, unsigned long > cursec, > unsigned int k, n; > unsigned int rev = 0, s = 1, r = 1; > unsigned int input_only = 1, overlap = 0; > - unsigned long sl[n_ready], pl[n_ready], ll[n_ready]; > + unsigned long sl[MAX_MAILBOXES], pl[MAX_MAILBOXES], ll[MAX_MAILBOXES]; I think we are going to see a recurring theme here. MAX_MAILBOXES==64 so this patch adds 1536 bytes to the stack on a 64 bit machine or 768 bytes on a 32 bit machine. Linus already commented on another VLA removal patch that 768 was a lot of stack space. That comment did, however say 'deep in some transfer call chain'. I don't know what a 'transfer call chain' (the transfer bit) is but is there some heuristic we can use to know how deep is deep? Or more to the point, is there some heuristic we can use to know what is an acceptable amount of stack space to use? As far as this patch is concerned wouldn't a kmalloc (with GFP_ATOMIC) be ok? We are in an interrupt handler, can we assume that since IO has just occurred that the IO will be so slow comparatively that a memory allocation will be quick. (assuming IO since eata.c only requests a single irq line.) thanks, Tobin.
Re: [PATCH 00/18] use ARRAY_SIZE macro
On Sun, Oct 01, 2017 at 03:30:38PM -0400, Jérémy Lefaure wrote: > Hi everyone, > Using ARRAY_SIZE improves the code readability. I used coccinelle (I > made a change to the array_size.cocci file [1]) to find several places > where ARRAY_SIZE could be used instead of other macros or sizeof > division. > > I tried to divide the changes into a patch per subsystem (excepted for > staging). If one of the patch should be split into several patches, let > me know. > > In order to reduce the size of the To: and Cc: lines, each patch of the > series is sent only to the maintainers and lists concerned by the patch. > This cover letter is sent to every list concerned by this series. Why don't you just send individual patches for each subsystem? I'm not a maintainer but I don't see how any one person is going to be able to apply this whole series, it is making it hard for maintainers if they have to pick patches out from among the series (if indeed any will bother doing that). I get that this will be more work for you but AFAIK we are optimizing for maintainers. Good luck, Tobin.
[PATCH 2/3] cciss: Fix checkpatch OPEN_BRACE
Checkpatch emits ERROR:OPEN_BRACE: that open brace { should be on the
previous line.
Move open brace to new line. Also add space after if/switch statement
since we introduce more checkpatch errors if not fixed at the same
time.
Signed-off-by: Tobin C. Harding
---
drivers/block/cciss_scsi.c | 15 +--
1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/drivers/block/cciss_scsi.c b/drivers/block/cciss_scsi.c
index 2b8cfd4..f5c21f3 100644
--- a/drivers/block/cciss_scsi.c
+++ b/drivers/block/cciss_scsi.c
@@ -699,10 +699,8 @@ static void complete_scsi_command(CommandList_struct *c,
int timeout,
ei->SenseLen);
scsi_set_resid(cmd, ei->ResidualCnt);
- if(ei->CommandStatus != 0)
- { /* an error has occurred */
- switch(ei->CommandStatus)
- {
+ if (ei->CommandStatus != 0) { /* an error has occurred */
+ switch (ei->CommandStatus) {
case CMD_TARGET_STATUS:
/* Pass it up to the upper layers... */
if (!ei->ScsiStatus) {
@@ -902,8 +900,7 @@ cciss_scsi_interpret_error(ctlr_info_t *h,
CommandList_struct *c)
ErrorInfo_struct *ei;
ei = c->err_info;
- switch(ei->CommandStatus)
- {
+ switch (ei->CommandStatus) {
case CMD_TARGET_STATUS:
dev_warn(&h->pdev->dev,
"cmd %p has completed with errors\n", c);
@@ -1182,8 +1179,7 @@ cciss_update_non_disk_devices(ctlr_info_t *h, int hostno)
cciss_scsi_get_device_id(h, scsi3addr,
this_device->device_id, sizeof(this_device->device_id));
- switch (this_device->devtype)
- {
+ switch (this_device->devtype) {
case 0x05: /* CD-ROM */ {
/* We don't *really* support actual CD-ROM devices,
@@ -1414,8 +1410,7 @@ cciss_scsi_queue_command_lck(struct scsi_cmnd *cmd, void
(*done)(struct scsi_cmn
memcpy(c->Request.CDB, cmd->cmnd, cmd->cmd_len);
c->Request.Type.Type = TYPE_CMD;
c->Request.Type.Attribute = ATTR_SIMPLE;
- switch(cmd->sc_data_direction)
- {
+ switch (cmd->sc_data_direction) {
case DMA_TO_DEVICE:
c->Request.Type.Direction = XFER_WRITE;
break;
--
2.7.4
[PATCH 3/3] cciss: Remove kmalloc cast
Coccinelle emits a warning about casting the return value of kmalloc(). Coccinelle suggests removing the cast as do kerneljanitors. Remove cast from kmalloc() call. Signed-off-by: Tobin C. Harding --- drivers/block/cciss_scsi.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/block/cciss_scsi.c b/drivers/block/cciss_scsi.c index f5c21f3..01a1f7e 100644 --- a/drivers/block/cciss_scsi.c +++ b/drivers/block/cciss_scsi.c @@ -647,8 +647,7 @@ cciss_scsi_setup(ctlr_info_t *h) struct cciss_scsi_adapter_data_t * shba; ccissscsi[h->ctlr].ndevices = 0; - shba = (struct cciss_scsi_adapter_data_t *) - kmalloc(sizeof(*shba), GFP_KERNEL); + shba = kmalloc(sizeof(*shba), GFP_KERNEL); if (shba == NULL) return; shba->scsi_host = NULL; -- 2.7.4
[PATCH 0/3] cciss: Fix coccinelle/checkpatch warnings.
Re-send after spell checking. :( Coccinelle warns about unnecessary cast on call to kmalloc(). Checkpatch emits various warnings when parsing file. Clean up two checkpatch warnings. Remove trailing whitespace and clean up opening brace position. Remove unnecessary cast on kmalloc(). Tobin C. Harding (3): cciss: Fix checkpatch TRAILING_WHITESPACE cciss: Fix checkpatch OPEN_BRACE cciss: Remove kmalloc cast drivers/block/cciss_scsi.c | 182 ++--- 1 file changed, 88 insertions(+), 94 deletions(-) -- 2.7.4
[PATCH 1/3] cciss: Fix checkpatch TRAILING_WHITESPACE
Checkpatch emits 85 trailing whitespace warnings.
Remove trailing whitespace.
Signed-off-by: Tobin C. Harding
---
drivers/block/cciss_scsi.c | 170 ++---
1 file changed, 85 insertions(+), 85 deletions(-)
diff --git a/drivers/block/cciss_scsi.c b/drivers/block/cciss_scsi.c
index a18de9d..2b8cfd4 100644
--- a/drivers/block/cciss_scsi.c
+++ b/drivers/block/cciss_scsi.c
@@ -17,15 +17,15 @@
*02111-1307, USA.
*
*Questions/Comments/Bugfixes to iss_storage...@hp.com
- *
+ *
*Author: Stephen M. Cameron
*/
#ifdef CONFIG_CISS_SCSI_TAPE
-/* Here we have code to present the driver as a scsi driver
- as it is simultaneously presented as a block driver. The
+/* Here we have code to present the driver as a scsi driver
+ as it is simultaneously presented as a block driver. The
reason for doing this is to allow access to SCSI tape drives
- through the array controller. Note in particular, neither
+ through the array controller. Note in particular, neither
physical nor logical disks are presented through the scsi layer. */
#include
@@ -37,7 +37,7 @@
#include
#include
-#include
+#include
#include "cciss_scsi.h"
@@ -120,7 +120,7 @@ struct cciss_scsi_adapter_data_t {
struct cciss_scsi_cmd_stack_t cmd_stack;
SGDescriptor_struct **cmd_sg_list;
int registered;
- spinlock_t lock; // to protect ccissscsi[ctlr];
+ spinlock_t lock; // to protect ccissscsi[ctlr];
};
#define CPQ_TAPE_LOCK(h, flags) spin_lock_irqsave( \
@@ -143,36 +143,36 @@ scsi_cmd_alloc(ctlr_info_t *h)
u64bit temp64;
sa = h->scsi_ctlr;
- stk = &sa->cmd_stack;
+ stk = &sa->cmd_stack;
- if (stk->top < 0)
+ if (stk->top < 0)
return NULL;
- c = stk->elem[stk->top];
+ c = stk->elem[stk->top];
/* memset(c, 0, sizeof(*c)); */
memset(&c->cmd, 0, sizeof(c->cmd));
memset(&c->Err, 0, sizeof(c->Err));
/* set physical addr of cmd and addr of scsi parameters */
- c->cmd.busaddr = c->busaddr;
+ c->cmd.busaddr = c->busaddr;
c->cmd.cmdindex = c->cmdindex;
- /* (__u32) (stk->cmd_pool_handle +
+ /* (__u32) (stk->cmd_pool_handle +
(sizeof(struct cciss_scsi_cmd_stack_elem_t)*stk->top)); */
temp64.val = (__u64) (c->busaddr + sizeof(CommandList_struct));
- /* (__u64) (stk->cmd_pool_handle +
+ /* (__u64) (stk->cmd_pool_handle +
(sizeof(struct cciss_scsi_cmd_stack_elem_t)*stk->top) +
sizeof(CommandList_struct)); */
stk->top--;
c->cmd.ErrDesc.Addr.lower = temp64.val32.lower;
c->cmd.ErrDesc.Addr.upper = temp64.val32.upper;
c->cmd.ErrDesc.Len = sizeof(ErrorInfo_struct);
-
+
c->cmd.ctlr = h->ctlr;
c->cmd.err_info = &c->Err;
return (CommandList_struct *) c;
}
-static void
+static void
scsi_cmd_free(ctlr_info_t *h, CommandList_struct *c)
{
/* assume only one process in here at a time, locking done by caller. */
@@ -183,7 +183,7 @@ scsi_cmd_free(ctlr_info_t *h, CommandList_struct *c)
struct cciss_scsi_cmd_stack_t *stk;
sa = h->scsi_ctlr;
- stk = &sa->cmd_stack;
+ stk = &sa->cmd_stack;
stk->top++;
if (stk->top >= stk->nelems) {
dev_err(&h->pdev->dev,
@@ -228,7 +228,7 @@ scsi_cmd_stack_setup(ctlr_info_t *h, struct
cciss_scsi_adapter_data_t *sa)
}
for (i = 0; i < stk->nelems; i++) {
stk->elem[i] = &stk->pool[i];
- stk->elem[i]->busaddr = (__u32) (stk->cmd_pool_handle +
+ stk->elem[i]->busaddr = (__u32) (stk->cmd_pool_handle +
(sizeof(struct cciss_scsi_cmd_stack_elem_t) * i));
stk->elem[i]->cmdindex = i;
}
@@ -244,7 +244,7 @@ scsi_cmd_stack_free(ctlr_info_t *h)
size_t size;
sa = h->scsi_ctlr;
- stk = &sa->cmd_stack;
+ stk = &sa->cmd_stack;
if (stk->top != stk->nelems-1) {
dev_warn(&h->pdev->dev,
"bug: %d scsi commands are still outstanding.\n",
@@ -266,7 +266,7 @@ print_cmd(CommandList_struct *cp)
printk("queue:%d\n", cp->Header.ReplyQueue);
printk("sglist:%d\n", cp->Header.SGList);
printk("sgtot:%d\n", cp->Header.SGTotal);
- printk("Tag:0x%08x/0x%08x\n", cp->Header.Tag.upper,
+ printk("Tag:0x%08x/0x%08x\n", cp->Header.Tag.upper,
cp->Header.Tag.lower);
printk("LUN:0x%8phN\n", cp->Header.LUN.LunAddrByte
[PATCH 0/3] cciss: Fix coccinelle/checkpatch warnings.
Coccinelle warns about unnesesary cast on call to kmalloc(). Checkpatch emits various warnings when parsing file. Clean up two checkpatch warnings. Remove trailing whitespace and clean up opending brace position. Remove unnesesary cast on kmalloc(). Tobin C. Harding (3): cciss: Fix checkpatch TRAILING_WHITESPACE cciss: Fix checkpatch OPEN_BRACE cciss: Remove kmalloc cast drivers/block/cciss_scsi.c | 182 ++--- 1 file changed, 88 insertions(+), 94 deletions(-) -- 2.7.4
[PATCH 2/3] cciss: Fix checkpatch OPEN_BRACE
Checkpatch emits ERROR:OPEN_BRACE: that open brace { should be on the
previous line.
Move open brace to new line. Also add space after if/switch statement
since we introduce more checkpatch errors if not fixed at the same
time.
Signed-off-by: Tobin C. Harding
---
drivers/block/cciss_scsi.c | 15 +--
1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/drivers/block/cciss_scsi.c b/drivers/block/cciss_scsi.c
index 2b8cfd4..f5c21f3 100644
--- a/drivers/block/cciss_scsi.c
+++ b/drivers/block/cciss_scsi.c
@@ -699,10 +699,8 @@ static void complete_scsi_command(CommandList_struct *c,
int timeout,
ei->SenseLen);
scsi_set_resid(cmd, ei->ResidualCnt);
- if(ei->CommandStatus != 0)
- { /* an error has occurred */
- switch(ei->CommandStatus)
- {
+ if (ei->CommandStatus != 0) { /* an error has occurred */
+ switch (ei->CommandStatus) {
case CMD_TARGET_STATUS:
/* Pass it up to the upper layers... */
if (!ei->ScsiStatus) {
@@ -902,8 +900,7 @@ cciss_scsi_interpret_error(ctlr_info_t *h,
CommandList_struct *c)
ErrorInfo_struct *ei;
ei = c->err_info;
- switch(ei->CommandStatus)
- {
+ switch (ei->CommandStatus) {
case CMD_TARGET_STATUS:
dev_warn(&h->pdev->dev,
"cmd %p has completed with errors\n", c);
@@ -1182,8 +1179,7 @@ cciss_update_non_disk_devices(ctlr_info_t *h, int hostno)
cciss_scsi_get_device_id(h, scsi3addr,
this_device->device_id, sizeof(this_device->device_id));
- switch (this_device->devtype)
- {
+ switch (this_device->devtype) {
case 0x05: /* CD-ROM */ {
/* We don't *really* support actual CD-ROM devices,
@@ -1414,8 +1410,7 @@ cciss_scsi_queue_command_lck(struct scsi_cmnd *cmd, void
(*done)(struct scsi_cmn
memcpy(c->Request.CDB, cmd->cmnd, cmd->cmd_len);
c->Request.Type.Type = TYPE_CMD;
c->Request.Type.Attribute = ATTR_SIMPLE;
- switch(cmd->sc_data_direction)
- {
+ switch (cmd->sc_data_direction) {
case DMA_TO_DEVICE:
c->Request.Type.Direction = XFER_WRITE;
break;
--
2.7.4
[PATCH 1/3] cciss: Fix checkpatch TRAILING_WHITESPACE
Checkpatch emits 85 trailing whitespace warnings.
Remove trailing whitespace.
Signed-off-by: Tobin C. Harding
---
drivers/block/cciss_scsi.c | 170 ++---
1 file changed, 85 insertions(+), 85 deletions(-)
diff --git a/drivers/block/cciss_scsi.c b/drivers/block/cciss_scsi.c
index a18de9d..2b8cfd4 100644
--- a/drivers/block/cciss_scsi.c
+++ b/drivers/block/cciss_scsi.c
@@ -17,15 +17,15 @@
*02111-1307, USA.
*
*Questions/Comments/Bugfixes to iss_storage...@hp.com
- *
+ *
*Author: Stephen M. Cameron
*/
#ifdef CONFIG_CISS_SCSI_TAPE
-/* Here we have code to present the driver as a scsi driver
- as it is simultaneously presented as a block driver. The
+/* Here we have code to present the driver as a scsi driver
+ as it is simultaneously presented as a block driver. The
reason for doing this is to allow access to SCSI tape drives
- through the array controller. Note in particular, neither
+ through the array controller. Note in particular, neither
physical nor logical disks are presented through the scsi layer. */
#include
@@ -37,7 +37,7 @@
#include
#include
-#include
+#include
#include "cciss_scsi.h"
@@ -120,7 +120,7 @@ struct cciss_scsi_adapter_data_t {
struct cciss_scsi_cmd_stack_t cmd_stack;
SGDescriptor_struct **cmd_sg_list;
int registered;
- spinlock_t lock; // to protect ccissscsi[ctlr];
+ spinlock_t lock; // to protect ccissscsi[ctlr];
};
#define CPQ_TAPE_LOCK(h, flags) spin_lock_irqsave( \
@@ -143,36 +143,36 @@ scsi_cmd_alloc(ctlr_info_t *h)
u64bit temp64;
sa = h->scsi_ctlr;
- stk = &sa->cmd_stack;
+ stk = &sa->cmd_stack;
- if (stk->top < 0)
+ if (stk->top < 0)
return NULL;
- c = stk->elem[stk->top];
+ c = stk->elem[stk->top];
/* memset(c, 0, sizeof(*c)); */
memset(&c->cmd, 0, sizeof(c->cmd));
memset(&c->Err, 0, sizeof(c->Err));
/* set physical addr of cmd and addr of scsi parameters */
- c->cmd.busaddr = c->busaddr;
+ c->cmd.busaddr = c->busaddr;
c->cmd.cmdindex = c->cmdindex;
- /* (__u32) (stk->cmd_pool_handle +
+ /* (__u32) (stk->cmd_pool_handle +
(sizeof(struct cciss_scsi_cmd_stack_elem_t)*stk->top)); */
temp64.val = (__u64) (c->busaddr + sizeof(CommandList_struct));
- /* (__u64) (stk->cmd_pool_handle +
+ /* (__u64) (stk->cmd_pool_handle +
(sizeof(struct cciss_scsi_cmd_stack_elem_t)*stk->top) +
sizeof(CommandList_struct)); */
stk->top--;
c->cmd.ErrDesc.Addr.lower = temp64.val32.lower;
c->cmd.ErrDesc.Addr.upper = temp64.val32.upper;
c->cmd.ErrDesc.Len = sizeof(ErrorInfo_struct);
-
+
c->cmd.ctlr = h->ctlr;
c->cmd.err_info = &c->Err;
return (CommandList_struct *) c;
}
-static void
+static void
scsi_cmd_free(ctlr_info_t *h, CommandList_struct *c)
{
/* assume only one process in here at a time, locking done by caller. */
@@ -183,7 +183,7 @@ scsi_cmd_free(ctlr_info_t *h, CommandList_struct *c)
struct cciss_scsi_cmd_stack_t *stk;
sa = h->scsi_ctlr;
- stk = &sa->cmd_stack;
+ stk = &sa->cmd_stack;
stk->top++;
if (stk->top >= stk->nelems) {
dev_err(&h->pdev->dev,
@@ -228,7 +228,7 @@ scsi_cmd_stack_setup(ctlr_info_t *h, struct
cciss_scsi_adapter_data_t *sa)
}
for (i = 0; i < stk->nelems; i++) {
stk->elem[i] = &stk->pool[i];
- stk->elem[i]->busaddr = (__u32) (stk->cmd_pool_handle +
+ stk->elem[i]->busaddr = (__u32) (stk->cmd_pool_handle +
(sizeof(struct cciss_scsi_cmd_stack_elem_t) * i));
stk->elem[i]->cmdindex = i;
}
@@ -244,7 +244,7 @@ scsi_cmd_stack_free(ctlr_info_t *h)
size_t size;
sa = h->scsi_ctlr;
- stk = &sa->cmd_stack;
+ stk = &sa->cmd_stack;
if (stk->top != stk->nelems-1) {
dev_warn(&h->pdev->dev,
"bug: %d scsi commands are still outstanding.\n",
@@ -266,7 +266,7 @@ print_cmd(CommandList_struct *cp)
printk("queue:%d\n", cp->Header.ReplyQueue);
printk("sglist:%d\n", cp->Header.SGList);
printk("sgtot:%d\n", cp->Header.SGTotal);
- printk("Tag:0x%08x/0x%08x\n", cp->Header.Tag.upper,
+ printk("Tag:0x%08x/0x%08x\n", cp->Header.Tag.upper,
cp->Header.Tag.lower);
printk("LUN:0x%8phN\n", cp->Header.LUN.LunAddrByte
[PATCH 3/3] cciss: Remove kmalloc cast
Coccinelle emits a warning about casting the return value of kmalloc(). Coccinelle suggests removing the cast as do kerneljanitors. Remove cast from kmalloc() call. Signed-off-by: Tobin C. Harding --- drivers/block/cciss_scsi.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/block/cciss_scsi.c b/drivers/block/cciss_scsi.c index f5c21f3..01a1f7e 100644 --- a/drivers/block/cciss_scsi.c +++ b/drivers/block/cciss_scsi.c @@ -647,8 +647,7 @@ cciss_scsi_setup(ctlr_info_t *h) struct cciss_scsi_adapter_data_t * shba; ccissscsi[h->ctlr].ndevices = 0; - shba = (struct cciss_scsi_adapter_data_t *) - kmalloc(sizeof(*shba), GFP_KERNEL); + shba = kmalloc(sizeof(*shba), GFP_KERNEL); if (shba == NULL) return; shba->scsi_host = NULL; -- 2.7.4
