Discussion:
[PATCH 0/6] various fixes for UFS-PM series
(too old to reply)
Dolev Raviv
2014-10-23 10:25:11 UTC
Permalink
Contains a couple of bug fixes reported by Akinobu Mita, In adition to
"static checker" warnings reported by Dan Carpenter.

Akinobu Mita (1):
scsi: ufs: fix reference counting of W-LUs

Dolev Raviv (4):
scsi: ufs: fix static checker errors in ufshcd_system_suspend
scsi: ufs: fix static checker warning in ufshcd_populate_vreg
scsi: ufs: fix static checker warning in __ufshcd_setup_clocks
scsi: ufs: fix static checker warning in ufshcd_parse_clock_info

Yaniv Gardi (1):
scsi: ufs: fix power info after link start-up

drivers/scsi/ufs/ufshcd-pltfrm.c | 15 +++----
drivers/scsi/ufs/ufshcd.c | 96 +++++++++++++++++++++++-----------------
drivers/scsi/ufs/ufshcd.h | 2 -
3 files changed, 61 insertions(+), 52 deletions(-)
--
1.8.5.2
--
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
Dolev Raviv
2014-10-23 10:25:13 UTC
Permalink
From: Yaniv Gardi <***@codeaurora.org>

After link start-up power mode will always be PWM G1. This is not
reflected in the pwr_info struct which will keep the previous values.
Since ufshcd_change_power_mode() tries to avoid unnecessary power mode
change if the requested power mode and current power mode are same,
power mode change won't execute again after driver initialization.

This patch solves the problem by setting pwr_info to PWM G1 after link
start-up.

Signed-off-by: Yaniv Gardi <***@codeaurora.org>
Signed-off-by: Dolev Raviv <***@codeaurora.org>

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 59b6544..77a4e38 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -2246,6 +2246,22 @@ static int ufshcd_uic_hibern8_exit(struct ufs_hba *hba)
return ret;
}

+ /**
+ * ufshcd_init_pwr_info - setting the POR (power on reset)
+ * values in hba power info
+ * @hba: per-adapter instance
+ */
+static void ufshcd_init_pwr_info(struct ufs_hba *hba)
+{
+ hba->pwr_info.gear_rx = UFS_PWM_G1;
+ hba->pwr_info.gear_tx = UFS_PWM_G1;
+ hba->pwr_info.lane_rx = 1;
+ hba->pwr_info.lane_tx = 1;
+ hba->pwr_info.pwr_rx = SLOWAUTO_MODE;
+ hba->pwr_info.pwr_tx = SLOWAUTO_MODE;
+ hba->pwr_info.hs_rate = 0;
+}
+
/**
* ufshcd_get_max_pwr_mode - reads the max power mode negotiated with device
* @hba: per-adapter instance
@@ -4118,6 +4134,8 @@ static int ufshcd_probe_hba(struct ufs_hba *hba)
if (ret)
goto out;

+ ufshcd_init_pwr_info(hba);
+
/* UniPro link is active now */
ufshcd_set_link_active(hba);
--
1.8.5.2
--
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
Dolev Raviv
2014-10-23 10:25:14 UTC
Permalink
This patch fixes newly introduced sparse warning in
ufshcd_system_suspend, introduced by UFS power management series.

Sparse warning:
drivers/scsi/ufs/ufshcd.c:5118 ufshcd_system_suspend()
error: we previously assumed 'hba' could be null (see line 5089)

To fix it, we return 0 in case HBA is not initialized or is
not powered.

Signed-off-by: Dolev Raviv <***@codeaurora.org>

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 77a4e38..d3f6ddb 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -5104,7 +5104,7 @@ int ufshcd_system_suspend(struct ufs_hba *hba)
int ret = 0;

if (!hba || !hba->is_powered)
- goto out;
+ return 0;

