Re: [PATCH v5 1/6] misc: Add Synopsys DesignWare xData IP driver
On Thu, Feb 11, 2021 at 03:02:03PM +0100, Greg Kroah-Hartman wrote:
> On Thu, Feb 11, 2021 at 03:50:19PM +0200, Leon Romanovsky wrote:
> > On Thu, Feb 11, 2021 at 01:32:13PM +0100, Greg Kroah-Hartman wrote:
> > > On Thu, Feb 11, 2021 at 01:42:43PM +0200, Leon Romanovsky wrote:
> > > > On Thu, Feb 11, 2021 at 10:08:38AM +0100, Gustavo Pimentel wrote:
> > > > > Add Synopsys DesignWare xData IP driver. This driver enables/disables
> > > > > the PCI traffic generator module pertain to the Synopsys DesignWare
> > > > > prototype.
> > > > >
> > > > > Signed-off-by: Gustavo Pimentel
> > > > > ---
> > > > > drivers/misc/dw-xdata-pcie.c | 394
> > > > > +++
> > > > > 1 file changed, 394 insertions(+)
> > > > > create mode 100644 drivers/misc/dw-xdata-pcie.c
> > > >
> > > > <...>
> > > >
> > > > > +MODULE_LICENSE("GPL v2");
> > > >
> > > > "GPL" and not "GPL v2".
> > >
> > > There is no difference, please go read module.h.
> >
> > I read and this is why I said it.
> > Documentation/process/license-rules.rst: "It exists for historic reasons."
> >
> > Historic, for me, means that new code is better do not use this.
>
> Nope, either is fine for new code, author gets to pick what they want.
> Personally, I like the explicitness of "GPL v2" for a variety of
> reasons.
Feel free to update the documentation.
Personally, I don't like two names for the same license. This is one of
the reasons why SPFX clearly marks the code license, It is why we have
"SPDX-License-Identifier: GPL-2.0" and not "SPDX-License-Identifier: GPL".
https://spdx.org/licenses/preview/
GNU General Public License v2.0 onlyGPL-2.0-only
GNU General Public License v2.0 or laterGPL-2.0-or-later
Thanks
>
> thanks,
>
> greg k-h
Re: [PATCH v5 1/6] misc: Add Synopsys DesignWare xData IP driver
On Thu, Feb 11, 2021 at 03:50:19PM +0200, Leon Romanovsky wrote:
> On Thu, Feb 11, 2021 at 01:32:13PM +0100, Greg Kroah-Hartman wrote:
> > On Thu, Feb 11, 2021 at 01:42:43PM +0200, Leon Romanovsky wrote:
> > > On Thu, Feb 11, 2021 at 10:08:38AM +0100, Gustavo Pimentel wrote:
> > > > Add Synopsys DesignWare xData IP driver. This driver enables/disables
> > > > the PCI traffic generator module pertain to the Synopsys DesignWare
> > > > prototype.
> > > >
> > > > Signed-off-by: Gustavo Pimentel
> > > > ---
> > > > drivers/misc/dw-xdata-pcie.c | 394
> > > > +++
> > > > 1 file changed, 394 insertions(+)
> > > > create mode 100644 drivers/misc/dw-xdata-pcie.c
> > >
> > > <...>
> > >
> > > > +MODULE_LICENSE("GPL v2");
> > >
> > > "GPL" and not "GPL v2".
> >
> > There is no difference, please go read module.h.
>
> I read and this is why I said it.
> Documentation/process/license-rules.rst: "It exists for historic reasons."
>
> Historic, for me, means that new code is better do not use this.
Nope, either is fine for new code, author gets to pick what they want.
Personally, I like the explicitness of "GPL v2" for a variety of
reasons.
thanks,
greg k-h
Re: [PATCH v5 1/6] misc: Add Synopsys DesignWare xData IP driver
On Thu, Feb 11, 2021 at 01:32:13PM +0100, Greg Kroah-Hartman wrote:
> On Thu, Feb 11, 2021 at 01:42:43PM +0200, Leon Romanovsky wrote:
> > On Thu, Feb 11, 2021 at 10:08:38AM +0100, Gustavo Pimentel wrote:
> > > Add Synopsys DesignWare xData IP driver. This driver enables/disables
> > > the PCI traffic generator module pertain to the Synopsys DesignWare
> > > prototype.
> > >
> > > Signed-off-by: Gustavo Pimentel
> > > ---
> > > drivers/misc/dw-xdata-pcie.c | 394
> > > +++
> > > 1 file changed, 394 insertions(+)
> > > create mode 100644 drivers/misc/dw-xdata-pcie.c
> >
> > <...>
> >
> > > +MODULE_LICENSE("GPL v2");
> >
> > "GPL" and not "GPL v2".
>
> There is no difference, please go read module.h.
I read and this is why I said it.
Documentation/process/license-rules.rst: "It exists for historic reasons."
Historic, for me, means that new code is better do not use this.
Thanks
Re: [PATCH v5 1/6] misc: Add Synopsys DesignWare xData IP driver
On Thu, Feb 11, 2021 at 01:42:43PM +0200, Leon Romanovsky wrote:
> On Thu, Feb 11, 2021 at 10:08:38AM +0100, Gustavo Pimentel wrote:
> > Add Synopsys DesignWare xData IP driver. This driver enables/disables
> > the PCI traffic generator module pertain to the Synopsys DesignWare
> > prototype.
> >
> > Signed-off-by: Gustavo Pimentel
> > ---
> > drivers/misc/dw-xdata-pcie.c | 394
> > +++
> > 1 file changed, 394 insertions(+)
> > create mode 100644 drivers/misc/dw-xdata-pcie.c
>
> <...>
>
> > +MODULE_LICENSE("GPL v2");
>
> "GPL" and not "GPL v2".
There is no difference, please go read module.h.
Re: [PATCH v5 1/6] misc: Add Synopsys DesignWare xData IP driver
On Thu, Feb 11, 2021 at 10:08:38AM +0100, Gustavo Pimentel wrote:
> Add Synopsys DesignWare xData IP driver. This driver enables/disables
> the PCI traffic generator module pertain to the Synopsys DesignWare
> prototype.
>
> Signed-off-by: Gustavo Pimentel
> ---
> drivers/misc/dw-xdata-pcie.c | 394
> +++
> 1 file changed, 394 insertions(+)
> create mode 100644 drivers/misc/dw-xdata-pcie.c
<...>
> +MODULE_LICENSE("GPL v2");
"GPL" and not "GPL v2".
Thanks
> +MODULE_DESCRIPTION("Synopsys DesignWare xData PCIe driver");
> +MODULE_AUTHOR("Gustavo Pimentel ");
> +
> --
> 2.7.4
>
RE: [PATCH v5 1/6] misc: Add Synopsys DesignWare xData IP driver
On Thu, Feb 11, 2021 at 10:33:25, Greg Kroah-Hartman
wrote:
> On Thu, Feb 11, 2021 at 10:21:07AM +, Gustavo Pimentel wrote:
> > On Thu, Feb 11, 2021 at 9:59:26, Greg Kroah-Hartman
> > wrote:
> >
> > > On Thu, Feb 11, 2021 at 09:50:33AM +, Gustavo Pimentel wrote:
> > > > On Thu, Feb 11, 2021 at 9:30:16, Greg Kroah-Hartman
> > > > 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.
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
Re: [PATCH v5 1/6] misc: Add Synopsys DesignWare xData IP driver
On Thu, Feb 11, 2021 at 10:21:07AM +, Gustavo Pimentel wrote:
> On Thu, Feb 11, 2021 at 9:59:26, Greg Kroah-Hartman
> wrote:
>
> > On Thu, Feb 11, 2021 at 09:50:33AM +, Gustavo Pimentel wrote:
> > > On Thu, Feb 11, 2021 at 9:30:16, Greg Kroah-Hartman
> > > 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
RE: [PATCH v5 1/6] misc: Add Synopsys DesignWare xData IP driver
On Thu, Feb 11, 2021 at 9:59:26, Greg Kroah-Hartman
wrote:
> On Thu, Feb 11, 2021 at 09:50:33AM +, Gustavo Pimentel wrote:
> > On Thu, Feb 11, 2021 at 9:30:16, Greg Kroah-Hartman
> > 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.
This is the reason why I added those mutexes, maybe this isn't necessary
and it's overkill. Please advise me if a different approach can be done.
-Gustavo
>
> thanks,
>
> greg k-h
Re: [PATCH v5 1/6] misc: Add Synopsys DesignWare xData IP driver
On Thu, Feb 11, 2021 at 09:50:33AM +, Gustavo Pimentel wrote:
> On Thu, Feb 11, 2021 at 9:30:16, Greg Kroah-Hartman
> 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?
thanks,
greg k-h
RE: [PATCH v5 1/6] misc: Add Synopsys DesignWare xData IP driver
On Thu, Feb 11, 2021 at 9:30:16, Greg Kroah-Hartman
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.
Thanks.
-Gustavo
>
> thanks,
>
> greg k-h
Re: [PATCH v5 1/6] misc: Add Synopsys DesignWare xData IP driver
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.
Same for the other sysfs file.
And why do you need a lock for this show function?
thanks,
greg k-h
