sbp-target: checking for NULL instead of IS_ERR

We changed this from kzalloc to sbp_mgt_get_req() so we need to change from checking for NULL to check for error pointers.

Fixes c064b2a78989 ('sbp-target: Conversion to percpu_ida tag pre-allocation')

Signed-off-by Dan Carpenter <[email protected]>

drivers/target/sbp/sbp_target.c +0 -0

index 251d532..a04b0605f 100644

Hi Dan + BootC,

Quoted 11 lines · Dan Carpenter

We changed this from kzalloc to sbp_mgt_get_req() so we need to change from checking for NULL to check for error pointers.

Fixes c064b2a78989 ('sbp-target: Conversion to percpu_ida tag pre-allocation')

Signed-off-by Dan Carpenter <[email protected]>

drivers/target/sbp/sbp_target.c +0 -0

index 251d532..a04b0605f 100644

drivers/target/sbp/sbp_target.c static void tgt_agent_fetch_work(struct work_struct *work) · line 957

Quoted 8 lines

drivers/target/sbp/sbp_target.c +1 -1

@@ -951,7 +951,7 @@ static void tgt_agent_fetch_work(struct work_struct *work)
 
 	while (next_orb && tgt_agent_check_active(agent)) {
 		req = sbp_mgt_get_req(sess, sess->card, next_orb);
-		if (!req) {
+		if (IS_ERR(req)) {
 			spin_lock_bh(&agent->lock);
 			agent->state = AGENT_STATE_DEAD;
 			spin_unlock_bh(&agent->lock);

Fixed + folded into the original patch.

Thanks Dan.

Chris, would you be so kind to review the original changes here:

sbp-target: Conversion to percpu_ida tag pre-allocation
http://www.spinics.net/lists/target-devel/msg11778.html

sbp-target: Convert to TARGET_SCF_ACK_KREF I/O krefs
http://www.spinics.net/lists/target-devel/msg11780.html

and verify on your local IEEE1394 target setup..?

Quoted 30 lines · Nicholas A. Bellinger

Hi Dan + BootC,

Quoted 11 lines · Dan Carpenter

We changed this from kzalloc to sbp_mgt_get_req() so we need to change from checking for NULL to check for error pointers.

Fixes c064b2a78989 ('sbp-target: Conversion to percpu_ida tag pre-allocation')

Signed-off-by Dan Carpenter <[email protected]>

drivers/target/sbp/sbp_target.c +0 -0

index 251d532..a04b0605f 100644

drivers/target/sbp/sbp_target.c static void tgt_agent_fetch_work(struct work_struct *work) · line 957

Quoted 8 lines

drivers/target/sbp/sbp_target.c +1 -1

@@ -951,7 +951,7 @@ static void tgt_agent_fetch_work(struct work_struct *work)
 
 	while (next_orb && tgt_agent_check_active(agent)) {
 		req = sbp_mgt_get_req(sess, sess->card, next_orb);
-		if (!req) {
+		if (IS_ERR(req)) {
 			spin_lock_bh(&agent->lock);
 			agent->state = AGENT_STATE_DEAD;
 			spin_unlock_bh(&agent->lock);

Fixed + folded into the original patch.

Thanks Dan.

Chris, would you be so kind to review the original changes here:

Quoted 7 lines

sbp-target: Conversion to percpu_ida tag pre-allocation
http://www.spinics.net/lists/target-devel/msg11778.html

sbp-target: Convert to TARGET_SCF_ACK_KREF I/O krefs
http://www.spinics.net/lists/target-devel/msg11780.html

and verify on your local IEEE1394 target setup..?

Hi Nic, Dan,

I’m away this weekend so I can’t test these for a few days at least, unfortunately. I must admit I only vaguely follow the changes here as I haven’t been keeping up with the pace of change in target-devel lately, but it generally looks OK I think.

Are these in linux-next or another branch somewhere I can easily clone them from?

How soon do you need my ACK/NAK on these?

Quoted 2 lines · Chris Boot

Are these in linux-next or another branch somewhere I can easily clone them from?

The patch series is in target-pending/for-next.

Quoted 4 lines · Nicholas A. Bellinger

Quoted 2 lines · Chris Boot

Are these in linux-next or another branch somewhere I can easily clone them from?

Quoted 1 line

The patch series is in target-pending/for-next.

Hi Nic,

I've just managed to resurrect a test rig for this (the hardware I had for it has stopped being usable, yay!), and my initial testing shows the updated code panics on the first submitted IO.

Quoted 5 lines · Chris Boot

Quoted 4 lines · Nicholas A. Bellinger

Quoted 2 lines · Chris Boot

Are these in linux-next or another branch somewhere I can easily clone them from?

Quoted 7 lines

Quoted 1 line

The patch series is in target-pending/for-next.

Hi Nic,

I've just managed to resurrect a test rig for this (the hardware I had for it has stopped being usable, yay!), and my initial testing shows the updated code panics on the first submitted IO.

So this isn't the first IO, it's exactly the 2nd IO. I'm hitting BUG_ON(se_cmd->se_tfo || se_cmd->se_sess) in target_submit_cmd_map_sgls().

I'm assuming the se_cmd is being reused due to percpu ida allocator, and the code must be missing something to clean up the se_cmd sufficiently once we're done with it.

At this point I'm out of my depth going through the target core, so I'd appreciate some pointers to get any further!

Quoted 17 lines · Chris Boot

Quoted 5 lines · Chris Boot

Quoted 4 lines · Nicholas A. Bellinger

Quoted 2 lines · Chris Boot

Are these in linux-next or another branch somewhere I can easily clone them from?

Quoted 7 lines

Quoted 1 line

The patch series is in target-pending/for-next.

Hi Nic,

I've just managed to resurrect a test rig for this (the hardware I had for it has stopped being usable, yay!), and my initial testing shows the updated code panics on the first submitted IO.

So this isn't the first IO, it's exactly the 2nd IO. I'm hitting BUG_ON(se_cmd->se_tfo || se_cmd->se_sess) in target_submit_cmd_map_sgls().

Quoted 6 lines

I'm assuming the se_cmd is being reused due to percpu ida allocator, and the code must be missing something to clean up the se_cmd sufficiently once we're done with it.

At this point I'm out of my depth going through the target core, so I'd appreciate some pointers to get any further!

Replying to myself again... Worked it out after reading the thread about the usb gadget target. Here's the patch you want to squash into your existing series:

drivers/target/sbp/sbp_target.c +2 -5

index a04b0605f8d0..d021997cc837 100644
@@ -933,6 +933,7 @@ static struct sbp_target_request *sbp_mgt_get_req(struct sbp_session *sess,
                return ERR_PTR(-ENOMEM);
 
        req = &((struct sbp_target_request *)se_sess->sess_cmd_map)[tag];
+       memset(req, 0, sizeof(*req));
        req->se_cmd.map_tag = tag;
        req->se_cmd.tag = next_orb;
 

@@ -1619,12 +1620,8 @@ static void sbp_mgt_agent_rw(struct fw_card *card,
                        rcode = RCODE_CONFLICT_ERROR;
                        goto out;
                }
-               // XXX:
-#if 0
-               req = sbp_mgt_get_req(agent->login->sess, card);
-#else
+
                req = kzalloc(sizeof(*req), GFP_ATOMIC);
-#endif
                if (!req) {
                        rcode = RCODE_CONFLICT_ERROR;
                        goto out;

I hope Thunderbird hasn't mangled this too badly.

With this applied, please add this to the patch for sbp_target:

Acked-by Chris Boot <[email protected]>