[PATCH] dcdbas: Check SMBIOS for protected buffer address

[Stuart Hayes]

Add support for a new method for BIOS to provide the address and length
of the protected SMI communication buffer, via SMBIOS OEM strings.

Signed-off-by: Stuart Hayes 
---
 drivers/platform/x86/dcdbas.c | 43 ---
 1 file changed, 30 insertions(+), 13 deletions(-)

diff --git a/drivers/platform/x86/dcdbas.c b/drivers/platform/x86/dcdbas.c
index 84f4cc839cc3..d513a59a5d47 100644
--- a/drivers/platform/x86/dcdbas.c
+++ b/drivers/platform/x86/dcdbas.c
@@ -15,6 +15,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -34,7 +35,7 @@
 #include "dcdbas.h"
 
 #define DRIVER_NAME"dcdbas"
-#define DRIVER_VERSION "5.6.0-3.3"
+#define DRIVER_VERSION "5.6.0-3.4"
 #define DRIVER_DESCRIPTION "Dell Systems Management Base Driver"
 
 static struct platform_device *dcdbas_pdev;
@@ -45,7 +46,7 @@ static unsigned long smi_data_buf_size;
 static unsigned long max_smi_data_buf_size = MAX_SMI_DATA_BUF_SIZE;
 static u32 smi_data_buf_phys_addr;
 static DEFINE_MUTEX(smi_data_lock);
-static u8 *eps_buffer;
+static u8 *bios_buffer;
 
 static unsigned int host_control_action;
 static unsigned int host_control_smi_type;
@@ -518,8 +519,10 @@ static inline struct smm_eps_table *check_eps_table(u8 
*addr)
 
 static int dcdbas_check_wsmt(void)
 {
+   const struct dmi_device *dev = NULL;
struct acpi_table_wsmt *wsmt = NULL;
struct smm_eps_table *eps = NULL;
+   u64 bios_buf_paddr;
u64 remap_size;
u8 *addr;
 
@@ -532,6 +535,17 @@ static int dcdbas_check_wsmt(void)
!(wsmt->protection_flags & 
ACPI_WSMT_COMM_BUFFER_NESTED_PTR_PROTECTION))
return 0;
 
+   /*
+* BIOS could provide the address/size of the protected buffer
+* in an SMBIOS string or in an EPS structure in 0xF.
+*/
+
+   /* Check SMBIOS for buffer address */
+   while ((dev = dmi_find_device(DMI_DEV_TYPE_OEM_STRING, NULL, dev)))
+   if (sscanf(dev->name, "30[%16llx;%8llx]", _buf_paddr,
+   _size) == 2)
+   goto remap;
+
/* Scan for EPS (entry point structure) */
for (addr = (u8 *)__va(0xf);
 addr < (u8 *)__va(0x10 - sizeof(struct smm_eps_table));
@@ -542,34 +556,37 @@ static int dcdbas_check_wsmt(void)
}
 
