On 6/15/2023 7:36 PM, Greg Kroah-Hartman wrote:
On Thu, Jun 15, 2023 at 07:17:34PM +0530, Souradeep Chowdhury wrote:
+static ssize_t ready_read(struct file *filp, char __user *userbuf,
+ size_t count, loff_t *ppos)
+{
+ int ret = 0;
+ char *buf;
+ struct dcc_drvdata *drvdata = filp->private_data;
+
+ mutex_lock(&drvdata->mutex);
+
+ if (!is_dcc_enabled(drvdata)) {
+ ret = -EINVAL;
+ goto out_unlock;
+ }
+
+ if (!FIELD_GET(BIT(1), readl(drvdata->base + dcc_status(drvdata->mem_map_ver))))
+ buf = "Y\n";
+ else
+ buf = "N\n";
+out_unlock:
+ mutex_unlock(&drvdata->mutex);
+
+ if (ret < 0)
+ return -EINVAL;
+ else
You do the "lock, get a value, unlock, do something with the value"
thing a bunch, but what prevents the value from changing after the lock
happens? So why is the lock needed at all?
The lock is used to prevent concurrent accesses of the drv_data when
scripts are being run from userspace.
How would that matter? The state can change instantly after the lock is
given up, and then the returned value is now incorrect. So no need for
a lock at all as you really aren't "protecting" anything, or am I
missing something else?
This lock is needed to protect the access to the global instance of drv_data
structure instantiated at probe time within each individual callbacks of
debugfs.
What exactly are you "protecting" here that could change in a way that
cause a problem?
You aren't returning a value that is ever guaranteed to be "correct"
except that it happened sometime in the past, it might be right anymore.
Hi Greg,
The lock doesn't add any value in this particular case and I will be
dropping it from here but in other cases it is being used to protect the
concurrent access of the data-structures used inside the drv_data mainly
the list which is being used to append register configurations, write
the configuration to the dcc_sram and also delete it while doing a
config reset. The lock is also used in case of software trigger to read
the bitmap of the lists to set register values.
Thanks,
Souradeep
thanks,
greg k-h
