Various bits of code are mixing making assumptions about the size of
dma_addr_t or resource_size_t, or mixing up pointer and integer types.
All these fixes are based on compiler warnings and so far as I can see
the bugs are practically harmless.
Ben.
Ben Hutchings (8):
IB/cxgb4: Fix formatting of physical address
farsync: Fix confusion about DMA address and buffer offset types
drm: Do not include page offset in argument to virt_to_page()
drm: Pass pointers to virt_to_page()
[SCSI] tgt: Pass pointers to virt_to_page(), not integers
uio: Pass pointers to virt_to_page(), not integers
rds: Pass pointers to virt_to_page(), not integers
[SCSI] pmcraid: Pass pointers to access_ok(), not integers
drivers/gpu/drm/drm_pci.c | 4 ++--
drivers/gpu/drm/drm_vm.c | 2 +-
drivers/infiniband/hw/cxgb4/device.c | 4 ++--
drivers/net/wan/farsync.c | 19 +++++++------------
drivers/scsi/pmcraid.c | 3 ++-
drivers/scsi/scsi_tgt_if.c | 2 +-
drivers/uio/uio.c | 6 ++++--
net/rds/message.c | 2 +-
8 files changed, 20 insertions(+), 22 deletions(-)
--
Ben Hutchings
If at first you don't succeed, you're doing about average.
Physical addresses may be wider than virtual addresses (e.g. on i386
with PAE) and must not be formatted with %p.
Compile-tested only.
Signed-off-by: Ben Hutchings <[email protected]>
---
The resource could also be printed using '%pR' or '%pr', but that makes
a bigger change to the output.
Ben.
drivers/infiniband/hw/cxgb4/device.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/infiniband/hw/cxgb4/device.c b/drivers/infiniband/hw/cxgb4/device.c
index 33d2cc6..4a03385 100644
--- a/drivers/infiniband/hw/cxgb4/device.c
+++ b/drivers/infiniband/hw/cxgb4/device.c
@@ -602,10 +602,10 @@ static int c4iw_rdev_open(struct c4iw_rdev *rdev)
rdev->lldi.vr->qp.size,
rdev->lldi.vr->cq.start,
rdev->lldi.vr->cq.size);
- PDBG("udb len 0x%x udb base %p db_reg %p gts_reg %p qpshift %lu "
+ PDBG("udb len 0x%x udb base %llx db_reg %p gts_reg %p qpshift %lu "
"qpmask 0x%x cqshift %lu cqmask 0x%x\n",
(unsigned)pci_resource_len(rdev->lldi.pdev, 2),
- (void *)(unsigned long)pci_resource_start(rdev->lldi.pdev, 2),
+ (u64)pci_resource_start(rdev->lldi.pdev, 2),
rdev->lldi.db_reg,
rdev->lldi.gts_reg,
rdev->qpshift, rdev->qpmask,
--
Ben Hutchings
If at first you don't succeed, you're doing about average.
Use dma_addr_t for DMA address parameters and u32 for shared memory
offset parameters.
Do not assume that dma_addr_t is the same as unsigned long; it will
not be in PAE configurations. Truncate DMA addresses to 32 bits when
printing them. This is OK because the DMA mask for this device is
32-bit (per default).
Compile-tested only.
Signed-off-by: Ben Hutchings <[email protected]>
---
drivers/net/wan/farsync.c | 19 +++++++------------
1 file changed, 7 insertions(+), 12 deletions(-)
diff --git a/drivers/net/wan/farsync.c b/drivers/net/wan/farsync.c
index 3f0c4f2..4b36676 100644
--- a/drivers/net/wan/farsync.c
+++ b/drivers/net/wan/farsync.c
@@ -886,15 +886,13 @@ fst_rx_dma_complete(struct fst_card_info *card, struct fst_port_info *port,
* Receive a frame through the DMA
*/
static inline void
-fst_rx_dma(struct fst_card_info *card, dma_addr_t skb,
- dma_addr_t mem, int len)
+fst_rx_dma(struct fst_card_info *card, dma_addr_t skb, u32 mem, int len)
{
/*
* This routine will setup the DMA and start it
*/
- dbg(DBG_RX, "In fst_rx_dma %lx %lx %d\n",
- (unsigned long) skb, (unsigned long) mem, len);
+ dbg(DBG_RX, "In fst_rx_dma %x %x %d\n", (u32)skb, mem, len);
if (card->dmarx_in_progress) {
dbg(DBG_ASS, "In fst_rx_dma while dma in progress\n");
}
@@ -915,20 +913,19 @@ fst_rx_dma(struct fst_card_info *card, dma_addr_t skb,
* Send a frame through the DMA
*/
static inline void
-fst_tx_dma(struct fst_card_info *card, unsigned char *skb,
- unsigned char *mem, int len)
+fst_tx_dma(struct fst_card_info *card, dma_addr_t skb, u32 mem, int len)
{
/*
* This routine will setup the DMA and start it.
*/
- dbg(DBG_TX, "In fst_tx_dma %p %p %d\n", skb, mem, len);
+ dbg(DBG_TX, "In fst_tx_dma %x %x %d\n", (u32)skb, mem, len);
if (card->dmatx_in_progress) {
dbg(DBG_ASS, "In fst_tx_dma while dma in progress\n");
}
- outl((unsigned long) skb, card->pci_conf + DMAPADR1); /* Copy from here */
- outl((unsigned long) mem, card->pci_conf + DMALADR1); /* to here */
+ outl(skb, card->pci_conf + DMAPADR1); /* Copy from here */
+ outl(mem, card->pci_conf + DMALADR1); /* to here */
outl(len, card->pci_conf + DMASIZ1); /* for this length */
outl(0x000000004, card->pci_conf + DMADPR1); /* In this direction */
@@ -1405,9 +1402,7 @@ do_bottom_half_tx(struct fst_card_info *card)
card->dma_len_tx = skb->len;
card->dma_txpos = port->txpos;
fst_tx_dma(card,
- (char *) card->
- tx_dma_handle_card,
- (char *)
+ card->tx_dma_handle_card,
BUF_OFFSET(txBuffer[pi]
[port->txpos][0]),
skb->len);
--
Ben Hutchings
If at first you don't succeed, you're doing about average.
By definition, the page offset will not affect the result.
Compile-tested only.
Signed-off-by: Ben Hutchings <[email protected]>
---
drivers/gpu/drm/drm_vm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_vm.c b/drivers/gpu/drm/drm_vm.c
index b5c5af7..8ef6503 100644
--- a/drivers/gpu/drm/drm_vm.c
+++ b/drivers/gpu/drm/drm_vm.c
@@ -301,7 +301,7 @@ static int drm_do_vm_dma_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
offset = (unsigned long)vmf->virtual_address - vma->vm_start; /* vm_[pg]off[set] should be 0 */
page_nr = offset >> PAGE_SHIFT; /* page_nr could just be vmf->pgoff */
- page = virt_to_page((dma->pagelist[page_nr] + (offset & (~PAGE_MASK))));
+ page = virt_to_page(dma->pagelist[page_nr]);
get_page(page);
vmf->page = page;
--
Ben Hutchings
If at first you don't succeed, you're doing about average.
Most architectures define virt_to_page() as a macro that casts its
argument such that an argument of type unsigned long will be accepted
without complaint. However, the proper type is void *, and passing
unsigned long results in a warning on MIPS.
Compile-tested only.
Signed-off-by: Ben Hutchings <[email protected]>
---
drivers/gpu/drm/drm_pci.c | 4 ++--
drivers/gpu/drm/drm_vm.c | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c
index 1f96cee..4c47954 100644
--- a/drivers/gpu/drm/drm_pci.c
+++ b/drivers/gpu/drm/drm_pci.c
@@ -80,7 +80,7 @@ drm_dma_handle_t *drm_pci_alloc(struct drm_device * dev, size_t size, size_t ali
/* Reserve */
for (addr = (unsigned long)dmah->vaddr, sz = size;
sz > 0; addr += PAGE_SIZE, sz -= PAGE_SIZE) {
- SetPageReserved(virt_to_page(addr));
+ SetPageReserved(virt_to_page((void *)addr));
}
return dmah;
@@ -103,7 +103,7 @@ void __drm_pci_free(struct drm_device * dev, drm_dma_handle_t * dmah)
/* Unreserve */
for (addr = (unsigned long)dmah->vaddr, sz = dmah->size;
sz > 0; addr += PAGE_SIZE, sz -= PAGE_SIZE) {
- ClearPageReserved(virt_to_page(addr));
+ ClearPageReserved(virt_to_page((void *)addr));
}
dma_free_coherent(&dev->pdev->dev, dmah->size, dmah->vaddr,
dmah->busaddr);
diff --git a/drivers/gpu/drm/drm_vm.c b/drivers/gpu/drm/drm_vm.c
index 8ef6503..93e95d7 100644
--- a/drivers/gpu/drm/drm_vm.c
+++ b/drivers/gpu/drm/drm_vm.c
@@ -301,7 +301,7 @@ static int drm_do_vm_dma_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
offset = (unsigned long)vmf->virtual_address - vma->vm_start; /* vm_[pg]off[set] should be 0 */
page_nr = offset >> PAGE_SHIFT; /* page_nr could just be vmf->pgoff */
- page = virt_to_page(dma->pagelist[page_nr]);
+ page = virt_to_page((void *)dma->pagelist[page_nr]);
get_page(page);
vmf->page = page;
--
Ben Hutchings
If at first you don't succeed, you're doing about average.
Most architectures define virt_to_page() as a macro that casts its
argument such that an argument of type unsigned long will be accepted
without complaint. However, the proper type is void *, and passing
unsigned long results in a warning on MIPS.
Compile-tested only.
Signed-off-by: Ben Hutchings <[email protected]>
---
drivers/scsi/scsi_tgt_if.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/scsi/scsi_tgt_if.c b/drivers/scsi/scsi_tgt_if.c
index 6209110..7199753 100644
--- a/drivers/scsi/scsi_tgt_if.c
+++ b/drivers/scsi/scsi_tgt_if.c
@@ -286,7 +286,7 @@ static int uspace_ring_map(struct vm_area_struct *vma, unsigned long addr,
int i, err;
for (i = 0; i < TGT_RING_PAGES; i++) {
- struct page *page = virt_to_page(ring->tr_pages[i]);
+ struct page *page = virt_to_page((void *)ring->tr_pages[i]);
err = vm_insert_page(vma, addr, page);
if (err)
return err;
--
Ben Hutchings
If at first you don't succeed, you're doing about average.
Most architectures define virt_to_page() as a macro that casts its
argument such that an argument of type unsigned long will be accepted
without complaint. However, the proper type is void *, and passing
unsigned long results in a warning on MIPS.
Compile-tested only.
Signed-off-by: Ben Hutchings <[email protected]>
---
drivers/uio/uio.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index 11d4e0a..ef30ec9 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -601,6 +601,7 @@ static int uio_vma_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
struct uio_device *idev = vma->vm_private_data;
struct page *page;
unsigned long offset;
+ void *addr;
int mi = uio_find_mem_index(vma);
if (mi < 0)
@@ -612,10 +613,11 @@ static int uio_vma_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
*/
offset = (vmf->pgoff - mi) << PAGE_SHIFT;
+ addr = (void *)(unsigned long)idev->info->mem[mi].addr + offset;
if (idev->info->mem[mi].memtype == UIO_MEM_LOGICAL)
- page = virt_to_page(idev->info->mem[mi].addr + offset);
+ page = virt_to_page(addr);
else
- page = vmalloc_to_page((void *)(unsigned long)idev->info->mem[mi].addr + offset);
+ page = vmalloc_to_page(addr);
get_page(page);
vmf->page = page;
return 0;
--
Ben Hutchings
If at first you don't succeed, you're doing about average.
Most architectures define virt_to_page() as a macro that casts its
argument such that an argument of type unsigned long will be accepted
without complaint. However, the proper type is void *, and passing
unsigned long results in a warning on MIPS.
Compile-tested only.
Signed-off-by: Ben Hutchings <[email protected]>
---
net/rds/message.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/rds/message.c b/net/rds/message.c
index aba232f..1913fc9 100644
--- a/net/rds/message.c
+++ b/net/rds/message.c
@@ -257,7 +257,7 @@ struct rds_message *rds_message_map_pages(unsigned long *page_addrs, unsigned in
for (i = 0; i < rm->data.op_nents; ++i) {
sg_set_page(&rm->data.op_sg[i],
- virt_to_page(page_addrs[i]),
+ virt_to_page((void *)page_addrs[i]),
PAGE_SIZE, 0);
}
--
Ben Hutchings
If at first you don't succeed, you're doing about average.
Most architectures define access_ok() as a macro that casts its
argument such that an argument of type unsigned long will be accepted
without complaint. However, the proper type is void *, and passing
unsigned long results in a warning on sparc64.
Compile-tested only.
Signed-off-by: Ben Hutchings <[email protected]>
---
drivers/scsi/pmcraid.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/scsi/pmcraid.c b/drivers/scsi/pmcraid.c
index 1eb7b028..4e0a2f3 100644
--- a/drivers/scsi/pmcraid.c
+++ b/drivers/scsi/pmcraid.c
@@ -3794,7 +3794,8 @@ static long pmcraid_ioctl_passthrough(
}
if (request_size > 0) {
- rc = access_ok(access, arg, request_offset + request_size);
+ rc = access_ok(access, (void *)arg,
+ request_offset + request_size);
if (!rc) {
rc = -EFAULT;
--
Ben Hutchings
If at first you don't succeed, you're doing about average.
On Sun, 2013-10-27 at 21:50 +0000, Ben Hutchings wrote:
> Physical addresses may be wider than virtual addresses (e.g. on i386
> with PAE) and must not be formatted with %p.
%pa works. %pa also prefixes with 0x.
> diff --git a/drivers/infiniband/hw/cxgb4/device.c b/drivers/infiniband/hw/cxgb4/device.c
[]
> @@ -602,10 +602,10 @@ static int c4iw_rdev_open(struct c4iw_rdev *rdev)
> rdev->lldi.vr->qp.size,
> rdev->lldi.vr->cq.start,
> rdev->lldi.vr->cq.size);
> - PDBG("udb len 0x%x udb base %p db_reg %p gts_reg %p qpshift %lu "
> + PDBG("udb len 0x%x udb base %llx db_reg %p gts_reg %p qpshift %lu "
> "qpmask 0x%x cqshift %lu cqmask 0x%x\n",
> (unsigned)pci_resource_len(rdev->lldi.pdev, 2),
> - (void *)(unsigned long)pci_resource_start(rdev->lldi.pdev, 2),
> + (u64)pci_resource_start(rdev->lldi.pdev, 2),
> rdev->lldi.db_reg,
> rdev->lldi.gts_reg,
> rdev->qpshift, rdev->qpmask,
>
>
On Sun, 2013-10-27 at 14:58 -0700, Joe Perches wrote:
> On Sun, 2013-10-27 at 21:50 +0000, Ben Hutchings wrote:
> > Physical addresses may be wider than virtual addresses (e.g. on i386
> > with PAE) and must not be formatted with %p.
>
> %pa works. %pa also prefixes with 0x.
Only as long as pci_resource_start() happens to be an lvalue. I'd
rather not introduce that assumption.
Ben.
> > diff --git a/drivers/infiniband/hw/cxgb4/device.c b/drivers/infiniband/hw/cxgb4/device.c
> []
> > @@ -602,10 +602,10 @@ static int c4iw_rdev_open(struct c4iw_rdev *rdev)
> > rdev->lldi.vr->qp.size,
> > rdev->lldi.vr->cq.start,
> > rdev->lldi.vr->cq.size);
> > - PDBG("udb len 0x%x udb base %p db_reg %p gts_reg %p qpshift %lu "
> > + PDBG("udb len 0x%x udb base %llx db_reg %p gts_reg %p qpshift %lu "
> > "qpmask 0x%x cqshift %lu cqmask 0x%x\n",
> > (unsigned)pci_resource_len(rdev->lldi.pdev, 2),
> > - (void *)(unsigned long)pci_resource_start(rdev->lldi.pdev, 2),
> > + (u64)pci_resource_start(rdev->lldi.pdev, 2),
> > rdev->lldi.db_reg,
> > rdev->lldi.gts_reg,
> > rdev->qpshift, rdev->qpmask,
> >
> >
>
>
>
--
Ben Hutchings
If at first you don't succeed, you're doing about average.
On Sun, 2013-10-27 at 22:02 +0000, Ben Hutchings wrote:
> On Sun, 2013-10-27 at 14:58 -0700, Joe Perches wrote:
> > On Sun, 2013-10-27 at 21:50 +0000, Ben Hutchings wrote:
> > > Physical addresses may be wider than virtual addresses (e.g. on i386
> > > with PAE) and must not be formatted with %p.
> >
> > %pa works. %pa also prefixes with 0x.
>
> Only as long as pci_resource_start() happens to be an lvalue. I'd
> rather not introduce that assumption.
pci_resource_start returns a resource_size_t which is a phys_addr_t.
Changing that from an lvalue would cause a _lot_ of breakage.
On Sun, 2013-10-27 at 15:14 -0700, Joe Perches wrote:
> On Sun, 2013-10-27 at 22:02 +0000, Ben Hutchings wrote:
> > On Sun, 2013-10-27 at 14:58 -0700, Joe Perches wrote:
> > > On Sun, 2013-10-27 at 21:50 +0000, Ben Hutchings wrote:
> > > > Physical addresses may be wider than virtual addresses (e.g. on i386
> > > > with PAE) and must not be formatted with %p.
> > >
> > > %pa works. %pa also prefixes with 0x.
> >
> > Only as long as pci_resource_start() happens to be an lvalue. I'd
> > rather not introduce that assumption.
>
> pci_resource_start returns a resource_size_t which is a phys_addr_t.
>
> Changing that from an lvalue would cause a _lot_ of breakage.
I don't think so. This doesn't find anything:
git grep '&[ (]*pci_resource_start'
and I was able to build drivers/{net,pci,scsi}/ successfully with
pci_resource_start() changed to an inline function.
Ben.
--
Ben Hutchings
If at first you don't succeed, you're doing about average.
On Sun, 2013-10-27 at 22:26 +0000, Ben Hutchings wrote:
> I don't think so. This doesn't find anything:
> git grep '&[ (]*pci_resource_start'
> and I was able to build drivers/{net,pci,scsi}/ successfully with
> pci_resource_start() changed to an inline function.
Hi again Ben. You're right.
It could be nice though to have some mechanism to get from
a pci_resource_start/end that could be emitted via %pa
without an intermediate or casting like
(unsigned long long)pci_resource_<foo>(struct resource *)
There are at least a few dozen uses of dmesg/seq_printf with
%llx or %lx and (some_cast)pci_resource_<foo> that could be
converted.
From: Ben Hutchings <[email protected]>
Date: Sun, 27 Oct 2013 21:51:44 +0000
> - dbg(DBG_TX, "In fst_tx_dma %p %p %d\n", skb, mem, len);
> + dbg(DBG_TX, "In fst_tx_dma %x %x %d\n", (u32)skb, mem, len);
Please use %p for the skb pointer instead of casting it (which btw
will introduce a warning on 64-bit).
From: Ben Hutchings <[email protected]>
Date: Sun, 27 Oct 2013 21:54:16 +0000
> Most architectures define virt_to_page() as a macro that casts its
> argument such that an argument of type unsigned long will be accepted
> without complaint. However, the proper type is void *, and passing
> unsigned long results in a warning on MIPS.
>
> Compile-tested only.
>
> Signed-off-by: Ben Hutchings <[email protected]>
This looks fine:
Acked-by: David S. Miller <[email protected]>
On Mon, 2013-10-28 at 00:26 -0400, David Miller wrote:
> From: Ben Hutchings <[email protected]>
> Date: Sun, 27 Oct 2013 21:51:44 +0000
>
> > - dbg(DBG_TX, "In fst_tx_dma %p %p %d\n", skb, mem, len);
> > + dbg(DBG_TX, "In fst_tx_dma %x %x %d\n", (u32)skb, mem, len);
>
> Please use %p for the skb pointer instead of casting it (which btw
> will introduce a warning on 64-bit).
skb is the DMA address of the data in the sk_buff. Yes, this is really
unusual naming.
Ben.
--
Ben Hutchings
[W]e found...that it wasn't as easy to get programs right as we had thought.
... I realized that a large part of my life from then on was going to be spent
in finding mistakes in my own programs. - Maurice Wilkes, 1949
From: Ben Hutchings <[email protected]>
Date: Mon, 28 Oct 2013 04:51:25 +0000
> On Mon, 2013-10-28 at 00:26 -0400, David Miller wrote:
>> From: Ben Hutchings <[email protected]>
>> Date: Sun, 27 Oct 2013 21:51:44 +0000
>>
>> > - dbg(DBG_TX, "In fst_tx_dma %p %p %d\n", skb, mem, len);
>> > + dbg(DBG_TX, "In fst_tx_dma %x %x %d\n", (u32)skb, mem, len);
>>
>> Please use %p for the skb pointer instead of casting it (which btw
>> will introduce a warning on 64-bit).
>
> skb is the DMA address of the data in the sk_buff. Yes, this is really
> unusual naming.
Hmmm, Ok then I guess. :-/
-----Original Message-----
From: David Miller [mailto:[email protected]]
Sent: Sunday, October 27, 2013 11:27 PM
To: [email protected]
Cc: Venkat Venkatsubra; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH 7/8] rds: Pass pointers to virt_to_page(), not integers
From: Ben Hutchings <[email protected]>
Date: Sun, 27 Oct 2013 21:54:16 +0000
> Most architectures define virt_to_page() as a macro that casts its
> argument such that an argument of type unsigned long will be accepted
> without complaint. However, the proper type is void *, and passing
> unsigned long results in a warning on MIPS.
>
> Compile-tested only.
>
> Signed-off-by: Ben Hutchings <[email protected]>
This looks fine:
Acked-by: David S. Miller <[email protected]>
Acked-by: Venkat Venkatsubra <[email protected]>