Change in coreboot[master]: drivers/wifi/generic: Add support for generating SMBIOS data

[Furquan Shaikh (Code Review)]

Furquan Shaikh has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/46034 )

Change subject: drivers/wifi/generic: Add support for generating SMBIOS data
......................................................................

drivers/wifi/generic: Add support for generating SMBIOS data

This change adds support in generic WiFi driver in coreboot to
generate SMBIOS data for the WiFi device. Currently, this is used only
for Intel WiFi devices and the function is copied over from Intel WiFi
driver in coreboot. This change is done in preparation for getting rid
of the separate chip driver for Intel WiFi in coreboot.

BUG=b:169802515
BRANCH=zork

Change-Id: If3c056718bdc57f6976ce8e3f8acc7665ec3ccd7
Signed-off-by: Furquan Shaikh 
---
M src/drivers/wifi/generic/generic.c
1 file changed, 40 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/46034/1

diff --git a/src/drivers/wifi/generic/generic.c b/src/drivers/wifi/generic/generic.c
index 903afdc..0fb4dc5 100644
--- a/src/drivers/wifi/generic/generic.c
+++ b/src/drivers/wifi/generic/generic.c
@@ -8,6 +8,7 @@
 #include <device/pci_def.h>
 #include <elog.h>
 #include <sar.h>
+#include <smbios.h>
 #include <string.h>
 #include <wrdd.h>
 #include "chip.h"
@@ -243,6 +244,42 @@
    pci_dev_log_wake(dev, ELOG_WAKE_SOURCE_PME_WIFI, 0);
 }

+#if CONFIG(GENERATE_SMBIOS_TABLES)
+static int smbios_write_intel_wifi(struct device *dev, int *handle, unsigned long *current)
+{
+	struct smbios_type_intel_wifi {
+		u8 type;
+		u8 length;
+		u16 handle;
+		u8 str;
+		u8 eos[2];
+	} __packed;
+
+	struct smbios_type_intel_wifi *t = (struct smbios_type_intel_wifi *)*current;
+	int len = sizeof(struct smbios_type_intel_wifi);
+
+	memset(t, 0, sizeof(struct smbios_type_intel_wifi));
+	t->type = 0x85;
+	t->length = len - 2;
+	t->handle = *handle;
+	/* Intel wifi driver expects this string to be in the table 0x85. */
+	t->str = smbios_add_string(t->eos, "KHOIHGIUCCHHII");
+
+	len = t->length + smbios_string_table_len(t->eos);
+	*current += len;
+	*handle += 1;
+	return len;
+}
+
+static int smbios_write_wifi(struct device *dev, int *handle, unsigned long *current)
+{
+	if (dev->vendor == PCI_VENDOR_ID_INTEL)
+		return smbios_write_intel_wifi(dev, handle, current);
+
+	return 0;
+}
+#endif
+
 struct device_operations wifi_generic_ops = {
    .read_resources		= pci_dev_read_resources,
    .set_resources		= pci_dev_set_resources,
@@ -251,6 +288,9 @@
    .ops_pci		= &pci_dev_ops_pci,
    .acpi_name		= wifi_generic_acpi_name,
    .acpi_fill_ssdt		= wifi_generic_fill_ssdt_generator,
+#if CONFIG(GENERATE_SMBIOS_TABLES)
+	.get_smbios_data		= smbios_write_wifi,
+#endif
 };

static void wifi_generic_enable(struct device *dev)

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46034 )

Change subject: drivers/wifi/generic: Add support for generating SMBIOS data
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/46034/1/src/drivers/wifi/generic/ge... 
File src/drivers/wifi/generic/generic.c:

https://review.coreboot.org/c/coreboot/+/46034/1/src/drivers/wifi/generic/ge... 
PS1, Line 248: smbios_write_intel_wifi
I am not really sure if this is still required. It was added ~6 years back(https://review.coreboot.org/c/coreboot/+/7295), but I don't see anything using this in Linux kernel. I have kept this as is for now to keep differences to a minimum from the intel wifi driver. But if this is not required, I think we should just drop it completely.

[Rob Barnes (Code Review)]

Rob Barnes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46034 )

