[PATCH 0/8] fix errors in the match process

[PATCH 0/8] fix errors in the match process

[Samuel Iglesias Gonsalvez]

Hello,

As Dan Carpenter suggested, I split the previous patches [0] in several ones,
facilitating the reading, understanding and, at the end, the review and
maintenance.

These patches fix some problems found in the match() process for the ipack
devices, among minor things.

Thanks,

Sam

[0] Link to the patches:

https://lkml.org/lkml/2012/5/23/99
https://lkml.org/lkml/2012/5/23/102
https://lkml.org/lkml/2012/5/23/96

Samuel Iglesias Gonsalvez (8):
  Staging ipack/bridges/tpci200: delete sysfs files
  Staging: ipack: return proper value in match() function
  Staging: ipack/bridges/tpci200: tpci200_slot_unmap_space() should
    return 0 if succeed.
  Staging: ipack/bridges/tpci200: tpci200_slot_map_space() should
    return 0 if succeed.
  Staging: ipack/bridges/tpci200: check if the remove function is
    available
  Staging: ipack/bridges/tpci200: fix indention.
  Staging: ipack/bridges/tpci200: remove name field from slot_irq
  Staging ipack/bridges/tpci200: removed check of
    tpci200->slots[dev->slot].dev

 drivers/staging/ipack/bridges/tpci200.c |  260 +++----------------------------
 drivers/staging/ipack/bridges/tpci200.h |    2 -
 drivers/staging/ipack/ipack.c           |    2 +-
 3 files changed, 24 insertions(+), 240 deletions(-)

-- 
1.7.10

[PATCH 1/8] Staging ipack/bridges/tpci200: delete sysfs files

[Samuel Iglesias Gonsalvez]

To perform the installation of a mezzanine it was needed to write on these
files, now it is not needed at all as the device model is properly implemented.

Signed-off-by: Samuel Iglesias Gonsalvez <siglesias@igalia.com>
---
 drivers/staging/ipack/bridges/tpci200.c |  225 ++-----------------------------
 1 file changed, 12 insertions(+), 213 deletions(-)

diff --git a/drivers/staging/ipack/bridges/tpci200.c b/drivers/staging/ipack/bridges/tpci200.c
index ad28707..75ed600 100644
--- a/drivers/staging/ipack/bridges/tpci200.c
+++ b/drivers/staging/ipack/bridges/tpci200.c
@@ -365,207 +365,9 @@ static irqreturn_t tpci200_interrupt(int irq, void *dev_id)
 	return ret;
 }
 
