linux-kernel - RE: [PATCH v10 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] [day] [month] [year] [list]

Date:   Sun, 21 Nov 2021 12:29:59 +0000
From:   Anand Ashok Dumbre <ANANDASH@...inx.com>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.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 v10 3/5] iio: adc: Add Xilinx AMS driver

Hi Andy,

Thanks for the review.

> -----Original Message-----
> From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
> Sent: Thursday 18 November 2021 3:43 PM
> To: Anand Ashok Dumbre <ANANDASH@...inx.com>
> Cc: linux-kernel@...r.kernel.org; jic23@...nel.org; lars@...afoo.de; linux-
> iio@...r.kernel.org; git <git@...inx.com>; Michal Simek
> <michals@...inx.com>; gregkh@...uxfoundation.org; rafael@...nel.org;
> linux-acpi@...r.kernel.org; heikki.krogerus@...ux.intel.com; Manish Narani
> <MNARANI@...inx.com>
> Subject: Re: [PATCH v10 3/5] iio: adc: Add Xilinx AMS driver
> 
> On Thu, Nov 18, 2021 at 02:14:41PM +0000, Anand Ashok Dumbre wrote:
> > > From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
> > > Sent: Wednesday 17 November 2021 8:03 PM On Wed, Nov 17, 2021 at
> > > 04:10:26PM +0000, Anand Ashok Dumbre wrote:
> 
> ...
> 
> > > > +			ret = ams_read_vcc_reg(ams, chan->address, val);
> > > > +			if (ret) {
> > > > +				mutex_unlock(&ams->lock);
> > > > +				return -EINVAL;
> > >
> > > Shadowed error code.
> >
> > I don’t understand.
> 
> You get an error code in ret variable, but you always return -EINVAL, why?

Got it. Will fix it.

> 
> > > > +			}
> 
> ...
> 
> > > default: ?
> >
> > This is limited by hw bits.
> > For default I will use the default scan_index value.
> > Is that ok?
> 
> I think so, you know hardware better, you decide what to put there.

Will do

> 
> ...
> 
> > > > +		if (ret || reg > (AMS_PL_MAX_EXT_CHANNEL + 30))
> > >
> > > Too many parentheses.
> >
> > Is it a good practice to not have parantheses around
> (AMS_PL_MAX_EXT_CHANNEL + 30) ?
> 
> It's minor thing, up to you.
> 
> > > > +			continue;
> 
> ...
> 
> > > > +	dev_chan_size = array_size((size_t)num_channels, sizeof(struct
> > > iio_chan_spec));
> > > > +	if (dev_chan_size == SIZE_MAX)
> > > > +		return -EINVAL;
> > >
> > > Why is it needed now?

I didn’t see kcalloc did it internally. I will remove it.

> > >
> > > > +	dev_channels = devm_kcalloc(&pdev->dev, (size_t)num_channels,
> > >
> > > Why casting?

Removed.

> > >
> > > > +				    sizeof(struct iio_chan_spec), GFP_KERNEL);
> > >
> > > sizeof(*dev_channels)
> > >
> > > > +	if (!dev_channels) {
> > > > +		ret = -ENOMEM;
> > > > +		goto free_mem;
> > > > +	}
> > >
> > > > +	memcpy(dev_channels, ams_channels,
> > > > +	       sizeof(*ams_channels) * num_channels);
> > >
> > > Hmm... according to the code the num_channels can be less than or
> > > equal to num_chan. Hence, what you should use is the
> devm_krealloc_array().
> > >
> > > static inline void *devm_krealloc_aray(...) {
> > > 	...see how krealloc_array() is defined...
> > > }
> > >
> > > No need to copy memory again.
> >
> > Will take a look.

I did see that there was previously a devm_krealloc_array implementation sent for the other Xilinx xadc driver.

https://patchwork.kernel.org/project/linux-iio/patch/20201102142228.14949-2-brgl@bgdev.pl/

From the conversations based here, should the use of devm_krealloc just be fine?
If not, I will send a patch with devm_krealloc for now, and then I can send a patch later for devm_krealloc array and
Its usage for both Xilinx-ams and xadc driver as well.


> 
> --
> With Best Regards,
> Andy Shevchenko
> 

Thanks,
Anand