From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eric Anholt Subject: Re: [PATCH 0/6] Fix crash after reloading a driver using ttm Date: Tue, 16 Apr 2019 16:09:27 -0700 Message-ID: <875zrdsg2g.fsf@anholt.net> References: <20190416003523.5069-1-kherbst@redhat.com> <8dd57fe9-6f8d-f381-613e-0c4c4970a0b1@gmail.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1376206931==" Return-path: Received: from anholt.net (anholt.net [50.246.234.109]) by gabe.freedesktop.org (Postfix) with ESMTP id 8ACFC89DBC for ; Tue, 16 Apr 2019 23:09:30 +0000 (UTC) In-Reply-To: <8dd57fe9-6f8d-f381-613e-0c4c4970a0b1@gmail.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: christian.koenig@amd.com, Karol Herbst , dri-devel@lists.freedesktop.org Cc: Alex Deucher List-Id: dri-devel@lists.freedesktop.org --===============1376206931== Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha512; protocol="application/pgp-signature" --=-=-= Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Christian K=C3=B6nig writes: > Am 16.04.19 um 02:35 schrieb Karol Herbst: >> Kobjects are supposed to be dynamically allocated, but with recent chang= es >> this rule was violated. Reverting those commits fixes crashes when a drm >> driver using TTM gets loaded again. >> >> The object in question is "ttm_mem_glob" declared inside >> "include/drm/ttm/ttm_memory.h" and instatiated inside >> "drivers/gpu/drm/ttm/ttm_memory.c". >> >> from "Documentation/kobject.txt": >> "Because kobjects are dynamic, they must not be declared statically or on >> the stack, but instead, always allocated dynamically. Future versions of >> the kernel will contain a run-time check for kobjects that are created >> statically and will warn the developer of this improper usage." >> >> Unloading ttm before reloading the driver workarounds that crash, because >> the memory backing the kobject member "kobj" is cleaned up. The kobject_= del >> and kobject_put function never free or clean up the kobject object leavi= ng >> it in an undefined state. >> >> I reverted a few more commits to make it less painful for me to rever th= is >> rather big change. > > Well, NAK. By reverting those change you also re-introduced the problems= =20 > we originally fixed with those patches. > > Please work on a proper fix instead, That's not Karol's responsibility, that's yours as the author. I would like to remind about Linux's regressions policy, quoting from Documentation/process/4.Coding.rst: "One final hazard worth mentioning is this: it can be tempting to make a change (which may bring big improvements) which causes something to break for existing users. This kind of change is called a "regression," and regressions have become most unwelcome in the mainline kernel. With few exceptions, changes which cause regressions will be backed out if the regression cannot be fixed in a timely manner. Far better to avoid the regression in the first place. It is often argued that a regression can be justified if it causes things to work for more people than it creates problems for. Why not make a change if it brings new functionality to ten systems for each one it breaks? The best answer to this question was expressed by Linus in July, 2007: :: So we don't fix bugs by introducing new problems. That way lies madness, and nobody ever knows if you actually make any real progress at all. Is it two steps forwards, one step back, or one step forward and two steps back?" --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEE/JuuFDWp9/ZkuCBXtdYpNtH8nugFAly2YKcACgkQtdYpNtH8 nugzpRAAsIt1Xyl9o/hzGYl0oY7pqybX6jm1LpADkaR/we0nMa3O2t9jwLLQNf9j QVw5pKnkJhufBtM7+3Wa65Ra+AesI7I1gC3aT/0Ap/TBhWV3mu90u4dpDCg9qESg EFEn68dTNOG3L7KZDE1zxNtPYuz2Ldc6jeZuZ173WdrrsTJnS/BQq0t68d5ccBke O0gk4an8I/kyf7Yi+4EMkw5MtVq2OnZmcHJHJgfHqwVVgylLjw3y7574xVmonTq/ elaVB5dqyfxS5o2UY1fhtbc/6GyJcPkq4UUoyM9lh3GrsjxsMnkI3de3F85R0nwd FtigqGy5Pmv/bTGgI5YCyelZ+DKjpgIPpaxYdcCUYyxT3u+2WKbtZjl+7Xw5JyQS 20NbFJFAPlFRWBg2r4lY4LD15bIQ4wRsmA2GEoeLSFbKHZMu9BsNGjEAGpZJGugh 9ZQYp7LbFxYQBX9Wiwi1Dj9exDz8JNOnVK69tkRvwv2aklAPAY3v/489guN9+hTW Mcgkn5JEaQ4p74h04aUToFah6n1HMj8LlFSER8X9MUCi48E15TeT33f5raZs6tU/ c3Q/CVSiR1B4ZtosI/8c9X6b1TL7klXT+sZ7/pdLWNm/IKJprmZe/D0cec8jKmxN 0APLEpb3EO0kMWFSYqlSAHawGyYCtGXes65AAD0q4byT0YO9SKg= =mU/W -----END PGP SIGNATURE----- --=-=-=-- --===============1376206931== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVs --===============1376206931==--