* [PATCH v2 1/2] platform/x86: amd-pmc: Store the pci_dev instance inside struct amd_pmc_dev @ 2021-10-21 9:29 Sanket Goswami 2021-10-21 18:18 ` Hans de Goede 0 siblings, 1 reply; 5+ messages in thread From: Sanket Goswami @ 2021-10-21 9:29 UTC (permalink / raw) To: Shyam-sundar.S-k, hdegoede, mgross Cc: platform-driver-x86, linux-kernel, Sanket Goswami 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@amd.com> --- Changes in v2: - Store the rdev info in amd_pmc_probe() as suggested by Hans. 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); base_addr = ((u64)base_addr_hi << 32 | base_addr_lo); dev->regbase = devm_ioremap(dev->dev, base_addr + AMD_PMC_BASE_ADDR_OFFSET, @@ -546,6 +547,7 @@ static int amd_pmc_remove(struct platform_device *pdev) struct amd_pmc_dev *dev = platform_get_drvdata(pdev); amd_pmc_dbgfs_unregister(dev); + pci_dev_put(dev->rdev); mutex_destroy(&dev->lock); return 0; } -- 2.25.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2 1/2] platform/x86: amd-pmc: Store the pci_dev instance inside struct amd_pmc_dev 2021-10-21 9:29 [PATCH v2 1/2] platform/x86: amd-pmc: Store the pci_dev instance inside struct amd_pmc_dev Sanket Goswami @ 2021-10-21 18:18 ` Hans de Goede 2021-10-22 6:55 ` Goswami, Sanket 0 siblings, 1 reply; 5+ messages in thread From: Hans de Goede @ 2021-10-21 18:18 UTC (permalink / raw) To: Sanket Goswami, Shyam-sundar.S-k, mgross Cc: platform-driver-x86, linux-kernel 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@amd.com> > --- > 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. 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; } Regards, Hans > base_addr = ((u64)base_addr_hi << 32 | base_addr_lo); > > dev->regbase = devm_ioremap(dev->dev, base_addr + AMD_PMC_BASE_ADDR_OFFSET, > @@ -546,6 +547,7 @@ static int amd_pmc_remove(struct platform_device *pdev) > struct amd_pmc_dev *dev = platform_get_drvdata(pdev); > > amd_pmc_dbgfs_unregister(dev); > + pci_dev_put(dev->rdev); > mutex_destroy(&dev->lock); > return 0; > } > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2 1/2] platform/x86: amd-pmc: Store the pci_dev instance inside struct amd_pmc_dev 2021-10-21 18:18 ` Hans de Goede @ 2021-10-22 6:55 ` Goswami, Sanket 2021-10-22 8:51 ` Hans de Goede 0 siblings, 1 reply; 5+ messages in thread From: Goswami, Sanket @ 2021-10-22 6:55 UTC (permalink / raw) To: Hans de Goede, Shyam-sundar.S-k, mgross; +Cc: platform-driver-x86, linux-kernel 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@amd.com> >> --- >> 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2 1/2] platform/x86: amd-pmc: Store the pci_dev instance inside struct amd_pmc_dev 2021-10-22 6:55 ` Goswami, Sanket @ 2021-10-22 8:51 ` Hans de Goede 2021-10-22 10:16 ` Goswami, Sanket 0 siblings, 1 reply; 5+ messages in thread From: Hans de Goede @ 2021-10-22 8:51 UTC (permalink / raw) To: Goswami, Sanket, Shyam-sundar.S-k, mgross Cc: platform-driver-x86, linux-kernel Hi Sanket, On 10/22/21 08:55, Goswami, Sanket wrote: > 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@amd.com> >>> --- >>> 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? What I was trying to say is that the patch does not apply, because it is trying to remove the pci_put_dev() line from a block of code like this: base_addr_hi = val & AMD_PMC_BASE_ADDR_LO_MASK; pci_dev_put(dev->rdev); base_addr = ((u64)base_addr_hi << 32 | base_addr_lo); But the actual code in platform-drivers-x86/review-hans (and for-next too) has: base_addr_hi = val & AMD_PMC_BASE_ADDR_LO_MASK; pci_dev_put(rdev); base_addr = ((u64)base_addr_hi << 32 | base_addr_lo); After your patch using dev->rdev instead of just rdev is fine (but please be consistent, which would mean use just rdev everywhere). But your patch is removing a line which does not exist in that form, IOW it is based on some intermediate version of amd-pmc.c and not on the HEAD of platform-drivers-x86/review-hans. Regards, Hans > >> >> 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 > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2 1/2] platform/x86: amd-pmc: Store the pci_dev instance inside struct amd_pmc_dev 2021-10-22 8:51 ` Hans de Goede @ 2021-10-22 10:16 ` Goswami, Sanket 0 siblings, 0 replies; 5+ messages in thread From: Goswami, Sanket @ 2021-10-22 10:16 UTC (permalink / raw) To: Hans de Goede, Shyam-sundar.S-k, mgross; +Cc: platform-driver-x86, linux-kernel Hi Hans, On 22-Oct-21 14:21, Hans de Goede wrote: > [CAUTION: External Email] > > Hi Sanket, > > On 10/22/21 08:55, Goswami, Sanket wrote: >> 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@amd.com> >>>> --- >>>> 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? > > What I was trying to say is that the patch does not apply, because it is > trying to remove the pci_put_dev() line from a block of code like this: > > base_addr_hi = val & AMD_PMC_BASE_ADDR_LO_MASK; > pci_dev_put(dev->rdev); > base_addr = ((u64)base_addr_hi << 32 | base_addr_lo); > > But the actual code in platform-drivers-x86/review-hans (and for-next too) has: > > base_addr_hi = val & AMD_PMC_BASE_ADDR_LO_MASK; > pci_dev_put(rdev); > base_addr = ((u64)base_addr_hi << 32 | base_addr_lo); > > > > After your patch using dev->rdev instead of just rdev is fine > (but please be consistent, which would mean use just rdev everywhere). > > But your patch is removing a line which does not exist in that form, > IOW it is based on some intermediate version of amd-pmc.c and not > on the HEAD of platform-drivers-x86/review-hans. I will rebase it to review-hans branch and will respin a new version. Thanks, Sanket ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-10-22 10:17 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-10-21 9:29 [PATCH v2 1/2] platform/x86: amd-pmc: Store the pci_dev instance inside struct amd_pmc_dev Sanket Goswami 2021-10-21 18:18 ` Hans de Goede 2021-10-22 6:55 ` Goswami, Sanket 2021-10-22 8:51 ` Hans de Goede 2021-10-22 10:16 ` Goswami, Sanket
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®