[PATCH v2 3/5] PCI: pci-bridge-emul: Export API functions

[Pali Rohár]

It allows kernel modules which are not compiled into kernel image to use
pci-bridge-emul API functions.

Signed-off-by: Pali Rohár 
Reviewed-by: Marek Behún 
---
 drivers/pci/pci-bridge-emul.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/pci/pci-bridge-emul.c b/drivers/pci/pci-bridge-emul.c
index ccf26d12ec61..139869d50eb2 100644
--- a/drivers/pci/pci-bridge-emul.c
+++ b/drivers/pci/pci-bridge-emul.c
@@ -294,6 +294,7 @@ int pci_bridge_emul_init(struct pci_bridge_emul *bridge,
 
return 0;
 }
+EXPORT_SYMBOL_GPL(pci_bridge_emul_init);
 
 /*
  * Cleanup a pci_bridge_emul structure that was previously initialized
@@ -305,6 +306,7 @@ void pci_bridge_emul_cleanup(struct pci_bridge_emul *bridge)
kfree(bridge->pcie_cap_regs_behavior);
kfree(bridge->pci_regs_behavior);
 }
+EXPORT_SYMBOL_GPL(pci_bridge_emul_cleanup);
 
 /*
  * Should be called by the PCI controller driver when reading the PCI
@@ -366,6 +368,7 @@ int pci_bridge_emul_conf_read(struct pci_bridge_emul 
*bridge, int where,
 
return PCIBIOS_SUCCESSFUL;
 }
+EXPORT_SYMBOL_GPL(pci_bridge_emul_conf_read);
 
 /*
  * Should be called by the PCI controller driver when writing the PCI
@@ -430,3 +433,4 @@ int pci_bridge_emul_conf_write(struct pci_bridge_emul 
*bridge, int where,
 
return PCIBIOS_SUCCESSFUL;
 }
+EXPORT_SYMBOL_GPL(pci_bridge_emul_conf_write);
-- 
2.20.1

[PATCH v2 5/5] PCI: aardvark: Move PCIe reset card code to advk_pcie_train_link()

[Pali Rohár]

Move code which belongs to link training (delays and resets) into
advk_pcie_train_link() function, so everything related to link training,
including timings is at one place.

After experiments it can be observed that link training in aardvark
hardware is very sensitive to timings and delays, so it is a good idea to
have this code at the same place as link training calls.

This patch does not change behavior of aardvark initialization.

Signed-off-by: Pali Rohár 
Tested-by: Marek Behún 
---
 drivers/pci/controller/pci-aardvark.c | 64 ++-
 1 file changed, 34 insertions(+), 30 deletions(-)

diff --git a/drivers/pci/controller/pci-aardvark.c 
b/drivers/pci/controller/pci-aardvark.c
index f5c1d231b0e2..fcea300fbcc0 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -253,6 +253,25 @@ static void advk_pcie_wait_for_retrain(struct advk_pcie 
*pcie)
}
 }
 
+static void advk_pcie_issue_perst(struct advk_pcie *pcie)
+{
+   u32 reg;
+
+   if (!pcie->reset_gpio)
+   return;
+
+   /* PERST does not work for some cards when link training is enabled */
+   reg = advk_readl(pcie, PCIE_CORE_CTRL0_REG);
+   reg &= ~LINK_TRAINING_EN;
+   advk_writel(pcie, reg, PCIE_CORE_CTRL0_REG);
+
+   /* 10ms delay is needed for some cards */
+   dev_info(&pcie->pdev->dev, "issuing PERST via reset GPIO for 10ms\n");
+   gpiod_set_value_cansleep(pcie->reset_gpio, 1);
+   usleep_range(1, 11000);
+   gpiod_set_value_cansleep(pcie->reset_gpio, 0);
+}
+
 static int advk_pcie_train_at_gen(struct advk_pcie *pcie, int gen)
 {
int ret, neg_gen;
@@ -300,6 +319,21 @@ static void advk_pcie_train_link(struct advk_pcie *pcie)
struct device *dev = &pcie->pdev->dev;
int neg_gen = -1, gen;
 
+   /*
+* Reset PCIe card via PERST# signal. Some cards are not detected
+* during link training when they are in some non-initial state.
+*/
+   advk_pcie_issue_perst(pcie);
+
+   /*
+* PERST# signal could have been asserted by pinctrl subsystem before
+* probe() callback has been called or issued explicitly by reset gpio
+* function advk_pcie_issue_perst(), making the endpoint going into
+* fundamental reset. As required by PCI Express spec a delay for at
+* least 100ms after such a reset before link training is needed.
+*/
+   msleep(PCI_PM_D3COLD_WAIT);
+
/*
 * Try link training at link gen specified by device tree property
 * 'max-link-speed'. If this fails, iteratively train at lower gen.
@@ -332,31 +366,10 @@ static void advk_pcie_train_link(struct advk_pcie *pcie)
dev_err(dev, "link never came up\n");
 }
 
-static void advk_pcie_issue_perst(struct advk_pcie *pcie)
-{
-   u32 reg;
-
-   if (!pcie->reset_gpio)
-   return;
-
-   /* PERST does not work for some cards when link training is enabled */
-   reg = advk_readl(pcie, PCIE_CORE_CTRL0_REG);
-   reg &= ~LINK_TRAINING_EN;
-   advk_writel(pcie, reg, PCIE_CORE_CTRL0_REG);
-
-   /* 10ms delay is needed for some cards */
-   dev_info(&pcie->pdev->dev, "issuing PERST via reset GPIO for 10ms\n");
-   gpiod_set_value_cansleep(pcie->reset_gpio, 1);
-   usleep_range(1, 11000);
-   gpiod_set_value_cansleep(pcie->reset_gpio, 0);
-}
-
 static void advk_pcie_setup_hw(struct advk_pcie *pcie)
 {
u32 reg;
 
-   advk_pcie_issue_perst(pcie);
-
/* Enable TX */
reg = advk_readl(pcie, PCIE_CORE_REF_CLK_REG);
reg |= PCIE_CORE_REF_CLK_TX_ENABLE;
@@ -433,15 +446,6 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
reg |= PIO_CTRL_ADDR_WIN_DISABLE;
advk_writel(pcie, reg, PIO_CTRL);
 
-   /*
-* PERST# signal could have been asserted by pinctrl subsystem before
-* probe() callback has been called or issued explicitly by reset gpio
-* function advk_pcie_issue_perst(), making the endpoint going into
-* fundamental reset. As required by PCI Express spec a delay for at
-* least 100ms after such a reset before link training is needed.
-*/
-   msleep(PCI_PM_D3COLD_WAIT);
-
advk_pcie_train_link(pcie);
 
/*
-- 
2.20.1

[PATCH v2 0/5] PCIe aardvark controller improvements

[Pali Rohár]

Hi,

we have some more improvements for PCIe aardvark controller (Armada 3720
SOC - EspressoBIN and Turris MOX).

The main improvement is that with these patches the driver can be compiled
as a module, and can be reloaded at runtime.

This series applies on top of Linus' master branch.

Marek & Pali


Changes in V2 for patch 4/5:
* Protect pci_stop_root_bus() and pci_remove_root_bus() function calls by
  pci_lock_rescan_remove() and pci_unlock_rescan_remove()


Pali Rohár (5):
  PCI: aardvark: Fix compilation on s390
  PCI: aardvark: Check for errors from pci_bridge_emul_init() call
  PCI: pci-bridge-emul: Export API functions
  PCI: aardvark: Implement driver 'remove' function and allow to build
it as module
  PCI: aardvark: Move PCIe reset card code to advk_pcie_train_link()

 drivers/pci/controller/Kconfig|   2 +-
 drivers/pci/controller/pci-aardvark.c | 104 --
 drivers/pci/pci-bridge-emul.c |   4 +
 3 files changed, 71 insertions(+), 39 deletions(-)

-- 
2.20.1

[PATCH v2 2/5] PCI: aardvark: Check for errors from pci_bridge_emul_init() call

[Pali Rohár]

Function pci_bridge_emul_init() may fail so correctly check for errors.

Fixes: 8a3ebd8de328 ("PCI: aardvark: Implement emulated root PCI bridge config 
space")
Signed-off-by: Pali Rohár 
Reviewed-by: Marek Behún 
---
 drivers/pci/controller/pci-aardvark.c | 11 +++
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/controller/pci-aardvark.c 
b/drivers/pci/controller/pci-aardvark.c
index 8caa80b19cf8..d5f58684d962 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -608,7 +608,7 @@ static struct pci_bridge_emul_ops advk_pci_bridge_emul_ops 
= {
  * Initialize the configuration space of the PCI-to-PCI bridge
  * associated with the given PCIe interface.
  */
-static void advk_sw_pci_bridge_init(struct advk_pcie *pcie)
+static int advk_sw_pci_bridge_init(struct advk_pcie *pcie)
 {
struct pci_bridge_emul *bridge = &pcie->bridge;
 
@@ -634,8 +634,7 @@ static void advk_sw_pci_bridge_init(struct advk_pcie *pcie)
bridge->data = pcie;
bridge->ops = &advk_pci_bridge_emul_ops;
 
-   pci_bridge_emul_init(bridge, 0);
-
+   return pci_bridge_emul_init(bridge, 0);
 }
 
 static bool advk_pcie_valid_device(struct advk_pcie *pcie, struct pci_bus *bus,
@@ -1169,7 +1168,11 @@ static int advk_pcie_probe(struct platform_device *pdev)
 
advk_pcie_setup_hw(pcie);
 
-   advk_sw_pci_bridge_init(pcie);
+   ret = advk_sw_pci_bridge_init(pcie);
+   if (ret) {
+   dev_err(dev, "Failed to register emulated root PCI bridge\n");
+   return ret;
+   }
 
ret = advk_pcie_init_irq_domain(pcie);
if (ret) {
-- 
2.20.1

Re: [PATCH 4/5] PCI: aardvark: Implement driver 'remove' function and allow to build it as module

[Pali Rohár]

On Monday 03 August 2020 14:00:37 Rob Herring wrote:
> On Mon, Aug 3, 2020 at 8:46 AM Pali Rohár  wrote:
> >
> > On Wednesday 29 July 2020 12:48:09 Rob Herring wrote:
> > > On Wed, Jul 15, 2020 at 04:25:56PM +0200, Marek Behún wrote:
> > > > From: Pali Rohár 
> > > >
> > > > Providing driver's 'remove' function allows kernel to bind and unbind 
> > > > devices
> > > > from aardvark driver. It also allows to build aardvark driver as a 
> > > > module.
> > > >
> > > > Compiling aardvark as a module simplifies development and debugging of
> > > > this driver as it can be reloaded at runtime without the need to reboot
> > > > to new kernel.
> > > >
> > > > Signed-off-by: Pali Rohár 
> > > > Reviewed-by: Marek Behún 
> > > > ---
> > > >  drivers/pci/controller/Kconfig|  2 +-
> > > >  drivers/pci/controller/pci-aardvark.c | 25 ++---
> > > >  2 files changed, 23 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/pci/controller/Kconfig 
> > > > b/drivers/pci/controller/Kconfig
> > > > index adddf21fa381..f9da5ff2c517 100644
> > > > --- a/drivers/pci/controller/Kconfig
> > > > +++ b/drivers/pci/controller/Kconfig
> > > > @@ -12,7 +12,7 @@ config PCI_MVEBU
> > > > select PCI_BRIDGE_EMUL
> > > >
> > > >  config PCI_AARDVARK
> > > > -   bool "Aardvark PCIe controller"
> > > > +   tristate "Aardvark PCIe controller"
> > > > depends on (ARCH_MVEBU && ARM64) || COMPILE_TEST
> > > > depends on OF
> > > > depends on PCI_MSI_IRQ_DOMAIN
> > > > diff --git a/drivers/pci/controller/pci-aardvark.c 
> > > > b/drivers/pci/controller/pci-aardvark.c
> > > > index d5f58684d962..0a5aa6d77f5d 100644
> > > > --- a/drivers/pci/controller/pci-aardvark.c
> > > > +++ b/drivers/pci/controller/pci-aardvark.c
> > > > @@ -14,6 +14,7 @@
> > > >  #include 
> > > >  #include 
> > > >  #include 
> > > > +#include 
> > > >  #include 
> > > >  #include 
> > > >  #include 
> > > > @@ -1114,6 +1115,7 @@ static int advk_pcie_probe(struct platform_device 
> > > > *pdev)
> > > >
> > > > pcie = pci_host_bridge_priv(bridge);
> > > > pcie->pdev = pdev;
> > > > +   platform_set_drvdata(pdev, pcie);
> > > >
> > > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > > pcie->base = devm_ioremap_resource(dev, res);
> > > > @@ -1204,18 +1206,35 @@ static int advk_pcie_probe(struct 
> > > > platform_device *pdev)
> > > > return 0;
> > > >  }
> > > >
> > > > +static int advk_pcie_remove(struct platform_device *pdev)
> > > > +{
> > > > +   struct advk_pcie *pcie = platform_get_drvdata(pdev);
> > > > +   struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie);
> > > > +
> > > > +   pci_stop_root_bus(bridge->bus);
> > > > +   pci_remove_root_bus(bridge->bus);
> > >
> > > Based on pci_host_common_remove() implementation, doesn't this need a
> > > lock around it (pci_lock_rescan_remove)?
> >
> > Well, I'm not sure. I looked into other pci drivers and none of
> > following drivers pci-tegra.c, pcie-altera.c, pcie-brcmstb.c,
> > pcie-iproc.c, pcie-mediatek.c, pcie-rockchip-host.c calls
> > pci_lock_rescan_remove() and pci_unlock_rescan_remove().
> 
> The mutex protects the bus->devices list, so yes I believe it is needed.
> 
> Rob

Thank you Rob! It means that all above pci drivers should be fixed too.

I will prepare a new version of aardvark patches with protecting pci
stop/remove functions. And later I can look at some common bridge remove
function for fixing those other pci drivers.

Re: [PATCH 4/5] PCI: aardvark: Implement driver 'remove' function and allow to build it as module

[Pali Rohár]

On Wednesday 29 July 2020 12:48:09 Rob Herring wrote:
> On Wed, Jul 15, 2020 at 04:25:56PM +0200, Marek Behún wrote:
> > From: Pali Rohár 
> > 
> > Providing driver's 'remove' function allows kernel to bind and unbind 
> > devices
> > from aardvark driver. It also allows to build aardvark driver as a module.
> > 
> > Compiling aardvark as a module simplifies development and debugging of
> > this driver as it can be reloaded at runtime without the need to reboot
> > to new kernel.
> > 
> > Signed-off-by: Pali Rohár 
> > Reviewed-by: Marek Behún 
> > ---
> >  drivers/pci/controller/Kconfig|  2 +-
> >  drivers/pci/controller/pci-aardvark.c | 25 ++---
> >  2 files changed, 23 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
> > index adddf21fa381..f9da5ff2c517 100644
> > --- a/drivers/pci/controller/Kconfig
> > +++ b/drivers/pci/controller/Kconfig
> > @@ -12,7 +12,7 @@ config PCI_MVEBU
> > select PCI_BRIDGE_EMUL
> >  
> >  config PCI_AARDVARK
> > -   bool "Aardvark PCIe controller"
> > +   tristate "Aardvark PCIe controller"
> > depends on (ARCH_MVEBU && ARM64) || COMPILE_TEST
> > depends on OF
> > depends on PCI_MSI_IRQ_DOMAIN
> > diff --git a/drivers/pci/controller/pci-aardvark.c 
> > b/drivers/pci/controller/pci-aardvark.c
> > index d5f58684d962..0a5aa6d77f5d 100644
> > --- a/drivers/pci/controller/pci-aardvark.c
> > +++ b/drivers/pci/controller/pci-aardvark.c
> > @@ -14,6 +14,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  #include 
> >  #include 
> >  #include 
> > @@ -1114,6 +1115,7 @@ static int advk_pcie_probe(struct platform_device 
> > *pdev)
> >  
> > pcie = pci_host_bridge_priv(bridge);
> > pcie->pdev = pdev;
> > +   platform_set_drvdata(pdev, pcie);
> >  
> > res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > pcie->base = devm_ioremap_resource(dev, res);
> > @@ -1204,18 +1206,35 @@ static int advk_pcie_probe(struct platform_device 
> > *pdev)
> > return 0;
> >  }
> >  
> > +static int advk_pcie_remove(struct platform_device *pdev)
> > +{
> > +   struct advk_pcie *pcie = platform_get_drvdata(pdev);
> > +   struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie);
> > +
> > +   pci_stop_root_bus(bridge->bus);
> > +   pci_remove_root_bus(bridge->bus);
> 
> Based on pci_host_common_remove() implementation, doesn't this need a 
> lock around it (pci_lock_rescan_remove)?

Well, I'm not sure. I looked into other pci drivers and none of
following drivers pci-tegra.c, pcie-altera.c, pcie-brcmstb.c,
pcie-iproc.c, pcie-mediatek.c, pcie-rockchip-host.c calls
pci_lock_rescan_remove() and pci_unlock_rescan_remove().

Only pci-host-common.c and pci-hyperv.c protect pci_stop_root_bus() and
pci_remove_root_bus() by locks.

> We should probably have a common function (say pci_bridge_remove) to 
> implement this. You could use pci_host_common_remove(), but you'd have 
> to adjust drvdata.

Ok, I agree that some common function could be useful as the same code
(pci_stop_root_bus(); pci_remove_root_bus();) is called in more pci
drivers.

But first we need to know if locks are required or not.

> > +
> > +   advk_pcie_remove_msi_irq_domain(pcie);
> > +   advk_pcie_remove_irq_domain(pcie);
> > +
> > +   return 0;
> > +}
> > +
> >  static const struct of_device_id advk_pcie_of_match_table[] = {
> > { .compatible = "marvell,armada-3700-pcie", },
> > {},
> >  };
> > +MODULE_DEVICE_TABLE(of, advk_pcie_of_match_table);
> >  
> >  static struct platform_driver advk_pcie_driver = {
> > .driver = {
> > .name = "advk-pcie",
> > .of_match_table = advk_pcie_of_match_table,
> > -   /* Driver unloading/unbinding currently not supported */
> > -   .suppress_bind_attrs = true,
> > },
> > .probe = advk_pcie_probe,
> > +   .remove = advk_pcie_remove,
> >  };
> > -builtin_platform_driver(advk_pcie_driver);
> > +module_platform_driver(advk_pcie_driver);
> > +
> > +MODULE_DESCRIPTION("Aardvark PCIe controller");
> > +MODULE_LICENSE("GPL v2");
> > -- 
> > 2.26.2
> > 

[PATCH 4/4] mmc: sdio: Export SDIO revision and info strings to userspace

[Pali Rohár]

For SDIO functions, SDIO cards and SD COMBO cards are exported revision
number and info strings from CISTPL_VERS_1 structure. Revision number
should indicate compliance of standard and info strings should contain
product information in same format as product information for PCMCIA cards.

Product information for PCMCIA cards should contain following strings in
this order: Manufacturer, Product Name, Lot number, Programming Conditions.

Note that not all SDIO cards export all those info strings in that order as
described in PCMCIA Metaformat Specification.

Signed-off-by: Pali Rohár 
---
 drivers/mmc/core/bus.c  | 12 
 drivers/mmc/core/sd.c   | 36 +---
 drivers/mmc/core/sdio.c | 24 
 drivers/mmc/core/sdio_bus.c | 34 ++
 4 files changed, 103 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c
index 70207f11a654..c2e70b757dd1 100644
--- a/drivers/mmc/core/bus.c
+++ b/drivers/mmc/core/bus.c
@@ -68,6 +68,7 @@ mmc_bus_uevent(struct device *dev, struct kobj_uevent_env 
*env)
 {
struct mmc_card *card = mmc_dev_to_card(dev);
const char *type;
+   unsigned int i;
int retval = 0;
 
switch (card->type) {
@@ -98,6 +99,17 @@ mmc_bus_uevent(struct device *dev, struct kobj_uevent_env 
*env)
card->cis.vendor, card->cis.device);
if (retval)
return retval;
+
+   retval = add_uevent_var(env, "SDIO_REVISION=%u.%u",
+   card->major_rev, card->minor_rev);
+   if (retval)
+   return retval;
+
+   for (i = 0; i < card->num_info; i++) {
+   retval = add_uevent_var(env, "SDIO_INFO%u=%s", i+1, 
card->info[i]);
+   if (retval)
+   return retval;
+   }
}
 
