Re: [PATCH v3 1/3] RAS: Introduce AMD Address Translation Library

[Borislav Petkov]

On Sun, Dec 10, 2023 at 01:49:30PM -0600, Yazen Ghannam wrote:
> diff --git a/drivers/ras/amd/atl/internal.h b/drivers/ras/amd/atl/internal.h
> new file mode 100644
> index 000000000000..08bc46f7cabf
> --- /dev/null
> +++ b/drivers/ras/amd/atl/internal.h
> @@ -0,0 +1,312 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * AMD Address Translation Library
> + *
> + * internal.h : Helper functions and common defines
> + *
> + * Copyright (c) 2023, Advanced Micro Devices, Inc.
> + * All Rights Reserved.
> + *
> + * Author: Yazen Ghannam <Yazen.Ghannam@xxxxxxx>
> + */
> +
> +#ifndef __AMD_ATL_INTERNAL_H__
> +#define __AMD_ATL_INTERNAL_H__
> +
> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>
> +
> +#include <asm/amd_nb.h>
> +
> +#include "reg_fields.h"
> +#include "stub.h"
> +
> +/* Maximum possible number of Coherent Stations within a single Data Fabric. */
> +#define MAX_CS_CHANNELS			32

Hmm, that's coherent stations and not chip select. Can we differentiate
between the two pls?

In my mind "cs" is chip select so can we call the other thing "coh_st"
or so?

...

> +/*
> + * Make a gap in 'data' that is 'num_bits' long starting at 'bit_num.

@data, @num_bits, @bit_num

This is the kernel-doc notation. You don't need make this function
kernel-doc yet but might as well get accustomed to the syntax. :)

> + * e.g. data		= 11111111'b
> + *	bit_num		= 3
> + *	num_bits	= 2
> + *	result		= 1111100111'b
> + */
> +static inline u64 expand_bits(u8 bit_num, u8 num_bits, u64 data)
> +{
> +	u64 temp1, temp2;
> +
> +	/*
> +	 * Return the original data if the "space" needed is '0'.
> +	 * This helps avoid the need to check for '0' at each
> +	 * caller.
> +	 */

Yeah, you don't need that comment - it is kinda obvious that the
function should check for nonsensical inputs.

> +	if (!num_bits)
> +		return data;
> +
> +	if (!bit_num)
> +		return data << num_bits;

Like here: I was gonna say that num_bits cannot be more than
BITS_PER_LONG (approximating here on 64-bit) because it'll turn @data
into 0 but people get what they asked for.

Might do here at least a warn or so:

	WARN_ON_ONCE(num_bits >= BITS_PER_LONG);

The same thing for the upper limit of bit_num.

> +	temp1 = data & GENMASK_ULL(bit_num - 1, 0);
> +
> +	temp2 = data & GENMASK_ULL(63, bit_num);
> +	temp2 <<= num_bits;
> +
> +	return temp1 | temp2;
> +}
> +
> +/*
> + * Remove bits in 'data' between low_bit and high_bit inclusive.
> + * e.g. data		= XXXYYZZZ'b
> + *	low_bit		= 3
> + *	high_bit	= 4
> + *	result		= XXXZZZ'b
> + */
> +static inline u64 remove_bits(u8 low_bit, u8 high_bit, u64 data)

Ditto for this one.

...

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette