netdev - Re: [PATCH 1/5] initdev:kernel: Asynchronously-discovered devicesynchronization, v5

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:	Mon, 4 May 2009 17:31:23 -0700
From:	David VomLehn <dvomlehn@...co.com>
To:	Sergey Vlasov <vsu@...linux.ru>
Cc:	linux-kernel@...r.kernel.org, akpm@...ux-foundation.org,
	linux-usb@...r.kernel.org, greg@...ah.com,
	linux-scsi@...r.kernel.org, netdev@...r.kernel.org,
	arjan@...radead.org
Subject: Re: [PATCH 1/5] initdev:kernel: Asynchronously-discovered
	devicesynchronization, v5

On Sat, May 02, 2009 at 09:31:53AM -0400, Sergey Vlasov wrote:
> On Fri, May 01, 2009 at 07:25:51PM -0700, David VomLehn wrote:
> > supported on USB and SCII buses.
>                        ^^^^ typo

Yes.

> > +initdev_found(int initdev_mask)
> > +	This function must be called each time a possible init device device
> > +	is found.  It is passed a bit mask created by ORing any of the
> > +	following flags that apply to the device found:
> > +		BOOTDEV_CONSOLE_MASK
> > +		BOOTDEV_NETDEV_MASK
> > +		BOOTDEV_BLOCK_MASK
> 
> Should these also be renamed to INITDEV_* ?

Absolutely.

> > +initdev_type_found(int initdev_mask, enum initdev_type type)
> > +	If the type of a init device could not be found, but was determined
> > +	later, this function can be used to indicate the device type. It
> > +	is passed the mask that was originally passed to initdev_found.
> > +	Note that, if a device for which initdev_found is called is
> > +	subsequently determine not to be a possible init device, this function
> > +	should not be called. Instead, initdev_probe_done should be
> > +	called.
> 
> This function is never used in your patches - maybe drop it for now
> and introduce when a need for it arises?

Agreed.

> > diff --git a/drivers/base/initdev.c b/drivers/base/initdev.c
> > new file mode 100644
> > index 0000000..eaf53e3
> > --- /dev/null
> > +++ b/drivers/base/initdev.c
...
> > +__init int initdevs_init(void)
> > +{
> > +	int	i;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(initdev_info); i++) {
> > +		atomic_set(&initdev_info[i].pending, 0);
> > +		init_waitqueue_head(&initdev_info[i].wq);
> > +	}
> > +
> > +	return 0;
> 
> The return value is useless - it is never checked.

Yes, it should be a void function.

> > +#ifdef CONFIG_NETCONSOLE
> > +static int initdev_add_netconsole(int initdev_mask)
> > +{
> > +	return (initdev_mask & BOOTDEV_NETDEV_MASK) ?
> > +		initdev_mask | BOOTDEV_CONSOLE : initdev_mask;
> 
> This should be BOOTDEV_CONSOLE_MASK.

Ouch. Good catch.
> > +			if (atomic_dec_return(&initdev_info[i].pending) == 0)
...
> 
> May be replaced by a more commonly used atomic function:
> 
> 			if (atomic_dec_and_test(&initdev_info[i].pending))

Definitely better.
...
> What if type == BOOTDEV_NETDEV, and CONFIG_NETCONSOLE is defined?
> In this case we must keep both BOOTDEV_NETDEV and BOOTDEV_CONSOLE
> (otherwise the subsequent initdev_probe_done(BOOTDEV_NETDEV_MASK) will
> mess up the pending counter for BOOTDEV_CONSOLE).
> 
> Or, as noted above, just drop this function if is it not actually
> needed anywhere.

The function is gone. If it gets resurrected, we'll just have to deal with it.

> > +			if (atomic_dec_return(&initdev_info[i].pending) == 0)
> 
> 			if (atomic_dec_and_test(&initdev_info[i].pending))

Yes.
> > +void initdev_wait(enum initdev_type type, bool (*done)(void))
> 
> This function should probably be __init - there is no point to invoke
> it outside of the boot-time initialization code.

True.

> Unfortunately, the rest of code and data in this file (which will be
> useless after the boot-time initialization is complete) cannot be
> discarded, which will cause typical complaints that the kernel gets
> more and more bloated with each release.

And I'll be one of the complainers. Still, the one sure way to stop kernel
growth is to stop adding functionality and stop fixing bugs. I console
myself wth the observation Michael Opdenacker made at this year's
System Size BOF at this year's ELC: system size is growing at a slower rate
than Moore's Law. So, even though the kernel grows each year, the memory it
uses gets cheaper and cheaper.

Thanks for your comments!

David VomLehn

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html