[PATCH 0/4] scsi: qla2xxx: Convert timers to use timer_setup()

[PATCH 0/4] scsi: qla2xxx: Convert timers to use timer_setup()

[Kees Cook]

This breaks out the logical steps to convert the qla2xxx timers:

1) init_timer() -> setup_timer()
2) refactor qla2x00_start_timer() to not pass callback as argument
3) qla2x00_timer() to use timer_setup()
4) qla2x00_sp_timeout() to use timer_setup()

The resulting diff is identical to the patch that appears to lock up
the driver. This should help identify which step causes this behavior.

-Kees

[PATCH 1/4] scsi: qla2xxx: Convert timers to use setup_timer()

[Kees Cook]

In preparation for unconditionally passing the struct timer_list pointer to
all timer callbacks, take the first step by switching from init_timer() and
open-coded .function and .data assignments to using the old setup_timer()
API.

Cc: Himanshu Madhani <Himanshu.Madhani@cavium.com>
Cc: Bart Van Assche <bart.vanassche@wdc.com>
Cc: qla2xxx-upstream@qlogic.com
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: linux-scsi@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 drivers/scsi/qla2xxx/qla_inline.h | 5 ++---
 drivers/scsi/qla2xxx/qla_os.c     | 5 ++---
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_inline.h b/drivers/scsi/qla2xxx/qla_inline.h
index 9a2c86eacf44..34cdb5d9c87c 100644
--- a/drivers/scsi/qla2xxx/qla_inline.h
+++ b/drivers/scsi/qla2xxx/qla_inline.h
@@ -269,10 +269,9 @@ qla2x00_rel_sp(srb_t *sp)
 static inline void
 qla2x00_init_timer(srb_t *sp, unsigned long tmo)
 {
-	init_timer(&sp->u.iocb_cmd.timer);
+	setup_timer(&sp->u.iocb_cmd.timer, qla2x00_sp_timeout,
+		    (unsigned long)sp);
 	sp->u.iocb_cmd.timer.expires = jiffies + tmo * HZ;
-	sp->u.iocb_cmd.timer.data = (unsigned long)sp;
-	sp->u.iocb_cmd.timer.function = qla2x00_sp_timeout;
 	add_timer(&sp->u.iocb_cmd.timer);
 	sp->free = qla2x00_sp_free;
 	if (IS_QLAFX00(sp->vha->hw) && (sp->type == SRB_FXIOCB_DCMD))
diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
index 3bd956d3bc5d..6c10a7b970c0 100644
--- a/drivers/scsi/qla2xxx/qla_os.c
+++ b/drivers/scsi/qla2xxx/qla_os.c
@@ -332,10 +332,9 @@ struct scsi_transport_template *qla2xxx_transport_vport_template = NULL;
 __inline__ void
 qla2x00_start_timer(scsi_qla_host_t *vha, void *func, unsigned long interval)
 {
-	init_timer(&vha->timer);
+	setup_timer(&vha->timer, (void (*)(unsigned long))func,
+		    (unsigned long)vha);
 	vha->timer.expires = jiffies + interval * HZ;
-	vha->timer.data = (unsigned long)vha;
-	vha->timer.function = (void (*)(unsigned long))func;
 	add_timer(&vha->timer);
 	vha->timer_active = 1;
 }
-- 
2.7.4

[PATCH 2/4] scsi: qla2xxx: Refactor qla2x00_start_timer()

[Kees Cook]

In preparation for unconditionally passing the struct timer_list pointer to
all timer callbacks, this refactors qla2x00_start_timer() to not pass a
callback argument, since all callers use the same callback.

Cc: Himanshu Madhani <Himanshu.Madhani@cavium.com>
Cc: Bart Van Assche <bart.vanassche@wdc.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: qla2xxx-upstream@qlogic.com
Cc: linux-scsi@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 drivers/scsi/qla2xxx/qla_gbl.h | 2 +-
 drivers/scsi/qla2xxx/qla_mid.c | 2 +-
 drivers/scsi/qla2xxx/qla_os.c  | 6 +++---
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_gbl.h b/drivers/scsi/qla2xxx/qla_gbl.h
index f852ca60c49f..541a087cdb45 100644
--- a/drivers/scsi/qla2xxx/qla_gbl.h
+++ b/drivers/scsi/qla2xxx/qla_gbl.h
@@ -207,7 +207,7 @@ int qla24xx_async_abort_cmd(srb_t *);
 extern struct scsi_host_template qla2xxx_driver_template;
 extern struct scsi_transport_template *qla2xxx_transport_vport_template;
 extern void qla2x00_timer(scsi_qla_host_t *);
-extern void qla2x00_start_timer(scsi_qla_host_t *, void *, unsigned long);
+extern void qla2x00_start_timer(scsi_qla_host_t *, unsigned long);
 extern void qla24xx_deallocate_vp_id(scsi_qla_host_t *);
 extern int qla24xx_disable_vp (scsi_qla_host_t *);
 extern int qla24xx_enable_vp (scsi_qla_host_t *);
diff --git a/drivers/scsi/qla2xxx/qla_mid.c b/drivers/scsi/qla2xxx/qla_mid.c
index c0f8f6c17b79..cbf544dbf883 100644
--- a/drivers/scsi/qla2xxx/qla_mid.c
+++ b/drivers/scsi/qla2xxx/qla_mid.c
@@ -487,7 +487,7 @@ qla24xx_create_vhost(struct fc_vport *fc_vport)
 	atomic_set(&vha->loop_state, LOOP_DOWN);
 	atomic_set(&vha->loop_down_timer, LOOP_DOWN_TIME);
 
-	qla2x00_start_timer(vha, qla2x00_timer, WATCH_INTERVAL);
+	qla2x00_start_timer(vha, WATCH_INTERVAL);
 
 	vha->req = base_vha->req;
 	host->can_queue = base_vha->req->length + 128;
diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
index 6c10a7b970c0..7e7e5d095c26 100644
--- a/drivers/scsi/qla2xxx/qla_os.c
+++ b/drivers/scsi/qla2xxx/qla_os.c
@@ -330,9 +330,9 @@ struct scsi_transport_template *qla2xxx_transport_vport_template = NULL;
  */
 
 __inline__ void
-qla2x00_start_timer(scsi_qla_host_t *vha, void *func, unsigned long interval)
+qla2x00_start_timer(scsi_qla_host_t *vha, unsigned long interval)
 {
-	setup_timer(&vha->timer, (void (*)(unsigned long))func,
+	setup_timer(&vha->timer, (void (*)(unsigned long))qla2x00_timer,
 		    (unsigned long)vha);
 	vha->timer.expires = jiffies + interval * HZ;
 	add_timer(&vha->timer);
@@ -3245,7 +3245,7 @@ qla2x00_probe_one(struct pci_dev *pdev, const struct pci_device_id *id)
 	base_vha->host->irq = ha->pdev->irq;
 
 	/* Initialized the timer */
-	qla2x00_start_timer(base_vha, qla2x00_timer, WATCH_INTERVAL);
+	qla2x00_start_timer(base_vha, WATCH_INTERVAL);
 	ql_dbg(ql_dbg_init, base_vha, 0x00ef,
 	    "Started qla2x00_timer with "
 	    "interval=%d.\n", WATCH_INTERVAL);
-- 
2.7.4

[PATCH 3/4] scsi: qla2xxx: Convert qla2x00_timer() to use timer_setup()

[Kees Cook]

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 for the qla2x00_timer() callback and
associated timer.

Cc: Himanshu Madhani <Himanshu.Madhani@cavium.com>
Cc: Bart Van Assche <bart.vanassche@wdc.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: qla2xxx-upstream@qlogic.com
Cc: linux-scsi@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 drivers/scsi/qla2xxx/qla_gbl.h | 2 +-
 drivers/scsi/qla2xxx/qla_os.c  | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_gbl.h b/drivers/scsi/qla2xxx/qla_gbl.h
index 541a087cdb45..ab5b88203886 100644
--- a/drivers/scsi/qla2xxx/qla_gbl.h
+++ b/drivers/scsi/qla2xxx/qla_gbl.h
@@ -206,7 +206,7 @@ int qla24xx_async_abort_cmd(srb_t *);
  */
 extern struct scsi_host_template qla2xxx_driver_template;
 extern struct scsi_transport_template *qla2xxx_transport_vport_template;
-extern void qla2x00_timer(scsi_qla_host_t *);
+extern void qla2x00_timer(struct timer_list *);
 extern void qla2x00_start_timer(scsi_qla_host_t *, unsigned long);
 extern void qla24xx_deallocate_vp_id(scsi_qla_host_t *);
 extern int qla24xx_disable_vp (scsi_qla_host_t *);
diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
index 7e7e5d095c26..f97e585dee77 100644
--- a/drivers/scsi/qla2xxx/qla_os.c
+++ b/drivers/scsi/qla2xxx/qla_os.c
@@ -332,8 +332,7 @@ struct scsi_transport_template *qla2xxx_transport_vport_template = NULL;
 __inline__ void
 qla2x00_start_timer(scsi_qla_host_t *vha, unsigned long interval)
 {
-	setup_timer(&vha->timer, (void (*)(unsigned long))qla2x00_timer,
-		    (unsigned long)vha);
+	timer_setup(&vha->timer, qla2x00_timer, 0);
 	vha->timer.expires = jiffies + interval * HZ;
 	add_timer(&vha->timer);
 	vha->timer_active = 1;
@@ -5994,8 +5993,9 @@ qla2x00_rst_aen(scsi_qla_host_t *vha)
 * Context: Interrupt
 ***************************************************************************/
 void
-qla2x00_timer(scsi_qla_host_t *vha)
+qla2x00_timer(struct timer_list *t)
 {
+	scsi_qla_host_t *vha = from_timer(vha, t, timer);
 	unsigned long	cpu_flags = 0;
 	int		start_dpc = 0;
 	int		index;
-- 
2.7.4

[PATCH 4/4] scsi: qla2xxx: Convert qla2x00_sp_timeout() to use timer_setup()

[Kees Cook]

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 for the
qla2x00_sp_timeout() callback and associated timer.

Cc: Himanshu Madhani <Himanshu.Madhani@cavium.com>
Cc: Bart Van Assche <bart.vanassche@wdc.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: qla2xxx-upstream@qlogic.com
Cc: linux-scsi@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 drivers/scsi/qla2xxx/qla_gbl.h    | 2 +-
 drivers/scsi/qla2xxx/qla_init.c   | 4 ++--
 drivers/scsi/qla2xxx/qla_inline.h | 3 +--
 3 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_gbl.h b/drivers/scsi/qla2xxx/qla_gbl.h
index ab5b88203886..3ad375f85b59 100644
--- a/drivers/scsi/qla2xxx/qla_gbl.h
+++ b/drivers/scsi/qla2xxx/qla_gbl.h
@@ -753,7 +753,7 @@ extern int qla82xx_restart_isp(scsi_qla_host_t *);
 /* IOCB related functions */
 extern int qla82xx_start_scsi(srb_t *);
 extern void qla2x00_sp_free(void *);
-extern void qla2x00_sp_timeout(unsigned long);
+extern void qla2x00_sp_timeout(struct timer_list *);
 extern void qla2x00_bsg_job_done(void *, int);
 extern void qla2x00_bsg_sp_free(void *);
 extern void qla2x00_start_iocbs(struct scsi_qla_host *, struct req_que *);
diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c
index b5b48ddca962..44cf875a484a 100644
--- a/drivers/scsi/qla2xxx/qla_init.c
+++ b/drivers/scsi/qla2xxx/qla_init.c
@@ -45,9 +45,9 @@ static void qla24xx_handle_prli_done_event(struct scsi_qla_host *,
 /* SRB Extensions ---------------------------------------------------------- */
 
 void
-qla2x00_sp_timeout(unsigned long __data)
+qla2x00_sp_timeout(struct timer_list *t)
 {
-	srb_t *sp = (srb_t *)__data;
+	srb_t *sp = from_timer(sp, t, u.iocb_cmd.timer);
 	struct srb_iocb *iocb;
 	scsi_qla_host_t *vha = sp->vha;
 	struct req_que *req;
diff --git a/drivers/scsi/qla2xxx/qla_inline.h b/drivers/scsi/qla2xxx/qla_inline.h
index 34cdb5d9c87c..17d2c20f1f75 100644
--- a/drivers/scsi/qla2xxx/qla_inline.h
+++ b/drivers/scsi/qla2xxx/qla_inline.h
@@ -269,8 +269,7 @@ qla2x00_rel_sp(srb_t *sp)
 static inline void
 qla2x00_init_timer(srb_t *sp, unsigned long tmo)
 {
-	setup_timer(&sp->u.iocb_cmd.timer, qla2x00_sp_timeout,
-		    (unsigned long)sp);
+	timer_setup(&sp->u.iocb_cmd.timer, qla2x00_sp_timeout, 0);
 	sp->u.iocb_cmd.timer.expires = jiffies + tmo * HZ;
 	add_timer(&sp->u.iocb_cmd.timer);
 	sp->free = qla2x00_sp_free;
-- 
2.7.4

Re: [PATCH 0/4] scsi: qla2xxx: Convert timers to use timer_setup()

[Kees Cook]

On Tue, Oct 31, 2017 at 12:13 PM, Kees Cook <keescook@chromium.org> wrote:
> This breaks out the logical steps to convert the qla2xxx timers:
>
> 1) init_timer() -> setup_timer()
> 2) refactor qla2x00_start_timer() to not pass callback as argument
> 3) qla2x00_timer() to use timer_setup()
> 4) qla2x00_sp_timeout() to use timer_setup()
>
> The resulting diff is identical to the patch that appears to lock up
> the driver. This should help identify which step causes this behavior.

Hi, just curious if there's been any progress on debugging the issue
you saw with this series?

Thanks!

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH 0/4] scsi: qla2xxx: Convert timers to use timer_setup()

[Madhani, Himanshu]

Hi Kees, 

> On Nov 1, 2017, at 11:46 AM, Kees Cook <keescook@chromium.org> wrote:
> 
> On Tue, Oct 31, 2017 at 12:13 PM, Kees Cook <keescook@chromium.org> wrote:
>> This breaks out the logical steps to convert the qla2xxx timers:
>> 
>> 1) init_timer() -> setup_timer()
>> 2) refactor qla2x00_start_timer() to not pass callback as argument
>> 3) qla2x00_timer() to use timer_setup()
>> 4) qla2x00_sp_timeout() to use timer_setup()
>> 
>> The resulting diff is identical to the patch that appears to lock up
>> the driver. This should help identify which step causes this behavior.
> 
> Hi, just curious if there's been any progress on debugging the issue
> you saw with this series?
> 

