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

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

[Colin King]

From: Colin Ian King <colin.king@canonical.com>

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]

Fixes: e7f63ffc1bf1 ("iommu/amd: Update logging information for new event type")
Signed-off-by: Colin Ian King <colin.king@canonical.com>

---
This is a re-working of an earlier incorrect fix that I posted
yesterday ("iommu/amd: remove redundant variable tag")

---
 drivers/iommu/amd_iommu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 596b95c50051..6adab2690793 100644
--- 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);
 		break;
 	default:
 		dev_err(dev, "UNKNOWN event[0]=0x%08x event[1]=0x%08x event[2]=0x%08x event[3]=0x%08x\n",
-- 
2.17.1

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

[Colin King]

From: Colin Ian King <colin.king@canonical.com>

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]

Fixes: e7f63ffc1bf1 ("iommu/amd: Update logging information for new event type")
Signed-off-by: Colin Ian King <colin.king@canonical.com>

---
This is a re-working of an earlier incorrect fix that I posted
yesterday ("iommu/amd: remove redundant variable tag")

---
 drivers/iommu/amd_iommu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 596b95c50051..6adab2690793 100644
--- 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);
 		break;
 	default:
 		dev_err(dev, "UNKNOWN event[0]=0x%08x event[1]=0x%08x event[2]=0x%08x event[3]=0x%08x\n",
-- 
2.17.1

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

[Joe Perches]

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.

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

[Joe Perches]

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.

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

[Joe Perches]

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.

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

[Gary R Hook]

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.

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

[Gary R Hook]

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.

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

[Joe Perches]

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.

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.

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

[Joe Perches]

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.

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.

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

[Hook, Gary]

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.

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

[Hook, Gary]

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.

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

[Hook, Gary]

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.

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

[Colin Ian King]

On 03/07/18 17:21, Hook, Gary wrote:
> 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.
> 

The opening square bracket is much earlier:

dev_err(dev, "AMD-Vi: Event logged [");

..and all the subsequent dev_err messages have the trailing square bracket.



> 
>> 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.

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

[Colin Ian King]

On 03/07/18 17:21, Hook, Gary wrote:
> 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.
> 

The opening square bracket is much earlier:

dev_err(dev, "AMD-Vi: Event logged [");

..and all the subsequent dev_err messages have the trailing square bracket.



> 
>> 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.

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

[Colin Ian King]

On 03/07/18 17:21, Hook, Gary wrote:
> 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.
> 

The opening square bracket is much earlier:

dev_err(dev, "AMD-Vi: Event logged [");

..and all the subsequent dev_err messages have the trailing square bracket.



> 
>> 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.

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

[Hook, Gary]

On 7/3/2018 11:24 AM, Colin Ian King wrote:
> On 03/07/18 17:21, Hook, Gary wrote:
>> 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.
>>
> 
> The opening square bracket is much earlier:
> 
> dev_err(dev, "AMD-Vi: Event logged [");
> 
> ..and all the subsequent dev_err messages have the trailing square bracket.

Gah! Mystery solved. I'd forgotten about that.

"Never mind."

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

[Hook, Gary]

On 7/3/2018 11:24 AM, Colin Ian King wrote:
> On 03/07/18 17:21, Hook, Gary wrote:
>> 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.
>>
> 
> The opening square bracket is much earlier:
> 
> dev_err(dev, "AMD-Vi: Event logged [");
> 
> ..and all the subsequent dev_err messages have the trailing square bracket.

Gah! Mystery solved. I'd forgotten about that.

"Never mind."

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

[Hook, Gary]

On 7/3/2018 11:24 AM, Colin Ian King wrote:
> On 03/07/18 17:21, Hook, Gary wrote:
>> 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.
>>
> 
> The opening square bracket is much earlier:
> 
> dev_err(dev, "AMD-Vi: Event logged [");
> 
> ..and all the subsequent dev_err messages have the trailing square bracket.

Gah! Mystery solved. I'd forgotten about that.

"Never mind."


_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

[Joe Perches]

On Tue, 2018-07-03 at 11:27 -0500, Hook, Gary wrote:
> On 7/3/2018 11:24 AM, Colin Ian King wrote:
> > On 03/07/18 17:21, Hook, Gary wrote:
> > > 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.
> > > 
> > 
> > The opening square bracket is much earlier:
> > 
> > dev_err(dev, "AMD-Vi: Event logged [");
> > 
> > ..and all the subsequent dev_err messages have the trailing square bracket.
> 
> Gah! Mystery solved. I'd forgotten about that.
> 
> "Never mind."
> 

It stems from the misconversion from printk to dev_err

The initial dev_err is fine but all the dev_err uses
in the switch/case should use pr_cont, not dev_err

(there's a suggested patch below this commit reference)

---
From 90ca3859f5ea90050d00e695355934b37357e7bb Mon Sep 17 00:00:00 2001
From: Gary R Hook <gary.hook@amd.com>
Date: Thu, 8 Mar 2018 18:34:41 -0600
Subject: [PATCH] iommu/amd: Use dev_err to send events to the system log

Remove printk and use a more preferable error logging function.

Signed-off-by: Gary R Hook <gary.hook@amd.com>
Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/iommu/amd_iommu.c | 55 ++++++++++++++++++++++++++++---------------------------
 1 file changed, 28 insertions(+), 27 deletions(-)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 997a947ddc3b..4cd19f93ca15 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -547,6 +547,7 @@ static void amd_iommu_report_page_fault(u16 devid, u16 domain_id,
 
 static void iommu_print_event(struct amd_iommu *iommu, void *__evt)
 {
+       struct device *dev = iommu->iommu.dev;
        int type, devid, domid, flags;
        volatile u32 *event = __evt;
        int count = 0;
@@ -573,53 +574,53 @@ static void iommu_print_event(struct amd_iommu *iommu, void *__evt)
                amd_iommu_report_page_fault(devid, domid, address, flags);
                return;
        } else {
-               printk(KERN_ERR "AMD-Vi: Event logged [");
+               dev_err(dev, "AMD-Vi: Event logged [");
        }
 
        switch (type) {
        case EVENT_TYPE_ILL_DEV:
-               printk("ILLEGAL_DEV_TABLE_ENTRY device=%02x:%02x.%x "
-                      "address=0x%016llx flags=0x%04x]\n",
-                      PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
-                      address, flags);
+               dev_err(dev, "ILLEGAL_DEV_TABLE_ENTRY device=%02x:%02x.%x "
+                       "address=0x%016llx flags=0x%04x]\n",
+                       PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
+                       address, flags);

etc...

so this could be:

---
 drivers/iommu/amd_iommu.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 596b95c50051..f78cfa094301 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -572,43 +572,42 @@ static void iommu_print_event(struct amd_iommu *iommu, void *__evt)
 	if (type = EVENT_TYPE_IO_FAULT) {
 		amd_iommu_report_page_fault(devid, pasid, address, flags);
 		return;
-	} else {
-		dev_err(dev, "AMD-Vi: Event logged [");
 	}
 
+	dev_err(dev, "AMD-Vi: Event logged [");
+
 	switch (type) {
 	case EVENT_TYPE_ILL_DEV:
-		dev_err(dev, "ILLEGAL_DEV_TABLE_ENTRY device=%02x:%02x.%x pasid=0x%05x address=0x%016llx flags=0x%04x]\n",
+		pr_cont("ILLEGAL_DEV_TABLE_ENTRY device=%02x:%02x.%x pasid=0x%05x address=0x%016llx flags=0x%04x]\n",
 			PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
 			pasid, address, flags);
 		dump_dte_entry(devid);
 		break;
 	case EVENT_TYPE_DEV_TAB_ERR:
-		dev_err(dev, "DEV_TAB_HARDWARE_ERROR device=%02x:%02x.%x "
-			"address=0x%016llx flags=0x%04x]\n",
+		pr_cont("DEV_TAB_HARDWARE_ERROR device=%02x:%02x.%x address=0x%016llx flags=0x%04x]\n",
 			PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
 			address, flags);
 		break;
 	case EVENT_TYPE_PAGE_TAB_ERR:
-		dev_err(dev, "PAGE_TAB_HARDWARE_ERROR device=%02x:%02x.%x domain=0x%04x address=0x%016llx flags=0x%04x]\n",
+		pr_cont("PAGE_TAB_HARDWARE_ERROR device=%02x:%02x.%x domain=0x%04x address=0x%016llx flags=0x%04x]\n",
 			PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
 			pasid, address, flags);
 		break;
 	case EVENT_TYPE_ILL_CMD:
-		dev_err(dev, "ILLEGAL_COMMAND_ERROR address=0x%016llx]\n", address);
+		pr_cont("ILLEGAL_COMMAND_ERROR address=0x%016llx]\n", address);
 		dump_command(address);
 		break;
 	case EVENT_TYPE_CMD_HARD_ERR:
-		dev_err(dev, "COMMAND_HARDWARE_ERROR address=0x%016llx flags=0x%04x]\n",
+		pr_cont("COMMAND_HARDWARE_ERROR address=0x%016llx flags=0x%04x]\n",
 			address, flags);
 		break;
 	case EVENT_TYPE_IOTLB_INV_TO:
-		dev_err(dev, "IOTLB_INV_TIMEOUT device=%02x:%02x.%x address=0x%016llx]\n",
+		pr_cont("IOTLB_INV_TIMEOUT device=%02x:%02x.%x address=0x%016llx]\n",
 			PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
 			address);
 		break;
 	case EVENT_TYPE_INV_DEV_REQ:
-		dev_err(dev, "INVALID_DEVICE_REQUEST device=%02x:%02x.%x pasid=0x%05x address=0x%016llx flags=0x%04x]\n",
+		pr_cont("INVALID_DEVICE_REQUEST device=%02x:%02x.%x pasid=0x%05x address=0x%016llx flags=0x%04x]\n",
 			PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
 			pasid, address, flags);
 		break;
@@ -616,12 +615,12 @@ 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",
+		pr_cont("INVALID_PPR_REQUEST device=%02x:%02x.%x pasid=0x%05x address=0x%016llx flags=0x%04x]\n",
 			PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
 			pasid, address, flags);
 		break;
 	default:
-		dev_err(dev, "UNKNOWN event[0]=0x%08x event[1]=0x%08x event[2]=0x%08x event[3]=0x%08x\n",
+		pr_cont("UNKNOWN event[0]=0x%08x event[1]=0x%08x event[2]=0x%08x event[3]=0x%08x\n",
 			event[0], event[1], event[2], event[3]);
 	}
 

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

[Joe Perches]

On Tue, 2018-07-03 at 11:27 -0500, Hook, Gary wrote:
> On 7/3/2018 11:24 AM, Colin Ian King wrote:
> > On 03/07/18 17:21, Hook, Gary wrote:
> > > 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.
> > > 
> > 
> > The opening square bracket is much earlier:
> > 
> > dev_err(dev, "AMD-Vi: Event logged [");
> > 
> > ..and all the subsequent dev_err messages have the trailing square bracket.
> 
> Gah! Mystery solved. I'd forgotten about that.
> 
> "Never mind."
> 

It stems from the misconversion from printk to dev_err

The initial dev_err is fine but all the dev_err uses
in the switch/case should use pr_cont, not dev_err

(there's a suggested patch below this commit reference)

---
From 90ca3859f5ea90050d00e695355934b37357e7bb Mon Sep 17 00:00:00 2001
From: Gary R Hook <gary.hook@amd.com>
Date: Thu, 8 Mar 2018 18:34:41 -0600
Subject: [PATCH] iommu/amd: Use dev_err to send events to the system log

Remove printk and use a more preferable error logging function.

Signed-off-by: Gary R Hook <gary.hook@amd.com>
Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/iommu/amd_iommu.c | 55 ++++++++++++++++++++++++++++---------------------------
 1 file changed, 28 insertions(+), 27 deletions(-)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 997a947ddc3b..4cd19f93ca15 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -547,6 +547,7 @@ static void amd_iommu_report_page_fault(u16 devid, u16 domain_id,
 
 static void iommu_print_event(struct amd_iommu *iommu, void *__evt)
 {
+       struct device *dev = iommu->iommu.dev;
        int type, devid, domid, flags;
        volatile u32 *event = __evt;
        int count = 0;
@@ -573,53 +574,53 @@ static void iommu_print_event(struct amd_iommu *iommu, void *__evt)
                amd_iommu_report_page_fault(devid, domid, address, flags);
                return;
        } else {
-               printk(KERN_ERR "AMD-Vi: Event logged [");
+               dev_err(dev, "AMD-Vi: Event logged [");
        }
 
        switch (type) {
        case EVENT_TYPE_ILL_DEV:
-               printk("ILLEGAL_DEV_TABLE_ENTRY device=%02x:%02x.%x "
-                      "address=0x%016llx flags=0x%04x]\n",
-                      PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
-                      address, flags);
+               dev_err(dev, "ILLEGAL_DEV_TABLE_ENTRY device=%02x:%02x.%x "
+                       "address=0x%016llx flags=0x%04x]\n",
+                       PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
+                       address, flags);

etc...

so this could be:

---
 drivers/iommu/amd_iommu.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 596b95c50051..f78cfa094301 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -572,43 +572,42 @@ static void iommu_print_event(struct amd_iommu *iommu, void *__evt)
 	if (type == EVENT_TYPE_IO_FAULT) {
 		amd_iommu_report_page_fault(devid, pasid, address, flags);
 		return;
-	} else {
-		dev_err(dev, "AMD-Vi: Event logged [");
 	}
 
+	dev_err(dev, "AMD-Vi: Event logged [");
+
 	switch (type) {
 	case EVENT_TYPE_ILL_DEV:
-		dev_err(dev, "ILLEGAL_DEV_TABLE_ENTRY device=%02x:%02x.%x pasid=0x%05x address=0x%016llx flags=0x%04x]\n",
+		pr_cont("ILLEGAL_DEV_TABLE_ENTRY device=%02x:%02x.%x pasid=0x%05x address=0x%016llx flags=0x%04x]\n",
 			PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
 			pasid, address, flags);
 		dump_dte_entry(devid);
 		break;
 	case EVENT_TYPE_DEV_TAB_ERR:
-		dev_err(dev, "DEV_TAB_HARDWARE_ERROR device=%02x:%02x.%x "
-			"address=0x%016llx flags=0x%04x]\n",
+		pr_cont("DEV_TAB_HARDWARE_ERROR device=%02x:%02x.%x address=0x%016llx flags=0x%04x]\n",
 			PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
 			address, flags);
 		break;
 	case EVENT_TYPE_PAGE_TAB_ERR:
-		dev_err(dev, "PAGE_TAB_HARDWARE_ERROR device=%02x:%02x.%x domain=0x%04x address=0x%016llx flags=0x%04x]\n",
+		pr_cont("PAGE_TAB_HARDWARE_ERROR device=%02x:%02x.%x domain=0x%04x address=0x%016llx flags=0x%04x]\n",
 			PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
 			pasid, address, flags);
 		break;
 	case EVENT_TYPE_ILL_CMD:
-		dev_err(dev, "ILLEGAL_COMMAND_ERROR address=0x%016llx]\n", address);
+		pr_cont("ILLEGAL_COMMAND_ERROR address=0x%016llx]\n", address);
 		dump_command(address);
 		break;
 	case EVENT_TYPE_CMD_HARD_ERR:
-		dev_err(dev, "COMMAND_HARDWARE_ERROR address=0x%016llx flags=0x%04x]\n",
+		pr_cont("COMMAND_HARDWARE_ERROR address=0x%016llx flags=0x%04x]\n",
 			address, flags);
 		break;
 	case EVENT_TYPE_IOTLB_INV_TO:
-		dev_err(dev, "IOTLB_INV_TIMEOUT device=%02x:%02x.%x address=0x%016llx]\n",
+		pr_cont("IOTLB_INV_TIMEOUT device=%02x:%02x.%x address=0x%016llx]\n",
 			PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
 			address);
 		break;
 	case EVENT_TYPE_INV_DEV_REQ:
-		dev_err(dev, "INVALID_DEVICE_REQUEST device=%02x:%02x.%x pasid=0x%05x address=0x%016llx flags=0x%04x]\n",
+		pr_cont("INVALID_DEVICE_REQUEST device=%02x:%02x.%x pasid=0x%05x address=0x%016llx flags=0x%04x]\n",
 			PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
 			pasid, address, flags);
 		break;
@@ -616,12 +615,12 @@ 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",
+		pr_cont("INVALID_PPR_REQUEST device=%02x:%02x.%x pasid=0x%05x address=0x%016llx flags=0x%04x]\n",
 			PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
 			pasid, address, flags);
 		break;
 	default:
-		dev_err(dev, "UNKNOWN event[0]=0x%08x event[1]=0x%08x event[2]=0x%08x event[3]=0x%08x\n",
+		pr_cont("UNKNOWN event[0]=0x%08x event[1]=0x%08x event[2]=0x%08x event[3]=0x%08x\n",
 			event[0], event[1], event[2], event[3]);
 	}
 

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

[Joe Perches]

On Tue, 2018-07-03 at 11:27 -0500, Hook, Gary wrote:
> On 7/3/2018 11:24 AM, Colin Ian King wrote:
> > On 03/07/18 17:21, Hook, Gary wrote:
> > > 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.
> > > 
> > 
> > The opening square bracket is much earlier:
> > 
> > dev_err(dev, "AMD-Vi: Event logged [");
> > 
> > ..and all the subsequent dev_err messages have the trailing square bracket.
> 
> Gah! Mystery solved. I'd forgotten about that.
> 
> "Never mind."
> 

It stems from the misconversion from printk to dev_err

The initial dev_err is fine but all the dev_err uses
in the switch/case should use pr_cont, not dev_err

(there's a suggested patch below this commit reference)

---
>From 90ca3859f5ea90050d00e695355934b37357e7bb Mon Sep 17 00:00:00 2001
From: Gary R Hook <gary.hook@amd.com>
Date: Thu, 8 Mar 2018 18:34:41 -0600
Subject: [PATCH] iommu/amd: Use dev_err to send events to the system log

Remove printk and use a more preferable error logging function.

Signed-off-by: Gary R Hook <gary.hook@amd.com>
Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/iommu/amd_iommu.c | 55 ++++++++++++++++++++++++++++---------------------------
 1 file changed, 28 insertions(+), 27 deletions(-)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 997a947ddc3b..4cd19f93ca15 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -547,6 +547,7 @@ static void amd_iommu_report_page_fault(u16 devid, u16 domain_id,
 
 static void iommu_print_event(struct amd_iommu *iommu, void *__evt)
 {
+       struct device *dev = iommu->iommu.dev;
        int type, devid, domid, flags;
        volatile u32 *event = __evt;
        int count = 0;
@@ -573,53 +574,53 @@ static void iommu_print_event(struct amd_iommu *iommu, void *__evt)
                amd_iommu_report_page_fault(devid, domid, address, flags);
                return;
        } else {
-               printk(KERN_ERR "AMD-Vi: Event logged [");
+               dev_err(dev, "AMD-Vi: Event logged [");
        }
 
        switch (type) {
        case EVENT_TYPE_ILL_DEV:
-               printk("ILLEGAL_DEV_TABLE_ENTRY device=%02x:%02x.%x "
-                      "address=0x%016llx flags=0x%04x]\n",
-                      PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
-                      address, flags);
+               dev_err(dev, "ILLEGAL_DEV_TABLE_ENTRY device=%02x:%02x.%x "
+                       "address=0x%016llx flags=0x%04x]\n",
+                       PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
+                       address, flags);

etc...

so this could be:

---
 drivers/iommu/amd_iommu.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 596b95c50051..f78cfa094301 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -572,43 +572,42 @@ static void iommu_print_event(struct amd_iommu *iommu, void *__evt)
 	if (type == EVENT_TYPE_IO_FAULT) {
 		amd_iommu_report_page_fault(devid, pasid, address, flags);
 		return;
-	} else {
-		dev_err(dev, "AMD-Vi: Event logged [");
 	}
 
+	dev_err(dev, "AMD-Vi: Event logged [");
+
 	switch (type) {
 	case EVENT_TYPE_ILL_DEV:
-		dev_err(dev, "ILLEGAL_DEV_TABLE_ENTRY device=%02x:%02x.%x pasid=0x%05x address=0x%016llx flags=0x%04x]\n",
+		pr_cont("ILLEGAL_DEV_TABLE_ENTRY device=%02x:%02x.%x pasid=0x%05x address=0x%016llx flags=0x%04x]\n",
 			PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
 			pasid, address, flags);
 		dump_dte_entry(devid);
 		break;
 	case EVENT_TYPE_DEV_TAB_ERR:
-		dev_err(dev, "DEV_TAB_HARDWARE_ERROR device=%02x:%02x.%x "
-			"address=0x%016llx flags=0x%04x]\n",
+		pr_cont("DEV_TAB_HARDWARE_ERROR device=%02x:%02x.%x address=0x%016llx flags=0x%04x]\n",
 			PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
 			address, flags);
 		break;
 	case EVENT_TYPE_PAGE_TAB_ERR:
-		dev_err(dev, "PAGE_TAB_HARDWARE_ERROR device=%02x:%02x.%x domain=0x%04x address=0x%016llx flags=0x%04x]\n",
+		pr_cont("PAGE_TAB_HARDWARE_ERROR device=%02x:%02x.%x domain=0x%04x address=0x%016llx flags=0x%04x]\n",
 			PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
 			pasid, address, flags);
 		break;
 	case EVENT_TYPE_ILL_CMD:
-		dev_err(dev, "ILLEGAL_COMMAND_ERROR address=0x%016llx]\n", address);
+		pr_cont("ILLEGAL_COMMAND_ERROR address=0x%016llx]\n", address);
 		dump_command(address);
 		break;
 	case EVENT_TYPE_CMD_HARD_ERR:
-		dev_err(dev, "COMMAND_HARDWARE_ERROR address=0x%016llx flags=0x%04x]\n",
+		pr_cont("COMMAND_HARDWARE_ERROR address=0x%016llx flags=0x%04x]\n",
 			address, flags);
 		break;
 	case EVENT_TYPE_IOTLB_INV_TO:
-		dev_err(dev, "IOTLB_INV_TIMEOUT device=%02x:%02x.%x address=0x%016llx]\n",
+		pr_cont("IOTLB_INV_TIMEOUT device=%02x:%02x.%x address=0x%016llx]\n",
 			PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
 			address);
 		break;
 	case EVENT_TYPE_INV_DEV_REQ:
-		dev_err(dev, "INVALID_DEVICE_REQUEST device=%02x:%02x.%x pasid=0x%05x address=0x%016llx flags=0x%04x]\n",
+		pr_cont("INVALID_DEVICE_REQUEST device=%02x:%02x.%x pasid=0x%05x address=0x%016llx flags=0x%04x]\n",
 			PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
 			pasid, address, flags);
 		break;
@@ -616,12 +615,12 @@ 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",
+		pr_cont("INVALID_PPR_REQUEST device=%02x:%02x.%x pasid=0x%05x address=0x%016llx flags=0x%04x]\n",
 			PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
 			pasid, address, flags);
 		break;
 	default:
-		dev_err(dev, "UNKNOWN event[0]=0x%08x event[1]=0x%08x event[2]=0x%08x event[3]=0x%08x\n",
+		pr_cont("UNKNOWN event[0]=0x%08x event[1]=0x%08x event[2]=0x%08x event[3]=0x%08x\n",
 			event[0], event[1], event[2], event[3]);
 	}
 

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

[Gary R Hook]

On 07/03/2018 01:40 AM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> 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]
> 
> Fixes: e7f63ffc1bf1 ("iommu/amd: Update logging information for new event type")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> 
> ---
> This is a re-working of an earlier incorrect fix that I posted
> yesterday ("iommu/amd: remove redundant variable tag")

Acked-by: Gary R Hook <gary.hook@amd.com>

> 
> ---
>   drivers/iommu/amd_iommu.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
> index 596b95c50051..6adab2690793 100644
> --- 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);
>   		break;
>   	default:
>   		dev_err(dev, "UNKNOWN event[0]=0x%08x event[1]=0x%08x event[2]=0x%08x event[3]=0x%08x\n",
> 

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

[Gary R Hook]

On 07/03/2018 01:40 AM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> 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]
> 
> Fixes: e7f63ffc1bf1 ("iommu/amd: Update logging information for new event type")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> 
> ---
> This is a re-working of an earlier incorrect fix that I posted
> yesterday ("iommu/amd: remove redundant variable tag")

Acked-by: Gary R Hook <gary.hook@amd.com>

> 
> ---
>   drivers/iommu/amd_iommu.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
> index 596b95c50051..6adab2690793 100644
> --- 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);
>   		break;
>   	default:
>   		dev_err(dev, "UNKNOWN event[0]=0x%08x event[1]=0x%08x event[2]=0x%08x event[3]=0x%08x\n",
> 

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

[Gary R Hook]

On 07/03/2018 01:40 AM, Colin King wrote:
> From: Colin Ian King <colin.king-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
> 
> 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]
> 
> Fixes: e7f63ffc1bf1 ("iommu/amd: Update logging information for new event type")
> Signed-off-by: Colin Ian King <colin.king-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
> 
> ---
> This is a re-working of an earlier incorrect fix that I posted
> yesterday ("iommu/amd: remove redundant variable tag")

Acked-by: Gary R Hook <gary.hook-5C7GfCeVMHo@public.gmane.org>

> 
> ---
>   drivers/iommu/amd_iommu.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
> index 596b95c50051..6adab2690793 100644
> --- 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);
>   		break;
>   	default:
>   		dev_err(dev, "UNKNOWN event[0]=0x%08x event[1]=0x%08x event[2]=0x%08x event[3]=0x%08x\n",
> 

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

[Walter Harms]

Am 03.07.2018 18:38, schrieb Joe Perches:
> On Tue, 2018-07-03 at 11:27 -0500, Hook, Gary wrote:
>> On 7/3/2018 11:24 AM, Colin Ian King wrote:
>>> On 03/07/18 17:21, Hook, Gary wrote:
>>>> 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.
>>>>
>>>
>>> The opening square bracket is much earlier:
>>>
>>> dev_err(dev, "AMD-Vi: Event logged [");
>>>
>>> ..and all the subsequent dev_err messages have the trailing square bracket.
>>
>> Gah! Mystery solved. I'd forgotten about that.
>>
>> "Never mind."
>>

It is only cosmetics but even the author got lost about the loose bracket.
I would suggest to remove all the brackets or if needed to move the [ in the
message. We have enought memory this days.

> -               printk(KERN_ERR "AMD-Vi: Event logged [");
> +               dev_err(dev, "AMD-Vi: Event logged [");

just my 2 cents,

re,
 wh

> 
> It stems from the misconversion from printk to dev_err
> 
> The initial dev_err is fine but all the dev_err uses
> in the switch/case should use pr_cont, not dev_err
> 
> (there's a suggested patch below this commit reference)
> 
> ---
>>From 90ca3859f5ea90050d00e695355934b37357e7bb Mon Sep 17 00:00:00 2001
> From: Gary R Hook <gary.hook@amd.com>
> Date: Thu, 8 Mar 2018 18:34:41 -0600
> Subject: [PATCH] iommu/amd: Use dev_err to send events to the system log
> 
> Remove printk and use a more preferable error logging function.
> 
> Signed-off-by: Gary R Hook <gary.hook@amd.com>
> Signed-off-by: Joerg Roedel <jroedel@suse.de>
> ---
>  drivers/iommu/amd_iommu.c | 55
> ++++++++++++++++++++++++++++---------------------------
>  1 file changed, 28 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
> index 997a947ddc3b..4cd19f93ca15 100644
> --- a/drivers/iommu/amd_iommu.c
> +++ b/drivers/iommu/amd_iommu.c
> @@ -547,6 +547,7 @@ static void amd_iommu_report_page_fault(u16 devid, u16
> domain_id,
>  
>  static void iommu_print_event(struct amd_iommu *iommu, void *__evt)
>  {
> +       struct device *dev = iommu->iommu.dev;
>         int type, devid, domid, flags;
>         volatile u32 *event = __evt;
>         int count = 0;
> @@ -573,53 +574,53 @@ static void iommu_print_event(struct amd_iommu *iommu,
> void *__evt)
>                 amd_iommu_report_page_fault(devid, domid, address, flags);
>                 return;
>         } else {
> -               printk(KERN_ERR "AMD-Vi: Event logged [");
> +               dev_err(dev, "AMD-Vi: Event logged [");
>         }
>  
>         switch (type) {
>         case EVENT_TYPE_ILL_DEV:
> -               printk("ILLEGAL_DEV_TABLE_ENTRY device=%02x:%02x.%x "
> -                      "address=0x%016llx flags=0x%04x]\n",
> -                      PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
> -                      address, flags);
> +               dev_err(dev, "ILLEGAL_DEV_TABLE_ENTRY device=%02x:%02x.%x "
> +                       "address=0x%016llx flags=0x%04x]\n",
> +                       PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
> +                       address, flags);
> 
> etc...
> 
> so this could be:
> 
> ---
>  drivers/iommu/amd_iommu.c | 23 +++++++++++------------
>  1 file changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
> index 596b95c50051..f78cfa094301 100644
> --- a/drivers/iommu/amd_iommu.c
> +++ b/drivers/iommu/amd_iommu.c
> @@ -572,43 +572,42 @@ static void iommu_print_event(struct amd_iommu *iommu,
> void *__evt)
>  	if (type = EVENT_TYPE_IO_FAULT) {
>  		amd_iommu_report_page_fault(devid, pasid, address, flags);
>  		return;
> -	} else {
> -		dev_err(dev, "AMD-Vi: Event logged [");
>  	}
>  
> +	dev_err(dev, "AMD-Vi: Event logged [");
> +
>  	switch (type) {
>  	case EVENT_TYPE_ILL_DEV:
> -		dev_err(dev, "ILLEGAL_DEV_TABLE_ENTRY device=%02x:%02x.%x pasid=0x%05x
> address=0x%016llx flags=0x%04x]\n",
> +		pr_cont("ILLEGAL_DEV_TABLE_ENTRY device=%02x:%02x.%x pasid=0x%05x
> address=0x%016llx flags=0x%04x]\n",
>  			PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
>  			pasid, address, flags);
>  		dump_dte_entry(devid);
>  		break;
>  	case EVENT_TYPE_DEV_TAB_ERR:
> -		dev_err(dev, "DEV_TAB_HARDWARE_ERROR device=%02x:%02x.%x "
> -			"address=0x%016llx flags=0x%04x]\n",
> +		pr_cont("DEV_TAB_HARDWARE_ERROR device=%02x:%02x.%x address=0x%016llx
> flags=0x%04x]\n",
>  			PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
>  			address, flags);
>  		break;
>  	case EVENT_TYPE_PAGE_TAB_ERR:
> -		dev_err(dev, "PAGE_TAB_HARDWARE_ERROR device=%02x:%02x.%x domain=0x%04x
> address=0x%016llx flags=0x%04x]\n",
> +		pr_cont("PAGE_TAB_HARDWARE_ERROR device=%02x:%02x.%x domain=0x%04x
> address=0x%016llx flags=0x%04x]\n",
>  			PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
>  			pasid, address, flags);
>  		break;
>  	case EVENT_TYPE_ILL_CMD:
> -		dev_err(dev, "ILLEGAL_COMMAND_ERROR address=0x%016llx]\n", address);
> +		pr_cont("ILLEGAL_COMMAND_ERROR address=0x%016llx]\n", address);
>  		dump_command(address);
>  		break;
>  	case EVENT_TYPE_CMD_HARD_ERR:
> -		dev_err(dev, "COMMAND_HARDWARE_ERROR address=0x%016llx flags=0x%04x]\n",
> +		pr_cont("COMMAND_HARDWARE_ERROR address=0x%016llx flags=0x%04x]\n",
>  			address, flags);
>  		break;
>  	case EVENT_TYPE_IOTLB_INV_TO:
> -		dev_err(dev, "IOTLB_INV_TIMEOUT device=%02x:%02x.%x address=0x%016llx]\n",
> +		pr_cont("IOTLB_INV_TIMEOUT device=%02x:%02x.%x address=0x%016llx]\n",
>  			PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
>  			address);
>  		break;
>  	case EVENT_TYPE_INV_DEV_REQ:
> -		dev_err(dev, "INVALID_DEVICE_REQUEST device=%02x:%02x.%x pasid=0x%05x
> address=0x%016llx flags=0x%04x]\n",
> +		pr_cont("INVALID_DEVICE_REQUEST device=%02x:%02x.%x pasid=0x%05x
> address=0x%016llx flags=0x%04x]\n",
>  			PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
>  			pasid, address, flags);
>  		break;
> @@ -616,12 +615,12 @@ 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",
> +		pr_cont("INVALID_PPR_REQUEST device=%02x:%02x.%x pasid=0x%05x
> address=0x%016llx flags=0x%04x]\n",
>  			PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
>  			pasid, address, flags);
>  		break;
>  	default:
> -		dev_err(dev, "UNKNOWN event[0]=0x%08x event[1]=0x%08x event[2]=0x%08x
> event[3]=0x%08x\n",
> +		pr_cont("UNKNOWN event[0]=0x%08x event[1]=0x%08x event[2]=0x%08x
> event[3]=0x%08x\n",
>  			event[0], event[1], event[2], event[3]);
>  	}
>  
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

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

[Walter Harms]

Am 03.07.2018 18:38, schrieb Joe Perches:
> On Tue, 2018-07-03 at 11:27 -0500, Hook, Gary wrote:
>> On 7/3/2018 11:24 AM, Colin Ian King wrote:
>>> On 03/07/18 17:21, Hook, Gary wrote:
>>>> 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.
>>>>
>>>
>>> The opening square bracket is much earlier:
>>>
>>> dev_err(dev, "AMD-Vi: Event logged [");
>>>
>>> ..and all the subsequent dev_err messages have the trailing square bracket.
>>
>> Gah! Mystery solved. I'd forgotten about that.
>>
>> "Never mind."
>>

It is only cosmetics but even the author got lost about the loose bracket.
I would suggest to remove all the brackets or if needed to move the [ in the
message. We have enought memory this days.

> -               printk(KERN_ERR "AMD-Vi: Event logged [");
> +               dev_err(dev, "AMD-Vi: Event logged [");

just my 2 cents,

re,
 wh

> 
> It stems from the misconversion from printk to dev_err
> 
> The initial dev_err is fine but all the dev_err uses
> in the switch/case should use pr_cont, not dev_err
> 
> (there's a suggested patch below this commit reference)
> 
> ---
>>From 90ca3859f5ea90050d00e695355934b37357e7bb Mon Sep 17 00:00:00 2001
> From: Gary R Hook <gary.hook@amd.com>
> Date: Thu, 8 Mar 2018 18:34:41 -0600
> Subject: [PATCH] iommu/amd: Use dev_err to send events to the system log
> 
> Remove printk and use a more preferable error logging function.
> 
> Signed-off-by: Gary R Hook <gary.hook@amd.com>
> Signed-off-by: Joerg Roedel <jroedel@suse.de>
> ---
>  drivers/iommu/amd_iommu.c | 55
> ++++++++++++++++++++++++++++---------------------------
>  1 file changed, 28 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
> index 997a947ddc3b..4cd19f93ca15 100644
> --- a/drivers/iommu/amd_iommu.c
> +++ b/drivers/iommu/amd_iommu.c
> @@ -547,6 +547,7 @@ static void amd_iommu_report_page_fault(u16 devid, u16
> domain_id,
>  
>  static void iommu_print_event(struct amd_iommu *iommu, void *__evt)
>  {
> +       struct device *dev = iommu->iommu.dev;
>         int type, devid, domid, flags;
>         volatile u32 *event = __evt;
>         int count = 0;
> @@ -573,53 +574,53 @@ static void iommu_print_event(struct amd_iommu *iommu,
> void *__evt)
>                 amd_iommu_report_page_fault(devid, domid, address, flags);
>                 return;
>         } else {
> -               printk(KERN_ERR "AMD-Vi: Event logged [");
> +               dev_err(dev, "AMD-Vi: Event logged [");
>         }
>  
>         switch (type) {
>         case EVENT_TYPE_ILL_DEV:
> -               printk("ILLEGAL_DEV_TABLE_ENTRY device=%02x:%02x.%x "
> -                      "address=0x%016llx flags=0x%04x]\n",
> -                      PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
> -                      address, flags);
> +               dev_err(dev, "ILLEGAL_DEV_TABLE_ENTRY device=%02x:%02x.%x "
> +                       "address=0x%016llx flags=0x%04x]\n",
> +                       PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
> +                       address, flags);
> 
> etc...
> 
> so this could be:
> 
> ---
>  drivers/iommu/amd_iommu.c | 23 +++++++++++------------
>  1 file changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
> index 596b95c50051..f78cfa094301 100644
> --- a/drivers/iommu/amd_iommu.c
> +++ b/drivers/iommu/amd_iommu.c
> @@ -572,43 +572,42 @@ static void iommu_print_event(struct amd_iommu *iommu,
> void *__evt)
>  	if (type == EVENT_TYPE_IO_FAULT) {
>  		amd_iommu_report_page_fault(devid, pasid, address, flags);
>  		return;
> -	} else {
> -		dev_err(dev, "AMD-Vi: Event logged [");
>  	}
>  
> +	dev_err(dev, "AMD-Vi: Event logged [");
> +
>  	switch (type) {
>  	case EVENT_TYPE_ILL_DEV:
> -		dev_err(dev, "ILLEGAL_DEV_TABLE_ENTRY device=%02x:%02x.%x pasid=0x%05x
> address=0x%016llx flags=0x%04x]\n",
> +		pr_cont("ILLEGAL_DEV_TABLE_ENTRY device=%02x:%02x.%x pasid=0x%05x
> address=0x%016llx flags=0x%04x]\n",
>  			PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
>  			pasid, address, flags);
>  		dump_dte_entry(devid);
>  		break;
>  	case EVENT_TYPE_DEV_TAB_ERR:
> -		dev_err(dev, "DEV_TAB_HARDWARE_ERROR device=%02x:%02x.%x "
> -			"address=0x%016llx flags=0x%04x]\n",
> +		pr_cont("DEV_TAB_HARDWARE_ERROR device=%02x:%02x.%x address=0x%016llx
> flags=0x%04x]\n",
>  			PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
>  			address, flags);
>  		break;
>  	case EVENT_TYPE_PAGE_TAB_ERR:
> -		dev_err(dev, "PAGE_TAB_HARDWARE_ERROR device=%02x:%02x.%x domain=0x%04x
> address=0x%016llx flags=0x%04x]\n",
> +		pr_cont("PAGE_TAB_HARDWARE_ERROR device=%02x:%02x.%x domain=0x%04x
> address=0x%016llx flags=0x%04x]\n",
>  			PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
>  			pasid, address, flags);
>  		break;
>  	case EVENT_TYPE_ILL_CMD:
> -		dev_err(dev, "ILLEGAL_COMMAND_ERROR address=0x%016llx]\n", address);
> +		pr_cont("ILLEGAL_COMMAND_ERROR address=0x%016llx]\n", address);
>  		dump_command(address);
>  		break;
>  	case EVENT_TYPE_CMD_HARD_ERR:
> -		dev_err(dev, "COMMAND_HARDWARE_ERROR address=0x%016llx flags=0x%04x]\n",
> +		pr_cont("COMMAND_HARDWARE_ERROR address=0x%016llx flags=0x%04x]\n",
>  			address, flags);
>  		break;
>  	case EVENT_TYPE_IOTLB_INV_TO:
> -		dev_err(dev, "IOTLB_INV_TIMEOUT device=%02x:%02x.%x address=0x%016llx]\n",
> +		pr_cont("IOTLB_INV_TIMEOUT device=%02x:%02x.%x address=0x%016llx]\n",
>  			PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
>  			address);
>  		break;
>  	case EVENT_TYPE_INV_DEV_REQ:
> -		dev_err(dev, "INVALID_DEVICE_REQUEST device=%02x:%02x.%x pasid=0x%05x
> address=0x%016llx flags=0x%04x]\n",
> +		pr_cont("INVALID_DEVICE_REQUEST device=%02x:%02x.%x pasid=0x%05x
> address=0x%016llx flags=0x%04x]\n",
>  			PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
>  			pasid, address, flags);
>  		break;
> @@ -616,12 +615,12 @@ 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",
> +		pr_cont("INVALID_PPR_REQUEST device=%02x:%02x.%x pasid=0x%05x
> address=0x%016llx flags=0x%04x]\n",
>  			PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
>  			pasid, address, flags);
>  		break;
>  	default:
> -		dev_err(dev, "UNKNOWN event[0]=0x%08x event[1]=0x%08x event[2]=0x%08x
> event[3]=0x%08x\n",
> +		pr_cont("UNKNOWN event[0]=0x%08x event[1]=0x%08x event[2]=0x%08x
> event[3]=0x%08x\n",
>  			event[0], event[1], event[2], event[3]);
>  	}
>  
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

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

[Joe Perches]

On Tue, 2018-07-03 at 18:57 +0200, Walter Harms wrote:
> It is only cosmetics but even the author got lost about the loose bracket.
> I would suggest to remove all the brackets or if needed to move the [ in the
> message. We have enought memory this days.

My suggestion would be to remove the separate dev_err
and use "Event log [" in each dev_err.

So here's an actual suggested patch that does a few things:

o Coalesce formats and realigns arguments
o Uses pr_fmt and dev_fmt to prefix "AMD-Vi: " to all messages
o Remove embedded "AMD-Vi: " from various formats

Some existing pr_<level> uses now are prefixed too.
---
 drivers/iommu/amd_iommu.c | 69 +++++++++++++++++++++++------------------------
 1 file changed, 34 insertions(+), 35 deletions(-)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 596b95c50051..01583a8ccaf9 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -17,6 +17,9 @@
  * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307 USA
  */
 
+#define pr_fmt(fmt) "AMD-Vi: " fmt
+#define dev_fmt pr_fmt
+
 #include <linux/ratelimit.h>
 #include <linux/pci.h>
 #include <linux/acpi.h>
@@ -273,9 +276,8 @@ static u16 get_alias(struct device *dev)
 		return pci_alias;
 	}
 
-	pr_info("AMD-Vi: Using IVRS reported alias %02x:%02x.%d "
-		"for device %s[%04x:%04x], kernel reported alias "
-		"%02x:%02x.%d\n", PCI_BUS_NUM(ivrs_alias), PCI_SLOT(ivrs_alias),
+	pr_info("Using IVRS reported alias %02x:%02x.%d for device %s[%04x:%04x], kernel reported alias %02x:%02x.%d\n",
+		PCI_BUS_NUM(ivrs_alias), PCI_SLOT(ivrs_alias),
 		PCI_FUNC(ivrs_alias), dev_name(dev), pdev->vendor, pdev->device,
 		PCI_BUS_NUM(pci_alias), PCI_SLOT(pci_alias),
 		PCI_FUNC(pci_alias));
@@ -287,7 +289,7 @@ static u16 get_alias(struct device *dev)
 	if (pci_alias = devid &&
 	    PCI_BUS_NUM(ivrs_alias) = pdev->bus->number) {
 		pci_add_dma_alias(pdev, ivrs_alias & 0xff);
-		pr_info("AMD-Vi: Added PCI DMA alias %02x.%d for %s\n",
+		pr_info("Added PCI DMA alias %02x.%d for %s\n",
 			PCI_SLOT(ivrs_alias), PCI_FUNC(ivrs_alias),
 			dev_name(dev));
 	}
@@ -507,8 +509,8 @@ static void dump_dte_entry(u16 devid)
 	int i;
 
 	for (i = 0; i < 4; ++i)
-		pr_err("AMD-Vi: DTE[%d]: %016llx\n", i,
-			amd_iommu_dev_table[devid].data[i]);
+		pr_err("DTE[%d]: %016llx\n",
+		       i, amd_iommu_dev_table[devid].data[i]);
 }
 
 static void dump_command(unsigned long phys_addr)
@@ -517,7 +519,7 @@ static void dump_command(unsigned long phys_addr)
 	int i;
 
 	for (i = 0; i < 4; ++i)
-		pr_err("AMD-Vi: CMD[%d]: %08x\n", i, cmd->data[i]);
+		pr_err("CMD[%d]: %08x\n", i, cmd->data[i]);
 }
 
 static void amd_iommu_report_page_fault(u16 devid, u16 domain_id,
@@ -532,10 +534,10 @@ static void amd_iommu_report_page_fault(u16 devid, u16 domain_id,
 		dev_data = get_dev_data(&pdev->dev);
 
 	if (dev_data && __ratelimit(&dev_data->rs)) {
-		dev_err(&pdev->dev, "AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x%04x address=0x%016llx flags=0x%04x]\n",
+		dev_err(&pdev->dev, "Event logged [IO_PAGE_FAULT domain=0x%04x address=0x%016llx flags=0x%04x]\n",
 			domain_id, address, flags);
 	} else if (printk_ratelimit()) {
-		pr_err("AMD-Vi: Event logged [IO_PAGE_FAULT device=%02x:%02x.%x domain=0x%04x address=0x%016llx flags=0x%04x]\n",
+		pr_err("Event logged [IO_PAGE_FAULT device=%02x:%02x.%x domain=0x%04x address=0x%016llx flags=0x%04x]\n",
 			PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
 			domain_id, address, flags);
 	}
@@ -562,7 +564,7 @@ static void iommu_print_event(struct amd_iommu *iommu, void *__evt)
 	if (type = 0) {
 		/* Did we hit the erratum? */
 		if (++count = LOOP_TIMEOUT) {
-			pr_err("AMD-Vi: No event written to event log\n");
+			pr_err("No event written to event log\n");
 			return;
 		}
 		udelay(1);
@@ -572,43 +574,40 @@ static void iommu_print_event(struct amd_iommu *iommu, void *__evt)
 	if (type = EVENT_TYPE_IO_FAULT) {
 		amd_iommu_report_page_fault(devid, pasid, address, flags);
 		return;
-	} else {
-		dev_err(dev, "AMD-Vi: Event logged [");
 	}
 
 	switch (type) {
 	case EVENT_TYPE_ILL_DEV:
-		dev_err(dev, "ILLEGAL_DEV_TABLE_ENTRY device=%02x:%02x.%x pasid=0x%05x address=0x%016llx flags=0x%04x]\n",
+		dev_err(dev, "Event logged [ILLEGAL_DEV_TABLE_ENTRY device=%02x:%02x.%x pasid=0x%05x address=0x%016llx flags=0x%04x]\n",
 			PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
 			pasid, address, flags);
 		dump_dte_entry(devid);
 		break;
 	case EVENT_TYPE_DEV_TAB_ERR:
-		dev_err(dev, "DEV_TAB_HARDWARE_ERROR device=%02x:%02x.%x "
-			"address=0x%016llx flags=0x%04x]\n",
+		dev_err(dev, "Event logged [DEV_TAB_HARDWARE_ERROR device=%02x:%02x.%x address=0x%016llx flags=0x%04x]\n",
 			PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
 			address, flags);
 		break;
 	case EVENT_TYPE_PAGE_TAB_ERR:
-		dev_err(dev, "PAGE_TAB_HARDWARE_ERROR device=%02x:%02x.%x domain=0x%04x address=0x%016llx flags=0x%04x]\n",
+		dev_err(dev, "Event logged [PAGE_TAB_HARDWARE_ERROR device=%02x:%02x.%x domain=0x%04x address=0x%016llx flags=0x%04x]\n",
 			PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
 			pasid, address, flags);
 		break;
 	case EVENT_TYPE_ILL_CMD:
-		dev_err(dev, "ILLEGAL_COMMAND_ERROR address=0x%016llx]\n", address);
+		dev_err(dev, "Event logged [ILLEGAL_COMMAND_ERROR address=0x%016llx]\n", address);
 		dump_command(address);
 		break;
 	case EVENT_TYPE_CMD_HARD_ERR:
-		dev_err(dev, "COMMAND_HARDWARE_ERROR address=0x%016llx flags=0x%04x]\n",
+		dev_err(dev, "Event logged [COMMAND_HARDWARE_ERROR address=0x%016llx flags=0x%04x]\n",
 			address, flags);
 		break;
 	case EVENT_TYPE_IOTLB_INV_TO:
-		dev_err(dev, "IOTLB_INV_TIMEOUT device=%02x:%02x.%x address=0x%016llx]\n",
+		dev_err(dev, "Event logged [IOTLB_INV_TIMEOUT device=%02x:%02x.%x address=0x%016llx]\n",
 			PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
 			address);
 		break;
 	case EVENT_TYPE_INV_DEV_REQ:
-		dev_err(dev, "INVALID_DEVICE_REQUEST device=%02x:%02x.%x pasid=0x%05x address=0x%016llx flags=0x%04x]\n",
+		dev_err(dev, "Event logged [INVALID_DEVICE_REQUEST device=%02x:%02x.%x pasid=0x%05x address=0x%016llx flags=0x%04x]\n",
 			PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
 			pasid, address, flags);
 		break;
@@ -616,12 +615,12 @@ 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, "Event logged [INVALID_PPR_REQUEST device=%02x:%02x.%x pasid=0x%05x address=0x%016llx flags=0x%04x]\n",
 			PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
 			pasid, address, flags);
 		break;
 	default:
-		dev_err(dev, "UNKNOWN event[0]=0x%08x event[1]=0x%08x event[2]=0x%08x event[3]=0x%08x\n",
+		dev_err(dev, "Event logged [UNKNOWN event[0]=0x%08x event[1]=0x%08x event[2]=0x%08x event[3]=0x%08x\n",
 			event[0], event[1], event[2], event[3]);
 	}
 
@@ -648,7 +647,7 @@ static void iommu_handle_ppr_entry(struct amd_iommu *iommu, u64 *raw)
 	struct amd_iommu_fault fault;
 
 	if (PPR_REQ_TYPE(raw[0]) != PPR_REQ_FAULT) {
-		pr_err_ratelimited("AMD-Vi: Unknown PPR request received\n");
+		pr_err_ratelimited("Unknown PPR request received\n");
 		return;
 	}
 
@@ -753,12 +752,12 @@ static void iommu_poll_ga_log(struct amd_iommu *iommu)
 			if (!iommu_ga_log_notifier)
 				break;
 
-			pr_debug("AMD-Vi: %s: devid=%#x, ga_tag=%#x\n",
+			pr_debug("%s: devid=%#x, ga_tag=%#x\n",
 				 __func__, GA_DEVID(log_entry),
 				 GA_TAG(log_entry));
 
 			if (iommu_ga_log_notifier(GA_TAG(log_entry)) != 0)
-				pr_err("AMD-Vi: GA log notifier failed.\n");
+				pr_err("GA log notifier failed\n");
 			break;
 		default:
 			break;
@@ -783,18 +782,18 @@ irqreturn_t amd_iommu_int_thread(int irq, void *data)
 			iommu->mmio_base + MMIO_STATUS_OFFSET);
 
 		if (status & MMIO_STATUS_EVT_INT_MASK) {
-			pr_devel("AMD-Vi: Processing IOMMU Event Log\n");
+			pr_devel("Processing IOMMU Event Log\n");
 			iommu_poll_events(iommu);
 		}
 
 		if (status & MMIO_STATUS_PPR_INT_MASK) {
-			pr_devel("AMD-Vi: Processing IOMMU PPR Log\n");
+			pr_devel("Processing IOMMU PPR Log\n");
 			iommu_poll_ppr_log(iommu);
 		}
 
 #ifdef CONFIG_IRQ_REMAP
 		if (status & MMIO_STATUS_GALOG_INT_MASK) {
-			pr_devel("AMD-Vi: Processing IOMMU GA Log\n");
+			pr_devel("Processing IOMMU GA Log\n");
 			iommu_poll_ga_log(iommu);
 		}
 #endif
@@ -838,7 +837,7 @@ static int wait_on_sem(volatile u64 *sem)
 	}
 
 	if (i = LOOP_TIMEOUT) {
-		pr_alert("AMD-Vi: Completion-Wait loop timed out\n");
+		pr_alert("Completion-Wait loop timed out\n");
 		return -EIO;
 	}
 
@@ -1030,7 +1029,7 @@ static int __iommu_queue_command_sync(struct amd_iommu *iommu,
 		/* Skip udelay() the first time around */
 		if (count++) {
 			if (count = LOOP_TIMEOUT) {
-				pr_err("AMD-Vi: Command buffer timeout\n");
+				pr_err("Command buffer timeout\n");
 				return -EIO;
 			}
 
@@ -2794,9 +2793,9 @@ int __init amd_iommu_init_dma_ops(void)
 		dma_ops = &dma_direct_ops;
 
 	if (amd_iommu_unmap_flush)
-		pr_info("AMD-Vi: IO/TLB flush on unmap enabled\n");
+		pr_info("IO/TLB flush on unmap enabled\n");
 	else
-		pr_info("AMD-Vi: Lazy IO/TLB flushing enabled\n");
+		pr_info("Lazy IO/TLB flushing enabled\n");
 
 	return 0;
 
@@ -2901,7 +2900,7 @@ static struct iommu_domain *amd_iommu_domain_alloc(unsigned type)
 	case IOMMU_DOMAIN_DMA:
 		dma_domain = dma_ops_domain_alloc();
 		if (!dma_domain) {
-			pr_err("AMD-Vi: Failed to allocate\n");
+			pr_err("Failed to allocate\n");
 			return NULL;
 		}
 		pdomain = &dma_domain->domain;
@@ -3121,7 +3120,7 @@ static void amd_iommu_get_resv_regions(struct device *dev,
 						 IOMMU_RESV_DIRECT);
 		if (!region) {
 			pr_err("Out of memory allocating dm-regions for %s\n",
-				dev_name(dev));
+			       dev_name(dev));
 			return;
 		}
 		list_add_tail(&region->list, head);
@@ -4317,7 +4316,7 @@ static int amd_ir_set_vcpu_affinity(struct irq_data *data, void *vcpu_info)
 	 * legacy mode. So, we force legacy mode instead.
 	 */
 	if (!AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir)) {
-		pr_debug("AMD-Vi: %s: Fall back to using intr legacy remap\n",
+		pr_debug("%s: Fall back to using intr legacy remap\n",
 			 __func__);
 		pi_data->is_guest_mode = false;
 	}

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

[Joe Perches]

On Tue, 2018-07-03 at 18:57 +0200, Walter Harms wrote:
> It is only cosmetics but even the author got lost about the loose bracket.
> I would suggest to remove all the brackets or if needed to move the [ in the
> message. We have enought memory this days.

My suggestion would be to remove the separate dev_err
and use "Event log [" in each dev_err.

So here's an actual suggested patch that does a few things:

o Coalesce formats and realigns arguments
o Uses pr_fmt and dev_fmt to prefix "AMD-Vi: " to all messages
o Remove embedded "AMD-Vi: " from various formats

Some existing pr_<level> uses now are prefixed too.
---
 drivers/iommu/amd_iommu.c | 69 +++++++++++++++++++++++------------------------
 1 file changed, 34 insertions(+), 35 deletions(-)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 596b95c50051..01583a8ccaf9 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -17,6 +17,9 @@
  * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307 USA
  */
 
+#define pr_fmt(fmt) "AMD-Vi: " fmt
+#define dev_fmt pr_fmt
+
 #include <linux/ratelimit.h>
 #include <linux/pci.h>
 #include <linux/acpi.h>
@@ -273,9 +276,8 @@ static u16 get_alias(struct device *dev)
 		return pci_alias;
 	}
 
-	pr_info("AMD-Vi: Using IVRS reported alias %02x:%02x.%d "
-		"for device %s[%04x:%04x], kernel reported alias "
-		"%02x:%02x.%d\n", PCI_BUS_NUM(ivrs_alias), PCI_SLOT(ivrs_alias),
+	pr_info("Using IVRS reported alias %02x:%02x.%d for device %s[%04x:%04x], kernel reported alias %02x:%02x.%d\n",
+		PCI_BUS_NUM(ivrs_alias), PCI_SLOT(ivrs_alias),
 		PCI_FUNC(ivrs_alias), dev_name(dev), pdev->vendor, pdev->device,
 		PCI_BUS_NUM(pci_alias), PCI_SLOT(pci_alias),
 		PCI_FUNC(pci_alias));
@@ -287,7 +289,7 @@ static u16 get_alias(struct device *dev)
 	if (pci_alias == devid &&
 	    PCI_BUS_NUM(ivrs_alias) == pdev->bus->number) {
 		pci_add_dma_alias(pdev, ivrs_alias & 0xff);
-		pr_info("AMD-Vi: Added PCI DMA alias %02x.%d for %s\n",
+		pr_info("Added PCI DMA alias %02x.%d for %s\n",
 			PCI_SLOT(ivrs_alias), PCI_FUNC(ivrs_alias),
 			dev_name(dev));
 	}
@@ -507,8 +509,8 @@ static void dump_dte_entry(u16 devid)
 	int i;
 
 	for (i = 0; i < 4; ++i)
-		pr_err("AMD-Vi: DTE[%d]: %016llx\n", i,
-			amd_iommu_dev_table[devid].data[i]);
+		pr_err("DTE[%d]: %016llx\n",
+		       i, amd_iommu_dev_table[devid].data[i]);
 }
 
 static void dump_command(unsigned long phys_addr)
@@ -517,7 +519,7 @@ static void dump_command(unsigned long phys_addr)
 	int i;
 
 	for (i = 0; i < 4; ++i)
-		pr_err("AMD-Vi: CMD[%d]: %08x\n", i, cmd->data[i]);
+		pr_err("CMD[%d]: %08x\n", i, cmd->data[i]);
 }
 
 static void amd_iommu_report_page_fault(u16 devid, u16 domain_id,
@@ -532,10 +534,10 @@ static void amd_iommu_report_page_fault(u16 devid, u16 domain_id,
 		dev_data = get_dev_data(&pdev->dev);
 
 	if (dev_data && __ratelimit(&dev_data->rs)) {
-		dev_err(&pdev->dev, "AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x%04x address=0x%016llx flags=0x%04x]\n",
+		dev_err(&pdev->dev, "Event logged [IO_PAGE_FAULT domain=0x%04x address=0x%016llx flags=0x%04x]\n",
 			domain_id, address, flags);
 	} else if (printk_ratelimit()) {
-		pr_err("AMD-Vi: Event logged [IO_PAGE_FAULT device=%02x:%02x.%x domain=0x%04x address=0x%016llx flags=0x%04x]\n",
+		pr_err("Event logged [IO_PAGE_FAULT device=%02x:%02x.%x domain=0x%04x address=0x%016llx flags=0x%04x]\n",
 			PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
 			domain_id, address, flags);
 	}
@@ -562,7 +564,7 @@ static void iommu_print_event(struct amd_iommu *iommu, void *__evt)
 	if (type == 0) {
 		/* Did we hit the erratum? */
 		if (++count == LOOP_TIMEOUT) {
-			pr_err("AMD-Vi: No event written to event log\n");
+			pr_err("No event written to event log\n");
 			return;
 		}
 		udelay(1);
@@ -572,43 +574,40 @@ static void iommu_print_event(struct amd_iommu *iommu, void *__evt)
 	if (type == EVENT_TYPE_IO_FAULT) {
 		amd_iommu_report_page_fault(devid, pasid, address, flags);
 		return;
-	} else {
-		dev_err(dev, "AMD-Vi: Event logged [");
 	}
 
 	switch (type) {
 	case EVENT_TYPE_ILL_DEV:
-		dev_err(dev, "ILLEGAL_DEV_TABLE_ENTRY device=%02x:%02x.%x pasid=0x%05x address=0x%016llx flags=0x%04x]\n",
+		dev_err(dev, "Event logged [ILLEGAL_DEV_TABLE_ENTRY device=%02x:%02x.%x pasid=0x%05x address=0x%016llx flags=0x%04x]\n",
 			PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
 			pasid, address, flags);
 		dump_dte_entry(devid);
 		break;
 	case EVENT_TYPE_DEV_TAB_ERR:
-		dev_err(dev, "DEV_TAB_HARDWARE_ERROR device=%02x:%02x.%x "
-			"address=0x%016llx flags=0x%04x]\n",
+		dev_err(dev, "Event logged [DEV_TAB_HARDWARE_ERROR device=%02x:%02x.%x address=0x%016llx flags=0x%04x]\n",
 			PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
 			address, flags);
 		break;
 	case EVENT_TYPE_PAGE_TAB_ERR:
-		dev_err(dev, "PAGE_TAB_HARDWARE_ERROR device=%02x:%02x.%x domain=0x%04x address=0x%016llx flags=0x%04x]\n",
+		dev_err(dev, "Event logged [PAGE_TAB_HARDWARE_ERROR device=%02x:%02x.%x domain=0x%04x address=0x%016llx flags=0x%04x]\n",
 			PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
 			pasid, address, flags);
 		break;
 	case EVENT_TYPE_ILL_CMD:
-		dev_err(dev, "ILLEGAL_COMMAND_ERROR address=0x%016llx]\n", address);
+		dev_err(dev, "Event logged [ILLEGAL_COMMAND_ERROR address=0x%016llx]\n", address);
 		dump_command(address);
 		break;
 	case EVENT_TYPE_CMD_HARD_ERR:
-		dev_err(dev, "COMMAND_HARDWARE_ERROR address=0x%016llx flags=0x%04x]\n",
+		dev_err(dev, "Event logged [COMMAND_HARDWARE_ERROR address=0x%016llx flags=0x%04x]\n",
 			address, flags);
 		break;
 	case EVENT_TYPE_IOTLB_INV_TO:
-		dev_err(dev, "IOTLB_INV_TIMEOUT device=%02x:%02x.%x address=0x%016llx]\n",
+		dev_err(dev, "Event logged [IOTLB_INV_TIMEOUT device=%02x:%02x.%x address=0x%016llx]\n",
 			PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
 			address);
 		break;
 	case EVENT_TYPE_INV_DEV_REQ:
-		dev_err(dev, "INVALID_DEVICE_REQUEST device=%02x:%02x.%x pasid=0x%05x address=0x%016llx flags=0x%04x]\n",
+		dev_err(dev, "Event logged [INVALID_DEVICE_REQUEST device=%02x:%02x.%x pasid=0x%05x address=0x%016llx flags=0x%04x]\n",
 			PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
 			pasid, address, flags);
 		break;
@@ -616,12 +615,12 @@ 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, "Event logged [INVALID_PPR_REQUEST device=%02x:%02x.%x pasid=0x%05x address=0x%016llx flags=0x%04x]\n",
 			PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
 			pasid, address, flags);
 		break;
 	default:
-		dev_err(dev, "UNKNOWN event[0]=0x%08x event[1]=0x%08x event[2]=0x%08x event[3]=0x%08x\n",
+		dev_err(dev, "Event logged [UNKNOWN event[0]=0x%08x event[1]=0x%08x event[2]=0x%08x event[3]=0x%08x\n",
 			event[0], event[1], event[2], event[3]);
 	}
 
@@ -648,7 +647,7 @@ static void iommu_handle_ppr_entry(struct amd_iommu *iommu, u64 *raw)
 	struct amd_iommu_fault fault;
 
 	if (PPR_REQ_TYPE(raw[0]) != PPR_REQ_FAULT) {
-		pr_err_ratelimited("AMD-Vi: Unknown PPR request received\n");
+		pr_err_ratelimited("Unknown PPR request received\n");
 		return;
 	}
 
@@ -753,12 +752,12 @@ static void iommu_poll_ga_log(struct amd_iommu *iommu)
 			if (!iommu_ga_log_notifier)
 				break;
 
-			pr_debug("AMD-Vi: %s: devid=%#x, ga_tag=%#x\n",
+			pr_debug("%s: devid=%#x, ga_tag=%#x\n",
 				 __func__, GA_DEVID(log_entry),
 				 GA_TAG(log_entry));
 
 			if (iommu_ga_log_notifier(GA_TAG(log_entry)) != 0)
-				pr_err("AMD-Vi: GA log notifier failed.\n");
+				pr_err("GA log notifier failed\n");
 			break;
 		default:
 			break;
@@ -783,18 +782,18 @@ irqreturn_t amd_iommu_int_thread(int irq, void *data)
 			iommu->mmio_base + MMIO_STATUS_OFFSET);
 
 		if (status & MMIO_STATUS_EVT_INT_MASK) {
-			pr_devel("AMD-Vi: Processing IOMMU Event Log\n");
+			pr_devel("Processing IOMMU Event Log\n");
 			iommu_poll_events(iommu);
 		}
 
 		if (status & MMIO_STATUS_PPR_INT_MASK) {
-			pr_devel("AMD-Vi: Processing IOMMU PPR Log\n");
+			pr_devel("Processing IOMMU PPR Log\n");
 			iommu_poll_ppr_log(iommu);
 		}
 
 #ifdef CONFIG_IRQ_REMAP
 		if (status & MMIO_STATUS_GALOG_INT_MASK) {
-			pr_devel("AMD-Vi: Processing IOMMU GA Log\n");
+			pr_devel("Processing IOMMU GA Log\n");
 			iommu_poll_ga_log(iommu);
 		}
 #endif
@@ -838,7 +837,7 @@ static int wait_on_sem(volatile u64 *sem)
 	}
 
 	if (i == LOOP_TIMEOUT) {
-		pr_alert("AMD-Vi: Completion-Wait loop timed out\n");
+		pr_alert("Completion-Wait loop timed out\n");
 		return -EIO;
 	}
 
@@ -1030,7 +1029,7 @@ static int __iommu_queue_command_sync(struct amd_iommu *iommu,
 		/* Skip udelay() the first time around */
 		if (count++) {
 			if (count == LOOP_TIMEOUT) {
-				pr_err("AMD-Vi: Command buffer timeout\n");
+				pr_err("Command buffer timeout\n");
 				return -EIO;
 			}
 
@@ -2794,9 +2793,9 @@ int __init amd_iommu_init_dma_ops(void)
 		dma_ops = &dma_direct_ops;
 
 	if (amd_iommu_unmap_flush)
-		pr_info("AMD-Vi: IO/TLB flush on unmap enabled\n");
+		pr_info("IO/TLB flush on unmap enabled\n");
 	else
-		pr_info("AMD-Vi: Lazy IO/TLB flushing enabled\n");
+		pr_info("Lazy IO/TLB flushing enabled\n");
 
 	return 0;
 
@@ -2901,7 +2900,7 @@ static struct iommu_domain *amd_iommu_domain_alloc(unsigned type)
 	case IOMMU_DOMAIN_DMA:
 		dma_domain = dma_ops_domain_alloc();
 		if (!dma_domain) {
-			pr_err("AMD-Vi: Failed to allocate\n");
+			pr_err("Failed to allocate\n");
 			return NULL;
 		}
 		pdomain = &dma_domain->domain;
@@ -3121,7 +3120,7 @@ static void amd_iommu_get_resv_regions(struct device *dev,
 						 IOMMU_RESV_DIRECT);
 		if (!region) {
 			pr_err("Out of memory allocating dm-regions for %s\n",
-				dev_name(dev));
+			       dev_name(dev));
 			return;
 		}
 		list_add_tail(&region->list, head);
@@ -4317,7 +4316,7 @@ static int amd_ir_set_vcpu_affinity(struct irq_data *data, void *vcpu_info)
 	 * legacy mode. So, we force legacy mode instead.
 	 */
 	if (!AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir)) {
-		pr_debug("AMD-Vi: %s: Fall back to using intr legacy remap\n",
+		pr_debug("%s: Fall back to using intr legacy remap\n",
 			 __func__);
 		pi_data->is_guest_mode = false;
 	}

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

[Joerg Roedel]

On Tue, Jul 03, 2018 at 10:34:12AM -0700, Joe Perches wrote:
> On Tue, 2018-07-03 at 18:57 +0200, Walter Harms wrote:
> > It is only cosmetics but even the author got lost about the loose bracket.
> > I would suggest to remove all the brackets or if needed to move the [ in the
> > message. We have enought memory this days.
> 
> My suggestion would be to remove the separate dev_err
> and use "Event log [" in each dev_err.
> 
> So here's an actual suggested patch that does a few things:
> 
> o Coalesce formats and realigns arguments
> o Uses pr_fmt and dev_fmt to prefix "AMD-Vi: " to all messages
> o Remove embedded "AMD-Vi: " from various formats

Nice cleanup! But I got lost a bit in the patches floating around in
this thread and their dependencies. Can someone sort it out and send it
a new patch-set to me?


Thanks,

	Joerg

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

[Joerg Roedel]

On Tue, Jul 03, 2018 at 10:34:12AM -0700, Joe Perches wrote:
> On Tue, 2018-07-03 at 18:57 +0200, Walter Harms wrote:
> > It is only cosmetics but even the author got lost about the loose bracket.
> > I would suggest to remove all the brackets or if needed to move the [ in the
> > message. We have enought memory this days.
> 
> My suggestion would be to remove the separate dev_err
> and use "Event log [" in each dev_err.
> 
> So here's an actual suggested patch that does a few things:
> 
> o Coalesce formats and realigns arguments
> o Uses pr_fmt and dev_fmt to prefix "AMD-Vi: " to all messages
> o Remove embedded "AMD-Vi: " from various formats

Nice cleanup! But I got lost a bit in the patches floating around in
this thread and their dependencies. Can someone sort it out and send it
a new patch-set to me?


Thanks,

	Joerg

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

[Joerg Roedel]

On Tue, Jul 03, 2018 at 10:34:12AM -0700, Joe Perches wrote:
> On Tue, 2018-07-03 at 18:57 +0200, Walter Harms wrote:
> > It is only cosmetics but even the author got lost about the loose bracket.
> > I would suggest to remove all the brackets or if needed to move the [ in the
> > message. We have enought memory this days.
> 
> My suggestion would be to remove the separate dev_err
> and use "Event log [" in each dev_err.
> 
> So here's an actual suggested patch that does a few things:
> 
> o Coalesce formats and realigns arguments
> o Uses pr_fmt and dev_fmt to prefix "AMD-Vi: " to all messages
> o Remove embedded "AMD-Vi: " from various formats

Nice cleanup! But I got lost a bit in the patches floating around in
this thread and their dependencies. Can someone sort it out and send it
a new patch-set to me?


Thanks,

	Joerg