[PATCH 0/2] coresight: next v4.16-rc5

[PATCH 0/2] coresight: next v4.16-rc5

[Mathieu Poirier]

Hi Greg,

These two patches came in last week and are confined to Coresight.  Given
the late(ish) time in the cycle I would have normally waited for the
next one but the changes are small and trivial.

Please consider - if you think it's too late they'll simply go in later.

Thanks,
Mathieu 

Bo Yan (1):
  coresight: etm4x: Fix bit shifting

Leo Yan (1):
  coresight: Use %px to print pcsr instead of %p

 drivers/hwtracing/coresight/coresight-cpu-debug.c   | 2 +-
 drivers/hwtracing/coresight/coresight-etm4x-sysfs.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

-- 
2.7.4

[PATCH 1/2] coresight: Use %px to print pcsr instead of %p

[Mathieu Poirier]

From: Leo Yan <leo.yan@linaro.org>

Commit ad67b74d2469 ("printk: hash addresses printed with %p") lets
printk specifier %p to hash all addresses before printing, this was
resulting in the high 32 bits of pcsr can only output zeros.  So
module cannot completely print pc value and it's pointless for debugging
purpose.

This patch fixes this by using %px to print pcsr instead.

Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Signed-off-by: Leo Yan <leo.yan@linaro.org>
Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 drivers/hwtracing/coresight/coresight-cpu-debug.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hwtracing/coresight/coresight-cpu-debug.c b/drivers/hwtracing/coresight/coresight-cpu-debug.c
index 6ea62c62ff27..9cdb3fbc8c1f 100644
--- a/drivers/hwtracing/coresight/coresight-cpu-debug.c
+++ b/drivers/hwtracing/coresight/coresight-cpu-debug.c
@@ -315,7 +315,7 @@ static void debug_dump_regs(struct debug_drvdata *drvdata)
 	}
 
 	pc = debug_adjust_pc(drvdata);
-	dev_emerg(dev, " EDPCSR:  [<%p>] %pS\n", (void *)pc, (void *)pc);
+	dev_emerg(dev, " EDPCSR:  [<%px>] %pS\n", (void *)pc, (void *)pc);
 
 	if (drvdata->edcidsr_present)
 		dev_emerg(dev, " EDCIDSR: %08x\n", drvdata->edcidsr);
-- 
2.7.4

[PATCH 2/2] coresight: etm4x: Fix bit shifting

[Mathieu Poirier]

From: Bo Yan <byan@nvidia.com>

ctxid_pid and vmid_val in config are of type u64. When an integer
0xFF is being left shifted more than 32 bits, the behavior is
undefined. The fix is to specify 0xFF as an unsigned long.

Detected by Coverity scan: CID 37650, 37651 (Bad bit shift operation)

