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

Daisuke Kobayashi (Fujitsu)
2025-01-06T02:41:08Z

Gentle ping. Could you check this patch?

> 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 ...