Message ID | CAPM=9txeN-qCRJvYV552zdo2H9iVy1ruVrq=YdZBP5Dmpc3Jmg@mail.gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [git,pull] drm for 5.15-rc1 | expand |
On Mon, Aug 30, 2021 at 10:53 PM Dave Airlie <airlied@gmail.com> wrote: > > There are a bunch of conflicts with your tree, but none of them seem > too serious, but I might have missed something. No worries. I enjoyed seeing the AMD code-names in the conflicts, they are using positively kernel-level naming. That said, I wonder why AMD people can't use consistent formatting, mixing ALL-CAPS with underscores, spaces, whatever: /* Sienna_Cichlid */ /* Yellow Carp */ /* Navy_Flounder */ /* DIMGREY_CAVEFISH */ /* Aldebaran */ /* CYAN_SKILLFISH */ /* BEIGE_GOBY */ which shows a distinct lack of professionalism and caring in the silly naming. Linus
On Wed, Sep 1, 2021 at 10:57 AM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > No worries. I enjoyed seeing the AMD code-names in the conflicts, they > are using positively kernel-level naming. Oh, I spoke too soon. The conflict in amdgpu_ras_eeprom.c is trivial to fix up, but the *code* is garbage. It does this (from commit 14fb496a84f1: "drm/amdgpu: set RAS EEPROM address from VBIOS"): ... control->i2c_address = 0; if (amdgpu_atomfirmware_ras_rom_addr(adev, (uint8_t*)&control->i2c_address)) { if (control->i2c_address == 0xA0) control->i2c_address = 0; ... and honestly, that just hurts to look at. It's completely wrong, even if it happens to work on a little-endian machine. Yes, yes, BE is irrelevant, and doubly so for an AMD GPU driver, but still. It's assigning a 8-bit value to a 32-bit entity by doing a pointer cast on the address, and then mixing things up by using/assigning to that same field. That's just *wrong* and nasty. Oh, the resolution would be easy - just take that broken code as-is - but I can't actually make myself do that. So I fixed it up to not be that incredibly ugly garbage. Please holler if I did something wrong. Linus
The pull request you sent on Tue, 31 Aug 2021 15:53:10 +1000:
> git://anongit.freedesktop.org/drm/drm tags/drm-next-2021-08-31-1
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/477f70cd2a67904e04c2c2b9bd0fa2e95222f2f6
Thank you!
On Wed, Sep 1, 2021 at 2:33 PM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > On Wed, Sep 1, 2021 at 10:57 AM Linus Torvalds > <torvalds@linux-foundation.org> wrote: > > > > No worries. I enjoyed seeing the AMD code-names in the conflicts, they > > are using positively kernel-level naming. > > Oh, I spoke too soon. > > The conflict in amdgpu_ras_eeprom.c is trivial to fix up, but the > *code* is garbage. > > It does this (from commit 14fb496a84f1: "drm/amdgpu: set RAS EEPROM > address from VBIOS"): > > ... > control->i2c_address = 0; > > if (amdgpu_atomfirmware_ras_rom_addr(adev, > (uint8_t*)&control->i2c_address)) > { > if (control->i2c_address == 0xA0) > control->i2c_address = 0; > ... > > and honestly, that just hurts to look at. It's completely wrong, even > if it happens to work on a little-endian machine. > > Yes, yes, BE is irrelevant, and doubly so for an AMD GPU driver, but still. > > It's assigning a 8-bit value to a 32-bit entity by doing a pointer > cast on the address, and then mixing things up by using/assigning to > that same field. > > That's just *wrong* and nasty. > > Oh, the resolution would be easy - just take that broken code as-is - > but I can't actually make myself do that. > > So I fixed it up to not be that incredibly ugly garbage. > > Please holler if I did something wrong. Fix looks good. Thanks, Alex