if (pm_runtime_suspended(hba->dev)) {
if (hba->rpm_lvl == hba->spm_lvl)
--
1.8.5.2
--
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Dolev Raviv
2014-10-23 10:25:12 UTC
Permalink
From: Akinobu Mita <***@gmail.com>

UFS driver adds three well known LUs in the initialization, but those
reference counts are not decremented, so it makes ufshcd module
impossible to unload.

This fixes it by putting scsi_device_put() in the initalization, and in
order to protect concurrent access to hba->sdev_ufs_device (UFS Device
W-LU) from manual delete, increment the reference count while requesting
device power mode setting.

The rest of W-LUs (hba->sdev_boot and hba->sdev_rpmb) are not directly
used from driver, so these references in struct ufs_hba are removed.

Signed-off-by: Akinobu Mita <***@fixstars.com>
Cc: Vinayak Holikatti <***@gmail.com>
Cc: Santosh Y <***@gmail.com>
Cc: Dolev Raviv <***@codeaurora.org>
Cc: Subhash Jadavani <***@codeaurora.org>
Cc: Yaniv Gardi <***@codeaurora.org>
Cc: Christoph Hellwig <***@lst.de>
Cc: "James E.J. Bottomley" <***@parallels.com>
Cc: linux-***@vger.kernel.org

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 497c38a..59b6544 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -2844,8 +2844,13 @@ static void ufshcd_slave_destroy(struct scsi_device *sdev)
hba = shost_priv(sdev->host);
scsi_deactivate_tcq(sdev, hba->nutrs);
/* Drop the reference as it won't be needed anymore */
- if (ufshcd_scsi_to_upiu_lun(sdev->lun) == UFS_UPIU_UFS_DEVICE_WLUN)
+ if (ufshcd_scsi_to_upiu_lun(sdev->lun) == UFS_UPIU_UFS_DEVICE_WLUN) {
+ unsigned long flags;
+
+ spin_lock_irqsave(hba->host->host_lock, flags);
hba->sdev_ufs_device = NULL;
+ spin_unlock_irqrestore(hba->host->host_lock, flags);
+ }
}

