| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <997bd6a17f54433dbdd2c43155682bdd@huawei.com>
Date: Tue, 16 Jun 2020 11:55:46 +0000
From: Shiju Jose <shiju.jose@...wei.com>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
CC: "linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>,
"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"rjw@...ysocki.net" <rjw@...ysocki.net>,
"bp@...en8.de" <bp@...en8.de>,
"james.morse@....com" <james.morse@....com>,
"lenb@...nel.org" <lenb@...nel.org>,
"tony.luck@...el.com" <tony.luck@...el.com>,
"dan.carpenter@...cle.com" <dan.carpenter@...cle.com>,
"zhangliguang@...ux.alibaba.com" <zhangliguang@...ux.alibaba.com>,
"Wangkefeng (OS Kernel Lab)" <wangkefeng.wang@...wei.com>,
"jroedel@...e.de" <jroedel@...e.de>,
yangyicong <yangyicong@...wei.com>,
Jonathan Cameron <jonathan.cameron@...wei.com>,
tanxiaofei <tanxiaofei@...wei.com>,
Linuxarm <linuxarm@...wei.com>
Subject: RE: [PATCH v9 2/2] PCI: hip: Add handling of HiSilicon HIP PCIe
controller errors
Hi Andy,
>-----Original Message-----
>From: linux-acpi-owner@...r.kernel.org [mailto:linux-acpi-
>owner@...r.kernel.org] On Behalf Of Andy Shevchenko
>Sent: 16 June 2020 10:31
>To: Shiju Jose <shiju.jose@...wei.com>
>Cc: linux-acpi@...r.kernel.org; linux-pci@...r.kernel.org; linux-
>kernel@...r.kernel.org; rjw@...ysocki.net; bp@...en8.de;
>james.morse@....com; lenb@...nel.org; tony.luck@...el.com;
>dan.carpenter@...cle.com; zhangliguang@...ux.alibaba.com; Wangkefeng
>(OS Kernel Lab) <wangkefeng.wang@...wei.com>; jroedel@...e.de;
>yangyicong <yangyicong@...wei.com>; Jonathan Cameron
><jonathan.cameron@...wei.com>; tanxiaofei <tanxiaofei@...wei.com>;
>Linuxarm <linuxarm@...wei.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@...ux.intel.com]
>> >Sent: 15 June 2020 13:01
>> >To: Shiju Jose <shiju.jose@...wei.com>
>> >Cc: linux-acpi@...r.kernel.org; linux-pci@...r.kernel.org; linux-
>> >kernel@...r.kernel.org; rjw@...ysocki.net; bp@...en8.de;
>> >james.morse@....com; lenb@...nel.org; tony.luck@...el.com;
>> >dan.carpenter@...cle.com; zhangliguang@...ux.alibaba.com; Wangkefeng
>> >(OS Kernel Lab) <wangkefeng.wang@...wei.com>; jroedel@...e.de;
>> >yangyicong <yangyicong@...wei.com>; Jonathan Cameron
>> ><jonathan.cameron@...wei.com>; tanxiaofei <tanxiaofei@...wei.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
Powered by blists - more mailing lists