Re: [PATCH v6 3/5] EDAC/amd64: Extend family ops functions

["Chatradhi, Naveen Krishna" <nchatrad@amd.com>]

Hi Boris

On 11/10/2021 11:15 PM, Borislav Petkov wrote:
> [CAUTION: External Email]
>
> On Thu, Oct 28, 2021 at 06:31:04PM +0530, Naveen Krishna Chatradhi wrote:
>> @@ -1217,28 +1214,39 @@ static void dump_misc_regs(struct amd64_pvt *pvt)
>>   /*
>>    * See BKDG, F2x[1,0][5C:40], F2[1,0][6C:60]
>>    */
>> -static void prep_chip_selects(struct amd64_pvt *pvt)
>> +static void k8_prep_chip_selects(struct amd64_pvt *pvt)
>>   {
>>        if (pvt->fam == 0xf && pvt->ext_model < K8_REV_F) {
>>                pvt->csels[0].b_cnt = pvt->csels[1].b_cnt = 8;
>>                pvt->csels[0].m_cnt = pvt->csels[1].m_cnt = 8;
>> -     } else if (pvt->fam == 0x15 && pvt->model == 0x30) {
>> -             pvt->csels[0].b_cnt = pvt->csels[1].b_cnt = 4;
>> -             pvt->csels[0].m_cnt = pvt->csels[1].m_cnt = 2;
>> -     } else if (pvt->fam >= 0x17) {
>> -             int umc;
>> -
>> -             for_each_umc(umc) {
>> -                     pvt->csels[umc].b_cnt = 4;
>> -                     pvt->csels[umc].m_cnt = 2;
>> -             }
>> -
>> -     } else {
>> +     } else if (pvt->fam == 0xf && pvt->ext_model >= K8_REV_F) {
>>                pvt->csels[0].b_cnt = pvt->csels[1].b_cnt = 8;
>>                pvt->csels[0].m_cnt = pvt->csels[1].m_cnt = 4;
>>        }
> Why is this function looking at the family if it is called a k8_...
> function which will be set only on K8?
Will modify.
>
>>   }
>>
>> +static void f15m30_prep_chip_selects(struct amd64_pvt *pvt)
>> +{
>> +     pvt->csels[0].b_cnt = pvt->csels[1].b_cnt = 4;
>> +     pvt->csels[0].m_cnt = pvt->csels[1].m_cnt = 2;
>> +}
>> +
>> +static void default_prep_chip_selects(struct amd64_pvt *pvt)
>> +{
>> +     pvt->csels[0].b_cnt = pvt->csels[1].b_cnt = 8;
>> +     pvt->csels[0].m_cnt = pvt->csels[1].m_cnt = 4;
>> +}
>> +
>> +static void f17_prep_chip_selects(struct amd64_pvt *pvt)
>> +{
>> +     int umc;
>> +
>> +     for_each_umc(umc) {
>> +             pvt->csels[umc].b_cnt = 4;
>> +             pvt->csels[umc].m_cnt = 2;
>> +     }
>> +}
>> +
>>   static void read_umc_base_mask(struct amd64_pvt *pvt)
>>   {
>>        u32 umc_base_reg, umc_base_reg_sec;
>> @@ -1297,11 +1305,6 @@ static void read_dct_base_mask(struct amd64_pvt *pvt)
>>   {
>>        int cs;
>>
>> -     prep_chip_selects(pvt);
>> -
>> -     if (pvt->umc)
>> -             return read_umc_base_mask(pvt);
>> -
>>        for_each_chip_select(cs, 0, pvt) {
>>                int reg0   = DCSB0 + (cs * 4);
>>                int reg1   = DCSB1 + (cs * 4);
>> @@ -2512,143 +2515,181 @@ static void debug_display_dimm_sizes(struct amd64_pvt *pvt, u8 ctrl)
>>        }
>>   }
>>
>> +/* Prototypes for family specific ops routines */
>> +static int init_csrows(struct mem_ctl_info *mci);
>> +static int init_csrows_df(struct mem_ctl_info *mci);
>> +static void read_mc_regs(struct amd64_pvt *pvt);
>> +static void __read_mc_regs_df(struct amd64_pvt *pvt);
>> +static void update_umc_err_info(struct mce *m, struct err_info *err);
> Prototypes belong in headers.
Sure, this wont be necessary based on your other comments.
>
>> +
>> +static const struct low_ops k8_ops = {
> So if you're going to do this, you can go a step further and get rid of
> all those static definitions which are completely unused except those of
> the current family we're loaded on.
>
> IOW, you should make all family-specific assignments dynamic and get rid
> of family_types and those ops. Here's an example I did for K8:
>
> diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
> index 1029fe84ba2e..5f1686f22947 100644
> --- a/drivers/edac/amd64_edac.c
> +++ b/drivers/edac/amd64_edac.c
> @@ -3656,8 +3656,18 @@ static struct amd64_family_type *per_family_init(struct amd64_pvt *pvt)
>
>          switch (pvt->fam) {
>          case 0xf:
> -               fam_type        = &family_types[K8_CPUS];
> -               pvt->ops        = &family_types[K8_CPUS].ops;
> +               fam_type->ctl_name              = "K8";
> +               fam_type->f1_id                 = PCI_DEVICE_ID_AMD_K8_NB_ADDRMAP;
> +               fam_type->f2_id                 = PCI_DEVICE_ID_AMD_K8_NB_MEMCTL;
> +               fam_type->max_mcs               = 2;
> +               pvt->ops->early_channel_count   = k8_early_channel_count;
> +               pvt->ops->map_sysaddr_to_csrow  = k8_map_sysaddr_to_csrow;
> +               pvt->ops->dbam_to_cs            = k8_dbam_to_chip_select;
> +               pvt->ops->prep_chip_select      = k8_prep_chip_selects;
> +               pvt->ops->get_base_mask         = read_dct_base_mask;
> +               pvt->ops->get_misc_regs         = __dump_misc_regs;
> +               pvt->ops->get_mc_regs           = read_mc_regs;
> +               pvt->ops->populate_csrows       = init_csrows;
>                  break;
>
>          case 0x10:
>
> After that, pvt and fam_type have been properly set up.

Agreed, will create family_XXh_init() under per_family_init()'s switch.

At present, we are handling the family_type in 4/5 of this set. Will 
club them.

>
>> @@ -3735,7 +3784,16 @@ static int hw_info_get(struct amd64_pvt *pvt)
>>        if (ret)
>>                return ret;
>>
>> -     read_mc_regs(pvt);
>> +     pvt->ops->get_mc_regs(pvt);
>> +
>> +     pvt->ops->prep_chip_select(pvt);
>> +
>> +     pvt->ops->get_base_mask(pvt);
>> +
>> +     determine_memory_type(pvt);
>> +     edac_dbg(1, "  DIMM type: %s\n", edac_mem_types[pvt->dram_type]);
> Move that dbg call at the end of determine_memory_type().
Sure
>
>> +
>> +     determine_ecc_sym_sz(pvt);
>>
>>        return 0;
>>   }
>> @@ -3786,7 +3844,7 @@ static int init_one_instance(struct amd64_pvt *pvt)
>>
>>        setup_mci_misc_attrs(mci);
>>
>> -     if (init_csrows(mci))
>> +     if (pvt->ops->populate_csrows(mci))
>>                mci->edac_cap = EDAC_FLAG_NONE;
>>
>>        ret = -ENODEV;
>> diff --git a/drivers/edac/amd64_edac.h b/drivers/edac/amd64_edac.h
>> index 85aa820bc165..881ff6322bc9 100644
>> --- a/drivers/edac/amd64_edac.h
>> +++ b/drivers/edac/amd64_edac.h
>> @@ -467,11 +467,17 @@ struct ecc_settings {
>>    * functions and per device encoding/decoding logic.
>>    */
>>   struct low_ops {
>> -     int (*early_channel_count)      (struct amd64_pvt *pvt);
>> +     int  (*early_channel_count)     (struct amd64_pvt *pvt);
>>        void (*map_sysaddr_to_csrow)    (struct mem_ctl_info *mci, u64 sys_addr,
>>                                         struct err_info *);
>> -     int (*dbam_to_cs)               (struct amd64_pvt *pvt, u8 dct,
>> +     int  (*dbam_to_cs)              (struct amd64_pvt *pvt, u8 dct,
>>                                         unsigned cs_mode, int cs_mask_nr);
>> +     void (*prep_chip_select)        (struct amd64_pvt *pvt);
> That name should be "prep_chip_selects" - plural.
Got it.
>
>> +     void (*get_base_mask)           (struct amd64_pvt *pvt);
>> +     void (*get_misc_regs)           (struct amd64_pvt *pvt);
>> +     void (*get_mc_regs)             (struct amd64_pvt *pvt);
>> +     int  (*populate_csrows)         (struct mem_ctl_info *mci);
>> +     void (*get_umc_err_info)        (struct mce *m, struct err_info *err);
> WARNING: Unnecessary space before function pointer arguments
> #652: FILE: drivers/edac/amd64_edac.h:470:
> +       int  (*early_channel_count)     (struct amd64_pvt *pvt);
>
> WARNING: Unnecessary space before function pointer arguments
> #656: FILE: drivers/edac/amd64_edac.h:473:
> +       int  (*dbam_to_cs)              (struct amd64_pvt *pvt, u8 dct,
>
> WARNING: Unnecessary space before function pointer arguments
> #658: FILE: drivers/edac/amd64_edac.h:475:
> +       void (*prep_chip_select)        (struct amd64_pvt *pvt);
>
> WARNING: Unnecessary space before function pointer arguments
> #659: FILE: drivers/edac/amd64_edac.h:476:
> +       void (*get_base_mask)           (struct amd64_pvt *pvt);
>
> WARNING: Unnecessary space before function pointer arguments
> #660: FILE: drivers/edac/amd64_edac.h:477:
> +       void (*get_misc_regs)           (struct amd64_pvt *pvt);
>
> WARNING: Unnecessary space before function pointer arguments
> #661: FILE: drivers/edac/amd64_edac.h:478:
> +       void (*get_mc_regs)             (struct amd64_pvt *pvt);
>
> WARNING: Unnecessary space before function pointer arguments
> #662: FILE: drivers/edac/amd64_edac.h:479:
> +       int  (*populate_csrows)         (struct mem_ctl_info *mci);
>
> WARNING: Unnecessary space before function pointer arguments
> #663: FILE: drivers/edac/amd64_edac.h:480:
> +       void (*get_umc_err_info)        (struct mce *m, struct err_info *err);
>
> total: 0 errors, 8 warnings, 507 lines checked
>
>
> Please integrate scripts/checkpatch.pl into your patch creation
> workflow. Some of the warnings/errors *actually* make sense.
I have noticed these warnings. As there were previous definitions, i 
continued with similar indentation. Will address all of them.
>
> --
> Regards/Gruss,
>      Boris.
Thanks for the review.
>
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpeople.kernel.org%2Ftglx%2Fnotes-about-netiquette&amp;data=04%7C01%7CNaveenKrishna.Chatradhi%40amd.com%7Ca162e192bf9a44cf719308d9a471f62b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637721631674685370%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=eLKOAi8ljBkPLL04EVk4q1jTDQ1PzaNBv2Wltll%2FU24%3D&amp;reserved=0