The description of the sysfs-pci device resources for the rom file is as follows: The 'rom' file is special in that it provides read-only access to the device's ROM file, if available. It's disabled by default, however, so applications should write the string "1" to the file to enable it before attempting a read call, and disable it following the access by writing "0" to the file. Note that the device must be enabled for a rom read to return data successfully. In the event a driver is not bound to the device, it can be enabled using the 'enable' file, documented above. Basically, you must write '1' first to read the rom, then '0' to disable read. The actual functionality of the rom file does not match this description. As it sits, on all versions of the kernel with sysfs that I have tried, the functionality is as follows: - Write anything at all to the file to open for read - Write a string with a length of 2 starting with '0' to disable read. (i.e. '0f', '0 ', '0!') This can be tested using printf: # head -n1 rom head: error reading ‘rom’: Invalid argument # printf 'asdf' > rom # head -n1 rom ��IBM VGA Compatible��Q12/02/14@%�l���+B8&��/h���:�u� # printf '0' > rom # head -n1 rom ��IBM VGA Compatible��Q12/02/14@%�l���+B8&��/h���:�u� # printf '0x' > rom head: error reading ‘rom’: Invalid argument This seems to be due to how the input is parsed in /drivers/pci/pci-sysfs.c: 1230 if ((off == 0) && (*buf == '') && (count == 2)) 1231 pdev->rom_attr_enabled = 0; 1232 else 1233 pdev->rom_attr_enabled = 1; The length of the input needed to set rom_attr_enabled to 0 is 2, and literally any other input is accepted to set rom_attr_enabled to 1. I suggest the documentation is updated to clearly define actual functionality, or the input parsing portion of /drivers/pci/pci-sysfs.c is changed to accept only 1 or 0.
I can reproduce this with the latest kernel. Is this set to 2 because of the likelyhood that a user is using `echo` instead of `printf`, and therefore, it should expect 2 for the length. Obviously the other side of the branch could be improved to expect a `1`.
I agree that it's more likely a user will use `echo 1 > rom`. The issue I encountered was using python to enable/disable access to the sysfs rom file. Since file.write() doesn't tack on a newline, it was not immediately apparent why the file was not becoming unreadable. Since the documentation did not lead me to resolving the issue, I ended up parsing the source to ensure I was able to properly disable access to the rom. I understand this is not typical use (and there may be better ways to access said rom programmatically), but it might be good to update the documentation to note the expected 2 character length for disabling access to the rom. I don't think it's necessary to change the functionality in this case, since it's just not a normally accessed function.
Okay, I will work on putting together a documentation change as I need to submit patches as part of a class. I'll CC you, as well as the appropriate list when I'm done.
I'd rather change the code to use kstrtobool() instead of the ad hoc parsing it currently does.
(In reply to Bjorn Helgaas from comment #4) > I'd rather change the code to use kstrtobool() instead of the ad hoc parsing > it currently does. Ooh, that's nice idea. Should that be exactly kstrtobool() or the from user version (kstrtobool_from_user())? I'm digging in to find the definition and documentation, noticing the two versions. Since the header of this function says the input is from the user... does that seem reasonable?
That's a good question and I don't know the answer. But there's an existing use of kstrtobool() and several of kstrtoul(), etc., and they should probably all be the same. I suspect (but have not verified) that the sysfs infrastructure already takes care of the _from_user issue for us. There are relatively few users of kstrtobool_from_user(), and they all seem to be in a direct struct file_operations path, not a path coming through sysfs.
I can easily get the former version (`kstrtobool`) working, will submit to the list, if someone would prefer the latter, from user version, I can amend the commit. Thank you for the input.