linux-kernel - Re: [PATCH v2,5/5] drivers: uio: new driver for fsl_85xx_cache

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]

Message-ID: <f2a1f91f92c0fe4bce46c28222dea355d96e2090.camel@buserror.net>
Date:   Wed, 15 Apr 2020 14:27:51 -0500
From:   Scott Wood <oss@...error.net>
To:     Christophe Leroy <christophe.leroy@....fr>,
        Wang Wenhu <wenhu.wang@...o.com>, gregkh@...uxfoundation.org,
        linux-kernel@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org
Cc:     kernel@...o.com, Michael Ellerman <mpe@...erman.id.au>
Subject: Re: [PATCH v2,5/5] drivers: uio: new driver for fsl_85xx_cache_sram

On Wed, 2020-04-15 at 18:52 +0200, Christophe Leroy wrote:
> 
> Le 15/04/2020 à 17:24, Wang Wenhu a écrit :
> > +
> > +		if (uiomem >= &info->mem[MAX_UIO_MAPS]) {
> 
> I'd prefer
> 		if (uiomem - info->mem >= MAX_UIO_MAPS) {
> 
> > +			dev_warn(&pdev->dev, "more than %d uio-maps for
> > device.\n",
> > +				 MAX_UIO_MAPS);
> > +			break;
> > +		}
> > +	}
> > +
> > +	while (uiomem < &info->mem[MAX_UIO_MAPS]) {
> 
> I'd prefer
> 
> 	while (uiomem - info->mem < MAX_UIO_MAPS) {
> 

I wouldn't.  You're turning a simple comparison into a division and a
comparison (if the compiler doesn't optimize it back into the original form),
and making it less clear in the process.

Of course, working with array indices to begin with instead of incrementing a
pointer would be more idiomatic.

> > +		uiomem->size = 0;
> > +		++uiomem;
> > +	}
> > +
> > +	if (info->mem[0].size == 0) {
> 
> Is there any point in doing all the clearing loop above if it's to bail 
> out here ?
> 
> Wouldn't it be cleaner to do the test above the clearing loop, by just 
> checking whether uiomem is still equal to info->mem ?

There's no point doing the clearing at all, since the array was allocated with
kzalloc().

> > +		dev_err(&pdev->dev, "error no valid uio-map configured\n");
> > +		ret = -EINVAL;
> > +		goto err_info_free_internel;
> > +	}
> > +
> > +	info->version = "0.1.0";
> 
> Could you define some DRIVER_VERSION in the top of the file next to 
> DRIVER_NAME instead of hard coding in the middle on a function ?

That's what v1 had, and Greg KH said to remove it.  I'm guessing that he
thought it was the common-but-pointless practice of having the driver print a
version number that never gets updated, rather than something the UIO API
(unfortunately, compared to a feature query interface) expects.  That said,
I'm not sure what the value is of making it a macro since it should only be
used once, that use is self documenting, it isn't tunable, etc.  Though if
this isn't a macro, UIO_NAME also shouldn't be (and if it is made a macro
again, it should be UIO_VERSION, not DRIVER_VERSION).

Does this really need a three-part version scheme?  What's wrong with a
version of "1", to be changed to "2" in the hopefully-unlikely event that the
userspace API changes?  Assuming UIO is used for this at all, which doesn't
seem like a great fit to me.

-Scott