lkml.org 
[lkml]   [2023]   [Dec]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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

    \
     
     \ /
      Last update: 2023-12-14 11:56    [W:2.395 / U:0.864 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site