Re: [PATCH v5 1/2] ACPI: Add early console framework for DBGP/DBG2.

[Konrad Rzeszutek Wilk <konrad@kernel.org>]

On Wed, Oct 10, 2012 at 01:31:54AM +0000, Zheng, Lv wrote:
> > > Signed-off-by: Lv Zheng <lv.zheng@intel.com>
> > > Reviewed-by: Len Brown <len.brown@intel.com>
> > > Reviewed-by: Rui Zhang <rui.zhang@intel.com>
> > > Reviewed-by: Ying Huang <ying.huang@intel.com>
> > > Reviewed-by: Konrad Rzeszutek Wilk <konrad@kernel.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.

The title of the patch does not have RFC anymore.. Please put it back
then.

> 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?

Correct. By the time you remove the 'RFC' part this should be the
default.
> 
> > > +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.

OK? How will this function get called three times then? I thought it
would just get called once?

> 
> > > +	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.

Well, you don't have to - not for this.
> 
> > > +		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".
> 
>