Hi Andy, >-----Original Message----- >From: linux-acpi-owner@xxxxxxxxxxxxxxx [mailto:linux-acpi- >owner@xxxxxxxxxxxxxxx] On Behalf Of Andy Shevchenko >Sent: 16 June 2020 10:31 >To: Shiju Jose <shiju.jose@xxxxxxxxxx> >Cc: linux-acpi@xxxxxxxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx; linux- >kernel@xxxxxxxxxxxxxxx; rjw@xxxxxxxxxxxxx; bp@xxxxxxxxx; >james.morse@xxxxxxx; lenb@xxxxxxxxxx; tony.luck@xxxxxxxxx; >dan.carpenter@xxxxxxxxxx; zhangliguang@xxxxxxxxxxxxxxxxx; Wangkefeng >(OS Kernel Lab) <wangkefeng.wang@xxxxxxxxxx>; jroedel@xxxxxxx; >yangyicong <yangyicong@xxxxxxxxxx>; Jonathan Cameron ><jonathan.cameron@xxxxxxxxxx>; tanxiaofei <tanxiaofei@xxxxxxxxxx>; >Linuxarm <linuxarm@xxxxxxxxxx> >Subject: Re: [PATCH v9 2/2] PCI: hip: Add handling of HiSilicon HIP PCIe >controller errors > >On Tue, Jun 16, 2020 at 09:12:56AM +0000, Shiju Jose wrote: >> >-----Original Message----- >> >From: Andy Shevchenko [mailto:andriy.shevchenko@xxxxxxxxxxxxxxx] >> >Sent: 15 June 2020 13:01 >> >To: Shiju Jose <shiju.jose@xxxxxxxxxx> >> >Cc: linux-acpi@xxxxxxxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx; linux- >> >kernel@xxxxxxxxxxxxxxx; rjw@xxxxxxxxxxxxx; bp@xxxxxxxxx; >> >james.morse@xxxxxxx; lenb@xxxxxxxxxx; tony.luck@xxxxxxxxx; >> >dan.carpenter@xxxxxxxxxx; zhangliguang@xxxxxxxxxxxxxxxxx; Wangkefeng >> >(OS Kernel Lab) <wangkefeng.wang@xxxxxxxxxx>; jroedel@xxxxxxx; >> >yangyicong <yangyicong@xxxxxxxxxx>; Jonathan Cameron >> ><jonathan.cameron@xxxxxxxxxx>; tanxiaofei <tanxiaofei@xxxxxxxxxx> >> >Subject: Re: [PATCH v9 2/2] PCI: hip: Add handling of HiSilicon HIP >> >PCIe controller errors >> > >> >On Mon, Jun 15, 2020 at 11:15:52AM +0100, Shiju Jose wrote: > >... > >> >bits.h ? >> >> Ok. I think bits.h was already included through some other .h files. > >You have direct users of the header here. >The rule of thumb is to include all headers of which you have direct users. >Some exceptions of course can be applied, but for generic headers like bits.h >there are only bitops.h or bitmap.h that guarantee inclusion of the >mentioned macros / definitions. > >I don't see any header of the same domain in the list. Ok. > >... > >> >> +#define HISI_PCIE_CORE_PORT_ID(v) (((v) % 8) << 1) >> > >> >% -> & ? >> (((v) % 8) << 1) is correct. We can make bit operation instead. > >y % x is usually being used when we consume y / x or in cases when y is >advanced and we need to keep it under some threshold. > >Here it's not obvious to me, and usual pattern is to use bitwise operations. > >In any case some clarification is needed. We want (v % 8) * 2 here to get the core port id, a numerical value but not a bit mask. Maybe you want us to use ((v) & 7) << 1? please point it out if I understand wrong. > >... > >> >> +struct hisi_pcie_error_private { >> >> + struct notifier_block nb; >> >> + struct platform_device *pdev; >> > >> >Do you really need platform device? Isn't struct device * enough? >> We need platform device as the error recovery device is a platform >> device, which provides us the "RST" reset method. > >Can't you derive platform device from struct device pointer? >I really didn't see an evidence you need to keep it like this. > >And in probably single case you may derive it, no? We will check by making this change. > >> >> +}; > >... > >> >> +static char *hisi_pcie_sub_module_name(u8 id) { >> >> + switch (id) { >> >> + case HISI_PCIE_SUB_MODULE_ID_AP: return "AP Layer"; >> >> + case HISI_PCIE_SUB_MODULE_ID_TL: return "TL Layer"; >> >> + case HISI_PCIE_SUB_MODULE_ID_MAC: return "MAC Layer"; >> >> + case HISI_PCIE_SUB_MODULE_ID_DL: return "DL Layer"; >> >> + case HISI_PCIE_SUB_MODULE_ID_SDI: return "SDI Layer"; >> >> + } >> > >> >match_string() ? >> >> match_string() does not work here because we need sub module id -> >> string conversion. > >Why? Are you using non-sequential (a.k.a. sparse) values? These are the sequential values. I mean in this case we do not have the third parameter to the match_string(), string to match with the strings in the array, we just have the value for the sub module id. Can you suggest some example of match_string() for the similar case? > > ... >... > >> >> + for (i = 0; i < HISI_PCIE_ERR_MISC_REGS; i++) { >> >> + if (edata->val_bits & >> >> + BIT_ULL(HISI_PCIE_LOCAL_VALID_ERR_MISC >> >+ i)) >> > >> >for_each_set_bit() ? >> >> Can't use for_each_set_bit() here because edata->val_bits contains >> valid bits for other fields of the error data as well, those need to printed >separately. > >So, I don't get why. > >You have at least two possibilities: >1/ use bitwise & to drop non-related bits (maybe in temporary variable) 2/ >use for_each_set_bit_from() Ok. I think for_each_set_bit_from() may be better. > > Thanks, Shiju
