[PATCH 3/5] drivers/infiniband: correct size computation

[PATCH 3/5] drivers/infiniband: correct size computation

[Julia Lawall]

From: Julia Lawall <julia@diku.dk>

The size argument to ioremap_nocache should be the size of desired
information, not the pointer to it.

The semantic patch that makes this change is as follows:
(http://coccinelle.lip6.fr/)

// <smpl>
@expression@
expression *x;
@@

x =
 <+...
-sizeof(x)
+sizeof(*x)
...+>// </smpl>

Signed-off-by: Julia Lawall <julia@diku.dk>

---
 drivers/infiniband/hw/nes/nes.c     |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/infiniband/hw/nes/nes.c b/drivers/infiniband/hw/nes/nes.c
index cbde0cf..b8c5372 100644
--- a/drivers/infiniband/hw/nes/nes.c
+++ b/drivers/infiniband/hw/nes/nes.c
@@ -521,7 +521,8 @@ static int __devinit nes_probe(struct pci_dev *pcidev, const struct pci_device_i
 	spin_lock_init(&nesdev->indexed_regs_lock);
 
 	/* Remap the PCI registers in adapter BAR0 to kernel VA space */
-	mmio_regs = ioremap_nocache(pci_resource_start(pcidev, BAR_0), sizeof(mmio_regs));
+	mmio_regs = ioremap_nocache(pci_resource_start(pcidev, BAR_0),
+				    sizeof(*mmio_regs));
 	if (mmio_regs == NULL) {
 		printk(KERN_ERR PFX "Unable to remap BAR0\n");
 		ret = -EIO;

RE: [PATCH 3/5] drivers/infiniband: correct size computation

[Tung, Chien Tin]

Thanks for pointing out the bug.  

> 	/* Remap the PCI registers in adapter BAR0 to kernel VA space */
>-	mmio_regs = ioremap_nocache(pci_resource_start(pcidev, BAR_0), sizeof(mmio_regs));
>+	mmio_regs = ioremap_nocache(pci_resource_start(pcidev, BAR_0),
>+				    sizeof(*mmio_regs));
> 	if (mmio_regs == NULL) {
> 		printk(KERN_ERR PFX "Unable to remap BAR0\n");
> 		ret = -EIO;

mmio_regs is initialized to NULL at the top of the function so *mmio_regs wouldn't be
a good idea.  Instead of sizeof(*mmio_regs) use pci_resource_len(pcidev, BAR_0).  If
you can recreate the patch with this change I will ack it.

Thanks,


Chien

RE: [PATCH 3/5] drivers/infiniband: correct size computation

[Julia Lawall]

On Mon, 7 Dec 2009, Tung, Chien Tin wrote:

> Thanks for pointing out the bug.  
> 
> > 	/* Remap the PCI registers in adapter BAR0 to kernel VA space */
> >-	mmio_regs = ioremap_nocache(pci_resource_start(pcidev, BAR_0), sizeof(mmio_regs));
> >+	mmio_regs = ioremap_nocache(pci_resource_start(pcidev, BAR_0),
> >+				    sizeof(*mmio_regs));
> > 	if (mmio_regs == NULL) {
> > 		printk(KERN_ERR PFX "Unable to remap BAR0\n");
> > 		ret = -EIO;
> 
> mmio_regs is initialized to NULL at the top of the function so 
> *mmio_regs wouldn't be a good idea.  Instead of sizeof(*mmio_regs) use 
> pci_resource_len(pcidev, BAR_0).  If you can recreate the patch with 
> this change I will ack it.

When you say that it isn't a good idea, do you mean that the result is 
wrong, or that it looks odd?  I didn't think sizeof looked at the value, 
but only at the type?

julia

RE: [PATCH 3/5] drivers/infiniband: correct size computation

[Tung, Chien Tin]

>> > 	/* Remap the PCI registers in adapter BAR0 to kernel VA space */
>> >-	mmio_regs = ioremap_nocache(pci_resource_start(pcidev, BAR_0), sizeof(mmio_regs));
>> >+	mmio_regs = ioremap_nocache(pci_resource_start(pcidev, BAR_0),
>> >+				    sizeof(*mmio_regs));
>> > 	if (mmio_regs == NULL) {
>> > 		printk(KERN_ERR PFX "Unable to remap BAR0\n");
>> > 		ret = -EIO;
>>
>> mmio_regs is initialized to NULL at the top of the function so
>> *mmio_regs wouldn't be a good idea.  Instead of sizeof(*mmio_regs) use
>> pci_resource_len(pcidev, BAR_0).  If you can recreate the patch with
>> this change I will ack it.
>
>When you say that it isn't a good idea, do you mean that the result is
>wrong, or that it looks odd?  I didn't think sizeof looked at the value,
>but only at the type?


I misspoke on the problem.  Mmio_regs is declared as a void pointer:

        void __iomem *mmio_regs = NULL;

Thus pci_resource_len(pcidev, BAR_0) is the correct fix.

Chien

RE: [PATCH 3/5] drivers/infiniband: correct size computation

[Julia Lawall]

On Mon, 7 Dec 2009, Tung, Chien Tin wrote:

> >> > 	/* Remap the PCI registers in adapter BAR0 to kernel VA space */
> >> >-	mmio_regs = ioremap_nocache(pci_resource_start(pcidev, BAR_0), sizeof(mmio_regs));
> >> >+	mmio_regs = ioremap_nocache(pci_resource_start(pcidev, BAR_0),
> >> >+				    sizeof(*mmio_regs));
> >> > 	if (mmio_regs == NULL) {
> >> > 		printk(KERN_ERR PFX "Unable to remap BAR0\n");
> >> > 		ret = -EIO;
> >>
> >> mmio_regs is initialized to NULL at the top of the function so
> >> *mmio_regs wouldn't be a good idea.  Instead of sizeof(*mmio_regs) use
> >> pci_resource_len(pcidev, BAR_0).  If you can recreate the patch with
> >> this change I will ack it.
> >
> >When you say that it isn't a good idea, do you mean that the result is
> >wrong, or that it looks odd?  I didn't think sizeof looked at the value,
> >but only at the type?
> 
> 
> I misspoke on the problem.  Mmio_regs is declared as a void pointer:
> 
>         void __iomem *mmio_regs = NULL;
> 
> Thus pci_resource_len(pcidev, BAR_0) is the correct fix.

OK, thanks for the clarification.  I will make another patch later today.

julia

RE: [PATCH 3/5] drivers/infiniband: correct size computation

[Julia Lawall]

From: Julia Lawall <julia@diku.dk>

The size argument to ioremap_nocache should be the size of desired
information, not the pointer to it.

The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)

// <smpl>
@expression@
expression *x;
@@

x =
 <+...
*sizeof(x)
...+>// </smpl>

Signed-off-by: Julia Lawall <julia@diku.dk>

---
 drivers/infiniband/hw/nes/nes.c     |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/infiniband/hw/nes/nes.c b/drivers/infiniband/hw/nes/nes.c
index cbde0cf..95db98f 100644
--- a/drivers/infiniband/hw/nes/nes.c
+++ b/drivers/infiniband/hw/nes/nes.c
@@ -521,7 +521,8 @@ static int __devinit nes_probe(struct pci_dev *pcidev, const struct pci_device_i
 	spin_lock_init(&nesdev->indexed_regs_lock);
 
 	/* Remap the PCI registers in adapter BAR0 to kernel VA space */
-	mmio_regs = ioremap_nocache(pci_resource_start(pcidev, BAR_0), sizeof(mmio_regs));
+	mmio_regs = ioremap_nocache(pci_resource_start(pcidev, BAR_0),
+				    pci_resource_len(pcidev, BAR_0));
 	if (mmio_regs == NULL) {
 		printk(KERN_ERR PFX "Unable to remap BAR0\n");
 		ret = -EIO;

RE: [PATCH 3/5] drivers/infiniband: correct size computation

[Tung, Chien Tin]

>From: Julia Lawall <julia@diku.dk>
>
>The size argument to ioremap_nocache should be the size of desired
>information, not the pointer to it.
>
>The semantic match that finds this problem is as follows:
>(http://coccinelle.lip6.fr/)
>
>// <smpl>
>@expression@
>expression *x;
>@@
>
>x =
> <+...
>*sizeof(x)
>...+>// </smpl>
>
>Signed-off-by: Julia Lawall <julia@diku.dk>
>
>---
> drivers/infiniband/hw/nes/nes.c     |    3 ++-
> 1 files changed, 2 insertions(+), 1 deletions(-)
>
>diff --git a/drivers/infiniband/hw/nes/nes.c b/drivers/infiniband/hw/nes/nes.c
>index cbde0cf..95db98f 100644
>--- a/drivers/infiniband/hw/nes/nes.c
>+++ b/drivers/infiniband/hw/nes/nes.c
>@@ -521,7 +521,8 @@ static int __devinit nes_probe(struct pci_dev *pcidev, const struct pci_device_i
> 	spin_lock_init(&nesdev->indexed_regs_lock);
>
> 	/* Remap the PCI registers in adapter BAR0 to kernel VA space */
>-	mmio_regs = ioremap_nocache(pci_resource_start(pcidev, BAR_0), sizeof(mmio_regs));
>+	mmio_regs = ioremap_nocache(pci_resource_start(pcidev, BAR_0),
>+				    pci_resource_len(pcidev, BAR_0));
> 	if (mmio_regs == NULL) {
> 		printk(KERN_ERR PFX "Unable to remap BAR0\n");
> 		ret = -EIO;

Acked-by: Chien Tung <chien.tin.tung@intel.com>

Re: [PATCH 3/5] drivers/infiniband: correct size computation

[Roland Dreier]

applied, thanks everyone.