Hi,
On 10/29/21 19:23, 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 v5:
> - Remove pci_dev_put() from amd_pmc_remove() as its no longer required.
>
> Changes in v4:
> - No change.
>
> Changes in v3:
> - Add pci_dev_put() in amd_pmc_remove().
>
> Changes in v2:
> - Store the rdev info in amd_pmc_probe() as suggested by Hans.
>
> drivers/platform/x86/amd-pmc.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/platform/x86/amd-pmc.c b/drivers/platform/x86/amd-pmc.c
> index 9af02860ed59..ea099f7759f2 100644
> --- a/drivers/platform/x86/amd-pmc.c
> +++ b/drivers/platform/x86/amd-pmc.c
> @@ -121,6 +121,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;
> @@ -538,6 +539,7 @@ static int amd_pmc_probe(struct platform_device *pdev)
> }
>
> dev->cpu_id = rdev->device;
> + dev->rdev = rdev;
> err = pci_write_config_dword(rdev, AMD_PMC_SMU_INDEX_ADDRESS, AMD_PMC_BASE_ADDR_LO);
> if (err) {
> dev_err(dev->dev, "error writing to 0x%x\n", AMD_PMC_SMU_INDEX_ADDRESS);
>
I'm afraid this is still not correct:
1. The pci_dev_put() at line 570 is still there, so after that line
you no longer have a reference to the pci_dev and the pointer may
end up pointing to free-ed memory
2. Once you drop the pci_dev_put() at line 570, all the error-exit
paths from probe after that, like this one :
dev->regbase = devm_ioremap(dev->dev, base_addr + AMD_PMC_BASE_ADDR_OFFSET,
AMD_PMC_MAPPING_SIZE);
if (!dev->regbase)
return -ENOMEM;
need to be changed to now, put the rdev (since that is now no longer
done on line 570), so this needs to be changed to:
dev->regbase = devm_ioremap(dev->dev, base_addr + AMD_PMC_BASE_ADDR_OFFSET,
AMD_PMC_MAPPING_SIZE);
if (!dev->regbase) {
err = -ENOMEM;
goto err_pci_dev_put;
}
and the same for:
dev->fch_virt_addr = devm_ioremap(dev->dev, fch_phys_addr, FCH_SSC_MAPPING_SIZE);
if (!dev->fch_virt_addr)
return -ENOMEM;
3. Since you now keep the reference on a succesfull probe, you need to add a
pci_dev_put(dev->rdev);
call to amd_pmc_remove()
Note that the changelog says you already did 3. in v3 but for some reason you
have completely dropped that and related changes now again :|
I've already asked for these changes and explained what you needed to do
several times now; and to be honest this is growing quite tiresome.
You should have noticed that somehow the changes from v3 disappeared here
yourself.
Please be more thorough and check your own work before posting for v6.
Regards,
Hans
