Re: [PATCH v4 2/6] efi / ras: CCIX Cache error reporting

[Jonathan Cameron <jonathan.cameron@huawei.com>]

On Thu, 14 Nov 2019 06:38:46 +0100
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:

> Em Wed, 13 Nov 2019 23:16:23 +0800
> Jonathan Cameron <Jonathan.Cameron@huawei.com> escreveu:
> 
> > As CCIX Request Agents have fully cache coherent caches,
> > the CCIX 1.0 Base Specification defines detailed error
> > reporting for these caches.
> > 
> > A CCIX cache error is reported via a CPER record as defined in the
> > UEFI 2.8 specification. The PER log section is defined in the
> > CCIX 1.0 Base Specification.
> > 
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > ---
...

> > +static int cper_ccix_cache_err_details(const char *pfx,
> > +				     struct acpi_hest_generic_data *gdata)
> > +{
> > +	struct cper_ccix_cache_error *full_cache_err;
> > +	struct cper_sec_ccix_cache_error *cache_err;
> > +	u16 vendor_data_len;
> > +	int i;
> > +
> > +	if (gdata->error_data_length < sizeof(*full_cache_err))
> > +		return -ENOSPC;
> > +
> > +	full_cache_err = acpi_hest_get_payload(gdata);
> > +
> > +	cache_err = &full_cache_err->cache_record;
> > +
> > +	if (cache_err->validation_bits & CCIX_CACHE_ERR_TYPE_VALID)
> > +		printk("%s""Cache Type: %s\n", pfx,
> > +		       cper_ccix_cache_type_str(cache_err->cache_type));
> > +
> > +	if (cache_err->validation_bits & CCIX_CACHE_ERR_OP_VALID)
> > +		printk("%s""Operation: %s\n", pfx,
> > +		       cper_ccix_cache_err_op_str(cache_err->op_type));
> > +
> > +	if (cache_err->validation_bits & CCIX_CACHE_ERR_CACHE_ERR_TYPE_VALID)
> > +		printk("%s""Cache Error Type: %s\n", pfx,
> > +		       cper_ccix_cache_err_type_str(cache_err->cache_error_type));
> > +
> > +	if (cache_err->validation_bits & CCIX_CACHE_ERR_LEVEL_VALID)
> > +		printk("%s""Level: %d\n", pfx, cache_err->cache_level);
> > +
> > +	if (cache_err->validation_bits & CCIX_CACHE_ERR_SET_VALID)
> > +		printk("%s""Set: %d\n", pfx, cache_err->set);
> > +
> > +	if (cache_err->validation_bits & CCIX_CACHE_ERR_WAY_VALID)
> > +		printk("%s""Way: %d\n", pfx, cache_err->way);
> > +
> > +	if (cache_err->validation_bits & CCIX_CACHE_ERR_INSTANCE_ID_VALID)
> > +		printk("%s""Instance ID: %d\n", pfx, cache_err->instance);
> > +
> > +	if (cache_err->validation_bits & CCIX_CACHE_ERR_VENDOR_DATA_VALID) {
> > +		if (gdata->error_data_length < sizeof(*full_cache_err) + 4)
> > +			return -ENOSPC;
> > +
> > +		vendor_data_len = cache_err->vendor_data[0] & GENMASK(15, 0);
> > +		if (gdata->error_data_length <
> > +		    sizeof(*full_cache_err) + vendor_data_len)
> > +			return -ENOSPC;
> > +
> > +		for (i = 0; i < vendor_data_len / 4 - 1; i++)
> > +			printk("%s""Vendor%d: 0x%08x\n", pfx, i,
> > +			       cache_err->vendor_data[i + 1]);  
> 
> I forgot to comment this at patch 1/6, as this is more a reflection
> than asking for a change...
> 
> Not sure what's the value of also printing events to the Kernel logs.
> 
> I mean, we do that for existent RAS drivers, mainly because the RAS report
> mechanism came after the printks, and someone could be relying at the
> kernel logs instead of using rasdaemon (or some other alternative software
> someone might write).
> 
> For new report mechanisms, perhaps we could be smarter - at least offering
> ways to disable the printks if a daemon is listening to the trace events.
> 
> Boris/Tony: what do you think?
> 

Indeed, seems like a sensible time to make such a change if people agree
it makes sense to do so.
I'll leave this for now and get a v5 out with the fixes you mention.

> > +	}
> > +
> > +	return 0;
> > +}
> > +
....
> 
> 
> Cheers,
> Mauro