Re: [PATCH 2/2] soc: qcom: smp2p: Add support for smp2p v2

[Deepak Kumar Singh <quic_deesin@quicinc.com>]

On 10/21/2025 3:09 PM, Konrad Dybcio wrote:
> On 10/21/25 10:23 AM, Deepak Kumar Singh wrote:
>>
>>
>> On 9/24/2025 8:27 PM, Konrad Dybcio wrote:
>>> On 9/24/25 6:18 AM, Jingyi Wang wrote:
>>>> From: Chris Lew <chris.lew@oss.qualcomm.com>
>>>>
>>>> Some remoteproc need smp2p v2 support, mirror the version written by the
>>>> remote if the remote supports v2. This is needed if the remote does not
>>>> have backwards compatibility with v1. And reset entry last value on SSR for
>>>> smp2p v2.
>>>>
>>>> Signed-off-by: Chris Lew <chris.lew@oss.qualcomm.com>
>>>> Co-developed-by: Jingyi Wang <jingyi.wang@oss.qualcomm.com>
>>>> Signed-off-by: Jingyi Wang <jingyi.wang@oss.qualcomm.com>
>>>> ---
>>>
>>> [...]
>>>
>>>> +static int qcom_smp2p_in_version(struct qcom_smp2p *smp2p)
>>>> +{
>>>> +    unsigned int smem_id = smp2p->smem_items[SMP2P_INBOUND];
>>>> +    unsigned int pid = smp2p->remote_pid;
>>>> +    struct smp2p_smem_item *in;
>>>> +    size_t size;
>>>> +
>>>> +    in = qcom_smem_get(pid, smem_id, &size);
>>>> +    if (IS_ERR(in))
>>>> +        return 0;
>>>> +
>>>> +    return in->version;
>>>
>>> are in and out versions supposed to be out of sync in case of
>>> error?
>>>
>> I think that should be ok. If we return error smp2p connection will be completely broken. With default version 1 partial features can be supported even if remote is using version 2. Some features like smp2p connection after subsystem restart may be affected by this.>> +}
> 
> Perhaps a different question is in order.. do we ever expect smem_get to
> fail under normal conditions?
> 
> [...]
> 
Good point, i think that should never happen for early boot processor 
which will use version 2. That can possibly happen for processor that is 
coming late than local host(version 1). In that case anyway we are 
setting default version 1 and proceeding. >>>>        /*
>>>>         * Make sure the rest of the header is written before we validate the
>>>>         * item by writing a valid version number.
>>>>         */
>>>>        wmb();
>>>> -    out->version = 1;
>>>> +    out->version = (in_version) ? in_version : 1;
>>>
>>> = in_version ?: 1
>>>
>>> Konrad
>>>
>> We want to assign in_version when value is 1/2 and 1 if value is 0 i.e. error case.
> 
> That's what this syntax does, with less characters
> 
> Konrad
Yes in_version ?:1 is short hand but i find (in_version) ? in_version : 
1; more readable and obvious.