[patch] sbp-target: checking for NULL instead of IS_ERR

[patch] sbp-target: checking for NULL instead of IS_ERR

[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 <dan.carpenter@oracle.com>

diff --git a/drivers/target/sbp/sbp_target.c b/drivers/target/sbp/sbp_target.c
index 251d532..a04b0605f 100644
--- a/drivers/target/sbp/sbp_target.c
+++ b/drivers/target/sbp/sbp_target.c
@@ -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);

Re: [patch] sbp-target: checking for NULL instead of IS_ERR

[Nicholas A. Bellinger]

Hi Dan + BootC,

On Wed, 2016-03-02 at 13:09 +0300, Dan Carpenter wrote:
> 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 <dan.carpenter@oracle.com>
> 
> diff --git a/drivers/target/sbp/sbp_target.c b/drivers/target/sbp/sbp_target.c
> index 251d532..a04b0605f 100644
> --- a/drivers/target/sbp/sbp_target.c
> +++ b/drivers/target/sbp/sbp_target.c
> @@ -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..?

Re: [patch] sbp-target: checking for NULL instead of IS_ERR

[Chris Boot]

On 5 Mar 2016, at 07:33, Nicholas A. Bellinger <nab@linux-iscsi.org> wrote:
> 
> Hi Dan + BootC,
> 
> On Wed, 2016-03-02 at 13:09 +0300, Dan Carpenter wrote:
>> 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 <dan.carpenter@oracle.com>
>> 
>> diff --git a/drivers/target/sbp/sbp_target.c b/drivers/target/sbp/sbp_target.c
>> index 251d532..a04b0605f 100644
>> --- a/drivers/target/sbp/sbp_target.c
>> +++ b/drivers/target/sbp/sbp_target.c
>> @@ -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..?

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?

Cheers,
Chris

-- 
Chris Boot
bootc@bootc.net

Re: [patch] sbp-target: checking for NULL instead of IS_ERR

[Nicholas A. Bellinger]

On Sat, 2016-03-05 at 08:45 +0000, Chris Boot wrote:
> On 5 Mar 2016, at 07:33, Nicholas A. Bellinger <nab@linux-iscsi.org> wrote:
> > 
> > Hi Dan + BootC,
> > 
> > On Wed, 2016-03-02 at 13:09 +0300, Dan Carpenter wrote:
> >> 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 <dan.carpenter@oracle.com>
> >> 
> >> diff --git a/drivers/target/sbp/sbp_target.c b/drivers/target/sbp/sbp_target.c
> >> index 251d532..a04b0605f 100644
> >> --- a/drivers/target/sbp/sbp_target.c
> >> +++ b/drivers/target/sbp/sbp_target.c
> >> @@ -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..?
> 
> 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?

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

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

Assuming the merge window opens on the 13th, any time over the next 7-10
days would be fine.

Re: [patch] sbp-target: checking for NULL instead of IS_ERR

[Chris Boot]

On 05/03/16 09:33, Nicholas A. Bellinger wrote:
> On Sat, 2016-03-05 at 08:45 +0000, Chris Boot wrote:
>> Are these in linux-next or another branch somewhere I can easily clone
>> them from?
> 
> 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.

I'll go and debug it now and see what I can get from it, but I thought
I'd let you know ASAP.

Cheers,
Chris

-- 
Chris Boot
bootc@bootc.net

Re: [patch] sbp-target: checking for NULL instead of IS_ERR

[Chris Boot]

On 10/03/16 20:56, Chris Boot wrote:
> On 05/03/16 09:33, Nicholas A. Bellinger wrote:
>> On Sat, 2016-03-05 at 08:45 +0000, Chris Boot wrote:
>>> Are these in linux-next or another branch somewhere I can easily clone
>>> them from?
>>
>> 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!

Thanks,
Chris

-- 
Chris Boot
bootc@bootc.net

Re: [patch] sbp-target: checking for NULL instead of IS_ERR

[Chris Boot]

On 10/03/16 21:52, Chris Boot wrote:
> On 10/03/16 20:56, Chris Boot wrote:
>> On 05/03/16 09:33, Nicholas A. Bellinger wrote:
>>> On Sat, 2016-03-05 at 08:45 +0000, Chris Boot wrote:
>>>> Are these in linux-next or another branch somewhere I can easily clone
>>>> them from?
>>>
>>> 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!

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:

diff --git a/drivers/target/sbp/sbp_target.c b/drivers/target/sbp/sbp_target.c
index a04b0605f8d0..d021997cc837 100644
--- a/drivers/target/sbp/sbp_target.c
+++ b/drivers/target/sbp/sbp_target.c
@@ -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 <bootc@bootc.net>

Thanks,
Chris

-- 
Chris Boot
bootc@bootc.net

Re: [patch] sbp-target: checking for NULL instead of IS_ERR

[Nicholas A. Bellinger]

On Thu, 2016-03-10 at 22:58 +0000, Chris Boot wrote:
> On 10/03/16 21:52, Chris Boot wrote:
> > On 10/03/16 20:56, Chris Boot wrote:
> >> On 05/03/16 09:33, Nicholas A. Bellinger wrote:
> >>> On Sat, 2016-03-05 at 08:45 +0000, Chris Boot wrote:
> >>>> Are these in linux-next or another branch somewhere I can easily clone
> >>>> them from?
> >>>
> >>> 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!
> 
> 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:
> 
> diff --git a/drivers/target/sbp/sbp_target.c b/drivers/target/sbp/sbp_target.c
> index a04b0605f8d0..d021997cc837 100644
> --- a/drivers/target/sbp/sbp_target.c
> +++ b/drivers/target/sbp/sbp_target.c
> @@ -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 <bootc@bootc.net>
> 

Applied to target-pending/for-next, and squashing into original patches
for -v4.

Thanks BootC!