linux-kernel - Re: [PATCH v9 3/5] 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, 17 Nov 2021 14:48:54 +0200
From:   Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:     Anand Ashok Dumbre <ANANDASH@...inx.com>
Cc:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "jic23@...nel.org" <jic23@...nel.org>,
        "lars@...afoo.de" <lars@...afoo.de>,
        "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
        git <git@...inx.com>, Michal Simek <michals@...inx.com>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "rafael@...nel.org" <rafael@...nel.org>,
        "linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>,
        "heikki.krogerus@...ux.intel.com" <heikki.krogerus@...ux.intel.com>,
        Manish Narani <MNARANI@...inx.com>
Subject: Re: [PATCH v9 3/5] iio: adc: Add Xilinx AMS driver

On Tue, Nov 16, 2021 at 08:29:21PM +0000, Anand Ashok Dumbre wrote:
> > From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
> > Sent: Tuesday 16 November 2021 5:39 PM
> > On Tue, Nov 16, 2021 at 03:08:40PM +0000, Anand Ashok Dumbre wrote:

...

> > > +#define AMS_ALARM_THR_MIN		0x0000
> > > +#define AMS_ALARM_THR_MAX		0xFFFF
> > 
> > If this is limited by hardware register, I would rather use (BIT(16) - 1)
> > notation. It will give immediately amount of bits used for the value.

> So ~(BIT(16) - 1) for AMS_ALARM_THR_MIN

This will give wrong value, so preserving 0 as plain decimal is fine.

> (BIT(16) - 1) for AMS_ALARM_THR_MAX

...

> > > +	u32 reg;
> > > +	int ret;
> > 
> > 	u32 expect = AMS_PS_CSTS_PS_READY;
> > 
> > (Use similar approach for other readX_poll_timeout() cases)
> > 
> > > +		ret = readl_poll_timeout(ams->base + AMS_PS_CSTS, reg,
> > > +					 (reg & AMS_PS_CSTS_PS_READY) ==
> > > +					 AMS_PS_CSTS_PS_READY, 0,
> > > +					 AMS_INIT_TIMEOUT_US);
> > 
> > 		ret = readl_poll_timeout(ams->base + AMS_PS_CSTS, reg,
> > 					 (reg & expect) == expect,
> > 					 0, AMS_INIT_TIMEOUT_US);

> > 0?!

Any comments on this?

Besides there are other cases you haven't answered on, so I assume you agreed
to change as suggested.

> > > +		if (ret)
> > > +			return ret;
> > 
> > ...
> > 
> > > +		ret = readl(ams->base + AMS_PL_CSTS);
> > > +		if (ret == 0)
> > > +			return ret;
> > 
> > Assigning u32 to int seems wrong.
> 
> It's a single bit register.
> Even if I use u32 here, the return type is int.

The problem here is that you checked not for error code, readl() doesn't return
an error. So semantic issue.

> So, is it ok if I read using u32 and return it by typecasting to int?

No. You need to have something like this:

	value = readl(...);
	if (value == 0)
		return 0;

this will keep proper meaning of each number and variable, while compiler may
optimize it.

...

> > > +					regval = readl(ams->pl_base +
> > > +						       AMS_REG_CONFIG4);
> > 
> > One line?
> > 
> > > +					regval = readl(ams->pl_base +
> > > +						       AMS_REG_CONFIG4);
> > 
> > Ditto and so on...
> > 
> It goes over 80 chars per line.

Is it a problem?

...

> > > +	schedule_delayed_work(&ams->ams_unmask_work,
> > > +			      msecs_to_jiffies(AMS_UNMASK_TIMEOUT_MS));
> > 
> > Can be one line.
> 
> Over 80 characters.

Is it a problem?

> Oh! I just saw that upto 100 chars is ok.

-- 
With Best Regards,
Andy Shevchenko