Sorry for delay. I will test this series later today and post update. 

> Thanks!
> 
> -Kees
> 
> -- 
> Kees Cook
> Pixel Security

Thanks,
- Himanshu

Re: [PATCH 0/4] scsi: qla2xxx: Convert timers to use timer_setup()

[Bart Van Assche]

On Wed, 2017-11-01 at 19:18 +0000, Madhani, Himanshu wrote:
> Hi Kees, 
> 
> > On Nov 1, 2017, at 11:46 AM, Kees Cook <keescook@chromium.org> wrote:
> > 
> > On Tue, Oct 31, 2017 at 12:13 PM, Kees Cook <keescook@chromium.org> wrote:
> > > This breaks out the logical steps to convert the qla2xxx timers:
> > > 
> > > 1) init_timer() -> setup_timer()
> > > 2) refactor qla2x00_start_timer() to not pass callback as argument
> > > 3) qla2x00_timer() to use timer_setup()
> > > 4) qla2x00_sp_timeout() to use timer_setup()
> > > 
> > > The resulting diff is identical to the patch that appears to lock up
> > > the driver. This should help identify which step causes this behavior.
> > 
> > Hi, just curious if there's been any progress on debugging the issue
> > you saw with this series?
> 
> Sorry for delay. I will test this series later today and post update. 

