On Wed, 14 Jun 2023 10:52:54 +0200, Oswald Buddenhagen wrote: > > On Wed, Jun 14, 2023 at 08:36:47AM +0200, Takashi Iwai wrote: > > On Tue, 13 Jun 2023 19:14:18 +0200, > > Oswald Buddenhagen wrote: > >> On Tue, Jun 13, 2023 at 05:43:58PM +0200, Takashi Iwai wrote: > >> > Crashing an existing application is the worst-case scenario. > >> > a new driver (which this effectively is) crashing a broken > >> application > >> is perfectly legitimate, as it doesn't affect any existing users. > > > > No, you can't ignore it. > > > you're allowing _hypothetical_ crappy 3rd party code to dictate what > you can and cannot do. that's a completely unreasonable and > counterproductive attitude, akin to letting hostage-takers set the > rules. Oswald, it's no hypothetical, I have seen lots of applications that did crash with such mixer element changes in the past. It's no dictation by 3rd party. We simply must not crash things by an update (unless it's a must, something like a security fix). > >> > Oh well, that's really not a change to be advertised for creating / > >> > deleting kctls from the put callback at all. > >> > and? it's done, and it's basically impossible to revert. so we > >> may > >> reap its full benefits just as well, as i did in that previous commit. > > > > Well, I can revert your commit, too... > > > sure, but my point was that there are likely many more such commits, > some of them not explicitly marked as such. it would be a very costly > and risky exercise to actually do that revert at this point. Sure, I didn't mean to do it immediately, it's no easy task. > > Basically the content protection shouldn't be covered by this rwsem. > > It's rather a misuse. > > > yes, sort of. > otoh, the commit message is rather convincing, and you clearly saw it > that way as well. I wasn't really convinced at that time, too, but the commit was the easiest workaround, so we agreed on taking it. Basically it's still a bad idea to use cards_rwsem lock for the content protection of each kctl. This should be revised, but it'll be a much wider work than a single revert or such, and certainly a lower priority task. > >> > Sorry, but my answer is same: NO. I see no reason why kctl > > >> deletion > >> > and creation _must_ be implemented _inevitably_ in that way. > >> > being the most straight-forward way to implement it certainly > >> qualifies as a good reason for doing it that way. > >> and i still see no convincing reason why it shouldn't. > > > > I still see no convincing reason why it must be done so, either. > > > the very convincing reason is that it was already done that way, and > you'd have to bring forward a very convincing justification for > further investment into a much more complex solution, esp. considering > what driver we're talking about. Nah, as I repeatedly wrote, the whole idea is too risky, may crash things easily, so should be avoided as much as possible. A correct use of API doesn't mean that everything keeps working as is, unfortunately. > > The way you're trying to implement is an anti-pattern, > > > that's something you keep repeating in various ways, but i see no > evidence that there is an _actual_ problem. There were actual problems, and we had to address them. The API is there and it should be usable in the ideal world, but we know that it breaks far more than expected. We don't prohibit that API, but the actual use should be limited for very special use cases. If it were triggered in only certain (rare and race-free) situations, it'd be acceptable. But your patch allows every user to trigger it by the normal kctl value adjustment, which is simply no-go. Takashi
