Re: [linux-nfc] [PATCH] NFC: cooperation with runtime PM
From: Krzysztof Kozlowski <hidden>
Date: 2021-05-12 13:55:02
Hi, Thanks for the patch. Few notes: On 12/05/2021 09:44, Oliver Neukum wrote:
We cannot rely on the underlying hardware to do correct runtime PM. NFC core needs to get PM reference while a device is operational, lest it be suspended when it is supposed to be waiting for a target to come into range.
Your word wrapping is unusually early - please wrap the commit msg as in coding style (so around 75-character). https://elixir.bootlin.com/linux/latest/source/Documentation/process/submitting-patches.rst#L578
quoted hunk
Signed-off-by: Oliver Neukum <oneukum@suse.com> --- net/nfc/core.c | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-)diff --git a/net/nfc/core.c b/net/nfc/core.c index 573c80c6ff7a..5ca4597c39c7 100644 --- a/net/nfc/core.c +++ b/net/nfc/core.c@@ -15,6 +15,7 @@ #include <linux/slab.h> #include <linux/rfkill.h> #include <linux/nfc.h> +#include <linux/pm_runtime.h> #include <net/genetlink.h>@@ -37,6 +38,7 @@ int nfc_fw_download(struct nfc_dev *dev, const char *firmware_name) pr_debug("%s do firmware %s\n", dev_name(&dev->dev), firmware_name); device_lock(&dev->dev); + pm_runtime_get_sync(&dev->dev);
This can fail. Probably you wanted pm_runtime_resume_and_get() here.
quoted hunk
if (!device_is_registered(&dev->dev)) { rc = -ENODEV;@@ -58,7 +60,10 @@ int nfc_fw_download(struct nfc_dev *dev, const char *firmware_name) if (rc) dev->fw_download_in_progress = false;
goto error
+ device_unlock(&dev->dev); + return rc;
Since last rc cannot be != 0, return 0 Blank line
quoted hunk
error: + pm_runtime_put(&dev->dev); device_unlock(&dev->dev); return rc; }@@ -73,9 +78,13 @@ int nfc_fw_download(struct nfc_dev *dev, const char *firmware_name) int nfc_fw_download_done(struct nfc_dev *dev, const char *firmware_name, u32 result) { + int rv;
"int rc"
quoted hunk
+ dev->fw_download_in_progress = false; - return nfc_genl_fw_download_done(dev, firmware_name, result); + rv = nfc_genl_fw_download_done(dev, firmware_name, result); + pm_runtime_put(&dev->dev); + return rv; } EXPORT_SYMBOL(nfc_fw_download_done);@@ -93,6 +102,7 @@ int nfc_dev_up(struct nfc_dev *dev) pr_debug("dev_name=%s\n", dev_name(&dev->dev)); device_lock(&dev->dev); + pm_runtime_get_sync(&dev->dev);
Same comments as before.
quoted hunk
if (dev->rfkill && rfkill_blocked(dev->rfkill)) { rc = -ERFKILL;@@ -124,7 +134,11 @@ int nfc_dev_up(struct nfc_dev *dev) if (dev->ops->discover_se && dev->ops->discover_se(dev)) pr_err("SE discovery failed\n"); + device_unlock(&dev->dev); + return rc;
Probably same comments as before apply.
quoted hunk
+ error: + pm_runtime_put(&dev->dev); device_unlock(&dev->dev); return rc; }@@ -161,6 +175,9 @@ int nfc_dev_down(struct nfc_dev *dev) dev->ops->dev_down(dev); dev->dev_up = false; + pm_runtime_put(&dev->dev); + device_unlock(&dev->dev); + return rc;
return 0
error: device_unlock(&dev->dev);
Best regards, Krzysztof