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

[Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>]

Thanks Krzysztof !

On 4/9/2025 11:40 AM, Krzysztof Kozlowski wrote:
> 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
> 
Thanks ! it seems little more deep to go for me. Appreciate your 
pointers here.
> 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.
> 
No, mine was quick research, what i got is below from my search and i 
mentioned in crisp. What you pointed above 
Documentation/dev-tools/sparse.rst looks great.

===
Sparse and Lockdep are tools used in the Linux kernel development to 
help with code analysis and debugging.

Sparse
Sparse is a static code analyzer specifically designed for the Linux 
kernel. It helps developers find potential issues in their code by 
performing checks that are not typically done by the compiler. Sparse 
annotations are special comments or attributes added to the code to 
guide Sparse in its analysis. Some common Sparse annotations include:

__attribute__((noderef)): Indicates that a pointer should not be 
dereferenced.
__attribute__((address_space(x))): Specifies the address space of a pointer.
__attribute__((force)): Forces a type conversion that Sparse would 
normally warn about.
Lockdep
Lockdep is a runtime lock validator used in the Linux kernel to detect 
potential deadlocks. It records information about the order in which 
locks are acquired and checks for inconsistencies that could lead to 
deadlocks. Lockdep annotations are used to perform runtime checks on 
locking correctness. Some common Lockdep annotations include:

lockdep_assert_held(&lock): Asserts that a particular lock is held at a 
certain time and generates a warning if it is not.
lockdep_pin_lock(&lock): Prevents accidental unlocking of a lock.
These tools are crucial for maintaining the stability and reliability of 
the kernel by catching potential issues early in the development process.
===

> 
> Best regards,
> Krzysztof