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.