Re: clear_bit_unlock_is_negative_byte

[Michael Schmitz <schmitzmic@gmail.com>]

Hi Matthew,

Am 21.07.2023 um 10:37 schrieb Michael Schmitz:
> Hi Matthew,
>
> Am 21.07.2023 um 07:27 schrieb Matthew Wilcox:
>> I'm looking to implement clear_bit_unlock_is_negative_byte() on every
>> architecture so we can delete the ifdeffery around maybe-we-have-it
>> and remove the simple implementation from filemap.c.  Here's what I've
>> come up with for m68k:
>>
>> +static inline bool clear_bit_unlock_is_negative_byte(unsigned int nr,
>> +               volatile unsigned long *p)
>> +{
>> +       char result;
>> +       char mask = 1 << nr;    /* nr guaranteed to be < 7 */
>> +
>> +       __asm__ __volatile__ ("eori %1, %2; smi %0"
>> +               : "=d" (result)
>> +               : "i" (mask), "o" (*p)
>> +               : "memory");
>> +       return result;
>> +}
>>
>> It compiles, so I feel Very Pleased With Myself, since I haven't written
>> m68k assmbly in 25 years.  But I have questions.
>
> Unfortunately, after plugging this in, the boot hangs at the first block
> device encountered:
>
> calling  proc_hardware_init+0x0/0x20 @ 1
> initcall proc_hardware_init+0x0/0x20 returned 0 after 40 usecs
> calling  rtc_init+0x0/0x5e @ 1
> initcall rtc_init+0x0/0x5e returned 0 after 40 usecs
> calling  atari_nvram_init+0x0/0x42 @ 1
> initcall atari_nvram_init+0x0/0x42 returned 0 after 40 usecs
> calling  nfhd_init+0x0/0x206 @ 1
> nfhd8: found device with 20971440 blocks (512 bytes)
>
> Doesn't eat up memory, doesn't sit in a tight spin, just never proceeds
> past that point.
>
> Running old and new code side by side (well, actually sequentially) in
> clear_bit_unlock_is_negative_byte()  and comparing results, the return
> values generated by both options match but the actual contents of the
> pointer passed to the function (after the mods) does differ:
>
> nfhd8: found device with 20971440 blocks (512 bytes)
> cbu_inb memval mismatch: 2004 12005
> cbu_inb memval mismatch: 2004 12005
>  nfhd8: AHDI p1 p2
> cbu_inb memval mismatch: 26 10027
> cbu_inb memval mismatch: 26 10027
>
> (first value is from the old code, second value from the new code).

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

Plenty more later in the boot:
calling  brd_init+0x0/0xca @ 1
cbu_inb memval mismatch: 2004 12005 2005 0
cbu_inb memval mismatch: 2004 12005 2005 0
cbu_inb memval mismatch: 26 10027 27 0
cbu_inb memval mismatch: 26 10027 27 0
cbu_inb memval mismatch: 2004 12005 2005 0
cbu_inb memval mismatch: 2004 12005 2005 0
cbu_inb memval mismatch: 36 10037 37 0
cbu_inb memval mismatch: 36 10037 37 0
cbu_inb memval mismatch: 2004 12005 2005 0
cbu_inb memval mismatch: 2004 12005 2005 0
cbu_inb memval mismatch: 36 10037 37 0
cbu_inb memval mismatch: 36 10037 37 0
cbu_inb memval mismatch: 2004 12005 2005 0
cbu_inb memval mismatch: 2004 12005 2005 0
cbu_inb memval mismatch: 26 10027 27 0
cbu_inb memval mismatch: 26 10027 27 0
cbu_inb memval mismatch: 2004 12005 2005 0
cbu_inb memval mismatch: 2004 12005 2005 0
cbu_inb memval mismatch: 36 10037 37 0
cbu_inb memval mismatch: 36 10037 37 0
cbu_inb memval mismatch: 2004 12005 2005 0
cbu_inb memval mismatch: 2004 12005 2005 0
cbu_inb memval mismatch: 26 10027 27 0
cbu_inb memval mismatch: 26 10027 27 0
cbu_inb memval mismatch: 2004 12005 2005 0
cbu_inb memval mismatch: 2004 12005 2005 0
cbu_inb memval mismatch: 26 10027 27 0
cbu_inb memval mismatch: 26 10027 27 0
cbu_inb memval mismatch: 2004 12005 2005 0
cbu_inb memval mismatch: 2004 12005 2005 0

>
> It looks to me that the eori operates on a 16 bit word here?

And it's bits 32-17 of the longword.

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:

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).

Bit 7 was already set before xor, and wasn't cleared. I suspect that's 
why the return value is no longer 1?

Time to dig out that 68k programmer's manual...

Cheers,

	Michael


>
> (Disclaimer: I have written m68k assembly occasionally in the past
> years, but I struggle with inline asm...)
>
> Cheers,
>
>     Michael
>
>>
>> First, m68k is big-endian, so I suspect I'm accessing the wrong byte.
>> Should something in there be adding 3 to 'p'?  Better to do it in the
>> asm, or in the constraints so the compiler can see it?
>>
>> Second, have I properly communicated to the assembler that this is
>> a byte-size operation, and it needs to check bit 7 and not bits 15 or 31
>> to set the negative flag?
>>
>> Third, can this be done better?  x86 has __GCC_ASM_FLAG_OUTPUTS__
>> so it doesn't need the equivalent of the SMI instruction to move the
>> condition to an output variable; it can just tell the compiler that
>> the N flag communicates the result that it's looking for.  Does m68k
>> have __GCC_ASM_FLAG_OUTPUTS__ or did nobody do that work yet?
>>
>> Fourth, we could do this is with ANDI instead of EORI.  It's mildly
>> safer, but we really shouldn't have two threads clearing the lock bit
>> that race with each other.  We can't do it with BCLR because that
>> doesn't set the N flag.  If we do that, we'd need to invert the mask.
>>
>> Appreciate your time looking at this.
>>