| Message ID | 1431622863-28575-3-git-send-email-julien.grall@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 14/05/15 18:00, Julien Grall wrote: > virt_to_mfn should take a void* rather an unsigned long. While it > doesn't really matter now, it would throw a compiler warning later when > virt_to_mfn will enforce the type. > > At the same time, avoid to compute new virtual address every time in the > loop and directly increment the parameter as we don't use it later. Reviewed-by: David Vrabel <david.vrabel@citrix.com> But... > --- a/drivers/xen/xenbus/xenbus_client.c > +++ b/drivers/xen/xenbus/xenbus_client.c > @@ -379,16 +379,16 @@ int xenbus_grant_ring(struct xenbus_device *dev, void *vaddr, > int i, j; > > for (i = 0; i < nr_pages; i++) { > - unsigned long addr = (unsigned long)vaddr + > - (PAGE_SIZE * i); > err = gnttab_grant_foreign_access(dev->otherend_id, > - virt_to_mfn(addr), 0); > + virt_to_mfn(vaddr), 0); > if (err < 0) { > xenbus_dev_fatal(dev, err, > "granting access to ring page"); > goto fail; > } > grefs[i] = err; > + > + vaddr = (char *)vaddr + PAGE_SIZE; You don't need the cast here since vaddr is a void *. David
Hi David, On 19/05/15 14:51, David Vrabel wrote: >> --- a/drivers/xen/xenbus/xenbus_client.c >> +++ b/drivers/xen/xenbus/xenbus_client.c >> @@ -379,16 +379,16 @@ int xenbus_grant_ring(struct xenbus_device *dev, void *vaddr, >> int i, j; >> >> for (i = 0; i < nr_pages; i++) { >> - unsigned long addr = (unsigned long)vaddr + >> - (PAGE_SIZE * i); >> err = gnttab_grant_foreign_access(dev->otherend_id, >> - virt_to_mfn(addr), 0); >> + virt_to_mfn(vaddr), 0); >> if (err < 0) { >> xenbus_dev_fatal(dev, err, >> "granting access to ring page"); >> goto fail; >> } >> grefs[i] = err; >> + >> + vaddr = (char *)vaddr + PAGE_SIZE; > > You don't need the cast here since vaddr is a void *. Arithmetic on void pointer is a GCC extension [1]. I wasn't sure what is the Linux policy on it. Regards, [1] https://gcc.gnu.org/onlinedocs/gcc/Pointer-Arith.html#Pointer-Arith
diff --git a/drivers/xen/xenbus/xenbus_client.c b/drivers/xen/xenbus/xenbus_client.c index a014016..d204562 100644 --- a/drivers/xen/xenbus/xenbus_client.c +++ b/drivers/xen/xenbus/xenbus_client.c @@ -379,16 +379,16 @@ int xenbus_grant_ring(struct xenbus_device *dev, void *vaddr, int i, j; for (i = 0; i < nr_pages; i++) { - unsigned long addr = (unsigned long)vaddr + - (PAGE_SIZE * i); err = gnttab_grant_foreign_access(dev->otherend_id, - virt_to_mfn(addr), 0); + virt_to_mfn(vaddr), 0); if (err < 0) { xenbus_dev_fatal(dev, err, "granting access to ring page"); goto fail; } grefs[i] = err; + + vaddr = (char *)vaddr + PAGE_SIZE; } return 0;
virt_to_mfn should take a void* rather an unsigned long. While it doesn't really matter now, it would throw a compiler warning later when virt_to_mfn will enforce the type. At the same time, avoid to compute new virtual address every time in the loop and directly increment the parameter as we don't use it later. Signed-off-by: Julien Grall <julien.grall@citrix.com> Cc: Wei Liu <wei.liu2@citrix.com> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com> Cc: David Vrabel <david.vrabel@citrix.com> --- drivers/xen/xenbus/xenbus_client.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)