| Message ID | 20220729135041.2285908-4-gregkh@linuxfoundation.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/5] soundwire: sysfs: move sdw_slave_dev_attr_group into the existing list of groups | expand |
> diff --git a/drivers/soundwire/sysfs_slave_dpn.c b/drivers/soundwire/sysfs_slave_dpn.c > index c4b6543c09fd..a3fb380ee519 100644 > --- a/drivers/soundwire/sysfs_slave_dpn.c > +++ b/drivers/soundwire/sysfs_slave_dpn.c > @@ -283,6 +283,9 @@ int sdw_slave_sysfs_dpn_init(struct sdw_slave *slave) > int ret; > int i; > > + if (!slave->prop.source_ports && !slave->prop.sink_ports) > + return 0; > + > mask = slave->prop.source_ports; > for_each_set_bit(i, &mask, 32) { > ret = add_all_attributes(&slave->dev, i, 1); I am struggling with this one since the driver is still adding attributes manually. You mentioned in the other thread that " That's what the is_visible() callback is for in the groups structure, you determine if the attribute is visable or not at runtime, you don't rely on the driver itself to add/remove attributes, that does not scale and again, is racy. " I interpret that as "there's still a race here", no?
On Fri, Jul 29, 2022 at 10:00:42AM -0500, Pierre-Louis Bossart wrote: > > > > diff --git a/drivers/soundwire/sysfs_slave_dpn.c b/drivers/soundwire/sysfs_slave_dpn.c > > index c4b6543c09fd..a3fb380ee519 100644 > > --- a/drivers/soundwire/sysfs_slave_dpn.c > > +++ b/drivers/soundwire/sysfs_slave_dpn.c > > @@ -283,6 +283,9 @@ int sdw_slave_sysfs_dpn_init(struct sdw_slave *slave) > > int ret; > > int i; > > > > + if (!slave->prop.source_ports && !slave->prop.sink_ports) > > + return 0; > > + > > mask = slave->prop.source_ports; > > for_each_set_bit(i, &mask, 32) { > > ret = add_all_attributes(&slave->dev, i, 1); > > I am struggling with this one since the driver is still adding > attributes manually. You mentioned in the other thread that > > " > That's what the is_visible() callback is for in the groups structure, > you determine if the attribute is visable or not at runtime, you don't > rely on the driver itself to add/remove attributes, that does not scale > and again, is racy. > " > > I interpret that as "there's still a race here", no? Yes, there is, BUT as you are creating all of these attributes "on the fly" for now, I don't see a simple conversion to fix that up. Let me do these, the easy ones first. Your dynamic attribute allocations are the harder things to do, let me think about those after I've fixed the rest of the tree up with the trivial ones :) thanks, greg k-h
On 29-07-22, 17:13, Greg Kroah-Hartman wrote: > On Fri, Jul 29, 2022 at 10:00:42AM -0500, Pierre-Louis Bossart wrote: > > > > > > > diff --git a/drivers/soundwire/sysfs_slave_dpn.c b/drivers/soundwire/sysfs_slave_dpn.c > > > index c4b6543c09fd..a3fb380ee519 100644 > > > --- a/drivers/soundwire/sysfs_slave_dpn.c > > > +++ b/drivers/soundwire/sysfs_slave_dpn.c > > > @@ -283,6 +283,9 @@ int sdw_slave_sysfs_dpn_init(struct sdw_slave *slave) > > > int ret; > > > int i; > > > > > > + if (!slave->prop.source_ports && !slave->prop.sink_ports) > > > + return 0; > > > + > > > mask = slave->prop.source_ports; > > > for_each_set_bit(i, &mask, 32) { > > > ret = add_all_attributes(&slave->dev, i, 1); > > > > I am struggling with this one since the driver is still adding > > attributes manually. You mentioned in the other thread that > > > > " > > That's what the is_visible() callback is for in the groups structure, > > you determine if the attribute is visable or not at runtime, you don't > > rely on the driver itself to add/remove attributes, that does not scale > > and again, is racy. > > " > > > > I interpret that as "there's still a race here", no? > > Yes, there is, BUT as you are creating all of these attributes "on the > fly" for now, I don't see a simple conversion to fix that up. Let me do > these, the easy ones first. Your dynamic attribute allocations are the > harder things to do, let me think about those after I've fixed the rest > of the tree up with the trivial ones :) Sounds good to me.. Yes the dynamic ones are the one that need attention. How do you propose to handle these?
diff --git a/drivers/soundwire/bus_type.c b/drivers/soundwire/bus_type.c index 81c77e6ddbad..4e4e62d1e475 100644 --- a/drivers/soundwire/bus_type.c +++ b/drivers/soundwire/bus_type.c @@ -121,8 +121,8 @@ static int sdw_drv_probe(struct device *dev) if (slave->ops && slave->ops->read_prop) slave->ops->read_prop(slave); - /* init the sysfs as we have properties now */ - ret = sdw_slave_sysfs_init(slave); + /* init the dynamic sysfs attributes we need */ + ret = sdw_slave_sysfs_dpn_init(slave); if (ret < 0) dev_warn(dev, "Slave sysfs init failed:%d\n", ret); diff --git a/drivers/soundwire/sysfs_local.h b/drivers/soundwire/sysfs_local.h index 3ab8658a7782..fa048e112629 100644 --- a/drivers/soundwire/sysfs_local.h +++ b/drivers/soundwire/sysfs_local.h @@ -15,7 +15,6 @@ extern const struct attribute_group *sdw_slave_status_attr_groups[]; extern const struct attribute_group *sdw_attr_groups[]; /* additional device-managed properties reported after driver probe */ -int sdw_slave_sysfs_init(struct sdw_slave *slave); int sdw_slave_sysfs_dpn_init(struct sdw_slave *slave); #endif /* __SDW_SYSFS_LOCAL_H */ diff --git a/drivers/soundwire/sysfs_slave.c b/drivers/soundwire/sysfs_slave.c index 4c716c167493..070e0d84be94 100644 --- a/drivers/soundwire/sysfs_slave.c +++ b/drivers/soundwire/sysfs_slave.c @@ -211,19 +211,6 @@ const struct attribute_group *sdw_attr_groups[] = { NULL, }; -int sdw_slave_sysfs_init(struct sdw_slave *slave) -{ - int ret; - - if (slave->prop.source_ports || slave->prop.sink_ports) { - ret = sdw_slave_sysfs_dpn_init(slave); - if (ret < 0) - return ret; - } - - return 0; -} - /* * the status is shown in capital letters for UNATTACHED and RESERVED * on purpose, to highligh users to the fact that these status values diff --git a/drivers/soundwire/sysfs_slave_dpn.c b/drivers/soundwire/sysfs_slave_dpn.c index c4b6543c09fd..a3fb380ee519 100644 --- a/drivers/soundwire/sysfs_slave_dpn.c +++ b/drivers/soundwire/sysfs_slave_dpn.c @@ -283,6 +283,9 @@ int sdw_slave_sysfs_dpn_init(struct sdw_slave *slave) int ret; int i; + if (!slave->prop.source_ports && !slave->prop.sink_ports) + return 0; + mask = slave->prop.source_ports; for_each_set_bit(i, &mask, 32) { ret = add_all_attributes(&slave->dev, i, 1);
Now that sdw_slave_sysfs_init() only calls sdw_slave_sysfs_dpn_init(), just do that instead and remove sdw_slave_sysfs_init() to get it out of the way to save a bit of logic and code size. Cc: Vinod Koul <vkoul@kernel.org> Cc: Bard Liao <yung-chuan.liao@linux.intel.com> Cc: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> Cc: Sanyog Kale <sanyog.r.kale@intel.com> Cc: alsa-devel@alsa-project.org Cc: linux-kernel@vger.kernel.org Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> --- drivers/soundwire/bus_type.c | 4 ++-- drivers/soundwire/sysfs_local.h | 1 - drivers/soundwire/sysfs_slave.c | 13 ------------- drivers/soundwire/sysfs_slave_dpn.c | 3 +++ 4 files changed, 5 insertions(+), 16 deletions(-)