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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <7bbe235d-be3a-4851-b9db-c3c9e956a9fd@kernel.org>
Date: Wed, 9 Apr 2025 08:10:34 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Mukesh Kumar Savaliya <quic_msavaliy@...cinc.com>
Cc: alexandre.belloni@...tlin.com, robh@...nel.org, krzk+dt@...nel.org,
 conor+dt@...nel.org, jarkko.nikula@...ux.intel.com,
 linux-i3c@...ts.infradead.org, linux-arm-msm@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 andersson@...nel.org, konradybcio@...nel.org
Subject: Re: [PATCH v3 2/3] i3c: master: Add Qualcomm I3C controller driver

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