Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH v2 1/5] iommu/amd - Add debugfs support
To:     Andy Shevchenko <andy.shevchenko@gmail.com>
Cc:     iommu@lists.linux-foundation.org, Joerg Roedel <joro@8bytes.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
References: <152055660594.63229.5131049527614494130.stgit@sosxen2.amd.com>
 <152055664720.63229.16209149030018336339.stgit@sosxen2.amd.com>
 <CAHp75VdK6qFopBE4rD4yNO5GHccxyte=GhYU7YGcWOEYSCyX8Q@mail.gmail.com>
From:   Gary R Hook <gary.hook@amd.com>
Message-ID: <30bd9559-0e44-bd18-6b9a-ec35bc8276f3@amd.com>
Date:   Tue, 13 Mar 2018 13:54:00 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.6.0
MIME-Version: 1.0
In-Reply-To: <CAHp75VdK6qFopBE4rD4yNO5GHccxyte=GhYU7YGcWOEYSCyX8Q@mail.gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Received-SPF: None (protection.outlook.com: amd.com does not designate
 permitted sender hosts)
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Mar 2018 18:54:05.4563 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 81d43744-021d-4bb6-4f6f-08d58913cbec
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN1PR12MB0702
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 03/13/2018 12:16 PM, Andy Shevchenko wrote:
> On Fri, Mar 9, 2018 at 2:50 AM, Gary R Hook <gary.hook@amd.com> wrote:
> 
>> +       default n
> 
> Redundant

Roger that.

>> +#include <linux/pci.h>
>> +#include <linux/iommu.h>
>> +#include <linux/debugfs.h>
> 
> Keep in order?

What order would that be? These few needed files are listed in the same
order as which they appear in amd_iommu.c. I'm gonna need a preference
spelled out, please (and a rationale, so I may better understand).

>> +#include "amd_iommu_proto.h"
>> +#include "amd_iommu_types.h"
> 
>> +/* DebugFS helpers */
>> +#define        OBUFP           (obuf + oboff)
>> +#define        OBUFLEN         obuflen
>> +#define        OBUFSPC         (OBUFLEN - oboff)
>> +#define        OSCNPRINTF(fmt, ...) \
>> +               scnprintf(OBUFP, OBUFSPC, fmt, ## __VA_ARGS__)
> 
> I don't see any advantages of this. Other way around, they will simple
> makes things hard to read an understand in place.

I used this technique in the CCP driver code (where it was accepted), in 
an effort to do the opposite of what you claim: make the code more 
readable. Given the 80 column limit, a large number of arguments, and 
very long statements, IMO something needs to give. I don't find the use 
of #defines to be obfuscating.

I'm not trying to argue, but rather simply state the perspective / 
reasoning I used to create a source file I feel is manageable. I have 17 
more iommu patches built upon this strategy, and this seems to be 
advantageous for all of them.

> 
> 
>> +       for (i = start ; i <= end ; i++)
> 
> Missed {}

Wasn't sure about the M.O. given that the body of this loop is a single 
if statement. And I don't see anywhere in
https://www.kernel.org/doc/html/latest/process/coding-style.html
in section 3.1 where curly braces are called for in this situation. May 
I ask for clarification on the style rule, please?

> 
>> +               if ((amd_iommu_dev_table[i].data[0] ^ 0x3)
>> +                   || amd_iommu_dev_table[i].data[1])
>> +                       n++;
>> +       return n;
>> +}
> 
>> +
>> +static ssize_t amd_iommu_debugfs_dtecount_read(struct file *filp,
>> +                                         char __user *ubuf,
>> +                                         size_t count, loff_t *offp)
>> +{
>> +       struct amd_iommu *iommu = filp->private_data;
> 
>> +       unsigned int obuflen = 512;
> 
> Sounds like way too much.

I can tune these up.

> 
>> +       if (!iommu)
>> +               return 0;
> 
> When this possible?

It was intended as a sanity check, but if this happens, much worse has 
already gone wrong. I'll remove.

> 
>> +       obuf = kmalloc(OBUFLEN, GFP_KERNEL);
>> +       if (!obuf)
>> +               return -ENOMEM;
>> +
>> +       n = amd_iommu_count_valid_dtes(0, 0xFFFF);
>> +       oboff += OSCNPRINTF("%d\n", n);
> 
>> +       return ret;
>> +}
> 
> 
>> @@ -89,6 +89,7 @@
>>   #define ACPI_DEVFLAG_ATSDIS             0x10000000
>>
>>   #define LOOP_TIMEOUT   100000
>> +
>>   /*
>>    * ACPI table definitions
>>    *
> 
> Doesn't belong to the patch.

I'm sorry, I don't understand. The added blank line doesn't belong to 
the patch?

> 
>> +#endif
>> +
>> +
> 
> Extra unneeded line.
> 
Thanks,