linux-kernel - RE: [PATCH 2/3] hpilo: add interrupt handler

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <92D64232F2316045B18937D5C9996B1522CAE2F71E@GVW1144EXB.americas.hpqcorp.net>
Date:	Tue, 18 Aug 2009 22:25:25 +0000
From:	"Altobelli, David" <david.altobelli@...com>
To:	Andrew Morton <akpm@...ux-foundation.org>
CC:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"gregkh@...e.de" <gregkh@...e.de>
Subject: RE: [PATCH 2/3] hpilo: add interrupt handler

From: Andrew Morton
> It would be very helpful if there were comments in hpilo.h which fully
> describe the roles of each lock.

Okay, thanks.  I'll put something together.

>>
>> ...
>>
>> @@ -496,27 +491,28 @@ static int ilo_close(struct inode *ip, s
>>  	int slot;
>>  	struct ccb_data *data;
>>  	struct ilo_hwinfo *hw;
>> +	unsigned long flags;
>>  
>>  	slot = iminor(ip) % MAX_CCB;
>>  	hw = container_of(ip->i_cdev, struct ilo_hwinfo, cdev);
>>  
>> -	spin_lock(&hw->alloc_lock);
>> -
>> -	if (is_device_reset(hw))
>> -		ilo_locked_reset(hw);
>> +	spin_lock(&hw->open_lock);
>>  
>>  	if (hw->ccb_alloc[slot]->ccb_cnt == 1) {
>>  
>>  		data = fp->private_data;
>>  
>> +		spin_lock_irqsave(&hw->alloc_lock, flags);
>> +		hw->ccb_alloc[slot] = NULL;
>> +		spin_unlock_irqrestore(&hw->alloc_lock, flags);
>> +
>>  		ilo_ccb_close(hw->ilo_dev, data);
>>  
>>  		kfree(data);
>> -		hw->ccb_alloc[slot] = NULL;
>>  	} else
>>  		hw->ccb_alloc[slot]->ccb_cnt--;
>>  
>> -	spin_unlock(&hw->alloc_lock);
>> +	spin_unlock(&hw->open_lock);
>>  
>>  	return 0;
>>  }

> Here I'd have expected that alloc_lock would provide the protection for
> ->ccb_cnt.  But the code doesn't do that - it instead appears to use
> ->open_lock.

Yes.  My intention is to use open_lock to serialize access by open and close,
and alloc_lock to serialize pointers being added to or deleted from hw->ccb_alloc[]
(list add/remove in ilo_open/close, and run list in ilo_isr).

I wanted a lock for open/close in order to keep the reference counts and 
manage ccb allocation.  I wanted to separate this from actually 
adding/removing a list element, because the list was searched by the interrupt 
handler, and I didn't want open/close to spend a long time with irqs disabled.

Perhaps my naming/logic needs improvement, I'm open to suggestions.

> The code doesn't tell us what open_lock's role is intended to be so
> this reviewer can't really tell whether or not this was a mistake.

> From this function I infer that the designed lock nesting is
>
>	open_lock
>	->alloc_lock
>	  ->fifo_lock
>
> yes?  It would be useful to document that also.

Yes, that is the design, although taking fifo_lock doesn't require 
holding alloc_lock.

> Have these changes been runtime tested with lockdep enabled?

CONFIG_LOCKDEP=y (along with the list in SubmitChecklist #12).
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/