Re: [PATCH] PCI BUS domain ID corruption

Re: [PATCH] PCI BUS domain ID corruption

[Bjorn Helgaas]

[+cc Lorenzo]

On Thu, Mar 8, 2018 at 12:55 PM, Sridhar Pitchai
<Sridhar.Pitchai@microsoft.com> wrote:
> Hi,
>
> I would like to submit the following patch. This patch addresses the issue
> when we try to add a VMBUS, the domain ID for the PCI bus is overwritten.

Hi Sridhar,

Thanks for the patch!

This area is maintained by Lorenzo now (cc'd), so please copy him on
the next round.

We can't apply it as-is because it contains no Signed-off-by (see
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst).

This email is also too fancy (HTML, etc) and I think will be rejected
by the mailing lists (see
http://vger.kernel.org/majordomo-info.html#taboo).  What you want is a
plain-text email.

Please also run "git log drivers/pci/host/pci-hyperv.c" and make your
subject and changelog match the format and style of previous patches.

It looks like this basically reverts 4a9b0933bdfc ("PCI: hv: Use
device serial number as PCI domain"), so you should explain what was
wrong with that commit.

Also, it doesn't look like this would apply cleanly to v4.16-rc1.
Your patch shows this:

 -               hbus->sysdata.domain = desc->ser & 0xFFFF;

but v4.16-rc1 has this:

                hbus->sysdata.domain = desc->ser;

Bjorn

> srpitcha@ linux srpitcha/patch >cat 0001-PCI-BUS-domain-ID-curruption.patch
>
> From a0c407f3e2d57c84ac349f064dcee1d2961e5ca3 Mon Sep 17 00:00:00 2001
>
> From: Sridhar Pitchai <sridhar.pitchai@gmail.com>
>
> Date: Thu, 8 Mar 2018 12:33:50 -0800
>
> Subject: [PATCH] PCI BUS domain ID corruption
>
>
>
>     When PCI BUS is added, PCI_BUS domain ID is set. When PCI_BUS and device
> to
>
>     the bus is racing against each other, the first device tends to
> overwrite
>
>     the domain ID. In order to avoid the race, this patch make sure when a
>
>     device is added to a bus it never updates the bus domain ID. Since we
> have
>
>     the transparent SRIOV mode now, the short VF device name is no longer
>
>     needed.
>
>
>
> diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
>
> index 1713bfc..6d43f81 100644
>
> --- a/drivers/pci/host/pci-hyperv.c
>
> +++ b/drivers/pci/host/pci-hyperv.c
>
> @@ -1321,19 +1321,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.
>
> -        * The lower 16 bits of the serial number is used, otherwise some
>
> -        * drivers may not be able to handle it.
>
> -        */
>
> -       if (list_empty(&hbus->children))
>
> -               hbus->sysdata.domain = desc->ser & 0xFFFF;
>
>         list_add_tail(&hpdev->list_entry, &hbus->children);
>
>         spin_unlock_irqrestore(&hbus->device_list_lock, flags);
>
>         return hpdev;
>
> --
>
> 2.7.4
>
>
>
> srpitcha@ linux srpitcha/patch >
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH] PCI BUS domain ID corruption

[Sridhar Pitchai]

Thanks Bjorn,
I will send another mail, with 
1: the patch applied to 4.16. 
2: I will send a text mail (instead of HTML). 
3: add Signed-off-by:

Thanks for your help.

Thanks,
Sridhar Pitchai

-----Original Message-----
From: Bjorn Helgaas <bhelgaas@google.com> 
Sent: Thursday, March 8, 2018 1:40 PM
To: Sridhar Pitchai <Sridhar.Pitchai@microsoft.com>
Cc: 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>; David Davis <David.Davis@microsoft.com>; devel@linuxdriverproject.org; linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org; Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Subject: Re: [PATCH] PCI BUS domain ID corruption

[+cc Lorenzo]

On Thu, Mar 8, 2018 at 12:55 PM, Sridhar Pitchai <Sridhar.Pitchai@microsoft.com> wrote:
> Hi,
>
> I would like to submit the following patch. This patch addresses the 
> issue when we try to add a VMBUS, the domain ID for the PCI bus is overwritten.

Hi Sridhar,

Thanks for the patch!

This area is maintained by Lorenzo now (cc'd), so please copy him on the next round.

We can't apply it as-is because it contains no Signed-off-by (see https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2Ftree%2FDocumentation%2Fprocess%2Fsubmitting-patches.rst&data=04%7C01%7CSridhar.Pitchai%40microsoft.com%7C99c90ec96a544d9dedd808d5853d33ff%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636561420271631010%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwifQ%3D%3D%7C-2&sdata=1zcgL6ZK7FwFka3x%2FAwJJnLMOQG5kQD0eQ1UguSjiCA%3D&reserved=0).

This email is also too fancy (HTML, etc) and I think will be rejected by the mailing lists (see https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fvger.kernel.org%2Fmajordomo-info.html%23taboo&data=04%7C01%7CSridhar.Pitchai%40microsoft.com%7C99c90ec96a544d9dedd808d5853d33ff%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636561420271631010%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwifQ%3D%3D%7C-2&sdata=icGYJhzoRJ%2Bw4xZPGrtrvDY%2BcNDys1HDllV4Agz11ro%3D&reserved=0).  What you want is a plain-text email.

Please also run "git log drivers/pci/host/pci-hyperv.c" and make your subject and changelog match the format and style of previous patches.

It looks like this basically reverts 4a9b0933bdfc ("PCI: hv: Use device serial number as PCI domain"), so you should explain what was wrong with that commit.

Also, it doesn't look like this would apply cleanly to v4.16-rc1.
Your patch shows this:

 -               hbus->sysdata.domain = desc->ser & 0xFFFF;

but v4.16-rc1 has this:

                hbus->sysdata.domain = desc->ser;

Bjorn

> srpitcha@ linux srpitcha/patch >cat 
> 0001-PCI-BUS-domain-ID-curruption.patch
>
> From a0c407f3e2d57c84ac349f064dcee1d2961e5ca3 Mon Sep 17 00:00:00 2001
>
> From: Sridhar Pitchai <sridhar.pitchai@gmail.com>
>
> Date: Thu, 8 Mar 2018 12:33:50 -0800
>
> Subject: [PATCH] PCI BUS domain ID corruption
>
>
>
>     When PCI BUS is added, PCI_BUS domain ID is set. When PCI_BUS and 
> device to
>
>     the bus is racing against each other, the first device tends to 
> overwrite
>
>     the domain ID. In order to avoid the race, this patch make sure 
> when a
>
>     device is added to a bus it never updates the bus domain ID. Since 
> we have
>
>     the transparent SRIOV mode now, the short VF device name is no 
> longer
>
>     needed.
>
>
>
> diff --git a/drivers/pci/host/pci-hyperv.c 
> b/drivers/pci/host/pci-hyperv.c
>
> index 1713bfc..6d43f81 100644
>
> --- a/drivers/pci/host/pci-hyperv.c
>
> +++ b/drivers/pci/host/pci-hyperv.c
>
> @@ -1321,19 +1321,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.
>
> -        * The lower 16 bits of the serial number is used, otherwise some
>
> -        * drivers may not be able to handle it.
>
> -        */
>
> -       if (list_empty(&hbus->children))
>
> -               hbus->sysdata.domain = desc->ser & 0xFFFF;
>
>         list_add_tail(&hpdev->list_entry, &hbus->children);
>
>         spin_unlock_irqrestore(&hbus->device_list_lock, flags);
>
>         return hpdev;
>
> --
>
> 2.7.4
>
>
>
> srpitcha@ linux srpitcha/patch >
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel