diff mbox series

i386:acpi: Remove _HID from the SMBus ACPI entry

Message ID 20200106152705.8258-1-minyard@acm.org (mailing list archive)
State New, archived
Headers show
Series i386:acpi: Remove _HID from the SMBus ACPI entry | expand

Commit Message

Corey Minyard Jan. 6, 2020, 3:27 p.m. UTC 
From: Corey Minyard <cminyard@mvista.com>

Per the ACPI spec (version 6.1, section 6.1.5 _HID) it is not required
on enumerated buses (like PCI in this case), _ADR is required (and is
already there).  And the _HID value is wrong.  Linux appears to ignore
the _HID entry, but it confuses Windows.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>
---
 hw/i386/acpi-build.c | 1 -
 1 file changed, 1 deletion(-)

Comments

Igor Mammedov Jan. 7, 2020, 3:52 p.m. UTC | #1 
On Mon,  6 Jan 2020 09:27:05 -0600
minyard@acm.org wrote:

> From: Corey Minyard <cminyard@mvista.com>
> 
> Per the ACPI spec (version 6.1, section 6.1.5 _HID) it is not required
> on enumerated buses (like PCI in this case), _ADR is required (and is
> already there).  And the _HID value is wrong.  Linux appears to ignore
> the _HID entry, but it confuses Windows.
> 
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Igor Mammedov <imammedo@redhat.com>

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/i386/acpi-build.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 7b8da62d41..ab73a8f4c8 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1815,7 +1815,6 @@ static void build_smb0(Aml *table, I2CBus *smbus, int devnr, int func)
>      Aml *scope = aml_scope("_SB.PCI0");
>      Aml *dev = aml_device("SMB0");
>  
> -    aml_append(dev, aml_name_decl("_HID", aml_eisaid("APP0005")));
>      aml_append(dev, aml_name_decl("_ADR", aml_int(devnr << 16 | func)));
>      build_acpi_ipmi_devices(dev, BUS(smbus), "\\_SB.PCI0.SMB0");
>      aml_append(scope, dev);
Igor Mammedov Jan. 7, 2020, 4:58 p.m. UTC | #2 
On Mon,  6 Jan 2020 09:27:05 -0600
minyard@acm.org wrote:

> From: Corey Minyard <cminyard@mvista.com>
> 
> Per the ACPI spec (version 6.1, section 6.1.5 _HID) it is not required
> on enumerated buses (like PCI in this case), _ADR is required (and is
> already there).  And the _HID value is wrong.  Linux appears to ignore
> the _HID entry, but it confuses Windows.

Corey,

Could you clarify as what "confuses Windows" means?
s/confuses Windows/description of the observed problem and on what windows version/

> 
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/i386/acpi-build.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 7b8da62d41..ab73a8f4c8 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1815,7 +1815,6 @@ static void build_smb0(Aml *table, I2CBus *smbus, int devnr, int func)
>      Aml *scope = aml_scope("_SB.PCI0");
>      Aml *dev = aml_device("SMB0");
>  
> -    aml_append(dev, aml_name_decl("_HID", aml_eisaid("APP0005")));
>      aml_append(dev, aml_name_decl("_ADR", aml_int(devnr << 16 | func)));
>      build_acpi_ipmi_devices(dev, BUS(smbus), "\\_SB.PCI0.SMB0");
>      aml_append(scope, dev);
Corey Minyard Jan. 7, 2020, 8:11 p.m. UTC | #3 
On Tue, Jan 07, 2020 at 05:58:21PM +0100, Igor Mammedov wrote:
> On Mon,  6 Jan 2020 09:27:05 -0600
> minyard@acm.org wrote:
> 
> > From: Corey Minyard <cminyard@mvista.com>
> > 
> > Per the ACPI spec (version 6.1, section 6.1.5 _HID) it is not required
> > on enumerated buses (like PCI in this case), _ADR is required (and is
> > already there).  And the _HID value is wrong.  Linux appears to ignore
> > the _HID entry, but it confuses Windows.
> 
> Corey,
> 
> Could you clarify as what "confuses Windows" means?
> s/confuses Windows/description of the observed problem and on what windows version/

