Thread: [PATCH] Fix missing sysfs vendor/model entries for some devices
Brought to you by:
aeb,
bencollins
From: Adam G. <ad...@po...> - 2023-11-26 10:56:47
|
Hi, When the Sony DVMC-DA1 is connected, /sys/bus/firewire/devices/fw1/ has a vendor entry, but is missing vendor_name, model, and model_name. This is the DVMC-DA1's config ROM: 00000000: fbe7 1e04 3439 3331 0040 64e0 0346 0008 ....4931.@d..F.. 00000010: 3c19 1400 81b6 0600 4600 0803 c083 000c <.......F....... 00000020: 0a00 008d 0300 00d1 0500 00c3 0a00 0081 ................ 00000030: bfcd 0200 2da0 0012 0000 0113 fe0c 0200 ....-........... 00000040: 0000 fa17 0800 0081 6ec6 0200 0346 0008 ........n....F.. 00000050: 3c19 1400 269e 0300 0000 0000 0000 0000 <...&........... 00000060: 796e 6f53 1d00 0500 0000 0000 0000 0000 ynoS............ 00000070: 434d 5644 3141 442d 0000 0000 CMVD1AD-.... Note that the root directory contains two entries for vendor. The first is a Vendor_ID immediate value, and the second is a Vendor_Info directory. The descriptor for vendor_name is after the Vendor_Info entry. However, search_leaf compares the directory entry high byte against the desired key_id without masking off the type bits, so it doesn't recognize the Vendor_Info entry. Also, the model and model_name are not in the root directory, but instead are in the vendor directory. search_leaf and show_immediate don't see these, because they only search the root directory. The following patch fixes the above problems. The new behavior is conformant with Annex A of 1394TA Document 1999027, Configuration ROM for AV/C Devices 1.0: "Some legacy devices may have their Model_IDs and textual descriptors in minimal ASCII subset in their vendor directories. It is recommended for a controller to read the Model_ID and its descriptor in the vendor directory of a target only if the Model_ID and its descriptor are not present in the root directory of the target." In addition to the DVMC-DA1, other devices are also affected. This patch also fixes the missing sysfs entries for the Sony DCR-TRV120 and Panasonic AG-DV1DC. The Sony DCR-TRV310 has been reported to have the same problem and should be fixed by the patch, but I don't have one for testing. --- core-device.c.orig 2023-09-23 02:11:13.000000000 -0700 +++ core-device.c 2023-11-25 20:58:02.000000000 -0800 @@ -54,13 +54,19 @@ 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; last_key = key; } + /* Not in root directory, check vendor directory if present */ + fw_csr_iterator_init(&ci, directory); + while (fw_csr_iterator_next(&ci, &key, &value)) + if (key == (CSR_VENDOR | CSR_DIRECTORY)) + return search_leaf(ci.p - 1 + value, search_key); + return NULL; } @@ -245,14 +251,32 @@ u32 key; }; +static int search_immediate(const u32 *directory, int search_key) +{ + struct fw_csr_iterator ci; + int key, value; + + fw_csr_iterator_init(&ci, directory); + while (fw_csr_iterator_next(&ci, &key, &value)) + if (key == search_key) + return value; + + /* Not in root directory, check vendor directory if present */ + fw_csr_iterator_init(&ci, directory); + while (fw_csr_iterator_next(&ci, &key, &value)) + if (key == (CSR_VENDOR | CSR_DIRECTORY)) + return search_immediate(ci.p - 1 + value, search_key); + + return -1; +} + static ssize_t show_immediate(struct device *dev, struct device_attribute *dattr, char *buf) { struct config_rom_attribute *attr = container_of(dattr, struct config_rom_attribute, attr); - struct fw_csr_iterator ci; const u32 *dir; - int key, value, ret = -ENOENT; + int value, ret = -ENOENT; down_read(&fw_device_rwsem); @@ -261,13 +285,9 @@ else dir = fw_device(dev)->config_rom + 5; - fw_csr_iterator_init(&ci, dir); - while (fw_csr_iterator_next(&ci, &key, &value)) - if (attr->key == key) { - ret = snprintf(buf, buf ? PAGE_SIZE : 0, - "0x%06x\n", value); - break; - } + value = search_immediate(dir, attr->key); + if (value != -1) + ret = snprintf(buf, buf ? PAGE_SIZE : 0, "0x%06x\n", value); up_read(&fw_device_rwsem); |
From: Takashi S. <o-t...@sa...> - 2023-11-27 10:56:01
|
On Sun, Nov 26, 2023 at 02:37:56AM -0800, Adam Goldman wrote: > Hi, > > When the Sony DVMC-DA1 is connected, /sys/bus/firewire/devices/fw1/ has > a vendor entry, but is missing vendor_name, model, and model_name. > > This is the DVMC-DA1's config ROM: > > 00000000: fbe7 1e04 3439 3331 0040 64e0 0346 0008 ....4931.@d..F.. > 00000010: 3c19 1400 81b6 0600 4600 0803 c083 000c <.......F....... > 00000020: 0a00 008d 0300 00d1 0500 00c3 0a00 0081 ................ > 00000030: bfcd 0200 2da0 0012 0000 0113 fe0c 0200 ....-........... > 00000040: 0000 fa17 0800 0081 6ec6 0200 0346 0008 ........n....F.. > 00000050: 3c19 1400 269e 0300 0000 0000 0000 0000 <...&........... > 00000060: 796e 6f53 1d00 0500 0000 0000 0000 0000 ynoS............ > 00000070: 434d 5644 3141 442d 0000 0000 CMVD1AD-.... > > Note that the root directory contains two entries for vendor. The first > is a Vendor_ID immediate value, and the second is a Vendor_Info > directory. The descriptor for vendor_name is after the Vendor_Info > entry. However, search_leaf compares the directory entry high byte > against the desired key_id without masking off the type bits, so it > doesn't recognize the Vendor_Info entry. > > Also, the model and model_name are not in the root directory, but > instead are in the vendor directory. search_leaf and show_immediate > don't see these, because they only search the root directory. > > The following patch fixes the above problems. > > The new behavior is conformant with Annex A of 1394TA Document 1999027, > Configuration ROM for AV/C Devices 1.0: "Some legacy devices may have > their Model_IDs and textual descriptors in minimal ASCII subset in their > vendor directories. It is recommended for a controller to read the > Model_ID and its descriptor in the vendor directory of a target only if > the Model_ID and its descriptor are not present in the root directory of > the target." > > In addition to the DVMC-DA1, other devices are also affected. This patch > also fixes the missing sysfs entries for the Sony DCR-TRV120 and > Panasonic AG-DV1DC. The Sony DCR-TRV310 has been reported to have the > same problem and should be fixed by the patch, but I don't have one for > testing. Thanks for the patch. Indeed, for the legacy layout of Configuration ROM, current implementation of core functionality does not pick up enough information from the vendor directory. I think you already know the similar case in the issue filed in systemd project[1]. The patch format is not compliant to Linux kernel development, while I would evaluate it. However, I need more actual examples. Would I ask you to provide images of Configuration ROM retrieved by your test devices? I think you own Sony DCR-TRV120 and Panasonic AG-DV1DC. I maintain collections of Configuration ROM[2] and it is preferable to receive merge request in github.com service. Of cource, you can send them directly to me, instead. I have a plan to move the collection to git.kernel.org, thus we take enough care of image license. I think it better to use CC0[3]. Well, I note that you can decode the content of Configuration ROM by 'config-rom-pretty-printer' in linux-firewire-utils. The tool is written by C language and can be build in your system, I believe: https://git.kernel.org/pub/scm/utils/ieee1394/linux-firewire-utils.git/ For example: ``` $ cat rom.img | config-rom-pretty-printer 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 ``` Finally, I apologize in advance that my patch review would be delayed since I'm involved to critical kernel regression in the combination of AMD Ryzen machine and a kind of 1394 OHCI cards[4]. It takes more days to review your patch, sorry. [1] IEEE1394_UNIT_FUNCTION_VIDEO not set on camcorder (because it does not have ATTR{model}?) https://github.com/systemd/systemd/issues/25029 [2] https://github.com/takaswie/am-config-roms/ [3] https://creativecommons.org/publicdomain/zero/1.0/ [4] https://lore.kernel.org/lkml/20231105144852.GA165906@workstation.local/ Thanks Takashi Sakamoto |
From: Adam G. <ad...@po...> - 2023-11-28 09:27:30
|
On Mon, Nov 27, 2023 at 07:40:03PM +0900, Takashi Sakamoto wrote: > Thanks for the patch. Indeed, for the legacy layout of Configuration ROM, > current implementation of core functionality does not pick up enough > information from the vendor directory. I think you already know the similar > case in the issue filed in systemd project. Thanks for your reply and for your work maintaining the FireWire subsystem. > The patch format is not compliant to Linux kernel development, while I > would evaluate it. Should I submit the patch again in canonical patch format[1]? > However, I need more actual examples. Would I ask you > to provide images of Configuration ROM retrieved by your test devices? > I think you own Sony DCR-TRV120 and Panasonic AG-DV1DC. I maintain > collections of Configuration ROM[2] and it is preferable to receive merge > request in github.com service. I've sent a merge request with the Configuration ROM images. [1] https://docs.kernel.org/process/submitting-patches.html#the-canonical-patch-format -- Adam |
From: Takashi S. <o-t...@sa...> - 2023-11-29 13:31:02
|
Hi, On Tue, Nov 28, 2023 at 01:27:03AM -0800, Adam Goldman wrote: > On Mon, Nov 27, 2023 at 07:40:03PM +0900, Takashi Sakamoto wrote: > > The patch format is not compliant to Linux kernel development, while I > > would evaluate it. > > Should I submit the patch again in canonical patch format[1]? Thanks for your care, however at this time I prepared remote branch and applied your patch[1]. I'd like to review it carefully so that it brings the other regressions since it has an effect to modalias for unit and device attributes for node exported via sysfs. After enough testing, I'll merge it to PR for v6.8 kernel. [1] https://git.kernel.org/pub/scm/linux/kernel/git/ieee1394/linux1394.git/log/?h=topic/support-legacy-layout-of-configuration-rom Thanks Takashi Sakamoto |