[v8,5/5] FIX driver

Commit Message

Gustavo Pimentel March 29, 2021, 9:51 a.m. UTC

Signed-off-by: Gustavo Pimentel <[e-mail]>
---
 drivers/misc/dw-xdata-pcie.c | 99 +++++++++++++++++++++++++-------------------
 1 file changed, 57 insertions(+), 42 deletions(-)

Comments

Willy Tarreau March 29, 2021, 10:03 a.m. UTC | #1

On Mon, Mar 29, 2021 at 11:51:38AM +0200, Gustavo Pimentel wrote:
> Signed-off-by: Gustavo Pimentel <[e-mail]>

Please make an effort, this is in no way an acceptable commit description
for a patch. The subject is already extremely vague "FIX driver" with no
context at all, and there is no description of the intent here. What is
someone supposed to think about the risk of keeping or reverting it if a
bisect section would end on this one ?

Thanks,
Willy

Greg Kroah-Hartman March 29, 2021, 10:06 a.m. UTC | #2

On Mon, Mar 29, 2021 at 11:51:38AM +0200, Gustavo Pimentel wrote:
> Signed-off-by: Gustavo Pimentel <[e-mail]>
> ---
>  drivers/misc/dw-xdata-pcie.c | 99 +++++++++++++++++++++++++-------------------
>  1 file changed, 57 insertions(+), 42 deletions(-)

Hi,

This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
a patch that has triggered this response.  He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created.  Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.

You are receiving this message because of the following common error(s)
as indicated below:

- Your patch did many different things all at once, making it difficult
  to review.  All Linux kernel patches need to only do one thing at a
  time.  If you need to do multiple things (such as clean up all coding
  style issues in a file/driver), do it in a sequence of patches, each
  one doing only one thing.  This will make it easier to review the
  patches to ensure that they are correct, and to help alleviate any
  merge issues that larger patches can cause.

- You did not specify a description of why the patch is needed, or
  possibly, any description at all, in the email body.  Please read the
  section entitled "The canonical patch format" in the kernel file,
  Documentation/SubmittingPatches for what is needed in order to
  properly describe the change.

- You did not write a descriptive Subject: for the patch, allowing Greg,
  and everyone else, to know what this patch is all about.  Please read
  the section entitled "The canonical patch format" in the kernel file,
  Documentation/SubmittingPatches for what a proper Subject: line should
  look like.

If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.

thanks,

greg k-h's patch email bot

Gustavo Pimentel March 29, 2021, 10:09 a.m. UTC | #3

On Mon, Mar 29, 2021 at 11:3:3, Willy Tarreau <[e-mail]> wrote:

> On Mon, Mar 29, 2021 at 11:51:38AM +0200, Gustavo Pimentel wrote:
> > Signed-off-by: Gustavo Pimentel <[e-mail]>
> 
> Please make an effort, this is in no way an acceptable commit description
> for a patch. The subject is already extremely vague "FIX driver" with no
> context at all, and there is no description of the intent here. What is
> someone supposed to think about the risk of keeping or reverting it if a
> bisect section would end on this one ?

Hi Willy,

this patch was sent by mistake, it was already squashed on patch #1 on 
v9.
Please check your inbox for the patch series v9, that was sent a few 
minutes ago.

-Gustavo

> 
> Thanks,
> Willy

Patch

diff --git a/drivers/misc/dw-xdata-pcie.c b/drivers/misc/dw-xdata-pcie.c
index 43fdd35..011516b 100644
--- a/drivers/misc/dw-xdata-pcie.c
+++ b/drivers/misc/dw-xdata-pcie.c
@@ -21,19 +21,7 @@ 
 
 #define DW_XDATA_EP_MEM_OFFSET		0x8000000
 
-struct dw_xdata_pcie_data {
-	/* xData registers location */
-	enum pci_barno			rg_bar;
-	off_t				rg_off;
-	size_t				rg_sz;
-};
-
-static const struct dw_xdata_pcie_data snps_edda_data = {
-	/* xData registers location */
-	.rg_bar				= BAR_0,
-	.rg_off				= 0x00000000,   /*   0 Kbytes */
-	.rg_sz				= 0x0000012c,   /* 300  bytes */
-};
+static DEFINE_IDA(xdata_ida);
 
 #define STATUS_DONE			BIT(0)
 
