Re: [PATCH v10 2/3] block: change annotation of rdb_CylBlocks in affs_hardblocks.h

[Michael Schmitz <schmitzmic@gmail.com>]

Hi Geert,

Am 16.06.2023 um 19:28 schrieb Geert Uytterhoeven:
> Hi Michael,
>
> On Fri, Jun 16, 2023 at 9:21 AM Michael Schmitz <schmitzmic@gmail.com> wrote:
>> Am 16.06.2023 um 17:48 schrieb Christoph Hellwig:
>>> On Fri, Jun 16, 2023 at 07:53:11AM +1200, Michael Schmitz wrote:
>>>> Thanks - now there's two __s32 fields in that header - one checksum each
>>>> for RDB and PB. No one has so far seen the need for a 'signed big endian 32
>>>> bit' type, and I'd rather avoid adding one to types.h. I'll leave those as
>>>> they are (with the tacit understanding that they are equally meant to be
>>>> big endian).
>>>
>>> We have those in a few other pleases and store them as __be32 as well.  The
>>> (implicit) cast to s32 will make them signed again.
>>
>> Where's that cast to s32 hidden? I've only seen
>>
>> #define __be32_to_cpu(x) ((__force __u32)(__be32)(x))
>>
>> which would make the checksums unsigned if __be32 was used.
>>
>> Whether the checksum code uses signed or unsigned math would require
>> inspection of the Amiga partitioning tool source which I don't have, so
>> I've kept __s32 to be safe.
>
> Unsurprisingly, block/partitions/amiga.c:checksum_block() calculates
> a checksum over __be32 words.  The actual signedness of the checksum
> field doesn't matter much[*], as using two-complement numbers, you can
> just assign a signed value to an unsigned field.
> It should definitely be __be32.
>
> [*] I guess it was made signed because the procedure to update the
> check goes like this:
>   1. set checksum field to zero,
>   2. calculate sum,
>   3. store negated sum in checksum field.

Thanks, that explains why the result of checksum_block() is tested 
against zero. Makes sense now.

Will fix in v12 ...

Cheers,

	Michael



>
> Gr{oetje,eeting}s,
>
>                         Geert
>