[PATCH v3]PCI: hv: fix PCI-BUS domainID corruption

[PATCH v3]PCI: hv: fix PCI-BUS domainID corruption

[Sridhar Pitchai]

Whenever PCI bus is added, HyperV guarantees the BUS id is unique. Even
with that when a first device is added to the bus, it overrides bus domain
ID with the device serial number. Sometime this can result in BUS ID not
being unique. In this case, when PCI_BUS and a device to bus is added, the
first device overwrites the bus domain ID to the device serial number,
which is 0. Since there exsist a PCI bus with domain ID 0 already the PCI
bus addition fails. This patch make sure when a device is added to a bus,
it never updated the bus domain ID. Since we have the transparent SRIOV
mode now, the short VF device name is no longer needed.

Fixes: 4a9b0933bdfc("PCI:hv:Use device serial number as PCI domain")
Cc: stable@vger.kernel.org
Signed-off-by: Sridhar Pitchai <srpitcha@microsoft.com>
---

Changes in v3:
* fix the commit comment. [KY Srinivasan, Michael Kelley]
---
 drivers/pci/host/pci-hyperv.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
index 2faf38e..ac67e56 100644
--- a/drivers/pci/host/pci-hyperv.c
+++ b/drivers/pci/host/pci-hyperv.c
@@ -1518,17 +1518,6 @@ static struct hv_pci_dev *new_pcichild_device(struct hv_pcibus_device *hbus,
 	get_pcichild(hpdev, hv_pcidev_ref_childlist);
 	spin_lock_irqsave(&hbus->device_list_lock, flags);
 
-	/*
-	 * When a device is being added to the bus, we set the PCI domain
-	 * number to be the device serial number, which is non-zero and
-	 * unique on the same VM.  The serial numbers start with 1, and
-	 * increase by 1 for each device.  So device names including this
-	 * can have shorter names than based on the bus instance UUID.
-	 * Only the first device serial number is used for domain, so the
-	 * domain number will not change after the first device is added.
-	 */
-	if (list_empty(&hbus->children))
-		hbus->sysdata.domain = desc->ser;
 	list_add_tail(&hpdev->list_entry, &hbus->children);
 	spin_unlock_irqrestore(&hbus->device_list_lock, flags);
 	return hpdev;
-- 
2.7.4 

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3]PCI: hv: fix PCI-BUS domainID corruption

[Lorenzo Pieralisi]

On Thu, Mar 15, 2018 at 12:03:07AM +0000, Sridhar Pitchai wrote:
> Whenever PCI bus is added, HyperV guarantees the BUS id is unique. Even

"Whenever a PCI bus is added"

> with that when a first device is added to the bus, it overrides bus domain
> ID with the device serial number. Sometime this can result in BUS ID not

Define "Sometime".

> being unique. In this case, when PCI_BUS and a device to bus is added, the
> first device overwrites the bus domain ID to the device serial number,
> which is 0. Since there exsist a PCI bus with domain ID 0 already the PCI

s/exsist/exist

> bus addition fails. This patch make sure when a device is added to a bus,
> it never updated the bus domain ID. 

s/updated/updates

> Since we have the transparent SRIOV mode now, the short VF device name
> is no longer needed.

I still do not understand what this means and how it is related to the
patch below, it may be clear to you, it is not to me, at all.

> Fixes: 4a9b0933bdfc("PCI:hv:Use device serial number as PCI domain")

Fixes: 4a9b0933bdfc ("PCI: hv: Use device serial number as PCI domain")

I asked you an explicit question. Commit above was added for a reason
I assume. This patch implies that kernel has been broken since v4.11
which is almost a year ago and nobody every noticed ? Or there are
systems where commit above is _necessary_ and this patch would break
them ?

I want a detailed explanation that highlights *why* it is safe to apply
this patch and send it to stable kernels, commit log above won't do.

Thanks,
Lorenzo

> Cc: stable@vger.kernel.org
> Signed-off-by: Sridhar Pitchai <srpitcha@microsoft.com>
> ---
> 
> Changes in v3:
> * fix the commit comment. [KY Srinivasan, Michael Kelley]
> ---
>  drivers/pci/host/pci-hyperv.c | 11 -----------
>  1 file changed, 11 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
> index 2faf38e..ac67e56 100644
> --- a/drivers/pci/host/pci-hyperv.c
> +++ b/drivers/pci/host/pci-hyperv.c
> @@ -1518,17 +1518,6 @@ static struct hv_pci_dev *new_pcichild_device(struct hv_pcibus_device *hbus,
>  	get_pcichild(hpdev, hv_pcidev_ref_childlist);
>  	spin_lock_irqsave(&hbus->device_list_lock, flags);
>  
> -	/*
> -	 * When a device is being added to the bus, we set the PCI domain
> -	 * number to be the device serial number, which is non-zero and
> -	 * unique on the same VM.  The serial numbers start with 1, and
> -	 * increase by 1 for each device.  So device names including this
> -	 * can have shorter names than based on the bus instance UUID.
> -	 * Only the first device serial number is used for domain, so the
> -	 * domain number will not change after the first device is added.
> -	 */
> -	if (list_empty(&hbus->children))
> -		hbus->sysdata.domain = desc->ser;
>  	list_add_tail(&hpdev->list_entry, &hbus->children);
>  	spin_unlock_irqrestore(&hbus->device_list_lock, flags);
>  	return hpdev;
> -- 
> 2.7.4 
> 
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH v3]PCI: hv: fix PCI-BUS domainID corruption

[Sridhar Pitchai]

Hi Lorenzo,
Answering the question inline.
Kindly let me know if it clarifies. I will send out another patch after we agree on the clarification.

Thanks
Sridhar Pitchai

-----Original Message-----
From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> 
Sent: Thursday, March 15, 2018 5:05 AM
To: Sridhar Pitchai <Sridhar.Pitchai@microsoft.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>; Jake Oshins <jakeo@microsoft.com>; Haiyang Zhang <haiyangz@microsoft.com>; Stephen Hemminger <sthemmin@microsoft.com>; Dexuan Cui <decui@microsoft.com>; KY Srinivasan <kys@microsoft.com>; Michael Kelley (EOSG) <Michael.H.Kelley@microsoft.com>; devel@linuxdriverproject.org; linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3]PCI: hv: fix PCI-BUS domainID corruption

