[PATCH v1] base: soc: Handle custom soc information sysfs entries

[PATCH v1] base: soc: Handle custom soc information sysfs entries

[Murali Nalajala]

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>
---
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      | 23 +++++++++++++++--------
 include/linux/sys_soc.h |  1 +
 2 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/drivers/base/soc.c b/drivers/base/soc.c
index 7c0c5ca..ad30d58 100644
--- a/drivers/base/soc.c
+++ b/drivers/base/soc.c
@@ -104,15 +104,11 @@ 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);
 
+	kfree(soc_dev->dev.groups);
 	kfree(soc_dev);
 }
 
@@ -121,6 +117,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 +133,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;
@@ -151,14 +156,16 @@ struct soc_device *soc_device_register(struct soc_device_attribute *soc_dev_attr
 
 	ret = device_register(&soc_dev->dev);
 	if (ret)
-		goto out3;
+		goto out4;
 
 	return soc_dev;
 
-out3:
+out4:
 	ida_simple_remove(&soc_ida, soc_dev->soc_dev_num);
 	put_device(&soc_dev->dev);
 	soc_dev = NULL;
+out3:
+	kfree(soc_attr_groups);
 out2:
 	kfree(soc_dev);
 out1:
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

Re: [PATCH v1] base: soc: Handle custom soc information sysfs entries

[Stephen Boyd]

Quoting Murali Nalajala (2019-10-03 16:51:50)
> @@ -151,14 +156,16 @@ struct soc_device *soc_device_register(struct soc_device_attribute *soc_dev_attr
>  
>         ret = device_register(&soc_dev->dev);
>         if (ret)
> -               goto out3;
> +               goto out4;
>  
>         return soc_dev;
>  
> -out3:
> +out4:
>         ida_simple_remove(&soc_ida, soc_dev->soc_dev_num);
>         put_device(&soc_dev->dev);
>         soc_dev = NULL;
> +out3:
> +       kfree(soc_attr_groups);

This code is tricky. put_device(&soc_dev->dev) will call soc_release()
so we set soc_dev to NULL before calling kfree() on the error path. This
way we don't doubly free a pointer that the release function will take
care of. I wonder if the release function could free the ida as well,
and then we could just make the device_register() failure path call
put_device() and return ERR_PTR(ret) directly. Then the error path is
simpler because we can avoid changing two pointers to NULL to avoid the
double free twice. Or just inline the ida remove and put_device() call
in the if and then goto out1 to consolidate the error pointer
conversion.

>  out2:
>         kfree(soc_dev);
>  out1:

Re: [PATCH v1] base: soc: Handle custom soc information sysfs entries

[Bjorn Andersson]

On Thu 03 Oct 22:38 PDT 2019, Stephen Boyd wrote:

> Quoting Murali Nalajala (2019-10-03 16:51:50)
> > @@ -151,14 +156,16 @@ struct soc_device *soc_device_register(struct soc_device_attribute *soc_dev_attr
> >  
> >         ret = device_register(&soc_dev->dev);
> >         if (ret)
> > -               goto out3;
> > +               goto out4;
> >  
> >         return soc_dev;
> >  
> > -out3:
> > +out4:
> >         ida_simple_remove(&soc_ida, soc_dev->soc_dev_num);
> >         put_device(&soc_dev->dev);
> >         soc_dev = NULL;
> > +out3:
> > +       kfree(soc_attr_groups);
> 
> This code is tricky. put_device(&soc_dev->dev) will call soc_release()
> so we set soc_dev to NULL before calling kfree() on the error path. This
> way we don't doubly free a pointer that the release function will take
> care of. I wonder if the release function could free the ida as well,
> and then we could just make the device_register() failure path call
> put_device() and return ERR_PTR(ret) directly. Then the error path is
> simpler because we can avoid changing two pointers to NULL to avoid the
> double free twice. Or just inline the ida remove and put_device() call
> in the if and then goto out1 to consolidate the error pointer
> conversion.
> 

But if we instead allocates the ida before the soc_dev, wouldn't the
error path be something like?:

foo:
	put_device(&soc_dev->dev);
bar:
	ida_simple_remove(&soc_ida, soc_num);
	return err;


I think we still need two exit paths from soc_device_register()
regardless of moving the ida_simple_remove() into the release, but we
could drop it from the unregister(). So not sure if this is cleaner...

Regards,
Bjorn

> >  out2:
> >         kfree(soc_dev);
> >  out1:

Re: [PATCH v1] base: soc: Handle custom soc information sysfs entries

[Stephen Boyd]

Quoting Bjorn Andersson (2019-10-03 22:50:57)
> On Thu 03 Oct 22:38 PDT 2019, Stephen Boyd wrote:
> 
> > Quoting Murali Nalajala (2019-10-03 16:51:50)
> > > @@ -151,14 +156,16 @@ struct soc_device *soc_device_register(struct soc_device_attribute *soc_dev_attr
> > >  
> > >         ret = device_register(&soc_dev->dev);
> > >         if (ret)
> > > -               goto out3;
> > > +               goto out4;
> > >  
> > >         return soc_dev;
> > >  
> > > -out3:
> > > +out4:
> > >         ida_simple_remove(&soc_ida, soc_dev->soc_dev_num);
> > >         put_device(&soc_dev->dev);
> > >         soc_dev = NULL;
> > > +out3:
> > > +       kfree(soc_attr_groups);
> > 
> > This code is tricky. put_device(&soc_dev->dev) will call soc_release()
> > so we set soc_dev to NULL before calling kfree() on the error path. This
> > way we don't doubly free a pointer that the release function will take
> > care of. I wonder if the release function could free the ida as well,
> > and then we could just make the device_register() failure path call
> > put_device() and return ERR_PTR(ret) directly. Then the error path is
> > simpler because we can avoid changing two pointers to NULL to avoid the
> > double free twice. Or just inline the ida remove and put_device() call
> > in the if and then goto out1 to consolidate the error pointer
> > conversion.
> > 
> 
> But if we instead allocates the ida before the soc_dev, wouldn't the
> error path be something like?:
> 
> foo:
>         put_device(&soc_dev->dev);
> bar:
>         ida_simple_remove(&soc_ida, soc_num);
>         return err;
> 
> 
> I think we still need two exit paths from soc_device_register()
> regardless of moving the ida_simple_remove() into the release, but we
> could drop it from the unregister(). So not sure if this is cleaner...
> 

It doesn't seem "safe" to let the number be reused before the device is
destroyed by put_device(). It would be clearer to do all the cleanup
from the release function so that the soc_device_unregister() path isn't
racy with another device being registered and reusing the same ID.

Of course this probably doesn't matter because the race I'm talking
about is extremely unlikely given there's only ever one soc device.
Reordering the put and remove would be fine too.

Re: [PATCH v1] base: soc: Handle custom soc information sysfs entries

[Greg KH]

On Thu, Oct 03, 2019 at 11:17:45PM -0700, Stephen Boyd wrote:
> Quoting Bjorn Andersson (2019-10-03 22:50:57)
> > On Thu 03 Oct 22:38 PDT 2019, Stephen Boyd wrote:
> > 
> > > Quoting Murali Nalajala (2019-10-03 16:51:50)
> > > > @@ -151,14 +156,16 @@ struct soc_device *soc_device_register(struct soc_device_attribute *soc_dev_attr
> > > >  
> > > >         ret = device_register(&soc_dev->dev);
> > > >         if (ret)
> > > > -               goto out3;
> > > > +               goto out4;
> > > >  
> > > >         return soc_dev;
> > > >  
> > > > -out3:
> > > > +out4:
> > > >         ida_simple_remove(&soc_ida, soc_dev->soc_dev_num);
> > > >         put_device(&soc_dev->dev);
> > > >         soc_dev = NULL;
> > > > +out3:
> > > > +       kfree(soc_attr_groups);
> > > 
> > > This code is tricky. put_device(&soc_dev->dev) will call soc_release()
> > > so we set soc_dev to NULL before calling kfree() on the error path. This
> > > way we don't doubly free a pointer that the release function will take
> > > care of. I wonder if the release function could free the ida as well,
> > > and then we could just make the device_register() failure path call
> > > put_device() and return ERR_PTR(ret) directly. Then the error path is
> > > simpler because we can avoid changing two pointers to NULL to avoid the
> > > double free twice. Or just inline the ida remove and put_device() call
> > > in the if and then goto out1 to consolidate the error pointer
> > > conversion.
> > > 
> > 
> > But if we instead allocates the ida before the soc_dev, wouldn't the
> > error path be something like?:
> > 
> > foo:
> >         put_device(&soc_dev->dev);
> > bar:
> >         ida_simple_remove(&soc_ida, soc_num);
> >         return err;
> > 
> > 
> > I think we still need two exit paths from soc_device_register()
> > regardless of moving the ida_simple_remove() into the release, but we
> > could drop it from the unregister(). So not sure if this is cleaner...
> > 
> 
> It doesn't seem "safe" to let the number be reused before the device is
> destroyed by put_device(). It would be clearer to do all the cleanup
> from the release function so that the soc_device_unregister() path isn't
> racy with another device being registered and reusing the same ID.
> 
> Of course this probably doesn't matter because the race I'm talking
> about is extremely unlikely given there's only ever one soc device.
> Reordering the put and remove would be fine too.

As the number is "owned" by the device, yes, it should just be removed
in the release function that frees the device memory, making this all
much simpler overall.

thanks,

greg k-h