/*
diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
index 5a2210c25aa7..429ca14fa7e1 100644
--- a/drivers/mmc/core/sd.c
+++ b/drivers/mmc/core/sd.c
@@ -709,10 +709,34 @@ static DEVICE_ATTR(dsr, S_IRUGO, mmc_dsr_show, NULL);
 
 MMC_DEV_ATTR(vendor, "0x%04x\n", card->cis.vendor);
 MMC_DEV_ATTR(device, "0x%04x\n", card->cis.device);
+MMC_DEV_ATTR(revision, "%u.%u\n", card->major_rev, card->minor_rev);
+
+#define sdio_info_attr(num)
\
+static ssize_t info##num##_show(struct device *dev, struct device_attribute 
*attr, char *buf)  \
+{  
\
+   struct mmc_card *card = mmc_dev_to_card(dev);   
\
+   
\
+   if (num > card->num_info)   
\
+   return -ENODATA;
\
+   if (!card->info[num-1][0])  
\
+   return 0;   
\
+   return sprintf(buf, "%s\n", card->info[num-1]); 
\
+}  
\
+static DEVICE_ATTR_RO(info##num)
+
+sdio_info_attr(1);
+sdio_info_attr(2);
+sdio_info_attr(3);
+sdio_info_attr(4);
 
 static struct attribute *sd_std_attrs[] = {
&dev_attr_vendor.attr,
&dev_attr_device.attr,
+   &dev_attr_revision.attr,
+   &dev_attr_info1.attr,
+   &dev_attr_info2.attr,
+   &dev_attr_info3.attr,
+   &dev_attr_info4.attr,
&dev_attr_cid.attr,
&dev_attr_csd.attr,
&dev_attr_scr.attr,
@@ -738,9 +762,15 @@ static umode_t sd_std_is_visible(struct kobject *kobj, 
struct attribute *attr,
struct device *dev = container_of(kobj, struct device, kobj);
struct mmc_card *card = mmc_dev_to_card(dev);
 
-   /* CIS vendor and device ids are available only for Combo cards */
-   if ((attr == &dev_attr_vendor.attr || attr == &dev_attr_device.attr) &&
-   card->type != MMC_TYPE_SD_COMBO)
+   /* CIS vendor and device ids, revision and info string are available 
only for Combo cards */
+   if ((attr == &dev_attr_vendor.attr ||
+attr == &dev_attr_device.attr ||
+attr == &dev_attr_revision.attr ||
+attr == &dev_attr_info1.attr ||
+attr == &dev_attr_info2.attr ||
+attr == &dev_attr_info3.attr ||
+attr == &am

[PATCH 2/4] mmc: sdio: Parse CISTPL_VERS_1 major and minor revision numbers

[Pali Rohár]

They should indicate compliance of standard.

Signed-off-by: Pali Rohár 
---
 drivers/mmc/core/sdio_cis.c   | 8 
 include/linux/mmc/card.h  | 2 ++
 include/linux/mmc/sdio_func.h | 2 ++
 3 files changed, 12 insertions(+)

diff --git a/drivers/mmc/core/sdio_cis.c b/drivers/mmc/core/sdio_cis.c
index 3efaa9534a77..44bea5e4aeda 100644
--- a/drivers/mmc/core/sdio_cis.c
+++ b/drivers/mmc/core/sdio_cis.c
@@ -23,12 +23,16 @@
 static int cistpl_vers_1(struct mmc_card *card, struct sdio_func *func,
 const unsigned char *buf, unsigned size)
 {
+   u8 major_rev, minor_rev;
unsigned i, nr_strings;
char **buffer, *string;
 
if (size < 2)
return 0;
 
+   major_rev = buf[0];
+   minor_rev = buf[1];
+
/* Find all null-terminated (including zero length) strings in
   the TPLLV1_INFO field. Trailing garbage is ignored. */
buf += 2;
@@ -60,9 +64,13 @@ static int cistpl_vers_1(struct mmc_card *card, struct 
sdio_func *func,
}
 
if (func) {
+   func->major_rev = major_rev;
+   func->minor_rev = minor_rev;
func->num_info = nr_strings;
func->info = (const char**)buffer;
} else {
+   card->major_rev = major_rev;
+   card->minor_rev = minor_rev;
card->num_info = nr_strings;
card->info = (const char**)buffer;
}
diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
index 7d46411ffaa2..42df06c6b19c 100644
--- a/include/linux/mmc/card.h
+++ b/include/linux/mmc/card.h
@@ -297,6 +297,8 @@ struct mmc_card {
struct sdio_cis cis;/* common tuple info */
struct sdio_func*sdio_func[SDIO_MAX_FUNCS]; /* SDIO functions 
(devices) */
struct sdio_func*sdio_single_irq; /* SDIO function when only 
one IRQ active */
+   u8  major_rev;  /* major revision number */
+   u8  minor_rev;  /* minor revision number */
unsignednum_info;   /* number of info strings */
const char  **info; /* info strings */
struct sdio_func_tuple  *tuples;/* unknown common tuples */
diff --git a/include/linux/mmc/sdio_func.h b/include/linux/mmc/sdio_func.h
index fa2aaab5e57a..478855b8e406 100644
--- a/include/linux/mmc/sdio_func.h
+++ b/include/linux/mmc/sdio_func.h
@@ -51,6 +51,8 @@ struct sdio_func {
 
u8  *tmpbuf;/* DMA:able scratch buffer */
 
+   u8  major_rev;  /* major revision number */
+   u8  minor_rev;  /* minor revision number */
unsignednum_info;   /* number of info strings */
const char  **info; /* info strings */
 
-- 
2.20.1

[PATCH 3/4] mmc: sdio: Extend sdio_config_attr macro and use it also for modalias

[Pali Rohár]

This simplify code for generating sdio config attributes and allows easily
define new sdio attributes.

Signed-off-by: Pali Rohár 
---
 drivers/mmc/core/sdio_bus.c | 20 ++--
 1 file changed, 6 insertions(+), 14 deletions(-)

diff --git a/drivers/mmc/core/sdio_bus.c b/drivers/mmc/core/sdio_bus.c
index 3cc928282af7..2384829c8fb2 100644
--- a/drivers/mmc/core/sdio_bus.c
+++ b/drivers/mmc/core/sdio_bus.c
@@ -28,29 +28,21 @@
 #define to_sdio_driver(d)  container_of(d, struct sdio_driver, drv)
 
 /* show configuration fields */
-#define sdio_config_attr(field, format_string) \
+#define sdio_config_attr(field, format_string, args...)
\
 static ssize_t \
 field##_show(struct device *dev, struct device_attribute *attr, char *buf) 
\
 {  \
struct sdio_func *func; \
\
func = dev_to_sdio_func (dev);  \
-   return sprintf (buf, format_string, func->field);   \
+   return sprintf(buf, format_string, args);   \
 }  \
 static DEVICE_ATTR_RO(field)
 
-sdio_config_attr(class, "0x%02x\n");
-sdio_config_attr(vendor, "0x%04x\n");
-sdio_config_attr(device, "0x%04x\n");
-
-static ssize_t modalias_show(struct device *dev, struct device_attribute 
*attr, char *buf)
-{
-   struct sdio_func *func = dev_to_sdio_func (dev);
-
-   return sprintf(buf, "sdio:c%02Xv%04Xd%04X\n",
-   func->class, func->vendor, func->device);
-}
-static DEVICE_ATTR_RO(modalias);
+sdio_config_attr(class, "0x%02x\n", func->class);
+sdio_config_attr(vendor, "0x%04x\n", func->vendor);
+sdio_config_attr(device, "0x%04x\n", func->device);
+sdio_config_attr(modalias, "sdio:c%02Xv%04Xd%04X\n", func->class, 
func->vendor, func->device);
 
 static struct attribute *sdio_dev_attrs[] = {
&dev_attr_class.attr,
-- 
2.20.1

[PATCH 1/4] mmc: sdio: Check for CISTPL_VERS_1 buffer size

[Pali Rohár]

Before parsing CISTPL_VERS_1 structure check that its size is at least two
bytes to prevent buffer overflow.

Signed-off-by: Pali Rohár 
---
 drivers/mmc/core/sdio_cis.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/mmc/core/sdio_cis.c b/drivers/mmc/core/sdio_cis.c
index e0655278c5c3..3efaa9534a77 100644
--- a/drivers/mmc/core/sdio_cis.c
+++ b/drivers/mmc/core/sdio_cis.c
@@ -24,10 +24,13 @@ static int cistpl_vers_1(struct mmc_card *card, struct 
sdio_func *func,
 const unsigned char *buf, unsigned size)
 {
unsigned i, nr_strings;
char **buffer, *string;
 
+   if (size < 2)
+   return 0;
+
/* Find all null-terminated (including zero length) strings in
   the TPLLV1_INFO field. Trailing garbage is ignored. */
buf += 2;
size -= 2;
 
-- 
2.20.1

[PATCH 0/4] mmc: sdio: Export CISTPL_VERS_1 attributes to userspace

[Pali Rohár]

CISTPL_VERS_1 structure contains useful information for identification
of SDIO cards. It contains revision number according to which standard
is SDIO card compliant. And also it contain human readable info strings
which should contain manufacturer name or product information, like for
old PCMCIA cards. SDIO simplified specification 3.00 just contain
reference to PCMCIA metaformat specification for definition of that
CISTPL_VERS_1 structure itself.

Human readable SDIO card strings can be useful for userspace to do card
identification. Until now kernel exported to userspace only vendor and
device numbers but these numbers do not help to identify new or unknown
cards.


I have tested these patches with Marwell 88W8997 SDIO card (WiFi+Bluetooth)
and here is content of attributes available in userspace:

$ grep . /sys/class/mmc_host/mmc0/mmc0:0001/* 
/sys/class/mmc_host/mmc0/mmc0:0001/*/*
/sys/class/mmc_host/mmc0/mmc0:0001/device:0x9140
/sys/class/mmc_host/mmc0/mmc0:0001/info1:Marvell
/sys/class/mmc_host/mmc0/mmc0:0001/info2:Wireless Device ID: 50
/sys/class/mmc_host/mmc0/mmc0:0001/ocr:0x0020
/sys/class/mmc_host/mmc0/mmc0:0001/rca:0x0001
/sys/class/mmc_host/mmc0/mmc0:0001/revision:1.0
/sys/class/mmc_host/mmc0/mmc0:0001/type:SDIO
/sys/class/mmc_host/mmc0/mmc0:0001/uevent:MMC_TYPE=SDIO
/sys/class/mmc_host/mmc0/mmc0:0001/uevent:SDIO_ID=02DF:9140
/sys/class/mmc_host/mmc0/mmc0:0001/uevent:SDIO_REVISION=1.0
/sys/class/mmc_host/mmc0/mmc0:0001/uevent:SDIO_INFO1=Marvell
/sys/class/mmc_host/mmc0/mmc0:0001/uevent:SDIO_INFO2=Wireless Device ID: 50
/sys/class/mmc_host/mmc0/mmc0:0001/uevent:SDIO_INFO3=
/sys/class/mmc_host/mmc0/mmc0:0001/vendor:0x02df
/sys/class/mmc_host/mmc0/mmc0:0001/mmc0:0001:1/class:0x00
/sys/class/mmc_host/mmc0/mmc0:0001/mmc0:0001:1/device:0x9141
/sys/class/mmc_host/mmc0/mmc0:0001/mmc0:0001:1/info1:Marvell WiFi Device
/sys/class/mmc_host/mmc0/mmc0:0001/mmc0:0001:1/modalias:sdio:c00v02DFd9141
/sys/class/mmc_host/mmc0/mmc0:0001/mmc0:0001:1/revision:1.0
/sys/class/mmc_host/mmc0/mmc0:0001/mmc0:0001:1/uevent:DRIVER=mwifiex_sdio
/sys/class/mmc_host/mmc0/mmc0:0001/mmc0:0001:1/uevent:SDIO_CLASS=00
/sys/class/mmc_host/mmc0/mmc0:0001/mmc0:0001:1/uevent:SDIO_ID=02DF:9141
/sys/class/mmc_host/mmc0/mmc0:0001/mmc0:0001:1/uevent:SDIO_REVISION=1.0
/sys/class/mmc_host/mmc0/mmc0:0001/mmc0:0001:1/uevent:SDIO_INFO1=Marvell WiFi 
Device
/sys/class/mmc_host/mmc0/mmc0:0001/mmc0:0001:1/uevent:SDIO_INFO2=
/sys/class/mmc_host/mmc0/mmc0:0001/mmc0:0001:1/uevent:MODALIAS=sdio:c00v02DFd9141
/sys/class/mmc_host/mmc0/mmc0:0001/mmc0:0001:1/vendor:0x02df
/sys/class/mmc_host/mmc0/mmc0:0001/mmc0:0001:2/class:0x00
/sys/class/mmc_host/mmc0/mmc0:0001/mmc0:0001:2/device:0x9142
/sys/class/mmc_host/mmc0/mmc0:0001/mmc0:0001:2/info1:Marvell Bluetooth Device
/sys/class/mmc_host/mmc0/mmc0:0001/mmc0:0001:2/modalias:sdio:c00v02DFd9142
/sys/class/mmc_host/mmc0/mmc0:0001/mmc0:0001:2/revision:1.0
/sys/class/mmc_host/mmc0/mmc0:0001/mmc0:0001:2/uevent:DRIVER=btmrvl_sdio
/sys/class/mmc_host/mmc0/mmc0:0001/mmc0:0001:2/uevent:SDIO_CLASS=00
/sys/class/mmc_host/mmc0/mmc0:0001/mmc0:0001:2/uevent:SDIO_ID=02DF:9142
/sys/class/mmc_host/mmc0/mmc0:0001/mmc0:0001:2/uevent:SDIO_REVISION=1.0
/sys/class/mmc_host/mmc0/mmc0:0001/mmc0:0001:2/uevent:SDIO_INFO1=Marvell 
Bluetooth Device
/sys/class/mmc_host/mmc0/mmc0:0001/mmc0:0001:2/uevent:SDIO_INFO2=
/sys/class/mmc_host/mmc0/mmc0:0001/mmc0:0001:2/uevent:MODALIAS=sdio:c00v02DFd9142
/sys/class/mmc_host/mmc0/mmc0:0001/mmc0:0001:2/vendor:0x02df
/sys/class/mmc_host/mmc0/mmc0:0001/power/control:auto
/sys/class/mmc_host/mmc0/mmc0:0001/power/runtime_active_time:0
/sys/class/mmc_host/mmc0/mmc0:0001/power/runtime_status:unsupported
/sys/class/mmc_host/mmc0/mmc0:0001/power/runtime_suspended_time:0
/sys/class/mmc_host/mmc0/mmc0:0001/subsystem/drivers_autoprobe:1

As can be seen SDIO card does not provide all 4 info strings as required by
SDIO/PCMCIA specificaion and the third and the second strings are empty.


Pali Rohár (4):
  mmc: sdio: Check for CISTPL_VERS_1 buffer size
  mmc: sdio: Parse CISTPL_VERS_1 major and minor revision numbers
  mmc: sdio: Extend sdio_config_attr macro and use it also for modalias
  mmc: sdio: Export SDIO revision and info strings to userspace

 drivers/mmc/core/bus.c| 12 
 drivers/mmc/core/sd.c | 36 +--
 drivers/mmc/core/sdio.c   | 24 
 drivers/mmc/core/sdio_bus.c   | 54 ++-
 drivers/mmc/core/sdio_cis.c   | 11 +++
 include/linux/mmc/card.h  |  2 ++
 include/linux/mmc/sdio_func.h |  2 ++
 7 files changed, 124 insertions(+), 17 deletions(-)

-- 
2.20.1

[PATCH] PCI: mvebu: Check if reset gpio is defined

[Pali Rohár]

Reset gpio is optional and it does not have to be defined for all boards.

So in mvebu_pcie_powerdown() like in mvebu_pcie_powerup() check that reset
gpio is defined prior usage to prevent NULL pointer dereference.

Signed-off-by: Pali Rohár 
---
 drivers/pci/controller/pci-mvebu.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pci-mvebu.c 
b/drivers/pci/controller/pci-mvebu.c
index 153a64676bc9..58607cbe84c8 100644
--- a/drivers/pci/controller/pci-mvebu.c
+++ b/drivers/pci/controller/pci-mvebu.c
@@ -947,7 +947,8 @@ static int mvebu_pcie_powerup(struct mvebu_pcie_port *port)
  */
 static void mvebu_pcie_powerdown(struct mvebu_pcie_port *port)
 {
-   gpiod_set_value_cansleep(port->reset_gpio, 1);
+   if (port->reset_gpio)
+   gpiod_set_value_cansleep(port->reset_gpio, 1);
 
clk_disable_unprepare(port->clk);
 }
-- 
2.20.1

[PATCH] pinctrl: armada-37xx: Add comment for pcie1_reset pin group

[Pali Rohár]

Group name 'pcie1' is misleading as it controls only PCIe reset pin. Like
other PCIe groups it should have been called 'pcie1_reset'. But due to
backward compatibility it is not possible to change existing group name.
So just add comment describing this PCIe reset functionality.

Signed-off-by: Pali Rohár 
---
 arch/arm64/boot/dts/marvell/armada-37xx.dtsi | 2 +-
 drivers/pinctrl/mvebu/pinctrl-armada-37xx.c  | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi 
b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
index 2bbc69b4dc99..d5b6c0a1c54a 100644
--- a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
+++ b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
@@ -316,7 +316,7 @@
};
 
pcie_reset_pins: pcie-reset-pins {
-   groups = "pcie1";
+   groups = "pcie1"; /* this actually 
controls "pcie1_reset" */
function = "gpio";
};
 
diff --git a/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c 
b/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c
index 5f125bd6279d..8fd8b2af9216 100644
--- a/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c
+++ b/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c
@@ -196,7 +196,7 @@ static struct armada_37xx_pin_group armada_37xx_sb_groups[] 
= {
PIN_GRP_GPIO("sdio_sb", 24, 6, BIT(2), "sdio"),
PIN_GRP_GPIO("rgmii", 6, 12, BIT(3), "mii"),
PIN_GRP_GPIO("smi", 18, 2, BIT(4), "smi"),
-   PIN_GRP_GPIO("pcie1", 3, 1, BIT(5), "pcie"),
+   PIN_GRP_GPIO("pcie1", 3, 1, BIT(5), "pcie"), /* this actually controls 
"pcie1_reset" */
PIN_GRP_GPIO("pcie1_clkreq", 4, 1, BIT(9), "pcie"),
PIN_GRP_GPIO("pcie1_wakeup", 5, 1, BIT(10), "pcie"),
PIN_GRP_GPIO("ptp", 20, 3, BIT(11) | BIT(12) | BIT(13), "ptp"),
-- 
2.20.1

Re: [PATCH v3] PCI: aardvark: Don't touch PCIe registers if no card connected

[Pali Rohár]

On Wednesday 15 July 2020 17:21:08 Lorenzo Pieralisi wrote:
> On Wed, Jul 15, 2020 at 02:17:26PM +0200, Pali Rohár wrote:
> > On Monday 13 July 2020 12:23:25 Lorenzo Pieralisi wrote:
> > > On Mon, Jul 13, 2020 at 10:27:47AM +0200, Pali Rohár wrote:
> > > > On Friday 10 July 2020 10:18:00 Lorenzo Pieralisi wrote:
> > > > > On Thu, Jul 09, 2020 at 05:09:59PM +0200, Pali Rohár wrote:
> > > > > > > I understand that but the bridge bus resource can be trimmed to 
> > > > > > > just
> > > > > > > contain the root bus because that's the only one where there is a
> > > > > > > chance you can enumerate a device.
> > > > > > 
> > > > > > It is possible to register only root bridge without endpoint?
> > > > > 
> > > > > It is possible to register the root bridge with a trimmed 
> > > > > IORESOURCE_BUS
> > > > > so that you don't enumerate anything other than the root port.
> > > > 
> > > > Hello Lorenzo! I really do not know how to achieve it. From code it
> > > > looks like that pci/probe.c scans child buses unconditionally.
> > > > 
> > > > pci-aardvark.c calls pci_host_probe() which calls functions
> > > > pci_scan_root_bus_bridge() which calls pci_scan_child_bus() which calls
> > > > pci_scan_child_bus_extend() which calls pci_scan_bridge_extend() (bridge
> > > > needs to be reconfigured) which then try to probe child bus via
> > > > pci_scan_child_bus_extend() because bridge is not card bus.
> > > > 
> > > > In function pci_scan_bridge_extend() I do not see a way how to skip
> > > > probing for child buses which would avoid enumerating aardvark root
> > > > bridge when PCIe device is not connected.
> > > > 
> > > > dmesg output contains:
> > > > 
> > > >   advk-pcie d007.pcie: link never came up
> > > >   advk-pcie d007.pcie: PCI host bridge to bus :00
> > > >   pci_bus :00: root bus resource [bus 00-ff]
> > > 
> > > This resource can be limited to the root bus number only before calling
> > > pci_host_probe() (ie see pci_parse_request_of_pci_ranges() and code in
> > > pci_scan_bridge_extend() that programs primary/secondary/subordinate
> > > busses) but I think that only papers over the issue, it does not fix it.
> > 
> > I looked at the code in pci/probe.c again and I do not think it is
> > possible to avoid scanning devices. pci_scan_child_bus_extend() is
> > unconditionally calling pci_scan_slot() for devfn=0 as the first thing.
> > And this function unconditionally calls pci_scan_device() which is
> > directly trying to read vendor id from config register.
> > 
> > So for me it looks like that kernel expects that can read vendor id and
> > device id from config register for device which is not connected.
> 
> Not if it is connected to a bus that the root port does not decode,
> that's what I am saying.
> 
> > And trying to read config register would cause those timeouts in
> > aardvark.
> 
> The root port (which effectively works as PCI bridge from this
> standpoint) does not issue config cycles for busses that aren't within
> its decoded bus range, which in turn is determined by the firmware
> IORESOURCE_BUS resource.
> 
> This issue is caused by devices that are connected downstream to
> the root port.
> 
> Anyway - patch merged

Could you send me a link to git commit? I have looked into
lpieralisi/pci.git repository, but I do not see it here.

> but I would be happy to keep this discussion going, somehow.

Ok, no problem. As I said if anybody has any idea or would like to see
some tests from me, I can do it and provide results.

> If the LPC20 VFIO/IOMMU/PCI microconference is approved it can be a
> good venue for this to happen.
> 
> Lorenzo

PCI: Race condition in pci_create_sysfs_dev_files

[Pali Rohár]

Hello Bjorn!

I see following error message in dmesg which looks like a race condition:

sysfs: cannot create duplicate filename 
'/devices/platform/soc/d007.pcie/pci:00/:00:00.0/config'

I looked at it deeper and found out that in PCI subsystem code is race
condition between pci_bus_add_device() and pci_sysfs_init() calls. Both
of these functions calls pci_create_sysfs_dev_files() and calling this
function more times for same pci device throws above error message.

There can be two different race conditions:

1. pci_bus_add_device() called pcibios_bus_add_device() or
pci_fixup_device() but have not called pci_create_sysfs_dev_files() yet.
Meanwhile pci_sysfs_init() is running and pci_create_sysfs_dev_files()
was called for newly registered device. In this case function
pci_create_sysfs_dev_files() is called two times, ones from
pci_bus_add_device() and once from pci_sysfs_init().

2. pci_sysfs_init() is called. It first sets sysfs_initialized to 1
which unblock calling pci_create_sysfs_dev_files(). Then another bus
registers new PCI device and calls pci_bus_add_device() which calls
pci_create_sysfs_dev_files() and registers sysfs files. Function
pci_sysfs_init() continues execution and calls function
pci_create_sysfs_dev_files() also for this newly registered device. So
pci_create_sysfs_dev_files() is again called two times.


I workaround both race conditions I created following hack patch. After
applying it I'm not getting that 'sysfs: cannot create duplicate filename'
error message anymore.

Can you look at it how to fix both race conditions in proper way?


diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index 8e40b3e6da77..691be2258c4e 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -316,7 +316,7 @@ void pci_bus_add_device(struct pci_dev *dev)
 */
pcibios_bus_add_device(dev);
pci_fixup_device(pci_fixup_final, dev);
-   pci_create_sysfs_dev_files(dev);
+   pci_create_sysfs_dev_files(dev, false);
pci_proc_attach_device(dev);
pci_bridge_d3_update(dev);
 
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 6d78df981d41..b0c4852a51dd 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -1328,13 +1328,13 @@ static int pci_create_capabilities_sysfs(struct pci_dev 
*dev)
return retval;
 }
 
-int __must_check pci_create_sysfs_dev_files(struct pci_dev *pdev)
+int __must_check pci_create_sysfs_dev_files(struct pci_dev *pdev, bool 
sysfs_initializing)
 {
int retval;
int rom_size;
struct bin_attribute *attr;
 
-   if (!sysfs_initialized)
+   if (!sysfs_initializing && !sysfs_initialized)
return -EACCES;
 
if (pdev->cfg_size > PCI_CFG_SPACE_SIZE)
@@ -1437,18 +1437,21 @@ void pci_remove_sysfs_dev_files(struct pci_dev *pdev)
 static int __init pci_sysfs_init(void)
 {
struct pci_dev *pdev = NULL;
-   int retval;
+   int retval = 0;
 
-   sysfs_initialized = 1;
for_each_pci_dev(pdev) {
-   retval = pci_create_sysfs_dev_files(pdev);
+   if (!pci_dev_is_added(pdev))
+   continue;
+   retval = pci_create_sysfs_dev_files(pdev, true);
if (retval) {
pci_dev_put(pdev);
-   return retval;
+   goto out;
}
}
 
-   return 0;
+out:
+   sysfs_initialized = 1;
+   return retval;
 }
 late_initcall(pci_sysfs_init);
 
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 6d3f75867106..304294c7171e 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -19,7 +19,7 @@ bool pcie_cap_has_rtctl(const struct pci_dev *dev);
 
 /* Functions internal to the PCI core code */
 
-int pci_create_sysfs_dev_files(struct pci_dev *pdev);
+int pci_create_sysfs_dev_files(struct pci_dev *pdev, bool sysfs_initializing);
 void pci_remove_sysfs_dev_files(struct pci_dev *pdev);
 #if !defined(CONFIG_DMI) && !defined(CONFIG_ACPI)
 static inline void pci_create_firmware_label_files(struct pci_dev *pdev)

Re: [PATCH] power: fix duplicated words in bq2415x_charger.h

[Pali Rohár]

On Wednesday 15 July 2020 18:30:01 Randy Dunlap wrote:
> From: Randy Dunlap 
> 
> Drop the doubled word "for".
> Change "It it" to "If it".
> 
> Signed-off-by: Randy Dunlap 
> Cc: Pali Rohár 
> Cc: Sebastian Reichel 
> Cc: linux...@vger.kernel.org

Thanks!

Acked-by: Pali Rohár 

> ---
>  include/linux/power/bq2415x_charger.h |4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> --- linux-next-20200714.orig/include/linux/power/bq2415x_charger.h
> +++ linux-next-20200714/include/linux/power/bq2415x_charger.h
> @@ -14,8 +14,8 @@
>   * value is -1 then default chip value (specified in datasheet) will be
>   * used.
>   *
> - * Value resistor_sense is needed for for configuring charge and
> - * termination current. It it is less or equal to zero, configuring charge
> + * Value resistor_sense is needed for configuring charge and
> + * termination current. If it is less or equal to zero, configuring charge
>   * and termination current will not be possible.
>   *
>   * For automode support is needed to provide name of power supply device
> 

Re: [PATCH v3] PCI: aardvark: Don't touch PCIe registers if no card connected

[Pali Rohár]

On Monday 13 July 2020 12:23:25 Lorenzo Pieralisi wrote:
> On Mon, Jul 13, 2020 at 10:27:47AM +0200, Pali Rohár wrote:
> > On Friday 10 July 2020 10:18:00 Lorenzo Pieralisi wrote:
> > > On Thu, Jul 09, 2020 at 05:09:59PM +0200, Pali Rohár wrote:
> > > > > I understand that but the bridge bus resource can be trimmed to just
> > > > > contain the root bus because that's the only one where there is a
> > > > > chance you can enumerate a device.
> > > > 
> > > > It is possible to register only root bridge without endpoint?
> > > 
> > > It is possible to register the root bridge with a trimmed IORESOURCE_BUS
> > > so that you don't enumerate anything other than the root port.
> > 
> > Hello Lorenzo! I really do not know how to achieve it. From code it
> > looks like that pci/probe.c scans child buses unconditionally.
> > 
> > pci-aardvark.c calls pci_host_probe() which calls functions
> > pci_scan_root_bus_bridge() which calls pci_scan_child_bus() which calls
> > pci_scan_child_bus_extend() which calls pci_scan_bridge_extend() (bridge
> > needs to be reconfigured) which then try to probe child bus via
> > pci_scan_child_bus_extend() because bridge is not card bus.
> > 
> > In function pci_scan_bridge_extend() I do not see a way how to skip
> > probing for child buses which would avoid enumerating aardvark root
> > bridge when PCIe device is not connected.
> > 
> > dmesg output contains:
> > 
> >   advk-pcie d007.pcie: link never came up
> >   advk-pcie d007.pcie: PCI host bridge to bus :00
> >   pci_bus :00: root bus resource [bus 00-ff]
> 
> This resource can be limited to the root bus number only before calling
> pci_host_probe() (ie see pci_parse_request_of_pci_ranges() and code in
> pci_scan_bridge_extend() that programs primary/secondary/subordinate
> busses) but I think that only papers over the issue, it does not fix it.

I looked at the code in pci/probe.c again and I do not think it is
possible to avoid scanning devices. pci_scan_child_bus_extend() is
unconditionally calling pci_scan_slot() for devfn=0 as the first thing.
And this function unconditionally calls pci_scan_device() which is
directly trying to read vendor id from config register.

So for me it looks like that kernel expects that can read vendor id and
device id from config register for device which is not connected.

And trying to read config register would cause those timeouts in
aardvark.

Re: [PATCH v3] PCI: aardvark: Don't touch PCIe registers if no card connected

[Pali Rohár]

On Monday 13 July 2020 17:41:40 Lorenzo Pieralisi wrote:
> On Mon, Jul 13, 2020 at 04:50:03PM +0200, Pali Rohár wrote:
> > On Monday 13 July 2020 12:23:25 Lorenzo Pieralisi wrote:
> > > I will go over the thread again but I suspect I can merge the patch even
> > > though I still believe there is work to be done to understand the issue
> > > we are facing.
> > 
> > Just to note that pci-mvebu.c also checks if pcie link is up before
> > trying to access the real PCIe interface registers, similarly as in my
> > patch.
> 
> I understand - that does not change my opinion though, the link check
> is just a workaround, it'd be best if we pinpoint the real issue which
> is likely to a HW one.

Lorenzo, if you have an idea how to debug this issue or if you would
like to see some test results, let me know. I can do some tests, but I
currently really do not know more then what I wrote in previous emails.

In my opinion, problem is in HW which Marvell has not documented nor
proved that it exists. Other option is that problem is in Compex card
which can be triggered only by Marvell aardvark HW.

Re: [PATCH v3] PCI: aardvark: Don't touch PCIe registers if no card connected

[Pali Rohár]

On Monday 13 July 2020 12:23:25 Lorenzo Pieralisi wrote:
> I will go over the thread again but I suspect I can merge the patch even
> though I still believe there is work to be done to understand the issue
> we are facing.

Just to note that pci-mvebu.c also checks if pcie link is up before
trying to access the real PCIe interface registers, similarly as in my
patch.

Re: [PATCH v3] PCI: aardvark: Don't touch PCIe registers if no card connected

[Pali Rohár]

On Friday 10 July 2020 10:18:00 Lorenzo Pieralisi wrote:
> On Thu, Jul 09, 2020 at 05:09:59PM +0200, Pali Rohár wrote:
> > > I understand that but the bridge bus resource can be trimmed to just
> > > contain the root bus because that's the only one where there is a
> > > chance you can enumerate a device.
> > 
> > It is possible to register only root bridge without endpoint?
> 
> It is possible to register the root bridge with a trimmed IORESOURCE_BUS
> so that you don't enumerate anything other than the root port.

Hello Lorenzo! I really do not know how to achieve it. From code it
looks like that pci/probe.c scans child buses unconditionally.

pci-aardvark.c calls pci_host_probe() which calls functions
pci_scan_root_bus_bridge() which calls pci_scan_child_bus() which calls
pci_scan_child_bus_extend() which calls pci_scan_bridge_extend() (bridge
needs to be reconfigured) which then try to probe child bus via
pci_scan_child_bus_extend() because bridge is not card bus.

In function pci_scan_bridge_extend() I do not see a way how to skip
probing for child buses which would avoid enumerating aardvark root
bridge when PCIe device is not connected.

dmesg output contains:

  advk-pcie d007.pcie: link never came up
  advk-pcie d007.pcie: PCI host bridge to bus :00
  pci_bus :00: root bus resource [bus 00-ff]
  pci_bus :00: root bus resource [mem 0xe800-0xe8ff]
  pci_bus :00: root bus resource [io  0x-0x] (bus address 
[0xe900-0xe900])
  pci_bus :00: scanning bus
  pci :00:00.0: [1b4b:0100] type 01 class 0x060400
  pci :00:00.0: reg 0x38: [mem 0x-0x07ff pref]
  pci_bus :00: fixups for bus
  pci :00:00.0: scanning [bus 00-00] behind bridge, pass 0
  pci :00:00.0: bridge configuration invalid ([bus 00-00]), reconfiguring
  pci :00:00.0: scanning [bus 00-00] behind bridge, pass 1
  pci_bus :01: scanning bus
  advk-pcie d007.pcie: advk_pcie_valid_device

Re: [PATCH v4 0/3] platform/x86: dell-wmi: new keys

[Pali Rohár]

On Thursday 09 July 2020 22:29:42 Andy Shevchenko wrote:
> On Wed, Jun 10, 2020 at 10:23 PM  wrote:
> >
> > > -Original Message-
> > > From: Y Paritcher 
> > > Sent: Wednesday, June 10, 2020 12:57 PM
> > > To: Pali Rohár
> > > Cc: linux-kernel@vger.kernel.org; platform-driver-...@vger.kernel.org;
> > > Matthew Garrett; Limonciello, Mario
> > > Subject: [PATCH v4 0/3] platform/x86: dell-wmi: new keys
> > >
> > >
> > > [EXTERNAL EMAIL]
> > >
> > > change since v3:
> > > No code changes.
> > > Update commit message to reflect info given by Mario at dell.
> > >
> > > Is there anything more i have to do for the patches that were reviewed
> > > or will they be picked up by the maintainers?
> > > Thanks
> > >
> > > Y Paritcher (3):
> > >   platform/x86: dell-wmi: add new backlight events
> > >   platform/x86: dell-wmi: add new keymap type 0x0012
> > >   platform/x86: dell-wmi: add new dmi mapping for keycode 0x
> > >
> > >  drivers/platform/x86/dell-wmi.c | 28 +---
> > >  1 file changed, 25 insertions(+), 3 deletions(-)
> > >
> > > --
> > > 2.27.0
> >
> > Andy,
> >
> > The whole series looks good to me now.  You can put this on the patches
> > when they're swooped up.
> >
> > Reviewed-by: Mario Limonciello 
> >
> > However I would like to note there was a comment that you had a direct 
> > question
> > asked by Pali that probably got lost in the thread.  This was on patch 3/3 
> > on v3.
> > I think it's worth answering as it could dictate a follow up patch to 
> > change behavior.
> >
> > The summary of my argument which led to his is nested somewhere in the 
> > thread was that
> > to most users this isn't useful since they can't act on it.  IE they can't 
> > use something
> > like setkeycodes and go on their merry way.  The user who could act on it 
> > by coming
> > to upstream and submitting questions and patches is more technical and 
> > having them
> > use dyndbg to turn on the messages about unknown shouldn't be a big deal.
> >
> > > I'm not sure, but I thought that
> > > throwing warning or info message is the correct solution. Driver cannot
> > > handle something, so it inform about it, instead of silently dropping
> > > event. Same behavior I'm seeing in other kernel drivers.
> >
> > > But looks like that you and Mario have opposite opinion, that kernel
> > > should not log unknown events and rather should drop them.
> >
> > > I would like to have behavior of dell-wmi same as in other drivers for
> > > consistency, so the best would be to ask WMI/platform maintainers. They
> > > could have opinion how to handle these problem globally.
> >
> > > ...
> >
> > > Darren & Andy, could you please say something to this, what do you think
> > > about silently dropping events/actions which are currently unknown for
> > > dell-wmi driver? It is better to log them or not? Currently we are
> > > logging them.
> >
> > Can you please advise which way you would rather have the subsystem go?
> 
> Seems Pali is okay with this version, so everything is settled I suppose.
> I will add it to my queue, thanks!

Hello Andy! Yes, I'm fine with this patch series, but question how to
handle these "unknown" events still remains.

Re: [PATCH v3] PCI: aardvark: Don't touch PCIe registers if no card connected

[Pali Rohár]

On Friday 10 July 2020 11:08:28 Bjorn Helgaas wrote:
> On Fri, Jul 10, 2020 at 05:44:58PM +0200, Pali Rohár wrote:
> > I can reproduce following issue: Connect Compex WLE900VX card, configure
> > aardvark to gen2 mode. And then card is detected only after the first
> > link training. If kernel tries to retrain link again (e.g. via ASPM
> > code) then card is not detected anymore. 
> 
> Somebody should go over the ASPM retrain link code and the PCIe spec
> with a fine-toothed comb.  Maybe we're doing something wrong there.

I think this is not ASPM related as card simply disappear just after
flipping PCI_EXP_LNKCTL_RL bit second time without changing ASPM bits.

> Or maybe aardvark has some hardware issue and we need some sort of
> quirk to work around it.

It is possible that this is aardvark issue. As I said I really do not
know.

In aardvark driver there is already merged workaround for this issue:
driver force gen1 aardvark mode for gen1 card.

> > Another issue which happens for WLE900VX, WLE600VX and WLE1216VS-20 (but
> > not for WLE200VX): Linux kernel can detect these cards only if it issues
> > card reset via PERST# signal and start link training (via standard pcie
> > endpoint register PCI_EXP_LNKCTL/PCI_EXP_LNKCTL_RL)
> 
> I think you mean "downstream port" (not "endpoint") register?

Yes.

> PCI_EXP_LNKCTL_RL is only applicable to *downstream ports* (root ports
> or switch downstream ports) and is reserved for endpoints.
> 
> > immediately after
> > enable link training in aardvark (via aardvark specific LINK_TRAINING_EN
> > bit). If there is e.g. 100ms delay between enabling link training and
> > setting PCI_EXP_LNKCTL_RL bit then these cards are not detected.
> 
> This sounds problematic.  Hardware should not be dependent on the
> software being "fast enough".  In general we should be able to insert
> arbitrary delays at any point without breaking anything.

Yes, it is problematic. For example following commit broke those cards:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f4c7d053d7f77cd5c1a1ba7c7ce085ddba13d1d7

And this commit fixed it (just msleep was moved to different stage):
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6964494582f56a3882c2c53b0edbfe99eb32b2e1

But we somehow need to deal with it until we find root cause.

Basically additional sleep in aardvark init phase can break WLE900VX
cards, but not WLE200VX.

And because WLE900VX works fine with pci-mvebu and WLE200VX works fine
with pci-aardvark we cannot deduce from it if problem for combination of
WLE900VX and aardvark is in WLE900VX or in aardvark.

> But I have the impression that aardvark requires more software
> hand-holding that most hardware does.  If it imposes timing
> requirements on the software, that *should* be documented in the
> aardvark spec.

There is absolutely nothing regarding to timings in documentation which
I saw. In documentation are just instructions/steps how to init PCI
subsystem and it is basically advk_pcie_setup_hw() function.

> > I read in kernel bugzilla that WLE600VX and WLE900VX cards are buggy and
> > more people have problems with them. But issues described in kernel
> > bugzilla (like card is reporting incorrect PCI device id) I'm not
> > observing.
> 
> Pointer?

Hm... I cannot find right now pointer to bugzilla, but I have pointer to
ath9k-devel mailing list with that incorrect device id:

https://www.mail-archive.com/ath9k-devel@lists.ath9k.org/msg07529.html

> Is the incorrect device ID 0x?

No, incorrect device ID in that case is 0xabcd and vendor ID is correct
(Qualcomm).

> That could be a symptom
> of a PCIe error.  If we read a device ID that's something other than
> 0, 0x, or the correct ID, that would be really weird.  Even 0
> would be really strange.

It is strange and also reason why discussion on that list is long.

As I said, I'm not seeing that problem with wrong device ID.

But I know people who are observing same problem on different boards
(which do not use aardvark) as described in above mailing list thread
with Compex ath10k cards.

> I suspect these wifi cards are a little special because they probably
> play unusual games with power for airplane mode and the like.

This is another/different problem and is already "documented" in kernel
bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=84821#c52

Re: [PATCH v3] PCI: aardvark: Don't touch PCIe registers if no card connected

[Pali Rohár]

On Friday 10 July 2020 10:18:00 Lorenzo Pieralisi wrote:
> I would be very grateful if you could describe what happens in HW
> when these conditions trigger - I would like to understand if this
> issue is aardvark specific or it isn't.

Hello Lorenzo! We are not sure what is the problem and where it happens.

There are more issues which happens randomly or under some specific
conditions.

I can reproduce following issue: Connect Compex WLE900VX card, configure
aardvark to gen2 mode. And then card is detected only after the first
link training. If kernel tries to retrain link again (e.g. via ASPM
code) then card is not detected anymore. To detect it again it is needed
to reset card via PERST# signal (assert PERST#, wait, de-assert PERST#).
PCI warm, hot or function reset does not help. When aardvark is
configured in gen1 mode then card is detected fine also after multiple
link training.

Above problem does not happen with Compex WLE200VX (ath9k) or Compex
WLE1216V5-20 cards.

Sometimes WLE900VX card disappear from the bus during usage. It just
stop communicates with ath10k driver and aardvark does not see link.

Another issue which happens for WLE900VX, WLE600VX and WLE1216VS-20 (but
not for WLE200VX): Linux kernel can detect these cards only if it issues
card reset via PERST# signal and start link training (via standard pcie
endpoint register PCI_EXP_LNKCTL/PCI_EXP_LNKCTL_RL) immediately after
enable link training in aardvark (via aardvark specific LINK_TRAINING_EN
bit). If there is e.g. 100ms delay between enabling link training and
setting PCI_EXP_LNKCTL_RL bit then these cards are not detected.

Also issuing reset via PERST# signal is required to detect these cards
if either board was rebooted (not started from cold power off state) or
if U-Boot touched/initialized PCIe aardvark.

WLE200VX works fine also after doing second or third link training and
also works without need to issue reset via PERST# signal.

And WLE900VX card is not detected even after resetting it via PERST#
signal if aardvark link training (LINK_TRAINING_EN bit) was enabled
prior toggling PERST#. PERST# signal is controlled via GPIO.

When I put WLE900VX card into board with uses mvebu PCI driver (not
aardvak) then card is working fine, there is no need to issue card reset
via PERST#, no need to explicitly set gen mode and card is also working
after more link training.

So basically I have no idea why it happens or where is the problem,
either in aardvark or in cards or on both places. As you can see each of
tested card has different set problems.

Today I tested card from different vendor but with same Qualcomm chip as
is in WLE900VX and I observe same behavior as from Compex WLE900VX. So
it looks like that card vendor does not have to matter, important is
wifi chip inside.

I read in kernel bugzilla that WLE600VX and WLE900VX cards are buggy and
more people have problems with them. But issues described in kernel
bugzilla (like card is reporting incorrect PCI device id) I'm not
observing.

If you have any idea how to either debug these problems or come up with
idea where could be the problem, please let me know.

Re: [PATCH v3] PCI: aardvark: Don't touch PCIe registers if no card connected

[Pali Rohár]

On Thursday 09 July 2020 15:47:01 Lorenzo Pieralisi wrote:
> On Thu, Jul 09, 2020 at 02:22:08PM +0200, Pali Rohár wrote:
> > On Thursday 09 July 2020 12:35:09 Lorenzo Pieralisi wrote:
> > > On Thu, Jul 02, 2020 at 10:30:36AM +0200, Pali Rohár wrote:
> > > > When there is no PCIe card connected and advk_pcie_rd_conf() or
> > > > advk_pcie_wr_conf() is called for PCI bus which doesn't belong to 
> > > > emulated
> > > > root bridge, the aardvark driver throws the following error message:
> > > > 
> > > >   advk-pcie d007.pcie: config read/write timed out
> > > > 
> > > > Obviously accessing PCIe registers of disconnected card is not possible.
> > > > 
> > > > Extend check in advk_pcie_valid_device() function for validating
> > > > availability of PCIe bus. If PCIe link is down, then the device is 
> > > > marked
> > > > as Not Found and the driver does not try to access these registers.
> > > > 
> > > > This is just an optimization to prevent accessing PCIe registers when 
> > > > card
> > > > is disconnected. Trying to access PCIe registers of disconnected card 
> > > > does
> > > > not cause any crash, kernel just needs to wait for a timeout. So if card
> > > > disappear immediately after checking for PCIe link (before accessing 
> > > > PCIe
> > > > registers), it does not cause any problems.
> > > > 
> > > > Signed-off-by: Pali Rohár 
> > > > 
> > > > ---
> > > > Changes in V3:
> > > > * Add comment to the code
> > > > Changes in V2:
> > > > * Update commit message, mention that this is optimization
> > > > ---
> > > >  drivers/pci/controller/pci-aardvark.c | 7 +++
> > > >  1 file changed, 7 insertions(+)
> > > > 
> > > > diff --git a/drivers/pci/controller/pci-aardvark.c 
> > > > b/drivers/pci/controller/pci-aardvark.c
> > > > index 90ff291c24f0..d18f389b36a1 100644
> > > > --- a/drivers/pci/controller/pci-aardvark.c
> > > > +++ b/drivers/pci/controller/pci-aardvark.c
> > > > @@ -644,6 +644,13 @@ static bool advk_pcie_valid_device(struct 
> > > > advk_pcie *pcie, struct pci_bus *bus,
> > > > if ((bus->number == pcie->root_bus_nr) && PCI_SLOT(devfn) != 0)
> > > > return false;
> > > >  
> > > > +   /*
> > > > +* If the link goes down after we check for link-up, nothing bad
> > > > +* happens but the config access times out.
> > > > +*/
> > > > +   if (bus->number != pcie->root_bus_nr && 
> > > > !advk_pcie_link_up(pcie))
> > > > +   return false;
> > > > +
> > > > return true;
> > > >  }
> > > 
> > > Question: this basically means that you can only effectively enumerate
> > > bus number == root_bus_nr and AFAICS if at probe the link did not
> > > come up it will never do, will it ?
> > > 
> > > Isn't this equivalent to limiting the bus numbers the bridge is capable
> > > of handling ?
> > > 
> > > Reworded: if in advk_pcie_setup_hw() the link does not come up, what's
> > > the point of trying to enumerate the bus hierarchy below the root bus ?
> > 
> > Hello Lorenzo!
> > 
> > PCIe link can theoretically come up even after boot, but aardvark driver
> > currently does not support link detection at runtime. So it checks and
> > enumerate device only at probe time.
> 
> If the link is not up at probe enumerating devices below the root
> bus is basically useless and that's actually what is causing the
> delays you are fixing. Is this correct ?

Yes, this is one (but not the only one) delay.

> > I do not know if hardware has some mechanism to inform kernel that PCIe
> > link come up (or down) and re-enumeration is required. Or the only
> > option is polling via advk_pcie_link_up().
> > 
> > So if device is not visible at the probe time then it would not appear
> > in system and cannot be used. This is current state.
> > 
> > Just to note that our hardware does not support physical hotplug of
> > mPCIe cards. You need to connect card when board is powered off.
> > 
> > So if at the aardvark probe time PCIe link is not up then trying to
> > enumerate devices under (software) root bridge is not needed. But it is

Re: [PATCH v3] PCI: aardvark: Don't touch PCIe registers if no card connected

[Pali Rohár]

On Thursday 09 July 2020 12:35:09 Lorenzo Pieralisi wrote:
> On Thu, Jul 02, 2020 at 10:30:36AM +0200, Pali Rohár wrote:
> > When there is no PCIe card connected and advk_pcie_rd_conf() or
> > advk_pcie_wr_conf() is called for PCI bus which doesn't belong to emulated
> > root bridge, the aardvark driver throws the following error message:
> > 
> >   advk-pcie d007.pcie: config read/write timed out
> > 
> > Obviously accessing PCIe registers of disconnected card is not possible.
> > 
> > Extend check in advk_pcie_valid_device() function for validating
> > availability of PCIe bus. If PCIe link is down, then the device is marked
> > as Not Found and the driver does not try to access these registers.
> > 
> > This is just an optimization to prevent accessing PCIe registers when card
> > is disconnected. Trying to access PCIe registers of disconnected card does
> > not cause any crash, kernel just needs to wait for a timeout. So if card
> > disappear immediately after checking for PCIe link (before accessing PCIe
> > registers), it does not cause any problems.
> > 
> > Signed-off-by: Pali Rohár 
> > 
> > ---
> > Changes in V3:
> > * Add comment to the code
> > Changes in V2:
> > * Update commit message, mention that this is optimization
> > ---
> >  drivers/pci/controller/pci-aardvark.c | 7 +++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/drivers/pci/controller/pci-aardvark.c 
> > b/drivers/pci/controller/pci-aardvark.c
> > index 90ff291c24f0..d18f389b36a1 100644
> > --- a/drivers/pci/controller/pci-aardvark.c
> > +++ b/drivers/pci/controller/pci-aardvark.c
> > @@ -644,6 +644,13 @@ static bool advk_pcie_valid_device(struct advk_pcie 
> > *pcie, struct pci_bus *bus,
> > if ((bus->number == pcie->root_bus_nr) && PCI_SLOT(devfn) != 0)
> > return false;
> >  
> > +   /*
> > +* If the link goes down after we check for link-up, nothing bad
> > +* happens but the config access times out.
> > +*/
> > +   if (bus->number != pcie->root_bus_nr && !advk_pcie_link_up(pcie))
> > +   return false;
> > +
> > return true;
> >  }
> 
> Question: this basically means that you can only effectively enumerate
> bus number == root_bus_nr and AFAICS if at probe the link did not
> come up it will never do, will it ?
> 
> Isn't this equivalent to limiting the bus numbers the bridge is capable
> of handling ?
> 
> Reworded: if in advk_pcie_setup_hw() the link does not come up, what's
> the point of trying to enumerate the bus hierarchy below the root bus ?

Hello Lorenzo!

PCIe link can theoretically come up even after boot, but aardvark driver
currently does not support link detection at runtime. So it checks and
enumerate device only at probe time.

I do not know if hardware has some mechanism to inform kernel that PCIe
link come up (or down) and re-enumeration is required. Or the only
option is polling via advk_pcie_link_up().

So if device is not visible at the probe time then it would not appear
in system and cannot be used. This is current state.

Just to note that our hardware does not support physical hotplug of
mPCIe cards. You need to connect card when board is powered off.

So if at the aardvark probe time PCIe link is not up then trying to
enumerate devices under (software) root bridge is not needed. But it is
needed to register/enumerate software root bridge device and currently
both is done by one (recursive) call pci_host_probe().

Re: [PATCH] PCI: aardvark: Indicate error in 'val' when config read fails

[Pali Rohár]

On Tuesday 07 July 2020 14:53:11 Lorenzo Pieralisi wrote:
> On Fri, Jun 19, 2020 at 12:56:18PM +0200, Pali Rohár wrote:
> > Hello Lorenzo! Could you please review this patch?
> > 
> > On Monday 01 June 2020 15:03:15 Pali Rohár wrote:
> > > Most callers of config read do not check for return value. But most of the
> > > ones that do, checks for error indication in 'val' variable.
> > > 
> > > This patch updates error handling in advk_pcie_rd_conf() function. If PIO
> > > transfer fails then 'val' variable is set to 0x which indicates
> > > failture.
> > > 
> > > Signed-off-by: Pali Rohár 
> > 
> > I should add credit for Bjorn as he found this issue
> 
> Could you provide a lore archive link to the relevant
> discussion please ? I will apply it then.

Hello Lorenzo! Here is link to the Bjorn's email:
https://lore.kernel.org/linux-pci/20200528162604.GA323482@bjorn-Precision-5520/

> Lorenzo
> 
> > Reported-by: Bjorn Helgaas 
> > 
> > > ---
> > >  drivers/pci/controller/pci-aardvark.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/pci/controller/pci-aardvark.c 
> > > b/drivers/pci/controller/pci-aardvark.c
> > > index 53a4cfd7d377..783a7f1f2c44 100644
> > > --- a/drivers/pci/controller/pci-aardvark.c
> > > +++ b/drivers/pci/controller/pci-aardvark.c
> > > @@ -691,8 +691,10 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, 
> > > u32 devfn,
> > >   advk_writel(pcie, 1, PIO_START);
> > >  
> > >   ret = advk_pcie_wait_pio(pcie);
> > > - if (ret < 0)
> > > + if (ret < 0) {
> > > + *val = 0x;
> > >   return PCIBIOS_SET_FAILED;
> > > + }
> > >  
> > >   advk_pcie_check_pio_status(pcie);
> > >  
> > > -- 
> > > 2.20.1
> > > 

[PATCH] mwifiex: Fix reporting 'operation not supported' error code

[Pali Rohár]

ENOTSUPP (double PP) is internal linux kernel code 524 available only in
kernel include file linux/errno.h and not exported to userspace.

EOPNOTSUPP (OP; double PP) is standard code 95 for reporting 'operation not
supported' available via kernel include file uapi/asm-generic/errno.h.

ENOTSUP (single P) is alias for EOPNOTSUPP defined only in userspace
include file bits/errno.h and not available in kernel.

Because Linux kernel does not support ENOTSUP (single P) and because
userspace does not support ENOTSUPP (double PP), report error code for
'operation not supported' via EOPNOTSUPP macro.

This patch fixes problem that mwifiex kernel driver sends to userspace
unsupported error codes like: "failed: -524 (No error information)".
After applying this patch userspace see: "failed: -95 (Not supported)".

Signed-off-by: Pali Rohár 
---
 .../net/wireless/marvell/mwifiex/cfg80211.c| 18 +-
 drivers/net/wireless/marvell/mwifiex/main.c|  2 +-
 drivers/net/wireless/marvell/mwifiex/sta_cmd.c |  4 ++--
 3 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c 
b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
index 867b5cf385a8..96848fa0e417 100644
--- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
+++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
@@ -3727,11 +3727,11 @@ mwifiex_cfg80211_tdls_mgmt(struct wiphy *wiphy, struct 
net_device *dev,
int ret;
 
if (!(wiphy->flags & WIPHY_FLAG_SUPPORTS_TDLS))
-   return -ENOTSUPP;
+   return -EOPNOTSUPP;
 
/* make sure we are in station mode and connected */
if (!(priv->bss_type == MWIFIEX_BSS_TYPE_STA && priv->media_connected))
-   return -ENOTSUPP;
+   return -EOPNOTSUPP;
 
switch (action_code) {
case WLAN_TDLS_SETUP_REQUEST:
@@ -3799,11 +3799,11 @@ mwifiex_cfg80211_tdls_oper(struct wiphy *wiphy, struct 
net_device *dev,
 
if (!(wiphy->flags & WIPHY_FLAG_SUPPORTS_TDLS) ||
!(wiphy->flags & WIPHY_FLAG_TDLS_EXTERNAL_SETUP))
-   return -ENOTSUPP;
+   return -EOPNOTSUPP;
 
/* make sure we are in station mode and connected */
if (!(priv->bss_type == MWIFIEX_BSS_TYPE_STA && priv->media_connected))
-   return -ENOTSUPP;
+   return -EOPNOTSUPP;
 
mwifiex_dbg(priv->adapter, MSG,
"TDLS peer=%pM, oper=%d\n", peer, action);
@@ -3833,7 +3833,7 @@ mwifiex_cfg80211_tdls_oper(struct wiphy *wiphy, struct 
net_device *dev,
default:
mwifiex_dbg(priv->adapter, ERROR,
"tdls_oper: operation not supported\n");
-   return -ENOTSUPP;
+   return -EOPNOTSUPP;
}
 
return mwifiex_tdls_oper(priv, peer, action);
@@ -3914,11 +3914,11 @@ mwifiex_cfg80211_add_station(struct wiphy *wiphy, 
struct net_device *dev,
struct mwifiex_private *priv = mwifiex_netdev_get_priv(dev);
 
if (!(params->sta_flags_set & BIT(NL80211_STA_FLAG_TDLS_PEER)))
-   return -ENOTSUPP;
+   return -EOPNOTSUPP;
 
/* make sure we are in station mode and connected */
if ((priv->bss_type != MWIFIEX_BSS_TYPE_STA) || !priv->media_connected)
-   return -ENOTSUPP;
+   return -EOPNOTSUPP;
 
return mwifiex_tdls_oper(priv, mac, MWIFIEX_TDLS_CREATE_LINK);
 }
@@ -4151,11 +4151,11 @@ mwifiex_cfg80211_change_station(struct wiphy *wiphy, 
struct net_device *dev,
 
/* we support change_station handler only for TDLS peers*/
if (!(params->sta_flags_set & BIT(NL80211_STA_FLAG_TDLS_PEER)))
-   return -ENOTSUPP;
+   return -EOPNOTSUPP;
 
/* make sure we are in station mode and connected */
if ((priv->bss_type != MWIFIEX_BSS_TYPE_STA) || !priv->media_connected)
-   return -ENOTSUPP;
+   return -EOPNOTSUPP;
 
priv->sta_params = params;
 
diff --git a/drivers/net/wireless/marvell/mwifiex/main.c 
b/drivers/net/wireless/marvell/mwifiex/main.c
index 529099137644..9ee5600351a7 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.c
+++ b/drivers/net/wireless/marvell/mwifiex/main.c
@@ -953,7 +953,7 @@ int mwifiex_set_mac_address(struct mwifiex_private *priv,
} else {
/* Internal mac address change */
if (priv->bss_type == MWIFIEX_BSS_TYPE_ANY)
-   return -ENOTSUPP;
+   return -EOPNOTSUPP;
 
mac_addr = old_mac_addr;
 
diff --git a/drivers/net/wireless/marvell/mwifiex/sta_cmd.c 
b/drivers/net/wireless/marvell/mwifiex/sta_cmd.c
index 8bd355d7974e..d3a968ef21ef 100644
--- a/drivers/net/wireless/marvell/mwifiex/sta_cmd.c
+++ b/drivers/net/wireless/marvell/mwifiex/sta_

[PATCH v3] PCI: aardvark: Don't touch PCIe registers if no card connected

[Pali Rohár]

When there is no PCIe card connected and advk_pcie_rd_conf() or
advk_pcie_wr_conf() is called for PCI bus which doesn't belong to emulated
root bridge, the aardvark driver throws the following error message:

  advk-pcie d007.pcie: config read/write timed out

Obviously accessing PCIe registers of disconnected card is not possible.

Extend check in advk_pcie_valid_device() function for validating
availability of PCIe bus. If PCIe link is down, then the device is marked
as Not Found and the driver does not try to access these registers.

This is just an optimization to prevent accessing PCIe registers when card
is disconnected. Trying to access PCIe registers of disconnected card does
not cause any crash, kernel just needs to wait for a timeout. So if card
disappear immediately after checking for PCIe link (before accessing PCIe
registers), it does not cause any problems.

Signed-off-by: Pali Rohár 

---
Changes in V3:
* Add comment to the code
Changes in V2:
* Update commit message, mention that this is optimization
---
 drivers/pci/controller/pci-aardvark.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/drivers/pci/controller/pci-aardvark.c 
b/drivers/pci/controller/pci-aardvark.c
index 90ff291c24f0..d18f389b36a1 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -644,6 +644,13 @@ static bool advk_pcie_valid_device(struct advk_pcie *pcie, 
struct pci_bus *bus,
if ((bus->number == pcie->root_bus_nr) && PCI_SLOT(devfn) != 0)
return false;
 
+   /*
+* If the link goes down after we check for link-up, nothing bad
+* happens but the config access times out.
+*/
+   if (bus->number != pcie->root_bus_nr && !advk_pcie_link_up(pcie))
+   return false;
+
return true;
 }
 
-- 
2.20.1

Re: [PATCH v2] PCI: aardvark: Don't touch PCIe registers if no card connected

[Pali Rohár]

On Wednesday 01 July 2020 16:34:42 Bjorn Helgaas wrote:
> On Wed, Jul 01, 2020 at 10:20:44AM +0200, Pali Rohár wrote:
> > When there is no PCIe card connected and advk_pcie_rd_conf() or
> > advk_pcie_wr_conf() is called for PCI bus which doesn't belong to emulated
> > root bridge, the aardvark driver throws the following error message:
> > 
> >   advk-pcie d007.pcie: config read/write timed out
> > 
> > Obviously accessing PCIe registers of disconnected card is not possible.
> > 
> > Extend check in advk_pcie_valid_device() function for validating
> > availability of PCIe bus. If PCIe link is down, then the device is marked
> > as Not Found and the driver does not try to access these registers.
> > 
> > This is just an optimization to prevent accessing PCIe registers when card
> > is disconnected. Trying to access PCIe registers of disconnected card does
> > not cause any crash, kernel just needs to wait for a timeout. So if card
> > disappear immediately after checking for PCIe link (before accessing PCIe
> > registers), it does not cause any problems.
> 
> Thanks, this is good.  I'd really like a short comment in the code as
> well, because this sort of link-up check tends to get copied to new
> drivers where it shouldn't be used, e.g., something like this:
> 
>   /*
>* If the link goes down after we check for link-up, nothing bad
>* happens but the config access times out.
>*/

Ok, it makes sense! I will send a new patch version.

> > Signed-off-by: Pali Rohár 
> > 
> > ---
> > Changes in V2:
> > * Update commit message, mention that this is optimization
> > ---
> >  drivers/pci/controller/pci-aardvark.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/pci/controller/pci-aardvark.c 
> > b/drivers/pci/controller/pci-aardvark.c
> > index 90ff291c24f0..53a4cfd7d377 100644
> > --- a/drivers/pci/controller/pci-aardvark.c
> > +++ b/drivers/pci/controller/pci-aardvark.c
> > @@ -644,6 +644,9 @@ static bool advk_pcie_valid_device(struct advk_pcie 
> > *pcie, struct pci_bus *bus,
> > if ((bus->number == pcie->root_bus_nr) && PCI_SLOT(devfn) != 0)
> > return false;
> >  
> > +   if (bus->number != pcie->root_bus_nr && !advk_pcie_link_up(pcie))
> > +   return false;
> > +
> > return true;
> >  }
> >  
> > -- 
> > 2.20.1
> > 

[PATCH v2] PCI: aardvark: Don't touch PCIe registers if no card connected

[Pali Rohár]

When there is no PCIe card connected and advk_pcie_rd_conf() or
advk_pcie_wr_conf() is called for PCI bus which doesn't belong to emulated
root bridge, the aardvark driver throws the following error message:

  advk-pcie d007.pcie: config read/write timed out

Obviously accessing PCIe registers of disconnected card is not possible.

Extend check in advk_pcie_valid_device() function for validating
availability of PCIe bus. If PCIe link is down, then the device is marked
as Not Found and the driver does not try to access these registers.

This is just an optimization to prevent accessing PCIe registers when card
is disconnected. Trying to access PCIe registers of disconnected card does
not cause any crash, kernel just needs to wait for a timeout. So if card
disappear immediately after checking for PCIe link (before accessing PCIe
registers), it does not cause any problems.

Signed-off-by: Pali Rohár 

---
Changes in V2:
* Update commit message, mention that this is optimization
---
 drivers/pci/controller/pci-aardvark.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/pci/controller/pci-aardvark.c 
b/drivers/pci/controller/pci-aardvark.c
index 90ff291c24f0..53a4cfd7d377 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -644,6 +644,9 @@ static bool advk_pcie_valid_device(struct advk_pcie *pcie, 
struct pci_bus *bus,
if ((bus->number == pcie->root_bus_nr) && PCI_SLOT(devfn) != 0)
return false;
 
+   if (bus->number != pcie->root_bus_nr && !advk_pcie_link_up(pcie))
+   return false;
+
return true;
 }
 
-- 
2.20.1

Re: [PATCH] PCI: aardvark: Don't touch PCIe registers if no card connected

[Pali Rohár]

On Tuesday 30 June 2020 09:58:48 Bjorn Helgaas wrote:
> On Tue, Jun 30, 2020 at 04:04:20PM +0200, Pali Rohár wrote:
> > On Tuesday 30 June 2020 08:51:27 Bjorn Helgaas wrote:
> > > On Thu, May 28, 2020 at 06:38:09PM +0200, Pali Rohár wrote:
> > > > On Thursday 28 May 2020 11:26:04 Bjorn Helgaas wrote:
> > > > > On Thu, May 28, 2020 at 04:31:41PM +0200, Pali Rohár wrote:
> > > > > > When there is no PCIe card connected and advk_pcie_rd_conf() or
> > > > > > advk_pcie_wr_conf() is called for PCI bus which doesn't belong to 
> > > > > > emulated
> > > > > > root bridge, the aardvark driver throws the following error message:
> > > > > > 
> > > > > >   advk-pcie d007.pcie: config read/write timed out
> > > > > > 
> > > > > > Obviously accessing PCIe registers of disconnected card is not 
> > > > > > possible.
> > > > > > 
> > > > > > Extend check in advk_pcie_valid_device() function for validating
> > > > > > availability of PCIe bus. If PCIe link is down, then the device is 
> > > > > > marked
> > > > > > as Not Found and the driver does not try to access these registers.
> > > > > > 
> > > > > > Signed-off-by: Pali Rohár 
> > > > > > ---
> > > > > >  drivers/pci/controller/pci-aardvark.c | 3 +++
> > > > > >  1 file changed, 3 insertions(+)
> > > > > > 
> > > > > > diff --git a/drivers/pci/controller/pci-aardvark.c 
> > > > > > b/drivers/pci/controller/pci-aardvark.c
> > > > > > index 90ff291c24f0..53a4cfd7d377 100644
> > > > > > --- a/drivers/pci/controller/pci-aardvark.c
> > > > > > +++ b/drivers/pci/controller/pci-aardvark.c
> > > > > > @@ -644,6 +644,9 @@ static bool advk_pcie_valid_device(struct 
> > > > > > advk_pcie *pcie, struct pci_bus *bus,
> > > > > > if ((bus->number == pcie->root_bus_nr) && PCI_SLOT(devfn) != 0)
> > > > > > return false;
> > > > > >  
> > > > > > +   if (bus->number != pcie->root_bus_nr && 
> > > > > > !advk_pcie_link_up(pcie))
> > > > > > +   return false;
> > > > > 
> > > > > I don't think this is the right fix.  This makes it racy because the
> > > > > link may go down after we call advk_pcie_valid_device() but before we
> > > > > perform the config read.
> > > > 
> > > > Yes, it is racy, but I do not think it cause problems. Trying to read
> > > > PCIe registers when device is not connected cause just those timeouts,
> > > > printing error message and increased delay in advk_pcie_wait_pio() due
> > > > to polling loop. This patch reduce unnecessary access to PCIe registers
> > > > when advk_pcie_wait_pio() polling just fail.
> > > 
> > > What happens when the device is removed after advk_pcie_link_up()
> > > returns true, but before we actually do the config access?
> > 
> > Do you mean to remove device physically at runtime? I was told that our
> > board would crash or issue reset. Removing device from mini PCIe slot
> > without power off is not supported.
> 
> Right, I don't think PCIe mini cards support hotplug.
> 
> > Anyway, currently we are trying to read from device registers even when
> > no device is connected. So when advk_pcie_link_up() returns true and
> > after that device is not connected (somehow board and kernel would be
> > still alive) I guess that it would behave as without applying this
> > patch. So kernel starts reading from register and would wait until
> > timeout expires. As device is not connected there would be no answer,
> > so kernel print error message to dmesg (same as in commit message) and
> > returns error that read failed.
> 
> OK, so if I understand correctly, checking advk_pcie_link_up() is
> strictly an optimization.  If we guess wrong (e.g., after calling
> advk_pcie_link_up(), the link went down because the card was removed,
> DPC triggered, etc), the only bad thing is that we wait for a timeout;
> it never causes a crash.

Yes.

> If that's the case, I'm fine with this.  But please add a comment to
> that effect.

Ok, I will send V2 with updated commit message.

> I think several other drivers check for the link being up because we
> actually crash if we try to read config space when the link is down.
> That's what I was trying to avoid here.
> 
> Bjorn

Re: [PATCH] PCI: aardvark: Don't touch PCIe registers if no card connected

[Pali Rohár]

On Tuesday 30 June 2020 08:51:27 Bjorn Helgaas wrote:
> On Thu, May 28, 2020 at 06:38:09PM +0200, Pali Rohár wrote:
> > On Thursday 28 May 2020 11:26:04 Bjorn Helgaas wrote:
> > > On Thu, May 28, 2020 at 04:31:41PM +0200, Pali Rohár wrote:
> > > > When there is no PCIe card connected and advk_pcie_rd_conf() or
> > > > advk_pcie_wr_conf() is called for PCI bus which doesn't belong to 
> > > > emulated
> > > > root bridge, the aardvark driver throws the following error message:
> > > > 
> > > >   advk-pcie d007.pcie: config read/write timed out
> > > > 
> > > > Obviously accessing PCIe registers of disconnected card is not possible.
> > > > 
> > > > Extend check in advk_pcie_valid_device() function for validating
> > > > availability of PCIe bus. If PCIe link is down, then the device is 
> > > > marked
> > > > as Not Found and the driver does not try to access these registers.
> > > > 
> > > > Signed-off-by: Pali Rohár 
> > > > ---
> > > >  drivers/pci/controller/pci-aardvark.c | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > > 
> > > > diff --git a/drivers/pci/controller/pci-aardvark.c 
> > > > b/drivers/pci/controller/pci-aardvark.c
> > > > index 90ff291c24f0..53a4cfd7d377 100644
> > > > --- a/drivers/pci/controller/pci-aardvark.c
> > > > +++ b/drivers/pci/controller/pci-aardvark.c
> > > > @@ -644,6 +644,9 @@ static bool advk_pcie_valid_device(struct advk_pcie 
> > > > *pcie, struct pci_bus *bus,
> > > > if ((bus->number == pcie->root_bus_nr) && PCI_SLOT(devfn) != 0)
> > > > return false;
> > > >  
> > > > +   if (bus->number != pcie->root_bus_nr && 
> > > > !advk_pcie_link_up(pcie))
> > > > +   return false;
> > > 
> > > I don't think this is the right fix.  This makes it racy because the
> > > link may go down after we call advk_pcie_valid_device() but before we
> > > perform the config read.
> > 
> > Yes, it is racy, but I do not think it cause problems. Trying to read
> > PCIe registers when device is not connected cause just those timeouts,
> > printing error message and increased delay in advk_pcie_wait_pio() due
> > to polling loop. This patch reduce unnecessary access to PCIe registers
> > when advk_pcie_wait_pio() polling just fail.
> 
> What happens when the device is removed after advk_pcie_link_up()
> returns true, but before we actually do the config access?

Do you mean to remove device physically at runtime? I was told that our
board would crash or issue reset. Removing device from mini PCIe slot
without power off is not supported.

Anyway, currently we are trying to read from device registers even when
no device is connected. So when advk_pcie_link_up() returns true and
after that device is not connected (somehow board and kernel would be
still alive) I guess that it would behave as without applying this
patch. So kernel starts reading from register and would wait until
timeout expires. As device is not connected there would be no answer,
so kernel print error message to dmesg (same as in commit message) and
returns error that read failed.

Re: [PATCH] PCI: aardvark: Don't touch PCIe registers if no card connected

[Pali Rohár]

Hello!

On Friday 29 May 2020 10:30:13 Pali Rohár wrote:
> On Thursday 28 May 2020 11:49:38 Bjorn Helgaas wrote:
> > On Thu, May 28, 2020 at 06:38:09PM +0200, Pali Rohár wrote:
> > > On Thursday 28 May 2020 11:26:04 Bjorn Helgaas wrote:
> > > > On Thu, May 28, 2020 at 04:31:41PM +0200, Pali Rohár wrote:
> > > > > When there is no PCIe card connected and advk_pcie_rd_conf() or
> > > > > advk_pcie_wr_conf() is called for PCI bus which doesn't belong to 
> > > > > emulated
> > > > > root bridge, the aardvark driver throws the following error message:
> > > > > 
> > > > >   advk-pcie d007.pcie: config read/write timed out
> > > > > 
> > > > > Obviously accessing PCIe registers of disconnected card is not 
> > > > > possible.
> > > > > 
> > > > > Extend check in advk_pcie_valid_device() function for validating
> > > > > availability of PCIe bus. If PCIe link is down, then the device is 
> > > > > marked
> > > > > as Not Found and the driver does not try to access these registers.
> > > > > 
> > > > > Signed-off-by: Pali Rohár 
> > > > > ---
> > > > >  drivers/pci/controller/pci-aardvark.c | 3 +++
> > > > >  1 file changed, 3 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/pci/controller/pci-aardvark.c 
> > > > > b/drivers/pci/controller/pci-aardvark.c
> > > > > index 90ff291c24f0..53a4cfd7d377 100644
> > > > > --- a/drivers/pci/controller/pci-aardvark.c
> > > > > +++ b/drivers/pci/controller/pci-aardvark.c
> > > > > @@ -644,6 +644,9 @@ static bool advk_pcie_valid_device(struct 
> > > > > advk_pcie *pcie, struct pci_bus *bus,
> > > > >   if ((bus->number == pcie->root_bus_nr) && PCI_SLOT(devfn) != 0)
> > > > >   return false;
> > > > >  
> > > > > + if (bus->number != pcie->root_bus_nr && 
> > > > > !advk_pcie_link_up(pcie))
> > > > > + return false;
> > > > 
> > > > I don't think this is the right fix.  This makes it racy because the
> > > > link may go down after we call advk_pcie_valid_device() but before we
> > > > perform the config read.
> > > 
> > > Yes, it is racy, but I do not think it cause problems. Trying to read
> > > PCIe registers when device is not connected cause just those timeouts,
> > > printing error message and increased delay in advk_pcie_wait_pio() due
> > > to polling loop. This patch reduce unnecessary access to PCIe registers
> > > when advk_pcie_wait_pio() polling just fail.
> > > 
> > > I think it is a good idea to not call blocking advk_pcie_wait_pio() when
> > > it is not needed. We could have faster enumeration of PCIe buses when
> > > card is not connected.
> > 
> > Maybe advk_pcie_check_pio_status() and advk_pcie_wait_pio() could be
> > combined so we could get the correct error status as soon as it's
> > available, without waiting for a timeout?
> 
> Any idea how to achieve it?
> 
> First call is polling function advk_pcie_wait_pio() and second call is
> advk_pcie_check_pio_status() which just reads status register and prints
> error message to dmesg.
> 
> So for me it looks like that combining these two functions into one does
> not change anything. We always need to call polling code prior to
> checking status register. And therefore need to wait for timeout. Unless
> something like in this proposed patch is not used (to skip whole
> register access if it would fail).

So to answer your question, correct status is possible to retrieve only
after waiting for timeout. As status would be available only after
timeout expires.

Therefore my proposed patch in this (or some other) form is needed if we
want to prevent trying to read from registers and waiting for answer
when card is disconnected.

I would really like to see this issue fixed, so booting linux kernel on
board without connected PCIe card would not be delayed.

Thomas, Lorenzo, Bjorn: do you have any idea how to fix it differently?
Or if not, could be my proposed patch accepted in some form?

> > In any event, the "return PCIBIOS_SET_FAILED" needs to be fixed.  Most
> > callers of config read do not check for failure, but most of the ones
> > that do, check for "val == ~0".  Only a few check for a status of
> > other than PCIBIOS_SUCCESSFUL.
> > 
> > > > I have no objection to removing the "config read/write timed out"
> > > > message.  The "return PCIBIOS_SET_FAILED" in the read case probably
> > > > should be augmented by setting "*val = 0x".
> 
> Now I see, "*val = 0x" should be really set when function
> advk_pcie_rd_conf() fails.

I have already sent separate patch which fixes this issue.

> > > > >   return true;
> > > > >  }
> > > > >  
> > > > > -- 
> > > > > 2.20.1
> > > > > 

[PATCH] mmc: sdio: Move SDIO IDs from rsi_sdio driver to common include file

[Pali Rohár]

Define appropriate macro names for consistency with other macros.

Signed-off-by: Pali Rohár 
---
Hello Ulf! I forgot to include change for rsi driver into patch series:
"mmc: sdio: Move SDIO IDs from drivers to common include file"
https://lore.kernel.org/linux-mmc/20200522144412.19712-1-p...@kernel.org/
As patch series was already merged, I'm sending this patch separately.
---
 drivers/net/wireless/rsi/rsi_91x_sdio.c | 8 
 drivers/net/wireless/rsi/rsi_sdio.h | 4 
 include/linux/mmc/sdio_ids.h| 4 
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/net/wireless/rsi/rsi_91x_sdio.c 
b/drivers/net/wireless/rsi/rsi_91x_sdio.c
index 5d6143a55187..a04ff75c409f 100644
--- a/drivers/net/wireless/rsi/rsi_91x_sdio.c
+++ b/drivers/net/wireless/rsi/rsi_91x_sdio.c
@@ -1038,10 +1038,10 @@ static int rsi_probe(struct sdio_func *pfunction,
goto fail_free_adapter;
}
 
-   if (pfunction->device == RSI_SDIO_PID_9113) {
+   if (pfunction->device == SDIO_DEVICE_ID_RSI_9113) {
rsi_dbg(ERR_ZONE, "%s: 9113 module detected\n", __func__);
adapter->device_model = RSI_DEV_9113;
-   } else  if (pfunction->device == RSI_SDIO_PID_9116) {
+   } else  if (pfunction->device == SDIO_DEVICE_ID_RSI_9116) {
rsi_dbg(ERR_ZONE, "%s: 9116 module detected\n", __func__);
adapter->device_model = RSI_DEV_9116;
} else {
@@ -1526,8 +1526,8 @@ static const struct dev_pm_ops rsi_pm_ops = {
 #endif
 
 static const struct sdio_device_id rsi_dev_table[] =  {
-   { SDIO_DEVICE(RSI_SDIO_VENDOR_ID, RSI_SDIO_PID_9113) },
-   { SDIO_DEVICE(RSI_SDIO_VENDOR_ID, RSI_SDIO_PID_9116) },
+   { SDIO_DEVICE(SDIO_VENDOR_ID_RSI, SDIO_DEVICE_ID_RSI_9113) },
+   { SDIO_DEVICE(SDIO_VENDOR_ID_RSI, SDIO_DEVICE_ID_RSI_9116) },
{ /* Blank */},
 };
 
diff --git a/drivers/net/wireless/rsi/rsi_sdio.h 
b/drivers/net/wireless/rsi/rsi_sdio.h
index c5cfb6238f73..9afc1d0d2684 100644
--- a/drivers/net/wireless/rsi/rsi_sdio.h
+++ b/drivers/net/wireless/rsi/rsi_sdio.h
@@ -28,10 +28,6 @@
 #include 
 #include "rsi_main.h"
 
-#define RSI_SDIO_VENDOR_ID   0x041B
-#define RSI_SDIO_PID_91130x9330
-#define RSI_SDIO_PID_91160x9116
-
 enum sdio_interrupt_type {
BUFFER_FULL = 0x0,
BUFFER_AVAILABLE= 0x2,
diff --git a/include/linux/mmc/sdio_ids.h b/include/linux/mmc/sdio_ids.h
index 15ed8ce9d394..ab41801c5f51 100644
--- a/include/linux/mmc/sdio_ids.h
+++ b/include/linux/mmc/sdio_ids.h
@@ -118,6 +118,10 @@
 #define SDIO_DEVICE_ID_SIANO_NOVA_A0   0x1100
 #define SDIO_DEVICE_ID_SIANO_STELLAR   0x5347
 
+#define SDIO_VENDOR_ID_RSI 0x041b
+#define SDIO_DEVICE_ID_RSI_91130x9330
+#define SDIO_DEVICE_ID_RSI_91160x9116
+
 #define SDIO_VENDOR_ID_TI_WL1251   0x104c
 #define SDIO_DEVICE_ID_TI_WL1251   0x9066
 
-- 
2.20.1

[PATCH] mwifiex: Use macro MWIFIEX_MAX_BSS_NUM for specifying limit of interfaces

[Pali Rohár]

This macro is already used in mwifiex driver for specifying upper limit and
is defined to value 3. So use it also in struct ieee80211_iface_limit.

Signed-off-by: Pali Rohár 
---
 drivers/net/wireless/marvell/mwifiex/cfg80211.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c 
b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
index 4e4f59c17ded..867b5cf385a8 100644
--- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
+++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
@@ -27,7 +27,8 @@ module_param(reg_alpha2, charp, 0);
 
 static const struct ieee80211_iface_limit mwifiex_ap_sta_limits[] = {
{
-   .max = 3, .types = BIT(NL80211_IFTYPE_STATION) |
+   .max = MWIFIEX_MAX_BSS_NUM,
+   .types = BIT(NL80211_IFTYPE_STATION) |
   BIT(NL80211_IFTYPE_P2P_GO) |
   BIT(NL80211_IFTYPE_P2P_CLIENT) |
   BIT(NL80211_IFTYPE_AP),
-- 
2.20.1

Re: [PATCH 4.19 023/206] PCI: aardvark: Dont blindly enable ASPM L0s and dont write to read-only register

[Pali Rohár]

Hello!

On Friday 26 June 2020 14:53:50 Pavel Machek wrote:
> Hi!
> 
> > [ Upstream commit 90c6cb4a355e7befcb557d217d1d8b8bd5875a05 ]
> > 
> > Trying to change Link Status register does not have any effect as this
> > is a read-only register. Trying to overwrite bits for Negotiated Link
> > Width does not make sense.
> 
> I don't quite get it. This says register is read only...
> 
> > In future proper change of link width can be done via Lane Count Select
> > bits in PCIe Control 0 register.
> > 
> > Trying to unconditionally enable ASPM L0s via ASPM Control bits in Link
> > Control register is wrong. There should be at least some detection if
> > endpoint supports L0s as isn't mandatory.
> 
> and this says it is wrong to set the bits as ASPM L0 is not
> mandatory.

Negotiated Link Width is in read-only 16bit Link Status register.

ASPM Control bits are in read-write 16bit Link Control register.

> > +++ b/drivers/pci/controller/pci-aardvark.c
> > @@ -321,10 +321,6 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
> >  
> > advk_pcie_wait_for_link(pcie);
> >  
> > -   reg = PCIE_CORE_LINK_L0S_ENTRY |
> > -   (1 << PCIE_CORE_LINK_WIDTH_SHIFT);
> > -   advk_writel(pcie, reg, PCIE_CORE_LINK_CTRL_STAT_REG);
> > -
> > reg = advk_readl(pcie, PCIE_CORE_CMD_STATUS_REG);
> > reg |= PCIE_CORE_CMD_MEM_ACCESS_EN |
> > PCIE_CORE_CMD_IO_ACCESS_EN |
> 
> ...but we only do single write.
> 
> So which is it?

That single write was via 32bit memory access which tried to overwrite
both registers (Link Status and Link Control) at the same time.

> Best regards,
>   Pavel
> -- 
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) 
> http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: drivers/pci/controller/pci-aardvark.c:350:2: error: implicit declaration of function 'gpiod_set_value_cansleep'

[Pali Rohár]

On Sunday 21 June 2020 11:05:22 kernel test robot wrote:
> >> drivers/pci/controller/pci-aardvark.c:350:2: error: implicit declaration 
> >> of function 'gpiod_set_value_cansleep' 
> >> [-Werror,-Wimplicit-function-declaration]
>gpiod_set_value_cansleep(pcie->reset_gpio, 1);
>^
>drivers/pci/controller/pci-aardvark.c:350:2: note: did you mean 
> 'gpio_set_value_cansleep'?
>include/linux/gpio.h:188:20: note: 'gpio_set_value_cansleep' declared here
>static inline void gpio_set_value_cansleep(unsigned gpio, int value)
>   ^
> >> drivers/pci/controller/pci-aardvark.c:1074:21: error: implicit declaration 
> >> of function 'devm_gpiod_get_from_of_node' 
> >> [-Werror,-Wimplicit-function-declaration]
>pcie->reset_gpio = devm_gpiod_get_from_of_node(dev, dev->of_node,
>   ^
> >> drivers/pci/controller/pci-aardvark.c:1076:14: error: use of undeclared 
> >> identifier 'GPIOD_OUT_LOW'
>   GPIOD_OUT_LOW,
>   ^
>20 warnings and 3 errors generated.
> 
> vim +/gpiod_set_value_cansleep +350 drivers/pci/controller/pci-aardvark.c
> 
>335
>336static void advk_pcie_issue_perst(struct advk_pcie *pcie)
>337{
>338u32 reg;
>339
>340if (!pcie->reset_gpio)
>341return;
>342
>343/* PERST does not work for some cards when link 
> training is enabled */
>344reg = advk_readl(pcie, PCIE_CORE_CTRL0_REG);
>345reg &= ~LINK_TRAINING_EN;
>346advk_writel(pcie, reg, PCIE_CORE_CTRL0_REG);
>347
>348/* 10ms delay is needed for some cards */
>349dev_info(&pcie->pdev->dev, "issuing PERST via reset 
> GPIO for 10ms\n");
>  > 350gpiod_set_value_cansleep(pcie->reset_gpio, 1);
>351usleep_range(1, 11000);
>352gpiod_set_value_cansleep(pcie->reset_gpio, 0);
>353}
>354

Hello!

Could you try to test following patch if it fixes above problems?

diff --git a/drivers/pci/controller/pci-aardvark.c 
b/drivers/pci/controller/pci-aardvark.c
index 90ff291c24f0..01f1fcbb8770 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -10,6 +10,7 @@
 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 

Both GPIOD_OUT_LOW and gpiod_set_value_cansleep() are defined in
linux/gpio/consumer.h header file.

I do not have s390 box nor s390 cross compiler to do verification.

Re: [RFC] power: supply: bq27xxx_battery: Fix polling interval after re-bind

[Pali Rohár]

On Friday 19 June 2020 19:55:21 Sebastian Reichel wrote:
> Hi,
> 
> On Wed, May 27, 2020 at 09:42:54AM +0200, Pali Rohár wrote:
> > On Tuesday 26 May 2020 21:16:28 Andrew F. Davis wrote:
> > > On 5/25/20 7:32 AM, Krzysztof Kozlowski wrote:
> > > > This reverts commit 8cfaaa811894a3ae2d7360a15a6cfccff3ebc7db.
> > > > 
> > > > If device was unbound and bound, the polling interval would be set to 0.
> > > > This is both unexpected and messes up with other bq27xxx devices (if
> > > > more than one battery device is used).
> > > > 
> > > > This reset of polling interval was added in commit 8cfaaa811894
> > > > ("bq27x00_battery: Fix OOPS caused by unregistring bq27x00 driver")
> > > > stating that power_supply_unregister() calls get_property().  However in
> > > > Linux kernel v3.1 and newer, such call trace does not exist.
> > > > Unregistering power supply does not call get_property() on unregistered
> > > > power supply.
> > > > 
> > > > Fixes: 8cfaaa811894 ("bq27x00_battery: Fix OOPS caused by unregistring 
> > > > bq27x00 driver")
> > > > Cc: 
> > > > Signed-off-by: Krzysztof Kozlowski 
> > > > 
> > > > ---
> > > > 
> > > > I really could not identify the issue being fixed in offending commit
> > > > 8cfaaa811894 ("bq27x00_battery: Fix OOPS caused by unregistring bq27x00
> > > > driver"), therefore maybe I missed here something important.
> > > > 
> > > > Please share your thoughts on this.
> > > 
> > > I'm having a hard time finding the OOPS also. Maybe there is a window
> > > where the poll function is running or about to run where
> > > cancel_delayed_work_sync() is called and cancels the work, only to have
> > > an interrupt or late get_property call in to the poll function and
> > > re-schedule it.
> > > 
> > > What we really need is to do is look at how we are handling the polling
> > > function. It gets called from the workqueue, from a threaded interrupt
> > > context, and from a power supply framework callback, possibly all at the
> > > same time. Sometimes its protected by a lock, sometimes not. Updating
> > > the device's cached data should always be locked.
> > > 
> > > What's more is the poll function is self-arming, so if we call
> > > cancel_delayed_work_sync() (remove it from the work queue then then wait
> > > for it to finish if running), are we sure it wont have just re-arm itself?
> > > 
> > > We should make the only way we call the poll function be through the
> > > work queue, (plus make sure all accesses to the cache are locked).
> > > 
> > > Andrew
> > 
> > I do not remember details too. It is long time ago.
> > 
> > CCing Ivaylo Dimitrov as he may remember something...

Hello!

I did some archeology and found some more details about this problem.

rmmoding bq module without that patch at that time sometimes caused oops
and reboot of N900 device. backtrace of crash contained:

===
[80084.031646] Backtrace:
[80084.031677] [] (kobject_uevent_env+0x0/0x3a0) from [] 
(kobject_uevent+0x14/0x18)
[80084.031768] [] (kobject_uevent+0x0/0x18) from [] 
(power_supply_changed_work+0x44/0x50 [power_supply])
[80084.031890] [] (power_supply_changed_work+0x0/0x50 [power_supply]) 
from [] (run_workqueue+0xd4/0x198)
[80084.031982]  r5:cf028000 r4:c40859a8
[80084.032012] [] (run_workqueue+0x0/0x198) from [] 
(worker_thread+0xf0/0x104)
[80084.032104]  r9: r8: r7:cf000780 r6:cf028000 r5:cf01ab40
[80084.032165] r4:cf029fb8
[80084.032196] [] (worker_thread+0x0/0x104) from [] 
(kthread+0x54/0x80)
[80084.032287]  r7: r6: r5:c006a714 r4:cf000780
[80084.032318] [] (kthread+0x0/0x80) from [] 
(do_exit+0x0/0x7bc)
[80084.032409]  r5: r4:
[80084.032440] Code: e353 1af9 e3e05015 eac6 (e59a802c)
===

I dig more in my disk storage and private archives and found following
email from Ivaylo which describe that problem and is also source of that
patch. I hope that Ivo would not be against putting copy of it here :-)

===
Date: Mon, 31 Oct 2011 16:00:33 +0200 (EET)
Message-ID: <1251363478.202876.1320069633552.javamail.apa...@mail82.abv.bg>

Hi,

The bug in bq27x00_battery is in function bq27x00_battery_poll. What happens is 
that after all pending poll work requests are canceled with

Re: [PATCH] hwmon: (dell-smm) Add Latitude 5480 to fan control whitelist

[Pali Rohár]

On Thursday 18 June 2020 21:55:29 Jeffrey Lin wrote:
> This allows manual PWM control without the BIOS fighting back on Dell
> Latitude 5480.
> 
> Signed-off-by: Jeffrey Lin 

If it is working fine on your machine, you can add my:

Acked-by: Pali Rohár 

> ---
>  drivers/hwmon/dell-smm-hwmon.c | 8 
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> index 16be012a95ed..ec448f5f2dc3 100644
> --- a/drivers/hwmon/dell-smm-hwmon.c
> +++ b/drivers/hwmon/dell-smm-hwmon.c
> @@ -1187,6 +1187,14 @@ static struct dmi_system_id 
> i8k_whitelist_fan_control[] __initdata = {
>   },
>   .driver_data = (void *)&i8k_fan_control_data[I8K_FAN_34A3_35A3],
>   },
> + {
> + .ident = "Dell Latitude 5480",
> + .matches = {
> + DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> + DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Latitude 5480"),
> + },
> + .driver_data = (void *)&i8k_fan_control_data[I8K_FAN_34A3_35A3],
> + },
>   {
>   .ident = "Dell Latitude E6440",
>   .matches = {
> -- 
> 2.27.0
> 

Re: [PATCH] PCI: aardvark: Indicate error in 'val' when config read fails

[Pali Rohár]

Hello Lorenzo! Could you please review this patch?

On Monday 01 June 2020 15:03:15 Pali Rohár wrote:
> Most callers of config read do not check for return value. But most of the
> ones that do, checks for error indication in 'val' variable.
> 
> This patch updates error handling in advk_pcie_rd_conf() function. If PIO
> transfer fails then 'val' variable is set to 0x which indicates
> failture.
> 
> Signed-off-by: Pali Rohár 

I should add credit for Bjorn as he found this issue

Reported-by: Bjorn Helgaas 

> ---
>  drivers/pci/controller/pci-aardvark.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/pci-aardvark.c 
> b/drivers/pci/controller/pci-aardvark.c
> index 53a4cfd7d377..783a7f1f2c44 100644
> --- a/drivers/pci/controller/pci-aardvark.c
> +++ b/drivers/pci/controller/pci-aardvark.c
> @@ -691,8 +691,10 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 
> devfn,
>   advk_writel(pcie, 1, PIO_START);
>  
>   ret = advk_pcie_wait_pio(pcie);
> - if (ret < 0)
> + if (ret < 0) {
> + *val = 0x;
>   return PCIBIOS_SET_FAILED;
> + }
>  
>   advk_pcie_check_pio_status(pcie);
>  
> -- 
> 2.20.1
> 

Re: [EXT] mwifiex: Firmware name for W8997 sdio wifi chip

[Pali Rohár]

On Friday 29 May 2020 11:12:11 Pali Rohár wrote:
> On Friday 29 May 2020 09:11:08 Ganapathi Bhat wrote:
> > Hi Pali,
> >  
> > > Hello Ganapathi! Seems that on both locations is older version of
> > > sdsd8997_combo_v4.bin firmware, not the latest one. On both location is
> > > available just version 16.68.1.p179. But we have newer version 
> > > 16.68.1.p197.
> > > Could you please recheck it?
> > 
> > p179 do have the fix but we will try to upstream p197 also soon.
> 
> Thank you!

Hello Ganapathi! Do you have any updates about upstreaming new firmware version?

Re: [PATCH v2 3/3] platform/x86: dell-wmi: add new dmi keys to bios_to_linux_keycode

[Pali Rohár]

On Wednesday 10 June 2020 12:35:09 mario.limoncie...@dell.com wrote:
> > -Original Message-
> > From: platform-driver-x86-ow...@vger.kernel.org  > ow...@vger.kernel.org> On Behalf Of Pali Rohár
> > Sent: Wednesday, June 10, 2020 4:45 AM
> > To: Limonciello, Mario
> > Cc: rdun...@infradead.org; y.li...@paritcher.com; linux-
> > ker...@vger.kernel.org; platform-driver-...@vger.kernel.org;
> > mj...@srcf.ucam.org
> > Subject: Re: [PATCH v2 3/3] platform/x86: dell-wmi: add new dmi keys to
> > bios_to_linux_keycode
> > 
> > 
> > [EXTERNAL EMAIL]
> > 
> > On Tuesday 09 June 2020 19:49:18 mario.limoncie...@dell.com wrote:
> > > >
> > > > Looking at the last two lines... and for me it looks like that 0x00FF
> > > > and 0x are just "placeholders" or special values for unknown /
> > > > custom / unsupported / reserved / special / ... codes.
> > > >
> > > > It is really suspicious why first 38 values are defined, then there is
> > > > gap, then one value 255 and then huge gap to 65535.
> > > >
> > > > Mario, this looks like some mapping table between internal Dell BIOS
> > key
> > > > code and standard Linux key code. Are you able to get access to some
> > > > documentation which contains explanation of those Dell key numbers?
> > > > It could really help us to understand these gaps and what is correct
> > > > interpretation of these numbers.
> > > >
> > >
> > > The codes are actually 4 bytes in the table, but in practice nothing
> > above the
> > > first two bytes is used.
> > >
> > > Those two called out are special though, here are their meanings:
> > >
> > > 0x00FF is user programmable function
> > > 0x is no function
> > >
> > > For the purpose of memory consumption I think it's reasonable to ignore
> > the
> > > upper 2 bytes and special case these two.
> > 
> > Thank you for information!
> > 
> > So 0x00FF is "user programmable" button. Do I understand it correctly
> > that Dell/BIOS does not explicitly provide meaning for these buttons,
> > they do not have fixed functionality and therefore user should configure
> > them as he want?
> 
> Correct
> 
> > 
> > And what does mean "no function"? I do not know what should I imagine if
> > I receive key press marked as "no function".
> 
> It means no action is expected to occur, should behave like a no-op.  I think
> discarding it makes fine sense.

Thank you! This was missing bit of information.

Just I'm curious, why firmware sends "no-op" event which we could ignore? :D

I can imagine that those events / scan codes may contain some
information which we can use...

> > 
> > > > E.g. I remember that pressing Fn+Q or Fn+W on some Dell Latitude
> > > > generates code 255, which could prove my thesis about "special codes"
> > > > (which are probably not found in e.g. Windows or Linux mapping tables).
> > > >
> > > > > >  };
> > > > > >
> > > > > >  /*
> > > > > > @@ -503,10 +504,7 @@ static void handle_dmi_entry(const struct
> > > > dmi_header *dm, void *opaque)
> > > > > > &table->keymap[i];
> > > > > >
> > > > > > /* Uninitialized entries are 0 aka KEY_RESERVED. */
> > > > > > -   u16 keycode = (bios_entry->keycode <
> > > > > > -  ARRAY_SIZE(bios_to_linux_keycode)) ?
> > > > > > -   bios_to_linux_keycode[bios_entry->keycode] :
> > > > > > -   KEY_RESERVED;
> > > > > > +   u16 keycode = 
> > > > > > bios_to_linux_keycode[bios_entry->keycode];
> > > > > >
> > > > > > /*
> > > > > >  * Log if we find an entry in the DMI table that we 
> > > > > > don't
> > > > > >
> > > > >
> > > > > Something like:
> > > > >
> > > > >   u16 keycode;
> > > > >
> > > > >   keycode = bios_entry->keycode == 0x ? KEY_UNKNOWN :
> > > > >   (bios_entry->keycode <
> > > > >  ARRAY_SIZE(bios_to_linux_keycode)) ?
> > > > >   bios_to_linux_keycode[bios_entry->keycode] :
> > > > >   KEY_RESERVED;
> > > > >
> > > > >
> > > > >
> > > > > Also please fix this:
> > > > > (no To-header on input) <>
> > > >
> > > > Hint: specifying git send-email with '--to' argument instead of '--cc'
> > > > should help.
> > > >
> > > > >
> > > > > --
> > > > > ~Randy
> > > > >

Re: [PATCH v4 0/3] platform/x86: dell-wmi: new keys

[Pali Rohár]

On Wednesday 10 June 2020 13:56:55 Y Paritcher wrote:
> change since v3:
> No code changes.
> Update commit message to reflect info given by Mario at dell.
> 
> Is there anything more i have to do for the patches that were reviewed
> or will they be picked up by the maintainers?
> Thanks
> 
> Y Paritcher (3):
>   platform/x86: dell-wmi: add new backlight events
>   platform/x86: dell-wmi: add new keymap type 0x0012
>   platform/x86: dell-wmi: add new dmi mapping for keycode 0x
> 
>  drivers/platform/x86/dell-wmi.c | 28 +---
>  1 file changed, 25 insertions(+), 3 deletions(-)
> 
> -- 
> 2.27.0
> 

I'm fine with this patch series too.

Reviewed-by: Pali Rohár 

Re: [PATCH v2 3/3] platform/x86: dell-wmi: add new dmi keys to bios_to_linux_keycode

[Pali Rohár]

On Tuesday 09 June 2020 19:49:18 mario.limoncie...@dell.com wrote:
> > 
> > Looking at the last two lines... and for me it looks like that 0x00FF
> > and 0x are just "placeholders" or special values for unknown /
> > custom / unsupported / reserved / special / ... codes.
> > 
> > It is really suspicious why first 38 values are defined, then there is
> > gap, then one value 255 and then huge gap to 65535.
> > 
> > Mario, this looks like some mapping table between internal Dell BIOS key
> > code and standard Linux key code. Are you able to get access to some
> > documentation which contains explanation of those Dell key numbers?
> > It could really help us to understand these gaps and what is correct
> > interpretation of these numbers.
> > 
> 
> The codes are actually 4 bytes in the table, but in practice nothing above the
> first two bytes is used.
> 
> Those two called out are special though, here are their meanings:
> 
> 0x00FF is user programmable function
> 0x is no function
> 
> For the purpose of memory consumption I think it's reasonable to ignore the
> upper 2 bytes and special case these two.

Thank you for information!

So 0x00FF is "user programmable" button. Do I understand it correctly
that Dell/BIOS does not explicitly provide meaning for these buttons,
they do not have fixed functionality and therefore user should configure
them as he want?

And what does mean "no function"? I do not know what should I imagine if
I receive key press marked as "no function".

> > E.g. I remember that pressing Fn+Q or Fn+W on some Dell Latitude
> > generates code 255, which could prove my thesis about "special codes"
> > (which are probably not found in e.g. Windows or Linux mapping tables).
> > 
> > > >  };
> > > >
> > > >  /*
> > > > @@ -503,10 +504,7 @@ static void handle_dmi_entry(const struct
> > dmi_header *dm, void *opaque)
> > > > &table->keymap[i];
> > > >
> > > > /* Uninitialized entries are 0 aka KEY_RESERVED. */
> > > > -   u16 keycode = (bios_entry->keycode <
> > > > -  ARRAY_SIZE(bios_to_linux_keycode)) ?
> > > > -   bios_to_linux_keycode[bios_entry->keycode] :
> > > > -   KEY_RESERVED;
> > > > +   u16 keycode = 
> > > > bios_to_linux_keycode[bios_entry->keycode];
> > > >
> > > > /*
> > > >  * Log if we find an entry in the DMI table that we 
> > > > don't
> > > >
> > >
> > > Something like:
> > >
> > >   u16 keycode;
> > >
> > >   keycode = bios_entry->keycode == 0x ? KEY_UNKNOWN :
> > >   (bios_entry->keycode <
> > >  ARRAY_SIZE(bios_to_linux_keycode)) ?
> > >   bios_to_linux_keycode[bios_entry->keycode] :
> > >   KEY_RESERVED;
> > >
> > >
> > >
> > > Also please fix this:
> > > (no To-header on input) <>
> > 
> > Hint: specifying git send-email with '--to' argument instead of '--cc'
> > should help.
> > 
> > >
> > > --
> > > ~Randy
> > >

Re: [PATCH 2/3] platform/x86: dell-wmi: add new keymap type 0x0012

[Pali Rohár]

On Monday 08 June 2020 20:36:58 mario.limoncie...@dell.com wrote:
> Can you please comment here how you would like to see events like this should 
> come
> through to userspace?
> 
> * Wrong power adapter (you have X and should have Y)
> * You have plugged a dock into the wrong port
> * Fn-lock mode

In my opinion, Fn-lock mode is related to input subsystem and should be
probably reported via input device. For a user, fn-lock is similar like
caps-lock, scroll-lock or num-lock. Also fn-lock is provided by other
laptops and therefore I would expect that kernel provide fn-lock state
for all laptops (thinkpad / latitude / ...) via same interface. And not
via dell-specific interface or general-vendor-message interface.

Wrong power adapter sounds like something related to power subsystem.
Adding Sebastian to the loop, maybe he can provide some useful ideas
about it.

And plugged dock into wrong port. This is probably dell-specific event
and some interface for "vendor" messages from kernel to userspace would
be needed to deliver such things.

Re: [PATCH v3 3/3] platform/x86: dell-wmi: add new dmi mapping for keycode 0xffff

[Pali Rohár]

Adding Darren and Andy, please look at this issue as this is something
which should be decided how to handle in all platform/wmi drivers.

On Monday 08 June 2020 23:52:54 Y Paritcher wrote:
> This looks to be a special value for some sort of custom scancode.
> This code could not be triggered for any keypress and is included
> from the 0xB2 DMI table.
> 
> This prevents the following messages from being logged at startup on a
> Dell Inspiron 5593:
> 
> dell_wmi: firmware scancode 0x48 maps to unrecognized keycode 0x
> dell_wmi: firmware scancode 0x50 maps to unrecognized keycode 0x
> 
> as per this code comment:
> 
>Log if we find an entry in the DMI table that we don't
>understand.  If this happens, we should figure out what
>the entry means and add it to bios_to_linux_keycode.
> 
> Signed-off-by: Y Paritcher 
> ---
>  drivers/platform/x86/dell-wmi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
> index e3bc2601e631..bbdb3e860892 100644
> --- a/drivers/platform/x86/dell-wmi.c
> +++ b/drivers/platform/x86/dell-wmi.c
> @@ -506,7 +506,7 @@ static void handle_dmi_entry(const struct dmi_header *dm, 
> void *opaque)
>   u16 keycode = (bios_entry->keycode <
>  ARRAY_SIZE(bios_to_linux_keycode)) ?
>   bios_to_linux_keycode[bios_entry->keycode] :
> - KEY_RESERVED;
> + (bios_entry->keycode == 0x ? KEY_UNKNOWN : 
> KEY_RESERVED);

I really do not like this patch in this form as it is inconsistent with
how are these key codes processed. And also because it does not solve
any problem. It just hides existence of the problem. And this is
something which should be avoided as it makes debugging harder and
basically does not bring any value.

To explain it a bit more. We have mapping table bios_to_linux_keycode[]
from Dell BIOS key codes to Linux key codes. This mapping table is used
for processing Dell WMI buffer of type 0x0010.

You are not able to trigger these events and therefore you and nobody
else in this discussion does not know yet what real key triggers this
Dell BIOS key code.

So it means that you are proposing to add "bogus" value into
bios_to_linux_keycode[] table (just implicitly via check to reduce
memory usage, but it does not matter) just because to hide existence of
the problem.

This bogus value "KEY_UNKNOWN" is there just as a placeholder because
nobody know correct value and nobody was able to trigger it.

Also how it differs situation with unknown keycode "39" which is also
missing in the table from yours with keycode "65535" which is also
missing in the table?

I think there is no difference, for both values we do not know what is
correct mapping to Linux keycodes and therefore value is not present in
bios_to_linux_keycode[] table.

If I accept to take this bogus value for keycode "65535", tomorrow can
somebody send me a patch which adds bogus value "1000" for the same
reason as you as it makes precedense. But it also does not solve any
problem.

We would just end up with mess and garbage in bios_to_linux_keycode[]
mapping table together with some correct values in table.

So we should ask here two main questions:

1) Is dell-wmi able to process key presses (or events) with scancode
0x48 and keycode 0x?

2) If we are not able to process them, what should we do?

Answer for first question is definitely "no".

And answer for second question, I'm not sure, but I thought that
throwing warning or info message is the correct solution. Driver cannot
handle something, so it inform about it, instead of silently dropping
event. Same behavior I'm seeing in other kernel drivers.

But looks like that you and Mario have opposite opinion, that kernel
should not log unknown events and rather should drop them.

I would like to have behavior of dell-wmi same as in other drivers for
consistency, so the best would be to ask WMI/platform maintainers. They
could have opinion how to handle these problem globally.

...

Darren & Andy, could you please say something to this, what do you think
about silently dropping events/actions which are currently unknown for
dell-wmi driver? It is better to log them or not? Currently we are
logging them.

>  
>   /*
>* Log if we find an entry in the DMI table that we don't
> -- 
> 2.27.0
> 

Re: [PATCH v2 2/3] platform/x86: dell-wmi: add new keymap type 0x0012

[Pali Rohár]

On Tuesday 09 June 2020 00:26:45 mario.limoncie...@dell.com wrote:
> > Mario, are you able to get some official documentation for these 0x0012
> > event types? I think it could be really useful for community so they can
> > understand and add easily new type of code and events. Because currently
> > we are just guessing what it could be. (It is sequence? Or single event?
> > Or single event with extended data? It is generic event? Or it is real
> > keypress? etc...)
> 
> It's a single event with more data in the subsequent words.  It is definitely
> not a real keypress.  It's supposed to be data that a user application would 
> show.
> 
> Remember the way WMI works on Linux and Windows is different.  On Windows
> userland applications get the events directly.  On Linux kernel drivers get 
> the
> events and either use it internally, pass to another kernel driver or pass to
> userland in the form of a translated event.
> 
> So on Windows the whole buffer gets looked at directly by the application and 
> the
> application will decode it to show a translated string.
> 
> I can certainly discuss internally about our team releasing a patch to export
> all these other events.  I would like to know what interface to recommend it 
> pass
> to userspace though, because as I said this is more than just a keycode that
> comes through in the event.  It's not useful to just do dev_info, it really 
> should
> be something that userspace can act on and show a translated message.
> I don't think we want to add another 15 Dell specific keycodes to the kernel 
> for the
> various events and add another 4 more when a laptop introduces another set of 
> keys.

Which interface to use for events? That is a good question and probably
this should be bring to the linux-input mailinglist. I think that
linux-input maintainers could have idea how to do it properly. We need
some interface which would be general enough and usable also by other
drivers / components and I'm sure that ACPI/WMI is not the only
subsystem which needs to send events from kernel to userspace.

Re: [PATCH v2 2/3] platform/x86: dell-wmi: add new keymap type 0x0012

[Pali Rohár]

On Monday 08 June 2020 20:57:38 Y Paritcher wrote:
> On 6/8/20 8:26 PM, mario.limoncie...@dell.com wrote:
> >> -Original Message-
> >> From: Pali Rohár 
> >> Sent: Monday, June 8, 2020 6:33 PM
> >> To: Y Paritcher
> >> Cc: linux-kernel@vger.kernel.org; platform-driver-...@vger.kernel.org;
> >> Matthew Garrett; Limonciello, Mario
> >> Subject: Re: [PATCH v2 2/3] platform/x86: dell-wmi: add new keymap type
> >> 0x0012
> >>
> >>
> >> [EXTERNAL EMAIL]
> >>
> >> On Monday 08 June 2020 19:05:29 Y Paritcher wrote:
> >>> These are events with extended data. The extended data is
> >>> currently ignored as userspace does not have a way to deal
> >>> it.
> >>>
> >>> Ignore event with a type of 0x0012 and a code of 0xe035, as
> >>> the keyboard controller takes care of Fn lock events by itself.
> >>
> >> Nice! This is information which is really important and need to have it
> >> documented.
> >>
> >>> This silences the following messages being logged when
> >>> pressing the Fn-lock key on a Dell Inspiron 5593:
> >>>
> >>> dell_wmi: Unknown WMI event type 0x12
> >>> dell_wmi: Unknown key with type 0x0012 and code 0xe035 pressed
> >>>
> >>> This is consistent with the behavior for the Fn-lock key
> >>> elsewhere in this file.
> >>>
> >>> Signed-off-by: Y Paritcher 
> >>
> >> I'm fine with this patch now.
> >>
> >> Reviewed-by: Pali Rohár 
> >>
> >>> ---
> >>>  drivers/platform/x86/dell-wmi.c | 20 +++-
> >>>  1 file changed, 19 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-
> >> wmi.c
> >>> index 0b2edfe2767d..6b510f8431a3 100644
> >>> --- a/drivers/platform/x86/dell-wmi.c
> >>> +++ b/drivers/platform/x86/dell-wmi.c
> >>> @@ -334,6 +334,15 @@ static const struct key_entry
> >> dell_wmi_keymap_type_0011[] = {
> >>>   { KE_IGNORE, KBD_LED_AUTO_100_TOKEN, { KEY_RESERVED } },
> >>>  };
> >>>
> >>> +/*
> >>> + * Keymap for WMI events of type 0x0012
> >>> + * They are events with extended data
> >>> + */
> >>> +static const struct key_entry dell_wmi_keymap_type_0012[] = {
> >>> + /* Fn-lock button pressed */
> >>> + { KE_IGNORE, 0xe035, { KEY_RESERVED } },
> >>> +};
> >>> +
> >>>  static void dell_wmi_process_key(struct wmi_device *wdev, int type, int
> >> code)
> >>>  {
> >>>   struct dell_wmi_priv *priv = dev_get_drvdata(&wdev->dev);
> >>> @@ -418,10 +427,11 @@ static void dell_wmi_notify(struct wmi_device
> >> *wdev,
> >>>
> >>>   switch (buffer_entry[1]) {
> >>>   case 0x: /* One key pressed or event occurred */
> >>> + case 0x0012: /* Event with extended data occurred */
> > 
> > I don't really like this being handled as a key as it's just discarding all
> > that extended data.

It is not ideal. But currently we handle all events as key press. And
non-key events are marked as ignored.

I agree, it should be handled in better way. But that needs to extend
driver. But it is something which should be done in next patches, not in
this one (if somebody has time to do it, etc...).

> >>
> >> Mario, are you able to get some official documentation for these 0x0012
> >> event types? I think it could be really useful for community so they can
> >> understand and add easily new type of code and events. Because currently
> >> we are just guessing what it could be. (It is sequence? Or single event?
> >> Or single event with extended data? It is generic event? Or it is real
> >> keypress? etc...)
> > 
> > It's a single event with more data in the subsequent words.  It is 
> > definitely
> > not a real keypress.  It's supposed to be data that a user application 
> > would show.
> > 
> > Remember the way WMI works on Linux and Windows is different.  On Windows
> > userland applications get the events directly.  On Linux kernel drivers get 
> > the
> > events and either use it internally, pass to another kernel driver or pass 
> > to
> > userland in the form of a translated event.
> > 
> > So on Windows the whole buffer gets looked at directly by the application 

Re: [PATCH v2 3/3] platform/x86: dell-wmi: add new dmi keys to bios_to_linux_keycode

[Pali Rohár]

On Monday 08 June 2020 20:43:45 Y Paritcher wrote:
> On 6/8/20 7:55 PM, Pali Rohár wrote:
> > Hello!
> > 
> > On Monday 08 June 2020 16:27:10 Randy Dunlap wrote:
> >> Hi--
> >>
> >> On 6/8/20 4:05 PM, Y Paritcher wrote:
> >>> Increase length of bios_to_linux_keycode to 2 bytes (the true size of a
> >>> keycode) to allow for a new keycode 0x, this silences the following
> >>> messages being logged at startup on a Dell Inspiron 5593:
> >>>
> >>> dell_wmi: firmware scancode 0x48 maps to unrecognized keycode 0x
> >>> dell_wmi: firmware scancode 0x50 maps to unrecognized keycode 0x
> > 
> > Which keys generate these two scancodes? Or how have you been able to
> > trigger these scancodes (in case they are not generated by key press)?
> > 
> > It is important to know for which key or event or feature we need to
> > include this patch and therefore what feature is currently
> > non-functional on that laptop.
> > 
> 
> As I said before:
> The DMI contains a table of firmware scancode to linux keycode mappings.
> this is parsed at boot and used together with the bios_to_linux_keycode
> entries & dell_wmi_keymap_type_ tables to create a keymap.
> 
> If a DMI entry does not have a corresponding entry in bios_to_linux_keycode
> we log a message to allow adding the correct linux keycode if known.
> This is regardless of if the key actually exists on the device.
> 
> To date, I have not been able to generate this keycode on my computer.

Ok, so you have just these bios scan codes in your DMI table, but you do
not know what they means nor if you can trigger them somehow.

You should include this information into commit message.

It is possible that your particular laptop model does not have
physically keys which can trigger these codes... (Just guessing)

> >>> as per this code comment:
> >>>
> >>>Log if we find an entry in the DMI table that we don't
> >>>understand.  If this happens, we should figure out what
> >>>the entry means and add it to bios_to_linux_keycode.
> >>>
> >>> These are keycodes included in the 0xB2 DMI table, for which the
> >>> corosponding keys are not known.
> >>
> >>   corresponding
> >>
> >>>
> >>> Now when a user will encounter this key, a proper message wil be printed:
> >>>
> >>> dell_wmi: Unknown key with type 0x and code 0x pressed
> >>>
> >>> This will then allow the key to be identified properly.
> >>>
> >>> Signed-off-by: Y Paritcher 
> >>> ---
> >>>  drivers/platform/x86/dell-wmi.c | 8 +++-
> >>>  1 file changed, 3 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/drivers/platform/x86/dell-wmi.c 
> >>> b/drivers/platform/x86/dell-wmi.c
> >>> index 6b510f8431a3..dae1db96b5a0 100644
> >>> --- a/drivers/platform/x86/dell-wmi.c
> >>> +++ b/drivers/platform/x86/dell-wmi.c
> >>> @@ -196,7 +196,7 @@ struct dell_dmi_results {
> >>>  };
> >>>  
> >>>  /* Uninitialized entries here are KEY_RESERVED == 0. */
> >>> -static const u16 bios_to_linux_keycode[256] = {
> >>> +static const u16 bios_to_linux_keycode[65536] = {
> >>
> >> It surely seems odd to me to expand an array from 512 bytes to 128 Kbytes
> >> just to handle one special case.  Can't it be handled in code as a
> >> special case?
> > 
> > I already wrote that more developers would not be happy about this
> > change. I would rather to see e.g. that Randy's suggestion with 0x
> > check as increasing memory usage.
> > 
> 
> Will change
> 
> >>>   [0] = KEY_MEDIA,
> >>>   [1] = KEY_NEXTSONG,
> >>>   [2] = KEY_PLAYPAUSE,
> >>> @@ -237,6 +237,7 @@ static const u16 bios_to_linux_keycode[256] = {
> >>>   [37]= KEY_UNKNOWN,
> >>>   [38]= KEY_MICMUTE,
> >>>   [255]   = KEY_PROG3,
> >>> + [65535] = KEY_UNKNOWN,
> > 
> > Looking at the last two lines... and for me it looks like that 0x00FF
> > and 0x are just "placeholders" or special values for unknown /
> > custom / unsupported / reserved / special / ... codes.
> > 
> 
> Probably so, but i have no way of knowing.

This was question for Mario as he is probably the only person who can
bring some light to this area.

> I just don't think there is a point spamming a users log with info t

Re: [PATCH 3/3] platform/x86: dell-wmi: add keys to bios_to_linux_keycode

[Pali Rohár]

On Monday 08 June 2020 20:58:38 mario.limoncie...@dell.com wrote:
> > -Original Message-
> > From: platform-driver-x86-ow...@vger.kernel.org  > ow...@vger.kernel.org> On Behalf Of Pali Rohár
> > Sent: Monday, June 8, 2020 3:48 PM
> > To: Limonciello, Mario
> > Cc: y.li...@paritcher.com; linux-kernel@vger.kernel.org; platform-driver-
> > x...@vger.kernel.org; mj...@srcf.ucam.org
> > Subject: Re: [PATCH 3/3] platform/x86: dell-wmi: add keys to
> > bios_to_linux_keycode
> > 
> > 
> > [EXTERNAL EMAIL]
> > 
> > On Monday 08 June 2020 15:46:44 mario.limoncie...@dell.com wrote:
> > > I would actually question if there is value to lines in dell-wmi.c like
> > this:
> > >
> > > pr_info("Unknown WMI event type 0x%x\n", (int)buffer_entry[1]);
> > >
> > > and
> > >
> > > pr_info("Unknown key with type 0x%04x and code 0x%04x pressed\n", type,
> > code);
> > >
> > > In both of those cases the information doesn't actually help the user, by
> > default it's
> > > ignored by the driver anyway.  It just notifies the user it's something
> > the driver doesn't
> > > comprehend.  I would think these are better suited to downgrade to debug.
> > And if
> > > a key combination isn't doing something expected the user can use dyndbg
> > to turn it
> > > back on and can be debugged what should be populated or "explicitly"
> > ignored.
> > 
> > My motivation for these messages was to provide information to user that
> > kernel received event, but was not able to process it as it do not
> > understand it.
> > 
> > It could help in situation when user press special key and nothing is
> > delivered to userspace. But he could see that something happened in log.
> > 
> 
> But does a user know what to do with this information?  From time to time
> coming to kernel mailing list, but that's it.

That is a good question. I'm really not sure if user can do anything
with it. But also users do not care about these kind of logs. So
probably even do not know about them.

What is nice in this solution that if you want to try "debug" such
problem you just need to ask user for logs. Nothing is needed to enabled
/ disable.

> I think same person who would know to come to kernel mailing list for a key
> not working can likely also hand turning on dyndbg to get the info.

You are probably right.

In past I did one thing thanks to these logs. I searched for these log
messages on interned. More results were on forum discussions. I tried to
contact users of those posts and I think 3-4 people wrote me back with
details which allowed me to extend dell-wmi driver to handle additional
key codes.

So I see two benefits from these logs: 1) no special setup is needed to
gather these logs (useful for non-power users) and 2) ability to search
on internet if we have laptops which generates such unknown key codes
and users are "complaining" or posting their logs for investigation on
places where are no kernel developers available.

So question is if these two points are reason why to stick with these
logs or turn them off by default.

I still think it can be useful.

> > Similar message is also printed by PS/2 keyboard driver atkbd.c:
> > 
> > case ATKBD_KEY_UNKNOWN:
> > dev_warn(&serio->dev,
> >  "Unknown key %s (%s set %d, code %#x on %s).\n",
> >  atkbd->release ? "released" : "pressed",
> >  atkbd->translated ? "translated" : "raw",
> >  atkbd->set, code, serio->phys);
> > dev_warn(&serio->dev,
> >  "Use 'setkeycodes %s%02x ' to make it 
> > known.\n",
> >  code & 0x80 ? "e0" : "", code & 0x7f);
> > input_sync(dev);
> > break;
> 
> I think the difference here is that user can actually do something from 
> userland
> to do with `setkeycodes` for PS2.

Of course, I agree.

Re: [PATCH 2/3] platform/x86: dell-wmi: add new keymap type 0x0012

[Pali Rohár]

On Monday 08 June 2020 15:40:53 mario.limoncie...@dell.com wrote:
> > dell_wmi: Unknown WMI event type 0x12
> > dell_wmi: Unknown key with type 0x0012 and code 0xe035 pressed
> 
> Event type 0x12 is for "System Events".  This is the type of events that
> you typically would see come in for things "like" the wrong power adapter
> being plugged in on Windows or stuff about plugging a Thunderbolt dock into
> a port that doesn't support Thunderbolt.
> 
> A message might look something like (paraphrased)
> "Your system requires a 180W power adapter to charge effectively, but you
> plugged in a 60W adapter."
> 
> There often is extended data with these events.  As such I don't believe all
> information in event type 0x0012 should be treated like scan codes like those 
> in
> 0x10 or 0x11.
> 
> I would guess that Fn-lock on this machine probably has extended data in the 
> next
> showing whether it was turned on and off.  If it does, perhaps it makes sense 
> to
> send this information to userspace as an evdev switch instead.

Thank you for information!

Interesting is that I got this email only now, after all other emails in
this thread. But seems that you have wrote it prior I asked question
about documentation for these events. So probably some email delivery
problem / delay.

Re: [PATCH v2 3/3] platform/x86: dell-wmi: add new dmi keys to bios_to_linux_keycode

[Pali Rohár]

Hello!

On Monday 08 June 2020 16:27:10 Randy Dunlap wrote:
> Hi--
> 
> On 6/8/20 4:05 PM, Y Paritcher wrote:
> > Increase length of bios_to_linux_keycode to 2 bytes (the true size of a
> > keycode) to allow for a new keycode 0x, this silences the following
> > messages being logged at startup on a Dell Inspiron 5593:
> > 
> > dell_wmi: firmware scancode 0x48 maps to unrecognized keycode 0x
> > dell_wmi: firmware scancode 0x50 maps to unrecognized keycode 0x

Which keys generate these two scancodes? Or how have you been able to
trigger these scancodes (in case they are not generated by key press)?

It is important to know for which key or event or feature we need to
include this patch and therefore what feature is currently
non-functional on that laptop.

> > as per this code comment:
> > 
> >Log if we find an entry in the DMI table that we don't
> >understand.  If this happens, we should figure out what
> >the entry means and add it to bios_to_linux_keycode.
> > 
> > These are keycodes included in the 0xB2 DMI table, for which the
> > corosponding keys are not known.
> 
>   corresponding
> 
> > 
> > Now when a user will encounter this key, a proper message wil be printed:
> > 
> > dell_wmi: Unknown key with type 0x and code 0x pressed
> > 
> > This will then allow the key to be identified properly.
> > 
> > Signed-off-by: Y Paritcher 
> > ---
> >  drivers/platform/x86/dell-wmi.c | 8 +++-
> >  1 file changed, 3 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/dell-wmi.c 
> > b/drivers/platform/x86/dell-wmi.c
> > index 6b510f8431a3..dae1db96b5a0 100644
> > --- a/drivers/platform/x86/dell-wmi.c
> > +++ b/drivers/platform/x86/dell-wmi.c
> > @@ -196,7 +196,7 @@ struct dell_dmi_results {
> >  };
> >  
> >  /* Uninitialized entries here are KEY_RESERVED == 0. */
> > -static const u16 bios_to_linux_keycode[256] = {
> > +static const u16 bios_to_linux_keycode[65536] = {
> 
> It surely seems odd to me to expand an array from 512 bytes to 128 Kbytes
> just to handle one special case.  Can't it be handled in code as a
> special case?

I already wrote that more developers would not be happy about this
change. I would rather to see e.g. that Randy's suggestion with 0x
check as increasing memory usage.

> > [0] = KEY_MEDIA,
> > [1] = KEY_NEXTSONG,
> > [2] = KEY_PLAYPAUSE,
> > @@ -237,6 +237,7 @@ static const u16 bios_to_linux_keycode[256] = {
> > [37]= KEY_UNKNOWN,
> > [38]= KEY_MICMUTE,
> > [255]   = KEY_PROG3,
> > +   [65535] = KEY_UNKNOWN,

Looking at the last two lines... and for me it looks like that 0x00FF
and 0x are just "placeholders" or special values for unknown /
custom / unsupported / reserved / special / ... codes.

It is really suspicious why first 38 values are defined, then there is
gap, then one value 255 and then huge gap to 65535.

Mario, this looks like some mapping table between internal Dell BIOS key
code and standard Linux key code. Are you able to get access to some
documentation which contains explanation of those Dell key numbers?
It could really help us to understand these gaps and what is correct
interpretation of these numbers.

E.g. I remember that pressing Fn+Q or Fn+W on some Dell Latitude
generates code 255, which could prove my thesis about "special codes"
(which are probably not found in e.g. Windows or Linux mapping tables).

> >  };
> >  
> >  /*
> > @@ -503,10 +504,7 @@ static void handle_dmi_entry(const struct dmi_header 
> > *dm, void *opaque)
> > &table->keymap[i];
> >  
> > /* Uninitialized entries are 0 aka KEY_RESERVED. */
> > -   u16 keycode = (bios_entry->keycode <
> > -  ARRAY_SIZE(bios_to_linux_keycode)) ?
> > -   bios_to_linux_keycode[bios_entry->keycode] :
> > -   KEY_RESERVED;
> > +   u16 keycode = bios_to_linux_keycode[bios_entry->keycode];
> >  
> > /*
> >  * Log if we find an entry in the DMI table that we don't
> > 
> 
> Something like:
> 
>   u16 keycode;
> 
>   keycode = bios_entry->keycode == 0x ? KEY_UNKNOWN :
>   (bios_entry->keycode <
>  ARRAY_SIZE(bios_to_linux_keycode)) ?
>   bios_to_linux_keycode[bios_entry->keycode] :
>   KEY_RESERVED;
> 
> 
> 
> Also please fix this:
> (no To-header on input) <>

Hint: specifying git send-email with '--to' argument instead of '--cc'
should help.

> 
> -- 
> ~Randy
> 

Re: [PATCH v2 2/3] platform/x86: dell-wmi: add new keymap type 0x0012

[Pali Rohár]

On Monday 08 June 2020 19:05:29 Y Paritcher wrote:
> These are events with extended data. The extended data is
> currently ignored as userspace does not have a way to deal
> it.
> 
> Ignore event with a type of 0x0012 and a code of 0xe035, as
> the keyboard controller takes care of Fn lock events by itself.

Nice! This is information which is really important and need to have it
documented.

> This silences the following messages being logged when
> pressing the Fn-lock key on a Dell Inspiron 5593:
> 
> dell_wmi: Unknown WMI event type 0x12
> dell_wmi: Unknown key with type 0x0012 and code 0xe035 pressed
> 
> This is consistent with the behavior for the Fn-lock key
> elsewhere in this file.
> 
> Signed-off-by: Y Paritcher 

I'm fine with this patch now.

Reviewed-by: Pali Rohár 

> ---
>  drivers/platform/x86/dell-wmi.c | 20 +++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
> index 0b2edfe2767d..6b510f8431a3 100644
> --- a/drivers/platform/x86/dell-wmi.c
> +++ b/drivers/platform/x86/dell-wmi.c
> @@ -334,6 +334,15 @@ static const struct key_entry 
> dell_wmi_keymap_type_0011[] = {
>   { KE_IGNORE, KBD_LED_AUTO_100_TOKEN, { KEY_RESERVED } },
>  };
>  
> +/*
> + * Keymap for WMI events of type 0x0012
> + * They are events with extended data
> + */
> +static const struct key_entry dell_wmi_keymap_type_0012[] = {
> + /* Fn-lock button pressed */
> + { KE_IGNORE, 0xe035, { KEY_RESERVED } },
> +};
> +
>  static void dell_wmi_process_key(struct wmi_device *wdev, int type, int code)
>  {
>   struct dell_wmi_priv *priv = dev_get_drvdata(&wdev->dev);
> @@ -418,10 +427,11 @@ static void dell_wmi_notify(struct wmi_device *wdev,
>  
>   switch (buffer_entry[1]) {
>   case 0x: /* One key pressed or event occurred */
> + case 0x0012: /* Event with extended data occurred */

Mario, are you able to get some official documentation for these 0x0012
event types? I think it could be really useful for community so they can
understand and add easily new type of code and events. Because currently
we are just guessing what it could be. (It is sequence? Or single event?
Or single event with extended data? It is generic event? Or it is real
keypress? etc...)

>   if (len > 2)
>   dell_wmi_process_key(wdev, 0x,
>buffer_entry[2]);
> - /* Other entries could contain additional information */
> + /* Extended data is currently ignored */
>   break;
>   case 0x0010: /* Sequence of keys pressed */
>   case 0x0011: /* Sequence of events occurred */
> @@ -556,6 +566,7 @@ static int dell_wmi_input_setup(struct wmi_device *wdev)
>ARRAY_SIZE(dell_wmi_keymap_type_) +
>ARRAY_SIZE(dell_wmi_keymap_type_0010) +
>ARRAY_SIZE(dell_wmi_keymap_type_0011) +
> +  ARRAY_SIZE(dell_wmi_keymap_type_0012) +
>1,
>sizeof(struct key_entry), GFP_KERNEL);
>   if (!keymap) {
> @@ -600,6 +611,13 @@ static int dell_wmi_input_setup(struct wmi_device *wdev)
>   pos++;
>   }
>  
> + /* Append table with events of type 0x0012 */
> + for (i = 0; i < ARRAY_SIZE(dell_wmi_keymap_type_0012); i++) {
> + keymap[pos] = dell_wmi_keymap_type_0012[i];
> + keymap[pos].code |= (0x0012 << 16);
> + pos++;
> + }
> +
>   /*
>* Now append also table with "legacy" events of type 0x. Some of
>* them are reported also on laptops which have scancodes in DMI.
> -- 
> 2.27.0
> 

Re: [PATCH v2 1/3] platform/x86: dell-wmi: add new backlight events

[Pali Rohár]

On Monday 08 June 2020 19:05:28 Y Paritcher wrote:
> Add events with a type of 0x0010 and a code of 0x57 / 0x58,
> this silences the following messages being logged on a
> Dell Inspiron 5593:
> 
> dell_wmi: Unknown key with type 0x0010 and code 0x0057 pressed
> dell_wmi: Unknown key with type 0x0010 and code 0x0058 pressed
> 
> These are brightness events and will be handled by acpi-video
> 
> Signed-off-by: Y Paritcher 

Reviewed-by: Pali Rohár 

> ---
>  drivers/platform/x86/dell-wmi.c | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
> index c25a4286d766..0b2edfe2767d 100644
> --- a/drivers/platform/x86/dell-wmi.c
> +++ b/drivers/platform/x86/dell-wmi.c
> @@ -255,6 +255,10 @@ static const struct key_entry 
> dell_wmi_keymap_type_0010[] = {
>   /* Keyboard backlight change notification */
>   { KE_IGNORE, 0x3f, { KEY_RESERVED } },
>  
> + /* Backlight brightness level */
> + { KE_KEY,0x57, { KEY_BRIGHTNESSDOWN } },
> + { KE_KEY,0x58, { KEY_BRIGHTNESSUP } },
> +
>   /* Mic mute */
>   { KE_KEY, 0x150, { KEY_MICMUTE } },
>  
> -- 
> 2.27.0
> 

Re: [PATCH 3/3] platform/x86: dell-wmi: add keys to bios_to_linux_keycode

[Pali Rohár]

On Monday 08 June 2020 15:46:44 mario.limoncie...@dell.com wrote:
> I would actually question if there is value to lines in dell-wmi.c like this:
> 
> pr_info("Unknown WMI event type 0x%x\n", (int)buffer_entry[1]);
> 
> and
> 
> pr_info("Unknown key with type 0x%04x and code 0x%04x pressed\n", type, code);
> 
> In both of those cases the information doesn't actually help the user, by 
> default it's
> ignored by the driver anyway.  It just notifies the user it's something the 
> driver doesn't
> comprehend.  I would think these are better suited to downgrade to debug.  
> And if
> a key combination isn't doing something expected the user can use dyndbg to 
> turn it
> back on and can be debugged what should be populated or "explicitly" ignored.

My motivation for these messages was to provide information to user that
kernel received event, but was not able to process it as it do not
understand it.

It could help in situation when user press special key and nothing is
delivered to userspace. But he could see that something happened in log.

Similar message is also printed by PS/2 keyboard driver atkbd.c:

case ATKBD_KEY_UNKNOWN:
dev_warn(&serio->dev,
 "Unknown key %s (%s set %d, code %#x on %s).\n",
 atkbd->release ? "released" : "pressed",
 atkbd->translated ? "translated" : "raw",
 atkbd->set, code, serio->phys);
dev_warn(&serio->dev,
 "Use 'setkeycodes %s%02x ' to make it 
known.\n",
 code & 0x80 ? "e0" : "", code & 0x7f);
input_sync(dev);
break;

Re: [PATCH 1/3] platform/x86: dell-wmi: add new backlight events

[Pali Rohár]

On Monday 08 June 2020 20:14:15 mario.limoncie...@dell.com wrote:
> > >>> index c25a4286d766..0b4f72f923cd 100644
> > >>> --- a/drivers/platform/x86/dell-wmi.c
> > >>> +++ b/drivers/platform/x86/dell-wmi.c
> > >>> @@ -252,6 +252,10 @@ static const struct key_entry
> > >> dell_wmi_keymap_type_0010[] = {
> > >>> /* Fn-lock switched to multimedia keys */
> > >>> { KE_IGNORE, 0x1, { KEY_RESERVED } },
> > >>>
> > >>> +   /* Backlight brightness level */
> > >>> +   { KE_KEY,0x57, { KEY_BRIGHTNESSDOWN } },
> > >>> +   { KE_KEY,0x58, { KEY_BRIGHTNESSUP } },
> > >>> +
> > >
> > > For these particular events are they emitted by another interface as well
> > in this
> > > platform?
> > >
> > > If so they should be KE_IGNORE so you don't end up with double
> > notifications to
> > > userspace.
> > Thank you both for the review,
> > This is my first patch so if i am doing something wrong please let me know.
> > 
> > Both before and after this change they are only emitted once (verified via
> > showkeys)
> > this is because `dell_wmi_process_key()` calls
> > `acpi_video_handles_brightness_key_presses()`
> > for brightness events, and `acpi_video_handles_brightness_key_presses()`
> > makes sure no duplicate acpi-video events are sent.
> 
> That's good to hear that it also filters it, but my opinion is that dell-wmi.c
> should also filter it.  So just change KE_KEY to KE_IGNORE like the other 
> events.

IIRC for other existing KEY_BRIGHTNESS* lines, KE_KEY/KE_IGNORE decision
is there just because kernel can be configured if ACPI layer would
handle them or not. And based on acpi_video_handles_brightness_key_presses()
function we know if ACPI layer processed these keys or not.

So in my opinion we should handle these new KEY_BRIGHTNESS* events in
the same way. So dell-wmi should process these events, but only in case
when ACPI layer did not processed them.

Re: [PATCH 2/3] platform/x86: dell-wmi: add new keymap type 0x0012

[Pali Rohár]

Hello!

On Monday 08 June 2020 16:12:52 Y Paritcher wrote:
> You are right.
> I had assumed (incorrectly) the were the same.
> I turned on dyndbg and got the events with the extended data.
> 
> Fn lock key switched to multimedia keys
> dell_wmi: Received WMI event (02 00 12 00 35 e0 01 00 00 00 00 00 00 00 00 00 
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00)
> the extended data is e0 01
> 
> Fn-lock switched to function keys
> dell_wmi: Received WMI event (02 00 12 00 35 e0 00 00 00 00 00 00 00 00 00 00 
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00)
> the extended data is e0 00

That is why I asked if buffer is really sequence of events. From your
log can be seen that it is not a sequence, but rather just one event.

> Therefore i agree this should have it's own case in `dell_wmi_process_key` 
> but i am
> not sure yet how to deal with it. any suggestion are helpful.

I guess you could handle it like for event type 0x which also
process one event where can be additional information in buffer.
See relevant code for 0x:

case 0x: /* One key pressed or event occurred */
if (len > 2)
dell_wmi_process_key(wdev, 0x,
 buffer_entry[2]);
/* Other entries could contain additional information */
break;

Just processing of additional information from buffer is not implemented
yet, probably nobody needed it yet.

> About sending it to userspace, I just followed what was already done, if that 
> is not
> desired we should change it for all the models.

Main question if we need to send this event to userspace. Or if we
should drop this event as it is duplicate which was already processed by
other layer. This is something which we do not know and it needs to be
investigated and documented/explained. So in future when will do some
changes in that code, we would know how to handle it without breaking
existing systems.

I would suggest to not describe changes in commit message, but rather
describe explanation of those changes in commit message. E.g. what was
decision and why you are doing it in that way.

It would help in future to know why such code is needed.

E.g. "event XYZ needs to be ignored as kernel receive its duplicate also
via keyboard controller". Or e.g. "event needs to be processed by wmi
driver because notebook's embedded controller sends it only via wmi".

Re: [PATCH 3/3] platform/x86: dell-wmi: add keys to bios_to_linux_keycode

[Pali Rohár]

Hello!

On Monday 08 June 2020 00:22:26 Y Paritcher wrote:
> Increase length of bios_to_linux_keycode to 2 bytes to allow for a new
> keycode 0x, this silences the following messages being logged at
> startup on a Dell Inspiron 5593
> 
> dell_wmi: firmware scancode 0x48 maps to unrecognized keycode 0x
> dell_wmi: firmware scancode 0x50 maps to unrecognized keycode 0x
> 
> Signed-off-by: Y Paritcher 
> ---
>  drivers/platform/x86/dell-wmi.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
> index f37e7e9093c2..5ef716e3034f 100644
> --- a/drivers/platform/x86/dell-wmi.c
> +++ b/drivers/platform/x86/dell-wmi.c
> @@ -196,7 +196,7 @@ struct dell_dmi_results {
>  };
>  
>  /* Uninitialized entries here are KEY_RESERVED == 0. */
> -static const u16 bios_to_linux_keycode[256] = {
> +static const u16 bios_to_linux_keycode[65536] = {

This change dramatically increase memory usage. I guess other that
maintainers would not like such change.

>   [0] = KEY_MEDIA,
>   [1] = KEY_NEXTSONG,
>   [2] = KEY_PLAYPAUSE,
> @@ -237,6 +237,7 @@ static const u16 bios_to_linux_keycode[256] = {
>   [37]= KEY_UNKNOWN,
>   [38]= KEY_MICMUTE,
>   [255]   = KEY_PROG3,
> + [65535] = KEY_UNKNOWN,

Also it seems that this change is not complete. It looks like that you
map two different scancodes (0x48 and 0x50) to same keycodes, moreover
both are unknown.

Could you please describe which key presses (or events) generate
delivering these WMI scancode events?

Note that purpose of printing unknown/unrecognized keys messages is to
inform that current pressed key was not processed or that it was
ignored.

For me it looks like this just just hide information that key was not
processed correctly as this change does not implement correct processing
of this key.

Also, could you share documentation about these 0x48/0x50 events? Or it
is under NDA?

>  };
>  
>  /*
> -- 
> 2.27.0
> 

Re: [PATCH 2/3] platform/x86: dell-wmi: add new keymap type 0x0012

[Pali Rohár]

Hello!

On Monday 08 June 2020 00:22:25 Y Paritcher wrote:
> Ignore events with a type of 0x0012 and a code of 0xe035,
> this silences the following messages being logged when
> pressing the Fn-lock key on a Dell Inspiron 5593:

Could you please explain why to ignore these events instead of sending
them to userspace via input layer? I think that userspace can be
interested in knowing when Fn lock key was pressed and I can imagine
that some it can use it for some purposes.

> dell_wmi: Unknown WMI event type 0x12
> dell_wmi: Unknown key with type 0x0012 and code 0xe035 pressed

These messages are printed to inform about fact that some events were
not processed. And they should not be silenced without reason. If for
some reasons it is needed to completely ignore some kind of events then
this reason should be documented (e.g. in commit message) so other
developers would know why that code is there. Not all Linux developers
have all those Dell machines for testing so they do not know all
hardware details.

> Signed-off-by: Y Paritcher 
> ---
>  drivers/platform/x86/dell-wmi.c | 17 +
>  1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
> index 0b4f72f923cd..f37e7e9093c2 100644
> --- a/drivers/platform/x86/dell-wmi.c
> +++ b/drivers/platform/x86/dell-wmi.c
> @@ -334,6 +334,14 @@ static const struct key_entry 
> dell_wmi_keymap_type_0011[] = {
>   { KE_IGNORE, KBD_LED_AUTO_100_TOKEN, { KEY_RESERVED } },
>  };
>  
> +/*
> + * Keymap for WMI events of type 0x0012
> + */
> +static const struct key_entry dell_wmi_keymap_type_0012[] = {
> + /* Fn-lock button pressed */
> + { KE_IGNORE, 0xe035, { KEY_RESERVED } },
> +};
> +
>  static void dell_wmi_process_key(struct wmi_device *wdev, int type, int code)
>  {
>   struct dell_wmi_priv *priv = dev_get_drvdata(&wdev->dev);
> @@ -425,6 +433,7 @@ static void dell_wmi_notify(struct wmi_device *wdev,
>   break;
>   case 0x0010: /* Sequence of keys pressed */
>   case 0x0011: /* Sequence of events occurred */
> + case 0x0012: /* Sequence of events occurred */

It is really sequence of events? Because you wrote that Fn-lock key was
pressed (and not generic event). Also it is really sequence? And not
just one event/key-press (with possibility of some additional details in
buffer)? It would be nice to put documentation for this type of events
to check and review that implementation is correct.

>   for (i = 2; i < len; ++i)
>   dell_wmi_process_key(wdev, buffer_entry[1],
>buffer_entry[i]);
> @@ -556,6 +565,7 @@ static int dell_wmi_input_setup(struct wmi_device *wdev)
>ARRAY_SIZE(dell_wmi_keymap_type_) +
>ARRAY_SIZE(dell_wmi_keymap_type_0010) +
>ARRAY_SIZE(dell_wmi_keymap_type_0011) +
> +  ARRAY_SIZE(dell_wmi_keymap_type_0012) +
>1,
>sizeof(struct key_entry), GFP_KERNEL);
>   if (!keymap) {
> @@ -600,6 +610,13 @@ static int dell_wmi_input_setup(struct wmi_device *wdev)
>   pos++;
>   }
>  
> + /* Append table with events of type 0x0012 */
> + for (i = 0; i < ARRAY_SIZE(dell_wmi_keymap_type_0012); i++) {
> + keymap[pos] = dell_wmi_keymap_type_0012[i];
> + keymap[pos].code |= (0x0012 << 16);
> + pos++;
> + }
> +
>   /*
>* Now append also table with "legacy" events of type 0x. Some of
>* them are reported also on laptops which have scancodes in DMI.
> -- 
> 2.27.0
> 

Re: [PATCH 1/3] platform/x86: dell-wmi: add new backlight events

[Pali Rohár]

On Monday 08 June 2020 00:22:24 Y Paritcher wrote:
> Ignore events with a type of 0x0010 and a code of 0x57 / 0x58,
> this silences the following messages being logged on a
> Dell Inspiron 5593:
> 
> dell_wmi: Unknown key with type 0x0010 and code 0x0057 pressed
> dell_wmi: Unknown key with type 0x0010 and code 0x0058 pressed
> 
> Signed-off-by: Y Paritcher 
> ---
>  drivers/platform/x86/dell-wmi.c | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
> index c25a4286d766..0b4f72f923cd 100644
> --- a/drivers/platform/x86/dell-wmi.c
> +++ b/drivers/platform/x86/dell-wmi.c
> @@ -252,6 +252,10 @@ static const struct key_entry 
> dell_wmi_keymap_type_0010[] = {
>   /* Fn-lock switched to multimedia keys */
>   { KE_IGNORE, 0x1, { KEY_RESERVED } },
>  
> + /* Backlight brightness level */
> + { KE_KEY,0x57, { KEY_BRIGHTNESSDOWN } },
> + { KE_KEY,0x58, { KEY_BRIGHTNESSUP } },
> +
>   /* Keyboard backlight change notification */
>   { KE_IGNORE, 0x3f, { KEY_RESERVED } },

Please, keep codes sorted.

>  
> -- 
> 2.27.0
> 

[PATCH 0/4] marvell: Fix firmware filenames for sd8977/sd8997 chipsets

[Pali Rohár]

This patch series fixes mwifiex and btmrvl drivers to load firmware for sd8977
and sd8997 chipsets from correct filename.

Both Marvell distribution package and linux-firmware repository [1] contain
firmware for these chipsets in files 
sdsd8977_combo_v2.bin/sdsd8997_combo_v4.bin.

Linux drivers mwifiex and btmrvl try to load firmware for these chipsets from
sd8977_uapsta.bin/sd8997_uapsta.bin files which obviously fails as these files
do not exist neither in linux-firmware [1] nor in Marvell distribution packages.

So the result is that Marvell sd8977 and sd8997 chipsets via mainline kernel
drivers (mwifiex and btmrvl) do not work out of box.

Each patch in this series fixes particular git commit which introduced usage
of incorrect firmware filename. Also each patch contains Fixes: line for easier
backporting to stable kernels.

mwifiex (1/4, 2/4) and btmrvl (3/4, 4/4) parts of this patch series can be
applied separately via wireless and bluetooth trees. I'm sending all four
patches in one patch series for easier review.

[1] - 
https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git/tree/mrvl

Pali Rohár (4):
  mwifiex: Fix firmware filename for sd8977 chipset
  mwifiex: Fix firmware filename for sd8997 chipset
  btmrvl: Fix firmware filename for sd8977 chipset
  btmrvl: Fix firmware filename for sd8997 chipset

 drivers/bluetooth/btmrvl_sdio.c | 8 
 drivers/net/wireless/marvell/mwifiex/sdio.h | 4 ++--
 2 files changed, 6 insertions(+), 6 deletions(-)

-- 
2.20.1

[PATCH 1/4] mwifiex: Fix firmware filename for sd8977 chipset

[Pali Rohár]

Firmware for sd8977 chipset is distributed by Marvell package and also as
part of the linux-firmware repository in filename sdsd8977_combo_v2.bin.

This patch fixes mwifiex driver to load correct firmware file for sd8977.

Fixes: 1a0f547831dce ("mwifiex: add support for sd8977 chipset")
Signed-off-by: Pali Rohár 
---
 drivers/net/wireless/marvell/mwifiex/sdio.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.h 
b/drivers/net/wireless/marvell/mwifiex/sdio.h
index 71cd8629b28e..0cac2296ed53 100644
--- a/drivers/net/wireless/marvell/mwifiex/sdio.h
+++ b/drivers/net/wireless/marvell/mwifiex/sdio.h
@@ -36,7 +36,7 @@
 #define SD8897_DEFAULT_FW_NAME "mrvl/sd8897_uapsta.bin"
 #define SD8887_DEFAULT_FW_NAME "mrvl/sd8887_uapsta.bin"
 #define SD8801_DEFAULT_FW_NAME "mrvl/sd8801_uapsta.bin"
-#define SD8977_DEFAULT_FW_NAME "mrvl/sd8977_uapsta.bin"
+#define SD8977_DEFAULT_FW_NAME "mrvl/sdsd8977_combo_v2.bin"
 #define SD8987_DEFAULT_FW_NAME "mrvl/sd8987_uapsta.bin"
 #define SD8997_DEFAULT_FW_NAME "mrvl/sd8997_uapsta.bin"
 
-- 
2.20.1

[PATCH 2/4] mwifiex: Fix firmware filename for sd8997 chipset

[Pali Rohár]

Firmware for sd8997 chipset is distributed by Marvell package and also as
part of the linux-firmware repository in filename sdsd8997_combo_v4.bin.

This patch fixes mwifiex driver to load correct firmware file for sd8997.

Fixes: 6d85ef00d9dfe ("mwifiex: add support for 8997 chipset")
Signed-off-by: Pali Rohár 
---
 drivers/net/wireless/marvell/mwifiex/sdio.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.h 
b/drivers/net/wireless/marvell/mwifiex/sdio.h
index 0cac2296ed53..8b476b007c5e 100644
--- a/drivers/net/wireless/marvell/mwifiex/sdio.h
+++ b/drivers/net/wireless/marvell/mwifiex/sdio.h
@@ -38,7 +38,7 @@
 #define SD8801_DEFAULT_FW_NAME "mrvl/sd8801_uapsta.bin"
 #define SD8977_DEFAULT_FW_NAME "mrvl/sdsd8977_combo_v2.bin"
 #define SD8987_DEFAULT_FW_NAME "mrvl/sd8987_uapsta.bin"
-#define SD8997_DEFAULT_FW_NAME "mrvl/sd8997_uapsta.bin"
+#define SD8997_DEFAULT_FW_NAME "mrvl/sdsd8997_combo_v4.bin"
 
 #define BLOCK_MODE 1
 #define BYTE_MODE  0
-- 
2.20.1

[PATCH 3/4] btmrvl: Fix firmware filename for sd8977 chipset

[Pali Rohár]

Firmware for sd8977 chipset is distributed by Marvell package and also as
part of the linux-firmware repository in filename sdsd8977_combo_v2.bin.

This patch fixes mwifiex driver to load correct firmware file for sd8977.

Fixes: 8c57983bf7a79 ("Bluetooth: btmrvl: add support for sd8977 chipset")
Signed-off-by: Pali Rohár 
---
 drivers/bluetooth/btmrvl_sdio.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/bluetooth/btmrvl_sdio.c b/drivers/bluetooth/btmrvl_sdio.c
index 0f3a020703ab..7aa2c94720bc 100644
--- a/drivers/bluetooth/btmrvl_sdio.c
+++ b/drivers/bluetooth/btmrvl_sdio.c
@@ -328,7 +328,7 @@ static const struct btmrvl_sdio_device btmrvl_sdio_sd8897 = 
{
 
 static const struct btmrvl_sdio_device btmrvl_sdio_sd8977 = {
.helper = NULL,
-   .firmware   = "mrvl/sd8977_uapsta.bin",
+   .firmware   = "mrvl/sdsd8977_combo_v2.bin",
.reg= &btmrvl_reg_8977,
.support_pscan_win_report = true,
.sd_blksz_fw_dl = 256,
@@ -1831,6 +1831,6 @@ MODULE_FIRMWARE("mrvl/sd8787_uapsta.bin");
 MODULE_FIRMWARE("mrvl/sd8797_uapsta.bin");
 MODULE_FIRMWARE("mrvl/sd8887_uapsta.bin");
 MODULE_FIRMWARE("mrvl/sd8897_uapsta.bin");
-MODULE_FIRMWARE("mrvl/sd8977_uapsta.bin");
+MODULE_FIRMWARE("mrvl/sdsd8977_combo_v2.bin");
 MODULE_FIRMWARE("mrvl/sd8987_uapsta.bin");
 MODULE_FIRMWARE("mrvl/sd8997_uapsta.bin");
-- 
2.20.1

[PATCH 4/4] btmrvl: Fix firmware filename for sd8997 chipset

[Pali Rohár]

Firmware for sd8997 chipset is distributed by Marvell package and also as
part of the linux-firmware repository in filename sdsd8997_combo_v4.bin.

This patch fixes mwifiex driver to load correct firmware file for sd8997.

Fixes: f0ef67485f591 ("Bluetooth: btmrvl: add sd8997 chipset support")
Signed-off-by: Pali Rohár 
---
 drivers/bluetooth/btmrvl_sdio.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/bluetooth/btmrvl_sdio.c b/drivers/bluetooth/btmrvl_sdio.c
index 7aa2c94720bc..4c7978cb1786 100644
--- a/drivers/bluetooth/btmrvl_sdio.c
+++ b/drivers/bluetooth/btmrvl_sdio.c
@@ -346,7 +346,7 @@ static const struct btmrvl_sdio_device btmrvl_sdio_sd8987 = 
{
 
 static const struct btmrvl_sdio_device btmrvl_sdio_sd8997 = {
.helper = NULL,
-   .firmware   = "mrvl/sd8997_uapsta.bin",
+   .firmware   = "mrvl/sdsd8997_combo_v4.bin",
.reg= &btmrvl_reg_8997,
.support_pscan_win_report = true,
.sd_blksz_fw_dl = 256,
@@ -1833,4 +1833,4 @@ MODULE_FIRMWARE("mrvl/sd8887_uapsta.bin");
 MODULE_FIRMWARE("mrvl/sd8897_uapsta.bin");
 MODULE_FIRMWARE("mrvl/sdsd8977_combo_v2.bin");
 MODULE_FIRMWARE("mrvl/sd8987_uapsta.bin");
-MODULE_FIRMWARE("mrvl/sd8997_uapsta.bin");
+MODULE_FIRMWARE("mrvl/sdsd8997_combo_v4.bin");
-- 
2.20.1

[PATCH] PCI: aardvark: Indicate error in 'val' when config read fails

[Pali Rohár]

Most callers of config read do not check for return value. But most of the
ones that do, checks for error indication in 'val' variable.

This patch updates error handling in advk_pcie_rd_conf() function. If PIO
transfer fails then 'val' variable is set to 0x which indicates
failture.

Signed-off-by: Pali Rohár 
---
 drivers/pci/controller/pci-aardvark.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pci-aardvark.c 
b/drivers/pci/controller/pci-aardvark.c
index 53a4cfd7d377..783a7f1f2c44 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -691,8 +691,10 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 
devfn,
advk_writel(pcie, 1, PIO_START);
 
ret = advk_pcie_wait_pio(pcie);
-   if (ret < 0)
+   if (ret < 0) {
+   *val = 0x;
return PCIBIOS_SET_FAILED;
+   }
 
advk_pcie_check_pio_status(pcie);
 
-- 
2.20.1

Re: [PATCH] mwifiex: Add support for NL80211_ATTR_MAX_AP_ASSOC_STA

[Pali Rohár]

On Thursday 21 May 2020 14:35:59 Pali Rohár wrote:
> SD8997 firmware sends TLV_TYPE_MAX_CONN with struct hw_spec_max_conn to
> inform kernel about maximum number of p2p connections and stations in AP
> mode.
> 
> During initialization of SD8997 wifi chip kernel prints warning:
> 
>   mwifiex_sdio mmc0:0001:1: Unknown GET_HW_SPEC TLV type: 0x217
> 
> This patch adds support for parsing TLV_TYPE_MAX_CONN (0x217) and sets
> appropriate cfg80211 member 'max_ap_assoc_sta' from retrieved structure.
> 
> It allows userspace to retrieve NL80211_ATTR_MAX_AP_ASSOC_STA attribute.
> 
> Signed-off-by: Pali Rohár 

Hello Kalle and Ganapathi, could you please review this patch?

> ---
>  drivers/net/wireless/marvell/mwifiex/cfg80211.c |  5 +
>  drivers/net/wireless/marvell/mwifiex/cmdevt.c   | 12 
>  drivers/net/wireless/marvell/mwifiex/fw.h   |  8 
>  drivers/net/wireless/marvell/mwifiex/main.h |  1 +
>  4 files changed, 26 insertions(+)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c 
> b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> index 12bfd653a..7998e91c9 100644
> --- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> +++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> @@ -4339,6 +4339,11 @@ int mwifiex_register_cfg80211(struct mwifiex_adapter 
> *adapter)
>   wiphy->iface_combinations = &mwifiex_iface_comb_ap_sta;
>   wiphy->n_iface_combinations = 1;
>  
> + if (adapter->max_sta_conn > adapter->max_p2p_conn)
> + wiphy->max_ap_assoc_sta = adapter->max_sta_conn;
> + else
> + wiphy->max_ap_assoc_sta = adapter->max_p2p_conn;
> +
>   /* Initialize cipher suits */
>   wiphy->cipher_suites = mwifiex_cipher_suites;
>   wiphy->n_cipher_suites = ARRAY_SIZE(mwifiex_cipher_suites);
> diff --git a/drivers/net/wireless/marvell/mwifiex/cmdevt.c 
> b/drivers/net/wireless/marvell/mwifiex/cmdevt.c
> index 589cc5eb1..d068b9075 100644
> --- a/drivers/net/wireless/marvell/mwifiex/cmdevt.c
> +++ b/drivers/net/wireless/marvell/mwifiex/cmdevt.c
> @@ -1495,6 +1495,7 @@ int mwifiex_ret_get_hw_spec(struct mwifiex_private 
> *priv,
>   struct mwifiex_adapter *adapter = priv->adapter;
>   struct mwifiex_ie_types_header *tlv;
>   struct hw_spec_api_rev *api_rev;
> + struct hw_spec_max_conn *max_conn;
>   u16 resp_size, api_id;
>   int i, left_len, parsed_len = 0;
>  
> @@ -1604,6 +1605,17 @@ int mwifiex_ret_get_hw_spec(struct mwifiex_private 
> *priv,
>   break;
>   }
>   break;
> + case TLV_TYPE_MAX_CONN:
> + max_conn = (struct hw_spec_max_conn *)tlv;
> + adapter->max_p2p_conn = max_conn->max_p2p_conn;
> + adapter->max_sta_conn = max_conn->max_sta_conn;
> + mwifiex_dbg(adapter, INFO,
> + "max p2p connections: %u\n",
> + adapter->max_p2p_conn);
> + mwifiex_dbg(adapter, INFO,
> + "max sta connections: %u\n",
> + adapter->max_sta_conn);
> + break;
>   default:
>   mwifiex_dbg(adapter, FATAL,
>   "Unknown GET_HW_SPEC TLV type: 
> %#x\n",
> diff --git a/drivers/net/wireless/marvell/mwifiex/fw.h 
> b/drivers/net/wireless/marvell/mwifiex/fw.h
> index 6f86f5b96..8047e3078 100644
> --- a/drivers/net/wireless/marvell/mwifiex/fw.h
> +++ b/drivers/net/wireless/marvell/mwifiex/fw.h
> @@ -220,6 +220,7 @@ enum MWIFIEX_802_11_PRIVACY_FILTER {
>  #define TLV_TYPE_BSS_MODE   (PROPRIETARY_TLV_BASE_ID + 206)
>  #define TLV_TYPE_RANDOM_MAC (PROPRIETARY_TLV_BASE_ID + 236)
>  #define TLV_TYPE_CHAN_ATTR_CFG  (PROPRIETARY_TLV_BASE_ID + 237)
> +#define TLV_TYPE_MAX_CONN   (PROPRIETARY_TLV_BASE_ID + 279)
>  
>  #define MWIFIEX_TX_DATA_BUF_SIZE_2K2048
>  
> @@ -2388,4 +2389,11 @@ struct mwifiex_opt_sleep_confirm {
>   __le16 action;
>   __le16 resp_ctrl;
>  } __packed;
> +
> +struct hw_spec_max_conn {
> + struct mwifiex_ie_types_header header;
> + u8 max_p2p_conn;
> + u8 max_sta_conn;
> +} __packed;
> +
>  #endif /* !_MWIFIEX_FW_H_ */
> diff --git a/drivers/net/wireless/marvell/mwifiex/main.h 
> b/drivers/net/wireless/marvell/mwifiex/main.h
> index afaffc325.

Re: [EXT] mwifiex: Firmware name for W8997 sdio wifi chip

[Pali Rohár]

On Friday 29 May 2020 09:11:08 Ganapathi Bhat wrote:
> Hi Pali,
>  
> > Hello Ganapathi! Seems that on both locations is older version of
> > sdsd8997_combo_v4.bin firmware, not the latest one. On both location is
> > available just version 16.68.1.p179. But we have newer version 16.68.1.p197.
> > Could you please recheck it?
> 
> p179 do have the fix but we will try to upstream p197 also soon.

Thank you!

Re: [EXT] mwifiex: Firmware name for W8997 sdio wifi chip

[Pali Rohár]

On Friday 29 May 2020 08:49:18 Ganapathi Bhat wrote:
> Hi Pali,
>  
> > Hello Ganapathi! Thank you for information. Can you point me to git tree or
> > location where are firmware files already updated?
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git
> 
> or
> 
> https://github.com/NXP/mwifiex-firmware

Hello Ganapathi! Seems that on both locations is older version of
sdsd8997_combo_v4.bin firmware, not the latest one. On both location is
available just version 16.68.1.p179. But we have newer version
16.68.1.p197. Could you please recheck it?

Re: [EXT] mwifiex: Firmware name for W8997 sdio wifi chip

[Pali Rohár]

On Friday 29 May 2020 08:43:56 Ganapathi Bhat wrote:
> Hi Pali,
> 
> > According to publicly available information, firmware for these W8xxx
> > Marvell wifi chips is vulnerable to security issue CVE-2019-6496 [1].
> > 
> > Are you able to update firmwares to the last versions and give us some
> > information which (old) firmware versions are affected by that security 
> > issue?
> > 
> > So Linux distribution would know if they are distributing older vulnerable
> > firmwares and should push security fixes with new firmware versions.
> > 
> 
> 88W8787, 88W8797, 88W8801, 88W8897, and 88W8997 models are all already 
> updated, with one exception:
> usb8897_uapsta.bin, which we will upstream soon;

Hello Ganapathi! Thank you for information. Can you point me to git tree
or location where are firmware files already updated?

Re: [PATCH] PCI: aardvark: Don't touch PCIe registers if no card connected

[Pali Rohár]

On Thursday 28 May 2020 11:49:38 Bjorn Helgaas wrote:
> On Thu, May 28, 2020 at 06:38:09PM +0200, Pali Rohár wrote:
> > On Thursday 28 May 2020 11:26:04 Bjorn Helgaas wrote:
> > > On Thu, May 28, 2020 at 04:31:41PM +0200, Pali Rohár wrote:
> > > > When there is no PCIe card connected and advk_pcie_rd_conf() or
> > > > advk_pcie_wr_conf() is called for PCI bus which doesn't belong to 
> > > > emulated
> > > > root bridge, the aardvark driver throws the following error message:
> > > > 
> > > >   advk-pcie d007.pcie: config read/write timed out
> > > > 
> > > > Obviously accessing PCIe registers of disconnected card is not possible.
> > > > 
> > > > Extend check in advk_pcie_valid_device() function for validating
> > > > availability of PCIe bus. If PCIe link is down, then the device is 
> > > > marked
> > > > as Not Found and the driver does not try to access these registers.
> > > > 
> > > > Signed-off-by: Pali Rohár 
> > > > ---
> > > >  drivers/pci/controller/pci-aardvark.c | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > > 
> > > > diff --git a/drivers/pci/controller/pci-aardvark.c 
> > > > b/drivers/pci/controller/pci-aardvark.c
> > > > index 90ff291c24f0..53a4cfd7d377 100644
> > > > --- a/drivers/pci/controller/pci-aardvark.c
> > > > +++ b/drivers/pci/controller/pci-aardvark.c
> > > > @@ -644,6 +644,9 @@ static bool advk_pcie_valid_device(struct advk_pcie 
> > > > *pcie, struct pci_bus *bus,
> > > > if ((bus->number == pcie->root_bus_nr) && PCI_SLOT(devfn) != 0)
> > > > return false;
> > > >  
> > > > +   if (bus->number != pcie->root_bus_nr && 
> > > > !advk_pcie_link_up(pcie))
> > > > +   return false;
> > > 
> > > I don't think this is the right fix.  This makes it racy because the
> > > link may go down after we call advk_pcie_valid_device() but before we
> > > perform the config read.
> > 
> > Yes, it is racy, but I do not think it cause problems. Trying to read
> > PCIe registers when device is not connected cause just those timeouts,
> > printing error message and increased delay in advk_pcie_wait_pio() due
> > to polling loop. This patch reduce unnecessary access to PCIe registers
> > when advk_pcie_wait_pio() polling just fail.
> > 
> > I think it is a good idea to not call blocking advk_pcie_wait_pio() when
> > it is not needed. We could have faster enumeration of PCIe buses when
> > card is not connected.
> 
> Maybe advk_pcie_check_pio_status() and advk_pcie_wait_pio() could be
> combined so we could get the correct error status as soon as it's
> available, without waiting for a timeout?

Any idea how to achieve it?

First call is polling function advk_pcie_wait_pio() and second call is
advk_pcie_check_pio_status() which just reads status register and prints
error message to dmesg.

So for me it looks like that combining these two functions into one does
not change anything. We always need to call polling code prior to
checking status register. And therefore need to wait for timeout. Unless
something like in this proposed patch is not used (to skip whole
register access if it would fail).

> In any event, the "return PCIBIOS_SET_FAILED" needs to be fixed.  Most
> callers of config read do not check for failure, but most of the ones
> that do, check for "val == ~0".  Only a few check for a status of
> other than PCIBIOS_SUCCESSFUL.
> 
> > > I have no objection to removing the "config read/write timed out"
> > > message.  The "return PCIBIOS_SET_FAILED" in the read case probably
> > > should be augmented by setting "*val = 0x".

Now I see, "*val = 0x" should be really set when function
advk_pcie_rd_conf() fails.

> > > > return true;
> > > >  }
> > > >  
> > > > -- 
> > > > 2.20.1
> > > > 

Re: [PATCH] PCI: aardvark: Don't touch PCIe registers if no card connected

[Pali Rohár]

On Thursday 28 May 2020 11:26:04 Bjorn Helgaas wrote:
> On Thu, May 28, 2020 at 04:31:41PM +0200, Pali Rohár wrote:
> > When there is no PCIe card connected and advk_pcie_rd_conf() or
> > advk_pcie_wr_conf() is called for PCI bus which doesn't belong to emulated
> > root bridge, the aardvark driver throws the following error message:
> > 
> >   advk-pcie d007.pcie: config read/write timed out
> > 
> > Obviously accessing PCIe registers of disconnected card is not possible.
> > 
> > Extend check in advk_pcie_valid_device() function for validating
> > availability of PCIe bus. If PCIe link is down, then the device is marked
> > as Not Found and the driver does not try to access these registers.
> > 
> > Signed-off-by: Pali Rohár 
> > ---
> >  drivers/pci/controller/pci-aardvark.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/pci/controller/pci-aardvark.c 
> > b/drivers/pci/controller/pci-aardvark.c
> > index 90ff291c24f0..53a4cfd7d377 100644
> > --- a/drivers/pci/controller/pci-aardvark.c
> > +++ b/drivers/pci/controller/pci-aardvark.c
> > @@ -644,6 +644,9 @@ static bool advk_pcie_valid_device(struct advk_pcie 
> > *pcie, struct pci_bus *bus,
> > if ((bus->number == pcie->root_bus_nr) && PCI_SLOT(devfn) != 0)
> > return false;
> >  
> > +   if (bus->number != pcie->root_bus_nr && !advk_pcie_link_up(pcie))
> > +   return false;
> 
> I don't think this is the right fix.  This makes it racy because the
> link may go down after we call advk_pcie_valid_device() but before we
> perform the config read.

Yes, it is racy, but I do not think it cause problems. Trying to read
PCIe registers when device is not connected cause just those timeouts,
printing error message and increased delay in advk_pcie_wait_pio() due
to polling loop. This patch reduce unnecessary access to PCIe registers
when advk_pcie_wait_pio() polling just fail.

I think it is a good idea to not call blocking advk_pcie_wait_pio() when
it is not needed. We could have faster enumeration of PCIe buses when
card is not connected.

> I have no objection to removing the "config read/write timed out"
> message.  The "return PCIBIOS_SET_FAILED" in the read case probably
> should be augmented by setting "*val = 0x".
> 
> > return true;
> >  }
> >  
> > -- 
> > 2.20.1
> > 

[PATCH] PCI: aardvark: Don't touch PCIe registers if no card connected

[Pali Rohár]

When there is no PCIe card connected and advk_pcie_rd_conf() or
advk_pcie_wr_conf() is called for PCI bus which doesn't belong to emulated
root bridge, the aardvark driver throws the following error message:

  advk-pcie d007.pcie: config read/write timed out

Obviously accessing PCIe registers of disconnected card is not possible.

Extend check in advk_pcie_valid_device() function for validating
availability of PCIe bus. If PCIe link is down, then the device is marked
as Not Found and the driver does not try to access these registers.

Signed-off-by: Pali Rohár 
---
 drivers/pci/controller/pci-aardvark.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/pci/controller/pci-aardvark.c 
b/drivers/pci/controller/pci-aardvark.c
index 90ff291c24f0..53a4cfd7d377 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -644,6 +644,9 @@ static bool advk_pcie_valid_device(struct advk_pcie *pcie, 
struct pci_bus *bus,
if ((bus->number == pcie->root_bus_nr) && PCI_SLOT(devfn) != 0)
return false;
 
+   if (bus->number != pcie->root_bus_nr && !advk_pcie_link_up(pcie))
+   return false;
+
return true;
 }
 
-- 
2.20.1

Re: [EXT] mwifiex: Firmware name for W8997 sdio wifi chip

[Pali Rohár]

On Saturday 16 May 2020 08:17:17 Ganapathi Bhat wrote:
> Hi Pali,
> 
> Thanks for this notice. We will try to push the new firmware and also, fix 
> the naming problem.
> 
> Regards,
> Ganapathi

Hello Ganapathi!

According to publicly available information, firmware for these W8xxx
Marvell wifi chips is vulnerable to security issue CVE-2019-6496 [1].

Are you able to update firmwares to the last versions and give us some
information which (old) firmware versions are affected by that security
issue?

So Linux distribution would know if they are distributing older
vulnerable firmwares and should push security fixes with new firmware
versions.

[1] - https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2019-6496

Re: [PATCH] mwifiex: Parse all API_VER_ID properties

[Pali Rohár]

On Thursday 21 May 2020 14:34:44 Pali Rohár wrote:
> During initialization of SD8997 wifi chip kernel prints warnings:
> 
>   mwifiex_sdio mmc0:0001:1: Unknown api_id: 3
>   mwifiex_sdio mmc0:0001:1: Unknown api_id: 4
> 
> This patch adds support for parsing all api ids provided by SD8997
> firmware.
> 
> Signed-off-by: Pali Rohár 
> ---
>  drivers/net/wireless/marvell/mwifiex/cmdevt.c | 17 +++--
>  drivers/net/wireless/marvell/mwifiex/fw.h |  2 ++
>  2 files changed, 17 insertions(+), 2 deletions(-)
> 

Hello! Could you please look at this trivial patch?

> diff --git a/drivers/net/wireless/marvell/mwifiex/cmdevt.c 
> b/drivers/net/wireless/marvell/mwifiex/cmdevt.c
> index 7e4b8cd52..589cc5eb1 100644
> --- a/drivers/net/wireless/marvell/mwifiex/cmdevt.c
> +++ b/drivers/net/wireless/marvell/mwifiex/cmdevt.c
> @@ -1581,8 +1581,21 @@ int mwifiex_ret_get_hw_spec(struct mwifiex_private 
> *priv,
>   adapter->fw_api_ver =
>   api_rev->major_ver;
>   mwifiex_dbg(adapter, INFO,
> - "Firmware api version %d\n",
> - adapter->fw_api_ver);
> + "Firmware api version 
> %d.%d\n",
> + adapter->fw_api_ver,
> + api_rev->minor_ver);
> + break;
> + case UAP_FW_API_VER_ID:
> + mwifiex_dbg(adapter, INFO,
> + "uAP api version %d.%d\n",
> + api_rev->major_ver,
> + api_rev->minor_ver);
> + break;
> + case CHANRPT_API_VER_ID:
> + mwifiex_dbg(adapter, INFO,
> + "channel report api version 
> %d.%d\n",
> + api_rev->major_ver,
> + api_rev->minor_ver);
>   break;
>   default:
>   mwifiex_dbg(adapter, FATAL,
> diff --git a/drivers/net/wireless/marvell/mwifiex/fw.h 
> b/drivers/net/wireless/marvell/mwifiex/fw.h
> index a415d73a7..6f86f5b96 100644
> --- a/drivers/net/wireless/marvell/mwifiex/fw.h
> +++ b/drivers/net/wireless/marvell/mwifiex/fw.h
> @@ -1052,6 +1052,8 @@ struct host_cmd_ds_802_11_ps_mode_enh {
>  enum API_VER_ID {
>   KEY_API_VER_ID = 1,
>   FW_API_VER_ID = 2,
> + UAP_FW_API_VER_ID = 3,
> + CHANRPT_API_VER_ID = 4,
>  };
>  
>  struct hw_spec_api_rev {
> -- 
> 2.20.1
> 

[PATCH v2 1/2] mmc: core: Do not export MMC_NAME= and MODALIAS=mmc:block for SDIO cards

[Pali Rohár]

SDIO non-combo cards are not handled by mmc_block driver and do not have
accessible CID register which is used for MMC_NAME= construction.

Signed-off-by: Pali Rohár 
Reviewed-by: Marek Behún 

---
Changes in V2:
* Use early returns pattern
---
 drivers/mmc/core/bus.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c
index 74de3f2dd..b1cb447da 100644
--- a/drivers/mmc/core/bus.c
+++ b/drivers/mmc/core/bus.c
@@ -93,6 +93,13 @@ mmc_bus_uevent(struct device *dev, struct kobj_uevent_env 
*env)
return retval;
}
 
+   /*
+* SDIO (non-combo) cards are not handled by mmc_block driver and do not
+* have accessible CID register which used by mmc_card_name() function.
+*/
+   if (card->type == MMC_TYPE_SDIO)
+   return 0;
+
retval = add_uevent_var(env, "MMC_NAME=%s", mmc_card_name(card));
if (retval)
return retval;
-- 
2.20.1

[PATCH v2 2/2] mmc: core: Export device/vendor ids from Common CIS for SDIO cards

[Pali Rohár]

Device/vendor ids from Common CIS (Card Information Structure) may be
different as device/vendor ids from CIS on particular SDIO function.

Kernel currently exports only device/vendor ids from SDIO functions and not
"main" device/vendor ids from Common CIS.

This patch exports "main" device/vendor ids for SDIO and SD combo cards at
top level mmc device in sysfs hierarchy.

Userspace can use e.g. udev rules to correctly match whole SDIO card based
on Common CIS device/vendor id and not only one particular SDIO function.
Having this information in userspace also helps developers to debug whole
SDIO card as e.g. kernel mmc quirks use device/vendor ids from Common CIS
and not from particular SDIO function. Also it allows to write userspace
applications which list all connected SDIO cards based on CIS ids.

Signed-off-by: Pali Rohár 
Reviewed-by: Marek Behún 

---
Changes in V2:
* Make sd_std_group static
* Put more information into commit message
---
 drivers/mmc/core/bus.c  |  7 +++
 drivers/mmc/core/sd.c   | 26 +-
 drivers/mmc/core/sdio.c | 20 +++-
 3 files changed, 51 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c
index b1cb447da..70207f11a 100644
--- a/drivers/mmc/core/bus.c
+++ b/drivers/mmc/core/bus.c
@@ -93,6 +93,13 @@ mmc_bus_uevent(struct device *dev, struct kobj_uevent_env 
*env)
return retval;
}
 
+   if (card->type == MMC_TYPE_SDIO || card->type == MMC_TYPE_SD_COMBO) {
+   retval = add_uevent_var(env, "SDIO_ID=%04X:%04X",
+   card->cis.vendor, card->cis.device);
+   if (retval)
+   return retval;
+   }
+
/*
 * SDIO (non-combo) cards are not handled by mmc_block driver and do not
 * have accessible CID register which used by mmc_card_name() function.
diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
index 76c7add36..ee1a51ff6 100644
--- a/drivers/mmc/core/sd.c
+++ b/drivers/mmc/core/sd.c
@@ -707,7 +707,12 @@ static ssize_t mmc_dsr_show(struct device *dev,
 
 static DEVICE_ATTR(dsr, S_IRUGO, mmc_dsr_show, NULL);
 
+MMC_DEV_ATTR(vendor, "0x%04x\n", card->cis.vendor);
+MMC_DEV_ATTR(device, "0x%04x\n", card->cis.device);
+
 static struct attribute *sd_std_attrs[] = {
+   &dev_attr_vendor.attr,
+   &dev_attr_device.attr,
&dev_attr_cid.attr,
&dev_attr_csd.attr,
&dev_attr_scr.attr,
@@ -726,7 +731,26 @@ static struct attribute *sd_std_attrs[] = {
&dev_attr_dsr.attr,
NULL,
 };
-ATTRIBUTE_GROUPS(sd_std);
+
+static umode_t sd_std_is_visible(struct kobject *kobj, struct attribute *attr,
+int index)
+{
+   struct device *dev = container_of(kobj, struct device, kobj);
+   struct mmc_card *card = mmc_dev_to_card(dev);
+
+   /* CIS vendor and device ids are available only for Combo cards */
+   if ((attr == &dev_attr_vendor.attr || attr == &dev_attr_device.attr) &&
+   card->type != MMC_TYPE_SD_COMBO)
+   return 0;
+
+   return attr->mode;
+}
+
+static const struct attribute_group sd_std_group = {
+   .attrs = sd_std_attrs,
+   .is_visible = sd_std_is_visible,
+};
+__ATTRIBUTE_GROUPS(sd_std);
 
 struct device_type sd_type = {
.groups = sd_std_groups,
diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index ebb387aa5..2d86a9db5 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -27,6 +27,24 @@
 #include "sdio_ops.h"
 #include "sdio_cis.h"
 
+MMC_DEV_ATTR(vendor, "0x%04x\n", card->cis.vendor);
+MMC_DEV_ATTR(device, "0x%04x\n", card->cis.device);
+MMC_DEV_ATTR(ocr, "0x%08x\n", card->ocr);
+MMC_DEV_ATTR(rca, "0x%04x\n", card->rca);
+
+static struct attribute *sdio_std_attrs[] = {
+   &dev_attr_vendor.attr,
+   &dev_attr_device.attr,
+   &dev_attr_ocr.attr,
+   &dev_attr_rca.attr,
+   NULL,
+};
+ATTRIBUTE_GROUPS(sdio_std);
+
+static struct device_type sdio_type = {
+   .groups = sdio_std_groups,
+};
+
 static int sdio_read_fbr(struct sdio_func *func)
 {
int ret;
@@ -598,7 +616,7 @@ static int mmc_sdio_init_card(struct mmc_host *host, u32 
ocr,
/*
 * Allocate card structure.
 */
-   card = mmc_alloc_card(host, NULL);
+   card = mmc_alloc_card(host, &sdio_type);
if (IS_ERR(card)) {
err = PTR_ERR(card);
goto err;
-- 
2.20.1

Re: [PATCH 2/2] mmc: core: Export device/vendor ids from Common CIS for SDIO cards

[Pali Rohár]

On Wednesday 27 May 2020 09:39:50 Ulf Hansson wrote:
> On Tue, 26 May 2020 at 17:43, Pali Rohár  wrote:
> >
> > Device/vendor ids from Common CIS (Card Information Structure) may be
> > different as device/vendor ids from CIS on particular SDIO function.
> >
> > Export these "main" device/vendor ids for SDIO and SD combo cards at top
> > level mmc device in sysfs so userspace can do better identification of
> > connected SDIO and SD combo cards.
> 
> What would userspace do with this information, more exactly?

Userspace can e.g. write udev rules based on Common CIS vendor/device
id. Or can exactly identify SDIO card by CIS vendor/device id. Also it
can be suitable for "lssdio" tool to print all information about SDIO
card.

Currently I do not know any way how userspace can retrieve these ids for
particular SDIO card. And correct identification is important.

Other important thing is that kernel on some places (e.g. mmc quirks)
uses Common CIS vendor/device id and on other places (e.g. binding
drivers) it uses SDIO function device/vendor ids.

So for debugging and developing kernel drivers it is needed to know
correct Common CIS vendor/device id and SDIO functions vendor/device
ids.

> Isn't it just sufficient to give events per SDIO func, as we already
> do in sdio_bus_uevent()?

No because some SDIO cards have different Common CIS vendor/device id
and different vendor/device ids for particular SDIO functions.

Common CIS vendor/device id is the "main" identification of SDIO card,
functions vendor/device ids just identify one of those functions.

For example look at my patch "mmc: sdio: Fix macro name for Marvell
device with ID 0x9134" [1]. Without knowing correct CIS vendor/device id
I was not able to fully understand problem and mess with names and ids.
Because mmc quirks list uses CIS vendor/device ids (I guess for obvious
reason as SDIO functions are not enumerated yet).

[1] - https://lore.kernel.org/linux-mmc/20200522144412.19712-2-p...@kernel.org/

> Kind regards
> Uffe
> 
> >
> > Signed-off-by: Pali Rohár 
> > Reviewed-by: Marek Behún 
> > ---
> >  drivers/mmc/core/bus.c  |  7 +++
> >  drivers/mmc/core/sd.c   | 26 +-
> >  drivers/mmc/core/sdio.c | 20 +++-
> >  3 files changed, 51 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c
> > index 103eea7cd..5d4b28b29 100644
> > --- a/drivers/mmc/core/bus.c
> > +++ b/drivers/mmc/core/bus.c
> > @@ -93,6 +93,13 @@ mmc_bus_uevent(struct device *dev, struct 
> > kobj_uevent_env *env)
> > return retval;
> > }
> >
> > +   if (card->type == MMC_TYPE_SDIO || card->type == MMC_TYPE_SD_COMBO) 
> > {
> > +   retval = add_uevent_var(env, "SDIO_ID=%04X:%04X",
> > +   card->cis.vendor, card->cis.device);
> > +   if (retval)
> > +   return retval;
> > +   }
> > +
> > if (card->type != MMC_TYPE_SDIO) {
> > retval = add_uevent_var(env, "MMC_NAME=%s", 
> > mmc_card_name(card));
> > if (retval)
> > diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
> > index 76c7add36..ee1a51ff6 100644
> > --- a/drivers/mmc/core/sd.c
> > +++ b/drivers/mmc/core/sd.c
> > @@ -707,7 +707,12 @@ static ssize_t mmc_dsr_show(struct device *dev,
> >
> >  static DEVICE_ATTR(dsr, S_IRUGO, mmc_dsr_show, NULL);
> >
> > +MMC_DEV_ATTR(vendor, "0x%04x\n", card->cis.vendor);
> > +MMC_DEV_ATTR(device, "0x%04x\n", card->cis.device);
> > +
> >  static struct attribute *sd_std_attrs[] = {
> > +   &dev_attr_vendor.attr,
> > +   &dev_attr_device.attr,
> > &dev_attr_cid.attr,
> > &dev_attr_csd.attr,
> > &dev_attr_scr.attr,
> > @@ -726,7 +731,26 @@ static struct attribute *sd_std_attrs[] = {
> > &dev_attr_dsr.attr,
> > NULL,
> >  };
> > -ATTRIBUTE_GROUPS(sd_std);
> > +
> > +static umode_t sd_std_is_visible(struct kobject *kobj, struct attribute 
> > *attr,
> > +int index)
> > +{
> > +   struct device *dev = container_of(kobj, struct device, kobj);
> > +   struct mmc_card *card = mmc_dev_to_card(dev);
> > +
> > +   /* CIS vendor and device ids are available only for Combo cards */
> > +   if ((attr == &dev_attr_vendor.attr || attr == 
> > &dev_attr_device.attr) &&
> > + 

Re: [RFC] power: supply: bq27xxx_battery: Fix polling interval after re-bind

[Pali Rohár]

On Tuesday 26 May 2020 21:16:28 Andrew F. Davis wrote:
> On 5/25/20 7:32 AM, Krzysztof Kozlowski wrote:
> > This reverts commit 8cfaaa811894a3ae2d7360a15a6cfccff3ebc7db.
> > 
> > If device was unbound and bound, the polling interval would be set to 0.
> > This is both unexpected and messes up with other bq27xxx devices (if
> > more than one battery device is used).
> > 
> > This reset of polling interval was added in commit 8cfaaa811894
> > ("bq27x00_battery: Fix OOPS caused by unregistring bq27x00 driver")
> > stating that power_supply_unregister() calls get_property().  However in
> > Linux kernel v3.1 and newer, such call trace does not exist.
> > Unregistering power supply does not call get_property() on unregistered
> > power supply.
> > 
> > Fixes: 8cfaaa811894 ("bq27x00_battery: Fix OOPS caused by unregistring 
> > bq27x00 driver")
> > Cc: 
> > Signed-off-by: Krzysztof Kozlowski 
> > 
> > ---
> > 
> > I really could not identify the issue being fixed in offending commit
> > 8cfaaa811894 ("bq27x00_battery: Fix OOPS caused by unregistring bq27x00
> > driver"), therefore maybe I missed here something important.
> > 
> > Please share your thoughts on this.
> 
> 
> I'm having a hard time finding the OOPS also. Maybe there is a window
> where the poll function is running or about to run where
> cancel_delayed_work_sync() is called and cancels the work, only to have
> an interrupt or late get_property call in to the poll function and
> re-schedule it.
> 
> What we really need is to do is look at how we are handling the polling
> function. It gets called from the workqueue, from a threaded interrupt
> context, and from a power supply framework callback, possibly all at the
> same time. Sometimes its protected by a lock, sometimes not. Updating
> the device's cached data should always be locked.
> 
> What's more is the poll function is self-arming, so if we call
> cancel_delayed_work_sync() (remove it from the work queue then then wait
> for it to finish if running), are we sure it wont have just re-arm itself?
> 
> We should make the only way we call the poll function be through the
> work queue, (plus make sure all accesses to the cache are locked).
> 
> Andrew

I do not remember details too. It is long time ago.

CCing Ivaylo Dimitrov as he may remember something...

> 
> > ---
> >  drivers/power/supply/bq27xxx_battery.c | 8 
> >  1 file changed, 8 deletions(-)
> > 
> > diff --git a/drivers/power/supply/bq27xxx_battery.c 
> > b/drivers/power/supply/bq27xxx_battery.c
> > index 942c92127b6d..4c94ee72de95 100644
> > --- a/drivers/power/supply/bq27xxx_battery.c
> > +++ b/drivers/power/supply/bq27xxx_battery.c
> > @@ -1905,14 +1905,6 @@ EXPORT_SYMBOL_GPL(bq27xxx_battery_setup);
> >  
> >  void bq27xxx_battery_teardown(struct bq27xxx_device_info *di)
> >  {
> > -   /*
> > -* power_supply_unregister call bq27xxx_battery_get_property which
> > -* call bq27xxx_battery_poll.
> > -* Make sure that bq27xxx_battery_poll will not call
> > -* schedule_delayed_work again after unregister (which cause OOPS).
> > -*/
> > -   poll_interval = 0;
> > -
> > cancel_delayed_work_sync(&di->work);
> >  
> > power_supply_unregister(di->bat);
> > 

[PATCH 2/2] mmc: core: Export device/vendor ids from Common CIS for SDIO cards

[Pali Rohár]

Device/vendor ids from Common CIS (Card Information Structure) may be
different as device/vendor ids from CIS on particular SDIO function.

Export these "main" device/vendor ids for SDIO and SD combo cards at top
level mmc device in sysfs so userspace can do better identification of
connected SDIO and SD combo cards.

Signed-off-by: Pali Rohár 
Reviewed-by: Marek Behún 
---
 drivers/mmc/core/bus.c  |  7 +++
 drivers/mmc/core/sd.c   | 26 +-
 drivers/mmc/core/sdio.c | 20 +++-
 3 files changed, 51 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c
index 103eea7cd..5d4b28b29 100644
--- a/drivers/mmc/core/bus.c
+++ b/drivers/mmc/core/bus.c
@@ -93,6 +93,13 @@ mmc_bus_uevent(struct device *dev, struct kobj_uevent_env 
*env)
return retval;
}
 
+   if (card->type == MMC_TYPE_SDIO || card->type == MMC_TYPE_SD_COMBO) {
+   retval = add_uevent_var(env, "SDIO_ID=%04X:%04X",
+   card->cis.vendor, card->cis.device);
+   if (retval)
+   return retval;
+   }
+
if (card->type != MMC_TYPE_SDIO) {
retval = add_uevent_var(env, "MMC_NAME=%s", 
mmc_card_name(card));
if (retval)
diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
index 76c7add36..ee1a51ff6 100644
--- a/drivers/mmc/core/sd.c
+++ b/drivers/mmc/core/sd.c
@@ -707,7 +707,12 @@ static ssize_t mmc_dsr_show(struct device *dev,
 
 static DEVICE_ATTR(dsr, S_IRUGO, mmc_dsr_show, NULL);
 
+MMC_DEV_ATTR(vendor, "0x%04x\n", card->cis.vendor);
+MMC_DEV_ATTR(device, "0x%04x\n", card->cis.device);
+
 static struct attribute *sd_std_attrs[] = {
+   &dev_attr_vendor.attr,
+   &dev_attr_device.attr,
&dev_attr_cid.attr,
&dev_attr_csd.attr,
&dev_attr_scr.attr,
@@ -726,7 +731,26 @@ static struct attribute *sd_std_attrs[] = {
&dev_attr_dsr.attr,
NULL,
 };
-ATTRIBUTE_GROUPS(sd_std);
+
+static umode_t sd_std_is_visible(struct kobject *kobj, struct attribute *attr,
+int index)
+{
+   struct device *dev = container_of(kobj, struct device, kobj);
+   struct mmc_card *card = mmc_dev_to_card(dev);
+
+   /* CIS vendor and device ids are available only for Combo cards */
+   if ((attr == &dev_attr_vendor.attr || attr == &dev_attr_device.attr) &&
+   card->type != MMC_TYPE_SD_COMBO)
+   return 0;
+
+   return attr->mode;
+}
+
+static const struct attribute_group sd_std_group = {
+   .attrs = sd_std_attrs,
+   .is_visible = sd_std_is_visible,
+};
+__ATTRIBUTE_GROUPS(sd_std);
 
 struct device_type sd_type = {
.groups = sd_std_groups,
diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index ebb387aa5..d708e0fbc 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -27,6 +27,24 @@
 #include "sdio_ops.h"
 #include "sdio_cis.h"
 
+MMC_DEV_ATTR(vendor, "0x%04x\n", card->cis.vendor);
+MMC_DEV_ATTR(device, "0x%04x\n", card->cis.device);
+MMC_DEV_ATTR(ocr, "0x%08x\n", card->ocr);
+MMC_DEV_ATTR(rca, "0x%04x\n", card->rca);
+
+static struct attribute *sdio_std_attrs[] = {
+   &dev_attr_vendor.attr,
+   &dev_attr_device.attr,
+   &dev_attr_ocr.attr,
+   &dev_attr_rca.attr,
+   NULL,
+};
+ATTRIBUTE_GROUPS(sdio_std);
+
+struct device_type sdio_type = {
+   .groups = sdio_std_groups,
+};
+
 static int sdio_read_fbr(struct sdio_func *func)
 {
int ret;
@@ -598,7 +616,7 @@ static int mmc_sdio_init_card(struct mmc_host *host, u32 
ocr,
/*
 * Allocate card structure.
 */
-   card = mmc_alloc_card(host, NULL);
+   card = mmc_alloc_card(host, &sdio_type);
if (IS_ERR(card)) {
err = PTR_ERR(card);
goto err;
-- 
2.20.1

[PATCH 1/2] mmc: core: Do not export MMC_NAME= and MODALIAS=mmc:block for SDIO cards

[Pali Rohár]

SDIO non-combo cards are not handled by mmc_block driver and do not have
accessible CID register which is used for MMC_NAME= construction.

Signed-off-by: Pali Rohár 
Reviewed-by: Marek Behún 
---
 drivers/mmc/core/bus.c | 23 ++-
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c
index 74de3f2dd..103eea7cd 100644
--- a/drivers/mmc/core/bus.c
+++ b/drivers/mmc/core/bus.c
@@ -93,15 +93,20 @@ mmc_bus_uevent(struct device *dev, struct kobj_uevent_env 
*env)
return retval;
}
 
-   retval = add_uevent_var(env, "MMC_NAME=%s", mmc_card_name(card));
-   if (retval)
-   return retval;
-
-   /*
-* Request the mmc_block device.  Note: that this is a direct request
-* for the module it carries no information as to what is inserted.
-*/
-   retval = add_uevent_var(env, "MODALIAS=mmc:block");
+   if (card->type != MMC_TYPE_SDIO) {
+   retval = add_uevent_var(env, "MMC_NAME=%s", 
mmc_card_name(card));
+   if (retval)
+   return retval;
+
+   /*
+* Request the mmc_block device.
+* Note: that this is a direct request for the module it carries
+* no information as to what is inserted.
+*/
+   retval = add_uevent_var(env, "MODALIAS=mmc:block");
+   if (retval)
+   return retval;
+   }
 
return retval;
 }
-- 
2.20.1

[PATCH 04/11] mmc: sdio: Move SDIO IDs from btmrvl driver to common include file

[Pali Rohár]

Define appropriate macro names for consistency with other Marvell macros.

Signed-off-by: Pali Rohár 
---
 drivers/bluetooth/btmrvl_sdio.c | 18 +-
 include/linux/mmc/sdio_ids.h|  8 
 2 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/drivers/bluetooth/btmrvl_sdio.c b/drivers/bluetooth/btmrvl_sdio.c
index 0f3a020703ab..a296f8526433 100644
--- a/drivers/bluetooth/btmrvl_sdio.c
+++ b/drivers/bluetooth/btmrvl_sdio.c
@@ -355,31 +355,31 @@ static const struct btmrvl_sdio_device btmrvl_sdio_sd8997 
= {
 
 static const struct sdio_device_id btmrvl_sdio_ids[] = {
/* Marvell SD8688 Bluetooth device */
-   { SDIO_DEVICE(SDIO_VENDOR_ID_MARVELL, 0x9105),
+   { SDIO_DEVICE(SDIO_VENDOR_ID_MARVELL, SDIO_DEVICE_ID_MARVELL_8688_BT),
.driver_data = (unsigned long)&btmrvl_sdio_sd8688 },
/* Marvell SD8787 Bluetooth device */
-   { SDIO_DEVICE(SDIO_VENDOR_ID_MARVELL, 0x911A),
+   { SDIO_DEVICE(SDIO_VENDOR_ID_MARVELL, SDIO_DEVICE_ID_MARVELL_8787_BT),
.driver_data = (unsigned long)&btmrvl_sdio_sd8787 },
/* Marvell SD8787 Bluetooth AMP device */
-   { SDIO_DEVICE(SDIO_VENDOR_ID_MARVELL, 0x911B),
+   { SDIO_DEVICE(SDIO_VENDOR_ID_MARVELL, 
SDIO_DEVICE_ID_MARVELL_8787_BT_AMP),
.driver_data = (unsigned long)&btmrvl_sdio_sd8787 },
/* Marvell SD8797 Bluetooth device */
-   { SDIO_DEVICE(SDIO_VENDOR_ID_MARVELL, 0x912A),
+   { SDIO_DEVICE(SDIO_VENDOR_ID_MARVELL, SDIO_DEVICE_ID_MARVELL_8797_BT),
.driver_data = (unsigned long)&btmrvl_sdio_sd8797 },
/* Marvell SD8887 Bluetooth device */
-   { SDIO_DEVICE(SDIO_VENDOR_ID_MARVELL, 0x9136),
+   { SDIO_DEVICE(SDIO_VENDOR_ID_MARVELL, SDIO_DEVICE_ID_MARVELL_8887_BT),
.driver_data = (unsigned long)&btmrvl_sdio_sd8887 },
/* Marvell SD8897 Bluetooth device */
-   { SDIO_DEVICE(SDIO_VENDOR_ID_MARVELL, 0x912E),
+   { SDIO_DEVICE(SDIO_VENDOR_ID_MARVELL, SDIO_DEVICE_ID_MARVELL_8897_BT),
.driver_data = (unsigned long)&btmrvl_sdio_sd8897 },
/* Marvell SD8977 Bluetooth device */
-   { SDIO_DEVICE(SDIO_VENDOR_ID_MARVELL, 0x9146),
+   { SDIO_DEVICE(SDIO_VENDOR_ID_MARVELL, SDIO_DEVICE_ID_MARVELL_8977_BT),
.driver_data = (unsigned long)&btmrvl_sdio_sd8977 },
/* Marvell SD8987 Bluetooth device */
-   { SDIO_DEVICE(SDIO_VENDOR_ID_MARVELL, 0x914A),
+   { SDIO_DEVICE(SDIO_VENDOR_ID_MARVELL, SDIO_DEVICE_ID_MARVELL_8987_BT),
.driver_data = (unsigned long)&btmrvl_sdio_sd8987 },
/* Marvell SD8997 Bluetooth device */
-   { SDIO_DEVICE(SDIO_VENDOR_ID_MARVELL, 0x9142),
+   { SDIO_DEVICE(SDIO_VENDOR_ID_MARVELL, SDIO_DEVICE_ID_MARVELL_8997_BT),
.driver_data = (unsigned long)&btmrvl_sdio_sd8997 },
 
{ } /* Terminating entry */
diff --git a/include/linux/mmc/sdio_ids.h b/include/linux/mmc/sdio_ids.h
index 90361ea7f5ed..1237e1048d06 100644
--- a/include/linux/mmc/sdio_ids.h
+++ b/include/linux/mmc/sdio_ids.h
@@ -60,16 +60,24 @@
 #define SDIO_DEVICE_ID_MARVELL_8688_BT 0x9105
 #define SDIO_DEVICE_ID_MARVELL_8786_WLAN   0x9116
 #define SDIO_DEVICE_ID_MARVELL_8787_WLAN   0x9119
+#define SDIO_DEVICE_ID_MARVELL_8787_BT 0x911a
+#define SDIO_DEVICE_ID_MARVELL_8787_BT_AMP 0x911b
 #define SDIO_DEVICE_ID_MARVELL_8797_F0 0x9128
 #define SDIO_DEVICE_ID_MARVELL_8797_WLAN   0x9129
+#define SDIO_DEVICE_ID_MARVELL_8797_BT 0x912a
 #define SDIO_DEVICE_ID_MARVELL_8897_WLAN   0x912d
+#define SDIO_DEVICE_ID_MARVELL_8897_BT 0x912e
 #define SDIO_DEVICE_ID_MARVELL_8887_F0 0x9134
 #define SDIO_DEVICE_ID_MARVELL_8887_WLAN   0x9135
+#define SDIO_DEVICE_ID_MARVELL_8887_BT 0x9136
 #define SDIO_DEVICE_ID_MARVELL_8801_WLAN   0x9139
 #define SDIO_DEVICE_ID_MARVELL_8997_F0 0x9140
 #define SDIO_DEVICE_ID_MARVELL_8997_WLAN   0x9141
+#define SDIO_DEVICE_ID_MARVELL_8997_BT 0x9142
 #define SDIO_DEVICE_ID_MARVELL_8977_WLAN   0x9145
+#define SDIO_DEVICE_ID_MARVELL_8977_BT 0x9146
 #define SDIO_DEVICE_ID_MARVELL_8987_WLAN   0x9149
+#define SDIO_DEVICE_ID_MARVELL_8987_BT 0x914a
 
 #define SDIO_VENDOR_ID_MEDIATEK0x037a
 
-- 
2.20.1

[PATCH 11/11] mmc: sdio: Sort all SDIO IDs in common include file

[Pali Rohár]

Fix ordering of SDIO IDs to conform to the comment above, which says vendor
first, device next.

Signed-off-by: Pali Rohár 
---
 include/linux/mmc/sdio_ids.h | 43 ++--
 1 file changed, 22 insertions(+), 21 deletions(-)

diff --git a/include/linux/mmc/sdio_ids.h b/include/linux/mmc/sdio_ids.h
index b19200aea56a..15ed8ce9d394 100644
--- a/include/linux/mmc/sdio_ids.h
+++ b/include/linux/mmc/sdio_ids.h
@@ -25,9 +25,23 @@
  * Vendors and devices.  Sort key: vendor first, device next.
  */
 
+#define SDIO_VENDOR_ID_STE 0x0020
+#define SDIO_DEVICE_ID_STE_CW1200  0x2280
+
+#define SDIO_VENDOR_ID_INTEL   0x0089
+#define SDIO_DEVICE_ID_INTEL_IWMC3200WIMAX 0x1402
+#define SDIO_DEVICE_ID_INTEL_IWMC3200WIFI  0x1403
+#define SDIO_DEVICE_ID_INTEL_IWMC3200TOP   0x1404
+#define SDIO_DEVICE_ID_INTEL_IWMC3200GPS   0x1405
+#define SDIO_DEVICE_ID_INTEL_IWMC3200BT0x1406
+#define SDIO_DEVICE_ID_INTEL_IWMC3200WIMAX_2G5 0x1407
+
 #define SDIO_VENDOR_ID_CGUYS   0x0092
 #define SDIO_DEVICE_ID_CGUYS_EW_CG1102GC   0x0004
 
+#define SDIO_VENDOR_ID_TI  0x0097
+#define SDIO_DEVICE_ID_TI_WL1271   0x4076
+
 #define SDIO_VENDOR_ID_ATHEROS 0x0271
 #define SDIO_DEVICE_ID_ATHEROS_AR6003_00   0x0300
 #define SDIO_DEVICE_ID_ATHEROS_AR6003_01   0x0301
@@ -41,34 +55,26 @@
 
 #define SDIO_VENDOR_ID_BROADCOM0x02d0
 #define SDIO_DEVICE_ID_BROADCOM_NINTENDO_WII   0x044b
-#define SDIO_DEVICE_ID_BROADCOM_43143  0xa887
 #define SDIO_DEVICE_ID_BROADCOM_43241  0x4324
 #define SDIO_DEVICE_ID_BROADCOM_4329   0x4329
 #define SDIO_DEVICE_ID_BROADCOM_4330   0x4330
 #define SDIO_DEVICE_ID_BROADCOM_4334   0x4334
-#define SDIO_DEVICE_ID_BROADCOM_43340  0xa94c
-#define SDIO_DEVICE_ID_BROADCOM_43341  0xa94d
 #define SDIO_DEVICE_ID_BROADCOM_4335_4339  0x4335
 #define SDIO_DEVICE_ID_BROADCOM_4339   0x4339
-#define SDIO_DEVICE_ID_BROADCOM_43362  0xa962
-#define SDIO_DEVICE_ID_BROADCOM_43364  0xa9a4
-#define SDIO_DEVICE_ID_BROADCOM_43430  0xa9a6
 #define SDIO_DEVICE_ID_BROADCOM_4345   0x4345
-#define SDIO_DEVICE_ID_BROADCOM_43455  0xa9bf
 #define SDIO_DEVICE_ID_BROADCOM_4354   0x4354
+#define SDIO_DEVICE_ID_BROADCOM_CYPRESS_89359  0x4355
 #define SDIO_DEVICE_ID_BROADCOM_4356   0x4356
 #define SDIO_DEVICE_ID_BROADCOM_4359   0x4359
 #define SDIO_DEVICE_ID_BROADCOM_CYPRESS_4373   0x4373
 #define SDIO_DEVICE_ID_BROADCOM_CYPRESS_43012  0xa804
-#define SDIO_DEVICE_ID_BROADCOM_CYPRESS_89359  0x4355
-
-#define SDIO_VENDOR_ID_INTEL   0x0089
-#define SDIO_DEVICE_ID_INTEL_IWMC3200WIMAX 0x1402
-#define SDIO_DEVICE_ID_INTEL_IWMC3200WIFI  0x1403
-#define SDIO_DEVICE_ID_INTEL_IWMC3200TOP   0x1404
-#define SDIO_DEVICE_ID_INTEL_IWMC3200GPS   0x1405
-#define SDIO_DEVICE_ID_INTEL_IWMC3200BT0x1406
-#define SDIO_DEVICE_ID_INTEL_IWMC3200WIMAX_2G5 0x1407
+#define SDIO_DEVICE_ID_BROADCOM_43143  0xa887
+#define SDIO_DEVICE_ID_BROADCOM_43340  0xa94c
+#define SDIO_DEVICE_ID_BROADCOM_43341  0xa94d
+#define SDIO_DEVICE_ID_BROADCOM_43362  0xa962
+#define SDIO_DEVICE_ID_BROADCOM_43364  0xa9a4
+#define SDIO_DEVICE_ID_BROADCOM_43430  0xa9a6
+#define SDIO_DEVICE_ID_BROADCOM_43455  0xa9bf
 
 #define SDIO_VENDOR_ID_MARVELL 0x02df
 #define SDIO_DEVICE_ID_MARVELL_LIBERTAS0x9103
@@ -112,12 +118,7 @@
 #define SDIO_DEVICE_ID_SIANO_NOVA_A0   0x1100
 #define SDIO_DEVICE_ID_SIANO_STELLAR   0x5347
 
-#define SDIO_VENDOR_ID_TI  0x0097
-#define SDIO_DEVICE_ID_TI_WL1271   0x4076
 #define SDIO_VENDOR_ID_TI_WL1251   0x104c
 #define SDIO_DEVICE_ID_TI_WL1251   0x9066
 
-#define SDIO_VENDOR_ID_STE 0x0020
-#define SDIO_DEVICE_ID_STE_CW1200  0x2280
-
 #endif /* LINUX_MMC_SDIO_IDS_H */
-- 
2.20.1

[PATCH 03/11] mmc: sdio: Move SDIO IDs from mwifiex driver to common include file

[Pali Rohár]

Add _WLAN suffix to macro names for consistency with other Marvell macros.
These IDs represents wlan function of combo bt/wlan cards. Other functions
of these cards have different IDs.

Signed-off-by: Pali Rohár 
---
 drivers/net/wireless/marvell/mwifiex/sdio.c | 38 +
 include/linux/mmc/sdio_ids.h| 10 ++
 2 files changed, 19 insertions(+), 29 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c 
b/drivers/net/wireless/marvell/mwifiex/sdio.c
index 6a2dcb01caf4..a042965962a2 100644
--- a/drivers/net/wireless/marvell/mwifiex/sdio.c
+++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
@@ -480,45 +480,25 @@ static void mwifiex_sdio_coredump(struct device *dev)
schedule_work(&card->work);
 }
 
-/* Device ID for SD8786 */
-#define SDIO_DEVICE_ID_MARVELL_8786   (0x9116)
-/* Device ID for SD8787 */
-#define SDIO_DEVICE_ID_MARVELL_8787   (0x9119)
-/* Device ID for SD8797 */
-#define SDIO_DEVICE_ID_MARVELL_8797   (0x9129)
-/* Device ID for SD8897 */
-#define SDIO_DEVICE_ID_MARVELL_8897   (0x912d)
-/* Device ID for SD8887 */
-#define SDIO_DEVICE_ID_MARVELL_8887   (0x9135)
-/* Device ID for SD8801 */
-#define SDIO_DEVICE_ID_MARVELL_8801   (0x9139)
-/* Device ID for SD8977 */
-#define SDIO_DEVICE_ID_MARVELL_8977   (0x9145)
-/* Device ID for SD8987 */
-#define SDIO_DEVICE_ID_MARVELL_8987   (0x9149)
-/* Device ID for SD8997 */
-#define SDIO_DEVICE_ID_MARVELL_8997   (0x9141)
-
-
 /* WLAN IDs */
 static const struct sdio_device_id mwifiex_ids[] = {
-   {SDIO_DEVICE(SDIO_VENDOR_ID_MARVELL, SDIO_DEVICE_ID_MARVELL_8786),
+   {SDIO_DEVICE(SDIO_VENDOR_ID_MARVELL, SDIO_DEVICE_ID_MARVELL_8786_WLAN),
.driver_data = (unsigned long) &mwifiex_sdio_sd8786},
-   {SDIO_DEVICE(SDIO_VENDOR_ID_MARVELL, SDIO_DEVICE_ID_MARVELL_8787),
+   {SDIO_DEVICE(SDIO_VENDOR_ID_MARVELL, SDIO_DEVICE_ID_MARVELL_8787_WLAN),
.driver_data = (unsigned long) &mwifiex_sdio_sd8787},
-   {SDIO_DEVICE(SDIO_VENDOR_ID_MARVELL, SDIO_DEVICE_ID_MARVELL_8797),
+   {SDIO_DEVICE(SDIO_VENDOR_ID_MARVELL, SDIO_DEVICE_ID_MARVELL_8797_WLAN),
.driver_data = (unsigned long) &mwifiex_sdio_sd8797},
-   {SDIO_DEVICE(SDIO_VENDOR_ID_MARVELL, SDIO_DEVICE_ID_MARVELL_8897),
+   {SDIO_DEVICE(SDIO_VENDOR_ID_MARVELL, SDIO_DEVICE_ID_MARVELL_8897_WLAN),
.driver_data = (unsigned long) &mwifiex_sdio_sd8897},
-   {SDIO_DEVICE(SDIO_VENDOR_ID_MARVELL, SDIO_DEVICE_ID_MARVELL_8887),
+   {SDIO_DEVICE(SDIO_VENDOR_ID_MARVELL, SDIO_DEVICE_ID_MARVELL_8887_WLAN),
.driver_data = (unsigned long)&mwifiex_sdio_sd8887},
-   {SDIO_DEVICE(SDIO_VENDOR_ID_MARVELL, SDIO_DEVICE_ID_MARVELL_8801),
+   {SDIO_DEVICE(SDIO_VENDOR_ID_MARVELL, SDIO_DEVICE_ID_MARVELL_8801_WLAN),
.driver_data = (unsigned long)&mwifiex_sdio_sd8801},
-   {SDIO_DEVICE(SDIO_VENDOR_ID_MARVELL, SDIO_DEVICE_ID_MARVELL_8977),
+   {SDIO_DEVICE(SDIO_VENDOR_ID_MARVELL, SDIO_DEVICE_ID_MARVELL_8977_WLAN),
.driver_data = (unsigned long)&mwifiex_sdio_sd8977},
-   {SDIO_DEVICE(SDIO_VENDOR_ID_MARVELL, SDIO_DEVICE_ID_MARVELL_8987),
+   {SDIO_DEVICE(SDIO_VENDOR_ID_MARVELL, SDIO_DEVICE_ID_MARVELL_8987_WLAN),
.driver_data = (unsigned long)&mwifiex_sdio_sd8987},
-   {SDIO_DEVICE(SDIO_VENDOR_ID_MARVELL, SDIO_DEVICE_ID_MARVELL_8997),
+   {SDIO_DEVICE(SDIO_VENDOR_ID_MARVELL, SDIO_DEVICE_ID_MARVELL_8997_WLAN),
.driver_data = (unsigned long)&mwifiex_sdio_sd8997},
{},
 };
diff --git a/include/linux/mmc/sdio_ids.h b/include/linux/mmc/sdio_ids.h
index 7e992a0e6cc0..90361ea7f5ed 100644
--- a/include/linux/mmc/sdio_ids.h
+++ b/include/linux/mmc/sdio_ids.h
@@ -58,8 +58,18 @@
 #define SDIO_DEVICE_ID_MARVELL_LIBERTAS0x9103
 #define SDIO_DEVICE_ID_MARVELL_8688_WLAN   0x9104
 #define SDIO_DEVICE_ID_MARVELL_8688_BT 0x9105
+#define SDIO_DEVICE_ID_MARVELL_8786_WLAN   0x9116
+#define SDIO_DEVICE_ID_MARVELL_8787_WLAN   0x9119
 #define SDIO_DEVICE_ID_MARVELL_8797_F0 0x9128
+#define SDIO_DEVICE_ID_MARVELL_8797_WLAN   0x9129
+#define SDIO_DEVICE_ID_MARVELL_8897_WLAN   0x912d
 #define SDIO_DEVICE_ID_MARVELL_8887_F0 0x9134
+#define SDIO_DEVICE_ID_MARVELL_8887_WLAN   0x9135
+#define SDIO_DEVICE_ID_MARVELL_8801_WLAN   0x9139
+#define SDIO_DEVICE_ID_MARVELL_8997_F0 0x9140
+#define SDIO_DEVICE_ID_MARVELL_8997_WLAN   0x9141
+#define SDIO_DEVICE_ID_MARVELL_8977_WLAN   0x9145
+#define SDIO_DEVICE_ID_MARVELL_8987_WLAN   0x9149
 
 #define SDIO_VENDOR_ID_MEDIATEK0x037a
 
-- 
2.20.1

[PATCH 02/11] mmc: sdio: Change macro names for Marvell 8688 modules

[Pali Rohár]

Add underscore as separator in Marvell 8688 macro names for better
readability and consistency.

Signed-off-by: Pali Rohár 
---
 drivers/net/wireless/marvell/libertas/if_sdio.c | 2 +-
 include/linux/mmc/sdio_ids.h| 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/marvell/libertas/if_sdio.c 
b/drivers/net/wireless/marvell/libertas/if_sdio.c
index acf61b93b782..44fbd0acb87a 100644
--- a/drivers/net/wireless/marvell/libertas/if_sdio.c
+++ b/drivers/net/wireless/marvell/libertas/if_sdio.c
@@ -65,7 +65,7 @@ static const struct sdio_device_id if_sdio_ids[] = {
{ SDIO_DEVICE(SDIO_VENDOR_ID_MARVELL,
SDIO_DEVICE_ID_MARVELL_LIBERTAS) },
{ SDIO_DEVICE(SDIO_VENDOR_ID_MARVELL,
-   SDIO_DEVICE_ID_MARVELL_8688WLAN) },
+   SDIO_DEVICE_ID_MARVELL_8688_WLAN) },
{ /* end: all zeroes */ },
 };
 
diff --git a/include/linux/mmc/sdio_ids.h b/include/linux/mmc/sdio_ids.h
index 96f43e0dc78f..7e992a0e6cc0 100644
--- a/include/linux/mmc/sdio_ids.h
+++ b/include/linux/mmc/sdio_ids.h
@@ -56,8 +56,8 @@
 
 #define SDIO_VENDOR_ID_MARVELL 0x02df
 #define SDIO_DEVICE_ID_MARVELL_LIBERTAS0x9103
-#define SDIO_DEVICE_ID_MARVELL_8688WLAN0x9104
-#define SDIO_DEVICE_ID_MARVELL_8688BT  0x9105
+#define SDIO_DEVICE_ID_MARVELL_8688_WLAN   0x9104
+#define SDIO_DEVICE_ID_MARVELL_8688_BT 0x9105
 #define SDIO_DEVICE_ID_MARVELL_8797_F0 0x9128
 #define SDIO_DEVICE_ID_MARVELL_8887_F0 0x9134
 
-- 
2.20.1

[PATCH 07/11] mmc: sdio: Move SDIO IDs from ath6kl driver to common include file

[Pali Rohár]

Also replace generic MANUFACTURER macros by proper SDIO IDs macros.

Check for "AR6003 or later" is slightly modified to use SDIO device IDs.
This allows removal of all custom MANUFACTURER macros from ath6kl.

Signed-off-by: Pali Rohár 
---
 drivers/net/wireless/ath/ath6kl/hif.h  |  6 --
 drivers/net/wireless/ath/ath6kl/sdio.c | 17 -
 include/linux/mmc/sdio_ids.h   | 10 ++
 3 files changed, 18 insertions(+), 15 deletions(-)

diff --git a/drivers/net/wireless/ath/ath6kl/hif.h 
b/drivers/net/wireless/ath/ath6kl/hif.h
index dc6bd8cd9b83..c6dafc38936a 100644
--- a/drivers/net/wireless/ath/ath6kl/hif.h
+++ b/drivers/net/wireless/ath/ath6kl/hif.h
@@ -35,12 +35,6 @@
 #define MAX_SCATTER_ENTRIES_PER_REQ  16
 #define MAX_SCATTER_REQ_TRANSFER_SIZE(32 * 1024)
 
-#define MANUFACTURER_ID_AR6003_BASE0x300
-#define MANUFACTURER_ID_AR6004_BASE0x400
-/* SDIO manufacturer ID and Codes */
-#define MANUFACTURER_ID_ATH6KL_BASE_MASK 0xFF00
-#define MANUFACTURER_CODE  0x271   /* Atheros */
-
 /* Mailbox address in SDIO address space */
 #define HIF_MBOX_BASE_ADDR 0x800
 #define HIF_MBOX_WIDTH 0x800
diff --git a/drivers/net/wireless/ath/ath6kl/sdio.c 
b/drivers/net/wireless/ath/ath6kl/sdio.c
index bb50680580f3..6b51a2dceadc 100644
--- a/drivers/net/wireless/ath/ath6kl/sdio.c
+++ b/drivers/net/wireless/ath/ath6kl/sdio.c
@@ -799,8 +799,7 @@ static int ath6kl_sdio_config(struct ath6kl *ar)
 
sdio_claim_host(func);
 
-   if ((ar_sdio->id->device & MANUFACTURER_ID_ATH6KL_BASE_MASK) >=
-   MANUFACTURER_ID_AR6003_BASE) {
+   if (ar_sdio->id->device >= SDIO_DEVICE_ID_ATHEROS_AR6003_00) {
/* enable 4-bit ASYNC interrupt on AR6003 or later */
ret = ath6kl_sdio_func0_cmd52_wr_byte(func->card,
CCCR_SDIO_IRQ_MODE_REG,
@@ -1409,13 +1408,13 @@ static void ath6kl_sdio_remove(struct sdio_func *func)
 }
 
 static const struct sdio_device_id ath6kl_sdio_devices[] = {
-   {SDIO_DEVICE(MANUFACTURER_CODE, (MANUFACTURER_ID_AR6003_BASE | 0x0))},
-   {SDIO_DEVICE(MANUFACTURER_CODE, (MANUFACTURER_ID_AR6003_BASE | 0x1))},
-   {SDIO_DEVICE(MANUFACTURER_CODE, (MANUFACTURER_ID_AR6004_BASE | 0x0))},
-   {SDIO_DEVICE(MANUFACTURER_CODE, (MANUFACTURER_ID_AR6004_BASE | 0x1))},
-   {SDIO_DEVICE(MANUFACTURER_CODE, (MANUFACTURER_ID_AR6004_BASE | 0x2))},
-   {SDIO_DEVICE(MANUFACTURER_CODE, (MANUFACTURER_ID_AR6004_BASE | 0x18))},
-   {SDIO_DEVICE(MANUFACTURER_CODE, (MANUFACTURER_ID_AR6004_BASE | 0x19))},
+   {SDIO_DEVICE(SDIO_VENDOR_ID_ATHEROS, SDIO_DEVICE_ID_ATHEROS_AR6003_00)},
+   {SDIO_DEVICE(SDIO_VENDOR_ID_ATHEROS, SDIO_DEVICE_ID_ATHEROS_AR6003_01)},
+   {SDIO_DEVICE(SDIO_VENDOR_ID_ATHEROS, SDIO_DEVICE_ID_ATHEROS_AR6004_00)},
+   {SDIO_DEVICE(SDIO_VENDOR_ID_ATHEROS, SDIO_DEVICE_ID_ATHEROS_AR6004_01)},
+   {SDIO_DEVICE(SDIO_VENDOR_ID_ATHEROS, SDIO_DEVICE_ID_ATHEROS_AR6004_02)},
+   {SDIO_DEVICE(SDIO_VENDOR_ID_ATHEROS, SDIO_DEVICE_ID_ATHEROS_AR6004_18)},
+   {SDIO_DEVICE(SDIO_VENDOR_ID_ATHEROS, SDIO_DEVICE_ID_ATHEROS_AR6004_19)},
{},
 };
 
diff --git a/include/linux/mmc/sdio_ids.h b/include/linux/mmc/sdio_ids.h
index 9ec675a7ac37..95b67ab7d06a 100644
--- a/include/linux/mmc/sdio_ids.h
+++ b/include/linux/mmc/sdio_ids.h
@@ -24,6 +24,16 @@
 /*
  * Vendors and devices.  Sort key: vendor first, device next.
  */
+
+#define SDIO_VENDOR_ID_ATHEROS 0x0271
+#define SDIO_DEVICE_ID_ATHEROS_AR6003_00   0x0300
+#define SDIO_DEVICE_ID_ATHEROS_AR6003_01   0x0301
+#define SDIO_DEVICE_ID_ATHEROS_AR6004_00   0x0400
+#define SDIO_DEVICE_ID_ATHEROS_AR6004_01   0x0401
+#define SDIO_DEVICE_ID_ATHEROS_AR6004_02   0x0402
+#define SDIO_DEVICE_ID_ATHEROS_AR6004_18   0x0418
+#define SDIO_DEVICE_ID_ATHEROS_AR6004_19   0x0419
+
 #define SDIO_VENDOR_ID_BROADCOM0x02d0
 #define SDIO_DEVICE_ID_BROADCOM_43143  0xa887
 #define SDIO_DEVICE_ID_BROADCOM_43241  0x4324
-- 
2.20.1

[PATCH 08/11] mmc: sdio: Move SDIO IDs from ath10k driver to common include file

[Pali Rohár]

Also replace generic MANUFACTURER macros by proper SDIO IDs macros.

Checks for device IDs are slightly modified to use SDIO device IDs.
This allows removal of all custom MANUFACTURER macros from ath10k.

Signed-off-by: Pali Rohár 
---
 drivers/net/wireless/ath/ath10k/sdio.c | 25 ++---
 drivers/net/wireless/ath/ath10k/sdio.h |  8 
 include/linux/mmc/sdio_ids.h   |  2 ++
 3 files changed, 12 insertions(+), 23 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/sdio.c 
b/drivers/net/wireless/ath/ath10k/sdio.c
index 1f709b65c29b..59e725515041 100644
--- a/drivers/net/wireless/ath/ath10k/sdio.c
+++ b/drivers/net/wireless/ath/ath10k/sdio.c
@@ -1083,10 +1083,10 @@ static void ath10k_sdio_set_mbox_info(struct ath10k *ar)
 
mbox_info->ext_info[0].htc_ext_addr = ATH10K_HIF_MBOX0_EXT_BASE_ADDR;
 
-   dev_id_base = FIELD_GET(QCA_MANUFACTURER_ID_BASE, device);
-   dev_id_chiprev = FIELD_GET(QCA_MANUFACTURER_ID_REV_MASK, device);
+   dev_id_base = (device & 0x0F00);
+   dev_id_chiprev = (device & 0x00FF);
switch (dev_id_base) {
-   case QCA_MANUFACTURER_ID_AR6005_BASE:
+   case (SDIO_DEVICE_ID_ATHEROS_AR6005 & 0x0F00):
if (dev_id_chiprev < 4)
mbox_info->ext_info[0].htc_ext_sz =
ATH10K_HIF_MBOX0_EXT_WIDTH;
@@ -1097,7 +1097,7 @@ static void ath10k_sdio_set_mbox_info(struct ath10k *ar)
mbox_info->ext_info[0].htc_ext_sz =
ATH10K_HIF_MBOX0_EXT_WIDTH_ROME_2_0;
break;
-   case QCA_MANUFACTURER_ID_QCA9377_BASE:
+   case (SDIO_DEVICE_ID_ATHEROS_QCA9377 & 0x0F00):
mbox_info->ext_info[0].htc_ext_sz =
ATH10K_HIF_MBOX0_EXT_WIDTH_ROME_2_0;
break;
@@ -2185,19 +2185,16 @@ static int ath10k_sdio_probe(struct sdio_func *func,
skb_queue_head_init(&ar_sdio->rx_head);
INIT_WORK(&ar_sdio->async_work_rx, ath10k_rx_indication_async_work);
 
-   dev_id_base = FIELD_GET(QCA_MANUFACTURER_ID_BASE, id->device);
-   switch (dev_id_base) {
-   case QCA_MANUFACTURER_ID_AR6005_BASE:
-   case QCA_MANUFACTURER_ID_QCA9377_BASE:
-   ar->dev_id = QCA9377_1_0_DEVICE_ID;
-   break;
-   default:
+   dev_id_base = (id->device & 0x0F00);
+   if (dev_id_base != (SDIO_DEVICE_ID_ATHEROS_AR6005 & 0x0F00) &&
+   dev_id_base != (SDIO_DEVICE_ID_ATHEROS_QCA9377 & 0x0F00)) {
ret = -ENODEV;
ath10k_err(ar, "unsupported device id %u (0x%x)\n",
   dev_id_base, id->device);
goto err_free_wq;
}
 
+   ar->dev_id = QCA9377_1_0_DEVICE_ID;
ar->id.vendor = id->vendor;
ar->id.device = id->device;
 
@@ -2246,10 +2243,8 @@ static void ath10k_sdio_remove(struct sdio_func *func)
 }
 
 static const struct sdio_device_id ath10k_sdio_devices[] = {
-   {SDIO_DEVICE(QCA_MANUFACTURER_CODE,
-(QCA_SDIO_ID_AR6005_BASE | 0xA))},
-   {SDIO_DEVICE(QCA_MANUFACTURER_CODE,
-(QCA_SDIO_ID_QCA9377_BASE | 0x1))},
+   {SDIO_DEVICE(SDIO_VENDOR_ID_ATHEROS, SDIO_DEVICE_ID_ATHEROS_AR6005)},
+   {SDIO_DEVICE(SDIO_VENDOR_ID_ATHEROS, SDIO_DEVICE_ID_ATHEROS_QCA9377)},
{},
 };
 
diff --git a/drivers/net/wireless/ath/ath10k/sdio.h 
b/drivers/net/wireless/ath/ath10k/sdio.h
index 33195f49acab..e8951f9cdb5f 100644
--- a/drivers/net/wireless/ath/ath10k/sdio.h
+++ b/drivers/net/wireless/ath/ath10k/sdio.h
@@ -10,14 +10,6 @@
 
 #define ATH10K_HIF_MBOX_BLOCK_SIZE  256
 
-#define QCA_MANUFACTURER_ID_BASEGENMASK(11, 8)
-#define QCA_MANUFACTURER_ID_AR6005_BASE 0x5
-#define QCA_MANUFACTURER_ID_QCA9377_BASE0x7
-#define QCA_SDIO_ID_AR6005_BASE 0x500
-#define QCA_SDIO_ID_QCA9377_BASE0x700
-#define QCA_MANUFACTURER_ID_REV_MASK0x00FF
-#define QCA_MANUFACTURER_CODE   0x271 /* Qualcomm/Atheros */
-
 #define ATH10K_SDIO_MAX_BUFFER_SIZE 4096 /*Unsure of this 
constant*/
 
 /* Mailbox address in SDIO address space */
diff --git a/include/linux/mmc/sdio_ids.h b/include/linux/mmc/sdio_ids.h
index 95b67ab7d06a..2894f7739acc 100644
--- a/include/linux/mmc/sdio_ids.h
+++ b/include/linux/mmc/sdio_ids.h
@@ -33,6 +33,8 @@
 #define SDIO_DEVICE_ID_ATHEROS_AR6004_02   0x0402
 #define SDIO_DEVICE_ID_ATHEROS_AR6004_18   0x0418
 #define SDIO_DEVICE_ID_ATHEROS_AR6004_19   0x0419
+#define SDIO_DEVICE_ID_ATHEROS_AR6005  0x050A
+#define SDIO_DEVICE_ID_ATHEROS_QCA9377 0x0701
 
 #define SDIO_VENDOR_ID_BROADCOM0x02d0
 #define SDIO_DEVICE_ID_BROADCOM_43143  0xa887
-- 
2.20.1

[PATCH 01/11] mmc: sdio: Fix macro name for Marvell device with ID 0x9134

[Pali Rohár]

Marvell SDIO device ID 0x9134 is used in SDIO Common CIS (Card Information
Structure) and not in SDIO wlan function (with ID 1). SDIO Common CIS is
accessed by function ID 0.

So change this misleading macro name to SDIO_DEVICE_ID_MARVELL_8887_F0 as
it does not refer to wlan function. It refers to function 0.

Wlan module on this SDIO card is available at function ID 1 and is
identified by different SDIO device ID 0x9135. Kernel quirks for SDIO
devices are matched against device ID from SDIO Common CIS. Therefore
device ID used in quirk is correct, just has misleading name.

Signed-off-by: Pali Rohár 
---
 drivers/mmc/core/quirks.h| 2 +-
 include/linux/mmc/sdio_ids.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/core/quirks.h b/drivers/mmc/core/quirks.h
index 3dba15bccce2..472fa2fdcf13 100644
--- a/drivers/mmc/core/quirks.h
+++ b/drivers/mmc/core/quirks.h
@@ -139,7 +139,7 @@ static const struct mmc_fixup sdio_fixup_methods[] = {
SDIO_FIXUP(SDIO_VENDOR_ID_MARVELL, SDIO_DEVICE_ID_MARVELL_8797_F0,
   add_quirk, MMC_QUIRK_BROKEN_IRQ_POLLING),
 
-   SDIO_FIXUP(SDIO_VENDOR_ID_MARVELL, SDIO_DEVICE_ID_MARVELL_8887WLAN,
+   SDIO_FIXUP(SDIO_VENDOR_ID_MARVELL, SDIO_DEVICE_ID_MARVELL_8887_F0,
   add_limit_rate_quirk, 15000),
 
END_FIXUP
diff --git a/include/linux/mmc/sdio_ids.h b/include/linux/mmc/sdio_ids.h
index 2e9a6e4634eb..96f43e0dc78f 100644
--- a/include/linux/mmc/sdio_ids.h
+++ b/include/linux/mmc/sdio_ids.h
@@ -59,7 +59,7 @@
 #define SDIO_DEVICE_ID_MARVELL_8688WLAN0x9104
 #define SDIO_DEVICE_ID_MARVELL_8688BT  0x9105
 #define SDIO_DEVICE_ID_MARVELL_8797_F0 0x9128
-#define SDIO_DEVICE_ID_MARVELL_8887WLAN0x9134
+#define SDIO_DEVICE_ID_MARVELL_8887_F0 0x9134
 
 #define SDIO_VENDOR_ID_MEDIATEK0x037a
 
-- 
2.20.1

[PATCH 09/11] mmc: sdio: Move SDIO IDs from b43-sdio driver to common include file

[Pali Rohár]

Define appropriate macro names for consistency with other macros.

Signed-off-by: Pali Rohár 
---
 drivers/net/wireless/broadcom/b43/sdio.c | 4 ++--
 include/linux/mmc/sdio_ids.h | 4 
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/broadcom/b43/sdio.c 
b/drivers/net/wireless/broadcom/b43/sdio.c
index 881a7938c494..02b0cfd535ab 100644
--- a/drivers/net/wireless/broadcom/b43/sdio.c
+++ b/drivers/net/wireless/broadcom/b43/sdio.c
@@ -180,8 +180,8 @@ static void b43_sdio_remove(struct sdio_func *func)
 }
 
 static const struct sdio_device_id b43_sdio_ids[] = {
-   { SDIO_DEVICE(0x02d0, 0x044b) }, /* Nintendo Wii WLAN daughter card */
-   { SDIO_DEVICE(0x0092, 0x0004) }, /* C-guys, Inc. EW-CG1102GC */
+   { SDIO_DEVICE(SDIO_VENDOR_ID_BROADCOM, 
SDIO_DEVICE_ID_BROADCOM_NINTENDO_WII) },
+   { SDIO_DEVICE(SDIO_VENDOR_ID_CGUYS, SDIO_DEVICE_ID_CGUYS_EW_CG1102GC) },
{ },
 };
 
diff --git a/include/linux/mmc/sdio_ids.h b/include/linux/mmc/sdio_ids.h
index 2894f7739acc..8e7a0683b927 100644
--- a/include/linux/mmc/sdio_ids.h
+++ b/include/linux/mmc/sdio_ids.h
@@ -25,6 +25,9 @@
  * Vendors and devices.  Sort key: vendor first, device next.
  */
 
+#define SDIO_VENDOR_ID_CGUYS   0x0092
+#define SDIO_DEVICE_ID_CGUYS_EW_CG1102GC   0x0004
+
 #define SDIO_VENDOR_ID_ATHEROS 0x0271
 #define SDIO_DEVICE_ID_ATHEROS_AR6003_00   0x0300
 #define SDIO_DEVICE_ID_ATHEROS_AR6003_01   0x0301
@@ -37,6 +40,7 @@
 #define SDIO_DEVICE_ID_ATHEROS_QCA9377 0x0701
 
 #define SDIO_VENDOR_ID_BROADCOM0x02d0
+#define SDIO_DEVICE_ID_BROADCOM_NINTENDO_WII   0x044b
 #define SDIO_DEVICE_ID_BROADCOM_43143  0xa887
 #define SDIO_DEVICE_ID_BROADCOM_43241  0x4324
 #define SDIO_DEVICE_ID_BROADCOM_4329   0x4329
-- 
2.20.1

[PATCH 06/11] mmc: sdio: Move SDIO IDs from smssdio driver to common include file

[Pali Rohár]

Define appropriate macro names for consistency with other Siano macros.

Signed-off-by: Pali Rohár 
---
 drivers/media/mmc/siano/smssdio.c | 10 +-
 include/linux/mmc/sdio_ids.h  |  5 +
 2 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/media/mmc/siano/smssdio.c 
b/drivers/media/mmc/siano/smssdio.c
index def5e93849d2..065b572e0272 100644
--- a/drivers/media/mmc/siano/smssdio.c
+++ b/drivers/media/mmc/siano/smssdio.c
@@ -58,15 +58,15 @@ static const struct sdio_device_id smssdio_ids[] = {
 .driver_data = SMS1XXX_BOARD_SIANO_VEGA},
{SDIO_DEVICE(SDIO_VENDOR_ID_SIANO, SDIO_DEVICE_ID_SIANO_VENICE),
 .driver_data = SMS1XXX_BOARD_SIANO_VEGA},
-   {SDIO_DEVICE(SDIO_VENDOR_ID_SIANO, 0x302),
+   {SDIO_DEVICE(SDIO_VENDOR_ID_SIANO, SDIO_DEVICE_ID_SIANO_MING),
.driver_data = SMS1XXX_BOARD_SIANO_MING},
-   {SDIO_DEVICE(SDIO_VENDOR_ID_SIANO, 0x500),
+   {SDIO_DEVICE(SDIO_VENDOR_ID_SIANO, SDIO_DEVICE_ID_SIANO_PELE),
.driver_data = SMS1XXX_BOARD_SIANO_PELE},
-   {SDIO_DEVICE(SDIO_VENDOR_ID_SIANO, 0x600),
+   {SDIO_DEVICE(SDIO_VENDOR_ID_SIANO, SDIO_DEVICE_ID_SIANO_RIO),
.driver_data = SMS1XXX_BOARD_SIANO_RIO},
-   {SDIO_DEVICE(SDIO_VENDOR_ID_SIANO, 0x700),
+   {SDIO_DEVICE(SDIO_VENDOR_ID_SIANO, SDIO_DEVICE_ID_SIANO_DENVER_2160),
.driver_data = SMS1XXX_BOARD_SIANO_DENVER_2160},
-   {SDIO_DEVICE(SDIO_VENDOR_ID_SIANO, 0x800),
+   {SDIO_DEVICE(SDIO_VENDOR_ID_SIANO, SDIO_DEVICE_ID_SIANO_DENVER_1530),
.driver_data = SMS1XXX_BOARD_SIANO_DENVER_1530},
{ /* end: all zeroes */ },
 };
diff --git a/include/linux/mmc/sdio_ids.h b/include/linux/mmc/sdio_ids.h
index c9aca57d4dea..9ec675a7ac37 100644
--- a/include/linux/mmc/sdio_ids.h
+++ b/include/linux/mmc/sdio_ids.h
@@ -88,6 +88,11 @@
 #define SDIO_DEVICE_ID_SIANO_NICE  0x0202
 #define SDIO_DEVICE_ID_SIANO_VEGA_A0   0x0300
 #define SDIO_DEVICE_ID_SIANO_VENICE0x0301
+#define SDIO_DEVICE_ID_SIANO_MING  0x0302
+#define SDIO_DEVICE_ID_SIANO_PELE  0x0500
+#define SDIO_DEVICE_ID_SIANO_RIO   0x0600
+#define SDIO_DEVICE_ID_SIANO_DENVER_2160   0x0700
+#define SDIO_DEVICE_ID_SIANO_DENVER_1530   0x0800
 #define SDIO_DEVICE_ID_SIANO_NOVA_A0   0x1100
 #define SDIO_DEVICE_ID_SIANO_STELLAR   0x5347
 
-- 
2.20.1

[PATCH 10/11] mmc: sdio: Fix Cypress SDIO IDs macros in common include file

[Pali Rohár]

All macro names for SDIO device IDs are prefixed by vendor name to which
device ID belongs. So for consistency add Broadcom string vendor prefix to
all Cypress macro names as they belong to SDIO Broadcom vendor ID.

Change also Cypress 43012 value from decimal do hexadecimal notation to be
consistent with all other values.

Signed-off-by: Pali Rohár 
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c | 6 +++---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c   | 4 ++--
 include/linux/mmc/sdio_ids.h  | 6 +++---
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c 
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
index b684a5b6d904..a1fdb618cf14 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
@@ -970,9 +970,9 @@ static const struct sdio_device_id brcmf_sdmmc_ids[] = {
BRCMF_SDIO_DEVICE(SDIO_DEVICE_ID_BROADCOM_4354),
BRCMF_SDIO_DEVICE(SDIO_DEVICE_ID_BROADCOM_4356),
BRCMF_SDIO_DEVICE(SDIO_DEVICE_ID_BROADCOM_4359),
-   BRCMF_SDIO_DEVICE(SDIO_DEVICE_ID_CYPRESS_4373),
-   BRCMF_SDIO_DEVICE(SDIO_DEVICE_ID_CYPRESS_43012),
-   BRCMF_SDIO_DEVICE(SDIO_DEVICE_ID_CYPRESS_89359),
+   BRCMF_SDIO_DEVICE(SDIO_DEVICE_ID_BROADCOM_CYPRESS_4373),
+   BRCMF_SDIO_DEVICE(SDIO_DEVICE_ID_BROADCOM_CYPRESS_43012),
+   BRCMF_SDIO_DEVICE(SDIO_DEVICE_ID_BROADCOM_CYPRESS_89359),
{ /* end: all zeroes */ }
 };
 MODULE_DEVICE_TABLE(sdio, brcmf_sdmmc_ids);
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c 
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
index 3a08252f1a53..1c9561665a67 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
@@ -4187,7 +4187,7 @@ static void brcmf_sdio_firmware_callback(struct device 
*dev, int err,
   bus->hostintmask, NULL);
 
switch (sdiod->func1->device) {
-   case SDIO_DEVICE_ID_CYPRESS_4373:
+   case SDIO_DEVICE_ID_BROADCOM_CYPRESS_4373:
brcmf_dbg(INFO, "set F2 watermark to 0x%x*4 bytes\n",
  CY_4373_F2_WATERMARK);
brcmf_sdiod_writeb(sdiod, SBSDIO_WATERMARK,
@@ -4201,7 +4201,7 @@ static void brcmf_sdio_firmware_callback(struct device 
*dev, int err,
   CY_4373_F2_WATERMARK |
   SBSDIO_MESBUSYCTRL_ENAB, &err);
break;
-   case SDIO_DEVICE_ID_CYPRESS_43012:
+   case SDIO_DEVICE_ID_BROADCOM_CYPRESS_43012:
brcmf_dbg(INFO, "set F2 watermark to 0x%x*4 bytes\n",
  CY_43012_F2_WATERMARK);
brcmf_sdiod_writeb(sdiod, SBSDIO_WATERMARK,
diff --git a/include/linux/mmc/sdio_ids.h b/include/linux/mmc/sdio_ids.h
index 8e7a0683b927..b19200aea56a 100644
--- a/include/linux/mmc/sdio_ids.h
+++ b/include/linux/mmc/sdio_ids.h
@@ -58,9 +58,9 @@
 #define SDIO_DEVICE_ID_BROADCOM_4354   0x4354
 #define SDIO_DEVICE_ID_BROADCOM_4356   0x4356
 #define SDIO_DEVICE_ID_BROADCOM_4359   0x4359
-#define SDIO_DEVICE_ID_CYPRESS_43730x4373
-#define SDIO_DEVICE_ID_CYPRESS_43012   43012
-#define SDIO_DEVICE_ID_CYPRESS_89359   0x4355
+#define SDIO_DEVICE_ID_BROADCOM_CYPRESS_4373   0x4373
+#define SDIO_DEVICE_ID_BROADCOM_CYPRESS_43012  0xa804
+#define SDIO_DEVICE_ID_BROADCOM_CYPRESS_89359  0x4355
 
 #define SDIO_VENDOR_ID_INTEL   0x0089
 #define SDIO_DEVICE_ID_INTEL_IWMC3200WIMAX 0x1402
-- 
2.20.1

[PATCH 05/11] mmc: sdio: Move SDIO IDs from btmtksdio driver to common include file

[Pali Rohár]

Define appropriate macro names for consistency with other macros.

Signed-off-by: Pali Rohár 
---
 drivers/bluetooth/btmtksdio.c | 4 ++--
 include/linux/mmc/sdio_ids.h  | 2 ++
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/bluetooth/btmtksdio.c b/drivers/bluetooth/btmtksdio.c
index 519788c442ca..bff095be2f97 100644
--- a/drivers/bluetooth/btmtksdio.c
+++ b/drivers/bluetooth/btmtksdio.c
@@ -51,9 +51,9 @@ static const struct btmtksdio_data mt7668_data = {
 };
 
 static const struct sdio_device_id btmtksdio_table[] = {
-   {SDIO_DEVICE(SDIO_VENDOR_ID_MEDIATEK, 0x7663),
+   {SDIO_DEVICE(SDIO_VENDOR_ID_MEDIATEK, SDIO_DEVICE_ID_MEDIATEK_MT7663),
 .driver_data = (kernel_ulong_t)&mt7663_data },
-   {SDIO_DEVICE(SDIO_VENDOR_ID_MEDIATEK, 0x7668),
+   {SDIO_DEVICE(SDIO_VENDOR_ID_MEDIATEK, SDIO_DEVICE_ID_MEDIATEK_MT7668),
 .driver_data = (kernel_ulong_t)&mt7668_data },
{ } /* Terminating entry */
 };
diff --git a/include/linux/mmc/sdio_ids.h b/include/linux/mmc/sdio_ids.h
index 1237e1048d06..c9aca57d4dea 100644
--- a/include/linux/mmc/sdio_ids.h
+++ b/include/linux/mmc/sdio_ids.h
@@ -80,6 +80,8 @@
 #define SDIO_DEVICE_ID_MARVELL_8987_BT 0x914a
 
 #define SDIO_VENDOR_ID_MEDIATEK0x037a
+#define SDIO_DEVICE_ID_MEDIATEK_MT7663 0x7663
+#define SDIO_DEVICE_ID_MEDIATEK_MT7668 0x7668
 
 #define SDIO_VENDOR_ID_SIANO   0x039a
 #define SDIO_DEVICE_ID_SIANO_NOVA_B0   0x0201
-- 
2.20.1

[PATCH 00/11] mmc: sdio: Move SDIO IDs from drivers to common include file

[Pali Rohár]

Most SDIO IDs are defined in the common include file linux/mmc/sdio_ids.h.
But some drivers define IDs locally or do not use existing macros from the
common include file.

This patch series fixes above inconsistency. It defines missing macro names
and moves all remaining SDIO IDs from drivers to the common include file.
Also some macro names are changed to follow existing naming conventions.

Pali Rohár (11):
  mmc: sdio: Fix macro name for Marvell device with ID 0x9134
  mmc: sdio: Change macro names for Marvell 8688 modules
  mmc: sdio: Move SDIO IDs from mwifiex driver to common include file
  mmc: sdio: Move SDIO IDs from btmrvl driver to common include file
  mmc: sdio: Move SDIO IDs from btmtksdio driver to common include file
  mmc: sdio: Move SDIO IDs from smssdio driver to common include file
  mmc: sdio: Move SDIO IDs from ath6kl driver to common include file
  mmc: sdio: Move SDIO IDs from ath10k driver to common include file
  mmc: sdio: Move SDIO IDs from b43-sdio driver to common include file
  mmc: sdio: Fix Cypress SDIO IDs macros in common include file
  mmc: sdio: Sort all SDIO IDs in common include file

 drivers/bluetooth/btmrvl_sdio.c   | 18 ++--
 drivers/bluetooth/btmtksdio.c |  4 +-
 drivers/media/mmc/siano/smssdio.c | 10 +-
 drivers/mmc/core/quirks.h |  2 +-
 drivers/net/wireless/ath/ath10k/sdio.c| 25 ++---
 drivers/net/wireless/ath/ath10k/sdio.h|  8 --
 drivers/net/wireless/ath/ath6kl/hif.h |  6 --
 drivers/net/wireless/ath/ath6kl/sdio.c| 17 ++--
 drivers/net/wireless/broadcom/b43/sdio.c  |  4 +-
 .../broadcom/brcm80211/brcmfmac/bcmsdh.c  |  6 +-
 .../broadcom/brcm80211/brcmfmac/sdio.c|  4 +-
 .../net/wireless/marvell/libertas/if_sdio.c   |  2 +-
 drivers/net/wireless/marvell/mwifiex/sdio.c   | 38 ++--
 include/linux/mmc/sdio_ids.h  | 94 ++-
 14 files changed, 120 insertions(+), 118 deletions(-)

-- 
2.20.1

[PATCH] mwifiex: Add support for NL80211_ATTR_MAX_AP_ASSOC_STA

[Pali Rohár]

SD8997 firmware sends TLV_TYPE_MAX_CONN with struct hw_spec_max_conn to
inform kernel about maximum number of p2p connections and stations in AP
mode.

During initialization of SD8997 wifi chip kernel prints warning:

  mwifiex_sdio mmc0:0001:1: Unknown GET_HW_SPEC TLV type: 0x217

This patch adds support for parsing TLV_TYPE_MAX_CONN (0x217) and sets
appropriate cfg80211 member 'max_ap_assoc_sta' from retrieved structure.

It allows userspace to retrieve NL80211_ATTR_MAX_AP_ASSOC_STA attribute.

Signed-off-by: Pali Rohár 
---
 drivers/net/wireless/marvell/mwifiex/cfg80211.c |  5 +
 drivers/net/wireless/marvell/mwifiex/cmdevt.c   | 12 
 drivers/net/wireless/marvell/mwifiex/fw.h   |  8 
 drivers/net/wireless/marvell/mwifiex/main.h |  1 +
 4 files changed, 26 insertions(+)

diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c 
b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
index 12bfd653a..7998e91c9 100644
--- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
+++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
@@ -4339,6 +4339,11 @@ int mwifiex_register_cfg80211(struct mwifiex_adapter 
*adapter)
wiphy->iface_combinations = &mwifiex_iface_comb_ap_sta;
wiphy->n_iface_combinations = 1;
 
+   if (adapter->max_sta_conn > adapter->max_p2p_conn)
+   wiphy->max_ap_assoc_sta = adapter->max_sta_conn;
+   else
+   wiphy->max_ap_assoc_sta = adapter->max_p2p_conn;
+
/* Initialize cipher suits */
wiphy->cipher_suites = mwifiex_cipher_suites;
wiphy->n_cipher_suites = ARRAY_SIZE(mwifiex_cipher_suites);
diff --git a/drivers/net/wireless/marvell/mwifiex/cmdevt.c 
b/drivers/net/wireless/marvell/mwifiex/cmdevt.c
index 589cc5eb1..d068b9075 100644
--- a/drivers/net/wireless/marvell/mwifiex/cmdevt.c
+++ b/drivers/net/wireless/marvell/mwifiex/cmdevt.c
@@ -1495,6 +1495,7 @@ int mwifiex_ret_get_hw_spec(struct mwifiex_private *priv,
struct mwifiex_adapter *adapter = priv->adapter;
struct mwifiex_ie_types_header *tlv;
struct hw_spec_api_rev *api_rev;
+   struct hw_spec_max_conn *max_conn;
u16 resp_size, api_id;
int i, left_len, parsed_len = 0;
 
@@ -1604,6 +1605,17 @@ int mwifiex_ret_get_hw_spec(struct mwifiex_private *priv,
break;
}
break;
+   case TLV_TYPE_MAX_CONN:
+   max_conn = (struct hw_spec_max_conn *)tlv;
+   adapter->max_p2p_conn = max_conn->max_p2p_conn;
+   adapter->max_sta_conn = max_conn->max_sta_conn;
+   mwifiex_dbg(adapter, INFO,
+   "max p2p connections: %u\n",
+   adapter->max_p2p_conn);
+   mwifiex_dbg(adapter, INFO,
+   "max sta connections: %u\n",
+   adapter->max_sta_conn);
+   break;
default:
mwifiex_dbg(adapter, FATAL,
"Unknown GET_HW_SPEC TLV type: 
%#x\n",
diff --git a/drivers/net/wireless/marvell/mwifiex/fw.h 
b/drivers/net/wireless/marvell/mwifiex/fw.h
index 6f86f5b96..8047e3078 100644
--- a/drivers/net/wireless/marvell/mwifiex/fw.h
+++ b/drivers/net/wireless/marvell/mwifiex/fw.h
@@ -220,6 +220,7 @@ enum MWIFIEX_802_11_PRIVACY_FILTER {
 #define TLV_TYPE_BSS_MODE   (PROPRIETARY_TLV_BASE_ID + 206)
 #define TLV_TYPE_RANDOM_MAC (PROPRIETARY_TLV_BASE_ID + 236)
 #define TLV_TYPE_CHAN_ATTR_CFG  (PROPRIETARY_TLV_BASE_ID + 237)
+#define TLV_TYPE_MAX_CONN   (PROPRIETARY_TLV_BASE_ID + 279)
 
 #define MWIFIEX_TX_DATA_BUF_SIZE_2K2048
 
@@ -2388,4 +2389,11 @@ struct mwifiex_opt_sleep_confirm {
__le16 action;
__le16 resp_ctrl;
 } __packed;
+
+struct hw_spec_max_conn {
+   struct mwifiex_ie_types_header header;
+   u8 max_p2p_conn;
+   u8 max_sta_conn;
+} __packed;
+
 #endif /* !_MWIFIEX_FW_H_ */
diff --git a/drivers/net/wireless/marvell/mwifiex/main.h 
b/drivers/net/wireless/marvell/mwifiex/main.h
index afaffc325..5923c5c14 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.h
+++ b/drivers/net/wireless/marvell/mwifiex/main.h
@@ -1022,6 +1022,7 @@ struct mwifiex_adapter {
bool ext_scan;
u8 fw_api_ver;
u8 key_api_major_ver, key_api_minor_ver;
+   u8 max_p2p_conn, max_sta_conn;
struct memory_type_mapping *mem_type_mapping_tbl;
u8 num_mem_types;
bool scan_chan_gap_enabled;
-- 
2.20.1

[PATCH] mwifiex: Parse all API_VER_ID properties

[Pali Rohár]

During initialization of SD8997 wifi chip kernel prints warnings:

  mwifiex_sdio mmc0:0001:1: Unknown api_id: 3
  mwifiex_sdio mmc0:0001:1: Unknown api_id: 4

This patch adds support for parsing all api ids provided by SD8997
firmware.

Signed-off-by: Pali Rohár 
---
 drivers/net/wireless/marvell/mwifiex/cmdevt.c | 17 +++--
 drivers/net/wireless/marvell/mwifiex/fw.h |  2 ++
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/cmdevt.c 
b/drivers/net/wireless/marvell/mwifiex/cmdevt.c
index 7e4b8cd52..589cc5eb1 100644
--- a/drivers/net/wireless/marvell/mwifiex/cmdevt.c
+++ b/drivers/net/wireless/marvell/mwifiex/cmdevt.c
@@ -1581,8 +1581,21 @@ int mwifiex_ret_get_hw_spec(struct mwifiex_private *priv,
adapter->fw_api_ver =
api_rev->major_ver;
mwifiex_dbg(adapter, INFO,
-   "Firmware api version %d\n",
-   adapter->fw_api_ver);
+   "Firmware api version 
%d.%d\n",
+   adapter->fw_api_ver,
+   api_rev->minor_ver);
+   break;
+   case UAP_FW_API_VER_ID:
+   mwifiex_dbg(adapter, INFO,
+   "uAP api version %d.%d\n",
+   api_rev->major_ver,
+   api_rev->minor_ver);
+   break;
+   case CHANRPT_API_VER_ID:
+   mwifiex_dbg(adapter, INFO,
+   "channel report api version 
%d.%d\n",
+   api_rev->major_ver,
+   api_rev->minor_ver);
break;
default:
mwifiex_dbg(adapter, FATAL,
diff --git a/drivers/net/wireless/marvell/mwifiex/fw.h 
b/drivers/net/wireless/marvell/mwifiex/fw.h
index a415d73a7..6f86f5b96 100644
--- a/drivers/net/wireless/marvell/mwifiex/fw.h
+++ b/drivers/net/wireless/marvell/mwifiex/fw.h
@@ -1052,6 +1052,8 @@ struct host_cmd_ds_802_11_ps_mode_enh {
 enum API_VER_ID {
KEY_API_VER_ID = 1,
FW_API_VER_ID = 2,
+   UAP_FW_API_VER_ID = 3,
+   CHANRPT_API_VER_ID = 4,
 };
 
 struct hw_spec_api_rev {
-- 
2.20.1

[PATCH] cw1200: Remove local sdio VENDOR and DEVICE id definitions

[Pali Rohár]

They are already present in linux/mmc/sdio_ids.h.

Signed-off-by: Pali Rohár 
---
 drivers/net/wireless/st/cw1200/cw1200_sdio.c | 9 +
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/net/wireless/st/cw1200/cw1200_sdio.c 
b/drivers/net/wireless/st/cw1200/cw1200_sdio.c
index 43e012073dbf..b65ec14136c7 100644
--- a/drivers/net/wireless/st/cw1200/cw1200_sdio.c
+++ b/drivers/net/wireless/st/cw1200/cw1200_sdio.c
@@ -14,6 +14,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 
 #include "cw1200.h"
@@ -48,14 +49,6 @@ struct hwbus_priv {
const struct cw1200_platform_data_sdio *pdata;
 };
 
-#ifndef SDIO_VENDOR_ID_STE
-#define SDIO_VENDOR_ID_STE 0x0020
-#endif
-
-#ifndef SDIO_DEVICE_ID_STE_CW1200
-#define SDIO_DEVICE_ID_STE_CW1200  0x2280
-#endif
-
 static const struct sdio_device_id cw1200_sdio_ids[] = {
{ SDIO_DEVICE(SDIO_VENDOR_ID_STE, SDIO_DEVICE_ID_STE_CW1200) },
{ /* end: all zeroes */ },
-- 
2.20.1

Re: [PATCH] platform/x86: dell-wmi: Ignore keyboard attached / detached events

[Pali Rohár]

On Wednesday 13 May 2020 15:05:44 Hans de Goede wrote:
> Ignore events with a type of 0x0011 and a code of 0xfff2 / 0xfff3,
> this silences the following messages being logged when the keyboard is
> detached / attached on a Dell Venue 11 Pro 7130:
> 
> [   63.621953] dell_wmi: Unknown key with type 0x0011 and code 0xfff2 pressed
> [   70.240558] dell_wmi: Unknown key with type 0x0011 and code 0xfff3 pressed
> 
> Note SW_TABLET_MODE is already reported through the intel_vbtn driver on
> this and other Dell devices, so dell_wmi should not report this too,
> to avoid duplicate events.
> 
> Signed-off-by: Hans de Goede 

Looks good, you can add my:

Acked-by: Pali Rohár 

> ---
>  drivers/platform/x86/dell-wmi.c | 10 ++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
> index 86e8dd6a8b33..c25a4286d766 100644
> --- a/drivers/platform/x86/dell-wmi.c
> +++ b/drivers/platform/x86/dell-wmi.c
> @@ -310,6 +310,16 @@ static const struct key_entry 
> dell_wmi_keymap_type_0011[] = {
>   /* Battery inserted */
>   { KE_IGNORE, 0xfff1, { KEY_RESERVED } },
>  
> + /*
> +  * Detachable keyboard detached / undocked
> +  * Note SW_TABLET_MODE is already reported through the intel_vbtn
> +  * driver for this, so we ignore it.
> +  */
> + { KE_IGNORE, 0xfff2, { KEY_RESERVED } },
> +
> + /* Detachable keyboard attached / docked */
> + { KE_IGNORE, 0xfff3, { KEY_RESERVED } },
> +
>   /* Keyboard backlight level changed */
>   { KE_IGNORE, KBD_LED_OFF_TOKEN,  { KEY_RESERVED } },
>   { KE_IGNORE, KBD_LED_ON_TOKEN,   { KEY_RESERVED } },
> -- 
> 2.26.0
> 

Re: Missing CLOCK_BOOTTIME_RAW?

[Pali Rohár]

On Monday 18 May 2020 14:13:48 Thomas Gleixner wrote:
> Pali Rohár  writes:
> > On Monday 18 May 2020 13:26:14 Thomas Gleixner wrote:
> >> System clock shifted by one hour? You mean DST change?
> >
> > Yes, clock was shifted by one hour.
> >
> >> If chronyd
> >> adjusts that by smoothing the frequency, that's just broken and really
> >> not the kernel's problem.
> >
> > And what other can time synchronization daemon does?
> 
> DST switching is a matter of switching time zones and has absolutely
> nothing to do with frequencies. In fact smearing DST is a blantant
> violation of the timekeeping guarantees.

Problem was that "external source" changed RTC.

But I think this is irrelevant here.

Important was state of machine. Machine was in state that had incorrect
system clock, had running NTP daemon and there was need to somehow make
system clock again correct.

And such situation may happen at any time. Either user unintentionally
change "current time" in his desktop GUI application or call "date"
application with "set" parameter (instead of get), ...

> I've never heard about that before. All I know is that chronyd can be
> configured to smear leap seconds, but that doesn't take 6 hours and does
> not screw with the time accuracy in the range of 20 minutes.
> 
> > Well, I think this is not related to chronyd. Any userspace application
> > may call adjtime(). It is not privilege that only chronyd is allowed to
> > use that syscall.
> 
> Of course not, but the kernel relies on that application behaving
> sanely. If it does not then the time stamps you are complaining about
> are the least of your worries.

I do not thing it is too bad... When I needed to deal in userspace with
time/date/clock I just needed either "current time in UTC" to show it to
user (possible in different timezone and pretty formatted) or I needed
"timestamp since some epoch" suitable for measuring time differences.

For first case I used CLOCK_REALTIME and for second case I used
CLOCK_MONOTONIC_RAW (as it was not affected by adjtime()).

And I would like to know, it is correct to use these two clocks in those
situations?

> > I agree that this is not a kernel problem.
> >
> > But I'm asking, how my userspace application can measure time difference?
> > As I wrote CLOCK_MONITONIC is not suitable as it is affected by those
> > NTP adjustments and that is why I thought that CLOCK_MONITONIC_RAW is
> > better as it is not affected by these NTP problems.
> >
> > But CLOCK_MONITONIC_RAW has a problem that is stopped during system
> > sleep and that is why I thought that CLOCK_BOOTTIME_RAW should be there.
> 
> And how do you make CLOCK_BOOTTIME_RAW accurate? The clock hardware
> can stop accross suspend/resume and the kernel then uses RTC or some
> other hardware to inject the sleep time. That injection is not and
> cannot be correct vs. the raw clock.
> 
> So exposing CLOCK_BOOTTIME_RAW would just provide yet another illusion
> of time.

Now I see. Usage of external source during suspend would just lead to
another problems. So idea about CLOCK_BOOTTIME_RAW is nice but there is
no way how to provide it.

Anyway, what would happen with CLOCK_BOOTTIME when during suspend is
that external RTC source shifted back? Is kernel guarantee that
CLOCK_BOOTTIME is always monotonic even in this case?

Re: Missing CLOCK_BOOTTIME_RAW?

[Pali Rohár]

On Monday 18 May 2020 13:26:14 Thomas Gleixner wrote:
> Pali Rohár  writes:
> > On Saturday 09 May 2020 11:49:27 Thomas Gleixner wrote:
> >> Sure, but what's the problem? The adjustemt is done to make the observed
> >> time as correct as possible.
> >
> > Yes. But correction may take lot of time, e.g. also more then one day.
> >
> > So during this period when correction is in progress, measuring time
> > difference via CLOCK_MONITONIC will have incorrect results.
> >
> > It already happened for me, system clock was shifted by one hour and
> > chronyd started adjustment, it slow down system clock. 6 real hours
> > passed and via system clock was measured time difference only about
> > 5 hours and 20 minutes (correction was still in progress as system
> > clock at that time was still shifted by about 20 minutes).
> 
> System clock shifted by one hour? You mean DST change?

Yes, clock was shifted by one hour.

> If chronyd
> adjusts that by smoothing the frequency, that's just broken and really
> not the kernel's problem.

And what other can time synchronization daemon does?

Well, I think this is not related to chronyd. Any userspace application
may call adjtime(). It is not privilege that only chronyd is allowed to
use that syscall.

I agree that this is not a kernel problem.

But I'm asking, how my userspace application can measure time difference?
As I wrote CLOCK_MONITONIC is not suitable as it is affected by those
NTP adjustments and that is why I thought that CLOCK_MONITONIC_RAW is
better as it is not affected by these NTP problems.

But CLOCK_MONITONIC_RAW has a problem that is stopped during system
sleep and that is why I thought that CLOCK_BOOTTIME_RAW should be there.

> Thanks,
> 
> tglx

Re: Missing CLOCK_BOOTTIME_RAW?

[Pali Rohár]

On Saturday 09 May 2020 11:49:27 Thomas Gleixner wrote:
> Pali,
> 
> Pali Rohár  writes:
> > On Friday 08 May 2020 22:59:57 Thomas Gleixner wrote:
> >> Pali Rohár  writes:
> >> Neither CLOCK_BOOTTIME nor CLOCK_MONOTONIC jump. They are frequency
> >> corrected when NTP, PTP or PPS are in use. The frequency correction is
> >> incremental an smothed. They really don't jump and they give you proper
> >> time in nanoseconds which is as close to the real nanoseconds as it
> >> gets.
> >
> > Hello! I should have been more precise about it. CLOCK_BOOTTIME and
> > CLOCK_MONOTONIC do not jump but I understood that they are affected by
> > adjtime(). So these clocks may tick faster or slower than real time. NTP
> > daemon when see that CLOCK_REALTIME is incorrect, it may speed up or
> > slow down its ticking. And this is affected also by CLOCK_BOOTTIME and
> > CLOCK_MONOTONIC, right?
> 
> Sure, but what's the problem? The adjustemt is done to make the observed
> time as correct as possible.

Yes. But correction may take lot of time, e.g. also more then one day.

So during this period when correction is in progress, measuring time
difference via CLOCK_MONITONIC will have incorrect results.

It already happened for me, system clock was shifted by one hour and
chronyd started adjustment, it slow down system clock. 6 real hours
passed and via system clock was measured time difference only about
5 hours and 20 minutes (correction was still in progress as system
clock at that time was still shifted by about 20 minutes).

So measuring time differences via clock affected by NTP adjustment
resulted in error which was more then 30 minutes.

CLOCK_MONOTONIC_RAW is not affected by this correction progress, so it
should gives better results. Or not?

> > You wrote that this clock is subject to drifts. What exactly may happen
> > with CLOCK_MONOTONIC_RAW?
> 
>   1) As the frequency of the raw clock is an estimate, resulting time
>  is drifting apart vs. the correct frequency.
> 
>   2) Depending on the crystal/oszillator there can be temperatur drift as
>  well.
> 
> Just for clarification. Even with NTP/PTP adjustment the resulting time
> stamps are never going to be precise vs. an atomic clock, but good
> enough for 99.% of the problems.

Ok, so it looks like that CLOCK_MONOTONIC_RAW is not the best choice.

> TBH, I have no idea what real world problem you are trying to solve.

Problem is simple: Measure time difference between two events and not to
be affected by the fact that system clock on machine is incorrect or
that time daemon is actually adjusting time.

I need to know if difference between those two events in more then some
period or not.

So if I want to know if time difference is more then 5 hours and 40
minutes then measurement via clock which is affected by NTP adjustment
would give me wrong result. As described above in reality 6 hours
passed but clock measured only 5 hours and 20 minutes.

> Thanks,
> 
> tglx