| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Sun, 28 Jul 2019 14:28:13 +0300
From: Oded Gabbay <oded.gabbay@...il.com>
To: linux-kernel@...r.kernel.org, oshpigelman@...ana.ai,
ttayar@...ana.ai, gregkh@...uxfoundation.org
Subject: [PATCH 4/9] habanalabs: don't change frequency if user context is valid
Instead of using the FD open counter to check if there is a user opened on
the device, check if there is a valid user context.
Use the new lazy context creation mutex to protect against multiple calls
to change the frequency.
This is in preparation for removing the FD open counter.
Signed-off-by: Oded Gabbay <oded.gabbay@...il.com>
---
drivers/misc/habanalabs/context.c | 9 +++++
drivers/misc/habanalabs/device.c | 40 +++++++++--------------
drivers/misc/habanalabs/goya/goya_hwmgr.c | 11 +++----
drivers/misc/habanalabs/habanalabs.h | 4 +--
drivers/misc/habanalabs/habanalabs_drv.c | 34 ++++++++-----------
5 files changed, 45 insertions(+), 53 deletions(-)
diff --git a/drivers/misc/habanalabs/context.c b/drivers/misc/habanalabs/context.c
index 3c1f7c29e908..a4cd47ced94d 100644
--- a/drivers/misc/habanalabs/context.c
+++ b/drivers/misc/habanalabs/context.c
@@ -220,9 +220,18 @@ bool hl_ctx_is_valid(struct hl_fpriv *hpriv)
if (rc) {
dev_err(hdev->dev, "Failed to create context %d\n", rc);
valid = false;
+ goto unlock_mutex;
}
+
+ /* Device is IDLE at this point so it is legal to change PLLs.
+ * There is no need to check anything because if the PLL is
+ * already HIGH, the set function will return without doing
+ * anything
+ */
+ hl_device_set_frequency(hdev, PLL_HIGH);
}
+unlock_mutex:
mutex_unlock(&hdev->lazy_ctx_creation_lock);
return valid;
diff --git a/drivers/misc/habanalabs/device.c b/drivers/misc/habanalabs/device.c
index b63beb73ec76..a791a1b58215 100644
--- a/drivers/misc/habanalabs/device.c
+++ b/drivers/misc/habanalabs/device.c
@@ -289,9 +289,13 @@ static void set_freq_to_low_job(struct work_struct *work)
struct hl_device *hdev = container_of(work, struct hl_device,
work_freq.work);
- if (atomic_read(&hdev->fd_open_cnt) == 0)
+ mutex_lock(&hdev->lazy_ctx_creation_lock);
+
+ if (!hdev->user_ctx)
hl_device_set_frequency(hdev, PLL_LOW);
+ mutex_unlock(&hdev->lazy_ctx_creation_lock);
+
schedule_delayed_work(&hdev->work_freq,
usecs_to_jiffies(HL_PLL_LOW_JOB_FREQ_USEC));
}
@@ -341,7 +345,7 @@ static int device_late_init(struct hl_device *hdev)
hdev->high_pll = hdev->asic_prop.high_pll;
/* force setting to low frequency */
- atomic_set(&hdev->curr_pll_profile, PLL_LOW);
+ hdev->curr_pll_profile = PLL_LOW;
if (hdev->pm_mng_profile == PM_AUTO)
hdev->asic_funcs->set_pll_profile(hdev, PLL_LOW);
@@ -390,38 +394,26 @@ static void device_late_fini(struct hl_device *hdev)
* @hdev: pointer to habanalabs device structure
* @freq: the new frequency value
*
- * Change the frequency if needed.
- * We allose to set PLL to low only if there is no user process
- * Returns 0 if no change was done, otherwise returns 1;
+ * Change the frequency if needed. This function has no protection against
+ * concurrency, therefore it is assumed that the calling function has protected
+ * itself against the case of calling this function from multiple threads with
+ * different values
+ *
+ * Returns 0 if no change was done, otherwise returns 1
*/
int hl_device_set_frequency(struct hl_device *hdev, enum hl_pll_frequency freq)
{
- enum hl_pll_frequency old_freq =
- (freq == PLL_HIGH) ? PLL_LOW : PLL_HIGH;
- int ret;
-
- if (hdev->pm_mng_profile == PM_MANUAL)
- return 0;
-
- ret = atomic_cmpxchg(&hdev->curr_pll_profile, old_freq, freq);
- if (ret == freq)
+ if ((hdev->pm_mng_profile == PM_MANUAL) ||
+ (hdev->curr_pll_profile == freq))
return 0;
- /*
- * in case we want to lower frequency, check if device is not
- * opened. We must have a check here to workaround race condition with
- * hl_device_open
- */
- if ((freq == PLL_LOW) && (atomic_read(&hdev->fd_open_cnt) > 0)) {
- atomic_set(&hdev->curr_pll_profile, PLL_HIGH);
- return 0;
- }
-
dev_dbg(hdev->dev, "Changing device frequency to %s\n",
freq == PLL_HIGH ? "high" : "low");
hdev->asic_funcs->set_pll_profile(hdev, freq);
+ hdev->curr_pll_profile = freq;
+
return 1;
}
diff --git a/drivers/misc/habanalabs/goya/goya_hwmgr.c b/drivers/misc/habanalabs/goya/goya_hwmgr.c
index a51d836542a1..8522c6e0a25e 100644
--- a/drivers/misc/habanalabs/goya/goya_hwmgr.c
+++ b/drivers/misc/habanalabs/goya/goya_hwmgr.c
@@ -254,11 +254,11 @@ static ssize_t pm_mng_profile_store(struct device *dev,
goto out;
}
- mutex_lock(&hdev->fd_open_cnt_lock);
+ mutex_lock(&hdev->lazy_ctx_creation_lock);
- if (atomic_read(&hdev->fd_open_cnt) > 0) {
+ if (hdev->user_ctx) {
dev_err(hdev->dev,
- "Can't change PM profile while user process is opened on the device\n");
+ "Can't change PM profile while user context is opened on the device\n");
count = -EPERM;
goto unlock_mutex;
}
@@ -266,7 +266,7 @@ static ssize_t pm_mng_profile_store(struct device *dev,
if (strncmp("auto", buf, strlen("auto")) == 0) {
/* Make sure we are in LOW PLL when changing modes */
if (hdev->pm_mng_profile == PM_MANUAL) {
- atomic_set(&hdev->curr_pll_profile, PLL_HIGH);
+ hdev->curr_pll_profile = PLL_HIGH;
hl_device_set_frequency(hdev, PLL_LOW);
hdev->pm_mng_profile = PM_AUTO;
}
@@ -279,11 +279,10 @@ static ssize_t pm_mng_profile_store(struct device *dev,
} else {
dev_err(hdev->dev, "value should be auto or manual\n");
count = -EINVAL;
- goto unlock_mutex;
}
unlock_mutex:
- mutex_unlock(&hdev->fd_open_cnt_lock);
+ mutex_unlock(&hdev->lazy_ctx_creation_lock);
out:
return count;
}
diff --git a/drivers/misc/habanalabs/habanalabs.h b/drivers/misc/habanalabs/habanalabs.h
index d0e55839d673..6354fc88ef8a 100644
--- a/drivers/misc/habanalabs/habanalabs.h
+++ b/drivers/misc/habanalabs/habanalabs.h
@@ -1190,10 +1190,10 @@ struct hl_device_reset_work {
* value is saved so in case of hard-reset, KMD will restore this
* value and update the F/W after the re-initialization
* @in_reset: is device in reset flow.
- * @curr_pll_profile: current PLL profile.
* @fd_open_cnt: number of open user processes.
* @cs_active_cnt: number of active command submissions on this device (active
* means already in H/W queues)
+ * @curr_pll_profile: current PLL profile.
* @major: habanalabs KMD major.
* @high_pll: high PLL profile frequency.
* @soft_reset_cnt: number of soft reset since KMD loading.
@@ -1268,9 +1268,9 @@ struct hl_device {
u64 timeout_jiffies;
u64 max_power;
atomic_t in_reset;
- atomic_t curr_pll_profile;
atomic_t fd_open_cnt;
atomic_t cs_active_cnt;
+ enum hl_pll_frequency curr_pll_profile;
u32 major;
u32 high_pll;
u32 soft_reset_cnt;
diff --git a/drivers/misc/habanalabs/habanalabs_drv.c b/drivers/misc/habanalabs/habanalabs_drv.c
index b2c52e46e130..a781aa936160 100644
--- a/drivers/misc/habanalabs/habanalabs_drv.c
+++ b/drivers/misc/habanalabs/habanalabs_drv.c
@@ -95,42 +95,40 @@ int hl_device_open(struct inode *inode, struct file *filp)
return -ENXIO;
}
+ hpriv = kzalloc(sizeof(*hpriv), GFP_KERNEL);
+ if (!hpriv)
+ return -ENOMEM;
+
mutex_lock(&hdev->fd_open_cnt_lock);
if (hl_device_disabled_or_in_reset(hdev)) {
dev_err_ratelimited(hdev->dev,
"Can't open %s because it is disabled or in reset\n",
dev_name(hdev->dev));
- mutex_unlock(&hdev->fd_open_cnt_lock);
- return -EPERM;
+ rc = -EPERM;
+ goto out_err;
}
if (hdev->in_debug) {
dev_err_ratelimited(hdev->dev,
"Can't open %s because it is being debugged by another user\n",
dev_name(hdev->dev));
- mutex_unlock(&hdev->fd_open_cnt_lock);
- return -EPERM;
+ rc = -EPERM;
+ goto out_err;
}
if (atomic_read(&hdev->fd_open_cnt)) {
dev_info_ratelimited(hdev->dev,
"Can't open %s because another user is working on it\n",
dev_name(hdev->dev));
- mutex_unlock(&hdev->fd_open_cnt_lock);
- return -EBUSY;
+ rc = -EBUSY;
+ goto out_err;
}
atomic_inc(&hdev->fd_open_cnt);
mutex_unlock(&hdev->fd_open_cnt_lock);
- hpriv = kzalloc(sizeof(*hpriv), GFP_KERNEL);
- if (!hpriv) {
- rc = -ENOMEM;
- goto close_device;
- }
-
hpriv->hdev = hdev;
filp->private_data = hpriv;
hpriv->filp = filp;
@@ -143,19 +141,13 @@ int hl_device_open(struct inode *inode, struct file *filp)
hpriv->taskpid = find_get_pid(current->pid);
- /*
- * Device is IDLE at this point so it is legal to change PLLs. There
- * is no need to check anything because if the PLL is already HIGH, the
- * set function will return without doing anything
- */
- hl_device_set_frequency(hdev, PLL_HIGH);
-
hl_debugfs_add_file(hpriv);
return 0;
-close_device:
- atomic_dec(&hdev->fd_open_cnt);
+out_err:
+ mutex_unlock(&hdev->fd_open_cnt_lock);
+ kfree(hpriv);
return rc;
}
--
2.17.1
Powered by blists - more mailing lists