Change subject: drivers/wifi/generic: Add support for generating SMBIOS data
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/46034/1/src/drivers/wifi/generic/ge... 
File src/drivers/wifi/generic/generic.c:

https://review.coreboot.org/c/coreboot/+/46034/1/src/drivers/wifi/generic/ge... 
PS1, Line 279: 	return 0;
What does returning 0 mean here? no-op or error? elog_add_event_wake returns a positive length.

[Duncan Laurie (Code Review)]

Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46034 )

Change subject: drivers/wifi/generic: Add support for generating SMBIOS data
......................................................................

Patch Set 1:

(2 comments)

https://review.coreboot.org/c/coreboot/+/46034/1/src/drivers/wifi/generic/ge... 
File src/drivers/wifi/generic/generic.c:

https://review.coreboot.org/c/coreboot/+/46034/1/src/drivers/wifi/generic/ge... 
PS1, Line 248: smbios_write_intel_wifi
> I am not really sure if this is still required. It was added ~6 years back(https://review.coreboot. […]

Maybe it means something to Windows?

https://review.coreboot.org/c/coreboot/+/46034/1/src/drivers/wifi/generic/ge... 
PS1, Line 279: 	return 0;
> What does returning 0 mean here? no-op or error? elog_add_event_wake returns a positive length.

Should just mean that nothing was written.

[Michael Niewöhner (Code Review)]

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46034 )

Change subject: drivers/wifi/generic: Add support for generating SMBIOS data
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/46034/1/src/drivers/wifi/generic/ge... 
File src/drivers/wifi/generic/generic.c:

https://review.coreboot.org/c/coreboot/+/46034/1/src/drivers/wifi/generic/ge... 
PS1, Line 248: smbios_write_intel_wifi
> Maybe it means something to Windows?

or option roms?

[Michael Niewöhner (Code Review)]

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46034 )

Change subject: drivers/wifi/generic: Add support for generating SMBIOS data
......................................................................

Patch Set 1: Code-Review+1

[Furquan Shaikh (Code Review)]

Hello build bot (Jenkins), Duncan Laurie, Michael Niewöhner, Rob Barnes, Karthik Ramasubramanian,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/46034

to look at the new patch set (#2).

Change subject: drivers/wifi/generic: Add support for generating SMBIOS data
......................................................................

drivers/wifi/generic: Add support for generating SMBIOS data

This change adds support in generic WiFi driver in coreboot to
generate SMBIOS data for the WiFi device. Currently, this is used only
for Intel WiFi devices and the function is copied over from Intel WiFi
driver in coreboot. This change is done in preparation for getting rid
of the separate chip driver for Intel WiFi in coreboot.

BUG=b:169802515
BRANCH=zork

Change-Id: If3c056718bdc57f6976ce8e3f8acc7665ec3ccd7
Signed-off-by: Furquan Shaikh 
---
M src/drivers/wifi/generic/generic.c
1 file changed, 41 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/46034/2

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46034 )

Change subject: drivers/wifi/generic: Add support for generating SMBIOS data
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/46034/1/src/drivers/wifi/generic/ge... 
File src/drivers/wifi/generic/generic.c:

https://review.coreboot.org/c/coreboot/+/46034/1/src/drivers/wifi/generic/ge... 
PS1, Line 248: smbios_write_intel_wifi
> or option roms?

Leaving this in as is for now.

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46034 )

Change subject: drivers/wifi/generic: Add support for generating SMBIOS data
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/46034/1/src/drivers/wifi/generic/ge... 
File src/drivers/wifi/generic/generic.c:

https://review.coreboot.org/c/coreboot/+/46034/1/src/drivers/wifi/generic/ge... 
PS1, Line 279: 	return 0;
> Should just mean that nothing was written.

That's right. It basically means nothing was written.

[Michael Niewöhner (Code Review)]

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46034 )

Change subject: drivers/wifi/generic: Add support for generating SMBIOS data
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/46034/1/src/drivers/wifi/generic/ge... 
File src/drivers/wifi/generic/generic.c:

https://review.coreboot.org/c/coreboot/+/46034/1/src/drivers/wifi/generic/ge... 
PS1, Line 248: smbios_write_intel_wifi
> Leaving this in as is for now.

