Skip to contentSkip to conversation

various fixes for UFS-PM series

PATCH Applied Dolev Raviv 3 participants 15 messages Oct 23, 2014 – Nov 20, 2014

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

cover various fixes for UFS-PM series

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(-)
Signature · 4 lines
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
#1 lore raw

PATCH 1/6 scsi: ufs: fix reference counting of W-LUs

From: Akinobu Mita <[email protected]>

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.

Cc Vinayak Holikatti <[email protected]>
Cc Santosh Y <[email protected]>
Cc Dolev Raviv <[email protected]>
Cc Subhash Jadavani <[email protected]>
Cc Yaniv Gardi <[email protected]>
Cc Christoph Hellwig <[email protected]>
Cc "James E.J. Bottomley" <[email protected]>
drivers/scsi/ufs/ufshcd.c +36 -38
index 497c38a..59b6544 100644
@@ -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);
drivers/scsi/ufs/ufshcd.h +0 -2
index 58ecdff..4a574aa 100644
@@ -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;
Signature · 4 lines
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
#2 lore raw

PATCH 2/6 scsi: ufs: fix power info after link start-up

In reply to Dolev Raviv

From: Yaniv Gardi <[email protected]>

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.

drivers/scsi/ufs/ufshcd.c +18 -0
index 59b6544..77a4e38 100644
@@ -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);
 
Signature · 4 lines
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
#3 lore raw

PATCH 3/6 scsi: ufs: fix static checker errors in ufshcd_system_suspend

In reply to Dolev Raviv

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.

drivers/scsi/ufs/ufshcd.c +1 -1
index 77a4e38..d3f6ddb 100644
@@ -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)
Signature · 4 lines
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
#4 lore raw

PATCH 4/6 scsi: ufs: fix static checker warning in ufshcd_populate_vreg

In reply to Dolev Raviv

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.

drivers/scsi/ufs/ufshcd-pltfrm.c +2 -4
index 8adf067..2cdec78 100644
@@ -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);
 
Signature · 4 lines
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
#5 lore raw

PATCH 5/6 scsi: ufs: fix static checker warning in __ufshcd_setup_clocks

In reply to Dolev Raviv

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.

drivers/scsi/ufs/ufshcd.c +1 -1
index d3f6ddb..b9da446 100644
@@ -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);
Signature · 4 lines
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
#6 lore raw

PATCH 6/6 scsi: ufs: fix static checker warning in ufshcd_parse_clock_info

In reply to Dolev Raviv

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.

drivers/scsi/ufs/ufshcd-pltfrm.c +3 -6
index 2cdec78..1c3467b 100644
@@ -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;
 }
Signature · 4 lines
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
#7 lore raw
In reply to Dolev Raviv
Quoted 2 lines · 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.

#8 lore raw
Quoted 6 lines · Dolev Raviv
Quoted 2 lines

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.

Quoted 4 lines

I'll happily apply these after I get a second review for them, preferably from the ufs community.

Signature · 2 lines
QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
#9 lore raw
Quoted 2 lines · 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?

#10 lore raw
Quoted 4 lines · Dolev Raviv
Quoted 2 lines

The first patch is Akinobu Mita's patch.
You can add my ack on it.

Sorry I meant 'reviewed-by'.

Quoted 3 lines

Given that you passed them on to me you should normally sign off on them as well.

My mistake.

Quoted 2 lines

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

No it shouldn't.

Thanks,
Dolev

Signature · 3 lines
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
#11 lore raw
Quoted 3 lines · Dolev Raviv
Quoted 1 line

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

No it shouldn't.

It seems like I'm getting a lot of the series from you and your signoff, why shouldn't you be listed in MAINTAINERS? From the list activity it seems like you and Sujit are the defator maintainers.

Santosh just send a patch to remove himself, and I only see very little activity from Vinayak, with his last ACK in early July and a few more earlier in the year.

#12 lore raw
Quoted 10 lines · Dolev Raviv
Quoted 3 lines
Quoted 1 line

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

No it shouldn't.

It seems like I'm getting a lot of the series from you and your signoff, why shouldn't you be listed in MAINTAINERS? From the list activity it seems like you and Sujit are the defator maintainers.

Thank you,
Sorry this mail was forgotten in the archives.
I'll have to pass, as I will not be able to be fully committed.

Quoted 5 lines

Santosh just send a patch to remove himself, and I only see very little activity from Vinayak, with his last ACK in early July and a few more earlier in the year.

Signature · 7 lines
Thanks,
Dolev

-- 
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
Reviewed-by on the v1 series
#13 lore raw
In reply to Dolev Raviv
Quoted 33 lines

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

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 5 lines
Maya Erez
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
Collaborative Project