Thank you for your detailed feedback and suggestions. Based on your comments, I have a few thoughts and questions to ensure we move forward effectively. Jonathan Cameron wrote: > On Thu, 12 Dec 2024 09:08:54 +0000 > "Daisuke Kobayashi (Fujitsu)" <kobayashi.da-06@xxxxxxxxxxx> wrote: > > > Jonathan Cameron wrote: > > > On Tue, 10 Dec 2024 13:08:21 +0900 > > > "Kobayashi,Daisuke" <kobayashi.da-06@xxxxxxxxxxx> wrote: ... snip ... > > Regarding your proposed file structure: > > > > budget0_power > > budget0_pm_state > > ... > > budget1_power > > budget1_pm_state > > ... > > budget2_xxx > > ... > > > > Is this the correct format? > > Yes. That is what I had mind. > Thanks, I see. > > > > In my understanding, implementing this would require 256 attributes > > corresponding to the DataSelectRegister, which seems overly redundant. > > Additionally, I was concerned about the mechanism of changing the > DataSelectRegister each time a file is read. > > I agree it is quite a large number of files, but there are larger sysfs interfaces so > I'm not that concerned. Do you have any data on how many entries are typically > used? > We could use an is_visible() callback and register all 256, or we could do > dynamic allocation though that would require walking to find out what is there. > > The implementation should cache the current dataselectregister value and only > write it if the value needs to change. Sensible userspace software would just > read all the elements of each budgetX_* before moving on to the next one. > Unfortunately, I don't have specific data on the typical number of entries used. However, I estimate that it would be within 10 at most. Could you please provide examples of large sysfs interfaces and where they are implemented? I would like to refer to them for implementation. In pci-sysfs.c, there is an attribute called resourceX_resize, and I believe its basic concept could be useful. However, the PowerBudget implementation is more complex because it involves multiple sysfs outputs for each index. If there are more reference implementations, I could propose a more mature implementation. > > > > Therefore, I proposed this patch implementation, even though it might not be > ideal. > > > > If you have a suitable implementation to achieve your proposed > > structure, I would greatly appreciate it if you could share it. > > > > Upon further reflection, the issue of needing to change the > > DataSelectRegister could potentially be avoided by initially retrieving and > storing the register values for all indices. > > Cacheing these is definitely an option to explore. I'm not sure we would want > to pay that cost at boot, but if not we could do it on first touch. The 0 value is > reserved anyway so if cached copy is 0, read it from the device and fill the > cache. > We would need to read some at boot to figure out which should be visible, but > that would be logN of max entries at worse, rather than all 256. If there are only > a few it would make sense to just read them all at boot. > Although the exact number of indices is uncertain, I believe it is sufficiently small. Therefore, I plan to adopt a method where all values are read and cached at boot time. I intend to create an array in the struct pci_dev and initialize it in the probe function. If there are any other suitable places or methods, please let me know. > Annoying there isn't a count register for these! I also feel that. Thank you. > > Jonathan > > > > > > > > Thank you for pointing out the coding style issue, I'll fix it in the next patch. > > > > > > > > > > Signed-off-by: "Kobayashi,Daisuke" <kobayashi.da-06@xxxxxxxxxxx> ... snip ...
