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

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Thu, Feb 11, 2021 at 10:21:07AM +0000, Gustavo Pimentel wrote:
> On Thu, Feb 11, 2021 at 9:59:26, Greg Kroah-Hartman 
> <gregkh@linuxfoundation.org> wrote:
> 
> > On Thu, Feb 11, 2021 at 09:50:33AM +0000, Gustavo Pimentel wrote:
> > > On Thu, Feb 11, 2021 at 9:30:16, Greg Kroah-Hartman 
> > > <gregkh@linuxfoundation.org> wrote:
> > > 
> > > > On Thu, Feb 11, 2021 at 10:08:38AM +0100, Gustavo Pimentel wrote:
> > > > > +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.
> > > 
> > > > 
> > > > 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.

thanks,

greg k-h