Re: [RFC PATCH 7/8] firewire: core: detect model name for legacy layout of configuration ROM
Brought to you by:
aeb,
bencollins
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 |