Re: [PATCH V23 2/3] misc: dcc: Add driver support for Data Capture and Compare unit(DCC)

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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



[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux