Re: [PATCH] mce-apei: do not rely on ACPI_ERST_GET_RECORD_ID for record id

["Luck, Tony" <tony.luck@intel.com>]

>  retry:
> -	rc = erst_get_record_id_next(&pos, record_id);
> -	if (rc)
> -		goto out;
> +	/*
> +	 * Some hardware is broken and doesn't actually advance the record id

I'd blame this on firmware rather than hardware.

> +	 * returned by ACPI_ERST_GET_RECORD_ID when we read a record like the
> +	 * spec says it is supposed to.  So instead use record_id == 0 to just
> +	 * grab the first record in the erst, and fall back only if we trip over
> +	 * a record that isn't a MCE record.
> +	 */
> +	if (lookup_record) {
> +		rc = erst_get_record_id_next(&pos, record_id);
> +		if (rc)
> +			goto out;
> +	} else {
> +		*record_id = 0;
> +	}
>  	/* no more record */
>  	if (*record_id == APEI_ERST_INVALID_RECORD_ID)
>  		goto out;
>  	rc = erst_read(*record_id, &rcd.hdr, sizeof(rcd));
> -	/* someone else has cleared the record, try next one */
> -	if (rc == -ENOENT)
> -		goto retry;
> -	else if (rc < 0)
> +	/*
> +	 * someone else has cleared the record, try next one if we are looking
> +	 * up records.  If we aren't looking up the record id's then just bail
> +	 * since this means we have an empty table.
> +	 */
> +	if (rc == -ENOENT) {
> +		if (lookup_record)
> +			goto retry;
> +		rc = 0;
> +		goto out;
> +	} else if (rc < 0) {
>  		goto out;
> -	/* try to skip other type records in storage */
> -	else if (rc != sizeof(rcd) ||
> -		 uuid_le_cmp(rcd.hdr.creator_id, CPER_CREATOR_MCE))
> +	} else if (rc != sizeof(rcd) ||
> +		 uuid_le_cmp(rcd.hdr.creator_id, CPER_CREATOR_MCE)) {
> +		/* try to skip other type records in storage */
> +		lookup_record = true;
>  		goto retry;

Are you still doomed by the buggy firmware if we take this "goto"?
You be back at the top of the loop excpecting erst_get_record_id_next()
to move on.  Does this just not happen in practice (finding non MCE
records in amognst the MCE ones)?

> +	}
> +	/* Use the record header as the source of truth for the record id. */
> +	*record_id = rcd.hdr.record_id;
>  	memcpy(m, &rcd.mce, sizeof(*m));
>  	rc = sizeof(*m);

-Tony