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]>
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);
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 <[email protected]>
>
> 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..?
On 5 Mar 2016, at 07:33, Nicholas A. Bellinger <[email protected]> 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 <[email protected]>
>>
>> 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
[email protected]
On Sat, 2016-03-05 at 08:45 +0000, Chris Boot wrote:
> On 5 Mar 2016, at 07:33, Nicholas A. Bellinger <[email protected]> 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 <[email protected]>
> >>
> >> 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.
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
[email protected]
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
[email protected]
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 <[email protected]>
Thanks,
Chris
--
Chris Boot
[email protected]
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 <[email protected]>
>
Applied to target-pending/for-next, and squashing into original patches
for -v4.
Thanks BootC!