Received-SPF: pass (google.com: domain of linux-kernel+bounces-39487-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Feedback-ID: ie8e14432:Fastmail
Date: Fri, 26 Jan 2024 10:17:05 +0900
From: Takashi Sakamoto <o-takashi@sakamocchi.jp>
To: Adam Goldman <adamg@pobox.com>
Cc: linux1394-devel@lists.sourceforge.net, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] firewire: core: mask previous entry's type bits when
 looking for leaf
Message-ID: <20240126011705.GA22564@workstation.local>
Mail-Followup-To: Adam Goldman <adamg@pobox.com>,
	linux1394-devel@lists.sourceforge.net, linux-kernel@vger.kernel.org
References: <ZbJQ0JdbGixJouvn@iguana.24-8.net>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZbJQ0JdbGixJouvn@iguana.24-8.net>

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