On Thu, Mar 15, 2018 at 12:03:07AM +0000, Sridhar Pitchai wrote:
> Whenever PCI bus is added, HyperV guarantees the BUS id is unique. Even

"Whenever a PCI bus is added"
Sridhar>> yes

> with that when a first device is added to the bus, it overrides bus domain
> ID with the device serial number. Sometime this can result in BUS ID not

Define "Sometime".

Sridhar>> HyperV when it creates a PCI bus it guarantees it provide a unique ID for it. But, that unique BUS ID is replaced with device serial number. 0 is a valid device serial number, and if there exists a PCI bus with domain ID 0 (Gen 1 version of hyperV VM have this for para virtual devices), this will result in PCI bus id not being unique. 

> being unique. In this case, when PCI_BUS and a device to bus is added, the
> first device overwrites the bus domain ID to the device serial number,
> which is 0. Since there exsist a PCI bus with domain ID 0 already the PCI

s/exsist/exist

Sridhar>> yes

> bus addition fails. This patch make sure when a device is added to a bus,
> it never updated the bus domain ID. 

s/updated/updates
Sridhar >> yes

> Since we have the transparent SRIOV mode now, the short VF device name
> is no longer needed.

I still do not understand what this means and how it is related to the
patch below, it may be clear to you, it is not to me, at all.

Sridhar >> the patch below, was introduced to make the device name small, by taking only 16bits of the serial number. Since we are not going to have the serial number updated to the BUS id, this has to be removed.

> Fixes: 4a9b0933bdfc("PCI:hv:Use device serial number as PCI domain")

Fixes: 4a9b0933bdfc ("PCI: hv: Use device serial number as PCI domain")
Sridhr >> yes

I asked you an explicit question. Commit above was added for a reason
I assume. This patch implies that kernel has been broken since v4.11
which is almost a year ago and nobody every noticed ? Or there are
systems where commit above is _necessary_ and this patch would break
them ?

I want a detailed explanation that highlights *why* it is safe to apply
this patch and send it to stable kernels, commit log above won't do.

Sridhar>> HyperV provides a unique domain ID for PCI BUS. But it is modified by the child device when it is added. This cannot produce a unique domain ID all the time. Here in the bug, we see the collision between the serial number and already existing PCI bus. The cleaner way is never touch the domain ID provided by hyperV during the PCI bus creation. As long as hyperV make sure it provides a unique domain ID for the PCI for a VM it will not break, and HyperV will guarantees that the domain for the PCI bus for a given VM will be always unique.
The original patch was also intending to have a unique domain ID for the PCI bus, by taking the serial number of the device, but it is not sufficient, when the device serial number is number which is the domain ID of the existing PCI bus.
With the current kernel we can repro this issue by adding a device with a serial number matching the existing PCI bus domain id. (in this case that happens to be zero).

Thanks,
Lorenzo

> Cc: stable@vger.kernel.org
> Signed-off-by: Sridhar Pitchai <srpitcha@microsoft.com>
> ---
> 
> Changes in v3:
> * fix the commit comment. [KY Srinivasan, Michael Kelley]
> ---
>  drivers/pci/host/pci-hyperv.c | 11 -----------
>  1 file changed, 11 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
> index 2faf38e..ac67e56 100644
> --- a/drivers/pci/host/pci-hyperv.c
> +++ b/drivers/pci/host/pci-hyperv.c
> @@ -1518,17 +1518,6 @@ static struct hv_pci_dev *new_pcichild_device(struct hv_pcibus_device *hbus,
>  	get_pcichild(hpdev, hv_pcidev_ref_childlist);
>  	spin_lock_irqsave(&hbus->device_list_lock, flags);
>  
> -	/*
> -	 * When a device is being added to the bus, we set the PCI domain
> -	 * number to be the device serial number, which is non-zero and
> -	 * unique on the same VM.  The serial numbers start with 1, and
> -	 * increase by 1 for each device.  So device names including this
> -	 * can have shorter names than based on the bus instance UUID.
> -	 * Only the first device serial number is used for domain, so the
> -	 * domain number will not change after the first device is added.
> -	 */
> -	if (list_empty(&hbus->children))
> -		hbus->sysdata.domain = desc->ser;
>  	list_add_tail(&hpdev->list_entry, &hbus->children);
>  	spin_unlock_irqrestore(&hbus->device_list_lock, flags);
>  	return hpdev;
> -- 
> 2.7.4 
> 
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3]PCI: hv: fix PCI-BUS domainID corruption

[Sridhar Pitchai]

Apologies for not aligning my reply, I just realized after looking into
the mail archive in LKML.  I have aligned the replay now.


On 3/15/18, 10:56 AM, "Sridhar Pitchai" <Sridhar.Pitchai@microsoft.com> wrote:

Hi Lorenzo,
Answering the question inline.
Kindly let me know if it clarifies. I will send out another patch after we agree on the clarification.

Thanks
Sridhar Pitchai

-----Original Message-----
From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Sent: Thursday, March 15, 2018 5:05 AM
To: Sridhar Pitchai <Sridhar.Pitchai@microsoft.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>; Jake Oshins <jakeo@microsoft.com>; Haiyang Zhang <haiyangz@microsoft.com>; Stephen Hemminger <sthemmin@microsoft.com>; Dexuan Cui <decui@microsoft.com>; KY Srinivasan <kys@microsoft.com>; Michael Kelley (EOSG) <Michael.H.Kelley@microsoft.com>; devel@linuxdriverproject.org; linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3]PCI: hv: fix PCI-BUS domainID corruption

On Thu, Mar 15, 2018 at 12:03:07AM +0000, Sridhar Pitchai wrote:
Whenever PCI bus is added, HyperV guarantees the BUS id is unique. Even

"Whenever a PCI bus is added"
Sridhar>> yes

with that when a first device is added to the bus, it overrides bus domain
ID with the device serial number. Sometime this can result in BUS ID not

Define "Sometime".

