Re: drivers/staging/qlge/qlge_main.c:4564 qlge_probe() warn: missing error code 'err'
On Mon, Mar 22, 2021 at 06:14:08PM +0300, Dan Carpenter wrote:
On Wed, Mar 10, 2021 at 08:21:37AM +0800, Coiby Xu wrote:
Hi Dan,
Thanks for finding this issue! I'll submit all the patches including the
one for the previous issue reported by you ("[bug report] staging: qlge:
Initialize devlink health dump framework") after finishing all items listed
in drivers/staging/qlge/TODO.
I just sent a patch for this.
Part of my QC process before I send a patch is to search my inbox to
see if I have already sent the patch before. I have the memory of a
gnat and static checker warnings all look the same after a while so I
once wrote the same patch three times before I implemented this as part
of my QC process.
Anyway, it turns out that I did report this bug already but since you
hadn't sent a fix, I decided to just send my fix instead of deleting it.
Thank you for sharing the info about your QC process.
When kbuild detects a bug in your patch then please fix it as soon as
you can. Certainly don't wait until "all the TODO items" are done. (0_0)
Thanks for the suggestion! The reason I haven't sent the patch for this
bug is I haven't tested the patch on a system using this qlge driver.
Red Hat has a system where I can do testing but I need to borrow it and
return it asap. So I want to test all the patches together when finishing
"all the TODO items".
Just do what I do and promise to "fix it tomorrow morning" and then
forget about it forever. ;)
Sorry for keeping you waiting. I'll fix a bug detected by kbuild as soon
as possible next time especially when a fix is unlikely to introduce
another issue.
regards,
dan carpenter
On Thu, Feb 25, 2021 at 01:03:03PM +0300, Dan Carpenter wrote:
> tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
master
> head: 29c395c77a9a514c5857c45ceae2665e9bd99ac7
> commit: 953b94009377419f28fd0153f91fcd5b5a347608 staging: qlge: Initialize
devlink health dump framework
> config: i386-randconfig-m021-20210225 (attached as .config)
> compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot
> Reported-by: Dan Carpenter
>
> smatch warnings:
> drivers/staging/qlge/qlge_main.c:4564 qlge_probe() warn: missing error code
'err'
>
> vim +/err +4564 drivers/staging/qlge/qlge_main.c
>
> 5d8e87265715a1 drivers/net/ethernet/qlogic/qlge/qlge_main.c Bill Pemberton
2012-12-03 4544 static int qlge_probe(struct pci_dev *pdev,
> c4e84bde1d595d drivers/net/qlge/qlge_main.c Ron Mercer
2008-09-18 4545 const struct pci_device_id *pci_entry)
> c4e84bde1d595d drivers/net/qlge/qlge_main.c Ron Mercer
2008-09-18 4546 {
> 953b9400937741 drivers/staging/qlge/qlge_main.c Coiby Xu
2021-01-23 4547struct qlge_netdev_priv *ndev_priv;
> f8c047be540197 drivers/staging/qlge/qlge_main.c Coiby Xu
2021-01-23 4548struct qlge_adapter *qdev = NULL;
> 953b9400937741 drivers/staging/qlge/qlge_main.c Coiby Xu
2021-01-23 4549 struct net_device *ndev = NULL;
> 953b9400937741 drivers/staging/qlge/qlge_main.c Coiby Xu
2021-01-23 4550struct devlink *devlink;
> f41e1a0a9462fc drivers/staging/qlge/qlge_main.c Dorothea Ehrl
2019-11-27 4551static int cards_found;
> c4e84bde1d595d drivers/net/qlge/qlge_main.c Ron Mercer
2008-09-18 4552int err = 0;
> c4e84bde1d595d drivers/net/qlge/qlge_main.c Ron Mercer
2008-09-18 4553
> 953b9400937741 drivers/staging/qlge/qlge_main.c Coiby Xu
2021-01-23 4554devlink = devlink_alloc(_devlink_ops, sizeof(struct
qlge_adapter));
> 953b9400937741 drivers/staging/qlge/qlge_main.c Coiby Xu
2021-01-23 4555if (!devlink)
> 953b9400937741 drivers/staging/qlge/qlge_main.c Coiby Xu
2021-01-23 4556return -ENOMEM;
> 953b9400937741 drivers/staging/qlge/qlge_main.c Coiby Xu
2021-01-23 4557
> 953b9400937741 drivers/staging/qlge/qlge_main.c Coiby Xu
2021-01-23 4558qdev = devlink_priv(devlink);
> 953b9400937741 drivers/staging/qlge/qlge_main.c Coiby Xu
2021-01-23 4559
> 953b9400937741 drivers/staging/qlge/qlge_main.c Coiby Xu
2021-01-23 4560ndev = alloc_etherdev_mq(sizeof(struct qlge_netdev_priv),
> 1b998958b301fb drivers/staging/qlge/qlge_main.c Scott Schafer
2019-12-11 4561 min(MAX_CPUS,
> 1b998958b301fb drivers/staging/qlge/qlge_main.c Scott Schafer
2019-12-11 4562
netif_get_num_default_rss_queues(
[PATCH] staging: qlge: deal with the case that devlink_health_reporter_create fails
From: Coiby Xu
devlink_health_reporter_create may fail. In that case, do the cleanup
work.
Reported-by: Dan Carpenter
Signed-off-by: Coiby Xu
---
drivers/staging/qlge/qlge_devlink.c | 10 +++---
drivers/staging/qlge/qlge_devlink.h | 2 +-
drivers/staging/qlge/qlge_main.c| 8 +++-
3 files changed, 15 insertions(+), 5 deletions(-)
diff --git a/drivers/staging/qlge/qlge_devlink.c
b/drivers/staging/qlge/qlge_devlink.c
index 86834d96cebf..0ab02d6d3817 100644
--- a/drivers/staging/qlge/qlge_devlink.c
+++ b/drivers/staging/qlge/qlge_devlink.c
@@ -148,16 +148,20 @@ static const struct devlink_health_reporter_ops
qlge_reporter_ops = {
.dump = qlge_reporter_coredump,
};
-void qlge_health_create_reporters(struct qlge_adapter *priv)
+long qlge_health_create_reporters(struct qlge_adapter *priv)
{
struct devlink *devlink;
+ long err = 0;
devlink = priv_to_devlink(priv);
priv->reporter =
devlink_health_reporter_create(devlink, _reporter_ops,
0, priv);
- if (IS_ERR(priv->reporter))
+ if (IS_ERR(priv->reporter)) {
+ err = PTR_ERR(priv->reporter);
netdev_warn(priv->ndev,
"Failed to create reporter, err = %ld\n",
- PTR_ERR(priv->reporter));
+ err);
+ }
+ return err;
}
diff --git a/drivers/staging/qlge/qlge_devlink.h
b/drivers/staging/qlge/qlge_devlink.h
index 19078e1ac694..94538e923f2f 100644
--- a/drivers/staging/qlge/qlge_devlink.h
+++ b/drivers/staging/qlge/qlge_devlink.h
@@ -4,6 +4,6 @@
#include
-void qlge_health_create_reporters(struct qlge_adapter *priv);
+long qlge_health_create_reporters(struct qlge_adapter *priv);
#endif /* QLGE_DEVLINK_H */
diff --git a/drivers/staging/qlge/qlge_main.c b/drivers/staging/qlge/qlge_main.c
index 5516be3af898..59d1ec580696 100644
--- a/drivers/staging/qlge/qlge_main.c
+++ b/drivers/staging/qlge/qlge_main.c
@@ -4620,7 +4620,11 @@ static int qlge_probe(struct pci_dev *pdev,
if (err)
goto netdev_free;
- qlge_health_create_reporters(qdev);
+ err = qlge_health_create_reporters(qdev);
+
+ if (err)
+ goto devlink_unregister;
+
/* Start up the timer to trigger EEH if
* the bus goes dead
*/
@@ -4632,6 +4636,8 @@ static int qlge_probe(struct pci_dev *pdev,
cards_found++;
return 0;
+devlink_unregister:
+ devlink_unregister(devlink);
netdev_free:
free_netdev(ndev);
devlink_free:
--
2.31.0
Re: drivers/staging/qlge/qlge_main.c:4564 qlge_probe() warn: missing error code 'err'
Hi Dan,
Thanks for finding this issue! I'll submit all the patches including the
one for the previous issue reported by you ("[bug report] staging: qlge:
Initialize devlink health dump framework") after finishing all items
listed in drivers/staging/qlge/TODO.
On Thu, Feb 25, 2021 at 01:03:03PM +0300, Dan Carpenter wrote:
tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
master
head: 29c395c77a9a514c5857c45ceae2665e9bd99ac7
commit: 953b94009377419f28fd0153f91fcd5b5a347608 staging: qlge: Initialize
devlink health dump framework
config: i386-randconfig-m021-20210225 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
Reported-by: Dan Carpenter
smatch warnings:
drivers/staging/qlge/qlge_main.c:4564 qlge_probe() warn: missing error code
'err'
vim +/err +4564 drivers/staging/qlge/qlge_main.c
5d8e87265715a1 drivers/net/ethernet/qlogic/qlge/qlge_main.c Bill Pemberton
2012-12-03 4544 static int qlge_probe(struct pci_dev *pdev,
c4e84bde1d595d drivers/net/qlge/qlge_main.c Ron Mercer
2008-09-18 4545 const struct pci_device_id *pci_entry)
c4e84bde1d595d drivers/net/qlge/qlge_main.c Ron Mercer
2008-09-18 4546 {
953b9400937741 drivers/staging/qlge/qlge_main.c Coiby Xu
2021-01-23 4547 struct qlge_netdev_priv *ndev_priv;
f8c047be540197 drivers/staging/qlge/qlge_main.c Coiby Xu
2021-01-23 4548 struct qlge_adapter *qdev = NULL;
953b9400937741 drivers/staging/qlge/qlge_main.c Coiby Xu
2021-01-23 4549 struct net_device *ndev = NULL;
953b9400937741 drivers/staging/qlge/qlge_main.c Coiby Xu
2021-01-23 4550 struct devlink *devlink;
f41e1a0a9462fc drivers/staging/qlge/qlge_main.c Dorothea Ehrl
2019-11-27 4551 static int cards_found;
c4e84bde1d595d drivers/net/qlge/qlge_main.c Ron Mercer
2008-09-18 4552 int err = 0;
c4e84bde1d595d drivers/net/qlge/qlge_main.c Ron Mercer
2008-09-18 4553
953b9400937741 drivers/staging/qlge/qlge_main.c Coiby Xu
2021-01-23 4554 devlink = devlink_alloc(_devlink_ops,
sizeof(struct qlge_adapter));
953b9400937741 drivers/staging/qlge/qlge_main.c Coiby Xu
2021-01-23 4555 if (!devlink)
953b9400937741 drivers/staging/qlge/qlge_main.c Coiby Xu
2021-01-23 4556 return -ENOMEM;
953b9400937741 drivers/staging/qlge/qlge_main.c Coiby Xu
2021-01-23 4557
953b9400937741 drivers/staging/qlge/qlge_main.c Coiby Xu
2021-01-23 4558 qdev = devlink_priv(devlink);
953b9400937741 drivers/staging/qlge/qlge_main.c Coiby Xu
2021-01-23 4559
953b9400937741 drivers/staging/qlge/qlge_main.c Coiby Xu
2021-01-23 4560 ndev = alloc_etherdev_mq(sizeof(struct
qlge_netdev_priv),
1b998958b301fb drivers/staging/qlge/qlge_main.c Scott Schafer
2019-12-11 4561 min(MAX_CPUS,
1b998958b301fb drivers/staging/qlge/qlge_main.c Scott Schafer
2019-12-11 4562
netif_get_num_default_rss_queues()));
c4e84bde1d595d drivers/net/qlge/qlge_main.c Ron Mercer
2008-09-18 4563 if (!ndev)
953b9400937741 drivers/staging/qlge/qlge_main.c Coiby Xu
2021-01-23 @4564 goto devlink_free;
"err = -ENOMEM;"
c4e84bde1d595d drivers/net/qlge/qlge_main.c Ron Mercer
2008-09-18 4565
953b9400937741 drivers/staging/qlge/qlge_main.c Coiby Xu
2021-01-23 4566 ndev_priv = netdev_priv(ndev);
953b9400937741 drivers/staging/qlge/qlge_main.c Coiby Xu
2021-01-23 4567 ndev_priv->qdev = qdev;
953b9400937741 drivers/staging/qlge/qlge_main.c Coiby Xu
2021-01-23 4568 ndev_priv->ndev = ndev;
953b9400937741 drivers/staging/qlge/qlge_main.c Coiby Xu
2021-01-23 4569 qdev->ndev = ndev;
953b9400937741 drivers/staging/qlge/qlge_main.c Coiby Xu
2021-01-23 4570 err = qlge_init_device(pdev, qdev, cards_found);
953b9400937741 drivers/staging/qlge/qlge_main.c Coiby Xu
2021-01-23 4571 if (err < 0)
953b9400937741 drivers/staging/qlge/qlge_main.c Coiby Xu
2021-01-23 4572 goto netdev_free;
c4e84bde1d595d drivers/net/qlge/qlge_main.c Ron Mercer
2008-09-18 4573
c4e84bde1d595d drivers/net/qlge/qlge_main.c Ron Mercer
2008-09-18 4574 SET_NETDEV
[PATCH v1 3/3] i40e: use minimal admin queue for kdump
The minimum size of admin send/receive queue is 1 and 2 respectively.
The admin send queue can't be set to 1 because in that case, the
firmware would fail to init.
Signed-off-by: Coiby Xu
---
drivers/net/ethernet/intel/i40e/i40e.h | 2 ++
drivers/net/ethernet/intel/i40e/i40e_main.c | 9 +++--
2 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/intel/i40e/i40e.h
b/drivers/net/ethernet/intel/i40e/i40e.h
index cd53981fa5e0..09217944baa4 100644
--- a/drivers/net/ethernet/intel/i40e/i40e.h
+++ b/drivers/net/ethernet/intel/i40e/i40e.h
@@ -66,6 +66,8 @@
#define I40E_FDIR_RING_COUNT 32
#define I40E_MAX_AQ_BUF_SIZE 4096
#define I40E_AQ_LEN256
+#define I40E_MIN_ARQ_LEN 1
+#define I40E_MIN_ASQ_LEN 2
#define I40E_AQ_WORK_LIMIT 66 /* max number of VFs + a little */
#define I40E_MAX_USER_PRIORITY 8
#define I40E_DEFAULT_TRAFFIC_CLASS BIT(0)
diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c
b/drivers/net/ethernet/intel/i40e/i40e_main.c
index d6868c7aee05..5d67fb12e576 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -15327,8 +15327,13 @@ static int i40e_probe(struct pci_dev *pdev, const
struct pci_device_id *ent)
i40e_check_recovery_mode(pf);
- hw->aq.num_arq_entries = I40E_AQ_LEN;
- hw->aq.num_asq_entries = I40E_AQ_LEN;
+ if (is_kdump_kernel()) {
+ hw->aq.num_arq_entries = I40E_MIN_ARQ_LEN;
+ hw->aq.num_asq_entries = I40E_MIN_ASQ_LEN;
+ } else {
+ hw->aq.num_arq_entries = I40E_AQ_LEN;
+ hw->aq.num_asq_entries = I40E_AQ_LEN;
+ }
hw->aq.arq_buf_size = I40E_MAX_AQ_BUF_SIZE;
hw->aq.asq_buf_size = I40E_MAX_AQ_BUF_SIZE;
pf->adminq_work_limit = I40E_AQ_WORK_LIMIT;
--
2.30.1
[PATCH v1 2/3] i40e: use minimal rx and tx ring buffers for kdump
Use the minimum of the number of descriptors thus we will allocate the
minimal ring buffers for kdump.
Signed-off-by: Coiby Xu
---
drivers/net/ethernet/intel/i40e/i40e_main.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c
b/drivers/net/ethernet/intel/i40e/i40e_main.c
index 77bf8c392750..d6868c7aee05 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -11029,6 +11029,11 @@ static int i40e_set_num_rings_in_vsi(struct i40e_vsi
*vsi)
return -ENODATA;
}
+ if (is_kdump_kernel()) {
+ vsi->num_tx_desc = I40E_MIN_NUM_DESCRIPTORS;
+ vsi->num_rx_desc = I40E_MIN_NUM_DESCRIPTORS;
+ }
+
return 0;
}
--
2.30.1
[PATCH v1 1/3] i40e: use minimal tx and rx pairs for kdump
Set the number of the MSI-X vectors to 1. When MSI-X is enabled, it's not allowed to use more TC queue pairs than MSI-X vectors (pf->num_lan_msix) exist. Thus the number of tx and rx pairs (vsi->num_queue_pairs) will be equal to the number of MSI-X vectors, i.e., 1. Signed-off-by: Coiby Xu --- drivers/net/ethernet/intel/i40e/i40e_main.c | 9 + 1 file changed, 9 insertions(+) diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c index 353deae139f9..77bf8c392750 100644 --- a/drivers/net/ethernet/intel/i40e/i40e_main.c +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c @@ -6,6 +6,7 @@ #include #include #include +#include /* Local includes */ #include "i40e.h" @@ -15485,6 +15486,14 @@ static int i40e_probe(struct pci_dev *pdev, const struct pci_device_id *ent) if (err) goto err_switch_setup; + /* Reduce tx and rx pairs for kdump +* When MSI-X is enabled, it's not allowed to use more TC queue +* pairs than MSI-X vectors (pf->num_lan_msix) exist. Thus +* vsi->num_queue_pairs will be equal to pf->num_lan_msix, i.e., 1. +*/ + if (is_kdump_kernel()) + pf->num_lan_msix = 1; + pf->udp_tunnel_nic.set_port = i40e_udp_tunnel_set_port; pf->udp_tunnel_nic.unset_port = i40e_udp_tunnel_unset_port; pf->udp_tunnel_nic.flags = UDP_TUNNEL_NIC_INFO_MAY_SLEEP; -- 2.30.1
Re: [RFC PATCH 4/4] i40e: don't open i40iw client for kdump
Hi Bhupesh,
Glad to meet you here:)
On Thu, Feb 25, 2021 at 03:41:55PM +0530, Bhupesh SHARMA wrote:
Hello Coiby,
On Mon, Feb 22, 2021 at 12:40 PM Coiby Xu wrote:
i40iw consumes huge amounts of memory. For example, on a x86_64 machine,
i40iw consumed 1.5GB for Intel Corporation Ethernet Connection X722 for
for 1GbE while "craskernel=auto" only reserved 160M. With the module
parameter "resource_profile=2", we can reduce the memory usage of i40iw
to ~300M which is still too much for kdump.
Disabling the client registration would spare us the client interface
operation open , i.e., i40iw_open for iwarp/uda device. Thus memory is
saved for kdump.
Signed-off-by: Coiby Xu
---
drivers/net/ethernet/intel/i40e/i40e_client.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/drivers/net/ethernet/intel/i40e/i40e_client.c
b/drivers/net/ethernet/intel/i40e/i40e_client.c
index a2dba32383f6..aafc2587f389 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_client.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_client.c
@@ -4,6 +4,7 @@
#include
#include
#include
+#include
#include "i40e.h"
#include "i40e_prototype.h"
@@ -741,6 +742,12 @@ int i40e_register_client(struct i40e_client *client)
{
int ret = 0;
+ /* Don't open i40iw client for kdump because i40iw will consume huge
+* amounts of memory.
+*/
+ if (is_kdump_kernel())
+ return ret;
+
Since crashkernel size can be manually set on the command line by a
user, and some users might be fine with a ~300M memory usage by i40iw
client [with resource_profile=2"], in my view, disabling the client
for all kdump cases seems too restrictive.
We can probably check the crash kernel size allocated (
$ cat /sys/kernel/kexec_crash_size) and then make a decision
accordingly, so for example something like:
+ if (is_kdump_kernel() && kexec_crash_size < 512M)
+ return ret;
What do you think?
Thanks for the suggestion! After having a discussion with the team, we
think it's better to not intervene i40iw in the kernel space. Actually
when kexec-tools is building initramfs for kdump, i40iw is not included
by default unless a user explicitly asks to include i40iw by changing
/etc/kdump.conf, i.e., adding 'dracut_args --add-drivers "i40iw"'.
Regards,
Bhupesh
if (!client) {
ret = -EIO;
goto out;
--
2.30.1
___
kexec mailing list
ke...@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
--
Best regards,
Coiby
Re: [RFC PATCH 4/4] i40e: don't open i40iw client for kdump
On Wed, Feb 24, 2021 at 08:48:41AM -0800, Jakub Kicinski wrote: On Wed, 24 Feb 2021 19:41:41 +0800 Coiby Xu wrote: On Tue, Feb 23, 2021 at 12:22:07PM -0800, Jakub Kicinski wrote: >On Mon, 22 Feb 2021 15:07:01 +0800 Coiby Xu wrote: >> i40iw consumes huge amounts of memory. For example, on a x86_64 machine, >> i40iw consumed 1.5GB for Intel Corporation Ethernet Connection X722 for >> for 1GbE while "craskernel=auto" only reserved 160M. With the module >> parameter "resource_profile=2", we can reduce the memory usage of i40iw >> to ~300M which is still too much for kdump. >> >> Disabling the client registration would spare us the client interface >> operation open , i.e., i40iw_open for iwarp/uda device. Thus memory is >> saved for kdump. >> >> Signed-off-by: Coiby Xu > >Is i40iw or whatever the client is not itself under a CONFIG which >kdump() kernels could be reasonably expected to disable? > I'm not sure if I understand you correctly. Do you mean we shouldn't disable i40iw for kdump? Forgive my ignorance - are the kdump kernels separate builds? AFAIK we don't build a kernel exclusively for kdump. If they are it'd be better to leave the choice of enabling RDMA to the user - through appropriate Kconfig options. i40iw is usually built as a loadable module. So if we want to leave the choce of enabling RDMA to the user, we could exclude this driver when building the initramfs for kdump, for example, dracut provides the omit_drivers option for this purpose. On the other hand, the users expect "crashkernel=auto" to work out of the box. So i40iw defeats this purpose. I'll discuss with my Red Hat team and the Intel team about whether RDMA is needed for kdump. Thanks for bringing up this issue! -- Best regards, Coiby
Re: [RFC PATCH 4/4] i40e: don't open i40iw client for kdump
Hi Jakub, Thank you for reviewing the patch! On Tue, Feb 23, 2021 at 12:22:07PM -0800, Jakub Kicinski wrote: On Mon, 22 Feb 2021 15:07:01 +0800 Coiby Xu wrote: i40iw consumes huge amounts of memory. For example, on a x86_64 machine, i40iw consumed 1.5GB for Intel Corporation Ethernet Connection X722 for for 1GbE while "craskernel=auto" only reserved 160M. With the module parameter "resource_profile=2", we can reduce the memory usage of i40iw to ~300M which is still too much for kdump. Disabling the client registration would spare us the client interface operation open , i.e., i40iw_open for iwarp/uda device. Thus memory is saved for kdump. Signed-off-by: Coiby Xu Is i40iw or whatever the client is not itself under a CONFIG which kdump() kernels could be reasonably expected to disable? I'm not sure if I understand you correctly. Do you mean we shouldn't disable i40iw for kdump? -- Best regards, Coiby
[RFC PATCH 4/4] i40e: don't open i40iw client for kdump
i40iw consumes huge amounts of memory. For example, on a x86_64 machine,
i40iw consumed 1.5GB for Intel Corporation Ethernet Connection X722 for
for 1GbE while "craskernel=auto" only reserved 160M. With the module
parameter "resource_profile=2", we can reduce the memory usage of i40iw
to ~300M which is still too much for kdump.
Disabling the client registration would spare us the client interface
operation open , i.e., i40iw_open for iwarp/uda device. Thus memory is
saved for kdump.
Signed-off-by: Coiby Xu
---
drivers/net/ethernet/intel/i40e/i40e_client.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/drivers/net/ethernet/intel/i40e/i40e_client.c
b/drivers/net/ethernet/intel/i40e/i40e_client.c
index a2dba32383f6..aafc2587f389 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_client.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_client.c
@@ -4,6 +4,7 @@
#include
#include
#include
+#include
#include "i40e.h"
#include "i40e_prototype.h"
@@ -741,6 +742,12 @@ int i40e_register_client(struct i40e_client *client)
{
int ret = 0;
+ /* Don't open i40iw client for kdump because i40iw will consume huge
+* amounts of memory.
+*/
+ if (is_kdump_kernel())
+ return ret;
+
if (!client) {
ret = -EIO;
goto out;
--
2.30.1
[RFC PATCH 3/4] i40e: use minimal admin queue for kdump
The minimum size of admin send/receive queue is 1 and 2 respectively.
The admin send queue can't be set to 1 because in that case, the
firmware would fail to init.
Signed-off-by: Coiby Xu
---
drivers/net/ethernet/intel/i40e/i40e.h | 2 ++
drivers/net/ethernet/intel/i40e/i40e_main.c | 9 +++--
2 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/intel/i40e/i40e.h
b/drivers/net/ethernet/intel/i40e/i40e.h
index 118473dfdcbd..e106c33ff958 100644
--- a/drivers/net/ethernet/intel/i40e/i40e.h
+++ b/drivers/net/ethernet/intel/i40e/i40e.h
@@ -66,6 +66,8 @@
#define I40E_FDIR_RING_COUNT 32
#define I40E_MAX_AQ_BUF_SIZE 4096
#define I40E_AQ_LEN256
+#define I40E_MIN_ARQ_LEN 1
+#define I40E_MIN_ASQ_LEN 2
#define I40E_AQ_WORK_LIMIT 66 /* max number of VFs + a little */
#define I40E_MAX_USER_PRIORITY 8
#define I40E_DEFAULT_TRAFFIC_CLASS BIT(0)
diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c
b/drivers/net/ethernet/intel/i40e/i40e_main.c
index 5307f1744766..2fd8db80b585 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -14847,8 +14847,13 @@ static int i40e_probe(struct pci_dev *pdev, const
struct pci_device_id *ent)
i40e_check_recovery_mode(pf);
- hw->aq.num_arq_entries = I40E_AQ_LEN;
- hw->aq.num_asq_entries = I40E_AQ_LEN;
+ if (is_kdump_kernel()) {
+ hw->aq.num_arq_entries = I40E_MIN_ARQ_LEN;
+ hw->aq.num_asq_entries = I40E_MIN_ASQ_LEN;
+ } else {
+ hw->aq.num_arq_entries = I40E_AQ_LEN;
+ hw->aq.num_asq_entries = I40E_AQ_LEN;
+ }
hw->aq.arq_buf_size = I40E_MAX_AQ_BUF_SIZE;
hw->aq.asq_buf_size = I40E_MAX_AQ_BUF_SIZE;
pf->adminq_work_limit = I40E_AQ_WORK_LIMIT;
--
2.30.0
[RFC PATCH 2/4] i40e: use minimal rx and tx ring buffers for kdump
Use the minimum of the number of descriptors thus we will allocate the
minimal ring buffers for kdump.
Signed-off-by: Coiby Xu
---
drivers/net/ethernet/intel/i40e/i40e_main.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c
b/drivers/net/ethernet/intel/i40e/i40e_main.c
index 069c86e2f69d..5307f1744766 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -10552,6 +10552,11 @@ static int i40e_set_num_rings_in_vsi(struct i40e_vsi
*vsi)
return -ENODATA;
}
+ if (is_kdump_kernel()) {
+ vsi->num_tx_desc = I40E_MIN_NUM_DESCRIPTORS;
+ vsi->num_rx_desc = I40E_MIN_NUM_DESCRIPTORS;
+ }
+
return 0;
}
--
2.30.0
[RFC PATCH 1/4] i40e: use minimal tx and rx pairs for kdump
Set the number of the MSI-X vectors to 1. When MSI-X is enabled, it's not allowed to use more TC queue pairs than MSI-X vectors (pf->num_lan_msix) exist. Thus the number of tx and rx pairs (vsi->num_queue_pairs) will be equal to the number of MSI-X vectors, i.e., 1. Signed-off-by: Coiby Xu --- drivers/net/ethernet/intel/i40e/i40e_main.c | 9 + 1 file changed, 9 insertions(+) diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c index 1db482d310c2..069c86e2f69d 100644 --- a/drivers/net/ethernet/intel/i40e/i40e_main.c +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c @@ -6,6 +6,7 @@ #include #include #include +#include /* Local includes */ #include "i40e.h" @@ -15000,6 +15001,14 @@ static int i40e_probe(struct pci_dev *pdev, const struct pci_device_id *ent) if (err) goto err_switch_setup; + /* Reduce tx and rx pairs for kdump +* When MSI-X is enabled, it's not allowed to use more TC queue +* pairs than MSI-X vectors (pf->num_lan_msix) exist. Thus +* vsi->num_queue_pairs will be equal to pf->num_lan_msix, i.e., 1. +*/ + if (is_kdump_kernel()) + pf->num_lan_msix = 1; + pf->udp_tunnel_nic.set_port = i40e_udp_tunnel_set_port; pf->udp_tunnel_nic.unset_port = i40e_udp_tunnel_unset_port; pf->udp_tunnel_nic.flags = UDP_TUNNEL_NIC_INFO_MAY_SLEEP; -- 2.30.0
Re: [PATCH][next] staging: qlge: fix read of an uninitialized pointer
On Wed, Feb 03, 2021 at 01:38:34PM +, Colin King wrote:
From: Colin Ian King
Currently the pointer 'reporter' is not being initialized and is
being read in a netdev_warn message. The pointer is not used
and is redundant, fix this by removing it and replacing the reference
to it with priv->reporter instead.
Addresses-Coverity: ("Uninitialized pointer read")
Fixes: 1053c27804df ("staging: qlge: coredump via devlink health reporter")
Signed-off-by: Colin Ian King
---
drivers/staging/qlge/qlge_devlink.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/staging/qlge/qlge_devlink.c
b/drivers/staging/qlge/qlge_devlink.c
index c6ef5163e241..86834d96cebf 100644
--- a/drivers/staging/qlge/qlge_devlink.c
+++ b/drivers/staging/qlge/qlge_devlink.c
@@ -150,7 +150,6 @@ static const struct devlink_health_reporter_ops
qlge_reporter_ops = {
void qlge_health_create_reporters(struct qlge_adapter *priv)
{
- struct devlink_health_reporter *reporter;
struct devlink *devlink;
devlink = priv_to_devlink(priv);
@@ -160,5 +159,5 @@ void qlge_health_create_reporters(struct qlge_adapter *priv)
if (IS_ERR(priv->reporter))
netdev_warn(priv->ndev,
"Failed to create reporter, err = %ld\n",
- PTR_ERR(reporter));
+ PTR_ERR(priv->reporter));
}
--
2.29.2
Thanks for fixing this issue.
Reviewed-by: Coiby Xu
--
Best regards,
Coiby
[PATCH v4 8/8] staging: qlge: add documentation for debugging qlge
Instructions and examples on kernel data structures dumping and
coredump.
Signed-off-by: Coiby Xu
---
.../networking/device_drivers/index.rst | 1 +
.../device_drivers/qlogic/index.rst | 18 +++
.../networking/device_drivers/qlogic/qlge.rst | 118 ++
MAINTAINERS | 6 +
4 files changed, 143 insertions(+)
create mode 100644 Documentation/networking/device_drivers/qlogic/index.rst
create mode 100644 Documentation/networking/device_drivers/qlogic/qlge.rst
diff --git a/Documentation/networking/device_drivers/index.rst
b/Documentation/networking/device_drivers/index.rst
index a3113ffd7a16..d8279de7bf25 100644
--- a/Documentation/networking/device_drivers/index.rst
+++ b/Documentation/networking/device_drivers/index.rst
@@ -15,6 +15,7 @@ Contents:
ethernet/index
fddi/index
hamradio/index
+ qlogic/index
wan/index
wifi/index
diff --git a/Documentation/networking/device_drivers/qlogic/index.rst
b/Documentation/networking/device_drivers/qlogic/index.rst
new file mode 100644
index ..ad05b04286e4
--- /dev/null
+++ b/Documentation/networking/device_drivers/qlogic/index.rst
@@ -0,0 +1,18 @@
+.. SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+
+QLogic QLGE Device Drivers
+===
+
+Contents:
+
+.. toctree::
+ :maxdepth: 2
+
+ qlge
+
+.. only:: subproject and html
+
+ Indices
+ ===
+
+ * :ref:`genindex`
diff --git a/Documentation/networking/device_drivers/qlogic/qlge.rst
b/Documentation/networking/device_drivers/qlogic/qlge.rst
new file mode 100644
index ..0b888253d152
--- /dev/null
+++ b/Documentation/networking/device_drivers/qlogic/qlge.rst
@@ -0,0 +1,118 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+===
+QLogic QLGE 10Gb Ethernet device driver
+===
+
+This driver use drgn and devlink for debugging.
+
+Dump kernel data structures in drgn
+---
+
+To dump kernel data structures, the following Python script can be used
+in drgn:
+
+.. code-block:: python
+
+ def align(x, a):
+ """the alignment a should be a power of 2
+ """
+ mask = a - 1
+ return (x+ mask) & ~mask
+
+ def struct_size(struct_type):
+ struct_str = "struct {}".format(struct_type)
+ return sizeof(Object(prog, struct_str, address=0x0))
+
+ def netdev_priv(netdevice):
+ NETDEV_ALIGN = 32
+ return netdevice.value_() + align(struct_size("net_device"),
NETDEV_ALIGN)
+
+ name = 'xxx'
+ qlge_device = None
+ netdevices = prog['init_net'].dev_base_head.address_of_()
+ for netdevice in list_for_each_entry("struct net_device", netdevices,
"dev_list"):
+ if netdevice.name.string_().decode('ascii') == name:
+ print(netdevice.name)
+
+ ql_adapter = Object(prog, "struct ql_adapter",
address=netdev_priv(qlge_device))
+
+The struct ql_adapter will be printed in drgn as follows,
+
+>>> ql_adapter
+(struct ql_adapter){
+.ricb = (struct ricb){
+.base_cq = (u8)0,
+.flags = (u8)120,
+.mask = (__le16)26637,
+.hash_cq_id = (u8 [1024]){ 172, 142, 255, 255 },
+.ipv6_hash_key = (__le32 [10]){},
+.ipv4_hash_key = (__le32 [4]){},
+},
+.flags = (unsigned long)0,
+.wol = (u32)0,
+.nic_stats = (struct nic_stats){
+.tx_pkts = (u64)0,
+.tx_bytes = (u64)0,
+.tx_mcast_pkts = (u64)0,
+.tx_bcast_pkts = (u64)0,
+.tx_ucast_pkts = (u64)0,
+.tx_ctl_pkts = (u64)0,
+.tx_pause_pkts = (u64)0,
+...
+},
+.active_vlans = (unsigned long [64]){
+0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 52780853100545, 18446744073709551615,
+18446619461681283072, 0, 42949673024, 2147483647,
+},
+.rx_ring = (struct rx_ring [17]){
+{
+.cqicb = (struct cqicb){
+.msix_vect = (u8)0,
+.reserved1 = (u8)0,
+.reserved2 = (u8)0,
+.flags = (u8)0,
+.len = (__le16)0,
+.rid = (__le16)0,
+...
+},
+
[PATCH v4 4/8] staging: qlge: coredump via devlink health reporter
$ devlink health dump show DEVICE reporter coredump -p -j
{
"Core Registers": {
"segment": 1,
"values": [
0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0
,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0
]
},
"Test Logic Regs": {
"segment": 2,
"values": [ 0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0 ]
},
"RMII Registers": {
"segment": 3,
"values": [
0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0
]
},
...
"Sem Registers": {
"segment": 50,
"values": [ 0,0,0,0 ]
}
}
Signed-off-by: Coiby Xu
---
drivers/staging/qlge/qlge_devlink.c | 132 ++--
1 file changed, 126 insertions(+), 6 deletions(-)
diff --git a/drivers/staging/qlge/qlge_devlink.c
b/drivers/staging/qlge/qlge_devlink.c
index d9c71f45211f..bf7d75ed5eae 100644
--- a/drivers/staging/qlge/qlge_devlink.c
+++ b/drivers/staging/qlge/qlge_devlink.c
@@ -2,16 +2,136 @@
#include "qlge.h"
#include "qlge_devlink.h"
-static int
-qlge_reporter_coredump(struct devlink_health_reporter *reporter,
- struct devlink_fmsg *fmsg, void *priv_ctx,
- struct netlink_ext_ack *extack)
+static int qlge_fill_seg_(struct devlink_fmsg *fmsg,
+ struct mpi_coredump_segment_header *seg_header,
+ u32 *reg_data)
{
- return 0;
+ int regs_num = (seg_header->seg_size
+ - sizeof(struct mpi_coredump_segment_header)) /
sizeof(u32);
+ int err;
+ int i;
+
+ err = devlink_fmsg_pair_nest_start(fmsg, seg_header->description);
+ if (err)
+ return err;
+ err = devlink_fmsg_obj_nest_start(fmsg);
+ if (err)
+ return err;
+ err = devlink_fmsg_u32_pair_put(fmsg, "segment", seg_header->seg_num);
+ if (err)
+ return err;
+ err = devlink_fmsg_arr_pair_nest_start(fmsg, "values");
+ if (err)
+ return err;
+ for (i = 0; i < regs_num; i++) {
+ err = devlink_fmsg_u32_put(fmsg, *reg_data);
+ if (err)
+ return err;
+ reg_data++;
+ }
+ err = devlink_fmsg_obj_nest_end(fmsg);
+ if (err)
+ return err;
+ err = devlink_fmsg_arr_pair_nest_end(fmsg);
+ if (err)
+ return err;
+ err = devlink_fmsg_pair_nest_end(fmsg);
+ return err;
+}
+
+#define FILL_SEG(seg_hdr, seg_regs)\
+ do {\
+ err = qlge_fill_seg_(fmsg, >seg_hdr, dump->seg_regs); \
+ if (err) { \
+ kvfree(dump); \
+ return err; \
+ } \
+ } while (0)
+
+static int qlge_reporter_coredump(struct devlink_health_reporter *reporter,
+ struct devlink_fmsg *fmsg, void *priv_ctx,
+ struct netlink_ext_ack *extack)
+{
+ int err = 0;
+
+ struct qlge_adapter *qdev = devlink_health_reporter_priv(reporter);
+ struct qlge_mpi_coredump *dump;
+
+ if (!netif_running(qdev->ndev))
+ return 0;
+
+ dump = kvmalloc(sizeof(*dump), GFP_KERNEL);
+ if (!dump)
+ return -ENOMEM;
+
+ err = qlge_core_dump(qdev, dump);
+ if (err) {
+ kvfree(dump);
+ return err;
+ }
+
+ qlge_soft_reset_mpi_risc(qdev);
+
+ FILL_SEG(core_regs_seg_hdr, mpi_core_regs);
+ FILL_SEG(test_logic_regs_seg_hdr, test_logic_regs);
+ FILL_SEG(rmii_regs_seg_hdr, rmii_regs);
+ FILL_SEG(fcmac1_regs_seg_hdr, fcmac1_regs);
+ FILL_SEG(fcmac2_regs_seg_hdr, fcmac2_regs);
+ FILL_SEG(fc1_mbx_regs_seg_hdr, fc1_mbx_regs);
+ FILL_SEG(ide_regs_seg_hdr, ide_regs);
+ FILL_SEG(nic1_mbx_regs_seg_hdr, nic1_mbx_regs);
+ FILL_SEG(smbus_regs_seg_hdr, smbus_regs);
+ FILL_SEG(fc2_mbx_regs_seg_hdr, fc2_mbx_regs);
+ FILL_SEG(nic2_mbx_regs_seg_hdr, nic2_mbx_regs);
+ FILL_SEG(i2c_regs_seg_hdr, i2c_regs);
+ FILL_SEG(memc_regs_seg_hdr, memc_regs);
+ FILL_SEG(pbus_regs_seg_hdr, pbus_regs)
[PATCH v4 3/8] staging: qlge: re-write qlge_init_device
Stop calling ql_release_all in qlge_init_device and free things one step
at a time.
struct qlge_adapter *qdev is now a private structure of struct devlink
and memset is not necessary.
Link: https://lore.kernel.org/patchwork/patch/1321092/#1516928
Suggested-by: Dan Carpenter
Signed-off-by: Coiby Xu
---
drivers/staging/qlge/qlge_main.c | 32
1 file changed, 20 insertions(+), 12 deletions(-)
diff --git a/drivers/staging/qlge/qlge_main.c b/drivers/staging/qlge/qlge_main.c
index bb9fc590d97b..2ec688d3d946 100644
--- a/drivers/staging/qlge/qlge_main.c
+++ b/drivers/staging/qlge/qlge_main.c
@@ -4394,13 +4394,13 @@ static int qlge_init_device(struct pci_dev *pdev,
struct qlge_adapter *qdev,
err = pcie_set_readrq(pdev, 4096);
if (err) {
dev_err(>dev, "Set readrq failed.\n");
- goto err_out1;
+ goto err_disable_pci;
}
err = pci_request_regions(pdev, DRV_NAME);
if (err) {
dev_err(>dev, "PCI region request failed.\n");
- return err;
+ goto err_disable_pci;
}
pci_set_master(pdev);
@@ -4416,7 +4416,7 @@ static int qlge_init_device(struct pci_dev *pdev, struct
qlge_adapter *qdev,
if (err) {
dev_err(>dev, "No usable DMA configuration.\n");
- goto err_out2;
+ goto err_release_pci;
}
/* Set PCIe reset type for EEH to fundamental. */
@@ -4427,7 +4427,7 @@ static int qlge_init_device(struct pci_dev *pdev, struct
qlge_adapter *qdev,
if (!qdev->reg_base) {
dev_err(>dev, "Register mapping failed.\n");
err = -ENOMEM;
- goto err_out2;
+ goto err_release_pci;
}
qdev->doorbell_area_size = pci_resource_len(pdev, 3);
@@ -4436,14 +4436,14 @@ static int qlge_init_device(struct pci_dev *pdev,
struct qlge_adapter *qdev,
if (!qdev->doorbell_area) {
dev_err(>dev, "Doorbell register mapping failed.\n");
err = -ENOMEM;
- goto err_out2;
+ goto err_iounmap_base;
}
err = qlge_get_board_info(qdev);
if (err) {
dev_err(>dev, "Register access failed.\n");
err = -EIO;
- goto err_out2;
+ goto err_iounmap_doorbell;
}
qdev->msg_enable = netif_msg_init(debug, default_msg);
spin_lock_init(>stats_lock);
@@ -4453,7 +4453,7 @@ static int qlge_init_device(struct pci_dev *pdev, struct
qlge_adapter *qdev,
vmalloc(sizeof(struct qlge_mpi_coredump));
if (!qdev->mpi_coredump) {
err = -ENOMEM;
- goto err_out2;
+ goto err_iounmap_doorbell;
}
if (qlge_force_coredump)
set_bit(QL_FRC_COREDUMP, >flags);
@@ -4462,7 +4462,7 @@ static int qlge_init_device(struct pci_dev *pdev, struct
qlge_adapter *qdev,
err = qdev->nic_ops->get_flash(qdev);
if (err) {
dev_err(>dev, "Invalid FLASH.\n");
- goto err_out2;
+ goto err_free_mpi_coredump;
}
/* Keep local copy of current mac address. */
@@ -4485,7 +4485,7 @@ static int qlge_init_device(struct pci_dev *pdev, struct
qlge_adapter *qdev,
ndev->name);
if (!qdev->workqueue) {
err = -ENOMEM;
- goto err_out2;
+ goto err_free_mpi_coredump;
}
INIT_DELAYED_WORK(>asic_reset_work, qlge_asic_reset_work);
@@ -4503,10 +4503,18 @@ static int qlge_init_device(struct pci_dev *pdev,
struct qlge_adapter *qdev,
DRV_NAME, DRV_VERSION);
}
return 0;
-err_out2:
- qlge_release_all(pdev);
-err_out1:
+
+err_free_mpi_coredump:
+ vfree(qdev->mpi_coredump);
+err_iounmap_doorbell:
+ iounmap(qdev->doorbell_area);
+err_iounmap_base:
+ iounmap(qdev->reg_base);
+err_release_pci:
+ pci_release_regions(pdev);
+err_disable_pci:
pci_disable_device(pdev);
+
return err;
}
--
2.29.2
[PATCH v4 7/8] staging: qlge: clean up debugging code in the QL_ALL_DUMP ifdef land
The debugging code in the following ifdef land
- QL_ALL_DUMP
- QL_REG_DUMP
- QL_DEV_DUMP
- QL_CB_DUMP
- QL_IB_DUMP
- QL_OB_DUMP
becomes unnecessary because,
- Device status and general registers can be obtained by ethtool.
- Coredump can be done via devlink health reporter.
- Structure related to the hardware (struct ql_adapter) can be obtained
by crash or drgn.
Link: https://lkml.org/lkml/2020/6/30/19
Suggested-by: Benjamin Poirier
Signed-off-by: Coiby Xu
---
drivers/staging/qlge/TODO | 4 -
drivers/staging/qlge/qlge.h | 82
drivers/staging/qlge/qlge_dbg.c | 688
drivers/staging/qlge/qlge_ethtool.c | 2 -
drivers/staging/qlge/qlge_main.c| 7 +-
5 files changed, 1 insertion(+), 782 deletions(-)
diff --git a/drivers/staging/qlge/TODO b/drivers/staging/qlge/TODO
index e68c95f47754..c76394b9451b 100644
--- a/drivers/staging/qlge/TODO
+++ b/drivers/staging/qlge/TODO
@@ -14,10 +14,6 @@
queues" is confusing.
* struct rx_ring is used for rx and tx completions, with some members relevant
to one case only
-* there is an inordinate amount of disparate debugging code, most of which is
- of questionable value. In particular, qlge_dbg.c has hundreds of lines of
- code bitrotting away in ifdef land (doesn't compile since commit
- 18c49b91777c ("qlge: do vlan cleanup", v3.1-rc1), 8 years ago).
* the flow control implementation in firmware is buggy (sends a flood of pause
frames, resets the link, device and driver buffer queues become
desynchronized), disable it by default
diff --git a/drivers/staging/qlge/qlge.h b/drivers/staging/qlge/qlge.h
index aa5721862140..55e0ad759250 100644
--- a/drivers/staging/qlge/qlge.h
+++ b/drivers/staging/qlge/qlge.h
@@ -2289,86 +2289,4 @@ void qlge_check_lb_frame(struct qlge_adapter *qdev,
struct sk_buff *skb);
int qlge_own_firmware(struct qlge_adapter *qdev);
int qlge_clean_lb_rx_ring(struct rx_ring *rx_ring, int budget);
-/* #define QL_ALL_DUMP */
-/* #define QL_REG_DUMP */
-/* #define QL_DEV_DUMP */
-/* #define QL_CB_DUMP */
-/* #define QL_IB_DUMP */
-/* #define QL_OB_DUMP */
-
-#ifdef QL_REG_DUMP
-void qlge_dump_xgmac_control_regs(struct qlge_adapter *qdev);
-void qlge_dump_routing_entries(struct qlge_adapter *qdev);
-void qlge_dump_regs(struct qlge_adapter *qdev);
-#define QL_DUMP_REGS(qdev) qlge_dump_regs(qdev)
-#define QL_DUMP_ROUTE(qdev) qlge_dump_routing_entries(qdev)
-#define QL_DUMP_XGMAC_CONTROL_REGS(qdev) qlge_dump_xgmac_control_regs(qdev)
-#else
-#define QL_DUMP_REGS(qdev)
-#define QL_DUMP_ROUTE(qdev)
-#define QL_DUMP_XGMAC_CONTROL_REGS(qdev)
-#endif
-
-#ifdef QL_STAT_DUMP
-void qlge_dump_stat(struct qlge_adapter *qdev);
-#define QL_DUMP_STAT(qdev) qlge_dump_stat(qdev)
-#else
-#define QL_DUMP_STAT(qdev)
-#endif
-
-#ifdef QL_DEV_DUMP
-void qlge_dump_qdev(struct qlge_adapter *qdev);
-#define QL_DUMP_QDEV(qdev) qlge_dump_qdev(qdev)
-#else
-#define QL_DUMP_QDEV(qdev)
-#endif
-
-#ifdef QL_CB_DUMP
-void qlge_dump_wqicb(struct wqicb *wqicb);
-void qlge_dump_tx_ring(struct tx_ring *tx_ring);
-void qlge_dump_ricb(struct ricb *ricb);
-void qlge_dump_cqicb(struct cqicb *cqicb);
-void qlge_dump_rx_ring(struct rx_ring *rx_ring);
-void qlge_dump_hw_cb(struct qlge_adapter *qdev, int size, u32 bit, u16 q_id);
-#define QL_DUMP_RICB(ricb) qlge_dump_ricb(ricb)
-#define QL_DUMP_WQICB(wqicb) qlge_dump_wqicb(wqicb)
-#define QL_DUMP_TX_RING(tx_ring) qlge_dump_tx_ring(tx_ring)
-#define QL_DUMP_CQICB(cqicb) qlge_dump_cqicb(cqicb)
-#define QL_DUMP_RX_RING(rx_ring) qlge_dump_rx_ring(rx_ring)
-#define QL_DUMP_HW_CB(qdev, size, bit, q_id) \
- qlge_dump_hw_cb(qdev, size, bit, q_id)
-#else
-#define QL_DUMP_RICB(ricb)
-#define QL_DUMP_WQICB(wqicb)
-#define QL_DUMP_TX_RING(tx_ring)
-#define QL_DUMP_CQICB(cqicb)
-#define QL_DUMP_RX_RING(rx_ring)
-#define QL_DUMP_HW_CB(qdev, size, bit, q_id)
-#endif
-
-#ifdef QL_OB_DUMP
-void qlge_dump_tx_desc(struct qlge_adapter *qdev, struct tx_buf_desc *tbd);
-void qlge_dump_ob_mac_iocb(struct qlge_adapter *qdev, struct
qlge_ob_mac_iocb_req *ob_mac_iocb);
-void qlge_dump_ob_mac_rsp(struct qlge_adapter *qdev, struct
qlge_ob_mac_iocb_rsp *ob_mac_rsp);
-#define QL_DUMP_OB_MAC_IOCB(qdev, ob_mac_iocb) qlge_dump_ob_mac_iocb(qdev,
ob_mac_iocb)
-#define QL_DUMP_OB_MAC_RSP(qdev, ob_mac_rsp) qlge_dump_ob_mac_rsp(qdev,
ob_mac_rsp)
-#else
-#define QL_DUMP_OB_MAC_IOCB(qdev, ob_mac_iocb)
-#define QL_DUMP_OB_MAC_RSP(qdev, ob_mac_rsp)
-#endif
-
-#ifdef QL_IB_DUMP
-void qlge_dump_ib_mac_rsp(struct qlge_adapter *qdev, struct
qlge_ib_mac_iocb_rsp *ib_mac_rsp);
-#define QL_DUMP_IB_MAC_RSP(qdev, ib_mac_rsp) qlge_dump_ib_mac_rsp(qdev,
ib_mac_rsp)
-#else
-#define QL_DUMP_IB_MAC_RSP(qdev, ib_mac_rsp)
-#endif
-
-#ifdef QL_ALL_DUMP
-void qlge_dump_all(struct qlge_adapter *qdev);
-#define QL_DUMP_ALL(qdev) qlge_dump_all(qdev)
-#else
-#define QL_DUMP_ALL(qdev)
-#endif
-
#endif /* _QLGE_H_ */
diff --git a/drivers/staging/qlge/qlge_dbg.c b/drivers/staging/qlge/q
[PATCH v4 6/8] staging: qlge: remove mpi_core_to_log which sends coredump to the kernel ring buffer
devlink health could be used to get coredump. No need to send so much
data to the kernel ring buffer.
Signed-off-by: Coiby Xu
---
drivers/staging/qlge/TODO | 2 --
drivers/staging/qlge/qlge.h | 3 ---
drivers/staging/qlge/qlge_dbg.c | 11 ---
drivers/staging/qlge/qlge_ethtool.c | 1 -
drivers/staging/qlge/qlge_main.c| 2 --
drivers/staging/qlge/qlge_mpi.c | 6 --
6 files changed, 25 deletions(-)
diff --git a/drivers/staging/qlge/TODO b/drivers/staging/qlge/TODO
index 5ac55664c3e2..e68c95f47754 100644
--- a/drivers/staging/qlge/TODO
+++ b/drivers/staging/qlge/TODO
@@ -18,8 +18,6 @@
of questionable value. In particular, qlge_dbg.c has hundreds of lines of
code bitrotting away in ifdef land (doesn't compile since commit
18c49b91777c ("qlge: do vlan cleanup", v3.1-rc1), 8 years ago).
-* triggering an ethtool regdump will hexdump a 176k struct to dmesg depending
- on some module parameters.
* the flow control implementation in firmware is buggy (sends a flood of pause
frames, resets the link, device and driver buffer queues become
desynchronized), disable it by default
diff --git a/drivers/staging/qlge/qlge.h b/drivers/staging/qlge/qlge.h
index 41f69751d34d..aa5721862140 100644
--- a/drivers/staging/qlge/qlge.h
+++ b/drivers/staging/qlge/qlge.h
@@ -2153,7 +2153,6 @@ struct qlge_adapter {
u32 port_init;
u32 link_status;
struct qlge_mpi_coredump *mpi_coredump;
- u32 core_is_dumped;
u32 link_config;
u32 led_config;
u32 max_frame_size;
@@ -2166,7 +2165,6 @@ struct qlge_adapter {
struct delayed_work mpi_work;
struct delayed_work mpi_port_cfg_work;
struct delayed_work mpi_idc_work;
- struct delayed_work mpi_core_to_log;
struct completion ide_completion;
const struct nic_operations *nic_ops;
u16 device_id;
@@ -2257,7 +2255,6 @@ int qlge_write_cfg(struct qlge_adapter *qdev, void *ptr,
int size, u32 bit,
void qlge_queue_fw_error(struct qlge_adapter *qdev);
void qlge_mpi_work(struct work_struct *work);
void qlge_mpi_reset_work(struct work_struct *work);
-void qlge_mpi_core_to_log(struct work_struct *work);
int qlge_wait_reg_rdy(struct qlge_adapter *qdev, u32 reg, u32 bit, u32 ebit);
void qlge_queue_asic_error(struct qlge_adapter *qdev);
void qlge_set_ethtool_ops(struct net_device *ndev);
diff --git a/drivers/staging/qlge/qlge_dbg.c b/drivers/staging/qlge/qlge_dbg.c
index b0d4ea071f32..5c64d6de3b30 100644
--- a/drivers/staging/qlge/qlge_dbg.c
+++ b/drivers/staging/qlge/qlge_dbg.c
@@ -1313,17 +1313,6 @@ void qlge_get_dump(struct qlge_adapter *qdev, void *buff)
}
}
-/* Coredump to messages log file using separate worker thread */
-void qlge_mpi_core_to_log(struct work_struct *work)
-{
- struct qlge_adapter *qdev =
- container_of(work, struct qlge_adapter, mpi_core_to_log.work);
-
- print_hex_dump(KERN_DEBUG, "Core is dumping to log file!\n",
- DUMP_PREFIX_OFFSET, 32, 4, qdev->mpi_coredump,
- sizeof(*qdev->mpi_coredump), false);
-}
-
#ifdef QL_REG_DUMP
static void qlge_dump_intr_states(struct qlge_adapter *qdev)
{
diff --git a/drivers/staging/qlge/qlge_ethtool.c
b/drivers/staging/qlge/qlge_ethtool.c
index 24b079523d5c..3e911f147dfc 100644
--- a/drivers/staging/qlge/qlge_ethtool.c
+++ b/drivers/staging/qlge/qlge_ethtool.c
@@ -617,7 +617,6 @@ static void qlge_get_regs(struct net_device *ndev,
struct qlge_adapter *qdev = netdev_to_qdev(ndev);
qlge_get_dump(qdev, p);
- qdev->core_is_dumped = 0;
if (!test_bit(QL_FRC_COREDUMP, >flags))
regs->len = sizeof(struct qlge_mpi_coredump);
else
diff --git a/drivers/staging/qlge/qlge_main.c b/drivers/staging/qlge/qlge_main.c
index 2ec688d3d946..747dbb54dde4 100644
--- a/drivers/staging/qlge/qlge_main.c
+++ b/drivers/staging/qlge/qlge_main.c
@@ -3800,7 +3800,6 @@ static void qlge_cancel_all_work_sync(struct qlge_adapter
*qdev)
cancel_delayed_work_sync(>mpi_reset_work);
cancel_delayed_work_sync(>mpi_work);
cancel_delayed_work_sync(>mpi_idc_work);
- cancel_delayed_work_sync(>mpi_core_to_log);
cancel_delayed_work_sync(>mpi_port_cfg_work);
}
@@ -4493,7 +4492,6 @@ static int qlge_init_device(struct pci_dev *pdev, struct
qlge_adapter *qdev,
INIT_DELAYED_WORK(>mpi_work, qlge_mpi_work);
INIT_DELAYED_WORK(>mpi_port_cfg_work, qlge_mpi_port_cfg_work);
INIT_DELAYED_WORK(>mpi_idc_work, qlge_mpi_idc_work);
- INIT_DELAYED_WORK(>mpi_core_to_log, qlge_mpi_core_to_log);
init_completion(>ide_completion);
mutex_init(>mpi_mutex);
diff --git a/drivers/staging/qlge/qlge_mpi.c b/drivers/staging/qlge/qlge_mpi.c
index 2b77995ec76c..2630ebf50341 100644
--- a/drivers/staging/qlge/qlge_mpi.c
+++ b/drivers/staging/qlge/qlge_mpi.c
@@ -
[PATCH v4 2/8] staging: qlge: Initialize devlink health dump framework
Initialize devlink health dump framework for the qlge driver so the
coredump could be done via devlink.
struct qlge_adapter is now used as the private data structure of
struct devlink so it could exist independently of struct net_device
and devlink reload could be supported in the future. The private data
of PCIe driver now points to qlge_adapter.
Since devlink_alloc will zero out struct qlge_adapter, memset in
qlge_init_device is not necessary.
Signed-off-by: Coiby Xu
---
drivers/staging/qlge/Kconfig| 1 +
drivers/staging/qlge/Makefile | 2 +-
drivers/staging/qlge/qlge.h | 13 +++
drivers/staging/qlge/qlge_devlink.c | 31 +++
drivers/staging/qlge/qlge_devlink.h | 9 ++
drivers/staging/qlge/qlge_ethtool.c | 36
drivers/staging/qlge/qlge_main.c| 125 +---
7 files changed, 151 insertions(+), 66 deletions(-)
create mode 100644 drivers/staging/qlge/qlge_devlink.c
create mode 100644 drivers/staging/qlge/qlge_devlink.h
diff --git a/drivers/staging/qlge/Kconfig b/drivers/staging/qlge/Kconfig
index a3cb25a3ab80..6d831ed67965 100644
--- a/drivers/staging/qlge/Kconfig
+++ b/drivers/staging/qlge/Kconfig
@@ -3,6 +3,7 @@
config QLGE
tristate "QLogic QLGE 10Gb Ethernet Driver Support"
depends on ETHERNET && PCI
+ select NET_DEVLINK
help
This driver supports QLogic ISP8XXX 10Gb Ethernet cards.
diff --git a/drivers/staging/qlge/Makefile b/drivers/staging/qlge/Makefile
index 1dc2568e820c..07c1898a512e 100644
--- a/drivers/staging/qlge/Makefile
+++ b/drivers/staging/qlge/Makefile
@@ -5,4 +5,4 @@
obj-$(CONFIG_QLGE) += qlge.o
-qlge-objs := qlge_main.o qlge_dbg.o qlge_mpi.o qlge_ethtool.o
+qlge-objs := qlge_main.o qlge_dbg.o qlge_mpi.o qlge_ethtool.o qlge_devlink.o
diff --git a/drivers/staging/qlge/qlge.h b/drivers/staging/qlge/qlge.h
index 1ac85f2f770f..41f69751d34d 100644
--- a/drivers/staging/qlge/qlge.h
+++ b/drivers/staging/qlge/qlge.h
@@ -2060,6 +2060,18 @@ struct nic_operations {
int (*port_initialize)(struct qlge_adapter *qdev);
};
+struct qlge_netdev_priv {
+ struct qlge_adapter *qdev;
+ struct net_device *ndev;
+};
+
+static inline
+struct qlge_adapter *netdev_to_qdev(struct net_device *ndev)
+{
+ struct qlge_netdev_priv *ndev_priv = netdev_priv(ndev);
+
+ return ndev_priv->qdev;
+}
/*
* The main Adapter structure definition.
* This structure has all fields relevant to the hardware.
@@ -2077,6 +2089,7 @@ struct qlge_adapter {
struct pci_dev *pdev;
struct net_device *ndev;/* Parent NET device */
+ struct devlink_health_reporter *reporter;
/* Hardware information */
u32 chip_rev_id;
u32 fw_rev_id;
diff --git a/drivers/staging/qlge/qlge_devlink.c
b/drivers/staging/qlge/qlge_devlink.c
new file mode 100644
index ..d9c71f45211f
--- /dev/null
+++ b/drivers/staging/qlge/qlge_devlink.c
@@ -0,0 +1,31 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+#include "qlge.h"
+#include "qlge_devlink.h"
+
+static int
+qlge_reporter_coredump(struct devlink_health_reporter *reporter,
+ struct devlink_fmsg *fmsg, void *priv_ctx,
+ struct netlink_ext_ack *extack)
+{
+ return 0;
+}
+
+static const struct devlink_health_reporter_ops qlge_reporter_ops = {
+ .name = "dummy",
+ .dump = qlge_reporter_coredump,
+};
+
+void qlge_health_create_reporters(struct qlge_adapter *priv)
+{
+ struct devlink_health_reporter *reporter;
+ struct devlink *devlink;
+
+ devlink = priv_to_devlink(priv);
+ priv->reporter =
+ devlink_health_reporter_create(devlink, _reporter_ops,
+ 0, priv);
+ if (IS_ERR(priv->reporter))
+ netdev_warn(priv->ndev,
+ "Failed to create reporter, err = %ld\n",
+ PTR_ERR(reporter));
+}
diff --git a/drivers/staging/qlge/qlge_devlink.h
b/drivers/staging/qlge/qlge_devlink.h
new file mode 100644
index ..19078e1ac694
--- /dev/null
+++ b/drivers/staging/qlge/qlge_devlink.h
@@ -0,0 +1,9 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+#ifndef QLGE_DEVLINK_H
+#define QLGE_DEVLINK_H
+
+#include
+
+void qlge_health_create_reporters(struct qlge_adapter *priv);
+
+#endif /* QLGE_DEVLINK_H */
diff --git a/drivers/staging/qlge/qlge_ethtool.c
b/drivers/staging/qlge/qlge_ethtool.c
index 3e577e1bc27c..24b079523d5c 100644
--- a/drivers/staging/qlge/qlge_ethtool.c
+++ b/drivers/staging/qlge/qlge_ethtool.c
@@ -366,7 +366,7 @@ static void
qlge_get_ethtool_stats(struct net_device *ndev,
struct ethtool_stats *stats, u64 *data)
{
- struct qlge_adapter *qdev = netdev_priv(ndev);
+ struct qlge_adapter *qdev = netdev_to_qdev(ndev);
int index, length;
length = QLGE_STATS_LEN;
@@ -3
[PATCH v4 5/8] staging: qlge: support force_coredump option for devlink health dump
With force_coredump module parameter set, devlink health dump will
reset the MPI RISC first which takes 5 secs to be finished.
Note that only NIC function that owns the firmware can do the
force_dumping. Otherwise devlink will receive an EPERM error.
Signed-off-by: Coiby Xu
---
drivers/staging/qlge/qlge_devlink.c | 13 +
1 file changed, 13 insertions(+)
diff --git a/drivers/staging/qlge/qlge_devlink.c
b/drivers/staging/qlge/qlge_devlink.c
index bf7d75ed5eae..c6ef5163e241 100644
--- a/drivers/staging/qlge/qlge_devlink.c
+++ b/drivers/staging/qlge/qlge_devlink.c
@@ -56,10 +56,23 @@ static int qlge_reporter_coredump(struct
devlink_health_reporter *reporter,
struct qlge_adapter *qdev = devlink_health_reporter_priv(reporter);
struct qlge_mpi_coredump *dump;
+ wait_queue_head_t wait;
if (!netif_running(qdev->ndev))
return 0;
+ if (test_bit(QL_FRC_COREDUMP, >flags)) {
+ if (qlge_own_firmware(qdev)) {
+ qlge_queue_fw_error(qdev);
+ init_waitqueue_head();
+ wait_event_timeout(wait, 0, 5 * HZ);
+ } else {
+ netif_err(qdev, ifup, qdev->ndev,
+ "Force Coredump failed because this NIC
function doesn't own the firmware\n");
+ return -EPERM;
+ }
+ }
+
dump = kvmalloc(sizeof(*dump), GFP_KERNEL);
if (!dump)
return -ENOMEM;
--
2.29.2
Re: [PATCH v4] HID: i2c-hid: add polling mode based on connected GPIO chip's pin status
Hi Greg and Barnabás, On Wed, Dec 09, 2020 at 04:44:35PM +0100, Greg KH wrote: On Wed, Dec 09, 2020 at 03:38:11PM +, Barnabás PÅ‘cze wrote: 2020. december 9., szerda 8:00 keltezéssel, Greg KH Ãrta: > On Tue, Dec 08, 2020 at 09:59:20PM +, Barnabás PÅ‘cze wrote: > > > 2020. november 25., szerda 16:07 keltezéssel, Greg KH Ãrta: > > > > > [...] > > > > > > > +static u8 polling_mode; > > > > +module_param(polling_mode, byte, 0444); > > > > +MODULE_PARM_DESC(polling_mode, "How to poll (default=0) - 0 disabled; 1 based on GPIO pin's status"); > > > > > > Module parameters are for the 1990's, they are global and horrible to > > > try to work with. You should provide something on a per-device basis, > > > as what happens if your system requires different things here for > > > different devices? You set this for all devices :( > > > [...] Thank you for pointing out that. > > > > Hi > > do you think something like what the usbcore has would be better? > > A module parameter like "quirks=::[,::]*"? > > Not really, that's just for debugging, and asking users to test > something, not for a final solution to anything. This patch is not only for debugging. The primary reason is as a fallback solution to save the touchpad. The mentioned touchpads will be fixed by Linux 5.11 which means the enthusiastic Linux users have to wait for ~10 months to get their touchpads fixed. My understanding is that this polling mode option is by no means intended as a final solution, it's purely for debugging/fallback: "Polling mode could be a fallback solution for enthusiastic Linux users when they have a new laptop. It also acts like a debugging feature. If polling mode works for a broken touchpad, we can almost be certain the root cause is related to the interrupt or power setting." What would you suggest instead of the module parameter? a debugfs file? That means it's only for debugging and you have to be root to change the value. Thank you for the helpful discussion and the suggestions! If we can answer the following two questions, it could help decide what's the next move. 1. How many machines have two or more I2C HID devices? For the laptops this patch try to fix, they only have one I2C HID device, i.e., the touchpad. If it's the case with most machines running Linux, then we don't need to support per-HID-I2C-device setting and module parameter is the most user-friendly since the user doesn't even need to know the / pair. 2. How many I2C HID devices like touchpads could be saved by this patch in the future? If polling could potentially save lots of I2C hid devices, we are more motivated to make it easier to use. -- Best regards, Coiby
Re: [PATCH v4] pinctrl: amd: remove debounce filter setting in IRQ type setting
On Fri, Dec 04, 2020 at 10:03:43AM +0100, Linus Walleij wrote:
On Wed, Nov 25, 2020 at 2:03 PM Coiby Xu wrote:
Debounce filter setting should be independent from IRQ type setting
because according to the ACPI specs, there are separate arguments for
specifying debounce timeout and IRQ type in GpioIo() and GpioInt().
Together with commit 06abe8291bc31839950f7d0362d9979edc88a666
("pinctrl: amd: fix incorrect way to disable debounce filter") and
Andy's patch "gpiolib: acpi: Take into account debounce settings" [1],
this will fix broken touchpads for laptops whose BIOS set the
debounce timeout to a relatively large value. For example, the BIOS
of Lenovo AMD gaming laptops including Legion-5 15ARH05 (R7000),
Legion-5P (R7000P) and IdeaPad Gaming 3 15ARH05, set the debounce
timeout to 124.8ms. This led to the kernel receiving only ~7 HID
reports per second from the Synaptics touchpad
(MSFT0001:00 06CB:7F28).
Existing touchpads like [2][3] are not troubled by this bug because
the debounce timeout has been set to 0 by the BIOS before enabling
the debounce filter in setting IRQ type.
[1]
https://lore.kernel.org/linux-gpio/2020222008.39993-11-andriy.shevche...@linux.intel.com/
[2] https://github.com/Syniurge/i2c-amd-mp2/issues/11#issuecomment-721331582
[3] https://forum.manjaro.org/t/random-short-touchpad-freezes/30832/28
Cc: Hans de Goede
Cc: Andy Shevchenko
Cc: Benjamin Tissoires
Cc: sta...@vger.kernel.org
Reviewed-by: Andy Shevchenko
BugLink: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1887190
Link:
https://lore.kernel.org/linux-gpio/CAHp75VcwiGREBUJ0A06EEw-SyabqYsp%2Bdqs2DpSrhaY-2GVdAA%40mail.gmail.com/
Signed-off-by: Coiby Xu
---
Changelog v4:
- Note in the commit message that this patch depends on other two
patches to fix the broken touchpad [Hans de Goede]
- Add in the commit message that one more touchpad could be fixed.
Patch applied for fixes adding a reference to Andy's commit.
Thanks for sorting this out!
Thank you for applying the patch!
Yours,
Linus Walleij
--
Best regards,
Coiby
Re: [PATCH v4] pinctrl: amd: remove debounce filter setting in IRQ type setting
On Wed, Nov 25, 2020 at 03:24:20PM +0200, Andy Shevchenko wrote:
On Wed, Nov 25, 2020 at 3:03 PM Coiby Xu wrote:
Debounce filter setting should be independent from IRQ type setting
because according to the ACPI specs, there are separate arguments for
specifying debounce timeout and IRQ type in GpioIo() and GpioInt().
Together with commit 06abe8291bc31839950f7d0362d9979edc88a666
("pinctrl: amd: fix incorrect way to disable debounce filter") and
Andy's patch "gpiolib: acpi: Take into account debounce settings" [1],
this will fix broken touchpads for laptops whose BIOS set the
debounce timeout to a relatively large value. For example, the BIOS
of Lenovo AMD gaming laptops including Legion-5 15ARH05 (R7000),
Legion-5P (R7000P) and IdeaPad Gaming 3 15ARH05, set the debounce
timeout to 124.8ms. This led to the kernel receiving only ~7 HID
reports per second from the Synaptics touchpad
(MSFT0001:00 06CB:7F28).
Existing touchpads like [2][3] are not troubled by this bug because
the debounce timeout has been set to 0 by the BIOS before enabling
the debounce filter in setting IRQ type.
[1]
https://lore.kernel.org/linux-gpio/2020222008.39993-11-andriy.shevche...@linux.intel.com/
JFYI: this is nowadays
8dcb7a15a585 ("gpiolib: acpi: Take into account debounce settings")
Thank you for the info! Next time I will also check the linux-next
tree:)
(No need to recend, just an information that can be applied maybe by Linus)
[2] https://github.com/Syniurge/i2c-amd-mp2/issues/11#issuecomment-721331582
[3] https://forum.manjaro.org/t/random-short-touchpad-freezes/30832/28
--
With Best Regards,
Andy Shevchenko
--
Best regards,
Coiby
[PATCH v4] HID: i2c-hid: add polling mode based on connected GPIO chip's pin status
For a broken touchpad, it may take several months or longer to be fixed.
Polling mode could be a fallback solution for enthusiastic Linux users
when they have a new laptop. It also acts like a debugging feature. If
polling mode works for a broken touchpad, we can almost be certain
the root cause is related to the interrupt or power setting.
This patch could fix touchpads of Lenovo AMD gaming laptops including
Legion-5 15ARH05 (R7000), Legion-5P (R7000P) and IdeaPad Gaming 3
15ARH05.
When polling mode is enabled, an I2C device can't wake up the suspended
system since enable/disable_irq_wake is invalid for polling mode.
Three module parameters are added to i2c-hid,
- polling_mode: by default set to 0, i.e., polling is disabled
- polling_interval_idle_ms: the polling internal when the touchpad
is idle, default to 10ms
- polling_interval_active_us: the polling internal when the touchpad
is active, default to 4000us
User can change the last two runtime polling parameter by writing to
/sys/module/i2c_hid/parameters/polling_interval_{idle_ms,active_us}.
Note xf86-input-synaptics doesn't work well with this polling mode
for the Synaptics touchpad. The Synaptics touchpad would often locks
into scroll mode when using multitouch gestures [1]. One remedy is to
decrease the polling interval.
Thanks to Barnabás's thorough review of this patch and the useful
feedback!
[1] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1887190/comments/235
Cc:
Cc: Barnabás Pőcze
BugLink: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1887190
Signed-off-by: Coiby Xu
---
drivers/hid/i2c-hid/i2c-hid-core.c | 152 +++--
1 file changed, 142 insertions(+), 10 deletions(-)
diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c
b/drivers/hid/i2c-hid/i2c-hid-core.c
index aeff1ffb0c8b..f25503f31ccf 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -36,6 +36,8 @@
#include
#include
#include
+#include
+#include
#include
#include
@@ -60,6 +62,25 @@
#define I2C_HID_PWR_ON 0x00
#define I2C_HID_PWR_SLEEP 0x01
+/* polling mode */
+#define I2C_HID_POLLING_DISABLED 0
+#define I2C_HID_POLLING_GPIO_PIN 1
+#define I2C_HID_POLLING_INTERVAL_ACTIVE_US 4000
+#define I2C_HID_POLLING_INTERVAL_IDLE_MS 10
+
+static u8 polling_mode;
+module_param(polling_mode, byte, 0444);
+MODULE_PARM_DESC(polling_mode, "How to poll (default=0) - 0 disabled; 1 based
on GPIO pin's status");
+
+static unsigned int polling_interval_active_us __read_mostly =
I2C_HID_POLLING_INTERVAL_ACTIVE_US;
+module_param(polling_interval_active_us, uint, 0644);
+MODULE_PARM_DESC(polling_interval_active_us,
+"Poll every {polling_interval_active_us} us when the touchpad
is active (default=" __stringify(I2C_HID_POLLING_INTERVAL_ACTIVE_US) " us)");
+
+static unsigned int polling_interval_idle_ms __read_mostly =
I2C_HID_POLLING_INTERVAL_IDLE_MS;
+module_param(polling_interval_idle_ms, uint, 0644);
+MODULE_PARM_DESC(polling_interval_idle_ms,
+"Poll every {polling_interval_idle_ms} ms when the touchpad is
idle (default=" __stringify(I2C_HID_POLLING_INTERVAL_IDLE_MS) "ms)");
/* debug option */
static bool debug;
module_param(debug, bool, 0444);
@@ -158,6 +179,10 @@ struct i2c_hid {
struct i2c_hid_platform_data pdata;
+ struct task_struct *polling_thread;
+ unsigned long irq_trigger_type;
+ struct irq_desc *irq_desc;
+
boolirq_wake_enabled;
struct mutexreset_lock;
};
@@ -772,7 +797,9 @@ static int i2c_hid_start(struct hid_device *hid)
i2c_hid_free_buffers(ihid);
ret = i2c_hid_alloc_buffers(ihid, bufsize);
- enable_irq(client->irq);
+
+ if (polling_mode == I2C_HID_POLLING_DISABLED)
+ enable_irq(client->irq);
if (ret)
return ret;
@@ -814,6 +841,96 @@ struct hid_ll_driver i2c_hid_ll_driver = {
};
EXPORT_SYMBOL_GPL(i2c_hid_ll_driver);
+static int get_gpio_pin_state(struct irq_desc *irq_desc)
+{
+ struct gpio_chip *gc = irq_data_get_irq_chip_data(_desc->irq_data);
+ int value;
+
+ /*
+* This part of code is borrowed from gpiod_get_raw_value_commit in
+* drivers/gpio/gpiolib.c. Ideally, gpiod_get_value_cansleep can be
re-used
+* instead but there is no API of converting (struct irq_desc *) to
+* (struct gpio_desc*).
+*/
+ value = gc->get ? gc->get(gc, irq_desc->irq_data.hwirq) : -EIO;
+ value = value < 0 ? value : !!value;
+ return value;
+}
+
+static bool interrupt_line_active(struct i2c_hid *ihid)
+{
+ int status = get_gpio_pin_state(ihid->irq_desc);
+ struct i2c_client *client = ihid->client;
+
+ if (status < 0) {
+ dev_dbg_ratelimited(>dev,
+
Re: [PATCH v3] HID: i2c-hid: add polling mode based on connected GPIO chip's pin status
On Wed, Nov 25, 2020 at 12:39:02PM +, Barnabás Pőcze wrote:
2020. november 25., szerda 11:57 keltezéssel, Coiby Xu Ãrta:
On Mon, Nov 23, 2020 at 04:32:40PM +, Barnabás Pőcze wrote:
>> [...]
>> >> >> +static int get_gpio_pin_state(struct irq_desc *irq_desc)
>> >> >> +{
>> >> >> +struct gpio_chip *gc =
irq_data_get_irq_chip_data(_desc->irq_data);
>> >> >> +
>> >> >> +return gc->get(gc, irq_desc->irq_data.hwirq);
>> >> >> +}
>> >> [...]
>> >> >> +ssize_t status = get_gpio_pin_state(irq_desc);
>> >> >
>> >> >`get_gpio_pin_state()` returns an `int`, so I am not sure why `ssize_t`
is used here.
>> >> >
>> >>
>> >> I used `ssize_t` because I found gpiolib-sysfs.c uses `ssize_t`
>> >>
>> >> // drivers/gpio/gpiolib-sysfs.c
>> >> static ssize_t value_show(struct device *dev,
>> >> struct device_attribute *attr, char *buf)
>> >> {
>> >> struct gpiod_data *data = dev_get_drvdata(dev);
>> >> struct gpio_desc *desc = data->desc;
>> >> ssize_t status;
>> >>
>> >> mutex_lock(>mutex);
>> >>
>> >> status = gpiod_get_value_cansleep(desc);
>> >> ...
>> >> return status;
>> >> }
>> >>
>> >> According to the book Advanced Programming in the UNIX Environment by
>> >> W. Richard Stevens,
>> >> With the 1990 POSIX.1 standard, the primitive system data type
>> >> ssize_t was introduced to provide the signed return value...
>> >>
>> >> So ssize_t is fairly common, for example, the read and write syscall
>> >> return a value of type ssize_t. But I haven't found out why ssize_t is
>> >> better int.
>> >> >
>> >
>> >Sorry if I wasn't clear, what prompted me to ask that question is the
following:
>> >`gc->get()` returns `int`, `get_gpio_pin_state()` returns `int`, yet you
still
>> >save the return value of `get_gpio_pin_state()` into a variable with type
>> >`ssize_t` for no apparent reason. In the example you cited, `ssize_t` is
used
>> >because the show() callback of a sysfs attribute must return `ssize_t`, but
here,
>> >`interrupt_line_active()` returns `bool`, so I don't see any advantage over
a
>> >plain `int`. Anyways, I believe either one is fine, I just found it odd.
>> >
>> I don't understand why "the show() callback of a sysfs attribute
>> must return `ssize_t`" instead of int. Do you think the rationale
>> behind it is the same for this case? If yes, using "ssize_t" for
>> status could be justified.
>> [...]
>
>Because it was decided that way, `ssize_t` is a better choice for that purpose
>than plain `int`. You can see it in include/linux/device.h, that both the
>show() and store() methods must return `ssize_t`.
>
Could you explain why `ssize_t` is a better choice? AFAIU, ssize_t
is used because we can return negative value to indicate an error.
ssize_t: "Signed integer type used for a count of bytes or an error
indication."[1]
And POSIX mandates that the return type of read() and write() be `ssize_t`,
so it makes sense to keep a similar interface in the kernel since show() and
store()
are called as a direct result of the user using the read() and write() system
calls, respectively.
If
we use ssize_t here, it's a reminder that reading a GPIO pin's status
could fail. And ssize_t reminds us it's a operation similar to read
or write. So ssize_t is better than int here. And maybe it's the same
reason why "it was decided that way".
[...]
I believe it's more appropriate to use ssize_t when it's about a "count of
elements",
but the GPIO pin state is a single boolean value (or an error indication), which
is returned as an `int`. Since it's returned as an `int` - I'm arguing that -
there is no reason to use `ssize_t` here. Anyways, both `ssize_t` and `int`
work fine
in this case.
So value_show in gpiolib-sysfs.c is a kind of being forced to use
ssize_t. I'll use int instead to avoid confusion in v4. Thank you for
the explanation!
[1]:
https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_12
Regards,
Barnabás Pőcze
--
Best regards,
Coiby
[PATCH v4] pinctrl: amd: remove debounce filter setting in IRQ type setting
Debounce filter setting should be independent from IRQ type setting
because according to the ACPI specs, there are separate arguments for
specifying debounce timeout and IRQ type in GpioIo() and GpioInt().
Together with commit 06abe8291bc31839950f7d0362d9979edc88a666
("pinctrl: amd: fix incorrect way to disable debounce filter") and
Andy's patch "gpiolib: acpi: Take into account debounce settings" [1],
this will fix broken touchpads for laptops whose BIOS set the
debounce timeout to a relatively large value. For example, the BIOS
of Lenovo AMD gaming laptops including Legion-5 15ARH05 (R7000),
Legion-5P (R7000P) and IdeaPad Gaming 3 15ARH05, set the debounce
timeout to 124.8ms. This led to the kernel receiving only ~7 HID
reports per second from the Synaptics touchpad
(MSFT0001:00 06CB:7F28).
Existing touchpads like [2][3] are not troubled by this bug because
the debounce timeout has been set to 0 by the BIOS before enabling
the debounce filter in setting IRQ type.
[1]
https://lore.kernel.org/linux-gpio/2020222008.39993-11-andriy.shevche...@linux.intel.com/
[2] https://github.com/Syniurge/i2c-amd-mp2/issues/11#issuecomment-721331582
[3] https://forum.manjaro.org/t/random-short-touchpad-freezes/30832/28
Cc: Hans de Goede
Cc: Andy Shevchenko
Cc: Benjamin Tissoires
Cc: sta...@vger.kernel.org
Reviewed-by: Andy Shevchenko
BugLink: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1887190
Link:
https://lore.kernel.org/linux-gpio/CAHp75VcwiGREBUJ0A06EEw-SyabqYsp%2Bdqs2DpSrhaY-2GVdAA%40mail.gmail.com/
Signed-off-by: Coiby Xu
---
Changelog v4:
- Note in the commit message that this patch depends on other two
patches to fix the broken touchpad [Hans de Goede]
- Add in the commit message that one more touchpad could be fixed.
Changelog v3:
- Explain why the driver mistakenly took the slightly deviated value
of debounce timeout in the commit message (patch 2/4) [Andy Shevchenko]
- Explain why other touchpads are affected by the issue of setting debounce
filter in IRQ type setting in the commit message (patch 4/4)
Changelog v2:
- Message-Id to Link and grammar fixes for commit messages [Andy Shevchenko]
---
drivers/pinctrl/pinctrl-amd.c | 7 ---
1 file changed, 7 deletions(-)
diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
index 4aea3e05e8c6..899c16c17b6d 100644
--- a/drivers/pinctrl/pinctrl-amd.c
+++ b/drivers/pinctrl/pinctrl-amd.c
@@ -429,7 +429,6 @@ static int amd_gpio_irq_set_type(struct irq_data *d,
unsigned int type)
pin_reg &= ~BIT(LEVEL_TRIG_OFF);
pin_reg &= ~(ACTIVE_LEVEL_MASK << ACTIVE_LEVEL_OFF);
pin_reg |= ACTIVE_HIGH << ACTIVE_LEVEL_OFF;
- pin_reg |= DB_TYPE_REMOVE_GLITCH << DB_CNTRL_OFF;
irq_set_handler_locked(d, handle_edge_irq);
break;
@@ -437,7 +436,6 @@ static int amd_gpio_irq_set_type(struct irq_data *d,
unsigned int type)
pin_reg &= ~BIT(LEVEL_TRIG_OFF);
pin_reg &= ~(ACTIVE_LEVEL_MASK << ACTIVE_LEVEL_OFF);
pin_reg |= ACTIVE_LOW << ACTIVE_LEVEL_OFF;
- pin_reg |= DB_TYPE_REMOVE_GLITCH << DB_CNTRL_OFF;
irq_set_handler_locked(d, handle_edge_irq);
break;
@@ -445,7 +443,6 @@ static int amd_gpio_irq_set_type(struct irq_data *d,
unsigned int type)
pin_reg &= ~BIT(LEVEL_TRIG_OFF);
pin_reg &= ~(ACTIVE_LEVEL_MASK << ACTIVE_LEVEL_OFF);
pin_reg |= BOTH_EADGE << ACTIVE_LEVEL_OFF;
- pin_reg |= DB_TYPE_REMOVE_GLITCH << DB_CNTRL_OFF;
irq_set_handler_locked(d, handle_edge_irq);
break;
@@ -453,8 +450,6 @@ static int amd_gpio_irq_set_type(struct irq_data *d,
unsigned int type)
pin_reg |= LEVEL_TRIGGER << LEVEL_TRIG_OFF;
pin_reg &= ~(ACTIVE_LEVEL_MASK << ACTIVE_LEVEL_OFF);
pin_reg |= ACTIVE_HIGH << ACTIVE_LEVEL_OFF;
- pin_reg &= ~(DB_CNTRl_MASK << DB_CNTRL_OFF);
- pin_reg |= DB_TYPE_PRESERVE_LOW_GLITCH << DB_CNTRL_OFF;
irq_set_handler_locked(d, handle_level_irq);
break;
@@ -462,8 +457,6 @@ static int amd_gpio_irq_set_type(struct irq_data *d,
unsigned int type)
pin_reg |= LEVEL_TRIGGER << LEVEL_TRIG_OFF;
pin_reg &= ~(ACTIVE_LEVEL_MASK << ACTIVE_LEVEL_OFF);
pin_reg |= ACTIVE_LOW << ACTIVE_LEVEL_OFF;
- pin_reg &= ~(DB_CNTRl_MASK << DB_CNTRL_OFF);
- pin_reg |= DB_TYPE_PRESERVE_HIGH_GLITCH << DB_CNTRL_OFF;
irq_set_handler_locked(d, handle_level_irq);
break;
--
2.29.2
Re: [PATCH v3] HID: i2c-hid: add polling mode based on connected GPIO chip's pin status
On Mon, Nov 23, 2020 at 04:32:40PM +, Barnabás Pőcze wrote:
[...]
>> >> +static int get_gpio_pin_state(struct irq_desc *irq_desc)
>> >> +{
>> >> + struct gpio_chip *gc =
irq_data_get_irq_chip_data(_desc->irq_data);
>> >> +
>> >> + return gc->get(gc, irq_desc->irq_data.hwirq);
>> >> +}
>> [...]
>> >> + ssize_t status = get_gpio_pin_state(irq_desc);
>> >
>> >`get_gpio_pin_state()` returns an `int`, so I am not sure why `ssize_t` is
used here.
>> >
>>
>> I used `ssize_t` because I found gpiolib-sysfs.c uses `ssize_t`
>>
>> // drivers/gpio/gpiolib-sysfs.c
>> static ssize_t value_show(struct device *dev,
>>struct device_attribute *attr, char *buf)
>> {
>>struct gpiod_data *data = dev_get_drvdata(dev);
>>struct gpio_desc *desc = data->desc;
>>ssize_t status;
>>
>>mutex_lock(>mutex);
>>
>>status = gpiod_get_value_cansleep(desc);
>> ...
>>return status;
>> }
>>
>> According to the book Advanced Programming in the UNIX Environment by
>> W. Richard Stevens,
>> With the 1990 POSIX.1 standard, the primitive system data type
>> ssize_t was introduced to provide the signed return value...
>>
>> So ssize_t is fairly common, for example, the read and write syscall
>> return a value of type ssize_t. But I haven't found out why ssize_t is
>> better int.
>> >
>
>Sorry if I wasn't clear, what prompted me to ask that question is the
following:
>`gc->get()` returns `int`, `get_gpio_pin_state()` returns `int`, yet you still
>save the return value of `get_gpio_pin_state()` into a variable with type
>`ssize_t` for no apparent reason. In the example you cited, `ssize_t` is used
>because the show() callback of a sysfs attribute must return `ssize_t`, but
here,
>`interrupt_line_active()` returns `bool`, so I don't see any advantage over a
>plain `int`. Anyways, I believe either one is fine, I just found it odd.
>
I don't understand why "the show() callback of a sysfs attribute
must return `ssize_t`" instead of int. Do you think the rationale
behind it is the same for this case? If yes, using "ssize_t" for
status could be justified.
[...]
Because it was decided that way, `ssize_t` is a better choice for that purpose
than plain `int`. You can see it in include/linux/device.h, that both the
show() and store() methods must return `ssize_t`.
Could you explain why `ssize_t` is a better choice? AFAIU, ssize_t
is used because we can return negative value to indicate an error. If
we use ssize_t here, it's a reminder that reading a GPIO pin's status
could fail. And ssize_t reminds us it's a operation similar to read
or write. So ssize_t is better than int here. And maybe it's the same
reason why "it was decided that way".
What I'm arguing here, is that there is no reason to use `ssize_t` in this case.
Because `get_gpio_pin_state()` returns `int`. So when you do
```
ssize_t status = get_gpio_pin_state(...);
```
then the return value of `get_gpio_pin_state()` (which is an `int`), will be
converted to an `ssize_t`, and saved into `status`. I'm arguing that that is
unnecessary and a plain `int` would work perfectly well in this case. Anyways,
both work fine, I just found the unnecessary use of `ssize_t` here odd.
Regards,
Barnabás Pőcze
--
Best regards,
Coiby
Re: [PATCH v3] HID: i2c-hid: add polling mode based on connected GPIO chip's pin status
On Sun, Nov 22, 2020 at 01:33:01PM +, Barnabás Pőcze wrote:
Hi
2020. november 22., vasárnap 11:15 keltezéssel, Coiby Xu Ãrta:
[...]
>> +static int get_gpio_pin_state(struct irq_desc *irq_desc)
>> +{
>> + struct gpio_chip *gc =
irq_data_get_irq_chip_data(_desc->irq_data);
>> +
>> + return gc->get(gc, irq_desc->irq_data.hwirq);
>> +}
[...]
>> + ssize_t status = get_gpio_pin_state(irq_desc);
>
>`get_gpio_pin_state()` returns an `int`, so I am not sure why `ssize_t` is
used here.
>
I used `ssize_t` because I found gpiolib-sysfs.c uses `ssize_t`
// drivers/gpio/gpiolib-sysfs.c
static ssize_t value_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
struct gpiod_data *data = dev_get_drvdata(dev);
struct gpio_desc *desc = data->desc;
ssize_t status;
mutex_lock(>mutex);
status = gpiod_get_value_cansleep(desc);
...
return status;
}
According to the book Advanced Programming in the UNIX Environment by
W. Richard Stevens,
With the 1990 POSIX.1 standard, the primitive system data type
ssize_t was introduced to provide the signed return value...
So ssize_t is fairly common, for example, the read and write syscall
return a value of type ssize_t. But I haven't found out why ssize_t is
better int.
>
Sorry if I wasn't clear, what prompted me to ask that question is the following:
`gc->get()` returns `int`, `get_gpio_pin_state()` returns `int`, yet you still
save the return value of `get_gpio_pin_state()` into a variable with type
`ssize_t` for no apparent reason. In the example you cited, `ssize_t` is used
because the show() callback of a sysfs attribute must return `ssize_t`, but
here,
`interrupt_line_active()` returns `bool`, so I don't see any advantage over a
plain `int`. Anyways, I believe either one is fine, I just found it odd.
I don't understand why "the show() callback of a sysfs attribute
must return `ssize_t`" instead of int. Do you think the rationale
behind it is the same for this case? If yes, using "ssize_t" for
status could be justified.
>> +
>> + if (status < 0) {
>> + dev_warn(>dev,
>> + "Failed to get GPIO Interrupt line status for %s",
>> + client->name);
>
>I think it's possible that the kernel message buffer is flooded with these
>messages, which is not optimal in my opinion.
>
Thank you! Replaced with dev_dbg in v4.
[...]
Have you looked at `dev_{warn,dbg,...}_ratelimited()`?
Thank you for pointing me to these functions!
Regards,
Barnabás Pőcze
--
Best regards,
Coiby
Re: [PATCH v3] HID: i2c-hid: add polling mode based on connected GPIO chip's pin status
Hi,
On Thu, Oct 22, 2020 at 02:22:51PM +, Barnabás Pőcze wrote:
Hi,
I think this looks a lot better than the first version, the issues around
suspend/resume are sorted out as far as I can see. However, I still have a
couple
comments, mainly minor ones.
Thank you for reviewing this patch!
[...]
+/* polling mode */
+#define I2C_HID_POLLING_DISABLED 0
+#define I2C_HID_POLLING_GPIO_PIN 1
+#define I2C_HID_POLLING_INTERVAL_ACTIVE_US 4000
+#define I2C_HID_POLLING_INTERVAL_IDLE_MS 10
+
+static u8 polling_mode;
+module_param(polling_mode, byte, 0444);
+MODULE_PARM_DESC(polling_mode, "How to poll - 0 disabled; 1 based on GPIO pin's
status");
+
Minor thing, but maybe the default value should be documented in the parameter
description?
+static unsigned int polling_interval_active_us =
I2C_HID_POLLING_INTERVAL_ACTIVE_US;
+module_param(polling_interval_active_us, uint, 0644);
+MODULE_PARM_DESC(polling_interval_active_us,
+"Poll every {polling_interval_active_us} us when the touchpad is
active. Default to 4000 us");
+
+static unsigned int polling_interval_idle_ms =
I2C_HID_POLLING_INTERVAL_IDLE_MS;
Since these two parameters are mostly read, I think the `__read_mostly`
attribute (linux/cache.h) is justified here.
+module_param(polling_interval_idle_ms, uint, 0644);
+MODULE_PARM_DESC(polling_interval_idle_ms,
+"Poll every {polling_interval_idle_ms} ms when the touchpad is
idle. Default to 10 ms");
This is minor stylistic thing; as far as I see, the prevalent pattern is to put
the default value at the end, in parenthesis:
E.g. "some parameter description (default=X)" or "... (default: X)" or
something similar
Maybe __stringify() (linux/stringify.h) could be used here and for the previous
module parameter?
E.g. "... (default=" __stringify(I2C_HID_POLLING_INTERVAL_IDLE_MS) ")"
Thank you for the above three suggestions! Will be applied in v4.
[...]
+static int get_gpio_pin_state(struct irq_desc *irq_desc)
+{
+ struct gpio_chip *gc = irq_data_get_irq_chip_data(_desc->irq_data);
+
+ return gc->get(gc, irq_desc->irq_data.hwirq);
+}
+
+static bool interrupt_line_active(struct i2c_client *client)
+{
+ unsigned long trigger_type = irq_get_trigger_type(client->irq);
Can the trigger type change? Because if not, then I think it'd be better to
store
the value somewhere and not query it every time.
The irq trigger type is obtained from ACPI so I don't think it won't
change.
+ struct irq_desc *irq_desc = irq_to_desc(client->irq);
Same here.
Thank you for the reminding!
+ ssize_t status = get_gpio_pin_state(irq_desc);
`get_gpio_pin_state()` returns an `int`, so I am not sure why `ssize_t` is used
here.
I used `ssize_t` because I found gpiolib-sysfs.c uses `ssize_t`
// drivers/gpio/gpiolib-sysfs.c
static ssize_t value_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
struct gpiod_data *data = dev_get_drvdata(dev);
struct gpio_desc *desc = data->desc;
ssize_t status;
mutex_lock(>mutex);
status = gpiod_get_value_cansleep(desc);
...
return status;
}
According to the book Advanced Programming in the UNIX Environment by
W. Richard Stevens,
With the 1990 POSIX.1 standard, the primitive system data type
ssize_t was introduced to provide the signed return value...
So ssize_t is fairly common, for example, the read and write syscall
return a value of type ssize_t. But I haven't found out why ssize_t is
better int.
+
+ if (status < 0) {
+ dev_warn(>dev,
+"Failed to get GPIO Interrupt line status for %s",
+client->name);
I think it's possible that the kernel message buffer is flooded with these
messages, which is not optimal in my opinion.
Thank you! Replaced with dev_dbg in v4.
+ return false;
+ }
+ /*
+* According to Windows Precsiontion Touchpad's specs
+*
https://docs.microsoft.com/en-us/windows-hardware/design/component-guidelines/windows-precision-touchpad-device-bus-connectivity,
+* GPIO Interrupt Assertion Leve could be either ActiveLow or
+* ActiveHigh.
+*/
+ if (trigger_type & IRQF_TRIGGER_LOW)
+ return !status;
+
+ return status;
+}
+
+static int i2c_hid_polling_thread(void *i2c_hid)
+{
+ struct i2c_hid *ihid = i2c_hid;
+ struct i2c_client *client = ihid->client;
+ unsigned int polling_interval_idle;
+
+ while (1) {
+ if (kthread_should_stop())
+ break;
I think this should be `while (!kthread_should_stop())`.
This simplifies the code. Thank you!
+
+ while (interrupt_line_active(client) &&
+ !test_bit(I2C_HID_READ_PENDING, >flags) &&
+ !kthread_should_stop()) {
+
Re: [PATCH v3 4/4] pinctrl: amd: remove debounce filter setting in IRQ type setting
On Mon, Nov 09, 2020 at 02:52:17PM +0100, Hans de Goede wrote: Hi, On 11/6/20 12:19 AM, Coiby Xu wrote: Debounce filter setting should be independent from IRQ type setting because according to the ACPI specs, there are separate arguments for specifying debounce timeout and IRQ type in GpioIo() and GpioInt(). This will fix broken touchpads for laptops whose BIOS set the debounce timeout to a relatively large value. For example, the BIOS of Lenovo Legion-5 AMD gaming laptops including 15ARH05 (R7000) and R7000P set the debounce timeout to 124.8ms. This led to the kernel receiving only ~7 HID reports per second from the Synaptics touchpad (MSFT0001:00 06CB:7F28). Existing touchpads like [1][2] are not troubled by this bug because the debounce timeout has been set to 0 by the BIOS before enabling the debounce filter in setting IRQ type. [1] https://github.com/Syniurge/i2c-amd-mp2/issues/11#issuecomment-721331582 [2] https://forum.manjaro.org/t/random-short-touchpad-freezes/30832/28 Cc: Hans de Goede Cc: Andy Shevchenko Cc: Benjamin Tissoires Cc: sta...@vger.kernel.org Reviewed-by: Andy Shevchenko BugLink: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1887190 Link: https://lore.kernel.org/linux-gpio/CAHp75VcwiGREBUJ0A06EEw-SyabqYsp%2Bdqs2DpSrhaY-2GVdAA%40mail.gmail.com/ Signed-off-by: Coiby Xu I'm not entirely sure about this patch. This is consistent with how we already stopped touching the debounce timeout setting during init, so that speaks in favor of this change. Still I'm worried a bit that this might have undesirable side effects. Now I can only confirm this patch won't affect the mentioned touchpads. I'll see if other distributions like Manjaro are willing to test it through the unstable channel. I guess this should be landed together with Andy's series to apply the debounce setting from the ACPI GPIO resources. Thank you for the reminding! You are right, Andy's patch "gpiolib: acpi: Take into account debounce settings" is needed to fix this kind of touchpad issues. Since that patch hasn't been merged, is there a way to refer to it in the commit message? Regards, Hans --- drivers/pinctrl/pinctrl-amd.c | 7 --- 1 file changed, 7 deletions(-) diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c index e9b761c2b77a..2d4acf21117c 100644 --- a/drivers/pinctrl/pinctrl-amd.c +++ b/drivers/pinctrl/pinctrl-amd.c @@ -468,7 +468,6 @@ static int amd_gpio_irq_set_type(struct irq_data *d, unsigned int type) pin_reg &= ~BIT(LEVEL_TRIG_OFF); pin_reg &= ~(ACTIVE_LEVEL_MASK << ACTIVE_LEVEL_OFF); pin_reg |= ACTIVE_HIGH << ACTIVE_LEVEL_OFF; - pin_reg |= DB_TYPE_REMOVE_GLITCH << DB_CNTRL_OFF; irq_set_handler_locked(d, handle_edge_irq); break; @@ -476,7 +475,6 @@ static int amd_gpio_irq_set_type(struct irq_data *d, unsigned int type) pin_reg &= ~BIT(LEVEL_TRIG_OFF); pin_reg &= ~(ACTIVE_LEVEL_MASK << ACTIVE_LEVEL_OFF); pin_reg |= ACTIVE_LOW << ACTIVE_LEVEL_OFF; - pin_reg |= DB_TYPE_REMOVE_GLITCH << DB_CNTRL_OFF; irq_set_handler_locked(d, handle_edge_irq); break; @@ -484,7 +482,6 @@ static int amd_gpio_irq_set_type(struct irq_data *d, unsigned int type) pin_reg &= ~BIT(LEVEL_TRIG_OFF); pin_reg &= ~(ACTIVE_LEVEL_MASK << ACTIVE_LEVEL_OFF); pin_reg |= BOTH_EADGE << ACTIVE_LEVEL_OFF; - pin_reg |= DB_TYPE_REMOVE_GLITCH << DB_CNTRL_OFF; irq_set_handler_locked(d, handle_edge_irq); break; @@ -492,8 +489,6 @@ static int amd_gpio_irq_set_type(struct irq_data *d, unsigned int type) pin_reg |= LEVEL_TRIGGER << LEVEL_TRIG_OFF; pin_reg &= ~(ACTIVE_LEVEL_MASK << ACTIVE_LEVEL_OFF); pin_reg |= ACTIVE_HIGH << ACTIVE_LEVEL_OFF; - pin_reg &= ~(DB_CNTRl_MASK << DB_CNTRL_OFF); - pin_reg |= DB_TYPE_PRESERVE_LOW_GLITCH << DB_CNTRL_OFF; irq_set_handler_locked(d, handle_level_irq); break; @@ -501,8 +496,6 @@ static int amd_gpio_irq_set_type(struct irq_data *d, unsigned int type) pin_reg |= LEVEL_TRIGGER << LEVEL_TRIG_OFF; pin_reg &= ~(ACTIVE_LEVEL_MASK << ACTIVE_LEVEL_OFF); pin_reg |= ACTIVE_LOW << ACTIVE_LEVEL_OFF; - pin_reg &= ~(DB_CNTRl_MASK << DB_CNTRL_OFF); - pin_reg |= DB_TYPE_PRESERVE_HIGH_GLITCH << DB_CNTRL_OFF; irq_set_handler_locked(d, handle_level_irq); break; -- Best regards, Coiby
Re: [PATCH v2 2/4] pinctrl: amd: use higher precision for 512 RtcClk
On Thu, Nov 05, 2020 at 01:13:07PM +0200, Andy Shevchenko wrote:
On Thu, Nov 5, 2020 at 1:07 AM Coiby Xu wrote:
RTC is 32.768kHz thus 512 RtcClk equals 15625 usec.
I could add that the documentation likely has dropped precision and
that's why the driver mistakenly took the slightly deviated value.
Thank you for the suggestion and other fixes in patch 4/4. Applied in v3.
Anyway.
Reviewed-by: Andy Shevchenko
Suggested-by: Andy Shevchenko
Suggested-by: Hans de Goede
Link:
https://lore.kernel.org/linux-gpio/2f4706a1-502f-75f0-9596-cc25b4933...@redhat.com/
Signed-off-by: Coiby Xu
---
drivers/pinctrl/pinctrl-amd.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
index d6b2b4bd337c..4aea3e05e8c6 100644
--- a/drivers/pinctrl/pinctrl-amd.c
+++ b/drivers/pinctrl/pinctrl-amd.c
@@ -156,7 +156,7 @@ static int amd_gpio_set_debounce(struct gpio_chip *gc,
unsigned offset,
pin_reg |= BIT(DB_TMR_OUT_UNIT_OFF);
pin_reg &= ~BIT(DB_TMR_LARGE_OFF);
} else if (debounce < 25) {
- time = debounce / 15600;
+ time = debounce / 15625;
pin_reg |= time & DB_TMR_OUT_MASK;
pin_reg &= ~BIT(DB_TMR_OUT_UNIT_OFF);
pin_reg |= BIT(DB_TMR_LARGE_OFF);
--
2.28.0
--
With Best Regards,
Andy Shevchenko
--
Best regards,
Coiby
[PATCH v3 1/4] pinctrl: amd: fix incorrect way to disable debounce filter
The correct way to disable debounce filter is to clear bit 5 and 6
of the register.
Cc: Hans de Goede
Link:
https://lore.kernel.org/linux-gpio/df2c008b-e7b5-4fdd-42ea-4d1c62b52...@redhat.com/
Signed-off-by: Coiby Xu
---
drivers/pinctrl/pinctrl-amd.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
index 9a760f5cd7ed..d6b2b4bd337c 100644
--- a/drivers/pinctrl/pinctrl-amd.c
+++ b/drivers/pinctrl/pinctrl-amd.c
@@ -166,14 +166,14 @@ static int amd_gpio_set_debounce(struct gpio_chip *gc,
unsigned offset,
pin_reg |= BIT(DB_TMR_OUT_UNIT_OFF);
pin_reg |= BIT(DB_TMR_LARGE_OFF);
} else {
- pin_reg &= ~DB_CNTRl_MASK;
+ pin_reg &= ~(DB_CNTRl_MASK << DB_CNTRL_OFF);
ret = -EINVAL;
}
} else {
pin_reg &= ~BIT(DB_TMR_OUT_UNIT_OFF);
pin_reg &= ~BIT(DB_TMR_LARGE_OFF);
pin_reg &= ~DB_TMR_OUT_MASK;
- pin_reg &= ~DB_CNTRl_MASK;
+ pin_reg &= ~(DB_CNTRl_MASK << DB_CNTRL_OFF);
}
writel(pin_reg, gpio_dev->base + offset * 4);
raw_spin_unlock_irqrestore(_dev->lock, flags);
--
2.28.0
[PATCH v3 3/4] pinctrl: amd: print debounce filter info in debugfs
Print the status of debounce filter as follows,
$ cat /sys/kernel/debug/gpio
pin129 interrupt is disabled| interrupt is masked| disable wakeup in
S0i3 state| disable wakeup in S3 state|
disable wakeup in S4/S5 state| input is high| pull-up is disabled| Pull-down
is disabled| output is disabled| debouncing filter disabled| 0x5
^^
pin130 interrupt is disabled| interrupt is masked| disable wakeup in
S0i3 state| disable wakeup in S3 state|
disable wakeup in S4/S5 state| input is high| pull-up is disabled| Pull-down
is disabled| output is disabled| debouncing filter (high) enabled| debouncing
timeout is 124800 (us)| 0x503c8
Signed-off-by: Coiby Xu
---
drivers/pinctrl/pinctrl-amd.c | 43 +--
1 file changed, 41 insertions(+), 2 deletions(-)
diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
index 4aea3e05e8c6..e9b761c2b77a 100644
--- a/drivers/pinctrl/pinctrl-amd.c
+++ b/drivers/pinctrl/pinctrl-amd.c
@@ -197,10 +197,16 @@ static int amd_gpio_set_config(struct gpio_chip *gc,
unsigned offset,
static void amd_gpio_dbg_show(struct seq_file *s, struct gpio_chip *gc)
{
u32 pin_reg;
+ u32 db_cntrl;
unsigned long flags;
unsigned int bank, i, pin_num;
struct amd_gpio *gpio_dev = gpiochip_get_data(gc);
+ bool tmr_out_unit;
+ unsigned int time;
+ unsigned int unit;
+ bool tmr_large;
+
char *level_trig;
char *active_level;
char *interrupt_enable;
@@ -214,6 +220,8 @@ static void amd_gpio_dbg_show(struct seq_file *s, struct
gpio_chip *gc)
char *pull_down_enable;
char *output_value;
char *output_enable;
+ char debounce_value[40];
+ char *debounce_enable;
for (bank = 0; bank < gpio_dev->hwbank_num; bank++) {
seq_printf(s, "GPIO bank%d\t", bank);
@@ -327,13 +335,44 @@ static void amd_gpio_dbg_show(struct seq_file *s, struct
gpio_chip *gc)
pin_sts = "input is low|";
}
+ db_cntrl = (DB_CNTRl_MASK << DB_CNTRL_OFF) & pin_reg;
+ if (db_cntrl) {
+ tmr_out_unit = pin_reg &
BIT(DB_TMR_OUT_UNIT_OFF);
+ tmr_large = pin_reg & BIT(DB_TMR_LARGE_OFF);
+ time = pin_reg & DB_TMR_OUT_MASK;
+ if (tmr_large) {
+ if (tmr_out_unit)
+ unit = 62500;
+ else
+ unit = 15625;
+ } else {
+ if (tmr_out_unit)
+ unit = 244;
+ else
+ unit = 61;
+ }
+ if ((DB_TYPE_REMOVE_GLITCH << DB_CNTRL_OFF) ==
db_cntrl)
+ debounce_enable = "debouncing filter
(high and low) enabled|";
+ else if ((DB_TYPE_PRESERVE_LOW_GLITCH <<
DB_CNTRL_OFF) == db_cntrl)
+ debounce_enable = "debouncing filter
(low) enabled|";
+ else
+ debounce_enable = "debouncing filter
(high) enabled|";
+
+ snprintf(debounce_value, sizeof(debounce_value),
+"debouncing timeout is %u (us)|", time
* unit);
+ } else {
+ debounce_enable = "debouncing filter disabled|";
+ snprintf(debounce_value,
sizeof(debounce_value), " ");
+ }
+
seq_printf(s, "%s %s %s %s %s %s\n"
- " %s %s %s %s %s %s %s 0x%x\n",
+ " %s %s %s %s %s %s %s %s %s 0x%x\n",
level_trig, active_level, interrupt_enable,
interrupt_mask, wake_cntrl0, wake_cntrl1,
wake_cntrl2, pin_sts, pull_up_sel,
pull_up_enable, pull_down_enable,
- output_value, output_enable, pin_reg);
+ output_value, output_enable,
+ debounce_enable, debounce_value, pin_reg);
}
}
}
--
2.28.0
[PATCH v3 4/4] pinctrl: amd: remove debounce filter setting in IRQ type setting
Debounce filter setting should be independent from IRQ type setting because according to the ACPI specs, there are separate arguments for specifying debounce timeout and IRQ type in GpioIo() and GpioInt(). This will fix broken touchpads for laptops whose BIOS set the debounce timeout to a relatively large value. For example, the BIOS of Lenovo Legion-5 AMD gaming laptops including 15ARH05 (R7000) and R7000P set the debounce timeout to 124.8ms. This led to the kernel receiving only ~7 HID reports per second from the Synaptics touchpad (MSFT0001:00 06CB:7F28). Existing touchpads like [1][2] are not troubled by this bug because the debounce timeout has been set to 0 by the BIOS before enabling the debounce filter in setting IRQ type. [1] https://github.com/Syniurge/i2c-amd-mp2/issues/11#issuecomment-721331582 [2] https://forum.manjaro.org/t/random-short-touchpad-freezes/30832/28 Cc: Hans de Goede Cc: Andy Shevchenko Cc: Benjamin Tissoires Cc: sta...@vger.kernel.org Reviewed-by: Andy Shevchenko BugLink: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1887190 Link: https://lore.kernel.org/linux-gpio/CAHp75VcwiGREBUJ0A06EEw-SyabqYsp%2Bdqs2DpSrhaY-2GVdAA%40mail.gmail.com/ Signed-off-by: Coiby Xu --- drivers/pinctrl/pinctrl-amd.c | 7 --- 1 file changed, 7 deletions(-) diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c index e9b761c2b77a..2d4acf21117c 100644 --- a/drivers/pinctrl/pinctrl-amd.c +++ b/drivers/pinctrl/pinctrl-amd.c @@ -468,7 +468,6 @@ static int amd_gpio_irq_set_type(struct irq_data *d, unsigned int type) pin_reg &= ~BIT(LEVEL_TRIG_OFF); pin_reg &= ~(ACTIVE_LEVEL_MASK << ACTIVE_LEVEL_OFF); pin_reg |= ACTIVE_HIGH << ACTIVE_LEVEL_OFF; - pin_reg |= DB_TYPE_REMOVE_GLITCH << DB_CNTRL_OFF; irq_set_handler_locked(d, handle_edge_irq); break; @@ -476,7 +475,6 @@ static int amd_gpio_irq_set_type(struct irq_data *d, unsigned int type) pin_reg &= ~BIT(LEVEL_TRIG_OFF); pin_reg &= ~(ACTIVE_LEVEL_MASK << ACTIVE_LEVEL_OFF); pin_reg |= ACTIVE_LOW << ACTIVE_LEVEL_OFF; - pin_reg |= DB_TYPE_REMOVE_GLITCH << DB_CNTRL_OFF; irq_set_handler_locked(d, handle_edge_irq); break; @@ -484,7 +482,6 @@ static int amd_gpio_irq_set_type(struct irq_data *d, unsigned int type) pin_reg &= ~BIT(LEVEL_TRIG_OFF); pin_reg &= ~(ACTIVE_LEVEL_MASK << ACTIVE_LEVEL_OFF); pin_reg |= BOTH_EADGE << ACTIVE_LEVEL_OFF; - pin_reg |= DB_TYPE_REMOVE_GLITCH << DB_CNTRL_OFF; irq_set_handler_locked(d, handle_edge_irq); break; @@ -492,8 +489,6 @@ static int amd_gpio_irq_set_type(struct irq_data *d, unsigned int type) pin_reg |= LEVEL_TRIGGER << LEVEL_TRIG_OFF; pin_reg &= ~(ACTIVE_LEVEL_MASK << ACTIVE_LEVEL_OFF); pin_reg |= ACTIVE_HIGH << ACTIVE_LEVEL_OFF; - pin_reg &= ~(DB_CNTRl_MASK << DB_CNTRL_OFF); - pin_reg |= DB_TYPE_PRESERVE_LOW_GLITCH << DB_CNTRL_OFF; irq_set_handler_locked(d, handle_level_irq); break; @@ -501,8 +496,6 @@ static int amd_gpio_irq_set_type(struct irq_data *d, unsigned int type) pin_reg |= LEVEL_TRIGGER << LEVEL_TRIG_OFF; pin_reg &= ~(ACTIVE_LEVEL_MASK << ACTIVE_LEVEL_OFF); pin_reg |= ACTIVE_LOW << ACTIVE_LEVEL_OFF; - pin_reg &= ~(DB_CNTRl_MASK << DB_CNTRL_OFF); - pin_reg |= DB_TYPE_PRESERVE_HIGH_GLITCH << DB_CNTRL_OFF; irq_set_handler_locked(d, handle_level_irq); break; -- 2.28.0
[PATCH v3 2/4] pinctrl: amd: use higher precision for 512 RtcClk
RTC is 32.768kHz thus 512 RtcClk equals 15625 usec. The documentation
likely has dropped precision and that's why the driver mistakenly took
the slightly deviated value.
Reported-by: Andy Shevchenko
Suggested-by: Andy Shevchenko
Suggested-by: Hans de Goede
Reviewed-by: Andy Shevchenko
Link:
https://lore.kernel.org/linux-gpio/2f4706a1-502f-75f0-9596-cc25b4933...@redhat.com/
Signed-off-by: Coiby Xu
---
drivers/pinctrl/pinctrl-amd.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
index d6b2b4bd337c..4aea3e05e8c6 100644
--- a/drivers/pinctrl/pinctrl-amd.c
+++ b/drivers/pinctrl/pinctrl-amd.c
@@ -156,7 +156,7 @@ static int amd_gpio_set_debounce(struct gpio_chip *gc,
unsigned offset,
pin_reg |= BIT(DB_TMR_OUT_UNIT_OFF);
pin_reg &= ~BIT(DB_TMR_LARGE_OFF);
} else if (debounce < 25) {
- time = debounce / 15600;
+ time = debounce / 15625;
pin_reg |= time & DB_TMR_OUT_MASK;
pin_reg &= ~BIT(DB_TMR_OUT_UNIT_OFF);
pin_reg |= BIT(DB_TMR_LARGE_OFF);
--
2.28.0
Re: [PATCH 4/4] pinctrl: amd: remove debounce filter setting in irq type setting
On Wed, Nov 04, 2020 at 10:42:38PM +0200, Andy Shevchenko wrote: On Wed, Nov 4, 2020 at 6:05 PM Coiby Xu wrote: Debounce filter setting should be independent from irq type setting because according to the ACPI specs, there are separate arguments for specifying debounce timeout and irq type in GpioIo and GpioInt. This will fix broken touchpads for Lenovo Legion-5 AMD gaming laptops including 15ARH05 (R7000) and R7000P whose BIOS set the debounce timeout to 124.8ms which led to kernel receiving only ~7 HID reports per second. to the kernel Thank you for correcting my grammar mistakes! Cc: Hans de Goede Cc: Andy Shevchenko Cc: 1887...@bugs.launchpad.net BugLink: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1887190 Message-Id: Link: https://lore.kernel.org/... -- With Best Regards, Andy Shevchenko -- Best regards, Coiby
Re: [PATCH 1/4] pinctrl: amd: fix incorrect way to disable debounce filter
On Wed, Nov 04, 2020 at 10:38:32PM +0200, Andy Shevchenko wrote: On Wed, Nov 4, 2020 at 6:05 PM Coiby Xu wrote: The correct way to disable debounce filter is to clear bit 5 and 6 of the register. Cc: Hans de Goede Message-ID: Can you use a Link tag with proper lore.kernel.org URL? Thank you for the suggestion. Applied in v2. -- With Best Regards, Andy Shevchenko -- Best regards, Coiby
[PATCH v2 2/4] pinctrl: amd: use higher precision for 512 RtcClk
RTC is 32.768kHz thus 512 RtcClk equals 15625 usec.
Suggested-by: Andy Shevchenko
Suggested-by: Hans de Goede
Link:
https://lore.kernel.org/linux-gpio/2f4706a1-502f-75f0-9596-cc25b4933...@redhat.com/
Signed-off-by: Coiby Xu
---
drivers/pinctrl/pinctrl-amd.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
index d6b2b4bd337c..4aea3e05e8c6 100644
--- a/drivers/pinctrl/pinctrl-amd.c
+++ b/drivers/pinctrl/pinctrl-amd.c
@@ -156,7 +156,7 @@ static int amd_gpio_set_debounce(struct gpio_chip *gc,
unsigned offset,
pin_reg |= BIT(DB_TMR_OUT_UNIT_OFF);
pin_reg &= ~BIT(DB_TMR_LARGE_OFF);
} else if (debounce < 25) {
- time = debounce / 15600;
+ time = debounce / 15625;
pin_reg |= time & DB_TMR_OUT_MASK;
pin_reg &= ~BIT(DB_TMR_OUT_UNIT_OFF);
pin_reg |= BIT(DB_TMR_LARGE_OFF);
--
2.28.0
[PATCH v2 1/4] pinctrl: amd: fix incorrect way to disable debounce filter
The correct way to disable debounce filter is to clear bit 5 and 6
of the register.
Cc: Hans de Goede
Link:
https://lore.kernel.org/linux-gpio/df2c008b-e7b5-4fdd-42ea-4d1c62b52...@redhat.com/
Signed-off-by: Coiby Xu
---
drivers/pinctrl/pinctrl-amd.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
index 9a760f5cd7ed..d6b2b4bd337c 100644
--- a/drivers/pinctrl/pinctrl-amd.c
+++ b/drivers/pinctrl/pinctrl-amd.c
@@ -166,14 +166,14 @@ static int amd_gpio_set_debounce(struct gpio_chip *gc,
unsigned offset,
pin_reg |= BIT(DB_TMR_OUT_UNIT_OFF);
pin_reg |= BIT(DB_TMR_LARGE_OFF);
} else {
- pin_reg &= ~DB_CNTRl_MASK;
+ pin_reg &= ~(DB_CNTRl_MASK << DB_CNTRL_OFF);
ret = -EINVAL;
}
} else {
pin_reg &= ~BIT(DB_TMR_OUT_UNIT_OFF);
pin_reg &= ~BIT(DB_TMR_LARGE_OFF);
pin_reg &= ~DB_TMR_OUT_MASK;
- pin_reg &= ~DB_CNTRl_MASK;
+ pin_reg &= ~(DB_CNTRl_MASK << DB_CNTRL_OFF);
}
writel(pin_reg, gpio_dev->base + offset * 4);
raw_spin_unlock_irqrestore(_dev->lock, flags);
--
2.28.0
[PATCH v2 4/4] pinctrl: amd: remove debounce filter setting in irq type setting
Debounce filter setting should be independent from irq type setting because according to the ACPI specs, there are separate arguments for specifying debounce timeout and irq type in GpioIo and GpioInt. This will fix broken touchpads for Lenovo Legion-5 AMD gaming laptops including 15ARH05 (R7000) and R7000P whose BIOS set the debounce timeout to 124.8ms which led to the kernel receiving only ~7 HID reports per second. Cc: Hans de Goede Cc: Andy Shevchenko Cc: Benjamin Tissoires BugLink: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1887190 Link: https://lore.kernel.org/linux-gpio/CAHp75VcwiGREBUJ0A06EEw-SyabqYsp%2Bdqs2DpSrhaY-2GVdAA%40mail.gmail.com/ Signed-off-by: Coiby Xu --- drivers/pinctrl/pinctrl-amd.c | 7 --- 1 file changed, 7 deletions(-) diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c index e9b761c2b77a..2d4acf21117c 100644 --- a/drivers/pinctrl/pinctrl-amd.c +++ b/drivers/pinctrl/pinctrl-amd.c @@ -468,7 +468,6 @@ static int amd_gpio_irq_set_type(struct irq_data *d, unsigned int type) pin_reg &= ~BIT(LEVEL_TRIG_OFF); pin_reg &= ~(ACTIVE_LEVEL_MASK << ACTIVE_LEVEL_OFF); pin_reg |= ACTIVE_HIGH << ACTIVE_LEVEL_OFF; - pin_reg |= DB_TYPE_REMOVE_GLITCH << DB_CNTRL_OFF; irq_set_handler_locked(d, handle_edge_irq); break; @@ -476,7 +475,6 @@ static int amd_gpio_irq_set_type(struct irq_data *d, unsigned int type) pin_reg &= ~BIT(LEVEL_TRIG_OFF); pin_reg &= ~(ACTIVE_LEVEL_MASK << ACTIVE_LEVEL_OFF); pin_reg |= ACTIVE_LOW << ACTIVE_LEVEL_OFF; - pin_reg |= DB_TYPE_REMOVE_GLITCH << DB_CNTRL_OFF; irq_set_handler_locked(d, handle_edge_irq); break; @@ -484,7 +482,6 @@ static int amd_gpio_irq_set_type(struct irq_data *d, unsigned int type) pin_reg &= ~BIT(LEVEL_TRIG_OFF); pin_reg &= ~(ACTIVE_LEVEL_MASK << ACTIVE_LEVEL_OFF); pin_reg |= BOTH_EADGE << ACTIVE_LEVEL_OFF; - pin_reg |= DB_TYPE_REMOVE_GLITCH << DB_CNTRL_OFF; irq_set_handler_locked(d, handle_edge_irq); break; @@ -492,8 +489,6 @@ static int amd_gpio_irq_set_type(struct irq_data *d, unsigned int type) pin_reg |= LEVEL_TRIGGER << LEVEL_TRIG_OFF; pin_reg &= ~(ACTIVE_LEVEL_MASK << ACTIVE_LEVEL_OFF); pin_reg |= ACTIVE_HIGH << ACTIVE_LEVEL_OFF; - pin_reg &= ~(DB_CNTRl_MASK << DB_CNTRL_OFF); - pin_reg |= DB_TYPE_PRESERVE_LOW_GLITCH << DB_CNTRL_OFF; irq_set_handler_locked(d, handle_level_irq); break; @@ -501,8 +496,6 @@ static int amd_gpio_irq_set_type(struct irq_data *d, unsigned int type) pin_reg |= LEVEL_TRIGGER << LEVEL_TRIG_OFF; pin_reg &= ~(ACTIVE_LEVEL_MASK << ACTIVE_LEVEL_OFF); pin_reg |= ACTIVE_LOW << ACTIVE_LEVEL_OFF; - pin_reg &= ~(DB_CNTRl_MASK << DB_CNTRL_OFF); - pin_reg |= DB_TYPE_PRESERVE_HIGH_GLITCH << DB_CNTRL_OFF; irq_set_handler_locked(d, handle_level_irq); break; -- 2.28.0
[PATCH v2 3/4] pinctrl: amd: print debounce filter info in debugfs
Print the status of debounce filter as follows,
$ cat /sys/kernel/debug/gpio
pin129 interrupt is disabled| interrupt is masked| disable wakeup in
S0i3 state| disable wakeup in S3 state|
disable wakeup in S4/S5 state| input is high| pull-up is disabled| Pull-down
is disabled| output is disabled| debouncing filter disabled| 0x5
^^
pin130 interrupt is disabled| interrupt is masked| disable wakeup in
S0i3 state| disable wakeup in S3 state|
disable wakeup in S4/S5 state| input is high| pull-up is disabled| Pull-down
is disabled| output is disabled| debouncing filter (high) enabled| debouncing
timeout is 124800 (us)| 0x503c8
Cc: Andy Shevchenko
Signed-off-by: Coiby Xu
---
drivers/pinctrl/pinctrl-amd.c | 43 +--
1 file changed, 41 insertions(+), 2 deletions(-)
diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
index 4aea3e05e8c6..e9b761c2b77a 100644
--- a/drivers/pinctrl/pinctrl-amd.c
+++ b/drivers/pinctrl/pinctrl-amd.c
@@ -197,10 +197,16 @@ static int amd_gpio_set_config(struct gpio_chip *gc,
unsigned offset,
static void amd_gpio_dbg_show(struct seq_file *s, struct gpio_chip *gc)
{
u32 pin_reg;
+ u32 db_cntrl;
unsigned long flags;
unsigned int bank, i, pin_num;
struct amd_gpio *gpio_dev = gpiochip_get_data(gc);
+ bool tmr_out_unit;
+ unsigned int time;
+ unsigned int unit;
+ bool tmr_large;
+
char *level_trig;
char *active_level;
char *interrupt_enable;
@@ -214,6 +220,8 @@ static void amd_gpio_dbg_show(struct seq_file *s, struct
gpio_chip *gc)
char *pull_down_enable;
char *output_value;
char *output_enable;
+ char debounce_value[40];
+ char *debounce_enable;
for (bank = 0; bank < gpio_dev->hwbank_num; bank++) {
seq_printf(s, "GPIO bank%d\t", bank);
@@ -327,13 +335,44 @@ static void amd_gpio_dbg_show(struct seq_file *s, struct
gpio_chip *gc)
pin_sts = "input is low|";
}
+ db_cntrl = (DB_CNTRl_MASK << DB_CNTRL_OFF) & pin_reg;
+ if (db_cntrl) {
+ tmr_out_unit = pin_reg &
BIT(DB_TMR_OUT_UNIT_OFF);
+ tmr_large = pin_reg & BIT(DB_TMR_LARGE_OFF);
+ time = pin_reg & DB_TMR_OUT_MASK;
+ if (tmr_large) {
+ if (tmr_out_unit)
+ unit = 62500;
+ else
+ unit = 15625;
+ } else {
+ if (tmr_out_unit)
+ unit = 244;
+ else
+ unit = 61;
+ }
+ if ((DB_TYPE_REMOVE_GLITCH << DB_CNTRL_OFF) ==
db_cntrl)
+ debounce_enable = "debouncing filter
(high and low) enabled|";
+ else if ((DB_TYPE_PRESERVE_LOW_GLITCH <<
DB_CNTRL_OFF) == db_cntrl)
+ debounce_enable = "debouncing filter
(low) enabled|";
+ else
+ debounce_enable = "debouncing filter
(high) enabled|";
+
+ snprintf(debounce_value, sizeof(debounce_value),
+"debouncing timeout is %u (us)|", time
* unit);
+ } else {
+ debounce_enable = "debouncing filter disabled|";
+ snprintf(debounce_value,
sizeof(debounce_value), " ");
+ }
+
seq_printf(s, "%s %s %s %s %s %s\n"
- " %s %s %s %s %s %s %s 0x%x\n",
+ " %s %s %s %s %s %s %s %s %s 0x%x\n",
level_trig, active_level, interrupt_enable,
interrupt_mask, wake_cntrl0, wake_cntrl1,
wake_cntrl2, pin_sts, pull_up_sel,
pull_up_enable, pull_down_enable,
- output_value, output_e
[PATCH 4/4] pinctrl: amd: remove debounce filter setting in irq type setting
Debounce filter setting should be independent from irq type setting because according to the ACPI specs, there are separate arguments for specifying debounce timeout and irq type in GpioIo and GpioInt. This will fix broken touchpads for Lenovo Legion-5 AMD gaming laptops including 15ARH05 (R7000) and R7000P whose BIOS set the debounce timeout to 124.8ms which led to kernel receiving only ~7 HID reports per second. Cc: Hans de Goede Cc: Andy Shevchenko Cc: 1887...@bugs.launchpad.net BugLink: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1887190 Message-Id: Signed-off-by: Coiby Xu --- drivers/pinctrl/pinctrl-amd.c | 7 --- 1 file changed, 7 deletions(-) diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c index 524d55546b61..5a1d518b563e 100644 --- a/drivers/pinctrl/pinctrl-amd.c +++ b/drivers/pinctrl/pinctrl-amd.c @@ -468,7 +468,6 @@ static int amd_gpio_irq_set_type(struct irq_data *d, unsigned int type) pin_reg &= ~BIT(LEVEL_TRIG_OFF); pin_reg &= ~(ACTIVE_LEVEL_MASK << ACTIVE_LEVEL_OFF); pin_reg |= ACTIVE_HIGH << ACTIVE_LEVEL_OFF; - pin_reg |= DB_TYPE_REMOVE_GLITCH << DB_CNTRL_OFF; irq_set_handler_locked(d, handle_edge_irq); break; @@ -476,7 +475,6 @@ static int amd_gpio_irq_set_type(struct irq_data *d, unsigned int type) pin_reg &= ~BIT(LEVEL_TRIG_OFF); pin_reg &= ~(ACTIVE_LEVEL_MASK << ACTIVE_LEVEL_OFF); pin_reg |= ACTIVE_LOW << ACTIVE_LEVEL_OFF; - pin_reg |= DB_TYPE_REMOVE_GLITCH << DB_CNTRL_OFF; irq_set_handler_locked(d, handle_edge_irq); break; @@ -484,7 +482,6 @@ static int amd_gpio_irq_set_type(struct irq_data *d, unsigned int type) pin_reg &= ~BIT(LEVEL_TRIG_OFF); pin_reg &= ~(ACTIVE_LEVEL_MASK << ACTIVE_LEVEL_OFF); pin_reg |= BOTH_EADGE << ACTIVE_LEVEL_OFF; - pin_reg |= DB_TYPE_REMOVE_GLITCH << DB_CNTRL_OFF; irq_set_handler_locked(d, handle_edge_irq); break; @@ -492,8 +489,6 @@ static int amd_gpio_irq_set_type(struct irq_data *d, unsigned int type) pin_reg |= LEVEL_TRIGGER << LEVEL_TRIG_OFF; pin_reg &= ~(ACTIVE_LEVEL_MASK << ACTIVE_LEVEL_OFF); pin_reg |= ACTIVE_HIGH << ACTIVE_LEVEL_OFF; - pin_reg &= ~(DB_CNTRl_MASK << DB_CNTRL_OFF); - pin_reg |= DB_TYPE_PRESERVE_LOW_GLITCH << DB_CNTRL_OFF; irq_set_handler_locked(d, handle_level_irq); break; @@ -501,8 +496,6 @@ static int amd_gpio_irq_set_type(struct irq_data *d, unsigned int type) pin_reg |= LEVEL_TRIGGER << LEVEL_TRIG_OFF; pin_reg &= ~(ACTIVE_LEVEL_MASK << ACTIVE_LEVEL_OFF); pin_reg |= ACTIVE_LOW << ACTIVE_LEVEL_OFF; - pin_reg &= ~(DB_CNTRl_MASK << DB_CNTRL_OFF); - pin_reg |= DB_TYPE_PRESERVE_HIGH_GLITCH << DB_CNTRL_OFF; irq_set_handler_locked(d, handle_level_irq); break; -- 2.28.0
[PATCH 2/4] pinctrl: amd: use higher precision for 512 RtcClk
RTC is 32.768kHz thus 512 RtcClk equals to 15625 usec.
Reported-by: Andy Shevchenko
Suggested-by: Andy Shevchenko
Suggested-by: Hans de Goede
Message-ID: <2f4706a1-502f-75f0-9596-cc25b4933...@redhat.com>
Message-ID:
Signed-off-by: Coiby Xu
---
drivers/pinctrl/pinctrl-amd.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
index d6b2b4bd337c..4aea3e05e8c6 100644
--- a/drivers/pinctrl/pinctrl-amd.c
+++ b/drivers/pinctrl/pinctrl-amd.c
@@ -156,7 +156,7 @@ static int amd_gpio_set_debounce(struct gpio_chip *gc,
unsigned offset,
pin_reg |= BIT(DB_TMR_OUT_UNIT_OFF);
pin_reg &= ~BIT(DB_TMR_LARGE_OFF);
} else if (debounce < 25) {
- time = debounce / 15600;
+ time = debounce / 15625;
pin_reg |= time & DB_TMR_OUT_MASK;
pin_reg &= ~BIT(DB_TMR_OUT_UNIT_OFF);
pin_reg |= BIT(DB_TMR_LARGE_OFF);
--
2.28.0
[PATCH 3/4] pinctrl: amd: print debounce filter info in debugfs
Print the status of debounce filter as follows,
$ cat /sys/kernel/debug/gpio
pin129 interrupt is disabled| interrupt is masked| disable wakeup in
S0i3 state| disable wakeup in S3 state|
disable wakeup in S4/S5 state| input is high| pull-up is disabled| Pull-down
is disabled| output is disabled| debouncing filter disabled| 0x5
^^
pin130 interrupt is disabled| interrupt is masked| disable wakeup in
S0i3 state| disable wakeup in S3 state|
disable wakeup in S4/S5 state| input is high| pull-up is disabled| Pull-down
is disabled| output is disabled| debouncing filter (high) enabled| debouncing
timeout is 124800 (us)| 0x503c8
Signed-off-by: Coiby Xu
---
drivers/pinctrl/pinctrl-amd.c | 43 +--
1 file changed, 41 insertions(+), 2 deletions(-)
diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
index 4aea3e05e8c6..524d55546b61 100644
--- a/drivers/pinctrl/pinctrl-amd.c
+++ b/drivers/pinctrl/pinctrl-amd.c
@@ -197,10 +197,16 @@ static int amd_gpio_set_config(struct gpio_chip *gc,
unsigned offset,
static void amd_gpio_dbg_show(struct seq_file *s, struct gpio_chip *gc)
{
u32 pin_reg;
+ u32 db_cntrl;
unsigned long flags;
unsigned int bank, i, pin_num;
struct amd_gpio *gpio_dev = gpiochip_get_data(gc);
+ bool tmr_out_unit;
+ unsigned int time;
+ unsigned int unit;
+ bool tmr_large;
+
char *level_trig;
char *active_level;
char *interrupt_enable;
@@ -214,6 +220,8 @@ static void amd_gpio_dbg_show(struct seq_file *s, struct
gpio_chip *gc)
char *pull_down_enable;
char *output_value;
char *output_enable;
+ char debounce_value[40];
+ char *debounce_enable;
for (bank = 0; bank < gpio_dev->hwbank_num; bank++) {
seq_printf(s, "GPIO bank%d\t", bank);
@@ -327,13 +335,44 @@ static void amd_gpio_dbg_show(struct seq_file *s, struct
gpio_chip *gc)
pin_sts = "input is low|";
}
+ db_cntrl = (DB_CNTRl_MASK << DB_CNTRL_OFF) & pin_reg;
+ if (db_cntrl) {
+ tmr_out_unit = pin_reg &
BIT(DB_TMR_OUT_UNIT_OFF);
+ tmr_large = pin_reg & BIT(DB_TMR_LARGE_OFF);
+ time = pin_reg & DB_TMR_OUT_MASK;
+ if (tmr_large) {
+ if (tmr_out_unit)
+ unit = 62500;
+ else
+ unit = 15625;
+ } else {
+ if (tmr_out_unit)
+ unit = 244;
+ else
+ unit = 61;
+ }
+ if ((DB_TYPE_REMOVE_GLITCH << DB_CNTRL_OFF) ==
db_cntrl)
+ debounce_enable = "debouncing filter
(high and low) enabled|";
+ else if ((DB_TYPE_PRESERVE_LOW_GLITCH <<
DB_CNTRL_OFF) == db_cntrl)
+ debounce_enable = "debouncing filter
(low) enabled|";
+ else
+ debounce_enable = "debouncing filter
(high) enabled|";
+
+ snprintf(debounce_value, sizeof(debounce_value),
+"debouncing timeout is %u (us)|", time
* unit);
+ } else {
+ debounce_enable = "debouncing filter disabled|";
+ snprintf(debounce_value,
sizeof(debounce_value), " ");
+ }
+
seq_printf(s, "%s %s %s %s %s %s\n"
- " %s %s %s %s %s %s %s 0x%x\n",
+ " %s %s %s %s %s %s %s %s %s 0x%x\n",
level_trig, active_level, interrupt_enable,
interrupt_mask, wake_cntrl0, wake_cntrl1,
wake_cntrl2, pin_sts, pull_up_sel,
pull_up_enable, pull_down_enable,
- output_value, output_enable, pin_reg);
+ output_value, output_enable,
+ debounce_enable, debounce_value, pin_reg);
}
}
}
--
2.28.0
[PATCH 1/4] pinctrl: amd: fix incorrect way to disable debounce filter
The correct way to disable debounce filter is to clear bit 5 and 6
of the register.
Cc: Hans de Goede
Message-ID:
Signed-off-by: Coiby Xu
---
drivers/pinctrl/pinctrl-amd.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
index 9a760f5cd7ed..d6b2b4bd337c 100644
--- a/drivers/pinctrl/pinctrl-amd.c
+++ b/drivers/pinctrl/pinctrl-amd.c
@@ -166,14 +166,14 @@ static int amd_gpio_set_debounce(struct gpio_chip *gc,
unsigned offset,
pin_reg |= BIT(DB_TMR_OUT_UNIT_OFF);
pin_reg |= BIT(DB_TMR_LARGE_OFF);
} else {
- pin_reg &= ~DB_CNTRl_MASK;
+ pin_reg &= ~(DB_CNTRl_MASK << DB_CNTRL_OFF);
ret = -EINVAL;
}
} else {
pin_reg &= ~BIT(DB_TMR_OUT_UNIT_OFF);
pin_reg &= ~BIT(DB_TMR_LARGE_OFF);
pin_reg &= ~DB_TMR_OUT_MASK;
- pin_reg &= ~DB_CNTRl_MASK;
+ pin_reg &= ~(DB_CNTRl_MASK << DB_CNTRL_OFF);
}
writel(pin_reg, gpio_dev->base + offset * 4);
raw_spin_unlock_irqrestore(_dev->lock, flags);
--
2.28.0
Re: [PATCH 01/15] iio: accel: remove unnecessary CONFIG_PM_SLEEP
On Sat, Oct 31, 2020 at 11:05:11AM +, Jonathan Cameron wrote: On Fri, 30 Oct 2020 22:34:10 +0800 Coiby Xu wrote: On Thu, Oct 29, 2020 at 07:06:40PM +0200, Andy Shevchenko wrote: >On Thu, Oct 29, 2020 at 4:42 PM Jonathan Cameron wrote: >> On Thu, 29 Oct 2020 15:48:56 +0800 >> Coiby Xu wrote: > >> Please put a cover letter on your next series explaining the context. >> In this particular case some of the replies you have gotten are >> general at it is a lot easier to find these sorts of things via >> replying to the cover letter. > >Looking at the number of duplicate messages I would suggest that one >needs to go through documentation on how to use git format-patch and >git send-email. > Thank you for the suggestion! Actually it's a tree-wide change and it seems the kernel community prefer individual patches or series for subsystems having the same maintainer over a huge patch set so I wrote some scripts to automate the process. That's why you see ~50 emails with almost the same commit message. The only difference of these commit messages is the name of PM macro. When doing a bit set like this, it's worth sending out a small subset first to shake out issue like those seen here. Once those get merged then send out out the reset. Thank you for the suggestion! Actually I've held off another ~150 emails and these ~200 emails were only part of work. I thought it's better to reach 4 or 5 subsystem to collect sufficient feedbacks considering some subsystems may respond slow. But I didn't realize a better way is to cut down the size of patch set sent to a subsystem. Thanks, Jonathan >-- >With Best Regards, >Andy Shevchenko -- Best regards, Coiby -- Best regards, Coiby
Re: [PATCH 1/9] mfd: maxim: remove unnecessary CONFIG_PM_SLEEP
On Thu, Oct 29, 2020 at 07:32:26PM +0100, Krzysztof Kozlowski wrote:
On Thu, Oct 29, 2020 at 06:06:39PM +0800, Coiby Xu wrote:
SIMPLE_DEV_PM_OPS has already took good care of CONFIG_PM_CONFIG.
I don't quite get what did you mean by "took good care". This should
cause warnings of unused structure. Comment applies to other patches as
well.
Sorry I made a mistake.
Also, the title prefix is: "mfd: max77686:"
Thank you for the reminding! I'll improve my script to avoid this issue
when extracting prefix from git log.
Best regards,
Krzysztof
Signed-off-by: Coiby Xu
---
drivers/mfd/max77686.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/mfd/max77686.c b/drivers/mfd/max77686.c
index 71faf503844b..8c701f8a9dd5 100644
--- a/drivers/mfd/max77686.c
+++ b/drivers/mfd/max77686.c
@@ -227,7 +227,6 @@ static int max77686_i2c_probe(struct i2c_client *i2c)
return 0;
}
-#ifdef CONFIG_PM_SLEEP
static int max77686_suspend(struct device *dev)
{
struct i2c_client *i2c = to_i2c_client(dev);
@@ -262,7 +261,6 @@ static int max77686_resume(struct device *dev)
return 0;
}
-#endif /* CONFIG_PM_SLEEP */
static SIMPLE_DEV_PM_OPS(max77686_pm, max77686_suspend, max77686_resume);
--
2.28.0
--
Best regards,
Coiby
Re: [PATCH 9/9] mfd: sprd-sc27xx-spi: remove unnecessary CONFIG_PM_SLEEP
Hi Chunyan, On Fri, Oct 30, 2020 at 12:02:03PM +0800, Chunyan Zhang wrote: Hi Coiby, After removing CONFIG_PM_SLEEP, sprd_pmic_suspend/resume() would not be built into symbol table with clang compiler though, that would cause clang compiler report warnings of "unused function" if CONFIG_PM_SLEEP is not set. So I also prefer to add a __maybe_unused instead as other people suggested in the mail list. Thank you for the suggestion! At least Lee prefers to CONFIG_PM_SLEEP thus to keep the status quo. I'll see he'll change his mind with the ongoing discussion [1]:) [1] https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg2361371.html Thanks, Chunyan On Thu, 29 Oct 2020 at 18:07, Coiby Xu wrote: SIMPLE_DEV_PM_OPS has already took good care of CONFIG_PM_CONFIG. Signed-off-by: Coiby Xu drivers/mfd/sprd-sc27xx-spi.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/mfd/sprd-sc27xx-spi.c b/drivers/mfd/sprd-sc27xx-spi.c index 6b7956604a0f..4db2ec9ef2ff 100644 --- a/drivers/mfd/sprd-sc27xx-spi.c +++ b/drivers/mfd/sprd-sc27xx-spi.c @@ -206,7 +206,6 @@ static int sprd_pmic_probe(struct spi_device *spi) return 0; -#ifdef CONFIG_PM_SLEEP static int sprd_pmic_suspend(struct device *dev) struct sprd_pmic *ddata = dev_get_drvdata(dev); @@ -226,7 +225,6 @@ static int sprd_pmic_resume(struct device *dev) return 0; -#endif static SIMPLE_DEV_PM_OPS(sprd_pmic_pm_ops, sprd_pmic_suspend, sprd_pmic_resume); 2.28.0 -- Best regards, Coiby
Re: [PATCH 01/15] iio: accel: remove unnecessary CONFIG_PM_SLEEP
On Thu, Oct 29, 2020 at 07:06:40PM +0200, Andy Shevchenko wrote: On Thu, Oct 29, 2020 at 4:42 PM Jonathan Cameron wrote: On Thu, 29 Oct 2020 15:48:56 +0800 Coiby Xu wrote: Please put a cover letter on your next series explaining the context. In this particular case some of the replies you have gotten are general at it is a lot easier to find these sorts of things via replying to the cover letter. Looking at the number of duplicate messages I would suggest that one needs to go through documentation on how to use git format-patch and git send-email. Thank you for the suggestion! Actually it's a tree-wide change and it seems the kernel community prefer individual patches or series for subsystems having the same maintainer over a huge patch set so I wrote some scripts to automate the process. That's why you see ~50 emails with almost the same commit message. The only difference of these commit messages is the name of PM macro. -- With Best Regards, Andy Shevchenko -- Best regards, Coiby
Re: [PATCH 3/9] mfd: intel_soc_pmic: remove unnecessary CONFIG_PM_SLEEP
On Thu, Oct 29, 2020 at 07:04:44PM +0200, Andy Shevchenko wrote: On Thu, Oct 29, 2020 at 5:27 PM Lee Jones wrote: On Thu, 29 Oct 2020, Coiby Xu wrote: > On Thu, Oct 29, 2020 at 01:00:29PM +0200, Andy Shevchenko wrote: > > On Thu, Oct 29, 2020 at 06:06:41PM +0800, Coiby Xu wrote: > > > SIMPLE_DEV_PM_OPS has already took good care of CONFIG_PM_CONFIG. > > > > Have you compiled this with > > % make W=1 ... > > ? > > > > Sorry my bad. I thought I had run "make modules" with CONFIG_PM_SLEEP > disabled. I'll run "make W=1 M=..." for each driver after adding > __maybe_unused in v2. No, thank you. Just keep it as it is. The current code is space saving. Perhaps you need to go thru __maybe_unused handling. There are pros and cons of each approach, but not above. Can you elaborate on the pros and cons of each approach? There's convincing reason to prefer __maybe_unused over CONFIG_PM_SLEEP according to Arnd Bergmann [1], > By and large, drivers handle this by using a CONFIG_PM_SLEEP ifdef. > > Unless you can make an extremely convincing argument why not to do > so here, I'd like you to handle it that way instead. [adding linux-pm to Cc] The main reason is that everyone gets the #ifdef wrong, I run into half a dozen new build regressions with linux-next every week on average, the typical problems being: - testing CONFIG_PM_SLEEP instead of CONFIG_PM, leading to an unused function warning - testing CONFIG_PM instead of CONFIG_PM_SLEEP, leading to a build failure - calling a function outside of the #ifdef only from inside an otherwise correct #ifdef, again leading to an unused function warning - causing a warning inside of the #ifdef but only testing if that is disabled, leading to a problem if the macro is set (this is rare these days for CONFIG_PM as that is normally enabled) Using __maybe_unused avoids all of the above. You option is valuable to me because I'm making a tree-wide change. Currently there are 929 drivers having device PM callbacks, $ grep -rI "\.pm = &" --include=*.c ./|wc -l 929 I put all files having device PM callbacks into four categories based on weather a file has CONFIG_PM_SLEEP or PM macro like SET_SYSTEM_SLEEP_PM_OPS, here are the statistics,  1. have both CONFIG_PM_SLEEP and PM_OPS macro: 213  2. have CONFIG_PM_SLEEP but no PM_OPS macro: 19  3. have PM macro but not CONFIG_PM_SLEEP: 347  4. no PM macro or CONFIG_PM_SLEEP: 302 [1] https://lore.kernel.org/patchwork/comment/919944/ -- With Best Regards, Andy Shevchenko -- Best regards, Coiby
Re: [PATCH 2/3] watchdog: st_lpc_wdt: remove unnecessary CONFIG_PM_SLEEP
On Thu, Oct 29, 2020 at 07:58:30AM +, Patrice CHOTARD wrote:
Hi Coiby
On 10/29/20 8:53 AM, Coiby Xu wrote:
SIMPLE_DEV_PM_OPS has already took good care of CONFIG_PM_CONFIG.
Signed-off-by: Coiby Xu
---
drivers/watchdog/st_lpc_wdt.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/watchdog/st_lpc_wdt.c b/drivers/watchdog/st_lpc_wdt.c
index 14ab6559c748..c1428d63dc9e 100644
--- a/drivers/watchdog/st_lpc_wdt.c
+++ b/drivers/watchdog/st_lpc_wdt.c
@@ -248,7 +248,6 @@ static int st_wdog_remove(struct platform_device *pdev)
return 0;
}
-#ifdef CONFIG_PM_SLEEP
static int st_wdog_suspend(struct device *dev)
{
struct st_wdog *st_wdog = watchdog_get_drvdata(_wdog_dev);
@@ -285,7 +284,6 @@ static int st_wdog_resume(struct device *dev)
return 0;
}
-#endif
static SIMPLE_DEV_PM_OPS(st_wdog_pm_ops,
st_wdog_suspend,
Reviewed-by: Patrice Chotard
Thank you for reviewing the patch and giving the credit despite
the compiling issue with CONFIG_PM_SLEEP disabled as pointed by
Guenter and others:)
Thanks
Patrice
--
Best regards,
Coiby
Re: [PATCH 01/15] iio: accel: remove unnecessary CONFIG_PM_SLEEP
HI Jonathan,
On Thu, Oct 29, 2020 at 02:40:07PM +, Jonathan Cameron wrote:
On Thu, 29 Oct 2020 15:48:56 +0800
Coiby Xu wrote:
SIMPLE_DEV_PM_OPS has already took good care of CONFIG_PM_CONFIG.
Signed-off-by: Coiby Xu
Hi Coiby,
Please put a cover letter on your next series explaining the context.
In this particular case some of the replies you have gotten are
general at it is a lot easier to find these sorts of things via
replying to the cover letter.
I will do it in v2. Thank you for the suggestion!
Jonathan
---
drivers/iio/accel/da311.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/iio/accel/da311.c b/drivers/iio/accel/da311.c
index 3b3df620ba27..55d4891556ca 100644
--- a/drivers/iio/accel/da311.c
+++ b/drivers/iio/accel/da311.c
@@ -263,7 +263,6 @@ static int da311_remove(struct i2c_client *client)
return da311_enable(client, false);
}
-#ifdef CONFIG_PM_SLEEP
static int da311_suspend(struct device *dev)
{
return da311_enable(to_i2c_client(dev), false);
@@ -273,7 +272,6 @@ static int da311_resume(struct device *dev)
{
return da311_enable(to_i2c_client(dev), true);
}
-#endif
static SIMPLE_DEV_PM_OPS(da311_pm_ops, da311_suspend, da311_resume);
--
Best regards,
Coiby
Re: [PATCH 05/15] iio: accel: remove unnecessary CONFIG_PM_SLEEP
On Thu, Oct 29, 2020 at 01:57:25PM +0200, Andy Shevchenko wrote: On Thu, Oct 29, 2020 at 11:05 AM Coiby Xu wrote: SET_SYSTEM_SLEEP_PM_OPS has already took good care of CONFIG_PM_CONFIG. Correct grammar and rethink about them. NAK for all of them. I'll add __maybe_unused, do the compiling testing for each changed driver and also correct the typo in v2. Thank you for the feedback! -- With Best Regards, Andy Shevchenko -- Best regards, Coiby
Re: [PATCH 01/25] ALSA: core: pcm: remove unnecessary CONFIG_PM_SLEEP
On Thu, Oct 29, 2020 at 08:48:55AM +0100, Takashi Iwai wrote:
On Thu, 29 Oct 2020 08:42:37 +0100,
Coiby Xu wrote:
SET_SYSTEM_SLEEP_PM_OPS has already took good care of CONFIG_PM_CONFIG.
Signed-off-by: Coiby Xu
It caused compile warnings. Was it already addressed in general?
It hasn't been addressed in general. Thank you for the reminding!
Or we may use __maybe_unused attribute instead, but it's just a matter
of taste.
I'll add __maybe_unused in v2 since __maybe_unused should be preferred over
CONFIG_PM_SLEEP according to Arnd Bergmann [1],
> By and large, drivers handle this by using a CONFIG_PM_SLEEP ifdef.
>
> Unless you can make an extremely convincing argument why not to do
> so here, I'd like you to handle it that way instead.
[adding linux-pm to Cc]
The main reason is that everyone gets the #ifdef wrong, I run into
half a dozen new build regressions with linux-next every week on
average, the typical problems being:
- testing CONFIG_PM_SLEEP instead of CONFIG_PM, leading to an unused
function warning
- testing CONFIG_PM instead of CONFIG_PM_SLEEP, leading to a build
failure
- calling a function outside of the #ifdef only from inside an
otherwise correct #ifdef, again leading to an unused function
warning
- causing a warning inside of the #ifdef but only testing if that
is disabled, leading to a problem if the macro is set (this is
rare these days for CONFIG_PM as that is normally enabled)
Using __maybe_unused avoids all of the above.
[1] https://lore.kernel.org/patchwork/comment/919944/
thanks,
Takashi
---
sound/core/pcm.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/sound/core/pcm.c b/sound/core/pcm.c
index be5714f1bb58..5a281ac92958 100644
--- a/sound/core/pcm.c
+++ b/sound/core/pcm.c
@@ -599,7 +599,6 @@ static const struct attribute_group *pcm_dev_attr_groups[];
* PM callbacks: we need to deal only with suspend here, as the resume is
* triggered either from user-space or the driver's resume callback
*/
-#ifdef CONFIG_PM_SLEEP
static int do_pcm_suspend(struct device *dev)
{
struct snd_pcm_str *pstr = container_of(dev, struct snd_pcm_str, dev);
@@ -608,7 +607,6 @@ static int do_pcm_suspend(struct device *dev)
snd_pcm_suspend_all(pstr->pcm);
return 0;
}
-#endif
static const struct dev_pm_ops pcm_dev_pm_ops = {
SET_SYSTEM_SLEEP_PM_OPS(do_pcm_suspend, NULL)
--
2.28.0
--
Best regards,
Coiby
Re: [PATCH 3/9] mfd: intel_soc_pmic: remove unnecessary CONFIG_PM_SLEEP
On Thu, Oct 29, 2020 at 01:00:29PM +0200, Andy Shevchenko wrote: On Thu, Oct 29, 2020 at 06:06:41PM +0800, Coiby Xu wrote: SIMPLE_DEV_PM_OPS has already took good care of CONFIG_PM_CONFIG. Have you compiled this with % make W=1 ... ? Sorry my bad. I thought I had run "make modules" with CONFIG_PM_SLEEP disabled. I'll run "make W=1 M=..." for each driver after adding __maybe_unused in v2. -- With Best Regards, Andy Shevchenko -- Best regards, Coiby
Re: [PATCH 5/5] i2c: stm32: remove unnecessary CONFIG_PM_SLEEP
Hi Fabrice,
On Thu, Oct 29, 2020 at 12:31:54PM +0100, Fabrice Gasnier wrote:
On 10/29/20 8:46 AM, Coiby Xu wrote:
SET_SYSTEM_SLEEP_PM_OPS has already took good care of CONFIG_PM_CONFIG.
Signed-off-by: Coiby Xu
---
drivers/i2c/busses/i2c-stm32f7.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/i2c/busses/i2c-stm32f7.c b/drivers/i2c/busses/i2c-stm32f7.c
index f41f51a176a1..95ac9dfdf458 100644
--- a/drivers/i2c/busses/i2c-stm32f7.c
+++ b/drivers/i2c/busses/i2c-stm32f7.c
@@ -2262,7 +2262,6 @@ static int __maybe_unused
stm32f7_i2c_runtime_resume(struct device *dev)
return 0;
}
-#ifdef CONFIG_PM_SLEEP
Hi Coiby,
This generates warnings when building with W=1 and CONFIG_PM_SLEEP=n.
Could you please add also "__maybe_unused" for relevant routines below ?
static int stm32f7_i2c_regs_backup(struct stm32f7_i2c_dev *i2c_dev)
^
e.g. insert: __maybe_unused
Thank you for the suggestion. I'll add "__maybe_unused" in v2.
Best regards,
Fabrice
{
int ret;
@@ -2356,7 +2355,6 @@ static int stm32f7_i2c_resume(struct device *dev)
return 0;
}
-#endif
static const struct dev_pm_ops stm32f7_i2c_pm_ops = {
SET_RUNTIME_PM_OPS(stm32f7_i2c_runtime_suspend,
--
Best regards,
Coiby
Re: [PATCH 3/3] watchdog: sirfsoc_wdt: remove unnecessary CONFIG_PM_SLEEP
On Thu, Oct 29, 2020 at 06:05:21AM -0700, Guenter Roeck wrote:
On 10/29/20 12:53 AM, Coiby Xu wrote:
SIMPLE_DEV_PM_OPS has already took good care of CONFIG_PM_CONFIG.
Signed-off-by: Coiby Xu
---
drivers/watchdog/sirfsoc_wdt.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/watchdog/sirfsoc_wdt.c b/drivers/watchdog/sirfsoc_wdt.c
index 734cf2966ecb..dc8341cd7d44 100644
--- a/drivers/watchdog/sirfsoc_wdt.c
+++ b/drivers/watchdog/sirfsoc_wdt.c
@@ -170,7 +170,6 @@ static int sirfsoc_wdt_probe(struct platform_device *pdev)
return 0;
}
-#ifdef CONFIG_PM_SLEEP
static int sirfsoc_wdt_suspend(struct device *dev)
And again: __maybe_unused
I would suggest to test compile the code with CONFIG_PM_SLEEP disabled.
I will test it before send v2! Thank you for the feedback!
{
return 0;
@@ -189,7 +188,6 @@ static int sirfsoc_wdt_resume(struct device *dev)
return 0;
}
-#endif
static SIMPLE_DEV_PM_OPS(sirfsoc_wdt_pm_ops,
sirfsoc_wdt_suspend, sirfsoc_wdt_resume);
--
Best regards,
Coiby
Re: [PATCH 04/25] ASoC: rockchip: remove unnecessary CONFIG_PM_SLEEP
On Thu, Oct 29, 2020 at 10:33:52AM +, Robin Murphy wrote:
On 2020-10-29 07:42, Coiby Xu wrote:
SET_SYSTEM_SLEEP_PM_OPS has already took good care of CONFIG_PM_CONFIG.
I don't see anything in the !CONFIG_PM_CONFIG side of
SET_SYSTEM_SLEEP_PM_OPS() that prevents unused function warnings for
the callbacks - does this change depend on some other patch or is it
just wrong?
Thank you for the feedback! I'll add "__maybe_unused" in v2.
Robin.
Signed-off-by: Coiby Xu
---
sound/soc/rockchip/rockchip_pdm.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/sound/soc/rockchip/rockchip_pdm.c
b/sound/soc/rockchip/rockchip_pdm.c
index 5adb293d0435..f3c19310aeeb 100644
--- a/sound/soc/rockchip/rockchip_pdm.c
+++ b/sound/soc/rockchip/rockchip_pdm.c
@@ -574,7 +574,6 @@ static int rockchip_pdm_remove(struct platform_device *pdev)
return 0;
}
-#ifdef CONFIG_PM_SLEEP
static int rockchip_pdm_suspend(struct device *dev)
{
struct rk_pdm_dev *pdm = dev_get_drvdata(dev);
@@ -601,7 +600,6 @@ static int rockchip_pdm_resume(struct device *dev)
return ret;
}
-#endif
static const struct dev_pm_ops rockchip_pdm_pm_ops = {
SET_RUNTIME_PM_OPS(rockchip_pdm_runtime_suspend,
--
Best regards,
Coiby
Re: [PATCH] power: supply: olpc_battery: remove unnecessary CONFIG_PM_SLEEP
On Thu, Oct 29, 2020 at 12:09:23PM +0100, Hans de Goede wrote: Hi, On 10/29/20 11:59 AM, Coiby Xu wrote: Hi Hans, Thank you for reviewing this patch! On Thu, Oct 29, 2020 at 11:04:36AM +0100, Hans de Goede wrote: Hi, On 10/29/20 8:41 AM, Coiby Xu wrote: SIMPLE_DEV_PM_OPS has already took good care of CONFIG_PM_CONFIG. No it does not, when CONFIG_PM_SLEEP is not set then the SET_SYSTEM_SLEEP_PM_OPS macro which SIMPLE_DEV_PM_OPS uses is a no-op, so nothing will reference xo15_sci_resume leading to a compiler warning when CONFIG_PM_SLEEP is not set. You could drop the ifdef and add __maybe_unused to the definition of xo15_sci_resume, but that feels like needless churn, best to just keep this as is IMHO. Actually, this is a tree-wide change by some semi-automation scripts. Thank you for pointing out the issue to prevent me from releasing another ~150 emails to flood other mailing lists. Currently there are 929 drivers has device PM callbacks, $ grep -rI "\.pm = &" --include=*.c ./|wc -l 929 I put all files having device PM callbacks into four categories based on weather a file has CONFIG_PM_SLEEP or PM macro like SET_SYSTEM_SLEEP_PM_OPS, here are the statistics,  1. have both CONFIG_PM_SLEEP and PM_OPS macro: 213  2. have CONFIG_PM_SLEEP but no PM_OPS macro: 19  3. have PM macro but not CONFIG_PM_SLEEP: 347  4. no PM macro or CONFIG_PM_SLEEP: 302 Some drivers which have PM macro but not CONFIG_PM_SLEEP like sound/x86/intel_hdmi_audio.c indeed use __maybe_unused to eliminate the compiling warning. In 2011, there's a patch proposing to remove ONFIG_PM altogether but an objection was turning CONFIG_PM on would increase the kernel size [1]. So __maybe_unused also have this issue. I would expect the compiler to remove the unused function, it knows it is unused, that is why __maybe_unused is necessary to suppress the warning and compilers are pretty smart and agressive wrt remove unnecessary code these days. Then __maybe_unused is a good solution and there's also convincing reason to prefer __maybe_unused over CONFIG_PM_SLEEP according to Arnd Bergmann [2], > By and large, drivers handle this by using a CONFIG_PM_SLEEP ifdef. > > Unless you can make an extremely convincing argument why not to do > so here, I'd like you to handle it that way instead. [adding linux-pm to Cc] The main reason is that everyone gets the #ifdef wrong, I run into half a dozen new build regressions with linux-next every week on average, the typical problems being: - testing CONFIG_PM_SLEEP instead of CONFIG_PM, leading to an unused function warning - testing CONFIG_PM instead of CONFIG_PM_SLEEP, leading to a build failure - calling a function outside of the #ifdef only from inside an otherwise correct #ifdef, again leading to an unused function warning - causing a warning inside of the #ifdef but only testing if that is disabled, leading to a problem if the macro is set (this is rare these days for CONFIG_PM as that is normally enabled) Using __maybe_unused avoids all of the above. Regards, Hans [2] https://lore.kernel.org/patchwork/comment/919944/ -- Best regards, Coiby
Re: [PATCH] power: supply: olpc_battery: remove unnecessary CONFIG_PM_SLEEP
Hi Hans,
Thank you for reviewing this patch!
On Thu, Oct 29, 2020 at 11:04:36AM +0100, Hans de Goede wrote:
Hi,
On 10/29/20 8:41 AM, Coiby Xu wrote:
SIMPLE_DEV_PM_OPS has already took good care of CONFIG_PM_CONFIG.
No it does not, when CONFIG_PM_SLEEP is not set then the
SET_SYSTEM_SLEEP_PM_OPS macro which SIMPLE_DEV_PM_OPS uses
is a no-op, so nothing will reference xo15_sci_resume leading to
a compiler warning when CONFIG_PM_SLEEP is not set.
You could drop the ifdef and add __maybe_unused to the definition
of xo15_sci_resume, but that feels like needless churn, best to
just keep this as is IMHO.
Actually, this is a tree-wide change by some semi-automation scripts.
Thank you for pointing out the issue to prevent me from releasing
another ~150 emails to flood other mailing lists.
Currently there are 929 drivers has device PM callbacks,
$ grep -rI "\.pm = &" --include=*.c ./|wc -l
929
I put all files having device PM callbacks into four categories
based on weather a file has CONFIG_PM_SLEEP or PM macro like
SET_SYSTEM_SLEEP_PM_OPS, here are the statistics,
1. have both CONFIG_PM_SLEEP and PM_OPS macro: 213
2. have CONFIG_PM_SLEEP but no PM_OPS macro: 19
3. have PM macro but not CONFIG_PM_SLEEP: 347
4. no PM macro or CONFIG_PM_SLEEP: 302
Some drivers which have PM macro but not CONFIG_PM_SLEEP like
sound/x86/intel_hdmi_audio.c indeed use __maybe_unused to eliminate
the compiling warning. In 2011, there's a patch proposing to remove
ONFIG_PM altogether but an objection was turning CONFIG_PM on would
increase the kernel size [1]. So __maybe_unused also have this issue.
(I made a mistake when I thought PM macros like SIMPLE_DEV_PM_OPS
didn't have this issue). What do you think? Btw, It's easy for me to
add CONFIG_PM_SLEEP for those drivers have PM macro but not
CONFIG_PM_SLEEP since I have already written the necessary automation
scripts.
[1]
https://lists.linux-foundation.org/pipermail/linux-pm/2011-February/030215.html
Also s/CONFIG_PM_CONFIG/CONFIG_PM_SLEEP/ in the commit message.
Thank you for pointing out the typo. I've written some scripts to
automate the whole process from changing code to submitting patches.
Somehow there is still this issue.
Regards,
Hans
Signed-off-by: Coiby Xu
---
arch/x86/platform/olpc/olpc-xo15-sci.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/arch/x86/platform/olpc/olpc-xo15-sci.c
b/arch/x86/platform/olpc/olpc-xo15-sci.c
index 85f4638764d6..716eefd735a4 100644
--- a/arch/x86/platform/olpc/olpc-xo15-sci.c
+++ b/arch/x86/platform/olpc/olpc-xo15-sci.c
@@ -192,7 +192,6 @@ static int xo15_sci_remove(struct acpi_device *device)
return 0;
}
-#ifdef CONFIG_PM_SLEEP
static int xo15_sci_resume(struct device *dev)
{
/* Enable all EC events */
@@ -204,7 +203,6 @@ static int xo15_sci_resume(struct device *dev)
return 0;
}
-#endif
static SIMPLE_DEV_PM_OPS(xo15_sci_pm, NULL, xo15_sci_resume);
--
Best regards,
Coiby
[PATCH 1/9] mfd: maxim: remove unnecessary CONFIG_PM_SLEEP
SIMPLE_DEV_PM_OPS has already took good care of CONFIG_PM_CONFIG.
Signed-off-by: Coiby Xu
---
drivers/mfd/max77686.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/mfd/max77686.c b/drivers/mfd/max77686.c
index 71faf503844b..8c701f8a9dd5 100644
--- a/drivers/mfd/max77686.c
+++ b/drivers/mfd/max77686.c
@@ -227,7 +227,6 @@ static int max77686_i2c_probe(struct i2c_client *i2c)
return 0;
}
-#ifdef CONFIG_PM_SLEEP
static int max77686_suspend(struct device *dev)
{
struct i2c_client *i2c = to_i2c_client(dev);
@@ -262,7 +261,6 @@ static int max77686_resume(struct device *dev)
return 0;
}
-#endif /* CONFIG_PM_SLEEP */
static SIMPLE_DEV_PM_OPS(max77686_pm, max77686_suspend, max77686_resume);
--
2.28.0
[PATCH 2/9] mfd: motorola-cpcap: remove unnecessary CONFIG_PM_SLEEP
SIMPLE_DEV_PM_OPS has already took good care of CONFIG_PM_CONFIG.
Signed-off-by: Coiby Xu
---
drivers/mfd/motorola-cpcap.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/mfd/motorola-cpcap.c b/drivers/mfd/motorola-cpcap.c
index 2283d88adcc2..97c07a5641c5 100644
--- a/drivers/mfd/motorola-cpcap.c
+++ b/drivers/mfd/motorola-cpcap.c
@@ -214,7 +214,6 @@ static const struct regmap_config cpcap_regmap_config = {
.val_format_endian = REGMAP_ENDIAN_LITTLE,
};
-#ifdef CONFIG_PM_SLEEP
static int cpcap_suspend(struct device *dev)
{
struct spi_device *spi = to_spi_device(dev);
@@ -232,7 +231,6 @@ static int cpcap_resume(struct device *dev)
return 0;
}
-#endif
static SIMPLE_DEV_PM_OPS(cpcap_pm, cpcap_suspend, cpcap_resume);
--
2.28.0
[PATCH 7/9] mfd: sec: remove unnecessary CONFIG_PM_SLEEP
SIMPLE_DEV_PM_OPS has already took good care of CONFIG_PM_CONFIG.
Signed-off-by: Coiby Xu
---
drivers/mfd/sec-core.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/mfd/sec-core.c b/drivers/mfd/sec-core.c
index 95473ff9bb4b..a5d19798d671 100644
--- a/drivers/mfd/sec-core.c
+++ b/drivers/mfd/sec-core.c
@@ -496,7 +496,6 @@ static void sec_pmic_shutdown(struct i2c_client *i2c)
regmap_update_bits(sec_pmic->regmap_pmic, reg, mask, 0);
}
-#ifdef CONFIG_PM_SLEEP
static int sec_pmic_suspend(struct device *dev)
{
struct i2c_client *i2c = to_i2c_client(dev);
@@ -529,7 +528,6 @@ static int sec_pmic_resume(struct device *dev)
return 0;
}
-#endif /* CONFIG_PM_SLEEP */
static SIMPLE_DEV_PM_OPS(sec_pmic_pm_ops, sec_pmic_suspend, sec_pmic_resume);
--
2.28.0
[PATCH 6/9] mfd: stmfx: remove unnecessary CONFIG_PM_SLEEP
SIMPLE_DEV_PM_OPS has already took good care of CONFIG_PM_CONFIG.
Signed-off-by: Coiby Xu
---
drivers/mfd/stmfx.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/mfd/stmfx.c b/drivers/mfd/stmfx.c
index 5e680bfdf5c9..e760cf2be02e 100644
--- a/drivers/mfd/stmfx.c
+++ b/drivers/mfd/stmfx.c
@@ -469,7 +469,6 @@ static int stmfx_remove(struct i2c_client *client)
return stmfx_chip_exit(client);
}
-#ifdef CONFIG_PM_SLEEP
static int stmfx_suspend(struct device *dev)
{
struct stmfx *stmfx = dev_get_drvdata(dev);
@@ -535,7 +534,6 @@ static int stmfx_resume(struct device *dev)
return 0;
}
-#endif
static SIMPLE_DEV_PM_OPS(stmfx_dev_pm_ops, stmfx_suspend, stmfx_resume);
--
2.28.0
[PATCH 9/9] mfd: sprd-sc27xx-spi: remove unnecessary CONFIG_PM_SLEEP
SIMPLE_DEV_PM_OPS has already took good care of CONFIG_PM_CONFIG.
Signed-off-by: Coiby Xu
---
drivers/mfd/sprd-sc27xx-spi.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/mfd/sprd-sc27xx-spi.c b/drivers/mfd/sprd-sc27xx-spi.c
index 6b7956604a0f..4db2ec9ef2ff 100644
--- a/drivers/mfd/sprd-sc27xx-spi.c
+++ b/drivers/mfd/sprd-sc27xx-spi.c
@@ -206,7 +206,6 @@ static int sprd_pmic_probe(struct spi_device *spi)
return 0;
}
-#ifdef CONFIG_PM_SLEEP
static int sprd_pmic_suspend(struct device *dev)
{
struct sprd_pmic *ddata = dev_get_drvdata(dev);
@@ -226,7 +225,6 @@ static int sprd_pmic_resume(struct device *dev)
return 0;
}
-#endif
static SIMPLE_DEV_PM_OPS(sprd_pmic_pm_ops, sprd_pmic_suspend,
sprd_pmic_resume);
--
2.28.0
[PATCH 4/9] mfd: max77620: remove unnecessary CONFIG_PM_SLEEP
SET_SYSTEM_SLEEP_PM_OPS has already took good care of CONFIG_PM_CONFIG.
Signed-off-by: Coiby Xu
---
drivers/mfd/max77620.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/mfd/max77620.c b/drivers/mfd/max77620.c
index fec2096474ad..17ea0ae6408d 100644
--- a/drivers/mfd/max77620.c
+++ b/drivers/mfd/max77620.c
@@ -574,7 +574,6 @@ static int max77620_probe(struct i2c_client *client,
return 0;
}
-#ifdef CONFIG_PM_SLEEP
static int max77620_set_fps_period(struct max77620_chip *chip,
int fps_id, int time_period)
{
@@ -681,7 +680,6 @@ static int max77620_i2c_resume(struct device *dev)
return 0;
}
-#endif
static const struct i2c_device_id max77620_id[] = {
{"max77620", MAX77620},
--
2.28.0
[PATCH 8/9] mfd: max14577: remove unnecessary CONFIG_PM_SLEEP
SIMPLE_DEV_PM_OPS has already took good care of CONFIG_PM_CONFIG.
Signed-off-by: Coiby Xu
---
drivers/mfd/max14577.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/mfd/max14577.c b/drivers/mfd/max14577.c
index be185e9d5f16..c619dd8172d5 100644
--- a/drivers/mfd/max14577.c
+++ b/drivers/mfd/max14577.c
@@ -482,7 +482,6 @@ static const struct i2c_device_id max14577_i2c_id[] = {
};
MODULE_DEVICE_TABLE(i2c, max14577_i2c_id);
-#ifdef CONFIG_PM_SLEEP
static int max14577_suspend(struct device *dev)
{
struct i2c_client *i2c = to_i2c_client(dev);
@@ -515,7 +514,6 @@ static int max14577_resume(struct device *dev)
return 0;
}
-#endif /* CONFIG_PM_SLEEP */
static SIMPLE_DEV_PM_OPS(max14577_pm, max14577_suspend, max14577_resume);
--
2.28.0
[PATCH 5/9] mfd: stpmic1: remove unnecessary CONFIG_PM_SLEEP
SIMPLE_DEV_PM_OPS has already took good care of CONFIG_PM_CONFIG.
Signed-off-by: Coiby Xu
---
drivers/mfd/stpmic1.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/mfd/stpmic1.c b/drivers/mfd/stpmic1.c
index eb3da558c3fb..8be7a6dd9bbb 100644
--- a/drivers/mfd/stpmic1.c
+++ b/drivers/mfd/stpmic1.c
@@ -162,7 +162,6 @@ static int stpmic1_probe(struct i2c_client *i2c,
return devm_of_platform_populate(dev);
}
-#ifdef CONFIG_PM_SLEEP
static int stpmic1_suspend(struct device *dev)
{
struct i2c_client *i2c = to_i2c_client(dev);
@@ -187,7 +186,6 @@ static int stpmic1_resume(struct device *dev)
return 0;
}
-#endif
static SIMPLE_DEV_PM_OPS(stpmic1_pm, stpmic1_suspend, stpmic1_resume);
--
2.28.0
[PATCH 3/9] mfd: intel_soc_pmic: remove unnecessary CONFIG_PM_SLEEP
SIMPLE_DEV_PM_OPS has already took good care of CONFIG_PM_CONFIG.
Signed-off-by: Coiby Xu
---
drivers/mfd/intel_soc_pmic_core.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/mfd/intel_soc_pmic_core.c
b/drivers/mfd/intel_soc_pmic_core.c
index ddd64f9e3341..c980af9ae1c0 100644
--- a/drivers/mfd/intel_soc_pmic_core.c
+++ b/drivers/mfd/intel_soc_pmic_core.c
@@ -122,7 +122,6 @@ static void intel_soc_pmic_shutdown(struct i2c_client *i2c)
return;
}
-#if defined(CONFIG_PM_SLEEP)
static int intel_soc_pmic_suspend(struct device *dev)
{
struct intel_soc_pmic *pmic = dev_get_drvdata(dev);
@@ -140,7 +139,6 @@ static int intel_soc_pmic_resume(struct device *dev)
return 0;
}
-#endif
static SIMPLE_DEV_PM_OPS(intel_soc_pmic_pm_ops, intel_soc_pmic_suspend,
intel_soc_pmic_resume);
--
2.28.0
[PATCH 10/15] iio: adc: stm32: remove unnecessary CONFIG_PM_SLEEP
SET_SYSTEM_SLEEP_PM_OPS has already took good care of CONFIG_PM_CONFIG.
Signed-off-by: Coiby Xu
---
drivers/iio/adc/stm32-adc.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c
index b3f31f147347..42f9013730f8 100644
--- a/drivers/iio/adc/stm32-adc.c
+++ b/drivers/iio/adc/stm32-adc.c
@@ -1988,7 +1988,6 @@ static int stm32_adc_remove(struct platform_device *pdev)
return 0;
}
-#if defined(CONFIG_PM_SLEEP)
static int stm32_adc_suspend(struct device *dev)
{
struct iio_dev *indio_dev = dev_get_drvdata(dev);
@@ -2018,7 +2017,6 @@ static int stm32_adc_resume(struct device *dev)
return stm32_adc_buffer_postenable(indio_dev);
}
-#endif
#if defined(CONFIG_PM)
static int stm32_adc_runtime_suspend(struct device *dev)
--
2.28.0
[PATCH 12/15] iio: imu: kmx61: remove unnecessary CONFIG_PM_SLEEP
SET_SYSTEM_SLEEP_PM_OPS has already took good care of CONFIG_PM_CONFIG.
Signed-off-by: Coiby Xu
---
drivers/iio/imu/kmx61.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/iio/imu/kmx61.c b/drivers/iio/imu/kmx61.c
index 61885e99d3fc..3b3d44ea8d24 100644
--- a/drivers/iio/imu/kmx61.c
+++ b/drivers/iio/imu/kmx61.c
@@ -1448,7 +1448,6 @@ static int kmx61_remove(struct i2c_client *client)
return 0;
}
-#ifdef CONFIG_PM_SLEEP
static int kmx61_suspend(struct device *dev)
{
int ret;
@@ -1474,7 +1473,6 @@ static int kmx61_resume(struct device *dev)
return kmx61_set_mode(data, stby, KMX61_ACC | KMX61_MAG, true);
}
-#endif
#ifdef CONFIG_PM
static int kmx61_runtime_suspend(struct device *dev)
--
2.28.0
[PATCH 09/15] iio: adc: palmas_gpadc: remove unnecessary CONFIG_PM_SLEEP
SET_SYSTEM_SLEEP_PM_OPS has already took good care of CONFIG_PM_CONFIG.
Signed-off-by: Coiby Xu
---
drivers/iio/adc/palmas_gpadc.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/iio/adc/palmas_gpadc.c b/drivers/iio/adc/palmas_gpadc.c
index 889b88768b63..76bafa5a4bf3 100644
--- a/drivers/iio/adc/palmas_gpadc.c
+++ b/drivers/iio/adc/palmas_gpadc.c
@@ -643,7 +643,6 @@ static int palmas_gpadc_remove(struct platform_device *pdev)
return 0;
}
-#ifdef CONFIG_PM_SLEEP
static int palmas_adc_wakeup_configure(struct palmas_gpadc *adc)
{
int adc_period, conv;
@@ -812,7 +811,6 @@ static int palmas_gpadc_resume(struct device *dev)
return 0;
};
-#endif
static const struct dev_pm_ops palmas_pm_ops = {
SET_SYSTEM_SLEEP_PM_OPS(palmas_gpadc_suspend,
--
2.28.0
[PATCH 11/15] iio: adc: at91_adc: remove unnecessary CONFIG_PM_SLEEP
SIMPLE_DEV_PM_OPS has already took good care of CONFIG_PM_CONFIG.
Signed-off-by: Coiby Xu
---
drivers/iio/adc/at91_adc.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/iio/adc/at91_adc.c b/drivers/iio/adc/at91_adc.c
index 9b2c548fae95..445072b239a6 100644
--- a/drivers/iio/adc/at91_adc.c
+++ b/drivers/iio/adc/at91_adc.c
@@ -1350,7 +1350,6 @@ static int at91_adc_remove(struct platform_device *pdev)
return 0;
}
-#ifdef CONFIG_PM_SLEEP
static int at91_adc_suspend(struct device *dev)
{
struct iio_dev *idev = dev_get_drvdata(dev);
@@ -1372,7 +1371,6 @@ static int at91_adc_resume(struct device *dev)
return 0;
}
-#endif
static SIMPLE_DEV_PM_OPS(at91_adc_pm_ops, at91_adc_suspend, at91_adc_resume);
--
2.28.0
[PATCH 1/3] ACPI: watchdog: remove unnecessary CONFIG_PM_SLEEP
SET_NOIRQ_SYSTEM_SLEEP_PM_OPS has already took good care of CONFIG_PM_CONFIG.
Signed-off-by: Coiby Xu
---
drivers/watchdog/wdat_wdt.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/watchdog/wdat_wdt.c b/drivers/watchdog/wdat_wdt.c
index 3065dd670a18..0d912ceb2ecd 100644
--- a/drivers/watchdog/wdat_wdt.c
+++ b/drivers/watchdog/wdat_wdt.c
@@ -465,7 +465,6 @@ static int wdat_wdt_probe(struct platform_device *pdev)
return devm_watchdog_register_device(dev, >wdd);
}
-#ifdef CONFIG_PM_SLEEP
static int wdat_wdt_suspend_noirq(struct device *dev)
{
struct wdat_wdt *wdat = dev_get_drvdata(dev);
@@ -526,7 +525,6 @@ static int wdat_wdt_resume_noirq(struct device *dev)
return wdat_wdt_start(>wdd);
}
-#endif
static const struct dev_pm_ops wdat_wdt_pm_ops = {
SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(wdat_wdt_suspend_noirq,
--
2.28.0
[PATCH 3/3] watchdog: sirfsoc_wdt: remove unnecessary CONFIG_PM_SLEEP
SIMPLE_DEV_PM_OPS has already took good care of CONFIG_PM_CONFIG.
Signed-off-by: Coiby Xu
---
drivers/watchdog/sirfsoc_wdt.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/watchdog/sirfsoc_wdt.c b/drivers/watchdog/sirfsoc_wdt.c
index 734cf2966ecb..dc8341cd7d44 100644
--- a/drivers/watchdog/sirfsoc_wdt.c
+++ b/drivers/watchdog/sirfsoc_wdt.c
@@ -170,7 +170,6 @@ static int sirfsoc_wdt_probe(struct platform_device *pdev)
return 0;
}
-#ifdef CONFIG_PM_SLEEP
static int sirfsoc_wdt_suspend(struct device *dev)
{
return 0;
@@ -189,7 +188,6 @@ static int sirfsoc_wdt_resume(struct device *dev)
return 0;
}
-#endif
static SIMPLE_DEV_PM_OPS(sirfsoc_wdt_pm_ops,
sirfsoc_wdt_suspend, sirfsoc_wdt_resume);
--
2.28.0
[PATCH 2/3] watchdog: st_lpc_wdt: remove unnecessary CONFIG_PM_SLEEP
SIMPLE_DEV_PM_OPS has already took good care of CONFIG_PM_CONFIG.
Signed-off-by: Coiby Xu
---
drivers/watchdog/st_lpc_wdt.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/watchdog/st_lpc_wdt.c b/drivers/watchdog/st_lpc_wdt.c
index 14ab6559c748..c1428d63dc9e 100644
--- a/drivers/watchdog/st_lpc_wdt.c
+++ b/drivers/watchdog/st_lpc_wdt.c
@@ -248,7 +248,6 @@ static int st_wdog_remove(struct platform_device *pdev)
return 0;
}
-#ifdef CONFIG_PM_SLEEP
static int st_wdog_suspend(struct device *dev)
{
struct st_wdog *st_wdog = watchdog_get_drvdata(_wdog_dev);
@@ -285,7 +284,6 @@ static int st_wdog_resume(struct device *dev)
return 0;
}
-#endif
static SIMPLE_DEV_PM_OPS(st_wdog_pm_ops,
st_wdog_suspend,
--
2.28.0
[PATCH] Drivers: ssb: remove unnecessary CONFIG_PM_SLEEP
SET_SYSTEM_SLEEP_PM_OPS has already took good care of CONFIG_PM_CONFIG.
Signed-off-by: Coiby Xu
---
drivers/ssb/pcihost_wrapper.c | 5 -
1 file changed, 5 deletions(-)
diff --git a/drivers/ssb/pcihost_wrapper.c b/drivers/ssb/pcihost_wrapper.c
index 410215c16920..6510f57469a0 100644
--- a/drivers/ssb/pcihost_wrapper.c
+++ b/drivers/ssb/pcihost_wrapper.c
@@ -18,7 +18,6 @@
#include
-#ifdef CONFIG_PM_SLEEP
static int ssb_pcihost_suspend(struct device *d)
{
struct pci_dev *dev = to_pci_dev(d);
@@ -62,8 +61,6 @@ static const struct dev_pm_ops ssb_pcihost_pm_ops = {
SET_SYSTEM_SLEEP_PM_OPS(ssb_pcihost_suspend, ssb_pcihost_resume)
};
-#endif /* CONFIG_PM_SLEEP */
-
static int ssb_pcihost_probe(struct pci_dev *dev,
const struct pci_device_id *id)
{
@@ -125,9 +122,7 @@ int ssb_pcihost_register(struct pci_driver *driver)
{
driver->probe = ssb_pcihost_probe;
driver->remove = ssb_pcihost_remove;
-#ifdef CONFIG_PM_SLEEP
driver->driver.pm = _pcihost_pm_ops;
-#endif
return pci_register_driver(driver);
}
--
2.28.0
[PATCH 15/15] iio: proximity: remove unnecessary CONFIG_PM_SLEEP
SET_SYSTEM_SLEEP_PM_OPS has already took good care of CONFIG_PM_CONFIG.
Signed-off-by: Coiby Xu
---
drivers/iio/proximity/sx9500.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/iio/proximity/sx9500.c b/drivers/iio/proximity/sx9500.c
index acb821cbad46..f4d55d217582 100644
--- a/drivers/iio/proximity/sx9500.c
+++ b/drivers/iio/proximity/sx9500.c
@@ -994,7 +994,6 @@ static int sx9500_remove(struct i2c_client *client)
return 0;
}
-#ifdef CONFIG_PM_SLEEP
static int sx9500_suspend(struct device *dev)
{
struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
@@ -1031,7 +1030,6 @@ static int sx9500_resume(struct device *dev)
return ret;
}
-#endif /* CONFIG_PM_SLEEP */
static const struct dev_pm_ops sx9500_pm_ops = {
SET_SYSTEM_SLEEP_PM_OPS(sx9500_suspend, sx9500_resume)
--
2.28.0
[PATCH 13/15] iio: light: us5182d: remove unnecessary CONFIG_PM_SLEEP
SET_SYSTEM_SLEEP_PM_OPS has already took good care of CONFIG_PM_CONFIG.
Signed-off-by: Coiby Xu
---
drivers/iio/light/us5182d.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/iio/light/us5182d.c b/drivers/iio/light/us5182d.c
index 393f27b75c75..15c6f8db3fdc 100644
--- a/drivers/iio/light/us5182d.c
+++ b/drivers/iio/light/us5182d.c
@@ -918,7 +918,7 @@ static int us5182d_remove(struct i2c_client *client)
return us5182d_shutdown_en(data, US5182D_CFG0_SHUTDOWN_EN);
}
-#if defined(CONFIG_PM_SLEEP) || defined(CONFIG_PM)
+#if defined(CONFIG_PM)
static int us5182d_suspend(struct device *dev)
{
struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
--
2.28.0
[PATCH 2/3] Input: adp5589: remove unnecessary CONFIG_PM_SLEEP
SIMPLE_DEV_PM_OPS has already took good care of CONFIG_PM_CONFIG.
Signed-off-by: Coiby Xu
---
drivers/input/keyboard/adp5589-keys.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/input/keyboard/adp5589-keys.c
b/drivers/input/keyboard/adp5589-keys.c
index eb0e9cd66bcb..50f94984e055 100644
--- a/drivers/input/keyboard/adp5589-keys.c
+++ b/drivers/input/keyboard/adp5589-keys.c
@@ -1077,7 +1077,6 @@ static int adp5589_remove(struct i2c_client *client)
return 0;
}
-#ifdef CONFIG_PM_SLEEP
static int adp5589_suspend(struct device *dev)
{
struct adp5589_kpad *kpad = dev_get_drvdata(dev);
@@ -1109,7 +1108,6 @@ static int adp5589_resume(struct device *dev)
return 0;
}
-#endif
static SIMPLE_DEV_PM_OPS(adp5589_dev_pm_ops, adp5589_suspend, adp5589_resume);
--
2.28.0
[PATCH 3/3] Input: nomadik-ske-keypad: remove unnecessary CONFIG_PM_SLEEP
SIMPLE_DEV_PM_OPS has already took good care of CONFIG_PM_CONFIG.
Signed-off-by: Coiby Xu
---
drivers/input/keyboard/nomadik-ske-keypad.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/input/keyboard/nomadik-ske-keypad.c
b/drivers/input/keyboard/nomadik-ske-keypad.c
index 608446e14614..b3052c8fa5d2 100644
--- a/drivers/input/keyboard/nomadik-ske-keypad.c
+++ b/drivers/input/keyboard/nomadik-ske-keypad.c
@@ -386,7 +386,6 @@ static int ske_keypad_remove(struct platform_device *pdev)
return 0;
}
-#ifdef CONFIG_PM_SLEEP
static int ske_keypad_suspend(struct device *dev)
{
struct platform_device *pdev = to_platform_device(dev);
@@ -414,7 +413,6 @@ static int ske_keypad_resume(struct device *dev)
return 0;
}
-#endif
static SIMPLE_DEV_PM_OPS(ske_keypad_dev_pm_ops,
ske_keypad_suspend, ske_keypad_resume);
--
2.28.0
[PATCH 1/3] Input: pmic8xxx-keypad: remove unnecessary CONFIG_PM_SLEEP
SIMPLE_DEV_PM_OPS has already took good care of CONFIG_PM_CONFIG.
Signed-off-by: Coiby Xu
---
drivers/input/keyboard/pmic8xxx-keypad.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/input/keyboard/pmic8xxx-keypad.c
b/drivers/input/keyboard/pmic8xxx-keypad.c
index 91d5811d6f0e..c04ab04331b2 100644
--- a/drivers/input/keyboard/pmic8xxx-keypad.c
+++ b/drivers/input/keyboard/pmic8xxx-keypad.c
@@ -621,7 +621,6 @@ static int pmic8xxx_kp_probe(struct platform_device *pdev)
return 0;
}
-#ifdef CONFIG_PM_SLEEP
static int pmic8xxx_kp_suspend(struct device *dev)
{
struct platform_device *pdev = to_platform_device(dev);
@@ -661,7 +660,6 @@ static int pmic8xxx_kp_resume(struct device *dev)
return 0;
}
-#endif
static SIMPLE_DEV_PM_OPS(pm8xxx_kp_pm_ops,
pmic8xxx_kp_suspend, pmic8xxx_kp_resume);
--
2.28.0
[PATCH 14/15] iio: light: remove unnecessary CONFIG_PM_SLEEP
SET_SYSTEM_SLEEP_PM_OPS has already took good care of CONFIG_PM_CONFIG.
Signed-off-by: Coiby Xu
---
drivers/iio/light/pa12203001.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/iio/light/pa12203001.c b/drivers/iio/light/pa12203001.c
index bfade6577a38..7737d2c8f0d5 100644
--- a/drivers/iio/light/pa12203001.c
+++ b/drivers/iio/light/pa12203001.c
@@ -417,14 +417,12 @@ static int pa12203001_suspend(struct device *dev)
}
#endif
-#ifdef CONFIG_PM_SLEEP
static int pa12203001_resume(struct device *dev)
{
struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
return pa12203001_power_chip(indio_dev, PA12203001_CHIP_ENABLE);
}
-#endif
#ifdef CONFIG_PM
static int pa12203001_runtime_resume(struct device *dev)
--
2.28.0
[PATCH 08/15] iio: magnetometer: mmc35240: remove unnecessary CONFIG_PM_SLEEP
SET_SYSTEM_SLEEP_PM_OPS has already took good care of CONFIG_PM_CONFIG.
Signed-off-by: Coiby Xu
---
drivers/iio/magnetometer/mmc35240.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/iio/magnetometer/mmc35240.c
b/drivers/iio/magnetometer/mmc35240.c
index 65f3d1ed0d59..37d330220433 100644
--- a/drivers/iio/magnetometer/mmc35240.c
+++ b/drivers/iio/magnetometer/mmc35240.c
@@ -521,7 +521,6 @@ static int mmc35240_probe(struct i2c_client *client,
return devm_iio_device_register(>dev, indio_dev);
}
-#ifdef CONFIG_PM_SLEEP
static int mmc35240_suspend(struct device *dev)
{
struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
@@ -548,7 +547,6 @@ static int mmc35240_resume(struct device *dev)
return 0;
}
-#endif
static const struct dev_pm_ops mmc35240_pm_ops = {
SET_SYSTEM_SLEEP_PM_OPS(mmc35240_suspend, mmc35240_resume)
--
2.28.0
[PATCH 07/15] iio: ssp: remove unnecessary CONFIG_PM_SLEEP
SET_SYSTEM_SLEEP_PM_OPS has already took good care of CONFIG_PM_CONFIG.
Signed-off-by: Coiby Xu
---
drivers/iio/common/ssp_sensors/ssp_dev.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/iio/common/ssp_sensors/ssp_dev.c
b/drivers/iio/common/ssp_sensors/ssp_dev.c
index 1aee87100038..e284ff702280 100644
--- a/drivers/iio/common/ssp_sensors/ssp_dev.c
+++ b/drivers/iio/common/ssp_sensors/ssp_dev.c
@@ -612,7 +612,6 @@ static int ssp_remove(struct spi_device *spi)
return 0;
}
-#ifdef CONFIG_PM_SLEEP
static int ssp_suspend(struct device *dev)
{
int ret;
@@ -661,7 +660,6 @@ static int ssp_resume(struct device *dev)
return 0;
}
-#endif /* CONFIG_PM_SLEEP */
static const struct dev_pm_ops ssp_pm_ops = {
SET_SYSTEM_SLEEP_PM_OPS(ssp_suspend, ssp_resume)
--
2.28.0
[PATCH 03/25] ASoC: fsl: remove unnecessary CONFIG_PM_SLEEP
SET_SYSTEM_SLEEP_PM_OPS has already took good care of CONFIG_PM_CONFIG.
Signed-off-by: Coiby Xu
---
sound/soc/fsl/imx-audmux.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/sound/soc/fsl/imx-audmux.c b/sound/soc/fsl/imx-audmux.c
index 25c18b9e348f..6d77188a4eab 100644
--- a/sound/soc/fsl/imx-audmux.c
+++ b/sound/soc/fsl/imx-audmux.c
@@ -349,7 +349,6 @@ static int imx_audmux_remove(struct platform_device *pdev)
return 0;
}
-#ifdef CONFIG_PM_SLEEP
static int imx_audmux_suspend(struct device *dev)
{
int i;
@@ -377,7 +376,6 @@ static int imx_audmux_resume(struct device *dev)
return 0;
}
-#endif /* CONFIG_PM_SLEEP */
static const struct dev_pm_ops imx_audmux_pm = {
SET_SYSTEM_SLEEP_PM_OPS(imx_audmux_suspend, imx_audmux_resume)
--
2.28.0
[PATCH 05/15] iio: accel: remove unnecessary CONFIG_PM_SLEEP
SET_SYSTEM_SLEEP_PM_OPS has already took good care of CONFIG_PM_CONFIG.
Signed-off-by: Coiby Xu
---
drivers/iio/accel/mma9551.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/iio/accel/mma9551.c b/drivers/iio/accel/mma9551.c
index 08a2303cc9df..c3b7a1633907 100644
--- a/drivers/iio/accel/mma9551.c
+++ b/drivers/iio/accel/mma9551.c
@@ -558,7 +558,6 @@ static int mma9551_runtime_resume(struct device *dev)
}
#endif
-#ifdef CONFIG_PM_SLEEP
static int mma9551_suspend(struct device *dev)
{
struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
@@ -584,7 +583,6 @@ static int mma9551_resume(struct device *dev)
return ret;
}
-#endif
static const struct dev_pm_ops mma9551_pm_ops = {
SET_SYSTEM_SLEEP_PM_OPS(mma9551_suspend, mma9551_resume)
--
2.28.0
[PATCH 04/15] iio: accel: remove unnecessary CONFIG_PM_SLEEP
SIMPLE_DEV_PM_OPS has already took good care of CONFIG_PM_CONFIG.
Signed-off-by: Coiby Xu
---
drivers/iio/accel/mc3230.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/iio/accel/mc3230.c b/drivers/iio/accel/mc3230.c
index 46e4283fc037..b966922b9899 100644
--- a/drivers/iio/accel/mc3230.c
+++ b/drivers/iio/accel/mc3230.c
@@ -160,7 +160,6 @@ static int mc3230_remove(struct i2c_client *client)
return mc3230_set_opcon(iio_priv(indio_dev), MC3230_MODE_OPCON_STANDBY);
}
-#ifdef CONFIG_PM_SLEEP
static int mc3230_suspend(struct device *dev)
{
struct mc3230_data *data;
@@ -178,7 +177,6 @@ static int mc3230_resume(struct device *dev)
return mc3230_set_opcon(data, MC3230_MODE_OPCON_WAKE);
}
-#endif
static SIMPLE_DEV_PM_OPS(mc3230_pm_ops, mc3230_suspend, mc3230_resume);
--
2.28.0
[PATCH 02/25] ASoC: fsl: fsl_ssi: remove unnecessary CONFIG_PM_SLEEP
SET_SYSTEM_SLEEP_PM_OPS has already took good care of CONFIG_PM_CONFIG.
Signed-off-by: Coiby Xu
---
sound/soc/fsl/fsl_ssi.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 404be27c15fe..065500a4cbc1 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -1669,7 +1669,6 @@ static int fsl_ssi_remove(struct platform_device *pdev)
return 0;
}
-#ifdef CONFIG_PM_SLEEP
static int fsl_ssi_suspend(struct device *dev)
{
struct fsl_ssi *ssi = dev_get_drvdata(dev);
@@ -1699,7 +1698,6 @@ static int fsl_ssi_resume(struct device *dev)
return regcache_sync(regs);
}
-#endif /* CONFIG_PM_SLEEP */
static const struct dev_pm_ops fsl_ssi_pm = {
SET_SYSTEM_SLEEP_PM_OPS(fsl_ssi_suspend, fsl_ssi_resume)
--
2.28.0
[PATCH 03/15] iio: accel: remove unnecessary CONFIG_PM_SLEEP
SET_SYSTEM_SLEEP_PM_OPS has already took good care of CONFIG_PM_CONFIG.
Signed-off-by: Coiby Xu
---
drivers/iio/accel/mma9553.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/iio/accel/mma9553.c b/drivers/iio/accel/mma9553.c
index c15908faa381..1d2e57158b31 100644
--- a/drivers/iio/accel/mma9553.c
+++ b/drivers/iio/accel/mma9553.c
@@ -1197,7 +1197,6 @@ static int mma9553_runtime_resume(struct device *dev)
}
#endif
-#ifdef CONFIG_PM_SLEEP
static int mma9553_suspend(struct device *dev)
{
struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
@@ -1223,7 +1222,6 @@ static int mma9553_resume(struct device *dev)
return ret;
}
-#endif
static const struct dev_pm_ops mma9553_pm_ops = {
SET_SYSTEM_SLEEP_PM_OPS(mma9553_suspend, mma9553_resume)
--
2.28.0
[PATCH 06/15] iio: accel: remove unnecessary CONFIG_PM_SLEEP
SIMPLE_DEV_PM_OPS has already took good care of CONFIG_PM_CONFIG.
Signed-off-by: Coiby Xu
---
drivers/iio/accel/dmard10.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/iio/accel/dmard10.c b/drivers/iio/accel/dmard10.c
index 90206f015857..64a15dea2a82 100644
--- a/drivers/iio/accel/dmard10.c
+++ b/drivers/iio/accel/dmard10.c
@@ -224,7 +224,6 @@ static int dmard10_remove(struct i2c_client *client)
return dmard10_shutdown(client);
}
-#ifdef CONFIG_PM_SLEEP
static int dmard10_suspend(struct device *dev)
{
return dmard10_shutdown(to_i2c_client(dev));
@@ -234,7 +233,6 @@ static int dmard10_resume(struct device *dev)
{
return dmard10_reset(to_i2c_client(dev));
}
-#endif
static SIMPLE_DEV_PM_OPS(dmard10_pm_ops, dmard10_suspend, dmard10_resume);
--
2.28.0
[PATCH 02/15] iio: accel: remove unnecessary CONFIG_PM_SLEEP
SIMPLE_DEV_PM_OPS has already took good care of CONFIG_PM_CONFIG.
Signed-off-by: Coiby Xu
---
drivers/iio/accel/da280.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/iio/accel/da280.c b/drivers/iio/accel/da280.c
index 4472dde6899e..c82d241491dd 100644
--- a/drivers/iio/accel/da280.c
+++ b/drivers/iio/accel/da280.c
@@ -160,7 +160,6 @@ static int da280_remove(struct i2c_client *client)
return da280_enable(client, false);
}
-#ifdef CONFIG_PM_SLEEP
static int da280_suspend(struct device *dev)
{
return da280_enable(to_i2c_client(dev), false);
@@ -170,7 +169,6 @@ static int da280_resume(struct device *dev)
{
return da280_enable(to_i2c_client(dev), true);
}
-#endif
static SIMPLE_DEV_PM_OPS(da280_pm_ops, da280_suspend, da280_resume);
--
2.28.0
[PATCH 04/25] ASoC: rockchip: remove unnecessary CONFIG_PM_SLEEP
SET_SYSTEM_SLEEP_PM_OPS has already took good care of CONFIG_PM_CONFIG.
Signed-off-by: Coiby Xu
---
sound/soc/rockchip/rockchip_pdm.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/sound/soc/rockchip/rockchip_pdm.c
b/sound/soc/rockchip/rockchip_pdm.c
index 5adb293d0435..f3c19310aeeb 100644
--- a/sound/soc/rockchip/rockchip_pdm.c
+++ b/sound/soc/rockchip/rockchip_pdm.c
@@ -574,7 +574,6 @@ static int rockchip_pdm_remove(struct platform_device *pdev)
return 0;
}
-#ifdef CONFIG_PM_SLEEP
static int rockchip_pdm_suspend(struct device *dev)
{
struct rk_pdm_dev *pdm = dev_get_drvdata(dev);
@@ -601,7 +600,6 @@ static int rockchip_pdm_resume(struct device *dev)
return ret;
}
-#endif
static const struct dev_pm_ops rockchip_pdm_pm_ops = {
SET_RUNTIME_PM_OPS(rockchip_pdm_runtime_suspend,
--
2.28.0
[PATCH 01/25] ALSA: core: pcm: remove unnecessary CONFIG_PM_SLEEP
SET_SYSTEM_SLEEP_PM_OPS has already took good care of CONFIG_PM_CONFIG.
Signed-off-by: Coiby Xu
---
sound/core/pcm.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/sound/core/pcm.c b/sound/core/pcm.c
index be5714f1bb58..5a281ac92958 100644
--- a/sound/core/pcm.c
+++ b/sound/core/pcm.c
@@ -599,7 +599,6 @@ static const struct attribute_group *pcm_dev_attr_groups[];
* PM callbacks: we need to deal only with suspend here, as the resume is
* triggered either from user-space or the driver's resume callback
*/
-#ifdef CONFIG_PM_SLEEP
static int do_pcm_suspend(struct device *dev)
{
struct snd_pcm_str *pstr = container_of(dev, struct snd_pcm_str, dev);
@@ -608,7 +607,6 @@ static int do_pcm_suspend(struct device *dev)
snd_pcm_suspend_all(pstr->pcm);
return 0;
}
-#endif
static const struct dev_pm_ops pcm_dev_pm_ops = {
SET_SYSTEM_SLEEP_PM_OPS(do_pcm_suspend, NULL)
--
2.28.0
[PATCH 01/15] iio: accel: remove unnecessary CONFIG_PM_SLEEP
SIMPLE_DEV_PM_OPS has already took good care of CONFIG_PM_CONFIG.
Signed-off-by: Coiby Xu
---
drivers/iio/accel/da311.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/iio/accel/da311.c b/drivers/iio/accel/da311.c
index 3b3df620ba27..55d4891556ca 100644
--- a/drivers/iio/accel/da311.c
+++ b/drivers/iio/accel/da311.c
@@ -263,7 +263,6 @@ static int da311_remove(struct i2c_client *client)
return da311_enable(client, false);
}
-#ifdef CONFIG_PM_SLEEP
static int da311_suspend(struct device *dev)
{
return da311_enable(to_i2c_client(dev), false);
@@ -273,7 +272,6 @@ static int da311_resume(struct device *dev)
{
return da311_enable(to_i2c_client(dev), true);
}
-#endif
static SIMPLE_DEV_PM_OPS(da311_pm_ops, da311_suspend, da311_resume);
--
2.28.0
[PATCH 06/25] ASoC: img-spdif-in: remove unnecessary CONFIG_PM_SLEEP
SET_SYSTEM_SLEEP_PM_OPS has already took good care of CONFIG_PM_CONFIG.
Signed-off-by: Coiby Xu
---
sound/soc/img/img-spdif-in.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/sound/soc/img/img-spdif-in.c b/sound/soc/img/img-spdif-in.c
index 46ff8a3621d5..bb73b7fc35da 100644
--- a/sound/soc/img/img-spdif-in.c
+++ b/sound/soc/img/img-spdif-in.c
@@ -823,7 +823,6 @@ static int img_spdif_in_dev_remove(struct platform_device
*pdev)
return 0;
}
-#ifdef CONFIG_PM_SLEEP
static int img_spdif_in_suspend(struct device *dev)
{
struct img_spdif_in *spdif = dev_get_drvdata(dev);
@@ -863,7 +862,6 @@ static int img_spdif_in_resume(struct device *dev)
return 0;
}
-#endif
static const struct of_device_id img_spdif_in_of_match[] = {
{ .compatible = "img,spdif-in" },
--
2.28.0
[PATCH 05/25] ASoC: img: remove unnecessary CONFIG_PM_SLEEP
SET_SYSTEM_SLEEP_PM_OPS has already took good care of CONFIG_PM_CONFIG.
Signed-off-by: Coiby Xu
---
sound/soc/img/img-i2s-in.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/sound/soc/img/img-i2s-in.c b/sound/soc/img/img-i2s-in.c
index 0843235d73c9..8364eb9c44bc 100644
--- a/sound/soc/img/img-i2s-in.c
+++ b/sound/soc/img/img-i2s-in.c
@@ -545,7 +545,6 @@ static int img_i2s_in_dev_remove(struct platform_device
*pdev)
return 0;
}
-#ifdef CONFIG_PM_SLEEP
static int img_i2s_in_suspend(struct device *dev)
{
struct img_i2s_in *i2s = dev_get_drvdata(dev);
@@ -592,7 +591,6 @@ static int img_i2s_in_resume(struct device *dev)
return 0;
}
-#endif
static const struct of_device_id img_i2s_in_of_match[] = {
{ .compatible = "img,i2s-in" },
--
2.28.0
[PATCH 08/25] ASoC: img-i2s-out: remove unnecessary CONFIG_PM_SLEEP
SET_SYSTEM_SLEEP_PM_OPS has already took good care of CONFIG_PM_CONFIG.
Signed-off-by: Coiby Xu
---
sound/soc/img/img-i2s-out.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/sound/soc/img/img-i2s-out.c b/sound/soc/img/img-i2s-out.c
index b56a18e7f3ac..7693b7fdf299 100644
--- a/sound/soc/img/img-i2s-out.c
+++ b/sound/soc/img/img-i2s-out.c
@@ -551,7 +551,6 @@ static int img_i2s_out_dev_remove(struct platform_device
*pdev)
return 0;
}
-#ifdef CONFIG_PM_SLEEP
static int img_i2s_out_suspend(struct device *dev)
{
struct img_i2s_out *i2s = dev_get_drvdata(dev);
@@ -598,7 +597,6 @@ static int img_i2s_out_resume(struct device *dev)
return 0;
}
-#endif
static const struct of_device_id img_i2s_out_of_match[] = {
{ .compatible = "img,i2s-out" },
--
2.28.0
[PATCH 09/25] ASoC: tegra: remove unnecessary CONFIG_PM_SLEEP
SET_SYSTEM_SLEEP_PM_OPS has already took good care of CONFIG_PM_CONFIG.
Signed-off-by: Coiby Xu
---
sound/soc/tegra/tegra30_ahub.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/sound/soc/tegra/tegra30_ahub.c b/sound/soc/tegra/tegra30_ahub.c
index 156e3b9d613c..9fc22d5e9f6e 100644
--- a/sound/soc/tegra/tegra30_ahub.c
+++ b/sound/soc/tegra/tegra30_ahub.c
@@ -629,7 +629,6 @@ static int tegra30_ahub_remove(struct platform_device *pdev)
return 0;
}
-#ifdef CONFIG_PM_SLEEP
static int tegra30_ahub_suspend(struct device *dev)
{
regcache_mark_dirty(ahub->regmap_ahub);
@@ -653,7 +652,6 @@ static int tegra30_ahub_resume(struct device *dev)
return ret;
}
-#endif
static const struct dev_pm_ops tegra30_ahub_pm_ops = {
SET_RUNTIME_PM_OPS(tegra30_ahub_runtime_suspend,
--
2.28.0
