linux-kernel - Re: [PATCH 09/13] media: qcom: camss: Add CSID Gen3 support for SM8550

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: <9255b3e4-874c-4919-b50a-919cf0f42f75@kernel.org>
Date: Thu, 11 Jul 2024 13:12:09 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Depeng Shao <quic_depengs@...cinc.com>, rfoss@...nel.org,
 todor.too@...il.com, bryan.odonoghue@...aro.org, mchehab@...nel.org,
 robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org
Cc: quic_eberman@...cinc.com, linux-media@...r.kernel.org,
 linux-arm-msm@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, kernel@...cinc.com,
 Yongsheng Li <quic_yon@...cinc.com>
Subject: Re: [PATCH 09/13] media: qcom: camss: Add CSID Gen3 support for
 SM8550

On 11/07/2024 13:08, Depeng Shao wrote:


>>
>>> + */
>>> +static int csid_reset(struct csid_device *csid)
>>> +{
>>> +	unsigned long time;
>>> +	u32 val;
>>> +	int i;
>>> +
>>> +	reinit_completion(&csid->reset_complete);
>>> +
>>> +	writel_relaxed(1, csid->base + CSID_TOP_IRQ_CLEAR);
>>> +	writel_relaxed(1, csid->base + CSID_IRQ_CMD);
>>> +	writel_relaxed(1, csid->base + CSID_TOP_IRQ_MASK);
>>> +
>>> +	for (i = 0; i < MSM_CSID_MAX_SRC_STREAMS; i++)
>>> +		if (csid->phy.en_vc & BIT(i)) {
>>> +			writel_relaxed(BIT(BUF_DONE_IRQ_STATUS_RDI_OFFSET + i),
>>> +						csid->base + CSID_BUF_DONE_IRQ_CLEAR);
>>> +			writel_relaxed(0x1 << IRQ_CMD_CLEAR, csid->base + CSID_IRQ_CMD);
>>> +			writel_relaxed(BIT(BUF_DONE_IRQ_STATUS_RDI_OFFSET + i),
>>> +						csid->base + CSID_BUF_DONE_IRQ_MASK);
>>> +		}
>>> +
>>> +	/* preserve registers */
>>> +	val = (0x1 << RST_LOCATION) | (0x1 << RST_MODE);
>>> +	writel_relaxed(val, csid->base + CSID_RST_CFG);
>>
>> ... here - using everywhere relaxed here is odd and looks racy. These
>> looks like some strict sequences.
>>
> Yes, these are some sequences to initialize the HW.

Hm? It's like you ignore the problem and just answer with whatever to
shut up the reviewer. Instead of replying with the same, address the
problem. Why ordering is not a problem here?

> 
>>
>>> +
>>> +	val = (0x1 << SELECT_HW_RST) | (0x1 << SELECT_IRQ_RST);
>>> +	writel_relaxed(val, csid->base + CSID_RST_CMD);
>>> +
>>> +	time = wait_for_completion_timeout(&csid->reset_complete,
>>> +					   msecs_to_jiffies(CSID_RESET_TIMEOUT_MS));
>>> +	if (!time) {
>>> +		dev_err(csid->camss->dev, "CSID reset timeout\n");
>>> +		return -EIO;
>>> +	}
>>> +
>>
>>
>>> +
>>> +static void csid_subdev_init(struct csid_device *csid)
>>> +{
>>> +	csid->testgen.modes = csid_testgen_modes;
>>> +	csid->testgen.nmodes = CSID_PAYLOAD_MODE_NUM_SUPPORTED_GEN2;
>>> +}
>>> +
>>> +const struct csid_hw_ops csid_ops_gen3 = {
>>
>> Isn't there a warning here?
>>
>>> +	.configure_stream = csid_configure_stream,
>>> +	.configure_testgen_pattern = csid_configure_testgen_pattern,
>>> +	.hw_version = csid_hw_version,
>>> +	.isr = csid_isr,
>>> +	.reset = csid_reset,
>>> +	.src_pad_code = csid_src_pad_code,
>>> +	.subdev_init = csid_subdev_init,
>>> +};
>>
>> Your patchset does not apply at all. Tried v6.9, v6.10, next. I see some
>> dependency above, but that means no one can test it and no one can apply it.
>>
>> Fix the warnings, I cannot verify it but I am sure you have them.
>>
> 
> My code base is next master branch, do you mean the 'declared and not 
> used' warning? I don't see this warning with below two version compiler 
> even though I just pick this patch and pull the code the latest version. 
> But it indeed have this issue, these structures are declared and will be 
> used later in "media: qcom: camss: Add sm8550 resources" patch, will 
> think about how to avoid this.
> 
> aarch64-linux-gnu-gcc (Ubuntu/Linaro 7.5.0-3ubuntu1~18.04) 7.5.0
> aarch64-linux-gnu-gcc (Ubuntu 11.4.0-1ubuntu1~22.04) 11.4.0

That's some old compilers... I am talking about recent GCC, recent clang
and of course W=1.



Best regards,
Krzysztof