Re: [RFC] IBM Power RAID driver (ipr)

[Brian King <brking@us.ibm.com>]

[-- Attachment #1: Type: text/plain, Size: 2705 bytes --]

Anton Blanchard wrote:
> - Since ipr_sleep_no_lock/ipr_sleep is used once, we can open code
>   a schedule_timeout there instead.

I agree. I will remove them.

> - Add a comment to explain why the wakeup strategy isnt racy. I
>   automatically thought it was (just like the kernel version of
>   sleep_on) until I could verify all wake ups are done under the scsi
>   host lock. I also have a trivial change to make the two sleep
>   functions alike.

I was actually thinking of changing the wakeup strategy because it 
doesn't handle spurious wakeups like the new interfaces in the kernel 
do. See diff below.

> - Im a bit worried about memory barriers wrt the allow_* things. As an
>   example I added some mb()s wherever ->allow_interrupts is set.

Can you elaborate on this? These flags should all be accessed/set while 
the host lock is held. If this is not sufficient, then wouldn't we have 
problems with more than just these flags, but rather any shared data?

>   Actually, do we need this flag at all? Cant we just set the bits on the
>   card to disable interrupts, then do a synchronize_irq()? Similarly it
>   would be nice to get rid of some of the other allow_ flags if we can
>   get around them.

The reason I put this flag in was for shared interrupts. If the isr gets 
called due to another adapter's interrupt while this adapter currently 
has memory space disabled because it is being reset, I wanted to avoid 
issuing a readl.

> - The forcing of the pci cacheline size worries me, could we have problems 
>   with enabling MWI etc after doing this?

Shouldn't be a problem, but I will certainly remove it if it doesn't 
belong here.

> - Do we need the extra ipr_set_pcix_cmd_reg during init? We are going to
>   do it after a reset arent we?

Yes, good catch. Removed.

> - It looks like the kmalloc in ipr_sdt_copy is called from process
>   context without any locks held, so we dont need an atomic allocation.

Actually, this does need to be an atomic allocation. If the adapter 
ipr_sdt_copy is being called for is the root/paging device a GFP_KERNEL 
allocation might hang indefinitely since at the point that this function 
is called, the adapter is not capable of doing I/O.


> @@ -1076,6 +1045,7 @@ static void ipr_mask_and_clear_all_inter
>  
>  	/* Stop new interrupts */
>  	ioa_cfg->allow_interrupts = 0;
> +	mb();

On the topic of memory barriers wrt adapter I/O, at one point I remember 
seeing sync instructions in the readl/writel macros for PPC64. Looking 
now, all I see is an eieio instruction before the readl. Looks like I 
will need to sprinkle a few more memory barriers in the driver...




-- 
Brian King
eServer Storage I/O
IBM Linux Technology Center

[-- Attachment #2: ipr-diff --]
[-- Type: application/x-java-vm, Size: 12289 bytes --]