linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] PCI: Add pci reset quirk for Huawei Intelligent NIC virtual function
@ 2020-11-28  6:18 Chiqijun
  2020-11-28 23:29 ` Bjorn Helgaas
  0 siblings, 1 reply; 10+ messages in thread
From: Chiqijun @ 2020-11-28  6:18 UTC (permalink / raw)
  To: bhelgaas
  Cc: linux-pci, linux-kernel, yin.yinshi, cloud.wangxiaoyun,
	zengweiliang.zengweiliang, chenlizhong

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>
---
 drivers/pci/quirks.c | 67 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 67 insertions(+)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index f70692ac79c5..bd6236ea9064 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3912,6 +3912,71 @@ 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_OP             0xE80
+#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 old_command;
+	u32 val;
+
+	if (probe)
+		return 0;
+
+	bar = pci_iomap(pdev, 0, 0);
+	if (!bar)
+		return -ENOTTY;
+
+	pci_read_config_word(pdev, PCI_COMMAND, &old_command);
+
+	/*
+	 * FLR cap bit bit30, FLR ACK bit: bit18, to avoid big-endian conversion
+	 * the big-endian bit6, bit10 is directly operated here
+	 */
+	val = readl(bar + HINIC_VF_FLR_TYPE);
+	if (!(val & (1UL << 6))) {
+		pci_iounmap(pdev, bar);
+		return -ENOTTY;
+	}
+
+	val = readl(bar + HINIC_VF_OP);
+	val = val | (1UL << 10);
+	writel(val, bar + HINIC_VF_OP);
+
+	/* Perform the actual device function reset */
+	pcie_flr(pdev);
+
+	pci_write_config_word(pdev, PCI_COMMAND,
+			      old_command | PCI_COMMAND_MEMORY);
+
+	/* Waiting for device reset complete */
+	timeout = jiffies + msecs_to_jiffies(HINIC_OPERATION_TIMEOUT);
+	do {
+		val = readl(bar + HINIC_VF_OP);
+		if (!(val & (1UL << 10)))
+			goto reset_complete;
+		msleep(20);
+	} while (time_before(jiffies, timeout));
+
+	val = readl(bar + HINIC_VF_OP);
+	if (!(val & (1UL << 10)))
+		goto reset_complete;
+
+	pci_warn(pdev, "Reset dev timeout, flr ack reg: %x\n",
+		 be32_to_cpu(val));
+
+reset_complete:
+	pci_write_config_word(pdev, PCI_COMMAND, old_command);
+	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 +3988,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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] PCI: Add pci reset quirk for Huawei Intelligent NIC virtual function
  2020-11-28  6:18 [PATCH] PCI: Add pci reset quirk for Huawei Intelligent NIC virtual function Chiqijun
@ 2020-11-28 23:29 ` Bjorn Helgaas
  2020-11-30 15:46   ` Alex Williamson
  2020-12-02  8:57   ` Chiqijun
  0 siblings, 2 replies; 10+ messages in thread
From: Bjorn Helgaas @ 2020-11-28 23:29 UTC (permalink / raw)
  To: Chiqijun
  Cc: bhelgaas, linux-pci, linux-kernel, yin.yinshi, cloud.wangxiaoyun,
	zengweiliang.zengweiliang, chenlizhong, Alex Williamson

[+cc Alex]

On Sat, Nov 28, 2020 at 02:18:25PM +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.

Is this because the Huawei Intelligent NIC isn't following the spec,
or is it because Linux isn't correctly waiting for the FLR to
complete?

If this is a Huawei Intelligent NIC defect, is there documentation
somewhere (errata) that you can reference?  Will it be fixed in future
designs, so we don't have to add future Device IDs to the quirk?

> Signed-off-by: Chiqijun <chiqijun@huawei.com>
> ---
>  drivers/pci/quirks.c | 67 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 67 insertions(+)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index f70692ac79c5..bd6236ea9064 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -3912,6 +3912,71 @@ 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_OP             0xE80
> +#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 old_command;
> +	u32 val;
> +
> +	if (probe)
> +		return 0;
> +
> +	bar = pci_iomap(pdev, 0, 0);
> +	if (!bar)
> +		return -ENOTTY;
> +
> +	pci_read_config_word(pdev, PCI_COMMAND, &old_command);
> +
> +	/*
> +	 * FLR cap bit bit30, FLR ACK bit: bit18, to avoid big-endian conversion
> +	 * the big-endian bit6, bit10 is directly operated here
> +	 */
> +	val = readl(bar + HINIC_VF_FLR_TYPE);
> +	if (!(val & (1UL << 6))) {
> +		pci_iounmap(pdev, bar);
> +		return -ENOTTY;
> +	}
> +
> +	val = readl(bar + HINIC_VF_OP);
> +	val = val | (1UL << 10);
> +	writel(val, bar + HINIC_VF_OP);
> +
> +	/* Perform the actual device function reset */
> +	pcie_flr(pdev);
> +
> +	pci_write_config_word(pdev, PCI_COMMAND,
> +			      old_command | PCI_COMMAND_MEMORY);
> +
> +	/* Waiting for device reset complete */
> +	timeout = jiffies + msecs_to_jiffies(HINIC_OPERATION_TIMEOUT);
> +	do {
> +		val = readl(bar + HINIC_VF_OP);
> +		if (!(val & (1UL << 10)))
> +			goto reset_complete;
> +		msleep(20);
> +	} while (time_before(jiffies, timeout));
> +
> +	val = readl(bar + HINIC_VF_OP);
> +	if (!(val & (1UL << 10)))
> +		goto reset_complete;
> +
> +	pci_warn(pdev, "Reset dev timeout, flr ack reg: %x\n",
> +		 be32_to_cpu(val));
> +
> +reset_complete:
> +	pci_write_config_word(pdev, PCI_COMMAND, old_command);
> +	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 +3988,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
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] PCI: Add pci reset quirk for Huawei Intelligent NIC virtual function
  2020-11-28 23:29 ` Bjorn Helgaas
@ 2020-11-30 15:46   ` Alex Williamson
  2020-12-02  9:18     ` Chiqijun
  2020-12-02  8:57   ` Chiqijun
  1 sibling, 1 reply; 10+ messages in thread
From: Alex Williamson @ 2020-11-30 15:46 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Chiqijun, bhelgaas, linux-pci, linux-kernel, yin.yinshi,
	cloud.wangxiaoyun, zengweiliang.zengweiliang, chenlizhong

