linux1394-devel Mailing List for IEEE 1394 for Linux (Page 14)
Brought to you by:
aeb,
bencollins
You can subscribe to this list here.
2000 |
Jan
|
Feb
|
Mar
(39) |
Apr
(154) |
May
(172) |
Jun
(237) |
Jul
(127) |
Aug
(135) |
Sep
(193) |
Oct
(175) |
Nov
(173) |
Dec
(148) |
---|---|---|---|---|---|---|---|---|---|---|---|---|
2001 |
Jan
(161) |
Feb
(225) |
Mar
(193) |
Apr
(158) |
May
(179) |
Jun
(292) |
Jul
(146) |
Aug
(134) |
Sep
(185) |
Oct
(190) |
Nov
(149) |
Dec
(161) |
2002 |
Jan
(186) |
Feb
(236) |
Mar
(254) |
Apr
(207) |
May
(189) |
Jun
(182) |
Jul
(202) |
Aug
(155) |
Sep
(149) |
Oct
(449) |
Nov
(191) |
Dec
(108) |
2003 |
Jan
(174) |
Feb
(242) |
Mar
(243) |
Apr
(255) |
May
(202) |
Jun
(290) |
Jul
(237) |
Aug
(178) |
Sep
(101) |
Oct
(153) |
Nov
(144) |
Dec
(95) |
2004 |
Jan
(162) |
Feb
(278) |
Mar
(282) |
Apr
(152) |
May
(127) |
Jun
(138) |
Jul
(94) |
Aug
(63) |
Sep
(64) |
Oct
(150) |
Nov
(102) |
Dec
(197) |
2005 |
Jan
(102) |
Feb
(172) |
Mar
(89) |
Apr
(158) |
May
(139) |
Jun
(160) |
Jul
(288) |
Aug
(89) |
Sep
(201) |
Oct
(92) |
Nov
(190) |
Dec
(139) |
2006 |
Jan
(121) |
Feb
(204) |
Mar
(133) |
Apr
(134) |
May
(91) |
Jun
(226) |
Jul
(122) |
Aug
(101) |
Sep
(144) |
Oct
(141) |
Nov
|
Dec
|
2023 |
Jan
(19) |
Feb
(1) |
Mar
(5) |
Apr
(5) |
May
(33) |
Jun
(17) |
Jul
|
Aug
|
Sep
(3) |
Oct
(1) |
Nov
(5) |
Dec
(40) |
2024 |
Jan
(26) |
Feb
(14) |
Mar
(26) |
Apr
(46) |
May
(17) |
Jun
(47) |
Jul
(23) |
Aug
(72) |
Sep
(33) |
Oct
|
Nov
|
Dec
|
From: Takashi S. <o-t...@sa...> - 2023-12-20 04:18:28
|
Hi, This series of change is to update my previous post for RFC[1], and for merge. Current core function of Linux FireWire subsystem has support for legacy layout of configuration ROM, described in annex of 1394TA document[2]. However, in a point of device attributes (e.g. nodes in sysfs), there are differences between devices with the legacy and standard layout of configuration ROM. The differences bring some inconveniences to users[3]. The series includes changes to solve them. The series includes changes relevant to driver matching procedure and notification to user space, thus could easily bring functional regression. For safe, the series includes some KUnit applications to test the change. However, backward incompatibility is inevitable due to change of modalias for device corresponding to unit. As long as I investigated, any unit drivers in kernel are not affected by the change. Additionally, less applications in user space are not as well. I think we can be optimistic to the regression. Changes from RFC: * rename file and KUnit tests so that they are related to device attributes * fix traverse failure when detecting textual descriptor * remove redundant indication for CSR key type [1] [RFC PATCH 0/8] firewire: core: support legacy layout of configuration ROM for AV/C device https://lore.kernel.org/lkml/202...@sa.../ [2] Configuration ROM for AV/C Devices 1.0 (December 12, 2000, 1394 Trading Association, TA Document 1999027) https://web.archive.org/web/20210216003030/http://1394ta.org/wp-content/uploads/2015/07/1999027.pdf [3] [PATCH] Fix missing sysfs vendor/model entries for some devices https://sourceforge.net/p/linux1394/mailman/message/55802731/ Takashi Sakamoto (8): firewire: core: adds constant qualifier for local helper functions firewire: core: replace magic number with macro firewire: test: add KUnit test for device attributes firewire: test: add test of device attributes for simple AV/C device firewire: test: add test of device attributes for legacy AV/C device firewire: core: detect numeric model identifier for legacy layout of configuration ROM firewire: core: detect model name for legacy layout of configuration ROM firewire: core: change modalias of unit device with backward incompatibility drivers/firewire/.kunitconfig | 1 + drivers/firewire/Kconfig | 16 ++ drivers/firewire/core-device.c | 127 +++++++++--- drivers/firewire/device-attribute-test.c | 251 +++++++++++++++++++++++ 4 files changed, 368 insertions(+), 27 deletions(-) create mode 100644 drivers/firewire/device-attribute-test.c -- 2.39.2 |
From: Takashi S. <o-t...@sa...> - 2023-12-20 04:18:27
|
Some local functions just handles given argument as mutable, thus it is preferable to add constant qualifier to them. Signed-off-by: Takashi Sakamoto <o-t...@sa...> --- drivers/firewire/core-device.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/firewire/core-device.c b/drivers/firewire/core-device.c index eeda7cc59e27..96b0b43da863 100644 --- a/drivers/firewire/core-device.c +++ b/drivers/firewire/core-device.c @@ -171,7 +171,7 @@ static const struct ieee1394_device_id *unit_match(struct device *dev, return NULL; } -static bool is_fw_unit(struct device *dev); +static bool is_fw_unit(const struct device *dev); static int fw_unit_match(struct device *dev, struct device_driver *drv) { @@ -679,7 +679,7 @@ static struct device_type fw_unit_type = { .release = fw_unit_release, }; -static bool is_fw_unit(struct device *dev) +static bool is_fw_unit(const struct device *dev) { return dev->type == &fw_unit_type; } @@ -838,7 +838,7 @@ static struct device_type fw_device_type = { .release = fw_device_release, }; -static bool is_fw_device(struct device *dev) +static bool is_fw_device(const struct device *dev) { return dev->type == &fw_device_type; } -- 2.39.2 |
From: Takashi S. <o-t...@sa...> - 2023-12-20 00:48:34
|
Hi Greg, On Tue, Dec 19, 2023 at 03:59:32PM +0100, Greg Kroah-Hartman wrote: > Now that the driver core can properly handle constant struct bus_type, > move the fw_bus_type variable to be a constant structure as well, > placing it into read-only memory which can not be modified at runtime. > > Cc: Takashi Sakamoto <o-t...@sa...> > Cc: lin...@li... > Signed-off-by: Greg Kroah-Hartman <gr...@li...> > --- > drivers/firewire/core-device.c | 2 +- > include/linux/firewire.h | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) Thanks for the patch. I applied it to for-next[1], since the following commits at v6.4 kernel have already allowed to keep it as constant (your absolute work!). * commit ad8685d0f61a ("driver core: bus: constify bus_unregister()") * commit 00c4a3c47da7 ("driver core: bus: constantify bus_register()") * commit d492cc2573a0 ("driver core: device.h: make struct bus_type a const *") [1] https://git.kernel.org/pub/scm/linux/kernel/git/ieee1394/linux1394.git/log/?h=for-next Thanks Takashi Sakamoto |
From: Takashi S. <o-t...@sa...> - 2023-12-18 12:50:41
|
Hi, On Mon, Dec 18, 2023 at 02:04:52AM -0800, Adam Goldman wrote: > Hi, > > On Sun, Dec 17, 2023 at 07:30:10PM +0900, Takashi Sakamoto wrote: > > - ret = fw_csr_string(dir, attr->key, buf, bufsize); > > + for (i = 0; i < ARRAY_SIZE(directories) && directories[i]; ++i) > > + ret = fw_csr_string(directories[i], attr->key, buf, bufsize); > > I believe this is incorrect. If the attribute is in the first directory > searched, the loop will continue. The second loop iteration will set ret > to -ENOENT because the attribute isn't in the second directory. Then > show_text_leaf will return -ENOENT even though the attribute existed. Exactly. It is a bug. I think we can solve it by aligning the pointers of directory in reverse order within the array, like: ``` diff --git a/drivers/firewire/core-device.c b/drivers/firewire/core-device.c index d3fc3270a00b..adae3268291f 100644 --- a/drivers/firewire/core-device.c +++ b/drivers/firewire/core-device.c @@ -326,13 +326,17 @@ static ssize_t show_text_leaf(struct device *dev, directories[0] = fw_unit(dev)->directory; } else { const u32 *root_directory = fw_device(dev)->config_rom + ROOT_DIR_OFFSET; + const u32 *vendor_directory = search_directory(root_directory, CSR_VENDOR); - directories[0] = root_directory; - - // Legacy layout of configuration ROM described in Annex 1 of 'Configuration ROM - // for AV/C Devices 1.0 (December 12, 2000, 1394 Trading Association, TA Document - // 1999027)'. - directories[1] = search_directory(root_directory, CSR_VENDOR); + if (!vendor_directory) { + directories[0] = root_directory; + } else { + // Legacy layout of configuration ROM described in Annex 1 of + // 'Configuration ROM for AV/C Devices 1.0 (December 12, 2000, 1394 + // Trading Association, TA Document 1999027)'. + directories[0] = vendor_directory; + directories[1] = root_directory; + } } if (buf) { @@ -342,8 +346,11 @@ static ssize_t show_text_leaf(struct device *dev, bufsize = 1; } - for (i = 0; i < ARRAY_SIZE(directories) && directories[i]; ++i) + for (i = 0; i < ARRAY_SIZE(directories) && directories[i]; ++i) { ret = fw_csr_string(directories[i], attr->key, buf, bufsize); + if (ret >= 0) + break; + } if (ret >= 0) { /* Strip trailing whitespace and add newline. */ ``` Thanks Takashi Sakamoto |
From: Takashi S. <o-t...@sa...> - 2023-12-18 12:47:02
|
Hi, On Mon, Dec 18, 2023 at 02:11:42AM -0800, Adam Goldman wrote: > Hi, > > On Sun, Dec 17, 2023 at 07:30:12PM +0900, Takashi Sakamoto wrote: > > + vendor_directory = search_directory(root_directory, CSR_DIRECTORY | CSR_VENDOR); > > Setting the CSR_DIRECTORY bit here is extraneous since search_directory() also sets it. Indeed. It is redundant. Thanks Takashi Sakamoto |
From: Adam G. <ad...@po...> - 2023-12-18 10:12:00
|
Hi, On Sun, Dec 17, 2023 at 07:30:12PM +0900, Takashi Sakamoto wrote: > + vendor_directory = search_directory(root_directory, CSR_DIRECTORY | CSR_VENDOR); Setting the CSR_DIRECTORY bit here is extraneous since search_directory() also sets it. -- Adam |
From: Adam G. <ad...@po...> - 2023-12-18 10:05:10
|
Hi, On Sun, Dec 17, 2023 at 07:30:10PM +0900, Takashi Sakamoto wrote: > - ret = fw_csr_string(dir, attr->key, buf, bufsize); > + for (i = 0; i < ARRAY_SIZE(directories) && directories[i]; ++i) > + ret = fw_csr_string(directories[i], attr->key, buf, bufsize); I believe this is incorrect. If the attribute is in the first directory searched, the loop will continue. The second loop iteration will set ret to -ENOENT because the attribute isn't in the second directory. Then show_text_leaf will return -ENOENT even though the attribute existed. -- Adam |
From: Takashi S. <o-t...@sa...> - 2023-12-17 10:30:45
|
As the last part of support for legacy layout of configuration ROM, this commit traverse vendor directory as well as root directory when constructing modalias for unit device. The change brings loss of backward compatibility since it can fill 'mo' field which is 0 at current implementation in the case. However, we can be optimistic against regression for unit drivers in kernel, due to some points: 1. ALSA drivers for audio and music units uses Model fields to match device, however all of supported devices does not have such legacy layout. 2. the other unit drivers does not use Model field to match device. The rest of concern is user space application. The most of application just take care of node device and does not use the modalias of unit device, thus the change does not affect to them. Although, systemd project gets affects since it includes hwdb to take udev to configure fw character device conveniently. I have a plan to work systemd so that the access permission of character device is kept for the previous and next version of Linux kernel. Signed-off-by: Takashi Sakamoto <o-t...@sa...> --- drivers/firewire/core-device.c | 21 +++++++++++++++++++-- drivers/firewire/csr-api-test.c | 2 +- 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/drivers/firewire/core-device.c b/drivers/firewire/core-device.c index f7a11004f972..e04486c5e0eb 100644 --- a/drivers/firewire/core-device.c +++ b/drivers/firewire/core-device.c @@ -153,8 +153,25 @@ static void get_ids(const u32 *directory, int *id) static void get_modalias_ids(const struct fw_unit *unit, int *id) { - get_ids(&fw_parent_device(unit)->config_rom[ROOT_DIR_OFFSET], id); - get_ids(unit->directory, id); + const u32 *root_directory = &fw_parent_device(unit)->config_rom[ROOT_DIR_OFFSET]; + const u32 *directories[] = {NULL, NULL, NULL}; + const u32 *vendor_directory; + int i; + + directories[0] = root_directory; + + // Legacy layout of configuration ROM described in Annex 1 of 'Configuration ROM for AV/C + // Devices 1.0 (December 12, 2000, 1394 Trading Association, TA Document 1999027)'. + vendor_directory = search_directory(root_directory, CSR_DIRECTORY | CSR_VENDOR); + if (!vendor_directory) { + directories[1] = unit->directory; + } else { + directories[1] = vendor_directory; + directories[2] = unit->directory; + } + + for (i = 0; i < ARRAY_SIZE(directories) && directories[i]; ++i) + get_ids(directories[i], id); } static bool match_ids(const struct ieee1394_device_id *id_table, int *id) diff --git a/drivers/firewire/csr-api-test.c b/drivers/firewire/csr-api-test.c index 779146d0cfba..753d8e83b8c2 100644 --- a/drivers/firewire/csr-api-test.c +++ b/drivers/firewire/csr-api-test.c @@ -178,7 +178,7 @@ static void csr_api_legacy_avc_device(struct kunit *test) }; struct device *node_dev = (struct device *)&node.device; struct device *unit0_dev = (struct device *)&unit0.device; - static const int unit_expected_ids[] = {0x00012345, 0x00000000, 0x00abcdef, 0x00543210}; + static const int unit_expected_ids[] = {0x00012345, 0x00fedcba, 0x00abcdef, 0x00543210}; char *buf = kunit_kzalloc(test, PAGE_SIZE, GFP_KERNEL); int ids[4] = {0, 0, 0, 0}; -- 2.39.2 |
From: Takashi S. <o-t...@sa...> - 2023-12-17 10:30:45
|
Some legacy devices have configuration ROM against standard AV/C device. They have vendor directory to store model identifier. It is described in annex of the following document. - Configuration ROM for AV/C Devices 1.0 (Dec. 12, 2000, 1394 Trading Association) In the case, current implementation of core function does not detect the model identifier, thus device attributes and modalias of unit have lack of it. Another KUnit test is required for the case. The following output is the parse result by config-rom-pretty-printer in linux-firewire-utils (https://git.kernel.org/pub/scm/utils/ieee1394/linux-firewire-utils.git/). The content of image is written by my hand. $ config-rom-pretty-printer < /tmp/rom.img ROM header and bus information block ----------------------------------------------------------------- 1024 04199fe7 bus_info_length 4, crc_length 25, crc 40935 1028 31333934 bus_name "1394" 1032 e0644000 irmc 1, cmc 1, isc 1, bmc 0, cyc_clk_acc 100, max_rec 4 (32) 1036 00112233 company_id 001122 | 1040 44556677 device_id 220189779575 | EUI-64 4822678189205111 root directory ----------------------------------------------------------------- 1044 0005dace directory_length 5, crc 56014 1048 03012345 vendor 1052 0c0083c0 node capabilities: per IEEE 1394 1056 8d000009 --> eui-64 leaf at 1092 1060 d1000002 --> unit directory at 1068 1064 c3000004 --> vendor directory at 1080 unit directory at 1068 ----------------------------------------------------------------- 1068 0002e107 directory_length 2, crc 57607 1072 12abcdef specifier id 1076 13543210 version vendor directory at 1080 ----------------------------------------------------------------- 1080 0002cb73 directory_length 2, crc 52083 1084 17fedcba model 1088 81000004 --> descriptor leaf at 1104 eui-64 leaf at 1092 ----------------------------------------------------------------- 1092 00026dc1 leaf_length 2, crc 28097 1096 00112233 company_id 001122 | 1100 44556677 device_id 220189779575 | EUI-64 4822678189205111 descriptor leaf at 1104 ----------------------------------------------------------------- 1104 00050e84 leaf_length 5, crc 3716 1108 00000000 textual descriptor 1112 00000000 minimal ASCII 1116 41424344 "ABCD" 1120 45464748 "EFGH" 1124 494a0000 "IJ" Signed-off-by: Takashi Sakamoto <o-t...@sa...> --- drivers/firewire/csr-api-test.c | 111 ++++++++++++++++++++++++++++++++ 1 file changed, 111 insertions(+) diff --git a/drivers/firewire/csr-api-test.c b/drivers/firewire/csr-api-test.c index d91852825efc..2430891c5dcb 100644 --- a/drivers/firewire/csr-api-test.c +++ b/drivers/firewire/csr-api-test.c @@ -45,6 +45,40 @@ static const u32 simple_avc_device[] = { 0x6d650000, }; +// Ibid. +// Annex A:Consideration for configuration ROM reader design (informative) +// A.1 Vendor directory +// +// Written by hand. +static const u32 legacy_avc_device[] = { + 0x04199fe7, + 0x31333934, + 0xe0644000, + 0x00112233, + 0x44556677, + 0x0005dace, // root directory. + 0x03012345, + 0x0c0083c0, + 0x8d000009, + 0xd1000002, + 0xc3000004, + 0x0002e107, // unit 0 directory. + 0x12abcdef, + 0x13543210, + 0x0002cb73, // vendor directory. + 0x17fedcba, + 0x81000004, + 0x00026dc1, // leaf for EUI-64. + 0x00112233, + 0x44556677, + 0x00050e84, // leaf for text descriptor. + 0x00000000, + 0x00000000, + 0x41424344, + 0x45464748, + 0x494a0000, +}; + static void csr_api_simple_avc_device(struct kunit *test) { static const struct fw_device node = { @@ -126,8 +160,85 @@ static void csr_api_simple_avc_device(struct kunit *test) KUNIT_EXPECT_MEMEQ(test, ids, unit_expected_ids, sizeof(ids)); } +static void csr_api_legacy_avc_device(struct kunit *test) +{ + static const struct fw_device node = { + .device = { + .type = &fw_device_type, + }, + .config_rom = legacy_avc_device, + .config_rom_length = sizeof(legacy_avc_device), + }; + static const struct fw_unit unit0 = { + .device = { + .type = &fw_unit_type, + .parent = (struct device *)&node.device, + }, + .directory = &legacy_avc_device[11], + }; + struct device *node_dev = (struct device *)&node.device; + struct device *unit0_dev = (struct device *)&unit0.device; + static const int unit_expected_ids[] = {0x00012345, 0x00000000, 0x00abcdef, 0x00543210}; + char *buf = kunit_kzalloc(test, PAGE_SIZE, GFP_KERNEL); + int ids[4] = {0, 0, 0, 0}; + + // Ensure associations for node and unit devices. + + KUNIT_ASSERT_TRUE(test, is_fw_device(node_dev)); + KUNIT_ASSERT_FALSE(test, is_fw_unit(node_dev)); + KUNIT_ASSERT_PTR_EQ(test, fw_device((node_dev)), &node); + + KUNIT_ASSERT_FALSE(test, is_fw_device(unit0_dev)); + KUNIT_ASSERT_TRUE(test, is_fw_unit(unit0_dev)); + KUNIT_ASSERT_PTR_EQ(test, fw_parent_device((&unit0)), &node); + KUNIT_ASSERT_PTR_EQ(test, fw_unit(unit0_dev), &unit0); + + // For entries in root directory. + + // Vendor immediate entry is found. + KUNIT_EXPECT_GT(test, show_immediate(node_dev, &config_rom_attributes[0].attr, buf), 0); + KUNIT_EXPECT_STREQ(test, buf, "0x012345\n"); + + // Model immediate entry is not found. + KUNIT_EXPECT_LT(test, show_immediate(node_dev, &config_rom_attributes[4].attr, buf), 0); + + // Descriptor leaf entry for vendor is not found. + KUNIT_EXPECT_LT(test, show_text_leaf(node_dev, &config_rom_attributes[5].attr, buf), 0); + + // Descriptor leaf entry for model is not found. + KUNIT_EXPECT_LT(test, show_text_leaf(node_dev, &config_rom_attributes[6].attr, buf), 0); + + // For entries in unit 0 directory. + + // Vendor immediate entry is not found. + KUNIT_EXPECT_LT(test, show_immediate(unit0_dev, &config_rom_attributes[0].attr, buf), 0); + + // Model immediate entry is not found. + KUNIT_EXPECT_LT(test, show_immediate(unit0_dev, &config_rom_attributes[4].attr, buf), 0); + + // Descriptor leaf entry for vendor is not found. + KUNIT_EXPECT_LT(test, show_text_leaf(unit0_dev, &config_rom_attributes[5].attr, buf), 0); + + // Descriptor leaf entry for model is not found. + KUNIT_EXPECT_LT(test, show_text_leaf(unit0_dev, &config_rom_attributes[6].attr, buf), 0); + + // Specifier_ID immediate entry is found. + KUNIT_EXPECT_GT(test, show_immediate(unit0_dev, &config_rom_attributes[2].attr, buf), 0); + KUNIT_EXPECT_STREQ(test, buf, "0xabcdef\n"); + + // Version immediate entry is found. + KUNIT_EXPECT_GT(test, show_immediate(unit0_dev, &config_rom_attributes[3].attr, buf), 0); + KUNIT_EXPECT_STREQ(test, buf, "0x543210\n"); + + kunit_kfree(test, buf); + + get_modalias_ids(&unit0, ids); + KUNIT_EXPECT_MEMEQ(test, ids, unit_expected_ids, sizeof(ids)); +} + static struct kunit_case csr_api_test_cases[] = { KUNIT_CASE(csr_api_simple_avc_device), + KUNIT_CASE(csr_api_legacy_avc_device), {} }; -- 2.39.2 |
From: Takashi S. <o-t...@sa...> - 2023-12-17 10:30:45
|
As the last part of support for legacy layout of configuration ROM, this commit traverse vendor directory as well as root directory when constructing modalias for unit device. The change brings loss of backward compatibility since it can fill 'mo' field which is 0 at current implementation in the case. However, we can be optimistic against regression for unit drivers in kernel, due to some points: 1. ALSA drivers for audio and music units uses Model fields to match device, however all of supported devices does not have such legacy layout. 2. the other unit drivers does not use Model field to match device. The rest of concern is user space application. The most of application just take care of node device and does not use the modalias of unit device, thus the change does not affect to them. Although, systemd project gets affects since it includes hwdb to take udev to configure fw character device conveniently. I have a plan to work systemd so that the access permission of character device is kept for the previous and next version of Linux kernel. Signed-off-by: Takashi Sakamoto <o-t...@sa...> --- drivers/firewire/core-device.c | 21 +++++++++++++++++++-- drivers/firewire/csr-api-test.c | 2 +- 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/drivers/firewire/core-device.c b/drivers/firewire/core-device.c index f7a11004f972..e04486c5e0eb 100644 --- a/drivers/firewire/core-device.c +++ b/drivers/firewire/core-device.c @@ -153,8 +153,25 @@ static void get_ids(const u32 *directory, int *id) static void get_modalias_ids(const struct fw_unit *unit, int *id) { - get_ids(&fw_parent_device(unit)->config_rom[ROOT_DIR_OFFSET], id); - get_ids(unit->directory, id); + const u32 *root_directory = &fw_parent_device(unit)->config_rom[ROOT_DIR_OFFSET]; + const u32 *directories[] = {NULL, NULL, NULL}; + const u32 *vendor_directory; + int i; + + directories[0] = root_directory; + + // Legacy layout of configuration ROM described in Annex 1 of 'Configuration ROM for AV/C + // Devices 1.0 (December 12, 2000, 1394 Trading Association, TA Document 1999027)'. + vendor_directory = search_directory(root_directory, CSR_DIRECTORY | CSR_VENDOR); + if (!vendor_directory) { + directories[1] = unit->directory; + } else { + directories[1] = vendor_directory; + directories[2] = unit->directory; + } + + for (i = 0; i < ARRAY_SIZE(directories) && directories[i]; ++i) + get_ids(directories[i], id); } static bool match_ids(const struct ieee1394_device_id *id_table, int *id) diff --git a/drivers/firewire/csr-api-test.c b/drivers/firewire/csr-api-test.c index 779146d0cfba..753d8e83b8c2 100644 --- a/drivers/firewire/csr-api-test.c +++ b/drivers/firewire/csr-api-test.c @@ -178,7 +178,7 @@ static void csr_api_legacy_avc_device(struct kunit *test) }; struct device *node_dev = (struct device *)&node.device; struct device *unit0_dev = (struct device *)&unit0.device; - static const int unit_expected_ids[] = {0x00012345, 0x00000000, 0x00abcdef, 0x00543210}; + static const int unit_expected_ids[] = {0x00012345, 0x00fedcba, 0x00abcdef, 0x00543210}; char *buf = kunit_kzalloc(test, PAGE_SIZE, GFP_KERNEL); int ids[4] = {0, 0, 0, 0}; -- 2.39.2 |
From: Takashi S. <o-t...@sa...> - 2023-12-17 10:30:40
|
As the part of support for legacy layout of configuration ROM, this commit traverse vendor directory as well as root directory when showing device attribute for node device. This change expects 'model_name' attribute appears in node device, however it is probable to see the other types of descriptor leaf if the vendor directory includes. Signed-off-by: Takashi Sakamoto <o-t...@sa...> --- drivers/firewire/core-device.c | 23 ++++++++++++++++------- drivers/firewire/csr-api-test.c | 5 +++-- 2 files changed, 19 insertions(+), 9 deletions(-) diff --git a/drivers/firewire/core-device.c b/drivers/firewire/core-device.c index dad5c9937b78..f7a11004f972 100644 --- a/drivers/firewire/core-device.c +++ b/drivers/firewire/core-device.c @@ -315,17 +315,25 @@ static ssize_t show_text_leaf(struct device *dev, { struct config_rom_attribute *attr = container_of(dattr, struct config_rom_attribute, attr); - const u32 *dir; + const u32 *directories[] = {NULL, NULL}; size_t bufsize; char dummy_buf[2]; - int ret; + int i, ret; down_read(&fw_device_rwsem); - if (is_fw_unit(dev)) - dir = fw_unit(dev)->directory; - else - dir = fw_device(dev)->config_rom + ROOT_DIR_OFFSET; + if (is_fw_unit(dev)) { + directories[0] = fw_unit(dev)->directory; + } else { + const u32 *root_directory = fw_device(dev)->config_rom + ROOT_DIR_OFFSET; + + directories[0] = root_directory; + + // Legacy layout of configuration ROM described in Annex 1 of 'Configuration ROM + // for AV/C Devices 1.0 (December 12, 2000, 1394 Trading Association, TA Document + // 1999027)'. + directories[1] = search_directory(root_directory, CSR_VENDOR); + } if (buf) { bufsize = PAGE_SIZE - 1; @@ -334,7 +342,8 @@ static ssize_t show_text_leaf(struct device *dev, bufsize = 1; } - ret = fw_csr_string(dir, attr->key, buf, bufsize); + for (i = 0; i < ARRAY_SIZE(directories) && directories[i]; ++i) + ret = fw_csr_string(directories[i], attr->key, buf, bufsize); if (ret >= 0) { /* Strip trailing whitespace and add newline. */ diff --git a/drivers/firewire/csr-api-test.c b/drivers/firewire/csr-api-test.c index 7278e7b927a8..779146d0cfba 100644 --- a/drivers/firewire/csr-api-test.c +++ b/drivers/firewire/csr-api-test.c @@ -206,8 +206,9 @@ static void csr_api_legacy_avc_device(struct kunit *test) // Descriptor leaf entry for vendor is not found. KUNIT_EXPECT_LT(test, show_text_leaf(node_dev, &config_rom_attributes[5].attr, buf), 0); - // Descriptor leaf entry for model is not found. - KUNIT_EXPECT_LT(test, show_text_leaf(node_dev, &config_rom_attributes[6].attr, buf), 0); + // Descriptor leaf entry for model is found. + KUNIT_EXPECT_GT(test, show_text_leaf(node_dev, &config_rom_attributes[6].attr, buf), 0); + KUNIT_EXPECT_STREQ(test, buf, "ABCDEFGHIJ\n"); // For entries in unit 0 directory. -- 2.39.2 |
From: Takashi S. <o-t...@sa...> - 2023-12-17 10:30:40
|
As the part of support for legacy layout of configuration ROM, this commit traverse vendor directory as well as root directory when showing device attribute for node device. This change expects 'model' attribute appears in node device, however it is probable to see the other types of immediate values if the vendor directory includes. Signed-off-by: Takashi Sakamoto <o-t...@sa...> --- drivers/firewire/core-device.c | 57 +++++++++++++++++++++++++-------- drivers/firewire/csr-api-test.c | 5 +-- 2 files changed, 47 insertions(+), 15 deletions(-) diff --git a/drivers/firewire/core-device.c b/drivers/firewire/core-device.c index 14c7461c05f6..dad5c9937b78 100644 --- a/drivers/firewire/core-device.c +++ b/drivers/firewire/core-device.c @@ -49,6 +49,22 @@ int fw_csr_iterator_next(struct fw_csr_iterator *ci, int *key, int *value) } EXPORT_SYMBOL(fw_csr_iterator_next); +static const u32 *search_directory(const u32 *directory, int search_key) +{ + struct fw_csr_iterator ci; + int key, value; + + search_key |= CSR_DIRECTORY; + + fw_csr_iterator_init(&ci, directory); + while (fw_csr_iterator_next(&ci, &key, &value)) { + if (key == search_key) + return ci.p - 1 + value; + } + + return NULL; +} + static const u32 *search_leaf(const u32 *directory, int search_key) { struct fw_csr_iterator ci; @@ -253,27 +269,42 @@ static ssize_t show_immediate(struct device *dev, 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; + const u32 *directories[] = {NULL, NULL}; + int i, value = -1; down_read(&fw_device_rwsem); - if (is_fw_unit(dev)) - dir = fw_unit(dev)->directory; - else - dir = fw_device(dev)->config_rom + ROOT_DIR_OFFSET; + if (is_fw_unit(dev)) { + directories[0] = fw_unit(dev)->directory; + } else { + const u32 *root_directory = fw_device(dev)->config_rom + ROOT_DIR_OFFSET; - 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; + directories[0] = root_directory; + + // Legacy layout of configuration ROM described in Annex 1 of 'Configuration ROM + // for AV/C Devices 1.0 (December 12, 2000, 1394 Trading Association, TA Document + // 1999027)'. + directories[1] = search_directory(root_directory, CSR_VENDOR); + } + + for (i = 0; i < ARRAY_SIZE(directories) && !!directories[i]; ++i) { + int key, val; + + fw_csr_iterator_init(&ci, directories[i]); + while (fw_csr_iterator_next(&ci, &key, &val)) { + if (attr->key == key) { + value = val; + break; + } } + } up_read(&fw_device_rwsem); - return ret; + if (value < 0) + return -ENOENT; + + return snprintf(buf, buf ? PAGE_SIZE : 0, "0x%06x\n", value); } #define IMMEDIATE_ATTR(name, key) \ diff --git a/drivers/firewire/csr-api-test.c b/drivers/firewire/csr-api-test.c index 2430891c5dcb..7278e7b927a8 100644 --- a/drivers/firewire/csr-api-test.c +++ b/drivers/firewire/csr-api-test.c @@ -199,8 +199,9 @@ static void csr_api_legacy_avc_device(struct kunit *test) KUNIT_EXPECT_GT(test, show_immediate(node_dev, &config_rom_attributes[0].attr, buf), 0); KUNIT_EXPECT_STREQ(test, buf, "0x012345\n"); - // Model immediate entry is not found. - KUNIT_EXPECT_LT(test, show_immediate(node_dev, &config_rom_attributes[4].attr, buf), 0); + // Model immediate entry is found. + KUNIT_EXPECT_GT(test, show_immediate(node_dev, &config_rom_attributes[4].attr, buf), 0); + KUNIT_EXPECT_STREQ(test, buf, "0xfedcba\n"); // Descriptor leaf entry for vendor is not found. KUNIT_EXPECT_LT(test, show_text_leaf(node_dev, &config_rom_attributes[5].attr, buf), 0); -- 2.39.2 |
From: Takashi S. <o-t...@sa...> - 2023-12-17 10:30:35
|
At present, core function can handle node which has configuration ROM similar to AV/C device somehow. The standard layout of configuration ROM for AV/C device is described in the following document. - Configuration ROM for AV/C Devices 1.0 (Dec. 12, 2000, 1394 Trading Association) This commit adds a KUnit test for this case. The following output is the parse result by config-rom-pretty-printer in linux-firewire-utils (https://git.kernel.org/pub/scm/utils/ieee1394/linux-firewire-utils.git/). $ config-rom-pretty-printer < /tmp/rom.img ROM header and bus information block ----------------------------------------------------------------- 1024 0404eabf bus_info_length 4, crc_length 4, crc 60095 1028 31333934 bus_name "1394" 1032 e0646102 irmc 1, cmc 1, isc 1, bmc 0, cyc_clk_acc 100, max_rec 6 (128) 1036 ffffffff company_id ffffff | 1040 ffffffff device_id 1099511627775 | EUI-64 18446744073709551615 root directory ----------------------------------------------------------------- 1044 00063287 directory_length 6, crc 12935 1048 03ffffff vendor 1052 8100000a --> descriptor leaf at 1092 1056 17ffffff model 1060 8100000e --> descriptor leaf at 1116 1064 0c0083c0 node capabilities: per IEEE 1394 1068 d1000001 --> unit directory at 1072 unit directory at 1072 ----------------------------------------------------------------- 1072 0004442d directory_length 4, crc 17453 1076 1200a02d specifier id 1080 13010001 version 1084 17ffffff model 1088 81000007 --> descriptor leaf at 1116 descriptor leaf at 1092 ----------------------------------------------------------------- 1092 0005c915 leaf_length 5, crc 51477 1096 00000000 textual descriptor 1100 00000000 minimal ASCII 1104 56656e64 "Vend" 1108 6f72204e "or N" 1112 616d6500 "ame" descriptor leaf at 1116 ----------------------------------------------------------------- 1116 00057f16 leaf_length 5, crc 32534 1120 00000000 textual descriptor 1124 00000000 minimal ASCII 1128 4d6f6465 "Mode" 1132 6c204e61 "l Na" 1136 6d650000 "me" Signed-off-by: Takashi Sakamoto <o-t...@sa...> --- drivers/firewire/csr-api-test.c | 119 ++++++++++++++++++++++++++++++++ 1 file changed, 119 insertions(+) diff --git a/drivers/firewire/csr-api-test.c b/drivers/firewire/csr-api-test.c index a76d767373e9..d91852825efc 100644 --- a/drivers/firewire/csr-api-test.c +++ b/drivers/firewire/csr-api-test.c @@ -8,7 +8,126 @@ #include <kunit/test.h> +// Configuration ROM for AV/C Devices 1.0 (Dec. 12, 2000, 1394 Trading Association) +// Annex C:Configuration ROM example(informative) +// C.1 Simple AV/C device +// +// Copied from the documentation. +static const u32 simple_avc_device[] = { + 0x0404eabf, + 0x31333934, + 0xe0646102, + 0xffffffff, + 0xffffffff, + 0x00063287, // root directory. + 0x03ffffff, + 0x8100000a, + 0x17ffffff, + 0x8100000e, + 0x0c0083c0, + 0xd1000001, + 0x0004442d, // unit 0 directory. + 0x1200a02d, + 0x13010001, + 0x17ffffff, + 0x81000007, + 0x0005c915, // leaf. + 0x00000000, + 0x00000000, + 0x56656e64, + 0x6f72204e, + 0x616d6500, + 0x00057f16, // leaf. + 0x00000000, + 0x00000000, + 0x4d6f6465, + 0x6c204e61, + 0x6d650000, +}; + +static void csr_api_simple_avc_device(struct kunit *test) +{ + static const struct fw_device node = { + .device = { + .type = &fw_device_type, + }, + .config_rom = simple_avc_device, + .config_rom_length = sizeof(simple_avc_device), + }; + static const struct fw_unit unit0 = { + .device = { + .type = &fw_unit_type, + .parent = (struct device *)&node.device, + }, + .directory = &simple_avc_device[12], + }; + struct device *node_dev = (struct device *)&node.device; + struct device *unit0_dev = (struct device *)&unit0.device; + static const int unit_expected_ids[] = {0x00ffffff, 0x00ffffff, 0x0000a02d, 0x00010001}; + char *buf = kunit_kzalloc(test, PAGE_SIZE, GFP_KERNEL); + int ids[4] = {0, 0, 0, 0}; + + // Ensure associations for node and unit devices. + + KUNIT_ASSERT_TRUE(test, is_fw_device(node_dev)); + KUNIT_ASSERT_FALSE(test, is_fw_unit(node_dev)); + KUNIT_ASSERT_PTR_EQ(test, fw_device(node_dev), &node); + + KUNIT_ASSERT_FALSE(test, is_fw_device(unit0_dev)); + KUNIT_ASSERT_TRUE(test, is_fw_unit(unit0_dev)); + KUNIT_ASSERT_PTR_EQ(test, fw_parent_device((&unit0)), &node); + KUNIT_ASSERT_PTR_EQ(test, fw_unit(unit0_dev), &unit0); + + // For entries in root directory. + + // Vendor immediate entry is found. + KUNIT_EXPECT_GT(test, show_immediate(node_dev, &config_rom_attributes[0].attr, buf), 0); + KUNIT_EXPECT_STREQ(test, buf, "0xffffff\n"); + + // Model immediate entry is found. + KUNIT_EXPECT_GT(test, show_immediate(node_dev, &config_rom_attributes[4].attr, buf), 0); + KUNIT_EXPECT_STREQ(test, buf, "0xffffff\n"); + + // Descriptor leaf entry for vendor is found. + KUNIT_EXPECT_GT(test, show_text_leaf(node_dev, &config_rom_attributes[5].attr, buf), 0); + KUNIT_EXPECT_STREQ(test, buf, "Vendor Name\n"); + + // Descriptor leaf entry for model is found. + KUNIT_EXPECT_GT(test, show_text_leaf(node_dev, &config_rom_attributes[6].attr, buf), 0); + KUNIT_EXPECT_STREQ(test, buf, "Model Name\n"); + + // For entries in unit 0 directory. + + // Vendor immediate entry is not found. + KUNIT_EXPECT_LT(test, show_immediate(unit0_dev, &config_rom_attributes[0].attr, buf), 0); + + // Model immediate entry is found. + KUNIT_EXPECT_GT(test, show_immediate(unit0_dev, &config_rom_attributes[4].attr, buf), 0); + KUNIT_EXPECT_STREQ(test, buf, "0xffffff\n"); + + // Descriptor leaf entry for vendor is not found. + KUNIT_EXPECT_LT(test, show_text_leaf(unit0_dev, &config_rom_attributes[5].attr, buf), 0); + + // Descriptor leaf entry for model is not found. + KUNIT_EXPECT_GT(test, show_text_leaf(unit0_dev, &config_rom_attributes[6].attr, buf), 0); + KUNIT_EXPECT_STREQ(test, buf, "Model Name\n"); + + // Specifier_ID immediate entry is found. + KUNIT_EXPECT_GT(test, show_immediate(unit0_dev, &config_rom_attributes[2].attr, buf), 0); + KUNIT_EXPECT_STREQ(test, buf, "0x00a02d\n"); + + // Version immediate entry is found. + KUNIT_EXPECT_GT(test, show_immediate(unit0_dev, &config_rom_attributes[3].attr, buf), 0); + KUNIT_EXPECT_STREQ(test, buf, "0x010001\n"); + + kunit_kfree(test, buf); + + get_modalias_ids(&unit0, ids); + KUNIT_EXPECT_MEMEQ(test, ids, unit_expected_ids, sizeof(ids)); +} + static struct kunit_case csr_api_test_cases[] = { + KUNIT_CASE(csr_api_simple_avc_device), {} }; -- 2.39.2 |
From: Takashi S. <o-t...@sa...> - 2023-12-17 10:30:34
|
The operation over CSR space is important since it is related to detection of nodes and units, addition of system devices. Any test of the operation is enough useful. This commit adds a skeleton of KUnit test for the purpose. Signed-off-by: Takashi Sakamoto <o-t...@sa...> --- drivers/firewire/.kunitconfig | 1 + drivers/firewire/Kconfig | 16 ++++++++++++++++ drivers/firewire/core-device.c | 4 ++++ drivers/firewire/csr-api-test.c | 19 +++++++++++++++++++ 4 files changed, 40 insertions(+) create mode 100644 drivers/firewire/csr-api-test.c diff --git a/drivers/firewire/.kunitconfig b/drivers/firewire/.kunitconfig index 1599e069395f..03104cdd06eb 100644 --- a/drivers/firewire/.kunitconfig +++ b/drivers/firewire/.kunitconfig @@ -2,3 +2,4 @@ CONFIG_KUNIT=y CONFIG_PCI=y CONFIG_FIREWIRE=y CONFIG_FIREWIRE_KUNIT_UAPI_TEST=y +CONFIG_FIREWIRE_KUNIT_CSR_API_TEST=y diff --git a/drivers/firewire/Kconfig b/drivers/firewire/Kconfig index 0a6596b027db..184906fdb77a 100644 --- a/drivers/firewire/Kconfig +++ b/drivers/firewire/Kconfig @@ -34,6 +34,22 @@ config FIREWIRE_KUNIT_UAPI_TEST For more information on KUnit and unit tests in general, refer to the KUnit documentation in Documentation/dev-tools/kunit/. +config FIREWIRE_KUNIT_CSR_API_TEST + tristate "KUnit tests for internal CSR API" if !KUNIT_ALL_TESTS + depends on FIREWIRE && KUNIT + default KUNIT_ALL_TESTS + help + This builds the KUnit tests for internal API for Control and + Status Registers. + + KUnit tests run during boot and output the results to the debug + log in TAP format (https://testanything.org/). Only useful for + kernel devs running KUnit test harness and are not for inclusion + into a production build. + + For more information on KUnit and unit tests in general, refer + to the KUnit documentation in Documentation/dev-tools/kunit/. + config FIREWIRE_OHCI tristate "OHCI-1394 controllers" depends on PCI && FIREWIRE && MMU diff --git a/drivers/firewire/core-device.c b/drivers/firewire/core-device.c index 137001f8a695..14c7461c05f6 100644 --- a/drivers/firewire/core-device.c +++ b/drivers/firewire/core-device.c @@ -1313,3 +1313,7 @@ void fw_node_event(struct fw_card *card, struct fw_node *node, int event) break; } } + +#ifdef CONFIG_FIREWIRE_KUNIT_CSR_API_TEST +#include "csr-api-test.c" +#endif diff --git a/drivers/firewire/csr-api-test.c b/drivers/firewire/csr-api-test.c new file mode 100644 index 000000000000..a76d767373e9 --- /dev/null +++ b/drivers/firewire/csr-api-test.c @@ -0,0 +1,19 @@ +// SPDX-License-Identifier: GPL-2.0-only +// +// csr-api-test.c - An application of Kunit to test implementation for CSR operation. +// +// Copyright (c) 2023 Takashi Sakamoto +// +// This file can not be built independently since it is intentionally included in core-device.c. + +#include <kunit/test.h> + +static struct kunit_case csr_api_test_cases[] = { + {} +}; + +static struct kunit_suite csr_api_test_suite = { + .name = "firewire-csr-api", + .test_cases = csr_api_test_cases, +}; +kunit_test_suite(csr_api_test_suite); -- 2.39.2 |
From: Takashi S. <o-t...@sa...> - 2023-12-17 10:30:33
|
Hi, Current core function of Linux FireWire subsystem partly supports legacy layout of configuration ROM, described in annex of 1394TA document[1]. It appears that some users have the demand of full support[2]. The series of change is to satisfy the demand. The change relates to driver matching procedure and notification to user space, thus could easily bring functional regression. For safe, the series includes KUnit application to test the change. However, backward incompatibility is inevitable due to change of modalias for unit device. As long as I investigated, any unit drivers in kernel are not affected by the change. Additionally, less applications in user space are not as well. I think we can be optimistic to the regression. Anyway, I'm pleased if receiving your comment or test report about the change. [1] Configuration ROM for AV/C Devices 1.0 (December 12, 2000, 1394 Trading Association, TA Document 1999027) https://web.archive.org/web/20210216003030/http://1394ta.org/wp-content/uploads/2015/07/1999027.pdf [2] [PATCH] Fix missing sysfs vendor/model entries for some devices https://sourceforge.net/p/linux1394/mailman/message/55802731/ Regards Takashi Sakamoto (8): firewire: core: adds constant qualifier for local helper functions firewire: core: replace magic number with macro firewire: test: add KUnit test for internal CSR API firewire: test: add test of CSR API for simple AV/C device firewire: test: add test of CSR API for legacy AV/C device firewire: core: detect numeric model identifier for legacy layout of configuration ROM firewire: core: detect model name for legacy layout of configuration ROM firewire: core: change modalias of unit device with backward incompatibility drivers/firewire/.kunitconfig | 1 + drivers/firewire/Kconfig | 16 ++ drivers/firewire/core-device.c | 117 +++++++++++---- drivers/firewire/csr-api-test.c | 251 ++++++++++++++++++++++++++++++++ 4 files changed, 358 insertions(+), 27 deletions(-) create mode 100644 drivers/firewire/csr-api-test.c -- 2.39.2 |
From: Takashi S. <o-t...@sa...> - 2023-12-17 10:30:33
|
In IEEE 1394 specification, the size of bus information block of configuration ROM is fixed to 5, thus the offset of root directory is 5. Current implementation to handle device structures has the hard-coded offset. This commit replaces the offset with macro. Signed-off-by: Takashi Sakamoto <o-t...@sa...> --- drivers/firewire/core-device.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/drivers/firewire/core-device.c b/drivers/firewire/core-device.c index 16c32cb38f0f..137001f8a695 100644 --- a/drivers/firewire/core-device.c +++ b/drivers/firewire/core-device.c @@ -31,6 +31,8 @@ #include "core.h" +#define ROOT_DIR_OFFSET 5 + void fw_csr_iterator_init(struct fw_csr_iterator *ci, const u32 *p) { ci->p = p + 1; @@ -135,7 +137,7 @@ static void get_ids(const u32 *directory, int *id) static void get_modalias_ids(const struct fw_unit *unit, int *id) { - get_ids(&fw_parent_device(unit)->config_rom[5], id); + get_ids(&fw_parent_device(unit)->config_rom[ROOT_DIR_OFFSET], id); get_ids(unit->directory, id); } @@ -259,7 +261,7 @@ static ssize_t show_immediate(struct device *dev, if (is_fw_unit(dev)) dir = fw_unit(dev)->directory; else - dir = fw_device(dev)->config_rom + 5; + dir = fw_device(dev)->config_rom + ROOT_DIR_OFFSET; fw_csr_iterator_init(&ci, dir); while (fw_csr_iterator_next(&ci, &key, &value)) @@ -292,7 +294,7 @@ static ssize_t show_text_leaf(struct device *dev, if (is_fw_unit(dev)) dir = fw_unit(dev)->directory; else - dir = fw_device(dev)->config_rom + 5; + dir = fw_device(dev)->config_rom + ROOT_DIR_OFFSET; if (buf) { bufsize = PAGE_SIZE - 1; @@ -446,7 +448,7 @@ static ssize_t units_show(struct device *dev, int key, value, i = 0; down_read(&fw_device_rwsem); - fw_csr_iterator_init(&ci, &device->config_rom[5]); + fw_csr_iterator_init(&ci, &device->config_rom[ROOT_DIR_OFFSET]); while (fw_csr_iterator_next(&ci, &key, &value)) { if (key != (CSR_UNIT | CSR_DIRECTORY)) continue; @@ -691,7 +693,7 @@ static void create_units(struct fw_device *device) int key, value, i; i = 0; - fw_csr_iterator_init(&ci, &device->config_rom[5]); + fw_csr_iterator_init(&ci, &device->config_rom[ROOT_DIR_OFFSET]); while (fw_csr_iterator_next(&ci, &key, &value)) { if (key != (CSR_UNIT | CSR_DIRECTORY)) continue; -- 2.39.2 |
From: Takashi S. <o-t...@sa...> - 2023-12-17 10:30:33
|
Some local functions just handles given argument as mutable, thus it is preferable to add constant qualifier to them. Signed-off-by: Takashi Sakamoto <o-t...@sa...> --- drivers/firewire/core-device.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/firewire/core-device.c b/drivers/firewire/core-device.c index aa597cda0d88..16c32cb38f0f 100644 --- a/drivers/firewire/core-device.c +++ b/drivers/firewire/core-device.c @@ -171,7 +171,7 @@ static const struct ieee1394_device_id *unit_match(struct device *dev, return NULL; } -static bool is_fw_unit(struct device *dev); +static bool is_fw_unit(const struct device *dev); static int fw_unit_match(struct device *dev, struct device_driver *drv) { @@ -679,7 +679,7 @@ static struct device_type fw_unit_type = { .release = fw_unit_release, }; -static bool is_fw_unit(struct device *dev) +static bool is_fw_unit(const struct device *dev) { return dev->type == &fw_unit_type; } @@ -838,7 +838,7 @@ static struct device_type fw_device_type = { .release = fw_device_release, }; -static bool is_fw_device(struct device *dev) +static bool is_fw_device(const struct device *dev) { return dev->type == &fw_device_type; } -- 2.39.2 |
From: Takashi S. <o-t...@sa...> - 2023-12-12 04:56:40
|
Hi, Thanks for your report. I've already realized the issue, and been investigating it with AMD developers[1]. You can see the detail in the message, but as long as I know the issue appears in the following combination of hardware. * any type of AMD Ryzen machine * PCI(e) extension card with * Asmedia ASM1083/1085 * VIA VT6306/6307/6308 I'm sorry for your inconvenience, and appreciate your patience. [1] https://lore.kernel.org/lkml/20231107121756.GA168964@workstation.local/ Takashi Sakamoto |
From: Tobias G. <tob...@23...> - 2023-12-12 00:13:51
|
[Resend because I wasn't subcribed before] Hi, in recent kernels (noticed with Debian kernel 6.5) I get a hard crash when the firewire-ohci module is loaded. I could bisect this issue to commit dcadfd7f7c74ef9ee415e072a19bdf6c085159eb is the first bad commit commit dcadfd7f7c74ef9ee415e072a19bdf6c085159eb Author: Takashi Sakamoto <o-t...@sa...> Date: Tue May 30 08:12:40 2023 +0900 firewire: core: use union for callback of transaction completion But I'm not sure how to continue debugging from here, since I don't get any output after [Dec10 19:17] firewire_ohci 0000:0b:00.0: enabling device (0000 -> 0003) [ +0.075791] firewire_ohci 0000:0b:00.0: added OHCI v1.0 device as card 0, 4 IR + 8 IT contexts, quirks 0x11 My hardware is: 0b:00.0 FireWire (IEEE 1394): VIA Technologies, Inc. VT6306/7/8 [Fire II(M)] IEEE 1394 OHCI Controller (rev c0) (prog-if 10 [OHCI]) Subsystem: VIA Technologies, Inc. VT6306/7/8 [Fire II(M)] IEEE 1394 OHCI Controller Flags: medium devsel, IRQ 255 Memory at fc800000 (32-bit, non-prefetchable) [disabled] [size=2K] I/O ports at d000 [disabled] [size=128] Capabilities: [50] Power Management version 2 Kernel modules: firewire_ohci There are currently no devices connected. I could work around the issue by blacklisting the module, since I currently don't use the FireWire hardware (but there are still some DV tapes waiting to be transferred...) Regards, Tobias |
From: Takashi S. <o-t...@sa...> - 2023-11-29 13:41:09
|
Hi, Thanks for your catching the bug and posting the patch. On Wed, Nov 29, 2023 at 05:34:08PM +0800, Yang Yingliang wrote: > From: Yang Yingliang <yan...@hu...> > > If device_register() fails, the refcount of device is not 0, the name > allocated in dev_set_name() is leaked. To fix this by calling put_device(), > so that it will be freed in callback function kobject_cleanup(). > > unreferenced object 0xffff9d99035c7a90 (size 8): > comm "systemd-udevd", pid 168, jiffies 4294672386 (age 152.089s) > hex dump (first 8 bytes): > 66 77 30 2e 30 00 ff ff fw0.0... > backtrace: > [<00000000e1d62bac>] __kmem_cache_alloc_node+0x1e9/0x360 > [<00000000bbeaff31>] __kmalloc_node_track_caller+0x44/0x1a0 > [<00000000491f2fb4>] kvasprintf+0x67/0xd0 > [<000000005b960ddc>] kobject_set_name_vargs+0x1e/0x90 > [<00000000427ac591>] dev_set_name+0x4e/0x70 > [<000000003b4e447d>] create_units+0xc5/0x110 > > fw_unit_release() will be called in the error path, move fw_device_get() > before calling device_register() to keep balanced with fw_device_put() in > fw_unit_release(). > > Fixes: 1fa5ae857bb1 ("driver core: get rid of struct device's bus_id string array") > Fixes: a1f64819fe9f ("firewire: struct device - replace bus_id with dev_name(), dev_set_name()") > Signed-off-by: Yang Yingliang <yan...@hu...> > --- > drivers/firewire/core-device.c | 11 ++++------- > 1 file changed, 4 insertions(+), 7 deletions(-) Applied to for-linus branch. I'll send it to upstream as fix for v6.7-rc4 within a few days. Thanks Takashi Sakamoto |
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 |
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-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-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-10-04 04:04:05
|
Hi, Thanks for your sending the patch. On Tue, Oct 03, 2023 at 09:16:50PM +0000, Kai Bosch wrote: > --- > drivers/firewire/net.c | 48 +++++++++++++++++++++--------------------- > 1 file changed, 24 insertions(+), 24 deletions(-) As long as I know, the coding style of Linux kernel[1] does not forbidden brief expression of 'unsigned int' (= just 'unsigned'). Furthermore, in C language specification, 'unsigned' and 'unsigned int' are the same[2]. I have no functional change over the patch. If the intension of patch is in the point of human readability, please note it in the commit comment. It is the most important for the kind of change. I'm waiting for your next post. [1] Linux kernel coding style https://www.kernel.org/doc/html/v4.10/process/coding-style.html [2] See '6.7.2 Type specifiers' in ISO/IEC 9899:1999, so called 'C99'. Regards Takashi Sakamoto |