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: <e93c50ce-30dd-45ef-b945-019e703bd7c3@kernel.org>
Date: Tue, 8 Apr 2025 20:41:15 +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 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.

> 
>>> +	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.

>>> +		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?

>>> +	pm_runtime_mark_last_busy(gi3c->se.dev);
>>> +	pm_runtime_put_autosuspend(gi3c->se.dev);
>>> +	mutex_unlock(&gi3c->lock);
>>> +}
>>> +
>>> +static void geni_i3c_abort_xfer(struct geni_i3c_dev *gi3c)
>>> +{
>>> +	unsigned long time_remaining;
>>> +	unsigned long flags;
>>> +
>>> +	reinit_completion(&gi3c->done);
>>> +	spin_lock_irqsave(&gi3c->irq_lock, flags);
>>> +	geni_i3c_handle_err(gi3c, GENI_TIMEOUT);
>>> +	geni_se_abort_m_cmd(&gi3c->se);
>>> +	spin_unlock_irqrestore(&gi3c->irq_lock, flags);
>>> +	time_remaining = wait_for_completion_timeout(&gi3c->done, XFER_TIMEOUT);
>>> +	if (!time_remaining)
>>> +		dev_err(gi3c->se.dev, "Timeout abort_m_cmd\n");
>>> +}
>>
>> ...
>>
>>> +
>>> +static int i3c_geni_resources_init(struct geni_i3c_dev *gi3c, struct platform_device *pdev)
>>> +{
>>> +	int ret;
>>> +
>>> +	gi3c->se.base = devm_platform_ioremap_resource(pdev, 0);
>>> +	if (IS_ERR(gi3c->se.base))
>>> +		return PTR_ERR(gi3c->se.base);
>>> +
>>> +	gi3c->se.clk = devm_clk_get(&pdev->dev, "se");
>>> +	if (IS_ERR(gi3c->se.clk))
>>> +		return dev_err_probe(&pdev->dev, PTR_ERR(gi3c->se.clk),
>>> +							"Unable to get serial engine core clock: %pe\n",
>>> +							gi3c->se.clk);
>>
>> Totally messed indentation.
>>
> yes, corrected.
>>> +	ret = geni_icc_get(&gi3c->se, NULL);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	/* Set the bus quota to a reasonable value for register access */
>>> +	gi3c->se.icc_paths[GENI_TO_CORE].avg_bw = GENI_DEFAULT_BW;
>>> +	gi3c->se.icc_paths[CPU_TO_GENI].avg_bw = GENI_DEFAULT_BW;
>>> +	ret = geni_icc_set_bw(&gi3c->se);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	/* Default source clock (se-clock-frequency) freq is 100Mhz */
>>> +	gi3c->clk_src_freq = KHZ(100000);
>>
>> And why can't you use clk_get_rate()?
>>
> During probe(), we need one time initialization of source clock 
> frequencey. HW has no clock set before this.

How is it possible that there is no clock or clock was not configured
but you need to know it? Anyway, it's tiring to keep discussing this.

>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int geni_i3c_probe(struct platform_device *pdev)
>>> +{
>>> +	u32 proto, tx_depth, fifo_disable;
>>> +	struct geni_i3c_dev *gi3c;
>>
>> Just store pdev->dev in local dev variable, to simplify everything here.
> yes, thats right. But i see other drivers are using same pdev->dev. Is 
> it fine ? if really required, will change it.

Are you going to discuss every little comment? And come with arguments
like "I found poor code, so I am allowed to do the same"?

Best regards,
Krzysztof