Re: [PATCH] firewire: core: mask previous entry's type bits when looking for leaf

Re: [PATCH] firewire: core: mask previous entry's type bits when looking for leaf

[Takashi Sakamoto]

Hi,

On Thu, Jan 25, 2024 at 04:15:12AM -0800, Adam Goldman wrote:
> When searching a configuration ROM directory for a leaf corresponding to 
> a specific key_ID, disregard the type bits of the directory entry.
> 
> The configuration ROM of FireWire devices can contain textual descriptor 
> leafs (strings) for vendor name and model name. We use these to populate 
> the vendor_name and model_name entries in sysfs. Each descriptor leaf has a 
> descriptor directory entry pointing to it. The identity of the descriptor 
> leaf is indicated by the key_ID of the directory entry immediately before 
> the descriptor leaf's directory entry. The key_ID is the low 6 bits of the 
> high byte of the directory entry, and the type is the high 2 bits.
> 
> In most cases, the directory entry before a descriptor directory entry will 
> be an immediate value (type=0). However, it is also valid for it to be a 
> directory (type=3). Thus, when looking for a specific leaf, we must mask 
> off the type bits and compare only the key_ID.
> 
> One affected device is the Sony DVMC-DA1, which is missing the vendor_name 
> sysfs entry without this change.
> 
> The recent commit 141d9c6e003b806d8faeddeec7053ee2691ea61a fixed missing 
> model and model_name entries for the DVMC-DA1, but those were caused by a 
> different issue.
> 
> Signed-Off-By: Adam Goldman <adamg@pobox.com>
> ---
> 
> --- linux-6.8-rc1.orig/drivers/firewire/core-device.c	2024-01-21 14:11:32.000000000 -0800
> +++ linux-6.8-rc1/drivers/firewire/core-device.c	2024-01-24 03:56:56.000000000 -0800
> @@ -72,7 +72,7 @@
>  
>  	fw_csr_iterator_init(&ci, directory);
>  	while (fw_csr_iterator_next(&ci, &key, &value)) {
> -		if (last_key == search_key &&
> +		if ((last_key & 0x3f) == search_key &&
>  		    key == (CSR_DESCRIPTOR | CSR_LEAF))
>  			return ci.p - 1 + value;

Would I request you to update the API documentation of fw_csr_string()
as well and send the renewed patch as take 2?


I have a mixed feeling about the change, but I'll finally accept it since
we face the exception against documentation.

As you know, in Annex A of document for configuration ROM of AV/C
device[1], we can see the legacy layout of configuration ROM (page 22).
In the layout, the descriptor leaf entry for vendor name locates after
the immediate value entry for vendor ID, then the directory entry for
vendor directory locates. However, in the case of Sony DVMC-DA1, the
descriptor leaf entry locates after the directory entry. It is an
exception.

The change brings the behaviour change of fw_csr_string() since it is
currently coded to pick up the text pointed by the descriptor leaf entry
just after the immediate value entry. Fortunately, as long as I know, the
change is safe and brings no regression.

In the concept of software stack in kernel, whether the string is picked
up or not is not so important, since it is just for human readability.
It is just enough to pick up information to detect the relationship
between node and unit and their corresponding drivers. The parse of
configuration ROM should be done in userspace application again.
Actually the most of userspace applications are programmed so.

In the case of systemd udev/hwdb, it is not so preferable to put
device-dependent code, therefore the device attributes are important in
our case. But the vendor string provided by the stack is not necessarily
useful since hwdb can fulfill it now[2].

... But the above are just theories. The world has many complicated
things, I know. So I accept your proposal. But note that the preferable
way is to find the other devices which have the configuration ROM similar
to the case. We have already implemented the support for the legacy layout
of configuration ROM. It's time for you to start collecting the
configuration ROMs for digital video devices, like I did for audio and
music units in IEEE 1394 bus[3][4].

[1] Configuration ROM for AV/C Devices 1.0 (1394 Trading Association, Dec 2000,
TA Document 1999027)
https://web.archive.org/web/20210216003030/http://1394ta.org/wp-content/uploads/2015/07/1999027.pdf
[2] https://github.com/systemd/systemd/pull/31054
[3] https://github.com/takaswie/am-config-roms/
[4] https://github.com/systemd/systemd/blob/main/hwdb.d/80-ieee1394-unit-function.hwdb


Thanks

Takashi Sakamoto

Re: [PATCH] firewire: core: mask previous entry's type bits when looking for leaf

[Adam Goldman]

On Fri, Jan 26, 2024 at 10:17:05AM +0900, Takashi Sakamoto wrote:
> Would I request you to update the API documentation of fw_csr_string()
> as well and send the renewed patch as take 2?
> 
> 
> I have a mixed feeling about the change, but I'll finally accept it since
> we face the exception against documentation.
> 
> As you know, in Annex A of document for configuration ROM of AV/C
> device[1], we can see the legacy layout of configuration ROM (page 22).
> In the layout, the descriptor leaf entry for vendor name locates after
> the immediate value entry for vendor ID, then the directory entry for
> vendor directory locates. However, in the case of Sony DVMC-DA1, the
> descriptor leaf entry locates after the directory entry. It is an
> exception.

Hi Takashi,

Thank you for your review and feedback.

After checking the 1394TA Configuration ROM document again, I agree that 
the leaf entry for vendor name should be after an immediate value entry 
according to this standard. The DVMC-DA1 does not conform. We should 
consider its configuration ROM format to be another variation of the 
legacy format. This variation is not mentioned in Annex A.

I will update the API documentation of fw_csr_string() and send a 
revised patch.

-- Adam

Re: [PATCH] firewire: core: mask previous entry's type bits when looking for leaf

[Takashi Sakamoto]

Hi,

On Fri, Jan 26, 2024 at 12:49:35AM -0800, Adam Goldman wrote:
> On Fri, Jan 26, 2024 at 10:17:05AM +0900, Takashi Sakamoto wrote:
> > Would I request you to update the API documentation of fw_csr_string()
> > as well and send the renewed patch as take 2?
> > 
> > 
> > I have a mixed feeling about the change, but I'll finally accept it since
> > we face the exception against documentation.
> > 
> > As you know, in Annex A of document for configuration ROM of AV/C
> > device[1], we can see the legacy layout of configuration ROM (page 22).
> > In the layout, the descriptor leaf entry for vendor name locates after
> > the immediate value entry for vendor ID, then the directory entry for
> > vendor directory locates. However, in the case of Sony DVMC-DA1, the
> > descriptor leaf entry locates after the directory entry. It is an
> > exception.
> 
> Hi Takashi,
> 
> Thank you for your review and feedback.
> 
> After checking the 1394TA Configuration ROM document again, I agree that 
> the leaf entry for vendor name should be after an immediate value entry 
> according to this standard. The DVMC-DA1 does not conform. We should 
> consider its configuration ROM format to be another variation of the 
> legacy format. This variation is not mentioned in Annex A.
> 
> I will update the API documentation of fw_csr_string() and send a 
> revised patch.

I think we can handle the quirk of configuration ROM without changing
the kernel API. Would you test the following patch? (not tested in my
side).

======== 8< --------

From 83bf1e04d308ea89c76c64e3168b9701f9d9191b Mon Sep 17 00:00:00 2001
From: Takashi Sakamoto <o-takashi@sakamocchi.jp>
Date: Fri, 26 Jan 2024 20:37:21 +0900
Subject: [PATCH] firewire: search descriptor leaf just after vendor directory
 entry in root directory

It appears that Sony DVMC-DA1 has a quirk that the descriptor leaf entry
locates just after the vendor directory entry in root directory. This is
not equivalent to the legacy layout of configuration ROM described in
Configuration ROM for AV/C Devices 1.0 (1394 Trading Association, Dec 2000,
TA Document 1999027).

This commit changes current implementation to parse configuration ROM for
device attributes so that the descriptor leaf entry can be detected for
the vendor name.

$ config-rom-pretty-printer < Sony-DVMC-DA1.img
               ROM header and bus information block
               -----------------------------------------------------------------
1024  041ee7fb  bus_info_length 4, crc_length 30, crc 59387
1028  31333934  bus_name "1394"
1032  e0644000  irmc 1, cmc 1, isc 1, bmc 0, cyc_clk_acc 100, max_rec 4 (32)
1036  08004603  company_id 080046     |
1040  0014193c  device_id 12886219068  | EUI-64 576537731003586876

               root directory
               -----------------------------------------------------------------
1044  0006b681  directory_length 6, crc 46721
1048  03080046  vendor
1052  0c0083c0  node capabilities: per IEEE 1394
1056  8d00000a  --> eui-64 leaf at 1096
1060  d1000003  --> unit directory at 1072
1064  c3000005  --> vendor directory at 1084
1068  8100000a  --> descriptor leaf at 1108

               unit directory at 1072
               -----------------------------------------------------------------
1072  0002cdbf  directory_length 2, crc 52671
1076  1200a02d  specifier id
1080  13010000  version

               vendor directory at 1084
               -----------------------------------------------------------------
1084  00020cfe  directory_length 2, crc 3326
1088  17fa0000  model
1092  81000008  --> descriptor leaf at 1124

               eui-64 leaf at 1096
               -----------------------------------------------------------------
1096  0002c66e  leaf_length 2, crc 50798
1100  08004603  company_id 080046     |
1104  0014193c  device_id 12886219068  | EUI-64 576537731003586876

               descriptor leaf at 1108
               -----------------------------------------------------------------
1108  00039e26  leaf_length 3, crc 40486
1112  00000000  textual descriptor
1116  00000000  minimal ASCII
1120  536f6e79  "Sony"

               descriptor leaf at 1124
               -----------------------------------------------------------------
1124  0005001d  leaf_length 5, crc 29
1128  00000000  textual descriptor
1132  00000000  minimal ASCII
1136  44564d43  "DVMC"
1140  2d444131  "-DA1"
1144  00000000

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 drivers/firewire/core-device.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/firewire/core-device.c b/drivers/firewire/core-device.c
index 0547253d16fe..30a734ee9de9 100644
--- a/drivers/firewire/core-device.c
+++ b/drivers/firewire/core-device.c
@@ -368,8 +368,16 @@ static ssize_t show_text_leaf(struct device *dev,
 	for (i = 0; i < ARRAY_SIZE(directories) && !!directories[i]; ++i) {
 		int result = fw_csr_string(directories[i], attr->key, buf, bufsize);
 		// Detected.
-		if (result >= 0)
+		if (result >= 0) {
 			ret = result;
+		} else if (i == 0 && attr->key == CSR_VENDOR) {
+			// Sony DVMC-DA1 has configuration ROM such that the descriptor leaf entry
+			// in the root directory follows to the directory entry for vendor ID
+			// instead of the immediate value for vendor ID.
+			result = fw_csr_string(directories[i], CSR_DIRECTORY | attr->key, buf, bufsize);
+			if (result >= 0)
+				ret = result;
+		}
 	}
 
 	if (ret >= 0) {
-- 
2.40.1

======== 8< --------


Regards

Takashi Sakamoto

Re: [PATCH] firewire: core: mask previous entry's type bits when looking for leaf

[Adam Goldman]

On Fri, Jan 26, 2024 at 09:19:17PM +0900, Takashi Sakamoto wrote:
> I think we can handle the quirk of configuration ROM without changing
> the kernel API. Would you test the following patch? (not tested in my
> side).
> 
> ======== 8< --------
> 
> >From 83bf1e04d308ea89c76c64e3168b9701f9d9191b Mon Sep 17 00:00:00 2001
> From: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> Date: Fri, 26 Jan 2024 20:37:21 +0900
> Subject: [PATCH] firewire: search descriptor leaf just after vendor directory
>  entry in root directory

Hi Takashi,

I tested your patch with the DVMC-DA1. I also tested it with another 
device with normal placement of the leaf entry. In both cases, it works.

-- Adam

Re: [PATCH] firewire: core: mask previous entry's type bits when looking for leaf

[Takashi Sakamoto]

Hi,

On Fri, Jan 26, 2024 at 08:53:42PM -0800, Adam Goldman wrote:
> On Fri, Jan 26, 2024 at 09:19:17PM +0900, Takashi Sakamoto wrote:
> > I think we can handle the quirk of configuration ROM without changing
> > the kernel API. Would you test the following patch? (not tested in my
> > side).
> > 
> > ======== 8< --------
> > 
> > >From 83bf1e04d308ea89c76c64e3168b9701f9d9191b Mon Sep 17 00:00:00 2001
> > From: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> > Date: Fri, 26 Jan 2024 20:37:21 +0900
> > Subject: [PATCH] firewire: search descriptor leaf just after vendor directory
> >  entry in root directory
> 
> Hi Takashi,
> 
> I tested your patch with the DVMC-DA1. I also tested it with another 
> device with normal placement of the leaf entry. In both cases, it works.

Thanks for your test. I reposted the patch in the series of changes for
v6.8-rc3[1].

The behaviour change of kernel API is not preferable within the same
version of kernel once the release candidates is public, while we need to
handle it as the series of changes to support the legacy layout of
configuration ROM. So I'll apply my version to for-linus branch and send
it to him.

Anyway thanks for your work and suggestion.

[1] https://lore.kernel.org/lkml/20240130100409.30128-1-o-takashi@sakamocchi.jp/


Thanks

Takashi Sakamoto