Re: [PATCH v2] Revert "swiotlb: panic if nslabs is too small"

From: Robin Murphy
Date: Thu Sep 01 2022 - 08:24:33 EST


On 2022-09-01 03:18, Dongli Zhang wrote:
Hi Yu,

On 8/31/22 5:24 PM, Yu Zhao wrote:
On Wed, Aug 31, 2022 at 4:20 PM Dongli Zhang <dongli.zhang@xxxxxxxxxx> wrote:

Hi Yu,

As we discussed in the past, the swiotlb panic on purpose

We should not panic() at all, especially on a platform that has been
working well since at least 4.14.

Did you check out this link I previously shared with you?
https://urldefense.com/v3/__https://lore.kernel.org/r/CAHk-=wit-DmhMfQErY29JSPjFgebx_Ld*pnerc4J2Ag990WwAA@xxxxxxxxxxxxxx/__;Kw!!ACWV5N9M2RV99hQ!PXzSLurBv7VqxI1451TV4zO3_-BYj4grk-HYBsXzSnA6nZcXaBzdsQ-rF2DAqlICSRPMt-efYv_Uu2A2CQ$

Thanks for sharing! I used to read that in the past. To be honest I am still
confused on when to use BUG/panic and when to not, as I still see many usage in
some patches.

Just about swiotlb, it may panic in many cases during boot, e.g.,:

https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1955655

That's really a different thing, but frankly that panic is also bogus anyway - there is no good reason to have different behaviour for failing to allocate a buffer slot because the buffer is full vs. failing to allocate a buffer slot because there is no buffer. If we can fail gracefully some of the time we should fail gracefully all of the time. Yes, there's a slight difference in that one case has a chance of succeeding if retried in future while the other definitely doesn't, but it's not SWIOTLB's place to decide that the entire system is terminally unusable just because some device can't make a DMA mapping.

Similarly for the other panics at init time, which seem to have originated from a mechanical refactoring of the memblock API with the expectation of preserving the existing behaviour at the time. Those have then just been moved around without anyone thinking to question why they're there, and if memblock *does* now return usable error information, why don't we start handling that error properly like we do in other init paths?

Really there is no reason to panic anywhere in SWIOTLB. This is old code, things have moved on over the last 20 years, and we can and should do much better. I'll add this to my list of things to look at for cleanup once I find a bit of free time, unless anyone else fancies taking it on.

Thanks,
Robin.