Re: clear_bit_unlock_is_negative_byte

[Michael Schmitz <schmitzmic@gmail.com>]

Hi Matthew,

Am 21.07.2023 um 13:32 schrieb Matthew Wilcox:
> On Fri, Jul 21, 2023 at 01:12:48PM +1200, Michael Schmitz wrote:
>> Logging the bit nr. and value passed in:
>>
>> nfhd8: found device with 20971440 blocks (512 bytes)
>> cbu_inb memval mismatch: 2004 12005 2005 0
>> cbu_inb memval mismatch: 2004 12005 2005 0
>>  nfhd8: AHDI p1 p2
>> cbu_inb memval mismatch: 36 10037 37 0
>> cbu_inb memval mismatch: 36 10037 37 0
>
> I'm not quite sure what values you're printing here?  And whether
> they're hex or decimal.  Could you show me the printk string?

All hex, except the last (nr). Values are mem (as modified by the 
original code), copy of mem (as modified by inline asm), the original 
value of mem, and nr.

>
>> The instruction you need is eori.b, and you'll have to increase the mem
>> pointer by 3 bytes. With that change, I see no further mismatches until the
>> return values begin to differ once disk access begins:
>
> Ah thanks.  The perils of working from Motorola official docs and then
> trying to use the GNU assembler ...
>
>> sd 0:0:1:0: [sdb] Preferred minimum I/O size 512 bytes
>> cbu_inb retval mismatch: 1 ff 2084 2084 2085 0
>> rtc-generic rtc-generic: registered as rtc0
>> cbu_inb retval mismatch: 1 ff 2094 2094 2095 0
>> ...
>> sd 0:0:0:0: [sda] Attached SCSI disk
>> probe of 0:0:0:0 returned 0 after 58395182 usecs
>> cbu_inb retval mismatch: 1 ff 2094 2094 2095 0
>>  sdb: RDSK (512) sdb1 (DOS^G)(res 2 spb 2) sdb2 (SFS^B)(res 2 spb 1) sdb3
>> (SFS^B)(res 2 spb 2) sdb4 ((res 2 spb 1)
>> sdb: p4 size 18446744071971831216 extends beyond EOD, enabling native
>> capacity
>> cbu_inb retval mismatch: 1 ff 2084 2084 2085 0
>>
>> (return value from old and new code, value of mem from old and new code,
>> original value, bit nr).
>
> OK, so the new code would set the byte to 0xff (that's how Sxx works).
> But it's returning a bool, so that shouldn't matter.  2084/2084/2085
> would make sense; one of the two functions has cleared the bottom bit.
>
>> Bit 7 was already set before xor, and wasn't cleared. I suspect that's why
>> the return value is no longer 1?
>
> Ah, it's not supposed to be cleared.  The way this works is that bit 0
> is the lock bit; if someone's waiting on the folio, they set bit 7.  If
> bit 7 is set when we clear bit 0, we look on the wait queue.  If there's
> nobody on the wait queue, we clear bit 7.

Right, that's what I meant to say. I'd only seen cases where bit 0 had 
been set and was cleared. This isn't an actual production system of 
sorts, just an ARAnyM instance I can fire up quickly to see patched 
kernels crash horribly.

>
> So ... I think you've fixed it!

This is what I have tests running on right now:

static inline bool clear_bit_unlock_is_negative_byte(unsigned int nr,
                 volatile unsigned long *p)
{
         unsigned char *cp = (unsigned char *) p;
         char result;
         char mask = 1 << nr;    /* nr guaranteed to be < 7 */

         __asm__ __volatile__ ("eori.b %1, %2; smi %0"
                 : "=d" (result)
                 : "i" (mask), "o" (*(cp+3))
                 : "memory");
         return result;
}

I'm sure you can do all the casting to char and increment by 3 in the 
asm argument...

If there's a simple way to exercise this code path using standard Unix 
tools (or stress-ng which I ought to have somewhere), drop me a hint.

Cheers,

	Michael