Yeah, I should have done that.  The error is not given, but the report
says" "It is detected by Windows 10 as 'Unknown Device' and there is no
driver available."  Link is https://bugs.launchpad.net/qemu/+bug/1856724

I'll add that to the text, along with the link.

-corey

> 
> > 
> > Signed-off-by: Corey Minyard <cminyard@mvista.com>
> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > Cc: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  hw/i386/acpi-build.c | 1 -
> >  1 file changed, 1 deletion(-)
> > 
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index 7b8da62d41..ab73a8f4c8 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -1815,7 +1815,6 @@ static void build_smb0(Aml *table, I2CBus *smbus, int devnr, int func)
> >      Aml *scope = aml_scope("_SB.PCI0");
> >      Aml *dev = aml_device("SMB0");
> >  
> > -    aml_append(dev, aml_name_decl("_HID", aml_eisaid("APP0005")));
> >      aml_append(dev, aml_name_decl("_ADR", aml_int(devnr << 16 | func)));
> >      build_acpi_ipmi_devices(dev, BUS(smbus), "\\_SB.PCI0.SMB0");
> >      aml_append(scope, dev);
>
Michael S. Tsirkin Jan. 13, 2020, 2:05 p.m. UTC | #4 
On Tue, Jan 07, 2020 at 02:11:06PM -0600, Corey Minyard wrote:
> On Tue, Jan 07, 2020 at 05:58:21PM +0100, Igor Mammedov wrote:
> > On Mon,  6 Jan 2020 09:27:05 -0600
> > minyard@acm.org wrote:
> > 
> > > From: Corey Minyard <cminyard@mvista.com>
> > > 
> > > Per the ACPI spec (version 6.1, section 6.1.5 _HID) it is not required
> > > on enumerated buses (like PCI in this case), _ADR is required (and is
> > > already there).  And the _HID value is wrong.  Linux appears to ignore
> > > the _HID entry, but it confuses Windows.
> > 
> > Corey,
> > 
> > Could you clarify as what "confuses Windows" means?
> > s/confuses Windows/description of the observed problem and on what windows version/
> 
> Yeah, I should have done that.  The error is not given, but the report
> says" "It is detected by Windows 10 as 'Unknown Device' and there is no
> driver available."  Link is https://bugs.launchpad.net/qemu/+bug/1856724
> 
> I'll add that to the text, along with the link.
> 
> -corey


ok so you will repost with igor's ack and tweaked commit log?

> > 
> > > 
> > > Signed-off-by: Corey Minyard <cminyard@mvista.com>
> > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > Cc: Igor Mammedov <imammedo@redhat.com>
> > > ---
> > >  hw/i386/acpi-build.c | 1 -
> > >  1 file changed, 1 deletion(-)
> > > 
> > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > index 7b8da62d41..ab73a8f4c8 100644
> > > --- a/hw/i386/acpi-build.c
> > > +++ b/hw/i386/acpi-build.c
> > > @@ -1815,7 +1815,6 @@ static void build_smb0(Aml *table, I2CBus *smbus, int devnr, int func)
> > >      Aml *scope = aml_scope("_SB.PCI0");
> > >      Aml *dev = aml_device("SMB0");
> > >  
> > > -    aml_append(dev, aml_name_decl("_HID", aml_eisaid("APP0005")));
> > >      aml_append(dev, aml_name_decl("_ADR", aml_int(devnr << 16 | func)));
> > >      build_acpi_ipmi_devices(dev, BUS(smbus), "\\_SB.PCI0.SMB0");
> > >      aml_append(scope, dev);
> >
diff mbox series

Patch

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 7b8da62d41..ab73a8f4c8 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1815,7 +1815,6 @@  static void build_smb0(Aml *table, I2CBus *smbus, int devnr, int func)
     Aml *scope = aml_scope("_SB.PCI0");
     Aml *dev = aml_device("SMB0");
 
-    aml_append(dev, aml_name_decl("_HID", aml_eisaid("APP0005")));
     aml_append(dev, aml_name_decl("_ADR", aml_int(devnr << 16 | func)));
     build_acpi_ipmi_devices(dev, BUS(smbus), "\\_SB.PCI0.SMB0");
     aml_append(scope, dev);