RE: [PATCH v5 2/2] Export Power Budgeting Extended Capability into pci-bus-sysfs

Daisuke Kobayashi (Fujitsu)
2024-12-13T09:42:17Z

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@fujitsu.com> wrote:
>
> > Jonathan Cameron wrote:
> > > On Tue, 10 Dec 2024 13:08:21 +0900
> > > "Kobayashi,Daisuke" <kobayashi.da-06@fujitsu.com> 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@fujitsu.com>
... snip ...