Yay, I found something. Some of Lenovo's Intel Wireless driver for Windows XP/Vista/7 seem to check that string "KHOIHGIUCCHHII"

I found at least these two containing that string:
- https://support.lenovo.com/bo/de/downloads/ds001145
- https://support.lenovo.com/by/de/downloads/ds030245

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46034 )

Change subject: drivers/wifi/generic: Add support for generating SMBIOS data
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/46034/1/src/drivers/wifi/generic/ge... 
File src/drivers/wifi/generic/generic.c:

https://review.coreboot.org/c/coreboot/+/46034/1/src/drivers/wifi/generic/ge... 
PS1, Line 248: smbios_write_intel_wifi
> Yay, I found something. […]

Thanks for digging that up Michael! We can leave this in as is.

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46034 )

Change subject: drivers/wifi/generic: Add support for generating SMBIOS data
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/46034/2/src/drivers/wifi/generic/ge... 
File src/drivers/wifi/generic/generic.c:

https://review.coreboot.org/c/coreboot/+/46034/2/src/drivers/wifi/generic/ge... 
PS2, Line 293: #if CONFIG(GENERATE_SMBIOS_TABLES)
suggestion: instead of using #if CONFIG here, you could just add the function, and have it return immediately if not defined, e.g.
```
if (!CONFIG(GENERATE_SMBIOS_TABLES))
return;
```

to avoids more #ifs

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46034 )

Change subject: drivers/wifi/generic: Add support for generating SMBIOS data
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/46034/2/src/drivers/wifi/generic/ge... 
File src/drivers/wifi/generic/generic.c:

https://review.coreboot.org/c/coreboot/+/46034/2/src/drivers/wifi/generic/ge... 
PS2, Line 293: #if CONFIG(GENERATE_SMBIOS_TABLES)
> suggestion: instead of using #if CONFIG here, you could just add the function, and have it return im […]

Actually, `get_smbios_data` structure member definition itself is guarded using #if CONFIG(GENERATE_SMBIOS_TABLES). So, I think we will need the same guard here.

[Duncan Laurie (Code Review)]

Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46034 )

Change subject: drivers/wifi/generic: Add support for generating SMBIOS data
......................................................................

Patch Set 3: Code-Review+2

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46034 )

Change subject: drivers/wifi/generic: Add support for generating SMBIOS data
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/46034/2/src/drivers/wifi/generic/ge... 
File src/drivers/wifi/generic/generic.c:

https://review.coreboot.org/c/coreboot/+/46034/2/src/drivers/wifi/generic/ge... 
PS2, Line 293: #if CONFIG(GENERATE_SMBIOS_TABLES)
> Actually, `get_smbios_data` structure member definition itself is guarded using #if CONFIG(GENERATE_ […]

Done

[Michael Niewöhner (Code Review)]

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46034 )

Change subject: drivers/wifi/generic: Add support for generating SMBIOS data
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/46034/3/src/drivers/wifi/generic/ge... 
File src/drivers/wifi/generic/generic.c:

https://review.coreboot.org/c/coreboot/+/46034/3/src/drivers/wifi/generic/ge... 
PS3, Line 294: 	
drop one tab

[Furquan Shaikh (Code Review)]

Hello build bot (Jenkins), Duncan Laurie, Tim Wawrzynczak, Michael Niewöhner, Rob Barnes, Karthik Ramasubramanian,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/46034

to look at the new patch set (#4).

Change subject: drivers/wifi/generic: Add support for generating SMBIOS data
......................................................................

drivers/wifi/generic: Add support for generating SMBIOS data

This change adds support in generic WiFi driver in coreboot to
generate SMBIOS data for the WiFi device. Currently, this is used only
for Intel WiFi devices and the function is copied over from Intel WiFi
driver in coreboot. This change is done in preparation for getting rid
of the separate chip driver for Intel WiFi in coreboot.

BUG=b:169802515
BRANCH=zork

Change-Id: If3c056718bdc57f6976ce8e3f8acc7665ec3ccd7
Signed-off-by: Furquan Shaikh 
---
M src/drivers/wifi/generic/generic.c
1 file changed, 41 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/46034/4

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46034 )

