Re: [PATCH v2 3/4] platform/x86: intel_pmc_core: Decode Snoop / Non Snoop LTR

[Bhardwaj, Rajneesh]

Hi Andy,

Thanks for your review. My comments below.

If you agree then i can quickly send v3 addressing all suggestions so we 
can make it in time for 4.20 merge window.


On 19-Oct-18 6:04 PM, Andy Shevchenko wrote:
> On Sat, Oct 6, 2018 at 9:54 AM Rajneesh Bhardwaj
> <rajneesh.bhardwaj@xxxxxxxxxxxxxxx> wrote:
> > The LTR values follow PCIE LTR encoding format and can be decoded as per
> > https://pcisig.com/sites/default/files/specification_documents/ECN_LatencyTolnReporting_14Aug08.pdf
> >
> > This adds support to translate the raw LTR values as read from the PMC
> > to meaningful values in nanosecond units of time.
> While I have pushed this to my review and testing queue, it needs a
> bit more work. See my comments below.
>
> > +static u32 convert_ltr_scale(u32 val)
> > +{
> > +       u32 scale = 0;
> Redundant, see below.
>
> > +       /*
> > +        * As per PCIE specification supporting document
> > +        * ECN_LatencyTolnReporting_14Aug08.pdf the Latency
> > +        * Tolerance Reporting data payload is encoded in a
> > +        * 3 bit scale and 10 bit value fields. Values are
> > +        * multiplied by the indicated scale to yield an absolute time
> > +        * value, expressible in a range from 1 nanosecond to
> > +        * 2^25*(2^10-1) = 34,326,183,936 nanoseconds.
> > +        *
> > +        * scale encoding is as follows:
> > +        *
> > +        * ----------------------------------------------
> > +        * |scale factor        |       Multiplier (ns) |
> > +        * ----------------------------------------------
> > +        * |    0               |       1               |
> > +        * |    1               |       32              |
> > +        * |    2               |       1024            |
> > +        * |    3               |       32768           |
> > +        * |    4               |       1048576         |
> > +        * |    5               |       33554432        |
> > +        * |    6               |       Invalid         |
> > +        * |    7               |       Invalid         |
> > +        * ----------------------------------------------
> > +        */
> > +       if (val > 5)
> > +               pr_warn("Invalid LTR scale factor.\n");
> if (...) {
>   pr_warn(...); // Btw, Does it recoverable state? What user will get
> with returned 0 as a multiplier?
>   return 0; // Btw, is 0 fits better than ~0? How hw would behave with
> this value?
> }

We show 0 LTR for invalid scale as PMC output is junk sometimes.


> > +       else
> > +               scale = 1U << (5 * (val));
> > +
> > +       return scale;
> return 1U << (5 * val);

We intend to return 0 so for invalid LTR scale. This will make retuen 
non zero and we dont want that.

> > +}
> >          for (index = 0; map[index].name ; index++) {
> > -               seq_printf(s, "IP %-2d :%-32s\tRAW LTR: 0x%x\n", index,
> > -                          map[index].name,
> > -                          pmc_core_reg_read(pmcdev, map[index].bit_mask));
> We use 32 characters for the names. Here are two minor issues:
> - inconsistency with the rest

intentional.

> - ping-pong style of programming (you changed 32 to 24 in the same
> series where you introduced 32 in the first place).

This is because the formatted output looks better with this width. I 
used 32 for the earlier patch because its consistent with rest and also 
does not look bad on screen.

> > +               decoded_snoop_ltr = decoded_non_snoop_ltr = 0;
> > +               ltr_raw_data = pmc_core_reg_read(pmcdev,
> > +                                                map[index].bit_mask);
> > +               snoop_ltr = ltr_raw_data & ~MTPMC_MASK;
> > +               nonsnoop_ltr = (ltr_raw_data >> 0x10) & ~MTPMC_MASK;
> > +
> > +               if (FIELD_GET(LTR_REQ_NONSNOOP, ltr_raw_data)) {
> > +                       scale = FIELD_GET(LTR_DECODED_SCALE, nonsnoop_ltr);
> > +                       val = FIELD_GET(LTR_DECODED_VAL, nonsnoop_ltr);
> > +                       decoded_non_snoop_ltr = val * convert_ltr_scale(scale);
> > +               }
> > +
> > +               if (FIELD_GET(LTR_REQ_SNOOP, ltr_raw_data)) {
> > +                       scale = FIELD_GET(LTR_DECODED_SCALE, snoop_ltr);
> > +                       val = FIELD_GET(LTR_DECODED_VAL, snoop_ltr);
> > +                       decoded_snoop_ltr = val * convert_ltr_scale(scale);
> > +               }
> > +
> > +               seq_printf(s, "IP %-2d :%-24s\tRaw LTR: 0x%-16x\t Non-Snoop LTR (ns): %-16llu\t Snoop LTR (ns): %-16llu\n",
> Here 0x%-16x would look a bit strange and difficult to parse. 0x%016x
> much better.

Sure, I can test how it looks with 0x%016x and modify it.

> After you remove the index, it would give you 4 more characters,
> though it 4 less than 8 you got from reducing 32 to 24.

I plan to keep the index as is. Reason for this is mentioned in previous 
patch that introduces this index.

> OTOH, those long texts perhaps may be compressed somehow, at least
> remove LTR duplicating from the last two. Remove spaces after '\t' as
> well.

Noted.

> > +                          index, map[index].name, ltr_raw_data,
> > +                          decoded_non_snoop_ltr,
> > +                          decoded_snoop_ltr);
> >          }
> >          return 0;
> >   }
> > --- a/drivers/platform/x86/intel_pmc_core.h
> > +++ b/drivers/platform/x86/intel_pmc_core.h
> > @@ -177,6 +177,11 @@ enum ppfear_regs {
> It might be good idea to include linux/bits.h here.

Certainly. Luckily 0day bot didnt complain about randconfigs etc so is 
it really needed as it will increase size?

> > +#define LTR_REQ_NONSNOOP                       BIT(31)
> > +#define LTR_REQ_SNOOP                          BIT(15)
> > +#define LTR_DECODED_VAL                                GENMASK(9, 0)
> > +#define LTR_DECODED_SCALE                      GENMASK(12, 10)