RE: [PATCH] scsi: megaraid: Convert timers to use timer_setup()

[Shivasharan Srikanteshwara]

> -Original Message-
> From: Sumit Saxena [mailto:sumit.sax...@broadcom.com]
> Sent: Wednesday, November 1, 2017 12:18 AM
> To: Kees Cook; Martin K. Petersen
> Cc: Kashyap Desai; Shivasharan Srikanteshwara; James E.J. Bottomley;
> PDL,MEGARAIDLINUX; linux-scsi@vger.kernel.org; linux-
> ker...@vger.kernel.org
> Subject: RE: [PATCH] scsi: megaraid: Convert timers to use timer_setup()
>
> -Original Message-
> From: Kees Cook [mailto:keesc...@chromium.org]
> Sent: Wednesday, October 25, 2017 3:37 PM
> To: Martin K. Petersen
> Cc: Kashyap Desai; Sumit Saxena; Shivasharan S; James E.J. Bottomley;
> megaraidlinux@broadcom.com; linux-scsi@vger.kernel.org; linux-
> ker...@vger.kernel.org
> Subject: [PATCH] scsi: megaraid: Convert timers to use timer_setup()
>
> In preparation for unconditionally passing the struct timer_list pointer
> to all
> timer callbacks, switch to using the new timer_setup() and
> from_timer() to pass the timer pointer explicitly. Also consolidates the
> timer
> setup functions arguments, which are all identical, and corrects on-stack
> timer
> usage.
>

Ran basic sanity test for megaraid_sas part, and the changes look good.
Acked-by: Shivasharan S <shivasharan.srikanteshw...@broadcom.com>
Tested-by: Shivasharan S <shivasharan.srikanteshw...@broadcom.com>

Hannes,
Can you please take a look at the megaraid_mbox and megaraid_mm changes?

Thanks,
Shivasharan

RE: [PATCH] scsi: megaraid: Convert timers to use timer_setup()

[Sumit Saxena]

-Original Message-
From: Kees Cook [mailto:keesc...@chromium.org]
Sent: Wednesday, October 25, 2017 3:37 PM
To: Martin K. Petersen
Cc: Kashyap Desai; Sumit Saxena; Shivasharan S; James E.J. Bottomley;
megaraidlinux@broadcom.com; linux-scsi@vger.kernel.org;
linux-ker...@vger.kernel.org
Subject: [PATCH] scsi: megaraid: Convert timers to use timer_setup()

In preparation for unconditionally passing the struct timer_list pointer
to all timer callbacks, switch to using the new timer_setup() and
from_timer() to pass the timer pointer explicitly. Also consolidates the
timer setup functions arguments, which are all identical, and corrects
on-stack timer usage.

Cc: Kashyap Desai 
Cc: Sumit Saxena 
Cc: Shivasharan S 
Cc: "James E.J. Bottomley" 
Cc: "Martin K. Petersen" 
Cc: megaraidlinux@broadcom.com
Cc: linux-scsi@vger.kernel.org
Signed-off-by: Kees Cook 
---
 drivers/scsi/megaraid/megaraid_ioctl.h  |  6 +
 drivers/scsi/megaraid/megaraid_mbox.c   | 26 ++---
 drivers/scsi/megaraid/megaraid_mm.c | 27 +++---
 drivers/scsi/megaraid/megaraid_sas_base.c   | 35
+++--
 drivers/scsi/megaraid/megaraid_sas_fusion.c | 15 +++--
 5 files changed, 47 insertions(+), 62 deletions(-)

diff --git a/drivers/scsi/megaraid/megaraid_ioctl.h
b/drivers/scsi/megaraid/megaraid_ioctl.h
index 05f6e4ec3453..eedcbde46459 100644
--- a/drivers/scsi/megaraid/megaraid_ioctl.h
+++ b/drivers/scsi/megaraid/megaraid_ioctl.h
@@ -19,6 +19,7 @@

 #include 
 #include 
+#include 

 #include "mbox_defs.h"

@@ -153,6 +154,11 @@ typedef struct uioc {

 } __attribute__ ((aligned(1024),packed)) uioc_t;

