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:	Wed, 10 Oct 2012 01:31:54 +0000
From:	"Zheng, Lv" <lv.zheng@...el.com>
To:	Konrad Rzeszutek Wilk <konrad@...nel.org>
CC:	"Brown, Len" <len.brown@...el.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>,
	"H. Peter Anvin" <hpa@...or.com>,
	Jason Wessel <jason.wessel@...driver.com>,
	"Tang, Feng" <feng.tang@...el.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>,
	"x86@...nel.org" <x86@...nel.org>,
	"platform-driver-x86@...r.kernel.org" 
	<platform-driver-x86@...r.kernel.org>
Subject: RE: [PATCH v5 1/2] ACPI: Add early console framework for DBGP/DBG2.

> > Signed-off-by: Lv Zheng <lv.zheng@...el.com>
> > Reviewed-by: Len Brown <len.brown@...el.com>
> > Reviewed-by: Rui Zhang <rui.zhang@...el.com>
> > Reviewed-by: Ying Huang <ying.huang@...el.com>
> > Reviewed-by: Konrad Rzeszutek Wilk <konrad@...nel.org>
> Please don't include that unless I (or other folks looking at your code) say
> explicitly 'Acked' or 'Reviewed-by'

ACK.
I'll remove these names and resend.  Thanks.

> > +#define DEBUG
> That should not be the default case.

RFC.
If we do not add ignore_loglevel to the command line, the acpi earlycon will be mute just as what you want.
Do you really want this to be:
#undef DEBUG
If we do this, all pr_debug invocations in this file will be empty in compilation stage and are there any other means for us to view the output of ACPI earlycon without recompiling?

> > +DECLARE_BITMAP(acpi_early_flags, MAX_ACPI_DBG_PORTS);
> static?

ACK.
I'll do the modification.

> > +int acpi_early_enabled;
> __read_mostly and you could also make it a bool.

NAK.
I think this variable will be read only once and written up to 16 times so __read_mostly is not required.
I'll check and add __init and __initdata for this patch.

> > +	set_bit(port, acpi_early_flags);
> > +	if (keep)
> > +		set_bit(port+MAX_ACPI_DBG_PORTS, acpi_early_flags);
> Huh? The bitmap is up to MAX_ACPI_DB_PORTS, but here you offset it past
> that? Why?

ACK.
It's my mistake.  The size of the bitmap should be "MAX_ACPI_DBG_PORTS<<1 or MAX_ACPI_DBG_PORTS*2".
The reason is:
I think MAX_ACPI_DBG_PORTS=4 is enough in this case.
Since the systems running Linux are 32/64 bit architectures, using 2 bitmaps will be waste.
As the systems are at least 32 bit, the MAX_ACPI_DB_PORTS is defined as 16 to make full use of the bitmap.

> > +	if (!acpi_early_console_enabled(info->port_index))
> > +		return 0;
> Not -ENODEV?

NAK.
This is to support "MULTIPLE DBG2 debug ports".

** MULTIPLE DBGP table versions and MULTIPLE DBG2 debug ports support **
The whole patch takes ENODEV as the semantics of "table not exist or table version not supported" in PROBE/START stage so that we can obtain the ability of probing Microsoft's future tables in the order from DBGn, ..., DBG2, DBGP.
We should not return here as users may pass the following command line:
earlyprintk=acpi2,keep
to let the first 2 debug ports mute, but take the 3rd as a Linux earlycon.

> > +	while (((unsigned long)entry) + sizeof(struct acpi_dbg2_device) <
> > +	       tbl_end) {
> Just make it one line. Ignore the 80 characters limit here.

ACK.
I'll try to implement this within 80 characters.

> > +		if (!max_entries || count++ < max_entries) {
> How about you just make this 'count'
> > +			pr_debug("early: DBG2 PROBE - console %d(%04x:%04x).\n",
> > +				 count-1,
> > +				 entry->port_type, entry->port_subtype);
> > +			devinfo.port_index = (u8)count-1;
> Then you don't this 'count -1'
> and then do
> 		count++ here?

ACK.
It's my mistake, the previous version uses port_index=1 as the first debug port, but this version takes 0 as the first port.

> > +			acpi_early_console_start(&devinfo);
> no check of the return value to see whether you should return immediately?

NAK.
See " MULTIPLE DBGP tables and MULTIPLE DBG2 debug ports support" above.

> > +	acpi_early_console_start(&devinfo);
> how about 'return acpi_early_console_start(..)'

NAK.
See " MULTIPLE DBGP tables and MULTIPLE DBG2 debug ports support" above.

> > +	if (acpi_table_parse(ACPI_SIG_DBG2, acpi_parse_dbg2) != 0)
> > +		acpi_table_parse(ACPI_SIG_DBGP, acpi_parse_dbgp);

RFC.
This is to support "MULTIPLE DBGP table versions".

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists