Messages in this thread | | | Date | Thu, 14 Dec 2023 11:54:17 +0100 | From | Borislav Petkov <> | Subject | Re: [PATCH v3 1/3] RAS: Introduce AMD Address Translation Library |
| |
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@amd.com> > + */ > + > +#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
| |