On Sat, 28 Nov 2020 17:29:19 -0600
Bjorn Helgaas <helgaas@kernel.org> wrote:

> [+cc Alex]
> 
> On Sat, Nov 28, 2020 at 02:18:25PM +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.  
> 
> Is this because the Huawei Intelligent NIC isn't following the spec,
> or is it because Linux isn't correctly waiting for the FLR to
> complete?

Seems like a spec compliance issue, I don't recall anything in the spec
about coordinating FLR between VFs.
 
> If this is a Huawei Intelligent NIC defect, is there documentation
> somewhere (errata) that you can reference?  Will it be fixed in future
> designs, so we don't have to add future Device IDs to the quirk?
> 
> > Signed-off-by: Chiqijun <chiqijun@huawei.com>
> > ---
> >  drivers/pci/quirks.c | 67 ++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 67 insertions(+)
> > 
> > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > index f70692ac79c5..bd6236ea9064 100644
> > --- a/drivers/pci/quirks.c
> > +++ b/drivers/pci/quirks.c
> > @@ -3912,6 +3912,71 @@ 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_OP             0xE80
> > +#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 old_command;
> > +	u32 val;
> > +
> > +	if (probe)
> > +		return 0;
> > +
> > +	bar = pci_iomap(pdev, 0, 0);
> > +	if (!bar)
> > +		return -ENOTTY;
> > +
> > +	pci_read_config_word(pdev, PCI_COMMAND, &old_command);
> > +
> > +	/*
> > +	 * FLR cap bit bit30, FLR ACK bit: bit18, to avoid big-endian conversion
> > +	 * the big-endian bit6, bit10 is directly operated here
> > +	 */
> > +	val = readl(bar + HINIC_VF_FLR_TYPE);
> > +	if (!(val & (1UL << 6))) {
> > +		pci_iounmap(pdev, bar);
> > +		return -ENOTTY;
> > +	}


I don't know exactly what this is testing, but it seems like a
feature/capability test that can fail, why is it not done as part of
the probe?  Can we define bit 6 with a macro?  Same for bit 10 in the
VF op register below.

> > +
> > +	val = readl(bar + HINIC_VF_OP);
> > +	val = val | (1UL << 10);
> > +	writel(val, bar + HINIC_VF_OP);
> > +
> > +	/* Perform the actual device function reset */
> > +	pcie_flr(pdev);
> > +
> > +	pci_write_config_word(pdev, PCI_COMMAND,
> > +			      old_command | PCI_COMMAND_MEMORY);
> > +
> > +	/* Waiting for device reset complete */
> > +	timeout = jiffies + msecs_to_jiffies(HINIC_OPERATION_TIMEOUT);

Yikes, 15s timeout!

> > +	do {
> > +		val = readl(bar + HINIC_VF_OP);
> > +		if (!(val & (1UL << 10)))
> > +			goto reset_complete;
> > +		msleep(20);
> > +	} while (time_before(jiffies, timeout));
> > +
> > +	val = readl(bar + HINIC_VF_OP);
> > +	if (!(val & (1UL << 10)))
> > +		goto reset_complete;
> > +
> > +	pci_warn(pdev, "Reset dev timeout, flr ack reg: %x\n",
> > +		 be32_to_cpu(val));
> > +
> > +reset_complete:
> > +	pci_write_config_word(pdev, PCI_COMMAND, old_command);
> > +	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 +3988,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
> >   
> 


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] PCI: Add pci reset quirk for Huawei Intelligent NIC virtual function
  2020-11-28 23:29 ` Bjorn Helgaas
  2020-11-30 15:46   ` Alex Williamson
@ 2020-12-02  8:57   ` Chiqijun
  1 sibling, 0 replies; 10+ messages in thread
From: Chiqijun @ 2020-12-02  8:57 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: bhelgaas, linux-pci, linux-kernel, Yinshi (Stone),
	Wangxiaoyun (Cloud),
	zengweiliang zengweiliang, Chenlizhong (IT Chip),
	Alex Williamson



On 2020/11/29 7:29, Bjorn Helgaas wrote:
> [+cc Alex]
> 
> On Sat, Nov 28, 2020 at 02:18:25PM +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.
> 
> Is this because the Huawei Intelligent NIC isn't following the spec,
> or is it because Linux isn't correctly waiting for the FLR to
> complete?

Huawei Intelligent NIC may have FLR exceeding 100ms specified by spec 
when multiple VFs are concurrently using FLR. This patch ensures that 
FLR is completed reliably.

> 
> If this is a Huawei Intelligent NIC defect, is there documentation
> somewhere (errata) that you can reference?  Will it be fixed in future
> designs, so we don't have to add future Device IDs to the quirk?

This problem is related to the VF specifications of Huawei Intelligent 
NIC. We will update the specifications on Huawei’s official website and 
explain this problem later;

In the follow-up Huawei Intelligent NIC series, we are trying to 
optimize the processing time of a single FLR and expand the 
specifications of concurrent VF FLR.

> 
>> Signed-off-by: Chiqijun <chiqijun@huawei.com>
>> ---
>>   drivers/pci/quirks.c | 67 ++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 67 insertions(+)
>>
>> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
>> index f70692ac79c5..bd6236ea9064 100644
>> --- a/drivers/pci/quirks.c
>> +++ b/drivers/pci/quirks.c
>> @@ -3912,6 +3912,71 @@ 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_OP             0xE80
>> +#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 old_command;
>> +	u32 val;
>> +
>> +	if (probe)
>> +		return 0;
>> +
>> +	bar = pci_iomap(pdev, 0, 0);
>> +	if (!bar)
>> +		return -ENOTTY;
>> +
>> +	pci_read_config_word(pdev, PCI_COMMAND, &old_command);
>> +
>> +	/*
>> +	 * FLR cap bit bit30, FLR ACK bit: bit18, to avoid big-endian conversion
>> +	 * the big-endian bit6, bit10 is directly operated here
>> +	 */
>> +	val = readl(bar + HINIC_VF_FLR_TYPE);
>> +	if (!(val & (1UL << 6))) {
>> +		pci_iounmap(pdev, bar);
>> +		return -ENOTTY;
>> +	}
>> +
>> +	val = readl(bar + HINIC_VF_OP);
>> +	val = val | (1UL << 10);
>> +	writel(val, bar + HINIC_VF_OP);
>> +
>> +	/* Perform the actual device function reset */
>> +	pcie_flr(pdev);
>> +
>> +	pci_write_config_word(pdev, PCI_COMMAND,
>> +			      old_command | PCI_COMMAND_MEMORY);
>> +
>> +	/* Waiting for device reset complete */
>> +	timeout = jiffies + msecs_to_jiffies(HINIC_OPERATION_TIMEOUT);
>> +	do {
>> +		val = readl(bar + HINIC_VF_OP);
>> +		if (!(val & (1UL << 10)))
>> +			goto reset_complete;
>> +		msleep(20);
>> +	} while (time_before(jiffies, timeout));
>> +
>> +	val = readl(bar + HINIC_VF_OP);
>> +	if (!(val & (1UL << 10)))
>> +		goto reset_complete;
>> +
>> +	pci_warn(pdev, "Reset dev timeout, flr ack reg: %x\n",
>> +		 be32_to_cpu(val));
>> +
>> +reset_complete:
>> +	pci_write_config_word(pdev, PCI_COMMAND, old_command);
>> +	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 +3988,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
>>
> .
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] PCI: Add pci reset quirk for Huawei Intelligent NIC virtual function
  2020-11-30 15:46   ` Alex Williamson
