Re: [PATCH] iommu/amd: fix missing tag from dev_err message

From: Hook, Gary
Date: Tue Jul 03 2018 - 12:21:51 EST

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.