[Skiboot] [PATCH v3 3/8] capp/phb: Introduce 'struct capp' to hold capp related info in 'struct phb'
fbarrat at linux.ibm.com
Fri Jan 11 03:19:11 AEDT 2019
Le 10/01/2019 à 15:13, Vaibhav Jain a écrit :
> Hi Fred,
> Thanks for reviewing this patch.
> Frederic Barrat <fbarrat at linux.ibm.com> writes:
>>> + capp->attached_pe = phb4_get_reserved_pe_number(&p->phb);
>> It is my understanding that we just want to invalidate the stored
>> attached_pe and that phb4_get_reserved_pe_number() is used, but it could
>> as well be -1. It's not used anyway.
> Want to use this member in future to set/remove the CAPP's IODA entries
> that we populate.
>> More fundamentally, why do we bother to store the attached_pe number? It
>> seems it's only used when calling set_capi_mode() to go back to PCI
>> mode, in which case, we don't care about the PE value.
> Agreed, but sending the wrong value to the set_capi_mode() function
> feels wrong and may catch some off gaurd in future.
I would argue that it also feels wrong to use the reserved value from
phb4_get_reserved_pe_number(), I was really wondering where that was
coming from when reading the patch. And I don't think it's that wrong to
pass an invalid value like -1 when switching to pci mode. It's unused in
that case anyway.
Anyway, it's not that important, and the code in the patch works.
>>> + capp->chip_id = p->chip_id;
>>> + /* Load capp microcode into the capp unit */
>>> + rc = load_capp_ucode(p);
>>> + if (rc == OPAL_SUCCESS)
>>> + p->capp = capp;
>>> + return rc;
>> Shouldn't we free the capp memory structure if we coulnd't load the
> Good catch. Will get it fixed.
More information about the Skiboot