/**
@@ -4062,6 +4067,8 @@ static void ufshcd_init_icc_levels(struct ufs_hba *hba)
static int ufshcd_scsi_add_wlus(struct ufs_hba *hba)
{
int ret = 0;
+ struct scsi_device *sdev_rpmb;
+ struct scsi_device *sdev_boot;

hba->sdev_ufs_device = __scsi_add_device(hba->host, 0, 0,
ufshcd_upiu_wlun_to_scsi_wlun(UFS_UPIU_UFS_DEVICE_WLUN), NULL);
@@ -4070,26 +4077,27 @@ static int ufshcd_scsi_add_wlus(struct ufs_hba *hba)
hba->sdev_ufs_device = NULL;
goto out;
}
+ scsi_device_put(hba->sdev_ufs_device);

- hba->sdev_boot = __scsi_add_device(hba->host, 0, 0,
+ sdev_boot = __scsi_add_device(hba->host, 0, 0,
ufshcd_upiu_wlun_to_scsi_wlun(UFS_UPIU_BOOT_WLUN), NULL);
- if (IS_ERR(hba->sdev_boot)) {
- ret = PTR_ERR(hba->sdev_boot);
- hba->sdev_boot = NULL;
+ if (IS_ERR(sdev_boot)) {
+ ret = PTR_ERR(sdev_boot);
goto remove_sdev_ufs_device;
}
+ scsi_device_put(sdev_boot);

- hba->sdev_rpmb = __scsi_add_device(hba->host, 0, 0,
+ sdev_rpmb = __scsi_add_device(hba->host, 0, 0,
ufshcd_upiu_wlun_to_scsi_wlun(UFS_UPIU_RPMB_WLUN), NULL);
- if (IS_ERR(hba->sdev_rpmb)) {
- ret = PTR_ERR(hba->sdev_rpmb);
- hba->sdev_rpmb = NULL;
+ if (IS_ERR(sdev_rpmb)) {
+ ret = PTR_ERR(sdev_rpmb);
goto remove_sdev_boot;
}
+ scsi_device_put(sdev_rpmb);
goto out;

remove_sdev_boot:
- scsi_remove_device(hba->sdev_boot);
+ scsi_remove_device(sdev_boot);
remove_sdev_ufs_device:
scsi_remove_device(hba->sdev_ufs_device);
out:
@@ -4097,30 +4105,6 @@ out:
}

/**
- * ufshcd_scsi_remove_wlus - Removes the W-LUs which were added by
- * ufshcd_scsi_add_wlus()
- * @hba: per-adapter instance
- *
- */
-static void ufshcd_scsi_remove_wlus(struct ufs_hba *hba)
-{
- if (hba->sdev_ufs_device) {
- scsi_remove_device(hba->sdev_ufs_device);
- hba->sdev_ufs_device = NULL;
- }
-
- if (hba->sdev_boot) {
- scsi_remove_device(hba->sdev_boot);
- hba->sdev_boot = NULL;
- }
-
- if (hba->sdev_rpmb) {
- scsi_remove_device(hba->sdev_rpmb);
- hba->sdev_rpmb = NULL;
- }
-}
-
-/**
* ufshcd_probe_hba - probe hba to detect device and initialize
* @hba: per-adapter instance
*
@@ -4675,11 +4659,25 @@ static int ufshcd_set_dev_pwr_mode(struct ufs_hba *hba,
{
unsigned char cmd[6] = { START_STOP };
struct scsi_sense_hdr sshdr;
- struct scsi_device *sdp = hba->sdev_ufs_device;
+ struct scsi_device *sdp;
+ unsigned long flags;
int ret;

- if (!sdp || !scsi_device_online(sdp))
- return -ENODEV;
+ spin_lock_irqsave(hba->host->host_lock, flags);
+ sdp = hba->sdev_ufs_device;
+ if (sdp) {
+ ret = scsi_device_get(sdp);
+ if (!ret && !scsi_device_online(sdp)) {
+ ret = -ENODEV;
+ scsi_device_put(sdp);
+ }
+ } else {
+ ret = -ENODEV;
+ }
+ spin_unlock_irqrestore(hba->host->host_lock, flags);
+
+ if (ret)
+ return ret;

/*
* If scsi commands fail, the scsi mid-layer schedules scsi error-
@@ -4718,6 +4716,7 @@ static int ufshcd_set_dev_pwr_mode(struct ufs_hba *hba,
if (!ret)
hba->curr_dev_pwr_mode = pwr_mode;
out:
+ scsi_device_put(sdp);
hba->host->eh_noresume = 0;
return ret;
}
@@ -5231,7 +5230,6 @@ EXPORT_SYMBOL(ufshcd_shutdown);
void ufshcd_remove(struct ufs_hba *hba)
{
scsi_remove_host(hba->host);
- ufshcd_scsi_remove_wlus(hba);
/* disable interrupts */
ufshcd_disable_intr(hba, hba->intr_mask);
ufshcd_hba_stop(hba);
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 58ecdff..4a574aa 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -392,8 +392,6 @@ struct ufs_hba {
* "UFS device" W-LU.
*/
struct scsi_device *sdev_ufs_device;
- struct scsi_device *sdev_rpmb;
- struct scsi_device *sdev_boot;

enum ufs_dev_pwr_mode curr_dev_pwr_mode;
enum uic_link_state uic_link_state;
--
1.8.5.2
--
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Dolev Raviv
2014-10-23 10:25:15 UTC
Permalink
This patch fixes newly introduced static checker warning in
ufshcd_populate_vreg, introduced by UFS power management series.

Warning:
drivers/scsi/ufs/ufshcd-pltfrm.c:167 ufshcd_populate_vreg()
warn: missing error code here? 'devm_kzalloc()' failed. 'ret' = '0'

To fix it we return -ENOMEM and skip the message print.

Signed-off-by: Dolev Raviv <***@codeaurora.org>

diff --git a/drivers/scsi/ufs/ufshcd-pltfrm.c b/drivers/scsi/ufs/ufshcd-pltfrm.c
index 8adf067..2cdec78 100644
--- a/drivers/scsi/ufs/ufshcd-pltfrm.c
+++ b/drivers/scsi/ufs/ufshcd-pltfrm.c
@@ -162,10 +162,8 @@ static int ufshcd_populate_vreg(struct device *dev, const char *name,
}

vreg = devm_kzalloc(dev, sizeof(*vreg), GFP_KERNEL);
- if (!vreg) {
- dev_err(dev, "No memory for %s regulator\n", name);
- goto out;
- }
+ if (!vreg)
+ return -ENOMEM;

vreg->name = kstrdup(name, GFP_KERNEL);
--
1.8.5.2
--
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
Dolev Raviv
2014-10-23 10:25:16 UTC
Permalink
This patch fixes newly introduced static checker warning in
__ufshcd_setup_clocks, introduced by UFS power management series.

Warning:
drivers/scsi/ufs/ufshcd.c:4474 __ufshcd_setup_clocks()
warn: we tested 'ret' before and it was 'false'

To fix it we remove the (!ret) from the condition.

Signed-off-by: Dolev Raviv <***@codeaurora.org>

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index d3f6ddb..b9da446 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -4473,7 +4473,7 @@ out:
if (!IS_ERR_OR_NULL(clki->clk) && clki->enabled)
clk_disable_unprepare(clki->clk);
}
- } else if (!ret && on) {
+ } else if (on) {
spin_lock_irqsave(hba->host->host_lock, flags);
hba->clk_gating.state = CLKS_ON;
spin_unlock_irqrestore(hba->host->host_lock, flags);
--
1.8.5.2
--
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Dolev Raviv
2014-10-23 10:25:17 UTC
Permalink
This patch fixes newly introduced static checker warning in
ufshcd_parse_clock_info, introduced by UFS power management series.

Warning:
drivers/scsi/ufs/ufshcd-pltfrm.c:138 ufshcd_parse_clock_info()
warn: passing devm_ allocated variable to kfree. 'clkfreq'

To fix it we remove the kfree(clkfreq) statement.
In addition we removed the redundant goto label.

Signed-off-by: Dolev Raviv <***@codeaurora.org>

diff --git a/drivers/scsi/ufs/ufshcd-pltfrm.c b/drivers/scsi/ufs/ufshcd-pltfrm.c
index 2cdec78..1c3467b 100644
--- a/drivers/scsi/ufs/ufshcd-pltfrm.c
+++ b/drivers/scsi/ufs/ufshcd-pltfrm.c
@@ -102,7 +102,6 @@ static int ufshcd_parse_clock_info(struct ufs_hba *hba)
clkfreq = devm_kzalloc(dev, sz * sizeof(*clkfreq),
GFP_KERNEL);
if (!clkfreq) {
- dev_err(dev, "%s: no memory\n", "freq-table-hz");
ret = -ENOMEM;
goto out;
}
@@ -112,19 +111,19 @@ static int ufshcd_parse_clock_info(struct ufs_hba *hba)
if (ret && (ret != -EINVAL)) {
dev_err(dev, "%s: error reading array %d\n",
"freq-table-hz", ret);
- goto free_clkfreq;
+ return ret;
}

for (i = 0; i < sz; i += 2) {
ret = of_property_read_string_index(np,
"clock-names", i/2, (const char **)&name);
if (ret)
- goto free_clkfreq;
+ goto out;

clki = devm_kzalloc(dev, sizeof(*clki), GFP_KERNEL);
if (!clki) {
ret = -ENOMEM;
- goto free_clkfreq;
+ goto out;
}

clki->min_freq = clkfreq[i];
@@ -134,8 +133,6 @@ static int ufshcd_parse_clock_info(struct ufs_hba *hba)
clki->min_freq, clki->max_freq, clki->name);
list_add_tail(&clki->list, &hba->clk_list_head);
}
-free_clkfreq:
- kfree(clkfreq);
out:
return ret;
}
--
1.8.5.2
--
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
Christoph Hellwig
2014-10-23 17:15:36 UTC
Permalink
Post by Dolev Raviv
Contains a couple of bug fixes reported by Akinobu Mita, In adition to
"static checker" warnings reported by Dan Carpenter.
Any reason you didn't add a signoff to the first patch?

I'll happily apply these after I get a second review for them,
preferably from the ufs community.
Dolev Raviv
2014-10-24 07:53:23 UTC
Permalink
Post by Christoph Hellwig
Post by Dolev Raviv
Contains a couple of bug fixes reported by Akinobu Mita, In adition to
"static checker" warnings reported by Dan Carpenter.
Any reason you didn't add a signoff to the first patch?
The first patch is Akinobu Mita's patch.
You can add my ack on it.
Post by Christoph Hellwig
I'll happily apply these after I get a second review for them,
preferably from the ufs community.
--
QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
Christoph Hellwig
2014-10-24 09:22:51 UTC
Permalink
Post by Dolev Raviv
The first patch is Akinobu Mita's patch.
You can add my ack on it.
Given that you passed them on to me you should normally sign off on them
as well.

Btw, MAINTAINERS doesn't mention you for the ufs driver, should it?
Loading...