A question about the patch: [PATCH] PCI/PM: Keep runtime PM enabled for unbound PCI devices
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??
Thanks
Mike
+ rc = pci_drv->probe(pci_dev, ddi->id);
if (rc) {
- pm_runtime_disable(dev);
- pm_runtime_set_suspended(dev);
- pm_runtime_put_noidle(dev);
+ pci_dev->driver = NULL;
+ pm_runtime_put_sync(dev);
}
- if (parent)
- pm_runtime_put(parent);
return rc;}
@@ -330,10 +325,8 @@ __pci_device_probe(struct pci_driver *drv, struct pci_dev *pci_dev)
id = pci_match_device(drv, pci_dev);
if (id)
error = pci_call_probe(drv, pci_dev, id);
- if (error >= 0) {
- pci_dev->driver = drv;
+ if (error >= 0)
error = 0;
- }
}
return error; }
.......................
Bjorn Helgaas
maintainer[+cc Rafael, linux-pm]
Quoted 48 lines · mike
Hi Huang Ying,
I see you are the author of this patch, commit id is:
967577b062417b4e4b8e27b711220f4124f5153aI have a question while I try to understand this patch,
So I would very grateful if you or others can give me some reply.................
(quoted diff) +2
-1- 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_runtimerelate??Thanks
Mike(quoted diff) +4
-8+ rc = pci_drv->probe(pci_dev, ddi->id); if (rc) { - pm_runtime_disable(dev); - pm_runtime_set_suspended(dev); - pm_runtime_put_noidle(dev); + pci_dev->driver = NULL; + pm_runtime_put_sync(dev); } - if (parent) - pm_runtime_put(parent); return rc; }@@ -330,10 +325,8 @@ __pci_device_probe(struct pci_driver *drv, struct pci_dev *pci_dev) id = pci_match_device(drv, pci_dev); if (id) error = pci_call_probe(drv, pci_dev, id); - if (error >= 0) { - pci_dev->driver = drv; + if (error >= 0) error = 0; - } } return error;}
.......................
Quoted 20 lines · Bjorn Helgaas
[+cc Rafael, linux-pm]
Quoted 17 lines · mike
Hi Huang Ying,
I see you are the author of this patch, commit id is:
967577b062417b4e4b8e27b711220f4124f5153aI have a question while I try to understand this patch,
So I would very grateful if you or others can give me some reply.................
(quoted diff) +2
-1- 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_runtimerelate??
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.
Alan Stern
Quoted 21 lines · Alan Stern
Quoted 19 lines · Bjorn Helgaas
[+cc Rafael, linux-pm]
Quoted 16 lines · mike
Hi Huang Ying,
I see you are the author of this patch, commit id is:
967577b062417b4e4b8e27b711220f4124f5153aI have a question while I try to understand this patch,
So I would very grateful if you or others can give me some reply.................
(quoted diff) +2
-1- 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.....
Quoted 8 lines
Quoted 1 line
Quoted 1 line
and I look inside the code, it may be
pm_runtimerelate??Yes, it is related to runtime PM. In the PCI subsystem, runtime PM doesn't do anything unless
pci_dev->driveris set. You can see this at the start ofpci_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->driverbefore 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.
Because it may be dangerous to call the functions in driver to do some actions in the device which probe failed.
Even if it will at last probe success, the driver itself must be very strong, because may be a lot critical data structs haven't be created or initialized, if we call driver function in probe state may be lead a invalid access to some pointer.
So I think this logic may have some issue, am I right?
If I am right, I have a *patch to fix this issue*, but I'm not sure it is safe enough to do PM ops in probe state.
Actually, I face a *bug report* to this issue, access the driver function in probe state, even though it can be fixed in driver ( Make the driver more stronger), but the root cause is this issue.
Pray for your reply, and I really want to know.
Thanks
Mike
Quoted 2 lines
Alan Stern
Quoted 30 lines · mike
Quoted 21 lines · Alan Stern
Quoted 19 lines · Bjorn Helgaas
[+cc Rafael, linux-pm]
Quoted 16 lines · mike
Hi Huang Ying,
I see you are the author of this patch, commit id is:
967577b062417b4e4b8e27b711220f4124f5153aI have a question while I try to understand this patch,
So I would very grateful if you or others can give me some reply.................
(quoted diff) +2
-1- 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.....Quoted 8 lines
Quoted 1 line
Quoted 1 line
and I look inside the code, it may be
pm_runtimerelate??Yes, it is related to runtime PM. In the PCI subsystem, runtime PM doesn't do anything unless
pci_dev->driveris set. You can see this at the start ofpci_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->driverbefore the probe routine runs.
Quoted 7 lines
Is there any situations , like in probe state,
pci_dev->driverhas been set. thepci_pm_runtime_xxx()has passedpci_dev->driverNULL check, but at this point, probe fail occurs, andpci_dev->driverto 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 pm_runtime_put_noidle() should be done as one of the latest actions in ->probe(), after the normal probe actions succeeded.
Best Regards,
Huang Ying
Quoted 36 lines · Huang Ying
Quoted 35 lines · mike
Quoted 21 lines · Alan Stern
Quoted 19 lines · Bjorn Helgaas
[+cc Rafael, linux-pm]
Quoted 16 lines · mike
Hi Huang Ying,
I see you are the author of this patch, commit id is:
967577b062417b4e4b8e27b711220f4124f5153aI have a question while I try to understand this patch,
So I would very grateful if you or others can give me some reply.................
(quoted diff) +2
-1- 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.....Quoted 8 lines
Quoted 1 line
Quoted 1 line
and I look inside the code, it may be
pm_runtimerelate??Yes, it is related to runtime PM. In the PCI subsystem, runtime PM doesn't do anything unless
pci_dev->driveris set. You can see this at the start ofpci_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->driverbefore the probe routine runs.Is there any situations , like in probe state,
pci_dev->driverhas been set. thepci_pm_runtime_xxx()has passedpci_dev->driverNULL check, but at this point, probe fail occurs, andpci_dev->driverto be set to NULL.
Quoted 7 lines
Quoted 2 lines
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, sopci_pm_runtime_xxx()will not be called untilpm_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;
I check the code and find out that, the case dev->driver is set but pci_dev->driver is not set just in remove state (__device_release_driver()), and after that state, all be set to NULL.
My understand is:dev->driver = NULL, also means unbound,
am I right?
Thanks
Mike
Quoted 4 lines
Best Regards,
Huang Ying
Quoted 50 lines · mike
Quoted 36 lines · Huang Ying
Quoted 35 lines · mike
Quoted 21 lines · Alan Stern
Quoted 19 lines · Bjorn Helgaas
[+cc Rafael, linux-pm]
Quoted 16 lines · mike
Hi Huang Ying,
I see you are the author of this patch, commit id is:
967577b062417b4e4b8e27b711220f4124f5153aI have a question while I try to understand this patch,
So I would very grateful if you or others can give me some reply.................
(quoted diff) +2
-1- 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.....Quoted 8 lines
Quoted 1 line
Quoted 1 line
and I look inside the code, it may be
pm_runtimerelate??Yes, it is related to runtime PM. In the PCI subsystem, runtime PM doesn't do anything unless
pci_dev->driveris set. You can see this at the start ofpci_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->driverbefore the probe routine runs.Is there any situations , like in probe state,
pci_dev->driverhas been set. thepci_pm_runtime_xxx()has passedpci_dev->driverNULL check, but at this point, probe fail occurs, andpci_dev->driverto be set to NULL.Quoted 7 lines
Quoted 2 lines
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, sopci_pm_runtime_xxx()will not be called untilpm_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()
Quoted 7 lines
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 circumstance.
Best Regards,
Huang Ying
Quoted 55 lines · Huang Ying
Quoted 54 lines · mike
Quoted 36 lines · Huang Ying
Quoted 35 lines · mike
Quoted 21 lines · Alan Stern
Quoted 19 lines · Bjorn Helgaas
[+cc Rafael, linux-pm]
Quoted 16 lines · mike
Hi Huang Ying,
I see you are the author of this patch, commit id is:
967577b062417b4e4b8e27b711220f4124f5153aI have a question while I try to understand this patch,
So I would very grateful if you or others can give me some reply.................
(quoted diff) +2
-1- 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.....Quoted 8 lines
Quoted 1 line
Quoted 1 line
and I look inside the code, it may be
pm_runtimerelate??Yes, it is related to runtime PM. In the PCI subsystem, runtime PM doesn't do anything unless
pci_dev->driveris set. You can see this at the start ofpci_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->driverbefore the probe routine runs.Is there any situations , like in probe state,
pci_dev->driverhas been set. thepci_pm_runtime_xxx()has passedpci_dev->driverNULL check, but at this point, probe fail occurs, andpci_dev->driverto be set to NULL.Quoted 7 lines
Quoted 2 lines
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, sopci_pm_runtime_xxx()will not be called untilpm_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
Quoted 5 lines
Quoted 3 lines
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;
I know I always been a question guy, i apologize for spend a lot time to reply this mail, but I really want to understand it,
Thanks
Mike
Quoted 6 lines
circumstance.
Best Regards,
Huang Ying
Quoted 64 lines · mike
Quoted 55 lines · Huang Ying
Quoted 54 lines · mike
Quoted 36 lines · Huang Ying
Quoted 35 lines · mike
Quoted 21 lines · Alan Stern
Quoted 19 lines · Bjorn Helgaas
[+cc Rafael, linux-pm]
Quoted 16 lines · mike
Hi Huang Ying,
I see you are the author of this patch, commit id is:
967577b062417b4e4b8e27b711220f4124f5153aI have a question while I try to understand this patch,
So I would very grateful if you or others can give me some reply.................
(quoted diff) +2
-1- 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.....Quoted 8 lines
Quoted 1 line
Quoted 1 line
and I look inside the code, it may be
pm_runtimerelate??Yes, it is related to runtime PM. In the PCI subsystem, runtime PM doesn't do anything unless
pci_dev->driveris set. You can see this at the start ofpci_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->driverbefore the probe routine runs.Is there any situations , like in probe state,
pci_dev->driverhas been set. thepci_pm_runtime_xxx()has passedpci_dev->driverNULL check, but at this point, probe fail occurs, andpci_dev->driverto be set to NULL.Quoted 7 lines
Quoted 2 lines
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, sopci_pm_runtime_xxx()will not be called untilpm_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; toQuoted 5 lines
Quoted 3 lines
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 meansdev->driveris NULL ?? butpci_dev->driveris set???
Quoted 7 lines
Because if use
pci_dev->drivercan put into low power state, means
pci_dev->driveris set, but in the situation, usedev->driverwill can't,means
dev->driver= null, but I have not find any case that
dev->driver= null, butpci_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().
Quoted 2 lines
I know I always been a question guy, i apologize for spend a lot time to reply this mail, but I really want to understand it,
Never mind.
Best Regards,
Huang Ying
Quoted 71 lines · Huang Ying
Quoted 70 lines · mike
Quoted 55 lines · Huang Ying
Quoted 54 lines · mike
Quoted 36 lines · Huang Ying
Quoted 35 lines · mike
Quoted 21 lines · Alan Stern
Quoted 19 lines · Bjorn Helgaas
[+cc Rafael, linux-pm]
Quoted 16 lines · mike
Hi Huang Ying,
I see you are the author of this patch, commit id is:
967577b062417b4e4b8e27b711220f4124f5153aI have a question while I try to understand this patch,
So I would very grateful if you or others can give me some reply.................
(quoted diff) +2
-1- 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.....Quoted 8 lines
Quoted 1 line
Quoted 1 line
and I look inside the code, it may be
pm_runtimerelate??Yes, it is related to runtime PM. In the PCI subsystem, runtime PM doesn't do anything unless
pci_dev->driveris set. You can see this at the start ofpci_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->driverbefore the probe routine runs.Is there any situations , like in probe state,
pci_dev->driverhas been set. thepci_pm_runtime_xxx()has passedpci_dev->driverNULL check, but at this point, probe fail occurs, andpci_dev->driverto be set to NULL.Quoted 7 lines
Quoted 2 lines
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, sopci_pm_runtime_xxx()will not be called untilpm_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; toQuoted 5 lines
Quoted 3 lines
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 meansdev->driveris NULL ?? butpci_dev->driveris set???Because if use
pci_dev->drivercan put into low power state, means
pci_dev->driveris set, but in the situation, usedev->driverwill can't,means
dev->driver= null, but I have not find any case that
Quoted 8 lines
Quoted 1 line
dev->driver= null, butpci_dev->driver!= null;Sorry I make a mistake here. The
dev->driver!= null inlocal_pci_probe(). We usepci_dev->driverinstead ofdev->driverinpci_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 inpci_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 ?
Thanks
Mike
Quoted 9 lines
Quoted 3 lines
I know I always been a question guy, i apologize for spend a lot time to reply this mail, but I really want to understand it,
Never mind.
Best Regards,
Huang Ying
Quoted 80 lines · mike
Quoted 71 lines · Huang Ying
Quoted 70 lines · mike
Quoted 55 lines · Huang Ying
Quoted 54 lines · mike
Quoted 36 lines · Huang Ying
Quoted 35 lines · mike
Quoted 21 lines · Alan Stern
Quoted 19 lines · Bjorn Helgaas
[+cc Rafael, linux-pm]
Quoted 16 lines · mike
Hi Huang Ying,
I see you are the author of this patch, commit id is:
967577b062417b4e4b8e27b711220f4124f5153aI have a question while I try to understand this patch,
So I would very grateful if you or others can give me some reply.................
(quoted diff) +2
-1- 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.....Quoted 8 lines
Quoted 1 line
Quoted 1 line
and I look inside the code, it may be
pm_runtimerelate??Yes, it is related to runtime PM. In the PCI subsystem, runtime PM doesn't do anything unless
pci_dev->driveris set. You can see this at the start ofpci_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->driverbefore the probe routine runs.Is there any situations , like in probe state,
pci_dev->driverhas been set. thepci_pm_runtime_xxx()has passedpci_dev->driverNULL check, but at this point, probe fail occurs, andpci_dev->driverto be set to NULL.Quoted 7 lines
Quoted 2 lines
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, sopci_pm_runtime_xxx()will not be called untilpm_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; toQuoted 5 lines
Quoted 3 lines
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 meansdev->driveris NULL ?? butpci_dev->driveris set???Because if use
pci_dev->drivercan put into low power state, means
pci_dev->driveris set, but in the situation, usedev->driverwill can't,means
dev->driver= null, but I have not find any case thatQuoted 8 lines
Quoted 1 line
dev->driver= null, butpci_dev->driver!= null;Sorry I make a mistake here. The
dev->driver!= null inlocal_pci_probe(). We usepci_dev->driverinstead ofdev->driverinpci_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 inpci_pm_runtime_xxx().
Quoted 5 lines
Yes, you are right, but what am I confuse is that, why check
dev->driverinpci_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().
Best Regards,
Huang Ying
Quoted 81 lines · Huang Ying
Quoted 80 lines · mike
Quoted 79 lines · Huang Ying
Quoted 64 lines · mike
Quoted 55 lines · Huang Ying
Quoted 54 lines · mike
Quoted 36 lines · Huang Ying
Quoted 35 lines · mike
Quoted 21 lines · Alan Stern
Quoted 19 lines · Bjorn Helgaas
[+cc Rafael, linux-pm]
Quoted 16 lines · mike
Hi Huang Ying,
I see you are the author of this patch, commit id is:
967577b062417b4e4b8e27b711220f4124f5153aI have a question while I try to understand this patch,
So I would very grateful if you or others can give me some reply.................
(quoted diff) +2
-1- 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.....Quoted 8 lines
Quoted 1 line
Quoted 1 line
and I look inside the code, it may be
pm_runtimerelate??Yes, it is related to runtime PM. In the PCI subsystem, runtime PM doesn't do anything unless
pci_dev->driveris set. You can see this at the start ofpci_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->driverbefore the probe routine runs.Is there any situations , like in probe state,
pci_dev->driverhas been set. thepci_pm_runtime_xxx()has passedpci_dev->driverNULL check, but at this point, probe fail occurs, andpci_dev->driverto be set to NULL.Quoted 7 lines
Quoted 2 lines
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, sopci_pm_runtime_xxx()will not be called untilpm_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; toQuoted 5 lines
Quoted 3 lines
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 meansdev->driveris NULL ?? butpci_dev->driveris set???Quoted 7 lines
Because if use
pci_dev->drivercan put into low power state, means
pci_dev->driveris set, but in the situation, usedev->driverwill can't,means
dev->driver= null, but I have not find any case that
dev->driver= null, butpci_dev->driver!= null;Sorry I make a mistake here. The
dev->driver!= null inlocal_pci_probe(). We usepci_dev->driverinstead ofdev->driverinpci_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 inpci_pm_runtime_xxx().
Quoted 8 lines
Quoted 5 lines
Yes, you are right, but what am I confuse is that, why check
dev->driverinpci_pm_runtime_xxx()can't keep the device in normal power state (D0) and SUSPENDED state when unbound.May be logic issue ?
Because
dev->driveris set beforelocal_pci_probe()and cleared afterpci_device_remove(). But we need a flag to be changed inlocal_pci_probe()andpci_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
Quoted 5 lines
Best Regards,
Huang Ying
Quoted 92 lines · Mike Qiu
Quoted 81 lines · Huang Ying
Quoted 80 lines · mike
Quoted 79 lines · Huang Ying
Quoted 64 lines · mike
Quoted 55 lines · Huang Ying
Quoted 54 lines · mike
Quoted 36 lines · Huang Ying
Quoted 35 lines · mike
Quoted 21 lines · Alan Stern
Quoted 19 lines · Bjorn Helgaas
[+cc Rafael, linux-pm]
Quoted 16 lines · mike
Hi Huang Ying,
I see you are the author of this patch, commit id is:
967577b062417b4e4b8e27b711220f4124f5153aI have a question while I try to understand this patch,
So I would very grateful if you or others can give me some reply.................
(quoted diff) +2
-1- 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.....Quoted 8 lines
Quoted 1 line
Quoted 1 line
and I look inside the code, it may be
pm_runtimerelate??Yes, it is related to runtime PM. In the PCI subsystem, runtime PM doesn't do anything unless
pci_dev->driveris set. You can see this at the start ofpci_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->driverbefore the probe routine runs.Is there any situations , like in probe state,
pci_dev->driverhas been set. thepci_pm_runtime_xxx()has passedpci_dev->driverNULL check, but at this point, probe fail occurs, andpci_dev->driverto be set to NULL.Quoted 7 lines
Quoted 2 lines
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, sopci_pm_runtime_xxx()will not be called untilpm_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; toQuoted 5 lines
Quoted 3 lines
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 meansdev->driveris NULL ?? butpci_dev->driveris set???Quoted 7 lines
Because if use
pci_dev->drivercan put into low power state, means
pci_dev->driveris set, but in the situation, usedev->driverwill can't,means
dev->driver= null, but I have not find any case that
dev->driver= null, butpci_dev->driver!= null;Sorry I make a mistake here. The
dev->driver!= null inlocal_pci_probe(). We usepci_dev->driverinstead ofdev->driverinpci_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 inpci_pm_runtime_xxx().Quoted 8 lines
Quoted 5 lines
Yes, you are right, but what am I confuse is that, why check
dev->driverinpci_pm_runtime_xxx()can't keep the device in normal power state (D0) and SUSPENDED state when unbound.May be logic issue ?
Because
dev->driveris set beforelocal_pci_probe()and cleared afterpci_device_remove(). But we need a flag to be changed inlocal_pci_probe()andpci_device_remove().Hi Ying,
Quoted 8 lines
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->driveris unset in probe state, it can checkdev->driverto see if the device diver can work. But for new logic it is really a big issue.
What is the other component and why is it doing that?
Checking dev->driver may not be a correct way to address this issue anyway.
Thanks!
Signature · 2 lines
I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center.
Quoted 95 lines · Rafael J. Wysocki
Quoted 94 lines · Mike Qiu
Quoted 81 lines · Huang Ying
Quoted 80 lines · mike
Quoted 79 lines · Huang Ying
Quoted 64 lines · mike
Quoted 55 lines · Huang Ying
Quoted 54 lines · mike
Quoted 36 lines · Huang Ying
Quoted 35 lines · mike
Quoted 21 lines · Alan Stern
Quoted 19 lines · Bjorn Helgaas
[+cc Rafael, linux-pm]
Quoted 16 lines · mike
Hi Huang Ying,
I see you are the author of this patch, commit id is:
967577b062417b4e4b8e27b711220f4124f5153aI 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.....Quoted 8 lines
Quoted 1 line
Quoted 1 line
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->driveris set. You can see this at the start ofpci_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->driverbefore the probe routine runs.Is there any situations , like in probe state,
pci_dev->driverhas been set. thepci_pm_runtime_xxx()has passedpci_dev->driverNULL check, but at this point, probe fail occurs, andpci_dev->driverto be set to NULL.Quoted 7 lines
Quoted 2 lines
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, sopci_pm_runtime_xxx()will not be called untilpm_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; toQuoted 5 lines
Quoted 3 lines
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 meansdev->driveris NULL ?? butpci_dev->driveris set???Quoted 7 lines
Because if use
pci_dev->drivercan put into low power state, means
pci_dev->driveris set, but in the situation, usedev->driverwill can't,means
dev->driver= null, but I have not find any case that
dev->driver= null, butpci_dev->driver!= null;Sorry I make a mistake here. The
dev->driver!= null inlocal_pci_probe(). We usepci_dev->driverinstead ofdev->driverinpci_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 inpci_pm_runtime_xxx().Quoted 8 lines
Quoted 5 lines
Yes, you are right, but what am I confuse is that, why check
dev->driverinpci_pm_runtime_xxx()can't keep the device in normal power state (D0) and SUSPENDED state when unbound.May be logic issue ?
Because
dev->driveris set beforelocal_pci_probe()and cleared afterpci_device_remove(). But we need a flag to be changed inlocal_pci_probe()andpci_device_remove().Hi Ying,
I'm now face one bug, and the root cause is this logic has some problem.
Quoted 7 lines
Quoted 6 lines
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->driveris unset in probe state, it can checkdev->driverto see if the device diver can work. But for new logic it is really a big issue.What is the other component and why is it doing that?
Some component like EEH in Power arch, it need to check whether the driver is work or not.
In old logic, if probed then dev->driver set, otherwise it will be NULL, it is safe to do so.
But in new, it has problem, it can call the driver API, which is very dangerous in probe state, maybe a lot key data structure haven't been setup yet, this lead to the kernel down and machine reboot. Also this can be fixed in driver, like check the driver data it self, this solution needs all the driver fix this issue, It may be a huge program.
So we need a new flag I think, or which old flag can we use to solve this issue ?
Thanks
Mike
Quoted 6 lines
Checking
dev->drivermay not be a correct way to address this issue anyway.Thanks!
Quoted 110 lines · Mike Qiu
Quoted 95 lines · Rafael J. Wysocki
Quoted 94 lines · Mike Qiu
Quoted 81 lines · Huang Ying
Quoted 80 lines · mike
Quoted 79 lines · Huang Ying
Quoted 64 lines · mike
Quoted 55 lines · Huang Ying
Quoted 54 lines · mike
Quoted 36 lines · Huang Ying
Quoted 35 lines · mike
Quoted 21 lines · Alan Stern
Quoted 19 lines · Bjorn Helgaas
[+cc Rafael, linux-pm]
On Wed, Nov 13, 2013 at 6:09 AM, mike<[email protected]> wrote:
Hi Huang Ying,I see you are the author of this patch, commit id is:
967577b062417b4e4b8e27b711220f4124f5153aI 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.....Quoted 8 lines
Quoted 1 line
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->driveris set. You can see this at the start ofpci_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->driverbefore the probe routine runs.Is there any situations , like in probe state,
pci_dev->driverhas been set. thepci_pm_runtime_xxx()has passedpci_dev->driverNULL check, but at this point, probe fail occurs, andpci_dev->driverto be set to NULL.Quoted 7 lines
Quoted 2 lines
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, sopci_pm_runtime_xxx()will not be called untilpm_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; toQuoted 5 lines
Quoted 3 lines
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 meansdev->driveris NULL ?? butpci_dev->driveris set???Quoted 7 lines
Because if use
pci_dev->drivercan put into low power state, means
pci_dev->driveris set, but in the situation, usedev->driverwill can't,means
dev->driver= null, but I have not find any case that
dev->driver= null, butpci_dev->driver!= null;Sorry I make a mistake here. The
dev->driver!= null inlocal_pci_probe(). We usepci_dev->driverinstead ofdev->driverinpci_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 inpci_pm_runtime_xxx().Quoted 8 lines
Quoted 5 lines
Yes, you are right, but what am I confuse is that, why check
dev->driverinpci_pm_runtime_xxx()can't keep the device in normal power state (D0) and SUSPENDED state when unbound.May be logic issue ?
Because
dev->driveris set beforelocal_pci_probe()and cleared afterpci_device_remove(). But we need a flag to be changed inlocal_pci_probe()andpci_device_remove().Hi Ying,
I'm now face one bug, and the root cause is this logic has some problem.
Quoted 7 lines
Quoted 6 lines
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->driveris unset in probe state, it can checkdev->driverto see if the device diver can work. But for new logic it is really a big issue.What is the other component and why is it doing that?
Some component like EEH in Power arch, it need to check whether the driver is work or not.
In old logic, if probed then
dev->driverset, otherwise it will be NULL, it is safe to do so.
Quoted 8 lines
But in new, it has problem, it can call the driver API, which is very dangerous in probe state, maybe a lot key data structure haven't been setup yet, this lead to the kernel down and machine reboot. Also this can be fixed in driver, like check the driver data it self, this solution needs all the driver fix this issue, It may be a huge program.
So we need a new flag I think, or which old flag can we use to solve this issue ?
I think a flag is not safe for you. Driver may be removed when you operate on it. Better to use device_lock() if possible, which will be held during device probe and driver remove.
Best Regards,
Huang Ying
Quoted 9 lines
Thanks
MikeQuoted 6 lines
Checking
dev->drivermay not be a correct way to address this issue anyway.Thanks!
Quoted 117 lines · Huang Ying
Quoted 116 lines · Mike Qiu
Quoted 95 lines · Rafael J. Wysocki
Quoted 94 lines · Mike Qiu
Quoted 81 lines · Huang Ying
Quoted 80 lines · mike
Quoted 79 lines · Huang Ying
Quoted 64 lines · mike
Quoted 55 lines · Huang Ying
Quoted 54 lines · mike
Quoted 36 lines · Huang Ying
Quoted 35 lines · mike
Quoted 29 lines · Alan Stern
On Wed, 13 Nov 2013, Bjorn Helgaas wrote:
[+cc Rafael, linux-pm]
On Wed, Nov 13, 2013 at 6:09 AM, mike<[email protected]> wrote:
Hi Huang Ying,I see you are the author of this patch, commit id is:
967577b062417b4e4b8e27b711220f4124f5153aI 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->driverhas been set. thepci_pm_runtime_xxx()has passedpci_dev->driverNULL check, but at this point, probe fail occurs, andpci_dev->driverto be set to NULL.Quoted 7 lines
Quoted 2 lines
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, sopci_pm_runtime_xxx()will not be called untilpm_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; toQuoted 5 lines
Quoted 3 lines
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 meansdev->driveris NULL ?? butpci_dev->driveris set???Quoted 7 lines
Because if use
pci_dev->drivercan put into low power state, means
pci_dev->driveris set, but in the situation, usedev->driverwill can't,means
dev->driver= null, but I have not find any case that
dev->driver= null, butpci_dev->driver!= null;Sorry I make a mistake here. The
dev->driver!= null inlocal_pci_probe(). We usepci_dev->driverinstead ofdev->driverinpci_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 inpci_pm_runtime_xxx().Quoted 8 lines
Quoted 5 lines
Yes, you are right, but what am I confuse is that, why check
dev->driverinpci_pm_runtime_xxx()can't keep the device in normal power state (D0) and SUSPENDED state when unbound.May be logic issue ?
Because
dev->driveris set beforelocal_pci_probe()and cleared afterpci_device_remove(). But we need a flag to be changed inlocal_pci_probe()andpci_device_remove().Hi Ying,
I'm now face one bug, and the root cause is this logic has some problem.
Quoted 7 lines
Quoted 6 lines
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->driveris unset in probe state, it can checkdev->driverto see if the device diver can work. But for new logic it is really a big issue.What is the other component and why is it doing that?
Some component like EEH in Power arch, it need to check whether the driver is work or not.
In old logic, if probed then
dev->driverset, otherwise it will be NULL, it is safe to do so.But in new, it has problem, it can call the driver API, which is very dangerous in probe state, maybe a lot key data structure haven't been setup yet, this lead to the kernel down and machine reboot. Also this can be fixed in driver, like check the driver data it self, this solution needs all the driver fix this issue, It may be a huge program.
Quoted 5 lines
Quoted 2 lines
So we need a new flag I think, or which old flag can we use to solve this issue ?
I think a flag is not safe for you. Driver may be removed when you operate on it.
Precisely. The old code is still unsafe although it happens to work in the given test conditions.
Quoted 2 lines
Better to use
device_lock()if possible, which will be held during device probe and driver remove.
Or generally synchronize it properly.
Thanks,
Rafael