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

From: Krzysztof Kozlowski <krzk@kernel.org>
To: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
Cc: alexandre.belloni@bootlin.com, robh@kernel.org,
	krzk+dt@kernel.org, conor+dt@kernel.org,
	jarkko.nikula@linux.intel.com, linux-i3c@lists.infradead.org,
	linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, andersson@kernel.org,
	konradybcio@kernel.org
Subject: Re: [PATCH v3 2/3] i3c: master: Add Qualcomm I3C controller driver
Date: Wed, 9 Apr 2025 08:10:34 +0200	[thread overview]
Message-ID: <7bbe235d-be3a-4851-b9db-c3c9e956a9fd@kernel.org> (raw)
In-Reply-To: <6ab62bb9-2758-4a12-aec3-6de9efc3075a@quicinc.com>

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

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c