RE: [PATCH v1] platform/mellanox: mlxbf-pmc: Add Mellanox BlueField PMC driver
From: Shravan Ramani
Date: Tue Jul 28 2020 - 01:04:34 EST
> -----Original Message-----
> From: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
> Sent: Monday, July 27, 2020 4:24 PM
> To: Shravan Ramani <sramani@xxxxxxxxxxxx>
> Cc: Andy Shevchenko <andy@xxxxxxxxxxxxx>; Darren Hart
> <dvhart@xxxxxxxxxxxxx>; Vadim Pasternak <vadimp@xxxxxxxxxxxx>; Jiri Pirko
> <jiri@xxxxxxxxxxxx>; Platform Driver <platform-driver-x86@xxxxxxxxxxxxxxx>;
> Linux Kernel Mailing List <linux-kernel@xxxxxxxxxxxxxxx>
> Subject: Re: [PATCH v1] platform/mellanox: mlxbf-pmc: Add Mellanox BlueField
> PMC driver
> On Mon, Jul 27, 2020 at 12:02 PM Shravan Kumar Ramani
> <sramani@xxxxxxxxxxxx> wrote:
> > The performance modules in BlueField are present in several hardware
> > blocks and each block provides access to these stats either through
> > counters that can be programmed to monitor supported events or through
> > memory-mapped registers that hold the relevant information.
> > The hardware blocks that include a performance module are:
> > * Tile (block containing 2 cores and a shared L2 cache)
> > * TRIO (PCIe root complex)
> > * MSS (Memory Sub-system containing the Memory Controller and L3
> > cache)
> > * GIC (Interrupt controller)
> > * SMMU (System Memory Management Unit) The mlx_pmc driver provides
> > access to all of these performance modules through a hwmon sysfs
> > interface.
> Just brief comments:
> - consider to revisit header block to see what is really necessary and what can
> be dropped
> - add comma to the arrays where last line is not a termination
> - look at match_string() / sysfs_match_string() API, I think they can be utilised
> - UUID manipulations (esp. with that GUID_INIT() against non-constant) seems
> too much, consider refactoring and cleaning up these pieces
Could you please elaborate on what approach you'd like me to take with the UUID manipulation?
I used the same approach as in drivers/platform/mellanox/mlxbf-bootctl.c which seemed like an appropriate example.
Any other pointers would be helpful.
Thanks for the feedback. Will address all the other comments in v2.
> - use kstroto*() API instead of sscanf. It has a range check
> With Best Regards,
> Andy Shevchenko