+/* For on-stack uioc timers. */
+struct uioc_timeout {
+   struct timer_list timer;
+   uioc_t*uioc;
+};

 /**
  * struct mraid_hba_info - information about the controller diff --git
a/drivers/scsi/megaraid/megaraid_mbox.c
b/drivers/scsi/megaraid/megaraid_mbox.c
index ec3c43854978..530358cdcb39 100644
--- a/drivers/scsi/megaraid/megaraid_mbox.c
+++ b/drivers/scsi/megaraid/megaraid_mbox.c
@@ -3904,19 +3904,19 @@ megaraid_sysfs_get_ldmap_done(uioc_t *uioc)
wake_up(_dev->sysfs_wait_q);
 }

-
 /**
  * megaraid_sysfs_get_ldmap_timeout - timeout handling for get ldmap
- * @data   : timed out packet
+ * @t  : timed out timer
  *
  * Timeout routine to recover and return to application, in case the
adapter
  * has stopped responding. A timeout of 60 seconds for this command seems
like
  * a good value.
  */
 static void
-megaraid_sysfs_get_ldmap_timeout(unsigned long data)
+megaraid_sysfs_get_ldmap_timeout(struct timer_list *t)
 {
-   uioc_t  *uioc = (uioc_t *)data;
+   struct uioc_timeout *timeout = from_timer(timeout, t, timer);
+   uioc_t  *uioc = timeout->uioc;
adapter_t   *adapter = (adapter_t *)uioc->buf_vaddr;
mraid_device_t  *raid_dev = ADAP2RAIDDEV(adapter);

@@ -3951,8 +3951,7 @@ megaraid_sysfs_get_ldmap(adapter_t *adapter)
mbox64_t*mbox64;
mbox_t  *mbox;
char*raw_mbox;
-   struct timer_list   sysfs_timer;
-   struct timer_list   *timerp;
+   struct uioc_timeout timeout;
caddr_t ldmap;
int rval = 0;

@@ -3988,14 +3987,12 @@ megaraid_sysfs_get_ldmap(adapter_t *adapter)
/*
 * Setup a timer to recover from a non-responding controller
 */
-   timerp  = _timer;
-   init_timer(timerp);
-
-   timerp->function= megaraid_sysfs_get_ldmap_timeout;
-   timerp->data= (unsigned long)uioc;
-   timerp->expires = jiffies + 60 * HZ;
+   timeout.uioc = uioc;
+   timer_setup_on_stack(,
+megaraid_sysfs_get_ldmap_timeout, 0);

Kees,
Does calling "timer_setup_on_stack" instead of "timer_setup" intentional ?
If yes, please help me understand the reason.
Otherwise changes look good to me.

-   add_timer(timerp);
+   timeout.timer.expires   = jiffies + 60 * HZ;
+   add_timer();

/*
 * Send the command to the firmware
@@ -4033,7 +4030,8 @@ megaraid_sysfs_get_ldmap(adapter_t *adapter)
}


-   del_timer_sync(timerp);
+   del_timer_sync();
+   destroy_timer_on_stack();

mutex_unlock(_dev->sysfs_mtx);

diff --git a/drivers/scsi/megaraid/megaraid_mm.c
b/drivers/scsi/megaraid/megaraid_mm.c
index 65b6f6ace3a5..bb802b0c12b8 100644
--- a/drivers/scsi/megaraid/megaraid_mm.c
+++ b/drivers/scsi/megaraid/megaraid_mm.c
@@ -35,7 +35,7 @@ static int kioc_to_mimd(uioc_t *, mimd_t __user *);
static int handle_drvrcmd(void __user *, uint8_t, int *);  static int
lld_ioctl(mraid_mmadp_t *, uioc_t *);  static void ioctl_done(uioc_t *);
-static 

Re: [PATCH] scsi: megaraid: Convert timers to use timer_setup()

[Martin K. Petersen]

Kees,

> In preparation for unconditionally passing the struct timer_list
> pointer to all timer callbacks, switch to using the new timer_setup()
> and from_timer() to pass the timer pointer explicitly. Also
> consolidates the timer setup functions arguments, which are all
> identical, and corrects on-stack timer usage.

Same with this one. Broadcom folks, please review!

Reviewed-by: Martin K. Petersen 

-- 
Martin K. Petersen  Oracle Linux Engineering