Message-ID: <20240902232524.cz77daj2tsajhrpb@skbuf>
Date: Tue, 3 Sep 2024 02:25:24 +0300
From: Vladimir Oltean <olteanv@...il.com>
To: Jacob Keller <jacob.e.keller@...el.com>
Cc: netdev <netdev@...r.kernel.org>,
	Anthony Nguyen <anthony.l.nguyen@...el.com>,
	Intel Wired LAN <intel-wired-lan@...ts.osuosl.org>,
	Przemek Kitszel <przemyslaw.kitszel@...el.com>
Subject: Re: [PATCH iwl-next v2 08/13] lib: packing: fix
 QUIRK_MSB_ON_THE_RIGHT behavior

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:
> The QUIRK_MSB_ON_THE_RIGHT quirk is intended to modify pack() and unpack()
> so that the most significant bit of each byte in the packed layout is on
> the right.
> 
> The way the quirk is currently implemented is broken whenever the packing
> code packs or unpacks any value that is not exactly a full byte.
> 
> The broken behavior can occur when packing any values smaller than one
> byte, when packing any value that is not exactly a whole number of bytes,
> or when the packing is not aligned to a byte boundary.
> 
> This quirk is documented in the following way:
> 
>   1. Normally (no quirks), we would do it like this:
> 
>   ::
> 
>     63 62 61 60 59 58 57 56 55 54 53 52 51 50 49 48 47 46 45 44 43 42 41 40 39 38 37 36 35 34 33 32
>     7                       6                       5                        4
>     31 30 29 28 27 26 25 24 23 22 21 20 19 18 17 16 15 14 13 12 11 10  9  8  7  6  5  4  3  2  1  0
>     3                       2                       1                        0
> 
>   <snip>
> 
>   2. If QUIRK_MSB_ON_THE_RIGHT is set, we do it like this:
> 
>   ::
> 
>     56 57 58 59 60 61 62 63 48 49 50 51 52 53 54 55 40 41 42 43 44 45 46 47 32 33 34 35 36 37 38 39
>     7                       6                        5                       4
>     24 25 26 27 28 29 30 31 16 17 18 19 20 21 22 23  8  9 10 11 12 13 14 15  0  1  2  3  4  5  6  7
>     3                       2                        1                       0
> 
>   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

> given byte within the payload. This reversal should be fixed to the bytes
> in the packing layout.
> 
> The logic to implement this quirk is handled within the
> adjust_for_msb_right_quirk() function. This function does not work properly
> when dealing with the bytes that contain only a partial amount of data.
> 
> In particular, consider trying to pack or unpack the range 53-44. We should
> always be mapping the bits from the logical ordering to their physical
> ordering in the same way, regardless of what sequence of bits we are
> unpacking.
> 
> This, we should grab the following logical bits:
> 
>   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
                         ^  ^  ^  ^  ^  ^  ^  ^  ^

> 
> And pack them into the physical bits:
> 
>    Physical: 48 49 50 51 52 53 54 55 40 41 42 43 44 45 46 47
>     Logical: 48 49 50 51 52 53                   44 45 46 47
>               ^  ^  ^  ^  ^  ^                    ^  ^  ^  ^
> 
> The current logic in adjust_for_msb_right_quirk is broken. I believe it is
> intending to map according to the following:
> 
>   Physical: 48 49 50 51 52 53 54 55 40 41 42 43 44 45 46 47
>    Logical:       48 49 50 51 52 53 44 45 46 47
>                    ^  ^  ^  ^  ^  ^  ^  ^  ^  ^
> 
> That is, it tries to keep the bits at the start and end of a packing
> together. This is wrong, as it makes the packing change what bit is being
> mapped to what based on which bits you're currently packing or unpacking.
> 
> Worse, the actual calculations within adjust_for_msb_right_quirk don't make
> sense.
> 
> Consider the case when packing the last byte of an unaligned packing. It
> might have a start bit of 7 and an end bit of 5. This would have a width of
> 3 bits. The new_start_bit will be calculated as the width - the box_end_bit
> - 1. This will underflow and produce a negative value, which will
> ultimate result in generating a new box_mask of all 0s.
> 
> For any other values, the result of the calculations of the
> new_box_end_bit, new_box_start_bit, and the new box_mask will result in the
> exact same values for the box_end_bit, box_start_bit, and box_mask. This
> makes the calculations completely irrelevant.
> 
> If box_end_bit is 0, and box_start_bit is 7, then the entire function of
> adjust_for_msb_right_quirk will boil down to just:
> 
>     *to_write = bitrev8(*to_write)
> 
> The other adjustments are attempting (incorrectly) to keep the bits in the
> same place but just reversed. This is not the right behavior even if
> implemented correctly, as it leaves the mapping dependent on the bit values
> being packed or unpacked.
> 
> Remove adjust_for_msb_right_quirk() and just use bitrev8 to reverse the
> byte order when interacting with the packed data.

Yes, just bitrev8() should be exactly what is needed for the "MSB on the
right within a packed byte" definition.

> 
> In particular, for packing, we need to reverse both the box_mask and the
> physical value being packed. This is done after shifting the value by
> box_end_bit so that the reversed mapping is always aligned to the physical
> buffer byte boundary. The box_mask is reversed as we're about to use it to
> clear any stale bits in the physical buffer at this block.
> 
> For unpacking, we need to reverse the contents of the physical buffer
> *before* masking with the box_mask. This is critical, as the box_mask is a
> logical mask of the bit layout before handling the QUIRK_MSB_ON_THE_RIGHT.
> 
> Add several new tests which cover this behavior. These tests will fail
> without the fix and pass afterwards. Note that no current drivers make use
> of QUIRK_MSB_ON_THE_RIGHT. I suspect this is why there have been no reports
> of this inconsistency before.
> 
> 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.

> Signed-off-by: Jacob Keller <jacob.e.keller@...el.com>
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@...el.com>
> ---

Reviewed-by: Vladimir Oltean <olteanv@...il.com>
Tested-by: Vladimir Oltean <olteanv@...il.com>

Powered by blists - more mailing lists