On 18/01/2025 02:59, Jon Pan-Doh wrote:
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.
I see, thanks for sharing it here.
My thinking is that it's preferable to have small and specialized
structs, with one used for collecting/showing error info and the other
to control the rates at which they are reported and generated. I'd
propose a similar change to the one above.
It seems like the preference is to minimize additional memory in
pci_dev. Any suggestions for a more appropriate name than "aer_info"?
Like I said, my preference would be to leave aer_stats as it is, but if
we have to stick to one struct, I'd go with "aer_control" or "aer_report".
All the best,
Karolina
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
