PCI: Separate stop and remove devices in pciehp

Hi Yinghai,
   The third patch make pci_stop_dev call pci_disable_sriov(dev).
It looks asymmetrical, because pci_enable_sriov(dev, NR_VIRTFN) always
be called by device driver. Why not work around this in device driver like ixgbe?
I'm not familiar with SRIOV, so I'm just a bit puzzled.

Thanks!
Yijing.

------------------------------------------------------------
Some driver (like ixgbe) does not call pci_disable_sriov() if
sriov is enabled via /sys/.../sriov_numvfs setting.
ixgbe does allow driver for PF get detached, but still have VFs
around.

But how about PF get removed via /sys or pciehp finally?

During hot-remove, VF will still hold one ref to PF and it
prevent PF to be removed.
That make the next hot-add fails, as old PF dev struct is still around.

We need to add pci_disable_sriov() calling during stop PF .

Need this one for v3.11

-v2: Accoring to Bjorn, move that calling to pci_stop_dev.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Cc: Jiang Liu <liuj97@gmail.com>
Cc: Alexander Duyck <alexander.h.duyck@intel.com>
Cc: Donald Dutile <ddutile@redhat.com>
Cc: Greg Rose <gregory.v.rose@intel.com>

---
 drivers/pci/remove.c |    2 ++
 1 file changed, 2 insertions(+)

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Tue, Jul 23, 2013 at 10:00 PM, Yijing Wang <wangyijing@huawei.com> wrote:
> Hi Yinghai,
>    The third patch make pci_stop_dev call pci_disable_sriov(dev).
> It looks asymmetrical, because pci_enable_sriov(dev, NR_VIRTFN) always
> be called by device driver. Why not work around this in device driver like ixgbe?
> I'm not familiar with SRIOV, so I'm just a bit puzzled.
>
> Thanks!
> Yijing.
>
> ------------------------------------------------------------
> Some driver (like ixgbe) does not call pci_disable_sriov() if
> sriov is enabled via /sys/.../sriov_numvfs setting.
> ixgbe does allow driver for PF get detached, but still have VFs
> around.
>
> But how about PF get removed via /sys or pciehp finally?
>
> During hot-remove, VF will still hold one ref to PF and it
> prevent PF to be removed.
> That make the next hot-add fails, as old PF dev struct is still around.
>
> We need to add pci_disable_sriov() calling during stop PF .
>
> Need this one for v3.11
>
> -v2: Accoring to Bjorn, move that calling to pci_stop_dev.
>
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> Cc: Jiang Liu <liuj97@gmail.com>
> Cc: Alexander Duyck <alexander.h.duyck@intel.com>
> Cc: Donald Dutile <ddutile@redhat.com>
> Cc: Greg Rose <gregory.v.rose@intel.com>
>
> ---
>  drivers/pci/remove.c |    2 ++
>  1 file changed, 2 insertions(+)
>
> Index: linux-2.6/drivers/pci/remove.c
> ===================================================================
> --- linux-2.6.orig/drivers/pci/remove.c
> +++ linux-2.6/drivers/pci/remove.c
> @@ -25,6 +25,8 @@ static void pci_stop_dev(struct pci_dev
>                 pci_proc_detach_device(dev);
>                 pci_remove_sysfs_dev_files(dev);
>                 device_del(&dev->dev);
> +               /* remove VF, if PF driver skip that */
> +               pci_disable_sriov(dev);
>                 dev->is_added = 0;
>         }

Can you post these as a new thread with -v2 patches?

Why did you put pci_disable_sriov() inside the "if (dev->is_added)"
block?  Is it related to is_added?
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Fri, Jul 26, 2013 at 3:05 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Tue, Jul 23, 2013 at 10:00 PM, Yijing Wang <wangyijing@huawei.com> wrote:
>> Hi Yinghai,
>>    The third patch make pci_stop_dev call pci_disable_sriov(dev).
>> It looks asymmetrical, because pci_enable_sriov(dev, NR_VIRTFN) always
>> be called by device driver. Why not work around this in device driver like ixgbe?
>> I'm not familiar with SRIOV, so I'm just a bit puzzled.
>>
>> Thanks!
>> Yijing.
>>
>> ------------------------------------------------------------
>> Some driver (like ixgbe) does not call pci_disable_sriov() if
>> sriov is enabled via /sys/.../sriov_numvfs setting.
>> ixgbe does allow driver for PF get detached, but still have VFs
>> around.
>>
>> But how about PF get removed via /sys or pciehp finally?
>>
>> During hot-remove, VF will still hold one ref to PF and it
>> prevent PF to be removed.
>> That make the next hot-add fails, as old PF dev struct is still around.
>>
>> We need to add pci_disable_sriov() calling during stop PF .
>>
>> Need this one for v3.11
>>
>> -v2: Accoring to Bjorn, move that calling to pci_stop_dev.
>>
>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>> Cc: Jiang Liu <liuj97@gmail.com>
>> Cc: Alexander Duyck <alexander.h.duyck@intel.com>
>> Cc: Donald Dutile <ddutile@redhat.com>
>> Cc: Greg Rose <gregory.v.rose@intel.com>
>>
>> ---
>>  drivers/pci/remove.c |    2 ++
>>  1 file changed, 2 insertions(+)
>>
>> Index: linux-2.6/drivers/pci/remove.c
>> ===================================================================
>> --- linux-2.6.orig/drivers/pci/remove.c
>> +++ linux-2.6/drivers/pci/remove.c
>> @@ -25,6 +25,8 @@ static void pci_stop_dev(struct pci_dev
>>                 pci_proc_detach_device(dev);
>>                 pci_remove_sysfs_dev_files(dev);
>>                 device_del(&dev->dev);
>> +               /* remove VF, if PF driver skip that */
>> +               pci_disable_sriov(dev);
>>                 dev->is_added = 0;
>>         }
>
> Can you post these as a new thread with -v2 patches?

ok, will repost them on top of pci/for-linus

>
> Why did you put pci_disable_sriov() inside the "if (dev->is_added)"
> block?  Is it related to is_added?

yes, only have pci_enable_sriov when is_added is true.
driver is detached when device_del() is called, and at that time
pci_disable_sriov should be just called.

Thanks

Yinghai
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html