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

["Hook, Gary" <ghook@amd.com>]

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.