@@ -71,7 +59,6 @@  struct dw_xdata_regs {
 struct dw_xdata_region {
 	phys_addr_t paddr;				/* physical address */
 	void __iomem *vaddr;				/* virtual address */
-	size_t sz;					/* size */
 };
 
 struct dw_xdata {
@@ -80,7 +67,6 @@  struct dw_xdata {
 	size_t max_rd_len;				/* max rd xfer len */
 	struct mutex mutex;
 	struct pci_dev *pdev;
-	struct device *dev;
 	struct miscdevice misc_dev;
 };
 
@@ -107,6 +93,7 @@  static void dw_xdata_stop(struct dw_xdata *dw)
 
 static void dw_xdata_start(struct dw_xdata *dw, bool write)
 {
+	struct device *dev = &dw->pdev->dev;
 	u32 control, status;
 
 	/* Stop first if xfer in progress */
@@ -144,7 +131,7 @@  static void dw_xdata_start(struct dw_xdata *dw, bool write)
 	mutex_unlock(&dw->mutex);
 
 	if (!(status & STATUS_DONE))
-		pci_dbg(dw->pdev, "xData: started %s direction\n",
+		dev_dbg(dev, "xData: started %s direction\n",
 			write ? "write" : "read");
 }
 
@@ -174,6 +161,7 @@  static u64 dw_xdata_perf_diff(u64 *m1, u64 *m2, u64 time)
 
 static void dw_xdata_perf(struct dw_xdata *dw, u64 *rate, bool write)
 {
+	struct device *dev = &dw->pdev->dev;
 	u64 data[2], time[2], diff;
 
 	mutex_lock(&dw->mutex);
@@ -206,7 +194,7 @@  static void dw_xdata_perf(struct dw_xdata *dw, u64 *rate, bool write)
 
 	mutex_unlock(&dw->mutex);
 
-	pci_dbg(dw->pdev, "xData: time=%llu us, %s=%llu MB/s\n",
+	dev_dbg(dev, "xData: time=%llu us, %s=%llu MB/s\n",
 		diff, write ? "write" : "read", *rate);
 }
 
@@ -233,7 +221,7 @@  static ssize_t write_store(struct device *dev, struct device_attribute *attr,
 	struct miscdevice *misc_dev = dev_get_drvdata(dev);
 	struct dw_xdata *dw = misc_dev_to_dw(misc_dev);
 
-	pci_dbg(dw->pdev, "xData: requested write transfer\n");
+	dev_dbg(dev, "xData: requested write transfer\n");
 
 	dw_xdata_start(dw, true);
 
@@ -260,7 +248,7 @@  static ssize_t read_store(struct device *dev, struct device_attribute *attr,
 	struct miscdevice *misc_dev = dev_get_drvdata(dev);
 	struct dw_xdata *dw = misc_dev_to_dw(misc_dev);
 
-	pci_dbg(dw->pdev, "xData: requested read transfer\n");
+	dev_dbg(dev, "xData: requested read transfer\n");
 
 	dw_xdata_start(dw, false);
 
@@ -275,7 +263,7 @@  static ssize_t stop_store(struct device *dev, struct device_attribute *attr,
 	struct miscdevice *misc_dev = dev_get_drvdata(dev);
 	struct dw_xdata *dw = misc_dev_to_dw(misc_dev);
 
-	pci_dbg(dw->pdev, "xData: requested stop any transfer\n");
+	dev_dbg(dev, "xData: requested stop any transfer\n");
 
 	dw_xdata_stop(dw);
 
@@ -296,43 +284,42 @@  ATTRIBUTE_GROUPS(xdata);
 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 device *dev = &pdev->dev;
 	struct dw_xdata *dw;
+	char name[24];
 	u64 addr;
 	int err;
+	int id;
 
 	/* Enable PCI device */
 	err = pcim_enable_device(pdev);
 	if (err) {
-		pci_err(pdev, "enabling device failed\n");
+		dev_err(dev, "enabling device failed\n");
 		return err;
 	}
 
 	/* Mapping PCI BAR regions */
-	err = pcim_iomap_regions(pdev, BIT(pdata->rg_bar), pci_name(pdev));
+	err = pcim_iomap_regions(pdev, BIT(BAR_0), pci_name(pdev));
 	if (err) {
-		pci_err(pdev, "xData BAR I/O remapping failed\n");
+		dev_err(dev, "xData BAR I/O remapping failed\n");
 		return err;
 	}
 
 	pci_set_master(pdev);
 
 	/* Allocate memory */
-	dw = devm_kzalloc(&pdev->dev, sizeof(*dw), GFP_KERNEL);
+	dw = devm_kzalloc(dev, sizeof(*dw), GFP_KERNEL);
 	if (!dw)
 		return -ENOMEM;
 
 	/* Data structure initialization */
 	mutex_init(&dw->mutex);
 
-	dw->rg_region.vaddr = pcim_iomap_table(pdev)[pdata->rg_bar];
+	dw->rg_region.vaddr = pcim_iomap_table(pdev)[BAR_0];
 	if (!dw->rg_region.vaddr)
 		return -ENOMEM;
 
-	dw->rg_region.vaddr += pdata->rg_off;
-	dw->rg_region.paddr = pdev->resource[pdata->rg_bar].start;
-	dw->rg_region.paddr += pdata->rg_off;
-	dw->rg_region.sz = pdata->rg_sz;
+	dw->rg_region.paddr = pdev->resource[BAR_0].start;
 
 	dw->max_wr_len = pcie_get_mps(pdev);
 	dw->max_wr_len >>= 2;
@@ -341,11 +328,22 @@  static int dw_xdata_pcie_probe(struct pci_dev *pdev,
 	dw->max_rd_len >>= 2;
 
 	dw->pdev = pdev;
-	dw->dev = &pdev->dev;
+
+	id = ida_simple_get(&xdata_ida, 0, 0, GFP_KERNEL);
+	if (id < 0) {
+		dev_err(dev, "xData: unable to get id\n");
+		return id;
+	}
+
+	snprintf(name, sizeof(name), DW_XDATA_DRIVER_NAME ".%d", id);
+	dw->misc_dev.name = kstrdup(name, GFP_KERNEL);
+	if (!dw->misc_dev.name) {
+		err = -ENOMEM;
+		goto err_ida_remove;
+	}
 
 	dw->misc_dev.minor = MISC_DYNAMIC_MINOR;
-	dw->misc_dev.name = kstrdup(DW_XDATA_DRIVER_NAME, GFP_KERNEL);
-	dw->misc_dev.parent = &pdev->dev;
+	dw->misc_dev.parent = dev;
 	dw->misc_dev.groups = xdata_groups;
 
 	writel(0x0, &(__dw_regs(dw)->RAM_addr));
@@ -354,9 +352,9 @@  static int dw_xdata_pcie_probe(struct pci_dev *pdev,
 	addr = dw->rg_region.paddr + DW_XDATA_EP_MEM_OFFSET;
 	writel(lower_32_bits(addr), &(__dw_regs(dw)->addr_lsb));
 	writel(upper_32_bits(addr), &(__dw_regs(dw)->addr_msb));
-	pci_dbg(pdev, "xData: target address = 0x%.16llx\n", addr);
+	dev_dbg(dev, "xData: target address = 0x%.16llx\n", addr);
 
-	pci_dbg(pdev, "xData: wr_len = %zu, rd_len = %zu\n",
+	dev_dbg(dev, "xData: wr_len = %zu, rd_len = %zu\n",
 		dw->max_wr_len * 4, dw->max_rd_len * 4);
 
 	/* Saving data structure reference */
@@ -364,24 +362,41 @@  static int dw_xdata_pcie_probe(struct pci_dev *pdev,
 
 	/* Register misc device */
 	err = misc_register(&dw->misc_dev);
-	if (err)
-		return err;
+	if (err) {
+		dev_err(dev, "xData: failed to register device\n");
+		goto err_kfree_name;
+	}
 
 	return 0;
+
+err_kfree_name:
+	kfree(dw->misc_dev.name);
+
+err_ida_remove:
+	ida_simple_remove(&xdata_ida, id);
+
+	return err;
 }
 
 static void dw_xdata_pcie_remove(struct pci_dev *pdev)
 {
 	struct dw_xdata *dw = pci_get_drvdata(pdev);
+	int id;
 
-	if (dw) {
-		dw_xdata_stop(dw);
-		misc_deregister(&dw->misc_dev);
-	}
+	if (sscanf(dw->misc_dev.name, DW_XDATA_DRIVER_NAME ".%d", &id) != 1)
+		return;
+
+	if (id < 0)
+		return;
+
+	dw_xdata_stop(dw);
+	misc_deregister(&dw->misc_dev);
+	kfree(dw->misc_dev.name);
+	ida_simple_remove(&xdata_ida, id);
 }
 
 static const struct pci_device_id dw_xdata_pcie_id_table[] = {
-	{ PCI_DEVICE_DATA(SYNOPSYS, EDDA, &snps_edda_data) },
+	{ PCI_DEVICE_DATA(SYNOPSYS, EDDA, NULL) },
 	{ }
 };
 MODULE_DEVICE_TABLE(pci, dw_xdata_pcie_id_table);