linux-kernel - Re: [PATCH v7 2/4] iio: adc: Add Xilinx AMS 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]

Date:   Mon, 25 Oct 2021 15:08:00 +0200
From:   Lars-Peter Clausen <lars@...afoo.de>
To:     Anand Ashok Dumbre <anand.ashok.dumbre@...inx.com>,
        linux-kernel@...r.kernel.org, jic23@...nel.org,
        linux-iio@...r.kernel.org, git@...inx.com, michal.simek@...inx.com,
        pmeerw@...erw.net, devicetree@...r.kernel.org
Cc:     Manish Narani <manish.narani@...inx.com>
Subject: Re: [PATCH v7 2/4] iio: adc: Add Xilinx AMS driver

On 10/19/21 5:20 PM, Anand Ashok Dumbre wrote:
> +static int ams_init_device(struct ams *ams)
> +{
> +	u32 reg;
> +	int ret;
> +
> [...]
> +	if (ams->pl_base) {
> +		writel(AMS_PL_RESET_VALUE, ams->pl_base + AMS_VP_VN);
> +
> +		ret = readl_poll_timeout(ams->base + AMS_PL_CSTS, reg,
> +					 (reg & AMS_PL_CSTS_ACCESS_MASK) ==
> +					 AMS_PL_CSTS_ACCESS_MASK, 0,
> +					 AMS_INIT_TIMEOUT_US);

The PL_CSTS register indicates whether the PL monitor can be accessed 
through the AMS.

But here we access the reset register even before the check. In addition 
there is really no point in polling the register as the state will not 
change. If the PL can not be accessed this should just return. And only 
after verifying that the PL can be accessed should the reset be done.

> +		if (ret)
> +			return ret;
> +
> +		/* put sysmon in a default state */
> +		ams_pl_update_reg(ams, AMS_REG_CONFIG1, AMS_CONF1_SEQ_MASK,
> +				  AMS_CONF1_SEQ_DEFAULT);
> +	}
> +
> +	[...]
> +
> +	return 0;
> +}
> +
>
> +static int ams_probe(struct platform_device *pdev)
> +{
> [..]
> +
> +	ret = ams_init_device(ams);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to initialize AMS\n");
> +		return ret;
> +	}
> +
> +	ret = ams_parse_dt(indio_dev, pdev);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failure in parsing DT\n");
> +		return ret;
> +	}
> +
These two need to be called the other way around. ams_init_device() 
wants to access the IO registers, but they are only mapped in 
ams_parse_dt().