Re: [RESEND v4 1/6] misc: Add Synopsys DesignWare xData IP driver
On Tue, Feb 09, 2021 at 03:28:16PM +, Gustavo Pimentel wrote:
> On Mon, Feb 8, 2021 at 22:53:54, Krzysztof Wilczyński
> wrote:
> > [...]
> > > Thanks for your review. I will wait for a couple of days, before sending
> > > a new version of this patch series based on your feedback.
> >
> > Thank you!
> >
> > There might be one more change, and improvement, to be done as per
> > Bjorn's feedback, see:
> >
> >
> > https://urldefense.com/v3/__https://lore.kernel.org/linux-pci/20210208193516.GA406304@bjorn-Precision-5520/__;!!A4F2R9G_pg!Oxp56pU_UN6M2BhfNRSdYqsFUncqVklBj_1IdLQD_w_V6dKRPDO_FjPUystMa5D39SRj8uo$
> >
> >
> > The code in question would be (exceprt from the patch):
> >
> > [...]
> > +static int dw_xdata_pcie_probe(struct pci_dev *pdev,
> > + const struct pci_device_id *pid)
> > +{
> > + const struct dw_xdata_pcie_data *pdata = (void *)pid->driver_data;
> > + struct dw_xdata *dw;
> > [...]
> > + dw->rg_region.vaddr = pcim_iomap_table(pdev)[pdata->rg_bar];
> > + if (!dw->rg_region.vaddr)
> > + return -ENOMEM;
> > [...]
> >
> > Perhaps something like the following would would?
> >
> > void __iomem * const *iomap_table;
> >
> > iomap_table = pcim_iomap_table(pdev);
> > if (!iomap_table)
> > return -ENOMEM;
> >
> > dw->rg_region.vaddr = iomap_table[pdata->rg_bar];
> > if (!dw->rg_region.vaddr)
> > return -ENOMEM;
> >
> > With sensible error messages added, of course. What do you think?
>
> I think all the improvements are welcome. I will do that.
> My only doubt is if Bjorn recommends removing the
> iomap_table[pdata->rg_bar] check, after adding the verification on the
> pcim_iomap_table, because all other drivers doesn't do that.
I misunderstood the usage of pcim_iomap_table() -- it looks like one
must call pcim_iomap_regions() *first*, and test its result, and
*that* is where we should catch any pcim_iomap_table() failures, e.g.,
rc = pcim_iomap_regions() # or pcim_iomap_regions_request_all()
if (rc)
return rc;# pcim_iomap_table() or other failure
vaddr = pcim_iomap_table()[BAR];
if (!vaddr)
return -ENOMEM; # BAR doesn't exist
You *do* correctly call pcim_iomap_regions() first, which calls
pcim_imap_table() internally, so if pcim_iomap_table() were to return
NULL, you should catch it there.
Then we assume that the subsequent "pcim_iomap_table()[BAR]" call will
succeed and NOT return NULL, so it should be safe to index into the
table. And if the table[BAR] entry is NULL, it means the BAR doesn't
exist or isn't mapped.
That sort of makes sense, but the API design doesn't quite seem
obviously correct to me. The table was created by
pcim_iomap_regions(), and pcim_iomap_table() is basically retrieving
that artifact.
I wonder if it could be improved by making pcim_iomap_table() strictly
internal to devres.c and having the pcim_iomap functions return the
table directly. Then the code would look something like this:
table = pcim_iomap_regions();
if (IS_ERR(table))
return PTR_ERR(table);# pcim_iomap_table() or other failure
vaddr = table[BAR]; # "table" is guaranteed to be non-NULL
if (!vaddr)
return -ENOMEM;
Obviously this is not something you should do for *this* series.
I think you should follow the example of other drivers, which means
keeping your patch exactly as you posted it. I'm just interested in
opinions on this as a possible future API improvement.
Bjorn
Re: [RESEND v4 1/6] misc: Add Synopsys DesignWare xData IP driver
Hi Gustavo,
[...]
> > The code in question would be (exceprt from the patch):
> >
> > [...]
> > +static int dw_xdata_pcie_probe(struct pci_dev *pdev,
> > + const struct pci_device_id *pid)
> > +{
> > + const struct dw_xdata_pcie_data *pdata = (void *)pid->driver_data;
> > + struct dw_xdata *dw;
> > [...]
> > + dw->rg_region.vaddr = pcim_iomap_table(pdev)[pdata->rg_bar];
> > + if (!dw->rg_region.vaddr)
> > + return -ENOMEM;
> > [...]
> >
> > Perhaps something like the following would would?
> >
> > void __iomem * const *iomap_table;
> >
> > iomap_table = pcim_iomap_table(pdev);
> > if (!iomap_table)
> > return -ENOMEM;
> >
> > dw->rg_region.vaddr = iomap_table[pdata->rg_bar];
> > if (!dw->rg_region.vaddr)
> > return -ENOMEM;
> >
> > With sensible error messages added, of course. What do you think?
>
> I think all the improvements are welcome. I will do that.
> My only doubt is if Bjorn recommends removing the
> iomap_table[pdata->rg_bar] check, after adding the verification on the
> pcim_iomap_table, because all other drivers doesn't do that.
Good point. I only found two drivers that do this extra check:
https://elixir.bootlin.com/linux/v5.11-rc7/source/drivers/crypto/ccp/sp-pci.c#L203
https://elixir.bootlin.com/linux/v5.11-rc7/source/drivers/net/ethernet/amd/xgbe/xgbe-pci.c#L252
Bjorn, do you think that there is some likelihood that the table might
be missing a mapped address for a given BAR?
I don't think that is the case, but should it be the case, then perhaps
adding a small wrapper that would take a BAR and do all the verification
internally might be a good idea.
Krzysztof
RE: [RESEND v4 1/6] misc: Add Synopsys DesignWare xData IP driver
On Mon, Feb 8, 2021 at 22:53:54, Krzysztof Wilczyński
wrote:
> [+cc Bjorn]
>
> Hi Gustavo,
>
> [...]
> > Thanks for your review. I will wait for a couple of days, before sending
> > a new version of this patch series based on your feedback.
>
> Thank you!
>
> There might be one more change, and improvement, to be done as per
> Bjorn's feedback, see:
>
>
> https://urldefense.com/v3/__https://lore.kernel.org/linux-pci/20210208193516.GA406304@bjorn-Precision-5520/__;!!A4F2R9G_pg!Oxp56pU_UN6M2BhfNRSdYqsFUncqVklBj_1IdLQD_w_V6dKRPDO_FjPUystMa5D39SRj8uo$
>
>
> The code in question would be (exceprt from the patch):
>
> [...]
> +static int dw_xdata_pcie_probe(struct pci_dev *pdev,
> +const struct pci_device_id *pid)
> +{
> + const struct dw_xdata_pcie_data *pdata = (void *)pid->driver_data;
> + struct dw_xdata *dw;
> [...]
> + dw->rg_region.vaddr = pcim_iomap_table(pdev)[pdata->rg_bar];
> + if (!dw->rg_region.vaddr)
> + return -ENOMEM;
> [...]
>
> Perhaps something like the following would would?
>
> void __iomem * const *iomap_table;
>
> iomap_table = pcim_iomap_table(pdev);
> if (!iomap_table)
> return -ENOMEM;
>
> dw->rg_region.vaddr = iomap_table[pdata->rg_bar];
> if (!dw->rg_region.vaddr)
> return -ENOMEM;
>
> With sensible error messages added, of course. What do you think?
I think all the improvements are welcome. I will do that.
My only doubt is if Bjorn recommends removing the
iomap_table[pdata->rg_bar] check, after adding the verification on the
pcim_iomap_table, because all other drivers doesn't do that.
Thanks!
-Gustavo
>
> Krzysztof
Re: [RESEND v4 1/6] misc: Add Synopsys DesignWare xData IP driver
[+cc Bjorn]
Hi Gustavo,
[...]
> Thanks for your review. I will wait for a couple of days, before sending
> a new version of this patch series based on your feedback.
Thank you!
There might be one more change, and improvement, to be done as per
Bjorn's feedback, see:
https://lore.kernel.org/linux-pci/20210208193516.GA406304@bjorn-Precision-5520/
The code in question would be (exceprt from the patch):
[...]
+static int dw_xdata_pcie_probe(struct pci_dev *pdev,
+ const struct pci_device_id *pid)
+{
+ const struct dw_xdata_pcie_data *pdata = (void *)pid->driver_data;
+ struct dw_xdata *dw;
[...]
+ dw->rg_region.vaddr = pcim_iomap_table(pdev)[pdata->rg_bar];
+ if (!dw->rg_region.vaddr)
+ return -ENOMEM;
[...]
Perhaps something like the following would would?
void __iomem * const *iomap_table;
iomap_table = pcim_iomap_table(pdev);
if (!iomap_table)
return -ENOMEM;
dw->rg_region.vaddr = iomap_table[pdata->rg_bar];
if (!dw->rg_region.vaddr)
return -ENOMEM;
With sensible error messages added, of course. What do you think?
Krzysztof
RE: [RESEND v4 1/6] misc: Add Synopsys DesignWare xData IP driver
On Sun, Feb 7, 2021 at 1:36:15, Krzysztof Wilczyński
wrote:
> Hi Gustavo,
>
> Thank you for all the work here!
>
> A few suggestions.
>
> [...]
> > +static void dw_xdata_stop(struct dw_xdata *dw)
> > +{
> > + u32 burst = readl(&(__dw_xdara_regs(dw)->burst_cnt));
> > +
> > + if (burst & BIT(31)) {
> > + burst &= ~(u32)BIT(31);
> > + writel(burst, &(__dw_regs(dw)->burst_cnt));
> > + }
> > +}
>
> Would it be possible to add a define for this "BIT(31)", similarly to
> the "XPERF_CONTROL_ENABLE", for example:
>
> #define XPERF_CONTROL_ENABLEBIT(5)
> #define XPERF_CONTROL_DISABLE BIT(31)
>
> What do you think?
I will a define for them.
>
> > +static void dw_xdata_start(struct dw_xdata *dw, bool write)
> > +{
> > + u32 control, status;
> > +
> > + /* Stop first if xfer in progress */
> > + dw_xdata_stop(dw);
> > +
> > + /* Clear status register */
> > + writel(0x0, &(__dw_regs(dw)->status));
> > +
> > + /* Burst count register set for continuous until stopped */
> > + writel(0x80001001, &(__dw_regs(dw)->burst_cnt));
>
> Would you mind adding a define (possibly with a comment, if it makes
> sense, of course) rather than open coding it here.
Ditto.
>
> > + /* Pattern register */
> > + writel(0x0, &(__dw_regs(dw)->pattern));
> > +
> > + /* Control register */
> > + control = CONTROL_DOORBELL | CONTROL_PATTERN_INC | CONTROL_NO_ADDR_INC;
> > + if (write) {
> > + control |= CONTROL_IS_WRITE;
> > + control |= CONTROL_LENGTH(dw->max_wr_len);
> > + } else {
> > + control |= CONTROL_LENGTH(dw->max_rd_len);
> > + }
> > + writel(control, &(__dw_regs(dw)->control));
> > +
> > + usleep_range(100, 150);
> [...]
>
> Why sleep here?
>
> Do you just add some delay that changes were reflected, or is it
> a requirement of sorts? What do you think about documenting why the
> sleep where? Would it make sense?
It requires to wait for some to allow the HW block to start the traffic
generation.
I will some comments explaining that.
>
> [...]
> > +static void dw_xdata_perf(struct dw_xdata *dw, u64 *rate, bool write)
> > +{
> > + u64 data[2], time[2], diff;
> > +
> > + /* First measurement */
> > + writel(0x0, &(__dw_regs(dw)->perf_control));
> > + dw_xdata_perf_meas(dw, [0], write);
> > + time[0] = jiffies;
> > + writel((u32)XPERF_CONTROL_ENABLE, &(__dw_regs(dw)->perf_control));
> > +
> > + /* Delay 100ms */
> > + mdelay(100);
> [...]
>
> The mdelay() is self-explanatory, so a comment that explains why to take
> two measurements that are 100 ms apart and how rate is calculated might
> be more useful (unless it would be an overkill here).
>
> If this is an arbitrary delay without any special meaning, then probably
> no comment is needed here.
>
> What do you think?
>
> [...]
> > + /* Calculations */
>
> What sort of calculations precisely? :)
Speed calculation. I will add some explanation about it on the code.
>
> [...]
> > +static int dw_xdata_pcie_probe(struct pci_dev *pdev,
> > + const struct pci_device_id *pid)
> > +{
> > + const struct dw_xdata_pcie_data *pdata = (void *)pid->driver_data;
> > + struct dw_xdata *dw;
> > + u64 addr;
> > + int err;
> > +
> > + /* Enable PCI device */
> > + err = pcim_enable_device(pdev);
>
> This comment might not be needed.
I normally put these comments as a code bookmark.
>
> [...]
> > + /* Mapping PCI BAR regions */
> > + err = pcim_iomap_regions(pdev, BIT(pdata->rg_bar), pci_name(pdev));
>
> This comment might also not be needed.
>
> [...]
> > + /* Allocate memory */
>
> And so this comment.
>
> [...]
> > + /* Data structure initialization */
> [...]
> > + /* Saving data structure reference */
> [...]
> > + /* Sysfs */
> [...]
>
> And possibly few of these are also not needed.
>
> Krzysztof
Thanks for your review. I will wait for a couple of days, before sending
a new version of this patch series based on your feedback.
-Gustavo
Re: [RESEND v4 1/6] misc: Add Synopsys DesignWare xData IP driver
Hi Gustavo,
Thank you for all the work here!
A few suggestions.
[...]
> +static void dw_xdata_stop(struct dw_xdata *dw)
> +{
> + u32 burst = readl(&(__dw_xdara_regs(dw)->burst_cnt));
> +
> + if (burst & BIT(31)) {
> + burst &= ~(u32)BIT(31);
> + writel(burst, &(__dw_regs(dw)->burst_cnt));
> + }
> +}
Would it be possible to add a define for this "BIT(31)", similarly to
the "XPERF_CONTROL_ENABLE", for example:
#define XPERF_CONTROL_ENABLE BIT(5)
#define XPERF_CONTROL_DISABLE BIT(31)
What do you think?
> +static void dw_xdata_start(struct dw_xdata *dw, bool write)
> +{
> + u32 control, status;
> +
> + /* Stop first if xfer in progress */
> + dw_xdata_stop(dw);
> +
> + /* Clear status register */
> + writel(0x0, &(__dw_regs(dw)->status));
> +
> + /* Burst count register set for continuous until stopped */
> + writel(0x80001001, &(__dw_regs(dw)->burst_cnt));
Would you mind adding a define (possibly with a comment, if it makes
sense, of course) rather than open coding it here.
> + /* Pattern register */
> + writel(0x0, &(__dw_regs(dw)->pattern));
> +
> + /* Control register */
> + control = CONTROL_DOORBELL | CONTROL_PATTERN_INC | CONTROL_NO_ADDR_INC;
> + if (write) {
> + control |= CONTROL_IS_WRITE;
> + control |= CONTROL_LENGTH(dw->max_wr_len);
> + } else {
> + control |= CONTROL_LENGTH(dw->max_rd_len);
> + }
> + writel(control, &(__dw_regs(dw)->control));
> +
> + usleep_range(100, 150);
[...]
Why sleep here?
Do you just add some delay that changes were reflected, or is it
a requirement of sorts? What do you think about documenting why the
sleep where? Would it make sense?
[...]
> +static void dw_xdata_perf(struct dw_xdata *dw, u64 *rate, bool write)
> +{
> + u64 data[2], time[2], diff;
> +
> + /* First measurement */
> + writel(0x0, &(__dw_regs(dw)->perf_control));
> + dw_xdata_perf_meas(dw, [0], write);
> + time[0] = jiffies;
> + writel((u32)XPERF_CONTROL_ENABLE, &(__dw_regs(dw)->perf_control));
> +
> + /* Delay 100ms */
> + mdelay(100);
[...]
The mdelay() is self-explanatory, so a comment that explains why to take
two measurements that are 100 ms apart and how rate is calculated might
be more useful (unless it would be an overkill here).
If this is an arbitrary delay without any special meaning, then probably
no comment is needed here.
What do you think?
[...]
> + /* Calculations */
What sort of calculations precisely? :)
[...]
> +static int dw_xdata_pcie_probe(struct pci_dev *pdev,
> +const struct pci_device_id *pid)
> +{
> + const struct dw_xdata_pcie_data *pdata = (void *)pid->driver_data;
> + struct dw_xdata *dw;
> + u64 addr;
> + int err;
> +
> + /* Enable PCI device */
> + err = pcim_enable_device(pdev);
This comment might not be needed.
[...]
> + /* Mapping PCI BAR regions */
> + err = pcim_iomap_regions(pdev, BIT(pdata->rg_bar), pci_name(pdev));
This comment might also not be needed.
[...]
> + /* Allocate memory */
And so this comment.
[...]
> + /* Data structure initialization */
[...]
> + /* Saving data structure reference */
[...]
> + /* Sysfs */
[...]
And possibly few of these are also not needed.
Krzysztof
Re: [RESEND v4 1/6] misc: Add Synopsys DesignWare xData IP driver
On Wed, Feb 03, 2021 at 11:12:46PM +0100, Gustavo Pimentel wrote: > + /* Sysfs */ > + err = sysfs_create_group(>dev.kobj, _attr_group); > + if (err) > + return err; > + > + err = sysfs_create_link(kernel_kobj, >dev.kobj, > + DW_XDATA_DRIVER_NAME); > + if (err) > + return err; Huge hint, if you EVER call sysfs_* in a driver, you are doing something wrong. You just raced userspace and lost, use the default attribute group for your driver so that the driver core can automatically create the needed sysfs files. And drop the symlink, that's just crazy, never do that, I don't think it's doing what you think it is doing, not to mention you did not document it... thanks, greg k-h