Sridhar>> HyperV when it creates a PCI bus it guarantees it provide a unique ID for it.
But, that unique BUS ID is replaced with device serial number. 0 is a valid
device serial number, and if there exists a PCI bus with domain ID 0 (Gen 1
version of hyperV VM have this for para virtual devices), this will result in
PCI bus id not being unique.


being unique. In this case, when PCI_BUS and a device to bus is added, the
first device overwrites the bus domain ID to the device serial number,
which is 0. Since there exsist a PCI bus with domain ID 0 already the PCI

s/exsist/exist

Sridhar>> yes

bus addition fails. This patch make sure when a device is added to a bus,
it never updated the bus domain ID.

s/updated/updates
Sridhar >> yes

Since we have the transparent SRIOV mode now, the short VF device name
is no longer needed.

I still do not understand what this means and how it is related to the
patch below, it may be clear to you, it is not to me, at all.

Sridhar >> the patch below, was introduced to make the device name small, by taking only
16bits of the serial number. Since we are not going to have the serial number
updated to the BUS id, this has to be removed.

Fixes: 4a9b0933bdfc("PCI:hv:Use device serial number as PCI domain")

Fixes: 4a9b0933bdfc ("PCI: hv: Use device serial number as PCI domain")
Sridhr >> yes

I asked you an explicit question. Commit above was added for a reason
I assume. This patch implies that kernel has been broken since v4.11
which is almost a year ago and nobody every noticed ? Or there are
systems where commit above is _necessary_ and this patch would break
them ?

I want a detailed explanation that highlights *why* it is safe to apply
this patch and send it to stable kernels, commit log above won't do.

Sridhar>> HyperV provides a unique domain ID for PCI BUS. But it is modified by the child
device when it is added. This cannot produce a unique domain ID all the time.
Here in the bug, we see the collision between the serial number and already
existing PCI bus. The cleaner way is never touch the domain ID provided by
hyperV during the PCI bus creation. As long as hyperV make sure it provides a
unique domain ID for the PCI for a VM it will not break, and HyperV will
guarantees that the domain for the PCI bus for a given VM will be always unique.
The original patch was also intending to have a unique domain ID for the PCI
bus, by taking the serial number of the device, but it is not sufficient, when
the device serial number is number which is the domain ID of the existing PCI
bus.  With the current kernel we can repro this issue by adding a device with a
serial number matching the existing PCI bus domain id. (in this case that
happens to be zero).


Thanks,
Lorenzo

Cc: stable@vger.kernel.org
Signed-off-by: Sridhar Pitchai <srpitcha@microsoft.com>
---
Changes in v3:
* fix the commit comment. [KY Srinivasan, Michael Kelley]
---
  drivers/pci/host/pci-hyperv.c | 11 -----------
  1 file changed, 11 deletions(-)
diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
index 2faf38e..ac67e56 100644
--- a/drivers/pci/host/pci-hyperv.c
+++ b/drivers/pci/host/pci-hyperv.c
@@ -1518,17 +1518,6 @@ static struct hv_pci_dev *new_pcichild_device(struct hv_pcibus_device *hbus,
  	get_pcichild(hpdev, hv_pcidev_ref_childlist);
  	spin_lock_irqsave(&hbus->device_list_lock, flags);
  
-	/*
-	 * When a device is being added to the bus, we set the PCI domain
-	 * number to be the device serial number, which is non-zero and
-	 * unique on the same VM.  The serial numbers start with 1, and
-	 * increase by 1 for each device.  So device names including this
-	 * can have shorter names than based on the bus instance UUID.
-	 * Only the first device serial number is used for domain, so the
-	 * domain number will not change after the first device is added.
-	 */
-	if (list_empty(&hbus->children))
-		hbus->sysdata.domain = desc->ser;
  	list_add_tail(&hpdev->list_entry, &hbus->children);
  	spin_unlock_irqrestore(&hbus->device_list_lock, flags);
  	return hpdev;
--
2.7.4 

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3]PCI: hv: fix PCI-BUS domainID corruption

[Sridhar Pitchai]

Hi Lorenzo,
Are we good with the explanation? Can I send the patch with the updated commit comments?

Thanks
Sridhar