@ 2020-12-02  9:18     ` Chiqijun
  2020-12-02 17:46       ` Alex Williamson
  2020-12-02 20:18       ` Bjorn Helgaas
  0 siblings, 2 replies; 10+ messages in thread
From: Chiqijun @ 2020-12-02  9:18 UTC (permalink / raw)
  To: Alex Williamson, Bjorn Helgaas
  Cc: bhelgaas, linux-pci, linux-kernel, Yinshi (Stone),
	Wangxiaoyun (Cloud),
	zengweiliang zengweiliang, Chenlizhong (IT Chip)



On 2020/11/30 23:46, Alex Williamson wrote:
> On Sat, 28 Nov 2020 17:29:19 -0600
> Bjorn Helgaas <helgaas@kernel.org> wrote:
> 
>> [+cc Alex]
>>
>> On Sat, Nov 28, 2020 at 02:18:25PM +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.
>>
>> Is this because the Huawei Intelligent NIC isn't following the spec,
>> or is it because Linux isn't correctly waiting for the FLR to
>> complete?
> 
> Seems like a spec compliance issue, I don't recall anything in the spec
> about coordinating FLR between VFs.

The spec stipulates that the FLR time of a single VF does not exceed 
100ms, but when multiple VMs are reset concurrently in Linux, there will 
be multiple VF parallel FLRs, VF of Huawei Intelligent NIC
  FLR will exceed 100ms in this case.

>   
>> If this is a Huawei Intelligent NIC defect, is there documentation
>> somewhere (errata) that you can reference?  Will it be fixed in future
>> designs, so we don't have to add future Device IDs to the quirk?
>>
>>> Signed-off-by: Chiqijun <chiqijun@huawei.com>
>>> ---
>>>   drivers/pci/quirks.c | 67 ++++++++++++++++++++++++++++++++++++++++++++
>>>   1 file changed, 67 insertions(+)
>>>
>>> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
>>> index f70692ac79c5..bd6236ea9064 100644
>>> --- a/drivers/pci/quirks.c
>>> +++ b/drivers/pci/quirks.c
>>> @@ -3912,6 +3912,71 @@ 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_OP             0xE80
>>> +#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 old_command;
>>> +	u32 val;
>>> +
>>> +	if (probe)
>>> +		return 0;
>>> +
>>> +	bar = pci_iomap(pdev, 0, 0);
>>> +	if (!bar)
>>> +		return -ENOTTY;
>>> +
>>> +	pci_read_config_word(pdev, PCI_COMMAND, &old_command);
>>> +
>>> +	/*
>>> +	 * FLR cap bit bit30, FLR ACK bit: bit18, to avoid big-endian conversion
>>> +	 * the big-endian bit6, bit10 is directly operated here
>>> +	 */
>>> +	val = readl(bar + HINIC_VF_FLR_TYPE);
>>> +	if (!(val & (1UL << 6))) {
>>> +		pci_iounmap(pdev, bar);
>>> +		return -ENOTTY;
>>> +	}
> 
> 
> I don't know exactly what this is testing, but it seems like a
> feature/capability test that can fail, why is it not done as part of
> the probe?  Can we define bit 6 with a macro?  Same for bit 10 in the
> VF op register below.

The firmware of Huawei Intelligent NIC does not support this feature in 
the old version. here is the reading ability to determine whether the 
firmware supports it.
In the next patch, I will add a comment here and replace bit 6 and bit 
10 with macro definitions.

> 
>>> +
>>> +	val = readl(bar + HINIC_VF_OP);
>>> +	val = val | (1UL << 10);
>>> +	writel(val, bar + HINIC_VF_OP);
>>> +
>>> +	/* Perform the actual device function reset */
>>> +	pcie_flr(pdev);
>>> +
>>> +	pci_write_config_word(pdev, PCI_COMMAND,
>>> +			      old_command | PCI_COMMAND_MEMORY);
>>> +
>>> +	/* Waiting for device reset complete */
>>> +	timeout = jiffies + msecs_to_jiffies(HINIC_OPERATION_TIMEOUT);
> 
> Yikes, 15s timeout!

Huawei Intelligent NIC supports a maximum of 496 VFs, so the total 
timeout period is set to 15s, which will not reach the timeout time 
under normal circumstances.

> 
>>> +	do {
>>> +		val = readl(bar + HINIC_VF_OP);
>>> +		if (!(val & (1UL << 10)))
>>> +			goto reset_complete;
>>> +		msleep(20);
>>> +	} while (time_before(jiffies, timeout));
>>> +
>>> +	val = readl(bar + HINIC_VF_OP);
>>> +	if (!(val & (1UL << 10)))
>>> +		goto reset_complete;
>>> +
>>> +	pci_warn(pdev, "Reset dev timeout, flr ack reg: %x\n",
>>> +		 be32_to_cpu(val));
>>> +
>>> +reset_complete:
>>> +	pci_write_config_word(pdev, PCI_COMMAND, old_command);
>>> +	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 +3988,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
>>>    
>>
> 
> .
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] PCI: Add pci reset quirk for Huawei Intelligent NIC virtual function
  2020-12-02  9:18     ` Chiqijun
