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. ... > >> +#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. ... > >> +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? > >> +}; ... > >> +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? > >> + return "unknown"; > > > >> +} > >> + > >> +static char *hisi_pcie_error_severity(u8 err_sev) { > >> + switch (err_sev) { > >> + case HISI_ERR_SEV_RECOVERABLE: return "recoverable"; > >> + case HISI_ERR_SEV_FATAL: return "fatal"; > >> + case HISI_ERR_SEV_CORRECTED: return "corrected"; > >> + case HISI_ERR_SEV_NONE: return "none"; > >> + } > > > >Ditto? > > Same as above. Ditto. > >> + return "unknown"; > >> +} ... > >> + 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() You really need to know better the kernel libraries. ... > >> + const struct hisi_pcie_error_data *error_data = > >> + acpi_hest_get_payload(gdata); > > > >One line is better to read. > > Checkpatch warning, the length of line will exceed 80 characters. No, it doesn't. Perhaps time to update? -- With Best Regards, Andy Shevchenko
