On Tue, Sep 21, 2010 at 11:41 PM, James Bottomley
<James.Bottomley@xxxxxxx> wrote:
> On Tue, 2010-09-21 at 23:19 +0800, Hillf Danton wrote:
>> The tests for QUEUE_FLAG_REENTER seem unnecessary.
>> And check for get_device() is added.
>
> OK, so you've done a few newbie patches; it's time to graduate.
>
What is the certificate :-)
>> Signed-off-by: Hillf Danton <dhillf@xxxxxxxxx>
>> ---
>>
>> --- o/linux-2.6.36-rc4/drivers/scsi/scsi_transport_fc.c    2010-09-13
>> 07:07:38.000000000 +0800
>> +++ m/linux-2.6.36-rc4/drivers/scsi/scsi_transport_fc.c    2010-09-21
>> 22:05:38.000000000 +0800
>> @@ -3766,16 +3766,11 @@ fc_bsg_goose_queue(struct fc_rport *rpor
>> Â Â Â if (!rport->rqst_q)
>> Â Â Â Â Â Â Â return;
>>
>> - Â Â get_device(&rport->dev);
>> + Â Â if (! get_device(&rport->dev))
>> + Â Â Â Â Â Â return;
>
> The expression in the if clause is never true ... can you tell me why?
>
It is simple. What get_device does is clear, and I am paranoid.
struct device *get_device(struct device *dev)
{
return dev ? to_dev(kobject_get(&dev->kobj)) : NULL;
}
>> Â Â Â spin_lock_irqsave(rport->rqst_q->queue_lock, flags);
>> - Â Â flagset = test_bit(QUEUE_FLAG_REENTER, &rport->rqst_q->queue_flags) &&
>> - Â Â Â Â Â Â Â !test_bit(QUEUE_FLAG_REENTER, &rport->rqst_q->queue_flags);
>> - Â Â if (flagset)
>> - Â Â Â Â Â Â queue_flag_set(QUEUE_FLAG_REENTER, rport->rqst_q);
>> Â Â Â __blk_run_queue(rport->rqst_q);
>> - Â Â if (flagset)
>> - Â Â Â Â Â Â queue_flag_clear(QUEUE_FLAG_REENTER, rport->rqst_q);
>> Â Â Â spin_unlock_irqrestore(rport->rqst_q->queue_lock, flags);
>
> this code doesn't do anything because there's a bug in it. ÂIf you can
> work out what it's trying to do, you should be able to fix the bug.
>
> James
>
It is hard to point out what is the bug, since flagset never is true.
And I guess that simply calling blk_run_queue() seems fine,
leaving check for QUEUE_FLAG_REENTER to be done in block core.
Hillf
--- o/linux-2.6.36-rc4/drivers/scsi/scsi_transport_fc.c 2010-09-13
07:07:38.000000000 +0800
+++ m/linux-2.6.36-rc4/drivers/scsi/scsi_transport_fc.c 2010-09-22
01:37:42.000000000 +0800
@@ -3760,24 +3760,11 @@ fail_host_msg:
static void
fc_bsg_goose_queue(struct fc_rport *rport)
{
- int flagset;
- unsigned long flags;
-
if (!rport->rqst_q)
return;
get_device(&rport->dev);
-
- spin_lock_irqsave(rport->rqst_q->queue_lock, flags);
- flagset = test_bit(QUEUE_FLAG_REENTER, &rport->rqst_q->queue_flags) &&
- !test_bit(QUEUE_FLAG_REENTER, &rport->rqst_q->queue_flags);
- if (flagset)
- queue_flag_set(QUEUE_FLAG_REENTER, rport->rqst_q);
- __blk_run_queue(rport->rqst_q);
- if (flagset)
- queue_flag_clear(QUEUE_FLAG_REENTER, rport->rqst_q);
- spin_unlock_irqrestore(rport->rqst_q->queue_lock, flags);
-
+ blk_run_queue(rport->rqst_q);
put_device(&rport->dev);
}
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
