Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Gustavo Pimentel <Gustavo.Pimentel@synopsys.com>
To:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>
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
Thread-Topic: [PATCH v5 1/6] misc: Add Synopsys DesignWare xData IP driver
Thread-Index: AQHXAFWPyAqykiKVp0+x9nNcxSm5BKpSsFcAgAAErwCAAAN3AIAAAIDg
Date:   Thu, 11 Feb 2021 10:21:07 +0000
Message-ID: <DM5PR12MB1835522220E35874106BB924DA8C9@DM5PR12MB1835.namprd12.prod.outlook.com>
References: <cover.1613034397.git.gustavo.pimentel@synopsys.com>
 <02835da8fc8c9293fecbe666a8db3fb79276fdde.1613034397.git.gustavo.pimentel@synopsys.com>
 <YCT5KDnAWex8fvbz@kroah.com>
 <DM5PR12MB1835A23E60363C730E4D69AFDA8C9@DM5PR12MB1835.namprd12.prod.outlook.com>
 <YCT//nmQpOJD9row@kroah.com>
In-Reply-To: <YCT//nmQpOJD9row@kroah.com>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 6be8513f-314d-470a-c96f-08d8ce76bed0
X-MS-Exchange-CrossTenant-originalarrivaltime: 11 Feb 2021 10:21:07.4212
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: c33c9f88-1eb7-4099-9700-16013fd9e8aa
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: Nd2kc/na+G9yijTBw6rR7kyzCa6YPpRkT5LQekqNtZawHlVCyCVsHQdUhyZ2SJM1KNiU+g5A9v4+YRGcCywYUw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR12MB4957
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Thu, Feb 11, 2021 at 9:59:26, Greg Kroah-Hartman=20
<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=20
> > <gregkh@linuxfoundation.org> wrote:
> >=20
> > > On Thu, Feb 11, 2021 at 10:08:38AM +0100, Gustavo Pimentel wrote:
> > > > +static ssize_t write_show(struct device *dev, struct device_attrib=
ute *attr,
> > > > +			  char *buf)
> > > > +{
> > > > +	struct pci_dev *pdev =3D to_pci_dev(dev);
> > > > +	struct dw_xdata *dw =3D 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);
> > >=20
> > > 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.
> >=20
> > Okay.
> >=20
> > >=20
> > > Same for the other sysfs file.
> > >=20
> > > And why do you need a lock for this show function?
> >=20
> > Maybe I understood it wrongly, please correct me in that case. The=20
> > dw_xdata_perf() is called on the write_show() and read_show(), to avoid=
 a=20
> > possible race condition between those calls, I have added this mutex.
>=20
> What race?  If the value changes with a write right after a read, what
> does it matter?
>=20
> What exactly are you trying to protect with this lock?

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

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

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

This is the reason why I added those mutexes, maybe this isn't necessary=20
and it's overkill. Please advise me if a different approach can be done.

-Gustavo

>=20
> thanks,
>=20
> greg k-h