From: Przemek Kitszel <[email protected]> Date: Thu, 9 Jan 2025 10:31:35 +0100
> On 1/9/25 02:54, Hongchen Zhang wrote: >> Hi Przemek, >> On 2025/1/8 下午4:59, Przemek Kitszel wrote: >>> On 1/8/25 04:09, Hongchen Zhang wrote: >> >>>> Hi Michal, >>>> On 2024/1/31 pm 7:58, Michal Schmidt wrote: >>>>> 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: >>> >>>>> [ 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 >>> >>>> 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