Change subject: drivers/wifi/generic: Add support for generating SMBIOS data
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/46034/3/src/drivers/wifi/generic/ge... 
File src/drivers/wifi/generic/generic.c:

https://review.coreboot.org/c/coreboot/+/46034/3/src/drivers/wifi/generic/ge... 
PS3, Line 294:
> drop one tab

Done

[Michael Niewöhner (Code Review)]

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46034 )

Change subject: drivers/wifi/generic: Add support for generating SMBIOS data
......................................................................

Patch Set 4: Code-Review+2

[Furquan Shaikh (Code Review)]

Furquan Shaikh has submitted this change. ( https://review.coreboot.org/c/coreboot/+/46034 )

Change subject: drivers/wifi/generic: Add support for generating SMBIOS data
......................................................................

drivers/wifi/generic: Add support for generating SMBIOS data

This change adds support in generic WiFi driver in coreboot to
generate SMBIOS data for the WiFi device. Currently, this is used only
for Intel WiFi devices and the function is copied over from Intel WiFi
driver in coreboot. This change is done in preparation for getting rid
of the separate chip driver for Intel WiFi in coreboot.

BUG=b:169802515
BRANCH=zork

Change-Id: If3c056718bdc57f6976ce8e3f8acc7665ec3ccd7
Signed-off-by: Furquan Shaikh 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/46034
Tested-by: build bot (Jenkins) 
Reviewed-by: Michael Niewöhner 
---
M src/drivers/wifi/generic/generic.c
1 file changed, 41 insertions(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Michael Niewöhner: Looks good to me, approved

diff --git a/src/drivers/wifi/generic/generic.c b/src/drivers/wifi/generic/generic.c
index 0705731..e551bf3 100644
--- a/src/drivers/wifi/generic/generic.c
+++ b/src/drivers/wifi/generic/generic.c
@@ -6,8 +6,10 @@
 #include <device/device.h>
 #include <device/pci.h>
 #include <device/pci_def.h>
+#include <device/pci_ids.h>
 #include <elog.h>
 #include <sar.h>
+#include <smbios.h>
 #include <string.h>
 #include <wrdd.h>
 #include "chip.h"
@@ -244,6 +246,42 @@
    	elog_add_event_wake(ELOG_WAKE_SOURCE_PME_WIFI, 0);
 }

+#if CONFIG(GENERATE_SMBIOS_TABLES)
+static int smbios_write_intel_wifi(struct device *dev, int *handle, unsigned long *current)
+{
+	struct smbios_type_intel_wifi {
+		u8 type;
+		u8 length;
+		u16 handle;
+		u8 str;
+		u8 eos[2];
+	} __packed;
+
+	struct smbios_type_intel_wifi *t = (struct smbios_type_intel_wifi *)*current;
+	int len = sizeof(struct smbios_type_intel_wifi);
+
+	memset(t, 0, sizeof(struct smbios_type_intel_wifi));
+	t->type = 0x85;
+	t->length = len - 2;
+	t->handle = *handle;
+	/* Intel wifi driver expects this string to be in the table 0x85. */
+	t->str = smbios_add_string(t->eos, "KHOIHGIUCCHHII");
+
+	len = t->length + smbios_string_table_len(t->eos);
+	*current += len;
+	*handle += 1;
+	return len;
+}
+
+static int smbios_write_wifi(struct device *dev, int *handle, unsigned long *current)
+{
+	if (dev->vendor == PCI_VENDOR_ID_INTEL)
+		return smbios_write_intel_wifi(dev, handle, current);
+
+	return 0;
+}
+#endif
+
 struct device_operations wifi_generic_ops = {
    .read_resources		= pci_dev_read_resources,
    .set_resources		= pci_dev_set_resources,
@@ -252,6 +290,9 @@
    .ops_pci		= &pci_dev_ops_pci,
    .acpi_name		= wifi_generic_acpi_name,
    .acpi_fill_ssdt		= wifi_generic_fill_ssdt_generator,
+#if CONFIG(GENERATE_SMBIOS_TABLES)
+	.get_smbios_data	= smbios_write_wifi,
+#endif
 };

static void wifi_generic_enable(struct device *dev)