Signed-off-by: Bo Yan <byan@nvidia.com>
Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 drivers/hwtracing/coresight/coresight-etm4x-sysfs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
index 4e6eab53e34e..d21961710713 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
@@ -1780,7 +1780,7 @@ static ssize_t ctxid_masks_store(struct device *dev,
 		 */
 		for (j = 0; j < 8; j++) {
 			if (maskbyte & 1)
-				config->ctxid_pid[i] &= ~(0xFF << (j * 8));
+				config->ctxid_pid[i] &= ~(0xFFUL << (j * 8));
 			maskbyte >>= 1;
 		}
 		/* Select the next ctxid comparator mask value */
@@ -1963,7 +1963,7 @@ static ssize_t vmid_masks_store(struct device *dev,
 		 */
 		for (j = 0; j < 8; j++) {
 			if (maskbyte & 1)
-				config->vmid_val[i] &= ~(0xFF << (j * 8));
+				config->vmid_val[i] &= ~(0xFFUL << (j * 8));
 			maskbyte >>= 1;
 		}
 		/* Select the next vmid comparator mask value */
-- 
2.7.4

[PATCH 0/2] coresight: next v4.16-rc5

[Greg KH]

On Tue, Mar 13, 2018 at 11:24:29AM -0600, Mathieu Poirier wrote:
> Hi Greg,
> 
> These two patches came in last week and are confined to Coresight.  Given
> the late(ish) time in the cycle I would have normally waited for the
> next one but the changes are small and trivial.
> 
> Please consider - if you think it's too late they'll simply go in later.

They aren't bugfixes, I'll queue them up now for 4.17-rc1.

thanks,

greg k-h

[PATCH 0/2] coresight: next v4.16-rc5

[Mathieu Poirier]

On 14 March 2018 at 10:59, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Tue, Mar 13, 2018 at 11:24:29AM -0600, Mathieu Poirier wrote:
>> Hi Greg,
>>
>> These two patches came in last week and are confined to Coresight.  Given
>> the late(ish) time in the cycle I would have normally waited for the
>> next one but the changes are small and trivial.
>>
>> Please consider - if you think it's too late they'll simply go in later.
>
> They aren't bugfixes, I'll queue them up now for 4.17-rc1.

Perfect - that's what I was hoping.  When reading my initial email I
realise it wasn't exactly clear.

Thanks,
Mathieu

>
> thanks,
>
> greg k-h

[PATCH 1/2] coresight: Use %px to print pcsr instead of %p

[Kees Cook]

On Tue, Mar 13, 2018 at 10:24 AM, Mathieu Poirier
<mathieu.poirier@linaro.org> wrote:
> From: Leo Yan <leo.yan@linaro.org>
>
> Commit ad67b74d2469 ("printk: hash addresses printed with %p") lets
> printk specifier %p to hash all addresses before printing, this was
> resulting in the high 32 bits of pcsr can only output zeros.  So
> module cannot completely print pc value and it's pointless for debugging
> purpose.
>
> This patch fixes this by using %px to print pcsr instead.

Why is %p (and now %px) needed here at all? %pS is already used, which
should give useful debugging details, yes?

-Kees

-- 
Kees Cook
Pixel Security

[PATCH 1/2] coresight: Use %px to print pcsr instead of %p

[Mathieu Poirier]

On 5 April 2018 at 00:26, Kees Cook <keescook@chromium.org> wrote:
> On Tue, Mar 13, 2018 at 10:24 AM, Mathieu Poirier
> <mathieu.poirier@linaro.org> wrote:
>> From: Leo Yan <leo.yan@linaro.org>
>>
>> Commit ad67b74d2469 ("printk: hash addresses printed with %p") lets
>> printk specifier %p to hash all addresses before printing, this was
>> resulting in the high 32 bits of pcsr can only output zeros.  So
>> module cannot completely print pc value and it's pointless for debugging
>> purpose.
>>
>> This patch fixes this by using %px to print pcsr instead.
>
> Why is %p (and now %px) needed here at all? %pS is already used, which
> should give useful debugging details, yes?

Hey Leo,

I've taken a second look at this and Kees has a point, %pS should be
sufficient here.  Please test again and see if %px is absolutely
necessary.  If you think that is the case please provide a snapshot of
the corner case that makes the change mandatory.

>
> -Kees
>
> --
> Kees Cook
> Pixel Security

[PATCH 1/2] coresight: Use %px to print pcsr instead of %p

[Leo Yan]

Hi Mathieu, Kees,

On Fri, Apr 06, 2018 at 09:47:31AM -0600, Mathieu Poirier wrote:
> On 5 April 2018 at 00:26, Kees Cook <keescook@chromium.org> wrote:
> > On Tue, Mar 13, 2018 at 10:24 AM, Mathieu Poirier
> > <mathieu.poirier@linaro.org> wrote:
> >> From: Leo Yan <leo.yan@linaro.org>
> >>
> >> Commit ad67b74d2469 ("printk: hash addresses printed with %p") lets
> >> printk specifier %p to hash all addresses before printing, this was
> >> resulting in the high 32 bits of pcsr can only output zeros.  So
> >> module cannot completely print pc value and it's pointless for debugging
> >> purpose.
> >>
> >> This patch fixes this by using %px to print pcsr instead.
> >
> > Why is %p (and now %px) needed here at all? %pS is already used, which
> > should give useful debugging details, yes?
> 
> Hey Leo,
> 
> I've taken a second look at this and Kees has a point, %pS should be
> sufficient here.  Please test again and see if %px is absolutely
> necessary.  If you think that is the case please provide a snapshot of
> the corner case that makes the change mandatory.

Sorry I missed replying for this patch due to my email filter and
thanks for suggestion.

I can think out one case which should use %px: if CPU is locked up in
firmwares (e.g. NS-EL2 or ARM-TF in S-EL3), we can rely on %px to read
back PC hex value; for this case the CPU PC address is out of kernel
space.

How about you guys think for this?

Thanks,
Leo Yan

[PATCH 1/2] coresight: Use %px to print pcsr instead of %p

[Kees Cook]

On Mon, Apr 9, 2018 at 9:10 AM, Leo Yan <leo.yan@linaro.org> wrote:
> Hi Mathieu, Kees,
>
> On Fri, Apr 06, 2018 at 09:47:31AM -0600, Mathieu Poirier wrote:
>> On 5 April 2018 at 00:26, Kees Cook <keescook@chromium.org> wrote:
>> > On Tue, Mar 13, 2018 at 10:24 AM, Mathieu Poirier
>> > <mathieu.poirier@linaro.org> wrote:
>> >> From: Leo Yan <leo.yan@linaro.org>
>> >>
>> >> Commit ad67b74d2469 ("printk: hash addresses printed with %p") lets
>> >> printk specifier %p to hash all addresses before printing, this was
>> >> resulting in the high 32 bits of pcsr can only output zeros.  So
>> >> module cannot completely print pc value and it's pointless for debugging
>> >> purpose.
>> >>
>> >> This patch fixes this by using %px to print pcsr instead.
>> >
>> > Why is %p (and now %px) needed here at all? %pS is already used, which
>> > should give useful debugging details, yes?
>>
>> Hey Leo,
>>
>> I've taken a second look at this and Kees has a point, %pS should be
>> sufficient here.  Please test again and see if %px is absolutely
>> necessary.  If you think that is the case please provide a snapshot of
>> the corner case that makes the change mandatory.
>
> Sorry I missed replying for this patch due to my email filter and
> thanks for suggestion.
>
> I can think out one case which should use %px: if CPU is locked up in
> firmwares (e.g. NS-EL2 or ARM-TF in S-EL3), we can rely on %px to read
> back PC hex value; for this case the CPU PC address is out of kernel
> space.
>
> How about you guys think for this?

Wouldn't %pS still give you a meaningful offset?

-Kees

-- 
Kees Cook
Pixel Security

[PATCH 1/2] coresight: Use %px to print pcsr instead of %p

[Leo Yan]

Hi Kees,

On Mon, Apr 09, 2018 at 10:54:58AM -0700, Kees Cook wrote:
> On Mon, Apr 9, 2018 at 9:10 AM, Leo Yan <leo.yan@linaro.org> wrote:
> > Hi Mathieu, Kees,
> >
> > On Fri, Apr 06, 2018 at 09:47:31AM -0600, Mathieu Poirier wrote:
> >> On 5 April 2018 at 00:26, Kees Cook <keescook@chromium.org> wrote:
> >> > On Tue, Mar 13, 2018 at 10:24 AM, Mathieu Poirier
> >> > <mathieu.poirier@linaro.org> wrote:
> >> >> From: Leo Yan <leo.yan@linaro.org>
> >> >>
> >> >> Commit ad67b74d2469 ("printk: hash addresses printed with %p") lets
> >> >> printk specifier %p to hash all addresses before printing, this was
> >> >> resulting in the high 32 bits of pcsr can only output zeros.  So
> >> >> module cannot completely print pc value and it's pointless for debugging
> >> >> purpose.
> >> >>
> >> >> This patch fixes this by using %px to print pcsr instead.
> >> >
> >> > Why is %p (and now %px) needed here at all? %pS is already used, which
> >> > should give useful debugging details, yes?
> >>
> >> Hey Leo,
> >>
> >> I've taken a second look at this and Kees has a point, %pS should be
> >> sufficient here.  Please test again and see if %px is absolutely
> >> necessary.  If you think that is the case please provide a snapshot of
> >> the corner case that makes the change mandatory.
> >
> > Sorry I missed replying for this patch due to my email filter and
> > thanks for suggestion.
> >
> > I can think out one case which should use %px: if CPU is locked up in
> > firmwares (e.g. NS-EL2 or ARM-TF in S-EL3), we can rely on %px to read
> > back PC hex value; for this case the CPU PC address is out of kernel
> > space.
> >
> > How about you guys think for this?
> 
> Wouldn't %pS still give you a meaningful offset?

My fault, I tested at my side %pS does output offset for PC value
out of kernel space (e.g. when PC = 0x12345678).

Thank you for the suggestions ;)  Will send new patch for the fixing.

Thanks,
Leo Yan