Re: [PATCH] firewire: core: mask previous entry's type bits when looking for leaf
Brought to you by:
aeb,
bencollins
From: Takashi S. <o-t...@sa...> - 2024-01-26 01:17:23
|
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 <ad...@po...> > --- > > --- 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 |