intel_scu_ipc: fix error path by turning to devm_* / pcim_*

PATCH Superseded Andy Shevchenko 2 participants 11 messages Oct 9, 2015 – Oct 12, 2015

Subsystems (3)

x86 platform drivers Intel MID (Mobile Internet Device) platform Intel scu drivers

lore

Share & tools

lore

Andy Shevchenko

author

2015-10-09 14:11 UTC #0 lore raw

PATCH 1/5 intel_scu_ipc: fix error path by turning to devm_* / pcim_*

The error handling is broken right now since it leaves resources unfreed. Convert the code to use managed resources to fix the error handling.

Signed-off-by Andy Shevchenko <[email protected]>

---
 drivers/platform/x86/intel_scu_ipc.c | 26 ++++++++------------------
 1 file changed, 8 insertions(+), 18 deletions(-)

drivers/platform/x86/intel_scu_ipc.c +8 ~~-18~~

index 187d108..7148535 100644
@@ -563,7 +563,6 @@ static int ipc_probe(struct pci_dev *dev, const struct pci_device_id *id)
 {
 	int err;
 	struct intel_scu_ipc_pdata_t *pdata;
-	resource_size_t base;
 
 	if (ipcdev.pdev)		/* We support only one SCU */
 		return -EBUSY;

@@ -573,32 +572,26 @@ static int ipc_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	ipcdev.pdev = pci_dev_get(dev);
 	ipcdev.irq_mode = pdata->irq_mode;
 
-	err = pci_enable_device(dev);
+	err = pcim_enable_device(dev);
 	if (err)
 		return err;
 
-	err = pci_request_regions(dev, "intel_scu_ipc");
+	err = pcim_iomap_regions(dev, 1 << 0, pci_name(dev));
 	if (err)
 		return err;
 
-	base = pci_resource_start(dev, 0);
-	if (!base)
-		return -ENOMEM;
-
 	init_completion(&ipcdev.cmd_complete);
 
-	if (request_irq(dev->irq, ioc, 0, "intel_scu_ipc", &ipcdev))
-		return -EBUSY;
+	err = devm_request_irq(&dev->dev, dev->irq, ioc, 0, "intel_scu_ipc",
+			       &ipcdev);
+	if (err)
+		return err;
 
-	ipcdev.ipc_base = ioremap_nocache(base, pci_resource_len(dev, 0));
-	if (!ipcdev.ipc_base)
-		return -ENOMEM;
+	ipcdev.ipc_base = pcim_iomap_table(dev)[0];
 
 	ipcdev.i2c_base = ioremap_nocache(pdata->i2c_base, pdata->i2c_len);
-	if (!ipcdev.i2c_base) {
-		iounmap(ipcdev.ipc_base);
+	if (!ipcdev.i2c_base)
 		return -ENOMEM;
-	}
 
 	intel_scu_devices_create();

@@ -617,10 +610,7 @@ static int ipc_probe(struct pci_dev *dev, const struct pci_device_id *id)
  */
 static void ipc_remove(struct pci_dev *pdev)
 {
-	free_irq(pdev->irq, &ipcdev);
-	pci_release_regions(pdev);
 	pci_dev_put(ipcdev.pdev);
-	iounmap(ipcdev.ipc_base);
 	iounmap(ipcdev.i2c_base);
 	ipcdev.pdev = NULL;
 	intel_scu_devices_destroy();

Signature · 1 line

2.5.3

Andy Shevchenko

author

2015-10-09 14:11 UTC #1 lore raw

PATCH 2/5 intel_scu_ipc: propagate struct intel_scu_ipc_dev

As much as poosible propagate struct intel_scu_ipc_dev.

There is no functional change.

Signed-off-by Andy Shevchenko <[email protected]>

---
 drivers/platform/x86/intel_scu_ipc.c | 134 +++++++++++++++++++----------------
 1 file changed, 74 insertions(+), 60 deletions(-)

drivers/platform/x86/intel_scu_ipc.c +74 ~~-60~~

index 7148535..6c9367f 100644
@@ -118,28 +118,30 @@ static struct intel_scu_ipc_dev  ipcdev; /* Only one for now */
 static DEFINE_MUTEX(ipclock); /* lock used to prevent multiple call to SCU */
 
 /*
+ * Send ipc command
  * Command Register (Write Only):
  * A write to this register results in an interrupt to the SCU core processor
  * Format:
  * |rfu2(8) | size(8) | command id(4) | rfu1(3) | ioc(1) | command(8)|
  */
-static inline void ipc_command(u32 cmd) /* Send ipc command */
+static inline void ipc_command(struct intel_scu_ipc_dev *scu, u32 cmd)
 {
-	if (ipcdev.irq_mode) {
-		reinit_completion(&ipcdev.cmd_complete);
-		writel(cmd | IPC_IOC, ipcdev.ipc_base);
+	if (scu->irq_mode) {
+		reinit_completion(&scu->cmd_complete);
+		writel(cmd | IPC_IOC, scu->ipc_base);
 	}
-	writel(cmd, ipcdev.ipc_base);
+	writel(cmd, scu->ipc_base);
 }
 
 /*
+ * Write ipc data
  * IPC Write Buffer (Write Only):
  * 16-byte buffer for sending data associated with IPC command to
  * SCU. Size of the data is specified in the IPC_COMMAND_REG register
  */
-static inline void ipc_data_writel(u32 data, u32 offset) /* Write ipc data */
+static inline void ipc_data_writel(struct intel_scu_ipc_dev *scu, u32 data, u32 offset)
 {
-	writel(data, ipcdev.ipc_base + 0x80 + offset);
+	writel(data, scu->ipc_base + 0x80 + offset);
 }
 
 /*

@@ -149,35 +151,37 @@ static inline void ipc_data_writel(u32 data, u32 offset) /* Write ipc data */
  * Format:
  * |rfu3(8)|error code(8)|initiator id(8)|cmd id(4)|rfu1(2)|error(1)|busy(1)|
  */
-static inline u8 ipc_read_status(void)
+static inline u8 ipc_read_status(struct intel_scu_ipc_dev *scu)
 {
-	return __raw_readl(ipcdev.ipc_base + 0x04);
+	return __raw_readl(scu->ipc_base + 0x04);
 }
 
-static inline u8 ipc_data_readb(u32 offset) /* Read ipc byte data */
+/* Read ipc byte data */
+static inline u8 ipc_data_readb(struct intel_scu_ipc_dev *scu, u32 offset)
 {
-	return readb(ipcdev.ipc_base + IPC_READ_BUFFER + offset);
+	return readb(scu->ipc_base + IPC_READ_BUFFER + offset);
 }
 
-static inline u32 ipc_data_readl(u32 offset) /* Read ipc u32 data */
+/* Read ipc u32 data */
+static inline u32 ipc_data_readl(struct intel_scu_ipc_dev *scu, u32 offset)
 {
-	return readl(ipcdev.ipc_base + IPC_READ_BUFFER + offset);
+	return readl(scu->ipc_base + IPC_READ_BUFFER + offset);
 }
 
 /* Wait till scu status is busy */
-static inline int busy_loop(void)
+static inline int busy_loop(struct intel_scu_ipc_dev *scu)
 {
-	u32 status = ipc_read_status();
+	u32 status = ipc_read_status(scu);
 	u32 loop_count = 100000;
 
 	/* break if scu doesn't reset busy bit after huge retry */
 	while ((status & BIT(0)) && --loop_count) {
 		udelay(1); /* scu processing time is in few u secods */
-		status = ipc_read_status();
+		status = ipc_read_status(scu);
 	}
 
 	if (status & BIT(0)) {
-		dev_err(&ipcdev.pdev->dev, "IPC timed out");
+		dev_err(&scu->pdev->dev, "IPC timed out");
 		return -ETIMEDOUT;
 	}

@@ -188,31 +192,32 @@ static inline int busy_loop(void)
 }
 
 /* Wait till ipc ioc interrupt is received or timeout in 3 HZ */
-static inline int ipc_wait_for_interrupt(void)
+static inline int ipc_wait_for_interrupt(struct intel_scu_ipc_dev *scu)
 {
 	int status;
 
-	if (!wait_for_completion_timeout(&ipcdev.cmd_complete, 3 * HZ)) {
-		struct device *dev = &ipcdev.pdev->dev;
+	if (!wait_for_completion_timeout(&scu->cmd_complete, 3 * HZ)) {
+		struct device *dev = &scu->pdev->dev;
 		dev_err(dev, "IPC timed out\n");
 		return -ETIMEDOUT;
 	}
 
-	status = ipc_read_status();
+	status = ipc_read_status(scu);
 	if (status & BIT(1))
 		return -EIO;
 
 	return 0;
 }
 
-static int intel_scu_ipc_check_status(void)
+static int intel_scu_ipc_check_status(struct intel_scu_ipc_dev *scu)
 {
-	return ipcdev.irq_mode ? ipc_wait_for_interrupt() : busy_loop();
+	return scu->irq_mode ? ipc_wait_for_interrupt(scu) : busy_loop(scu);
 }
 
 /* Read/Write power control(PMIC in Langwell, MSIC in PenWell) registers */
 static int pwr_reg_rdwr(u16 *addr, u8 *data, u32 count, u32 op, u32 id)
 {
+	struct intel_scu_ipc_dev *scu = &ipcdev;
 	int nc;
 	u32 offset = 0;
 	int err;

@@ -223,7 +228,7 @@ static int pwr_reg_rdwr(u16 *addr, u8 *data, u32 count, u32 op, u32 id)
 
 	mutex_lock(&ipclock);
 
-	if (ipcdev.pdev == NULL) {
+	if (scu->pdev == NULL) {
 		mutex_unlock(&ipclock);
 		return -ENODEV;
 	}

@@ -235,27 +240,27 @@ static int pwr_reg_rdwr(u16 *addr, u8 *data, u32 count, u32 op, u32 id)
 
 	if (id == IPC_CMD_PCNTRL_R) {
 		for (nc = 0, offset = 0; nc < count; nc++, offset += 4)
-			ipc_data_writel(wbuf[nc], offset);
-		ipc_command((count * 2) << 16 | id << 12 | 0 << 8 | op);
+			ipc_data_writel(scu, wbuf[nc], offset);
+		ipc_command(scu, (count * 2) << 16 | id << 12 | 0 << 8 | op);
 	} else if (id == IPC_CMD_PCNTRL_W) {
 		for (nc = 0; nc < count; nc++, offset += 1)
 			cbuf[offset] = data[nc];
 		for (nc = 0, offset = 0; nc < count; nc++, offset += 4)
-			ipc_data_writel(wbuf[nc], offset);
-		ipc_command((count * 3) << 16 | id << 12 | 0 << 8 | op);
+			ipc_data_writel(scu, wbuf[nc], offset);
+		ipc_command(scu, (count * 3) << 16 | id << 12 | 0 << 8 | op);
 	} else if (id == IPC_CMD_PCNTRL_M) {
 		cbuf[offset] = data[0];
 		cbuf[offset + 1] = data[1];
-		ipc_data_writel(wbuf[0], 0); /* Write wbuff */
-		ipc_command(4 << 16 | id << 12 | 0 << 8 | op);
+		ipc_data_writel(scu, wbuf[0], 0); /* Write wbuff */
+		ipc_command(scu, 4 << 16 | id << 12 | 0 << 8 | op);
 	}
 
-	err = intel_scu_ipc_check_status();
+	err = intel_scu_ipc_check_status(scu);
 	if (!err && id == IPC_CMD_PCNTRL_R) { /* Read rbuf */
 		/* Workaround: values are read as 0 without memcpy_fromio */
-		memcpy_fromio(cbuf, ipcdev.ipc_base + 0x90, 16);
+		memcpy_fromio(cbuf, scu->ipc_base + 0x90, 16);
 		for (nc = 0; nc < count; nc++)
-			data[nc] = ipc_data_readb(nc);
+			data[nc] = ipc_data_readb(scu, nc);
 	}
 	mutex_unlock(&ipclock);
 	return err;

@@ -436,15 +441,16 @@ EXPORT_SYMBOL(intel_scu_ipc_update_register);
  */
 int intel_scu_ipc_simple_command(int cmd, int sub)
 {
+	struct intel_scu_ipc_dev *scu = &ipcdev;
 	int err;
 
 	mutex_lock(&ipclock);
-	if (ipcdev.pdev == NULL) {
+	if (scu->pdev == NULL) {
 		mutex_unlock(&ipclock);
 		return -ENODEV;
 	}
-	ipc_command(sub << 12 | cmd);
-	err = intel_scu_ipc_check_status();
+	ipc_command(scu, sub << 12 | cmd);
+	err = intel_scu_ipc_check_status(scu);
 	mutex_unlock(&ipclock);
 	return err;
 }

@@ -465,23 +471,24 @@ EXPORT_SYMBOL(intel_scu_ipc_simple_command);
 int intel_scu_ipc_command(int cmd, int sub, u32 *in, int inlen,
 			  u32 *out, int outlen)
 {
+	struct intel_scu_ipc_dev *scu = &ipcdev;
 	int i, err;
 
 	mutex_lock(&ipclock);
-	if (ipcdev.pdev == NULL) {
+	if (scu->pdev == NULL) {
 		mutex_unlock(&ipclock);
 		return -ENODEV;
 	}
 
 	for (i = 0; i < inlen; i++)
-		ipc_data_writel(*in++, 4 * i);
+		ipc_data_writel(scu, *in++, 4 * i);
 
-	ipc_command((inlen << 16) | (sub << 12) | cmd);
-	err = intel_scu_ipc_check_status();
+	ipc_command(scu, (inlen << 16) | (sub << 12) | cmd);
+	err = intel_scu_ipc_check_status(scu);
 
 	if (!err) {
 		for (i = 0; i < outlen; i++)
-			*out++ = ipc_data_readl(4 * i);
+			*out++ = ipc_data_readl(scu, 4 * i);
 	}
 
 	mutex_unlock(&ipclock);

@@ -507,25 +514,26 @@ EXPORT_SYMBOL(intel_scu_ipc_command);
  */
 int intel_scu_ipc_i2c_cntrl(u32 addr, u32 *data)
 {
+	struct intel_scu_ipc_dev *scu = &ipcdev;
 	u32 cmd = 0;
 
 	mutex_lock(&ipclock);
-	if (ipcdev.pdev == NULL) {
+	if (scu->pdev == NULL) {
 		mutex_unlock(&ipclock);
 		return -ENODEV;
 	}
 	cmd = (addr >> 24) & 0xFF;
 	if (cmd == IPC_I2C_READ) {
-		writel(addr, ipcdev.i2c_base + IPC_I2C_CNTRL_ADDR);
+		writel(addr, scu->i2c_base + IPC_I2C_CNTRL_ADDR);
 		/* Write not getting updated without delay */
 		mdelay(1);
-		*data = readl(ipcdev.i2c_base + I2C_DATA_ADDR);
+		*data = readl(scu->i2c_base + I2C_DATA_ADDR);
 	} else if (cmd == IPC_I2C_WRITE) {
-		writel(*data, ipcdev.i2c_base + I2C_DATA_ADDR);
+		writel(*data, scu->i2c_base + I2C_DATA_ADDR);
 		mdelay(1);
-		writel(addr, ipcdev.i2c_base + IPC_I2C_CNTRL_ADDR);
+		writel(addr, scu->i2c_base + IPC_I2C_CNTRL_ADDR);
 	} else {
-		dev_err(&ipcdev.pdev->dev,
+		dev_err(&scu->pdev->dev,
 			"intel_scu_ipc: I2C INVALID_CMD = 0x%x\n", cmd);
 
 		mutex_unlock(&ipclock);

@@ -545,8 +553,10 @@ EXPORT_SYMBOL(intel_scu_ipc_i2c_cntrl);
  */
 static irqreturn_t ioc(int irq, void *dev_id)
 {
-	if (ipcdev.irq_mode)
-		complete(&ipcdev.cmd_complete);
+	struct intel_scu_ipc_dev *scu = dev_id;
+
+	if (scu->irq_mode)
+		complete(&scu->cmd_complete);
 
 	return IRQ_HANDLED;
 }

@@ -562,15 +572,16 @@ static irqreturn_t ioc(int irq, void *dev_id)
 static int ipc_probe(struct pci_dev *dev, const struct pci_device_id *id)
 {
 	int err;
+	struct intel_scu_ipc_dev *scu = &ipcdev;
 	struct intel_scu_ipc_pdata_t *pdata;
 
-	if (ipcdev.pdev)		/* We support only one SCU */
+	if (scu->pdev)		/* We support only one SCU */
 		return -EBUSY;
 
 	pdata = (struct intel_scu_ipc_pdata_t *)id->driver_data;
 
-	ipcdev.pdev = pci_dev_get(dev);
-	ipcdev.irq_mode = pdata->irq_mode;
+	scu->pdev = pci_dev_get(dev);
+	scu->irq_mode = pdata->irq_mode;
 
 	err = pcim_enable_device(dev);
 	if (err)

@@ -580,21 +591,22 @@ static int ipc_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	if (err)
 		return err;
 
-	init_completion(&ipcdev.cmd_complete);
+	init_completion(&scu->cmd_complete);
 
 	err = devm_request_irq(&dev->dev, dev->irq, ioc, 0, "intel_scu_ipc",
-			       &ipcdev);
+			       scu);
 	if (err)
 		return err;
 
-	ipcdev.ipc_base = pcim_iomap_table(dev)[0];
+	scu->ipc_base = pcim_iomap_table(dev)[0];
 
-	ipcdev.i2c_base = ioremap_nocache(pdata->i2c_base, pdata->i2c_len);
-	if (!ipcdev.i2c_base)
+	scu->i2c_base = ioremap_nocache(pdata->i2c_base, pdata->i2c_len);
+	if (!scu->i2c_base)
 		return -ENOMEM;
 
 	intel_scu_devices_create();
 
+	pci_set_drvdata(dev, scu);
 	return 0;
 }

@@ -610,9 +622,11 @@ static int ipc_probe(struct pci_dev *dev, const struct pci_device_id *id)
  */
 static void ipc_remove(struct pci_dev *pdev)
 {
-	pci_dev_put(ipcdev.pdev);
-	iounmap(ipcdev.i2c_base);
-	ipcdev.pdev = NULL;
+	struct intel_scu_ipc_dev *scu = pci_get_drvdata(pdev);
+
+	pci_dev_put(scu->pdev);
+	scu->pdev = NULL;
+	iounmap(scu->i2c_base);
 	intel_scu_devices_destroy();
 }

Signature · 1 line

2.5.3

Andy Shevchenko

author

2015-10-09 14:11 UTC #2 lore raw

PATCH 3/5 intel_scu_ipc: convert to use struct device *

In reply to Andy Shevchenko

Switch the code to use struct device * instead of struct pci_dev * since there is no reason to accee PCI related features in the driver.

Signed-off-by Andy Shevchenko <[email protected]>

---
 drivers/platform/x86/intel_scu_ipc.c | 41 ++++++++++++++++--------------------
 1 file changed, 18 insertions(+), 23 deletions(-)

drivers/platform/x86/intel_scu_ipc.c +18 ~~-23~~

index 6c9367f..69e39cb 100644
@@ -92,11 +92,8 @@ static struct intel_scu_ipc_pdata_t intel_scu_ipc_tangier_pdata = {
 	.irq_mode = 0,
 };
 
-static int ipc_probe(struct pci_dev *dev, const struct pci_device_id *id);
-static void ipc_remove(struct pci_dev *pdev);
-
 struct intel_scu_ipc_dev {
-	struct pci_dev *pdev;
+	struct device *dev;
 	void __iomem *ipc_base;
 	void __iomem *i2c_base;
 	struct completion cmd_complete;

@@ -181,7 +178,7 @@ static inline int busy_loop(struct intel_scu_ipc_dev *scu)
 	}
 
 	if (status & BIT(0)) {
-		dev_err(&scu->pdev->dev, "IPC timed out");
+		dev_err(scu->dev, "IPC timed out");
 		return -ETIMEDOUT;
 	}

@@ -197,8 +194,7 @@ static inline int ipc_wait_for_interrupt(struct intel_scu_ipc_dev *scu)
 	int status;
 
 	if (!wait_for_completion_timeout(&scu->cmd_complete, 3 * HZ)) {
-		struct device *dev = &scu->pdev->dev;
-		dev_err(dev, "IPC timed out\n");
+		dev_err(scu->dev, "IPC timed out\n");
 		return -ETIMEDOUT;
 	}

@@ -228,7 +224,7 @@ static int pwr_reg_rdwr(u16 *addr, u8 *data, u32 count, u32 op, u32 id)
 
 	mutex_lock(&ipclock);
 
-	if (scu->pdev == NULL) {
+	if (scu->dev == NULL) {
 		mutex_unlock(&ipclock);
 		return -ENODEV;
 	}

@@ -445,7 +441,7 @@ int intel_scu_ipc_simple_command(int cmd, int sub)
 	int err;
 
 	mutex_lock(&ipclock);
-	if (scu->pdev == NULL) {
+	if (scu->dev == NULL) {
 		mutex_unlock(&ipclock);
 		return -ENODEV;
 	}

@@ -475,7 +471,7 @@ int intel_scu_ipc_command(int cmd, int sub, u32 *in, int inlen,
 	int i, err;
 
 	mutex_lock(&ipclock);
-	if (scu->pdev == NULL) {
+	if (scu->dev == NULL) {
 		mutex_unlock(&ipclock);
 		return -ENODEV;
 	}

@@ -518,7 +514,7 @@ int intel_scu_ipc_i2c_cntrl(u32 addr, u32 *data)
 	u32 cmd = 0;
 
 	mutex_lock(&ipclock);
-	if (scu->pdev == NULL) {
+	if (scu->dev == NULL) {
 		mutex_unlock(&ipclock);
 		return -ENODEV;
 	}

@@ -533,7 +529,7 @@ int intel_scu_ipc_i2c_cntrl(u32 addr, u32 *data)
 		mdelay(1);
 		writel(addr, scu->i2c_base + IPC_I2C_CNTRL_ADDR);
 	} else {
-		dev_err(&scu->pdev->dev,
+		dev_err(scu->dev,
 			"intel_scu_ipc: I2C INVALID_CMD = 0x%x\n", cmd);
 
 		mutex_unlock(&ipclock);

@@ -563,42 +559,42 @@ static irqreturn_t ioc(int irq, void *dev_id)
 
 /**
  *	ipc_probe	-	probe an Intel SCU IPC
- *	@dev: the PCI device matching
+ *	@pdev: the PCI device matching
  *	@id: entry in the match table
  *
  *	Enable and install an intel SCU IPC. This appears in the PCI space
  *	but uses some hard coded addresses as well.
  */
-static int ipc_probe(struct pci_dev *dev, const struct pci_device_id *id)
+static int ipc_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 {
 	int err;
 	struct intel_scu_ipc_dev *scu = &ipcdev;
 	struct intel_scu_ipc_pdata_t *pdata;
 
-	if (scu->pdev)		/* We support only one SCU */
+	if (scu->dev)		/* We support only one SCU */
 		return -EBUSY;
 
 	pdata = (struct intel_scu_ipc_pdata_t *)id->driver_data;
 
-	scu->pdev = pci_dev_get(dev);
+	scu->dev = &pdev->dev;
 	scu->irq_mode = pdata->irq_mode;
 
-	err = pcim_enable_device(dev);
+	err = pcim_enable_device(pdev);
 	if (err)
 		return err;
 
-	err = pcim_iomap_regions(dev, 1 << 0, pci_name(dev));
+	err = pcim_iomap_regions(pdev, 1 << 0, pci_name(pdev));
 	if (err)
 		return err;
 
 	init_completion(&scu->cmd_complete);
 
-	err = devm_request_irq(&dev->dev, dev->irq, ioc, 0, "intel_scu_ipc",
+	err = devm_request_irq(&pdev->dev, dev->irq, ioc, 0, "intel_scu_ipc",
 			       scu);
 	if (err)
 		return err;
 
-	scu->ipc_base = pcim_iomap_table(dev)[0];
+	scu->ipc_base = pcim_iomap_table(pdev)[0];
 
 	scu->i2c_base = ioremap_nocache(pdata->i2c_base, pdata->i2c_len);
 	if (!scu->i2c_base)

@@ -606,7 +602,7 @@ static int ipc_probe(struct pci_dev *dev, const struct pci_device_id *id)
 
 	intel_scu_devices_create();
 
-	pci_set_drvdata(dev, scu);
+	pci_set_drvdata(pdev, scu);
 	return 0;
 }

@@ -624,8 +620,7 @@ static void ipc_remove(struct pci_dev *pdev)
 {
 	struct intel_scu_ipc_dev *scu = pci_get_drvdata(pdev);
 
-	pci_dev_put(scu->pdev);
-	scu->pdev = NULL;
+	scu->dev = NULL;
 	iounmap(scu->i2c_base);
 	intel_scu_devices_destroy();
 }

Signature · 1 line

2.5.3

Andy Shevchenko

author

2015-10-09 14:11 UTC #3 lore raw

PATCH 4/5 intel_scu_ipc: switch to use module_pci_driver() macro

In reply to Andy Shevchenko

The use of macro cleans up the code a bit.

Signed-off-by Andy Shevchenko <[email protected]>

---
 drivers/platform/x86/intel_scu_ipc.c | 25 +++++++------------------
 1 file changed, 7 insertions(+), 18 deletions(-)

drivers/platform/x86/intel_scu_ipc.c +7 ~~-18~~

index 69e39cb..9de2029 100644
@@ -567,10 +567,15 @@ static irqreturn_t ioc(int irq, void *dev_id)
  */
 static int ipc_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 {
+	int platform;		/* Platform type */
 	int err;
 	struct intel_scu_ipc_dev *scu = &ipcdev;
 	struct intel_scu_ipc_pdata_t *pdata;
 
+	platform = intel_mid_identify_cpu();
+	if (platform == 0)
+		return -ENODEV;
+
 	if (scu->dev)		/* We support only one SCU */
 		return -EBUSY;

@@ -589,7 +594,7 @@ static int ipc_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 
 	init_completion(&scu->cmd_complete);
 
-	err = devm_request_irq(&pdev->dev, dev->irq, ioc, 0, "intel_scu_ipc",
+	err = devm_request_irq(&pdev->dev, pdev->irq, ioc, 0, "intel_scu_ipc",
 			       scu);
 	if (err)
 		return err;

@@ -651,24 +656,8 @@ static struct pci_driver ipc_driver = {
 	.remove = ipc_remove,
 };
 
-static int __init intel_scu_ipc_init(void)
-{
-	int platform;		/* Platform type */
-
-	platform = intel_mid_identify_cpu();
-	if (platform == 0)
-		return -ENODEV;
-	return  pci_register_driver(&ipc_driver);
-}
-
-static void __exit intel_scu_ipc_exit(void)
-{
-	pci_unregister_driver(&ipc_driver);
-}
+module_pci_driver(ipc_driver);
 
 MODULE_AUTHOR("Sreedhara DS <[email protected]>");
 MODULE_DESCRIPTION("Intel SCU IPC driver");
 MODULE_LICENSE("GPL");
-
-module_init(intel_scu_ipc_init);
-module_exit(intel_scu_ipc_exit);

Signature · 1 line

2.5.3

Darren Hart

2015-10-11 04:39 UTC #4 lore raw

Re: [PATCH 4/5] intel_scu_ipc: switch to use module_pci_driver() macro

Quoted 1 line · Andy Shevchenko

The use of macro cleans up the code a bit.

Thanks Andriy,

Minor point of feedback: it helps review to state specifics of how something is cleaned up, etc. That way I am not assuming anything and can verify your intentions with what I'm reading. This is a trivial example, but illustrative.

For this patch, you might have said:

Eliminate some boilerplate code by using module_pci_driver() instead of init/exit, moving the salient bits from init into probe.

This provides more context for the reviewer and makes the intent explicit from the commit message.

Quoted 65 lines

Signed-off-by Andy Shevchenko <[email protected]>

---
 drivers/platform/x86/intel_scu_ipc.c | 25 +++++++------------------
 1 file changed, 7 insertions(+), 18 deletions(-)

drivers/platform/x86/intel_scu_ipc.c +7 ~~-18~~

index 69e39cb..9de2029 100644
@@ -567,10 +567,15 @@ static irqreturn_t ioc(int irq, void *dev_id)
  */
 static int ipc_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 {
+	int platform;		/* Platform type */
 	int err;
 	struct intel_scu_ipc_dev *scu = &ipcdev;
 	struct intel_scu_ipc_pdata_t *pdata;
 
+	platform = intel_mid_identify_cpu();
+	if (platform == 0)
+		return -ENODEV;
+
 	if (scu->dev)		/* We support only one SCU */
 		return -EBUSY;

@@ -589,7 +594,7 @@ static int ipc_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 
 	init_completion(&scu->cmd_complete);
 
-	err = devm_request_irq(&pdev->dev, dev->irq, ioc, 0, "intel_scu_ipc",
+	err = devm_request_irq(&pdev->dev, pdev->irq, ioc, 0, "intel_scu_ipc",
 			       scu);
 	if (err)
 		return err;

@@ -651,24 +656,8 @@ static struct pci_driver ipc_driver = {
 	.remove = ipc_remove,
 };
 
-static int __init intel_scu_ipc_init(void)
-{
-	int platform;		/* Platform type */
-
-	platform = intel_mid_identify_cpu();
-	if (platform == 0)
-		return -ENODEV;
-	return  pci_register_driver(&ipc_driver);
-}
-
-static void __exit intel_scu_ipc_exit(void)
-{
-	pci_unregister_driver(&ipc_driver);
-}
+module_pci_driver(ipc_driver);
 
 MODULE_AUTHOR("Sreedhara DS <[email protected]>");
 MODULE_DESCRIPTION("Intel SCU IPC driver");
 MODULE_LICENSE("GPL");
-
-module_init(intel_scu_ipc_init);
-module_exit(intel_scu_ipc_exit);

Signature · 1 line

2.5.3

Signature · 2 lines

Darren Hart
Intel Open Source Technology Center

Andy Shevchenko

author

2015-10-09 14:11 UTC #5 lore raw

PATCH 5/5 intel_scu_ipc: move ipclock to struct intel_scu_ipc_dev

In reply to Andy Shevchenko

No need to use global variable for a mutex. Move it to be a member of SCU IPC device structure.

While here, protect dev member assignment in ->remove() since user may potentially call unbind from sysfs even if driver is built-in.

Signed-off-by Andy Shevchenko <[email protected]>

---
 drivers/platform/x86/intel_scu_ipc.c | 34 +++++++++++++++++++---------------
 1 file changed, 19 insertions(+), 15 deletions(-)

drivers/platform/x86/intel_scu_ipc.c +19 ~~-15~~

index 9de2029..16f1621 100644
@@ -92,11 +92,13 @@ static struct intel_scu_ipc_pdata_t intel_scu_ipc_tangier_pdata = {
 	.irq_mode = 0,
 };
 
+/* @lock: lock used to prevent multiple call to SCU */
 struct intel_scu_ipc_dev {
 	struct device *dev;
 	void __iomem *ipc_base;
 	void __iomem *i2c_base;
 	struct completion cmd_complete;
+	struct mutex lock;
 	u8 irq_mode;
 };

@@ -112,8 +114,6 @@ static struct intel_scu_ipc_dev  ipcdev; /* Only one for now */
 #define IPC_I2C_CNTRL_ADDR	0
 #define I2C_DATA_ADDR		0x04
 
-static DEFINE_MUTEX(ipclock); /* lock used to prevent multiple call to SCU */
-
 /*
  * Send ipc command
  * Command Register (Write Only):

@@ -222,10 +222,10 @@ static int pwr_reg_rdwr(u16 *addr, u8 *data, u32 count, u32 op, u32 id)
 
 	memset(cbuf, 0, sizeof(cbuf));
 
-	mutex_lock(&ipclock);
+	mutex_lock(&scu->lock);
 
 	if (scu->dev == NULL) {
-		mutex_unlock(&ipclock);
+		mutex_unlock(&scu->lock);
 		return -ENODEV;
 	}

@@ -258,7 +258,7 @@ static int pwr_reg_rdwr(u16 *addr, u8 *data, u32 count, u32 op, u32 id)
 		for (nc = 0; nc < count; nc++)
 			data[nc] = ipc_data_readb(scu, nc);
 	}
-	mutex_unlock(&ipclock);
+	mutex_unlock(&scu->lock);
 	return err;
 }

@@ -440,14 +440,14 @@ int intel_scu_ipc_simple_command(int cmd, int sub)
 	struct intel_scu_ipc_dev *scu = &ipcdev;
 	int err;
 
-	mutex_lock(&ipclock);
+	mutex_lock(&scu->lock);
 	if (scu->dev == NULL) {
-		mutex_unlock(&ipclock);
+		mutex_unlock(&scu->lock);
 		return -ENODEV;
 	}
 	ipc_command(scu, sub << 12 | cmd);
 	err = intel_scu_ipc_check_status(scu);
-	mutex_unlock(&ipclock);
+	mutex_unlock(&scu->lock);
 	return err;
 }
 EXPORT_SYMBOL(intel_scu_ipc_simple_command);

@@ -470,9 +470,9 @@ int intel_scu_ipc_command(int cmd, int sub, u32 *in, int inlen,
 	struct intel_scu_ipc_dev *scu = &ipcdev;
 	int i, err;
 
-	mutex_lock(&ipclock);
+	mutex_lock(&scu->lock);
 	if (scu->dev == NULL) {
-		mutex_unlock(&ipclock);
+		mutex_unlock(&scu->lock);
 		return -ENODEV;
 	}

@@ -487,7 +487,7 @@ int intel_scu_ipc_command(int cmd, int sub, u32 *in, int inlen,
 			*out++ = ipc_data_readl(scu, 4 * i);
 	}
 
-	mutex_unlock(&ipclock);
+	mutex_unlock(&scu->lock);
 	return err;
 }
 EXPORT_SYMBOL(intel_scu_ipc_command);

@@ -513,9 +513,9 @@ int intel_scu_ipc_i2c_cntrl(u32 addr, u32 *data)
 	struct intel_scu_ipc_dev *scu = &ipcdev;
 	u32 cmd = 0;
 
-	mutex_lock(&ipclock);
+	mutex_lock(&scu->lock);
 	if (scu->dev == NULL) {
-		mutex_unlock(&ipclock);
+		mutex_unlock(&scu->lock);
 		return -ENODEV;
 	}
 	cmd = (addr >> 24) & 0xFF;

@@ -532,10 +532,10 @@ int intel_scu_ipc_i2c_cntrl(u32 addr, u32 *data)
 		dev_err(scu->dev,
 			"intel_scu_ipc: I2C INVALID_CMD = 0x%x\n", cmd);
 
-		mutex_unlock(&ipclock);
+		mutex_unlock(&scu->lock);
 		return -EIO;
 	}
-	mutex_unlock(&ipclock);
+	mutex_unlock(&scu->lock);
 	return 0;
 }
 EXPORT_SYMBOL(intel_scu_ipc_i2c_cntrl);

@@ -593,6 +593,7 @@ static int ipc_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 		return err;
 
 	init_completion(&scu->cmd_complete);
+	mutex_init(&scu->lock);
 
 	err = devm_request_irq(&pdev->dev, pdev->irq, ioc, 0, "intel_scu_ipc",
 			       scu);

@@ -625,7 +626,10 @@ static void ipc_remove(struct pci_dev *pdev)
 {
 	struct intel_scu_ipc_dev *scu = pci_get_drvdata(pdev);
 
+	mutex_lock(&scu->lock);
 	scu->dev = NULL;
+	mutex_unlock(&scu->lock);
+
 	iounmap(scu->i2c_base);
 	intel_scu_devices_destroy();
 }

Signature · 1 line

2.5.3

Andy Shevchenko

author

2015-10-09 15:21 UTC #6 lore raw

Re: [PATCH 5/5] intel_scu_ipc: move ipclock to struct intel_scu_ipc_dev

Quoted 3 lines · Andy Shevchenko

No need to use global variable for a mutex. Move it to be a member of SCU IPC
device structure.

This one is wrong, sorry.

Quoted 4 lines

While here, protect dev member assignment in ->remove() since user may
potentially call unbind from sysfs even if driver is built-in.

This part is still right.

Quoted 152 lines

Signed-off-by Andy Shevchenko <[email protected]>

---
 drivers/platform/x86/intel_scu_ipc.c | 34 +++++++++++++++++++-------
--------
 1 file changed, 19 insertions(+), 15 deletions(-)

drivers/platform/x86/intel_scu_ipc.c +2 -0

index 9de2029..16f1621 100644
@@ -92,11 +92,13 @@ static struct intel_scu_ipc_pdata_t 
 intel_scu_ipc_tangier_pdata = {
 	.irq_mode = 0,
 };
 
+/* @lock: lock used to prevent multiple call to SCU */
 struct intel_scu_ipc_dev {
 	struct device *dev;
 	void __iomem *ipc_base;
 	void __iomem *i2c_base;
 	struct completion cmd_complete;
+	struct mutex lock;
 	u8 irq_mode;
 };

@@ -112,8 +114,6 @@ static struct intel_scu_ipc_dev  ipcdev; /* Only

one for now */
#define IPC_I2C_CNTRL_ADDR 0
#define I2C_DATA_ADDR 0x04

-static DEFINE_MUTEX(ipclock); /* lock used to prevent multiple call to SCU */

drivers/platform/x86/intel_scu_ipc.c +6 -7

-
 /*
  * Send ipc command
  * Command Register (Write Only):

@@ -222,10 +222,10 @@ static int pwr_reg_rdwr(u16 *addr, u8 *data, 
 u32 count, u32 op, u32 id)
 
 	memset(cbuf, 0, sizeof(cbuf));
 
-	mutex_lock(&ipclock);
+	mutex_lock(&scu->lock);
 
 	if (scu->dev == NULL) {
-		mutex_unlock(&ipclock);
+		mutex_unlock(&scu->lock);
 		return -ENODEV;
 	}

@@ -258,7 +258,7 @@ static int pwr_reg_rdwr(u16 *addr, u8 *data, u32 
 count, u32 op, u32 id)
 		for (nc = 0; nc < count; nc++)
 			data[nc] = ipc_data_readb(scu, nc);
 	}
-	mutex_unlock(&ipclock);
+	mutex_unlock(&scu->lock);
 	return err;
 }

@@ -440,14 +440,14 @@ int intel_scu_ipc_simple_command(int cmd, int 
 sub)
 	struct intel_scu_ipc_dev *scu = &ipcdev;
 	int err;
 
-	mutex_lock(&ipclock);
+	mutex_lock(&scu->lock);
 	if (scu->dev == NULL) {
-		mutex_unlock(&ipclock);
+		mutex_unlock(&scu->lock);
 		return -ENODEV;
 	}
 	ipc_command(scu, sub << 12 | cmd);
 	err = intel_scu_ipc_check_status(scu);
-	mutex_unlock(&ipclock);
+	mutex_unlock(&scu->lock);
 	return err;
 }

EXPORT_SYMBOL(intel_scu_ipc_simple_command);

drivers/platform/x86/intel_scu_ipc.c +3 -3

@@ -470,9 +470,9 @@ int intel_scu_ipc_command(int cmd, int sub, u32 
 *in, int inlen,
 	struct intel_scu_ipc_dev *scu = &ipcdev;
 	int i, err;
 
-	mutex_lock(&ipclock);
+	mutex_lock(&scu->lock);
 	if (scu->dev == NULL) {
-		mutex_unlock(&ipclock);
+		mutex_unlock(&scu->lock);
 		return -ENODEV;
 	}

@@ -487,7 +487,7 @@ int intel_scu_ipc_command(int cmd, int sub, u32 
 *in, int inlen,
 			*out++ = ipc_data_readl(scu, 4 * i);
 	}
 
-	mutex_unlock(&ipclock);
+	mutex_unlock(&scu->lock);
 	return err;
 }

EXPORT_SYMBOL(intel_scu_ipc_command);

drivers/platform/x86/intel_scu_ipc.c +4 -4

@@ -513,9 +513,9 @@ int intel_scu_ipc_i2c_cntrl(u32 addr, u32 *data)
 	struct intel_scu_ipc_dev *scu = &ipcdev;
 	u32 cmd = 0;
 
-	mutex_lock(&ipclock);
+	mutex_lock(&scu->lock);
 	if (scu->dev == NULL) {
-		mutex_unlock(&ipclock);
+		mutex_unlock(&scu->lock);
 		return -ENODEV;
 	}
 	cmd = (addr >> 24) & 0xFF;

@@ -532,10 +532,10 @@ int intel_scu_ipc_i2c_cntrl(u32 addr, u32 
 *data)
 		dev_err(scu->dev,
 			"intel_scu_ipc: I2C INVALID_CMD = 0x%x\n", 
 cmd);
 
-		mutex_unlock(&ipclock);
+		mutex_unlock(&scu->lock);
 		return -EIO;
 	}
-	mutex_unlock(&ipclock);
+	mutex_unlock(&scu->lock);
 	return 0;

}
EXPORT_SYMBOL(intel_scu_ipc_i2c_cntrl);

drivers/platform/x86/intel_scu_ipc.c +1 -0

@@ -593,6 +593,7 @@ static int ipc_probe(struct pci_dev *pdev, const 
 struct pci_device_id *id)
 		return err;
 
 	init_completion(&scu->cmd_complete);
+	mutex_init(&scu->lock);
 
 	err = devm_request_irq(&pdev->dev, pdev->irq, ioc, 0,

"intel_scu_ipc",

drivers/platform/x86/intel_scu_ipc.c +3 -0

 			       scu);

@@ -625,7 +626,10 @@ static void ipc_remove(struct pci_dev *pdev)
 {
 	struct intel_scu_ipc_dev *scu = pci_get_drvdata(pdev);
 
+	mutex_lock(&scu->lock);
 	scu->dev = NULL;
+	mutex_unlock(&scu->lock);
+
 	iounmap(scu->i2c_base);
 	intel_scu_devices_destroy();
 }

Signature · 2 lines

Andy Shevchenko <[email protected]>
Intel Finland Oy

Darren Hart

2015-10-11 04:43 UTC #7 lore raw

Re: [PATCH 5/5] intel_scu_ipc: move ipclock to struct intel_scu_ipc_dev

Quoted 5 lines · Andy Shevchenko

Quoted 3 lines · Andy Shevchenko
No need to use global variable for a mutex. Move it to be a member of SCU IPC
device structure.

Quoted 8 lines

This one is wrong, sorry.
Quoted 4 lines
While here, protect dev member assignment in ->remove() since user may
potentially call unbind from sysfs even if driver is built-in.
This part is still right.

OK.

With my minor comment to 1/5 and this change, care to send a v2?

Signature · 2 lines

Darren Hart
Intel Open Source Technology Center

Andy Shevchenko

author

2015-10-12 08:02 UTC #8 lore raw

Re: [PATCH 5/5] intel_scu_ipc: move ipclock to struct intel_scu_ipc_dev

Quoted 15 lines · Darren Hart

Quoted 14 lines · Andy Shevchenko
Quoted 4 lines · Andy Shevchenko
No need to use global variable for a mutex. Move it to be a member of
SCU IPC
device structure.
This one is wrong, sorry.
Quoted 5 lines
While here, protect dev member assignment in ->remove() since user
may
potentially call unbind from sysfs even if driver is built-in.

Quoted 5 lines

Quoted 1 line
This part is still right.
OK.
With my minor comment to 1/5 and this change, care to send a v2?

Will do later this week.
Also I will update message of the patch 4 as you suggested.

Signature · 2 lines

Andy Shevchenko <[email protected]>
Intel Finland Oy

Darren Hart

2015-10-11 04:21 UTC #9 lore raw

In reply to Andy Shevchenko

Quoted 4 lines · Andy Shevchenko

The error handling is broken right now since it leaves resources unfreed. Convert the code to use managed resources to fix the error handling.
Signed-off-by Andy Shevchenko <[email protected]>

Thanks Andriy,

Please always explicitly Cc LKML.

Is this hardware you able to test explicitly?

Quoted 8 lines

---
 drivers/platform/x86/intel_scu_ipc.c | 26 ++++++++------------------
 1 file changed, 8 insertions(+), 18 deletions(-)

drivers/platform/x86/intel_scu_ipc.c +0 -0

index 187d108..7148535 100644

...

Quoted 4 lines

drivers/platform/x86/intel_scu_ipc.c +1 -1

-	err = pci_request_regions(dev, "intel_scu_ipc");
+	err = pcim_iomap_regions(dev, 1 << 0, pci_name(dev));
 	if (err)
 		return err;

drivers/platform/x86/intel_scu_ipc.c static int ipc_probe(struct pci_dev *dev, const struct pci_device_id *id) · line 585

Quoted 10 lines

drivers/platform/x86/intel_scu_ipc.c +1 -6

 
-	base = pci_resource_start(dev, 0);
-	if (!base)
-		return -ENOMEM;
-
 	init_completion(&ipcdev.cmd_complete);
 
-	if (request_irq(dev->irq, ioc, 0, "intel_scu_ipc", &ipcdev))
-		return -EBUSY;
+	err = devm_request_irq(&dev->dev, dev->irq, ioc, 0, "intel_scu_ipc",

You switched to using pci_name(dev) above, seems to me the same rationale should apply here. Any reason not to use pci_name(dev) here instead of "intel_scu_ipc"?

Thanks,

Signature · 2 lines

Darren Hart
Intel Open Source Technology Center

Andy Shevchenko

author

2015-10-12 08:00 UTC #10 lore raw

Quoted 6 lines · Darren Hart

Quoted 5 lines · Andy Shevchenko
The error handling is broken right now since it leaves resources unfreed.
Convert the code to use managed resources to fix the error handling.

Quoted 7 lines

Quoted 1 line
Signed-off-by Andy Shevchenko <[email protected]>
Thanks Andriy,
Please always explicitly Cc LKML.
Is this hardware you able to test explicitly?

Yes, the patch series has been tested on Intel Medfield and Intel Edison (Merrifield) boards.

Quoted 38 lines

Quoted 11 lines
---
 drivers/platform/x86/intel_scu_ipc.c | 26 ++++++++----------------
--
 1 file changed, 8 insertions(+), 18 deletions(-)
drivers/platform/x86/intel_scu_ipc.c +0 -0
index 187d108..7148535 100644
...
Quoted 15 lines
drivers/platform/x86/intel_scu_ipc.c +1 -6
-	err = pci_request_regions(dev, "intel_scu_ipc");
+	err = pcim_iomap_regions(dev, 1 << 0, pci_name(dev));
 	if (err)
 		return err;
 
-	base = pci_resource_start(dev, 0);
-	if (!base)
-		return -ENOMEM;
-
 	init_completion(&ipcdev.cmd_complete);
 
-	if (request_irq(dev->irq, ioc, 0, "intel_scu_ipc", 
&ipcdev))
drivers/platform/x86/intel_scu_ipc.c +1 -1
-		return -EBUSY;
+	err = devm_request_irq(&dev->dev, dev->irq, ioc, 0, 
Quoted 1 line
"intel_scu_ipc",
You switched to using pci_name(dev) above, seems to me the same rationale should
apply here. Any reason not to use pci_name(dev) here instead of "intel_scu_ipc"?
Thanks,

Signature · 2 lines

Andy Shevchenko <[email protected]>
Intel Finland Oy