lkml.org 
[lkml]   [2014]   [Mar]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCHv3 0/6] Crashdump Accepting Active IOMMU
Date
Hi Joerg,

On Tue 3/4/2014 9:06 AM, Joerg Roedel wrote:
>
>On Fri, Jan 10, 2014 at 03:07:26PM -0700, Bill Sumner wrote:
>> Bill Sumner (6):
>> Crashdump-Accepting-Active-IOMMU-Flags-and-Prototype
>> Crashdump-Accepting-Active-IOMMU-Utility-functions
>> Crashdump-Accepting-Active-IOMMU-Domain-Interfaces
>> Crashdump-Accepting-Active-IOMMU-Copy-Translations
>> Crashdump-Accepting-Active-IOMMU-Debug-Print-IOMMU
>> Crashdump-Accepting-Active-IOMMU-Call-From-Mainline
>>
>> drivers/iommu/intel-iommu.c | 1293
>> ++++++++++++++++++++++++++++++++++++++++---
>> 1 file changed, 1225 insertions(+), 68 deletions(-)
>
>This is a pretty big change, before merging I would like to get more eyes on it. Please make sure you get more reviews on the code and the general concept. A few things I noticed while looking at it:
Yes, as I was developing this, I realized that it is far more code than most Linux submissions. In addition to more eyes reviewing the code, I hope to see additional members of the community trying it out -- testing it on a wide variety of system configurations. Let me encourage additional reviewers and testers to look at this fix to crashdump.
>
>* You put a lot of code into #define CONFIG_CRASH_DUMP. Isn't the same approach necessary to for KEXEC?
Until I saw your email, I had been focused only on the immediate problem of fixing crashdump and had not considered using this approach in the more-general context of KEXEC. It certainly looks like it could be useful with KEXEC as well.

I would like to take-on the effort of expanding this technique into KEXEC as a separate follow-on project so that the original "fix crashdump" project is not delayed. On a quick look, I believe that the amount of additional code would be minimal; but the expansion of the testing matrix might be quite large.
>
>* Please get rid of all the pr_debug stuff.
Will do.
>
>* I think it makes sense to put the functions to access the IOMMU
> configuration values of the old kernel into a new file. This is better
> than adding over 1000 new lines to intel-iommu.c
Sounds good. I will move these functions into a new file.
>
>* I have seen your new single-patch for this change. A single patch with
> more than 1200 changed lines is not something anyone is willing to
> review. Please split the patches again in a meaningful and bisectable
> way.
I will return to a multiple-patch set for future submissions.


-- Bill


\
 
 \ /
  Last update: 2014-03-10 21:21    [W:0.159 / U:0.936 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site