| Message ID | 20231004130243.493617-1-krzysztof.kozlowski@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | soundwire: fix initializing sysfs for same devices on different buses | expand |
On Wed, Oct 04, 2023 at 03:02:43PM +0200, 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: > > 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(-) > > 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, Shouldn't some documenation also be changed somewhere that describes the device id? thanks, greg k-h
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 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, > > 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(-) > > 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); > } >
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.
On Wed, Oct 04, 2023 at 09:57:49AM -0400, Pierre-Louis Bossart wrote: > > > 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. 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. thanks, greg k-h
>>>>> 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.
On Wed, Oct 04, 2023 at 11:16:09AM -0400, Pierre-Louis Bossart wrote: > > 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. FWIW DT is much less affected here since all the inter-device references are explicit in the DT (modulo needing to work around breakage) so we're not hard coding in the way ACPI so unfortunately requires.
On 10/4/23 11:40, Mark Brown wrote: > On Wed, Oct 04, 2023 at 11:16:09AM -0400, Pierre-Louis Bossart wrote: > >>> 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. > > FWIW DT is much less affected here since all the inter-device references > are explicit in the DT (modulo needing to work around breakage) so we're > not hard coding in the way ACPI so unfortunately requires. Isn't there a contradiction between making "all inter-device references explicit in the DT" and having a device name use an IDA, which cannot possibly known ahead of time? 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.
On Wed, Oct 04, 2023 at 03:00:40PM -0400, Pierre-Louis Bossart wrote: > On 10/4/23 11:40, Mark Brown wrote: > > FWIW DT is much less affected here since all the inter-device references > > are explicit in the DT (modulo needing to work around breakage) so we're > > not hard coding in the way ACPI so unfortunately requires. > Isn't there a contradiction between making "all inter-device references > explicit in the DT" and having a device name use an IDA, which cannot > possibly known ahead of time? No, the thing with DT is that we don't use the device name for binding at all - it's printed in things but it's not part of how we do lookups (unless there's something I didn't notice in the Soundwire specifics I guess). Lookups are done with inter-node references in the DT.
> 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(-)