Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1350817 > unrolled thread

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

Started by"Nicholas A. Bellinger" <nab@linux-iscsi.org>
First post2016-03-05 08:40 +0100
Last post2016-03-11 05:40 +0100
Articles 7 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [patch] sbp-target: checking for NULL instead of IS_ERR "Nicholas A. Bellinger" <nab@linux-iscsi.org> - 2016-03-05 08:40 +0100
    Re: [patch] sbp-target: checking for NULL instead of IS_ERR Chris Boot <bootc@bootc.net> - 2016-03-05 10:00 +0100
      Re: [patch] sbp-target: checking for NULL instead of IS_ERR "Nicholas A. Bellinger" <nab@linux-iscsi.org> - 2016-03-05 10:40 +0100
        Re: [patch] sbp-target: checking for NULL instead of IS_ERR Chris Boot <bootc@bootc.net> - 2016-03-10 22:00 +0100
          Re: [patch] sbp-target: checking for NULL instead of IS_ERR Chris Boot <bootc@bootc.net> - 2016-03-10 23:00 +0100
            Re: [patch] sbp-target: checking for NULL instead of IS_ERR Chris Boot <bootc@bootc.net> - 2016-03-11 00:00 +0100
              Re: [patch] sbp-target: checking for NULL instead of IS_ERR "Nicholas A. Bellinger" <nab@linux-iscsi.org> - 2016-03-11 05:40 +0100

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

From"Nicholas A. Bellinger" <nab@linux-iscsi.org>
Date2016-03-05 08:40 +0100
SubjectRe: [patch] sbp-target: checking for NULL instead of IS_ERR
Message-ID<r9eYF-2Pv-3@gated-at.bofh.it>
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..?

[toc] | [next] | [standalone]

#1350850

FromChris Boot <bootc@bootc.net>
Date2016-03-05 10:00 +0100
Message-ID<r9ge7-3AS-19@gated-at.bofh.it>
In reply to#1350817
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

[toc] | [prev] | [next] | [standalone]

#1350856

From"Nicholas A. Bellinger" <nab@linux-iscsi.org>
Date2016-03-05 10:40 +0100
Message-ID<r9gQO-47B-9@gated-at.bofh.it>
In reply to#1350850
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.

[toc] | [prev] | [next] | [standalone]

#1355378

FromChris Boot <bootc@bootc.net>
Date2016-03-10 22:00 +0100
Message-ID<rbfQC-3PQ-13@gated-at.bofh.it>
In reply to#1350856
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

[toc] | [prev] | [next] | [standalone]

#1355421

FromChris Boot <bootc@bootc.net>
Date2016-03-10 23:00 +0100
Message-ID<rbgMF-4AF-3@gated-at.bofh.it>
In reply to#1355378
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

[toc] | [prev] | [next] | [standalone]

#1355440

FromChris Boot <bootc@bootc.net>
Date2016-03-11 00:00 +0100
Message-ID<rbhIJ-5dA-1@gated-at.bofh.it>
In reply to#1355421
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

[toc] | [prev] | [next] | [standalone]

#1355606

From"Nicholas A. Bellinger" <nab@linux-iscsi.org>
Date2016-03-11 05:40 +0100
Message-ID<rbn1L-Iq-1@gated-at.bofh.it>
In reply to#1355440
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!

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web