On 3/15/18, 11:24 AM, "Sridhar Pitchai" <Sridhar.Pitchai@microsoft.com> wrote:

    Apologies for not aligning my reply, I just realized after looking into
    the mail archive in LKML.  I have aligned the replay now.
    
    
    On 3/15/18, 10:56 AM, "Sridhar Pitchai" <Sridhar.Pitchai@microsoft.com> wrote:
    
    Hi Lorenzo,
    Answering the question inline.
    Kindly let me know if it clarifies. I will send out another patch after we agree on the clarification.
    
    Thanks
    Sridhar Pitchai
    
    -----Original Message-----
    From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
    Sent: Thursday, March 15, 2018 5:05 AM
    To: Sridhar Pitchai <Sridhar.Pitchai@microsoft.com>
    Cc: Bjorn Helgaas <bhelgaas@google.com>; Jake Oshins <jakeo@microsoft.com>; Haiyang Zhang <haiyangz@microsoft.com>; Stephen Hemminger <sthemmin@microsoft.com>; Dexuan Cui <decui@microsoft.com>; KY Srinivasan <kys@microsoft.com>; Michael Kelley (EOSG) <Michael.H.Kelley@microsoft.com>; devel@linuxdriverproject.org; linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org
    Subject: Re: [PATCH v3]PCI: hv: fix PCI-BUS domainID corruption
    
    On Thu, Mar 15, 2018 at 12:03:07AM +0000, Sridhar Pitchai wrote:
    Whenever PCI bus is added, HyperV guarantees the BUS id is unique. Even
    
    "Whenever a PCI bus is added"
    Sridhar>> yes
    
    with that when a first device is added to the bus, it overrides bus domain
    ID with the device serial number. Sometime this can result in BUS ID not
    
    Define "Sometime".
    
    Sridhar>> HyperV when it creates a PCI bus it guarantees it provide a unique ID for it.
    But, that unique BUS ID is replaced with device serial number. 0 is a valid
    device serial number, and if there exists a PCI bus with domain ID 0 (Gen 1
    version of hyperV VM have this for para virtual devices), this will result in
    PCI bus id not being unique.
    
    
    being unique. In this case, when PCI_BUS and a device to bus is added, the
    first device overwrites the bus domain ID to the device serial number,
    which is 0. Since there exsist a PCI bus with domain ID 0 already the PCI
    
    s/exsist/exist
    
    Sridhar>> yes
    
    bus addition fails. This patch make sure when a device is added to a bus,
    it never updated the bus domain ID.
    
    s/updated/updates
    Sridhar >> yes
    
    Since we have the transparent SRIOV mode now, the short VF device name
    is no longer needed.
    
    I still do not understand what this means and how it is related to the
    patch below, it may be clear to you, it is not to me, at all.
    
    Sridhar >> the patch below, was introduced to make the device name small, by taking only
    16bits of the serial number. Since we are not going to have the serial number
    updated to the BUS id, this has to be removed.
    
    Fixes: 4a9b0933bdfc("PCI:hv:Use device serial number as PCI domain")
    
    Fixes: 4a9b0933bdfc ("PCI: hv: Use device serial number as PCI domain")
    Sridhr >> yes
    
    I asked you an explicit question. Commit above was added for a reason
    I assume. This patch implies that kernel has been broken since v4.11
    which is almost a year ago and nobody every noticed ? Or there are
    systems where commit above is _necessary_ and this patch would break
    them ?
    
    I want a detailed explanation that highlights *why* it is safe to apply
    this patch and send it to stable kernels, commit log above won't do.
    
    Sridhar>> HyperV provides a unique domain ID for PCI BUS. But it is modified by the child
    device when it is added. This cannot produce a unique domain ID all the time.
    Here in the bug, we see the collision between the serial number and already
    existing PCI bus. The cleaner way is never touch the domain ID provided by
    hyperV during the PCI bus creation. As long as hyperV make sure it provides a
    unique domain ID for the PCI for a VM it will not break, and HyperV will
    guarantees that the domain for the PCI bus for a given VM will be always unique.
    The original patch was also intending to have a unique domain ID for the PCI
    bus, by taking the serial number of the device, but it is not sufficient, when
    the device serial number is number which is the domain ID of the existing PCI
    bus.  With the current kernel we can repro this issue by adding a device with a
    serial number matching the existing PCI bus domain id. (in this case that
    happens to be zero).
    
    
    Thanks,
    Lorenzo
    
    Cc: stable@vger.kernel.org
    Signed-off-by: Sridhar Pitchai <srpitcha@microsoft.com>
    ---
    Changes in v3:
    * fix the commit comment. [KY Srinivasan, Michael Kelley]
    ---
      drivers/pci/host/pci-hyperv.c | 11 -----------
      1 file changed, 11 deletions(-)
    diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
    index 2faf38e..ac67e56 100644
    --- a/drivers/pci/host/pci-hyperv.c
    +++ b/drivers/pci/host/pci-hyperv.c
    @@ -1518,17 +1518,6 @@ static struct hv_pci_dev *new_pcichild_device(struct hv_pcibus_device *hbus,
      	get_pcichild(hpdev, hv_pcidev_ref_childlist);
      	spin_lock_irqsave(&hbus->device_list_lock, flags);
      
    -	/*
    -	 * When a device is being added to the bus, we set the PCI domain
    -	 * number to be the device serial number, which is non-zero and
    -	 * unique on the same VM.  The serial numbers start with 1, and
    -	 * increase by 1 for each device.  So device names including this
    -	 * can have shorter names than based on the bus instance UUID.
    -	 * Only the first device serial number is used for domain, so the
    -	 * domain number will not change after the first device is added.
    -	 */
    -	if (list_empty(&hbus->children))
    -		hbus->sysdata.domain = desc->ser;
      	list_add_tail(&hpdev->list_entry, &hbus->children);
      	spin_unlock_irqrestore(&hbus->device_list_lock, flags);
      	return hpdev;
    --
    2.7.4 
    
    

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3]PCI: hv: fix PCI-BUS domainID corruption

[Lorenzo Pieralisi]

On Tue, Mar 20, 2018 at 05:56:15PM +0000, Sridhar Pitchai wrote:
> Hi Lorenzo,

> Are we good with the explanation? Can I send the patch with the
> updated commit comments?

Almost.

[...]

>     Since we have the transparent SRIOV mode now, the short VF device name
>     is no longer needed.

Can you correlate transparent SRIOV mode to the point you are making
below ? Please explain what transparent SRIOV mode allows you to remove
and why. The rest of the explanation seems OK.

Please follow this email format:

http://vger.kernel.org/lkml/#s3-9

Thanks,
Lorenzo

