From: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Date: Thu, 9 Jan 2025 10:31:35 +0100

On 1/9/25 02:54, Hongchen Zhang wrote:

quoted

Hi Przemek,
On 2025/1/8 下午4:59, Przemek Kitszel wrote:

quoted

On 1/8/25 04:09, Hongchen Zhang wrote:

quoted

quoted

Hi Michal,
On 2024/1/31 pm 7:58, Michal Schmidt wrote:

quoted

new_rcp->recipe_bitmap was written to as if it were an aligned bitmap.
It is an 8-byte array, but aligned only to 4.
Use put_unaligned to set its value.

Additionally, values in ice commands are typically in little-endian.
I assume the recipe bitmap should be too, so use the *_le64
conversion.
I don't have a big-endian system with ice to test this.

I tested that the driver does not crash when probing on aarch64
anymore,
which is good enough for me. I don't know if the LAG feature actually
works.

This is what the crash looked like without the fix:

quoted

quoted

[   17.599142] Call trace:
[   17.599143]  ice_create_lag_recipe.constprop.0+0xbc/0x11c [ice]
[   17.599172]  ice_init_lag+0xcc/0x22c [ice]
[   17.599201]  ice_init_features+0x160/0x2b4 [ice]
[   17.599230]  ice_probe+0x2d0/0x30c [ice]
[   17.599258]  local_pci_probe+0x58/0xb0
[   17.599262]  work_for_cpu_fn+0x20/0x30

quoted

I encountered the same problem on a LoongArch LS3C6000 machine. Can
this patch be merged now?

What kernel base do you use?, we have merged the Steven Patches long ago

My test is based on 6.6.61 which contains Steven's patch:
  8ec08ba97fab 2024-05-07  ice: Refactor FW data type and fix bitmap
casting issue [Steven Zou]

It seems that Steven's patch can not solve the unaligned access
problem caused by new_rcp->recipe_bitmap, So is Michal's patch (may
need some change in ice_add_sw_recipe()) still needed?

thank you, I see now

I agree that ice_aqc_recipe_data_elem::recipe_bitmap[8] should be
changed to __le64, together with updated accesses. Best way to do so

Too bad I didn't notice that in ice_create_lag_recipe(), the cast is
still here. It's not valid to cast 1-byte array to a naturally aligned
one (of unsigned longs).

You can't simply change it to __le64 as 8-byte vars are
naturally-aligned, while here its offset is 4 bytes.

The best solution would be to change it to two __le32s and avoid using
bitmap helpers like set_bit() at all -- just manually assign bits there.
Alternatively, if you want -- you can use __le64, but then pack the
structure by 4 bytes, but I don't think it would give any benefit
comparing to the former.

will be as in Steven's patch.

@Michal, will you be OK with us reimplementing this, or you want to
follow up?

Thanks,
Olek