RE: [PATCH v1] platform/mellanox: mlxbf-pmc: Add Mellanox BlueField PMC driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> -----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
> here
> - 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.

Regards,
Shravan

> - use kstroto*() API instead of sscanf. It has a range check
> 
> 
> --
> With Best Regards,
> Andy Shevchenko




[Index of Archives]     [Linux Kernel Development]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux