Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 8 Oct 2019 11:06:43 -0700
From:   Bjorn Andersson <bjorn.andersson@linaro.org>
To:     Murali Nalajala <mnalajal@codeaurora.org>
Cc:     gregkh@linuxfoundation.org, rafael@kernel.org, swboyd@chromium.org,
        linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] base: soc: Handle custom soc information sysfs entries
Message-ID: <20191008180643.GJ63675@minitux>
References: <1570480662-25252-1-git-send-email-mnalajal@codeaurora.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1570480662-25252-1-git-send-email-mnalajal@codeaurora.org>
User-Agent: Mutt/1.12.1 (2019-06-15)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon 07 Oct 13:37 PDT 2019, Murali Nalajala wrote:

> Soc framework exposed sysfs entries are not sufficient for some
> of the h/w platforms. Currently there is no interface where soc
> drivers can expose further information about their SoCs via soc
> framework. This change address this limitation where clients can
> pass their custom entries as attribute group and soc framework
> would expose them as sysfs properties.
> 
> Signed-off-by: Murali Nalajala <mnalajal@codeaurora.org>

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

> ---
> Changes in v2:
> - Address comments from Stephen Boyd about "soc_dev" clean up in error paths.
> 
> Changes in v1:
> - Remove NULL initialization of "soc_attr_groups"
> - Taken care of freeing "soc_attr_groups" in soc_release()
> - Addressed Stephen Boyd comments on usage of "kalloc"
> 
>  drivers/base/soc.c      | 30 +++++++++++++++++-------------
>  include/linux/sys_soc.h |  1 +
>  2 files changed, 18 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/base/soc.c b/drivers/base/soc.c
> index 7c0c5ca..4af11a4 100644
> --- a/drivers/base/soc.c
> +++ b/drivers/base/soc.c
> @@ -104,15 +104,12 @@ static ssize_t soc_info_get(struct device *dev,
>  	.is_visible = soc_attribute_mode,
>  };
>  
> -static const struct attribute_group *soc_attr_groups[] = {
> -	&soc_attr_group,
> -	NULL,
> -};
> -
>  static void soc_release(struct device *dev)
>  {
>  	struct soc_device *soc_dev = container_of(dev, struct soc_device, dev);
>  
> +	ida_simple_remove(&soc_ida, soc_dev->soc_dev_num);
> +	kfree(soc_dev->dev.groups);
>  	kfree(soc_dev);
>  }
>  
> @@ -121,6 +118,7 @@ static void soc_release(struct device *dev)
>  struct soc_device *soc_device_register(struct soc_device_attribute *soc_dev_attr)
>  {
>  	struct soc_device *soc_dev;
> +	const struct attribute_group **soc_attr_groups;
>  	int ret;
>  
>  	if (!soc_bus_type.p) {
> @@ -136,10 +134,18 @@ struct soc_device *soc_device_register(struct soc_device_attribute *soc_dev_attr
>  		goto out1;
>  	}
>  
> +	soc_attr_groups = kcalloc(3, sizeof(*soc_attr_groups), GFP_KERNEL);
> +	if (!soc_attr_groups) {
> +		ret = -ENOMEM;
> +		goto out2;
> +	}
> +	soc_attr_groups[0] = &soc_attr_group;
> +	soc_attr_groups[1] = soc_dev_attr->custom_attr_group;
> +
>  	/* Fetch a unique (reclaimable) SOC ID. */
>  	ret = ida_simple_get(&soc_ida, 0, 0, GFP_KERNEL);
>  	if (ret < 0)
> -		goto out2;
> +		goto out3;
>  	soc_dev->soc_dev_num = ret;
>  
>  	soc_dev->attr = soc_dev_attr;
> @@ -150,15 +156,15 @@ struct soc_device *soc_device_register(struct soc_device_attribute *soc_dev_attr
>  	dev_set_name(&soc_dev->dev, "soc%d", soc_dev->soc_dev_num);
>  
>  	ret = device_register(&soc_dev->dev);
> -	if (ret)
> -		goto out3;
> +	if (ret) {
> +		put_device(&soc_dev->dev);
> +		return ERR_PTR(ret);
> +	}
>  
>  	return soc_dev;
>  
>  out3:
> -	ida_simple_remove(&soc_ida, soc_dev->soc_dev_num);
> -	put_device(&soc_dev->dev);
> -	soc_dev = NULL;
> +	kfree(soc_attr_groups);
>  out2:
>  	kfree(soc_dev);
>  out1:
> @@ -169,8 +175,6 @@ struct soc_device *soc_device_register(struct soc_device_attribute *soc_dev_attr
>  /* Ensure soc_dev->attr is freed prior to calling soc_device_unregister. */
>  void soc_device_unregister(struct soc_device *soc_dev)
>  {
> -	ida_simple_remove(&soc_ida, soc_dev->soc_dev_num);
> -
>  	device_unregister(&soc_dev->dev);
>  	early_soc_dev_attr = NULL;
>  }
> diff --git a/include/linux/sys_soc.h b/include/linux/sys_soc.h
> index 48ceea8..d9b3cf0 100644
> --- a/include/linux/sys_soc.h
> +++ b/include/linux/sys_soc.h
> @@ -15,6 +15,7 @@ struct soc_device_attribute {
>  	const char *serial_number;
>  	const char *soc_id;
>  	const void *data;
> +	const struct attribute_group *custom_attr_group;
>  };
>  
>  /**
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>