Hello Himanshu and Kees,

The qla2xxx driver does not crash if apply these four patches on kernel
v4.14-rc8 and load the qla2xxx driver on my test setup. Point-to-point
mode seems broken again but I don't think that's related to this patch
series.

Bart.

Re: [PATCH 0/4] scsi: qla2xxx: Convert timers to use timer_setup()

[Madhani, Himanshu]

Hi Bart, 

> On Nov 5, 2017, at 4:18 PM, Bart Van Assche <bart.vanassche@wdc.com> wrote:
> 
> On Wed, 2017-11-01 at 19:18 +0000, Madhani, Himanshu wrote:
>> Hi Kees, 
>> 
>>> On Nov 1, 2017, at 11:46 AM, Kees Cook <keescook@chromium.org> wrote:
>>> 
>>> On Tue, Oct 31, 2017 at 12:13 PM, Kees Cook <keescook@chromium.org> wrote:
>>>> This breaks out the logical steps to convert the qla2xxx timers:
>>>> 
>>>> 1) init_timer() -> setup_timer()
>>>> 2) refactor qla2x00_start_timer() to not pass callback as argument
>>>> 3) qla2x00_timer() to use timer_setup()
>>>> 4) qla2x00_sp_timeout() to use timer_setup()
>>>> 
>>>> The resulting diff is identical to the patch that appears to lock up
>>>> the driver. This should help identify which step causes this behavior.
>>> 
>>> Hi, just curious if there's been any progress on debugging the issue
>>> you saw with this series?
>> 
>> Sorry for delay. I will test this series later today and post update. 
> 
> Hello Himanshu and Kees,
> 
> The qla2xxx driver does not crash if apply these four patches on kernel
> v4.14-rc8 and load the qla2xxx driver on my test setup. Point-to-point
> mode seems broken again but I don't think that's related to this patch
> series.
> 
> Bart.

