Message-ID: <1dfb07bd3461414e8db492e643d7de88@ausx13mpc120.AMER.DELL.COM>
Date:   Mon, 9 Jul 2018 04:38:16 +0000
From:   <Mario.Limonciello@...l.com>
To:     <srinivas.pandruvada@...ux.intel.com>, <dvhart@...radead.org>,
        <andy.shevchenko@...il.com>
CC:     <alex.hung@...onical.com>, <andy@...radead.org>,
        <platform-driver-x86@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <rjw@...ysocki.net>
Subject: RE: [PATCH v2] platform/x86: intel-hid: Add support for Device
 Specific Methods

> -----Original Message-----
> From: Srinivas Pandruvada [mailto:srinivas.pandruvada@...ux.intel.com]
> Sent: Saturday, July 7, 2018 8:44 AM
> To: Darren Hart; Andy Shevchenko
> Cc: Limonciello, Mario; Alex Hung; Andy Shevchenko; Platform Driver; Linux Kernel
> Mailing List; Rafael J. Wysocki
> Subject: Re: [PATCH v2] platform/x86: intel-hid: Add support for Device Specific
> Methods
> 
> On Fri, 2018-07-06 at 16:59 -0700, Darren Hart wrote:
> > On Mon, Jul 02, 2018 at 05:06:14PM +0300, Andy Shevchenko wrote:
> > > On Mon, Jul 2, 2018 at 4:51 PM,  <Mario.Limonciello@...l.com>
> > > wrote:
> > >
> > > > > > So there are some customers who will have issue with power
> > > > > > button
> > > > > > without this patch, so it should be also marked for stable
> > > > > > also.
> > > > > > Also this may be a candidate for 4.18-rcX.
> > > > > >
> > > > >
> > > > > I'm not sure Greg will take this selling point for rather big
> > > > > patch.
> > > > > From changelog, honestly, I don't see any regression
> > > > > description,
> > > > > looks like "it wasn't working before change anyway".
> > > > >
> > > >
> > > > Just for adding some context.
> > > >
> > > > Some platforms have moved to different interface in ASL in FW
> > > > upgrade
> > > > due to deficiencies/bugs present with old interface.  So yes it's
> > > > platform FW
> > > > change in behavior that leads to Linux kernel
> > > > regression.  Windows driver has supported
> > > > both interfaces for a long time.  Linux kernel however doesn't
> > > > support this interface until now.
> > > >
> > > > > For now, I pushed this to my review and testing queue as is,
> > > > > thanks!
> > > >
> > > > If not stable I think it would at least be ideal to try to bring
> > > > this to 4.18-rcX if possible for
> > > > compatibility with more platforms that will come with this other
> > > > interface instead.
> > >
> > > Citing Linus:
> > >
> > > --- 8< --- 8< ---
> > > So please, people, the "fixes" during the rc series really should
> > > be
> > > things that are _regressions_. If it used to work, and it no longer
> > > does,
> > > then fixing that is a good and proper fix. Or if something oopses
> > > or has a
> > > security implication, then the fix for that is a real fix.
> > >
> > > But if it's something that has never worked, even if it "fixes"
> > > some
> > > behavior, then it's new development, and that should come in during
> > > the
> > > merge window. Just because you think it's a "fix" doesn't mean that
> > > it
> > > really is one, at least in the "during the rc series" sense.
> > > --- 8< --- 8< ---
> > >
> > > So, if we can sell him that it used to work and firmware fix is a
> > > Linux regression I'm fine.
> > >
> > > Darren, what do you think?
> >
> > So if I understand this correctly, we have this timeline.
> >
> > Linux v4.16
> > - Machine A works
> > Linux v4.17
> > - Machine A works
> > - Machine A updates firmware
> > - Machine A stops working
> > Linux v4.18
> > - Machine A still doesn't work
> >
> > So it is not a *Linux kernel* regression.
> >
> > The patch is too large for standard stable rules.
> >
> > It is a regression from any user's perspective - the machine worked,
> > they followed good digital hygiene and updated their firmware, and
> > now
> > it doesn't. This user will now think twice before they update their
> > BIOS
> > again, since it may fundamentally change the platform, rather than
> > committing to only fixing things below the OS Interface layer. :-(
> >
> > The risk of course is that this introduces new bugs - and as with
> > anything that still uses _DSM (sigh, why?) that is quite possible due
> > to
> > the opaque interface. Very unfortunate to see _DSM ADDED to a
> > previously
> > _DSM free implementation. Linus is right, this is not a fix, this is
> > feature development.
> >
> > I strongly advocate for vendors to have more control over their
> > drivers,
> > but this scenario really frustrates me. I don't think I can justify
> > this
> > to Linus as a fix. But before we just say "no" (because hey, I want
> > these fixes available as early as possible too), let's ask Rafael if
> > he
> > has an opinion or if there is precedent for this in his experience
> > with
> > ACPI drivers in general:
> >
> > Rafael?
> >
> > Finally, we can also just ask Linus. The firmware update broke the
> > power
> > button, we can get it working again by supporting the new API with
> > this
> > patch... see what he says.

> Mario can add more.
> But I think Dell has released a BIOS fix, so that power button can
> still work using non _DSM way. So I think we can wait for normal
> release cycle.

Yeah, I added some comments on a separate reply already indicating this.

The affected system we know about has reverted the new interface.

This all stems from an expectation that the _DSM has been there for "many"
releases on the Windows side.  Long enough that even all the corporate downgrade
scenarios to older Windows versions and the drivers with them all worked properly.

If this doesn't end up being a candidate for backporting to stable we'll probably
end up asking our various OS partners to backport it as a SAUCE type patch in distro
kernels to eventually be able to turn this back on.

Powered by blists - more mailing lists