This is a multipart MIME message.

--==_Exmh_-17874974750
Content-Type: text/plain; charset=us-ascii

> As far as I can tell, this, nonatomically,
> 
> compares old and new (skalar) level
> aborts if not higher
> stores new level
> calls machine dependent function to translate level to mask and store in
> right place
> 
> similar, for lowerspl().
> 
> Why is this safe to do?
> 

I don't think it is....



--==_Exmh_-17874974750
Content-Type: message/rfc822 ; name="1195"
Content-Description: 1195
Content-Disposition: attachment; filename="1195"

Delivery-Date: Tue Jun  1 12:42:52 1999
	id MAA20856; Tue, 1 Jun 1999 12:42:51 +0100 (BST)
	id AA14255; Tue, 1 Jun 99 12:42:50 BST
	id MAA20852; Tue, 1 Jun 1999 12:42:48 +0100 (BST)
	id MAA27556; Tue, 1 Jun 1999 12:42:46 +0100
Message-Id: <199906011142.MAA27556@sun52.NIS.cambridge>
To: mark@causality.com
Cc: richard.earnshaw@arm.com
Reply-To: richard.earnshaw@arm.com
Organization: ARM Ltd.
Subject: Re: Installing 1.4 
In-Reply-To: Your message of "Tue, 01 Jun 1999 10:54:16 BST."
             <Pine.SOL.3.96.990601105233.20029C-100000@fm3.facility.pipex.com> 
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Date: Tue, 01 Jun 1999 12:42:46 +0100
From: Richard Earnshaw <rearnsha@arm.com>
Content-Length: 2995

> Hmm, I'll see if I can find a printer to recreate this with. On the RiscPC
> I have never been able to create the problem so it does appear to be
> printer dependant. The lpt port is just part of a standard SMC superIO and
> just uses an attachment  to the dev/ic/lpt.c driver.
> 

If it's any help to know this, my printer is an Epson Stylus 600.

Probably not relevant, but whilst having a stare at the source trying to 
play mind games with what was going on, I think I've spotted a potential 
race in the raisespl code; perhaps you can convince me this can't happen:

        mov     r3, r0                  /* Save the new value */
        ldr     r1, Lcurrent_spl_level  /* Get the current spl level */
        ldr     r0, [r1]
        cmp     r3, r0
        movle   pc, lr

>From this point on, the SPL level is changed.  Interrupts are not disabled 
at this point (as far as I can tell), so can't an interrupt occur which 
could in turn (maybe indirectly -- context switch?) try to raise the spl 
level?  If so, and it is trying to raise it by not more than the current 
raise it will note that the spl level has changed and simply exit.  
However, since the thread really raising the spl has been suspended, the 
interrupts won't be completely masked.   As far as I can tell, the 
potential race lasts between here and the point in irq_setmasks that the 
code disables interrupts.

        str     r3, [r1]                /* Store the new spl level */

        ldr     r2, Lspl_masks          /* Get the spl mask */
        ldr     r2, [r2, r3, lsl #2]

        ldr     r1, Lspl_mask           /* Store in the current spl mask */
        str     r2, [r1]

        stmfd   sp!, {r0, lr}           /* Preserve registers */
        bl      _irq_setmasks           /* Update the actual masks */
        ldmfd   sp!, {r0, pc}           /* Exit */

If I'm right, then it would be possible for interrupts at a particular 
level to continue even if the code thinks that they are blocked.  The fix 
would be to always disable interrupts from before the store until after 
the masks have been updated.  Its possible that there is a similar issue 
with splx, though I think lowerspl is probably safe.

A possible fix is below (but not tested); I've also moved some 
instructions around to reduce the number of stalls on a StrongARM.

ENTRY(raisespl)
	ldr	r1, Lcurrent_spl_level	/* Get the current spl level */
	mov	r3, r0			/* Save the new value */
	ldr	r0, [r1]
	cmp	r3, r0
	movle	pc, lr

	mov	ip, r4			/* Put R4 somewhere safe */
	mrs	r4, cpsr_all		/* Disable interrupts */
	orr	r2, r4,  #(I32_bit)
	msr	cpsr_all, r2

	ldr	r2, Lspl_masks		/* Get the spl mask */
	str	r3, [r1]		/* Store the new spl level */

	ldr	r2, [r2, r3, lsl #2]

	ldr	r1, Lspl_mask		/* Store in the current spl mask */
	stmfd	sp!, {r0, ip, lr}	/* Preserve registers */
	str	r2, [r1]

	bl	_irq_setmasks		/* Update the actual masks */
	msr	cpsr_all, r4		/* Restore interrupt status */
	ldmfd	sp!, {r0, r4, pc}	/* Exit */



--==_Exmh_-17874974750--