[Skiboot] [PATCH v3 3/8] capp/phb: Introduce 'struct capp' to hold capp related info in 'struct phb'

[Frederic Barrat]

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.

   Fred


>>
>>> +	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
>> microcode?
> Good catch. Will get it fixed.
>