-#ifdef CONFIG_SYSFS
-
-static struct ipack_device *tpci200_slot_register(unsigned int tpci200_number,
-						  unsigned int slot_position)
-{
-	int found = 0;
-	struct ipack_device  *dev;
-	struct tpci200_board *tpci200;
-
-	list_for_each_entry(tpci200, &tpci200_list, list) {
-		if (tpci200->number == tpci200_number) {
-			found = 1;
-			break;
-		}
-	}
-
-	if (!found) {
-		pr_err("carrier board not found for the device\n");
-		return NULL;
-	}
-
-	if (slot_position >= TPCI200_NB_SLOT) {
-		pr_info("Slot [%d:%d] doesn't exist!\n", tpci200_number,
-			slot_position);
-		return NULL;
-	}
-
-	if (mutex_lock_interruptible(&tpci200->mutex))
-		return NULL;
-
-	if (tpci200->slots[slot_position].dev != NULL) {
-		pr_err("Slot [%d:%d] already installed !\n", tpci200_number,
-		       slot_position);
-		goto err_unlock;
-	}
-
-	/*
-	 * Give the same IRQ number as the slot number.
-	 * The TPCI200 has assigned his own two IRQ by PCI bus driver
-	 */
-	dev = ipack_device_register(tpci200->info->ipack_bus,
-				    slot_position, slot_position);
-	if (dev == NULL) {
-		pr_info("Slot [%d:%d] Unable to register an ipack device\n",
-			tpci200_number, slot_position);
-		goto err_unlock;
-	}
-
-	tpci200->slots[slot_position].dev = dev;
-	mutex_unlock(&tpci200->mutex);
-	return dev;
-
-err_unlock:
-	mutex_unlock(&tpci200->mutex);
-	return NULL;
-}
-
-static ssize_t tpci200_store_board(struct device *pdev, const char *buf,
-				   size_t count, int slot)
-{
-	struct tpci200_board *card = dev_get_drvdata(pdev);
-	struct ipack_device *dev = card->slots[slot].dev;
-
-	if (dev != NULL)
-		return -EBUSY;
-
-	dev = tpci200_slot_register(card->number, slot);
-	if (dev == NULL)
-		return -ENODEV;
-
-	return count;
-}
-
-static ssize_t tpci200_show_board(struct device *pdev, char *buf, int slot)
-{
-	struct tpci200_board *card = dev_get_drvdata(pdev);
-	struct ipack_device *dev = card->slots[slot].dev;
-
-	if (dev != NULL)
-		return snprintf(buf, PAGE_SIZE, "%s\n", dev_name(&dev->dev));
-	else
-		return snprintf(buf, PAGE_SIZE, "none\n");
-}
-
-static ssize_t tpci200_show_description(struct device *pdev,
-					struct device_attribute *attr,
-					char *buf)
-{
-	return snprintf(buf, PAGE_SIZE,
-			"TEWS tpci200 carrier PCI for Industry-pack mezzanines.\n");
-}
-
-static ssize_t tpci200_show_board_slot0(struct device *pdev,
-					struct device_attribute *attr,
-					char *buf)
-{
-	return tpci200_show_board(pdev, buf, 0);
-}
-
-static ssize_t tpci200_store_board_slot0(struct device *pdev,
-					 struct device_attribute *attr,
-					 const char *buf, size_t count)
-{
-	return tpci200_store_board(pdev, buf, count, 0);
-}
-
-static ssize_t tpci200_show_board_slot1(struct device *pdev,
-					struct device_attribute *attr,
-					char *buf)
-{
-	return tpci200_show_board(pdev, buf, 1);
-}
-
-static ssize_t tpci200_store_board_slot1(struct device *pdev,
-					 struct device_attribute *attr,
-					 const char *buf, size_t count)
-{
-	return tpci200_store_board(pdev, buf, count, 1);
-}
-
-static ssize_t tpci200_show_board_slot2(struct device *pdev,
-					struct device_attribute *attr,
-					char *buf)
-{
-	return tpci200_show_board(pdev, buf, 2);
-}
-
-static ssize_t tpci200_store_board_slot2(struct device *pdev,
-					 struct device_attribute *attr,
-					 const char *buf, size_t count)
-{
-	return tpci200_store_board(pdev, buf, count, 2);
-}
-
-
-static ssize_t tpci200_show_board_slot3(struct device *pdev,
-					struct device_attribute *attr,
-					char *buf)
-{
-	return tpci200_show_board(pdev, buf, 3);
-}
-
-static ssize_t tpci200_store_board_slot3(struct device *pdev,
-					 struct device_attribute *attr,
-					 const char *buf, size_t count)
-{
-	return tpci200_store_board(pdev, buf, count, 3);
-}
-
-/* Declaration of the device attributes for the TPCI200 */
-static DEVICE_ATTR(description, S_IRUGO,
-		   tpci200_show_description, NULL);
-static DEVICE_ATTR(board_slot0, S_IRUGO | S_IWUSR,
-		   tpci200_show_board_slot0, tpci200_store_board_slot0);
-static DEVICE_ATTR(board_slot1, S_IRUGO | S_IWUSR,
-		   tpci200_show_board_slot1, tpci200_store_board_slot1);
-static DEVICE_ATTR(board_slot2, S_IRUGO | S_IWUSR,
-		   tpci200_show_board_slot2, tpci200_store_board_slot2);
-static DEVICE_ATTR(board_slot3, S_IRUGO | S_IWUSR,
-		   tpci200_show_board_slot3, tpci200_store_board_slot3);
-
-static struct attribute *tpci200_attrs[] = {
-	&dev_attr_description.attr,
-	&dev_attr_board_slot0.attr,
-	&dev_attr_board_slot1.attr,
-	&dev_attr_board_slot2.attr,
-	&dev_attr_board_slot3.attr,
-	NULL,
-};
-
-static struct attribute_group tpci200_attr_group = {
-	.attrs = tpci200_attrs,
-};
-
-static int tpci200_create_sysfs_files(struct tpci200_board *card)
-{
-	return sysfs_create_group(&card->info->pdev->dev.kobj,
-				  &tpci200_attr_group);
-}
-
-static void tpci200_remove_sysfs_files(struct tpci200_board *card)
-{
-	sysfs_remove_group(&card->info->pdev->dev.kobj, &tpci200_attr_group);
-}
-
-#else
-
-static int tpci200_create_sysfs_files(struct tpci200_board *card)
-{
-	return 0;
-}
-
-static void tpci200_remove_sysfs_files(struct tpci200_board *card)
-{
-}
-
-#endif /* CONFIG_SYSFS */
-
 static int tpci200_register(struct tpci200_board *tpci200)
 {
-	int  i;
+	int i;
 	int res;
 	unsigned long ioidint_base;
 	unsigned long mem_base;
@@ -574,12 +376,6 @@ static int tpci200_register(struct tpci200_board *tpci200)
 	if (pci_enable_device(tpci200->info->pdev) < 0)
 		return -ENODEV;
 
-	if (tpci200_create_sysfs_files(tpci200) < 0) {
-		pr_err("failed creating sysfs files\n");
-		res = -EFAULT;
-		goto out_disable_pci;
-	}
-
 	/* Request IP interface register (Bar 2) */
 	res = pci_request_region(tpci200->info->pdev, TPCI200_IP_INTERFACE_BAR,
 				 "Carrier IP interface registers");
@@ -587,7 +383,7 @@ static int tpci200_register(struct tpci200_board *tpci200)
 		pr_err("(bn 0x%X, sn 0x%X) failed to allocate PCI resource for BAR 2 !",
 		       tpci200->info->pdev->bus->number,
 		       tpci200->info->pdev->devfn);
-		goto out_remove_sysfs;
+		goto out_disable_pci;
 	}
 
 	/* Request IO ID INT space (Bar 3) */
