RE: [PATCH v9 2/2] PCI: hip: Add handling of HiSilicon HIP PCIe controller errors

[Shiju Jose <shiju.jose@huawei.com>]

Hi Andy,

>-----Original Message-----
>From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-
>owner@vger.kernel.org] On Behalf Of Andy Shevchenko
>Sent: 16 June 2020 10:31
>To: Shiju Jose <shiju.jose@huawei.com>
>Cc: linux-acpi@vger.kernel.org; linux-pci@vger.kernel.org; linux-
>kernel@vger.kernel.org; rjw@rjwysocki.net; bp@alien8.de;
>james.morse@arm.com; lenb@kernel.org; tony.luck@intel.com;
>dan.carpenter@oracle.com; zhangliguang@linux.alibaba.com; Wangkefeng
>(OS Kernel Lab) <wangkefeng.wang@huawei.com>; jroedel@suse.de;
>yangyicong <yangyicong@huawei.com>; Jonathan Cameron
><jonathan.cameron@huawei.com>; tanxiaofei <tanxiaofei@huawei.com>;
>Linuxarm <linuxarm@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.shevchenko@linux.intel.com]
>> >Sent: 15 June 2020 13:01
>> >To: Shiju Jose <shiju.jose@huawei.com>
>> >Cc: linux-acpi@vger.kernel.org; linux-pci@vger.kernel.org; linux-
>> >kernel@vger.kernel.org; rjw@rjwysocki.net; bp@alien8.de;
>> >james.morse@arm.com; lenb@kernel.org; tony.luck@intel.com;
>> >dan.carpenter@oracle.com; zhangliguang@linux.alibaba.com; Wangkefeng
>> >(OS Kernel Lab) <wangkefeng.wang@huawei.com>; jroedel@suse.de;
>> >yangyicong <yangyicong@huawei.com>; Jonathan Cameron
>> ><jonathan.cameron@huawei.com>; tanxiaofei <tanxiaofei@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