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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Zth59xLYZ4skc4yb@Asurada-Nvidia>
Date: Wed, 4 Sep 2024 08:17:11 -0700
From: Nicolin Chen <nicolinc@...dia.com>
To: Dan Carpenter <dan.carpenter@...aro.org>
CC: <will@...nel.org>, <robin.murphy@....com>, <joro@...tes.org>,
	<jgg@...dia.com>, <thierry.reding@...il.com>, <vdumpa@...dia.com>,
	<jonathanh@...dia.com>, <linux-kernel@...r.kernel.org>,
	<iommu@...ts.linux.dev>, <linux-arm-kernel@...ts.infradead.org>,
	<linux-tegra@...r.kernel.org>
Subject: Re: [PATCH v14 08/10] iommu/arm-smmu-v3: Add in-kernel support for
 NVIDIA Tegra241 (Grace) CMDQV

Hi Dan,

On Wed, Sep 04, 2024 at 10:29:26AM +0300, Dan Carpenter wrote:
 
> I was reviewing Smatch warnings:
> 
>     drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c:616 tegra241_cmdqv_init_vintf()
>     error: Calling ida_alloc_max() with a 'max' argument which is a power of 2. -1 missing?
> 
> The problem is that we're calling ida_alloc_max() where max is always zero.
> 
> > +static int tegra241_cmdqv_init_vintf(struct tegra241_cmdqv *cmdqv, u16 max_idx,
> > +                                  struct tegra241_vintf *vintf)
> > +{
> > +
> > +     u16 idx;
> > +     int ret;
> > +
> > +     ret = ida_alloc_max(&cmdqv->vintf_ids, max_idx, GFP_KERNEL);
> > +     if (ret < 0)
> > +             return ret;
> > +     idx = ret;
> 
> max_idx is always zero so idx is always zero.

There is a followup series adding support for max[1, max_vintf].
And I guess that would make Smatch happy. I'd personally prefer
keep this by ignoring the Smatch warning. But if you think the
common practice is to drop it and add back, I'd be okay with it.

> > +
> > +     vintf->idx = idx;
> > +     vintf->cmdqv = cmdqv;
> > +     vintf->base = cmdqv->base + TEGRA241_VINTF(idx);
> > +
> > +     vintf->lvcmdqs = kcalloc(cmdqv->num_lvcmdqs_per_vintf,
> > +                              sizeof(*vintf->lvcmdqs), GFP_KERNEL);
> > +     if (!vintf->lvcmdqs) {
> > +             ida_free(&cmdqv->vintf_ids, idx);
> > +             return -ENOMEM;
> > +     }
> > +
> > +     cmdqv->vintfs[idx] = vintf;
> 
> We only use the first element of this array.
> 
> > +     return ret;
> > +}
> 
> We could get rid of the ida_ stuff and change the cmdqv->vintfs[] array to a
> pointer.  It would simplify the code.

As mentioned above, a following series is adding other vintfs.
There is no warning/error to this array, I'd prefer we keep it
as is.

Thanks
Nicolin

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