@ 2020-12-02 17:46       ` Alex Williamson
  2020-12-03 11:29         ` Chiqijun
  2020-12-02 20:18       ` Bjorn Helgaas
  1 sibling, 1 reply; 10+ messages in thread
From: Alex Williamson @ 2020-12-02 17:46 UTC (permalink / raw)
  To: Chiqijun
  Cc: Bjorn Helgaas, bhelgaas, linux-pci, linux-kernel, Yinshi (Stone),
	Wangxiaoyun (Cloud),
	zengweiliang zengweiliang, Chenlizhong (IT Chip)

On Wed, 2 Dec 2020 17:18:12 +0800
Chiqijun <chiqijun@huawei.com> wrote:

> On 2020/11/30 23:46, Alex Williamson wrote:
> > On Sat, 28 Nov 2020 17:29:19 -0600
> > Bjorn Helgaas <helgaas@kernel.org> wrote:
> >   
> >> [+cc Alex]
> >>
> >> On Sat, Nov 28, 2020 at 02:18:25PM +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.  
> >>
> >> Is this because the Huawei Intelligent NIC isn't following the spec,
> >> or is it because Linux isn't correctly waiting for the FLR to
> >> complete?  
> > 
> > Seems like a spec compliance issue, I don't recall anything in the spec
> > about coordinating FLR between VFs.  
> 
> The spec stipulates that the FLR time of a single VF does not exceed 
> 100ms, but when multiple VMs are reset concurrently in Linux, there will 
> be multiple VF parallel FLRs, VF of Huawei Intelligent NIC
>   FLR will exceed 100ms in this case.
> 
> >     
> >> If this is a Huawei Intelligent NIC defect, is there documentation
> >> somewhere (errata) that you can reference?  Will it be fixed in future
> >> designs, so we don't have to add future Device IDs to the quirk?
> >>  
> >>> Signed-off-by: Chiqijun <chiqijun@huawei.com>
> >>> ---
> >>>   drivers/pci/quirks.c | 67 ++++++++++++++++++++++++++++++++++++++++++++
> >>>   1 file changed, 67 insertions(+)
> >>>
> >>> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> >>> index f70692ac79c5..bd6236ea9064 100644
> >>> --- a/drivers/pci/quirks.c
> >>> +++ b/drivers/pci/quirks.c
> >>> @@ -3912,6 +3912,71 @@ 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_OP             0xE80
> >>> +#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 old_command;
> >>> +	u32 val;
> >>> +
> >>> +	if (probe)
> >>> +		return 0;
> >>> +
> >>> +	bar = pci_iomap(pdev, 0, 0);
> >>> +	if (!bar)
> >>> +		return -ENOTTY;
> >>> +
> >>> +	pci_read_config_word(pdev, PCI_COMMAND, &old_command);
> >>> +
> >>> +	/*
> >>> +	 * FLR cap bit bit30, FLR ACK bit: bit18, to avoid big-endian conversion
> >>> +	 * the big-endian bit6, bit10 is directly operated here
> >>> +	 */
> >>> +	val = readl(bar + HINIC_VF_FLR_TYPE);
> >>> +	if (!(val & (1UL << 6))) {
> >>> +		pci_iounmap(pdev, bar);
> >>> +		return -ENOTTY;
> >>> +	}  
> > 
> > 
> > I don't know exactly what this is testing, but it seems like a
> > feature/capability test that can fail, why is it not done as part of
> > the probe?  Can we define bit 6 with a macro?  Same for bit 10 in the
> > VF op register below.  
> 
> The firmware of Huawei Intelligent NIC does not support this feature in 
> the old version. here is the reading ability to determine whether the 
> firmware supports it.
> In the next patch, I will add a comment here and replace bit 6 and bit 
> 10 with macro definitions.


The question remains why this is not done as part of the probe.  If the
device firmware doesn't support it, isn't it better to try a regular
FLR and have it return error if the time is exceeded rather than claim
we have a functional device specific reset quirk that will always fail
without ever attempting to FLR the VF?  Thanks,

Alex

 
> >>> +
> >>> +	val = readl(bar + HINIC_VF_OP);
> >>> +	val = val | (1UL << 10);
> >>> +	writel(val, bar + HINIC_VF_OP);
> >>> +
> >>> +	/* Perform the actual device function reset */
> >>> +	pcie_flr(pdev);
> >>> +
> >>> +	pci_write_config_word(pdev, PCI_COMMAND,
> >>> +			      old_command | PCI_COMMAND_MEMORY);
> >>> +
> >>> +	/* Waiting for device reset complete */
> >>> +	timeout = jiffies + msecs_to_jiffies(HINIC_OPERATION_TIMEOUT);  
> > 
> > Yikes, 15s timeout!  
> 
> Huawei Intelligent NIC supports a maximum of 496 VFs, so the total 
> timeout period is set to 15s, which will not reach the timeout time 
> under normal circumstances.
> 
> >   
> >>> +	do {
> >>> +		val = readl(bar + HINIC_VF_OP);
> >>> +		if (!(val & (1UL << 10)))
> >>> +			goto reset_complete;
> >>> +		msleep(20);
> >>> +	} while (time_before(jiffies, timeout));
> >>> +
> >>> +	val = readl(bar + HINIC_VF_OP);
> >>> +	if (!(val & (1UL << 10)))
> >>> +		goto reset_complete;
> >>> +
> >>> +	pci_warn(pdev, "Reset dev timeout, flr ack reg: %x\n",
> >>> +		 be32_to_cpu(val));
> >>> +
> >>> +reset_complete:
> >>> +	pci_write_config_word(pdev, PCI_COMMAND, old_command);
> >>> +	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 +3988,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
> >>>      
> >>  
> > 
> > .
> >   
> 


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] PCI: Add pci reset quirk for Huawei Intelligent NIC virtual function
  2020-12-02  9:18     ` Chiqijun
  2020-12-02 17:46       ` Alex Williamson
@ 2020-12-02 20:18       ` Bjorn Helgaas
  2020-12-03 11:32         ` Chiqijun
  1 sibling, 1 reply; 10+ messages in thread
From: Bjorn Helgaas @ 2020-12-02 20:18 UTC (permalink / raw)
  To: Chiqijun
  Cc: Alex Williamson, bhelgaas, linux-pci, linux-kernel,
	Yinshi (Stone), Wangxiaoyun (Cloud),
	zengweiliang zengweiliang, Chenlizhong (IT Chip)

On Wed, Dec 02, 2020 at 05:18:12PM +0800, Chiqijun wrote:
> On 2020/11/30 23:46, Alex Williamson wrote:
> > On Sat, 28 Nov 2020 17:29:19 -0600
> > Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > On Sat, Nov 28, 2020 at 02:18:25PM +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.
> > > 
> > > Is this because the Huawei Intelligent NIC isn't following the spec,
> > > or is it because Linux isn't correctly waiting for the FLR to
> > > complete?
> > 
> > Seems like a spec compliance issue, I don't recall anything in the spec
> > about coordinating FLR between VFs.
> 
> The spec stipulates that the FLR time of a single VF does not exceed 100ms,
> but when multiple VMs are reset concurrently in Linux, there will be
> multiple VF parallel FLRs, VF of Huawei Intelligent NIC
>  FLR will exceed 100ms in this case.

Can you somehow just serialize Huawei Intelligent NIC FLR and
otherwise use the normal FLR path instead of the iomap, PCI_COMMAND
fiddling, and huge timeout below?

> > > If this is a Huawei Intelligent NIC defect, is there documentation
> > > somewhere (errata) that you can reference?  Will it be fixed in future
> > > designs, so we don't have to add future Device IDs to the quirk?
> > > 
> > > > Signed-off-by: Chiqijun <chiqijun@huawei.com>
> > > > ---
> > > >   drivers/pci/quirks.c | 67 ++++++++++++++++++++++++++++++++++++++++++++
> > > >   1 file changed, 67 insertions(+)
> > > > 
> > > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > > > index f70692ac79c5..bd6236ea9064 100644
> > > > --- a/drivers/pci/quirks.c
> > > > +++ b/drivers/pci/quirks.c
> > > > @@ -3912,6 +3912,71 @@ 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_OP             0xE80
> > > > +#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 old_command;
> > > > +	u32 val;
> > > > +
> > > > +	if (probe)
> > > > +		return 0;
> > > > +
> > > > +	bar = pci_iomap(pdev, 0, 0);
> > > > +	if (!bar)
> > > > +		return -ENOTTY;
> > > > +
> > > > +	pci_read_config_word(pdev, PCI_COMMAND, &old_command);
> > > > +
> > > > +	/*
> > > > +	 * FLR cap bit bit30, FLR ACK bit: bit18, to avoid big-endian conversion
> > > > +	 * the big-endian bit6, bit10 is directly operated here
> > > > +	 */
> > > > +	val = readl(bar + HINIC_VF_FLR_TYPE);
> > > > +	if (!(val & (1UL << 6))) {
> > > > +		pci_iounmap(pdev, bar);
> > > > +		return -ENOTTY;
> > > > +	}
> > 
> > 
> > I don't know exactly what this is testing, but it seems like a
> > feature/capability test that can fail, why is it not done as part of
> > the probe?  Can we define bit 6 with a macro?  Same for bit 10 in the
> > VF op register below.
> 
> The firmware of Huawei Intelligent NIC does not support this feature in the
> old version. here is the reading ability to determine whether the firmware
> supports it.
> In the next patch, I will add a comment here and replace bit 6 and bit 10
> with macro definitions.
> 
> > 
> > > > +
> > > > +	val = readl(bar + HINIC_VF_OP);
> > > > +	val = val | (1UL << 10);
> > > > +	writel(val, bar + HINIC_VF_OP);
> > > > +
> > > > +	/* Perform the actual device function reset */
> > > > +	pcie_flr(pdev);
> > > > +
> > > > +	pci_write_config_word(pdev, PCI_COMMAND,
> > > > +			      old_command | PCI_COMMAND_MEMORY);
> > > > +
> > > > +	/* Waiting for device reset complete */
> > > > +	timeout = jiffies + msecs_to_jiffies(HINIC_OPERATION_TIMEOUT);
> > 
> > Yikes, 15s timeout!
> 
> Huawei Intelligent NIC supports a maximum of 496 VFs, so the total timeout
> period is set to 15s, which will not reach the timeout time under normal
> circumstances.
> 
> > 
> > > > +	do {
> > > > +		val = readl(bar + HINIC_VF_OP);
> > > > +		if (!(val & (1UL << 10)))
> > > > +			goto reset_complete;
> > > > +		msleep(20);
> > > > +	} while (time_before(jiffies, timeout));
> > > > +
> > > > +	val = readl(bar + HINIC_VF_OP);
> > > > +	if (!(val & (1UL << 10)))
> > > > +		goto reset_complete;
> > > > +
> > > > +	pci_warn(pdev, "Reset dev timeout, flr ack reg: %x\n",
> > > > +		 be32_to_cpu(val));
> > > > +
> > > > +reset_complete:
> > > > +	pci_write_config_word(pdev, PCI_COMMAND, old_command);
> > > > +	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 +3988,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
> > > 
> > 
> > .
> > 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] PCI: Add pci reset quirk for Huawei Intelligent NIC virtual function
  2020-12-02 17:46       ` Alex Williamson
