Re: [PATCH v2 2/3] trafgen: xilinx: add axi traffic generator driver

From: Greg KH
Date: Mon Dec 09 2019 - 03:06:58 EST

Next message: Greg KH: "Re: [PATCH v2 1/3] dt-bindings: misc: Add dt bindings for traffic generator"
Previous message: Xiaotao Yin: "[PATCH] iommu/iova: kmemleak when pfn_lo equals IOVA_ANCHOR"
In reply to: shubhrajyoti . datta: "[PATCH v2 2/3] trafgen: xilinx: add axi traffic generator driver"
Next in thread: Greg KH: "Re: [PATCH v2 1/3] dt-bindings: misc: Add dt bindings for traffic generator"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Mon, Dec 09, 2019 at 11:41:27AM +0530, shubhrajyoti.datta@xxxxxxxxx wrote:
> +/* Macro */

We know it's a macro, no need to say it :)

> +#define to_xtg_dev_info(n) ((struct xtg_dev_info *)dev_get_drvdata(n))

No need for the cast, and really, no need for this macro at all. Please
drop it.

> + * FIXME: This structure is shared with the user application and
> + * hence need to be synchronized. We know these kind of structures
> + * should not be defined in the driver and this need to be fixed
> + * if found a proper placeholder (in uapi/).

Woah! This isn't ok to leave as a fixme. Please fix properly.

As it is, this is NOT a portable data structure by any means.

> + * FIXME: This structure is shared with the user application and
> + * hence need to be synchronized. We know these kind of structures
> + * should not be defined in the driver and this need to be fixed
> + * if found a proper placeholder (in uapi/).

Same here, this is just flat out not going to work.