@@ -668,8 +464,7 @@ static int tpci200_register(struct tpci200_board *tpci200)
 		pr_err("(bn 0x%X, sn 0x%X) unable to register IRQ !",
 		       tpci200->info->pdev->bus->number,
 		       tpci200->info->pdev->devfn);
-		tpci200_unregister(tpci200);
-		goto out_err;
+		goto out_release_ioid_int_space;
 	}
 
 	return 0;
@@ -678,11 +473,8 @@ out_release_ioid_int_space:
 	pci_release_region(tpci200->info->pdev, TPCI200_IO_ID_INT_SPACES_BAR);
 out_release_ip_space:
 	pci_release_region(tpci200->info->pdev, TPCI200_IP_INTERFACE_BAR);
-out_remove_sysfs:
-	tpci200_remove_sysfs_files(tpci200);
 out_disable_pci:
 	pci_disable_device(tpci200->info->pdev);
-out_err:
 	return res;
 }
 
@@ -1038,7 +830,7 @@ out_err:
 static int tpci200_pciprobe(struct pci_dev *pdev,
 			    const struct pci_device_id *id)
 {
-	int ret;
+	int ret, i;
 	struct tpci200_board *tpci200;
 
 	tpci200 = kzalloc(sizeof(struct tpci200_board), GFP_KERNEL);
@@ -1081,13 +873,20 @@ static int tpci200_pciprobe(struct pci_dev *pdev,
 	dev_set_drvdata(&pdev->dev, tpci200);
 	/* add the registered device in an internal linked list */
 	list_add_tail(&tpci200->list, &tpci200_list);
+
+	/*
+	 * Give the same IRQ number as the slot number.
+	 * The TPCI200 has assigned his own two IRQ by PCI bus driver
+	 */
+	for (i = 0; i < TPCI200_NB_SLOT; i++)
+		tpci200->slots[i].dev =
+			ipack_device_register(tpci200->info->ipack_bus, i, i);
 	return ret;
 }
 
 static void __tpci200_pci_remove(struct tpci200_board *tpci200)
 {
 	tpci200_uninstall(tpci200);
-	tpci200_remove_sysfs_files(tpci200);
 	list_del(&tpci200->list);
 	ipack_bus_unregister(tpci200->info->ipack_bus);
 	kfree(tpci200->info);
-- 
1.7.10

[PATCH 2/8] Staging: ipack: return proper value in match() function

[Samuel Iglesias Gonsalvez]

It should return the same value given by the match function of the ipack_driver
that has been called.

Returning 0 here, means that the match has failed and it could be succeed.

Signed-off-by: Samuel Iglesias Gonsalvez <siglesias@igalia.com>
---
 drivers/staging/ipack/ipack.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/ipack/ipack.c b/drivers/staging/ipack/ipack.c
index 2b4fa51..e6dc21a 100644
--- a/drivers/staging/ipack/ipack.c
+++ b/drivers/staging/ipack/ipack.c
@@ -48,7 +48,7 @@ static int ipack_bus_match(struct device *device, struct device_driver *driver)
 	if (ret)
 		dev->driver = drv;
 
-	return 0;
+	return ret;
 }
 
 static int ipack_bus_probe(struct device *device)
-- 
1.7.10

[PATCH 3/8] Staging: ipack/bridges/tpci200: tpci200_slot_unmap_space() should return 0 if succeed.

[Samuel Iglesias Gonsalvez]

tpci200_slot_unmap_space() should return 0 if the operation was properly done. If
not, the caller will think that something wrong happened.

Signed-off-by: Samuel Iglesias Gonsalvez <siglesias@igalia.com>
---
 drivers/staging/ipack/bridges/tpci200.c |   21 +++++++--------------
 1 file changed, 7 insertions(+), 14 deletions(-)

diff --git a/drivers/staging/ipack/bridges/tpci200.c b/drivers/staging/ipack/bridges/tpci200.c
index 75ed600..85560c7 100644
--- a/drivers/staging/ipack/bridges/tpci200.c
+++ b/drivers/staging/ipack/bridges/tpci200.c
@@ -551,20 +551,15 @@ out:
 
 static int tpci200_slot_unmap_space(struct ipack_device *dev, int space)
 {
-	int res;
 	struct ipack_addr_space *virt_addr_space;
 	struct tpci200_board *tpci200;
 
 	tpci200 = check_slot(dev);
-	if (tpci200 == NULL) {
-		res = -EINVAL;
-		goto out;
-	}
+	if (tpci200 == NULL)
+		return -EINVAL;
 
-	if (mutex_lock_interruptible(&tpci200->mutex)) {
-		res = -ERESTARTSYS;
-		goto out;
-	}
+	if (mutex_lock_interruptible(&tpci200->mutex))
+		return -ERESTARTSYS;
 
 	switch (space) {
 	case IPACK_IO_SPACE:
@@ -594,9 +589,8 @@ static int tpci200_slot_unmap_space(struct ipack_device *dev, int space)
 	default:
 		pr_err("Slot [%d:%d] space number %d doesn't exist !\n",
 		       dev->bus_nr, dev->slot, space);
-		res = -EINVAL;
-		goto out_unlock;
-		break;
+		mutex_unlock(&tpci200->mutex);
+		return -EINVAL;
 	}
 
 	iounmap(virt_addr_space->address);
@@ -605,8 +599,7 @@ static int tpci200_slot_unmap_space(struct ipack_device *dev, int space)
 	virt_addr_space->size = 0;
 out_unlock:
 	mutex_unlock(&tpci200->mutex);
-out:
-	return res;
+	return 0;
 }
 
 static int tpci200_slot_unregister(struct ipack_device *dev)
-- 
1.7.10

[PATCH 4/8] Staging: ipack/bridges/tpci200: tpci200_slot_map_space() should return 0 if succeed.

[Samuel Iglesias Gonsalvez]

tpci200_slot_map_space() should return 0 if the operation was properly
done. If not, the caller will think that something wrong happened.

This patch establish the returned value to 0. It is overwritten in case of
error.

Signed-off-by: Samuel Iglesias Gonsalvez <siglesias@igalia.com>
---
 drivers/staging/ipack/bridges/tpci200.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/ipack/bridges/tpci200.c b/drivers/staging/ipack/bridges/tpci200.c
index 85560c7..0c2a50a 100644
--- a/drivers/staging/ipack/bridges/tpci200.c
+++ b/drivers/staging/ipack/bridges/tpci200.c
@@ -628,7 +628,7 @@ static int tpci200_slot_unregister(struct ipack_device *dev)
 static int tpci200_slot_map_space(struct ipack_device *dev,
 				  unsigned int memory_size, int space)
 {
-	int res;
+	int res = 0;
 	unsigned int size_to_map;
 	void __iomem *phys_address;
 	struct ipack_addr_space *virt_addr_space;
-- 
1.7.10

[PATCH 5/8] Staging: ipack/bridges/tpci200: check if the remove function is available

[Samuel Iglesias Gonsalvez]

To avoid a dereference of a NULL pointer, the availability of the function is
checked before its use.

Signed-off-by: Samuel Iglesias Gonsalvez <siglesias@igalia.com>
---
 drivers/staging/ipack/bridges/tpci200.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/staging/ipack/bridges/tpci200.c b/drivers/staging/ipack/bridges/tpci200.c
index 0c2a50a..ee26d9a 100644
--- a/drivers/staging/ipack/bridges/tpci200.c
+++ b/drivers/staging/ipack/bridges/tpci200.c
@@ -764,6 +764,8 @@ out:
 static void tpci200_slot_remove(struct tpci200_slot *slot)
 {
 	if ((slot->dev == NULL) ||
+	    (slot->dev->driver == NULL) ||
+	    (slot->dev->driver->ops == NULL) ||
 	    (slot->dev->driver->ops->remove == NULL))
 		return;
 
-- 
1.7.10

[PATCH 6/8] Staging: ipack/bridges/tpci200: fix indention.

[Samuel Iglesias Gonsalvez]

Signed-off-by: Samuel Iglesias Gonsalvez <siglesias@igalia.com>
---
 drivers/staging/ipack/bridges/tpci200.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/ipack/bridges/tpci200.c b/drivers/staging/ipack/bridges/tpci200.c
index ee26d9a..24070a3 100644
--- a/drivers/staging/ipack/bridges/tpci200.c
+++ b/drivers/staging/ipack/bridges/tpci200.c
@@ -582,7 +582,7 @@ static int tpci200_slot_unmap_space(struct ipack_device *dev, int space)
 		if (dev->mem_space.address == NULL) {
 			pr_info("Slot [%d:%d] MEM space not mapped !\n",
 				dev->bus_nr, dev->slot);
-		goto out_unlock;
+			goto out_unlock;
 		}
 		virt_addr_space = &dev->mem_space;
 		break;
-- 
1.7.10

[PATCH 7/8] Staging: ipack/bridges/tpci200: remove name field from slot_irq

[Samuel Iglesias Gonsalvez]

This field is not needed at all, as the IRQ is registered for the carrier not
for the mezzanine.

Signed-off-by: Samuel Iglesias Gonsalvez <siglesias@igalia.com>
---
 drivers/staging/ipack/bridges/tpci200.c |    1 -
 drivers/staging/ipack/bridges/tpci200.h |    2 --
 2 files changed, 3 deletions(-)

diff --git a/drivers/staging/ipack/bridges/tpci200.c b/drivers/staging/ipack/bridges/tpci200.c
index 24070a3..96bfe5b 100644
--- a/drivers/staging/ipack/bridges/tpci200.c
+++ b/drivers/staging/ipack/bridges/tpci200.c
@@ -750,7 +750,6 @@ static int tpci200_request_irq(struct ipack_device *dev, int vector,
 	slot_irq->vector = vector;
 	slot_irq->handler = handler;
 	slot_irq->arg = arg;
-	slot_irq->name = dev_name(&dev->dev);
 
 	tpci200->slots[dev->slot].irq = slot_irq;
 	res = __tpci200_request_irq(tpci200, dev);
diff --git a/drivers/staging/ipack/bridges/tpci200.h b/drivers/staging/ipack/bridges/tpci200.h
index 0b547ee..55f6776 100644
--- a/drivers/staging/ipack/bridges/tpci200.h
+++ b/drivers/staging/ipack/bridges/tpci200.h
@@ -106,14 +106,12 @@
  * @vector	Vector number
  * @handler	Handler called when IRQ arrives
  * @arg		Handler argument
- * @name	IRQ name
  *
  */
 struct slot_irq {
 	int		vector;
 	int		(*handler)(void *);
 	void		*arg;
-	const char	*name;
 };
 
 /**
-- 
1.7.10

[PATCH 8/8] Staging ipack/bridges/tpci200: removed check of tpci200->slots[dev->slot].dev

[Samuel Iglesias Gonsalvez]

When ipack_device_register() is called, the variable
tpci200->slots[dev->slot].dev has not assigned a value and it gives an error
when the mezzanine driver is reading a register from the board for the match()
function, as all the I/O functions call check_slot().

Signed-off-by: Samuel Iglesias Gonsalvez <siglesias@igalia.com>
---
 drivers/staging/ipack/bridges/tpci200.c |    7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/staging/ipack/bridges/tpci200.c b/drivers/staging/ipack/bridges/tpci200.c
index 96bfe5b..6751625 100644
--- a/drivers/staging/ipack/bridges/tpci200.c
+++ b/drivers/staging/ipack/bridges/tpci200.c
@@ -59,13 +59,6 @@ static struct tpci200_board *check_slot(struct ipack_device *dev)
 		return NULL;
 	}
 
-	BUG_ON(tpci200->slots == NULL);
-	if (tpci200->slots[dev->slot].dev == NULL) {
-		pr_info("Slot [%d:%d] is not registered !\n", dev->bus_nr,
-			dev->slot);
-		return NULL;
-	}
-
 	return tpci200;
 }
 
-- 
1.7.10

Re: [PATCH 3/8] Staging: ipack/bridges/tpci200: tpci200_slot_unmap_space() should return 0 if succeed.

[Dan Carpenter]

On Wed, May 23, 2012 at 03:54:42PM +0200, Samuel Iglesias Gonsalvez wrote:
> tpci200_slot_unmap_space() should return 0 if the operation was properly done. If
> not, the caller will think that something wrong happened.
> 

It's weird and frustrating that gcc didn't warn that we used "res"
without initializing it.  It feels like the goto out_unlock places
should return an error.  But actually the callers don't don't check
the return values so I guess it doesn't matter.

All these patches look good.

regards,
dan carpenter