>     I still do not understand what this means and how it is related to the
>     patch below, it may be clear to you, it is not to me, at all.
>     
>     Sridhar >> the patch below, was introduced to make the device name small, by taking only
>     16bits of the serial number. Since we are not going to have the serial number
>     updated to the BUS id, this has to be removed.
>     
>     Fixes: 4a9b0933bdfc("PCI:hv:Use device serial number as PCI domain")
>     
>     Fixes: 4a9b0933bdfc ("PCI: hv: Use device serial number as PCI domain")
>     Sridhr >> yes
>     
>     I asked you an explicit question. Commit above was added for a reason
>     I assume. This patch implies that kernel has been broken since v4.11
>     which is almost a year ago and nobody every noticed ? Or there are
>     systems where commit above is _necessary_ and this patch would break
>     them ?
>     
>     I want a detailed explanation that highlights *why* it is safe to apply
>     this patch and send it to stable kernels, commit log above won't do.
>     
>     Sridhar>> HyperV provides a unique domain ID for PCI BUS. But it is modified by the child
>     device when it is added. This cannot produce a unique domain ID all the time.
>     Here in the bug, we see the collision between the serial number and already
>     existing PCI bus. The cleaner way is never touch the domain ID provided by
>     hyperV during the PCI bus creation. As long as hyperV make sure it provides a
>     unique domain ID for the PCI for a VM it will not break, and HyperV will
>     guarantees that the domain for the PCI bus for a given VM will be always unique.
>     The original patch was also intending to have a unique domain ID for the PCI
>     bus, by taking the serial number of the device, but it is not sufficient, when
>     the device serial number is number which is the domain ID of the existing PCI
>     bus.  With the current kernel we can repro this issue by adding a device with a
>     serial number matching the existing PCI bus domain id. (in this case that
>     happens to be zero).
>     
>     
>     Thanks,
>     Lorenzo
>     
>     Cc: stable@vger.kernel.org
>     Signed-off-by: Sridhar Pitchai <srpitcha@microsoft.com>
>     ---
>     Changes in v3:
>     * fix the commit comment. [KY Srinivasan, Michael Kelley]
>     ---
>       drivers/pci/host/pci-hyperv.c | 11 -----------
>       1 file changed, 11 deletions(-)
>     diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
>     index 2faf38e..ac67e56 100644
>     --- a/drivers/pci/host/pci-hyperv.c
>     +++ b/drivers/pci/host/pci-hyperv.c
>     @@ -1518,17 +1518,6 @@ static struct hv_pci_dev *new_pcichild_device(struct hv_pcibus_device *hbus,
>       	get_pcichild(hpdev, hv_pcidev_ref_childlist);
>       	spin_lock_irqsave(&hbus->device_list_lock, flags);
>       
>     -	/*
>     -	 * When a device is being added to the bus, we set the PCI domain
>     -	 * number to be the device serial number, which is non-zero and
>     -	 * unique on the same VM.  The serial numbers start with 1, and
>     -	 * increase by 1 for each device.  So device names including this
>     -	 * can have shorter names than based on the bus instance UUID.
>     -	 * Only the first device serial number is used for domain, so the
>     -	 * domain number will not change after the first device is added.
>     -	 */
>     -	if (list_empty(&hbus->children))
>     -		hbus->sysdata.domain = desc->ser;
>       	list_add_tail(&hpdev->list_entry, &hbus->children);
>       	spin_unlock_irqrestore(&hbus->device_list_lock, flags);
>       	return hpdev;
>     --
>     2.7.4 
>     
>     
> 
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3]PCI: hv: fix PCI-BUS domainID corruption

[Sridhar Pitchai]

Hi Lorenzo,
   Transparent SRIOV is exposing the NIC directly to the kernel via
para-virtual device, unlike creating a netdev and associating it with the bond
driver. Further descriptions here,
    https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=0c195567a8f6e82ea5535cd9f1d54a1626dd233e

Previously, when using the bond driver, unique and persistent VF NIC name
was required, so we used serial number as PCI domain which is included as
part of the VF NIC name.  Transparent SRIOV mode puts VF NIC based on MAC
match as a slave of synthetic NIC, so VF NIC’s name is no longer important.

Thanks,
Sridhar

On 3/20/18, 11:32 AM, "Lorenzo Pieralisi" <lorenzo.pieralisi@arm.com> wrote:

    On Tue, Mar 20, 2018 at 05:56:15PM +0000, Sridhar Pitchai wrote:
    > Hi Lorenzo,
    
    > Are we good with the explanation? Can I send the patch with the
    > updated commit comments?
    
    Almost.
    
    [...]
    
    >     Since we have the transparent SRIOV mode now, the short VF device name
    >     is no longer needed.
    
    Can you correlate transparent SRIOV mode to the point you are making
    below ? Please explain what transparent SRIOV mode allows you to remove
    and why. The rest of the explanation seems OK.
    
    Please follow this email format:
    
    https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fvger.kernel.org%2Flkml%2F%23s3-9&data=04%7C01%7CSridhar.Pitchai%40microsoft.com%7Cc5cdcb7951f64318e52708d58e90e6f2%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636571675366181738%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwifQ%3D%3D%7C-1&sdata=yBdqc4NQZsO7O9vfgJsr5olU8GfLNjF5e9EAaCb7vq4%3D&reserved=0
    
    Thanks,
    Lorenzo
    
    >     I still do not understand what this means and how it is related to the
    >     patch below, it may be clear to you, it is not to me, at all.
    >     
    >     Sridhar >> the patch below, was introduced to make the device name small, by taking only
    >     16bits of the serial number. Since we are not going to have the serial number
    >     updated to the BUS id, this has to be removed.
    >     
    >     Fixes: 4a9b0933bdfc("PCI:hv:Use device serial number as PCI domain")
    >     
    >     Fixes: 4a9b0933bdfc ("PCI: hv: Use device serial number as PCI domain")
    >     Sridhr >> yes
    >     
    >     I asked you an explicit question. Commit above was added for a reason
    >     I assume. This patch implies that kernel has been broken since v4.11
    >     which is almost a year ago and nobody every noticed ? Or there are
    >     systems where commit above is _necessary_ and this patch would break
    >     them ?
    >     
    >     I want a detailed explanation that highlights *why* it is safe to apply
    >     this patch and send it to stable kernels, commit log above won't do.
    >     
    >     Sridhar>> HyperV provides a unique domain ID for PCI BUS. But it is modified by the child
    >     device when it is added. This cannot produce a unique domain ID all the time.
    >     Here in the bug, we see the collision between the serial number and already
    >     existing PCI bus. The cleaner way is never touch the domain ID provided by
    >     hyperV during the PCI bus creation. As long as hyperV make sure it provides a
    >     unique domain ID for the PCI for a VM it will not break, and HyperV will
    >     guarantees that the domain for the PCI bus for a given VM will be always unique.
    >     The original patch was also intending to have a unique domain ID for the PCI
    >     bus, by taking the serial number of the device, but it is not sufficient, when
    >     the device serial number is number which is the domain ID of the existing PCI
    >     bus.  With the current kernel we can repro this issue by adding a device with a
    >     serial number matching the existing PCI bus domain id. (in this case that
    >     happens to be zero).
    >     
    >     
    >     Thanks,
    >     Lorenzo
    >     
    >     Cc: stable@vger.kernel.org
    >     Signed-off-by: Sridhar Pitchai <srpitcha@microsoft.com>
    >     ---
    >     Changes in v3:
    >     * fix the commit comment. [KY Srinivasan, Michael Kelley]
    >     ---
    >       drivers/pci/host/pci-hyperv.c | 11 -----------
    >       1 file changed, 11 deletions(-)
    >     diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
    >     index 2faf38e..ac67e56 100644
    >     --- a/drivers/pci/host/pci-hyperv.c
    >     +++ b/drivers/pci/host/pci-hyperv.c
    >     @@ -1518,17 +1518,6 @@ static struct hv_pci_dev *new_pcichild_device(struct hv_pcibus_device *hbus,
    >       	get_pcichild(hpdev, hv_pcidev_ref_childlist);
    >       	spin_lock_irqsave(&hbus->device_list_lock, flags);
    >       
    >     -	/*
    >     -	 * When a device is being added to the bus, we set the PCI domain
    >     -	 * number to be the device serial number, which is non-zero and
    >     -	 * unique on the same VM.  The serial numbers start with 1, and
    >     -	 * increase by 1 for each device.  So device names including this
    >     -	 * can have shorter names than based on the bus instance UUID.
    >     -	 * Only the first device serial number is used for domain, so the
    >     -	 * domain number will not change after the first device is added.
    >     -	 */
    >     -	if (list_empty(&hbus->children))
    >     -		hbus->sysdata.domain = desc->ser;
    >       	list_add_tail(&hpdev->list_entry, &hbus->children);
    >       	spin_unlock_irqrestore(&hbus->device_list_lock, flags);
    >       	return hpdev;
    >     --
    >     2.7.4 
    >     
    >     
    > 
    

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3]PCI: hv: fix PCI-BUS domainID corruption