@ 2020-12-03 11:29         ` Chiqijun
  2020-12-03 23:56           ` Alex Williamson
  0 siblings, 1 reply; 10+ messages in thread
From: Chiqijun @ 2020-12-03 11:29 UTC (permalink / raw)
  To: Alex Williamson
  Cc: Bjorn Helgaas, bhelgaas, linux-pci, linux-kernel, Yinshi (Stone),
	Wangxiaoyun (Cloud),
	zengweiliang zengweiliang, Chenlizhong (IT Chip)



On 2020/12/3 1:46, Alex Williamson wrote:
> On Wed, 2 Dec 2020 17:18:12 +0800
> Chiqijun <chiqijun@huawei.com> wrote:
> 
>> On 2020/11/30 23:46, Alex Williamson wrote:
>>> On Sat, 28 Nov 2020 17:29:19 -0600
>>> Bjorn Helgaas <helgaas@kernel.org> wrote:
>>>    
>>>> [+cc Alex]
>>>>
>>>> On Sat, Nov 28, 2020 at 02:18:25PM +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.
>>>>
>>>> Is this because the Huawei Intelligent NIC isn't following the spec,
>>>> or is it because Linux isn't correctly waiting for the FLR to
>>>> complete?
>>>
>>> Seems like a spec compliance issue, I don't recall anything in the spec
>>> about coordinating FLR between VFs.
>>
>> The spec stipulates that the FLR time of a single VF does not exceed
>> 100ms, but when multiple VMs are reset concurrently in Linux, there will
>> be multiple VF parallel FLRs, VF of Huawei Intelligent NIC
>>    FLR will exceed 100ms in this case.
>>
>>>      
>>>> If this is a Huawei Intelligent NIC defect, is there documentation
>>>> somewhere (errata) that you can reference?  Will it be fixed in future
>>>> designs, so we don't have to add future Device IDs to the quirk?
>>>>   
>>>>> Signed-off-by: Chiqijun <chiqijun@huawei.com>
>>>>> ---
>>>>>    drivers/pci/quirks.c | 67 ++++++++++++++++++++++++++++++++++++++++++++
>>>>>    1 file changed, 67 insertions(+)
>>>>>
>>>>> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
>>>>> index f70692ac79c5..bd6236ea9064 100644
>>>>> --- a/drivers/pci/quirks.c
>>>>> +++ b/drivers/pci/quirks.c
>>>>> @@ -3912,6 +3912,71 @@ 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_OP             0xE80
>>>>> +#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 old_command;
>>>>> +	u32 val;
>>>>> +
>>>>> +	if (probe)
>>>>> +		return 0;
>>>>> +
>>>>> +	bar = pci_iomap(pdev, 0, 0);
>>>>> +	if (!bar)
>>>>> +		return -ENOTTY;
>>>>> +
>>>>> +	pci_read_config_word(pdev, PCI_COMMAND, &old_command);
>>>>> +
>>>>> +	/*
>>>>> +	 * FLR cap bit bit30, FLR ACK bit: bit18, to avoid big-endian conversion
>>>>> +	 * the big-endian bit6, bit10 is directly operated here
>>>>> +	 */
>>>>> +	val = readl(bar + HINIC_VF_FLR_TYPE);
>>>>> +	if (!(val & (1UL << 6))) {
>>>>> +		pci_iounmap(pdev, bar);
>>>>> +		return -ENOTTY;
>>>>> +	}
>>>
>>>
>>> I don't know exactly what this is testing, but it seems like a
>>> feature/capability test that can fail, why is it not done as part of
>>> the probe?  Can we define bit 6 with a macro?  Same for bit 10 in the
>>> VF op register below.
>>
>> The firmware of Huawei Intelligent NIC does not support this feature in
>> the old version. here is the reading ability to determine whether the
>> firmware supports it.
>> In the next patch, I will add a comment here and replace bit 6 and bit
>> 10 with macro definitions.
> 
> 
> The question remains why this is not done as part of the probe.  If the
> device firmware doesn't support it, isn't it better to try a regular
> FLR and have it return error if the time is exceeded rather than claim
> we have a functional device specific reset quirk that will always fail
> without ever attempting to FLR the VF?  Thanks,
> 
> Alex
> 

