Re: [PATCH V7 5/5] platform/x86: Intel PMT Crashlog capability driver

[Alexander Duyck]

On Thu, Oct 1, 2020 at 11:47 AM Andy Shevchenko
<andy.shevchenko@xxxxxxxxx> wrote:
>
> On Thu, Oct 1, 2020 at 9:33 PM Alexander Duyck
> <alexander.duyck@xxxxxxxxx> wrote:
> > On Thu, Oct 1, 2020 at 9:37 AM Andy Shevchenko
> > <andy.shevchenko@xxxxxxxxx> wrote:
> > > On Thu, Oct 1, 2020 at 4:43 AM David E. Box <david.e.box@xxxxxxxxxxxxxxx> wrote:
>
> ...
>
> > Arguably not much. I'll drop the comment.
> >
> > > > +       control &= ~(CRASHLOG_FLAG_MASK | CRASHLOG_FLAG_DISABLE);
> > >
> > > How does the second constant play any role here?
> >
> > The "control" flags are bits 28-31, while the disable flag is bit 27
> > if I recall.
>
> Okay, then it adds more confusion to the same comment here and there.
> Good you are about to drop the comment.
>
> > Specifically bit 31 is read only, bit 28 will clear bit 31, bit 29
> > will cause the crashlog to be generated and set bit 31, and bit 30 is
> > just reserved 0.
>
> Can this be added as a comment somewhere in the code?

I'll do that with the definitions themselves.

> ...
>
> > > > +       ret = intel_pmt_dev_create(entry, &pmt_crashlog_ns, parent);
> > > > +       if (!ret)
> > > > +               return 0;
>
> (2)
>
> > > > +
> > > > +       dev_err(parent, "Failed to add crashlog controls\n");
> > > > +       intel_pmt_dev_destroy(entry, &pmt_crashlog_ns);
> > > > +
> > > > +       return ret;
> > >
> > > Can we use traditional patterns?
> > > if (ret) {
> > >   ...
> > > }
> > > return ret;
> >
> > I can switch it if that is preferred.
>
> Yes, please. The (2) is really hard to parse (easy to miss ! part and
> be confused by return 0 one).
>
> ...
>
> > > Are you going to duplicate this in each driver? Consider to refactor
> > > to avoid duplication of a lot of code.
> >
> > So the issue lies in the complexity of pmt_telem_add_entry versus
> > pmt_crashlog_add_entry. Specifically I end up needing disc_res and the
> > discovery table when I go to create the controls for the crashlog
> > device. Similarly we have a third device that we plan to add called a
> > watcher which will require us to keep things split up like this so we
> > thought it best to split it up this way.
>
> Could you revisit and think how this can be deduplicated. I see at
> least one variant with a hooks (callbacks) which you supply depending
> on the driver, but the for-loop is kept in one place.

I'll see what I can do.

> ...
>
> > > > +               .name   = DRV_NAME,
> > >
> > > > +MODULE_ALIAS("platform:" DRV_NAME);
> > >
> > > I'm not sure I have interpreted this:
> > >         - Use 'raw' string instead of defines for device names
> > > correctly. Can you elaborate?
> >
> > Again I am not sure what this is in reference to. If you can point me
> > to some documentation somewhere I can take a look.
>
> Reference to your own changelog of this series!

So the issue is we have two authors so it is a matter of keeping track
of who is working on what.

So apparently that was in reference to the MFD driver which was
instantiating the devices using defines and there was only one spot
where they were being used. The reason why I was confused is because
the commit message had nothing to do with this patch and it I haven't
really done any work on the MFD driver myself. The link to the 'raw'
discussion can be found here:
https://lore.kernel.org/lkml/20200728075859.GH1850026@dell/