Hi Hans, On 21-Oct-21 23:48, Hans de Goede wrote: > [CAUTION: External Email] > > Hi, > > On 10/21/21 11:29, Sanket Goswami wrote: >> Store the root port information in amd_pmc_probe() so that the >> information can be used across multiple routines. >> >> Signed-off-by: Sanket Goswami <Sanket.Goswami@xxxxxxx> >> --- >> Changes in v2: >> - Store the rdev info in amd_pmc_probe() as suggested by Hans. > > Thank you, but there are still some issues, see below. > > >> drivers/platform/x86/amd-pmc.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/platform/x86/amd-pmc.c b/drivers/platform/x86/amd-pmc.c >> index 55f14bdfdbfd..502f37eaba1f 100644 >> --- a/drivers/platform/x86/amd-pmc.c >> +++ b/drivers/platform/x86/amd-pmc.c >> @@ -119,6 +119,7 @@ struct amd_pmc_dev { >> u16 minor; >> u16 rev; >> struct device *dev; >> + struct pci_dev *rdev; >> struct mutex lock; /* generic mutex lock */ >> #if IS_ENABLED(CONFIG_DEBUG_FS) >> struct dentry *dbgfs_dir; >> @@ -482,6 +483,7 @@ static int amd_pmc_probe(struct platform_device *pdev) >> return -ENODEV; >> } >> >> + dev->rdev = rdev; >> dev->cpu_id = rdev->device; >> err = pci_write_config_dword(rdev, AMD_PMC_SMU_INDEX_ADDRESS, AMD_PMC_BASE_ADDR_LO); >> if (err) { >> @@ -512,7 +514,6 @@ static int amd_pmc_probe(struct platform_device *pdev) >> } >> >> base_addr_hi = val & AMD_PMC_BASE_ADDR_LO_MASK; >> - pci_dev_put(dev->rdev); > > The current code here actually reads: > > pci_dev_put(rdev); > > Note (rdev) not (dev->rdev). I don't know what you based this on, this is weird. rdev is already retrieved before doing this: pci_dev_put(dev->rdev); i.e. in amd_pmc_probe() rdev = pci_get_domain_bus_and_slot(0, 0, PCI_DEVFN(0, 0)); if (!rdev || !pci_match_id(pmc_pci_ids, rdev)) { pci_dev_put(rdev); return -ENODEV; } after this I am storing rdev in "dev->rdev" i.e. dev->rdev = rdev; after this I am using "dev->rdev" at places where "rdev" was getting used earlier. Do you see any problem? > > Also there are a bunch of error-exits from amd_pmc_probe() which not all > need a "pci_dev_put(rdev)" added to them before there "return ERROR;" > statement. > > It would be best to add: > > err_pci_dev_put: > pci_dev_put(rdev); > return err; > > Add the end off the function (after the return 0;) and replace all > "return FOO" error-exits with: > > err = <FOO>; > goto err_pci_dev_put; > } > Thank you, will take it as a separate patch in v3. Thanks, Sanket