Messages in this thread Patch in this message | ![/](/images/icornerl.gif) | | Date | Mon, 18 Dec 2023 21:50:26 +0900 | From | Takashi Sakamoto <> | Subject | Re: [RFC PATCH 7/8] firewire: core: detect model name for legacy layout of configuration ROM |
| |
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
| ![\](/images/icornerr.gif) |