linux-kernel - RE: [PATCH v10 2/2] PCI: hip: Add handling of HiSilicon HIP PCIe controller errors

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <761e579035d346bf8cce2dfc6857587c@huawei.com>
Date:   Thu, 18 Jun 2020 16:35:31 +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>,
        "helgaas@...nel.org" <helgaas@...nel.org>,
        "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>,
        Linuxarm <linuxarm@...wei.com>,
        yangyicong <yangyicong@...wei.com>,
        Jonathan Cameron <jonathan.cameron@...wei.com>,
        tanxiaofei <tanxiaofei@...wei.com>
Subject: RE: [PATCH v10 2/2] PCI: hip: Add handling of HiSilicon HIP PCIe
 controller errors

Hi Andy,

>-----Original Message-----
>From: Andy Shevchenko [mailto:andriy.shevchenko@...ux.intel.com]
>Sent: 18 June 2020 16:56
>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; helgaas@...nel.org;
>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; Linuxarm
><linuxarm@...wei.com>; yangyicong <yangyicong@...wei.com>; Jonathan
>Cameron <jonathan.cameron@...wei.com>; tanxiaofei
><tanxiaofei@...wei.com>
>Subject: Re: [PATCH v10 2/2] PCI: hip: Add handling of HiSilicon HIP PCIe
>controller errors
>
>On Thu, Jun 18, 2020 at 04:40:51PM +0100, Shiju Jose wrote:
>> From: Yicong Yang <yangyicong@...ilicon.com>
>>
>> The HiSilicon HIP PCIe controller is capable of handling errors on
>> root port and perform port reset separately at each root port.
>>
>> Add error handling driver for HIP PCIe controller to log and report
>> recoverable errors. Perform root port reset and restore link status
>> after the recovery.
>>
>> Following are some of the PCIe controller's recoverable errors 1.
>> completion transmission timeout error.
>> 2. CRS retry counter over the threshold error.
>> 3. ECC 2 bit errors
>> 4. AXI bresponse/rresponse errors etc.
>>
>> The driver placed in the drivers/pci/controller/ because the HIP PCIe
>> controller does not use DWC ip.
>
>> Reviewed-by: Andy Shevchenko <andriy.shevchenko@...el.com>
>
>Hmm... Did I give a tag?
>
>...
>
>> +static guid_t hisi_pcie_sec_guid =
>> +		GUID_INIT(0xB2889FC9, 0xE7D7, 0x4F9D,
>> +			0xA8, 0x67, 0xAF, 0x42, 0xE9, 0x8B, 0xE7, 0x72);
>
>Drop one TAB in each line and add two spaces before 0xA8 on the last.

Sure.

>
>
>...
>
>> +	idx = HISI_PCIE_LOCAL_VALID_ERR_MISC;
>
>> +	for_each_set_bit_from(idx, (const unsigned long *)&edata->val_bits,
>
>Can't you make val_bits unsigned long? Because this casting is incorrect.
>Otherwise, make a local copy into unsigned long variable.

The data val_bits in the error record is 64 bits, thus used u64.
Casting is added because of a compilation warning on _find_nex_bit_ function as it 
expects the type of the address as const unsigned long*.
Probably I will make local copy of val_bits into unsigned long variable.

>
>> +			      HISI_PCIE_LOCAL_VALID_ERR_MISC +
>HISI_PCIE_ERR_MISC_REGS)
>> +		dev_info(dev, "ERR_MISC_%d = 0x%x\n", idx -
>HISI_PCIE_LOCAL_VALID_ERR_MISC,
>> +			 edata->err_misc[idx]);
>
>...
>
>> +static int hisi_pcie_error_handler_probe(struct platform_device
>> +*pdev) {
>> +	struct hisi_pcie_error_private *priv;
>> +	int ret;
>> +
>
>> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
>
>(1)
>
>> +	if (!priv)
>> +		return -ENOMEM;
>> +
>> +	priv->nb.notifier_call = hisi_pcie_notify_error;
>> +	priv->dev = &pdev->dev;
>> +	ret = ghes_register_event_notifier(&priv->nb);
>> +	if (ret) {
>> +		dev_err(&pdev->dev,
>> +			"Failed to register hisi_pcie_notify_error
>function\n");
>> +		return ret;
>> +	}
>> +
>> +	platform_set_drvdata(pdev, priv);
>> +
>> +	return 0;
>> +}
>> +
>> +static int hisi_pcie_error_handler_remove(struct platform_device
>> +*pdev) {
>> +	struct hisi_pcie_error_private *priv = platform_get_drvdata(pdev);
>> +
>> +	ghes_unregister_event_notifier(&priv->nb);
>
>> +	kfree(priv);
>
>See (1), as I told you, this is double free.
>Have you tested this?
>
>> +	return 0;
>> +}
>
>--
>With Best Regards,
>Andy Shevchenko
>

Thanks,
Shiju