The firmware has always supported regular FLR. The regular FLR process 
waits for 100ms after the FLR is triggered and the FLR is considered to 
be completed, but the Huawei Intelligent NIC will exceed 100ms when the 
VF FLR is parallel, so we now need to increase the host to confirm that 
the firmware completes the FLR processing operation.
So in the probe stage, we return to support FLR, but there is no place 
to return whether the firmware supports FLR completion ack capability. 
We need to add checks during FLR, If the firmware does not support FLR 
completion ack capability, then return -ENOTTY, the kernel will still 
execute the regular FLR process.

>   
>>>>> +
>>>>> +	val = readl(bar + HINIC_VF_OP);
>>>>> +	val = val | (1UL << 10);
>>>>> +	writel(val, bar + HINIC_VF_OP);
>>>>> +
>>>>> +	/* Perform the actual device function reset */
>>>>> +	pcie_flr(pdev);
>>>>> +
>>>>> +	pci_write_config_word(pdev, PCI_COMMAND,
>>>>> +			      old_command | PCI_COMMAND_MEMORY);
>>>>> +
>>>>> +	/* Waiting for device reset complete */
>>>>> +	timeout = jiffies + msecs_to_jiffies(HINIC_OPERATION_TIMEOUT);
>>>
>>> Yikes, 15s timeout!
>>
>> Huawei Intelligent NIC supports a maximum of 496 VFs, so the total
>> timeout period is set to 15s, which will not reach the timeout time
>> under normal circumstances.
>>
>>>    
>>>>> +	do {
>>>>> +		val = readl(bar + HINIC_VF_OP);
>>>>> +		if (!(val & (1UL << 10)))
>>>>> +			goto reset_complete;
>>>>> +		msleep(20);
>>>>> +	} while (time_before(jiffies, timeout));
>>>>> +
>>>>> +	val = readl(bar + HINIC_VF_OP);
>>>>> +	if (!(val & (1UL << 10)))
>>>>> +		goto reset_complete;
>>>>> +
>>>>> +	pci_warn(pdev, "Reset dev timeout, flr ack reg: %x\n",
>>>>> +		 be32_to_cpu(val));
>>>>> +
>>>>> +reset_complete:
>>>>> +	pci_write_config_word(pdev, PCI_COMMAND, old_command);
>>>>> +	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 +3988,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
>>>>>       
>>>>   
>>>
>>> .
>>>    
>>
> 
> .
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] PCI: Add pci reset quirk for Huawei Intelligent NIC virtual function
  2020-12-02 20:18       ` Bjorn Helgaas
@ 2020-12-03 11:32         ` Chiqijun
  0 siblings, 0 replies; 10+ messages in thread
From: Chiqijun @ 2020-12-03 11:32 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Alex Williamson, bhelgaas, linux-pci, linux-kernel,
	Yinshi (Stone), Wangxiaoyun (Cloud),
	zengweiliang zengweiliang, Chenlizhong (IT Chip)



On 2020/12/3 4:18, Bjorn Helgaas wrote:
> On Wed, Dec 02, 2020 at 05:18:12PM +0800, Chiqijun wrote:
>> On 2020/11/30 23:46, Alex Williamson wrote:
>>> On Sat, 28 Nov 2020 17:29:19 -0600
>>> Bjorn Helgaas <helgaas@kernel.org> wrote:
>>>> On Sat, Nov 28, 2020 at 02:18:25PM +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.
>>>>
>>>> Is this because the Huawei Intelligent NIC isn't following the spec,
>>>> or is it because Linux isn't correctly waiting for the FLR to
>>>> complete?
>>>
>>> Seems like a spec compliance issue, I don't recall anything in the spec
>>> about coordinating FLR between VFs.
>>
>> The spec stipulates that the FLR time of a single VF does not exceed 100ms,
>> but when multiple VMs are reset concurrently in Linux, there will be
>> multiple VF parallel FLRs, VF of Huawei Intelligent NIC
>>   FLR will exceed 100ms in this case.
> 
> Can you somehow just serialize Huawei Intelligent NIC FLR and
> otherwise use the normal FLR path instead of the iomap, PCI_COMMAND
> fiddling, and huge timeout below?

