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: Tue, 16 Jan 2024 09:40:55 -0800
From: Brian Norris <briannorris@...omium.org>
To: kernel test robot <lkp@...el.com>,
	NĂ­colas F. R. A. Prado <nfraprado@...labora.com>
Cc: NĂ­colas F. R. A. Prado <nfraprado@...labora.com>,
	Tzung-Bi Shih <tzungbi@...nel.org>, llvm@...ts.linux.dev,
	oe-kbuild-all@...ts.linux.dev, kernel@...labora.com,
	AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
	Julius Werner <jwerner@...omium.org>,
	chrome-platform@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/4] firmware: google: cbmem: Add to module device table

Hi Nicolas,

On Mon, Jan 15, 2024 at 10:53:48AM +0800, kernel test robot wrote:
> All warnings (new ones prefixed by >>):
> 
> >> drivers/firmware/google/cbmem.c:118:40: warning: unused variable 'cbmem_ids' [-Wunused-const-variable]
>      118 | static const struct coreboot_device_id cbmem_ids[] = {
>          |                                        ^~~~~~~~~
>    1 warning generated.
> 
> 
> vim +/cbmem_ids +118 drivers/firmware/google/cbmem.c
> 
>    117	
>  > 118	static const struct coreboot_device_id cbmem_ids[] = {
>    119		{ .tag = LB_TAG_CBMEM_ENTRY },
>    120		{ /* sentinel */ }
>    121	};
>    122	MODULE_DEVICE_TABLE(coreboot, cbmem_ids);
>    123	

I was wondering why we have a seemingly unique "unused variable" failure
mode in comparison to other similarly-structured device/bus drivers, and
I realized that's because we're not relying on the same structure for
both MODULE_DEVICE_TABLE (struct coreboot_device_id) and for the driver
definition (struct coreboot_driver -> 'u32 tag'). Thus, this structure
is only used for #define MODULE builds, and otherwise not used.

Rather than wrapping these definitions in "#ifdef MODULE", perhaps we
can settle on a single field, and replace `struct coreboot_driver::tag`
with an instance of `struct coreboot_device_id`? That would normally be
a breaking change that would require changing all drivers at the same
time as the bus (or else some kind of intermediate transition state),
but considering there are only 4 driver implementations and they all
live under the same maintainer tree, that seems like it should still be
OK (IMO).

If it makes the series more readable/incremental, perhaps the switchover
can be the last patch in the series, and there can remain some
duplication (and potential -Wunused-const-variable issues) for the
middle of the series.

Brian

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