Patchwork [-v2,1/2] PCI: decrease pci_dev->enable_cnt when no pcie capability found

login
register
mail settings
Submitter Yijing Wang
Date April 15, 2013, 6:58 a.m.
Message ID <1366009135-19088-1-git-send-email-wangyijing@huawei.com>
Download mbox | patch
Permalink /patch/236503/
State Rejected
Headers show

Comments

Yijing Wang - April 15, 2013, 6:58 a.m.
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(-)
Bjorn Helgaas - April 15, 2013, 3:27 p.m.
[+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
huang ying - April 16, 2013, 8:22 a.m.
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
Yijing Wang - April 16, 2013, 8:55 a.m.
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
> 
> .
>

Patch

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);
 	/*