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

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

Hi Andy,

>-----Original Message-----
>From: Andy Shevchenko [mailto:andriy.shevchenko@linux.intel.com]
>Sent: 18 June 2020 16:56
>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; helgaas@kernel.org;
>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; Linuxarm
><linuxarm@huawei.com>; yangyicong <yangyicong@huawei.com>; Jonathan
>Cameron <jonathan.cameron@huawei.com>; tanxiaofei
><tanxiaofei@huawei.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@hisilicon.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@intel.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