Re: [PATCH v3 2/3] i3c: master: Add Qualcomm I3C controller driver

[Krzysztof Kozlowski <krzk@kernel.org>]

On 09/04/2025 07:48, Mukesh Kumar Savaliya wrote:
> Hi Krzysztof,
> 
> On 4/9/2025 12:11 AM, Krzysztof Kozlowski wrote:
>> On 08/04/2025 15:23, Mukesh Kumar Savaliya wrote:
>>>>> +
>>>>> +static int i3c_geni_runtime_get_mutex_lock(struct geni_i3c_dev *gi3c)
>>>>> +{
>>>>
>>>> You miss sparse/lockdep annotations.
>>>>
>>> This is called in pair only, but to avoid repeated code in caller
>>> functions, we have designed this wrapper.
>>> i3c_geni_runtime_get_mutex_lock()
>>> i3c_geni_runtime_put_mutex_unlock().
>>>
>>> caller function maintains the parity. e.g. geni_i3c_master_priv_xfers().
>>>
>>> Does a comment help here ? Then i can write up to add.
>>
>> I do not see how this is relevant to my comment at all.
>>
> What i understood is you suspect about lock/unlock imbalance right ?
> I know that Lockdep annotations will be used to check if locks are 
> acquired and released in a proper order.
> 
> You want me to add below code in both the functions mentioned ?
>      lockdep_assert_held(&gi3c->lock);
> 
> What exact sparse/attribute can be added ? I am not sure about that.

I don't think you tried enough.

git grep sparse -- Documentation/
which gives you the file name, so:
git grep lock -- Documentation/dev-tools/sparse.rst

Use sparse instead of lockdep.

>>>
>>>>> +	int ret;
>>>>> +
>>>>> +	mutex_lock(&gi3c->lock);
>>>>> +	reinit_completion(&gi3c->done);
>>>>> +	ret = pm_runtime_get_sync(gi3c->se.dev);
>>>>> +	if (ret < 0) {
>>>>> +		dev_err(gi3c->se.dev, "error turning on SE resources:%d\n", ret);
>>>>> +		pm_runtime_put_noidle(gi3c->se.dev);
>>>>> +		/* Set device in suspended since resume failed */
>>>>> +		pm_runtime_set_suspended(gi3c->se.dev);
>>>>> +		mutex_unlock(&gi3c->lock);
>>>>
>>>> Either you lock or don't lock, don't mix these up.
>>>>
>>> Caller is taking care of not calling i3c_geni_runtime_put_mutex_unlock()
>>> if this failed.
>>
>>
>> I do not see how this is relevant to my comment at all.
>>
> same as above


>>>>> +		return ret;
>>>>> +	}
>>>>> +
>>>>> +	return 0;
>>>>> +}
>>>>> +
>>>>> +static void i3c_geni_runtime_put_mutex_unlock(struct geni_i3c_dev *gi3c)
>>>>> +{
>>>>
>>>> Missing annotations.
>>>>
>>> Shall i add a comment here ?
>>
>> Do you understand what is sparse? And lockdep?
>>
> Little but not clear on exact sparse attribute to be added. please help 
> me. if you can help with some clear comment and sample, will be easier 
> if you can.

You did not even bother to grep for simple term.


Best regards,
Krzysztof