[RFC,7/8] firewire: core: detect model name for legacy layout of configuration ROM
Commit Message
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-takashi@sakamocchi.jp>
---
drivers/firewire/core-device.c | 23 ++++++++++++++++-------
drivers/firewire/csr-api-test.c | 5 +++--
2 files changed, 19 insertions(+), 9 deletions(-)
Comments
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
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
@@ -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. */
@@ -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.