Re: [PATCH] base: soc: Export soc_device_to_device API
On Thu, Sep 26, 2019 at 07:33:21AM -0700, mnala...@codeaurora.org wrote:
> On 2019-09-23 21:50, Greg KH wrote:
> > On Mon, Sep 23, 2019 at 02:35:33PM -0700, mnala...@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
> > > > > > > > > > > > >
> > > > > > > > > > > > > ---
> > > > > > > > > > > > > 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 _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.
> > > > > > > > > > > >
> > > > > >
Re: [PATCH] base: soc: Export soc_device_to_device API
On 2019-09-23 21:50, Greg KH wrote:
On Mon, Sep 23, 2019 at 02:35:33PM -0700, mnala...@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
> > > > > > > > > > ---
> > > > > > > > > > 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 _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
Re: [PATCH] base: soc: Export soc_device_to_device API
On Mon, Sep 23, 2019 at 02:35:33PM -0700, mnala...@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
> > > > > > > > > > > ---
> > > > > > > > > > > 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 _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?
> > > > > > > >
>
Re: [PATCH] base: soc: Export soc_device_to_device API
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
> > > > > > > > ---
> > > > > > > > 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 _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
Re: [PATCH] base: soc: Export soc_device_to_device API
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
> > > > > > > > > ---
> > > > > > > > > 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 _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
> >
Re: [PATCH] base: soc: Export soc_device_to_device API
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
> > > > > > > > ---
> > > > > > > > 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 _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
Re: [PATCH] base: soc: Export soc_device_to_device API
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
> > > > > > ---
> > > > > > 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 _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-x2 root 00 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
lrwxrwxrwx1 root 0
Re: [PATCH] base: soc: Export soc_device_to_device API
On Thu, Sep 19, 2019 at 03:27:45PM -0700, mnala...@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
> > > > > ---
> > > > > 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 _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
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
> > > > > > > ---
> > > > > > > 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 _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
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
> > > > > > ---
> > > > > > 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 _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
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
> > ---
> > 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 _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
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
> > > > > ---
> > > > > 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 _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
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
> > > > ---
> > > > 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 _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
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
> > > ---
> > > 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 _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
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
> > ---
> > 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 _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
Regards,
Bjorn
Re: [PATCH] base: soc: Export soc_device_to_device API
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
> ---
> 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 _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
[PATCH] base: soc: Export soc_device_to_device API
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
---
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 _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