> +static void xtg_access_rams(struct xtg_dev_info *tg, int where,
> + int count, int flags, u32 *data)
> +{
> + u32 index;
> +
> + switch (flags) {
> + case XTG_WRITE_RAM_ZERO:
> + memset_io(tg->regs + where, 0, count);
> +#ifdef CONFIG_PHYS_ADDR_T_64BIT

No #ifdef in .c code please, fix this correctly.

> + writel(0x0, tg->regs + where +
> + (XTG_COMMAND_RAM_MSB_OFFSET - XTG_COMMAND_RAM_OFFSET) +
> + XTG_EXTCMD_RAM_BLOCK_SIZE - XTG_CMD_RAM_BLOCK_SIZE);
> +#endif
> + break;
> + case XTG_WRITE_RAM:
> + for (index = 0; count > 0; index++, count -= 4)
> + writel(data[index], tg->regs + where + index * 4);
> +#ifdef CONFIG_PHYS_ADDR_T_64BIT

Same here, and everywhere else.

> +/**
> + * xtg_sysfs_ioctl - Implements sysfs operations

sysfs is not an ioctl. Please fix your naming.

> + * @dev: Device structure
> + * @buf: Value to write
> + * @opcode: Ioctl opcode
> + *
> + * Return: value read from the sysfs opcode.
> + */

Why are you creating kernel doc documentation for static functions?
That's not needed at all, right?

> +static ssize_t xtg_sysfs_ioctl(struct device *dev, const char *buf,
> + enum xtg_sysfs_ioctl_opcode opcode)
> +{
> + struct xtg_dev_info *tg = to_xtg_dev_info(dev);
> + unsigned long wrval;
> + ssize_t status, rdval = 0;
> +
> + if (opcode > XTG_GET_STREAM_TRANSFERCNT) {
> + status = kstrtoul(buf, 0, &wrval);
> + if (status < 0)
> + return status;
> + }
> +
> + switch (opcode) {
> + case XTG_GET_MASTER_CMP_STS:
> + rdval = (readl(tg->regs + XTG_MCNTL_OFFSET) &
> + XTG_MCNTL_MSTEN_MASK) ? 1 : 0;
> + break;
> +
> + case XTG_GET_MASTER_LOOP_EN:
> + rdval = (readl(tg->regs + XTG_MCNTL_OFFSET) &
> + XTG_MCNTL_LOOPEN_MASK) ? 1 : 0;
> + break;
> +
> + case XTG_GET_SLV_CTRL_REG:
> + rdval = readl(tg->regs + XTG_SCNTL_OFFSET);
> + break;
> +
> + case XTG_GET_ERR_STS:
> + rdval = readl(tg->regs + XTG_ERR_STS_OFFSET) &
> + XTG_ERR_ALL_ERRS_MASK;
> + break;
> +
> + case XTG_GET_CFG_STS:
> + rdval = readl(tg->regs + XTG_CFG_STS_OFFSET);
> + break;
> +
> + case XTG_GET_LAST_VALID_INDEX:
> + rdval = (((tg->last_wr_valid_idx << 16) & 0xffff0000) |
> + (tg->last_rd_valid_idx & 0xffff));
> + break;
> +
> + case XTG_GET_DEVICE_ID:
> + rdval = tg->id;
> + break;
> +
> + case XTG_GET_RESOURCE:
> + rdval = (unsigned long)tg->regs;
> + break;
> +
> + case XTG_GET_STATIC_ENABLE:
> + rdval = readl(tg->regs + XTG_STATIC_CNTL_OFFSET);
> + break;
> +
> + case XTG_GET_STATIC_BURSTLEN:
> + rdval = readl(tg->regs + XTG_STATIC_LEN_OFFSET);
> + break;
> +
> + case XTG_GET_STATIC_TRANSFERDONE:
> + rdval = (readl(tg->regs + XTG_STATIC_CNTL_OFFSET) &
> + XTG_STATIC_CNTL_TD_MASK);
> + break;
> +
> + case XTG_GET_STREAM_ENABLE:
> + rdval = readl(tg->regs + XTG_STREAM_CNTL_OFFSET);
> + break;
> +
> + case XTG_GET_STREAM_TRANSFERLEN:
> + rdval = (readl(tg->regs + XTG_STREAM_TL_OFFSET) &
> + XTG_STREAM_TL_TLEN_MASK);
> + break;
> +
> + case XTG_GET_STREAM_TRANSFERCNT:
> + rdval = ((readl(tg->regs + XTG_STREAM_TL_OFFSET) &
> + XTG_STREAM_TL_TCNT_MASK) >>
> + XTG_STREAM_TL_TCNT_SHIFT);
> + break;
> +
> + case XTG_GET_STREAM_TKTS1:
> + rdval = readl(tg->regs + XTG_STREAM_TKTS1_OFFSET);
> + break;
> + case XTG_GET_STREAM_TKTS2:
> + rdval = readl(tg->regs + XTG_STREAM_TKTS2_OFFSET);
> + break;
> + case XTG_GET_STREAM_TKTS3:
> + rdval = readl(tg->regs + XTG_STREAM_TKTS3_OFFSET);
> + break;
> + case XTG_GET_STREAM_TKTS4:
> + rdval = readl(tg->regs + XTG_STREAM_TKTS4_OFFSET);
> + break;
> +
> + case XTG_GET_STREAM_CFG:
> + rdval = (readl(tg->regs + XTG_STREAM_CFG_OFFSET));
> + break;
> +
> + case XTG_START_MASTER_LOGIC:
> + if (wrval)
> + writel(readl(tg->regs + XTG_MCNTL_OFFSET) |
> + XTG_MCNTL_MSTEN_MASK,
> + tg->regs + XTG_MCNTL_OFFSET);
> + break;
> +
> + case XTG_MASTER_LOOP_EN:
> + if (wrval)
> + writel(readl(tg->regs + XTG_MCNTL_OFFSET) |
> + XTG_MCNTL_LOOPEN_MASK,
> + tg->regs + XTG_MCNTL_OFFSET);
> + else
> + writel(readl(tg->regs + XTG_MCNTL_OFFSET) &
> + ~XTG_MCNTL_LOOPEN_MASK,
> + tg->regs + XTG_MCNTL_OFFSET);
> + break;
> +
> + case XTG_SET_SLV_CTRL_REG:
> + writel(wrval, tg->regs + XTG_SCNTL_OFFSET);
> + break;
> +
> + case XTG_ENABLE_ERRORS:
> + wrval &= XTG_ERR_ALL_ERRS_MASK;
> + writel(wrval, tg->regs + XTG_ERR_EN_OFFSET);
> + break;
> +
> + case XTG_CLEAR_ERRORS:
> + wrval &= XTG_ERR_ALL_ERRS_MASK;
> + writel(readl(tg->regs + XTG_ERR_STS_OFFSET) | wrval,
> + tg->regs + XTG_ERR_STS_OFFSET);
> + break;
> +
> + case XTG_ENABLE_INTRS:
> + if (wrval & XTG_MASTER_CMP_INTR) {
> + pr_info("Enabling Master Complete Interrupt\n");
> + writel(readl(tg->regs + XTG_ERR_EN_OFFSET) |
> + XTG_ERR_EN_MSTIRQEN_MASK,
> + tg->regs + XTG_ERR_EN_OFFSET);
> + }
> + if (wrval & XTG_MASTER_ERR_INTR) {
> + pr_info("Enabling Interrupt on Master Errors\n");
> + writel(readl(tg->regs + XTG_MSTERR_INTR_OFFSET) |
> + XTG_MSTERR_INTR_MINTREN_MASK,
> + tg->regs + XTG_MSTERR_INTR_OFFSET);
> + }
> + if (wrval & XTG_SLAVE_ERR_INTR) {
> + pr_info("Enabling Interrupt on Slave Errors\n");
> + writel(readl(tg->regs + XTG_SCNTL_OFFSET) |
> + XTG_SCNTL_ERREN_MASK,
> + tg->regs + XTG_SCNTL_OFFSET);
> + }
> + break;
> +
> + case XTG_CLEAR_MRAM:
> + xtg_access_rams(tg, tg->xtg_mram_offset,
> + XTG_MASTER_RAM_SIZE,
> + XTG_WRITE_RAM_ZERO, NULL);
> + break;
> +
> + case XTG_CLEAR_CRAM:
> + xtg_access_rams(tg, XTG_COMMAND_RAM_OFFSET,
> + XTG_COMMAND_RAM_SIZE,
> + XTG_WRITE_RAM_ZERO, NULL);
> + break;
> +
> + case XTG_CLEAR_PRAM:
> + xtg_access_rams(tg, XTG_PARAM_RAM_OFFSET,
> + XTG_PARAM_RAM_SIZE,
> + XTG_WRITE_RAM_ZERO, NULL);
> + break;
> +
> + case XTG_SET_STATIC_ENABLE:
> + if (wrval) {
> + wrval &= XTG_STATIC_CNTL_STEN_MASK;
> + writel(readl(tg->regs + XTG_STATIC_CNTL_OFFSET) | wrval,
> + tg->regs + XTG_STATIC_CNTL_OFFSET);
> + } else {
> + writel(readl(tg->regs + XTG_STATIC_CNTL_OFFSET) &
> + ~XTG_STATIC_CNTL_STEN_MASK,
> + tg->regs + XTG_STATIC_CNTL_OFFSET);
> + }
> + break;
> +
> + case XTG_SET_STATIC_BURSTLEN:
> + writel(wrval, tg->regs + XTG_STATIC_LEN_OFFSET);
> + break;
> +
> + case XTG_SET_STATIC_TRANSFERDONE:
> + wrval |= XTG_STATIC_CNTL_TD_MASK;
> + writel(readl(tg->regs + XTG_STATIC_CNTL_OFFSET) | wrval,
> + tg->regs + XTG_STATIC_CNTL_OFFSET);
> + break;
> +
> + case XTG_SET_STREAM_ENABLE:
> + if (wrval) {
> + rdval = readl(tg->regs + XTG_STREAM_CNTL_OFFSET);
> + rdval |= XTG_STREAM_CNTL_STEN_MASK,
> + writel(rdval,
> + tg->regs + XTG_STREAM_CNTL_OFFSET);
> + } else {
> + writel(readl(tg->regs + XTG_STREAM_CNTL_OFFSET) &
> + ~XTG_STREAM_CNTL_STEN_MASK,
> + tg->regs + XTG_STREAM_CNTL_OFFSET);
> + }
> + break;
> +
> + case XTG_SET_STREAM_TRANSFERLEN:
> + wrval &= XTG_STREAM_TL_TLEN_MASK;
> + rdval = readl(tg->regs + XTG_STREAM_TL_OFFSET);
> + rdval &= ~XTG_STREAM_TL_TLEN_MASK;
> + writel(rdval | wrval,
> + tg->regs + XTG_STREAM_TL_OFFSET);
> + break;
> +
> + case XTG_SET_STREAM_TRANSFERCNT:
> + wrval = ((wrval << XTG_STREAM_TL_TCNT_SHIFT) &
> + XTG_STREAM_TL_TCNT_MASK);
> + rdval = readl(tg->regs + XTG_STREAM_TL_OFFSET);
> + rdval = rdval & ~XTG_STREAM_TL_TCNT_MASK;
> + writel(rdval | wrval,
> + tg->regs + XTG_STREAM_TL_OFFSET);
> + break;
> +
> + case XTG_SET_STREAM_TKTS1:
> + writel(wrval, tg->regs + XTG_STREAM_TKTS1_OFFSET);
> + break;
> + case XTG_SET_STREAM_TKTS2:
> + writel(wrval, tg->regs + XTG_STREAM_TKTS2_OFFSET);
> + break;
> + case XTG_SET_STREAM_TKTS3:
> + writel(wrval, tg->regs + XTG_STREAM_TKTS3_OFFSET);
> + break;
> + case XTG_SET_STREAM_TKTS4:
> + writel(wrval, tg->regs + XTG_STREAM_TKTS4_OFFSET);
> + break;
> +
> + case XTG_SET_STREAM_CFG:
> + writel(wrval, tg->regs + XTG_STREAM_CFG_OFFSET);
> + break;
> +
> + default:
> + break;
> + }
> +
> + return rdval;
> +}
> +
> +/* Sysfs functions */
> +
> +static ssize_t id_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + ssize_t rdval = xtg_sysfs_ioctl(dev, buf, XTG_GET_DEVICE_ID);
> +
> + return snprintf(buf, PAGE_SIZE, "%zd\n", rdval);

You "know" the size of the data for sysfs, no need for snprintf() or
friends at all, just use sprintf() please.

> +static ssize_t xtg_pram_read(struct file *filp, struct kobject *kobj,
> + struct bin_attribute *bin_attr,
> + char *buf, loff_t off, size_t count)
> +{
> + pr_info("No read access to Parameter RAM\n");
> +
> + return 0;
> +}

This is pointless, why do you have/need this?

Looks like a good way to spam the kernel log :(

And never use pr_* calls in a driver, always use dev_* instead.

> + /*
> + * Create sysfs file entries for the device
> + */
> + err = sysfs_create_group(&dev->kobj, &xtg_attributes);

You just raced with userspace and lost. Set the attribute groups
pointer up correctly and the driver core will create/remove your
attributes for you. This code does it wrong.

> + dev_info(&pdev->dev, "Probing xilinx traffic generator success\n");

Do NOT be noisy when everything goes correctly. Drivers should not
print out anything if all is well, please drop this.

thanks,

greg k-h

Next message: Greg KH: "Re: [PATCH v2 1/3] dt-bindings: misc: Add dt bindings for traffic generator"
Previous message: Xiaotao Yin: "[PATCH] iommu/iova: kmemleak when pfn_lo equals IOVA_ANCHOR"
In reply to: shubhrajyoti . datta: "[PATCH v2 2/3] trafgen: xilinx: add axi traffic generator driver"
Next in thread: Greg KH: "Re: [PATCH v2 1/3] dt-bindings: misc: Add dt bindings for traffic generator"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]