RE: [PATCH] scsi: megaraid: Convert timers to use timer_setup()
> -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-s...@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-s...@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()
> -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-s...@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-s...@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 Tested-by: Shivasharan S 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()
-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-s...@vger.kernel.org; linux-kernel@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 DesaiCc: Sumit Saxena Cc: Shivasharan S Cc: "James E.J. Bottomley" Cc: "Martin K. Petersen" Cc: megaraidlinux@broadcom.com Cc: linux-s...@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()
-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-s...@vger.kernel.org; linux-kernel@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-s...@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 void lld_timedout(unsigned long); +static void lld_timedout(struct timer_list *); static void hinfo_to_cinfo(mraid_hba_info_t *, mcontroller_t *); static mraid_mmadp_t
Re: [PATCH] scsi: megaraid: Convert timers to use timer_setup()
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
Re: [PATCH] scsi: megaraid: Convert timers to use timer_setup()
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