On Tue, Jan 05, 2021 at 02:44:00PM +0100, Christian König wrote:
> Hi Bjorn,
>
> Darren stumbled over an AMD GPU with nonsense in it's resizeable BAR capability dword.
>
> This is most likely fixable with a VBIOS update, but we already sold quite a bunch of those boards with the problem.
>
> The driver still loads without this, but the performance isn't the best.
>
> Do you have any objection to merge this through the drm branch?
I'm OK with this in general, but please pay attention to your commit
logs:
- Subject line prefix is "PCI: " (capitalized).
- First word of subject is capitalized ("Add ...", "Export ...").
- No period at end of subject ("4/4 ... XT Pulse.").
- No "v2" included in subject line ("PCI: Add BAR ... v2").
- Include parentheses after function names, e.g.,
"pci_rebar_get_possible_sizes()".
- Commit logs should say directly what the patch does using
imperative mood, not just "to assist with X". E.g., "Export
pci_rebar_get_possible_sizes() for use by modular drivers."
This should make sense even without reading the subject line.
- No "v2" verbiage included in commit log (you can include it after
the "---" line if you want). That verbiage in 2/4 also contains
two typos.
- 2/4 does two things, so split it into two patches. Then the
subject and commit logs will make more sense.
- Splitting 2/4 will also give you an opportunity to explain why we
need to change "static inline" to "static __always_inline". If we
don't need that change, don't change it.
If we *do* need it, that means we likely need it for many other
things in include/linux/pci.h, and I would want to fix all those
places at once in a separate patch.
- 4/4 should be more specific about what's wrong. Based on the
code, this device advertises support for BAR 0 being 256MB, 512MB,
or 1GB, but it actually supports 2GB, 4GB, 8GB, and 16GB as well.
Please spell that out in the commit log. "Otherwise the CPU can't
fully access the BAR" really doesn't tell me what the patch does.
