linux-kernel - Re: [PATCH v3 2/6] iommu/vt-d: Add Intel IOMMU debugfs to show context internals

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5A30905D.9000303@linux.intel.com>
Date:   Wed, 13 Dec 2017 10:28:45 +0800
From:   Lu Baolu <baolu.lu@...ux.intel.com>
To:     "Mehta, Sohil" <sohil.mehta@...el.com>,
        "joro@...tes.org" <joro@...tes.org>,
        "alex.williamson@...hat.com" <alex.williamson@...hat.com>
Cc:     "Yu, Fenghua" <fenghua.yu@...el.com>,
        "Shankar, Ravi V" <ravi.v.shankar@...el.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Kammela, Gayatri" <gayatri.kammela@...el.com>,
        "iommu@...ts.linux-foundation.org" <iommu@...ts.linux-foundation.org>,
        "dwmw2@...radead.org" <dwmw2@...radead.org>,
        "Shevchenko, Andriy" <andriy.shevchenko@...el.com>
Subject: Re: [PATCH v3 2/6] iommu/vt-d: Add Intel IOMMU debugfs to show
 context internals

Hi,

Sorry for late reply.

On 12/08/2017 04:19 AM, Mehta, Sohil wrote:
> On Wed, 2017-12-06 at 16:16 +0800, Lu Baolu wrote:
>> Hi,
>>
>> On 12/06/2017 11:43 AM, Sohil Mehta wrote:
>>> From: Gayatri Kammela <gayatri.kammela@...el.com>
>>>
>>>  
>>> +	seq_printf(m, "%s Context table entries for Bus: %d\n",
>>> +		   ext ? "Lower" : "", bus);
>>> +	seq_printf(m, "[entry]\tDID :B :D .F\tLow\t\tHigh\n");
>> WARNING: Prefer seq_puts to seq_printf
>> #119: FILE: drivers/iommu/intel-iommu-debug.c:59:
>> +    seq_printf(m, "[entry]\tDID :B :D .F\tLow\t\tHigh\n");
>>
>> (caught by checkpatch.pl)
>>
> Hi Lu,
>
> We'll fix this and the other checkpatch.pl warnings.
>
>
>>> +
>>> +static void root_tbl_entry_show(struct seq_file *m, void *unused,
>> Why do you define the "unused" parameter which will never been used?
>> The same questions to other show functions.
>>
> Some functions in our code that are registered with seq_file needed to
> have an unused parameter since seq_file.h defines the show function as:
> 	int (*show) (struct seq_file *m, void *v);
>
> But a lot of other functions including the one you pointed don't need
> to have the unused parameter. We'll remove it from those.
>
>
>>> +void __init intel_iommu_debugfs_init(void)
>>> +{
>>> +	struct dentry *iommu_debug_root;
>>> +
>>> +	iommu_debug_root = debugfs_create_dir("intel_iommu",
>>> NULL);
>>> +
>>> +	if (!iommu_debug_root) {
>>> +		pr_err("can't create debugfs dir\n");
>> I don't think we need a pr_err() here. System works well even
>> debugfs_create_dir() returns NULL.
>>
>> This is same to all pr_err() in this file.
>>
> Would the recommendation be to use pr_warn instead of pr_err or should
> we entirely skip the message altogether?

Greg ever educated me about the use of debugfs_ functions in
this thread.

https://spinics.net/lists/linux-usb/msg159384.html

At least we should avoid the warning/error messages here.

Best regards,
Lu Baolu