Re: [PATCH platform-next v2 0/3] platform/mellanox: Add nvsw core and SPC6 host driver

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 7048 bytes --]

On Tue, 20 Oct 2026, Felix Radensky wrote:

> On Mon, 19 Oct 2026, Ilpo Järvinen wrote:
> 
> > Also please check what sashiko.dev has to say about these patches.
> >
> > Note that sashiko is an AI review tool so please apply healthy bit of
> > critisism to any of its findings.
> 
> Done. The criticism turned out to be necessary, though not in the way
> I expected: none of the seven findings was a false positive, but three
> of them pointed at fixes that would have introduced bugs. I checked
> each finding against the CPLD register map rather than against the
> code alone, which is what caught those three.
> 
> Also, on your comment in patch 1: the label is gone and
> nvsw_core_init() now returns directly when
> nvsw_core_mux_topology_init() fails.
> 
> Three it got right, fixed as suggested
> ======================================
> 
> - pdb1_pwr_status..pdb4_pwr_status were exposed 0644 over
>   NVSW_REG_PS_DC_OK_OFFSET, which is not listed in
>   nvsw_writeable_reg(), so every write returned -EIO. They are 0444
>   now and the ABI documentation says read only.
> 
> - The air cooled pwr_cycle had mode 0244 rather than 0200, denying
>   read access to root while granting it to everyone else on a write
>   only attribute. The liquid cooled table already had 0200.
> 
> - Patch 3's commit message claimed the air cooled systems expose power
>   supply alarm indications, but no attribute is described over the
>   alarm registers. I dropped the claim rather than adding attributes,
>   as we have no plans to expose the alarm registers from this driver.
> 
> One where it correctly refused to guess
> =======================================
> 
> It flagged NVSW_REG_FAN_OFFSET as being used with two contradictory
> polarities: my ABI text says bit value 1 means the drawer is present,
> while mlxreg_fan_read() treats a set bit as "FAN is not connected". It
> presented both directions without picking one, which was right, since
> nothing in the patch settles it. The CPLD map does: the bits are
> FAN1_PRSNT_N..FAN5_PRSNT_N, "'0' - Present, '1' - Not Present". So
> mlxreg-fan is correct and my ABI text was wrong, fixed in the
> documentation with no code change. It also noted in passing that
> psu_status might have the same problem "if active-low", and it does,
> while psu_ac_status and psu_dc_status sharing that description block
> are active high. The polarity is now stated per attribute.
> 
> Incidentally, mlxreg-fan.c has a comment claiming the presence bit is
> one when the FAN is connected, which its own code contradicts five
> lines later. That is probably where my wrong ABI text came from -
> happy to send a trivial patch for it if you want.

Sure, lets fix all problems we know of if you have time, even if they are 
simple/trivial. :-) You can send it separately as I assume it's not 
related to this series in other than that it was discovered while 
reviewing this series.

> Two where the problem is real but a proposed fix was not
> ========================================================
> 
> asic2_health..asic4_health were broken: the masks sit at bits [3:2],
> [5:4] and [7:6] but all three carried .bit = 1, so ror32() shifted by
> zero and they returned 0/8/12 and so on instead of the documented 0, 2
> and 3. It offered two remedies - set .bit to 3, 5 and 7, or move .reg
> to NVSW_REG_ASIC{2,3,4}_HEALTH_OFFSET - and the second would have been
> worse than the bug, since all four health fields are packed into
> 0x2550 and 0x2553 is the ASIC thermal status register. v3 sets .bit to
> 3, 5 and 7.
> 
> The fan drawer LEDs are accurate against mainline: mlxreg_led_config()
> gates on (regval & data->bit) and never looks at data->slot, so with
> .bit = 0 all ten are skipped. But giving them a .bit would be wrong,
> because the capability register holds the number of fan drawers rather
> than a bitmask, so masking a count of 5 would register drawers 1 and 3
> and skip the rest. mlxreg-fan already handles both conventions; only
> leds-mlxreg does not, so the fix belongs there and we will submit it
> separately. The LED table is unchanged in v3.
> 
> The one where the fix was to delete the code
> ============================================
> 
> It reported that nvsw_core_reg_def was unsorted, so regcache_init()
> logged "Driver needs fixing: Unsorted reg_defaults", and that its
> entries never reach the hardware anyway. Both are true:
> nvsw_volatile_reg() covers every register the driver declares, so
> regcache_default_sync() skips all of them.
> 
> The obvious fix is the one mlx-platform, nvsw-sn2201 and mlxreg-lc all
> use, looping over reg_defaults with regmap_write(). Checking the two
> registers first showed that would have been a mistake. Neither is
> relevant on SPC6: 0x2537 is not a PWM control register on this
> hardware, the name being inherited from mlx-platform, and writing the
> 0x00 default would assert two active low signals that reset
> de-asserted; 0x2543 is the low aggregate interrupt mask, which
> mlxreg-hotplug programs from its own .mask_low when a board describes
> a hotplug device, and no SPC6 system does.
> 
> So v3 deletes the array rather than sorting it, along with the cache
> configuration that then has nothing left to hold. One side effect
> worth flagging, since it undoes part of what you asked for in v1:
> linux/bits.h is no longer included in nvsw-core.c, as the GENMASK in
> that array was its only user there.
> 
> SPC6 does need one register default, but neither of those two: the
> CPLD can override the fan PWM and drive the fans at full speed on a
> fan fault, and mlx-platform clears the enable bit for that override on
> every system it supports. On SPC6 the bit has moved to a different
> register, so the write is done from the board driver in patch 3,
> through the set_reg_default callback the core already provides.
> 
> The finding was real, both halves of it. But the fix it implied would
> have turned dead code into a bug.
> 
> What the tool was worth
> =======================
> 
> One finding it did not make, which it led me to: following the same
> reasoning as the pdb*_pwr_status finding, I checked all 85 registers
> in nvsw_writeable_reg() against the register map and found one more
> mismatch, NVSW_REG_FU_CAP_OFFSET. Dropped in v3.
> 
> So: every finding described something real, which is better than I
> expected, but three pointed at remedies that would have been wrong,
> and each of those would have looked plausible in review. Worth
> running, but not worth applying without checking what the registers
> actually are.
>
> Thanks for the pointer, and for the review.

Thanks for checking.

I'll try to go through the updated version still in this cycle as the 
series seemed to be in relatively good shape already (but no promises as 
merge window is already close and it is still many lines to read even if 
most of them are trivial ones).

-- 
 i.