[v3] PCI: Add pci reset quirk for Huawei Intelligent NIC virtual function

[v3] PCI: Add pci reset quirk for Huawei Intelligent NIC virtual function

[Chiqijun]

When multiple VFs do FLR at the same time, the firmware is
processed serially, resulting in some VF FLRs being delayed more
than 100ms, when the virtual machine restarts and the device
driver is loaded, the firmware is doing the corresponding VF
FLR, causing the driver to fail to load.

To solve this problem, add host and firmware status synchronization
during FLR.

Signed-off-by: Chiqijun <chiqijun@huawei.com>
---
v3:
 - The MSE bit in the VF configuration space is hardwired to zero,
   remove the setting of PCI_COMMAND_MEMORY bit. Add comment for
   set PCI_COMMAND register.

v2:
 - Update comments
 - Use the HINIC_VF_FLR_CAP_BIT_SHIFT and HINIC_VF_FLR_PROC_BIT_SHIFT
   macro instead of the magic number
---
 drivers/pci/quirks.c | 77 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 77 insertions(+)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index f70692ac79c5..9c310012ef19 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3912,6 +3912,81 @@ static int delay_250ms_after_flr(struct pci_dev *dev, int probe)
 	return 0;
 }
 
+#define PCI_DEVICE_ID_HINIC_VF      0x375E
+#define HINIC_VF_FLR_TYPE           0x1000
+#define HINIC_VF_FLR_CAP_BIT_SHIFT  6
+#define HINIC_VF_OP                 0xE80
+#define HINIC_VF_FLR_PROC_BIT_SHIFT 10
+#define HINIC_OPERATION_TIMEOUT     15000
+
+/* Device-specific reset method for Huawei Intelligent NIC virtual functions */
+static int reset_hinic_vf_dev(struct pci_dev *pdev, int probe)
+{
+	unsigned long timeout;
+	void __iomem *bar;
+	u16 command;
+	u32 val;
+
+	if (probe)
+		return 0;
+
+	bar = pci_iomap(pdev, 0, 0);
+	if (!bar)
+		return -ENOTTY;
+
+	/*
+	 * FLR cap bit bit30, FLR processing bit: bit18, to avoid big-endian
+	 * conversion the big-endian bit6, bit10 is directly operated here.
+	 *
+	 * Get and check firmware capabilities.
+	 */
+	val = readl(bar + HINIC_VF_FLR_TYPE);
+	if (!(val & (1UL << HINIC_VF_FLR_CAP_BIT_SHIFT))) {
+		pci_iounmap(pdev, bar);
+		return -ENOTTY;
+	}
+
+	/*
+	 * Set the processing bit for the start of FLR, which will be cleared
+	 * by the firmware after FLR is completed.
+	 */
+	val = readl(bar + HINIC_VF_OP);
+	val = val | (1UL << HINIC_VF_FLR_PROC_BIT_SHIFT);
+	writel(val, bar + HINIC_VF_OP);
+
+	/* Perform the actual device function reset */
+	pcie_flr(pdev);
+
+	/*
+	 * The device must learn BDF after FLR in order to respond to BAR's
+	 * read request, therefore, we issue a configure write request to let
+	 * the device capture BDF.
+	 */
+	pci_read_config_word(pdev, PCI_COMMAND, &command);
+	pci_write_config_word(pdev, PCI_COMMAND, command);
+
+	/* Waiting for device reset complete */
+	timeout = jiffies + msecs_to_jiffies(HINIC_OPERATION_TIMEOUT);
+	do {
+		val = readl(bar + HINIC_VF_OP);
+		if (!(val & (1UL << HINIC_VF_FLR_PROC_BIT_SHIFT)))
+			goto reset_complete;
+		msleep(20);
+	} while (time_before(jiffies, timeout));
+
+	val = readl(bar + HINIC_VF_OP);
+	if (!(val & (1UL << HINIC_VF_FLR_PROC_BIT_SHIFT)))
+		goto reset_complete;
+
+	pci_warn(pdev, "Reset dev timeout, flr ack reg: %x\n",
+		 be32_to_cpu(val));
+
+reset_complete:
+	pci_iounmap(pdev, bar);
+
+	return 0;
+}
+
 static const struct pci_dev_reset_methods pci_dev_reset_methods[] = {
 	{ PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82599_SFP_VF,
 		 reset_intel_82599_sfp_virtfn },
@@ -3923,6 +3998,8 @@ static const struct pci_dev_reset_methods pci_dev_reset_methods[] = {
 	{ PCI_VENDOR_ID_INTEL, 0x0953, delay_250ms_after_flr },
 	{ PCI_VENDOR_ID_CHELSIO, PCI_ANY_ID,
 		reset_chelsio_generic_dev },
+	{ PCI_VENDOR_ID_HUAWEI, PCI_DEVICE_ID_HINIC_VF,
+		reset_hinic_vf_dev },
 	{ 0 }
 };
 
-- 
2.17.1

Re: [v3] PCI: Add pci reset quirk for Huawei Intelligent NIC virtual function

[Alex Williamson]

On Fri, 25 Dec 2020 17:25:30 +0800
Chiqijun <chiqijun@huawei.com> wrote:

> When multiple VFs do FLR at the same time, the firmware is
> processed serially, resulting in some VF FLRs being delayed more
> than 100ms, when the virtual machine restarts and the device
> driver is loaded, the firmware is doing the corresponding VF
> FLR, causing the driver to fail to load.
> 
> To solve this problem, add host and firmware status synchronization
> during FLR.
> 
> Signed-off-by: Chiqijun <chiqijun@huawei.com>
> ---
> v3:
>  - The MSE bit in the VF configuration space is hardwired to zero,
>    remove the setting of PCI_COMMAND_MEMORY bit. Add comment for
>    set PCI_COMMAND register.
> 
> v2:
>  - Update comments
>  - Use the HINIC_VF_FLR_CAP_BIT_SHIFT and HINIC_VF_FLR_PROC_BIT_SHIFT
>    macro instead of the magic number
> ---
>  drivers/pci/quirks.c | 77 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 77 insertions(+)


Reviewed-by: Alex Williamson <alex.williamson@redhat.com>

> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index f70692ac79c5..9c310012ef19 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -3912,6 +3912,81 @@ static int delay_250ms_after_flr(struct pci_dev *dev, int probe)
>  	return 0;
>  }
>  
> +#define PCI_DEVICE_ID_HINIC_VF      0x375E
> +#define HINIC_VF_FLR_TYPE           0x1000
> +#define HINIC_VF_FLR_CAP_BIT_SHIFT  6
> +#define HINIC_VF_OP                 0xE80
> +#define HINIC_VF_FLR_PROC_BIT_SHIFT 10
> +#define HINIC_OPERATION_TIMEOUT     15000
> +
> +/* Device-specific reset method for Huawei Intelligent NIC virtual functions */
> +static int reset_hinic_vf_dev(struct pci_dev *pdev, int probe)
> +{
> +	unsigned long timeout;
> +	void __iomem *bar;
> +	u16 command;
> +	u32 val;
> +
> +	if (probe)
> +		return 0;
> +
> +	bar = pci_iomap(pdev, 0, 0);
> +	if (!bar)
> +		return -ENOTTY;
> +
> +	/*
> +	 * FLR cap bit bit30, FLR processing bit: bit18, to avoid big-endian
> +	 * conversion the big-endian bit6, bit10 is directly operated here.
> +	 *
> +	 * Get and check firmware capabilities.
> +	 */
> +	val = readl(bar + HINIC_VF_FLR_TYPE);
> +	if (!(val & (1UL << HINIC_VF_FLR_CAP_BIT_SHIFT))) {
> +		pci_iounmap(pdev, bar);
> +		return -ENOTTY;
> +	}
> +
> +	/*
> +	 * Set the processing bit for the start of FLR, which will be cleared
> +	 * by the firmware after FLR is completed.
> +	 */
> +	val = readl(bar + HINIC_VF_OP);
> +	val = val | (1UL << HINIC_VF_FLR_PROC_BIT_SHIFT);
> +	writel(val, bar + HINIC_VF_OP);
> +
> +	/* Perform the actual device function reset */
> +	pcie_flr(pdev);
> +
> +	/*
> +	 * The device must learn BDF after FLR in order to respond to BAR's
> +	 * read request, therefore, we issue a configure write request to let
> +	 * the device capture BDF.
> +	 */
> +	pci_read_config_word(pdev, PCI_COMMAND, &command);
> +	pci_write_config_word(pdev, PCI_COMMAND, command);
> +
> +	/* Waiting for device reset complete */
> +	timeout = jiffies + msecs_to_jiffies(HINIC_OPERATION_TIMEOUT);
> +	do {
> +		val = readl(bar + HINIC_VF_OP);
> +		if (!(val & (1UL << HINIC_VF_FLR_PROC_BIT_SHIFT)))
> +			goto reset_complete;
> +		msleep(20);
> +	} while (time_before(jiffies, timeout));
> +
> +	val = readl(bar + HINIC_VF_OP);
> +	if (!(val & (1UL << HINIC_VF_FLR_PROC_BIT_SHIFT)))
> +		goto reset_complete;
> +
> +	pci_warn(pdev, "Reset dev timeout, flr ack reg: %x\n",
> +		 be32_to_cpu(val));
> +
> +reset_complete:
> +	pci_iounmap(pdev, bar);
> +
> +	return 0;
> +}
> +
>  static const struct pci_dev_reset_methods pci_dev_reset_methods[] = {
>  	{ PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82599_SFP_VF,
>  		 reset_intel_82599_sfp_virtfn },
> @@ -3923,6 +3998,8 @@ static const struct pci_dev_reset_methods pci_dev_reset_methods[] = {
>  	{ PCI_VENDOR_ID_INTEL, 0x0953, delay_250ms_after_flr },
>  	{ PCI_VENDOR_ID_CHELSIO, PCI_ANY_ID,
>  		reset_chelsio_generic_dev },
> +	{ PCI_VENDOR_ID_HUAWEI, PCI_DEVICE_ID_HINIC_VF,
> +		reset_hinic_vf_dev },
>  	{ 0 }
>  };
>  

Re: [v3] PCI: Add pci reset quirk for Huawei Intelligent NIC virtual function

[Bjorn Helgaas]

s/pci reset/reset/ in subject (it's obvious this is for PCI).

On Fri, Dec 25, 2020 at 05:25:30PM +0800, Chiqijun wrote:
> When multiple VFs do FLR at the same time, the firmware is
> processed serially, resulting in some VF FLRs being delayed more
> than 100ms, when the virtual machine restarts and the device
> driver is loaded, the firmware is doing the corresponding VF
> FLR, causing the driver to fail to load.
> 
> To solve this problem, add host and firmware status synchronization
> during FLR.
> 
> Signed-off-by: Chiqijun <chiqijun@huawei.com>
> ---
> v3:
>  - The MSE bit in the VF configuration space is hardwired to zero,
>    remove the setting of PCI_COMMAND_MEMORY bit. Add comment for
>    set PCI_COMMAND register.
> 
> v2:
>  - Update comments
>  - Use the HINIC_VF_FLR_CAP_BIT_SHIFT and HINIC_VF_FLR_PROC_BIT_SHIFT
>    macro instead of the magic number
> ---
>  drivers/pci/quirks.c | 77 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 77 insertions(+)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index f70692ac79c5..9c310012ef19 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -3912,6 +3912,81 @@ static int delay_250ms_after_flr(struct pci_dev *dev, int probe)
>  	return 0;
>  }
>  
> +#define PCI_DEVICE_ID_HINIC_VF      0x375E
> +#define HINIC_VF_FLR_TYPE           0x1000
> +#define HINIC_VF_FLR_CAP_BIT_SHIFT  6
> +#define HINIC_VF_OP                 0xE80
> +#define HINIC_VF_FLR_PROC_BIT_SHIFT 10
> +#define HINIC_OPERATION_TIMEOUT     15000

Add a comment so we know the scale here.  "15 sec" or "15000 msec"
or similar.

> +/* Device-specific reset method for Huawei Intelligent NIC virtual functions */
> +static int reset_hinic_vf_dev(struct pci_dev *pdev, int probe)
> +{
> +	unsigned long timeout;
> +	void __iomem *bar;
> +	u16 command;
> +	u32 val;
> +
> +	if (probe)
> +		return 0;
> +
> +	bar = pci_iomap(pdev, 0, 0);
> +	if (!bar)
> +		return -ENOTTY;
> +
> +	/*
> +	 * FLR cap bit bit30, FLR processing bit: bit18, to avoid big-endian
> +	 * conversion the big-endian bit6, bit10 is directly operated here.

I don't understand the big-endian comments here.  Unless the above
adds useful information, I'd say just remove it.

Obviously, the code here has to work correctly on both big- and
little-endian systems.

Below you use be32_to_cpu() before printing HINIC_VF_OP.  Why aren't
you using it here for HINIC_VF_FLR_TYPE?  be32_to_cpu() is common in
drivers/net/ethernet/huawei/hinic/, which I assume is for the same
device.

> +	 * Get and check firmware capabilities.
> +	 */
> +	val = readl(bar + HINIC_VF_FLR_TYPE);
> +	if (!(val & (1UL << HINIC_VF_FLR_CAP_BIT_SHIFT))) {
> +		pci_iounmap(pdev, bar);
> +		return -ENOTTY;
> +	}
> +
> +	/*
> +	 * Set the processing bit for the start of FLR, which will be cleared
> +	 * by the firmware after FLR is completed.
> +	 */
> +	val = readl(bar + HINIC_VF_OP);
> +	val = val | (1UL << HINIC_VF_FLR_PROC_BIT_SHIFT);
> +	writel(val, bar + HINIC_VF_OP);
> +
> +	/* Perform the actual device function reset */
> +	pcie_flr(pdev);
> +
> +	/*
> +	 * The device must learn BDF after FLR in order to respond to BAR's
> +	 * read request, therefore, we issue a configure write request to let
> +	 * the device capture BDF.
> +	 */
> +	pci_read_config_word(pdev, PCI_COMMAND, &command);
> +	pci_write_config_word(pdev, PCI_COMMAND, command);

I assume this is because of this requirement from PCIe r5.0, sec
2.2.9:

  Functions must capture the Bus and Device Numbers supplied with all
  Type 0 Configuration Write Requests completed by the Function, and
  supply these numbers in the Bus and Device Number fields of the
  Completer ID for all Completions generated by the Device/Function.

I'm a little concerned because it seems like this requirement should
apply to *all* resets, and I don't see where we do a similar write
following other resets.  Can you help me out?  Do we need this in
other cases?  Do we do it?

I'm also slightly nervous about writing the Command register, even
though we just reset the device (so the register should be all zeroes)
and we're writing the same value we just read from it.  Wouldn't
writing 0 to the Vendor ID register, which is guaranteed to be HwInit,
accomplish the same?

> +	/* Waiting for device reset complete */
> +	timeout = jiffies + msecs_to_jiffies(HINIC_OPERATION_TIMEOUT);
> +	do {
> +		val = readl(bar + HINIC_VF_OP);
> +		if (!(val & (1UL << HINIC_VF_FLR_PROC_BIT_SHIFT)))
> +			goto reset_complete;
> +		msleep(20);
> +	} while (time_before(jiffies, timeout));
> +
> +	val = readl(bar + HINIC_VF_OP);
> +	if (!(val & (1UL << HINIC_VF_FLR_PROC_BIT_SHIFT)))
> +		goto reset_complete;
> +
> +	pci_warn(pdev, "Reset dev timeout, flr ack reg: %x\n",

"%#010x" so it's obvious that this is hex, no matter what the value.

> +		 be32_to_cpu(val));
> +
> +reset_complete:
> +	pci_iounmap(pdev, bar);
> +
> +	return 0;
> +}
> +
>  static const struct pci_dev_reset_methods pci_dev_reset_methods[] = {
>  	{ PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82599_SFP_VF,
>  		 reset_intel_82599_sfp_virtfn },
> @@ -3923,6 +3998,8 @@ static const struct pci_dev_reset_methods pci_dev_reset_methods[] = {
>  	{ PCI_VENDOR_ID_INTEL, 0x0953, delay_250ms_after_flr },
>  	{ PCI_VENDOR_ID_CHELSIO, PCI_ANY_ID,
>  		reset_chelsio_generic_dev },
> +	{ PCI_VENDOR_ID_HUAWEI, PCI_DEVICE_ID_HINIC_VF,
> +		reset_hinic_vf_dev },
>  	{ 0 }
>  };
>  
> -- 
> 2.17.1
> 

Re: [v3] PCI: Add pci reset quirk for Huawei Intelligent NIC virtual function

[Chiqijun]

On 2021/1/9 6:25, Bjorn Helgaas wrote:
> s/pci reset/reset/ in subject (it's obvious this is for PCI).

Will fix.

> 
> On Fri, Dec 25, 2020 at 05:25:30PM +0800, Chiqijun wrote:
>> When multiple VFs do FLR at the same time, the firmware is
>> processed serially, resulting in some VF FLRs being delayed more
>> than 100ms, when the virtual machine restarts and the device
>> driver is loaded, the firmware is doing the corresponding VF
>> FLR, causing the driver to fail to load.
>>
>> To solve this problem, add host and firmware status synchronization
>> during FLR.
>>
>> Signed-off-by: Chiqijun <chiqijun@huawei.com>
>> ---
>> v3:
>>   - The MSE bit in the VF configuration space is hardwired to zero,
>>     remove the setting of PCI_COMMAND_MEMORY bit. Add comment for
>>     set PCI_COMMAND register.
>>
>> v2:
>>   - Update comments
>>   - Use the HINIC_VF_FLR_CAP_BIT_SHIFT and HINIC_VF_FLR_PROC_BIT_SHIFT
>>     macro instead of the magic number
>> ---
>>   drivers/pci/quirks.c | 77 ++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 77 insertions(+)
>>
>> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
>> index f70692ac79c5..9c310012ef19 100644
>> --- a/drivers/pci/quirks.c
>> +++ b/drivers/pci/quirks.c
>> @@ -3912,6 +3912,81 @@ static int delay_250ms_after_flr(struct pci_dev *dev, int probe)
>>   	return 0;
>>   }
>>   
>> +#define PCI_DEVICE_ID_HINIC_VF      0x375E
>> +#define HINIC_VF_FLR_TYPE           0x1000
>> +#define HINIC_VF_FLR_CAP_BIT_SHIFT  6
>> +#define HINIC_VF_OP                 0xE80
>> +#define HINIC_VF_FLR_PROC_BIT_SHIFT 10
>> +#define HINIC_OPERATION_TIMEOUT     15000
> 
> Add a comment so we know the scale here.  "15 sec" or "15000 msec"
> or similar.

Will fix.

> 
>> +/* Device-specific reset method for Huawei Intelligent NIC virtual functions */
>> +static int reset_hinic_vf_dev(struct pci_dev *pdev, int probe)
>> +{
>> +	unsigned long timeout;
>> +	void __iomem *bar;
>> +	u16 command;
>> +	u32 val;
>> +
>> +	if (probe)
>> +		return 0;
>> +
>> +	bar = pci_iomap(pdev, 0, 0);
>> +	if (!bar)
>> +		return -ENOTTY;
>> +
>> +	/*
>> +	 * FLR cap bit bit30, FLR processing bit: bit18, to avoid big-endian
>> +	 * conversion the big-endian bit6, bit10 is directly operated here.
> 
> I don't understand the big-endian comments here.  Unless the above
> adds useful information, I'd say just remove it.
> 
> Obviously, the code here has to work correctly on both big- and
> little-endian systems.
> 
> Below you use be32_to_cpu() before printing HINIC_VF_OP.  Why aren't
> you using it here for HINIC_VF_FLR_TYPE?  be32_to_cpu() is common in
> drivers/net/ethernet/huawei/hinic/, which I assume is for the same
> device.
I only considered using the device on the little endian system before, 
but we should also consider using it on the big endian system, Will fix 
it in the next patch. Thanks.

> 
>> +	 * Get and check firmware capabilities.
>> +	 */
>> +	val = readl(bar + HINIC_VF_FLR_TYPE);
>> +	if (!(val & (1UL << HINIC_VF_FLR_CAP_BIT_SHIFT))) {
>> +		pci_iounmap(pdev, bar);
>> +		return -ENOTTY;
>> +	}
>> +
>> +	/*
>> +	 * Set the processing bit for the start of FLR, which will be cleared
>> +	 * by the firmware after FLR is completed.
>> +	 */
>> +	val = readl(bar + HINIC_VF_OP);
>> +	val = val | (1UL << HINIC_VF_FLR_PROC_BIT_SHIFT);
>> +	writel(val, bar + HINIC_VF_OP);
>> +
>> +	/* Perform the actual device function reset */
>> +	pcie_flr(pdev);
>> +
>> +	/*
>> +	 * The device must learn BDF after FLR in order to respond to BAR's
>> +	 * read request, therefore, we issue a configure write request to let
>> +	 * the device capture BDF.
>> +	 */
>> +	pci_read_config_word(pdev, PCI_COMMAND, &command);
>> +	pci_write_config_word(pdev, PCI_COMMAND, command);
> 
> I assume this is because of this requirement from PCIe r5.0, sec
> 2.2.9:
> 
>    Functions must capture the Bus and Device Numbers supplied with all
>    Type 0 Configuration Write Requests completed by the Function, and
>    supply these numbers in the Bus and Device Number fields of the
>    Completer ID for all Completions generated by the Device/Function.
> 
> I'm a little concerned because it seems like this requirement should
> apply to *all* resets, and I don't see where we do a similar write
> following other resets.  Can you help me out?  Do we need this in
> other cases?  Do we do it?
> 

This depends on the hardware device. The HINIC device clears the BDF 
information of the VF during FLR, so it relies on Configuration Write 
Requests to capture BDF. If other devices do not clear the DBF 
information during FLR, this operation is not required.
In addition, I did not find other devices directly access the BAR 
register after FLR in resets.

> I'm also slightly nervous about writing the Command register, even
> though we just reset the device (so the register should be all zeroes)
> and we're writing the same value we just read from it.  Wouldn't
> writing 0 to the Vendor ID register, which is guaranteed to be HwInit,
> accomplish the same?
> 

OK, writing 0 to the Vendor ID register can also achieve the same 
effect. Will fix it in the next patch.

>> +	/* Waiting for device reset complete */
>> +	timeout = jiffies + msecs_to_jiffies(HINIC_OPERATION_TIMEOUT);
>> +	do {
>> +		val = readl(bar + HINIC_VF_OP);
>> +		if (!(val & (1UL << HINIC_VF_FLR_PROC_BIT_SHIFT)))
>> +			goto reset_complete;
>> +		msleep(20);
>> +	} while (time_before(jiffies, timeout));
>> +
>> +	val = readl(bar + HINIC_VF_OP);
>> +	if (!(val & (1UL << HINIC_VF_FLR_PROC_BIT_SHIFT)))
>> +		goto reset_complete;
>> +
>> +	pci_warn(pdev, "Reset dev timeout, flr ack reg: %x\n",
> 
> "%#010x" so it's obvious that this is hex, no matter what the value.
> 

Will fix.
Thanks.

>> +		 be32_to_cpu(val));
>> +
>> +reset_complete:
>> +	pci_iounmap(pdev, bar);
>> +
>> +	return 0;
>> +}
>> +
>>   static const struct pci_dev_reset_methods pci_dev_reset_methods[] = {
>>   	{ PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82599_SFP_VF,
>>   		 reset_intel_82599_sfp_virtfn },
>> @@ -3923,6 +3998,8 @@ static const struct pci_dev_reset_methods pci_dev_reset_methods[] = {
>>   	{ PCI_VENDOR_ID_INTEL, 0x0953, delay_250ms_after_flr },
>>   	{ PCI_VENDOR_ID_CHELSIO, PCI_ANY_ID,
>>   		reset_chelsio_generic_dev },
>> +	{ PCI_VENDOR_ID_HUAWEI, PCI_DEVICE_ID_HINIC_VF,
>> +		reset_hinic_vf_dev },
>>   	{ 0 }
>>   };
>>   
>> -- 
>> 2.17.1
>>
> .
> 

Re: [v3] PCI: Add pci reset quirk for Huawei Intelligent NIC virtual function

[Bjorn Helgaas]

[Alex is a reset expert, hoping he can chime in]

On Thu, Jan 21, 2021 at 08:53:12PM +0800, Chiqijun wrote:
> On 2021/1/9 6:25, Bjorn Helgaas wrote:
> > On Fri, Dec 25, 2020 at 05:25:30PM +0800, Chiqijun wrote:
> > > When multiple VFs do FLR at the same time, the firmware is
> > > processed serially, resulting in some VF FLRs being delayed more
> > > than 100ms, when the virtual machine restarts and the device
> > > driver is loaded, the firmware is doing the corresponding VF
> > > FLR, causing the driver to fail to load.
> > > 
> > > To solve this problem, add host and firmware status synchronization
> > > during FLR.
> > > 
> > > Signed-off-by: Chiqijun <chiqijun@huawei.com>
> > > ...

> > > +	 * Get and check firmware capabilities.
> > > +	 */
> > > +	val = readl(bar + HINIC_VF_FLR_TYPE);
> > > +	if (!(val & (1UL << HINIC_VF_FLR_CAP_BIT_SHIFT))) {
> > > +		pci_iounmap(pdev, bar);
> > > +		return -ENOTTY;
> > > +	}
> > > +
> > > +	/*
> > > +	 * Set the processing bit for the start of FLR, which will be cleared
> > > +	 * by the firmware after FLR is completed.
> > > +	 */
> > > +	val = readl(bar + HINIC_VF_OP);
> > > +	val = val | (1UL << HINIC_VF_FLR_PROC_BIT_SHIFT);
> > > +	writel(val, bar + HINIC_VF_OP);
> > > +
> > > +	/* Perform the actual device function reset */
> > > +	pcie_flr(pdev);
> > > +
> > > +	/*
> > > +	 * The device must learn BDF after FLR in order to respond to BAR's
> > > +	 * read request, therefore, we issue a configure write request to let
> > > +	 * the device capture BDF.
> > > +	 */
> > > +	pci_read_config_word(pdev, PCI_COMMAND, &command);
> > > +	pci_write_config_word(pdev, PCI_COMMAND, command);
> > 
> > I assume this is because of this requirement from PCIe r5.0, sec
> > 2.2.9:
> > 
> >    Functions must capture the Bus and Device Numbers supplied with all
> >    Type 0 Configuration Write Requests completed by the Function, and
> >    supply these numbers in the Bus and Device Number fields of the
> >    Completer ID for all Completions generated by the Device/Function.
> > 
> > I'm a little concerned because it seems like this requirement should
> > apply to *all* resets, and I don't see where we do a similar write
> > following other resets.  Can you help me out?  Do we need this in
> > other cases?  Do we do it?
> 
> This depends on the hardware device. The HINIC device clears the BDF
> information of the VF during FLR, so it relies on Configuration
> Write Requests to capture BDF. If other devices do not clear the DBF
> information during FLR, this operation is not required.

If the spec says devices must keep the latched BDF during FLR, and the
HINIC doesn't comply with that, then it makes sense to do a config
write here in HINIC-specific code.

But if devices are allowed to clear the BDF during FLR, the OS has to
assume they all do, and the generic code for FLR (and probably other
resets) should do a config write so devices can latch the BDF again.

> In addition, I did not find other devices directly access the BAR register
> after FLR in resets.

I didn't catch your meaning here.

If a device loses the BDF during FLR and we don't do something to
allow it to latch the BDF again, any completions from the device will
have the wrong information.  We will likely do *some* config write to
the device eventually, which will fix this, but we can't rely on some
unknown future write to do this.  If it's a problem, we need to
explicitly do a write for this purpose.

Bjorn

Re: [v3] PCI: Add pci reset quirk for Huawei Intelligent NIC virtual function

[Chiqijun]

On 2021/1/21 23:30, Bjorn Helgaas wrote:
> [Alex is a reset expert, hoping he can chime in]
> 
> On Thu, Jan 21, 2021 at 08:53:12PM +0800, Chiqijun wrote:
>> On 2021/1/9 6:25, Bjorn Helgaas wrote:
>>> On Fri, Dec 25, 2020 at 05:25:30PM +0800, Chiqijun wrote:
>>>> When multiple VFs do FLR at the same time, the firmware is
>>>> processed serially, resulting in some VF FLRs being delayed more
>>>> than 100ms, when the virtual machine restarts and the device
>>>> driver is loaded, the firmware is doing the corresponding VF
>>>> FLR, causing the driver to fail to load.
>>>>
>>>> To solve this problem, add host and firmware status synchronization
>>>> during FLR.
>>>>
>>>> Signed-off-by: Chiqijun <chiqijun@huawei.com>
>>>> ...
> 
>>>> +	 * Get and check firmware capabilities.
>>>> +	 */
>>>> +	val = readl(bar + HINIC_VF_FLR_TYPE);
>>>> +	if (!(val & (1UL << HINIC_VF_FLR_CAP_BIT_SHIFT))) {
>>>> +		pci_iounmap(pdev, bar);
>>>> +		return -ENOTTY;
>>>> +	}
>>>> +
>>>> +	/*
>>>> +	 * Set the processing bit for the start of FLR, which will be cleared
>>>> +	 * by the firmware after FLR is completed.
>>>> +	 */
>>>> +	val = readl(bar + HINIC_VF_OP);
>>>> +	val = val | (1UL << HINIC_VF_FLR_PROC_BIT_SHIFT);
>>>> +	writel(val, bar + HINIC_VF_OP);
>>>> +
>>>> +	/* Perform the actual device function reset */
>>>> +	pcie_flr(pdev);
>>>> +
>>>> +	/*
>>>> +	 * The device must learn BDF after FLR in order to respond to BAR's
>>>> +	 * read request, therefore, we issue a configure write request to let
>>>> +	 * the device capture BDF.
>>>> +	 */
>>>> +	pci_read_config_word(pdev, PCI_COMMAND, &command);
>>>> +	pci_write_config_word(pdev, PCI_COMMAND, command);
>>>
>>> I assume this is because of this requirement from PCIe r5.0, sec
>>> 2.2.9:
>>>
>>>     Functions must capture the Bus and Device Numbers supplied with all
>>>     Type 0 Configuration Write Requests completed by the Function, and
>>>     supply these numbers in the Bus and Device Number fields of the
>>>     Completer ID for all Completions generated by the Device/Function.
>>>
>>> I'm a little concerned because it seems like this requirement should
>>> apply to *all* resets, and I don't see where we do a similar write
>>> following other resets.  Can you help me out?  Do we need this in
>>> other cases?  Do we do it?
>>
>> This depends on the hardware device. The HINIC device clears the BDF
>> information of the VF during FLR, so it relies on Configuration
>> Write Requests to capture BDF. If other devices do not clear the DBF
>> information during FLR, this operation is not required.
> 
> If the spec says devices must keep the latched BDF during FLR, and the
> HINIC doesn't comply with that, then it makes sense to do a config
> write here in HINIC-specific code.
> 
> But if devices are allowed to clear the BDF during FLR, the OS has to
> assume they all do, and the generic code for FLR (and probably other
> resets) should do a config write so devices can latch the BDF again.
> 
>> In addition, I did not find other devices directly access the BAR register
>> after FLR in resets.
> 
> I didn't catch your meaning here.
> 
> If a device loses the BDF during FLR and we don't do something to
> allow it to latch the BDF again, any completions from the device will
> have the wrong information.  We will likely do *some* config write to
> the device eventually, which will fix this, but we can't rely on some
> unknown future write to do this.  If it's a problem, we need to
> explicitly do a write for this purpose.
> 
> Bjorn
> .
> 
>     
The spec does not specify whether the BDF needs to be kept after FLR, 
but the section 2.2.9 of the PCIe r5.0 has the following description:

     If a Function must generate a Completion prior to the initial
     device Configuration Write Request, 0's must be entered into the
     Bus Number and Device Number fields

Does this mean that we should always get the expected completion
after initializing the device?