FLR triggering is not controlled by Huawei Intelligent NIC. For example, 
multiple VFs are assigned to different VMs, one VM corresponds to a qemu 
process, multiple VMs perform life cycle operations (such as reset) at 
the same time, and FLR triggered by different Qemu processes will be 
parallel.
The FLR of a single VF of Huawei Intelligent NIC is within 100ms (for 
example, 50ms), while the regular FLR process will sleep for 100ms. If 
the VF FLR is forced to be serialized (for example, adding a lock), the 
overall FLR time will become longer and the VF FLR between different 
cards also become serial, the actual effect will be worse than setting 
huge timeout.

> 
>>>> If this is a Huawei Intelligent NIC defect, is there documentation
>>>> somewhere (errata) that you can reference?  Will it be fixed in future
>>>> designs, so we don't have to add future Device IDs to the quirk?
>>>>
>>>>> Signed-off-by: Chiqijun <chiqijun@huawei.com>
>>>>> ---
>>>>>    drivers/pci/quirks.c | 67 ++++++++++++++++++++++++++++++++++++++++++++
>>>>>    1 file changed, 67 insertions(+)
>>>>>
>>>>> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
>>>>> index f70692ac79c5..bd6236ea9064 100644
>>>>> --- a/drivers/pci/quirks.c
>>>>> +++ b/drivers/pci/quirks.c
>>>>> @@ -3912,6 +3912,71 @@ 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_OP             0xE80
>>>>> +#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 old_command;
>>>>> +	u32 val;
>>>>> +
>>>>> +	if (probe)
>>>>> +		return 0;
>>>>> +
>>>>> +	bar = pci_iomap(pdev, 0, 0);
>>>>> +	if (!bar)
>>>>> +		return -ENOTTY;
>>>>> +
>>>>> +	pci_read_config_word(pdev, PCI_COMMAND, &old_command);
>>>>> +
>>>>> +	/*
>>>>> +	 * FLR cap bit bit30, FLR ACK bit: bit18, to avoid big-endian conversion
>>>>> +	 * the big-endian bit6, bit10 is directly operated here
>>>>> +	 */
>>>>> +	val = readl(bar + HINIC_VF_FLR_TYPE);
>>>>> +	if (!(val & (1UL << 6))) {
>>>>> +		pci_iounmap(pdev, bar);
>>>>> +		return -ENOTTY;
>>>>> +	}
>>>
>>>
>>> I don't know exactly what this is testing, but it seems like a
>>> feature/capability test that can fail, why is it not done as part of
>>> the probe?  Can we define bit 6 with a macro?  Same for bit 10 in the
>>> VF op register below.
>>
>> The firmware of Huawei Intelligent NIC does not support this feature in the
>> old version. here is the reading ability to determine whether the firmware
>> supports it.
>> In the next patch, I will add a comment here and replace bit 6 and bit 10
>> with macro definitions.
>>
>>>
>>>>> +
>>>>> +	val = readl(bar + HINIC_VF_OP);
>>>>> +	val = val | (1UL << 10);
>>>>> +	writel(val, bar + HINIC_VF_OP);
>>>>> +
>>>>> +	/* Perform the actual device function reset */
>>>>> +	pcie_flr(pdev);
>>>>> +
>>>>> +	pci_write_config_word(pdev, PCI_COMMAND,
>>>>> +			      old_command | PCI_COMMAND_MEMORY);
>>>>> +
>>>>> +	/* Waiting for device reset complete */
>>>>> +	timeout = jiffies + msecs_to_jiffies(HINIC_OPERATION_TIMEOUT);
>>>
>>> Yikes, 15s timeout!
>>
>> Huawei Intelligent NIC supports a maximum of 496 VFs, so the total timeout
>> period is set to 15s, which will not reach the timeout time under normal
>> circumstances.
>>
>>>
>>>>> +	do {
>>>>> +		val = readl(bar + HINIC_VF_OP);
>>>>> +		if (!(val & (1UL << 10)))
>>>>> +			goto reset_complete;
>>>>> +		msleep(20);
>>>>> +	} while (time_before(jiffies, timeout));
>>>>> +
>>>>> +	val = readl(bar + HINIC_VF_OP);
>>>>> +	if (!(val & (1UL << 10)))
>>>>> +		goto reset_complete;
>>>>> +
>>>>> +	pci_warn(pdev, "Reset dev timeout, flr ack reg: %x\n",
>>>>> +		 be32_to_cpu(val));
>>>>> +
>>>>> +reset_complete:
>>>>> +	pci_write_config_word(pdev, PCI_COMMAND, old_command);
>>>>> +	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 +3988,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
>>>>
>>>
>>> .
>>>
> .
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] PCI: Add pci reset quirk for Huawei Intelligent NIC virtual function
  2020-12-03 11:29         ` Chiqijun
@ 2020-12-03 23:56           ` Alex Williamson
  0 siblings, 0 replies; 10+ messages in thread
From: Alex Williamson @ 2020-12-03 23:56 UTC (permalink / raw)
  To: Chiqijun
  Cc: Bjorn Helgaas, bhelgaas, linux-pci, linux-kernel, Yinshi (Stone),
	Wangxiaoyun (Cloud),
	zengweiliang zengweiliang, Chenlizhong (IT Chip)

On Thu, 3 Dec 2020 19:29:17 +0800
Chiqijun <chiqijun@huawei.com> wrote:

