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 <[email protected]>
---
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
Quoting Murali Nalajala (2019-10-07 13:37:42)
> 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 <[email protected]>
> ---
Reviewed-by: Stephen Boyd <[email protected]>
On Tue, Oct 08, 2019 at 08:33:11AM -0700, Stephen Boyd wrote:
> Quoting Murali Nalajala (2019-10-07 13:37:42)
> > 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 <[email protected]>
> > ---
>
> Reviewed-by: Stephen Boyd <[email protected]>
>
Nice, can we convert the existing soc drivers to use this interface
instead of the "export the device pointer" mess that they currently
have? That way we can drop that function entirely.
thanks,
greg k-h
On 2019-10-08 08:43, Greg KH wrote:
> On Tue, Oct 08, 2019 at 08:33:11AM -0700, Stephen Boyd wrote:
>> Quoting Murali Nalajala (2019-10-07 13:37:42)
>> > 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 <[email protected]>
>> > ---
>>
>> Reviewed-by: Stephen Boyd <[email protected]>
>>
>
> Nice, can we convert the existing soc drivers to use this interface
> instead of the "export the device pointer" mess that they currently
> have? That way we can drop that function entirely.
>
Thank you for the reviews.
In the current linux tree i can find these driver instances who is using
"soc_device_to_device" for populating their sysfs entries.
drivers/soc/ux500/ux500-soc-id.c: parent =
soc_device_to_device(soc_dev);
drivers/soc/tegra/fuse/fuse-tegra.c: return
soc_device_to_device(dev);
drivers/soc/amlogic/meson-gx-socinfo.c: dev =
soc_device_to_device(soc_dev);
drivers/soc/amlogic/meson-mx-socinfo.c:
dev_info(soc_device_to_device(soc_dev), "Amlogic %s %s detected\n",
drivers/soc/imx/soc-imx8.c: ret =
device_create_file(soc_device_to_device(soc_dev),
drivers/soc/imx/soc-imx-scu.c: ret =
device_create_file(soc_device_to_device(soc_dev),
drivers/soc/versatile/soc-realview.c:
device_create_file(soc_device_to_device(soc_dev), &realview_manf_attr);
drivers/soc/versatile/soc-realview.c:
device_create_file(soc_device_to_device(soc_dev), &realview_board_attr);
drivers/soc/versatile/soc-realview.c:
device_create_file(soc_device_to_device(soc_dev), &realview_arch_attr);
drivers/soc/versatile/soc-realview.c:
device_create_file(soc_device_to_device(soc_dev), &realview_build_attr);
drivers/soc/versatile/soc-integrator.c: dev =
soc_device_to_device(soc_dev);
These drivers can use the current proposed approach to expose their
sysfs entries.
Will try to address these and submit. But i can't able to test these
changes because i do not have these h/w's
> thanks,
>
> greg k-h
On Tue, Oct 08, 2019 at 10:10:08AM -0700, [email protected] wrote:
> On 2019-10-08 08:43, Greg KH wrote:
> > On Tue, Oct 08, 2019 at 08:33:11AM -0700, Stephen Boyd wrote:
> > > Quoting Murali Nalajala (2019-10-07 13:37:42)
> > > > 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 <[email protected]>
> > > > ---
> > >
> > > Reviewed-by: Stephen Boyd <[email protected]>
> > >
> >
> > Nice, can we convert the existing soc drivers to use this interface
> > instead of the "export the device pointer" mess that they currently
> > have? That way we can drop that function entirely.
> >
> Thank you for the reviews.
> In the current linux tree i can find these driver instances who is using
> "soc_device_to_device" for populating their sysfs entries.
>
> drivers/soc/ux500/ux500-soc-id.c: parent =
> soc_device_to_device(soc_dev);
> drivers/soc/tegra/fuse/fuse-tegra.c: return soc_device_to_device(dev);
> drivers/soc/amlogic/meson-gx-socinfo.c: dev = soc_device_to_device(soc_dev);
> drivers/soc/amlogic/meson-mx-socinfo.c:
> dev_info(soc_device_to_device(soc_dev), "Amlogic %s %s detected\n",
> drivers/soc/imx/soc-imx8.c: ret =
> device_create_file(soc_device_to_device(soc_dev),
> drivers/soc/imx/soc-imx-scu.c: ret =
> device_create_file(soc_device_to_device(soc_dev),
> drivers/soc/versatile/soc-realview.c:
> device_create_file(soc_device_to_device(soc_dev), &realview_manf_attr);
> drivers/soc/versatile/soc-realview.c:
> device_create_file(soc_device_to_device(soc_dev), &realview_board_attr);
> drivers/soc/versatile/soc-realview.c:
> device_create_file(soc_device_to_device(soc_dev), &realview_arch_attr);
> drivers/soc/versatile/soc-realview.c:
> device_create_file(soc_device_to_device(soc_dev), &realview_build_attr);
> drivers/soc/versatile/soc-integrator.c: dev = soc_device_to_device(soc_dev);
>
> These drivers can use the current proposed approach to expose their sysfs
> entries.
> Will try to address these and submit. But i can't able to test these changes
> because i do not have these h/w's
Build testing should be sufficient.
thanks,
greg k-h
On Tue 08 Oct 08:43 PDT 2019, Greg KH wrote:
> On Tue, Oct 08, 2019 at 08:33:11AM -0700, Stephen Boyd wrote:
> > Quoting Murali Nalajala (2019-10-07 13:37:42)
> > > 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 <[email protected]>
> > > ---
> >
> > Reviewed-by: Stephen Boyd <[email protected]>
> >
>
> Nice, can we convert the existing soc drivers to use this interface
> instead of the "export the device pointer" mess that they currently
> have? That way we can drop that function entirely.
>
Unfortunately we can't just drop it, because it's used on some SoCs as
the parent of all platform_devices.
But we can definitely get rid of all that uses it to create sysfs files.
Regards,
Bjorn
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 <[email protected]>
Reviewed-by: Bjorn Andersson <[email protected]>
> ---
> 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
>