[Lorenzo Pieralisi]

On Tue, Mar 20, 2018 at 11:00:36PM +0000, Sridhar Pitchai wrote:
> Hi Lorenzo,
>    Transparent SRIOV is exposing the NIC directly to the kernel via
>    para-virtual device, unlike creating a netdev and associating it
>    with the bond driver. Further descriptions here,
>    https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=0c195567a8f6e82ea5535cd9f1d54a1626dd233e
> 
> Previously, when using the bond driver, unique and persistent VF NIC
> name was required, so we used serial number as PCI domain which is
> included as part of the VF NIC name.  Transparent SRIOV mode puts VF
> NIC based on MAC match as a slave of synthetic NIC, so VF NIC’s name
> is no longer important.

Please read the link I sent you in relation to email formatting.

Then add your description above in a way that anyone not 100% familiar
with hyperv can understand it - that's what the commit log is for.

You are sending this patch to stable kernels, patch above has been in
the kernel from v4.14. The patch you are fixing since v4.11, you ought
to be careful since you do not want to have broken kernel versions owing
to stable patches mismatches, that's why I asked and I will ask again,
are you sure you won't trigger a regression by sending this fix to
stable ?

I assume the bond driver mechanism is now done and dusted.

Thanks,
Lorenzo

> Thanks,
> Sridhar
> 
> On 3/20/18, 11:32 AM, "Lorenzo Pieralisi" <lorenzo.pieralisi@arm.com> wrote:
> 
>     On Tue, Mar 20, 2018 at 05:56:15PM +0000, Sridhar Pitchai wrote:
>     > Hi Lorenzo,
>     
>     > Are we good with the explanation? Can I send the patch with the
>     > updated commit comments?
>     
>     Almost.
>     
>     [...]
>     
>     >     Since we have the transparent SRIOV mode now, the short VF device name
>     >     is no longer needed.
>     
>     Can you correlate transparent SRIOV mode to the point you are making
>     below ? Please explain what transparent SRIOV mode allows you to remove
>     and why. The rest of the explanation seems OK.
>     
>     Please follow this email format:
>     
>     https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fvger.kernel.org%2Flkml%2F%23s3-9&data=04%7C01%7CSridhar.Pitchai%40microsoft.com%7Cc5cdcb7951f64318e52708d58e90e6f2%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636571675366181738%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwifQ%3D%3D%7C-1&sdata=yBdqc4NQZsO7O9vfgJsr5olU8GfLNjF5e9EAaCb7vq4%3D&reserved=0
>     
>     Thanks,
>     Lorenzo
>     
>     >     I still do not understand what this means and how it is related to the
>     >     patch below, it may be clear to you, it is not to me, at all.
>     >     
>     >     Sridhar >> the patch below, was introduced to make the device name small, by taking only
>     >     16bits of the serial number. Since we are not going to have the serial number
>     >     updated to the BUS id, this has to be removed.
>     >     
>     >     Fixes: 4a9b0933bdfc("PCI:hv:Use device serial number as PCI domain")
>     >     
>     >     Fixes: 4a9b0933bdfc ("PCI: hv: Use device serial number as PCI domain")
>     >     Sridhr >> yes
>     >     
>     >     I asked you an explicit question. Commit above was added for a reason
>     >     I assume. This patch implies that kernel has been broken since v4.11
>     >     which is almost a year ago and nobody every noticed ? Or there are
>     >     systems where commit above is _necessary_ and this patch would break
>     >     them ?
>     >     
>     >     I want a detailed explanation that highlights *why* it is safe to apply
>     >     this patch and send it to stable kernels, commit log above won't do.
>     >     
>     >     Sridhar>> HyperV provides a unique domain ID for PCI BUS. But it is modified by the child
>     >     device when it is added. This cannot produce a unique domain ID all the time.
>     >     Here in the bug, we see the collision between the serial number and already
>     >     existing PCI bus. The cleaner way is never touch the domain ID provided by
>     >     hyperV during the PCI bus creation. As long as hyperV make sure it provides a
>     >     unique domain ID for the PCI for a VM it will not break, and HyperV will
>     >     guarantees that the domain for the PCI bus for a given VM will be always unique.
>     >     The original patch was also intending to have a unique domain ID for the PCI
>     >     bus, by taking the serial number of the device, but it is not sufficient, when
>     >     the device serial number is number which is the domain ID of the existing PCI
>     >     bus.  With the current kernel we can repro this issue by adding a device with a
>     >     serial number matching the existing PCI bus domain id. (in this case that
>     >     happens to be zero).
>     >     
>     >     
>     >     Thanks,
>     >     Lorenzo
>     >     
>     >     Cc: stable@vger.kernel.org
>     >     Signed-off-by: Sridhar Pitchai <srpitcha@microsoft.com>
>     >     ---
>     >     Changes in v3:
>     >     * fix the commit comment. [KY Srinivasan, Michael Kelley]
>     >     ---
>     >       drivers/pci/host/pci-hyperv.c | 11 -----------
>     >       1 file changed, 11 deletions(-)
>     >     diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
>     >     index 2faf38e..ac67e56 100644
>     >     --- a/drivers/pci/host/pci-hyperv.c
>     >     +++ b/drivers/pci/host/pci-hyperv.c
>     >     @@ -1518,17 +1518,6 @@ static struct hv_pci_dev *new_pcichild_device(struct hv_pcibus_device *hbus,
>     >       	get_pcichild(hpdev, hv_pcidev_ref_childlist);
>     >       	spin_lock_irqsave(&hbus->device_list_lock, flags);
>     >       
>     >     -	/*
>     >     -	 * When a device is being added to the bus, we set the PCI domain
>     >     -	 * number to be the device serial number, which is non-zero and
>     >     -	 * unique on the same VM.  The serial numbers start with 1, and
>     >     -	 * increase by 1 for each device.  So device names including this
>     >     -	 * can have shorter names than based on the bus instance UUID.
>     >     -	 * Only the first device serial number is used for domain, so the
>     >     -	 * domain number will not change after the first device is added.
>     >     -	 */
>     >     -	if (list_empty(&hbus->children))
>     >     -		hbus->sysdata.domain = desc->ser;
>     >       	list_add_tail(&hpdev->list_entry, &hbus->children);
>     >       	spin_unlock_irqrestore(&hbus->device_list_lock, flags);
>     >       	return hpdev;
>     >     --
>     >     2.7.4 
>     >     
>     >     
>     > 
>     
> 
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3]PCI: hv: fix PCI-BUS domainID corruption

