bnx2fc: cleanup task management IO when it times out.

PATCH Bhanu Prakash Gollapudi 2 participants 4 messages May 7, 2012 – May 8, 2012 1 message not in the archive SCSI

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Bhanu Prakash Gollapudi

author

2012-05-07 23:31 UTC #1 lore raw

Quoted 11 lines · Mike Christie

Review
Quoted 1 line · Bhanu Prakash Gollapudi
(quoted diff) +0 -0
b/drivers/scsi/bnx2fc/bnx2fc_tgt.c
drivers/scsi/bnx2fc/bnx2fc_tgt.c void bnx2fc_flush_active_ios(struct bnx2fc_rport
Quoted 4 lines
index d3ee231..082a25c 100644
drivers/scsi/bnx2fc/bnx2fc_tgt.c +0 -0
@@ -185,6 +185,16 @@ void bnx2fc_flush_active_ios(struct bnx2fc_rport
*tgt)
Quoted 3 lines
drivers/scsi/bnx2fc/bnx2fc_tgt.c +0 -0
 BUG_ON(rc);
 }
 

Quoted 7 lines

drivers/scsi/bnx2fc/bnx2fc_tgt.c void bnx2fc_flush_active_ios(struct bnx2fc_rport · line 190
Quoted 3 lines
drivers/scsi/bnx2fc/bnx2fc_tgt.c +3 -0
+ list_for_each_safe(list, tmp, &tgt->active_tm_queue) {
+ i++;
+ io_req = (struct bnx2fc_cmd *)list;
Why didn't you use list_for_each_entry_safe()? Or, when using list_for_each_safe, instead of the cast are we supposed to be using list_entry()?

Mike, the list is the first field in the structure, so the cast here is right.

Thanks,
Bhanu

Quoted 12 lines

drivers/scsi/bnx2fc/bnx2fc_tgt.c +7 -0

+ list_del_init(&io_req->link);
+ io_req->on_tmf_queue = 0;
+ BNX2FC_IO_DBG(io_req, "tm_queue cleanup\n");
+ if (io_req->wait_for_comp)
+ complete(&io_req->tm_done);
+ }
+
 list_for_each_safe(list, tmp, &tgt->els_queue) {
 i++;
 io_req = (struct bnx2fc_cmd *)list;

Mike Christie

2012-05-08 00:15 UTC #2 lore raw

Quoted 16 lines · Bhanu Prakash Gollapudi

Quoted 15 lines · Mike Christie
Review
Quoted 1 line · Bhanu Prakash Gollapudi
(quoted diff) +0 -0
b/drivers/scsi/bnx2fc/bnx2fc_tgt.c
drivers/scsi/bnx2fc/bnx2fc_tgt.c void bnx2fc_flush_active_ios(struct bnx2fc_rport
Quoted 4 lines
index d3ee231..082a25c 100644
drivers/scsi/bnx2fc/bnx2fc_tgt.c +0 -0
@@ -185,6 +185,16 @@ void bnx2fc_flush_active_ios(struct bnx2fc_rport
*tgt)
Quoted 6 lines
drivers/scsi/bnx2fc/bnx2fc_tgt.c +3 -0
 BUG_ON(rc);
 }
 
+ list_for_each_safe(list, tmp, &tgt->active_tm_queue) {
+ i++;
+ io_req = (struct bnx2fc_cmd *)list;

Quoted 7 lines

Quoted 3 lines
Why didn't you use list_for_each_entry_safe()? Or, when using list_for_each_safe, instead of the cast are we supposed to be using list_entry()?
Mike, the list is the first field in the structure, so the cast here is right.

I am saying that normally if there is a function that does the same thing we are supposed to use that instead.

Mike Christie

2012-05-08 00:27 UTC #3 lore raw

Quoted 21 lines · Mike Christie

Quoted 20 lines · Bhanu Prakash Gollapudi
Quoted 18 lines · Mike Christie
Review
Quoted 1 line · Bhanu Prakash Gollapudi
(quoted diff) +0 -0
b/drivers/scsi/bnx2fc/bnx2fc_tgt.c
drivers/scsi/bnx2fc/bnx2fc_tgt.c void bnx2fc_flush_active_ios(struct bnx2fc_rport
Quoted 4 lines
index d3ee231..082a25c 100644
drivers/scsi/bnx2fc/bnx2fc_tgt.c +0 -0
@@ -185,6 +185,16 @@ void bnx2fc_flush_active_ios(struct bnx2fc_rport
*tgt)
drivers/scsi/bnx2fc/bnx2fc_tgt.c void bnx2fc_flush_active_ios(struct bnx2fc_rport · line 190
Quoted 6 lines
drivers/scsi/bnx2fc/bnx2fc_tgt.c +3 -0
 BUG_ON(rc);
 }
 
+ list_for_each_safe(list, tmp, &tgt->active_tm_queue) {
+ i++;
+ io_req = (struct bnx2fc_cmd *)list;
Why didn't you use list_for_each_entry_safe()? Or, when using list_for_each_safe, instead of the cast are we supposed to be using list_entry()?

Quoted 6 lines

Quoted 3 lines
Mike, the list is the first field in the structure, so the cast here is right.
I am saying that normally if there is a function that does the same thing we are supposed to use that instead.

I think though since it works and it used throughout the driver already it is ok to do. But I think in a patch later it should be changed.

Bhanu Prakash Gollapudi

author

2012-05-08 00:36 UTC #4 lore raw

Quoted 26 lines · Mike Christie

Quoted 25 lines · Mike Christie
Quoted 23 lines · Bhanu Prakash Gollapudi
Quoted 11 lines · Mike Christie
Quoted 1 line · Bhanu Prakash Gollapudi
diff --git a/drivers/scsi/bnx2fc/bnx2fc_tgt.c
b/drivers/scsi/bnx2fc/bnx2fc_tgt.c
Quoted 4 lines
index d3ee231..082a25c 100644
--- a/drivers/scsi/bnx2fc/bnx2fc_tgt.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_tgt.c
@@ -185,6 +185,16 @@ void bnx2fc_flush_active_ios(struct bnx2fc_rport
*tgt)
Quoted 3 lines
BUG_ON(rc);
}
Quoted 7 lines
Quoted 3 lines
+ list_for_each_safe(list, tmp,&tgt->active_tm_queue) {
+ i++;
+ io_req = (struct bnx2fc_cmd *)list;
Why didn't you use list_for_each_entry_safe()? Or, when using list_for_each_safe, instead of the cast are we supposed to be using list_entry()?
Mike, the list is the first field in the structure, so the cast here is right.

Quoted 6 lines

Quoted 2 lines
I am saying that normally if there is a function that does the same thing we are supposed to use that instead.
I think though since it works and it used throughout the driver already it is ok to do. But I think in a patch later it should be changed.

Thanks Mike. In my next opportunity to submit the code, I'll remember to do this.