[V3,09/16] platform/x86/intel/pmc: Allow pmc_core_ssram_init to fail
Commit Message
Currently, if the PMC SSRAM initialization fails, no error is returned and
the only indication is that a PMC device has not been created. Instead,
allow an error to be returned and handled directly by the caller.
Signed-off-by: David E. Box <david.e.box@linux.intel.com>
---
V3 - New patch split from V2 PATCH 9
- Add dev_warn on pmc_core_ssram_init fail
drivers/platform/x86/intel/pmc/core.h | 2 +-
drivers/platform/x86/intel/pmc/core.h | 2 +-
drivers/platform/x86/intel/pmc/core_ssram.c | 21 +++++++++++++--------
drivers/platform/x86/intel/pmc/mtl.c | 12 ++++++++----
3 files changed, 22 insertions(+), 13 deletions(-)
Comments
On Wed, 11 Oct 2023, David E. Box wrote:
> Currently, if the PMC SSRAM initialization fails, no error is returned and
> the only indication is that a PMC device has not been created. Instead,
> allow an error to be returned and handled directly by the caller.
You might have a good reason for it but why isn't the call into
pmc_core_pmc_add() changed in this patch to take the error value into
account?
(I vaguely remember this was probably discussed in the context of some
earlier patch touching this area that it was about the other code dealing
with NULLs or something like that).
On Thu, 2023-10-12 at 18:01 +0300, Ilpo Järvinen wrote:
> On Wed, 11 Oct 2023, David E. Box wrote:
>
> > Currently, if the PMC SSRAM initialization fails, no error is returned and
> > the only indication is that a PMC device has not been created. Instead,
> > allow an error to be returned and handled directly by the caller.
>
> You might have a good reason for it but why isn't the call into
> pmc_core_pmc_add() changed in this patch to take the error value into
> account?
Good catch. The return value of pmc_core_pmc_add() is first used in the next
patch but should be used here.
David
>
> (I vaguely remember this was probably discussed in the context of some
> earlier patch touching this area that it was about the other code dealing
> with NULLs or something like that).
On Thu, 12 Oct 2023, David E. Box wrote:
> On Thu, 2023-10-12 at 18:01 +0300, Ilpo Järvinen wrote:
> > On Wed, 11 Oct 2023, David E. Box wrote:
> >
> > > Currently, if the PMC SSRAM initialization fails, no error is returned and
> > > the only indication is that a PMC device has not been created. Instead,
> > > allow an error to be returned and handled directly by the caller.
> >
> > You might have a good reason for it but why isn't the call into
> > pmc_core_pmc_add() changed in this patch to take the error value into
> > account?
>
> Good catch. The return value of pmc_core_pmc_add() is first used in the next
> patch but should be used here.
>
> >
> > (I vaguely remember this was probably discussed in the context of some
> > earlier patch touching this area that it was about the other code dealing
> > with NULLs or something like that).
Okay but please also take into consideration what I tried to imply above:
Since you are doing what looks a major logic change in the next patch, it
might be okay to _not use_ that return value until then if you e.g., need
to add lots of rollback that isn't there already to the code that is going
away anyway in the next patch. (After all, returning an error code from a
function that was void previously isn't going to magically break the old
calling code).
@@ -492,7 +492,7 @@ int pmc_core_resume_common(struct pmc_dev *pmcdev);
int get_primary_reg_base(struct pmc *pmc);
extern void pmc_core_get_low_power_modes(struct pmc_dev *pmcdev);
-extern void pmc_core_ssram_init(struct pmc_dev *pmcdev);
+extern int pmc_core_ssram_init(struct pmc_dev *pmcdev);
int spt_core_init(struct pmc_dev *pmcdev);
int cnp_core_init(struct pmc_dev *pmcdev);
@@ -35,20 +35,20 @@ static inline u64 get_base(void __iomem *addr, u32 offset)
return lo_hi_readq(addr + offset) & GENMASK_ULL(63, 3);
}
-static void
+static int
pmc_core_pmc_add(struct pmc_dev *pmcdev, u64 pwrm_base,
const struct pmc_reg_map *reg_map, int pmc_index)
{
struct pmc *pmc = pmcdev->pmcs[pmc_index];
if (!pwrm_base)
- return;
+ return -ENODEV;
/* Memory for primary PMC has been allocated in core.c */
if (!pmc) {
pmc = devm_kzalloc(&pmcdev->pdev->dev, sizeof(*pmc), GFP_KERNEL);
if (!pmc)
- return;
+ return -ENOMEM;
}
pmc->map = reg_map;
@@ -57,10 +57,12 @@ pmc_core_pmc_add(struct pmc_dev *pmcdev, u64 pwrm_base,
if (!pmc->regbase) {
devm_kfree(&pmcdev->pdev->dev, pmc);
- return;
+ return -ENOMEM;
}
pmcdev->pmcs[pmc_index] = pmc;
+
+ return 0;
}
static void
@@ -96,7 +98,7 @@ pmc_core_ssram_get_pmc(struct pmc_dev *pmcdev, void __iomem *ssram, u32 offset,
iounmap(ssram);
}
-void pmc_core_ssram_init(struct pmc_dev *pmcdev)
+int pmc_core_ssram_init(struct pmc_dev *pmcdev)
{
void __iomem *ssram;
struct pci_dev *pcidev;
@@ -105,7 +107,7 @@ void pmc_core_ssram_init(struct pmc_dev *pmcdev)
pcidev = pci_get_domain_bus_and_slot(0, 0, PCI_DEVFN(20, 2));
if (!pcidev)
- goto out;
+ return -ENODEV;
ret = pcim_enable_device(pcidev);
if (ret)
@@ -123,11 +125,14 @@ void pmc_core_ssram_init(struct pmc_dev *pmcdev)
pmc_core_ssram_get_pmc(pmcdev, ssram, SSRAM_PCH_OFFSET, PMC_IDX_PCH);
iounmap(ssram);
-out:
- return;
+
+ return 0;
disable_dev:
+ pmcdev->ssram_pcidev = NULL;
pci_disable_device(pcidev);
release_dev:
pci_dev_put(pcidev);
+
+ return ret;
}
@@ -990,12 +990,16 @@ int mtl_core_init(struct pmc_dev *pmcdev)
mtl_d3_fixup();
pmcdev->resume = mtl_resume;
-
pmcdev->regmap_list = mtl_pmc_info_list;
- pmc_core_ssram_init(pmcdev);
- /* If regbase not assigned, set map and discover using legacy method */
- if (!pmc->regbase) {
+ /*
+ * If ssram init fails use legacy method to at least get the
+ * primary PMC
+ */
+ ret = pmc_core_ssram_init(pmcdev);
+ if (ret) {
+ dev_warn(&pmcdev->pdev->dev,
+ "ssram init failed, %d, using legacy init\n", ret);
pmc->map = &mtl_socm_reg_map;
ret = get_primary_reg_base(pmc);
if (ret)