if (!eps) {
-   dev_dbg(_pdev->dev, "found WSMT, but no EPS found\n");
+   dev_dbg(_pdev->dev, "found WSMT, but no firmware buffer 
found\n");
return -ENODEV;
}
+   bios_buf_paddr = eps->smm_comm_buff_addr;
+   remap_size = eps->num_of_4k_pages * PAGE_SIZE;
 
+remap:
/*
 * Get physical address of buffer and map to virtual address.
 * Table gives size in 4K pages, regardless of actual system page size.
 */
-   if (upper_32_bits(eps->smm_comm_buff_addr + 8)) {
-   dev_warn(_pdev->dev, "found WSMT, but EPS buffer address 
is above 4GB\n");
+   if (upper_32_bits(bios_buf_paddr + 8)) {
+   dev_warn(_pdev->dev, "found WSMT, but buffer address is 
above 4GB\n");
return -EINVAL;
}
/*
 * Limit remap size to MAX_SMI_DATA_BUF_SIZE + 8 (since the first 8
 * bytes are used for a semaphore, not the data buffer itself).
 */
-   remap_size = eps->num_of_4k_pages * PAGE_SIZE;
if (remap_size > MAX_SMI_DATA_BUF_SIZE + 8)
remap_size = MAX_SMI_DATA_BUF_SIZE + 8;
-   eps_buffer = memremap(eps->smm_comm_buff_addr, remap_size, MEMREMAP_WB);
-   if (!eps_buffer) {
-   dev_warn(_pdev->dev, "found WSMT, but failed to map EPS 
buffer\n");
+
+   bios_buffer = memremap(bios_buf_paddr, remap_size, MEMREMAP_WB);
+   if (!bios_buffer) {
+   dev_warn(_pdev->dev, "found WSMT, but failed to map 
buffer\n");
return -ENOMEM;
}
 
/* First 8 bytes is for a semaphore, not part of the smi_data_buf */
-   smi_data_buf_phys_addr = eps->smm_comm_buff_addr + 8;
-   smi_data_buf = eps_buffer + 8;
+   smi_data_buf_phys_addr = bios_buf_paddr + 8;
+   smi_data_buf = bios_buffer + 8;
smi_data_buf_size = remap_size - 8;
max_smi_data_buf_size = smi_data_buf_size;
wsmt_enabled = true;
@@ -736,8 +753,8 @@ static void __exit dcdbas_exit(void)
 */
if (dcdbas_pdev)
smi_data_buf_free();
-   if (eps_buffer)
-   memunmap(eps_buffer);
+   if (bios_buffer)
+   memunmap(bios_buffer);
platform_device_unregister(dcdbas_pdev_reg);
platform_driver_unregister(_driver);
 }
-- 
2.18.1

Re: [PATCH v3 3/3] PCI: pciehp: Add dmi table for in-band presence disabled

[Stuart Hayes]

On 10/21/19 8:47 AM, Mika Westerberg wrote:
> On Thu, Oct 17, 2019 at 03:32:56PM -0400, Stuart Hayes wrote:
>> Some systems have in-band presence detection disabled for hot-plug PCI
>> slots, but do not report this in the slot capabilities 2 (SLTCAP2) register.
>> On these systems, presence detect can become active well after the link is
>> reported to be active, which can cause the slots to be disabled after a
>> device is connected.
>>
>> Add a dmi table to flag these systems as having in-band presence disabled.
>>
>> Signed-off-by: Stuart Hayes 
>> ---
>>  drivers/pci/hotplug/pciehp_hpc.c | 14 ++
>>  1 file changed, 14 insertions(+)
>>
>> diff --git a/drivers/pci/hotplug/pciehp_hpc.c 
>> b/drivers/pci/hotplug/pciehp_hpc.c
>> index 02eb811a014f..4d377a2a62ce 100644
>> --- a/drivers/pci/hotplug/pciehp_hpc.c
>> +++ b/drivers/pci/hotplug/pciehp_hpc.c
>> @@ -14,6 +14,7 @@
>>  
>>  #define dev_fmt(fmt) "pciehp: " fmt
>>  
>> +#include 
>>  #include 
>>  #include 
>>  #include 
>> @@ -26,6 +27,16 @@
>>  #include "../pci.h"
>>  #include "pciehp.h"
>>  
>> +static const struct dmi_system_id inband_presence_disabled_dmi_table[] = {
>> +{
>> +.ident = "Dell System",
>> +.matches = {
>> +DMI_MATCH(DMI_OEM_STRING, "Dell System"),
> 
> Sorry if this has been discussed previously already but isn't this going
> to apply on all Dell systems, not just the affected ones? Is this the
> intention?
> 

Yes, that is the intention. Applying this just makes the hotplug code wait for 
the presence detect bit to be set before proceeding, which ideally wouldn't 
hurt 
anything--for devices that don't have inband presence detect disabled, presence
detect should already be up when the code in patch 2/3 starts to wait for it.

The only issue should be with broken hotplug implementations that don't ever 
bring presence detect active (these apparently exist)--but even those would 
still 
work, they would just take an extra second to come up.

On the other hand, a number of Dell systems have (and will have) NVMe 
implementations that have inband presence detect disabled (but they won't have
the new bit implemented to report that), and they don't work correctly without 
this.

Re: [PATCH v3 2/3] PCI: pciehp: Wait for PDS if in-band presence is disabled

[Stuart Hayes]

On 10/21/19 8:41 AM, Mika Westerberg wrote:
> On Thu, Oct 17, 2019 at 03:32:55PM -0400, Stuart Hayes wrote:
>> From: Alexandru Gagniuc 
>>
>> When inband presence is disabled, PDS may come up at any time, or not
>> at all. PDS being low may indicate that the card is still mating, and
>> we could expect contact bounce to bring down the link as well.
>>
>> It is reasonable to assume that most cards will mate in a hotplug slot
>> in about a second. Thus, when we know PDS only reflects out-of-band
>> presence, it's worthwhile to wait the extra second or so to make sure
>> the card is properly mated before loading the driver, and to prevent
>> the hotplug code from disabling a device if the presence detect change
>> goes active after the device is enabled.
>>
>> Signed-off-by: Alexandru Gagniuc 
>> Signed-off-by: Stuart Hayes 
> 
> Reviewed-by: Mika Westerberg 
> 
> One nit below.
> 
>> ---
>> v2:
>>   replace while(true) loop with do...while
>> v3
>>   remove unused variable declaration (pds)
>>   modify text of warning message
>>
>>  drivers/pci/hotplug/pciehp_hpc.c | 19 +++
>>  1 file changed, 19 insertions(+)
>>
>> diff --git a/drivers/pci/hotplug/pciehp_hpc.c 
>> b/drivers/pci/hotplug/pciehp_hpc.c
>> index dc109d521f30..02eb811a014f 100644
>> --- a/drivers/pci/hotplug/pciehp_hpc.c
>> +++ b/drivers/pci/hotplug/pciehp_hpc.c
>> @@ -242,6 +242,22 @@ static bool pci_bus_check_dev(struct pci_bus *bus, int 
>> devfn)
>>  return found;
>>  }
>>  
>> +static void pcie_wait_for_presence(struct pci_dev *pdev)
>> +{
>> +int timeout = 1250;
>> +u16 slot_status;
>> +
>> +do {
>> +pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, _status);
>> +if (!!(slot_status & PCI_EXP_SLTSTA_PDS))
> 
> It is more readable if you write it like:
> 
>   if (slot_status & PCI_EXP_SLTSTA_PDS)
> 

I agree, it is more readable, and the double bang shouldn't be needed for an 
"if" condition.  Thanks.

>> +return;
>> +msleep(10);
>> +timeout -= 10;
>> +} while (timeout > 0);
>> +
>> +pci_info(pdev, "Timeout waiting for Presence Detect state to be set\n");
>> +}
>> +
>>  int pciehp_check_link_status(struct controller *ctrl)
>>  {
>>  struct pci_dev *pdev = ctrl_dev(ctrl);
>> @@ -251,6 +267,9 @@ int pciehp_check_link_status(struct controller *ctrl)
>>  if (!pcie_wait_for_link(pdev, true))
>>  return -1;
>>  
>> +if (ctrl->inband_presence_disabled)
>> +pcie_wait_for_presence(pdev);
>> +
>>  found = pci_bus_check_dev(ctrl->pcie->port->subordinate,
>>  PCI_DEVFN(0, 0));
>>  
>> -- 
>> 2.18.1

[PATCH v3 2/3] PCI: pciehp: Wait for PDS if in-band presence is disabled

[Stuart Hayes]

From: Alexandru Gagniuc 

When inband presence is disabled, PDS may come up at any time, or not
at all. PDS being low may indicate that the card is still mating, and
we could expect contact bounce to bring down the link as well.

It is reasonable to assume that most cards will mate in a hotplug slot
in about a second. Thus, when we know PDS only reflects out-of-band
presence, it's worthwhile to wait the extra second or so to make sure
the card is properly mated before loading the driver, and to prevent
the hotplug code from disabling a device if the presence detect change
goes active after the device is enabled.

Signed-off-by: Alexandru Gagniuc 
Signed-off-by: Stuart Hayes 
---
v2:
  replace while(true) loop with do...while
v3
  remove unused variable declaration (pds)
  modify text of warning message

 drivers/pci/hotplug/pciehp_hpc.c | 19 +++
 1 file changed, 19 insertions(+)

diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index dc109d521f30..02eb811a014f 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -242,6 +242,22 @@ static bool pci_bus_check_dev(struct pci_bus *bus, int 
devfn)
return found;
 }
 
+static void pcie_wait_for_presence(struct pci_dev *pdev)
+{
+   int timeout = 1250;
+   u16 slot_status;
+
+   do {
+   pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, _status);
+   if (!!(slot_status & PCI_EXP_SLTSTA_PDS))
+   return;
+   msleep(10);
+   timeout -= 10;
+   } while (timeout > 0);
+
+   pci_info(pdev, "Timeout waiting for Presence Detect state to be set\n");
+}
+
 int pciehp_check_link_status(struct controller *ctrl)
 {
struct pci_dev *pdev = ctrl_dev(ctrl);
@@ -251,6 +267,9 @@ int pciehp_check_link_status(struct controller *ctrl)
if (!pcie_wait_for_link(pdev, true))
return -1;
 
+   if (ctrl->inband_presence_disabled)
+   pcie_wait_for_presence(pdev);
+
found = pci_bus_check_dev(ctrl->pcie->port->subordinate,
PCI_DEVFN(0, 0));
 
-- 
2.18.1

[PATCH v3 1/3] PCI: pciehp: Add support for disabling in-band presence

[Stuart Hayes]

From: Alexandru Gagniuc 

The presence detect state (PDS) is normally a logical or of in-band and
out-of-band presence. As of PCIe 4.0, there is the option to disable
in-band presence so that the PDS bit always reflects the state of the
out-of-band presence.

The recommendation of the PCIe spec is to disable in-band presence
whenever supported.

Signed-off-by: Alexandru Gagniuc 
---
 drivers/pci/hotplug/pciehp.h | 1 +
 drivers/pci/hotplug/pciehp_hpc.c | 9 -
 include/uapi/linux/pci_regs.h| 2 ++
 3 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
index 654c972b8ea0..27e4cd6529b0 100644
--- a/drivers/pci/hotplug/pciehp.h
+++ b/drivers/pci/hotplug/pciehp.h
@@ -83,6 +83,7 @@ struct controller {
struct pcie_device *pcie;
 
u32 slot_cap;   /* capabilities and quirks */
+   unsigned int inband_presence_disabled:1;
 
u16 slot_ctrl;  /* control register access */
struct mutex ctrl_lock;
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 1a522c1c4177..dc109d521f30 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -811,7 +811,7 @@ static inline void dbg_ctrl(struct controller *ctrl)
 struct controller *pcie_init(struct pcie_device *dev)
 {
struct controller *ctrl;
-   u32 slot_cap, link_cap;
+   u32 slot_cap, slot_cap2, link_cap;
u8 poweron;
struct pci_dev *pdev = dev->port;
struct pci_bus *subordinate = pdev->subordinate;
@@ -869,6 +869,13 @@ struct controller *pcie_init(struct pcie_device *dev)
FLAG(link_cap, PCI_EXP_LNKCAP_DLLLARC),
pdev->broken_cmd_compl ? " (with Cmd Compl erratum)" : "");
 
+   pcie_capability_read_dword(pdev, PCI_EXP_SLTCAP2, _cap2);
+   if (slot_cap2 & PCI_EXP_SLTCAP2_IBPD) {
+   pcie_write_cmd_nowait(ctrl, PCI_EXP_SLTCTL_IBPD_DISABLE,
+ PCI_EXP_SLTCTL_IBPD_DISABLE);
+   ctrl->inband_presence_disabled = 1;
+   }
+
/*
 * If empty slot's power status is on, turn power off.  The IRQ isn't
 * requested yet, so avoid triggering a notification with this command.
diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index 29d6e93fd15e..ea1cf9546e4d 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -604,6 +604,7 @@
 #define  PCI_EXP_SLTCTL_PWR_OFF0x0400 /* Power Off */
 #define  PCI_EXP_SLTCTL_EIC0x0800  /* Electromechanical Interlock Control 
*/
 #define  PCI_EXP_SLTCTL_DLLSCE 0x1000  /* Data Link Layer State Changed Enable 
*/
+#define  PCI_EXP_SLTCTL_IBPD_DISABLE   0x4000 /* In-band PD disable */
 #define PCI_EXP_SLTSTA 26  /* Slot Status */
 #define  PCI_EXP_SLTSTA_ABP0x0001  /* Attention Button Pressed */
 #define  PCI_EXP_SLTSTA_PFD0x0002  /* Power Fault Detected */
@@ -676,6 +677,7 @@
 #define PCI_EXP_LNKSTA250  /* Link Status 2 */
 #define PCI_CAP_EXP_ENDPOINT_SIZEOF_V2 52  /* v2 endpoints with link end 
here */
 #define PCI_EXP_SLTCAP252  /* Slot Capabilities 2 */
+#define  PCI_EXP_SLTCAP2_IBPD  0x0001  /* In-band PD Disable Supported */
 #define PCI_EXP_SLTCTL256  /* Slot Control 2 */
 #define PCI_EXP_SLTSTA258  /* Slot Status 2 */
 
-- 
2.18.1

[PATCH v3 0/3] PCI: pciehp: Do not turn off slot if presence comes up after link

[Stuart Hayes]

In older PCIe specs, PDS (presence detect) would come up when the
"in-band" presence detect pin connected, and would be up before DLLLA
(link active).

In PCIe 4.0 (as an ECN) and in PCIe 5.0, there is a new bit to show if
in-band presence detection can be disabled for the slot, and another bit
that disables it--and a recommendation that it should be disabled if it
can be. In addition, certain OEMs disable in-band presence detection
without implementing these bits.

This means it is possible to get a "card present" interrupt after the
link is up and the driver is loaded.  This causes an erroneous removal
of the device driver, followed by an immediate re-probing.

This patch set defines these new bits, uses them to disable in-band
presence detection if it can be, waits for PDS to go up if in-band
presence detection is disabled, and adds a DMI table that will let us
know if we should assume in-band presence is disabled on a system.

The first two patches in this set come from a patch set that was
submitted but not accepted many months ago by Alexandru Gagniuc [1].
The first is unmodified, the second has the commit message and timeout 
modified.

[1] https://patchwork.kernel.org/cover/10909167/
[v3,0/4] PCI: pciehp: Do not turn off slot if presence comes up after link

v2:
- modify loop in pcie_wait_for_presence to do..while

v3:
- remove unused variable declaration
- modify text of warning message

Alexandru Gagniuc (2):
  PCI: pciehp: Add support for disabling in-band presence
  PCI: pciehp: Wait for PDS if in-band presence is disabled

Stuart Hayes (1):
  PCI: pciehp: Add dmi table for in-band presence disabled

 drivers/pci/hotplug/pciehp.h |  1 +
 drivers/pci/hotplug/pciehp_hpc.c | 45 +++-
 include/uapi/linux/pci_regs.h|  2 ++
 3 files changed, 47 insertions(+), 1 deletion(-)

-- 
2.18.1

[PATCH v3 3/3] PCI: pciehp: Add dmi table for in-band presence disabled

[Stuart Hayes]

Some systems have in-band presence detection disabled for hot-plug PCI
slots, but do not report this in the slot capabilities 2 (SLTCAP2) register.
On these systems, presence detect can become active well after the link is
reported to be active, which can cause the slots to be disabled after a
device is connected.

Add a dmi table to flag these systems as having in-band presence disabled.

Signed-off-by: Stuart Hayes 
---
 drivers/pci/hotplug/pciehp_hpc.c | 14 ++
 1 file changed, 14 insertions(+)

diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 02eb811a014f..4d377a2a62ce 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -14,6 +14,7 @@
 
 #define dev_fmt(fmt) "pciehp: " fmt
 
+#include 
 #include 
 #include 
 #include 
@@ -26,6 +27,16 @@
 #include "../pci.h"
 #include "pciehp.h"
 
+static const struct dmi_system_id inband_presence_disabled_dmi_table[] = {
+   {
+   .ident = "Dell System",
+   .matches = {
+   DMI_MATCH(DMI_OEM_STRING, "Dell System"),
+   },
+   },
+   {}
+};
+
 static inline struct pci_dev *ctrl_dev(struct controller *ctrl)
 {
return ctrl->pcie->port;
@@ -895,6 +906,9 @@ struct controller *pcie_init(struct pcie_device *dev)
ctrl->inband_presence_disabled = 1;
}
 
+   if (dmi_first_match(inband_presence_disabled_dmi_table))
+   ctrl->inband_presence_disabled = 1;
+
/*
 * If empty slot's power status is on, turn power off.  The IRQ isn't
 * requested yet, so avoid triggering a notification with this command.
-- 
2.18.1

[PATCH v2 0/3] PCI: pciehp: Do not turn off slot if presence comes up after link

[Stuart Hayes]

In older PCIe specs, PDS (presence detect) would come up when the
"in-band" presence detect pin connected, and would be up before DLLLA
(link active).

In PCIe 4.0 (as an ECN) and in PCIe 5.0, there is a new bit to show if
in-band presence detection can be disabled for the slot, and another bit
that disables it--and a recommendation that it should be disabled if it
can be. In addition, certain OEMs disable in-band presence detection
without implementing these bits.

This means it is possible to get a "card present" interrupt after the
link is up and the driver is loaded.  This causes an erroneous removal
of the device driver, followed by an immediate re-probing.

This patch set defines these new bits, uses them to disable in-band
presence detection if it can be, waits for PDS to go up if in-band
presence detection is disabled, and adds a DMI table that will let us
know if we should assume in-band presence is disabled on a system.

The first two patches in this set come from a patch set that was
submitted but not accepted many months ago by Alexandru Gagniuc [1].
The first is unmodified, the second has the commit message and timeout 
modified.

[1] https://patchwork.kernel.org/cover/10909167/
[v3,0/4] PCI: pciehp: Do not turn off slot if presence comes up after link

v2:
- modify loop in pcie_wait_for_presence to do..while

Alexandru Gagniuc (2):
  PCI: pciehp: Add support for disabling in-band presence
  PCI: pciehp: Wait for PDS if in-band presence is disabled

Stuart Hayes (1):
  PCI: pciehp: Add dmi table for in-band presence disabled

 drivers/pci/hotplug/pciehp.h |  1 +
 drivers/pci/hotplug/pciehp_hpc.c | 45 +++-
 include/uapi/linux/pci_regs.h|  2 ++
 3 files changed, 47 insertions(+), 1 deletion(-)

-- 
2.18.1

[PATCH v2 2/3] PCI: pciehp: Wait for PDS if in-band presence is disabled

[Stuart Hayes]

From: Alexandru Gagniuc 

When inband presence is disabled, PDS may come up at any time, or not
at all. PDS being low may indicate that the card is still mating, and
we could expect contact bounce to bring down the link as well.

It is reasonable to assume that most cards will mate in a hotplug slot
in about a second. Thus, when we know PDS only reflects out-of-band
presence, it's worthwhile to wait the extra second or so to make sure
the card is properly mated before loading the driver, and to prevent
the hotplug code from disabling a device if the presence detect change
goes active after the device is enabled.

Signed-off-by: Alexandru Gagniuc 
Signed-off-by: Stuart Hayes 
---
v2:
  replace while(true) loop with do...while

 drivers/pci/hotplug/pciehp_hpc.c | 20 
 1 file changed, 20 insertions(+)

diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index dc109d521f30..db5c7b082fda 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -242,6 +242,23 @@ static bool pci_bus_check_dev(struct pci_bus *bus, int 
devfn)
return found;
 }
 
+static void pcie_wait_for_presence(struct pci_dev *pdev)
+{
+   int timeout = 1250;
+   bool pds;
+   u16 slot_status;
+
+   do {
+   pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, _status);
+   if (!!(slot_status & PCI_EXP_SLTSTA_PDS))
+   return;
+   msleep(10);
+   timeout -= 10;
+   } while (timeout > 0);
+
+   pci_info(pdev, "Presence Detect state not set in 1250 msec\n");
+}
+
 int pciehp_check_link_status(struct controller *ctrl)
 {
struct pci_dev *pdev = ctrl_dev(ctrl);
@@ -251,6 +268,9 @@ int pciehp_check_link_status(struct controller *ctrl)
if (!pcie_wait_for_link(pdev, true))
return -1;
 
+   if (ctrl->inband_presence_disabled)
+   pcie_wait_for_presence(pdev);
+
found = pci_bus_check_dev(ctrl->pcie->port->subordinate,
PCI_DEVFN(0, 0));
 
-- 
2.18.1

[PATCH v2 3/3] PCI: pciehp: Add dmi table for in-band presence disabled

[Stuart Hayes]

Some systems have in-band presence detection disabled for hot-plug PCI
slots, but do not report this in the slot capabilities 2 (SLTCAP2) register.
On these systems, presence detect can become active well after the link is
reported to be active, which can cause the slots to be disabled after a
device is connected.

Add a dmi table to flag these systems as having in-band presence disabled.

Signed-off-by: Stuart Hayes 
---
 drivers/pci/hotplug/pciehp_hpc.c | 14 ++
 1 file changed, 14 insertions(+)

diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index db5c7b082fda..f864f0875c53 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -14,6 +14,7 @@
 
 #define dev_fmt(fmt) "pciehp: " fmt
 
+#include 
 #include 
 #include 
 #include 
@@ -26,6 +27,16 @@
 #include "../pci.h"
 #include "pciehp.h"
 
+static const struct dmi_system_id inband_presence_disabled_dmi_table[] = {
+   {
+   .ident = "Dell System",
+   .matches = {
+   DMI_MATCH(DMI_OEM_STRING, "Dell System"),
+   },
+   },
+   {}
+};
+
 static inline struct pci_dev *ctrl_dev(struct controller *ctrl)
 {
return ctrl->pcie->port;
@@ -896,6 +907,9 @@ struct controller *pcie_init(struct pcie_device *dev)
ctrl->inband_presence_disabled = 1;
}
 
+   if (dmi_first_match(inband_presence_disabled_dmi_table))
+   ctrl->inband_presence_disabled = 1;
+
/*
 * If empty slot's power status is on, turn power off.  The IRQ isn't
 * requested yet, so avoid triggering a notification with this command.
-- 
2.18.1

[PATCH v2 1/3] PCI: pciehp: Add support for disabling in-band presence

[Stuart Hayes]

From: Alexandru Gagniuc 

The presence detect state (PDS) is normally a logical or of in-band and
out-of-band presence. As of PCIe 4.0, there is the option to disable
in-band presence so that the PDS bit always reflects the state of the
out-of-band presence.

The recommendation of the PCIe spec is to disable in-band presence
whenever supported.

Signed-off-by: Alexandru Gagniuc 
---
 drivers/pci/hotplug/pciehp.h | 1 +
 drivers/pci/hotplug/pciehp_hpc.c | 9 -
 include/uapi/linux/pci_regs.h| 2 ++
 3 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
index 654c972b8ea0..27e4cd6529b0 100644
--- a/drivers/pci/hotplug/pciehp.h
+++ b/drivers/pci/hotplug/pciehp.h
@@ -83,6 +83,7 @@ struct controller {
struct pcie_device *pcie;
 
u32 slot_cap;   /* capabilities and quirks */
+   unsigned int inband_presence_disabled:1;
 
u16 slot_ctrl;  /* control register access */
struct mutex ctrl_lock;
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 1a522c1c4177..dc109d521f30 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -811,7 +811,7 @@ static inline void dbg_ctrl(struct controller *ctrl)
 struct controller *pcie_init(struct pcie_device *dev)
 {
struct controller *ctrl;
-   u32 slot_cap, link_cap;
+   u32 slot_cap, slot_cap2, link_cap;
u8 poweron;
struct pci_dev *pdev = dev->port;
struct pci_bus *subordinate = pdev->subordinate;
@@ -869,6 +869,13 @@ struct controller *pcie_init(struct pcie_device *dev)
FLAG(link_cap, PCI_EXP_LNKCAP_DLLLARC),
pdev->broken_cmd_compl ? " (with Cmd Compl erratum)" : "");
 
+   pcie_capability_read_dword(pdev, PCI_EXP_SLTCAP2, _cap2);
+   if (slot_cap2 & PCI_EXP_SLTCAP2_IBPD) {
+   pcie_write_cmd_nowait(ctrl, PCI_EXP_SLTCTL_IBPD_DISABLE,
+ PCI_EXP_SLTCTL_IBPD_DISABLE);
+   ctrl->inband_presence_disabled = 1;
+   }
+
/*
 * If empty slot's power status is on, turn power off.  The IRQ isn't
 * requested yet, so avoid triggering a notification with this command.
diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index 29d6e93fd15e..ea1cf9546e4d 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -604,6 +604,7 @@
 #define  PCI_EXP_SLTCTL_PWR_OFF0x0400 /* Power Off */
 #define  PCI_EXP_SLTCTL_EIC0x0800  /* Electromechanical Interlock Control 
*/
 #define  PCI_EXP_SLTCTL_DLLSCE 0x1000  /* Data Link Layer State Changed Enable 
*/
+#define  PCI_EXP_SLTCTL_IBPD_DISABLE   0x4000 /* In-band PD disable */
 #define PCI_EXP_SLTSTA 26  /* Slot Status */
 #define  PCI_EXP_SLTSTA_ABP0x0001  /* Attention Button Pressed */
 #define  PCI_EXP_SLTSTA_PFD0x0002  /* Power Fault Detected */
@@ -676,6 +677,7 @@
 #define PCI_EXP_LNKSTA250  /* Link Status 2 */
 #define PCI_CAP_EXP_ENDPOINT_SIZEOF_V2 52  /* v2 endpoints with link end 
here */
 #define PCI_EXP_SLTCAP252  /* Slot Capabilities 2 */
+#define  PCI_EXP_SLTCAP2_IBPD  0x0001  /* In-band PD Disable Supported */
 #define PCI_EXP_SLTCTL256  /* Slot Control 2 */
 #define PCI_EXP_SLTSTA258  /* Slot Status 2 */
 
-- 
2.18.1

Re: [PATCH 2/3] PCI: pciehp: Wait for PDS if in-band presence is disabled

[Stuart Hayes]

On Thu, Oct 10, 2019 at 12:40 AM Andy Shevchenko
 wrote:
>
> On Thu, Oct 10, 2019 at 8:37 AM Andy Shevchenko
>  wrote:
> > On Wed, Oct 9, 2019 at 11:05 PM Stuart Hayes  
> > wrote:
>
> > > +static void pcie_wait_for_presence(struct pci_dev *pdev)
> > > +{
> > > +   int timeout = 1250;
>
> > > +   bool pds;
>
> Also this is redundant. Just use the following outside the loop
>
>  if (!retries)
>pc_info(...);
>
> .
>
> > > +   u16 slot_status;
> > > +
> > > +   while (true) {
> > > +   pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, 
> > > _status);
> > > +   pds = !!(slot_status & PCI_EXP_SLTSTA_PDS);
> > > +   if (pds || timeout <= 0)
> > > +   break;
> > > +   msleep(10);
> > > +   timeout -= 10;
> > > +   }
> >
> > Can we avoid infinite loops? They are hard to parse (in most cases,
> > and especially when it's a timeout loop)
> >
> > unsigned int retries = 125; // 1250 ms
> >
> > do {
> >  ...
> > } while (--retries);
> >
> > > +
> > > +   if (!pds)
> > > +   pci_info(pdev, "Presence Detect state not set in 1250 
> > > msec\n");
> > > +}
> >
> > --
> > With Best Regards,
> > Andy Shevchenko
>
>
>
> --
> With Best Regards,
> Andy Shevchenko

Thank you for the feedback!  An infinite loop is used several other places in
this driver--this keeps the style similar.  I can change it as you suggest,
though, if that would be preferable to consistency.

[PATCH 2/3] PCI: pciehp: Wait for PDS if in-band presence is disabled

[Stuart Hayes]

From: Alexandru Gagniuc 

When inband presence is disabled, PDS may come up at any time, or not
at all. PDS being low may indicate that the card is still mating, and
we could expect contact bounce to bring down the link as well.

It is reasonable to assume that most cards will mate in a hotplug slot
in about a second. Thus, when we know PDS only reflects out-of-band
presence, it's worthwhile to wait the extra second or so to make sure
the card is properly mated before loading the driver, and to prevent
the hotplug code from disabling a device if the presence detect change
goes active after the device is enabled.

Signed-off-by: Alexandru Gagniuc 
Signed-off-by: Stuart Hayes 
---
 drivers/pci/hotplug/pciehp_hpc.c | 22 ++
 1 file changed, 22 insertions(+)

diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index dc109d521f30..1282641c6458 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -242,6 +242,25 @@ static bool pci_bus_check_dev(struct pci_bus *bus, int 
devfn)
return found;
 }
 
+static void pcie_wait_for_presence(struct pci_dev *pdev)
+{
+   int timeout = 1250;
+   bool pds;
+   u16 slot_status;
+
+   while (true) {
+   pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, _status);
+   pds = !!(slot_status & PCI_EXP_SLTSTA_PDS);
+   if (pds || timeout <= 0)
+   break;
+   msleep(10);
+   timeout -= 10;
+   }
+
+   if (!pds)
+   pci_info(pdev, "Presence Detect state not set in 1250 msec\n");
+}
+
 int pciehp_check_link_status(struct controller *ctrl)
 {
struct pci_dev *pdev = ctrl_dev(ctrl);
@@ -251,6 +270,9 @@ int pciehp_check_link_status(struct controller *ctrl)
if (!pcie_wait_for_link(pdev, true))
return -1;
 
+   if (ctrl->inband_presence_disabled)
+   pcie_wait_for_presence(pdev);
+
found = pci_bus_check_dev(ctrl->pcie->port->subordinate,
PCI_DEVFN(0, 0));
 
-- 
2.18.1

[PATCH 3/3] PCI: pciehp: Add dmi table for in-band presence disabled

[Stuart Hayes]

Some systems have in-band presence detection disabled for hot-plug PCI
slots, but do not report this in the slot capabilities 2 (SLTCAP2) register.
On these systems, presence detect can become active well after the link is
reported to be active, which can cause the slots to be disabled after a
device is connected.

Add a dmi table to flag these systems as having in-band presence disabled.

Signed-off-by: Stuart Hayes 
---
 drivers/pci/hotplug/pciehp_hpc.c | 14 ++
 1 file changed, 14 insertions(+)

diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 1282641c6458..cabd745b844e 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -14,6 +14,7 @@
 
 #define dev_fmt(fmt) "pciehp: " fmt
 
+#include 
 #include 
 #include 
 #include 
@@ -26,6 +27,16 @@
 #include "../pci.h"
 #include "pciehp.h"
 
+static const struct dmi_system_id inband_presence_disabled_dmi_table[] = {
+   {
+   .ident = "Dell System",
+   .matches = {
+   DMI_MATCH(DMI_OEM_STRING, "Dell System"),
+   },
+   },
+   {}
+};
+
 static inline struct pci_dev *ctrl_dev(struct controller *ctrl)
 {
return ctrl->pcie->port;
@@ -898,6 +909,9 @@ struct controller *pcie_init(struct pcie_device *dev)
ctrl->inband_presence_disabled = 1;
}
 
+   if (dmi_first_match(inband_presence_disabled_dmi_table))
+   ctrl->inband_presence_disabled = 1;
+
/*
 * If empty slot's power status is on, turn power off.  The IRQ isn't
 * requested yet, so avoid triggering a notification with this command.
-- 
2.18.1

[PATCH 1/3] PCI: pciehp: Add support for disabling in-band presence

[Stuart Hayes]

From: Alexandru Gagniuc 

The presence detect state (PDS) is normally a logical or of in-band and
out-of-band presence. As of PCIe 4.0, there is the option to disable
in-band presence so that the PDS bit always reflects the state of the
out-of-band presence.

The recommendation of the PCIe spec is to disable in-band presence
whenever supported.

Signed-off-by: Alexandru Gagniuc 
---
 drivers/pci/hotplug/pciehp.h | 1 +
 drivers/pci/hotplug/pciehp_hpc.c | 9 -
 include/uapi/linux/pci_regs.h| 2 ++
 3 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
index 654c972b8ea0..27e4cd6529b0 100644
--- a/drivers/pci/hotplug/pciehp.h
+++ b/drivers/pci/hotplug/pciehp.h
@@ -83,6 +83,7 @@ struct controller {
struct pcie_device *pcie;
 
u32 slot_cap;   /* capabilities and quirks */
+   unsigned int inband_presence_disabled:1;
 
u16 slot_ctrl;  /* control register access */
struct mutex ctrl_lock;
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 1a522c1c4177..dc109d521f30 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -811,7 +811,7 @@ static inline void dbg_ctrl(struct controller *ctrl)
 struct controller *pcie_init(struct pcie_device *dev)
 {
struct controller *ctrl;
-   u32 slot_cap, link_cap;
+   u32 slot_cap, slot_cap2, link_cap;
u8 poweron;
struct pci_dev *pdev = dev->port;
struct pci_bus *subordinate = pdev->subordinate;
@@ -869,6 +869,13 @@ struct controller *pcie_init(struct pcie_device *dev)
FLAG(link_cap, PCI_EXP_LNKCAP_DLLLARC),
pdev->broken_cmd_compl ? " (with Cmd Compl erratum)" : "");
 
+   pcie_capability_read_dword(pdev, PCI_EXP_SLTCAP2, _cap2);
+   if (slot_cap2 & PCI_EXP_SLTCAP2_IBPD) {
+   pcie_write_cmd_nowait(ctrl, PCI_EXP_SLTCTL_IBPD_DISABLE,
+ PCI_EXP_SLTCTL_IBPD_DISABLE);
+   ctrl->inband_presence_disabled = 1;
+   }
+
/*
 * If empty slot's power status is on, turn power off.  The IRQ isn't
 * requested yet, so avoid triggering a notification with this command.
diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index 29d6e93fd15e..ea1cf9546e4d 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -604,6 +604,7 @@
 #define  PCI_EXP_SLTCTL_PWR_OFF0x0400 /* Power Off */
 #define  PCI_EXP_SLTCTL_EIC0x0800  /* Electromechanical Interlock Control 
*/
 #define  PCI_EXP_SLTCTL_DLLSCE 0x1000  /* Data Link Layer State Changed Enable 
*/
+#define  PCI_EXP_SLTCTL_IBPD_DISABLE   0x4000 /* In-band PD disable */
 #define PCI_EXP_SLTSTA 26  /* Slot Status */
 #define  PCI_EXP_SLTSTA_ABP0x0001  /* Attention Button Pressed */
 #define  PCI_EXP_SLTSTA_PFD0x0002  /* Power Fault Detected */
@@ -676,6 +677,7 @@
 #define PCI_EXP_LNKSTA250  /* Link Status 2 */
 #define PCI_CAP_EXP_ENDPOINT_SIZEOF_V2 52  /* v2 endpoints with link end 
here */
 #define PCI_EXP_SLTCAP252  /* Slot Capabilities 2 */
+#define  PCI_EXP_SLTCAP2_IBPD  0x0001  /* In-band PD Disable Supported */
 #define PCI_EXP_SLTCTL256  /* Slot Control 2 */
 #define PCI_EXP_SLTSTA258  /* Slot Status 2 */
 
-- 
2.18.1

[PATCH 0/3] PCI: pciehp: Do not turn off slot if presence comes up after link

[Stuart Hayes]

In older PCIe specs, PDS (presence detect) would come up when the
"in-band" presence detect pin connected, and would be up before DLLLA
(link active).

In PCIe 4.0 (as an ECN) and in PCIe 5.0, there is a new bit to show if
in-band presence detection can be disabled for the slot, and another bit
that disables it--and a recommendation that it should be disabled if it
can be. In addition, certain OEMs disable in-band presence detection
without implementing these bits.

This means it is possible to get a "card present" interrupt after the
link is up and the driver is loaded.  This causes an erroneous removal
of the device driver, followed by an immediate re-probing.

This patch set defines these new bits, uses them to disable in-band
presence detection if it can be, waits for PDS to go up if in-band
presence detection is disabled, and adds a DMI table that will let us
know if we should assume in-band presence is disabled on a system.

The first two patches in this set come from a patch set that was
submitted but not accepted many months ago by Alexandru Gagniuc [1].
The first is unmodified, the second has the commit message and timeout 
modified.

[1] https://patchwork.kernel.org/cover/10909167/
[v3,0/4] PCI: pciehp: Do not turn off slot if presence comes up after link

Alexandru Gagniuc (2):
  PCI: pciehp: Add support for disabling in-band presence
  PCI: pciehp: Wait for PDS if in-band presence is disabled

Stuart Hayes (1):
  PCI: pciehp: Add dmi table for in-band presence disabled

 drivers/pci/hotplug/pciehp.h |  1 +
 drivers/pci/hotplug/pciehp_hpc.c | 45 +++-
 include/uapi/linux/pci_regs.h|  2 ++
 3 files changed, 47 insertions(+), 1 deletion(-)

-- 
2.18.1

Re: [PATCH 0/3] PCI: pciehp: Do not turn off slot if presence comes up after link

[Stuart Hayes]

On Tue, Oct 1, 2019 at 11:13 PM Lukas Wunner  wrote:
>
> On Tue, Oct 01, 2019 at 05:14:16PM -0400, Stuart Hayes wrote:
> > This patch set is based on a patch set [1] submitted many months ago by
> > Alexandru Gagniuc, who is no longer working on it.
> >
> > [1] https://patchwork.kernel.org/cover/10909167/
> > [v3,0/4] PCI: pciehp: Do not turn off slot if presence comes up after 
> > link
>
> If I'm not mistaken, these two are identical to Alex' patches, right?
>
>   PCI: pciehp: Add support for disabling in-band presence
>   PCI: pciehp: Wait for PDS when in-band presence is disabled
>
> I'm not sure if it's appropriate to change the author and
> omit Alex' Signed-off-by.
>
> Otherwise I have no objections against this series.
>
> Thanks,
>
> Lukas

Thanks!  The first patch is identical to the one Alex submitted, and
the second is nearly so... they both basically his work.  I wasn't
sure what proper etiquette was--I was thinking the signed-off-by was
taking responsibility that the patch was ok (functional, not
copyrighted by someone else, etc) rather than giving credit, but he
definitely deserves credit for them.  I'm happy to add a signed-off-by
for Alex on the first two and resubmit if he doesn't object.

Re: [PATCH 3/3] PCI: pciehp: Add dmi table for in-band presence disabled

[Stuart Hayes]

On Tue, Oct 1, 2019 at 4:36 PM Alex G.  wrote:
>
>
>
> On 10/1/19 4:14 PM, Stuart Hayes wrote:
> > Some systems have in-band presence detection disabled for hot-plug PCI 
> > slots,
> > but do not report this in the slot capabilities 2 (SLTCAP2) register.  On
> > these systems, presence detect can become active well after the link is
> > reported to be active, which can cause the slots to be disabled after a
> > device is connected.
> >
> > Add a dmi table to flag these systems as having in-band presence disabled.
> >
> > Signed-off-by: Stuart Hayes 
> > ---
> >   drivers/pci/hotplug/pciehp_hpc.c | 14 ++
> >   1 file changed, 14 insertions(+)
> >
> > diff --git a/drivers/pci/hotplug/pciehp_hpc.c 
> > b/drivers/pci/hotplug/pciehp_hpc.c
> > index 1282641c6458..1dd01e752587 100644
> > --- a/drivers/pci/hotplug/pciehp_hpc.c
> > +++ b/drivers/pci/hotplug/pciehp_hpc.c
> > @@ -14,6 +14,7 @@
> >
> >   #define dev_fmt(fmt) "pciehp: " fmt
> >
> > +#include 
> >   #include 
> >   #include 
> >   #include 
> > @@ -26,6 +27,16 @@
> >   #include "../pci.h"
> >   #include "pciehp.h"
> >
> > +static const struct dmi_system_id inband_presence_disabled_dmi_table[] = {
> > + {
> > + .ident = "Dell System",
> > + .matches = {
> > + DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc"),
> > + },
> > + },
> > + {}
> > +};
> > +
>
> I'm not sure that all Dell systems that were ever made or will be made
> have in-band presence disabled on all their hotplug slots.
>
> This was a problem with the NVMe hot-swap bays on 14G servers. I can't
> guarantee that any other slot or machine will need this workaround. The
> best way I found to implement this is to check for the PCI-ID of the
> switches behind the port.
>
> Alex

That is definitely true, not all Dell systems actually disable in-band
presence detect and need this.  However, we have a number of systems
that need this, and we don't have the PCI IDs for all of these yet, so
we decided it was preferable to just make all Dell systems wait for
presence detect to go active.  Since all of our systems support
presence detect (either in-band or out-of-band), it shouldn't have any
negative effects--on systems that support in-band presence, it will
already be active and it won't spend any extra time waiting for it.
If someone plugs in a device that has hot-plug slots with no support
for presence detect at all (even though Dell doesn't support any), it
should still work--it'll just take an extra second for them to come
up.

[PATCH 3/3] PCI: pciehp: Add dmi table for in-band presence disabled

[Stuart Hayes]

Some systems have in-band presence detection disabled for hot-plug PCI slots,
but do not report this in the slot capabilities 2 (SLTCAP2) register.  On
these systems, presence detect can become active well after the link is
reported to be active, which can cause the slots to be disabled after a
device is connected.

Add a dmi table to flag these systems as having in-band presence disabled.

Signed-off-by: Stuart Hayes 
---
 drivers/pci/hotplug/pciehp_hpc.c | 14 ++
 1 file changed, 14 insertions(+)

diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 1282641c6458..1dd01e752587 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -14,6 +14,7 @@
 
 #define dev_fmt(fmt) "pciehp: " fmt
 
+#include 
 #include 
 #include 
 #include 
@@ -26,6 +27,16 @@
 #include "../pci.h"
 #include "pciehp.h"
 
+static const struct dmi_system_id inband_presence_disabled_dmi_table[] = {
+   {
+   .ident = "Dell System",
+   .matches = {
+   DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc"),
+   },
+   },
+   {}
+};
+
 static inline struct pci_dev *ctrl_dev(struct controller *ctrl)
 {
return ctrl->pcie->port;
@@ -898,6 +909,9 @@ struct controller *pcie_init(struct pcie_device *dev)
ctrl->inband_presence_disabled = 1;
}
 
+   if (dmi_first_match(inband_presence_disabled_dmi_table))
+   ctrl->inband_presence_disabled = 1;
+
/*
 * If empty slot's power status is on, turn power off.  The IRQ isn't
 * requested yet, so avoid triggering a notification with this command.
-- 
2.18.1

[PATCH 0/3] PCI: pciehp: Do not turn off slot if presence comes up after link

[Stuart Hayes]

In older PCIe specs, PDS (presence detect) would come up when the
"in-band" presence detect pin connected, and would be up before DLLLA
(link active).

In PCIe 4.0 (as an ECN) and in PCIe 5.0, there is a new bit to show if
in-band presence detection can be disabled for the slot, and another bit
that disables it--and a recommendation that it should be disabled if it
can be. In addition, certain OEMs disable in-band presence detection
without implementing these bits.

This means it is possible to get a "card present" interrupt after the
link is up and the driver is loaded.  This causes an erroneous removal
of the device driver, followed by an immediate re-probing.

This patch set defines these new bits, uses them to disable in-band
presence detection if it can be, waits for PDS to go up if in-band
presence detection is disabled, and adds a DMI table that will let us
know if we should assume in-band presence is disabled on a system.

This patch set is based on a patch set [1] submitted many months ago by
Alexandru Gagniuc, who is no longer working on it.

[1] https://patchwork.kernel.org/cover/10909167/
[v3,0/4] PCI: pciehp: Do not turn off slot if presence comes up after link

Stuart Hayes (3):
  PCI: pciehp: Add support for disabling in-band presence
  PCI: pciehp: Wait for PDS when in-band presence is disabled
  PCI: pciehp: Add dmi table for systems with in-band presence disabled

 drivers/pci/hotplug/pciehp.h |  1 +
 drivers/pci/hotplug/pciehp_hpc.c | 45 +++-
 include/uapi/linux/pci_regs.h|  2 ++
 3 files changed, 47 insertions(+), 1 deletion(-)

-- 
2.18.1

[PATCH 2/3] PCI: pciehp: Wait for PDS if in-band presence is disabled

[Stuart Hayes]

When inband presence is disabled, PDS may come up at any time, or not
at all. PDS being low may indicate that the card is still mating, and
we could expect contact bounce to bring down the link as well.

It is reasonable to assume that most cards will mate in a hotplug slot
in about a second. Thus, when we know PDS only reflects out-of-band
presence, it's worthwhile to wait the extra second or so to make sure
the card is properly mated before loading the driver, and to prevent
the hotplug code from disabling a device if the presence detect change
goes active after the device is enabled.

Signed-off-by: Stuart Hayes 
---
 drivers/pci/hotplug/pciehp_hpc.c | 22 ++
 1 file changed, 22 insertions(+)

diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index dc109d521f30..1282641c6458 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -242,6 +242,25 @@ static bool pci_bus_check_dev(struct pci_bus *bus, int 
devfn)
return found;
 }
 
+static void pcie_wait_for_presence(struct pci_dev *pdev)
+{
+   int timeout = 1250;
+   bool pds;
+   u16 slot_status;
+
+   while (true) {
+   pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, _status);
+   pds = !!(slot_status & PCI_EXP_SLTSTA_PDS);
+   if (pds || timeout <= 0)
+   break;
+   msleep(10);
+   timeout -= 10;
+   }
+
+   if (!pds)
+   pci_info(pdev, "Presence Detect state not set in 1250 msec\n");
+}
+
 int pciehp_check_link_status(struct controller *ctrl)
 {
struct pci_dev *pdev = ctrl_dev(ctrl);
@@ -251,6 +270,9 @@ int pciehp_check_link_status(struct controller *ctrl)
if (!pcie_wait_for_link(pdev, true))
return -1;
 
+   if (ctrl->inband_presence_disabled)
+   pcie_wait_for_presence(pdev);
+
found = pci_bus_check_dev(ctrl->pcie->port->subordinate,
PCI_DEVFN(0, 0));
 
-- 
2.18.1

[PATCH 1/3] PCI: pciehp: Add support for disabling in-band presence

[Stuart Hayes]

The presence detect state (PDS) is normally a logical or of in-band and
out-of-band presence. As of PCIe 4.0, there is the option to disable
in-band presence so that the PDS bit always reflects the state of the
out-of-band presence.

The recommendation of the PCIe spec is to disable in-band presence
whenever supported.

Signed-off-by: Stuart Hayes 
---
 drivers/pci/hotplug/pciehp.h | 1 +
 drivers/pci/hotplug/pciehp_hpc.c | 9 -
 include/uapi/linux/pci_regs.h| 2 ++
 3 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
index 654c972b8ea0..27e4cd6529b0 100644
--- a/drivers/pci/hotplug/pciehp.h
+++ b/drivers/pci/hotplug/pciehp.h
@@ -83,6 +83,7 @@ struct controller {
struct pcie_device *pcie;
 
u32 slot_cap;   /* capabilities and quirks */
+   unsigned int inband_presence_disabled:1;
 
u16 slot_ctrl;  /* control register access */
struct mutex ctrl_lock;
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 1a522c1c4177..dc109d521f30 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -811,7 +811,7 @@ static inline void dbg_ctrl(struct controller *ctrl)
 struct controller *pcie_init(struct pcie_device *dev)
 {
struct controller *ctrl;
-   u32 slot_cap, link_cap;
+   u32 slot_cap, slot_cap2, link_cap;
u8 poweron;
struct pci_dev *pdev = dev->port;
struct pci_bus *subordinate = pdev->subordinate;
@@ -869,6 +869,13 @@ struct controller *pcie_init(struct pcie_device *dev)
FLAG(link_cap, PCI_EXP_LNKCAP_DLLLARC),
pdev->broken_cmd_compl ? " (with Cmd Compl erratum)" : "");
 
+   pcie_capability_read_dword(pdev, PCI_EXP_SLTCAP2, _cap2);
+   if (slot_cap2 & PCI_EXP_SLTCAP2_IBPD) {
+   pcie_write_cmd_nowait(ctrl, PCI_EXP_SLTCTL_IBPD_DISABLE,
+ PCI_EXP_SLTCTL_IBPD_DISABLE);
+   ctrl->inband_presence_disabled = 1;
+   }
+
/*
 * If empty slot's power status is on, turn power off.  The IRQ isn't
 * requested yet, so avoid triggering a notification with this command.
diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index 29d6e93fd15e..ea1cf9546e4d 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -604,6 +604,7 @@
 #define  PCI_EXP_SLTCTL_PWR_OFF0x0400 /* Power Off */
 #define  PCI_EXP_SLTCTL_EIC0x0800  /* Electromechanical Interlock Control 
*/
 #define  PCI_EXP_SLTCTL_DLLSCE 0x1000  /* Data Link Layer State Changed Enable 
*/
+#define  PCI_EXP_SLTCTL_IBPD_DISABLE   0x4000 /* In-band PD disable */
 #define PCI_EXP_SLTSTA 26  /* Slot Status */
 #define  PCI_EXP_SLTSTA_ABP0x0001  /* Attention Button Pressed */
 #define  PCI_EXP_SLTSTA_PFD0x0002  /* Power Fault Detected */
@@ -676,6 +677,7 @@
 #define PCI_EXP_LNKSTA250  /* Link Status 2 */
 #define PCI_CAP_EXP_ENDPOINT_SIZEOF_V2 52  /* v2 endpoints with link end 
here */
 #define PCI_EXP_SLTCAP252  /* Slot Capabilities 2 */
+#define  PCI_EXP_SLTCAP2_IBPD  0x0001  /* In-band PD Disable Supported */
 #define PCI_EXP_SLTCTL256  /* Slot Control 2 */
 #define PCI_EXP_SLTSTA258  /* Slot Status 2 */
 
-- 
2.18.1

Re: [PATCH] iommu/amd: Reserve exclusion range in iova-domain

[Stuart Hayes]

Tested on a Dell PowerEdge R7425 system on which this problem is easily 
reproducible.

Tested-by: Stuart Hayes 

Re: [PATCH] iommu/amd: Reserve exclusion range in iova-domain

[Stuart Hayes]

Tested on a Dell PowerEdge R7425 system on which this problem is easily 
reproducible.

Tested-by: Stuart Hayes 

Re: [PATCH] platform/x86: dell_rbu: fix lock imbalance in img_update_realloc

[Stuart Hayes]

On 2/11/2019 7:09 AM, Christoph Hellwig wrote:
> We need to ensure rbu_data.lock is always held on return.
> 
> Fixes: 289790a3ea94 ("platform/x86: dell_rbu: stop abusing the DMA API")
> Reported-by: Dan Carpenter 
> Signed-off-by: Christoph Hellwig 
> ---
>  drivers/platform/x86/dell_rbu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/dell_rbu.c b/drivers/platform/x86/dell_rbu.c
> index 8104ca0c44ca..031c68903583 100644
> --- a/drivers/platform/x86/dell_rbu.c
> +++ b/drivers/platform/x86/dell_rbu.c
> @@ -436,6 +436,7 @@ static int img_update_realloc(unsigned long size)
>   ordernum = get_order(size);
>   image_update_buffer =
>   (unsigned char *)__get_free_pages(GFP_DMA32, ordernum);
> + spin_lock(_data.lock);
>   if (!image_update_buffer) {
>   pr_debug("Not enough memory for image update:"
>   "size = %ld\n", size);
> @@ -446,7 +447,6 @@ static int img_update_realloc(unsigned long size)
>   if (WARN_ON_ONCE(img_buf_phys_addr > BIOS_SCAN_LIMIT))
>   return -EINVAL; /* can't happen per definition */
>  
> - spin_lock(_data.lock);
>   rbu_data.image_update_buffer = image_update_buffer;
>   rbu_data.image_update_buffer_size = size;
>   rbu_data.bios_image_size = rbu_data.image_update_buffer_size;
> 

Acked-by: Stuart Hayes 

Re: [PATCH] dell_rbu: stop abusing the DMA API

[Stuart Hayes]

On 1/29/2019 1:34 AM, Christoph Hellwig wrote:
> For some odd reason dell_rbu actually seems to want the physical and
> not a bus address for the allocated buffer.  Lets assume that actually
> is correct given that it is BIOS-related and that is a good source
> of insanity.  In that case we should not use dma_alloc_coherent with
> a NULL device to allocate memory, but use GFP_DMA32 to stay under
> the 32-bit BIOS limit.
> 
> Signed-off-by: Christoph Hellwig 
> ---
>  drivers/platform/x86/dell_rbu.c | 52 ++---
>  1 file changed, 15 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/platform/x86/dell_rbu.c b/drivers/platform/x86/dell_rbu.c
> index ccefa84f7305..fba7d96c1714 100644
> --- a/drivers/platform/x86/dell_rbu.c
> +++ b/drivers/platform/x86/dell_rbu.c
> @@ -59,7 +59,6 @@ static struct _rbu_data {
>   unsigned long image_update_buffer_size;
>   unsigned long bios_image_size;
>   int image_update_ordernum;
> - int dma_alloc;
>   spinlock_t lock;
>   unsigned long packet_read_count;
>   unsigned long num_packets;
> @@ -89,7 +88,6 @@ static struct packet_data packet_data_head;
>  
>  static struct platform_device *rbu_device;
>  static int context;
> -static dma_addr_t dell_rbu_dmaaddr;
>  
>  static void init_packet_head(void)
>  {
> @@ -380,12 +378,8 @@ static void img_update_free(void)
>*/
>   memset(rbu_data.image_update_buffer, 0,
>   rbu_data.image_update_buffer_size);
> - if (rbu_data.dma_alloc == 1)
> - dma_free_coherent(NULL, rbu_data.bios_image_size,
> - rbu_data.image_update_buffer, dell_rbu_dmaaddr);
> - else
> - free_pages((unsigned long) rbu_data.image_update_buffer,
> - rbu_data.image_update_ordernum);
> + free_pages((unsigned long) rbu_data.image_update_buffer,
> + rbu_data.image_update_ordernum);
>  
>   /*
>* Re-initialize the rbu_data variables after a free
> @@ -394,7 +388,6 @@ static void img_update_free(void)
>   rbu_data.image_update_buffer = NULL;
>   rbu_data.image_update_buffer_size = 0;
>   rbu_data.bios_image_size = 0;
> - rbu_data.dma_alloc = 0;
>  }
>  
>  /*
> @@ -410,10 +403,8 @@ static void img_update_free(void)
>  static int img_update_realloc(unsigned long size)
>  {
>   unsigned char *image_update_buffer = NULL;
> - unsigned long rc;
>   unsigned long img_buf_phys_addr;
>   int ordernum;
> - int dma_alloc = 0;
>  
>   /*
>* check if the buffer of sufficient size has been
> @@ -444,36 +435,23 @@ static int img_update_realloc(unsigned long size)
>  
>   ordernum = get_order(size);
>   image_update_buffer =
> - (unsigned char *) __get_free_pages(GFP_KERNEL, ordernum);
> -
> - img_buf_phys_addr =
> - (unsigned long) virt_to_phys(image_update_buffer);
> -
> - if (img_buf_phys_addr > BIOS_SCAN_LIMIT) {
> - free_pages((unsigned long) image_update_buffer, ordernum);
> - ordernum = -1;
> - image_update_buffer = dma_alloc_coherent(NULL, size,
> - _rbu_dmaaddr, GFP_KERNEL);
> - dma_alloc = 1;
> - }
> -
> - spin_lock(_data.lock);
> -
> - if (image_update_buffer != NULL) {
> - rbu_data.image_update_buffer = image_update_buffer;
> - rbu_data.image_update_buffer_size = size;
> - rbu_data.bios_image_size =
> - rbu_data.image_update_buffer_size;
> - rbu_data.image_update_ordernum = ordernum;
> - rbu_data.dma_alloc = dma_alloc;
> - rc = 0;
> - } else {
> + (unsigned char *)__get_free_pages(GFP_DMA32, ordernum);
> + if (!image_update_buffer) {
>   pr_debug("Not enough memory for image update:"
>   "size = %ld\n", size);
> - rc = -ENOMEM;
> + return -ENOMEM;
>   }
>  
> - return rc;
> + img_buf_phys_addr = (unsigned long)virt_to_phys(image_update_buffer);
> + if (WARN_ON_ONCE(img_buf_phys_addr > BIOS_SCAN_LIMIT))
> + return -EINVAL; /* can't happen per defintion */
> +
> + spin_lock(_data.lock);
> +     rbu_data.image_update_buffer = image_update_buffer;
> + rbu_data.image_update_buffer_size = size;
> + rbu_data.bios_image_size = rbu_data.image_update_buffer_size;
> + rbu_data.image_update_ordernum = ordernum;
> + return 0;
>  }
>  
>  static ssize_t read_packet_data(char *buffer, loff_t pos, size_t count)
> 

Acked-by: Stuart Hayes 

[PATCH v2 3/5] firmware: dell_rbu: Move dell_rbu to drivers/platform/x86

[Stuart Hayes]

Move dell_rbu to the more appropriate directory drivers/platform/x86.

Signed-off-by: Stuart Hayes 
---
v2 changes:
 - add commit message

 drivers/firmware/Kconfig  | 12 
 drivers/firmware/Makefile |  1 -
 drivers/platform/x86/Kconfig  | 12 
 drivers/platform/x86/Makefile |  1 +
 drivers/{firmware => platform/x86}/dell_rbu.c |  0
 5 files changed, 13 insertions(+), 13 deletions(-)
 rename drivers/{firmware => platform/x86}/dell_rbu.c (100%)

diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index 6e83880046d7..02f39d20efce 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -145,18 +145,6 @@ config EFI_PCDP
  See DIG64_HCDPv20_042804.pdf available from
  <http://www.dig64.org/specifications/> 
 
-config DELL_RBU
-   tristate "BIOS update support for DELL systems via sysfs"
-   depends on X86
-   select FW_LOADER
-   select FW_LOADER_USER_HELPER
-   help
-Say m if you want to have the option of updating the BIOS for your
-DELL system. Note you need a Dell OpenManage or Dell Update package 
(DUP)
-supporting application to communicate with the BIOS regarding the new
-image for the image update to take effect.
-See  for more details on the driver.
-
 config DCDBAS
tristate "Dell Systems Management Base Driver"
depends on X86
diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
index e18a041cfc53..61887ba9df1d 100644
--- a/drivers/firmware/Makefile
+++ b/drivers/firmware/Makefile
@@ -11,7 +11,6 @@ obj-$(CONFIG_DMI) += dmi_scan.o
 obj-$(CONFIG_DMI_SYSFS)+= dmi-sysfs.o
 obj-$(CONFIG_EDD)  += edd.o
 obj-$(CONFIG_EFI_PCDP) += pcdp.o
-obj-$(CONFIG_DELL_RBU)  += dell_rbu.o
 obj-$(CONFIG_DCDBAS)   += dcdbas.o
 obj-$(CONFIG_DMIID)+= dmi-id.o
 obj-$(CONFIG_ISCSI_IBFT_FIND)  += iscsi_ibft_find.o
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 0c1aa6c314f5..cb037da32107 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -227,6 +227,18 @@ config DELL_RBTN
  To compile this driver as a module, choose M here: the module will
  be called dell-rbtn.
 
+config DELL_RBU
+   tristate "BIOS update support for DELL systems via sysfs"
+   depends on X86
+   select FW_LOADER
+   select FW_LOADER_USER_HELPER
+   help
+Say m if you want to have the option of updating the BIOS for your
+DELL system. Note you need a Dell OpenManage or Dell Update package 
(DUP)
+supporting application to communicate with the BIOS regarding the new
+image for the image update to take effect.
+See  for more details on the driver.
+
 
 config FUJITSU_LAPTOP
tristate "Fujitsu Laptop Extras"
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index e6d1becf81ce..8843f8e22127 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -23,6 +23,7 @@ obj-$(CONFIG_DELL_WMI_AIO)+= dell-wmi-aio.o
 obj-$(CONFIG_DELL_WMI_LED) += dell-wmi-led.o
 obj-$(CONFIG_DELL_SMO8800) += dell-smo8800.o
 obj-$(CONFIG_DELL_RBTN)+= dell-rbtn.o
+obj-$(CONFIG_DELL_RBU)  += dell_rbu.o
 obj-$(CONFIG_ACER_WMI) += acer-wmi.o
 obj-$(CONFIG_ACER_WIRELESS)+= acer-wireless.o
 obj-$(CONFIG_ACERHDF)  += acerhdf.o
diff --git a/drivers/firmware/dell_rbu.c b/drivers/platform/x86/dell_rbu.c
similarity index 100%
rename from drivers/firmware/dell_rbu.c
rename to drivers/platform/x86/dell_rbu.c
-- 
2.19.0

[PATCH v2 4/5] firmware: dcdbas: Move dcdbas to drivers/platform/x86

[Stuart Hayes]

Move dcdbas to the more appropriate directory drivers/platform/x86.

Signed-off-by: Stuart Hayes 
---
v2 changes:
 - add commit message

 drivers/firmware/Kconfig| 16 
 drivers/firmware/Makefile   |  1 -
 drivers/platform/x86/Kconfig| 16 
 drivers/platform/x86/Makefile   |  1 +
 drivers/{firmware => platform/x86}/dcdbas.c |  0
 drivers/{firmware => platform/x86}/dcdbas.h |  0
 drivers/platform/x86/dell-smbios-smm.c  |  2 +-
 7 files changed, 18 insertions(+), 18 deletions(-)
 rename drivers/{firmware => platform/x86}/dcdbas.c (100%)
 rename drivers/{firmware => platform/x86}/dcdbas.h (100%)

diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index 02f39d20efce..6d0c28fd3bad 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -145,22 +145,6 @@ config EFI_PCDP
  See DIG64_HCDPv20_042804.pdf available from
  <http://www.dig64.org/specifications/> 
 
-config DCDBAS
-   tristate "Dell Systems Management Base Driver"
-   depends on X86
-   help
- The Dell Systems Management Base Driver provides a sysfs interface
- for systems management software to perform System Management
- Interrupts (SMIs) and Host Control Actions (system power cycle or
- power off after OS shutdown) on certain Dell systems.
-
- See  for more details on the driver
- and the Dell systems on which Dell systems management software makes
- use of this driver.
-
- Say Y or M here to enable the driver for use by Dell systems
- management software such as Dell OpenManage.
-
 config DMIID
 bool "Export DMI identification via sysfs to userspace"
 depends on DMI
diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
index 61887ba9df1d..edda4206d8fc 100644
--- a/drivers/firmware/Makefile
+++ b/drivers/firmware/Makefile
@@ -11,7 +11,6 @@ obj-$(CONFIG_DMI) += dmi_scan.o
 obj-$(CONFIG_DMI_SYSFS)+= dmi-sysfs.o
 obj-$(CONFIG_EDD)  += edd.o
 obj-$(CONFIG_EFI_PCDP) += pcdp.o
-obj-$(CONFIG_DCDBAS)   += dcdbas.o
 obj-$(CONFIG_DMIID)+= dmi-id.o
 obj-$(CONFIG_ISCSI_IBFT_FIND)  += iscsi_ibft_find.o
 obj-$(CONFIG_ISCSI_IBFT)   += iscsi_ibft.o
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index cb037da32107..1c7e553c28ce 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -105,6 +105,22 @@ config ASUS_LAPTOP
 
  If you have an ACPI-compatible ASUS laptop, say Y or M here.
 
+config DCDBAS
+   tristate "Dell Systems Management Base Driver"
+   depends on X86
+   help
+ The Dell Systems Management Base Driver provides a sysfs interface
+ for systems management software to perform System Management
+ Interrupts (SMIs) and Host Control Actions (system power cycle or
+ power off after OS shutdown) on certain Dell systems.
+
+ See  for more details on the driver
+ and the Dell systems on which Dell systems management software makes
+ use of this driver.
+
+ Say Y or M here to enable the driver for use by Dell systems
+ management software such as Dell OpenManage.
+
 #
 # The DELL_SMBIOS driver depends on ACPI_WMI and/or DCDBAS if those
 # backends are selected. The "depends" line prevents a configuration
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index 8843f8e22127..4e2712c9c0b0 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -12,6 +12,7 @@ obj-$(CONFIG_EEEPC_WMI)   += eeepc-wmi.o
 obj-$(CONFIG_MSI_LAPTOP)   += msi-laptop.o
 obj-$(CONFIG_ACPI_CMPC)+= classmate-laptop.o
 obj-$(CONFIG_COMPAL_LAPTOP)+= compal-laptop.o
+obj-$(CONFIG_DCDBAS)   += dcdbas.o
 obj-$(CONFIG_DELL_SMBIOS)  += dell-smbios.o
 dell-smbios-objs   := dell-smbios-base.o
 dell-smbios-$(CONFIG_DELL_SMBIOS_WMI)  += dell-smbios-wmi.o
diff --git a/drivers/firmware/dcdbas.c b/drivers/platform/x86/dcdbas.c
similarity index 100%
rename from drivers/firmware/dcdbas.c
rename to drivers/platform/x86/dcdbas.c
diff --git a/drivers/firmware/dcdbas.h b/drivers/platform/x86/dcdbas.h
similarity index 100%
rename from drivers/firmware/dcdbas.h
rename to drivers/platform/x86/dcdbas.h
diff --git a/drivers/platform/x86/dell-smbios-smm.c 
b/drivers/platform/x86/dell-smbios-smm.c
index 97a90bebc360..ab9b822a6dfe 100644
--- a/drivers/platform/x86/dell-smbios-smm.c
+++ b/drivers/platform/x86/dell-smbios-smm.c
@@ -18,7 +18,7 @@
 #include 
 #include 
 #include 
-#include "../../firmware/dcdbas.h"
+#include "dcdbas.h"
 #include "dell-smbios.h"
 
 static int da_command_address;
-- 
2.19.0

[PATCH v2 4/5] firmware: dcdbas: Move dcdbas to drivers/platform/x86

[Stuart Hayes]

Move dcdbas to the more appropriate directory drivers/platform/x86.

Signed-off-by: Stuart Hayes 
---
v2 changes:
 - add commit message

 drivers/firmware/Kconfig| 16 
 drivers/firmware/Makefile   |  1 -
 drivers/platform/x86/Kconfig| 16 
 drivers/platform/x86/Makefile   |  1 +
 drivers/{firmware => platform/x86}/dcdbas.c |  0
 drivers/{firmware => platform/x86}/dcdbas.h |  0
 drivers/platform/x86/dell-smbios-smm.c  |  2 +-
 7 files changed, 18 insertions(+), 18 deletions(-)
 rename drivers/{firmware => platform/x86}/dcdbas.c (100%)
 rename drivers/{firmware => platform/x86}/dcdbas.h (100%)

diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index 02f39d20efce..6d0c28fd3bad 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -145,22 +145,6 @@ config EFI_PCDP
  See DIG64_HCDPv20_042804.pdf available from
  <http://www.dig64.org/specifications/> 
 
-config DCDBAS
-   tristate "Dell Systems Management Base Driver"
-   depends on X86
-   help
- The Dell Systems Management Base Driver provides a sysfs interface
- for systems management software to perform System Management
- Interrupts (SMIs) and Host Control Actions (system power cycle or
- power off after OS shutdown) on certain Dell systems.
-
- See  for more details on the driver
- and the Dell systems on which Dell systems management software makes
- use of this driver.
-
- Say Y or M here to enable the driver for use by Dell systems
- management software such as Dell OpenManage.
-
 config DMIID
 bool "Export DMI identification via sysfs to userspace"
 depends on DMI
diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
index 61887ba9df1d..edda4206d8fc 100644
--- a/drivers/firmware/Makefile
+++ b/drivers/firmware/Makefile
@@ -11,7 +11,6 @@ obj-$(CONFIG_DMI) += dmi_scan.o
 obj-$(CONFIG_DMI_SYSFS)+= dmi-sysfs.o
 obj-$(CONFIG_EDD)  += edd.o
 obj-$(CONFIG_EFI_PCDP) += pcdp.o
-obj-$(CONFIG_DCDBAS)   += dcdbas.o
 obj-$(CONFIG_DMIID)+= dmi-id.o
 obj-$(CONFIG_ISCSI_IBFT_FIND)  += iscsi_ibft_find.o
 obj-$(CONFIG_ISCSI_IBFT)   += iscsi_ibft.o
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index cb037da32107..1c7e553c28ce 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -105,6 +105,22 @@ config ASUS_LAPTOP
 
  If you have an ACPI-compatible ASUS laptop, say Y or M here.
 
+config DCDBAS
+   tristate "Dell Systems Management Base Driver"
+   depends on X86
+   help
+ The Dell Systems Management Base Driver provides a sysfs interface
+ for systems management software to perform System Management
+ Interrupts (SMIs) and Host Control Actions (system power cycle or
+ power off after OS shutdown) on certain Dell systems.
+
+ See  for more details on the driver
+ and the Dell systems on which Dell systems management software makes
+ use of this driver.
+
+ Say Y or M here to enable the driver for use by Dell systems
+ management software such as Dell OpenManage.
+
 #
 # The DELL_SMBIOS driver depends on ACPI_WMI and/or DCDBAS if those
 # backends are selected. The "depends" line prevents a configuration
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index 8843f8e22127..4e2712c9c0b0 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -12,6 +12,7 @@ obj-$(CONFIG_EEEPC_WMI)   += eeepc-wmi.o
 obj-$(CONFIG_MSI_LAPTOP)   += msi-laptop.o
 obj-$(CONFIG_ACPI_CMPC)+= classmate-laptop.o
 obj-$(CONFIG_COMPAL_LAPTOP)+= compal-laptop.o
+obj-$(CONFIG_DCDBAS)   += dcdbas.o
 obj-$(CONFIG_DELL_SMBIOS)  += dell-smbios.o
 dell-smbios-objs   := dell-smbios-base.o
 dell-smbios-$(CONFIG_DELL_SMBIOS_WMI)  += dell-smbios-wmi.o
diff --git a/drivers/firmware/dcdbas.c b/drivers/platform/x86/dcdbas.c
similarity index 100%
rename from drivers/firmware/dcdbas.c
rename to drivers/platform/x86/dcdbas.c
diff --git a/drivers/firmware/dcdbas.h b/drivers/platform/x86/dcdbas.h
similarity index 100%
rename from drivers/firmware/dcdbas.h
rename to drivers/platform/x86/dcdbas.h
diff --git a/drivers/platform/x86/dell-smbios-smm.c 
b/drivers/platform/x86/dell-smbios-smm.c
index 97a90bebc360..ab9b822a6dfe 100644
--- a/drivers/platform/x86/dell-smbios-smm.c
+++ b/drivers/platform/x86/dell-smbios-smm.c
@@ -18,7 +18,7 @@
 #include 
 #include 
 #include 
-#include "../../firmware/dcdbas.h"
+#include "dcdbas.h"
 #include "dell-smbios.h"
 
 static int da_command_address;
-- 
2.19.0

[PATCH v2 3/5] firmware: dell_rbu: Move dell_rbu to drivers/platform/x86

[Stuart Hayes]

Move dell_rbu to the more appropriate directory drivers/platform/x86.

Signed-off-by: Stuart Hayes 
---
v2 changes:
 - add commit message

 drivers/firmware/Kconfig  | 12 
 drivers/firmware/Makefile |  1 -
 drivers/platform/x86/Kconfig  | 12 
 drivers/platform/x86/Makefile |  1 +
 drivers/{firmware => platform/x86}/dell_rbu.c |  0
 5 files changed, 13 insertions(+), 13 deletions(-)
 rename drivers/{firmware => platform/x86}/dell_rbu.c (100%)

diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index 6e83880046d7..02f39d20efce 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -145,18 +145,6 @@ config EFI_PCDP
  See DIG64_HCDPv20_042804.pdf available from
  <http://www.dig64.org/specifications/> 
 
-config DELL_RBU
-   tristate "BIOS update support for DELL systems via sysfs"
-   depends on X86
-   select FW_LOADER
-   select FW_LOADER_USER_HELPER
-   help
-Say m if you want to have the option of updating the BIOS for your
-DELL system. Note you need a Dell OpenManage or Dell Update package 
(DUP)
-supporting application to communicate with the BIOS regarding the new
-image for the image update to take effect.
-See  for more details on the driver.
-
 config DCDBAS
tristate "Dell Systems Management Base Driver"
depends on X86
diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
index e18a041cfc53..61887ba9df1d 100644
--- a/drivers/firmware/Makefile
+++ b/drivers/firmware/Makefile
@@ -11,7 +11,6 @@ obj-$(CONFIG_DMI) += dmi_scan.o
 obj-$(CONFIG_DMI_SYSFS)+= dmi-sysfs.o
 obj-$(CONFIG_EDD)  += edd.o
 obj-$(CONFIG_EFI_PCDP) += pcdp.o
-obj-$(CONFIG_DELL_RBU)  += dell_rbu.o
 obj-$(CONFIG_DCDBAS)   += dcdbas.o
 obj-$(CONFIG_DMIID)+= dmi-id.o
 obj-$(CONFIG_ISCSI_IBFT_FIND)  += iscsi_ibft_find.o
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 0c1aa6c314f5..cb037da32107 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -227,6 +227,18 @@ config DELL_RBTN
  To compile this driver as a module, choose M here: the module will
  be called dell-rbtn.
 
+config DELL_RBU
+   tristate "BIOS update support for DELL systems via sysfs"
+   depends on X86
+   select FW_LOADER
+   select FW_LOADER_USER_HELPER
+   help
+Say m if you want to have the option of updating the BIOS for your
+DELL system. Note you need a Dell OpenManage or Dell Update package 
(DUP)
+supporting application to communicate with the BIOS regarding the new
+image for the image update to take effect.
+See  for more details on the driver.
+
 
 config FUJITSU_LAPTOP
tristate "Fujitsu Laptop Extras"
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index e6d1becf81ce..8843f8e22127 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -23,6 +23,7 @@ obj-$(CONFIG_DELL_WMI_AIO)+= dell-wmi-aio.o
 obj-$(CONFIG_DELL_WMI_LED) += dell-wmi-led.o
 obj-$(CONFIG_DELL_SMO8800) += dell-smo8800.o
 obj-$(CONFIG_DELL_RBTN)+= dell-rbtn.o
+obj-$(CONFIG_DELL_RBU)  += dell_rbu.o
 obj-$(CONFIG_ACER_WMI) += acer-wmi.o
 obj-$(CONFIG_ACER_WIRELESS)+= acer-wireless.o
 obj-$(CONFIG_ACERHDF)  += acerhdf.o
diff --git a/drivers/firmware/dell_rbu.c b/drivers/platform/x86/dell_rbu.c
similarity index 100%
rename from drivers/firmware/dell_rbu.c
rename to drivers/platform/x86/dell_rbu.c
-- 
2.19.0

[PATCH v2 1/5] firmware: dell_rbu: Make payload memory uncachable

[Stuart Hayes]

The dell_rbu driver takes firmware update payloads and puts them in memory so
the system BIOS can find them after a reboot.  This sometimes fails (though
rarely), because the memory containing the payload is in the CPU cache but
never gets written back to main memory before the system is rebooted (CPU
cache contents are lost on reboot).

With this patch, the payload memory will be changed to uncachable to ensure
that the payload is actually in main memory before the system is rebooted.

Signed-off-by: Stuart Hayes 
---
This patch has been discussed previously, see history at
https://patchwork.kernel.org/patch/10512079/

 drivers/firmware/dell_rbu.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/drivers/firmware/dell_rbu.c b/drivers/firmware/dell_rbu.c
index fb8af5cb7c9b..ccefa84f7305 100644
--- a/drivers/firmware/dell_rbu.c
+++ b/drivers/firmware/dell_rbu.c
@@ -45,6 +45,7 @@
 #include 
 #include 
 #include 
+#include 
 
 MODULE_AUTHOR("Abhay Salunke ");
 MODULE_DESCRIPTION("Driver for updating BIOS image on DELL systems");
@@ -181,6 +182,11 @@ static int create_packet(void *data, size_t length)
packet_data_temp_buf = NULL;
}
}
+   /*
+* set to uncachable or it may never get written back before reboot
+*/
+   set_memory_uc((unsigned long)packet_data_temp_buf, 1 << ordernum);
+
spin_lock(_data.lock);
 
newpacket->data = packet_data_temp_buf;
@@ -349,6 +355,8 @@ static void packet_empty_list(void)
 * to make sure there are no stale RBU packets left in memory
 */
memset(newpacket->data, 0, rbu_data.packetsize);
+   set_memory_wb((unsigned long)newpacket->data,
+   1 << newpacket->ordernum);
free_pages((unsigned long) newpacket->data,
newpacket->ordernum);
kfree(newpacket);
-- 
2.19.0

[PATCH v2 1/5] firmware: dell_rbu: Make payload memory uncachable

[Stuart Hayes]

The dell_rbu driver takes firmware update payloads and puts them in memory so
the system BIOS can find them after a reboot.  This sometimes fails (though
rarely), because the memory containing the payload is in the CPU cache but
never gets written back to main memory before the system is rebooted (CPU
cache contents are lost on reboot).

With this patch, the payload memory will be changed to uncachable to ensure
that the payload is actually in main memory before the system is rebooted.

Signed-off-by: Stuart Hayes 
---
This patch has been discussed previously, see history at
https://patchwork.kernel.org/patch/10512079/

 drivers/firmware/dell_rbu.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/drivers/firmware/dell_rbu.c b/drivers/firmware/dell_rbu.c
index fb8af5cb7c9b..ccefa84f7305 100644
--- a/drivers/firmware/dell_rbu.c
+++ b/drivers/firmware/dell_rbu.c
@@ -45,6 +45,7 @@
 #include 
 #include 
 #include 
+#include 
 
 MODULE_AUTHOR("Abhay Salunke ");
 MODULE_DESCRIPTION("Driver for updating BIOS image on DELL systems");
@@ -181,6 +182,11 @@ static int create_packet(void *data, size_t length)
packet_data_temp_buf = NULL;
}
}
+   /*
+* set to uncachable or it may never get written back before reboot
+*/
+   set_memory_uc((unsigned long)packet_data_temp_buf, 1 << ordernum);
+
spin_lock(_data.lock);
 
newpacket->data = packet_data_temp_buf;
@@ -349,6 +355,8 @@ static void packet_empty_list(void)
 * to make sure there are no stale RBU packets left in memory
 */
memset(newpacket->data, 0, rbu_data.packetsize);
+   set_memory_wb((unsigned long)newpacket->data,
+   1 << newpacket->ordernum);
free_pages((unsigned long) newpacket->data,
newpacket->ordernum);
kfree(newpacket);
-- 
2.19.0

[PATCH v2 5/5] MAINTAINERS: Update maintainer for dcdbas and dell_rbu

[Stuart Hayes]

Assign maintainer for dell_rbu driver, and reassign maintainer of dcdbas
from inactive maintainer (current maintainer is aware of this change--
see https://www.spinics.net/lists/platform-driver-x86/msg16336.html).

Signed-off-by: Stuart Hayes 
Acked-by: Doug Warzecha 
---
v2 changes:
 - remove extra whitespace
 - add acked-by from (previous) maintainer of dcdbas

 MAINTAINERS | 11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 4ece30f15777..efb162a422a3 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4188,6 +4188,12 @@ M:   Pali Rohár 
 S: Maintained
 F: drivers/platform/x86/dell-rbtn.*
 
+DELL REMOTE BIOS UPDATE DRIVER
+M: Stuart Hayes 
+L: platform-driver-...@vger.kernel.org
+S: Maintained
+F: drivers/platform/x86/dell_rbu.c
+
 DELL LAPTOP SMM DRIVER
 M: Pali Rohár 
 S: Maintained
@@ -4195,10 +4201,11 @@ F:  drivers/hwmon/dell-smm-hwmon.c
 F: include/uapi/linux/i8k.h
 
 DELL SYSTEMS MANAGEMENT BASE DRIVER (dcdbas)
-M: Doug Warzecha 
+M: Stuart Hayes 
+L: platform-driver-...@vger.kernel.org
 S: Maintained
 F: Documentation/dcdbas.txt
-F: drivers/firmware/dcdbas.*
+F: drivers/platform/x86/dcdbas.*
 
 DELL WMI NOTIFICATIONS DRIVER
 M: Matthew Garrett 
-- 
2.19.0

[PATCH v2 5/5] MAINTAINERS: Update maintainer for dcdbas and dell_rbu

[Stuart Hayes]

Assign maintainer for dell_rbu driver, and reassign maintainer of dcdbas
from inactive maintainer (current maintainer is aware of this change--
see https://www.spinics.net/lists/platform-driver-x86/msg16336.html).

Signed-off-by: Stuart Hayes 
Acked-by: Doug Warzecha 
---
v2 changes:
 - remove extra whitespace
 - add acked-by from (previous) maintainer of dcdbas

 MAINTAINERS | 11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 4ece30f15777..efb162a422a3 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4188,6 +4188,12 @@ M:   Pali Rohár 
 S: Maintained
 F: drivers/platform/x86/dell-rbtn.*
 
+DELL REMOTE BIOS UPDATE DRIVER
+M: Stuart Hayes 
+L: platform-driver-...@vger.kernel.org
+S: Maintained
+F: drivers/platform/x86/dell_rbu.c
+
 DELL LAPTOP SMM DRIVER
 M: Pali Rohár 
 S: Maintained
@@ -4195,10 +4201,11 @@ F:  drivers/hwmon/dell-smm-hwmon.c
 F: include/uapi/linux/i8k.h
 
 DELL SYSTEMS MANAGEMENT BASE DRIVER (dcdbas)
-M: Doug Warzecha 
+M: Stuart Hayes 
+L: platform-driver-...@vger.kernel.org
 S: Maintained
 F: Documentation/dcdbas.txt
-F: drivers/firmware/dcdbas.*
+F: drivers/platform/x86/dcdbas.*
 
 DELL WMI NOTIFICATIONS DRIVER
 M: Matthew Garrett 
-- 
2.19.0

[PATCH v2 2/5] firmware: dcdbas: Add support for WSMT ACPI table

[Stuart Hayes]

If the WSMT ACPI table is present and indicates that a fixed communication
buffer should be used, use the firmware-specified buffer instead of
allocating a buffer in memory for communications between the dcdbas driver
and firmare.

Signed-off-by: Stuart Hayes 
---
This patch has been discussed previously, see history at
https://lore.kernel.org/patchwork/patch/958893/

 drivers/firmware/dcdbas.c | 123 --
 drivers/firmware/dcdbas.h |  10 
 2 files changed, 127 insertions(+), 6 deletions(-)

diff --git a/drivers/firmware/dcdbas.c b/drivers/firmware/dcdbas.c
index 0bdea60c65dd..ae28e48ff7dc 100644
--- a/drivers/firmware/dcdbas.c
+++ b/drivers/firmware/dcdbas.c
@@ -21,6 +21,7 @@
  */
 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -41,7 +42,7 @@
 #include "dcdbas.h"
 
 #define DRIVER_NAME"dcdbas"
-#define DRIVER_VERSION "5.6.0-3.2"
+#define DRIVER_VERSION "5.6.0-3.3"
 #define DRIVER_DESCRIPTION "Dell Systems Management Base Driver"
 
 static struct platform_device *dcdbas_pdev;
@@ -49,19 +50,23 @@ static struct platform_device *dcdbas_pdev;
 static u8 *smi_data_buf;
 static dma_addr_t smi_data_buf_handle;
 static unsigned long smi_data_buf_size;
+static unsigned long max_smi_data_buf_size = MAX_SMI_DATA_BUF_SIZE;
 static u32 smi_data_buf_phys_addr;
 static DEFINE_MUTEX(smi_data_lock);
+static u8 *eps_buffer;
 
 static unsigned int host_control_action;
 static unsigned int host_control_smi_type;
 static unsigned int host_control_on_shutdown;
 
+static bool wsmt_enabled;
+
 /**
  * smi_data_buf_free: free SMI data buffer
  */
 static void smi_data_buf_free(void)
 {
-   if (!smi_data_buf)
+   if (!smi_data_buf || wsmt_enabled)
return;
 
dev_dbg(_pdev->dev, "%s: phys: %x size: %lu\n",
@@ -86,7 +91,7 @@ static int smi_data_buf_realloc(unsigned long size)
if (smi_data_buf_size >= size)
return 0;
 
-   if (size > MAX_SMI_DATA_BUF_SIZE)
+   if (size > max_smi_data_buf_size)
return -EINVAL;
 
/* new buffer is needed */
@@ -169,7 +174,7 @@ static ssize_t smi_data_write(struct file *filp, struct 
kobject *kobj,
 {
ssize_t ret;
 
-   if ((pos + count) > MAX_SMI_DATA_BUF_SIZE)
+   if ((pos + count) > max_smi_data_buf_size)
return -EINVAL;
 
mutex_lock(_data_lock);
@@ -322,8 +327,20 @@ static ssize_t smi_request_store(struct device *dev,
ret = count;
break;
case 1:
-   /* Calling Interface SMI */
-   smi_cmd->ebx = (u32) virt_to_phys(smi_cmd->command_buffer);
+   /*
+* Calling Interface SMI
+*
+* Provide physical address of command buffer field within
+* the struct smi_cmd to BIOS.
+*
+* Because the address that smi_cmd (smi_data_buf) points to
+* will be from memremap() of a non-memory address if WSMT
+* is present, we can't use virt_to_phys() on smi_cmd, so
+* we have to use the physical address that was saved when
+* the virtual address for smi_cmd was received.
+*/
+   smi_cmd->ebx = smi_data_buf_phys_addr +
+   offsetof(struct smi_cmd, command_buffer);
ret = dcdbas_smi_request(smi_cmd);
if (!ret)
ret = count;
@@ -482,6 +499,93 @@ static void dcdbas_host_control(void)
}
 }
 
+/* WSMT */
+
+static u8 checksum(u8 *buffer, u8 length)
+{
+   u8 sum = 0;
+   u8 *end = buffer + length;
+
+   while (buffer < end)
+   sum += *buffer++;
+   return sum;
+}
+
+static inline struct smm_eps_table *check_eps_table(u8 *addr)
+{
+   struct smm_eps_table *eps = (struct smm_eps_table *)addr;
+
+   if (strncmp(eps->smm_comm_buff_anchor, SMM_EPS_SIG, 4) != 0)
+   return NULL;
+
+   if (checksum(addr, eps->length) != 0)
+   return NULL;
+
+   return eps;
+}
+
+static int dcdbas_check_wsmt(void)
+{
+   struct acpi_table_wsmt *wsmt = NULL;
+   struct smm_eps_table *eps = NULL;
+   u64 remap_size;
+   u8 *addr;
+
+   acpi_get_table(ACPI_SIG_WSMT, 0, (struct acpi_table_header **));
+   if (!wsmt)
+   return 0;
+
+   /* Check if WSMT ACPI table shows that protection is enabled */
+   if (!(wsmt->protection_flags & ACPI_WSMT_FIXED_COMM_BUFFERS) ||
+   !(wsmt->protection_flags & 
ACPI_WSMT_COMM_BUFFER_NESTED_PTR_PROTECTION))
+   return 0;
+
+   /* Scan for EPS (entry point structure) */
+   for (addr = (u8 *)__va(0xf);
+addr < (u8 *)__va(0x10 - sizeof(struct smm_eps_table));
+

[PATCH v2 0/5] Update and move Dell drivers dell_rbu and dcdbas

[Stuart Hayes]

The dell_rbu and dcdbas drivers need some changes, and should be moved to
drivers/platform/x86.  Additionally, dell_rbu needs a maintainer, and the
listed maintainer for dcdbas is inactive and needs to be changed.

v2 changes:
 - add commit messages to patches that move dcdbas and dell_rbu
 - remove extra whitespace in MAINTAINERS
 - add acked-by from (previous) maintainer of dcdbas

Stuart Hayes (5):
  firmware: dell_rbu: Make payload memory uncachable
  firmware: dcdbas: Add support for WSMT ACPI table
  firmware: dell_rbu: Move dell_rbu to drivers/platform/x86
  firmware: dcdbas: Move dcdbas to drivers/platform/x86
  MAINTAINERS: Update maintainer for dcdbas and dell_rbu

 MAINTAINERS   |  11 +-
 drivers/firmware/Kconfig  |  28 
 drivers/firmware/Makefile |   2 -
 drivers/platform/x86/Kconfig  |  28 
 drivers/platform/x86/Makefile |   2 +
 drivers/{firmware => platform/x86}/dcdbas.c   | 123 +-
 drivers/{firmware => platform/x86}/dcdbas.h   |  10 ++
 drivers/platform/x86/dell-smbios-smm.c|   2 +-
 drivers/{firmware => platform/x86}/dell_rbu.c |   8 ++
 9 files changed, 175 insertions(+), 39 deletions(-)
 rename drivers/{firmware => platform/x86}/dcdbas.c (82%)
 rename drivers/{firmware => platform/x86}/dcdbas.h (93%)
 rename drivers/{firmware => platform/x86}/dell_rbu.c (98%)

-- 
2.19.0.221.g150f307af

[PATCH v2 2/5] firmware: dcdbas: Add support for WSMT ACPI table

[Stuart Hayes]

If the WSMT ACPI table is present and indicates that a fixed communication
buffer should be used, use the firmware-specified buffer instead of
allocating a buffer in memory for communications between the dcdbas driver
and firmare.

Signed-off-by: Stuart Hayes 
---
This patch has been discussed previously, see history at
https://lore.kernel.org/patchwork/patch/958893/

 drivers/firmware/dcdbas.c | 123 --
 drivers/firmware/dcdbas.h |  10 
 2 files changed, 127 insertions(+), 6 deletions(-)

diff --git a/drivers/firmware/dcdbas.c b/drivers/firmware/dcdbas.c
index 0bdea60c65dd..ae28e48ff7dc 100644
--- a/drivers/firmware/dcdbas.c
+++ b/drivers/firmware/dcdbas.c
@@ -21,6 +21,7 @@
  */
 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -41,7 +42,7 @@
 #include "dcdbas.h"
 
 #define DRIVER_NAME"dcdbas"
-#define DRIVER_VERSION "5.6.0-3.2"
+#define DRIVER_VERSION "5.6.0-3.3"
 #define DRIVER_DESCRIPTION "Dell Systems Management Base Driver"
 
 static struct platform_device *dcdbas_pdev;
@@ -49,19 +50,23 @@ static struct platform_device *dcdbas_pdev;
 static u8 *smi_data_buf;
 static dma_addr_t smi_data_buf_handle;
 static unsigned long smi_data_buf_size;
+static unsigned long max_smi_data_buf_size = MAX_SMI_DATA_BUF_SIZE;
 static u32 smi_data_buf_phys_addr;
 static DEFINE_MUTEX(smi_data_lock);
+static u8 *eps_buffer;
 
 static unsigned int host_control_action;
 static unsigned int host_control_smi_type;
 static unsigned int host_control_on_shutdown;
 
+static bool wsmt_enabled;
+
 /**
  * smi_data_buf_free: free SMI data buffer
  */
 static void smi_data_buf_free(void)
 {
-   if (!smi_data_buf)
+   if (!smi_data_buf || wsmt_enabled)
return;
 
dev_dbg(_pdev->dev, "%s: phys: %x size: %lu\n",
@@ -86,7 +91,7 @@ static int smi_data_buf_realloc(unsigned long size)
if (smi_data_buf_size >= size)
return 0;
 
-   if (size > MAX_SMI_DATA_BUF_SIZE)
+   if (size > max_smi_data_buf_size)
return -EINVAL;
 
/* new buffer is needed */
@@ -169,7 +174,7 @@ static ssize_t smi_data_write(struct file *filp, struct 
kobject *kobj,
 {
ssize_t ret;
 
-   if ((pos + count) > MAX_SMI_DATA_BUF_SIZE)
+   if ((pos + count) > max_smi_data_buf_size)
return -EINVAL;
 
mutex_lock(_data_lock);
@@ -322,8 +327,20 @@ static ssize_t smi_request_store(struct device *dev,
ret = count;
break;
case 1:
-   /* Calling Interface SMI */
-   smi_cmd->ebx = (u32) virt_to_phys(smi_cmd->command_buffer);
+   /*
+* Calling Interface SMI
+*
+* Provide physical address of command buffer field within
+* the struct smi_cmd to BIOS.
+*
+* Because the address that smi_cmd (smi_data_buf) points to
+* will be from memremap() of a non-memory address if WSMT
+* is present, we can't use virt_to_phys() on smi_cmd, so
+* we have to use the physical address that was saved when
+* the virtual address for smi_cmd was received.
+*/
+   smi_cmd->ebx = smi_data_buf_phys_addr +
+   offsetof(struct smi_cmd, command_buffer);
ret = dcdbas_smi_request(smi_cmd);
if (!ret)
ret = count;
@@ -482,6 +499,93 @@ static void dcdbas_host_control(void)
}
 }
 
+/* WSMT */
+
+static u8 checksum(u8 *buffer, u8 length)
+{
+   u8 sum = 0;
+   u8 *end = buffer + length;
+
+   while (buffer < end)
+   sum += *buffer++;
+   return sum;
+}
+
+static inline struct smm_eps_table *check_eps_table(u8 *addr)
+{
+   struct smm_eps_table *eps = (struct smm_eps_table *)addr;
+
+   if (strncmp(eps->smm_comm_buff_anchor, SMM_EPS_SIG, 4) != 0)
+   return NULL;
+
+   if (checksum(addr, eps->length) != 0)
+   return NULL;
+
+   return eps;
+}
+
+static int dcdbas_check_wsmt(void)
+{
+   struct acpi_table_wsmt *wsmt = NULL;
+   struct smm_eps_table *eps = NULL;
+   u64 remap_size;
+   u8 *addr;
+
+   acpi_get_table(ACPI_SIG_WSMT, 0, (struct acpi_table_header **));
+   if (!wsmt)
+   return 0;
+
+   /* Check if WSMT ACPI table shows that protection is enabled */
+   if (!(wsmt->protection_flags & ACPI_WSMT_FIXED_COMM_BUFFERS) ||
+   !(wsmt->protection_flags & 
ACPI_WSMT_COMM_BUFFER_NESTED_PTR_PROTECTION))
+   return 0;
+
+   /* Scan for EPS (entry point structure) */
+   for (addr = (u8 *)__va(0xf);
+addr < (u8 *)__va(0x10 - sizeof(struct smm_eps_table));
+

[PATCH v2 0/5] Update and move Dell drivers dell_rbu and dcdbas

[Stuart Hayes]

The dell_rbu and dcdbas drivers need some changes, and should be moved to
drivers/platform/x86.  Additionally, dell_rbu needs a maintainer, and the
listed maintainer for dcdbas is inactive and needs to be changed.

v2 changes:
 - add commit messages to patches that move dcdbas and dell_rbu
 - remove extra whitespace in MAINTAINERS
 - add acked-by from (previous) maintainer of dcdbas

Stuart Hayes (5):
  firmware: dell_rbu: Make payload memory uncachable
  firmware: dcdbas: Add support for WSMT ACPI table
  firmware: dell_rbu: Move dell_rbu to drivers/platform/x86
  firmware: dcdbas: Move dcdbas to drivers/platform/x86
  MAINTAINERS: Update maintainer for dcdbas and dell_rbu

 MAINTAINERS   |  11 +-
 drivers/firmware/Kconfig  |  28 
 drivers/firmware/Makefile |   2 -
 drivers/platform/x86/Kconfig  |  28 
 drivers/platform/x86/Makefile |   2 +
 drivers/{firmware => platform/x86}/dcdbas.c   | 123 +-
 drivers/{firmware => platform/x86}/dcdbas.h   |  10 ++
 drivers/platform/x86/dell-smbios-smm.c|   2 +-
 drivers/{firmware => platform/x86}/dell_rbu.c |   8 ++
 9 files changed, 175 insertions(+), 39 deletions(-)
 rename drivers/{firmware => platform/x86}/dcdbas.c (82%)
 rename drivers/{firmware => platform/x86}/dcdbas.h (93%)
 rename drivers/{firmware => platform/x86}/dell_rbu.c (98%)

-- 
2.19.0.221.g150f307af

[PATCH 3/5] firmware: dell_rbu: Move dell_rbu to drivers/platform/x86

[Stuart Hayes]

From: Stuart Hayes 

Signed-off-by: Stuart Hayes 
---
 drivers/firmware/Kconfig  | 12 
 drivers/firmware/Makefile |  1 -
 drivers/platform/x86/Kconfig  | 12 
 drivers/platform/x86/Makefile |  1 +
 drivers/{firmware => platform/x86}/dell_rbu.c |  0
 5 files changed, 13 insertions(+), 13 deletions(-)
 rename drivers/{firmware => platform/x86}/dell_rbu.c (100%)

diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index 6e83880046d7..02f39d20efce 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -145,18 +145,6 @@ config EFI_PCDP
  See DIG64_HCDPv20_042804.pdf available from
  <http://www.dig64.org/specifications/> 
 
-config DELL_RBU
-   tristate "BIOS update support for DELL systems via sysfs"
-   depends on X86
-   select FW_LOADER
-   select FW_LOADER_USER_HELPER
-   help
-Say m if you want to have the option of updating the BIOS for your
-DELL system. Note you need a Dell OpenManage or Dell Update package 
(DUP)
-supporting application to communicate with the BIOS regarding the new
-image for the image update to take effect.
-See  for more details on the driver.
-
 config DCDBAS
tristate "Dell Systems Management Base Driver"
depends on X86
diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
index e18a041cfc53..61887ba9df1d 100644
--- a/drivers/firmware/Makefile
+++ b/drivers/firmware/Makefile
@@ -11,7 +11,6 @@ obj-$(CONFIG_DMI) += dmi_scan.o
 obj-$(CONFIG_DMI_SYSFS)+= dmi-sysfs.o
 obj-$(CONFIG_EDD)  += edd.o
 obj-$(CONFIG_EFI_PCDP) += pcdp.o
-obj-$(CONFIG_DELL_RBU)  += dell_rbu.o
 obj-$(CONFIG_DCDBAS)   += dcdbas.o
 obj-$(CONFIG_DMIID)+= dmi-id.o
 obj-$(CONFIG_ISCSI_IBFT_FIND)  += iscsi_ibft_find.o
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 0c1aa6c314f5..cb037da32107 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -227,6 +227,18 @@ config DELL_RBTN
  To compile this driver as a module, choose M here: the module will
  be called dell-rbtn.
 
+config DELL_RBU
+   tristate "BIOS update support for DELL systems via sysfs"
+   depends on X86
+   select FW_LOADER
+   select FW_LOADER_USER_HELPER
+   help
+Say m if you want to have the option of updating the BIOS for your
+DELL system. Note you need a Dell OpenManage or Dell Update package 
(DUP)
+supporting application to communicate with the BIOS regarding the new
+image for the image update to take effect.
+See  for more details on the driver.
+
 
 config FUJITSU_LAPTOP
tristate "Fujitsu Laptop Extras"
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index e6d1becf81ce..8843f8e22127 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -23,6 +23,7 @@ obj-$(CONFIG_DELL_WMI_AIO)+= dell-wmi-aio.o
 obj-$(CONFIG_DELL_WMI_LED) += dell-wmi-led.o
 obj-$(CONFIG_DELL_SMO8800) += dell-smo8800.o
 obj-$(CONFIG_DELL_RBTN)+= dell-rbtn.o
+obj-$(CONFIG_DELL_RBU)  += dell_rbu.o
 obj-$(CONFIG_ACER_WMI) += acer-wmi.o
 obj-$(CONFIG_ACER_WIRELESS)+= acer-wireless.o
 obj-$(CONFIG_ACERHDF)  += acerhdf.o
diff --git a/drivers/firmware/dell_rbu.c b/drivers/platform/x86/dell_rbu.c
similarity index 100%
rename from drivers/firmware/dell_rbu.c
rename to drivers/platform/x86/dell_rbu.c
-- 
2.19.0.221.g150f307af

[PATCH 2/5] firmware: dcdbas: Add support for WSMT ACPI table

[Stuart Hayes]

From: Stuart Hayes 

If the WSMT ACPI table is present and indicates that a fixed communication
buffer should be used, use the firmware-specified buffer instead of
allocating a buffer in memory for communications between the dcdbas driver
and firmare.

Signed-off-by: Stuart Hayes 
---
This patch has been discussed previously, see history at
https://lore.kernel.org/patchwork/patch/958893/


 drivers/firmware/dcdbas.c | 123 --
 drivers/firmware/dcdbas.h |  10 
 2 files changed, 127 insertions(+), 6 deletions(-)

diff --git a/drivers/firmware/dcdbas.c b/drivers/firmware/dcdbas.c
index 0bdea60c65dd..ae28e48ff7dc 100644
--- a/drivers/firmware/dcdbas.c
+++ b/drivers/firmware/dcdbas.c
@@ -21,6 +21,7 @@
  */
 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -41,7 +42,7 @@
 #include "dcdbas.h"
 
 #define DRIVER_NAME"dcdbas"
-#define DRIVER_VERSION "5.6.0-3.2"
+#define DRIVER_VERSION "5.6.0-3.3"
 #define DRIVER_DESCRIPTION "Dell Systems Management Base Driver"
 
 static struct platform_device *dcdbas_pdev;
@@ -49,19 +50,23 @@ static struct platform_device *dcdbas_pdev;
 static u8 *smi_data_buf;
 static dma_addr_t smi_data_buf_handle;
 static unsigned long smi_data_buf_size;
+static unsigned long max_smi_data_buf_size = MAX_SMI_DATA_BUF_SIZE;
 static u32 smi_data_buf_phys_addr;
 static DEFINE_MUTEX(smi_data_lock);
+static u8 *eps_buffer;
 
 static unsigned int host_control_action;
 static unsigned int host_control_smi_type;
 static unsigned int host_control_on_shutdown;
 
+static bool wsmt_enabled;
+
 /**
  * smi_data_buf_free: free SMI data buffer
  */
 static void smi_data_buf_free(void)
 {
-   if (!smi_data_buf)
+   if (!smi_data_buf || wsmt_enabled)
return;
 
dev_dbg(_pdev->dev, "%s: phys: %x size: %lu\n",
@@ -86,7 +91,7 @@ static int smi_data_buf_realloc(unsigned long size)
if (smi_data_buf_size >= size)
return 0;
 
-   if (size > MAX_SMI_DATA_BUF_SIZE)
+   if (size > max_smi_data_buf_size)
return -EINVAL;
 
/* new buffer is needed */
@@ -169,7 +174,7 @@ static ssize_t smi_data_write(struct file *filp, struct 
kobject *kobj,
 {
ssize_t ret;
 
-   if ((pos + count) > MAX_SMI_DATA_BUF_SIZE)
+   if ((pos + count) > max_smi_data_buf_size)
return -EINVAL;
 
mutex_lock(_data_lock);
@@ -322,8 +327,20 @@ static ssize_t smi_request_store(struct device *dev,
ret = count;
break;
case 1:
-   /* Calling Interface SMI */
-   smi_cmd->ebx = (u32) virt_to_phys(smi_cmd->command_buffer);
+   /*
+* Calling Interface SMI
+*
+* Provide physical address of command buffer field within
+* the struct smi_cmd to BIOS.
+*
+* Because the address that smi_cmd (smi_data_buf) points to
+* will be from memremap() of a non-memory address if WSMT
+* is present, we can't use virt_to_phys() on smi_cmd, so
+* we have to use the physical address that was saved when
+* the virtual address for smi_cmd was received.
+*/
+   smi_cmd->ebx = smi_data_buf_phys_addr +
+   offsetof(struct smi_cmd, command_buffer);
ret = dcdbas_smi_request(smi_cmd);
if (!ret)
ret = count;
@@ -482,6 +499,93 @@ static void dcdbas_host_control(void)
}
 }
 
+/* WSMT */
+
+static u8 checksum(u8 *buffer, u8 length)
+{
+   u8 sum = 0;
+   u8 *end = buffer + length;
+
+   while (buffer < end)
+   sum += *buffer++;
+   return sum;
+}
+
+static inline struct smm_eps_table *check_eps_table(u8 *addr)
+{
+   struct smm_eps_table *eps = (struct smm_eps_table *)addr;
+
+   if (strncmp(eps->smm_comm_buff_anchor, SMM_EPS_SIG, 4) != 0)
+   return NULL;
+
+   if (checksum(addr, eps->length) != 0)
+   return NULL;
+
+   return eps;
+}
+
+static int dcdbas_check_wsmt(void)
+{
+   struct acpi_table_wsmt *wsmt = NULL;
+   struct smm_eps_table *eps = NULL;
+   u64 remap_size;
+   u8 *addr;
+
+   acpi_get_table(ACPI_SIG_WSMT, 0, (struct acpi_table_header **));
+   if (!wsmt)
+   return 0;
+
+   /* Check if WSMT ACPI table shows that protection is enabled */
+   if (!(wsmt->protection_flags & ACPI_WSMT_FIXED_COMM_BUFFERS) ||
+   !(wsmt->protection_flags & 
ACPI_WSMT_COMM_BUFFER_NESTED_PTR_PROTECTION))
+   return 0;
+
+   /* Scan for EPS (entry point structure) */
+   for (addr = (u8 *)__va(0xf);
+addr < (u8 *)__va(0x10 - sizeof(struct smm_eps_table));
+

[PATCH 0/5] Update and move Dell drivers dell_rbu and dcdbas

[Stuart Hayes]

From: Stuart Hayes 

The dell_rbu and dcdbas drivers need some changes, and should be moved to
drivers/platform/x86.  Additionally, dell_rbu needs a maintainer, and the
listed maintainer for dcdbas is inactive and needs to be changed.

Stuart Hayes (5):
  firmware: dell_rbu: Make payload memory uncachable
  firmware: dcdbas: Add support for WSMT ACPI table
  firmware: dell_rbu: Move dell_rbu to drivers/platform/x86
  firmware: dcdbas: Move dcdbas to drivers/platform/x86
  MAINTAINERS: Update maintainer for dcdbas and dell_rbu

 MAINTAINERS   |  11 +-
 drivers/firmware/Kconfig  |  28 
 drivers/firmware/Makefile |   2 -
 drivers/platform/x86/Kconfig  |  28 
 drivers/platform/x86/Makefile |   2 +
 drivers/{firmware => platform/x86}/dcdbas.c   | 123 +-
 drivers/{firmware => platform/x86}/dcdbas.h   |  10 ++
 drivers/platform/x86/dell-smbios-smm.c|   2 +-
 drivers/{firmware => platform/x86}/dell_rbu.c |   8 ++
 9 files changed, 175 insertions(+), 39 deletions(-)
 rename drivers/{firmware => platform/x86}/dcdbas.c (82%)
 rename drivers/{firmware => platform/x86}/dcdbas.h (93%)
 rename drivers/{firmware => platform/x86}/dell_rbu.c (98%)

-- 
2.19.0.221.g150f307af

[PATCH 5/5] MAINTAINERS: Update maintainer for dcdbas and dell_rbu

[Stuart Hayes]

From: Stuart Hayes 

Assign maintainer for dell_rbu driver, and reassign maintainer of dcdbas
from inactive maintainer (current maintainer is aware of this change--
see https://www.spinics.net/lists/platform-driver-x86/msg16336.html).

Signed-off-by: Stuart Hayes 
---
 MAINTAINERS | 11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 4ece30f15777..455b3fe6954e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4188,6 +4188,12 @@ M:   Pali Rohár 
 S: Maintained
 F: drivers/platform/x86/dell-rbtn.*
 
+DELL REMOTE BIOS UPDATE DRIVER
+M: Stuart Hayes  
+L: platform-driver-...@vger.kernel.org
+S: Maintained
+F: drivers/platform/x86/dell_rbu.c
+
 DELL LAPTOP SMM DRIVER
 M: Pali Rohár 
 S: Maintained
@@ -4195,10 +4201,11 @@ F:  drivers/hwmon/dell-smm-hwmon.c
 F: include/uapi/linux/i8k.h
 
 DELL SYSTEMS MANAGEMENT BASE DRIVER (dcdbas)
-M: Doug Warzecha 
+M: Stuart Hayes 
+L: platform-driver-...@vger.kernel.org
 S: Maintained
 F: Documentation/dcdbas.txt
-F: drivers/firmware/dcdbas.*
+F: drivers/platform/x86/dcdbas.*
 
 DELL WMI NOTIFICATIONS DRIVER
 M: Matthew Garrett 
-- 
2.19.0.221.g150f307af

[PATCH 1/5] firmware: dell_rbu: Make payload memory uncachable

[Stuart Hayes]

From: Stuart Hayes 

The dell_rbu driver takes firmware update payloads and puts them in memory so
the system BIOS can find them after a reboot.  This sometimes fails (though
rarely), because the memory containing the payload is in the CPU cache but
never gets written back to main memory before the system is rebooted (CPU
cache contents are lost on reboot).

With this patch, the payload memory will be changed to uncachable to ensure
that the payload is actually in main memory before the system is rebooted.

Signed-off-by: Stuart Hayes 
---
This patch has been discussed previously, see history at
https://patchwork.kernel.org/patch/10512079/


 drivers/firmware/dell_rbu.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/drivers/firmware/dell_rbu.c b/drivers/firmware/dell_rbu.c
index fb8af5cb7c9b..ccefa84f7305 100644
--- a/drivers/firmware/dell_rbu.c
+++ b/drivers/firmware/dell_rbu.c
@@ -45,6 +45,7 @@
 #include 
 #include 
 #include 
+#include 
 
 MODULE_AUTHOR("Abhay Salunke ");
 MODULE_DESCRIPTION("Driver for updating BIOS image on DELL systems");
@@ -181,6 +182,11 @@ static int create_packet(void *data, size_t length)
packet_data_temp_buf = NULL;
}
}
+   /*
+* set to uncachable or it may never get written back before reboot
+*/
+   set_memory_uc((unsigned long)packet_data_temp_buf, 1 << ordernum);
+
spin_lock(_data.lock);
 
newpacket->data = packet_data_temp_buf;
@@ -349,6 +355,8 @@ static void packet_empty_list(void)
 * to make sure there are no stale RBU packets left in memory
 */
memset(newpacket->data, 0, rbu_data.packetsize);
+   set_memory_wb((unsigned long)newpacket->data,
+   1 << newpacket->ordernum);
free_pages((unsigned long) newpacket->data,
newpacket->ordernum);
kfree(newpacket);
-- 
2.19.0.221.g150f307af

[PATCH 3/5] firmware: dell_rbu: Move dell_rbu to drivers/platform/x86

[Stuart Hayes]

From: Stuart Hayes 

Signed-off-by: Stuart Hayes 
---
 drivers/firmware/Kconfig  | 12 
 drivers/firmware/Makefile |  1 -
 drivers/platform/x86/Kconfig  | 12 
 drivers/platform/x86/Makefile |  1 +
 drivers/{firmware => platform/x86}/dell_rbu.c |  0
 5 files changed, 13 insertions(+), 13 deletions(-)
 rename drivers/{firmware => platform/x86}/dell_rbu.c (100%)

diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index 6e83880046d7..02f39d20efce 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -145,18 +145,6 @@ config EFI_PCDP
  See DIG64_HCDPv20_042804.pdf available from
  <http://www.dig64.org/specifications/> 
 
-config DELL_RBU
-   tristate "BIOS update support for DELL systems via sysfs"
-   depends on X86
-   select FW_LOADER
-   select FW_LOADER_USER_HELPER
-   help
-Say m if you want to have the option of updating the BIOS for your
-DELL system. Note you need a Dell OpenManage or Dell Update package 
(DUP)
-supporting application to communicate with the BIOS regarding the new
-image for the image update to take effect.
-See  for more details on the driver.
-
 config DCDBAS
tristate "Dell Systems Management Base Driver"
depends on X86
diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
index e18a041cfc53..61887ba9df1d 100644
--- a/drivers/firmware/Makefile
+++ b/drivers/firmware/Makefile
@@ -11,7 +11,6 @@ obj-$(CONFIG_DMI) += dmi_scan.o
 obj-$(CONFIG_DMI_SYSFS)+= dmi-sysfs.o
 obj-$(CONFIG_EDD)  += edd.o
 obj-$(CONFIG_EFI_PCDP) += pcdp.o
-obj-$(CONFIG_DELL_RBU)  += dell_rbu.o
 obj-$(CONFIG_DCDBAS)   += dcdbas.o
 obj-$(CONFIG_DMIID)+= dmi-id.o
 obj-$(CONFIG_ISCSI_IBFT_FIND)  += iscsi_ibft_find.o
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 0c1aa6c314f5..cb037da32107 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -227,6 +227,18 @@ config DELL_RBTN
  To compile this driver as a module, choose M here: the module will
  be called dell-rbtn.
 
+config DELL_RBU
+   tristate "BIOS update support for DELL systems via sysfs"
+   depends on X86
+   select FW_LOADER
+   select FW_LOADER_USER_HELPER
+   help
+Say m if you want to have the option of updating the BIOS for your
+DELL system. Note you need a Dell OpenManage or Dell Update package 
(DUP)
+supporting application to communicate with the BIOS regarding the new
+image for the image update to take effect.
+See  for more details on the driver.
+
 
 config FUJITSU_LAPTOP
tristate "Fujitsu Laptop Extras"
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index e6d1becf81ce..8843f8e22127 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -23,6 +23,7 @@ obj-$(CONFIG_DELL_WMI_AIO)+= dell-wmi-aio.o
 obj-$(CONFIG_DELL_WMI_LED) += dell-wmi-led.o
 obj-$(CONFIG_DELL_SMO8800) += dell-smo8800.o
 obj-$(CONFIG_DELL_RBTN)+= dell-rbtn.o
+obj-$(CONFIG_DELL_RBU)  += dell_rbu.o
 obj-$(CONFIG_ACER_WMI) += acer-wmi.o
 obj-$(CONFIG_ACER_WIRELESS)+= acer-wireless.o
 obj-$(CONFIG_ACERHDF)  += acerhdf.o
diff --git a/drivers/firmware/dell_rbu.c b/drivers/platform/x86/dell_rbu.c
similarity index 100%
rename from drivers/firmware/dell_rbu.c
rename to drivers/platform/x86/dell_rbu.c
-- 
2.19.0.221.g150f307af

[PATCH 2/5] firmware: dcdbas: Add support for WSMT ACPI table

[Stuart Hayes]

From: Stuart Hayes 

If the WSMT ACPI table is present and indicates that a fixed communication
buffer should be used, use the firmware-specified buffer instead of
allocating a buffer in memory for communications between the dcdbas driver
and firmare.

Signed-off-by: Stuart Hayes 
---
This patch has been discussed previously, see history at
https://lore.kernel.org/patchwork/patch/958893/


 drivers/firmware/dcdbas.c | 123 --
 drivers/firmware/dcdbas.h |  10 
 2 files changed, 127 insertions(+), 6 deletions(-)

diff --git a/drivers/firmware/dcdbas.c b/drivers/firmware/dcdbas.c
index 0bdea60c65dd..ae28e48ff7dc 100644
--- a/drivers/firmware/dcdbas.c
+++ b/drivers/firmware/dcdbas.c
@@ -21,6 +21,7 @@
  */
 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -41,7 +42,7 @@
 #include "dcdbas.h"
 
 #define DRIVER_NAME"dcdbas"
-#define DRIVER_VERSION "5.6.0-3.2"
+#define DRIVER_VERSION "5.6.0-3.3"
 #define DRIVER_DESCRIPTION "Dell Systems Management Base Driver"
 
 static struct platform_device *dcdbas_pdev;
@@ -49,19 +50,23 @@ static struct platform_device *dcdbas_pdev;
 static u8 *smi_data_buf;
 static dma_addr_t smi_data_buf_handle;
 static unsigned long smi_data_buf_size;
+static unsigned long max_smi_data_buf_size = MAX_SMI_DATA_BUF_SIZE;
 static u32 smi_data_buf_phys_addr;
 static DEFINE_MUTEX(smi_data_lock);
+static u8 *eps_buffer;
 
 static unsigned int host_control_action;
 static unsigned int host_control_smi_type;
 static unsigned int host_control_on_shutdown;
 
+static bool wsmt_enabled;
+
 /**
  * smi_data_buf_free: free SMI data buffer
  */
 static void smi_data_buf_free(void)
 {
-   if (!smi_data_buf)
+   if (!smi_data_buf || wsmt_enabled)
return;
 
dev_dbg(_pdev->dev, "%s: phys: %x size: %lu\n",
@@ -86,7 +91,7 @@ static int smi_data_buf_realloc(unsigned long size)
if (smi_data_buf_size >= size)
return 0;
 
-   if (size > MAX_SMI_DATA_BUF_SIZE)
+   if (size > max_smi_data_buf_size)
return -EINVAL;
 
/* new buffer is needed */
@@ -169,7 +174,7 @@ static ssize_t smi_data_write(struct file *filp, struct 
kobject *kobj,
 {
ssize_t ret;
 
-   if ((pos + count) > MAX_SMI_DATA_BUF_SIZE)
+   if ((pos + count) > max_smi_data_buf_size)
return -EINVAL;
 
mutex_lock(_data_lock);
@@ -322,8 +327,20 @@ static ssize_t smi_request_store(struct device *dev,
ret = count;
break;
case 1:
-   /* Calling Interface SMI */
-   smi_cmd->ebx = (u32) virt_to_phys(smi_cmd->command_buffer);
+   /*
+* Calling Interface SMI
+*
+* Provide physical address of command buffer field within
+* the struct smi_cmd to BIOS.
+*
+* Because the address that smi_cmd (smi_data_buf) points to
+* will be from memremap() of a non-memory address if WSMT
+* is present, we can't use virt_to_phys() on smi_cmd, so
+* we have to use the physical address that was saved when
+* the virtual address for smi_cmd was received.
+*/
+   smi_cmd->ebx = smi_data_buf_phys_addr +
+   offsetof(struct smi_cmd, command_buffer);
ret = dcdbas_smi_request(smi_cmd);
if (!ret)
ret = count;
@@ -482,6 +499,93 @@ static void dcdbas_host_control(void)
}
 }
 
+/* WSMT */
+
+static u8 checksum(u8 *buffer, u8 length)
+{
+   u8 sum = 0;
+   u8 *end = buffer + length;
+
+   while (buffer < end)
+   sum += *buffer++;
+   return sum;
+}
+
+static inline struct smm_eps_table *check_eps_table(u8 *addr)
+{
+   struct smm_eps_table *eps = (struct smm_eps_table *)addr;
+
+   if (strncmp(eps->smm_comm_buff_anchor, SMM_EPS_SIG, 4) != 0)
+   return NULL;
+
+   if (checksum(addr, eps->length) != 0)
+   return NULL;
+
+   return eps;
+}
+
+static int dcdbas_check_wsmt(void)
+{
+   struct acpi_table_wsmt *wsmt = NULL;
+   struct smm_eps_table *eps = NULL;
+   u64 remap_size;
+   u8 *addr;
+
+   acpi_get_table(ACPI_SIG_WSMT, 0, (struct acpi_table_header **));
+   if (!wsmt)
+   return 0;
+
+   /* Check if WSMT ACPI table shows that protection is enabled */
+   if (!(wsmt->protection_flags & ACPI_WSMT_FIXED_COMM_BUFFERS) ||
+   !(wsmt->protection_flags & 
ACPI_WSMT_COMM_BUFFER_NESTED_PTR_PROTECTION))
+   return 0;
+
+   /* Scan for EPS (entry point structure) */
+   for (addr = (u8 *)__va(0xf);
+addr < (u8 *)__va(0x10 - sizeof(struct smm_eps_table));
+

[PATCH 0/5] Update and move Dell drivers dell_rbu and dcdbas

[Stuart Hayes]

From: Stuart Hayes 

The dell_rbu and dcdbas drivers need some changes, and should be moved to
drivers/platform/x86.  Additionally, dell_rbu needs a maintainer, and the
listed maintainer for dcdbas is inactive and needs to be changed.

Stuart Hayes (5):
  firmware: dell_rbu: Make payload memory uncachable
  firmware: dcdbas: Add support for WSMT ACPI table
  firmware: dell_rbu: Move dell_rbu to drivers/platform/x86
  firmware: dcdbas: Move dcdbas to drivers/platform/x86
  MAINTAINERS: Update maintainer for dcdbas and dell_rbu

 MAINTAINERS   |  11 +-
 drivers/firmware/Kconfig  |  28 
 drivers/firmware/Makefile |   2 -
 drivers/platform/x86/Kconfig  |  28 
 drivers/platform/x86/Makefile |   2 +
 drivers/{firmware => platform/x86}/dcdbas.c   | 123 +-
 drivers/{firmware => platform/x86}/dcdbas.h   |  10 ++
 drivers/platform/x86/dell-smbios-smm.c|   2 +-
 drivers/{firmware => platform/x86}/dell_rbu.c |   8 ++
 9 files changed, 175 insertions(+), 39 deletions(-)
 rename drivers/{firmware => platform/x86}/dcdbas.c (82%)
 rename drivers/{firmware => platform/x86}/dcdbas.h (93%)
 rename drivers/{firmware => platform/x86}/dell_rbu.c (98%)

-- 
2.19.0.221.g150f307af

[PATCH 5/5] MAINTAINERS: Update maintainer for dcdbas and dell_rbu

[Stuart Hayes]

From: Stuart Hayes 

Assign maintainer for dell_rbu driver, and reassign maintainer of dcdbas
from inactive maintainer (current maintainer is aware of this change--
see https://www.spinics.net/lists/platform-driver-x86/msg16336.html).

Signed-off-by: Stuart Hayes 
---
 MAINTAINERS | 11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 4ece30f15777..455b3fe6954e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4188,6 +4188,12 @@ M:   Pali Rohár 
 S: Maintained
 F: drivers/platform/x86/dell-rbtn.*
 
+DELL REMOTE BIOS UPDATE DRIVER
+M: Stuart Hayes  
+L: platform-driver-...@vger.kernel.org
+S: Maintained
+F: drivers/platform/x86/dell_rbu.c
+
 DELL LAPTOP SMM DRIVER
 M: Pali Rohár 
 S: Maintained
@@ -4195,10 +4201,11 @@ F:  drivers/hwmon/dell-smm-hwmon.c
 F: include/uapi/linux/i8k.h
 
 DELL SYSTEMS MANAGEMENT BASE DRIVER (dcdbas)
-M: Doug Warzecha 
+M: Stuart Hayes 
+L: platform-driver-...@vger.kernel.org
 S: Maintained
 F: Documentation/dcdbas.txt
-F: drivers/firmware/dcdbas.*
+F: drivers/platform/x86/dcdbas.*
 
 DELL WMI NOTIFICATIONS DRIVER
 M: Matthew Garrett 
-- 
2.19.0.221.g150f307af

[PATCH 1/5] firmware: dell_rbu: Make payload memory uncachable

[Stuart Hayes]

From: Stuart Hayes 

The dell_rbu driver takes firmware update payloads and puts them in memory so
the system BIOS can find them after a reboot.  This sometimes fails (though
rarely), because the memory containing the payload is in the CPU cache but
never gets written back to main memory before the system is rebooted (CPU
cache contents are lost on reboot).

With this patch, the payload memory will be changed to uncachable to ensure
that the payload is actually in main memory before the system is rebooted.

Signed-off-by: Stuart Hayes 
---
This patch has been discussed previously, see history at
https://patchwork.kernel.org/patch/10512079/


 drivers/firmware/dell_rbu.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/drivers/firmware/dell_rbu.c b/drivers/firmware/dell_rbu.c
index fb8af5cb7c9b..ccefa84f7305 100644
--- a/drivers/firmware/dell_rbu.c
+++ b/drivers/firmware/dell_rbu.c
@@ -45,6 +45,7 @@
 #include 
 #include 
 #include 
+#include 
 
 MODULE_AUTHOR("Abhay Salunke ");
 MODULE_DESCRIPTION("Driver for updating BIOS image on DELL systems");
@@ -181,6 +182,11 @@ static int create_packet(void *data, size_t length)
packet_data_temp_buf = NULL;
}
}
+   /*
+* set to uncachable or it may never get written back before reboot
+*/
+   set_memory_uc((unsigned long)packet_data_temp_buf, 1 << ordernum);
+
spin_lock(_data.lock);
 
newpacket->data = packet_data_temp_buf;
@@ -349,6 +355,8 @@ static void packet_empty_list(void)
 * to make sure there are no stale RBU packets left in memory
 */
memset(newpacket->data, 0, rbu_data.packetsize);
+   set_memory_wb((unsigned long)newpacket->data,
+   1 << newpacket->ordernum);
free_pages((unsigned long) newpacket->data,
newpacket->ordernum);
kfree(newpacket);
-- 
2.19.0.221.g150f307af

[PATCH 4/5] firmware: dcdbas: Move dcdbas to drivers/platform/x86

[Stuart Hayes]

From: Stuart Hayes 

Signed-off-by: Stuart Hayes 
---
 drivers/firmware/Kconfig| 16 
 drivers/firmware/Makefile   |  1 -
 drivers/platform/x86/Kconfig| 16 
 drivers/platform/x86/Makefile   |  1 +
 drivers/{firmware => platform/x86}/dcdbas.c |  0
 drivers/{firmware => platform/x86}/dcdbas.h |  0
 drivers/platform/x86/dell-smbios-smm.c  |  2 +-
 7 files changed, 18 insertions(+), 18 deletions(-)
 rename drivers/{firmware => platform/x86}/dcdbas.c (100%)
 rename drivers/{firmware => platform/x86}/dcdbas.h (100%)

diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index 02f39d20efce..6d0c28fd3bad 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -145,22 +145,6 @@ config EFI_PCDP
  See DIG64_HCDPv20_042804.pdf available from
  <http://www.dig64.org/specifications/> 
 
-config DCDBAS
-   tristate "Dell Systems Management Base Driver"
-   depends on X86
-   help
- The Dell Systems Management Base Driver provides a sysfs interface
- for systems management software to perform System Management
- Interrupts (SMIs) and Host Control Actions (system power cycle or
- power off after OS shutdown) on certain Dell systems.
-
- See  for more details on the driver
- and the Dell systems on which Dell systems management software makes
- use of this driver.
-
- Say Y or M here to enable the driver for use by Dell systems
- management software such as Dell OpenManage.
-
 config DMIID
 bool "Export DMI identification via sysfs to userspace"
 depends on DMI
diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
index 61887ba9df1d..edda4206d8fc 100644
--- a/drivers/firmware/Makefile
+++ b/drivers/firmware/Makefile
@@ -11,7 +11,6 @@ obj-$(CONFIG_DMI) += dmi_scan.o
 obj-$(CONFIG_DMI_SYSFS)+= dmi-sysfs.o
 obj-$(CONFIG_EDD)  += edd.o
 obj-$(CONFIG_EFI_PCDP) += pcdp.o
-obj-$(CONFIG_DCDBAS)   += dcdbas.o
 obj-$(CONFIG_DMIID)+= dmi-id.o
 obj-$(CONFIG_ISCSI_IBFT_FIND)  += iscsi_ibft_find.o
 obj-$(CONFIG_ISCSI_IBFT)   += iscsi_ibft.o
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index cb037da32107..1c7e553c28ce 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -105,6 +105,22 @@ config ASUS_LAPTOP
 
  If you have an ACPI-compatible ASUS laptop, say Y or M here.
 
+config DCDBAS
+   tristate "Dell Systems Management Base Driver"
+   depends on X86
+   help
+ The Dell Systems Management Base Driver provides a sysfs interface
+ for systems management software to perform System Management
+ Interrupts (SMIs) and Host Control Actions (system power cycle or
+ power off after OS shutdown) on certain Dell systems.
+
+ See  for more details on the driver
+ and the Dell systems on which Dell systems management software makes
+ use of this driver.
+
+ Say Y or M here to enable the driver for use by Dell systems
+ management software such as Dell OpenManage.
+
 #
 # The DELL_SMBIOS driver depends on ACPI_WMI and/or DCDBAS if those
 # backends are selected. The "depends" line prevents a configuration
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index 8843f8e22127..4e2712c9c0b0 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -12,6 +12,7 @@ obj-$(CONFIG_EEEPC_WMI)   += eeepc-wmi.o
 obj-$(CONFIG_MSI_LAPTOP)   += msi-laptop.o
 obj-$(CONFIG_ACPI_CMPC)+= classmate-laptop.o
 obj-$(CONFIG_COMPAL_LAPTOP)+= compal-laptop.o
+obj-$(CONFIG_DCDBAS)   += dcdbas.o
 obj-$(CONFIG_DELL_SMBIOS)  += dell-smbios.o
 dell-smbios-objs   := dell-smbios-base.o
 dell-smbios-$(CONFIG_DELL_SMBIOS_WMI)  += dell-smbios-wmi.o
diff --git a/drivers/firmware/dcdbas.c b/drivers/platform/x86/dcdbas.c
similarity index 100%
rename from drivers/firmware/dcdbas.c
rename to drivers/platform/x86/dcdbas.c
diff --git a/drivers/firmware/dcdbas.h b/drivers/platform/x86/dcdbas.h
similarity index 100%
rename from drivers/firmware/dcdbas.h
rename to drivers/platform/x86/dcdbas.h
diff --git a/drivers/platform/x86/dell-smbios-smm.c 
b/drivers/platform/x86/dell-smbios-smm.c
index 97a90bebc360..ab9b822a6dfe 100644
--- a/drivers/platform/x86/dell-smbios-smm.c
+++ b/drivers/platform/x86/dell-smbios-smm.c
@@ -18,7 +18,7 @@
 #include 
 #include 
 #include 
-#include "../../firmware/dcdbas.h"
+#include "dcdbas.h"
 #include "dell-smbios.h"
 
 static int da_command_address;
-- 
2.19.0.221.g150f307af

[PATCH 4/5] firmware: dcdbas: Move dcdbas to drivers/platform/x86

[Stuart Hayes]

From: Stuart Hayes 

Signed-off-by: Stuart Hayes 
---
 drivers/firmware/Kconfig| 16 
 drivers/firmware/Makefile   |  1 -
 drivers/platform/x86/Kconfig| 16 
 drivers/platform/x86/Makefile   |  1 +
 drivers/{firmware => platform/x86}/dcdbas.c |  0
 drivers/{firmware => platform/x86}/dcdbas.h |  0
 drivers/platform/x86/dell-smbios-smm.c  |  2 +-
 7 files changed, 18 insertions(+), 18 deletions(-)
 rename drivers/{firmware => platform/x86}/dcdbas.c (100%)
 rename drivers/{firmware => platform/x86}/dcdbas.h (100%)

diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index 02f39d20efce..6d0c28fd3bad 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -145,22 +145,6 @@ config EFI_PCDP
  See DIG64_HCDPv20_042804.pdf available from
  <http://www.dig64.org/specifications/> 
 
-config DCDBAS
-   tristate "Dell Systems Management Base Driver"
-   depends on X86
-   help
- The Dell Systems Management Base Driver provides a sysfs interface
- for systems management software to perform System Management
- Interrupts (SMIs) and Host Control Actions (system power cycle or
- power off after OS shutdown) on certain Dell systems.
-
- See  for more details on the driver
- and the Dell systems on which Dell systems management software makes
- use of this driver.
-
- Say Y or M here to enable the driver for use by Dell systems
- management software such as Dell OpenManage.
-
 config DMIID
 bool "Export DMI identification via sysfs to userspace"
 depends on DMI
diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
index 61887ba9df1d..edda4206d8fc 100644
--- a/drivers/firmware/Makefile
+++ b/drivers/firmware/Makefile
@@ -11,7 +11,6 @@ obj-$(CONFIG_DMI) += dmi_scan.o
 obj-$(CONFIG_DMI_SYSFS)+= dmi-sysfs.o
 obj-$(CONFIG_EDD)  += edd.o
 obj-$(CONFIG_EFI_PCDP) += pcdp.o
-obj-$(CONFIG_DCDBAS)   += dcdbas.o
 obj-$(CONFIG_DMIID)+= dmi-id.o
 obj-$(CONFIG_ISCSI_IBFT_FIND)  += iscsi_ibft_find.o
 obj-$(CONFIG_ISCSI_IBFT)   += iscsi_ibft.o
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index cb037da32107..1c7e553c28ce 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -105,6 +105,22 @@ config ASUS_LAPTOP
 
  If you have an ACPI-compatible ASUS laptop, say Y or M here.
 
+config DCDBAS
+   tristate "Dell Systems Management Base Driver"
+   depends on X86
+   help
+ The Dell Systems Management Base Driver provides a sysfs interface
+ for systems management software to perform System Management
+ Interrupts (SMIs) and Host Control Actions (system power cycle or
+ power off after OS shutdown) on certain Dell systems.
+
+ See  for more details on the driver
+ and the Dell systems on which Dell systems management software makes
+ use of this driver.
+
+ Say Y or M here to enable the driver for use by Dell systems
+ management software such as Dell OpenManage.
+
 #
 # The DELL_SMBIOS driver depends on ACPI_WMI and/or DCDBAS if those
 # backends are selected. The "depends" line prevents a configuration
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index 8843f8e22127..4e2712c9c0b0 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -12,6 +12,7 @@ obj-$(CONFIG_EEEPC_WMI)   += eeepc-wmi.o
 obj-$(CONFIG_MSI_LAPTOP)   += msi-laptop.o
 obj-$(CONFIG_ACPI_CMPC)+= classmate-laptop.o
 obj-$(CONFIG_COMPAL_LAPTOP)+= compal-laptop.o
+obj-$(CONFIG_DCDBAS)   += dcdbas.o
 obj-$(CONFIG_DELL_SMBIOS)  += dell-smbios.o
 dell-smbios-objs   := dell-smbios-base.o
 dell-smbios-$(CONFIG_DELL_SMBIOS_WMI)  += dell-smbios-wmi.o
diff --git a/drivers/firmware/dcdbas.c b/drivers/platform/x86/dcdbas.c
similarity index 100%
rename from drivers/firmware/dcdbas.c
rename to drivers/platform/x86/dcdbas.c
diff --git a/drivers/firmware/dcdbas.h b/drivers/platform/x86/dcdbas.h
similarity index 100%
rename from drivers/firmware/dcdbas.h
rename to drivers/platform/x86/dcdbas.h
diff --git a/drivers/platform/x86/dell-smbios-smm.c 
b/drivers/platform/x86/dell-smbios-smm.c
index 97a90bebc360..ab9b822a6dfe 100644
--- a/drivers/platform/x86/dell-smbios-smm.c
+++ b/drivers/platform/x86/dell-smbios-smm.c
@@ -18,7 +18,7 @@
 #include 
 #include 
 #include 
-#include "../../firmware/dcdbas.h"
+#include "dcdbas.h"
 #include "dell-smbios.h"
 
 static int da_command_address;
-- 
2.19.0.221.g150f307af

[PATCH resend v4] dell_rbu: make firmware payload memory uncachable

[Stuart Hayes]

The dell_rbu driver takes firmware update payloads and puts them in memory so
the system BIOS can find them after a reboot.  This sometimes fails (though
rarely), because the memory containing the payload is in the CPU cache but
never gets written back to main memory before the system is rebooted (CPU
cache contents are lost on reboot).

With this patch, the payload memory will be changed to uncachable to ensure
that the payload is actually in main memory before the system is rebooted.

Signed-off-by: Stuart Hayes 
Reviewed-by: Takashi Iwai 
Signed-off-by: Takashi Iwai 
---
v2 Added include, removed extra parentheses
v3 Corrected formatting and include line
v4 Moved set_memory_uc() outside the while loop so that the memory is
   definitely allocated before it is set to uncachable

This driver has no maintainer.


diff --git a/drivers/firmware/dell_rbu.c b/drivers/firmware/dell_rbu.c
index 2f452f1f7c8a..53f27a6e2d76 100644
--- a/drivers/firmware/dell_rbu.c
+++ b/drivers/firmware/dell_rbu.c
@@ -45,6 +45,7 @@
 #include 
 #include 
 #include 
+#include 
 
 MODULE_AUTHOR("Abhay Salunke ");
 MODULE_DESCRIPTION("Driver for updating BIOS image on DELL systems");
@@ -181,6 +182,11 @@ static int create_packet(void *data, size_t length)
packet_data_temp_buf = NULL;
}
}
+   /*
+* set to uncachable or it may never get written back before reboot
+*/
+   set_memory_uc((unsigned long)packet_data_temp_buf, 1 << ordernum);
+
spin_lock(_data.lock);
 
newpacket->data = packet_data_temp_buf;
@@ -349,6 +355,8 @@ static void packet_empty_list(void)
 * to make sure there are no stale RBU packets left in memory
 */
memset(newpacket->data, 0, rbu_data.packetsize);
+   set_memory_wb((unsigned long)newpacket->data,
+   1 << newpacket->ordernum);
free_pages((unsigned long) newpacket->data,
newpacket->ordernum);
kfree(newpacket);

[PATCH resend v4] dell_rbu: make firmware payload memory uncachable

[Stuart Hayes]

The dell_rbu driver takes firmware update payloads and puts them in memory so
the system BIOS can find them after a reboot.  This sometimes fails (though
rarely), because the memory containing the payload is in the CPU cache but
never gets written back to main memory before the system is rebooted (CPU
cache contents are lost on reboot).

With this patch, the payload memory will be changed to uncachable to ensure
that the payload is actually in main memory before the system is rebooted.

Signed-off-by: Stuart Hayes 
Reviewed-by: Takashi Iwai 
Signed-off-by: Takashi Iwai 
---
v2 Added include, removed extra parentheses
v3 Corrected formatting and include line
v4 Moved set_memory_uc() outside the while loop so that the memory is
   definitely allocated before it is set to uncachable

This driver has no maintainer.


diff --git a/drivers/firmware/dell_rbu.c b/drivers/firmware/dell_rbu.c
index 2f452f1f7c8a..53f27a6e2d76 100644
--- a/drivers/firmware/dell_rbu.c
+++ b/drivers/firmware/dell_rbu.c
@@ -45,6 +45,7 @@
 #include 
 #include 
 #include 
+#include 
 
 MODULE_AUTHOR("Abhay Salunke ");
 MODULE_DESCRIPTION("Driver for updating BIOS image on DELL systems");
@@ -181,6 +182,11 @@ static int create_packet(void *data, size_t length)
packet_data_temp_buf = NULL;
}
}
+   /*
+* set to uncachable or it may never get written back before reboot
+*/
+   set_memory_uc((unsigned long)packet_data_temp_buf, 1 << ordernum);
+
spin_lock(_data.lock);
 
newpacket->data = packet_data_temp_buf;
@@ -349,6 +355,8 @@ static void packet_empty_list(void)
 * to make sure there are no stale RBU packets left in memory
 */
memset(newpacket->data, 0, rbu_data.packetsize);
+   set_memory_wb((unsigned long)newpacket->data,
+   1 << newpacket->ordernum);
free_pages((unsigned long) newpacket->data,
newpacket->ordernum);
kfree(newpacket);

[PATCH v5] dcdbas: Add support for WSMT ACPI table

[Stuart Hayes]

If the WSMT ACPI table is present and indicates that a fixed communication
buffer should be used, use the firmware-specified buffer instead of
allocating a buffer in memory for communications between the dcdbas driver
and firmare.

Signed-off-by: Stuart Hayes 
---
v2 Bumped driver version to 5.6.0-3.3
v3 Removed dependency on ACPI in Kconfig
   Moved the added #include to be in alphabetical order
   Added comments in smi_request_store()
   Simplified checksum code
   Changed loop searching 0xf to be more readable
   Reworked calculation of remap_size & smi_data_buf_size
v4 Fixed comment that starts with "Calling Interface SMI"
   Fixed formatting of first "if" statement in dcdbas_check_wsmt()
v5 Reworked comment that starts with "Calling Interface SMI"
   Changed EPS scanning loop to check every 16 bytes


 drivers/firmware/dcdbas.c | 123 +++---
 drivers/firmware/dcdbas.h |  10 
 2 files changed, 127 insertions(+), 6 deletions(-)

diff --git a/drivers/firmware/dcdbas.c b/drivers/firmware/dcdbas.c
index 0bdea60c65dd..ae28e48ff7dc 100644
--- a/drivers/firmware/dcdbas.c
+++ b/drivers/firmware/dcdbas.c
@@ -21,6 +21,7 @@
  */
 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -41,7 +42,7 @@
 #include "dcdbas.h"
 
 #define DRIVER_NAME"dcdbas"
-#define DRIVER_VERSION "5.6.0-3.2"
+#define DRIVER_VERSION "5.6.0-3.3"
 #define DRIVER_DESCRIPTION "Dell Systems Management Base Driver"
 
 static struct platform_device *dcdbas_pdev;
@@ -49,19 +50,23 @@ static struct platform_device *dcdbas_pdev;
 static u8 *smi_data_buf;
 static dma_addr_t smi_data_buf_handle;
 static unsigned long smi_data_buf_size;
+static unsigned long max_smi_data_buf_size = MAX_SMI_DATA_BUF_SIZE;
 static u32 smi_data_buf_phys_addr;
 static DEFINE_MUTEX(smi_data_lock);
+static u8 *eps_buffer;
 
 static unsigned int host_control_action;
 static unsigned int host_control_smi_type;
 static unsigned int host_control_on_shutdown;
 
+static bool wsmt_enabled;
+
 /**
  * smi_data_buf_free: free SMI data buffer
  */
 static void smi_data_buf_free(void)
 {
-   if (!smi_data_buf)
+   if (!smi_data_buf || wsmt_enabled)
return;
 
dev_dbg(_pdev->dev, "%s: phys: %x size: %lu\n",
@@ -86,7 +91,7 @@ static int smi_data_buf_realloc(unsigned long size)
if (smi_data_buf_size >= size)
return 0;
 
-   if (size > MAX_SMI_DATA_BUF_SIZE)
+   if (size > max_smi_data_buf_size)
return -EINVAL;
 
/* new buffer is needed */
@@ -169,7 +174,7 @@ static ssize_t smi_data_write(struct file *filp, struct 
kobject *kobj,
 {
ssize_t ret;
 
-   if ((pos + count) > MAX_SMI_DATA_BUF_SIZE)
+   if ((pos + count) > max_smi_data_buf_size)
return -EINVAL;
 
mutex_lock(_data_lock);
@@ -322,8 +327,20 @@ static ssize_t smi_request_store(struct device *dev,
ret = count;
break;
case 1:
-   /* Calling Interface SMI */
-   smi_cmd->ebx = (u32) virt_to_phys(smi_cmd->command_buffer);
+   /*
+* Calling Interface SMI
+*
+* Provide physical address of command buffer field within
+* the struct smi_cmd to BIOS.
+*
+* Because the address that smi_cmd (smi_data_buf) points to
+* will be from memremap() of a non-memory address if WSMT
+* is present, we can't use virt_to_phys() on smi_cmd, so
+* we have to use the physical address that was saved when
+* the virtual address for smi_cmd was received.
+*/
+   smi_cmd->ebx = smi_data_buf_phys_addr +
+   offsetof(struct smi_cmd, command_buffer);
ret = dcdbas_smi_request(smi_cmd);
if (!ret)
ret = count;
@@ -482,6 +499,93 @@ static void dcdbas_host_control(void)
}
 }
 
+/* WSMT */
+
+static u8 checksum(u8 *buffer, u8 length)
+{
+   u8 sum = 0;
+   u8 *end = buffer + length;
+
+   while (buffer < end)
+   sum += *buffer++;
+   return sum;
+}
+
+static inline struct smm_eps_table *check_eps_table(u8 *addr)
+{
+   struct smm_eps_table *eps = (struct smm_eps_table *)addr;
+
+   if (strncmp(eps->smm_comm_buff_anchor, SMM_EPS_SIG, 4) != 0)
+   return NULL;
+
+   if (checksum(addr, eps->length) != 0)
+   return NULL;
+
+   return eps;
+}
+
+static int dcdbas_check_wsmt(void)
+{
+   struct acpi_table_wsmt *wsmt = NULL;
+   struct smm_eps_table *eps = NULL;
+   u64 remap_size;
+   u8 *addr;
+
+   acpi_get_table(ACPI_SIG_WSMT, 0, (struct acpi_table_header **));
+   if (!wsmt)
+ 

[PATCH v5] dcdbas: Add support for WSMT ACPI table

[Stuart Hayes]

If the WSMT ACPI table is present and indicates that a fixed communication
buffer should be used, use the firmware-specified buffer instead of
allocating a buffer in memory for communications between the dcdbas driver
and firmare.

Signed-off-by: Stuart Hayes 
---
v2 Bumped driver version to 5.6.0-3.3
v3 Removed dependency on ACPI in Kconfig
   Moved the added #include to be in alphabetical order
   Added comments in smi_request_store()
   Simplified checksum code
   Changed loop searching 0xf to be more readable
   Reworked calculation of remap_size & smi_data_buf_size
v4 Fixed comment that starts with "Calling Interface SMI"
   Fixed formatting of first "if" statement in dcdbas_check_wsmt()
v5 Reworked comment that starts with "Calling Interface SMI"
   Changed EPS scanning loop to check every 16 bytes


 drivers/firmware/dcdbas.c | 123 +++---
 drivers/firmware/dcdbas.h |  10 
 2 files changed, 127 insertions(+), 6 deletions(-)

diff --git a/drivers/firmware/dcdbas.c b/drivers/firmware/dcdbas.c
index 0bdea60c65dd..ae28e48ff7dc 100644
--- a/drivers/firmware/dcdbas.c
+++ b/drivers/firmware/dcdbas.c
@@ -21,6 +21,7 @@
  */
 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -41,7 +42,7 @@
 #include "dcdbas.h"
 
 #define DRIVER_NAME"dcdbas"
-#define DRIVER_VERSION "5.6.0-3.2"
+#define DRIVER_VERSION "5.6.0-3.3"
 #define DRIVER_DESCRIPTION "Dell Systems Management Base Driver"
 
 static struct platform_device *dcdbas_pdev;
@@ -49,19 +50,23 @@ static struct platform_device *dcdbas_pdev;
 static u8 *smi_data_buf;
 static dma_addr_t smi_data_buf_handle;
 static unsigned long smi_data_buf_size;
+static unsigned long max_smi_data_buf_size = MAX_SMI_DATA_BUF_SIZE;
 static u32 smi_data_buf_phys_addr;
 static DEFINE_MUTEX(smi_data_lock);
+static u8 *eps_buffer;
 
 static unsigned int host_control_action;
 static unsigned int host_control_smi_type;
 static unsigned int host_control_on_shutdown;
 
+static bool wsmt_enabled;
+
 /**
  * smi_data_buf_free: free SMI data buffer
  */
 static void smi_data_buf_free(void)
 {
-   if (!smi_data_buf)
+   if (!smi_data_buf || wsmt_enabled)
return;
 
dev_dbg(_pdev->dev, "%s: phys: %x size: %lu\n",
@@ -86,7 +91,7 @@ static int smi_data_buf_realloc(unsigned long size)
if (smi_data_buf_size >= size)
return 0;
 
-   if (size > MAX_SMI_DATA_BUF_SIZE)
+   if (size > max_smi_data_buf_size)
return -EINVAL;
 
/* new buffer is needed */
@@ -169,7 +174,7 @@ static ssize_t smi_data_write(struct file *filp, struct 
kobject *kobj,
 {
ssize_t ret;
 
-   if ((pos + count) > MAX_SMI_DATA_BUF_SIZE)
+   if ((pos + count) > max_smi_data_buf_size)
return -EINVAL;
 
mutex_lock(_data_lock);
@@ -322,8 +327,20 @@ static ssize_t smi_request_store(struct device *dev,
ret = count;
break;
case 1:
-   /* Calling Interface SMI */
-   smi_cmd->ebx = (u32) virt_to_phys(smi_cmd->command_buffer);
+   /*
+* Calling Interface SMI
+*
+* Provide physical address of command buffer field within
+* the struct smi_cmd to BIOS.
+*
+* Because the address that smi_cmd (smi_data_buf) points to
+* will be from memremap() of a non-memory address if WSMT
+* is present, we can't use virt_to_phys() on smi_cmd, so
+* we have to use the physical address that was saved when
+* the virtual address for smi_cmd was received.
+*/
+   smi_cmd->ebx = smi_data_buf_phys_addr +
+   offsetof(struct smi_cmd, command_buffer);
ret = dcdbas_smi_request(smi_cmd);
if (!ret)
ret = count;
@@ -482,6 +499,93 @@ static void dcdbas_host_control(void)
}
 }
 
+/* WSMT */
+
+static u8 checksum(u8 *buffer, u8 length)
+{
+   u8 sum = 0;
+   u8 *end = buffer + length;
+
+   while (buffer < end)
+   sum += *buffer++;
+   return sum;
+}
+
+static inline struct smm_eps_table *check_eps_table(u8 *addr)
+{
+   struct smm_eps_table *eps = (struct smm_eps_table *)addr;
+
+   if (strncmp(eps->smm_comm_buff_anchor, SMM_EPS_SIG, 4) != 0)
+   return NULL;
+
+   if (checksum(addr, eps->length) != 0)
+   return NULL;
+
+   return eps;
+}
+
+static int dcdbas_check_wsmt(void)
+{
+   struct acpi_table_wsmt *wsmt = NULL;
+   struct smm_eps_table *eps = NULL;
+   u64 remap_size;
+   u8 *addr;
+
+   acpi_get_table(ACPI_SIG_WSMT, 0, (struct acpi_table_header **));
+   if (!wsmt)
+ 

Re: [PATCH v3] dcdbas: Add support for WSMT ACPI table

[Stuart Hayes]

On 7/2/2018 11:15 AM, mario.limoncie...@dell.com wrote:
>>
>>> I don't believe SMM communication ACPI table has ever been implemented by
>> Dell
>>> on server or client BIOS.  I do agree this table describes the behavior 
>>> that DCDBAS
>> driver
>>> has used since before even UEFI BIOS pretty accurately.
>>
>> So, EPS table has been for ages in Dell machines?
>> Can we consider it as a predecessor of that SMM communication ACPI table?
> 
> No, EPS is new this year, specifically for server BIOS to be able to support 
> SMM communication
> when WSMT is enabled.  The code tests in Stuart's patch will detect if WSMT 
> is enabled
> and if it's enabled test if EPS was defined.  On server BIOS when EPS is 
> defined dcdbas
> will be able to communicate using addresses defined in EPS.
> 
> Server BIOS will support EPS for applications using dcdbas interface and may 
> at a later time
> introduce same WMI interface as client too (but applications will need time 
> to update so
> they need to support both).
> 
> Actually Stuart's patch will cause client BIOS that has WSMT enabled make 
> dcdbas fail
> initialization (as it should because dcdbas doesn't have a region that it can 
> successfully
> communicate).  
> 
> In client machines we moved this communication to ACPI buffer allocated by 
> WMI, which
> is why we have dell-smbios-wmi now in kernel.
> 
> I think once some variation of Stuart's patch is merged, I'll send a follow
> up patch to drop this test because it's no longer necessary:
> https://github.com/torvalds/linux/blob/master/drivers/platform/x86/dell-smbios-smm.c#L106
> 
> 
>>
>>> Stuart and I did discuss with server BIOS (who uses this EPS mechanism) to 
>>> see if
>> its possible
>>> to move EPS to SMM communication ACPI table however since it's been
>> deprecated by
>>> UEFI 2.7 they weren't willing to adopt it.
>>
>> It's pity, but the motivation to deprecate is "lack of use" which is
>> not true. That's why I would suggest to escalate this to UEFI
>> committee.
>>
>>> Stuart, anything else you want to add here?
>>
>> Darren, what's your opinion about this?
>>
>> P.S. I'm not against this approach (just some technical comments I
>> already shared), but on the other hand it would be nice to have undo
>> that deprecation and follow the standard in new firmwares.
>> Would you agree?
> 
> Sure.  Due to the timing of how long this will take, even if SMM communication
> ACPI table is undone from deprecation we may have to still support both EPS
> and SMM communication ACPI table though (maybe it would be order of 
> preference).
> 
> 

I have confirmation that the EPS table will be 16-byte aligned, so I can make 
that
change.  I'll send a v5 with that and the updated comment.

Re: [PATCH v3] dcdbas: Add support for WSMT ACPI table

[Stuart Hayes]

On 7/2/2018 11:15 AM, mario.limoncie...@dell.com wrote:
>>
>>> I don't believe SMM communication ACPI table has ever been implemented by
>> Dell
>>> on server or client BIOS.  I do agree this table describes the behavior 
>>> that DCDBAS
>> driver
>>> has used since before even UEFI BIOS pretty accurately.
>>
>> So, EPS table has been for ages in Dell machines?
>> Can we consider it as a predecessor of that SMM communication ACPI table?
> 
> No, EPS is new this year, specifically for server BIOS to be able to support 
> SMM communication
> when WSMT is enabled.  The code tests in Stuart's patch will detect if WSMT 
> is enabled
> and if it's enabled test if EPS was defined.  On server BIOS when EPS is 
> defined dcdbas
> will be able to communicate using addresses defined in EPS.
> 
> Server BIOS will support EPS for applications using dcdbas interface and may 
> at a later time
> introduce same WMI interface as client too (but applications will need time 
> to update so
> they need to support both).
> 
> Actually Stuart's patch will cause client BIOS that has WSMT enabled make 
> dcdbas fail
> initialization (as it should because dcdbas doesn't have a region that it can 
> successfully
> communicate).  
> 
> In client machines we moved this communication to ACPI buffer allocated by 
> WMI, which
> is why we have dell-smbios-wmi now in kernel.
> 
> I think once some variation of Stuart's patch is merged, I'll send a follow
> up patch to drop this test because it's no longer necessary:
> https://github.com/torvalds/linux/blob/master/drivers/platform/x86/dell-smbios-smm.c#L106
> 
> 
>>
>>> Stuart and I did discuss with server BIOS (who uses this EPS mechanism) to 
>>> see if
>> its possible
>>> to move EPS to SMM communication ACPI table however since it's been
>> deprecated by
>>> UEFI 2.7 they weren't willing to adopt it.
>>
>> It's pity, but the motivation to deprecate is "lack of use" which is
>> not true. That's why I would suggest to escalate this to UEFI
>> committee.
>>
>>> Stuart, anything else you want to add here?
>>
>> Darren, what's your opinion about this?
>>
>> P.S. I'm not against this approach (just some technical comments I
>> already shared), but on the other hand it would be nice to have undo
>> that deprecation and follow the standard in new firmwares.
>> Would you agree?
> 
> Sure.  Due to the timing of how long this will take, even if SMM communication
> ACPI table is undone from deprecation we may have to still support both EPS
> and SMM communication ACPI table though (maybe it would be order of 
> preference).
> 
> 

I have confirmation that the EPS table will be 16-byte aligned, so I can make 
that
change.  I'll send a v5 with that and the updated comment.

Re: [PATCH v3] dcdbas: Add support for WSMT ACPI table

[Stuart Hayes]

On 06/27/2018 06:52 PM, Andy Shevchenko wrote:
> On Fri, Jun 15, 2018 at 1:31 AM, Stuart Hayes  
> wrote:
>> On 6/14/2018 12:25 PM, Andy Shevchenko wrote:
>>> On Thu, Jun 14, 2018 at 5:22 PM, Stuart Hayes  
>>> wrote:
>>>> On 6/13/2018 3:54 AM, Andy Shevchenko wrote:
>>>
>>>>>> +* Provide physical address of command buffer field 
>>>>>> within
>>>>>> +* the struct smi_cmd... can't use virt_to_phys on 
>>>>>> smi_cmd
>>>>>> +* because address may be from memremap.
>>>>>
>>>>> Wait, memremap() might return a virtual address. How we be sure that
>>>>> we got still physical address here?
>>>
>>>> Before this patch, the address in smi_cmd always came from an alloc, so
>>>> virt_to_phys() was used to get the physical address here.  With WSMT, we
>>>> could be using a BIOS-provided buffer for SMI, in which case the address in
>>>> smi_cmd will come from memremap(), so we can't use virt_to_phys() on it.
>>>> So instead I changed this to use the physical address of smi_data_buf that
>>>> is stored in smi_data_buf_phys_addr, which will be valid regardless of how
>>>> the address of smi_data_buf was generated.
>>>
>>> Yes, but what does guarantee that memremap() will return you still
>>> physical address?
> 
>> Sorry, I'm not sure I understand the question.
>>
>> Up to now, this driver always just allocated a buffer from main memory that
>> it used to send/receive information from BIOS when it generated a SMI.  
>> That's
>> what smi_cmd points to where this comment is.  And it was safe to use
>> virt_to_phys() on this address.
>>
>> With this patch, though, the driver may now be using a buffer that isn't part
>> of main memory--it could now be using a buffer that BIOS provided the 
>> physical
>> address for, and this would not be part of main memory.
> 
> Hmm... But is it CPU address or bus address what BIOS provides?
> 
> If it's a CPU address why do you need to call memremap() on it in the
> first place?
> I could guess that you want to access it from CPU side and rather
> would get faults.
> 

The BIOS-provided EPS provides the physical (bus) address of the fixed SMI 
buffer.
This memory will be of type EfiReservedMemoryType, so it will not be usable RAM
to the linux kernel and won't already be mapped.

>>  So smi_cmd may contain
>> a virtual address that memremap() provided.  And because memremap() is just
>> like ioremap(), the driver can no longer use virt_to_phys(smi_cmd) to get the
>> physical address of the buffer.
> 
> Yes, and ioremap() is dedicated for the resources that are not
> available directly by the memory accesses, but rather require some bus
> transactions (like MMIO)>>
>> My comment is just pointing that out... I was trying to say, "the code can't
>> use virt_to_phys(smi_cmd) to get the virtual address here".
>>
> 
> Please, add bits from above paragraphs to elaborate this in the comment.
> 

No problem, will do.

>> memremap() should always return a virtual address that points to the physical
>> address we send it (unless it fails of course).
> 
>>>>>> +   /* Scan for EPS (entry point structure) */
>>>>>> +   for (addr = (u8 *)__va(0xf);
>>>>>> +addr < (u8 *)__va(0x10 - sizeof(struct smm_eps_table));
>>>>>
>>>>>> +addr += 1) {
>>>>>
>>>>> This wasn't commented IIRC and changed. So, why?
>>>
>>>> I changed this is response to your earlier comment (7 june)... you had 
>>>> pointed
>>>> out that it would be better if I put an "if (eps) break;" inside the for 
>>>> loop
>>>> instead of having "&& !eps" in the condition of the for loop.  I put the 
>>>> note
>>>> "Changed loop searching 0xf to be more readable" in the list of 
>>>> changes for
>>>> patch version v3 to cover this change.
>>>
>>> Thanks, but here I meant += 1 vs += 16 step.
>>>
>>
>> Sorry, I thought I had answered this earlier.  The spec does not say that 
>> the EPS
>> table will be on a 16-byte boundary.  And I just added a printk in this 
>> driver to
>> see where it is on the system I had at hand, and it isn't on a 16-byte 
>> boundary:
> 
> Oh, that's sad.
> 
> Btw, does XSDT have a link 

Re: [PATCH v3] dcdbas: Add support for WSMT ACPI table

[Stuart Hayes]

On 06/27/2018 06:52 PM, Andy Shevchenko wrote:
> On Fri, Jun 15, 2018 at 1:31 AM, Stuart Hayes  
> wrote:
>> On 6/14/2018 12:25 PM, Andy Shevchenko wrote:
>>> On Thu, Jun 14, 2018 at 5:22 PM, Stuart Hayes  
>>> wrote:
>>>> On 6/13/2018 3:54 AM, Andy Shevchenko wrote:
>>>
>>>>>> +* Provide physical address of command buffer field 
>>>>>> within
>>>>>> +* the struct smi_cmd... can't use virt_to_phys on 
>>>>>> smi_cmd
>>>>>> +* because address may be from memremap.
>>>>>
>>>>> Wait, memremap() might return a virtual address. How we be sure that
>>>>> we got still physical address here?
>>>
>>>> Before this patch, the address in smi_cmd always came from an alloc, so
>>>> virt_to_phys() was used to get the physical address here.  With WSMT, we
>>>> could be using a BIOS-provided buffer for SMI, in which case the address in
>>>> smi_cmd will come from memremap(), so we can't use virt_to_phys() on it.
>>>> So instead I changed this to use the physical address of smi_data_buf that
>>>> is stored in smi_data_buf_phys_addr, which will be valid regardless of how
>>>> the address of smi_data_buf was generated.
>>>
>>> Yes, but what does guarantee that memremap() will return you still
>>> physical address?
> 
>> Sorry, I'm not sure I understand the question.
>>
>> Up to now, this driver always just allocated a buffer from main memory that
>> it used to send/receive information from BIOS when it generated a SMI.  
>> That's
>> what smi_cmd points to where this comment is.  And it was safe to use
>> virt_to_phys() on this address.
>>
>> With this patch, though, the driver may now be using a buffer that isn't part
>> of main memory--it could now be using a buffer that BIOS provided the 
>> physical
>> address for, and this would not be part of main memory.
> 
> Hmm... But is it CPU address or bus address what BIOS provides?
> 
> If it's a CPU address why do you need to call memremap() on it in the
> first place?
> I could guess that you want to access it from CPU side and rather
> would get faults.
> 

The BIOS-provided EPS provides the physical (bus) address of the fixed SMI 
buffer.
This memory will be of type EfiReservedMemoryType, so it will not be usable RAM
to the linux kernel and won't already be mapped.

>>  So smi_cmd may contain
>> a virtual address that memremap() provided.  And because memremap() is just
>> like ioremap(), the driver can no longer use virt_to_phys(smi_cmd) to get the
>> physical address of the buffer.
> 
> Yes, and ioremap() is dedicated for the resources that are not
> available directly by the memory accesses, but rather require some bus
> transactions (like MMIO)>>
>> My comment is just pointing that out... I was trying to say, "the code can't
>> use virt_to_phys(smi_cmd) to get the virtual address here".
>>
> 
> Please, add bits from above paragraphs to elaborate this in the comment.
> 

No problem, will do.

>> memremap() should always return a virtual address that points to the physical
>> address we send it (unless it fails of course).
> 
>>>>>> +   /* Scan for EPS (entry point structure) */
>>>>>> +   for (addr = (u8 *)__va(0xf);
>>>>>> +addr < (u8 *)__va(0x10 - sizeof(struct smm_eps_table));
>>>>>
>>>>>> +addr += 1) {
>>>>>
>>>>> This wasn't commented IIRC and changed. So, why?
>>>
>>>> I changed this is response to your earlier comment (7 june)... you had 
>>>> pointed
>>>> out that it would be better if I put an "if (eps) break;" inside the for 
>>>> loop
>>>> instead of having "&& !eps" in the condition of the for loop.  I put the 
>>>> note
>>>> "Changed loop searching 0xf to be more readable" in the list of 
>>>> changes for
>>>> patch version v3 to cover this change.
>>>
>>> Thanks, but here I meant += 1 vs += 16 step.
>>>
>>
>> Sorry, I thought I had answered this earlier.  The spec does not say that 
>> the EPS
>> table will be on a 16-byte boundary.  And I just added a printk in this 
>> driver to
>> see where it is on the system I had at hand, and it isn't on a 16-byte 
>> boundary:
> 
> Oh, that's sad.
> 
> Btw, does XSDT have a link 

Re: [PATCH v4] dcdbas: Add support for WSMT ACPI table

[Stuart Hayes]

On 6/14/2018 12:26 PM, Andy Shevchenko wrote:
> On Thu, Jun 14, 2018 at 6:45 PM, Stuart Hayes  
> wrote:
>>
>> If the WSMT ACPI table is present and indicates that a fixed communication
>> buffer should be used, use the firmware-specified buffer instead of
>> allocating a buffer in memory for communications between the dcdbas driver
>> and firmare.
> 
> Thanks for an update.
> 
> I answered to previous thread. The questions / comments are still
> applicable here.
> 

I answered your questions in the previous thread (a while back).

Please let me know if there are any more concerns with this.  Thanks!

 

Re: [PATCH v4] dcdbas: Add support for WSMT ACPI table

[Stuart Hayes]

On 6/14/2018 12:26 PM, Andy Shevchenko wrote:
> On Thu, Jun 14, 2018 at 6:45 PM, Stuart Hayes  
> wrote:
>>
>> If the WSMT ACPI table is present and indicates that a fixed communication
>> buffer should be used, use the firmware-specified buffer instead of
>> allocating a buffer in memory for communications between the dcdbas driver
>> and firmare.
> 
> Thanks for an update.
> 
> I answered to previous thread. The questions / comments are still
> applicable here.
> 

I answered your questions in the previous thread (a while back).

Please let me know if there are any more concerns with this.  Thanks!

 

Re: [PATCH v3] dcdbas: Add support for WSMT ACPI table

[Stuart Hayes]

On 6/14/2018 12:25 PM, Andy Shevchenko wrote:
> On Thu, Jun 14, 2018 at 5:22 PM, Stuart Hayes  
> wrote:
>> On 6/13/2018 3:54 AM, Andy Shevchenko wrote:
> 
>>>> +* Provide physical address of command buffer field within
>>>> +* the struct smi_cmd... can't use virt_to_phys on smi_cmd
>>>> +* because address may be from memremap.
>>>
>>> Wait, memremap() might return a virtual address. How we be sure that
>>> we got still physical address here?
> 
>> Before this patch, the address in smi_cmd always came from an alloc, so
>> virt_to_phys() was used to get the physical address here.  With WSMT, we
>> could be using a BIOS-provided buffer for SMI, in which case the address in
>> smi_cmd will come from memremap(), so we can't use virt_to_phys() on it.
>> So instead I changed this to use the physical address of smi_data_buf that
>> is stored in smi_data_buf_phys_addr, which will be valid regardless of how
>> the address of smi_data_buf was generated.
> 
> Yes, but what does guarantee that memremap() will return you still
> physical address?
> 

Sorry, I'm not sure I understand the question.

Up to now, this driver always just allocated a buffer from main memory that
it used to send/receive information from BIOS when it generated a SMI.  That's
what smi_cmd points to where this comment is.  And it was safe to use
virt_to_phys() on this address.

With this patch, though, the driver may now be using a buffer that isn't part
of main memory--it could now be using a buffer that BIOS provided the physical
address for, and this would not be part of main memory.  So smi_cmd may contain
a virtual address that memremap() provided.  And because memremap() is just
like ioremap(), the driver can no longer use virt_to_phys(smi_cmd) to get the
physical address of the buffer.

My comment is just pointing that out... I was trying to say, "the code can't
use virt_to_phys(smi_cmd) to get the virtual address here".

memremap() should always return a virtual address that points to the physical
address we send it (unless it fails of course).


>>>> +   return 0;
>>>> +
>>>> +   /* Scan for EPS (entry point structure) */
>>>> +   for (addr = (u8 *)__va(0xf);
>>>> +addr < (u8 *)__va(0x10 - sizeof(struct smm_eps_table));
>>>
>>>> +addr += 1) {
>>>
>>> This wasn't commented IIRC and changed. So, why?
> 
>> I changed this is response to your earlier comment (7 june)... you had 
>> pointed
>> out that it would be better if I put an "if (eps) break;" inside the for loop
>> instead of having "&& !eps" in the condition of the for loop.  I put the note
>> "Changed loop searching 0xf to be more readable" in the list of changes 
>> for
>> patch version v3 to cover this change.
> 
> Thanks, but here I meant += 1 vs += 16 step.
> 

Sorry, I thought I had answered this earlier.  The spec does not say that the 
EPS
table will be on a 16-byte boundary.  And I just added a printk in this driver 
to
see where it is on the system I had at hand, and it isn't on a 16-byte boundary:

[ 4680.192542] dcdbas - EPS table at 5761efb7
[ 4680.194012] dcdbas dcdbas: WSMT found, using firmware-provided SMI buffer.
[ 4680.195327] dcdbas dcdbas: Dell Systems Management Base Driver (version 
5.6.0-3.3)

Re: [PATCH v3] dcdbas: Add support for WSMT ACPI table

[Stuart Hayes]

On 6/14/2018 12:25 PM, Andy Shevchenko wrote:
> On Thu, Jun 14, 2018 at 5:22 PM, Stuart Hayes  
> wrote:
>> On 6/13/2018 3:54 AM, Andy Shevchenko wrote:
> 
>>>> +* Provide physical address of command buffer field within
>>>> +* the struct smi_cmd... can't use virt_to_phys on smi_cmd
>>>> +* because address may be from memremap.
>>>
>>> Wait, memremap() might return a virtual address. How we be sure that
>>> we got still physical address here?
> 
>> Before this patch, the address in smi_cmd always came from an alloc, so
>> virt_to_phys() was used to get the physical address here.  With WSMT, we
>> could be using a BIOS-provided buffer for SMI, in which case the address in
>> smi_cmd will come from memremap(), so we can't use virt_to_phys() on it.
>> So instead I changed this to use the physical address of smi_data_buf that
>> is stored in smi_data_buf_phys_addr, which will be valid regardless of how
>> the address of smi_data_buf was generated.
> 
> Yes, but what does guarantee that memremap() will return you still
> physical address?
> 

Sorry, I'm not sure I understand the question.

Up to now, this driver always just allocated a buffer from main memory that
it used to send/receive information from BIOS when it generated a SMI.  That's
what smi_cmd points to where this comment is.  And it was safe to use
virt_to_phys() on this address.

With this patch, though, the driver may now be using a buffer that isn't part
of main memory--it could now be using a buffer that BIOS provided the physical
address for, and this would not be part of main memory.  So smi_cmd may contain
a virtual address that memremap() provided.  And because memremap() is just
like ioremap(), the driver can no longer use virt_to_phys(smi_cmd) to get the
physical address of the buffer.

My comment is just pointing that out... I was trying to say, "the code can't
use virt_to_phys(smi_cmd) to get the virtual address here".

memremap() should always return a virtual address that points to the physical
address we send it (unless it fails of course).


>>>> +   return 0;
>>>> +
>>>> +   /* Scan for EPS (entry point structure) */
>>>> +   for (addr = (u8 *)__va(0xf);
>>>> +addr < (u8 *)__va(0x10 - sizeof(struct smm_eps_table));
>>>
>>>> +addr += 1) {
>>>
>>> This wasn't commented IIRC and changed. So, why?
> 
>> I changed this is response to your earlier comment (7 june)... you had 
>> pointed
>> out that it would be better if I put an "if (eps) break;" inside the for loop
>> instead of having "&& !eps" in the condition of the for loop.  I put the note
>> "Changed loop searching 0xf to be more readable" in the list of changes 
>> for
>> patch version v3 to cover this change.
> 
> Thanks, but here I meant += 1 vs += 16 step.
> 

Sorry, I thought I had answered this earlier.  The spec does not say that the 
EPS
table will be on a 16-byte boundary.  And I just added a printk in this driver 
to
see where it is on the system I had at hand, and it isn't on a 16-byte boundary:

[ 4680.192542] dcdbas - EPS table at 5761efb7
[ 4680.194012] dcdbas dcdbas: WSMT found, using firmware-provided SMI buffer.
[ 4680.195327] dcdbas dcdbas: Dell Systems Management Base Driver (version 
5.6.0-3.3)

[PATCH v4] dcdbas: Add support for WSMT ACPI table

[Stuart Hayes]

If the WSMT ACPI table is present and indicates that a fixed communication
buffer should be used, use the firmware-specified buffer instead of
allocating a buffer in memory for communications between the dcdbas driver
and firmare.

Signed-off-by: Stuart Hayes 
---
v2 Bumped driver version to 5.6.0-3.3
v3 Removed dependency on ACPI in Kconfig
   Moved the added #include to be in alphabetical order
   Added comments in smi_request_store()
   Simplified checksum code
   Changed loop searching 0xf to be more readable
   Reworked calculation of remap_size & smi_data_buf_size
v4 Fixed comment that starts with "Calling Interface SMI"
   Fixed formatting of first "if" statement in dcdbas_check_wsmt()


 drivers/firmware/dcdbas.c | 118 +++---
 drivers/firmware/dcdbas.h |  10 
 2 files changed, 122 insertions(+), 6 deletions(-)

diff --git a/drivers/firmware/dcdbas.c b/drivers/firmware/dcdbas.c
index 0bdea60c65dd..4cac70d05bfc 100644
--- a/drivers/firmware/dcdbas.c
+++ b/drivers/firmware/dcdbas.c
@@ -21,6 +21,7 @@
  */
 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -41,7 +42,7 @@
 #include "dcdbas.h"
 
 #define DRIVER_NAME"dcdbas"
-#define DRIVER_VERSION "5.6.0-3.2"
+#define DRIVER_VERSION "5.6.0-3.3"
 #define DRIVER_DESCRIPTION "Dell Systems Management Base Driver"
 
 static struct platform_device *dcdbas_pdev;
@@ -49,19 +50,23 @@ static struct platform_device *dcdbas_pdev;
 static u8 *smi_data_buf;
 static dma_addr_t smi_data_buf_handle;
 static unsigned long smi_data_buf_size;
+static unsigned long max_smi_data_buf_size = MAX_SMI_DATA_BUF_SIZE;
 static u32 smi_data_buf_phys_addr;
 static DEFINE_MUTEX(smi_data_lock);
+static u8 *eps_buffer;
 
 static unsigned int host_control_action;
 static unsigned int host_control_smi_type;
 static unsigned int host_control_on_shutdown;
 
+static bool wsmt_enabled;
+
 /**
  * smi_data_buf_free: free SMI data buffer
  */
 static void smi_data_buf_free(void)
 {
-   if (!smi_data_buf)
+   if (!smi_data_buf || wsmt_enabled)
return;
 
dev_dbg(_pdev->dev, "%s: phys: %x size: %lu\n",
@@ -86,7 +91,7 @@ static int smi_data_buf_realloc(unsigned long size)
if (smi_data_buf_size >= size)
return 0;
 
-   if (size > MAX_SMI_DATA_BUF_SIZE)
+   if (size > max_smi_data_buf_size)
return -EINVAL;
 
/* new buffer is needed */
@@ -169,7 +174,7 @@ static ssize_t smi_data_write(struct file *filp, struct 
kobject *kobj,
 {
ssize_t ret;
 
-   if ((pos + count) > MAX_SMI_DATA_BUF_SIZE)
+   if ((pos + count) > max_smi_data_buf_size)
return -EINVAL;
 
mutex_lock(_data_lock);
@@ -322,8 +327,15 @@ static ssize_t smi_request_store(struct device *dev,
ret = count;
break;
case 1:
-   /* Calling Interface SMI */
-   smi_cmd->ebx = (u32) virt_to_phys(smi_cmd->command_buffer);
+   /*
+* Calling Interface SMI
+*
+* Provide physical address of command buffer field within
+* the struct smi_cmd... can't use virt_to_phys() on smi_cmd
+* because address may be from memremap().
+*/
+   smi_cmd->ebx = smi_data_buf_phys_addr +
+   offsetof(struct smi_cmd, command_buffer);
ret = dcdbas_smi_request(smi_cmd);
if (!ret)
ret = count;
@@ -482,6 +494,93 @@ static void dcdbas_host_control(void)
}
 }
 
+/* WSMT */
+
+static u8 checksum(u8 *buffer, u8 length)
+{
+   u8 sum = 0;
+   u8 *end = buffer + length;
+
+   while (buffer < end)
+   sum += *buffer++;
+   return sum;
+}
+
+static inline struct smm_eps_table *check_eps_table(u8 *addr)
+{
+   struct smm_eps_table *eps = (struct smm_eps_table *)addr;
+
+   if (strncmp(eps->smm_comm_buff_anchor, SMM_EPS_SIG, 4) != 0)
+   return NULL;
+
+   if (checksum(addr, eps->length) != 0)
+   return NULL;
+
+   return eps;
+}
+
+static int dcdbas_check_wsmt(void)
+{
+   struct acpi_table_wsmt *wsmt = NULL;
+   struct smm_eps_table *eps = NULL;
+   u64 remap_size;
+   u8 *addr;
+
+   acpi_get_table(ACPI_SIG_WSMT, 0, (struct acpi_table_header **));
+   if (!wsmt)
+   return 0;
+
+   /* Check if WSMT ACPI table shows that protection is enabled */
+   if (!(wsmt->protection_flags & ACPI_WSMT_FIXED_COMM_BUFFERS) ||
+   !(wsmt->protection_flags & 
ACPI_WSMT_COMM_BUFFER_NESTED_PTR_PROTECTION))
+   return 0;
+
+   /* Scan for EPS (entry point structure) */
+   for (addr = (u8 *)__va(0xf);
+addr < (u8 *)__va

[PATCH v4] dcdbas: Add support for WSMT ACPI table

[Stuart Hayes]

If the WSMT ACPI table is present and indicates that a fixed communication
buffer should be used, use the firmware-specified buffer instead of
allocating a buffer in memory for communications between the dcdbas driver
and firmare.

Signed-off-by: Stuart Hayes 
---
v2 Bumped driver version to 5.6.0-3.3
v3 Removed dependency on ACPI in Kconfig
   Moved the added #include to be in alphabetical order
   Added comments in smi_request_store()
   Simplified checksum code
   Changed loop searching 0xf to be more readable
   Reworked calculation of remap_size & smi_data_buf_size
v4 Fixed comment that starts with "Calling Interface SMI"
   Fixed formatting of first "if" statement in dcdbas_check_wsmt()


 drivers/firmware/dcdbas.c | 118 +++---
 drivers/firmware/dcdbas.h |  10 
 2 files changed, 122 insertions(+), 6 deletions(-)

diff --git a/drivers/firmware/dcdbas.c b/drivers/firmware/dcdbas.c
index 0bdea60c65dd..4cac70d05bfc 100644
--- a/drivers/firmware/dcdbas.c
+++ b/drivers/firmware/dcdbas.c
@@ -21,6 +21,7 @@
  */
 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -41,7 +42,7 @@
 #include "dcdbas.h"
 
 #define DRIVER_NAME"dcdbas"
-#define DRIVER_VERSION "5.6.0-3.2"
+#define DRIVER_VERSION "5.6.0-3.3"
 #define DRIVER_DESCRIPTION "Dell Systems Management Base Driver"
 
 static struct platform_device *dcdbas_pdev;
@@ -49,19 +50,23 @@ static struct platform_device *dcdbas_pdev;
 static u8 *smi_data_buf;
 static dma_addr_t smi_data_buf_handle;
 static unsigned long smi_data_buf_size;
+static unsigned long max_smi_data_buf_size = MAX_SMI_DATA_BUF_SIZE;
 static u32 smi_data_buf_phys_addr;
 static DEFINE_MUTEX(smi_data_lock);
+static u8 *eps_buffer;
 
 static unsigned int host_control_action;
 static unsigned int host_control_smi_type;
 static unsigned int host_control_on_shutdown;
 
+static bool wsmt_enabled;
+
 /**
  * smi_data_buf_free: free SMI data buffer
  */
 static void smi_data_buf_free(void)
 {
-   if (!smi_data_buf)
+   if (!smi_data_buf || wsmt_enabled)
return;
 
dev_dbg(_pdev->dev, "%s: phys: %x size: %lu\n",
@@ -86,7 +91,7 @@ static int smi_data_buf_realloc(unsigned long size)
if (smi_data_buf_size >= size)
return 0;
 
-   if (size > MAX_SMI_DATA_BUF_SIZE)
+   if (size > max_smi_data_buf_size)
return -EINVAL;
 
/* new buffer is needed */
@@ -169,7 +174,7 @@ static ssize_t smi_data_write(struct file *filp, struct 
kobject *kobj,
 {
ssize_t ret;
 
-   if ((pos + count) > MAX_SMI_DATA_BUF_SIZE)
+   if ((pos + count) > max_smi_data_buf_size)
return -EINVAL;
 
mutex_lock(_data_lock);
@@ -322,8 +327,15 @@ static ssize_t smi_request_store(struct device *dev,
ret = count;
break;
case 1:
-   /* Calling Interface SMI */
-   smi_cmd->ebx = (u32) virt_to_phys(smi_cmd->command_buffer);
+   /*
+* Calling Interface SMI
+*
+* Provide physical address of command buffer field within
+* the struct smi_cmd... can't use virt_to_phys() on smi_cmd
+* because address may be from memremap().
+*/
+   smi_cmd->ebx = smi_data_buf_phys_addr +
+   offsetof(struct smi_cmd, command_buffer);
ret = dcdbas_smi_request(smi_cmd);
if (!ret)
ret = count;
@@ -482,6 +494,93 @@ static void dcdbas_host_control(void)
}
 }
 
+/* WSMT */
+
+static u8 checksum(u8 *buffer, u8 length)
+{
+   u8 sum = 0;
+   u8 *end = buffer + length;
+
+   while (buffer < end)
+   sum += *buffer++;
+   return sum;
+}
+
+static inline struct smm_eps_table *check_eps_table(u8 *addr)
+{
+   struct smm_eps_table *eps = (struct smm_eps_table *)addr;
+
+   if (strncmp(eps->smm_comm_buff_anchor, SMM_EPS_SIG, 4) != 0)
+   return NULL;
+
+   if (checksum(addr, eps->length) != 0)
+   return NULL;
+
+   return eps;
+}
+
+static int dcdbas_check_wsmt(void)
+{
+   struct acpi_table_wsmt *wsmt = NULL;
+   struct smm_eps_table *eps = NULL;
+   u64 remap_size;
+   u8 *addr;
+
+   acpi_get_table(ACPI_SIG_WSMT, 0, (struct acpi_table_header **));
+   if (!wsmt)
+   return 0;
+
+   /* Check if WSMT ACPI table shows that protection is enabled */
+   if (!(wsmt->protection_flags & ACPI_WSMT_FIXED_COMM_BUFFERS) ||
+   !(wsmt->protection_flags & 
ACPI_WSMT_COMM_BUFFER_NESTED_PTR_PROTECTION))
+   return 0;
+
+   /* Scan for EPS (entry point structure) */
+   for (addr = (u8 *)__va(0xf);
+addr < (u8 *)__va

Re: [PATCH v3] dcdbas: Add support for WSMT ACPI table

[Stuart Hayes]

On 6/13/2018 3:54 AM, Andy Shevchenko wrote:
>> +   /* Calling Interface SMI
> 
> I suppose the style of the comments like
> /*
>  * Calling ...
> ...

Yes... goof on my part.  Thanks.

>> +*
>> +* Provide physical address of command buffer field within
>> +* the struct smi_cmd... can't use virt_to_phys on smi_cmd
>> +* because address may be from memremap.
> 
> Wait, memremap() might return a virtual address. How we be sure that
> we got still physical address here?
> 
> (Also note () when referring to functions)
>
>> +*/
>> +   smi_cmd->ebx = smi_data_buf_phys_addr +
>> +   offsetof(struct smi_cmd, command_buffer);
> 
> Btw, can it be one line (~83 character are okay for my opinion).
> 

Before this patch, the address in smi_cmd always came from an alloc, so
virt_to_phys() was used to get the physical address here.  With WSMT, we
could be using a BIOS-provided buffer for SMI, in which case the address in
smi_cmd will come from memremap(), so we can't use virt_to_phys() on it.
So instead I changed this to use the physical address of smi_data_buf that
is stored in smi_data_buf_phys_addr, which will be valid regardless of how
the address of smi_data_buf was generated.

But that would be like 97 characters long if I made it all one line...

>> +   acpi_get_table(ACPI_SIG_WSMT, 0, (struct acpi_table_header **));
>> +   if (!wsmt)
>> +   return 0;
>> +
>> +   /* Check if WSMT ACPI table shows that protection is enabled */
>> +   if (!(wsmt->protection_flags & ACPI_WSMT_FIXED_COMM_BUFFERS)
> 
>> +   || !(wsmt->protection_flags
>> +& ACPI_WSMT_COMM_BUFFER_NESTED_PTR_PROTECTION))
> 
> Better to indent like
> 
> if (... ||
>  !(... & ...)
> 

Yes thanks.

>> +   return 0;
>> +
>> +   /* Scan for EPS (entry point structure) */
>> +   for (addr = (u8 *)__va(0xf);
>> +addr < (u8 *)__va(0x10 - sizeof(struct smm_eps_table));
> 
>> +addr += 1) {
> 
> This wasn't commented IIRC and changed. So, why?
> 

I changed this is response to your earlier comment (7 june)... you had pointed
out that it would be better if I put an "if (eps) break;" inside the for loop
instead of having "&& !eps" in the condition of the for loop.  I put the note
"Changed loop searching 0xf to be more readable" in the list of changes for
patch version v3 to cover this change.

Thanks again for your time!

Re: [PATCH v3] dcdbas: Add support for WSMT ACPI table

[Stuart Hayes]

On 6/13/2018 3:54 AM, Andy Shevchenko wrote:
>> +   /* Calling Interface SMI
> 
> I suppose the style of the comments like
> /*
>  * Calling ...
> ...

Yes... goof on my part.  Thanks.

>> +*
>> +* Provide physical address of command buffer field within
>> +* the struct smi_cmd... can't use virt_to_phys on smi_cmd
>> +* because address may be from memremap.
> 
> Wait, memremap() might return a virtual address. How we be sure that
> we got still physical address here?
> 
> (Also note () when referring to functions)
>
>> +*/
>> +   smi_cmd->ebx = smi_data_buf_phys_addr +
>> +   offsetof(struct smi_cmd, command_buffer);
> 
> Btw, can it be one line (~83 character are okay for my opinion).
> 

Before this patch, the address in smi_cmd always came from an alloc, so
virt_to_phys() was used to get the physical address here.  With WSMT, we
could be using a BIOS-provided buffer for SMI, in which case the address in
smi_cmd will come from memremap(), so we can't use virt_to_phys() on it.
So instead I changed this to use the physical address of smi_data_buf that
is stored in smi_data_buf_phys_addr, which will be valid regardless of how
the address of smi_data_buf was generated.

But that would be like 97 characters long if I made it all one line...

>> +   acpi_get_table(ACPI_SIG_WSMT, 0, (struct acpi_table_header **));
>> +   if (!wsmt)
>> +   return 0;
>> +
>> +   /* Check if WSMT ACPI table shows that protection is enabled */
>> +   if (!(wsmt->protection_flags & ACPI_WSMT_FIXED_COMM_BUFFERS)
> 
>> +   || !(wsmt->protection_flags
>> +& ACPI_WSMT_COMM_BUFFER_NESTED_PTR_PROTECTION))
> 
> Better to indent like
> 
> if (... ||
>  !(... & ...)
> 

Yes thanks.

>> +   return 0;
>> +
>> +   /* Scan for EPS (entry point structure) */
>> +   for (addr = (u8 *)__va(0xf);
>> +addr < (u8 *)__va(0x10 - sizeof(struct smm_eps_table));
> 
>> +addr += 1) {
> 
> This wasn't commented IIRC and changed. So, why?
> 

I changed this is response to your earlier comment (7 june)... you had pointed
out that it would be better if I put an "if (eps) break;" inside the for loop
instead of having "&& !eps" in the condition of the for loop.  I put the note
"Changed loop searching 0xf to be more readable" in the list of changes for
patch version v3 to cover this change.

Thanks again for your time!

[PATCH v3] dcdbas: Add support for WSMT ACPI table

[Stuart Hayes]

If the WSMT ACPI table is present and indicates that a fixed communication
buffer should be used, use the firmware-specified buffer instead of
allocating a buffer in memory for communications between the dcdbas driver
and firmare.

Signed-off-by: Stuart Hayes 
---
v2 Bumped driver version to 5.6.0-3.3
v3 Removed dependency on ACPI in Kconfig
   Moved the added #include to be in alphabetical order
   Added comments in smi_request_store()
   Simplified checksum code
   Changed loop searching 0xf to be more readable
   Reworked calculation of remap_size & smi_data_buf_size


 drivers/firmware/dcdbas.c | 118 +++---
 drivers/firmware/dcdbas.h |  10 
 2 files changed, 122 insertions(+), 6 deletions(-)

diff --git a/drivers/firmware/dcdbas.c b/drivers/firmware/dcdbas.c
index 0bdea60c65dd..e95dc9aee2fa 100644
--- a/drivers/firmware/dcdbas.c
+++ b/drivers/firmware/dcdbas.c
@@ -21,6 +21,7 @@
  */
 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -41,7 +42,7 @@
 #include "dcdbas.h"
 
 #define DRIVER_NAME"dcdbas"
-#define DRIVER_VERSION "5.6.0-3.2"
+#define DRIVER_VERSION "5.6.0-3.3"
 #define DRIVER_DESCRIPTION "Dell Systems Management Base Driver"
 
 static struct platform_device *dcdbas_pdev;
@@ -49,19 +50,23 @@ static struct platform_device *dcdbas_pdev;
 static u8 *smi_data_buf;
 static dma_addr_t smi_data_buf_handle;
 static unsigned long smi_data_buf_size;
+static unsigned long max_smi_data_buf_size = MAX_SMI_DATA_BUF_SIZE;
 static u32 smi_data_buf_phys_addr;
 static DEFINE_MUTEX(smi_data_lock);
+static u8 *eps_buffer;
 
 static unsigned int host_control_action;
 static unsigned int host_control_smi_type;
 static unsigned int host_control_on_shutdown;
 
+static bool wsmt_enabled;
+
 /**
  * smi_data_buf_free: free SMI data buffer
  */
 static void smi_data_buf_free(void)
 {
-   if (!smi_data_buf)
+   if (!smi_data_buf || wsmt_enabled)
return;
 
dev_dbg(_pdev->dev, "%s: phys: %x size: %lu\n",
@@ -86,7 +91,7 @@ static int smi_data_buf_realloc(unsigned long size)
if (smi_data_buf_size >= size)
return 0;
 
-   if (size > MAX_SMI_DATA_BUF_SIZE)
+   if (size > max_smi_data_buf_size)
return -EINVAL;
 
/* new buffer is needed */
@@ -169,7 +174,7 @@ static ssize_t smi_data_write(struct file *filp, struct 
kobject *kobj,
 {
ssize_t ret;
 
-   if ((pos + count) > MAX_SMI_DATA_BUF_SIZE)
+   if ((pos + count) > max_smi_data_buf_size)
return -EINVAL;
 
mutex_lock(_data_lock);
@@ -322,8 +327,14 @@ static ssize_t smi_request_store(struct device *dev,
ret = count;
break;
case 1:
-   /* Calling Interface SMI */
-   smi_cmd->ebx = (u32) virt_to_phys(smi_cmd->command_buffer);
+   /* Calling Interface SMI
+*
+* Provide physical address of command buffer field within
+* the struct smi_cmd... can't use virt_to_phys on smi_cmd
+* because address may be from memremap.
+*/
+   smi_cmd->ebx = smi_data_buf_phys_addr +
+   offsetof(struct smi_cmd, command_buffer);
ret = dcdbas_smi_request(smi_cmd);
if (!ret)
ret = count;
@@ -482,6 +493,94 @@ static void dcdbas_host_control(void)
}
 }
 
+/* WSMT */
+
+static u8 checksum(u8 *buffer, u8 length)
+{
+   u8 sum = 0;
+   u8 *end = buffer + length;
+
+   while (buffer < end)
+   sum += *buffer++;
+   return sum;
+}
+
+static inline struct smm_eps_table *check_eps_table(u8 *addr)
+{
+   struct smm_eps_table *eps = (struct smm_eps_table *)addr;
+
+   if (strncmp(eps->smm_comm_buff_anchor, SMM_EPS_SIG, 4) != 0)
+   return NULL;
+
+   if (checksum(addr, eps->length) != 0)
+   return NULL;
+
+   return eps;
+}
+
+static int dcdbas_check_wsmt(void)
+{
+   struct acpi_table_wsmt *wsmt = NULL;
+   struct smm_eps_table *eps = NULL;
+   u64 remap_size;
+   u8 *addr;
+
+   acpi_get_table(ACPI_SIG_WSMT, 0, (struct acpi_table_header **));
+   if (!wsmt)
+   return 0;
+
+   /* Check if WSMT ACPI table shows that protection is enabled */
+   if (!(wsmt->protection_flags & ACPI_WSMT_FIXED_COMM_BUFFERS)
+   || !(wsmt->protection_flags
+& ACPI_WSMT_COMM_BUFFER_NESTED_PTR_PROTECTION))
+   return 0;
+
+   /* Scan for EPS (entry point structure) */
+   for (addr = (u8 *)__va(0xf);
+addr < (u8 *)__va(0x10 - sizeof(struct smm_eps_table));
+addr += 1) {
+   eps = check_eps_table(addr);
+   if (eps)
+

[PATCH v3] dcdbas: Add support for WSMT ACPI table

[Stuart Hayes]

If the WSMT ACPI table is present and indicates that a fixed communication
buffer should be used, use the firmware-specified buffer instead of
allocating a buffer in memory for communications between the dcdbas driver
and firmare.

Signed-off-by: Stuart Hayes 
---
v2 Bumped driver version to 5.6.0-3.3
v3 Removed dependency on ACPI in Kconfig
   Moved the added #include to be in alphabetical order
   Added comments in smi_request_store()
   Simplified checksum code
   Changed loop searching 0xf to be more readable
   Reworked calculation of remap_size & smi_data_buf_size


 drivers/firmware/dcdbas.c | 118 +++---
 drivers/firmware/dcdbas.h |  10 
 2 files changed, 122 insertions(+), 6 deletions(-)

diff --git a/drivers/firmware/dcdbas.c b/drivers/firmware/dcdbas.c
index 0bdea60c65dd..e95dc9aee2fa 100644
--- a/drivers/firmware/dcdbas.c
+++ b/drivers/firmware/dcdbas.c
@@ -21,6 +21,7 @@
  */
 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -41,7 +42,7 @@
 #include "dcdbas.h"
 
 #define DRIVER_NAME"dcdbas"
-#define DRIVER_VERSION "5.6.0-3.2"
+#define DRIVER_VERSION "5.6.0-3.3"
 #define DRIVER_DESCRIPTION "Dell Systems Management Base Driver"
 
 static struct platform_device *dcdbas_pdev;
@@ -49,19 +50,23 @@ static struct platform_device *dcdbas_pdev;
 static u8 *smi_data_buf;
 static dma_addr_t smi_data_buf_handle;
 static unsigned long smi_data_buf_size;
+static unsigned long max_smi_data_buf_size = MAX_SMI_DATA_BUF_SIZE;
 static u32 smi_data_buf_phys_addr;
 static DEFINE_MUTEX(smi_data_lock);
+static u8 *eps_buffer;
 
 static unsigned int host_control_action;
 static unsigned int host_control_smi_type;
 static unsigned int host_control_on_shutdown;
 
+static bool wsmt_enabled;
+
 /**
  * smi_data_buf_free: free SMI data buffer
  */
 static void smi_data_buf_free(void)
 {
-   if (!smi_data_buf)
+   if (!smi_data_buf || wsmt_enabled)
return;
 
dev_dbg(_pdev->dev, "%s: phys: %x size: %lu\n",
@@ -86,7 +91,7 @@ static int smi_data_buf_realloc(unsigned long size)
if (smi_data_buf_size >= size)
return 0;
 
-   if (size > MAX_SMI_DATA_BUF_SIZE)
+   if (size > max_smi_data_buf_size)
return -EINVAL;
 
/* new buffer is needed */
@@ -169,7 +174,7 @@ static ssize_t smi_data_write(struct file *filp, struct 
kobject *kobj,
 {
ssize_t ret;
 
-   if ((pos + count) > MAX_SMI_DATA_BUF_SIZE)
+   if ((pos + count) > max_smi_data_buf_size)
return -EINVAL;
 
mutex_lock(_data_lock);
@@ -322,8 +327,14 @@ static ssize_t smi_request_store(struct device *dev,
ret = count;
break;
case 1:
-   /* Calling Interface SMI */
-   smi_cmd->ebx = (u32) virt_to_phys(smi_cmd->command_buffer);
+   /* Calling Interface SMI
+*
+* Provide physical address of command buffer field within
+* the struct smi_cmd... can't use virt_to_phys on smi_cmd
+* because address may be from memremap.
+*/
+   smi_cmd->ebx = smi_data_buf_phys_addr +
+   offsetof(struct smi_cmd, command_buffer);
ret = dcdbas_smi_request(smi_cmd);
if (!ret)
ret = count;
@@ -482,6 +493,94 @@ static void dcdbas_host_control(void)
}
 }
 
+/* WSMT */
+
+static u8 checksum(u8 *buffer, u8 length)
+{
+   u8 sum = 0;
+   u8 *end = buffer + length;
+
+   while (buffer < end)
+   sum += *buffer++;
+   return sum;
+}
+
+static inline struct smm_eps_table *check_eps_table(u8 *addr)
+{
+   struct smm_eps_table *eps = (struct smm_eps_table *)addr;
+
+   if (strncmp(eps->smm_comm_buff_anchor, SMM_EPS_SIG, 4) != 0)
+   return NULL;
+
+   if (checksum(addr, eps->length) != 0)
+   return NULL;
+
+   return eps;
+}
+
+static int dcdbas_check_wsmt(void)
+{
+   struct acpi_table_wsmt *wsmt = NULL;
+   struct smm_eps_table *eps = NULL;
+   u64 remap_size;
+   u8 *addr;
+
+   acpi_get_table(ACPI_SIG_WSMT, 0, (struct acpi_table_header **));
+   if (!wsmt)
+   return 0;
+
+   /* Check if WSMT ACPI table shows that protection is enabled */
+   if (!(wsmt->protection_flags & ACPI_WSMT_FIXED_COMM_BUFFERS)
+   || !(wsmt->protection_flags
+& ACPI_WSMT_COMM_BUFFER_NESTED_PTR_PROTECTION))
+   return 0;
+
+   /* Scan for EPS (entry point structure) */
+   for (addr = (u8 *)__va(0xf);
+addr < (u8 *)__va(0x10 - sizeof(struct smm_eps_table));
+addr += 1) {
+   eps = check_eps_table(addr);
+   if (eps)
+

Re: [PATCH resend v2] dcdbas: Add support for WSMT ACPI table

[Stuart Hayes]

On 6/8/2018 8:04 PM, Darren Hart wrote:
> On Thu, Jun 07, 2018 at 08:11:41PM +0300, Andy Shevchenko wrote:
>> On Thu, Jun 7, 2018 at 6:59 PM, Stuart Hayes  
>> wrote:
>>> If the WSMT ACPI table is present and indicates that a fixed communication
>>> buffer should be used, use the firmware-specified buffer instead of
>>> allocating a buffer in memory for communications between the dcdbas driver
>>> and firmare.
>>
>>>  config DCDBAS
>>> tristate "Dell Systems Management Base Driver"
>>> -   depends on X86
>>> +   depends on X86 && ACPI
> 
> 
>>
>> Hmm... I'm not sure about this dependency.
>> So, the question is do all users actually need this? How did it work
>> previously? How has this been tested in case when command line has
>> "acpi=off" (yes, this one just for sake of test, I don't believe it's
>> a real use case)?
> 
> Yeah... after the 4.16 and 4.17 KConfig fumbling around the SMBIOS
> driver which intersected with this one this needs to be thoroughly
> covered, tested, and thought through. Linus was generous in the
> number of attempts it took us to get that right.
> 
> Did DCDBAS ever work on a system without ACPI?
> 

It appears to compile ok without ACPI enabled... looks like acpi_get_table
just returns a constant when CONFIG_ACPI isn't there, which makes all the WSMT
stuff get optimized out.  So I don't guess we even need an "#ifdef CONFIG_ACPI".

>>
>>>  #include 
>>>  #include 
>>>  #include 
>>> +#include 
>>
>> Please, try to keep an order as much as possible.
>> For example, in given context this line should be before string.h (I
>> didn't check the actual file, perhaps even upper).
>>
>>>  #include 
>>>
>>>  #include "dcdbas.h"
>>
>>> /* Calling Interface SMI */
>>> -   smi_cmd->ebx = (u32) virt_to_phys(smi_cmd->command_buffer);
>>> +   smi_cmd->ebx = smi_data_buf_phys_addr
>>> +   + offsetof(struct smi_cmd, command_buffer);
>>
>> Please, keep at least + on the previous line.
>> Also, I'm not sure what is the difference now. Especially for previous
>> users when this wasn't the part of the driver.
>> Some explanation needed.
>>

I'll fix this.

>>> +static u8 checksum(u8 *buffer, u8 length)
>>> +{
>>> +   u8 sum = 0;
>>> +   u8 *end = buffer + length;
>>> +
>>> +   while (buffer < end)
>>> +   sum = (u8)(sum + *(buffer++));
>>
>> Why not simple
>>
>> sum += *buffer++;
>>
>> ?
>>
>>> +   return sum;
>>> +}
>>
>> And I would rather check if we have similar algoritms already in the
>> kernel which  we might re-use.
> 
> Seems to be some options in lib/checksum.c to check.
> 

I couldn't find anything in checksum.c or elsewhere that I could just
include that would do a byte checksum, not a word.  I copied this code
from acpi_tb_checksum (in drivers/acpi/acpica/tbprint.c), but I can
shorten it as suggested.

>>
>>> +
>>> +static inline struct smm_eps_table *check_eps_table(u8 *addr)
>>> +{
>>> +   struct smm_eps_table *eps = (struct smm_eps_table *)addr;
>>> +
>>
>>> +   if (strncmp(SMM_EPS_SIG, eps->smm_comm_buff_anchor, 4) != 0)
>>
>> I'm not sure about strings operation here.
>> I would rather do like with other magic constants: introduce hex value
>> and compare it as unsigned integer.
>>
>> Also, it might be a warning, since \0 wasn't ever checked from the
>> string literal. Though, I'm not sure if it applicable to strncmp()
>> function (it's for strncpy for sure).
> 
> I think we're OK here, and we're being consistent with the
> dell-wmi-descriptor test for "DELL WMI". I don't recall if it was that
> one or something else, but doing it in HEX ended up being more
> confusing. The \0 isn't an issue since strncmp will only compare the n
> (4) bytes.
> 
>>
>>> +   return NULL;
>>> +
>>> +   if (checksum(addr, eps->length) != 0)
>>> +   return NULL;
>>> +
>>> +   return eps;
>>> +}
>>> +
>>> +static int dcdbas_check_wsmt(void)
>>> +{
>>> +   struct acpi_table_wsmt *wsmt = NULL;
>>> +   struct smm_eps_table *eps = NULL;
>>> +   u8 *addr;
>>> +
>>> +   acpi_get_table(ACPI_SIG_WSMT, 0, (struct ac

Re: [PATCH resend v2] dcdbas: Add support for WSMT ACPI table

[Stuart Hayes]

On 6/8/2018 8:04 PM, Darren Hart wrote:
> On Thu, Jun 07, 2018 at 08:11:41PM +0300, Andy Shevchenko wrote:
>> On Thu, Jun 7, 2018 at 6:59 PM, Stuart Hayes  
>> wrote:
>>> If the WSMT ACPI table is present and indicates that a fixed communication
>>> buffer should be used, use the firmware-specified buffer instead of
>>> allocating a buffer in memory for communications between the dcdbas driver
>>> and firmare.
>>
>>>  config DCDBAS
>>> tristate "Dell Systems Management Base Driver"
>>> -   depends on X86
>>> +   depends on X86 && ACPI
> 
> 
>>
>> Hmm... I'm not sure about this dependency.
>> So, the question is do all users actually need this? How did it work
>> previously? How has this been tested in case when command line has
>> "acpi=off" (yes, this one just for sake of test, I don't believe it's
>> a real use case)?
> 
> Yeah... after the 4.16 and 4.17 KConfig fumbling around the SMBIOS
> driver which intersected with this one this needs to be thoroughly
> covered, tested, and thought through. Linus was generous in the
> number of attempts it took us to get that right.
> 
> Did DCDBAS ever work on a system without ACPI?
> 

It appears to compile ok without ACPI enabled... looks like acpi_get_table
just returns a constant when CONFIG_ACPI isn't there, which makes all the WSMT
stuff get optimized out.  So I don't guess we even need an "#ifdef CONFIG_ACPI".

>>
>>>  #include 
>>>  #include 
>>>  #include 
>>> +#include 
>>
>> Please, try to keep an order as much as possible.
>> For example, in given context this line should be before string.h (I
>> didn't check the actual file, perhaps even upper).
>>
>>>  #include 
>>>
>>>  #include "dcdbas.h"
>>
>>> /* Calling Interface SMI */
>>> -   smi_cmd->ebx = (u32) virt_to_phys(smi_cmd->command_buffer);
>>> +   smi_cmd->ebx = smi_data_buf_phys_addr
>>> +   + offsetof(struct smi_cmd, command_buffer);
>>
>> Please, keep at least + on the previous line.
>> Also, I'm not sure what is the difference now. Especially for previous
>> users when this wasn't the part of the driver.
>> Some explanation needed.
>>

I'll fix this.

>>> +static u8 checksum(u8 *buffer, u8 length)
>>> +{
>>> +   u8 sum = 0;
>>> +   u8 *end = buffer + length;
>>> +
>>> +   while (buffer < end)
>>> +   sum = (u8)(sum + *(buffer++));
>>
>> Why not simple
>>
>> sum += *buffer++;
>>
>> ?
>>
>>> +   return sum;
>>> +}
>>
>> And I would rather check if we have similar algoritms already in the
>> kernel which  we might re-use.
> 
> Seems to be some options in lib/checksum.c to check.
> 

I couldn't find anything in checksum.c or elsewhere that I could just
include that would do a byte checksum, not a word.  I copied this code
from acpi_tb_checksum (in drivers/acpi/acpica/tbprint.c), but I can
shorten it as suggested.

>>
>>> +
>>> +static inline struct smm_eps_table *check_eps_table(u8 *addr)
>>> +{
>>> +   struct smm_eps_table *eps = (struct smm_eps_table *)addr;
>>> +
>>
>>> +   if (strncmp(SMM_EPS_SIG, eps->smm_comm_buff_anchor, 4) != 0)
>>
>> I'm not sure about strings operation here.
>> I would rather do like with other magic constants: introduce hex value
>> and compare it as unsigned integer.
>>
>> Also, it might be a warning, since \0 wasn't ever checked from the
>> string literal. Though, I'm not sure if it applicable to strncmp()
>> function (it's for strncpy for sure).
> 
> I think we're OK here, and we're being consistent with the
> dell-wmi-descriptor test for "DELL WMI". I don't recall if it was that
> one or something else, but doing it in HEX ended up being more
> confusing. The \0 isn't an issue since strncmp will only compare the n
> (4) bytes.
> 
>>
>>> +   return NULL;
>>> +
>>> +   if (checksum(addr, eps->length) != 0)
>>> +   return NULL;
>>> +
>>> +   return eps;
>>> +}
>>> +
>>> +static int dcdbas_check_wsmt(void)
>>> +{
>>> +   struct acpi_table_wsmt *wsmt = NULL;
>>> +   struct smm_eps_table *eps = NULL;
>>> +   u8 *addr;
>>> +
>>> +   acpi_get_table(ACPI_SIG_WSMT, 0, (struct ac

[PATCH resend v2] dcdbas: Add support for WSMT ACPI table

[Stuart Hayes]

If the WSMT ACPI table is present and indicates that a fixed communication
buffer should be used, use the firmware-specified buffer instead of
allocating a buffer in memory for communications between the dcdbas driver
and firmare.

Signed-off-by: Stuart Hayes 
Tested-by: Mario Limonciello 
Acked-by: Doug Warzecha 
---
v2 Bumped driver version to 5.6.0-3.3


 drivers/firmware/Kconfig  |   2 +-
 drivers/firmware/dcdbas.c | 102 +++---
 drivers/firmware/dcdbas.h |  11 +
 3 files changed, 109 insertions(+), 6 deletions(-)

diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index b7c748248e53..a2bd6092bfa1 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -125,7 +125,7 @@ config DELL_RBU
 
 config DCDBAS
tristate "Dell Systems Management Base Driver"
-   depends on X86
+   depends on X86 && ACPI
help
  The Dell Systems Management Base Driver provides a sysfs interface
  for systems management software to perform System Management
diff --git a/drivers/firmware/dcdbas.c b/drivers/firmware/dcdbas.c
index 0bdea60c65dd..1699cfdcefe4 100644
--- a/drivers/firmware/dcdbas.c
+++ b/drivers/firmware/dcdbas.c
@@ -36,12 +36,13 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 
 #include "dcdbas.h"
 
 #define DRIVER_NAME"dcdbas"
-#define DRIVER_VERSION "5.6.0-3.2"
+#define DRIVER_VERSION "5.6.0-3.3"
 #define DRIVER_DESCRIPTION "Dell Systems Management Base Driver"
 
 static struct platform_device *dcdbas_pdev;
@@ -49,19 +50,23 @@ static struct platform_device *dcdbas_pdev;
 static u8 *smi_data_buf;
 static dma_addr_t smi_data_buf_handle;
 static unsigned long smi_data_buf_size;
+static unsigned long max_smi_data_buf_size = MAX_SMI_DATA_BUF_SIZE;
 static u32 smi_data_buf_phys_addr;
 static DEFINE_MUTEX(smi_data_lock);
+static u8 *eps_buffer;
 
 static unsigned int host_control_action;
 static unsigned int host_control_smi_type;
 static unsigned int host_control_on_shutdown;
 
+static bool wsmt_enabled;
+
 /**
  * smi_data_buf_free: free SMI data buffer
  */
 static void smi_data_buf_free(void)
 {
-   if (!smi_data_buf)
+   if (!smi_data_buf || wsmt_enabled)
return;
 
dev_dbg(_pdev->dev, "%s: phys: %x size: %lu\n",
@@ -86,7 +91,7 @@ static int smi_data_buf_realloc(unsigned long size)
if (smi_data_buf_size >= size)
return 0;
 
-   if (size > MAX_SMI_DATA_BUF_SIZE)
+   if (size > max_smi_data_buf_size)
return -EINVAL;
 
/* new buffer is needed */
@@ -169,7 +174,7 @@ static ssize_t smi_data_write(struct file *filp, struct 
kobject *kobj,
 {
ssize_t ret;
 
-   if ((pos + count) > MAX_SMI_DATA_BUF_SIZE)
+   if ((pos + count) > max_smi_data_buf_size)
return -EINVAL;
 
mutex_lock(_data_lock);
@@ -323,7 +328,8 @@ static ssize_t smi_request_store(struct device *dev,
break;
case 1:
/* Calling Interface SMI */
-   smi_cmd->ebx = (u32) virt_to_phys(smi_cmd->command_buffer);
+   smi_cmd->ebx = smi_data_buf_phys_addr
+   + offsetof(struct smi_cmd, command_buffer);
ret = dcdbas_smi_request(smi_cmd);
if (!ret)
ret = count;
@@ -482,6 +488,85 @@ static void dcdbas_host_control(void)
}
 }
 
+/* WSMT */
+
+static u8 checksum(u8 *buffer, u8 length)
+{
+   u8 sum = 0;
+   u8 *end = buffer + length;
+
+   while (buffer < end)
+   sum = (u8)(sum + *(buffer++));
+   return sum;
+}
+
+static inline struct smm_eps_table *check_eps_table(u8 *addr)
+{
+   struct smm_eps_table *eps = (struct smm_eps_table *)addr;
+
+   if (strncmp(SMM_EPS_SIG, eps->smm_comm_buff_anchor, 4) != 0)
+   return NULL;
+
+   if (checksum(addr, eps->length) != 0)
+   return NULL;
+
+   return eps;
+}
+
+static int dcdbas_check_wsmt(void)
+{
+   struct acpi_table_wsmt *wsmt = NULL;
+   struct smm_eps_table *eps = NULL;
+   u8 *addr;
+
+   acpi_get_table(ACPI_SIG_WSMT, 0, (struct acpi_table_header **));
+   if (!wsmt)
+   return 0;
+
+   /* Check if WSMT ACPI table shows that protection is enabled */
+   if (!(wsmt->protection_flags & ACPI_WSMT_FIXED_COMM_BUFFERS)
+   || !(wsmt->protection_flags
+& ACPI_WSMT_COMM_BUFFER_NESTED_PTR_PROTECTION))
+   return 0;
+
+   /* Scan for EPS (entry point structure) */
+   for (addr = (u8 *)__va(0xf);
+addr < (u8 *)__va(0x10 - sizeof(struct smm_eps_table)) && !eps;
+addr += 1)
+   eps = check_eps_table(addr);
+
+   if (!eps) {
+   dev_dbg(_pdev->dev, "fo

[PATCH resend v2] dcdbas: Add support for WSMT ACPI table

[Stuart Hayes]

If the WSMT ACPI table is present and indicates that a fixed communication
buffer should be used, use the firmware-specified buffer instead of
allocating a buffer in memory for communications between the dcdbas driver
and firmare.

Signed-off-by: Stuart Hayes 
Tested-by: Mario Limonciello 
Acked-by: Doug Warzecha 
---
v2 Bumped driver version to 5.6.0-3.3


 drivers/firmware/Kconfig  |   2 +-
 drivers/firmware/dcdbas.c | 102 +++---
 drivers/firmware/dcdbas.h |  11 +
 3 files changed, 109 insertions(+), 6 deletions(-)

diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index b7c748248e53..a2bd6092bfa1 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -125,7 +125,7 @@ config DELL_RBU
 
 config DCDBAS
tristate "Dell Systems Management Base Driver"
-   depends on X86
+   depends on X86 && ACPI
help
  The Dell Systems Management Base Driver provides a sysfs interface
  for systems management software to perform System Management
diff --git a/drivers/firmware/dcdbas.c b/drivers/firmware/dcdbas.c
index 0bdea60c65dd..1699cfdcefe4 100644
--- a/drivers/firmware/dcdbas.c
+++ b/drivers/firmware/dcdbas.c
@@ -36,12 +36,13 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 
 #include "dcdbas.h"
 
 #define DRIVER_NAME"dcdbas"
-#define DRIVER_VERSION "5.6.0-3.2"
+#define DRIVER_VERSION "5.6.0-3.3"
 #define DRIVER_DESCRIPTION "Dell Systems Management Base Driver"
 
 static struct platform_device *dcdbas_pdev;
@@ -49,19 +50,23 @@ static struct platform_device *dcdbas_pdev;
 static u8 *smi_data_buf;
 static dma_addr_t smi_data_buf_handle;
 static unsigned long smi_data_buf_size;
+static unsigned long max_smi_data_buf_size = MAX_SMI_DATA_BUF_SIZE;
 static u32 smi_data_buf_phys_addr;
 static DEFINE_MUTEX(smi_data_lock);
+static u8 *eps_buffer;
 
 static unsigned int host_control_action;
 static unsigned int host_control_smi_type;
 static unsigned int host_control_on_shutdown;
 
+static bool wsmt_enabled;
+
 /**
  * smi_data_buf_free: free SMI data buffer
  */
 static void smi_data_buf_free(void)
 {
-   if (!smi_data_buf)
+   if (!smi_data_buf || wsmt_enabled)
return;
 
dev_dbg(_pdev->dev, "%s: phys: %x size: %lu\n",
@@ -86,7 +91,7 @@ static int smi_data_buf_realloc(unsigned long size)
if (smi_data_buf_size >= size)
return 0;
 
-   if (size > MAX_SMI_DATA_BUF_SIZE)
+   if (size > max_smi_data_buf_size)
return -EINVAL;
 
/* new buffer is needed */
@@ -169,7 +174,7 @@ static ssize_t smi_data_write(struct file *filp, struct 
kobject *kobj,
 {
ssize_t ret;
 
-   if ((pos + count) > MAX_SMI_DATA_BUF_SIZE)
+   if ((pos + count) > max_smi_data_buf_size)
return -EINVAL;
 
mutex_lock(_data_lock);
@@ -323,7 +328,8 @@ static ssize_t smi_request_store(struct device *dev,
break;
case 1:
/* Calling Interface SMI */
-   smi_cmd->ebx = (u32) virt_to_phys(smi_cmd->command_buffer);
+   smi_cmd->ebx = smi_data_buf_phys_addr
+   + offsetof(struct smi_cmd, command_buffer);
ret = dcdbas_smi_request(smi_cmd);
if (!ret)
ret = count;
@@ -482,6 +488,85 @@ static void dcdbas_host_control(void)
}
 }
 
+/* WSMT */
+
+static u8 checksum(u8 *buffer, u8 length)
+{
+   u8 sum = 0;
+   u8 *end = buffer + length;
+
+   while (buffer < end)
+   sum = (u8)(sum + *(buffer++));
+   return sum;
+}
+
+static inline struct smm_eps_table *check_eps_table(u8 *addr)
+{
+   struct smm_eps_table *eps = (struct smm_eps_table *)addr;
+
+   if (strncmp(SMM_EPS_SIG, eps->smm_comm_buff_anchor, 4) != 0)
+   return NULL;
+
+   if (checksum(addr, eps->length) != 0)
+   return NULL;
+
+   return eps;
+}
+
+static int dcdbas_check_wsmt(void)
+{
+   struct acpi_table_wsmt *wsmt = NULL;
+   struct smm_eps_table *eps = NULL;
+   u8 *addr;
+
+   acpi_get_table(ACPI_SIG_WSMT, 0, (struct acpi_table_header **));
+   if (!wsmt)
+   return 0;
+
+   /* Check if WSMT ACPI table shows that protection is enabled */
+   if (!(wsmt->protection_flags & ACPI_WSMT_FIXED_COMM_BUFFERS)
+   || !(wsmt->protection_flags
+& ACPI_WSMT_COMM_BUFFER_NESTED_PTR_PROTECTION))
+   return 0;
+
+   /* Scan for EPS (entry point structure) */
+   for (addr = (u8 *)__va(0xf);
+addr < (u8 *)__va(0x10 - sizeof(struct smm_eps_table)) && !eps;
+addr += 1)
+   eps = check_eps_table(addr);
+
+   if (!eps) {
+   dev_dbg(_pdev->dev, "fo

[PATCH v2] dcdbas: Add support for WSMT ACPI table

[Stuart Hayes]

If the WSMT ACPI table is present and indicates that a fixed communication
buffer should be used, use the firmware-specified buffer instead of
allocating a buffer in memory for communications between the dcdbas driver
and firmare.

Signed-off-by: Stuart Hayes <stuart.w.ha...@gmail.com>
---
v2 Bumped driver version to 5.6.0-3.3


 drivers/firmware/Kconfig  |   2 +-
 drivers/firmware/dcdbas.c | 102 +++---
 drivers/firmware/dcdbas.h |  11 +
 3 files changed, 109 insertions(+), 6 deletions(-)

diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index b7c748248e53..a2bd6092bfa1 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -125,7 +125,7 @@ config DELL_RBU
 
 config DCDBAS
tristate "Dell Systems Management Base Driver"
-   depends on X86
+   depends on X86 && ACPI
help
  The Dell Systems Management Base Driver provides a sysfs interface
  for systems management software to perform System Management
diff --git a/drivers/firmware/dcdbas.c b/drivers/firmware/dcdbas.c
index 0bdea60c65dd..1699cfdcefe4 100644
--- a/drivers/firmware/dcdbas.c
+++ b/drivers/firmware/dcdbas.c
@@ -36,12 +36,13 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 
 #include "dcdbas.h"
 
 #define DRIVER_NAME"dcdbas"
-#define DRIVER_VERSION "5.6.0-3.2"
+#define DRIVER_VERSION "5.6.0-3.3"
 #define DRIVER_DESCRIPTION "Dell Systems Management Base Driver"
 
 static struct platform_device *dcdbas_pdev;
@@ -49,19 +50,23 @@ static struct platform_device *dcdbas_pdev;
 static u8 *smi_data_buf;
 static dma_addr_t smi_data_buf_handle;
 static unsigned long smi_data_buf_size;
+static unsigned long max_smi_data_buf_size = MAX_SMI_DATA_BUF_SIZE;
 static u32 smi_data_buf_phys_addr;
 static DEFINE_MUTEX(smi_data_lock);
+static u8 *eps_buffer;
 
 static unsigned int host_control_action;
 static unsigned int host_control_smi_type;
 static unsigned int host_control_on_shutdown;
 
+static bool wsmt_enabled;
+
 /**
  * smi_data_buf_free: free SMI data buffer
  */
 static void smi_data_buf_free(void)
 {
-   if (!smi_data_buf)
+   if (!smi_data_buf || wsmt_enabled)
return;
 
dev_dbg(_pdev->dev, "%s: phys: %x size: %lu\n",
@@ -86,7 +91,7 @@ static int smi_data_buf_realloc(unsigned long size)
if (smi_data_buf_size >= size)
return 0;
 
-   if (size > MAX_SMI_DATA_BUF_SIZE)
+   if (size > max_smi_data_buf_size)
return -EINVAL;
 
/* new buffer is needed */
@@ -169,7 +174,7 @@ static ssize_t smi_data_write(struct file *filp, struct 
kobject *kobj,
 {
ssize_t ret;
 
-   if ((pos + count) > MAX_SMI_DATA_BUF_SIZE)
+   if ((pos + count) > max_smi_data_buf_size)
return -EINVAL;
 
mutex_lock(_data_lock);
@@ -323,7 +328,8 @@ static ssize_t smi_request_store(struct device *dev,
break;
case 1:
/* Calling Interface SMI */
-   smi_cmd->ebx = (u32) virt_to_phys(smi_cmd->command_buffer);
+   smi_cmd->ebx = smi_data_buf_phys_addr
+   + offsetof(struct smi_cmd, command_buffer);
ret = dcdbas_smi_request(smi_cmd);
if (!ret)
ret = count;
@@ -482,6 +488,85 @@ static void dcdbas_host_control(void)
}
 }
 
+/* WSMT */
+
+static u8 checksum(u8 *buffer, u8 length)
+{
+   u8 sum = 0;
+   u8 *end = buffer + length;
+
+   while (buffer < end)
+   sum = (u8)(sum + *(buffer++));
+   return sum;
+}
+
+static inline struct smm_eps_table *check_eps_table(u8 *addr)
+{
+   struct smm_eps_table *eps = (struct smm_eps_table *)addr;
+
+   if (strncmp(SMM_EPS_SIG, eps->smm_comm_buff_anchor, 4) != 0)
+   return NULL;
+
+   if (checksum(addr, eps->length) != 0)
+   return NULL;
+
+   return eps;
+}
+
+static int dcdbas_check_wsmt(void)
+{
+   struct acpi_table_wsmt *wsmt = NULL;
+   struct smm_eps_table *eps = NULL;
+   u8 *addr;
+
+   acpi_get_table(ACPI_SIG_WSMT, 0, (struct acpi_table_header **));
+   if (!wsmt)
+   return 0;
+
+   /* Check if WSMT ACPI table shows that protection is enabled */
+   if (!(wsmt->protection_flags & ACPI_WSMT_FIXED_COMM_BUFFERS)
+   || !(wsmt->protection_flags
+& ACPI_WSMT_COMM_BUFFER_NESTED_PTR_PROTECTION))
+   return 0;
+
+   /* Scan for EPS (entry point structure) */
+   for (addr = (u8 *)__va(0xf);
+addr < (u8 *)__va(0x10 - sizeof(struct smm_eps_table)) && !eps;
+addr += 1)
+   eps = check_eps_table(addr);
+
+   if (!eps) {
+   dev_dbg(_pdev->dev, "found WSMT, but no EPS found

[PATCH v2] dcdbas: Add support for WSMT ACPI table

[Stuart Hayes]

If the WSMT ACPI table is present and indicates that a fixed communication
buffer should be used, use the firmware-specified buffer instead of
allocating a buffer in memory for communications between the dcdbas driver
and firmare.

Signed-off-by: Stuart Hayes 
---
v2 Bumped driver version to 5.6.0-3.3


 drivers/firmware/Kconfig  |   2 +-
 drivers/firmware/dcdbas.c | 102 +++---
 drivers/firmware/dcdbas.h |  11 +
 3 files changed, 109 insertions(+), 6 deletions(-)

diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index b7c748248e53..a2bd6092bfa1 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -125,7 +125,7 @@ config DELL_RBU
 
 config DCDBAS
tristate "Dell Systems Management Base Driver"
-   depends on X86
+   depends on X86 && ACPI
help
  The Dell Systems Management Base Driver provides a sysfs interface
  for systems management software to perform System Management
diff --git a/drivers/firmware/dcdbas.c b/drivers/firmware/dcdbas.c
index 0bdea60c65dd..1699cfdcefe4 100644
--- a/drivers/firmware/dcdbas.c
+++ b/drivers/firmware/dcdbas.c
@@ -36,12 +36,13 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 
 #include "dcdbas.h"
 
 #define DRIVER_NAME"dcdbas"
-#define DRIVER_VERSION "5.6.0-3.2"
+#define DRIVER_VERSION "5.6.0-3.3"
 #define DRIVER_DESCRIPTION "Dell Systems Management Base Driver"
 
 static struct platform_device *dcdbas_pdev;
@@ -49,19 +50,23 @@ static struct platform_device *dcdbas_pdev;
 static u8 *smi_data_buf;
 static dma_addr_t smi_data_buf_handle;
 static unsigned long smi_data_buf_size;
+static unsigned long max_smi_data_buf_size = MAX_SMI_DATA_BUF_SIZE;
 static u32 smi_data_buf_phys_addr;
 static DEFINE_MUTEX(smi_data_lock);
+static u8 *eps_buffer;
 
 static unsigned int host_control_action;
 static unsigned int host_control_smi_type;
 static unsigned int host_control_on_shutdown;
 
+static bool wsmt_enabled;
+
 /**
  * smi_data_buf_free: free SMI data buffer
  */
 static void smi_data_buf_free(void)
 {
-   if (!smi_data_buf)
+   if (!smi_data_buf || wsmt_enabled)
return;
 
dev_dbg(_pdev->dev, "%s: phys: %x size: %lu\n",
@@ -86,7 +91,7 @@ static int smi_data_buf_realloc(unsigned long size)
if (smi_data_buf_size >= size)
return 0;
 
-   if (size > MAX_SMI_DATA_BUF_SIZE)
+   if (size > max_smi_data_buf_size)
return -EINVAL;
 
/* new buffer is needed */
@@ -169,7 +174,7 @@ static ssize_t smi_data_write(struct file *filp, struct 
kobject *kobj,
 {
ssize_t ret;
 
-   if ((pos + count) > MAX_SMI_DATA_BUF_SIZE)
+   if ((pos + count) > max_smi_data_buf_size)
return -EINVAL;
 
mutex_lock(_data_lock);
@@ -323,7 +328,8 @@ static ssize_t smi_request_store(struct device *dev,
break;
case 1:
/* Calling Interface SMI */
-   smi_cmd->ebx = (u32) virt_to_phys(smi_cmd->command_buffer);
+   smi_cmd->ebx = smi_data_buf_phys_addr
+   + offsetof(struct smi_cmd, command_buffer);
ret = dcdbas_smi_request(smi_cmd);
if (!ret)
ret = count;
@@ -482,6 +488,85 @@ static void dcdbas_host_control(void)
}
 }
 
+/* WSMT */
+
+static u8 checksum(u8 *buffer, u8 length)
+{
+   u8 sum = 0;
+   u8 *end = buffer + length;
+
+   while (buffer < end)
+   sum = (u8)(sum + *(buffer++));
+   return sum;
+}
+
+static inline struct smm_eps_table *check_eps_table(u8 *addr)
+{
+   struct smm_eps_table *eps = (struct smm_eps_table *)addr;
+
+   if (strncmp(SMM_EPS_SIG, eps->smm_comm_buff_anchor, 4) != 0)
+   return NULL;
+
+   if (checksum(addr, eps->length) != 0)
+   return NULL;
+
+   return eps;
+}
+
+static int dcdbas_check_wsmt(void)
+{
+   struct acpi_table_wsmt *wsmt = NULL;
+   struct smm_eps_table *eps = NULL;
+   u8 *addr;
+
+   acpi_get_table(ACPI_SIG_WSMT, 0, (struct acpi_table_header **));
+   if (!wsmt)
+   return 0;
+
+   /* Check if WSMT ACPI table shows that protection is enabled */
+   if (!(wsmt->protection_flags & ACPI_WSMT_FIXED_COMM_BUFFERS)
+   || !(wsmt->protection_flags
+& ACPI_WSMT_COMM_BUFFER_NESTED_PTR_PROTECTION))
+   return 0;
+
+   /* Scan for EPS (entry point structure) */
+   for (addr = (u8 *)__va(0xf);
+addr < (u8 *)__va(0x10 - sizeof(struct smm_eps_table)) && !eps;
+addr += 1)
+   eps = check_eps_table(addr);
+
+   if (!eps) {
+   dev_dbg(_pdev->dev, "found WSMT, but no EPS found\n");
+   

Re: [PATCH RESENT v4] dell_rbu: make firmware payload memory uncachable

[Stuart Hayes]

On 4/18/2018 12:46 AM, Takashi Iwai wrote:
> From: Stuart Hayes <stuart.w.ha...@gmail.com>
> 
> The dell_rbu driver takes firmware update payloads and puts them in memory so
> the system BIOS can find them after a reboot.  This sometimes fails (though
> rarely), because the memory containing the payload is in the CPU cache but
> never gets written back to main memory before the system is rebooted (CPU
> cache contents are lost on reboot).
> 
> With this patch, the payload memory will be changed to uncachable to ensure
> that the payload is actually in main memory before the system is rebooted.
> 
> Signed-off-by: Stuart Hayes <stuart.w.ha...@gmail.com>
> Reviewed-by: Takashi Iwai <ti...@suse.de>
> Signed-off-by: Takashi Iwai <ti...@suse.de>
> ---
> v2 Added include, removed extra parentheses
> v3 Corrected formatting and include line
> v4 Moved set_memory_uc() outside the while loop so that the memory is
>definitely allocated before it is set to uncachable
> 
> Andrew, could you pick up this orphan one?  Thanks!
> 
> diff --git a/drivers/firmware/dell_rbu.c b/drivers/firmware/dell_rbu.c
> index 2f452f1f7c8a..53f27a6e2d76 100644
> --- a/drivers/firmware/dell_rbu.c
> +++ b/drivers/firmware/dell_rbu.c
> @@ -45,6 +45,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  MODULE_AUTHOR("Abhay Salunke <abhay_salu...@dell.com>");
>  MODULE_DESCRIPTION("Driver for updating BIOS image on DELL systems");
> @@ -181,6 +182,11 @@ static int create_packet(void *data, size_t length)
>   packet_data_temp_buf = NULL;
>   }
>   }
> + /*
> +  * set to uncachable or it may never get written back before reboot
> +  */
> + set_memory_uc((unsigned long)packet_data_temp_buf, 1 << ordernum);
> +
>   spin_lock(_data.lock);
>  
>   newpacket->data = packet_data_temp_buf;
> @@ -349,6 +355,8 @@ static void packet_empty_list(void)
>* to make sure there are no stale RBU packets left in memory
>*/
>   memset(newpacket->data, 0, rbu_data.packetsize);
> + set_memory_wb((unsigned long)newpacket->data,
> + 1 << newpacket->ordernum);
>   free_pages((unsigned long) newpacket->data,
>   newpacket->ordernum);
>   kfree(newpacket);
> 

Just a reminder... I don't think this has been picked up yet.  Please let me 
know if there's anything I could do.
Thanks!
Stuart

Re: [PATCH RESENT v4] dell_rbu: make firmware payload memory uncachable

[Stuart Hayes]

On 4/18/2018 12:46 AM, Takashi Iwai wrote:
> From: Stuart Hayes 
> 
> The dell_rbu driver takes firmware update payloads and puts them in memory so
> the system BIOS can find them after a reboot.  This sometimes fails (though
> rarely), because the memory containing the payload is in the CPU cache but
> never gets written back to main memory before the system is rebooted (CPU
> cache contents are lost on reboot).
> 
> With this patch, the payload memory will be changed to uncachable to ensure
> that the payload is actually in main memory before the system is rebooted.
> 
> Signed-off-by: Stuart Hayes 
> Reviewed-by: Takashi Iwai 
> Signed-off-by: Takashi Iwai 
> ---
> v2 Added include, removed extra parentheses
> v3 Corrected formatting and include line
> v4 Moved set_memory_uc() outside the while loop so that the memory is
>definitely allocated before it is set to uncachable
> 
> Andrew, could you pick up this orphan one?  Thanks!
> 
> diff --git a/drivers/firmware/dell_rbu.c b/drivers/firmware/dell_rbu.c
> index 2f452f1f7c8a..53f27a6e2d76 100644
> --- a/drivers/firmware/dell_rbu.c
> +++ b/drivers/firmware/dell_rbu.c
> @@ -45,6 +45,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  MODULE_AUTHOR("Abhay Salunke ");
>  MODULE_DESCRIPTION("Driver for updating BIOS image on DELL systems");
> @@ -181,6 +182,11 @@ static int create_packet(void *data, size_t length)
>   packet_data_temp_buf = NULL;
>   }
>   }
> + /*
> +  * set to uncachable or it may never get written back before reboot
> +  */
> + set_memory_uc((unsigned long)packet_data_temp_buf, 1 << ordernum);
> +
>   spin_lock(_data.lock);
>  
>   newpacket->data = packet_data_temp_buf;
> @@ -349,6 +355,8 @@ static void packet_empty_list(void)
>* to make sure there are no stale RBU packets left in memory
>*/
>   memset(newpacket->data, 0, rbu_data.packetsize);
> + set_memory_wb((unsigned long)newpacket->data,
> + 1 << newpacket->ordernum);
>   free_pages((unsigned long) newpacket->data,
>   newpacket->ordernum);
>   kfree(newpacket);
> 

Just a reminder... I don't think this has been picked up yet.  Please let me 
know if there's anything I could do.
Thanks!
Stuart

[PATCH] dcdbas: Add support for WSMT ACPI table

[Stuart Hayes]

If the WSMT ACPI table is present and indicates that a fixed communication
buffer should be used, use the firmware-specified buffer instead of
allocating a buffer in memory for communications between the dcdbas driver
and firmware.

Signed-off-by: Stuart Hayes <stuart.w.ha...@gmail.com>
Reviewed-by: Mario Limonciello <mario.limoncie...@dell.com>
---
 drivers/firmware/Kconfig  |   2 +-
 drivers/firmware/dcdbas.c | 100 --
 drivers/firmware/dcdbas.h |  11 +
 3 files changed, 108 insertions(+), 5 deletions(-)

diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index b7c748248e53..a2bd6092bfa1 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -125,7 +125,7 @@ config DELL_RBU
 
 config DCDBAS
tristate "Dell Systems Management Base Driver"
-   depends on X86
+   depends on X86 && ACPI
help
  The Dell Systems Management Base Driver provides a sysfs interface
  for systems management software to perform System Management
diff --git a/drivers/firmware/dcdbas.c b/drivers/firmware/dcdbas.c
index 0bdea60c65dd..cdcc10d5f04b 100644
--- a/drivers/firmware/dcdbas.c
+++ b/drivers/firmware/dcdbas.c
@@ -36,6 +36,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 
 #include "dcdbas.h"
@@ -49,19 +50,23 @@ static struct platform_device *dcdbas_pdev;
 static u8 *smi_data_buf;
 static dma_addr_t smi_data_buf_handle;
 static unsigned long smi_data_buf_size;
+static unsigned long max_smi_data_buf_size = MAX_SMI_DATA_BUF_SIZE;
 static u32 smi_data_buf_phys_addr;
 static DEFINE_MUTEX(smi_data_lock);
+static u8 *eps_buffer;
 
 static unsigned int host_control_action;
 static unsigned int host_control_smi_type;
 static unsigned int host_control_on_shutdown;
 
+static bool wsmt_enabled;
+
 /**
  * smi_data_buf_free: free SMI data buffer
  */
 static void smi_data_buf_free(void)
 {
-   if (!smi_data_buf)
+   if (!smi_data_buf || wsmt_enabled)
return;
 
dev_dbg(_pdev->dev, "%s: phys: %x size: %lu\n",
@@ -86,7 +91,7 @@ static int smi_data_buf_realloc(unsigned long size)
if (smi_data_buf_size >= size)
return 0;
 
-   if (size > MAX_SMI_DATA_BUF_SIZE)
+   if (size > max_smi_data_buf_size)
return -EINVAL;
 
/* new buffer is needed */
@@ -169,7 +174,7 @@ static ssize_t smi_data_write(struct file *filp, struct 
kobject *kobj,
 {
ssize_t ret;
 
-   if ((pos + count) > MAX_SMI_DATA_BUF_SIZE)
+   if ((pos + count) > max_smi_data_buf_size)
return -EINVAL;
 
mutex_lock(_data_lock);
@@ -323,7 +328,8 @@ static ssize_t smi_request_store(struct device *dev,
break;
case 1:
/* Calling Interface SMI */
-   smi_cmd->ebx = (u32) virt_to_phys(smi_cmd->command_buffer);
+   smi_cmd->ebx = smi_data_buf_phys_addr
+   + offsetof(struct smi_cmd, command_buffer);
ret = dcdbas_smi_request(smi_cmd);
if (!ret)
ret = count;
@@ -482,6 +488,85 @@ static void dcdbas_host_control(void)
}
 }
 
+/* WSMT */
+
+static u8 checksum(u8 *buffer, u8 length)
+{
+   u8 sum = 0;
+   u8 *end = buffer + length;
+
+   while (buffer < end)
+   sum = (u8)(sum + *(buffer++));
+   return sum;
+}
+
+static inline struct smm_eps_table *check_eps_table(u8 *addr)
+{
+   struct smm_eps_table *eps = (struct smm_eps_table *)addr;
+
+   if (strncmp(SMM_EPS_SIG, eps->smm_comm_buff_anchor, 4) != 0)
+   return NULL;
+
+   if (checksum(addr, eps->length) != 0)
+   return NULL;
+
+   return eps;
+}
+
+static int dcdbas_check_wsmt(void)
+{
+   struct acpi_table_wsmt *wsmt = NULL;
+   struct smm_eps_table *eps = NULL;
+   u8 *addr;
+
+   acpi_get_table(ACPI_SIG_WSMT, 0, (struct acpi_table_header **));
+   if (!wsmt)
+   return 0;
+
+   /* Check if WSMT ACPI table shows that protection is enabled */
+   if (!(wsmt->protection_flags & ACPI_WSMT_FIXED_COMM_BUFFERS)
+   || !(wsmt->protection_flags
+& ACPI_WSMT_COMM_BUFFER_NESTED_PTR_PROTECTION))
+   return 0;
+
+   /* Scan for EPS (entry point structure) */
+   for (addr = (u8 *)__va(0xf);
+addr < (u8 *)__va(0x10 - sizeof(struct smm_eps_table)) && !eps;
+addr += 1)
+   eps = check_eps_table(addr);
+
+   if (!eps) {
+   dev_dbg(_pdev->dev, "found WSMT, but no EPS found\n");
+   return -ENODEV;
+   }
+
+   /*
+* Get physical address of buffer and map to virtual address.
+* Table gives size in 4K pages, regardless of actual system page size.
+*/
+   if (eps-&

[PATCH] dcdbas: Add support for WSMT ACPI table

[Stuart Hayes]

If the WSMT ACPI table is present and indicates that a fixed communication
buffer should be used, use the firmware-specified buffer instead of
allocating a buffer in memory for communications between the dcdbas driver
and firmware.

Signed-off-by: Stuart Hayes 
Reviewed-by: Mario Limonciello 
---
 drivers/firmware/Kconfig  |   2 +-
 drivers/firmware/dcdbas.c | 100 --
 drivers/firmware/dcdbas.h |  11 +
 3 files changed, 108 insertions(+), 5 deletions(-)

diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index b7c748248e53..a2bd6092bfa1 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -125,7 +125,7 @@ config DELL_RBU
 
 config DCDBAS
tristate "Dell Systems Management Base Driver"
-   depends on X86
+   depends on X86 && ACPI
help
  The Dell Systems Management Base Driver provides a sysfs interface
  for systems management software to perform System Management
diff --git a/drivers/firmware/dcdbas.c b/drivers/firmware/dcdbas.c
index 0bdea60c65dd..cdcc10d5f04b 100644
--- a/drivers/firmware/dcdbas.c
+++ b/drivers/firmware/dcdbas.c
@@ -36,6 +36,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 
 #include "dcdbas.h"
@@ -49,19 +50,23 @@ static struct platform_device *dcdbas_pdev;
 static u8 *smi_data_buf;
 static dma_addr_t smi_data_buf_handle;
 static unsigned long smi_data_buf_size;
+static unsigned long max_smi_data_buf_size = MAX_SMI_DATA_BUF_SIZE;
 static u32 smi_data_buf_phys_addr;
 static DEFINE_MUTEX(smi_data_lock);
+static u8 *eps_buffer;
 
 static unsigned int host_control_action;
 static unsigned int host_control_smi_type;
 static unsigned int host_control_on_shutdown;
 
+static bool wsmt_enabled;
+
 /**
  * smi_data_buf_free: free SMI data buffer
  */
 static void smi_data_buf_free(void)
 {
-   if (!smi_data_buf)
+   if (!smi_data_buf || wsmt_enabled)
return;
 
dev_dbg(_pdev->dev, "%s: phys: %x size: %lu\n",
@@ -86,7 +91,7 @@ static int smi_data_buf_realloc(unsigned long size)
if (smi_data_buf_size >= size)
return 0;
 
-   if (size > MAX_SMI_DATA_BUF_SIZE)
+   if (size > max_smi_data_buf_size)
return -EINVAL;
 
/* new buffer is needed */
@@ -169,7 +174,7 @@ static ssize_t smi_data_write(struct file *filp, struct 
kobject *kobj,
 {
ssize_t ret;
 
-   if ((pos + count) > MAX_SMI_DATA_BUF_SIZE)
+   if ((pos + count) > max_smi_data_buf_size)
return -EINVAL;
 
mutex_lock(_data_lock);
@@ -323,7 +328,8 @@ static ssize_t smi_request_store(struct device *dev,
break;
case 1:
/* Calling Interface SMI */
-   smi_cmd->ebx = (u32) virt_to_phys(smi_cmd->command_buffer);
+   smi_cmd->ebx = smi_data_buf_phys_addr
+   + offsetof(struct smi_cmd, command_buffer);
ret = dcdbas_smi_request(smi_cmd);
if (!ret)
ret = count;
@@ -482,6 +488,85 @@ static void dcdbas_host_control(void)
}
 }
 
+/* WSMT */
+
+static u8 checksum(u8 *buffer, u8 length)
+{
+   u8 sum = 0;
+   u8 *end = buffer + length;
+
+   while (buffer < end)
+   sum = (u8)(sum + *(buffer++));
+   return sum;
+}
+
+static inline struct smm_eps_table *check_eps_table(u8 *addr)
+{
+   struct smm_eps_table *eps = (struct smm_eps_table *)addr;
+
+   if (strncmp(SMM_EPS_SIG, eps->smm_comm_buff_anchor, 4) != 0)
+   return NULL;
+
+   if (checksum(addr, eps->length) != 0)
+   return NULL;
+
+   return eps;
+}
+
+static int dcdbas_check_wsmt(void)
+{
+   struct acpi_table_wsmt *wsmt = NULL;
+   struct smm_eps_table *eps = NULL;
+   u8 *addr;
+
+   acpi_get_table(ACPI_SIG_WSMT, 0, (struct acpi_table_header **));
+   if (!wsmt)
+   return 0;
+
+   /* Check if WSMT ACPI table shows that protection is enabled */
+   if (!(wsmt->protection_flags & ACPI_WSMT_FIXED_COMM_BUFFERS)
+   || !(wsmt->protection_flags
+& ACPI_WSMT_COMM_BUFFER_NESTED_PTR_PROTECTION))
+   return 0;
+
+   /* Scan for EPS (entry point structure) */
+   for (addr = (u8 *)__va(0xf);
+addr < (u8 *)__va(0x10 - sizeof(struct smm_eps_table)) && !eps;
+addr += 1)
+   eps = check_eps_table(addr);
+
+   if (!eps) {
+   dev_dbg(_pdev->dev, "found WSMT, but no EPS found\n");
+   return -ENODEV;
+   }
+
+   /*
+* Get physical address of buffer and map to virtual address.
+* Table gives size in 4K pages, regardless of actual system page size.
+*/
+   if (eps->smm_comm_buff_addr + 8 > U32_MAX) {
+   dev_warn(_pdev->dev, 

[PATCH v4] dell_rbu: make firmware payload memory uncachable

[Stuart Hayes]

The dell_rbu driver takes firmware update payloads and puts them in memory so
the system BIOS can find them after a reboot.  This sometimes fails (though
rarely), because the memory containing the payload is in the CPU cache but
never gets written back to main memory before the system is rebooted (CPU
cache contents are lost on reboot).

With this patch, the payload memory will be changed to uncachable to ensure
that the payload is actually in main memory before the system is rebooted.

Signed-off-by: Stuart Hayes <stuart.w.ha...@gmail.com>
---
v2 Added include, removed extra parentheses
v3 Corrected formatting and include line
v4 Moved set_memory_uc() outside the while loop so that the memory is
   definitely allocated before it is set to uncachable

This driver has no maintainer.


diff --git a/drivers/firmware/dell_rbu.c b/drivers/firmware/dell_rbu.c
index 2f452f1f7c8a..53f27a6e2d76 100644
--- a/drivers/firmware/dell_rbu.c
+++ b/drivers/firmware/dell_rbu.c
@@ -45,6 +45,7 @@
 #include 
 #include 
 #include 
+#include 
 
 MODULE_AUTHOR("Abhay Salunke <abhay_salu...@dell.com>");
 MODULE_DESCRIPTION("Driver for updating BIOS image on DELL systems");
@@ -181,6 +182,11 @@ static int create_packet(void *data, size_t length)
packet_data_temp_buf = NULL;
}
}
+   /*
+* set to uncachable or it may never get written back before reboot
+*/
+   set_memory_uc((unsigned long)packet_data_temp_buf, 1 << ordernum);
+
spin_lock(_data.lock);
 
newpacket->data = packet_data_temp_buf;
@@ -349,6 +355,8 @@ static void packet_empty_list(void)
 * to make sure there are no stale RBU packets left in memory
 */
memset(newpacket->data, 0, rbu_data.packetsize);
+   set_memory_wb((unsigned long)newpacket->data,
+   1 << newpacket->ordernum);
free_pages((unsigned long) newpacket->data,
newpacket->ordernum);
kfree(newpacket);

[PATCH v4] dell_rbu: make firmware payload memory uncachable

[Stuart Hayes]

The dell_rbu driver takes firmware update payloads and puts them in memory so
the system BIOS can find them after a reboot.  This sometimes fails (though
rarely), because the memory containing the payload is in the CPU cache but
never gets written back to main memory before the system is rebooted (CPU
cache contents are lost on reboot).

With this patch, the payload memory will be changed to uncachable to ensure
that the payload is actually in main memory before the system is rebooted.

Signed-off-by: Stuart Hayes 
---
v2 Added include, removed extra parentheses
v3 Corrected formatting and include line
v4 Moved set_memory_uc() outside the while loop so that the memory is
   definitely allocated before it is set to uncachable

This driver has no maintainer.


diff --git a/drivers/firmware/dell_rbu.c b/drivers/firmware/dell_rbu.c
index 2f452f1f7c8a..53f27a6e2d76 100644
--- a/drivers/firmware/dell_rbu.c
+++ b/drivers/firmware/dell_rbu.c
@@ -45,6 +45,7 @@
 #include 
 #include 
 #include 
+#include 
 
 MODULE_AUTHOR("Abhay Salunke ");
 MODULE_DESCRIPTION("Driver for updating BIOS image on DELL systems");
@@ -181,6 +182,11 @@ static int create_packet(void *data, size_t length)
packet_data_temp_buf = NULL;
}
}
+   /*
+* set to uncachable or it may never get written back before reboot
+*/
+   set_memory_uc((unsigned long)packet_data_temp_buf, 1 << ordernum);
+
spin_lock(_data.lock);
 
newpacket->data = packet_data_temp_buf;
@@ -349,6 +355,8 @@ static void packet_empty_list(void)
 * to make sure there are no stale RBU packets left in memory
 */
memset(newpacket->data, 0, rbu_data.packetsize);
+   set_memory_wb((unsigned long)newpacket->data,
+   1 << newpacket->ordernum);
free_pages((unsigned long) newpacket->data,
newpacket->ordernum);
kfree(newpacket);

Re: [PATCH v3] dell_rbu: make firmware payload memory uncachable

[Stuart Hayes]

On 4/4/2018 3:30 PM, Takashi Iwai wrote:
> On Wed, 28 Mar 2018 17:07:47 +0200,
> Stuart Hayes wrote:
>>
>> @@ -180,6 +181,12 @@ static int create_packet(void *data, size_t length)
>>  invalid_addr_packet_array[idx++] = packet_data_temp_buf;
>>  packet_data_temp_buf = NULL;
>>  }
>> +/*
>> + * set to uncachable or it may never get written back before
>> + * reboot
>> + */
>> +set_memory_uc((unsigned long)packet_data_temp_buf,
>> +1 << ordernum);
> 
> Won't this cause Oops when the if-condition above meets?
> Namely packet_data_temp_buf is set to NULL there.
> 
> Maybe better to try a fault injection to check the error handling.
> 
> 
> thanks,
> 
> Takashi
> 

Yes, thank you for catching my mistake.
Stuart

Re: [PATCH v3] dell_rbu: make firmware payload memory uncachable

[Stuart Hayes]

On 4/4/2018 3:30 PM, Takashi Iwai wrote:
> On Wed, 28 Mar 2018 17:07:47 +0200,
> Stuart Hayes wrote:
>>
>> @@ -180,6 +181,12 @@ static int create_packet(void *data, size_t length)
>>  invalid_addr_packet_array[idx++] = packet_data_temp_buf;
>>  packet_data_temp_buf = NULL;
>>  }
>> +/*
>> + * set to uncachable or it may never get written back before
>> + * reboot
>> + */
>> +set_memory_uc((unsigned long)packet_data_temp_buf,
>> +1 << ordernum);
> 
> Won't this cause Oops when the if-condition above meets?
> Namely packet_data_temp_buf is set to NULL there.
> 
> Maybe better to try a fault injection to check the error handling.
> 
> 
> thanks,
> 
> Takashi
> 

Yes, thank you for catching my mistake.
Stuart

[PATCH v3] dell_rbu: make firmware payload memory uncachable

[Stuart Hayes]

The dell_rbu driver takes firmware update payloads and puts them in memory so
the system BIOS can find them after a reboot.  This sometimes fails (though
rarely), because the memory containing the payload is in the CPU cache but
never gets written back to main memory before the system is rebooted (CPU
cache contents are lost on reboot).

With this patch, the payload memory will be changed to uncachable to ensure
that the payload is actually in main memory before the system is rebooted.

Signed-off-by: Stuart Hayes <stuart.w.ha...@gmail.com>
---
v2 Added include, removed extra parentheses
v3 Corrected formatting and include line

This driver has no maintainer.


diff --git a/drivers/firmware/dell_rbu.c b/drivers/firmware/dell_rbu.c
index 2f452f1f7c8a..778efbb72c76 100644
--- a/drivers/firmware/dell_rbu.c
+++ b/drivers/firmware/dell_rbu.c
@@ -45,6 +45,7 @@
 #include 
 #include 
 #include 
+#include 
 
 MODULE_AUTHOR("Abhay Salunke <abhay_salu...@dell.com>");
 MODULE_DESCRIPTION("Driver for updating BIOS image on DELL systems");
@@ -180,6 +181,12 @@ static int create_packet(void *data, size_t length)
invalid_addr_packet_array[idx++] = packet_data_temp_buf;
packet_data_temp_buf = NULL;
}
+   /*
+* set to uncachable or it may never get written back before
+* reboot
+*/
+   set_memory_uc((unsigned long)packet_data_temp_buf,
+   1 << ordernum);
}
spin_lock(_data.lock);
 
@@ -349,6 +356,8 @@ static void packet_empty_list(void)
 * to make sure there are no stale RBU packets left in memory
 */
memset(newpacket->data, 0, rbu_data.packetsize);
+   set_memory_wb((unsigned long)newpacket->data,
+   1 << newpacket->ordernum);
free_pages((unsigned long) newpacket->data,
newpacket->ordernum);
kfree(newpacket);

[PATCH v3] dell_rbu: make firmware payload memory uncachable

[Stuart Hayes]

The dell_rbu driver takes firmware update payloads and puts them in memory so
the system BIOS can find them after a reboot.  This sometimes fails (though
rarely), because the memory containing the payload is in the CPU cache but
never gets written back to main memory before the system is rebooted (CPU
cache contents are lost on reboot).

With this patch, the payload memory will be changed to uncachable to ensure
that the payload is actually in main memory before the system is rebooted.

Signed-off-by: Stuart Hayes 
---
v2 Added include, removed extra parentheses
v3 Corrected formatting and include line

This driver has no maintainer.


diff --git a/drivers/firmware/dell_rbu.c b/drivers/firmware/dell_rbu.c
index 2f452f1f7c8a..778efbb72c76 100644
--- a/drivers/firmware/dell_rbu.c
+++ b/drivers/firmware/dell_rbu.c
@@ -45,6 +45,7 @@
 #include 
 #include 
 #include 
+#include 
 
 MODULE_AUTHOR("Abhay Salunke ");
 MODULE_DESCRIPTION("Driver for updating BIOS image on DELL systems");
@@ -180,6 +181,12 @@ static int create_packet(void *data, size_t length)
invalid_addr_packet_array[idx++] = packet_data_temp_buf;
packet_data_temp_buf = NULL;
}
+   /*
+* set to uncachable or it may never get written back before
+* reboot
+*/
+   set_memory_uc((unsigned long)packet_data_temp_buf,
+   1 << ordernum);
}
spin_lock(_data.lock);
 
@@ -349,6 +356,8 @@ static void packet_empty_list(void)
 * to make sure there are no stale RBU packets left in memory
 */
memset(newpacket->data, 0, rbu_data.packetsize);
+   set_memory_wb((unsigned long)newpacket->data,
+   1 << newpacket->ordernum);
free_pages((unsigned long) newpacket->data,
newpacket->ordernum);
kfree(newpacket);

Re: [PATCH v2] dell_rbu: make firmware payload memory uncachable

[Stuart Hayes]

Please disregard this version.  I'm sorry (and intensely embarrassed).

 

Re: [PATCH v2] dell_rbu: make firmware payload memory uncachable

[Stuart Hayes]

Please disregard this version.  I'm sorry (and intensely embarrassed).

 

[PATCH v2] dell_rbu: make firmware payload memory uncachable

[Stuart Hayes]

The dell_rbu driver takes firmware update payloads and puts them in memory so
the system BIOS can find them after a reboot.  This sometimes fails (though
rarely), because the memory containing the payload is in the CPU cache but
never gets written back to main memory before the system is rebooted (CPU
cache contents are lost on reboot).

With this patch, the payload memory will be changed to uncachable to ensure
that the payload is actually in main memory before the system is rebooted.

Signed-off-by: Stuart Hayes <stuart.w.ha...@gmail.com>
---
v2 Added include, removed extra parentheses
This patch has no maintainer.


diff --git a/drivers/firmware/dell_rbu.c b/drivers/firmware/dell_rbu.c
index 2f452f1f7c8a..640a88c3418d 100644
--- a/drivers/firmware/dell_rbu.c
+++ b/drivers/firmware/dell_rbu.c
@@ -45,6 +45,7 @@
 #include 
 #include 
 #include 
+#include 
 
 MODULE_AUTHOR("Abhay Salunke <abhay_salu...@dell.com>");
 MODULE_DESCRIPTION("Driver for updating BIOS image on DELL systems");
@@ -180,6 +181,11 @@ static int create_packet(void *data, size_t length)
invalid_addr_packet_array[idx++] = packet_data_temp_buf;
packet_data_temp_buf = NULL;
}
+   /*
+* set to uncachable or it may never get written back before
+* reboot
+*/
+   set_memory_uc(packet_data_temp_buf, 1 << ordernum);
}
spin_lock(_data.lock);
 
@@ -349,6 +355,7 @@ static void packet_empty_list(void)
 * to make sure there are no stale RBU packets left in memory
 */
memset(newpacket->data, 0, rbu_data.packetsize);
+   set_memory_wb(newpacket->data, 1 << newpacket->ordernum);
free_pages((unsigned long) newpacket->data,
newpacket->ordernum);
kfree(newpacket);

[PATCH v2] dell_rbu: make firmware payload memory uncachable

[Stuart Hayes]

The dell_rbu driver takes firmware update payloads and puts them in memory so
the system BIOS can find them after a reboot.  This sometimes fails (though
rarely), because the memory containing the payload is in the CPU cache but
never gets written back to main memory before the system is rebooted (CPU
cache contents are lost on reboot).

With this patch, the payload memory will be changed to uncachable to ensure
that the payload is actually in main memory before the system is rebooted.

Signed-off-by: Stuart Hayes 
---
v2 Added include, removed extra parentheses
This patch has no maintainer.


diff --git a/drivers/firmware/dell_rbu.c b/drivers/firmware/dell_rbu.c
index 2f452f1f7c8a..640a88c3418d 100644
--- a/drivers/firmware/dell_rbu.c
+++ b/drivers/firmware/dell_rbu.c
@@ -45,6 +45,7 @@
 #include 
 #include 
 #include 
+#include 
 
 MODULE_AUTHOR("Abhay Salunke ");
 MODULE_DESCRIPTION("Driver for updating BIOS image on DELL systems");
@@ -180,6 +181,11 @@ static int create_packet(void *data, size_t length)
invalid_addr_packet_array[idx++] = packet_data_temp_buf;
packet_data_temp_buf = NULL;
}
+   /*
+* set to uncachable or it may never get written back before
+* reboot
+*/
+   set_memory_uc(packet_data_temp_buf, 1 << ordernum);
}
spin_lock(_data.lock);
 
@@ -349,6 +355,7 @@ static void packet_empty_list(void)
 * to make sure there are no stale RBU packets left in memory
 */
memset(newpacket->data, 0, rbu_data.packetsize);
+   set_memory_wb(newpacket->data, 1 << newpacket->ordernum);
free_pages((unsigned long) newpacket->data,
newpacket->ordernum);
kfree(newpacket);

Re: [PATCH] dell_rbu: make firmware payload memory uncachable

[Stuart Hayes]

On 3/22/2018 9:58 PM, kbuild test robot wrote:
> Hi Stuart,
> 
> Thank you for the patch! Yet something to improve:
> 
> [auto build test ERROR on linus/master]
> [also build test ERROR on v4.16-rc6 next-20180322]
> [if your patch is applied to the wrong git tree, please drop us a note to 
> help improve the system]
> 
> url:
> https://github.com/0day-ci/linux/commits/Stuart-Hayes/dell_rbu-make-firmware-payload-memory-uncachable/20180323-094405
> config: i386-randconfig-x014-201811 (attached as .config)
> compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
> reproduce:
> # save the attached .config to linux build tree
> make ARCH=i386 
> 

Please disregard this one, will send a "v2" shortly.  Not sure how I managed to 
send this version...
Thanks!

Re: [PATCH] dell_rbu: make firmware payload memory uncachable

[Stuart Hayes]

On 3/22/2018 9:58 PM, kbuild test robot wrote:
> Hi Stuart,
> 
> Thank you for the patch! Yet something to improve:
> 
> [auto build test ERROR on linus/master]
> [also build test ERROR on v4.16-rc6 next-20180322]
> [if your patch is applied to the wrong git tree, please drop us a note to 
> help improve the system]
> 
> url:
> https://github.com/0day-ci/linux/commits/Stuart-Hayes/dell_rbu-make-firmware-payload-memory-uncachable/20180323-094405
> config: i386-randconfig-x014-201811 (attached as .config)
> compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
> reproduce:
> # save the attached .config to linux build tree
> make ARCH=i386 
> 

Please disregard this one, will send a "v2" shortly.  Not sure how I managed to 
send this version...
Thanks!

[PATCH] dell_rbu: make firmware payload memory uncachable

[Stuart Hayes]

The dell_rbu driver takes firmware update payloads and puts them in memory so
the system BIOS can find them after a reboot.  This sometimes fails (though
rarely), because the memory containing the payload is in the CPU cache but
never gets written back to main memory before the system is rebooted (CPU
cache contents are lost on reboot).

With this patch, the payload memory will be changed to uncachable to ensure
that the payload is actually in main memory before the system is rebooted.

Signed-off-by: Stuart Hayes <stuart.w.ha...@gmail.com>
---
Note that there is no maintainer for this driver, so I'd be grateful if
someone could apply this... thank you!

diff --git a/drivers/firmware/dell_rbu.c b/drivers/firmware/dell_rbu.c
index 2f452f1..6b84814 100644
--- a/drivers/firmware/dell_rbu.c
+++ b/drivers/firmware/dell_rbu.c
@@ -180,6 +180,11 @@ static int create_packet(void *data, size_t length)
invalid_addr_packet_array[idx++] = packet_data_temp_buf;
packet_data_temp_buf = NULL;
}
+   /*
+* set to uncachable or it may never get written back before
+* reboot
+*/
+   set_memory_uc(packet_data_temp_buf, 1 << (ordernum));
}
spin_lock(_data.lock);
 
@@ -349,6 +354,7 @@ static void packet_empty_list(void)
 * to make sure there are no stale RBU packets left in memory
 */
memset(newpacket->data, 0, rbu_data.packetsize);
+   set_memory_wb(newpacket->data, (1 << newpacket->ordernum));
free_pages((unsigned long) newpacket->data,
newpacket->ordernum);
kfree(newpacket);

[PATCH] dell_rbu: make firmware payload memory uncachable

[Stuart Hayes]

The dell_rbu driver takes firmware update payloads and puts them in memory so
the system BIOS can find them after a reboot.  This sometimes fails (though
rarely), because the memory containing the payload is in the CPU cache but
never gets written back to main memory before the system is rebooted (CPU
cache contents are lost on reboot).

With this patch, the payload memory will be changed to uncachable to ensure
that the payload is actually in main memory before the system is rebooted.

Signed-off-by: Stuart Hayes 
---
Note that there is no maintainer for this driver, so I'd be grateful if
someone could apply this... thank you!

diff --git a/drivers/firmware/dell_rbu.c b/drivers/firmware/dell_rbu.c
index 2f452f1..6b84814 100644
--- a/drivers/firmware/dell_rbu.c
+++ b/drivers/firmware/dell_rbu.c
@@ -180,6 +180,11 @@ static int create_packet(void *data, size_t length)
invalid_addr_packet_array[idx++] = packet_data_temp_buf;
packet_data_temp_buf = NULL;
}
+   /*
+* set to uncachable or it may never get written back before
+* reboot
+*/
+   set_memory_uc(packet_data_temp_buf, 1 << (ordernum));
}
spin_lock(_data.lock);
 
@@ -349,6 +354,7 @@ static void packet_empty_list(void)
 * to make sure there are no stale RBU packets left in memory
 */
memset(newpacket->data, 0, rbu_data.packetsize);
+   set_memory_wb(newpacket->data, (1 << newpacket->ordernum));
free_pages((unsigned long) newpacket->data,
newpacket->ordernum);
kfree(newpacket);

Re: [PATCH] scsi/eh: fix hang adding ehandler wakeups after decrementing host_busy

[Stuart Hayes]

My apologies... yes, your patch also fixes my issue.  I was looking at the two 
new places from which you were calling scsi_eh_wakeup(), and didn't notice that 
you moved the spinlock in scsi_device_unbusy()... moving the spinlock in 
scsi_device_unbusy() also should the issue I'm seeing, given that 
scsi_eh_scmd_add() also uses the spinlock.

I tested your patch on my issue, and it did indeed fix my issue.

So you can add...

Tested-by: Stuart Hayes <stuart.w.ha...@gmail.com>

Thanks
Stuart


On 11/21/2017 2:09 AM, Pavel Tikhomirov wrote:
> My patch should also fix your issue too, please see explanation in reply to 
> your patch. Do your testing show that it doesn't?
> 
> Thanks, Pavel.
> 
> On 11/21/2017 09:10 AM, Stuart Hayes wrote:
>> Pavel,
>>
>> It turns out that the error handler on our systems was not getting woken up 
>> for a different reason... I submitted a patch earlier today that fixes the 
>> issue I were seeing (I CCed you on the patch).
>>
>> Before I got my hands on the failing system and was able to root cause it, I 
>> was pretty sure that your patch was going to fix our issue, because after I 
>> examined the code paths, I couldn't find any other reason that the error 
>> handler would not get woken up.  I tried forcing the bug that your patch 
>> fixes to occur, by compiling in some mdelay()s in a key place or two in the 
>> scsi code, but it never failed for me that way.  With my patch, several 
>> systems that previously failed in 10 minutes or less successfully ran for 
>> many days.
>>
>> Thanks,
>> Stuart
>>
>> On 11/9/2017 8:54 AM, Pavel Tikhomirov wrote:
>>>> Are there any issues with this patch 
>>>> (https://patchwork.kernel.org/patch/9938919/) that Pavel Tikhomirov 
>>>> submitted back in September?  I am willing to help if there's anything I 
>>>> can do to help get it accepted.
>>>
>>> Hi, Stuart, I asked James Bottomley about the patch status offlist and it 
>>> seems that the problem is - patch lacks testing and review. I would highly 
>>> appreciate review from your side and anyone who wants to participate!
>>>
>>> And if you can confirm that the patch solves the problem on your 
>>> environment with no side effects please add "Tested-by:" tag also.
>>>
>>> Thanks, Pavel
>>>
>>> On 09/05/2017 03:54 PM, Pavel Tikhomirov wrote:
>>>> We have a problem on several our nodes with scsi EH. Imagine such an
>>>> order of execution of two threads:
>>>>
>>>> CPU1 scsi_eh_scmd_add    CPU2 scsi_host_queue_ready
>>>> /* shost->host_busy == 1 initialy */
>>>>
>>>>  if (shost->shost_state == SHOST_RECOVERY)
>>>>  /* does not get here */
>>>>  return 0;
>>>>
>>>> lock(shost->host_lock);
>>>> shost->shost_state = SHOST_RECOVERY;
>>>>
>>>>  busy = shost->host_busy++;
>>>>  /* host->can_queue == 1 initialy, busy == 1
>>>>   * - go to starved label */
>>>>  lock(shost->host_lock) /* wait */
>>>>
>>>> shost->host_failed++;
>>>> /* shost->host_busy == 2, shost->host_failed == 1 */
>>>> call scsi_eh_wakeup(shost) {
>>>>  if (host_busy == host_failed) {
>>>>  /* does not get here */
>>>>  wake_up_process(shost->ehandler)
>>>>  }
>>>> }
>>>> unlock(shost->host_lock)
>>>>
>>>>  /* acquire lock */
>>>>  shost->host_busy--;
>>>>
>>>> Finaly we do not wakeup scsi_error_handler and all other commands
>>>> coming will hang as we are in never ending recovery state as there
>>>> is no one left to wakeup handler.
>>>>
>>>> So scsi disc in these host becomes unresponsive and all bio on node
>>>> hangs. (We trigger these problem when scsi cmnds to DVD drive timeout.)
>>>>
>>>> Main idea of the fix is to try to do wake up every time we decrement
>>>> host_busy or increment host_failed(the latter is already OK).
>>>>
>>>> Now the very *last* one of busy threads getting host_lock after
>>>> decrementing host_busy will see all write operations on host's
>>>> shost_state, host_busy and host_failed completed thanks to implied
>>>> memory barriers on spin_lock/un

Re: [PATCH] scsi/eh: fix hang adding ehandler wakeups after decrementing host_busy

[Stuart Hayes]

My apologies... yes, your patch also fixes my issue.  I was looking at the two 
new places from which you were calling scsi_eh_wakeup(), and didn't notice that 
you moved the spinlock in scsi_device_unbusy()... moving the spinlock in 
scsi_device_unbusy() also should the issue I'm seeing, given that 
scsi_eh_scmd_add() also uses the spinlock.

I tested your patch on my issue, and it did indeed fix my issue.

So you can add...

Tested-by: Stuart Hayes 

Thanks
Stuart


On 11/21/2017 2:09 AM, Pavel Tikhomirov wrote:
> My patch should also fix your issue too, please see explanation in reply to 
> your patch. Do your testing show that it doesn't?
> 
> Thanks, Pavel.
> 
> On 11/21/2017 09:10 AM, Stuart Hayes wrote:
>> Pavel,
>>
>> It turns out that the error handler on our systems was not getting woken up 
>> for a different reason... I submitted a patch earlier today that fixes the 
>> issue I were seeing (I CCed you on the patch).
>>
>> Before I got my hands on the failing system and was able to root cause it, I 
>> was pretty sure that your patch was going to fix our issue, because after I 
>> examined the code paths, I couldn't find any other reason that the error 
>> handler would not get woken up.  I tried forcing the bug that your patch 
>> fixes to occur, by compiling in some mdelay()s in a key place or two in the 
>> scsi code, but it never failed for me that way.  With my patch, several 
>> systems that previously failed in 10 minutes or less successfully ran for 
>> many days.
>>
>> Thanks,
>> Stuart
>>
>> On 11/9/2017 8:54 AM, Pavel Tikhomirov wrote:
>>>> Are there any issues with this patch 
>>>> (https://patchwork.kernel.org/patch/9938919/) that Pavel Tikhomirov 
>>>> submitted back in September?  I am willing to help if there's anything I 
>>>> can do to help get it accepted.
>>>
>>> Hi, Stuart, I asked James Bottomley about the patch status offlist and it 
>>> seems that the problem is - patch lacks testing and review. I would highly 
>>> appreciate review from your side and anyone who wants to participate!
>>>
>>> And if you can confirm that the patch solves the problem on your 
>>> environment with no side effects please add "Tested-by:" tag also.
>>>
>>> Thanks, Pavel
>>>
>>> On 09/05/2017 03:54 PM, Pavel Tikhomirov wrote:
>>>> We have a problem on several our nodes with scsi EH. Imagine such an
>>>> order of execution of two threads:
>>>>
>>>> CPU1 scsi_eh_scmd_add    CPU2 scsi_host_queue_ready
>>>> /* shost->host_busy == 1 initialy */
>>>>
>>>>  if (shost->shost_state == SHOST_RECOVERY)
>>>>  /* does not get here */
>>>>  return 0;
>>>>
>>>> lock(shost->host_lock);
>>>> shost->shost_state = SHOST_RECOVERY;
>>>>
>>>>  busy = shost->host_busy++;
>>>>  /* host->can_queue == 1 initialy, busy == 1
>>>>   * - go to starved label */
>>>>  lock(shost->host_lock) /* wait */
>>>>
>>>> shost->host_failed++;
>>>> /* shost->host_busy == 2, shost->host_failed == 1 */
>>>> call scsi_eh_wakeup(shost) {
>>>>  if (host_busy == host_failed) {
>>>>  /* does not get here */
>>>>  wake_up_process(shost->ehandler)
>>>>  }
>>>> }
>>>> unlock(shost->host_lock)
>>>>
>>>>  /* acquire lock */
>>>>  shost->host_busy--;
>>>>
>>>> Finaly we do not wakeup scsi_error_handler and all other commands
>>>> coming will hang as we are in never ending recovery state as there
>>>> is no one left to wakeup handler.
>>>>
>>>> So scsi disc in these host becomes unresponsive and all bio on node
>>>> hangs. (We trigger these problem when scsi cmnds to DVD drive timeout.)
>>>>
>>>> Main idea of the fix is to try to do wake up every time we decrement
>>>> host_busy or increment host_failed(the latter is already OK).
>>>>
>>>> Now the very *last* one of busy threads getting host_lock after
>>>> decrementing host_busy will see all write operations on host's
>>>> shost_state, host_busy and host_failed completed thanks to implied
>>>> memory barriers on spin_lock/unlock, so at the time of busy==failed
>

Re: [PATCH] scsi/eh: fix hang adding ehandler wakeups after decrementing host_busy

[Stuart Hayes]

Pavel,

It turns out that the error handler on our systems was not getting woken up for 
a different reason... I submitted a patch earlier today that fixes the issue I 
were seeing (I CCed you on the patch).

Before I got my hands on the failing system and was able to root cause it, I 
was pretty sure that your patch was going to fix our issue, because after I 
examined the code paths, I couldn't find any other reason that the error 
handler would not get woken up.  I tried forcing the bug that your patch fixes 
to occur, by compiling in some mdelay()s in a key place or two in the scsi 
code, but it never failed for me that way.  With my patch, several systems that 
previously failed in 10 minutes or less successfully ran for many days. 

Thanks,
Stuart

On 11/9/2017 8:54 AM, Pavel Tikhomirov wrote:
>> Are there any issues with this patch 
>> (https://patchwork.kernel.org/patch/9938919/) that Pavel Tikhomirov 
>> submitted back in September?  I am willing to help if there's anything I can 
>> do to help get it accepted.
> 
> Hi, Stuart, I asked James Bottomley about the patch status offlist and it 
> seems that the problem is - patch lacks testing and review. I would highly 
> appreciate review from your side and anyone who wants to participate!
> 
> And if you can confirm that the patch solves the problem on your environment 
> with no side effects please add "Tested-by:" tag also.
> 
> Thanks, Pavel
> 
> On 09/05/2017 03:54 PM, Pavel Tikhomirov wrote:
>> We have a problem on several our nodes with scsi EH. Imagine such an
>> order of execution of two threads:
>>
>> CPU1 scsi_eh_scmd_add    CPU2 scsi_host_queue_ready
>> /* shost->host_busy == 1 initialy */
>>
>>     if (shost->shost_state == SHOST_RECOVERY)
>>     /* does not get here */
>>     return 0;
>>
>> lock(shost->host_lock);
>> shost->shost_state = SHOST_RECOVERY;
>>
>>     busy = shost->host_busy++;
>>     /* host->can_queue == 1 initialy, busy == 1
>>  * - go to starved label */
>>     lock(shost->host_lock) /* wait */
>>
>> shost->host_failed++;
>> /* shost->host_busy == 2, shost->host_failed == 1 */
>> call scsi_eh_wakeup(shost) {
>> if (host_busy == host_failed) {
>>     /* does not get here */
>>     wake_up_process(shost->ehandler)
>> }
>> }
>> unlock(shost->host_lock)
>>
>>     /* acquire lock */
>>     shost->host_busy--;
>>
>> Finaly we do not wakeup scsi_error_handler and all other commands
>> coming will hang as we are in never ending recovery state as there
>> is no one left to wakeup handler.
>>
>> So scsi disc in these host becomes unresponsive and all bio on node
>> hangs. (We trigger these problem when scsi cmnds to DVD drive timeout.)
>>
>> Main idea of the fix is to try to do wake up every time we decrement
>> host_busy or increment host_failed(the latter is already OK).
>>
>> Now the very *last* one of busy threads getting host_lock after
>> decrementing host_busy will see all write operations on host's
>> shost_state, host_busy and host_failed completed thanks to implied
>> memory barriers on spin_lock/unlock, so at the time of busy==failed
>> we will trigger wakeup in at least one thread. (Thats why putting
>> recovery and failed checks under lock)
>>
>> Signed-off-by: Pavel Tikhomirov 
>> ---
>>   drivers/scsi/scsi_lib.c | 21 +
>>   1 file changed, 17 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
>> index f6097b89d5d3..6c99221d60aa 100644
>> --- a/drivers/scsi/scsi_lib.c
>> +++ b/drivers/scsi/scsi_lib.c
>> @@ -320,12 +320,11 @@ void scsi_device_unbusy(struct scsi_device *sdev)
>>   if (starget->can_queue > 0)
>>   atomic_dec(>target_busy);
>>   +    spin_lock_irqsave(shost->host_lock, flags);
>>   if (unlikely(scsi_host_in_recovery(shost) &&
>> - (shost->host_failed || shost->host_eh_scheduled))) {
>> -    spin_lock_irqsave(shost->host_lock, flags);
>> + (shost->host_failed || shost->host_eh_scheduled)))
>>   scsi_eh_wakeup(shost);
>> -    spin_unlock_irqrestore(shost->host_lock, flags);
>> -    }
>> +    spin_unlock_irqrestore(shost->host_lock, flags);
>>     atomic_dec(>device_busy);
>>   }
>> @@ -1503,6 +1502,13 @@ static inline int scsi_host_queue_ready(struct 
>> request_queue *q,
>>   spin_unlock_irq(shost->host_lock);
>>   out_dec:
>>   atomic_dec(>host_busy);
>> +
>> +    spin_lock_irq(shost->host_lock);
>> +    if (unlikely(scsi_host_in_recovery(shost) &&
>> + (shost->host_failed || shost->host_eh_scheduled)))
>> +    scsi_eh_wakeup(shost);
>> +    spin_unlock_irq(shost->host_lock);
>> +
>>   return 0;
>>   }
>>   @@ -1964,6 +1970,13 @@ static blk_status_t scsi_queue_rq(struct 
>> blk_mq_hw_ctx *hctx,
>>     out_dec_host_busy:
>>   atomic_dec(>host_busy);
>> +
>> +    

Re: [PATCH] scsi/eh: fix hang adding ehandler wakeups after decrementing host_busy

[Stuart Hayes]

Pavel,

It turns out that the error handler on our systems was not getting woken up for 
a different reason... I submitted a patch earlier today that fixes the issue I 
were seeing (I CCed you on the patch).

Before I got my hands on the failing system and was able to root cause it, I 
was pretty sure that your patch was going to fix our issue, because after I 
examined the code paths, I couldn't find any other reason that the error 
handler would not get woken up.  I tried forcing the bug that your patch fixes 
to occur, by compiling in some mdelay()s in a key place or two in the scsi 
code, but it never failed for me that way.  With my patch, several systems that 
previously failed in 10 minutes or less successfully ran for many days. 

Thanks,
Stuart

On 11/9/2017 8:54 AM, Pavel Tikhomirov wrote:
>> Are there any issues with this patch 
>> (https://patchwork.kernel.org/patch/9938919/) that Pavel Tikhomirov 
>> submitted back in September?  I am willing to help if there's anything I can 
>> do to help get it accepted.
> 
> Hi, Stuart, I asked James Bottomley about the patch status offlist and it 
> seems that the problem is - patch lacks testing and review. I would highly 
> appreciate review from your side and anyone who wants to participate!
> 
> And if you can confirm that the patch solves the problem on your environment 
> with no side effects please add "Tested-by:" tag also.
> 
> Thanks, Pavel
> 
> On 09/05/2017 03:54 PM, Pavel Tikhomirov wrote:
>> We have a problem on several our nodes with scsi EH. Imagine such an
>> order of execution of two threads:
>>
>> CPU1 scsi_eh_scmd_add    CPU2 scsi_host_queue_ready
>> /* shost->host_busy == 1 initialy */
>>
>>     if (shost->shost_state == SHOST_RECOVERY)
>>     /* does not get here */
>>     return 0;
>>
>> lock(shost->host_lock);
>> shost->shost_state = SHOST_RECOVERY;
>>
>>     busy = shost->host_busy++;
>>     /* host->can_queue == 1 initialy, busy == 1
>>  * - go to starved label */
>>     lock(shost->host_lock) /* wait */
>>
>> shost->host_failed++;
>> /* shost->host_busy == 2, shost->host_failed == 1 */
>> call scsi_eh_wakeup(shost) {
>> if (host_busy == host_failed) {
>>     /* does not get here */
>>     wake_up_process(shost->ehandler)
>> }
>> }
>> unlock(shost->host_lock)
>>
>>     /* acquire lock */
>>     shost->host_busy--;
>>
>> Finaly we do not wakeup scsi_error_handler and all other commands
>> coming will hang as we are in never ending recovery state as there
>> is no one left to wakeup handler.
>>
>> So scsi disc in these host becomes unresponsive and all bio on node
>> hangs. (We trigger these problem when scsi cmnds to DVD drive timeout.)
>>
>> Main idea of the fix is to try to do wake up every time we decrement
>> host_busy or increment host_failed(the latter is already OK).
>>
>> Now the very *last* one of busy threads getting host_lock after
>> decrementing host_busy will see all write operations on host's
>> shost_state, host_busy and host_failed completed thanks to implied
>> memory barriers on spin_lock/unlock, so at the time of busy==failed
>> we will trigger wakeup in at least one thread. (Thats why putting
>> recovery and failed checks under lock)
>>
>> Signed-off-by: Pavel Tikhomirov 
>> ---
>>   drivers/scsi/scsi_lib.c | 21 +
>>   1 file changed, 17 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
>> index f6097b89d5d3..6c99221d60aa 100644
>> --- a/drivers/scsi/scsi_lib.c
>> +++ b/drivers/scsi/scsi_lib.c
>> @@ -320,12 +320,11 @@ void scsi_device_unbusy(struct scsi_device *sdev)
>>   if (starget->can_queue > 0)
>>   atomic_dec(>target_busy);
>>   +    spin_lock_irqsave(shost->host_lock, flags);
>>   if (unlikely(scsi_host_in_recovery(shost) &&
>> - (shost->host_failed || shost->host_eh_scheduled))) {
>> -    spin_lock_irqsave(shost->host_lock, flags);
>> + (shost->host_failed || shost->host_eh_scheduled)))
>>   scsi_eh_wakeup(shost);
>> -    spin_unlock_irqrestore(shost->host_lock, flags);
>> -    }
>> +    spin_unlock_irqrestore(shost->host_lock, flags);
>>     atomic_dec(>device_busy);
>>   }
>> @@ -1503,6 +1502,13 @@ static inline int scsi_host_queue_ready(struct 
>> request_queue *q,
>>   spin_unlock_irq(shost->host_lock);
>>   out_dec:
>>   atomic_dec(>host_busy);
>> +
>> +    spin_lock_irq(shost->host_lock);
>> +    if (unlikely(scsi_host_in_recovery(shost) &&
>> + (shost->host_failed || shost->host_eh_scheduled)))
>> +    scsi_eh_wakeup(shost);
>> +    spin_unlock_irq(shost->host_lock);
>> +
>>   return 0;
>>   }
>>   @@ -1964,6 +1970,13 @@ static blk_status_t scsi_queue_rq(struct 
>> blk_mq_hw_ctx *hctx,
>>     out_dec_host_busy:
>>   atomic_dec(>host_busy);
>> +
>> +    spin_lock_irq(shost->host_lock);
>> +    if 

[PATCH] scsi_error: ensure EH wakes up on error to prevent host getting stuck

[Stuart Hayes]

When a command is added to the host's error handler command queue, there is a 
chance that the error handler will not be woken up.  This can happen when one 
CPU is running scsi_eh_scmd_add() at the same time as another CPU is running 
scsi_device_unbusy() for a different command on the same host.  Each function 
changes one value, and then looks at the value of a variable that the other 
function has just changed, but if they both see stale data, neither will 
actually wake up the error handler.

In scsi_eh_scmd_add, host_failed is incremented, then scsi_eh_wakeup() is 
called, which sees that host_busy is still 2, so it doesn't actually wake up 
the handler.  Meanwhile, in scsi_device_unbusy(), host_busy is decremented, and 
then it sees that host_failed is 0, so it doesn't even call scsi_eh_wakeup().

Signed-off-by: Stuart Hyaes 

---
diff -pur linux-4.14/drivers/scsi/scsi_error.c 
linux-4.14-stu/drivers/scsi/scsi_error.c
--- linux-4.14/drivers/scsi/scsi_error.c2017-11-12 12:46:13.0 
-0600
+++ linux-4.14-stu/drivers/scsi/scsi_error.c2017-11-17 14:22:19.230867923 
-0600
@@ -243,6 +243,10 @@ void scsi_eh_scmd_add(struct scsi_cmnd *
scsi_eh_reset(scmd);
list_add_tail(>eh_entry, >eh_cmd_q);
shost->host_failed++;
+   /*
+* See scsi_device_unbusy() for explanation of smp_mb().
+*/
+   smp_mb();
scsi_eh_wakeup(shost);
spin_unlock_irqrestore(shost->host_lock, flags);
 }
diff -pur linux-4.14/drivers/scsi/scsi_lib.c 
linux-4.14-stu/drivers/scsi/scsi_lib.c
--- linux-4.14/drivers/scsi/scsi_lib.c  2017-11-12 12:46:13.0 -0600
+++ linux-4.14-stu/drivers/scsi/scsi_lib.c  2017-11-17 14:22:15.814867833 
-0600
@@ -325,6 +325,15 @@ void scsi_device_unbusy(struct scsi_devi
unsigned long flags;
 
atomic_dec(>host_busy);
+   
+   /* This function changes host_busy and looks at host_failed, while
+* scsi_eh_scmd_add() updates host_failed and looks at host_busy (in
+* scsi_eh_wakeup())... if these happen simultaneously without the smp
+* memory barrier, each can see the old value, such that neither will
+* wake up the error handler, which can cause the host controller to
+* be hung forever.
+*/
+   smp_mb();
if (starget->can_queue > 0)
atomic_dec(>target_busy);
 


---
This email has been checked for viruses by Avast antivirus software.
https://www.avast.com/antivirus

[PATCH] scsi_error: ensure EH wakes up on error to prevent host getting stuck

[Stuart Hayes]

When a command is added to the host's error handler command queue, there is a 
chance that the error handler will not be woken up.  This can happen when one 
CPU is running scsi_eh_scmd_add() at the same time as another CPU is running 
scsi_device_unbusy() for a different command on the same host.  Each function 
changes one value, and then looks at the value of a variable that the other 
function has just changed, but if they both see stale data, neither will 
actually wake up the error handler.

In scsi_eh_scmd_add, host_failed is incremented, then scsi_eh_wakeup() is 
called, which sees that host_busy is still 2, so it doesn't actually wake up 
the handler.  Meanwhile, in scsi_device_unbusy(), host_busy is decremented, and 
then it sees that host_failed is 0, so it doesn't even call scsi_eh_wakeup().

Signed-off-by: Stuart Hyaes 

---
diff -pur linux-4.14/drivers/scsi/scsi_error.c 
linux-4.14-stu/drivers/scsi/scsi_error.c
--- linux-4.14/drivers/scsi/scsi_error.c2017-11-12 12:46:13.0 
-0600
+++ linux-4.14-stu/drivers/scsi/scsi_error.c2017-11-17 14:22:19.230867923 
-0600
@@ -243,6 +243,10 @@ void scsi_eh_scmd_add(struct scsi_cmnd *
scsi_eh_reset(scmd);
list_add_tail(>eh_entry, >eh_cmd_q);
shost->host_failed++;
+   /*
+* See scsi_device_unbusy() for explanation of smp_mb().
+*/
+   smp_mb();
scsi_eh_wakeup(shost);
spin_unlock_irqrestore(shost->host_lock, flags);
 }
diff -pur linux-4.14/drivers/scsi/scsi_lib.c 
linux-4.14-stu/drivers/scsi/scsi_lib.c
--- linux-4.14/drivers/scsi/scsi_lib.c  2017-11-12 12:46:13.0 -0600
+++ linux-4.14-stu/drivers/scsi/scsi_lib.c  2017-11-17 14:22:15.814867833 
-0600
@@ -325,6 +325,15 @@ void scsi_device_unbusy(struct scsi_devi
unsigned long flags;
 
atomic_dec(>host_busy);
+   
+   /* This function changes host_busy and looks at host_failed, while
+* scsi_eh_scmd_add() updates host_failed and looks at host_busy (in
+* scsi_eh_wakeup())... if these happen simultaneously without the smp
+* memory barrier, each can see the old value, such that neither will
+* wake up the error handler, which can cause the host controller to
+* be hung forever.
+*/
+   smp_mb();
if (starget->can_queue > 0)
atomic_dec(>target_busy);
 


---
This email has been checked for viruses by Avast antivirus software.
https://www.avast.com/antivirus

Re: [PATCH] scsi/eh: fix hang adding ehandler wakeups after decrementing host_busy

[Stuart Hayes]

Are there any issues with this patch 
(https://patchwork.kernel.org/patch/9938919/) that Pavel Tikhomirov submitted 
back in September?  I am willing to help if there's anything I can do to help 
get it accepted.

The failing case I'm working on involves lots of servers with disk read/write 
activity with periodic INQUIRY commands that aren't supported by the target 
device, so the error handler gets invoked periodically, and some of the systems 
will hang after a day or two.

Thanks
Stuart

---
This email has been checked for viruses by Avast antivirus software.
https://www.avast.com/antivirus

Re: [PATCH] scsi/eh: fix hang adding ehandler wakeups after decrementing host_busy

[Stuart Hayes]

Are there any issues with this patch 
(https://patchwork.kernel.org/patch/9938919/) that Pavel Tikhomirov submitted 
back in September?  I am willing to help if there's anything I can do to help 
get it accepted.

The failing case I'm working on involves lots of servers with disk read/write 
activity with periodic INQUIRY commands that aren't supported by the target 
device, so the error handler gets invoked periodically, and some of the systems 
will hang after a day or two.

Thanks
Stuart

---
This email has been checked for viruses by Avast antivirus software.
https://www.avast.com/antivirus