scsi: qla2xxx: Convert timers to use timer_setup()

PATCH NAK Kees Cook 4 participants 8 messages Oct 25, 2017 – Oct 31, 2017

Subsystems (2)

SCSI QLogic QLA2XXX FC-SCSI driver

lore

Share & tools

lore

Kees Cook

author

2017-10-25 10:07 UTC #0 lore raw

PATCH 1/1 scsi: qla2xxx: 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.

Cc [email protected]

Cc "James E.J. Bottomley" <[email protected]>

Cc "Martin K. Petersen" <[email protected]>

Cc [email protected]

Signed-off-by Kees Cook <[email protected]>

---
 drivers/scsi/qla2xxx/qla_gbl.h    |  6 +++---
 drivers/scsi/qla2xxx/qla_init.c   |  4 ++--
 drivers/scsi/qla2xxx/qla_inline.h |  4 +---
 drivers/scsi/qla2xxx/qla_mid.c    |  2 +-
 drivers/scsi/qla2xxx/qla_os.c     | 11 +++++------
 5 files changed, 12 insertions(+), 15 deletions(-)

drivers/scsi/qla2xxx/qla_gbl.h +3 -3

index f852ca60c49f..3ad375f85b59 100644
@@ -206,8 +206,8 @@ 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_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 *);
 extern int qla24xx_enable_vp (scsi_qla_host_t *);

@@ -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 *);

drivers/scsi/qla2xxx/qla_init.c +2 -2

index 4c53199db371..0f834f7a923a 100644
@@ -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;

drivers/scsi/qla2xxx/qla_inline.h +1 -3

index 9a2c86eacf44..17d2c20f1f75 100644
@@ -269,10 +269,8 @@ 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);
+	timer_setup(&sp->u.iocb_cmd.timer, qla2x00_sp_timeout, 0);
 	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))

drivers/scsi/qla2xxx/qla_mid.c +1 -1

index c0f8f6c17b79..cbf544dbf883 100644
@@ -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;

drivers/scsi/qla2xxx/qla_os.c +5 -6

index 937209805baf..3b585d4e548d 100644
@@ -330,12 +330,10 @@ 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)
 {
-	init_timer(&vha->timer);
+	timer_setup(&vha->timer, qla2x00_timer, 0);
 	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;
 }

@@ -3246,7 +3244,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);

@@ -5995,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;

Signature · 6 lines

2.7.4


-- 
Kees Cook
Pixel Security

Martin K. Petersen

Reviewed-by on v1 1/1

2017-10-31 15:49 UTC #1 lore raw

Kees,

Quoted 3 lines

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.

Cavium folks: Please verify!

Reviewed-by Martin K. Petersen <[email protected]>

Signature · 1 line

Martin K. Petersen	Oracle Linux Engineering

Madhani, Himanshu

NAK

2017-10-31 18:03 UTC #2 lore raw

Hi Martin/Kees,

Quoted 11 lines · Martin K. Petersen

Kees,
Quoted 3 lines
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.
Cavium folks: Please verify!

I’ve checked on my setup and i am seeing regression with this patch applied.

Driver load locks up the system with this patch applied.

NACK

Quoted 4 lines

Reviewed-by Martin K. Petersen <[email protected]>
Signature · 1 line
Martin K. Petersen	Oracle Linux Engineering

Thanks,
- Himanshu

Bart Van Assche

2017-10-31 18:12 UTC #3 lore raw

Quoted 5 lines · Himanshu

Quoted 4 lines · Martin K. Petersen
Quoted 3 lines
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.

Quoted 7 lines

Quoted 1 line
Cavium folks: Please verify!
I’ve checked on my setup and i am seeing regression with this patch applied.
Driver load locks up the system with this patch applied.
NACK

That feedback is not very helpful ...

Anyway, what kernel source tree did you use in your testing? I may be able to free up some time to look into this myself.

Bart.

Madhani, Himanshu

2017-10-31 18:16 UTC #4 lore raw

Hi Bart,

Quoted 10 lines · Bart Van Assche

Quoted 7 lines · Himanshu
Quoted 5 lines · Martin K. Petersen
Quoted 3 lines
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.
Cavium folks: Please verify!

Quoted 8 lines

Quoted 5 lines
I’ve checked on my setup and i am seeing regression with this patch applied.
Driver load locks up the system with this patch applied.
NACK
That feedback is not very helpful …

Here’s my setup details

4.14.0-rc7 + scsi_misc-4.15 qla2xxx patches from Martin’s tree + current patch.

System has 3 adapters 8G/16G/32G.

Quoted 4 lines

Anyway, what kernel source tree did you use in your testing? I may be able to free up some time to look into this myself.
Bart.

Thanks,
- Himanshu

Kees Cook

author

2017-10-31 18:28 UTC #5 lore raw

Quoted 15 lines · Himanshu

Hi Bart,
Quoted 13 lines · Bart Van Assche
Quoted 10 lines · Himanshu
Quoted 5 lines · Martin K. Petersen
Quoted 3 lines
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.
Cavium folks: Please verify!
I’ve checked on my setup and i am seeing regression with this patch applied.
Driver load locks up the system with this patch applied.

How very strange. I don't see any code change with this patch. Even the passed arguments are the same; it's only enforcing the types now. And the system is fine with only this patch reverted?

-Kees

Quoted 19 lines

Quoted 5 lines
Quoted 2 lines
NACK
That feedback is not very helpful …
Here’s my setup details
4.14.0-rc7 + scsi_misc-4.15 qla2xxx patches from Martin’s tree + current patch.
System has 3 adapters 8G/16G/32G.
Quoted 4 lines
Anyway, what kernel source tree did you use in your testing? I may be able to free up some time to look into this myself.
Bart.
Thanks,
- Himanshu

Signature · 2 lines

Kees Cook
Pixel Security

Madhani, Himanshu

2017-10-31 18:36 UTC #6 lore raw

Hi Kees,

Quoted 5 lines · Kees Cook

How very strange. I don't see any code change with this patch. Even the passed arguments are the same; it's only enforcing the types now. And the system is fine with only this patch reverted?

Yes. Agree patch looks okay, but with the patch built into driver I see system going into infinite loop of trying to bring link up and lock up after few minutes, Only way to recover is hard reboot.

I reverted this patch after system boot up and rebuild qla2xxx and able to load driver.

Thanks,
- Himanshu

Kees Cook

author

2017-10-31 18:45 UTC #7 lore raw

Quoted 8 lines · Himanshu

Hi Kees,
Quoted 5 lines · Kees Cook
How very strange. I don't see any code change with this patch. Even the passed arguments are the same; it's only enforcing the types now. And the system is fine with only this patch reverted?

Quoted 6 lines

Yes. Agree patch looks okay, but with the patch built into driver I see system going into infinite loop of trying to bring link up and lock up after few minutes, Only way to recover is hard reboot.
I reverted this patch after system boot up and rebuild qla2xxx and able to load driver.

The patch is a result of three logical steps, so maybe something unexpected is happening. I'll send the 3 steps broken out...

-Kees

Signature · 2 lines

Kees Cook
Pixel Security