[Sridhar Pitchai]

    Please read the link I sent you in relation to email formatting.
    
    Then add your description above in a way that anyone not 100% familiar
    with hyperv can understand it - that's what the commit log is for.
    
    You are sending this patch to stable kernels, patch above has been in
    the kernel from v4.14. The patch you are fixing since v4.11, you ought
    to be careful since you do not want to have broken kernel versions owing
    to stable patches mismatches, that's why I asked and I will ask again,
    are you sure you won't trigger a regression by sending this fix to
    stable ?
    
    I assume the bond driver mechanism is now done and dusted.
    
That is correct. I have sent a v4 version of the patch. I am sending this
patch for stable kernel. We have tested and I am sure this should not trigger
regression by sending this fix to stable.

Thanks
Sridhar Pitchai     
 

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3]PCI: hv: fix PCI-BUS domainID corruption

[Greg KH]

On Fri, Mar 23, 2018 at 04:41:02PM +0000, Sridhar Pitchai wrote:
>     Please read the link I sent you in relation to email formatting.
>     
>     Then add your description above in a way that anyone not 100% familiar
>     with hyperv can understand it - that's what the commit log is for.
>     
>     You are sending this patch to stable kernels, patch above has been in
>     the kernel from v4.14. The patch you are fixing since v4.11, you ought
>     to be careful since you do not want to have broken kernel versions owing
>     to stable patches mismatches, that's why I asked and I will ask again,
>     are you sure you won't trigger a regression by sending this fix to
>     stable ?
>     
>     I assume the bond driver mechanism is now done and dusted.
>     
> That is correct. I have sent a v4 version of the patch. I am sending this
> patch for stable kernel. We have tested and I am sure this should not trigger
> regression by sending this fix to stable.


<formletter>

This is not the correct way to submit patches for inclusion in the
stable kernel tree.  Please read:
    https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.

</formletter>
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3]PCI: hv: fix PCI-BUS domainID corruption

[Sridhar Pitchai]

  
    > This is not the correct way to submit patches for inclusion in the
    > stable kernel tree.  Please read:
    >  <url>
    > for how to do this properly.

My bad. I am sending the patch to the mainline kernel with a Cc: tag for stable
and hoping the patch will be automatically merged to the stable kernels.

Thanks,
Sridhar Pitchai

Re: [PATCH v3]PCI: hv: fix PCI-BUS domainID corruption

[Bjorn Helgaas]

[I composed most of this before seeing Lorenzo's response, so sorry
about the duplication.  Maybe seeing things stated a different way
will help :)]

On Tue, Mar 20, 2018 at 11:00:36PM +0000, Sridhar Pitchai wrote:
> Hi Lorenzo,
>    Transparent SRIOV is exposing the NIC directly to the kernel via
> para-virtual device, unlike creating a netdev and associating it with the bond
> driver. Further descriptions here,
>     https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=0c195567a8f6e82ea5535cd9f1d54a1626dd233e
> 
> Previously, when using the bond driver, unique and persistent VF NIC name
> was required, so we used serial number as PCI domain which is included as
> part of the VF NIC name.  Transparent SRIOV mode puts VF NIC based on MAC
> match as a slave of synthetic NIC, so VF NIC’s name is no longer important.

Hi Sridhar,

A few hints about submitting patches more efficiently:

1) You never have to ask "Are we OK with the explanation?  If so, I'll
send a patch with updated changelog."  That forces an extra
round-trip.  Simply post a new version with your proposed update.  If
Lorenzo has more questions, he'll say so and you can do another
version.

2) When Lorenzo is asking for clarification, he's not really asking
for the clarification in an email response, because the email thread
will soon be forgotten and lost in the archives.  What we really want
is for the permanent git changelog to make sense to someone in the
future.  The easiest way is to post a new patch version with a
revised changelog that answers the questions.

3) Please capitalize and punctuate consistently with previous history,
e.g., "PCI: hv: Fix domain ID corruption" for your title, "SR-IOV"
(not "SRIOV") and "bus" (not "BUS") in changelog.  Both "para-virtual"
and "paravirtual" are used in the kernel, but "paravirtual" is much
more common.  Run "git log" and "git log --oneline" on your file and
follow the same style.

4) When you reference a previous commit, please use this style:
0c195567a8f6 ("netvsc: transparent VF management"), i.e., 12-char SHA1
followed by title.  You seem to have removed some spaces from the
commit you mention in the "Fixes" tag.

And a few content questions/observations of my own:

