Re: [Intel-wired-lan] [PATCH iwl-next v2 08/13] lib: packing: fix QUIRK_MSB_ON_THE_RIGHT behavior

[Jacob Keller <jacob.e.keller@intel.com>]

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.