What kind of errors you are seeing with Point-to-point mode. I’ll look at those
and will submit fixes for it along with the other issues you have raised earlier.

Kees, 

I’ll get back to validating the series again now. Sorry for delay. 

Thanks,
- Himanshu

Re: [PATCH 0/4] scsi: qla2xxx: Convert timers to use timer_setup()

[Bart Van Assche]

[-- Attachment #1: Type: text/plain, Size: 1242 bytes --]

On Mon, 2017-11-06 at 19:58 +0000, Madhani, Himanshu wrote:
> On Nov 5, 2017, at 4:18 PM, Bart Van Assche <bart.vanassche@wdc.com> wrote:
> > The qla2xxx driver does not crash if apply these four patches on kernel
> > v4.14-rc8 and load the qla2xxx driver on my test setup. Point-to-point
> > mode seems broken again but I don't think that's related to this patch
> > series.
> 
> What kind of errors you are seeing with Point-to-point mode. I’ll look at those
> and will submit fixes for it along with the other issues you have raised earlier.

Hello Himanshu,

The test I ran is as follows (this test passed in the past):
* Connect the two ports of a dual-port HBA to each other.
* Configure qemu such that it allows the guest VM to access the FC HBA on
  the host.
* Start the VM and build the Linux kernel with debug options enabled.
* After having booted the debug kernel, restart LIO by running the
  restart-lio-fc script (attached).
* After the script finished, run lsscsi. Several LIO LUNs should be shown.

The script should finish after a few seconds but with the latest kernel it
takes much longer and no LIO LUNs are shown in the lsscsi output.

Please let me know if you need more information.

Bart.

[-- Attachment #2: restart-lio-fc --]
[-- Type: application/x-shellscript, Size: 4820 bytes --]

[-- Attachment #3: tcm_node.sh --]
[-- Type: application/x-shellscript, Size: 639 bytes --]