1) "Fix domain ID corruption" isn't a very good title because it
suggests you're fixing a memory corruption or similar defect.  But in
fact, I think you're removing something that used to be a feature
(added by 4a9b0933bdfc ("PCI: hv: Use device serial number as PCI
domain")) but is now no longer needed and in fact now causes a
problem.

2) Your changelog does mention 4a9b0933bdfc, which is good, but
there must be some other <commit X> that makes it safe to remove
4a9b0933bdfc, i.e., <commit X> removes the need for using the device
serial number as the PCI domain.  <Commit X> *must* be mentioned in
the changelog.  Otherwise, people may backport this patch to a kernel
that doesn't include <commit X>, and things will break.

3) I don't understand what you mean by "transparent SR-IOV mode".  Is
that something different than regular SR-IOV?  If so, what exactly is
the difference?  I don't think the PCIe specs mention a "transparent
mode", so is it a Hyper-V thing?  It seems important, but I don't see
any pci-hyperv.c commits that mention it.

Here's a stab at the sort of changelog I would be looking for.
Obviously I don't understand much about Hyper-V and pci-hyperv.c, so
please correct the things I got wrong:

  When Linux runs as a guest in a Hyper-V VM, pci-hyperv.c
  paravirtualizes access to PCI devices assigned to the guest.  For
  each of those devices, hv_pci_probe() creates a virtual PCI bus in
  its own unique PCI domain.

  4a9b0933bdfc ("PCI: hv: Use device serial number as PCI domain")
  overrode that unique PCI domain to be the Hyper-V device serial
  number to make device names more convenient <or whatever the real
  reason is; I don't quite understand this part>.

  One problem with 4a9b0933bdfc is that the Hyper-V device serial
  number is not necessarily unique, so we may end up with two buses
  with the same domain and bus number, and adding the second bus
  fails.

  We no longer need to override the PCI domain numbers because <commit
  X> removed the need for that.

  Revert 4a9b0933bdfc ("PCI: hv: Use device serial number as PCI
  domain") so we can reliably support multiple devices being assigned
  to a guest.

  This revert should only be backported to kernels that contain
  <commit X>.

Bjorn
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3]PCI: hv: fix PCI-BUS domainID corruption

[Sridhar Pitchai]

    > Previously, when using the bond driver, unique and persistent VF NIC name
    > was required, so we used serial number as PCI domain which is included as
    > part of the VF NIC name.  Transparent SRIOV mode puts VF NIC based on MAC
    > match as a slave of synthetic NIC, so VF NIC’s name is no longer important.
    
    Hi Sridhar,
    
    A few hints about submitting patches more efficiently:
    
    1) You never have to ask "Are we OK with the explanation?  If so, I'll
    send a patch with updated changelog."  That forces an extra
    round-trip.  Simply post a new version with your proposed update.  If
    Lorenzo has more questions, he'll say so and you can do another
    version.
    
    2) When Lorenzo is asking for clarification, he's not really asking
    for the clarification in an email response, because the email thread
    will soon be forgotten and lost in the archives.  What we really want
    is for the permanent git changelog to make sense to someone in the
    future.  The easiest way is to post a new patch version with a
    revised changelog that answers the questions.
    
    3) Please capitalize and punctuate consistently with previous history,
    e.g., "PCI: hv: Fix domain ID corruption" for your title, "SR-IOV"
    (not "SRIOV") and "bus" (not "BUS") in changelog.  Both "para-virtual"
    and "paravirtual" are used in the kernel, but "paravirtual" is much
    more common.  Run "git log" and "git log --oneline" on your file and
    follow the same style.
    
    4) When you reference a previous commit, please use this style:
    0c195567a8f6 ("netvsc: transparent VF management"), i.e., 12-char SHA1
    followed by title.  You seem to have removed some spaces from the
    commit you mention in the "Fixes" tag.
    
    And a few content questions/observations of my own:
    
    1) "Fix domain ID corruption" isn't a very good title because it
    suggests you're fixing a memory corruption or similar defect.  But in
    fact, I think you're removing something that used to be a feature
    (added by 4a9b0933bdfc ("PCI: hv: Use device serial number as PCI
    domain")) but is now no longer needed and in fact now causes a
    problem.
    
    2) Your changelog does mention 4a9b0933bdfc, which is good, but
    there must be some other <commit X> that makes it safe to remove
    4a9b0933bdfc, i.e., <commit X> removes the need for using the device
    serial number as the PCI domain.  <Commit X> *must* be mentioned in
    the changelog.  Otherwise, people may backport this patch to a kernel
    that doesn't include <commit X>, and things will break.
    
    3) I don't understand what you mean by "transparent SR-IOV mode".  Is
    that something different than regular SR-IOV?  If so, what exactly is
    the difference?  I don't think the PCIe specs mention a "transparent
    mode", so is it a Hyper-V thing?  It seems important, but I don't see
    any pci-hyperv.c commits that mention it.
    
    Here's a stab at the sort of changelog I would be looking for.
    Obviously I don't understand much about Hyper-V and pci-hyperv.c, so
    please correct the things I got wrong:
    
      When Linux runs as a guest in a Hyper-V VM, pci-hyperv.c
      paravirtualizes access to PCI devices assigned to the guest.  For
      each of those devices, hv_pci_probe() creates a virtual PCI bus in
      its own unique PCI domain.
    
      4a9b0933bdfc ("PCI: hv: Use device serial number as PCI domain")
      overrode that unique PCI domain to be the Hyper-V device serial
      number to make device names more convenient <or whatever the real
      reason is; I don't quite understand this part>.
    
      One problem with 4a9b0933bdfc is that the Hyper-V device serial
      number is not necessarily unique, so we may end up with two buses
      with the same domain and bus number, and adding the second bus
      fails.
    
      We no longer need to override the PCI domain numbers because <commit
      X> removed the need for that.
    
      Revert 4a9b0933bdfc ("PCI: hv: Use device serial number as PCI
      domain") so we can reliably support multiple devices being assigned
      to a guest.
    
      This revert should only be backported to kernels that contain
      <commit X>.
    
    Bjorn

Thanks for your comments Bjorn. I took some time to go over the all the
comments and sending a new version of the patch with all the comments 
incorporated.

Thanks
Sridhar    

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel