Hi Andy,
>-----Original Message-----
>From: linux-acpi-ow...@vger.kernel.org [mailto:linux-acpi-
>ow...@vger.kernel.org] On Behalf Of Andy Shevchenko
>Sent: 16 June 2020 10:31
>To: Shiju Jose <shiju.j...@huawei.com>
>Cc: linux-a...@vger.kernel.org; linux-...@vger.kernel.org; linux-
>ker...@vger.kernel.org; r...@rjwysocki.net; b...@alien8.de;
>james.mo...@arm.com; l...@kernel.org; tony.l...@intel.com;
>dan.carpen...@oracle.com; zhangligu...@linux.alibaba.com; Wangkefeng
>(OS Kernel Lab) <wangkefeng.w...@huawei.com>; jroe...@suse.de;
>yangyicong <yangyic...@huawei.com>; Jonathan Cameron
><jonathan.came...@huawei.com>; tanxiaofei <tanxiao...@huawei.com>;
>Linuxarm <linux...@huawei.com>
>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.shevche...@linux.intel.com]
>> >Sent: 15 June 2020 13:01
>> >To: Shiju Jose <shiju.j...@huawei.com>
>> >Cc: linux-a...@vger.kernel.org; linux-...@vger.kernel.org; linux-
>> >ker...@vger.kernel.org; r...@rjwysocki.net; b...@alien8.de;
>> >james.mo...@arm.com; l...@kernel.org; tony.l...@intel.com;
>> >dan.carpen...@oracle.com; zhangligu...@linux.alibaba.com; Wangkefeng
>> >(OS Kernel Lab) <wangkefeng.w...@huawei.com>; jroe...@suse.de;
>> >yangyicong <yangyic...@huawei.com>; Jonathan Cameron
>> ><jonathan.came...@huawei.com>; tanxiaofei <tanxiao...@huawei.com>
>> >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
