RE: [PATCH v5 1/6] misc: Add Synopsys DesignWare xData IP driver

On Thu, Feb 11, 2021 at 10:33:25, Greg Kroah-Hartman 
[off-list ref] wrote:

On Thu, Feb 11, 2021 at 10:21:07AM +0000, Gustavo Pimentel wrote:

quoted

On Thu, Feb 11, 2021 at 9:59:26, Greg Kroah-Hartman 
[off-list ref] wrote:

quoted

On Thu, Feb 11, 2021 at 09:50:33AM +0000, Gustavo Pimentel wrote:

quoted

On Thu, Feb 11, 2021 at 9:30:16, Greg Kroah-Hartman 
[off-list ref] wrote:

quoted

On Thu, Feb 11, 2021 at 10:08:38AM +0100, Gustavo Pimentel wrote:

quoted

+static ssize_t write_show(struct device *dev, struct device_attribute *attr,
+			  char *buf)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+	struct dw_xdata *dw = pci_get_drvdata(pdev);
+	u64 rate;
+
+	mutex_lock(&dw->mutex);
+	dw_xdata_perf(dw, &rate, true);
+	mutex_unlock(&dw->mutex);
+
+	return sysfs_emit(buf, "%llu MB/s\n", rate);

Do not put units in a sysfs file, that should be in the documentation,
otherwise this forces userspace to "parse" the units which is a mess.

Okay.

quoted

Same for the other sysfs file.

And why do you need a lock for this show function?

Maybe I understood it wrongly, please correct me in that case. The 
dw_xdata_perf() is called on the write_show() and read_show(), to avoid a 
possible race condition between those calls, I have added this mutex.

What race?  If the value changes with a write right after a read, what
does it matter?

What exactly are you trying to protect with this lock?

The write_store() does a procedure to enable the traffic on the write 
direction, however, the write_show() does a different procedure to 
calculate the link throughput speed, which uses a different set of 
registers on the HW.

Similar happens on the read_store() (which enable the traffic on the read 
direction) and on the read_show()

To summarize write_store() follows the same approach of read_store() and 
the write_show() of the read_show(). I added the mutex on those functions 
for instance to avoid while during the write_show() call the possibility 
of been called the read_show() messing up the link throughput speed 
calculation.
Or while during the write_store() call to be called the read_store or 
even the write_show() for the same reasons.

If you need to protect these types of things, but the lock down in the
function that does this, not above it which forces people to audit
everything and manually try to determine what lock is doing what for
what.

Make it impossible to get wrong, as it is, you have to do extra work
here to keep things working properly, always a bad idea in an api.

I think I understood what you mean, I will *reduce* the mutex scope to 
the basic functions that are called by the sysfs *_store() and *_show().

-Gustavo

thanks,

greg k-h