[PATCH] soundwire: fix initializing sysfs for same devices on different buses

[PATCH] soundwire: fix initializing sysfs for same devices on different buses

[Krzysztof Kozlowski]

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,
 			     id->class_id, id->unique_id);
 	}
 
-- 
2.34.1

Re: [PATCH] soundwire: fix initializing sysfs for same devices on different buses

[Greg Kroah-Hartman]

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

Re: [PATCH] soundwire: fix initializing sysfs for same devices on different buses

[Krzysztof Kozlowski]

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

Re: [PATCH] soundwire: fix initializing sysfs for same devices on different buses

[Pierre-Louis Bossart]

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);
>  	}
>  

Re: [PATCH] soundwire: fix initializing sysfs for same devices on different buses

[Greg Kroah-Hartman]

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

Re: [PATCH] soundwire: fix initializing sysfs for same devices on different buses

[Pierre-Louis Bossart]

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.

Re: [PATCH] soundwire: fix initializing sysfs for same devices on different buses

[Greg Kroah-Hartman]

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

Re: [PATCH] soundwire: fix initializing sysfs for same devices on different buses

[Pierre-Louis Bossart]

>>>>> 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.

Re: [PATCH] soundwire: fix initializing sysfs for same devices on different buses

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 993 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] soundwire: fix initializing sysfs for same devices on different buses

[Pierre-Louis Bossart]

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.

Re: [PATCH] soundwire: fix initializing sysfs for same devices on different buses

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 783 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] soundwire: fix initializing sysfs for same devices on different buses

[Pierre-Louis Bossart]

> 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.

Re: [PATCH] soundwire: fix initializing sysfs for same devices on different buses

[Mukunda,Vijendar]

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.

Re: [PATCH] soundwire: fix initializing sysfs for same devices on different buses

[Mukunda,Vijendar]

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.