Messages in this thread |  | | From | (Eric W. Biederman) | Date | Tue, 08 Jan 2013 16:04:52 -0800 | Subject | Re: [PATCH v7u1 26/31] x86: Don't enable swiotlb if there is not enough ram for it |
| |
Yinghai Lu <yinghai@kernel.org> writes:
> On Mon, Jan 7, 2013 at 7:50 PM, Yinghai Lu <yinghai@kernel.org> wrote: >> On Mon, Jan 7, 2013 at 7:13 PM, Eric W. Biederman <ebiederm@xmission.com> wrote: >>> I meant we should detect failure to allocate bounce buffers in in >>> swiotlb_init() instead of panicing. >>> >>> I meant swiotlb_map_single() should either panic or simply fail. >>> >>> If I have read lib/swiotlb.c correctly the only place we allocate a >>> bounce buffer is in swiotlb_map_single. If there are more places we can >>> allocate bounce buffers those need to be handled as well. >> >> ok, will give it a try. > > please check if you are ok with attached. >
It looks like the right direction. Certainly enough to test and see if the code will work.
I don't see the point of adding a nopanic case to the swiotlb initialization. That just looks like unnecessary complications. Certainly a nopanic case implemented by passing a nopanic parameter looks like the wrong way to go. At most you want to return an error code and do:
swiotlb_init() { if (swiotlb_init_with_default_size() == -ENOMEM) panic("Cannot allocate SWIOTLB buffer"); }
The page freeing in swiotlb_init_with_tbl appears to be in the wrong function. I suggest looking at swiotlb_late_init which apparently is allowed to fail for some ideas.
> looks like it need more change of lines.
The size of the change matters less than how clean and maintainable the result is. If done carefully I expect you can have net fewer lines but not needing to handle the case when the swiotlb apis are unavailable.
Eric
|  |