Re: A question about the patch: [PATCH] PCI/PM: Keep runtime PM enabled for unbound PCI devices

[Mike Qiu <qiudayu@linux.vnet.ibm.com>]

On 11/14/2013 04:54 PM, Huang Ying wrote:
> On Thu, 2013-11-14 at 16:37 +0800, mike wrote:
>> On 11/14/2013 04:25 PM, Huang Ying wrote:
>>> On Thu, 2013-11-14 at 16:12 +0800, mike wrote:
>>>> On 11/14/2013 03:53 PM, Huang Ying wrote:
>>>>> On Thu, 2013-11-14 at 15:19 +0800, mike wrote:
>>>>>> On 11/14/2013 01:59 PM, Huang Ying wrote:
>>>>>>> On Thu, 2013-11-14 at 11:23 +0800, mike wrote:
>>>>>>>> On 11/14/2013 03:20 AM, Alan Stern wrote:
>>>>>>>>> On Wed, 13 Nov 2013, Bjorn Helgaas wrote:
>>>>>>>>>
>>>>>>>>>> [+cc Rafael, linux-pm]
>>>>>>>>>>
>>>>>>>>>> On Wed, Nov 13, 2013 at 6:09 AM, mike<qiudayu@linux.vnet.ibm.com>  wrote:
>>>>>>>>>>> Hi Huang Ying,
>>>>>>>>>>>
>>>>>>>>>>> I see you are the author of this patch, commit id is:
>>>>>>>>>>> 967577b062417b4e4b8e27b711220f4124f5153a
>>>>>>>>>>>
>>>>>>>>>>> I have a question while I try to understand this patch,
>>>>>>>>>>> So I would very grateful if you or others can give me some reply.....
>>>>>>>>>>>
>>>>>>>>>>> ............
>>>>>>>>>>> -       rc = ddi->drv->probe(ddi->dev, ddi->id);
>>>>>>>>>>> +       pm_runtime_get_sync(dev);
>>>>>>>>>>> +       pci_dev->driver = pci_drv;
>>>>>>>>>>> ^^^^^^^^^^^^^^^^^^^^^^^^^^
>>>>>>>>>>> I see here you make the driver to initialize before probe,
>>>>>>>>>>> But I have no idea of why you do this change.....
>>>>>>>>>>>
>>>>>>>>>>> and I look inside the code, it may be pm_runtime relate??
>>>>>>>>> Yes, it is related to runtime PM.  In the PCI subsystem, runtime PM
>>>>>>>>> doesn't do anything unless pci_dev->driver is set.  You can see this at
>>>>>>>>> the start of pci_pm_runtime_suspend().
>>>>>>>>>
>>>>>>>>> Since we want the driver's probe routine to be able to carry out
>>>>>>>>> runtime PM operations, we have to set pci_dev->driver before the probe
>>>>>>>>> routine runs.
>>>>>>>> Is there any situations , like in  probe state,  pci_dev->driver
>>>>>>>> has been set. the  pci_pm_runtime_xxx() has passed
>>>>>>>> pci_dev->driver NULL check, but at this point, probe fail
>>>>>>>> occurs, and  pci_dev->driver to be set to NULL.
>>>>>>>>
>>>>>>>> What will happen ? Or this situation will never happen?
>>>>>>>> I'm confuse about this.
>>>>>>> I think that will never happen.  Before ->probe(), pm_runtime_get_sync()
>>>>>>> is called, so pci_pm_runtime_xxx() will not be called until
>>>>>>> pm_runtime_put_noidle() is called in ->probe().  And
>>>>>>>      should be done as one of the latest actions in
>>>>>>> ->probe(), after the normal probe actions succeeded.
>>>>>> OK, just as your description, it seems OK.
>>>>>> But this is really a issue as I explained in last email.
>>>>>>
>>>>>> So I want to know if there are any side-effect of changing the code
>>>>>> in pci_pm_runtime_xxx()
>>>>>>
>>>>>>      if (!pci_dev->driver)
>>>>>>             return 0;
>>>>>>      to
>>>>>>
>>>>>>      if (!dev->driver)
>>>>>>             return 0;
>>>>>>
>>>>> If you make this change, we can not put devices into low power state
>>>>> (runtime suspend the device) in ->probe().  That is expected in some
>>>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>>> This means dev->driver is NULL ?? but pci_dev->driver is set???
>>>>
>>>> Because if use pci_dev->driver can put into low power state, means
>>>>
>>>> pci_dev->driver is set, but in the situation, use dev->driver will can't,
>>>>
>>>> means dev->driver = null, but I have not find any case that
>>>>
>>>> dev->driver = null, but pci_dev->driver != null;
>>> Sorry I make a mistake here.  The dev->driver != null in
>>> local_pci_probe().  We use pci_dev->driver instead of dev->driver in
>>> pci_pm_runtime_xxx() because we want device to be kept in normal power
>>> state (D0) and SUSPENDED state when unbound.The
>>> pm_runtime_put/get_sync in pci_device_remove/local_pci_probe will not
>>> change the power state of the device because of the check in
>>> pci_pm_runtime_xxx().
>> Yes, you are right, but what am I confuse is that, why check dev->driver
>> in pci_pm_runtime_xxx() can't keep the device in normal power
>> state (D0) and SUSPENDED state when unbound.
>>
>> May be logic issue ?
> Because dev->driver is set before local_pci_probe() and cleared after
> pci_device_remove().  But we need a flag to be changed in
> local_pci_probe() and pci_device_remove().
Hi Ying,

I'm now face one bug, and the root cause is this logic has some problem.

The other component calls the ops in driver during probe state, which a 
lot of critical data struct haven't been setup yet.

This never happen in old logic, because dev->driver is unset in probe 
state, it can check dev->driver to see if the device diver can work. But 
for new logic it is really a big issue.

Shall I add an other flag like 'unsigned int probe_state:1' in struct 
pci_dev instead of setting dev->driver before probe and change the logic 
back?

Then in pci_pm_runtime_xxx() can check this flag instead of pci_dev->driver.

If my logic does not affect your PM logic, I will send out the patch for 
review. Otherwise I will consider other solutions.

Thanks
Mike
>
> Best Regards,
> Huang Ying
>
>