lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <YB9EDzI7mSrzXUUB@rocinante>
Date:   Sun, 7 Feb 2021 02:36:15 +0100
From:   Krzysztof WilczyƄski <kw@...ux.com>
To:     Gustavo Pimentel <Gustavo.Pimentel@...opsys.com>
Cc:     linux-doc@...r.kernel.org, linux-pci@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Derek Kiernan <derek.kiernan@...inx.com>,
        Dragan Cvetic <dragan.cvetic@...inx.com>,
        Arnd Bergmann <arnd@...db.de>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jonathan Corbet <corbet@....net>
Subject: 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, &data[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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