| Message ID | 1458040168-3769-7-git-send-email-vinod.koul@intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 36e7972c0d3f8819a5d9335c36c5dcd168cd2b72 |
| Headers | show |
On Tue, Mar 15, 2016 at 04:39:28PM +0530, Vinod Koul wrote: > In driver remove we call pci_dev_put(), that is not required as we never > call pci_dev_get() so remove that. Why is the fix for this not to call pci_dev_get()?
On Wed, Mar 16, 2016 at 10:08:29AM +0000, Mark Brown wrote: > On Tue, Mar 15, 2016 at 04:39:28PM +0530, Vinod Koul wrote: > > In driver remove we call pci_dev_put(), that is not required as we never > > call pci_dev_get() so remove that. > > Why is the fix for this not to call pci_dev_get()? Why do I need either, I see no reason why driver should be doing this, so removed :)
On Wed, Mar 16, 2016 at 04:22:44PM +0530, Vinod Koul wrote: > On Wed, Mar 16, 2016 at 10:08:29AM +0000, Mark Brown wrote: > > Why is the fix for this not to call pci_dev_get()? > Why do I need either, I see no reason why driver should be doing this, > so removed :) Well, the PCI documentation says that drivers are expected to record a reference to their devices in probe(). This is a bit unusual given that normally the driver core takes a reference to the device for us but presumably there's some reason for this?
On Wed, 16 Mar 2016 12:03:10 +0100, Mark Brown wrote: > > On Wed, Mar 16, 2016 at 04:22:44PM +0530, Vinod Koul wrote: > > On Wed, Mar 16, 2016 at 10:08:29AM +0000, Mark Brown wrote: > > > > Why is the fix for this not to call pci_dev_get()? > > > Why do I need either, I see no reason why driver should be doing this, > > so removed :) > > Well, the PCI documentation says that drivers are expected to record a > reference to their devices in probe(). This is a bit unusual given that > normally the driver core takes a reference to the device for us but > presumably there's some reason for this? Maybe the document is obsoleted. The PCI core, at least the probe / remove via the normal PCI bus, takes pci_dev_get() and pci_dev_put() already there. Takashi
On Wed, Mar 16, 2016 at 12:35:08PM +0100, Takashi Iwai wrote: > On Wed, 16 Mar 2016 12:03:10 +0100, > Mark Brown wrote: > > > > On Wed, Mar 16, 2016 at 04:22:44PM +0530, Vinod Koul wrote: > > > On Wed, Mar 16, 2016 at 10:08:29AM +0000, Mark Brown wrote: > > > > > > Why is the fix for this not to call pci_dev_get()? > > > > > Why do I need either, I see no reason why driver should be doing this, > > > so removed :) > > > > Well, the PCI documentation says that drivers are expected to record a > > reference to their devices in probe(). This is a bit unusual given that > > normally the driver core takes a reference to the device for us but > > presumably there's some reason for this? > > Maybe the document is obsoleted. The PCI core, at least the probe / > remove via the normal PCI bus, takes pci_dev_get() and pci_dev_put() > already there. Yes that is my understanding too, that is why we removed this from driver here.. Mark, is this fine now?
On Wed, Mar 16, 2016 at 08:14:05PM +0530, Vinod Koul wrote: > On Wed, Mar 16, 2016 at 12:35:08PM +0100, Takashi Iwai wrote: > > Maybe the document is obsoleted. The PCI core, at least the probe / > > remove via the normal PCI bus, takes pci_dev_get() and pci_dev_put() > > already there. > Yes that is my understanding too, that is why we removed this from driver > here.. > Mark, is this fine now? You might want to put some of this analysis in the changelog.
On Wed, Mar 16, 2016 at 02:57:53PM +0000, Mark Brown wrote: > On Wed, Mar 16, 2016 at 08:14:05PM +0530, Vinod Koul wrote: > > On Wed, Mar 16, 2016 at 12:35:08PM +0100, Takashi Iwai wrote: > > > > Maybe the document is obsoleted. The PCI core, at least the probe / > > > remove via the normal PCI bus, takes pci_dev_get() and pci_dev_put() > > > already there. > > > Yes that is my understanding too, that is why we removed this from driver > > here.. > > > Mark, is this fine now? > > You might want to put some of this analysis in the changelog. Yup makes sense, v3 on the way then !
diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c index 72971dc55c52..07d9bc1cf3cb 100644 --- a/sound/soc/intel/skylake/skl.c +++ b/sound/soc/intel/skylake/skl.c @@ -724,7 +724,6 @@ static void skl_remove(struct pci_dev *pci) if (pci_dev_run_wake(pci)) pm_runtime_get_noresume(&pci->dev); - pci_dev_put(pci); /* codec removal, invoke bus_device_remove */ snd_hdac_ext_bus_device_remove(ebus);