Hi,
This series of change is take 2 of my previous post[1].
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 v1 series:
* fix evaluation of uninitialized variable in 7th patch
[1] [PATCH 0/8] firewire: core: support legacy layout of configuration ROM
for AV/C device
https://lore.kernel.org/lkml/[email protected]/
[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
Some local functions just handles given argument as mutable, thus it is
preferable to add constant qualifier to them.
Signed-off-by: Takashi Sakamoto <[email protected]>
---
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
At present, core function can handle node which has configuration ROM
similar to standard AV/C device somehow. The standard layout of
configuration ROM 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 the above case.
The following output is the parse result for the hard-coded data, 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 <[email protected]>
---
drivers/firewire/device-attribute-test.c | 119 +++++++++++++++++++++++
1 file changed, 119 insertions(+)
diff --git a/drivers/firewire/device-attribute-test.c b/drivers/firewire/device-attribute-test.c
index 87cfdf97c898..e2c5587d0757 100644
--- a/drivers/firewire/device-attribute-test.c
+++ b/drivers/firewire/device-attribute-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_config_rom[] = {
+ 0x0404eabf,
+ 0x31333934,
+ 0xe0646102,
+ 0xffffffff,
+ 0xffffffff,
+ 0x00063287, // root directory.
+ 0x03ffffff,
+ 0x8100000a,
+ 0x17ffffff,
+ 0x8100000e,
+ 0x0c0083c0,
+ 0xd1000001,
+ 0x0004442d, // unit 0 directory.
+ 0x1200a02d,
+ 0x13010001,
+ 0x17ffffff,
+ 0x81000007,
+ 0x0005c915, // leaf for textual descriptor.
+ 0x00000000,
+ 0x00000000,
+ 0x56656e64,
+ 0x6f72204e,
+ 0x616d6500,
+ 0x00057f16, // leaf for textual descriptor.
+ 0x00000000,
+ 0x00000000,
+ 0x4d6f6465,
+ 0x6c204e61,
+ 0x6d650000,
+};
+
+static void device_attr_simple_avc(struct kunit *test)
+{
+ static const struct fw_device node = {
+ .device = {
+ .type = &fw_device_type,
+ },
+ .config_rom = simple_avc_config_rom,
+ .config_rom_length = sizeof(simple_avc_config_rom),
+ };
+ static const struct fw_unit unit0 = {
+ .device = {
+ .type = &fw_unit_type,
+ .parent = (struct device *)&node.device,
+ },
+ .directory = &simple_avc_config_rom[12],
+ };
+ struct device *node_dev = (struct device *)&node.device;
+ struct device *unit0_dev = (struct device *)&unit0.device;
+ static const int unit0_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 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, unit0_expected_ids, sizeof(ids));
+}
+
static struct kunit_case device_attr_test_cases[] = {
+ KUNIT_CASE(device_attr_simple_avc),
{}
};
--
2.39.2
The traverse over CSR space results in attributes of node and unit
devices. Any test of the traverse is useful.
This commit adds a skeleton of KUnit test for the purpose.
Signed-off-by: Takashi Sakamoto <[email protected]>
---
drivers/firewire/.kunitconfig | 1 +
drivers/firewire/Kconfig | 16 ++++++++++++++++
drivers/firewire/core-device.c | 4 ++++
drivers/firewire/device-attribute-test.c | 19 +++++++++++++++++++
4 files changed, 40 insertions(+)
create mode 100644 drivers/firewire/device-attribute-test.c
diff --git a/drivers/firewire/.kunitconfig b/drivers/firewire/.kunitconfig
index 1599e069395f..76444a2d5e12 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_DEVICE_ATTRIBUTE_TEST=y
diff --git a/drivers/firewire/Kconfig b/drivers/firewire/Kconfig
index 0a6596b027db..552a39df8cbd 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_DEVICE_ATTRIBUTE_TEST
+ tristate "KUnit tests for device attributes" if !KUNIT_ALL_TESTS
+ depends on FIREWIRE && KUNIT
+ default KUNIT_ALL_TESTS
+ help
+ This builds the KUnit tests for device attribute for node and
+ unit.
+
+ 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 315a2fe41643..fe1e64df476c 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_DEVICE_ATTRIBUTE_TEST
+#include "device-attribute-test.c"
+#endif
diff --git a/drivers/firewire/device-attribute-test.c b/drivers/firewire/device-attribute-test.c
new file mode 100644
index 000000000000..87cfdf97c898
--- /dev/null
+++ b/drivers/firewire/device-attribute-test.c
@@ -0,0 +1,19 @@
+// SPDX-License-Identifier: GPL-2.0-only
+//
+// device-attribute-test.c - An application of Kunit to test implementation for device attributes.
+//
+// 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 device_attr_test_cases[] = {
+ {}
+};
+
+static struct kunit_suite device_attr_test_suite = {
+ .name = "firewire-device-attribute",
+ .test_cases = device_attr_test_cases,
+};
+kunit_test_suite(device_attr_test_suite);
--
2.39.2
As the part of support for legacy layout of configuration ROM, this
commit traverses 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 <[email protected]>
---
drivers/firewire/core-device.c | 59 ++++++++++++++++++------
drivers/firewire/device-attribute-test.c | 5 +-
2 files changed, 49 insertions(+), 15 deletions(-)
diff --git a/drivers/firewire/core-device.c b/drivers/firewire/core-device.c
index fe1e64df476c..d0ce583b93c4 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,44 @@ 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;
+ const u32 *vendor_directory = search_directory(root_directory, CSR_VENDOR);
- 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;
+ 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;
+ }
+ }
+
+ 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;
}
+ }
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/device-attribute-test.c b/drivers/firewire/device-attribute-test.c
index 495af65c33b5..689115433425 100644
--- a/drivers/firewire/device-attribute-test.c
+++ b/drivers/firewire/device-attribute-test.c
@@ -199,8 +199,9 @@ static void device_attr_legacy_avc(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
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, and this commit is for
the purpose.
The following output is the parse result for the hard-coded data, by
config-rom-pretty-printer in linux-firewire-utils
(https://git.kernel.org/pub/scm/utils/ieee1394/linux-firewire-utils.git/).
The data 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 <[email protected]>
---
drivers/firewire/device-attribute-test.c | 111 +++++++++++++++++++++++
1 file changed, 111 insertions(+)
diff --git a/drivers/firewire/device-attribute-test.c b/drivers/firewire/device-attribute-test.c
index e2c5587d0757..495af65c33b5 100644
--- a/drivers/firewire/device-attribute-test.c
+++ b/drivers/firewire/device-attribute-test.c
@@ -45,6 +45,40 @@ static const u32 simple_avc_config_rom[] = {
0x6d650000,
};
+// Ibid.
+// Annex A:Consideration for configuration ROM reader design (informative)
+// A.1 Vendor directory
+//
+// Written by hand.
+static const u32 legacy_avc_config_rom[] = {
+ 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 textual descriptor.
+ 0x00000000,
+ 0x00000000,
+ 0x41424344,
+ 0x45464748,
+ 0x494a0000,
+};
+
static void device_attr_simple_avc(struct kunit *test)
{
static const struct fw_device node = {
@@ -126,8 +160,85 @@ static void device_attr_simple_avc(struct kunit *test)
KUNIT_EXPECT_MEMEQ(test, ids, unit0_expected_ids, sizeof(ids));
}
+static void device_attr_legacy_avc(struct kunit *test)
+{
+ static const struct fw_device node = {
+ .device = {
+ .type = &fw_device_type,
+ },
+ .config_rom = legacy_avc_config_rom,
+ .config_rom_length = sizeof(legacy_avc_config_rom),
+ };
+ static const struct fw_unit unit0 = {
+ .device = {
+ .type = &fw_unit_type,
+ .parent = (struct device *)&node.device,
+ },
+ .directory = &legacy_avc_config_rom[11],
+ };
+ struct device *node_dev = (struct device *)&node.device;
+ struct device *unit0_dev = (struct device *)&unit0.device;
+ static const int unit0_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, unit0_expected_ids, sizeof(ids));
+}
+
static struct kunit_case device_attr_test_cases[] = {
KUNIT_CASE(device_attr_simple_avc),
+ KUNIT_CASE(device_attr_legacy_avc),
{}
};
--
2.39.2
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 <[email protected]>
---
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 96b0b43da863..315a2fe41643 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
As the part of support for legacy layout of configuration ROM, this
commit traverses 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 <[email protected]>
---
drivers/firewire/core-device.c | 31 ++++++++++++++++++------
drivers/firewire/device-attribute-test.c | 5 ++--
2 files changed, 27 insertions(+), 9 deletions(-)
diff --git a/drivers/firewire/core-device.c b/drivers/firewire/core-device.c
index d0ce583b93c4..029ba0ff7083 100644
--- a/drivers/firewire/core-device.c
+++ b/drivers/firewire/core-device.c
@@ -317,17 +317,29 @@ 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 = -ENOENT;
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;
+ const u32 *vendor_directory = 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] = root_directory;
+ directories[1] = vendor_directory;
+ }
+ }
if (buf) {
bufsize = PAGE_SIZE - 1;
@@ -336,7 +348,12 @@ 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) {
+ int result = fw_csr_string(directories[i], attr->key, buf, bufsize);
+ // Detected.
+ if (result >= 0)
+ ret = result;
+ }
if (ret >= 0) {
/* Strip trailing whitespace and add newline. */
diff --git a/drivers/firewire/device-attribute-test.c b/drivers/firewire/device-attribute-test.c
index 689115433425..da2a4a09bf84 100644
--- a/drivers/firewire/device-attribute-test.c
+++ b/drivers/firewire/device-attribute-test.c
@@ -206,8 +206,9 @@ static void device_attr_legacy_avc(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
As the last part of support for legacy layout of configuration ROM, this
commit traverses vendor directory as well as root directory when
constructing modalias for unit device. The change brings loss of backward
compatibility since it can fill model field ('mo') 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 use the model fields to match
device, however all of supported devices does not have such legacy
layout.
2. the other unit drivers (e.g. sbp2) does not use the model field to
match device.
The rest of concern is user space application. The most of applications
just take care of node device and does not use the modalias of unit
device, thus the change does not affect to them. But systemd project is
known to get affects from the change since it includes hwdb to take udev
to configure fw character device conveniently. I have a plan to work for
systemd so that the access permission of character device could be kept
across the change.
Signed-off-by: Takashi Sakamoto <[email protected]>
---
drivers/firewire/core-device.c | 21 +++++++++++++++++++--
drivers/firewire/device-attribute-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 029ba0ff7083..829b3d590ccd 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_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/device-attribute-test.c b/drivers/firewire/device-attribute-test.c
index da2a4a09bf84..2f123c6b0a16 100644
--- a/drivers/firewire/device-attribute-test.c
+++ b/drivers/firewire/device-attribute-test.c
@@ -178,7 +178,7 @@ static void device_attr_legacy_avc(struct kunit *test)
};
struct device *node_dev = (struct device *)&node.device;
struct device *unit0_dev = (struct device *)&unit0.device;
- static const int unit0_expected_ids[] = {0x00012345, 0x00000000, 0x00abcdef, 0x00543210};
+ static const int unit0_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
On Thu, Dec 21, 2023 at 10:48:41PM +0900, Takashi Sakamoto wrote:
> Hi,
>
> This series of change is take 2 of my previous post[1].
>
> 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 v1 series:
> * fix evaluation of uninitialized variable in 7th patch
>
> [1] [PATCH 0/8] firewire: core: support legacy layout of configuration ROM
> for AV/C device
> https://lore.kernel.org/lkml/[email protected]/
> [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
Applied the above patches to for-next branch[1]. Thanks for your reviewing.
[1] https://git.kernel.org/pub/scm/linux/kernel/git/ieee1394/linux1394.git/log/?h=for-next
Takashi Sakamoto