> On 2020/12/3 1:46, Alex Williamson wrote:
> > On Wed, 2 Dec 2020 17:18:12 +0800
> > Chiqijun <chiqijun@huawei.com> wrote:
> >   
> >> On 2020/11/30 23:46, Alex Williamson wrote:  
> >>> On Sat, 28 Nov 2020 17:29:19 -0600
> >>> Bjorn Helgaas <helgaas@kernel.org> wrote:
> >>>      
> >>>> [+cc Alex]
> >>>>
> >>>> On Sat, Nov 28, 2020 at 02:18:25PM +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.  
> >>>>
> >>>> Is this because the Huawei Intelligent NIC isn't following the spec,
> >>>> or is it because Linux isn't correctly waiting for the FLR to
> >>>> complete?  
> >>>
> >>> Seems like a spec compliance issue, I don't recall anything in the spec
> >>> about coordinating FLR between VFs.  
> >>
> >> The spec stipulates that the FLR time of a single VF does not exceed
> >> 100ms, but when multiple VMs are reset concurrently in Linux, there will
> >> be multiple VF parallel FLRs, VF of Huawei Intelligent NIC
> >>    FLR will exceed 100ms in this case.
> >>  
> >>>        
> >>>> If this is a Huawei Intelligent NIC defect, is there documentation
> >>>> somewhere (errata) that you can reference?  Will it be fixed in future
> >>>> designs, so we don't have to add future Device IDs to the quirk?
> >>>>     
> >>>>> Signed-off-by: Chiqijun <chiqijun@huawei.com>
> >>>>> ---
> >>>>>    drivers/pci/quirks.c | 67 ++++++++++++++++++++++++++++++++++++++++++++
> >>>>>    1 file changed, 67 insertions(+)
> >>>>>
> >>>>> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> >>>>> index f70692ac79c5..bd6236ea9064 100644
> >>>>> --- a/drivers/pci/quirks.c
> >>>>> +++ b/drivers/pci/quirks.c
> >>>>> @@ -3912,6 +3912,71 @@ 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_OP             0xE80
> >>>>> +#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 old_command;
> >>>>> +	u32 val;
> >>>>> +
> >>>>> +	if (probe)
> >>>>> +		return 0;
> >>>>> +
> >>>>> +	bar = pci_iomap(pdev, 0, 0);
> >>>>> +	if (!bar)
> >>>>> +		return -ENOTTY;
> >>>>> +
> >>>>> +	pci_read_config_word(pdev, PCI_COMMAND, &old_command);
> >>>>> +
> >>>>> +	/*
> >>>>> +	 * FLR cap bit bit30, FLR ACK bit: bit18, to avoid big-endian conversion
> >>>>> +	 * the big-endian bit6, bit10 is directly operated here
> >>>>> +	 */
> >>>>> +	val = readl(bar + HINIC_VF_FLR_TYPE);
> >>>>> +	if (!(val & (1UL << 6))) {
> >>>>> +		pci_iounmap(pdev, bar);
> >>>>> +		return -ENOTTY;
> >>>>> +	}  
> >>>
> >>>
> >>> I don't know exactly what this is testing, but it seems like a
> >>> feature/capability test that can fail, why is it not done as part of
> >>> the probe?  Can we define bit 6 with a macro?  Same for bit 10 in the
> >>> VF op register below.  
> >>
> >> The firmware of Huawei Intelligent NIC does not support this feature in
> >> the old version. here is the reading ability to determine whether the
> >> firmware supports it.
> >> In the next patch, I will add a comment here and replace bit 6 and bit
> >> 10 with macro definitions.  
> > 
> > 
> > The question remains why this is not done as part of the probe.  If the
> > device firmware doesn't support it, isn't it better to try a regular
> > FLR and have it return error if the time is exceeded rather than claim
> > we have a functional device specific reset quirk that will always fail
> > without ever attempting to FLR the VF?  Thanks,
> > 
> > Alex
> >   
> 
> The firmware has always supported regular FLR. The regular FLR process 
> waits for 100ms after the FLR is triggered and the FLR is considered to 
> be completed, but the Huawei Intelligent NIC will exceed 100ms when the 
> VF FLR is parallel, so we now need to increase the host to confirm that 
> the firmware completes the FLR processing operation.
> So in the probe stage, we return to support FLR, but there is no place 
> to return whether the firmware supports FLR completion ack capability. 
> We need to add checks during FLR, If the firmware does not support FLR 
> completion ack capability, then return -ENOTTY, the kernel will still 
> execute the regular FLR process.

I see, so we implicitly know the device supports FLR and even though
it's the device specific reset that essentially acks support for a
function level reset, we can still fall through to the base FLR reset
when we're called in the non-probe case.  A bit inconsistent, but OK.
Thanks,

Alex

 
> >>>>> +
> >>>>> +	val = readl(bar + HINIC_VF_OP);
> >>>>> +	val = val | (1UL << 10);
> >>>>> +	writel(val, bar + HINIC_VF_OP);
> >>>>> +
> >>>>> +	/* Perform the actual device function reset */
> >>>>> +	pcie_flr(pdev);
> >>>>> +
> >>>>> +	pci_write_config_word(pdev, PCI_COMMAND,
> >>>>> +			      old_command | PCI_COMMAND_MEMORY);
> >>>>> +
> >>>>> +	/* Waiting for device reset complete */
> >>>>> +	timeout = jiffies + msecs_to_jiffies(HINIC_OPERATION_TIMEOUT);  
> >>>
> >>> Yikes, 15s timeout!  
> >>
> >> Huawei Intelligent NIC supports a maximum of 496 VFs, so the total
> >> timeout period is set to 15s, which will not reach the timeout time
> >> under normal circumstances.
> >>  
> >>>      
> >>>>> +	do {
> >>>>> +		val = readl(bar + HINIC_VF_OP);
> >>>>> +		if (!(val & (1UL << 10)))
> >>>>> +			goto reset_complete;
> >>>>> +		msleep(20);
> >>>>> +	} while (time_before(jiffies, timeout));
> >>>>> +
> >>>>> +	val = readl(bar + HINIC_VF_OP);
> >>>>> +	if (!(val & (1UL << 10)))
> >>>>> +		goto reset_complete;
> >>>>> +
> >>>>> +	pci_warn(pdev, "Reset dev timeout, flr ack reg: %x\n",
> >>>>> +		 be32_to_cpu(val));
> >>>>> +
> >>>>> +reset_complete:
> >>>>> +	pci_write_config_word(pdev, PCI_COMMAND, old_command);
> >>>>> +	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 +3988,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
> >>>>>         
> >>>>     
> >>>
> >>> .
> >>>      
> >>  
> > 
> > .
> >   
> 


^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2020-12-03 23:57 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-28  6:18 [PATCH] PCI: Add pci reset quirk for Huawei Intelligent NIC virtual function Chiqijun
2020-11-28 23:29 ` Bjorn Helgaas
2020-11-30 15:46   ` Alex Williamson
2020-12-02  9:18     ` Chiqijun
2020-12-02 17:46       ` Alex Williamson
2020-12-03 11:29         ` Chiqijun
2020-12-03 23:56           ` Alex Williamson
2020-12-02 20:18       ` Bjorn Helgaas
2020-12-03 11:32         ` Chiqijun
2020-12-02  8:57   ` Chiqijun

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).