[lkml]   [2018]   [Jul]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCH] iommu/amd: fix missing tag from dev_err message
On 7/3/2018 10:55 AM, Joe Perches wrote:
> On Tue, 2018-07-03 at 07:56 -0500, Gary R Hook wrote:
>> On 07/03/2018 05:07 AM, Joe Perches wrote:
>>> On Tue, 2018-07-03 at 07:40 +0100, Colin King wrote:
>>>> Currently tag is being assigned but not used, it is missing from
>>>> the dev_err message, so add it in.
>>>> Cleans up clang warning:
>>>> warning: variable 'tag' set but not used [-Wunused-but-set-variable]
>>> []
>>>> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
>>> []
>>>> @@ -616,9 +616,9 @@ static void iommu_print_event(struct amd_iommu *iommu, void *__evt)
>>>> pasid = ((event[0] >> 16) & 0xFFFF)
>>>> | ((event[1] << 6) & 0xF0000);
>>>> tag = event[1] & 0x03FF;
>>>> - dev_err(dev, "INVALID_PPR_REQUEST device=%02x:%02x.%x pasid=0x%05x address=0x%016llx flags=0x%04x]\n",
>>>> + dev_err(dev, "INVALID_PPR_REQUEST device=%02x:%02x.%x pasid=0x%05x address=0x%016llx flags=0x%04x tag=0x%03x]\n",
>>>> PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
>>>> - pasid, address, flags);
>>>> + pasid, address, flags, tag);
>>> Seems to have a superfluous ] that should be removed.
>> Yeah, I pretty much messed up all of the log messages in that function.
>> My apologies. I'll create a patch for that problem; it shouldn't be
>> fixed here.

Well, no, I misremembered. The extraneous square brace has been there
forever. Needs fixin', though.

> I also wonder why event is declared volatile and then
> dereferenced with [<constant>] multiple times.
> Maybe each array dereference should be stored as a
> local variable instead.

(I know you know this, but as I understand it) Event is pointing into
the (hardware's) event buffer, and the data structure has the potential
of changing out from under us if the device does something without our
knowledge. Since volatile hints to the compiler of this possibility, I
believe the compiler should manage this situation. But I could be wrong.

I don't know that we need to atomically copy all 16 bytes into a local
buffer, as I don't think it's possible for the device to step on itself.
It will just stop recording events if the buffer gets full. At this
moment I think volatile is overkill, at least for the EPYC/Ryzen IOMMU.

 \ /
  Last update: 2018-07-03 18:23    [W:0.082 / U:0.056 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site