| Message ID | 20231004130243.493617-1-krzysztof.kozlowski@linaro.org |
|---|---|
| State | Accepted |
| Commit | 8a8a9ac8a4972ee69d3dd3d1ae43963ae39cee18 |
| Headers | show |
| Series | soundwire: fix initializing sysfs for same devices on different buses | expand |
On 04/10/2023 15:11, Greg Kroah-Hartman wrote: >> if (id->unique_id == SDW_IGNORED_UNIQUE_ID) { >> - /* name shall be sdw:link:mfg:part:class */ >> - dev_set_name(&slave->dev, "sdw:%01x:%04x:%04x:%02x", >> - bus->link_id, id->mfg_id, id->part_id, >> + /* name shall be sdw:bus:link:mfg:part:class */ >> + dev_set_name(&slave->dev, "sdw:%01x:%01x:%04x:%04x:%02x", >> + bus->id, bus->link_id, id->mfg_id, id->part_id, >> id->class_id); >> } else { >> - /* name shall be sdw:link:mfg:part:class:unique */ >> - dev_set_name(&slave->dev, "sdw:%01x:%04x:%04x:%02x:%01x", >> - bus->link_id, id->mfg_id, id->part_id, >> + /* name shall be sdw:bus:link:mfg:part:class:unique */ >> + dev_set_name(&slave->dev, "sdw:%01x:%01x:%04x:%04x:%02x:%01x", >> + bus->id, bus->link_id, id->mfg_id, id->part_id, > > Shouldn't some documenation also be changed somewhere that describes the > device id? +Cc Srini, The only reference I found is Documentation/ABI/testing/sysfs-bus-soundwire-slave [1] and it did not specify the format of each device name entry. Vinod, Srini, Pierre-Louis, Any hints from your side if we have it documented anywhere else? [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/ABI/testing/sysfs-bus-soundwire-slave?h=v6.6-rc4 Best regards, Krzysztof
On Wed, Oct 04, 2023 at 09:16:47AM -0400, Pierre-Louis Bossart wrote: > > > On 10/4/23 09:02, Krzysztof Kozlowski wrote: > > If same devices with same device IDs are present on different soundwire > > buses, the probe fails due to conflicting device names and sysfs > > entries: > > > > sysfs: cannot create duplicate filename '/bus/soundwire/devices/sdw:0:0217:0204:00:0' > > > > The link ID is 0 for both devices, so they should be differentiated by > > bus ID. Add the bus ID so, the device names and sysfs entries look > > like: > > I am pretty sure this will break Intel platforms by changing the device > names. > > sof_sdw.c: else if (is_unique_device(adr_link, sdw_version, mfg_id, > part_id, > sof_sdw.c: > "sdw:%01x:%04x:%04x:%02x", link_id, > sof_sdw.c: > "sdw:%01x:%04x:%04x:%02x:%01x", link_id, device id name changes shouldn't break things, what is requring them to look a specific way? thanks, greg k-h
On 10/4/23 09:38, Greg Kroah-Hartman wrote: > On Wed, Oct 04, 2023 at 09:16:47AM -0400, Pierre-Louis Bossart wrote: >> >> >> On 10/4/23 09:02, Krzysztof Kozlowski wrote: >>> If same devices with same device IDs are present on different soundwire >>> buses, the probe fails due to conflicting device names and sysfs >>> entries: >>> >>> sysfs: cannot create duplicate filename '/bus/soundwire/devices/sdw:0:0217:0204:00:0' >>> >>> The link ID is 0 for both devices, so they should be differentiated by >>> bus ID. Add the bus ID so, the device names and sysfs entries look >>> like: >> >> I am pretty sure this will break Intel platforms by changing the device >> names. >> >> sof_sdw.c: else if (is_unique_device(adr_link, sdw_version, mfg_id, >> part_id, >> sof_sdw.c: >> "sdw:%01x:%04x:%04x:%02x", link_id, >> sof_sdw.c: >> "sdw:%01x:%04x:%04x:%02x:%01x", link_id, > > device id name changes shouldn't break things, what is requring them to > look a specific way? it's the ASoC dailink creation that relies on strings, we have similar cases for I2C. There's no requirement that the name follows any specific convention, just that when you want to rely on a specific device for an ASoC card you need to use the string that matches its device name. I am not sure how we would modify the Intel machine driver though because the bus ID is IDA-based, so there's no way to predict what it might be.
>>>>> If same devices with same device IDs are present on different soundwire >>>>> buses, the probe fails due to conflicting device names and sysfs >>>>> entries: >>>>> >>>>> sysfs: cannot create duplicate filename '/bus/soundwire/devices/sdw:0:0217:0204:00:0' >>>>> >>>>> The link ID is 0 for both devices, so they should be differentiated by >>>>> bus ID. Add the bus ID so, the device names and sysfs entries look >>>>> like: >>>> >>>> I am pretty sure this will break Intel platforms by changing the device >>>> names. >>>> >>>> sof_sdw.c: else if (is_unique_device(adr_link, sdw_version, mfg_id, >>>> part_id, >>>> sof_sdw.c: >>>> "sdw:%01x:%04x:%04x:%02x", link_id, >>>> sof_sdw.c: >>>> "sdw:%01x:%04x:%04x:%02x:%01x", link_id, >>> >>> device id name changes shouldn't break things, what is requring them to >>> look a specific way? >> >> it's the ASoC dailink creation that relies on strings, we have similar >> cases for I2C. >> >> There's no requirement that the name follows any specific convention, >> just that when you want to rely on a specific device for an ASoC card >> you need to use the string that matches its device name. > > matching the name is fine (if you are matching it against an existing > name) but expecting the name to be anything specific is not going to > work as the name is dynamic and can/will change each boot. Not following, sorry. In the SoundWire context, the device name directly follows the ACPI or Device Tree information, I don't really see how its name could change on each boot (assuming no DSDT override or overlays of course). The platform descriptors are pretty much fixed, aren't they? Intel and AMD make such assumptions on names for pretty much all machine drivers, it's not really something new - probably 15+ years? Adding Mark Brown in CC: to make sure he's aware of this thread.
> I think we keep circling on the differences between "Controller" and > "link" (aka bus). A Controller can have one or more links. A system can > have one or more controllers. > > Intel platforms have one controller and 4 or more links. > QCOM platforms have one or more controllers with one link each. > > I am not sure how this IDA-generated bus_id helps deal with these two > cases, since we can't really make any assumptions on how > controllers/links will be started and probed. > > What we are missing is a hierarchical controller/link definition, IOW a > controller_id should be given to the master by a higher level instead of > using an IDA. Tentative patches to introduce a 'controller_id' that's not an IDA are here: https://github.com/thesofproject/linux/pull/4616 Initial results are positive for Intel devices. it *should* work for other devices but I can't test. If folks at Linaro/Qualcomm and AMD can give it a try, that would be much appreciated. Thanks.
On 05/10/23 17:54, Pierre-Louis Bossart wrote: > > >> I think we keep circling on the differences between "Controller" and >> "link" (aka bus). A Controller can have one or more links. A system can >> have one or more controllers. >> >> Intel platforms have one controller and 4 or more links. >> QCOM platforms have one or more controllers with one link each. >> >> I am not sure how this IDA-generated bus_id helps deal with these two >> cases, since we can't really make any assumptions on how >> controllers/links will be started and probed. >> >> What we are missing is a hierarchical controller/link definition, IOW a >> controller_id should be given to the master by a higher level instead of >> using an IDA. > Tentative patches to introduce a 'controller_id' that's not an IDA are > here: https://github.com/thesofproject/linux/pull/4616 > > Initial results are positive for Intel devices. it *should* work for > other devices but I can't test. If folks at Linaro/Qualcomm and AMD can > give it a try, that would be much appreciated. Will test on AMD platforms and let you know the result. > > Thanks.
On 05/10/23 18:08, Mukunda,Vijendar wrote: > On 05/10/23 17:54, Pierre-Louis Bossart wrote: >> >>> I think we keep circling on the differences between "Controller" and >>> "link" (aka bus). A Controller can have one or more links. A system can >>> have one or more controllers. >>> >>> Intel platforms have one controller and 4 or more links. >>> QCOM platforms have one or more controllers with one link each. >>> >>> I am not sure how this IDA-generated bus_id helps deal with these two >>> cases, since we can't really make any assumptions on how >>> controllers/links will be started and probed. >>> >>> What we are missing is a hierarchical controller/link definition, IOW a >>> controller_id should be given to the master by a higher level instead of >>> using an IDA. >> Tentative patches to introduce a 'controller_id' that's not an IDA are >> here: https://github.com/thesofproject/linux/pull/4616 >> >> Initial results are positive for Intel devices. it *should* work for >> other devices but I can't test. If folks at Linaro/Qualcomm and AMD can >> give it a try, that would be much appreciated. > Will test on AMD platforms and let you know the result. "soundwire: bus: introduce controller_id " patch needs to be modified and controller id should be set to zero for amd platforms as we are populating multiple links under same controller id. > >> Thanks.
diff --git a/drivers/soundwire/slave.c b/drivers/soundwire/slave.c index c1c1a2ac293a..4db43ea53d47 100644 --- a/drivers/soundwire/slave.c +++ b/drivers/soundwire/slave.c @@ -39,14 +39,14 @@ int sdw_slave_add(struct sdw_bus *bus, slave->dev.fwnode = fwnode; if (id->unique_id == SDW_IGNORED_UNIQUE_ID) { - /* name shall be sdw:link:mfg:part:class */ - dev_set_name(&slave->dev, "sdw:%01x:%04x:%04x:%02x", - bus->link_id, id->mfg_id, id->part_id, + /* name shall be sdw:bus:link:mfg:part:class */ + dev_set_name(&slave->dev, "sdw:%01x:%01x:%04x:%04x:%02x", + bus->id, bus->link_id, id->mfg_id, id->part_id, id->class_id); } else { - /* name shall be sdw:link:mfg:part:class:unique */ - dev_set_name(&slave->dev, "sdw:%01x:%04x:%04x:%02x:%01x", - bus->link_id, id->mfg_id, id->part_id, + /* name shall be sdw:bus:link:mfg:part:class:unique */ + dev_set_name(&slave->dev, "sdw:%01x:%01x:%04x:%04x:%02x:%01x", + bus->id, bus->link_id, id->mfg_id, id->part_id, id->class_id, id->unique_id); }
If same devices with same device IDs are present on different soundwire buses, the probe fails due to conflicting device names and sysfs entries: sysfs: cannot create duplicate filename '/bus/soundwire/devices/sdw:0:0217:0204:00:0' The link ID is 0 for both devices, so they should be differentiated by bus ID. Add the bus ID so, the device names and sysfs entries look like: sdw:1:0:0217:0204:00:0 -> ../../../devices/platform/soc@0/6ab0000.soundwire-controller/sdw-master-1/sdw:1:0:0217:0204:00:0 sdw:3:0:0217:0204:00:0 -> ../../../devices/platform/soc@0/6b10000.soundwire-controller/sdw-master-3/sdw:3:0:0217:0204:00:0 Fixes: 7c3cd189b86d ("soundwire: Add Master registration") Cc: <stable@vger.kernel.org> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> --- Sending as RFT, because I did not test it on that many devices and user-spaces. --- drivers/soundwire/slave.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)