On 9/2/2024 4:25 PM, Vladimir Oltean wrote:
> Hi Jacob,
>
> It's very cool that you and Przemek (and possibly others) spent the time
> to untangle this. Thanks! Just a microscopic nitpick below.
>
> On Wed, Aug 28, 2024 at 01:57:24PM -0700, Jacob Keller wrote:
>> That is, QUIRK_MSB_ON_THE_RIGHT does not affect byte positioning, but
>> inverts bit offsets inside a byte.
>>
>> Essentially, the mapping for physical bit offsets should be reserved for a
> ~~~~~~~~
> reversed
>
Yep.
>> Logical: 55 54 53 52 51 50 49 48 47 45 44 43 42 41 40 39
>> ^ ^ ^ ^ ^ ^ ^ ^ ^
>
> These 16 bits should have been 55-40. Bit 46 is missing, and bit 39 is
> extraneous.
>
> Also, I honestly think that another "Byte boundary:" line would help the
> reader see the transformation proposed as an example better. Like this:
>
> Byte boundary: | | |
> Logical: 55 54 53 52 51 50 49 48 47 46 45 44 43 42 41 40
> ^ ^ ^ ^ ^ ^ ^ ^ ^
>
Good catch, and yea a byte boundary helps readability.
>> Fixes: 554aae35007e ("lib: Add support for generic packing operations")
>
> When there is no user-observable issue in mainline, I believe there is
> no reason for a Fixes: tag, even if the bug is very real. My understanding
> is that the role of the tag is to help the backporting process to stable.
> Using it here could possibly confuse the maintainers that it needs to be
> backported, even though it is spelled out that it needs not be.
>
Fair. I view the fixes tags as also helpful because it helps quickly
locate the code when I'm reviewing a past commit when trying to
understand why something was changed. I could move this to a mention in
the commit message text without an explicit fixes tag though.
