[PATCH] base: soc: Export soc_device_to_device API

[PATCH] base: soc: Export soc_device_to_device API

[Murali Nalajala]

If the soc drivers want to add custom sysfs entries it needs to
access "dev" field in "struct soc_device". This can be achieved
by "soc_device_to_device" API. Soc drivers which are built as a
module they need above API to be exported. Otherwise one can
observe compilation issues.

Signed-off-by: Murali Nalajala <mnalajal@codeaurora.org>
---
 drivers/base/soc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/base/soc.c b/drivers/base/soc.c
index 7c0c5ca..4ad52f6 100644
--- a/drivers/base/soc.c
+++ b/drivers/base/soc.c
@@ -41,6 +41,7 @@ struct device *soc_device_to_device(struct soc_device *soc_dev)
 {
 	return &soc_dev->dev;
 }
+EXPORT_SYMBOL_GPL(soc_device_to_device);
 
 static umode_t soc_attribute_mode(struct kobject *kobj,
 				struct attribute *attr,
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] base: soc: Export soc_device_to_device API

[Greg KH]

On Thu, Sep 19, 2019 at 02:13:44PM -0700, Murali Nalajala wrote:
> If the soc drivers want to add custom sysfs entries it needs to
> access "dev" field in "struct soc_device". This can be achieved
> by "soc_device_to_device" API. Soc drivers which are built as a
> module they need above API to be exported. Otherwise one can
> observe compilation issues.
> 
> Signed-off-by: Murali Nalajala <mnalajal@codeaurora.org>
> ---
>  drivers/base/soc.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/base/soc.c b/drivers/base/soc.c
> index 7c0c5ca..4ad52f6 100644
> --- a/drivers/base/soc.c
> +++ b/drivers/base/soc.c
> @@ -41,6 +41,7 @@ struct device *soc_device_to_device(struct soc_device *soc_dev)
>  {
>  	return &soc_dev->dev;
>  }
> +EXPORT_SYMBOL_GPL(soc_device_to_device);
>  
>  static umode_t soc_attribute_mode(struct kobject *kobj,
>  				struct attribute *attr,

What in-kernel driver needs this?

Is linux-next breaking without this?

We don't export things unless we have a user of the export.

Also, adding "custom" sysfs attributes is almost always not the correct
thing to do at all.  The driver should be doing it, by setting up the
attribute group properly so that the driver core can do it automatically
for it.

No driver should be doing individual add/remove of sysfs files.  If it
does so, it is almost guaranteed to be doing it incorrectly and racing
userspace.

And yes, there's loads of in-kernel examples of doing this wrong, I've
been working on fixing that up, look at the patches now in Linus's tree
for platform and USB drivers that do this as examples of how to do it
right.

thanks,

greg k-h

Re: [PATCH] base: soc: Export soc_device_to_device API

[Bjorn Andersson]

On Thu 19 Sep 14:32 PDT 2019, Greg KH wrote:

> On Thu, Sep 19, 2019 at 02:13:44PM -0700, Murali Nalajala wrote:
> > If the soc drivers want to add custom sysfs entries it needs to
> > access "dev" field in "struct soc_device". This can be achieved
> > by "soc_device_to_device" API. Soc drivers which are built as a
> > module they need above API to be exported. Otherwise one can
> > observe compilation issues.
> > 
> > Signed-off-by: Murali Nalajala <mnalajal@codeaurora.org>
> > ---
> >  drivers/base/soc.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/base/soc.c b/drivers/base/soc.c
> > index 7c0c5ca..4ad52f6 100644
> > --- a/drivers/base/soc.c
> > +++ b/drivers/base/soc.c
> > @@ -41,6 +41,7 @@ struct device *soc_device_to_device(struct soc_device *soc_dev)
> >  {
> >  	return &soc_dev->dev;
> >  }
> > +EXPORT_SYMBOL_GPL(soc_device_to_device);
> >  
> >  static umode_t soc_attribute_mode(struct kobject *kobj,
> >  				struct attribute *attr,
> 
> What in-kernel driver needs this?
> 

Half of the drivers interacting with the soc driver calls this API,
several of these I see no reason for being builtin (e.g.
ux500 andversatile). So I think this patch makes sense to allow us to
build these as modules.

> Is linux-next breaking without this?
> 

No, we postponed the addition of any sysfs attributes in the Qualcomm
socinfo driver.

> We don't export things unless we have a user of the export.
> 
> Also, adding "custom" sysfs attributes is almost always not the correct
> thing to do at all.  The driver should be doing it, by setting up the
> attribute group properly so that the driver core can do it automatically
> for it.
> 
> No driver should be doing individual add/remove of sysfs files.  If it
> does so, it is almost guaranteed to be doing it incorrectly and racing
> userspace.
> 

The problem here is that the attributes are expected to be attached to
the soc driver, which is separate from the platform-specific drivers. So
there's no way to do platform specific attributes the right way.

> And yes, there's loads of in-kernel examples of doing this wrong, I've
> been working on fixing that up, look at the patches now in Linus's tree
> for platform and USB drivers that do this as examples of how to do it
> right.
> 

Agreed, this patch should not be used as an approval for any crazy
attributes; but it's necessary in order to extend the soc device's
attributes, per the current design.

So:

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

Regards,
Bjorn

Re: [PATCH] base: soc: Export soc_device_to_device API

[Greg KH]

On Thu, Sep 19, 2019 at 02:53:00PM -0700, Bjorn Andersson wrote:
> On Thu 19 Sep 14:32 PDT 2019, Greg KH wrote:
> 
> > On Thu, Sep 19, 2019 at 02:13:44PM -0700, Murali Nalajala wrote:
> > > If the soc drivers want to add custom sysfs entries it needs to
> > > access "dev" field in "struct soc_device". This can be achieved
> > > by "soc_device_to_device" API. Soc drivers which are built as a
> > > module they need above API to be exported. Otherwise one can
> > > observe compilation issues.
> > > 
> > > Signed-off-by: Murali Nalajala <mnalajal@codeaurora.org>
> > > ---
> > >  drivers/base/soc.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/drivers/base/soc.c b/drivers/base/soc.c
> > > index 7c0c5ca..4ad52f6 100644
> > > --- a/drivers/base/soc.c
> > > +++ b/drivers/base/soc.c
> > > @@ -41,6 +41,7 @@ struct device *soc_device_to_device(struct soc_device *soc_dev)
> > >  {
> > >  	return &soc_dev->dev;
> > >  }
> > > +EXPORT_SYMBOL_GPL(soc_device_to_device);
> > >  
> > >  static umode_t soc_attribute_mode(struct kobject *kobj,
> > >  				struct attribute *attr,
> > 
> > What in-kernel driver needs this?
> > 
> 
> Half of the drivers interacting with the soc driver calls this API,
> several of these I see no reason for being builtin (e.g.
> ux500 andversatile). So I think this patch makes sense to allow us to
> build these as modules.
> 
> > Is linux-next breaking without this?
> > 
> 
> No, we postponed the addition of any sysfs attributes in the Qualcomm
> socinfo driver.
> 
> > We don't export things unless we have a user of the export.
> > 
> > Also, adding "custom" sysfs attributes is almost always not the correct
> > thing to do at all.  The driver should be doing it, by setting up the
> > attribute group properly so that the driver core can do it automatically
> > for it.
> > 
> > No driver should be doing individual add/remove of sysfs files.  If it
> > does so, it is almost guaranteed to be doing it incorrectly and racing
> > userspace.
> > 
> 
> The problem here is that the attributes are expected to be attached to
> the soc driver, which is separate from the platform-specific drivers. So
> there's no way to do platform specific attributes the right way.
> 
> > And yes, there's loads of in-kernel examples of doing this wrong, I've
> > been working on fixing that up, look at the patches now in Linus's tree
> > for platform and USB drivers that do this as examples of how to do it
> > right.
> > 
> 
> Agreed, this patch should not be used as an approval for any crazy
> attributes; but it's necessary in order to extend the soc device's
> attributes, per the current design.

Wait, no, let's not let the "current design" remain if it is broken!

Why can't the soc driver handle the attributes properly so that the
individual driver doesn't have to do the create/remove?

thanks,

greg k-h

Re: [PATCH] base: soc: Export soc_device_to_device API

[Bjorn Andersson]

On Thu 19 Sep 14:58 PDT 2019, Greg KH wrote:

> On Thu, Sep 19, 2019 at 02:53:00PM -0700, Bjorn Andersson wrote:
> > On Thu 19 Sep 14:32 PDT 2019, Greg KH wrote:
> > 
> > > On Thu, Sep 19, 2019 at 02:13:44PM -0700, Murali Nalajala wrote:
> > > > If the soc drivers want to add custom sysfs entries it needs to
> > > > access "dev" field in "struct soc_device". This can be achieved
> > > > by "soc_device_to_device" API. Soc drivers which are built as a
> > > > module they need above API to be exported. Otherwise one can
> > > > observe compilation issues.
> > > > 
> > > > Signed-off-by: Murali Nalajala <mnalajal@codeaurora.org>
> > > > ---
> > > >  drivers/base/soc.c | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > > 
> > > > diff --git a/drivers/base/soc.c b/drivers/base/soc.c
> > > > index 7c0c5ca..4ad52f6 100644
> > > > --- a/drivers/base/soc.c
> > > > +++ b/drivers/base/soc.c
> > > > @@ -41,6 +41,7 @@ struct device *soc_device_to_device(struct soc_device *soc_dev)
> > > >  {
> > > >  	return &soc_dev->dev;
> > > >  }
> > > > +EXPORT_SYMBOL_GPL(soc_device_to_device);
> > > >  
> > > >  static umode_t soc_attribute_mode(struct kobject *kobj,
> > > >  				struct attribute *attr,
> > > 
> > > What in-kernel driver needs this?
> > > 
> > 
> > Half of the drivers interacting with the soc driver calls this API,
> > several of these I see no reason for being builtin (e.g.
> > ux500 andversatile). So I think this patch makes sense to allow us to
> > build these as modules.
> > 
> > > Is linux-next breaking without this?
> > > 
> > 
> > No, we postponed the addition of any sysfs attributes in the Qualcomm
> > socinfo driver.
> > 
> > > We don't export things unless we have a user of the export.
> > > 
> > > Also, adding "custom" sysfs attributes is almost always not the correct
> > > thing to do at all.  The driver should be doing it, by setting up the
> > > attribute group properly so that the driver core can do it automatically
> > > for it.
> > > 
> > > No driver should be doing individual add/remove of sysfs files.  If it
> > > does so, it is almost guaranteed to be doing it incorrectly and racing
> > > userspace.
> > > 
> > 
> > The problem here is that the attributes are expected to be attached to
> > the soc driver, which is separate from the platform-specific drivers. So
> > there's no way to do platform specific attributes the right way.
> > 
> > > And yes, there's loads of in-kernel examples of doing this wrong, I've
> > > been working on fixing that up, look at the patches now in Linus's tree
> > > for platform and USB drivers that do this as examples of how to do it
> > > right.
> > > 
> > 
> > Agreed, this patch should not be used as an approval for any crazy
> > attributes; but it's necessary in order to extend the soc device's
> > attributes, per the current design.
> 
> Wait, no, let's not let the "current design" remain if it is broken!
> 
> Why can't the soc driver handle the attributes properly so that the
> individual driver doesn't have to do the create/remove?
> 

The custom attributes that these drivers want to add to the common ones
are known in advance, so I presume we could have them passed into
soc_device_register() and registered together with the common
attributes...

It sounds like it's worth a prototype.

Regards,
Bjorn

Re: [PATCH] base: soc: Export soc_device_to_device API

[Greg KH]

On Thu, Sep 19, 2019 at 03:14:56PM -0700, Bjorn Andersson wrote:
> On Thu 19 Sep 14:58 PDT 2019, Greg KH wrote:
> 
> > On Thu, Sep 19, 2019 at 02:53:00PM -0700, Bjorn Andersson wrote:
> > > On Thu 19 Sep 14:32 PDT 2019, Greg KH wrote:
> > > 
> > > > On Thu, Sep 19, 2019 at 02:13:44PM -0700, Murali Nalajala wrote:
> > > > > If the soc drivers want to add custom sysfs entries it needs to
> > > > > access "dev" field in "struct soc_device". This can be achieved
> > > > > by "soc_device_to_device" API. Soc drivers which are built as a
> > > > > module they need above API to be exported. Otherwise one can
> > > > > observe compilation issues.
> > > > > 
> > > > > Signed-off-by: Murali Nalajala <mnalajal@codeaurora.org>
> > > > > ---
> > > > >  drivers/base/soc.c | 1 +
> > > > >  1 file changed, 1 insertion(+)
> > > > > 
> > > > > diff --git a/drivers/base/soc.c b/drivers/base/soc.c
> > > > > index 7c0c5ca..4ad52f6 100644
> > > > > --- a/drivers/base/soc.c
> > > > > +++ b/drivers/base/soc.c
> > > > > @@ -41,6 +41,7 @@ struct device *soc_device_to_device(struct soc_device *soc_dev)
> > > > >  {
> > > > >  	return &soc_dev->dev;
> > > > >  }
> > > > > +EXPORT_SYMBOL_GPL(soc_device_to_device);
> > > > >  
> > > > >  static umode_t soc_attribute_mode(struct kobject *kobj,
> > > > >  				struct attribute *attr,
> > > > 
> > > > What in-kernel driver needs this?
> > > > 
> > > 
> > > Half of the drivers interacting with the soc driver calls this API,
> > > several of these I see no reason for being builtin (e.g.
> > > ux500 andversatile). So I think this patch makes sense to allow us to
> > > build these as modules.
> > > 
> > > > Is linux-next breaking without this?
> > > > 
> > > 
> > > No, we postponed the addition of any sysfs attributes in the Qualcomm
> > > socinfo driver.
> > > 
> > > > We don't export things unless we have a user of the export.
> > > > 
> > > > Also, adding "custom" sysfs attributes is almost always not the correct
> > > > thing to do at all.  The driver should be doing it, by setting up the
> > > > attribute group properly so that the driver core can do it automatically
> > > > for it.
> > > > 
> > > > No driver should be doing individual add/remove of sysfs files.  If it
> > > > does so, it is almost guaranteed to be doing it incorrectly and racing
> > > > userspace.
> > > > 
> > > 
> > > The problem here is that the attributes are expected to be attached to
> > > the soc driver, which is separate from the platform-specific drivers. So
> > > there's no way to do platform specific attributes the right way.
> > > 
> > > > And yes, there's loads of in-kernel examples of doing this wrong, I've
> > > > been working on fixing that up, look at the patches now in Linus's tree
> > > > for platform and USB drivers that do this as examples of how to do it
> > > > right.
> > > > 
> > > 
> > > Agreed, this patch should not be used as an approval for any crazy
> > > attributes; but it's necessary in order to extend the soc device's
> > > attributes, per the current design.
> > 
> > Wait, no, let's not let the "current design" remain if it is broken!
> > 
> > Why can't the soc driver handle the attributes properly so that the
> > individual driver doesn't have to do the create/remove?
> > 
> 
> The custom attributes that these drivers want to add to the common ones
> are known in advance, so I presume we could have them passed into
> soc_device_register() and registered together with the common
> attributes...
> 
> It sounds like it's worth a prototype.

Do you have an in-kernel example I can look at to get an idea of what is
needed here?

thanks,

greg k-h

Re: [PATCH] base: soc: Export soc_device_to_device API

[mnalajal]

On 2019-09-19 14:58, Greg KH wrote:
> On Thu, Sep 19, 2019 at 02:53:00PM -0700, Bjorn Andersson wrote:
>> On Thu 19 Sep 14:32 PDT 2019, Greg KH wrote:
>> 
>> > On Thu, Sep 19, 2019 at 02:13:44PM -0700, Murali Nalajala wrote:
>> > > If the soc drivers want to add custom sysfs entries it needs to
>> > > access "dev" field in "struct soc_device". This can be achieved
>> > > by "soc_device_to_device" API. Soc drivers which are built as a
>> > > module they need above API to be exported. Otherwise one can
>> > > observe compilation issues.
>> > >
>> > > Signed-off-by: Murali Nalajala <mnalajal@codeaurora.org>
>> > > ---
>> > >  drivers/base/soc.c | 1 +
>> > >  1 file changed, 1 insertion(+)
>> > >
>> > > diff --git a/drivers/base/soc.c b/drivers/base/soc.c
>> > > index 7c0c5ca..4ad52f6 100644
>> > > --- a/drivers/base/soc.c
>> > > +++ b/drivers/base/soc.c
>> > > @@ -41,6 +41,7 @@ struct device *soc_device_to_device(struct soc_device *soc_dev)
>> > >  {
>> > >  	return &soc_dev->dev;
>> > >  }
>> > > +EXPORT_SYMBOL_GPL(soc_device_to_device);
>> > >
>> > >  static umode_t soc_attribute_mode(struct kobject *kobj,
>> > >  				struct attribute *attr,
>> >
>> > What in-kernel driver needs this?
>> >
>> 
>> Half of the drivers interacting with the soc driver calls this API,
>> several of these I see no reason for being builtin (e.g.
>> ux500 andversatile). So I think this patch makes sense to allow us to
>> build these as modules.
>> 
>> > Is linux-next breaking without this?
>> >
>> 
>> No, we postponed the addition of any sysfs attributes in the Qualcomm
>> socinfo driver.
>> 
>> > We don't export things unless we have a user of the export.
>> >
>> > Also, adding "custom" sysfs attributes is almost always not the correct
>> > thing to do at all.  The driver should be doing it, by setting up the
>> > attribute group properly so that the driver core can do it automatically
>> > for it.
>> >
>> > No driver should be doing individual add/remove of sysfs files.  If it
>> > does so, it is almost guaranteed to be doing it incorrectly and racing
>> > userspace.
>> >
>> 
>> The problem here is that the attributes are expected to be attached to
>> the soc driver, which is separate from the platform-specific drivers. 
>> So
>> there's no way to do platform specific attributes the right way.
>> 
>> > And yes, there's loads of in-kernel examples of doing this wrong, I've
>> > been working on fixing that up, look at the patches now in Linus's tree
>> > for platform and USB drivers that do this as examples of how to do it
>> > right.
>> >
>> 
>> Agreed, this patch should not be used as an approval for any crazy
>> attributes; but it's necessary in order to extend the soc device's
>> attributes, per the current design.
> 
> Wait, no, let's not let the "current design" remain if it is broken!
> 
> Why can't the soc driver handle the attributes properly so that the
> individual driver doesn't have to do the create/remove?
> 
> thanks,
> 
> greg k-h

Current soc framework is allowing the soc info drivers to create very 
limited
soc information as sysfs entries like machine name, family, revision and 
soc_id.
Sometimes it become a limited information for the clients to know more 
about soc
information. In this scenario soc info drivers are adding their own 
sysfs entries
on top of what soc framework is doing like hw_platform, 
hw_platform_subtype etc.
I believe this will be an issue for the other soc vendors as well. It is 
good that
if we could add these into the soc framework rather than individual 
clients defining
and adding them as per their requirement.

Re: [PATCH] base: soc: Export soc_device_to_device API

[Bjorn Andersson]

On Thu 19 Sep 15:25 PDT 2019, Greg KH wrote:

> On Thu, Sep 19, 2019 at 03:14:56PM -0700, Bjorn Andersson wrote:
> > On Thu 19 Sep 14:58 PDT 2019, Greg KH wrote:
> > 
> > > On Thu, Sep 19, 2019 at 02:53:00PM -0700, Bjorn Andersson wrote:
> > > > On Thu 19 Sep 14:32 PDT 2019, Greg KH wrote:
> > > > 
> > > > > On Thu, Sep 19, 2019 at 02:13:44PM -0700, Murali Nalajala wrote:
> > > > > > If the soc drivers want to add custom sysfs entries it needs to
> > > > > > access "dev" field in "struct soc_device". This can be achieved
> > > > > > by "soc_device_to_device" API. Soc drivers which are built as a
> > > > > > module they need above API to be exported. Otherwise one can
> > > > > > observe compilation issues.
> > > > > > 
> > > > > > Signed-off-by: Murali Nalajala <mnalajal@codeaurora.org>
> > > > > > ---
> > > > > >  drivers/base/soc.c | 1 +
> > > > > >  1 file changed, 1 insertion(+)
> > > > > > 
> > > > > > diff --git a/drivers/base/soc.c b/drivers/base/soc.c
> > > > > > index 7c0c5ca..4ad52f6 100644
> > > > > > --- a/drivers/base/soc.c
> > > > > > +++ b/drivers/base/soc.c
> > > > > > @@ -41,6 +41,7 @@ struct device *soc_device_to_device(struct soc_device *soc_dev)
> > > > > >  {
> > > > > >  	return &soc_dev->dev;
> > > > > >  }
> > > > > > +EXPORT_SYMBOL_GPL(soc_device_to_device);
> > > > > >  
> > > > > >  static umode_t soc_attribute_mode(struct kobject *kobj,
> > > > > >  				struct attribute *attr,
> > > > > 
> > > > > What in-kernel driver needs this?
> > > > > 
> > > > 
> > > > Half of the drivers interacting with the soc driver calls this API,
> > > > several of these I see no reason for being builtin (e.g.
> > > > ux500 andversatile). So I think this patch makes sense to allow us to
> > > > build these as modules.
> > > > 
> > > > > Is linux-next breaking without this?
> > > > > 
> > > > 
> > > > No, we postponed the addition of any sysfs attributes in the Qualcomm
> > > > socinfo driver.
> > > > 
> > > > > We don't export things unless we have a user of the export.
> > > > > 
> > > > > Also, adding "custom" sysfs attributes is almost always not the correct
> > > > > thing to do at all.  The driver should be doing it, by setting up the
> > > > > attribute group properly so that the driver core can do it automatically
> > > > > for it.
> > > > > 
> > > > > No driver should be doing individual add/remove of sysfs files.  If it
> > > > > does so, it is almost guaranteed to be doing it incorrectly and racing
> > > > > userspace.
> > > > > 
> > > > 
> > > > The problem here is that the attributes are expected to be attached to
> > > > the soc driver, which is separate from the platform-specific drivers. So
> > > > there's no way to do platform specific attributes the right way.
> > > > 
> > > > > And yes, there's loads of in-kernel examples of doing this wrong, I've
> > > > > been working on fixing that up, look at the patches now in Linus's tree
> > > > > for platform and USB drivers that do this as examples of how to do it
> > > > > right.
> > > > > 
> > > > 
> > > > Agreed, this patch should not be used as an approval for any crazy
> > > > attributes; but it's necessary in order to extend the soc device's
> > > > attributes, per the current design.
> > > 
> > > Wait, no, let's not let the "current design" remain if it is broken!
> > > 
> > > Why can't the soc driver handle the attributes properly so that the
> > > individual driver doesn't have to do the create/remove?
> > > 
> > 
> > The custom attributes that these drivers want to add to the common ones
> > are known in advance, so I presume we could have them passed into
> > soc_device_register() and registered together with the common
> > attributes...
> > 
> > It sounds like it's worth a prototype.
> 
> Do you have an in-kernel example I can look at to get an idea of what is
> needed here?
> 

realview_soc_probe(), in drivers/soc/versatile/soc-realview.c,
implements the current mechanism of acquiring the soc's struct device
and then issuing a few device_create_file calls on that.

Regards,
Bjorn

Re: [PATCH] base: soc: Export soc_device_to_device API

[Greg KH]

On Thu, Sep 19, 2019 at 03:40:17PM -0700, Bjorn Andersson wrote:
> On Thu 19 Sep 15:25 PDT 2019, Greg KH wrote:
> 
> > On Thu, Sep 19, 2019 at 03:14:56PM -0700, Bjorn Andersson wrote:
> > > On Thu 19 Sep 14:58 PDT 2019, Greg KH wrote:
> > > 
> > > > On Thu, Sep 19, 2019 at 02:53:00PM -0700, Bjorn Andersson wrote:
> > > > > On Thu 19 Sep 14:32 PDT 2019, Greg KH wrote:
> > > > > 
> > > > > > On Thu, Sep 19, 2019 at 02:13:44PM -0700, Murali Nalajala wrote:
> > > > > > > If the soc drivers want to add custom sysfs entries it needs to
> > > > > > > access "dev" field in "struct soc_device". This can be achieved
> > > > > > > by "soc_device_to_device" API. Soc drivers which are built as a
> > > > > > > module they need above API to be exported. Otherwise one can
> > > > > > > observe compilation issues.
> > > > > > > 
> > > > > > > Signed-off-by: Murali Nalajala <mnalajal@codeaurora.org>
> > > > > > > ---
> > > > > > >  drivers/base/soc.c | 1 +
> > > > > > >  1 file changed, 1 insertion(+)
> > > > > > > 
> > > > > > > diff --git a/drivers/base/soc.c b/drivers/base/soc.c
> > > > > > > index 7c0c5ca..4ad52f6 100644
> > > > > > > --- a/drivers/base/soc.c
> > > > > > > +++ b/drivers/base/soc.c
> > > > > > > @@ -41,6 +41,7 @@ struct device *soc_device_to_device(struct soc_device *soc_dev)
> > > > > > >  {
> > > > > > >  	return &soc_dev->dev;
> > > > > > >  }
> > > > > > > +EXPORT_SYMBOL_GPL(soc_device_to_device);
> > > > > > >  
> > > > > > >  static umode_t soc_attribute_mode(struct kobject *kobj,
> > > > > > >  				struct attribute *attr,
> > > > > > 
> > > > > > What in-kernel driver needs this?
> > > > > > 
> > > > > 
> > > > > Half of the drivers interacting with the soc driver calls this API,
> > > > > several of these I see no reason for being builtin (e.g.
> > > > > ux500 andversatile). So I think this patch makes sense to allow us to
> > > > > build these as modules.
> > > > > 
> > > > > > Is linux-next breaking without this?
> > > > > > 
> > > > > 
> > > > > No, we postponed the addition of any sysfs attributes in the Qualcomm
> > > > > socinfo driver.
> > > > > 
> > > > > > We don't export things unless we have a user of the export.
> > > > > > 
> > > > > > Also, adding "custom" sysfs attributes is almost always not the correct
> > > > > > thing to do at all.  The driver should be doing it, by setting up the
> > > > > > attribute group properly so that the driver core can do it automatically
> > > > > > for it.
> > > > > > 
> > > > > > No driver should be doing individual add/remove of sysfs files.  If it
> > > > > > does so, it is almost guaranteed to be doing it incorrectly and racing
> > > > > > userspace.
> > > > > > 
> > > > > 
> > > > > The problem here is that the attributes are expected to be attached to
> > > > > the soc driver, which is separate from the platform-specific drivers. So
> > > > > there's no way to do platform specific attributes the right way.
> > > > > 
> > > > > > And yes, there's loads of in-kernel examples of doing this wrong, I've
> > > > > > been working on fixing that up, look at the patches now in Linus's tree
> > > > > > for platform and USB drivers that do this as examples of how to do it
> > > > > > right.
> > > > > > 
> > > > > 
> > > > > Agreed, this patch should not be used as an approval for any crazy
> > > > > attributes; but it's necessary in order to extend the soc device's
> > > > > attributes, per the current design.
> > > > 
> > > > Wait, no, let's not let the "current design" remain if it is broken!
> > > > 
> > > > Why can't the soc driver handle the attributes properly so that the
> > > > individual driver doesn't have to do the create/remove?
> > > > 
> > > 
> > > The custom attributes that these drivers want to add to the common ones
> > > are known in advance, so I presume we could have them passed into
> > > soc_device_register() and registered together with the common
> > > attributes...
> > > 
> > > It sounds like it's worth a prototype.
> > 
> > Do you have an in-kernel example I can look at to get an idea of what is
> > needed here?
> > 
> 
> realview_soc_probe(), in drivers/soc/versatile/soc-realview.c,
> implements the current mechanism of acquiring the soc's struct device
> and then issuing a few device_create_file calls on that.

That looks to be a trivial driver to fix up.  Look at 6d03c140db2e
("USB: phy: fsl-usb: convert platform driver to use dev_groups") as an
example of how to do this.

Also gotta love the total lack of error checking when calling
device_create_file() in that driver :(

thanks,

greg k-h

Re: [PATCH] base: soc: Export soc_device_to_device API

[Greg KH]

On Thu, Sep 19, 2019 at 03:27:45PM -0700, mnalajal@codeaurora.org wrote:
> On 2019-09-19 14:58, Greg KH wrote:
> > On Thu, Sep 19, 2019 at 02:53:00PM -0700, Bjorn Andersson wrote:
> > > On Thu 19 Sep 14:32 PDT 2019, Greg KH wrote:
> > > 
> > > > On Thu, Sep 19, 2019 at 02:13:44PM -0700, Murali Nalajala wrote:
> > > > > If the soc drivers want to add custom sysfs entries it needs to
> > > > > access "dev" field in "struct soc_device". This can be achieved
> > > > > by "soc_device_to_device" API. Soc drivers which are built as a
> > > > > module they need above API to be exported. Otherwise one can
> > > > > observe compilation issues.
> > > > >
> > > > > Signed-off-by: Murali Nalajala <mnalajal@codeaurora.org>
> > > > > ---
> > > > >  drivers/base/soc.c | 1 +
> > > > >  1 file changed, 1 insertion(+)
> > > > >
> > > > > diff --git a/drivers/base/soc.c b/drivers/base/soc.c
> > > > > index 7c0c5ca..4ad52f6 100644
> > > > > --- a/drivers/base/soc.c
> > > > > +++ b/drivers/base/soc.c
> > > > > @@ -41,6 +41,7 @@ struct device *soc_device_to_device(struct soc_device *soc_dev)
> > > > >  {
> > > > >  	return &soc_dev->dev;
> > > > >  }
> > > > > +EXPORT_SYMBOL_GPL(soc_device_to_device);
> > > > >
> > > > >  static umode_t soc_attribute_mode(struct kobject *kobj,
> > > > >  				struct attribute *attr,
> > > >
> > > > What in-kernel driver needs this?
> > > >
> > > 
> > > Half of the drivers interacting with the soc driver calls this API,
> > > several of these I see no reason for being builtin (e.g.
> > > ux500 andversatile). So I think this patch makes sense to allow us to
> > > build these as modules.
> > > 
> > > > Is linux-next breaking without this?
> > > >
> > > 
> > > No, we postponed the addition of any sysfs attributes in the Qualcomm
> > > socinfo driver.
> > > 
> > > > We don't export things unless we have a user of the export.
> > > >
> > > > Also, adding "custom" sysfs attributes is almost always not the correct
> > > > thing to do at all.  The driver should be doing it, by setting up the
> > > > attribute group properly so that the driver core can do it automatically
> > > > for it.
> > > >
> > > > No driver should be doing individual add/remove of sysfs files.  If it
> > > > does so, it is almost guaranteed to be doing it incorrectly and racing
> > > > userspace.
> > > >
> > > 
> > > The problem here is that the attributes are expected to be attached to
> > > the soc driver, which is separate from the platform-specific
> > > drivers. So
> > > there's no way to do platform specific attributes the right way.
> > > 
> > > > And yes, there's loads of in-kernel examples of doing this wrong, I've
> > > > been working on fixing that up, look at the patches now in Linus's tree
> > > > for platform and USB drivers that do this as examples of how to do it
> > > > right.
> > > >
> > > 
> > > Agreed, this patch should not be used as an approval for any crazy
> > > attributes; but it's necessary in order to extend the soc device's
> > > attributes, per the current design.
> > 
> > Wait, no, let's not let the "current design" remain if it is broken!
> > 
> > Why can't the soc driver handle the attributes properly so that the
> > individual driver doesn't have to do the create/remove?
> > 
> > thanks,
> > 
> > greg k-h
> 
> Current soc framework is allowing the soc info drivers to create very
> limited
> soc information as sysfs entries like machine name, family, revision and
> soc_id.
> Sometimes it become a limited information for the clients to know more about
> soc
> information. In this scenario soc info drivers are adding their own sysfs
> entries
> on top of what soc framework is doing like hw_platform, hw_platform_subtype
> etc.
> I believe this will be an issue for the other soc vendors as well. It is
> good that
> if we could add these into the soc framework rather than individual clients
> defining
> and adding them as per their requirement.

It's fine to add sysfs files (as long as you document them in
Documentation/ABI/) but don't do so file-by-file in a driver as that is
racy and will not work.  See 6d03c140db2e ("USB: phy: fsl-usb: convert
platform driver to use dev_groups") as an example of how to do this in a
simple way to have the driver core do all of the work for you
automatically.

thanks,

greg k-h

Re: [PATCH] base: soc: Export soc_device_to_device API

[mnalajal]

On 2019-09-19 15:45, Greg KH wrote:
> On Thu, Sep 19, 2019 at 03:40:17PM -0700, Bjorn Andersson wrote:
>> On Thu 19 Sep 15:25 PDT 2019, Greg KH wrote:
>> 
>> > On Thu, Sep 19, 2019 at 03:14:56PM -0700, Bjorn Andersson wrote:
>> > > On Thu 19 Sep 14:58 PDT 2019, Greg KH wrote:
>> > >
>> > > > On Thu, Sep 19, 2019 at 02:53:00PM -0700, Bjorn Andersson wrote:
>> > > > > On Thu 19 Sep 14:32 PDT 2019, Greg KH wrote:
>> > > > >
>> > > > > > On Thu, Sep 19, 2019 at 02:13:44PM -0700, Murali Nalajala wrote:
>> > > > > > > If the soc drivers want to add custom sysfs entries it needs to
>> > > > > > > access "dev" field in "struct soc_device". This can be achieved
>> > > > > > > by "soc_device_to_device" API. Soc drivers which are built as a
>> > > > > > > module they need above API to be exported. Otherwise one can
>> > > > > > > observe compilation issues.
>> > > > > > >
>> > > > > > > Signed-off-by: Murali Nalajala <mnalajal@codeaurora.org>
>> > > > > > > ---
>> > > > > > >  drivers/base/soc.c | 1 +
>> > > > > > >  1 file changed, 1 insertion(+)
>> > > > > > >
>> > > > > > > diff --git a/drivers/base/soc.c b/drivers/base/soc.c
>> > > > > > > index 7c0c5ca..4ad52f6 100644
>> > > > > > > --- a/drivers/base/soc.c
>> > > > > > > +++ b/drivers/base/soc.c
>> > > > > > > @@ -41,6 +41,7 @@ struct device *soc_device_to_device(struct soc_device *soc_dev)
>> > > > > > >  {
>> > > > > > >  	return &soc_dev->dev;
>> > > > > > >  }
>> > > > > > > +EXPORT_SYMBOL_GPL(soc_device_to_device);
>> > > > > > >
>> > > > > > >  static umode_t soc_attribute_mode(struct kobject *kobj,
>> > > > > > >  				struct attribute *attr,
>> > > > > >
>> > > > > > What in-kernel driver needs this?
>> > > > > >
>> > > > >
>> > > > > Half of the drivers interacting with the soc driver calls this API,
>> > > > > several of these I see no reason for being builtin (e.g.
>> > > > > ux500 andversatile). So I think this patch makes sense to allow us to
>> > > > > build these as modules.
>> > > > >
>> > > > > > Is linux-next breaking without this?
>> > > > > >
>> > > > >
>> > > > > No, we postponed the addition of any sysfs attributes in the Qualcomm
>> > > > > socinfo driver.
>> > > > >
>> > > > > > We don't export things unless we have a user of the export.
>> > > > > >
>> > > > > > Also, adding "custom" sysfs attributes is almost always not the correct
>> > > > > > thing to do at all.  The driver should be doing it, by setting up the
>> > > > > > attribute group properly so that the driver core can do it automatically
>> > > > > > for it.
>> > > > > >
>> > > > > > No driver should be doing individual add/remove of sysfs files.  If it
>> > > > > > does so, it is almost guaranteed to be doing it incorrectly and racing
>> > > > > > userspace.
>> > > > > >
>> > > > >
>> > > > > The problem here is that the attributes are expected to be attached to
>> > > > > the soc driver, which is separate from the platform-specific drivers. So
>> > > > > there's no way to do platform specific attributes the right way.
>> > > > >
>> > > > > > And yes, there's loads of in-kernel examples of doing this wrong, I've
>> > > > > > been working on fixing that up, look at the patches now in Linus's tree
>> > > > > > for platform and USB drivers that do this as examples of how to do it
>> > > > > > right.
>> > > > > >
>> > > > >
>> > > > > Agreed, this patch should not be used as an approval for any crazy
>> > > > > attributes; but it's necessary in order to extend the soc device's
>> > > > > attributes, per the current design.
>> > > >
>> > > > Wait, no, let's not let the "current design" remain if it is broken!
>> > > >
>> > > > Why can't the soc driver handle the attributes properly so that the
>> > > > individual driver doesn't have to do the create/remove?
>> > > >
>> > >
>> > > The custom attributes that these drivers want to add to the common ones
>> > > are known in advance, so I presume we could have them passed into
>> > > soc_device_register() and registered together with the common
>> > > attributes...
>> > >
>> > > It sounds like it's worth a prototype.
>> >
>> > Do you have an in-kernel example I can look at to get an idea of what is
>> > needed here?
>> >
>> 
>> realview_soc_probe(), in drivers/soc/versatile/soc-realview.c,
>> implements the current mechanism of acquiring the soc's struct device
>> and then issuing a few device_create_file calls on that.
> 
> That looks to be a trivial driver to fix up.  Look at 6d03c140db2e
> ("USB: phy: fsl-usb: convert platform driver to use dev_groups") as an
> example of how to do this.
> 
> Also gotta love the total lack of error checking when calling
> device_create_file() in that driver :(
> 
> thanks,
> 
> greg k-h

You can see example here 
https://android.googlesource.com/kernel/msm/+/android-7.1.0_r0.2/drivers/soc/qcom/socinfo.c#1356
I want to add the custom entries to "soc" device path. This can be 
achieved if i go with "dev_groups"?

sys/devices/soc0 # ls -l
total 0
-r--r--r--    1 root     0             4096 Jan  1 00:01 family
-r--r--r--    1 root     0             4096 Jan  1 00:01 machine
drwxr-xr-x    2 root     0                0 Jan  1 00:01 power
-r--r--r--    1 root     0             4096 Jan  1 00:01 revision
-r--r--r--    1 root     0             4096 Jan  1 00:01 serial_number
lrwxrwxrwx    1 root     0                0 Jan  1 00:01 subsystem -> 
../../bus/soc
-rw-r--r--    1 root     0             4096 Jan  1 00:01 uevent

Re: [PATCH] base: soc: Export soc_device_to_device API

[Bjorn Andersson]

On Thu 19 Sep 15:45 PDT 2019, Greg KH wrote:

> On Thu, Sep 19, 2019 at 03:40:17PM -0700, Bjorn Andersson wrote:
> > On Thu 19 Sep 15:25 PDT 2019, Greg KH wrote:
> > 
> > > On Thu, Sep 19, 2019 at 03:14:56PM -0700, Bjorn Andersson wrote:
> > > > On Thu 19 Sep 14:58 PDT 2019, Greg KH wrote:
> > > > 
> > > > > On Thu, Sep 19, 2019 at 02:53:00PM -0700, Bjorn Andersson wrote:
> > > > > > On Thu 19 Sep 14:32 PDT 2019, Greg KH wrote:
> > > > > > 
> > > > > > > On Thu, Sep 19, 2019 at 02:13:44PM -0700, Murali Nalajala wrote:
> > > > > > > > If the soc drivers want to add custom sysfs entries it needs to
> > > > > > > > access "dev" field in "struct soc_device". This can be achieved
> > > > > > > > by "soc_device_to_device" API. Soc drivers which are built as a
> > > > > > > > module they need above API to be exported. Otherwise one can
> > > > > > > > observe compilation issues.
> > > > > > > > 
> > > > > > > > Signed-off-by: Murali Nalajala <mnalajal@codeaurora.org>
> > > > > > > > ---
> > > > > > > >  drivers/base/soc.c | 1 +
> > > > > > > >  1 file changed, 1 insertion(+)
> > > > > > > > 
> > > > > > > > diff --git a/drivers/base/soc.c b/drivers/base/soc.c
> > > > > > > > index 7c0c5ca..4ad52f6 100644
> > > > > > > > --- a/drivers/base/soc.c
> > > > > > > > +++ b/drivers/base/soc.c
> > > > > > > > @@ -41,6 +41,7 @@ struct device *soc_device_to_device(struct soc_device *soc_dev)
> > > > > > > >  {
> > > > > > > >  	return &soc_dev->dev;
> > > > > > > >  }
> > > > > > > > +EXPORT_SYMBOL_GPL(soc_device_to_device);
> > > > > > > >  
> > > > > > > >  static umode_t soc_attribute_mode(struct kobject *kobj,
> > > > > > > >  				struct attribute *attr,
> > > > > > > 
> > > > > > > What in-kernel driver needs this?
> > > > > > > 
> > > > > > 
> > > > > > Half of the drivers interacting with the soc driver calls this API,
> > > > > > several of these I see no reason for being builtin (e.g.
> > > > > > ux500 andversatile). So I think this patch makes sense to allow us to
> > > > > > build these as modules.
> > > > > > 
> > > > > > > Is linux-next breaking without this?
> > > > > > > 
> > > > > > 
> > > > > > No, we postponed the addition of any sysfs attributes in the Qualcomm
> > > > > > socinfo driver.
> > > > > > 
> > > > > > > We don't export things unless we have a user of the export.
> > > > > > > 
> > > > > > > Also, adding "custom" sysfs attributes is almost always not the correct
> > > > > > > thing to do at all.  The driver should be doing it, by setting up the
> > > > > > > attribute group properly so that the driver core can do it automatically
> > > > > > > for it.
> > > > > > > 
> > > > > > > No driver should be doing individual add/remove of sysfs files.  If it
> > > > > > > does so, it is almost guaranteed to be doing it incorrectly and racing
> > > > > > > userspace.
> > > > > > > 
> > > > > > 
> > > > > > The problem here is that the attributes are expected to be attached to
> > > > > > the soc driver, which is separate from the platform-specific drivers. So
> > > > > > there's no way to do platform specific attributes the right way.
> > > > > > 
> > > > > > > And yes, there's loads of in-kernel examples of doing this wrong, I've
> > > > > > > been working on fixing that up, look at the patches now in Linus's tree
> > > > > > > for platform and USB drivers that do this as examples of how to do it
> > > > > > > right.
> > > > > > > 
> > > > > > 
> > > > > > Agreed, this patch should not be used as an approval for any crazy
> > > > > > attributes; but it's necessary in order to extend the soc device's
> > > > > > attributes, per the current design.
> > > > > 
> > > > > Wait, no, let's not let the "current design" remain if it is broken!
> > > > > 
> > > > > Why can't the soc driver handle the attributes properly so that the
> > > > > individual driver doesn't have to do the create/remove?
> > > > > 
> > > > 
> > > > The custom attributes that these drivers want to add to the common ones
> > > > are known in advance, so I presume we could have them passed into
> > > > soc_device_register() and registered together with the common
> > > > attributes...
> > > > 
> > > > It sounds like it's worth a prototype.
> > > 
> > > Do you have an in-kernel example I can look at to get an idea of what is
> > > needed here?
> > > 
> > 
> > realview_soc_probe(), in drivers/soc/versatile/soc-realview.c,
> > implements the current mechanism of acquiring the soc's struct device
> > and then issuing a few device_create_file calls on that.
> 
> That looks to be a trivial driver to fix up.  Look at 6d03c140db2e
> ("USB: phy: fsl-usb: convert platform driver to use dev_groups") as an
> example of how to do this.
> 

The difference between the two cases is that in the fsl-usb case it's
attributes of the device itself, while in the soc case the realview-soc
driver (or the others doing this) calls soc_device_register() to
register a new (dangling) soc device, which it then adds its attributes
onto.

We can't use dev_groups, because the soc_device (soc.c) isn't actually a
driver and the list of attributes is a combination of things from soc.c
and e.g. soc-realview.c.

But if we pass a struct attribute_group into soc_device_register() and
then have that register both groups using dev.groups, this should be
much cleaner at least.

> Also gotta love the total lack of error checking when calling
> device_create_file() in that driver :(
> 

That's how we roll in the shire...

Regards,
Bjorn

Re: [PATCH] base: soc: Export soc_device_to_device API

[Greg KH]

On Thu, Sep 19, 2019 at 08:36:51PM -0700, Bjorn Andersson wrote:
> On Thu 19 Sep 15:45 PDT 2019, Greg KH wrote:
> 
> > On Thu, Sep 19, 2019 at 03:40:17PM -0700, Bjorn Andersson wrote:
> > > On Thu 19 Sep 15:25 PDT 2019, Greg KH wrote:
> > > 
> > > > On Thu, Sep 19, 2019 at 03:14:56PM -0700, Bjorn Andersson wrote:
> > > > > On Thu 19 Sep 14:58 PDT 2019, Greg KH wrote:
> > > > > 
> > > > > > On Thu, Sep 19, 2019 at 02:53:00PM -0700, Bjorn Andersson wrote:
> > > > > > > On Thu 19 Sep 14:32 PDT 2019, Greg KH wrote:
> > > > > > > 
> > > > > > > > On Thu, Sep 19, 2019 at 02:13:44PM -0700, Murali Nalajala wrote:
> > > > > > > > > If the soc drivers want to add custom sysfs entries it needs to
> > > > > > > > > access "dev" field in "struct soc_device". This can be achieved
> > > > > > > > > by "soc_device_to_device" API. Soc drivers which are built as a
> > > > > > > > > module they need above API to be exported. Otherwise one can
> > > > > > > > > observe compilation issues.
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Murali Nalajala <mnalajal@codeaurora.org>
> > > > > > > > > ---
> > > > > > > > >  drivers/base/soc.c | 1 +
> > > > > > > > >  1 file changed, 1 insertion(+)
> > > > > > > > > 
> > > > > > > > > diff --git a/drivers/base/soc.c b/drivers/base/soc.c
> > > > > > > > > index 7c0c5ca..4ad52f6 100644
> > > > > > > > > --- a/drivers/base/soc.c
> > > > > > > > > +++ b/drivers/base/soc.c
> > > > > > > > > @@ -41,6 +41,7 @@ struct device *soc_device_to_device(struct soc_device *soc_dev)
> > > > > > > > >  {
> > > > > > > > >  	return &soc_dev->dev;
> > > > > > > > >  }
> > > > > > > > > +EXPORT_SYMBOL_GPL(soc_device_to_device);
> > > > > > > > >  
> > > > > > > > >  static umode_t soc_attribute_mode(struct kobject *kobj,
> > > > > > > > >  				struct attribute *attr,
> > > > > > > > 
> > > > > > > > What in-kernel driver needs this?
> > > > > > > > 
> > > > > > > 
> > > > > > > Half of the drivers interacting with the soc driver calls this API,
> > > > > > > several of these I see no reason for being builtin (e.g.
> > > > > > > ux500 andversatile). So I think this patch makes sense to allow us to
> > > > > > > build these as modules.
> > > > > > > 
> > > > > > > > Is linux-next breaking without this?
> > > > > > > > 
> > > > > > > 
> > > > > > > No, we postponed the addition of any sysfs attributes in the Qualcomm
> > > > > > > socinfo driver.
> > > > > > > 
> > > > > > > > We don't export things unless we have a user of the export.
> > > > > > > > 
> > > > > > > > Also, adding "custom" sysfs attributes is almost always not the correct
> > > > > > > > thing to do at all.  The driver should be doing it, by setting up the
> > > > > > > > attribute group properly so that the driver core can do it automatically
> > > > > > > > for it.
> > > > > > > > 
> > > > > > > > No driver should be doing individual add/remove of sysfs files.  If it
> > > > > > > > does so, it is almost guaranteed to be doing it incorrectly and racing
> > > > > > > > userspace.
> > > > > > > > 
> > > > > > > 
> > > > > > > The problem here is that the attributes are expected to be attached to
> > > > > > > the soc driver, which is separate from the platform-specific drivers. So
> > > > > > > there's no way to do platform specific attributes the right way.
> > > > > > > 
> > > > > > > > And yes, there's loads of in-kernel examples of doing this wrong, I've
> > > > > > > > been working on fixing that up, look at the patches now in Linus's tree
> > > > > > > > for platform and USB drivers that do this as examples of how to do it
> > > > > > > > right.
> > > > > > > > 
> > > > > > > 
> > > > > > > Agreed, this patch should not be used as an approval for any crazy
> > > > > > > attributes; but it's necessary in order to extend the soc device's
> > > > > > > attributes, per the current design.
> > > > > > 
> > > > > > Wait, no, let's not let the "current design" remain if it is broken!
> > > > > > 
> > > > > > Why can't the soc driver handle the attributes properly so that the
> > > > > > individual driver doesn't have to do the create/remove?
> > > > > > 
> > > > > 
> > > > > The custom attributes that these drivers want to add to the common ones
> > > > > are known in advance, so I presume we could have them passed into
> > > > > soc_device_register() and registered together with the common
> > > > > attributes...
> > > > > 
> > > > > It sounds like it's worth a prototype.
> > > > 
> > > > Do you have an in-kernel example I can look at to get an idea of what is
> > > > needed here?
> > > > 
> > > 
> > > realview_soc_probe(), in drivers/soc/versatile/soc-realview.c,
> > > implements the current mechanism of acquiring the soc's struct device
> > > and then issuing a few device_create_file calls on that.
> > 
> > That looks to be a trivial driver to fix up.  Look at 6d03c140db2e
> > ("USB: phy: fsl-usb: convert platform driver to use dev_groups") as an
> > example of how to do this.
> > 
> 
> The difference between the two cases is that in the fsl-usb case it's
> attributes of the device itself, while in the soc case the realview-soc
> driver (or the others doing this) calls soc_device_register() to
> register a new (dangling) soc device, which it then adds its attributes
> onto.

That sounds really really odd.  Why can't the soc device do the creation
"automatically" when the device is registered?  The soc core should
handle this for the soc "drivers", that's what it is there for.

> We can't use dev_groups, because the soc_device (soc.c) isn't actually a
> driver and the list of attributes is a combination of things from soc.c
> and e.g. soc-realview.c.
> 
> But if we pass a struct attribute_group into soc_device_register() and
> then have that register both groups using dev.groups, this should be
> much cleaner at least.

Don't you have a structure you can store these in as well?

thanks,

greg k-h

Re: [PATCH] base: soc: Export soc_device_to_device API

[mnalajal]

On 2019-09-19 23:10, Greg KH wrote:
> On Thu, Sep 19, 2019 at 08:36:51PM -0700, Bjorn Andersson wrote:
>> On Thu 19 Sep 15:45 PDT 2019, Greg KH wrote:
>> 
>> > On Thu, Sep 19, 2019 at 03:40:17PM -0700, Bjorn Andersson wrote:
>> > > On Thu 19 Sep 15:25 PDT 2019, Greg KH wrote:
>> > >
>> > > > On Thu, Sep 19, 2019 at 03:14:56PM -0700, Bjorn Andersson wrote:
>> > > > > On Thu 19 Sep 14:58 PDT 2019, Greg KH wrote:
>> > > > >
>> > > > > > On Thu, Sep 19, 2019 at 02:53:00PM -0700, Bjorn Andersson wrote:
>> > > > > > > On Thu 19 Sep 14:32 PDT 2019, Greg KH wrote:
>> > > > > > >
>> > > > > > > > On Thu, Sep 19, 2019 at 02:13:44PM -0700, Murali Nalajala wrote:
>> > > > > > > > > If the soc drivers want to add custom sysfs entries it needs to
>> > > > > > > > > access "dev" field in "struct soc_device". This can be achieved
>> > > > > > > > > by "soc_device_to_device" API. Soc drivers which are built as a
>> > > > > > > > > module they need above API to be exported. Otherwise one can
>> > > > > > > > > observe compilation issues.
>> > > > > > > > >
>> > > > > > > > > Signed-off-by: Murali Nalajala <mnalajal@codeaurora.org>
>> > > > > > > > > ---
>> > > > > > > > >  drivers/base/soc.c | 1 +
>> > > > > > > > >  1 file changed, 1 insertion(+)
>> > > > > > > > >
>> > > > > > > > > diff --git a/drivers/base/soc.c b/drivers/base/soc.c
>> > > > > > > > > index 7c0c5ca..4ad52f6 100644
>> > > > > > > > > --- a/drivers/base/soc.c
>> > > > > > > > > +++ b/drivers/base/soc.c
>> > > > > > > > > @@ -41,6 +41,7 @@ struct device *soc_device_to_device(struct soc_device *soc_dev)
>> > > > > > > > >  {
>> > > > > > > > >  	return &soc_dev->dev;
>> > > > > > > > >  }
>> > > > > > > > > +EXPORT_SYMBOL_GPL(soc_device_to_device);
>> > > > > > > > >
>> > > > > > > > >  static umode_t soc_attribute_mode(struct kobject *kobj,
>> > > > > > > > >  				struct attribute *attr,
>> > > > > > > >
>> > > > > > > > What in-kernel driver needs this?
>> > > > > > > >
>> > > > > > >
>> > > > > > > Half of the drivers interacting with the soc driver calls this API,
>> > > > > > > several of these I see no reason for being builtin (e.g.
>> > > > > > > ux500 andversatile). So I think this patch makes sense to allow us to
>> > > > > > > build these as modules.
>> > > > > > >
>> > > > > > > > Is linux-next breaking without this?
>> > > > > > > >
>> > > > > > >
>> > > > > > > No, we postponed the addition of any sysfs attributes in the Qualcomm
>> > > > > > > socinfo driver.
>> > > > > > >
>> > > > > > > > We don't export things unless we have a user of the export.
>> > > > > > > >
>> > > > > > > > Also, adding "custom" sysfs attributes is almost always not the correct
>> > > > > > > > thing to do at all.  The driver should be doing it, by setting up the
>> > > > > > > > attribute group properly so that the driver core can do it automatically
>> > > > > > > > for it.
>> > > > > > > >
>> > > > > > > > No driver should be doing individual add/remove of sysfs files.  If it
>> > > > > > > > does so, it is almost guaranteed to be doing it incorrectly and racing
>> > > > > > > > userspace.
>> > > > > > > >
>> > > > > > >
>> > > > > > > The problem here is that the attributes are expected to be attached to
>> > > > > > > the soc driver, which is separate from the platform-specific drivers. So
>> > > > > > > there's no way to do platform specific attributes the right way.
>> > > > > > >
>> > > > > > > > And yes, there's loads of in-kernel examples of doing this wrong, I've
>> > > > > > > > been working on fixing that up, look at the patches now in Linus's tree
>> > > > > > > > for platform and USB drivers that do this as examples of how to do it
>> > > > > > > > right.
>> > > > > > > >
>> > > > > > >
>> > > > > > > Agreed, this patch should not be used as an approval for any crazy
>> > > > > > > attributes; but it's necessary in order to extend the soc device's
>> > > > > > > attributes, per the current design.
>> > > > > >
>> > > > > > Wait, no, let's not let the "current design" remain if it is broken!
>> > > > > >
>> > > > > > Why can't the soc driver handle the attributes properly so that the
>> > > > > > individual driver doesn't have to do the create/remove?
>> > > > > >
>> > > > >
>> > > > > The custom attributes that these drivers want to add to the common ones
>> > > > > are known in advance, so I presume we could have them passed into
>> > > > > soc_device_register() and registered together with the common
>> > > > > attributes...
>> > > > >
>> > > > > It sounds like it's worth a prototype.
>> > > >
>> > > > Do you have an in-kernel example I can look at to get an idea of what is
>> > > > needed here?
>> > > >
>> > >
>> > > realview_soc_probe(), in drivers/soc/versatile/soc-realview.c,
>> > > implements the current mechanism of acquiring the soc's struct device
>> > > and then issuing a few device_create_file calls on that.
>> >
>> > That looks to be a trivial driver to fix up.  Look at 6d03c140db2e
>> > ("USB: phy: fsl-usb: convert platform driver to use dev_groups") as an
>> > example of how to do this.
>> >
>> 
>> The difference between the two cases is that in the fsl-usb case it's
>> attributes of the device itself, while in the soc case the 
>> realview-soc
>> driver (or the others doing this) calls soc_device_register() to
>> register a new (dangling) soc device, which it then adds its 
>> attributes
>> onto.
> 
> That sounds really really odd.  Why can't the soc device do the 
> creation
> "automatically" when the device is registered?  The soc core should
> handle this for the soc "drivers", that's what it is there for.
> 
Clients are registering to soc framework using "soce_device_register()"
with "soc_device_attribute". This attribute structure does not have all
the sysfs fields what client are interested. Hence clients are handling
their required sysfs fields in their drivers.
https://elixir.bootlin.com/linux/v5.3/source/drivers/base/soc.c#L114

>> We can't use dev_groups, because the soc_device (soc.c) isn't actually 
>> a
>> driver and the list of attributes is a combination of things from 
>> soc.c
>> and e.g. soc-realview.c.
>> 
>> But if we pass a struct attribute_group into soc_device_register() and
>> then have that register both groups using dev.groups, this should be
>> much cleaner at least.
> 
> Don't you have a structure you can store these in as well?
At present client is populating entries one-by-one
https://android.googlesource.com/kernel/msm/+/android-7.1.0_r0.2/drivers/soc/qcom/socinfo.c#1254

> 
> thanks,
> 
> greg k-h

Re: [PATCH] base: soc: Export soc_device_to_device API

[Greg KH]

On Mon, Sep 23, 2019 at 02:35:33PM -0700, mnalajal@codeaurora.org wrote:
> On 2019-09-19 23:10, Greg KH wrote:
> > On Thu, Sep 19, 2019 at 08:36:51PM -0700, Bjorn Andersson wrote:
> > > On Thu 19 Sep 15:45 PDT 2019, Greg KH wrote:
> > > 
> > > > On Thu, Sep 19, 2019 at 03:40:17PM -0700, Bjorn Andersson wrote:
> > > > > On Thu 19 Sep 15:25 PDT 2019, Greg KH wrote:
> > > > >
> > > > > > On Thu, Sep 19, 2019 at 03:14:56PM -0700, Bjorn Andersson wrote:
> > > > > > > On Thu 19 Sep 14:58 PDT 2019, Greg KH wrote:
> > > > > > >
> > > > > > > > On Thu, Sep 19, 2019 at 02:53:00PM -0700, Bjorn Andersson wrote:
> > > > > > > > > On Thu 19 Sep 14:32 PDT 2019, Greg KH wrote:
> > > > > > > > >
> > > > > > > > > > On Thu, Sep 19, 2019 at 02:13:44PM -0700, Murali Nalajala wrote:
> > > > > > > > > > > If the soc drivers want to add custom sysfs entries it needs to
> > > > > > > > > > > access "dev" field in "struct soc_device". This can be achieved
> > > > > > > > > > > by "soc_device_to_device" API. Soc drivers which are built as a
> > > > > > > > > > > module they need above API to be exported. Otherwise one can
> > > > > > > > > > > observe compilation issues.
> > > > > > > > > > >
> > > > > > > > > > > Signed-off-by: Murali Nalajala <mnalajal@codeaurora.org>
> > > > > > > > > > > ---
> > > > > > > > > > >  drivers/base/soc.c | 1 +
> > > > > > > > > > >  1 file changed, 1 insertion(+)
> > > > > > > > > > >
> > > > > > > > > > > diff --git a/drivers/base/soc.c b/drivers/base/soc.c
> > > > > > > > > > > index 7c0c5ca..4ad52f6 100644
> > > > > > > > > > > --- a/drivers/base/soc.c
> > > > > > > > > > > +++ b/drivers/base/soc.c
> > > > > > > > > > > @@ -41,6 +41,7 @@ struct device *soc_device_to_device(struct soc_device *soc_dev)
> > > > > > > > > > >  {
> > > > > > > > > > >  	return &soc_dev->dev;
> > > > > > > > > > >  }
> > > > > > > > > > > +EXPORT_SYMBOL_GPL(soc_device_to_device);
> > > > > > > > > > >
> > > > > > > > > > >  static umode_t soc_attribute_mode(struct kobject *kobj,
> > > > > > > > > > >  				struct attribute *attr,
> > > > > > > > > >
> > > > > > > > > > What in-kernel driver needs this?
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > Half of the drivers interacting with the soc driver calls this API,
> > > > > > > > > several of these I see no reason for being builtin (e.g.
> > > > > > > > > ux500 andversatile). So I think this patch makes sense to allow us to
> > > > > > > > > build these as modules.
> > > > > > > > >
> > > > > > > > > > Is linux-next breaking without this?
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > No, we postponed the addition of any sysfs attributes in the Qualcomm
> > > > > > > > > socinfo driver.
> > > > > > > > >
> > > > > > > > > > We don't export things unless we have a user of the export.
> > > > > > > > > >
> > > > > > > > > > Also, adding "custom" sysfs attributes is almost always not the correct
> > > > > > > > > > thing to do at all.  The driver should be doing it, by setting up the
> > > > > > > > > > attribute group properly so that the driver core can do it automatically
> > > > > > > > > > for it.
> > > > > > > > > >
> > > > > > > > > > No driver should be doing individual add/remove of sysfs files.  If it
> > > > > > > > > > does so, it is almost guaranteed to be doing it incorrectly and racing
> > > > > > > > > > userspace.
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > The problem here is that the attributes are expected to be attached to
> > > > > > > > > the soc driver, which is separate from the platform-specific drivers. So
> > > > > > > > > there's no way to do platform specific attributes the right way.
> > > > > > > > >
> > > > > > > > > > And yes, there's loads of in-kernel examples of doing this wrong, I've
> > > > > > > > > > been working on fixing that up, look at the patches now in Linus's tree
> > > > > > > > > > for platform and USB drivers that do this as examples of how to do it
> > > > > > > > > > right.
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > Agreed, this patch should not be used as an approval for any crazy
> > > > > > > > > attributes; but it's necessary in order to extend the soc device's
> > > > > > > > > attributes, per the current design.
> > > > > > > >
> > > > > > > > Wait, no, let's not let the "current design" remain if it is broken!
> > > > > > > >
> > > > > > > > Why can't the soc driver handle the attributes properly so that the
> > > > > > > > individual driver doesn't have to do the create/remove?
> > > > > > > >
> > > > > > >
> > > > > > > The custom attributes that these drivers want to add to the common ones
> > > > > > > are known in advance, so I presume we could have them passed into
> > > > > > > soc_device_register() and registered together with the common
> > > > > > > attributes...
> > > > > > >
> > > > > > > It sounds like it's worth a prototype.
> > > > > >
> > > > > > Do you have an in-kernel example I can look at to get an idea of what is
> > > > > > needed here?
> > > > > >
> > > > >
> > > > > realview_soc_probe(), in drivers/soc/versatile/soc-realview.c,
> > > > > implements the current mechanism of acquiring the soc's struct device
> > > > > and then issuing a few device_create_file calls on that.
> > > >
> > > > That looks to be a trivial driver to fix up.  Look at 6d03c140db2e
> > > > ("USB: phy: fsl-usb: convert platform driver to use dev_groups") as an
> > > > example of how to do this.
> > > >
> > > 
> > > The difference between the two cases is that in the fsl-usb case it's
> > > attributes of the device itself, while in the soc case the
> > > realview-soc
> > > driver (or the others doing this) calls soc_device_register() to
> > > register a new (dangling) soc device, which it then adds its
> > > attributes
> > > onto.
> > 
> > That sounds really really odd.  Why can't the soc device do the creation
> > "automatically" when the device is registered?  The soc core should
> > handle this for the soc "drivers", that's what it is there for.
> > 
> Clients are registering to soc framework using "soce_device_register()"
> with "soc_device_attribute". This attribute structure does not have all
> the sysfs fields what client are interested. Hence clients are handling
> their required sysfs fields in their drivers.
> https://elixir.bootlin.com/linux/v5.3/source/drivers/base/soc.c#L114

Then you should fix that :)

> > > We can't use dev_groups, because the soc_device (soc.c) isn't
> > > actually a
> > > driver and the list of attributes is a combination of things from
> > > soc.c
> > > and e.g. soc-realview.c.
> > > 
> > > But if we pass a struct attribute_group into soc_device_register() and
> > > then have that register both groups using dev.groups, this should be
> > > much cleaner at least.
> > 
> > Don't you have a structure you can store these in as well?
> At present client is populating entries one-by-one
> https://android.googlesource.com/kernel/msm/+/android-7.1.0_r0.2/drivers/soc/qcom/socinfo.c#1254

And that is known to be broken and racy and will cause problems with
userspace.  This should be fixed...

thanks,

greg k-h

Re: [PATCH] base: soc: Export soc_device_to_device API

[mnalajal]

On 2019-09-23 21:50, Greg KH wrote:
> On Mon, Sep 23, 2019 at 02:35:33PM -0700, mnalajal@codeaurora.org 
> wrote:
>> On 2019-09-19 23:10, Greg KH wrote:
>> > On Thu, Sep 19, 2019 at 08:36:51PM -0700, Bjorn Andersson wrote:
>> > > On Thu 19 Sep 15:45 PDT 2019, Greg KH wrote:
>> > >
>> > > > On Thu, Sep 19, 2019 at 03:40:17PM -0700, Bjorn Andersson wrote:
>> > > > > On Thu 19 Sep 15:25 PDT 2019, Greg KH wrote:
>> > > > >
>> > > > > > On Thu, Sep 19, 2019 at 03:14:56PM -0700, Bjorn Andersson wrote:
>> > > > > > > On Thu 19 Sep 14:58 PDT 2019, Greg KH wrote:
>> > > > > > >
>> > > > > > > > On Thu, Sep 19, 2019 at 02:53:00PM -0700, Bjorn Andersson wrote:
>> > > > > > > > > On Thu 19 Sep 14:32 PDT 2019, Greg KH wrote:
>> > > > > > > > >
>> > > > > > > > > > On Thu, Sep 19, 2019 at 02:13:44PM -0700, Murali Nalajala wrote:
>> > > > > > > > > > > If the soc drivers want to add custom sysfs entries it needs to
>> > > > > > > > > > > access "dev" field in "struct soc_device". This can be achieved
>> > > > > > > > > > > by "soc_device_to_device" API. Soc drivers which are built as a
>> > > > > > > > > > > module they need above API to be exported. Otherwise one can
>> > > > > > > > > > > observe compilation issues.
>> > > > > > > > > > >
>> > > > > > > > > > > Signed-off-by: Murali Nalajala <mnalajal@codeaurora.org>
>> > > > > > > > > > > ---
>> > > > > > > > > > >  drivers/base/soc.c | 1 +
>> > > > > > > > > > >  1 file changed, 1 insertion(+)
>> > > > > > > > > > >
>> > > > > > > > > > > diff --git a/drivers/base/soc.c b/drivers/base/soc.c
>> > > > > > > > > > > index 7c0c5ca..4ad52f6 100644
>> > > > > > > > > > > --- a/drivers/base/soc.c
>> > > > > > > > > > > +++ b/drivers/base/soc.c
>> > > > > > > > > > > @@ -41,6 +41,7 @@ struct device *soc_device_to_device(struct soc_device *soc_dev)
>> > > > > > > > > > >  {
>> > > > > > > > > > >  	return &soc_dev->dev;
>> > > > > > > > > > >  }
>> > > > > > > > > > > +EXPORT_SYMBOL_GPL(soc_device_to_device);
>> > > > > > > > > > >
>> > > > > > > > > > >  static umode_t soc_attribute_mode(struct kobject *kobj,
>> > > > > > > > > > >  				struct attribute *attr,
>> > > > > > > > > >
>> > > > > > > > > > What in-kernel driver needs this?
>> > > > > > > > > >
>> > > > > > > > >
>> > > > > > > > > Half of the drivers interacting with the soc driver calls this API,
>> > > > > > > > > several of these I see no reason for being builtin (e.g.
>> > > > > > > > > ux500 andversatile). So I think this patch makes sense to allow us to
>> > > > > > > > > build these as modules.
>> > > > > > > > >
>> > > > > > > > > > Is linux-next breaking without this?
>> > > > > > > > > >
>> > > > > > > > >
>> > > > > > > > > No, we postponed the addition of any sysfs attributes in the Qualcomm
>> > > > > > > > > socinfo driver.
>> > > > > > > > >
>> > > > > > > > > > We don't export things unless we have a user of the export.
>> > > > > > > > > >
>> > > > > > > > > > Also, adding "custom" sysfs attributes is almost always not the correct
>> > > > > > > > > > thing to do at all.  The driver should be doing it, by setting up the
>> > > > > > > > > > attribute group properly so that the driver core can do it automatically
>> > > > > > > > > > for it.
>> > > > > > > > > >
>> > > > > > > > > > No driver should be doing individual add/remove of sysfs files.  If it
>> > > > > > > > > > does so, it is almost guaranteed to be doing it incorrectly and racing
>> > > > > > > > > > userspace.
>> > > > > > > > > >
>> > > > > > > > >
>> > > > > > > > > The problem here is that the attributes are expected to be attached to
>> > > > > > > > > the soc driver, which is separate from the platform-specific drivers. So
>> > > > > > > > > there's no way to do platform specific attributes the right way.
>> > > > > > > > >
>> > > > > > > > > > And yes, there's loads of in-kernel examples of doing this wrong, I've
>> > > > > > > > > > been working on fixing that up, look at the patches now in Linus's tree
>> > > > > > > > > > for platform and USB drivers that do this as examples of how to do it
>> > > > > > > > > > right.
>> > > > > > > > > >
>> > > > > > > > >
>> > > > > > > > > Agreed, this patch should not be used as an approval for any crazy
>> > > > > > > > > attributes; but it's necessary in order to extend the soc device's
>> > > > > > > > > attributes, per the current design.
>> > > > > > > >
>> > > > > > > > Wait, no, let's not let the "current design" remain if it is broken!
>> > > > > > > >
>> > > > > > > > Why can't the soc driver handle the attributes properly so that the
>> > > > > > > > individual driver doesn't have to do the create/remove?
>> > > > > > > >
>> > > > > > >
>> > > > > > > The custom attributes that these drivers want to add to the common ones
>> > > > > > > are known in advance, so I presume we could have them passed into
>> > > > > > > soc_device_register() and registered together with the common
>> > > > > > > attributes...
>> > > > > > >
>> > > > > > > It sounds like it's worth a prototype.
>> > > > > >
>> > > > > > Do you have an in-kernel example I can look at to get an idea of what is
>> > > > > > needed here?
>> > > > > >
>> > > > >
>> > > > > realview_soc_probe(), in drivers/soc/versatile/soc-realview.c,
>> > > > > implements the current mechanism of acquiring the soc's struct device
>> > > > > and then issuing a few device_create_file calls on that.
>> > > >
>> > > > That looks to be a trivial driver to fix up.  Look at 6d03c140db2e
>> > > > ("USB: phy: fsl-usb: convert platform driver to use dev_groups") as an
>> > > > example of how to do this.
>> > > >
>> > >
>> > > The difference between the two cases is that in the fsl-usb case it's
>> > > attributes of the device itself, while in the soc case the
>> > > realview-soc
>> > > driver (or the others doing this) calls soc_device_register() to
>> > > register a new (dangling) soc device, which it then adds its
>> > > attributes
>> > > onto.
>> >
>> > That sounds really really odd.  Why can't the soc device do the creation
>> > "automatically" when the device is registered?  The soc core should
>> > handle this for the soc "drivers", that's what it is there for.
>> >
>> Clients are registering to soc framework using 
>> "soce_device_register()"
>> with "soc_device_attribute". This attribute structure does not have 
>> all
>> the sysfs fields what client are interested. Hence clients are 
>> handling
>> their required sysfs fields in their drivers.
>> https://elixir.bootlin.com/linux/v5.3/source/drivers/base/soc.c#L114
> 
> Then you should fix that :)
If i understand, you are asking me to address additional sysfs entries 
from the client side using "default attribute" groups.
I saw your patches about "dev_groups" usage which might be part of 
5.4-rc1.
If i go with above approach, i end up seeing the soc information at two 
different sysfs paths i.e.
Is this my understanding correct?

1. /sys/devices/soc0/*
2. /sys/bus/platform/drivers/msm-socinfo/*

Couple of things which i can think of addressing this issue is:
1. Modify the soc framework APIs to pass the client side sysfs 
attributes. This will ensure all the soc information fall under 
/sys/devices/soc0/*
2. Modify "struct soc_device_attribute" and add more entries. So that we 
do not need to change any soc framework.
Problem here is others might have a different requirement which will not 
be full fill if i do this.

> 
>> > > We can't use dev_groups, because the soc_device (soc.c) isn't
>> > > actually a
>> > > driver and the list of attributes is a combination of things from
>> > > soc.c
>> > > and e.g. soc-realview.c.
>> > >
>> > > But if we pass a struct attribute_group into soc_device_register() and
>> > > then have that register both groups using dev.groups, this should be
>> > > much cleaner at least.
>> >
>> > Don't you have a structure you can store these in as well?
>> At present client is populating entries one-by-one
>> https://android.googlesource.com/kernel/msm/+/android-7.1.0_r0.2/drivers/soc/qcom/socinfo.c#1254
> 
> And that is known to be broken and racy and will cause problems with
> userspace.  This should be fixed...
I saw your explanation here about race 
http://kroah.com/log/blog/2013/06/26/how-to-create-a-sysfs-file-correctly/
> 
> thanks,
> 
> greg k-h

Re: [PATCH] base: soc: Export soc_device_to_device API

[Greg KH]

On Thu, Sep 26, 2019 at 07:33:21AM -0700, mnalajal@codeaurora.org wrote:
> On 2019-09-23 21:50, Greg KH wrote:
> > On Mon, Sep 23, 2019 at 02:35:33PM -0700, mnalajal@codeaurora.org wrote:
> > > On 2019-09-19 23:10, Greg KH wrote:
> > > > On Thu, Sep 19, 2019 at 08:36:51PM -0700, Bjorn Andersson wrote:
> > > > > On Thu 19 Sep 15:45 PDT 2019, Greg KH wrote:
> > > > >
> > > > > > On Thu, Sep 19, 2019 at 03:40:17PM -0700, Bjorn Andersson wrote:
> > > > > > > On Thu 19 Sep 15:25 PDT 2019, Greg KH wrote:
> > > > > > >
> > > > > > > > On Thu, Sep 19, 2019 at 03:14:56PM -0700, Bjorn Andersson wrote:
> > > > > > > > > On Thu 19 Sep 14:58 PDT 2019, Greg KH wrote:
> > > > > > > > >
> > > > > > > > > > On Thu, Sep 19, 2019 at 02:53:00PM -0700, Bjorn Andersson wrote:
> > > > > > > > > > > On Thu 19 Sep 14:32 PDT 2019, Greg KH wrote:
> > > > > > > > > > >
> > > > > > > > > > > > On Thu, Sep 19, 2019 at 02:13:44PM -0700, Murali Nalajala wrote:
> > > > > > > > > > > > > If the soc drivers want to add custom sysfs entries it needs to
> > > > > > > > > > > > > access "dev" field in "struct soc_device". This can be achieved
> > > > > > > > > > > > > by "soc_device_to_device" API. Soc drivers which are built as a
> > > > > > > > > > > > > module they need above API to be exported. Otherwise one can
> > > > > > > > > > > > > observe compilation issues.
> > > > > > > > > > > > >
> > > > > > > > > > > > > Signed-off-by: Murali Nalajala <mnalajal@codeaurora.org>
> > > > > > > > > > > > > ---
> > > > > > > > > > > > >  drivers/base/soc.c | 1 +
> > > > > > > > > > > > >  1 file changed, 1 insertion(+)
> > > > > > > > > > > > >
> > > > > > > > > > > > > diff --git a/drivers/base/soc.c b/drivers/base/soc.c
> > > > > > > > > > > > > index 7c0c5ca..4ad52f6 100644
> > > > > > > > > > > > > --- a/drivers/base/soc.c
> > > > > > > > > > > > > +++ b/drivers/base/soc.c
> > > > > > > > > > > > > @@ -41,6 +41,7 @@ struct device *soc_device_to_device(struct soc_device *soc_dev)
> > > > > > > > > > > > >  {
> > > > > > > > > > > > >  	return &soc_dev->dev;
> > > > > > > > > > > > >  }
> > > > > > > > > > > > > +EXPORT_SYMBOL_GPL(soc_device_to_device);
> > > > > > > > > > > > >
> > > > > > > > > > > > >  static umode_t soc_attribute_mode(struct kobject *kobj,
> > > > > > > > > > > > >  				struct attribute *attr,
> > > > > > > > > > > >
> > > > > > > > > > > > What in-kernel driver needs this?
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > Half of the drivers interacting with the soc driver calls this API,
> > > > > > > > > > > several of these I see no reason for being builtin (e.g.
> > > > > > > > > > > ux500 andversatile). So I think this patch makes sense to allow us to
> > > > > > > > > > > build these as modules.
> > > > > > > > > > >
> > > > > > > > > > > > Is linux-next breaking without this?
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > No, we postponed the addition of any sysfs attributes in the Qualcomm
> > > > > > > > > > > socinfo driver.
> > > > > > > > > > >
> > > > > > > > > > > > We don't export things unless we have a user of the export.
> > > > > > > > > > > >
> > > > > > > > > > > > Also, adding "custom" sysfs attributes is almost always not the correct
> > > > > > > > > > > > thing to do at all.  The driver should be doing it, by setting up the
> > > > > > > > > > > > attribute group properly so that the driver core can do it automatically
> > > > > > > > > > > > for it.
> > > > > > > > > > > >
> > > > > > > > > > > > No driver should be doing individual add/remove of sysfs files.  If it
> > > > > > > > > > > > does so, it is almost guaranteed to be doing it incorrectly and racing
> > > > > > > > > > > > userspace.
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > The problem here is that the attributes are expected to be attached to
> > > > > > > > > > > the soc driver, which is separate from the platform-specific drivers. So
> > > > > > > > > > > there's no way to do platform specific attributes the right way.
> > > > > > > > > > >
> > > > > > > > > > > > And yes, there's loads of in-kernel examples of doing this wrong, I've
> > > > > > > > > > > > been working on fixing that up, look at the patches now in Linus's tree
> > > > > > > > > > > > for platform and USB drivers that do this as examples of how to do it
> > > > > > > > > > > > right.
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > Agreed, this patch should not be used as an approval for any crazy
> > > > > > > > > > > attributes; but it's necessary in order to extend the soc device's
> > > > > > > > > > > attributes, per the current design.
> > > > > > > > > >
> > > > > > > > > > Wait, no, let's not let the "current design" remain if it is broken!
> > > > > > > > > >
> > > > > > > > > > Why can't the soc driver handle the attributes properly so that the
> > > > > > > > > > individual driver doesn't have to do the create/remove?
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > The custom attributes that these drivers want to add to the common ones
> > > > > > > > > are known in advance, so I presume we could have them passed into
> > > > > > > > > soc_device_register() and registered together with the common
> > > > > > > > > attributes...
> > > > > > > > >
> > > > > > > > > It sounds like it's worth a prototype.
> > > > > > > >
> > > > > > > > Do you have an in-kernel example I can look at to get an idea of what is
> > > > > > > > needed here?
> > > > > > > >
> > > > > > >
> > > > > > > realview_soc_probe(), in drivers/soc/versatile/soc-realview.c,
> > > > > > > implements the current mechanism of acquiring the soc's struct device
> > > > > > > and then issuing a few device_create_file calls on that.
> > > > > >
> > > > > > That looks to be a trivial driver to fix up.  Look at 6d03c140db2e
> > > > > > ("USB: phy: fsl-usb: convert platform driver to use dev_groups") as an
> > > > > > example of how to do this.
> > > > > >
> > > > >
> > > > > The difference between the two cases is that in the fsl-usb case it's
> > > > > attributes of the device itself, while in the soc case the
> > > > > realview-soc
> > > > > driver (or the others doing this) calls soc_device_register() to
> > > > > register a new (dangling) soc device, which it then adds its
> > > > > attributes
> > > > > onto.
> > > >
> > > > That sounds really really odd.  Why can't the soc device do the creation
> > > > "automatically" when the device is registered?  The soc core should
> > > > handle this for the soc "drivers", that's what it is there for.
> > > >
> > > Clients are registering to soc framework using
> > > "soce_device_register()"
> > > with "soc_device_attribute". This attribute structure does not have
> > > all
> > > the sysfs fields what client are interested. Hence clients are
> > > handling
> > > their required sysfs fields in their drivers.
> > > https://elixir.bootlin.com/linux/v5.3/source/drivers/base/soc.c#L114
> > 
> > Then you should fix that :)
> If i understand, you are asking me to address additional sysfs entries from
> the client side using "default attribute" groups.
> I saw your patches about "dev_groups" usage which might be part of 5.4-rc1.
> If i go with above approach, i end up seeing the soc information at two
> different sysfs paths i.e.
> Is this my understanding correct?
> 
> 1. /sys/devices/soc0/*
> 2. /sys/bus/platform/drivers/msm-socinfo/*

Ah!

Ugh.

Ok, if the soc "core" wants devices to put sysfs files under the "socX"
device, then it needs to provide for a way to do this in a sane manner.
Exposing the "struct device" of the device here is NOT the sane way to
do this.

So we are back to the original request I made here, the SOC "core" needs
to be able to create these files for you, so an attribute group list
must be available for the soc "driver" to set.

What it is doing now is not ok.

> Couple of things which i can think of addressing this issue is:
> 1. Modify the soc framework APIs to pass the client side sysfs attributes.
> This will ensure all the soc information fall under /sys/devices/soc0/*

"pass"?  You mean "create", right?

> 2. Modify "struct soc_device_attribute" and add more entries. So that we do
> not need to change any soc framework.
> Problem here is others might have a different requirement which will not be
> full fill if i do this.

I don't understand what you mean by this second option, sorry.

See my suggestion above, what we have now is just not ok.

thanks,

greg k-h