On 8/10/2016 1:51 AM, ira.weiny wrote:
> On Tue, Aug 09, 2016 at 09:11:46PM +0300, Leon Romanovsky wrote:
>> On Tue, Aug 09, 2016 at 11:16:26AM -0400, ira.weiny@xxxxxxxxx wrote:
>>> From: Mike Marciniszyn <mike.marciniszyn@xxxxxxxxx>
>>>
>
> [snip]
>
>>> diff --git a/drivers/infiniband/hw/hfi1/debugfs.c b/drivers/infiniband/hw/hfi1/debugfs.c
>>> index dbab9d9cc288..c35bef8dd5aa 100644
>>> --- a/drivers/infiniband/hw/hfi1/debugfs.c
>>> +++ b/drivers/infiniband/hw/hfi1/debugfs.c
>>> @@ -223,28 +223,35 @@ DEBUGFS_SEQ_FILE_OPEN(ctx_stats)
>>> DEBUGFS_FILE_OPS(ctx_stats);
>>>
>>> static void *_qp_stats_seq_start(struct seq_file *s, loff_t *pos)
>>> -__acquires(RCU)
>>> + __acquires(RCU)
>>> {
>>> struct qp_iter *iter;
>>> loff_t n = *pos;
>>>
>>> - rcu_read_lock();
>>> iter = qp_iter_init(s->private);
>>> +
>>> + /* stop calls rcu_read_unlock */
>>> + rcu_read_lock();
>>
>> IMHO, it should be placed after your if(!iter) check below.
>
> I know this seems weird but this makes the rcu locking "balanced". Returning
> NULL here still calls "stop" which is going to call rcu_read_unlock. If we
> move rcu_read_lock we need to maintain state to determine if we called lock or
> not. This is easier. To me it does seem odd that the sequence operation does
> not stop on its own when NULL is returned but that is the way it works.
>
>>
>>> +
>>> if (!iter)
>>> return NULL;
>>>
>>> - while (n--) {
>>> + if (qp_iter_next(iter)) {
>>> + kfree(iter);
>>> + return NULL;
>>> + }
>>> + while (n--)
>>> if (qp_iter_next(iter)) {
>>> kfree(iter);
>>> return NULL;
>>> }
>>
>> It looks like you forgot to remove the lines above.
>
> Nope this replaces a call to qp_iter_next which was in qp_iter_init.
>
> As the commit messages says we move the implict next's back into the respective
> start functions.
>
> Ira
Thanks, applied.
--
Doug Ledford <dledford@xxxxxxxxxx>
GPG Key ID: 0E572FDD
Attachment:
signature.asc
Description: OpenPGP digital signature