linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: Gustavo Pimentel <Gustavo.Pimentel@synopsys.com>
Cc: "linux-doc@vger.kernel.org" <linux-doc@vger.kernel.org>,
	"linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Derek Kiernan <derek.kiernan@xilinx.com>,
	Dragan Cvetic <dragan.cvetic@xilinx.com>,
	Arnd Bergmann <arnd@arndb.de>,
	Andrew Morton <akpm@linux-foundation.org>,
	Jonathan Corbet <corbet@lwn.net>,
	Bjorn Helgaas <bhelgaas@google.com>
Subject: Re: [PATCH v5 1/6] misc: Add Synopsys DesignWare xData IP driver
Date: Thu, 11 Feb 2021 11:33:25 +0100	[thread overview]
Message-ID: <YCUH9eCwiJiB5t3g@kroah.com> (raw)
In-Reply-To: <DM5PR12MB1835522220E35874106BB924DA8C9@DM5PR12MB1835.namprd12.prod.outlook.com>

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

  reply	other threads:[~2021-02-11 10:37 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-02-11  9:08 [PATCH v5 0/6] misc: Add " Gustavo Pimentel
2021-02-11  9:08 ` [PATCH v5 1/6] misc: " Gustavo Pimentel
2021-02-11  9:30   ` Greg Kroah-Hartman
2021-02-11  9:50     ` Gustavo Pimentel
2021-02-11  9:59       ` Greg Kroah-Hartman
2021-02-11 10:21         ` Gustavo Pimentel
2021-02-11 10:33           ` Greg Kroah-Hartman [this message]
2021-02-11 10:51             ` Gustavo Pimentel
2021-02-11 11:42   ` Leon Romanovsky
2021-02-11 12:32     ` Greg Kroah-Hartman
2021-02-11 13:50       ` Leon Romanovsky
2021-02-11 14:02         ` Greg Kroah-Hartman
2021-02-11 14:13           ` Leon Romanovsky
2021-02-11  9:08 ` [PATCH v5 2/6] misc: Add Synopsys DesignWare xData IP driver to Makefile Gustavo Pimentel
2021-02-11  9:28   ` Greg Kroah-Hartman
2021-02-11  9:40     ` Gustavo Pimentel
2021-02-11  9:08 ` [PATCH v5 3/6] misc: Add Synopsys DesignWare xData IP driver to Kconfig Gustavo Pimentel
2021-02-11 11:29   ` Krzysztof Wilczyński
2021-02-11 13:33     ` Gustavo Pimentel
2021-02-11  9:08 ` [PATCH v5 4/6] Documentation: misc-devices: Add Documentation for dw-xdata-pcie driver Gustavo Pimentel
2021-02-11  9:08 ` [PATCH v5 5/6] MAINTAINERS: Add Synopsys xData IP driver maintainer Gustavo Pimentel
2021-02-11  9:08 ` [PATCH v5 6/6] docs: ABI: Add sysfs documentation interface of dw-xdata-pcie driver Gustavo Pimentel
2021-02-11  9:29   ` Leon Romanovsky
2021-02-11  9:36     ` Greg Kroah-Hartman

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YCUH9eCwiJiB5t3g@kroah.com \
    --to=gregkh@linuxfoundation.org \
    --cc=Gustavo.Pimentel@synopsys.com \
    --cc=akpm@linux-foundation.org \
    --cc=arnd@arndb.de \
    --cc=bhelgaas@google.com \
    --cc=corbet@lwn.net \
    --cc=derek.kiernan@xilinx.com \
    --cc=dragan.cvetic@xilinx.com \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --subject='Re: [PATCH v5 1/6] misc: Add Synopsys DesignWare xData IP driver' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).