[PATCH] powerpc/powernv: add hdat attribute to sysfs

[Andrew Donnellan]

On 23/02/17 13:29, Matt Brown wrote:
> From: Matt Brown <brownmatt1997 at gmail.com>

Did you intend for this to be different from the email you're using to 
send + sign-off?

>
> The HDAT data area is consumed by skiboot and turned into a device-tree.
> In some cases we would like to look directly at the HDAT, so this patch
> adds a sysfs node to allow it to be viewed.  This is not possible through
> /dev/mem as it is reserved memory which is stopped by the /dev/mem filter.
>
> Signed-off-by: Matt Brown <matthew.brown.dev at gmail.com>

Oliver obviously knows more about how HDAT actually works, I won't 
repeat his comments.

I haven't checked the actual important bit of how HDAT is exposed in 
device tree but other than that things look fairly good. Boring 
stylistic comments below!

> ---
>  arch/powerpc/include/asm/opal.h              |  1 +
>  arch/powerpc/platforms/powernv/opal-msglog.c | 49 ++++++++++++++++++++++++++++
>  arch/powerpc/platforms/powernv/opal.c        |  2 ++
>  3 files changed, 52 insertions(+)
>
> diff --git a/arch/powerpc/include/asm/opal.h b/arch/powerpc/include/asm/opal.h
> index 5c7db0f..b26944e 100644
> --- a/arch/powerpc/include/asm/opal.h
> +++ b/arch/powerpc/include/asm/opal.h
> @@ -277,6 +277,7 @@ extern int opal_async_comp_init(void);
>  extern int opal_sensor_init(void);
>  extern int opal_hmi_handler_init(void);
>  extern int opal_event_init(void);
> +extern void opal_hdat_sysfs_init(void);
>
>  extern int opal_machine_check(struct pt_regs *regs);
>  extern bool opal_mce_check_early_recovery(struct pt_regs *regs);
> diff --git a/arch/powerpc/platforms/powernv/opal-msglog.c b/arch/powerpc/platforms/powernv/opal-msglog.c
> index 39d6ff9..a637055 100644
> --- a/arch/powerpc/platforms/powernv/opal-msglog.c
> +++ b/arch/powerpc/platforms/powernv/opal-msglog.c

I think I know why you've put it in opal-msglog.c - the existing code in 
there does similar things in exposing a chunk of skiboot stuff via a 
sysfs file - but it's not immediately obvious given that HDAT and msglog 
are quite different things. Perhaps add a new file, opal-hdat.c?

> @@ -31,7 +31,13 @@ struct memcons {
>  	__be32 in_cons;
>  };
>
> +struct hdatInfo {

Kernel coding style tends to avoid using camelCase naming, instead 
preferring all-lowercase separated by underscores. (same applies to a 
couple of other places)

> +	char *base;
> +	u64 size;
> +};
> +
>  static struct memcons *opal_memcons = NULL;
> +static struct hdatInfo hdat_inf;
>
>  ssize_t opal_msglog_copy(char *to, loff_t pos, size_t count)
>  {
> @@ -136,3 +142,46 @@ void __init opal_msglog_sysfs_init(void)
>  	if (sysfs_create_bin_file(opal_kobj, &opal_msglog_attr) != 0)
>  		pr_warn("OPAL: sysfs file creation failed\n");
>  }
> +
> +
> +
> +/* Read function for HDAT attribute in sysfs */
> +static ssize_t hdat_read(struct file *file, struct kobject *kobj,
> +			 struct bin_attribute *bin_attr, char *to,
> +			 loff_t pos, size_t count)
> +{
> +	if (!hdat_inf.base)
> +		return -ENODEV;
> +
> +	return memory_read_from_buffer(to, count, &pos, hdat_inf.base,
> +					hdat_inf.size);
> +}
> +
> +
> +/* HDAT attribute for sysfs */
> +static struct bin_attribute hdat_attr = {
> +	.attr = {.name = "hdat", .mode = 0444},
> +	.read = hdat_read
> +};
> +
> +void __init opal_hdat_sysfs_init(void)
> +{
> +	u64 hdatAddr[2];
> +
> +	/* Check for the hdat-map prop in device-tree */
> +	if (of_property_read_u64_array(opal_node, "hdat-map", hdatAddr, 2)) {
> +		pr_debug("OPAL: Property hdat-map not found.\n");
> +		return;
> +	}
> +
> +	/* Print out hdat-map values. [0]: base, [1]: size */
> +	pr_debug("HDAT Base address: %#llx\n", hdatAddr[0]);
> +	pr_debug("HDAT Size: %#llx\n", hdatAddr[1]);

Prefix these with "OPAL: " like the other messages?

> +
> +	hdat_inf.base = phys_to_virt(hdatAddr[0]);
> +	hdat_inf.size = hdatAddr[1];
> +
> +	if (sysfs_create_bin_file(opal_kobj, &hdat_attr) != 0)
> +		pr_debug("OPAL: sysfs file creation for HDAT failed");
> +
> +}
> diff --git a/arch/powerpc/platforms/powernv/opal.c b/arch/powerpc/platforms/powernv/opal.c
> index 2822935..cae3745 100644
> --- a/arch/powerpc/platforms/powernv/opal.c
> +++ b/arch/powerpc/platforms/powernv/opal.c
> @@ -740,6 +740,8 @@ static int __init opal_init(void)
>  		opal_sys_param_init();
>  		/* Setup message log sysfs interface. */
>  		opal_msglog_sysfs_init();
> +		/* Create hdat object under sys/firmware/opal */
> +		opal_hdat_sysfs_init();
>  	}
>
>  	/* Initialize platform devices: IPMI backend, PRD & flash interface */
>

-- 
Andrew Donnellan              OzLabs, ADL Canberra
andrew.donnellan at au1.ibm.com  IBM Australia Limited