| Message ID | 1366009135-19088-1-git-send-email-wangyijing@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
[+cc Rafael, Huang] On Mon, Apr 15, 2013 at 12:58 AM, Yijing Wang <wangyijing@huawei.com> wrote: > We should decrease dev->enable_cnt when no pcie port capability > found for balance. > > Signed-off-by: Yijing Wang <wangyijing@huawei.com> > Cc: Jiang Liu <jiang.liu@huawei.com> > Cc: Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com> > Cc: Shengzhou Liu <Shengzhou.Liu@freescale.com> > --- > drivers/pci/pcie/portdrv_core.c | 4 ++-- > 1 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c > index 31063ac..aef3fac 100644 > --- a/drivers/pci/pcie/portdrv_core.c > +++ b/drivers/pci/pcie/portdrv_core.c > @@ -369,8 +369,8 @@ int pcie_port_device_register(struct pci_dev *dev) > > /* Get and check PCI Express port services */ > capabilities = get_port_device_capability(dev); > - if (!capabilities) > - return 0; > + if (!capabilities) > + goto error_disable; > > pci_set_master(dev); > /* Does this fix a problem you observed? If so, please refer to it in your changelog. I think this patch is incorrect because pcie_portdrv_probe() will return 0 (success) with the device disabled. When we call pcie_portdrv_remove(), we will attempt to disable the device again, even though it's already disabled. I don't know whether it is desirable for pcie_portdrv_probe() to succeed when no capabilities are available or not. Maybe somebody else has an opinion. Bjorn -- 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 Mon, Apr 15, 2013 at 11:27 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: > [+cc Rafael, Huang] > > On Mon, Apr 15, 2013 at 12:58 AM, Yijing Wang <wangyijing@huawei.com> wrote: >> We should decrease dev->enable_cnt when no pcie port capability >> found for balance. >> >> Signed-off-by: Yijing Wang <wangyijing@huawei.com> >> Cc: Jiang Liu <jiang.liu@huawei.com> >> Cc: Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com> >> Cc: Shengzhou Liu <Shengzhou.Liu@freescale.com> >> --- >> drivers/pci/pcie/portdrv_core.c | 4 ++-- >> 1 files changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c >> index 31063ac..aef3fac 100644 >> --- a/drivers/pci/pcie/portdrv_core.c >> +++ b/drivers/pci/pcie/portdrv_core.c >> @@ -369,8 +369,8 @@ int pcie_port_device_register(struct pci_dev *dev) >> >> /* Get and check PCI Express port services */ >> capabilities = get_port_device_capability(dev); >> - if (!capabilities) >> - return 0; >> + if (!capabilities) >> + goto error_disable; >> >> pci_set_master(dev); >> /* > > Does this fix a problem you observed? If so, please refer to it in > your changelog. > > I think this patch is incorrect because pcie_portdrv_probe() will > return 0 (success) with the device disabled. When we call > pcie_portdrv_remove(), we will attempt to disable the device again, > even though it's already disabled. > > I don't know whether it is desirable for pcie_portdrv_probe() to > succeed when no capabilities are available or not. Maybe somebody > else has an opinion. I think this patchset is incorrect too. Even if there is no PCIe services for the PCIe port. We still need the PCIe drivers, at least for runtime power management. Although runtime power management is disabled for the PCIe port now, I think we will enable it again after we fixed the corresponding issues. Best Regards, Huang Ying -- 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 2013/4/16 16:22, huang ying wrote: > On Mon, Apr 15, 2013 at 11:27 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: >> [+cc Rafael, Huang] >> >> On Mon, Apr 15, 2013 at 12:58 AM, Yijing Wang <wangyijing@huawei.com> wrote: >>> We should decrease dev->enable_cnt when no pcie port capability >>> found for balance. >>> >>> Signed-off-by: Yijing Wang <wangyijing@huawei.com> >>> Cc: Jiang Liu <jiang.liu@huawei.com> >>> Cc: Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com> >>> Cc: Shengzhou Liu <Shengzhou.Liu@freescale.com> >>> --- >>> drivers/pci/pcie/portdrv_core.c | 4 ++-- >>> 1 files changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c >>> index 31063ac..aef3fac 100644 >>> --- a/drivers/pci/pcie/portdrv_core.c >>> +++ b/drivers/pci/pcie/portdrv_core.c >>> @@ -369,8 +369,8 @@ int pcie_port_device_register(struct pci_dev *dev) >>> >>> /* Get and check PCI Express port services */ >>> capabilities = get_port_device_capability(dev); >>> - if (!capabilities) >>> - return 0; >>> + if (!capabilities) >>> + goto error_disable; >>> >>> pci_set_master(dev); >>> /* >> >> Does this fix a problem you observed? If so, please refer to it in >> your changelog. >> >> I think this patch is incorrect because pcie_portdrv_probe() will >> return 0 (success) with the device disabled. When we call >> pcie_portdrv_remove(), we will attempt to disable the device again, >> even though it's already disabled. >> >> I don't know whether it is desirable for pcie_portdrv_probe() to >> succeed when no capabilities are available or not. Maybe somebody >> else has an opinion. > > I think this patchset is incorrect too. Even if there is no PCIe > services for the PCIe port. We still need the PCIe drivers, at least > for runtime power management. Although runtime power management is > disabled for the PCIe port now, I think we will enable it again after > we fixed the corresponding issues. Hi Huang Ying, Thanks for your comments! I will drop this patch, because as you said pcie port device need pcie port driver regardless any pcie capabilities found. But there is still a problem about two pci_disable_device() called in pcie_portdrv_remove(). In this case, If we unbind pcie port driver, the pcie port device will be disabled. The child devices under it cannot use anymore. I send this patch in another reply. Thanks! Yijing > > Best Regards, > Huang Ying > > . >
diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c index 31063ac..aef3fac 100644 --- a/drivers/pci/pcie/portdrv_core.c +++ b/drivers/pci/pcie/portdrv_core.c @@ -369,8 +369,8 @@ int pcie_port_device_register(struct pci_dev *dev) /* Get and check PCI Express port services */ capabilities = get_port_device_capability(dev); - if (!capabilities) - return 0; + if (!capabilities) + goto error_disable; pci_set_master(dev); /*
We should decrease dev->enable_cnt when no pcie port capability found for balance. Signed-off-by: Yijing Wang <wangyijing@huawei.com> Cc: Jiang Liu <jiang.liu@huawei.com> Cc: Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com> Cc: Shengzhou Liu <Shengzhou.Liu@freescale.com> --- drivers/pci/pcie/portdrv_core.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)