Re: [PATCH v2 3/3] ALSA: hda: Disabled unused audio controller for Dell platforms with Switchable Graphics

[Pali RohÃr]

On Sunday 11 March 2018 14:03:13 Mario.Limonciello@xxxxxxxx wrote:
> > -----Original Message-----
> > From: platform-driver-x86-owner@xxxxxxxxxxxxxxx [mailto:platform-driver-x86-
> > owner@xxxxxxxxxxxxxxx] On Behalf Of Pali RohÃr
> > Sent: Saturday, March 10, 2018 6:38 PM
> > To: Limonciello, Mario <Mario_Limonciello@xxxxxxxx>
> > Cc: kai.heng.feng@xxxxxxxxxxxxx; mjg59@xxxxxxxxxxxxx; dvhart@xxxxxxxxxxxxx;
> > andy@xxxxxxxxxxxxx; tiwai@xxxxxxxx; platform-driver-x86@xxxxxxxxxxxxxxx; linux-
> > kernel@xxxxxxxxxxxxxxx; alsa-devel@xxxxxxxxxxxxxxxx
> > Subject: Re: [PATCH v2 3/3] ALSA: hda: Disabled unused audio controller for Dell
> > platforms with Switchable Graphics
> > 
> > On Friday 09 March 2018 09:59:39 Mario.Limonciello@xxxxxxxx wrote:
> > > > > Pali is your concern that this code for matching vendor/subsystem is running
> > > > > on non-Dell too?  The only other recommendation I think that can be to restrict
> > > > > to matching Dell OEM strings in SMBIOS table, but I don't think that's any better
> > > > > than the matching for VID/SSVID.
> > > >
> > > > My concern is about adding a new machine specific code into generic
> > > > driver, which check is done just by PCI vendor and subvendor.
> > > >
> > > > In future there can be new models or other PCI devices which matches
> > > > above condition even they would not have any switchable graphics, nor
> > > > they would manufactured by Dell.
> > >
> > > Uh Dell subsystem ID means it's Dell no?
> > 
> > What would prevent you to take PCI device marked with Dell ID and put it
> > into non-Dell computer? I do not believe that Dell PCI devices are
> > configured to work only in Dell branded devices and refuse to power up
> > in others.
> 
> I think the missing aspect is that this is only used in AIO and laptop form
> factors where the discrete graphics is in a non-removable form factor.

Why we are not checking if kernel is running on AIO or laptop form
factor then? Or it is not possible?

Basically what I see there is that we need to detect if current HW
platform has switchable graphics and check how is configured AUDIO MUX.

But instead of directly checking hw state of audio MUX, we are trying to
check something different which could get us information of state of the
audio mux.

I suspect that we do not have a way how to check audio MUX directly, so
it needs to be done indirectly -- via some Dell SMBIOS call and some
other heuristic. This is something which should be specified either in
comment or in commit message (problem of type: we need X, but check for
Y).

And if we are doing this check indirectly, we should do the most
specific test and not more general.

I think that PCI vendor ID check of audio device is more general test
then checking if kernel is running on Dell laptop (check via DMI). And
if we can check also if running on AIO or laptop form, then it would be
more specific test.

> Running with your hypothetical though, what would happen is if it's
> on a non-Dell machine the PCI check would pass and then the code
> would not be executed by dell-laptop (since dell-smbios didn't load).

Right.

> If it was on a Dell machine it would load but the BIOS would return
> either Switchable graphics turned off, or invalid token.
> 
> Even though these aren't real for switchable graphics on Dell I don't
> see a problem with either of these situations if it ever came up.

I see, this solution is working...

... but, I see there a very bad precedense. What would happen if another
laptop manufactor comes with similar solution for hybrid graphics. Does
it mean that hda audio driver would try to call for every one vendor its
vendor dependent API function (EFI, SMM, WMI, whatever) to check if
current HW has some switchable graphics and needs special checks?

Those vendor dependent API functions (which Dell SMBIOS is) should be
really called on vendor hardware.

Otherwise audio drivers would load bunch of the other vendor dependent
platform modules and all of those modules (except maximally one) just
return error.

> > 
> > If there is Dell ID then it just means that PCI device itself is Dell's.
> > And not that machine in which that device is plugged is also Dell.
> > 
> > > > Also I can imagine that in future (or maybe already now?) it is possible
> > > > to find PCI device which pass above checks and connect this PCI device
> > > > into desktop /server / any non-laptop device.
> > > >
> > > > If this switchable graphics solution is specific to dell laptops, then
> > > > rather checking for PCI vendor/subvevendor main check, there should be
> > > > main check via DMI strings.
> > >
> > > Right now this is affected to both AIO desktop and laptops.
> > >
> > > IIRC you won't end up with switchable graphics in traditional desktop that you
> > > can remove PCI card.  If this code was run on a traditional desktop with a
> > > AMD PCI card that BIOS query result should be invalid token (which will infer
> > > switchable off to this routine).
> > >
> > > >
> > > > Hardware is changing relatively quickly and there is absolutely no
> > > > guarantee that e.g. NVIDIA would not start providing audio controller in
> > > > similar like AMD and it would be put in those Dell machines.
> > >
> > > Kai Heng can explain exactly why NVIDIA isn't affected.
> > > This is probably good information to include in the commit message too.
> > 
> > Yes, extending commit message is a good idea.
> > 
> > But here I'm talking about future, NVIDIA cards could be in future.
> > 
> > I still think that whitelisting devices based on vendor ID by some
> > measurements at one time is a bad idea. It is fragile which can stop
> > working in the future.
> 
> Compiling a whitelist is a wasted effort because it will have to change
> Every year for every new platform that has AMD switchable graphics.

I agree, But see that this patch already uses vendor ID whitelisting.

> This heuristic that is selected covers switchable graphics back for the
> past generations that Canonical has tested and fixed, not just this
> current one.
> 
> If that situation you refer to happens, it will be on new hardware that's
> not yet enabled by the Linux kernel.  We can cross that bridge when we
> come to it with either a newly proposed heuristic or some adjustments
> for whitelist/blacklist.

-- 
Pali RohÃr
pali.rohar@xxxxxxxxx

Attachment: signature.asc
Description: PGP signature