On Thu, Jan 16, 2025 at 2:12 AM Karolina Stolarek
<karolina.stolarek@xxxxxxxxxx> wrote:
> On 15/01/2025 08:42, Jon Pan-Doh wrote:
> > Update name to reflect the broader definition of structs/variables that
> > are stored (e.g. ratelimits).
>
> I understand the intention behind this change, but I'm not fully
> convinced if we should mix AER error attributes with the tools to
> control error reporting/generation (ratelimits). I'd argue that
> "aer_info" name still doesn't express what the structure does.
>
> aer_stats sits in pci_dev, so I can see why you decided to use it; it's
> one of the few available places where we could keep a stateful ratelimit.
>
> How about creating a struct to keep all the ratelimits in one place and
> embedding that in the pci_dev?
In a previous version, I had a separate struct aer_ratelimits in
pci_dev (similar to your patch[1]):
@@ -346,7 +346,7 @@ struct pci_dev {
u8 hdr_type; /* PCI header type (`multi'
flag masked out) */
#ifdef CONFIG_PCIEAER
u16 aer_cap; /* AER capability offset */
struct aer_stats *aer_stats; /* AER stats for this device */
+ struct aer_ratelimits *aer_ratelimits; /* AER ratelimits
for this device */
However, in an internal review, Bjorn said:
> I don't think we need to burn two pointers here since we always populate both at the
> same time. Perhaps combine the stats and ratelimits into a single "struct aer_info" or
> similar.
It seems like the preference is to minimize additional memory in
pci_dev. Any suggestions for a more appropriate name than "aer_info"?
Otherwise, is this a good enough justification to maintain two pointers Bjorn?
Thanks,
Jon
[1] https://lore.kernel.org/linux-pci/68ef082c855b4e1d094dcfc9a861f43488b64922.1736341506.git.karolina.stolarek@xxxxxxxxxx/#Z31include:linux:pci.h
