Re: [PATCH v2 11/11] RAS: add DeviceTree firmware-first CPER provider

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

> >> +Firmware-first CPER via DeviceTree
> >> +----------------------------------
> >> +
> >> +Some systems expose Common Platform Error Record (CPER) data
> >> +via DeviceTree instead of ACPI HEST tables.  
> > 
> > I'd argue this isn't really DT specific, it's just not ACPI table.
> > You could for instance use PRP0001 and wire this up on ACPI with only
> > one trivial change to generic property.h accessor for the boolean.
> > 
> > Or use another firmware information source entirely.  
> 
> I'm intentionally keeping the scope DT-only for this series,
> so I'll keep the wording DT-focused.

Why?  Generally when the support works fine with generic firmware
accessors that's preferred because there are no real disadvantages.

> >> +#include <acpi/ghes.h>
> >> +#include <acpi/ghes_cper.h>
> >> +
> >> +static atomic_t ghes_ffh_source_ids = ATOMIC_INIT(0);  
> > I'd normally expect an IDA or similar. If nothing else it clearly
> > indicates we only want a unique ID.  
> 
> I'll keep the atomic for now; it's just a monotonic unique ID with no
> lifetime tracking. If you strongly prefer IDA I can switch.

If it doesn't 'need' to be monotonic due to some design issue then
yes I'd prefer an IDA.


> >> +	spinlock_t lock;
> >> +};  
> > 
> >   
> >> +
> >> +static void ghes_ffh_process(struct ghes_ffh *ctx)
> >> +{
> >> +	unsigned long flags;
> >> +	int sev;
> >> +
> >> +	spin_lock_irqsave(&ctx->lock, flags);  
> > 
> > guard() + include cleanup.h. Then can do returns in error paths.  
> 
> Agreed. I'll switch to guard() and include <linux/cleanup.h>.
A general process thing.  If you agree with a suggestion, just
do it and crop that section of the email thread out.
Reply that you agree tends not to benefit anyone!


> 
> >> +			ghes_estatus_cache_add(ctx->generic, ctx->estatus);
> >> +	}
> >> +
> >> +	ghes_cper_handle_status(ctx->dev, ctx->generic, ctx->estatus, ctx->sync);
> >> +
> >> +	ghes_ffh_ack(ctx);
> >> +
> >> +out:
> >> +	spin_unlock_irqrestore(&ctx->lock, flags);
> >> +}
...

> >> +{
> >> +	struct ghes_ffh *ctx;
> >> +	struct resource *res;
> >> +	int rc;
> >> +
> >> +	ctx = devm_kzalloc(&pdev->dev, sizeof(*ctx), GFP_KERNEL);
> >> +	if (!ctx)
> >> +		return -ENOMEM;
> >> +
> >> +	spin_lock_init(&ctx->lock);
> >> +	ctx->dev = &pdev->dev;
> >> +	ctx->sync = of_property_read_bool(pdev->dev.of_node, "arm,sea-notify");  
> > Hmm. I'd allow for other firmware types with
> > 	device_property_read_bool() instead.  
> 
> Given DT-only scope, I'll keep of_property_read_bool() here.
> 
> >> +
> >> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >> +	if (!res) {
> >> +		dev_err(&pdev->dev, "status region missing\n");  

Jonathan