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:   Wed, 27 Oct 2021 09:35:07 +0000
From:   Anand Ashok Dumbre <ANANDASH@...inx.com>
To:     Lars-Peter Clausen <lars@...afoo.de>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "jic23@...nel.org" <jic23@...nel.org>,
        "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
        git <git@...inx.com>, Michal Simek <michals@...inx.com>,
        "pmeerw@...erw.net" <pmeerw@...erw.net>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>
CC:     Manish Narani <MNARANI@...inx.com>
Subject: RE: [PATCH v7 2/4] iio: adc: Add Xilinx AMS driver

Hi Lars,

Thanks for the review.
I did a bit of digging and found the rest of the review.

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

I missed this. I will fix this and change the order of execution.

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

That is correct. I will fix this as well.

Thanks,
Anand