Re: [PATCHv5] staging: vme_user: provide DMA functionality

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sun, Oct 18, 2015 at 10:31 AM, Martyn Welch <martyn@xxxxxxxxxxxx> wrote:
>
>
> On 11/10/15 01:13, Dmitry Kalinkin wrote:
>>
>> This introduces a new dma device that provides a single ioctl call that
>> provides DMA read and write functionality to the user space.
>>
>> Signed-off-by: Dmitry Kalinkin <dmitry.kalinkin@xxxxxxxxx>
>> Cc: Igor Alekseev <igor.alekseev@xxxxxxx>
>> ---
>>
>> In the last reply Martyn suggested a rework of this to make it use existing
>> bus/vme/ctl instead of creating a new bus/vme/dma%i device and also
>> dynamically
>> allocate a dma resource in each request.
>>
>> I think this doesn't need those adjustments. I think that dynamic allocation
>> doesn't solve any practical problem that isn't caused by current kernel
>> api.
>
>
> That would depend on what resources had already been allocated to other
> drivers using the kernel api and what resources the underlying bridge had to
> make available. This driver will currently only load if all the resources it
> wishes to expose to user space are available. That said, such a modification
> is clearly separate from adding DMA support to user space, so the argument
> is rather academic.
Other drives meaning vme_pio, I don't see any others. All this time
we are discussing how many GE PIO boards one can plug into a crate
with or without vme_user. Most of people have zero of them.
Also, VME DMA API has no users in kernel, we are just adding one now.
>
>>    I also think that separate device is a good feature because it allows
>> for
>> discovery of dma capatibility from userspace.
>
>
> Given the current user space api, that's true.
>
>> The interface with separate
>> chardev also allows to provide DMA read() and write() syscalls that can
>> come handy in pair with /bin/dd.
>
>
> But this patch doesn't implement such a feature does it?
Actually, initial (never published) version of this patch exposed
read(),write(),
and an ioctl to set the access cycle. It was working, but with subtlety for
A64 addressing. I come across some problems when using large offsets
that would not fit in signed long long. I was using FMODE_UNSIGNED_OFFSET
to fix the kernel side of things, but it seemed like userspace didn't like
the "negative" offsets. I've looked a bit at glibc sources and decided
to give up.
Now that I remember this, my original argument is kind of busted.
>
> (Generally happy with this for now, however couple of comments below.)
>
>
>>
>> v5:
>> Added a validation for dma_op argument in vme_user_sg_to_dma_list(). It is
>> already checked in caller but we would like to silence a warning:
>>
>>     drivers/staging/vme/devices/vme_user.c: In function
>> 'vme_user_ioctl.isra.4':
>>>>
>>>> drivers/staging/vme/devices/vme_user.c:324:7: warning: 'dest' may be
>>>> used uninitialized in this function [-Wmaybe-uninitialized]
>>
>>        ret = vme_dma_list_add(dma_list, src, dest, hw_len);
>>            ^
>>     drivers/staging/vme/devices/vme_user.c:296:52: note: 'dest' was
>> declared here
>>        struct vme_dma_attr *pci_attr, *vme_attr, *src, *dest;
>>                                                         ^
>>>>
>>>> drivers/staging/vme/devices/vme_user.c:324:7: warning: 'src' may be used
>>>> uninitialized in this function [-Wmaybe-uninitialized]
>>
>>        ret = vme_dma_list_add(dma_list, src, dest, hw_len);
>>            ^
>>     drivers/staging/vme/devices/vme_user.c:296:46: note: 'src' was
>> declared here
>>        struct vme_dma_attr *pci_attr, *vme_attr, *src, *dest;
>>
>> ---
>>   drivers/staging/vme/devices/vme_user.c | 205
>> ++++++++++++++++++++++++++++++++-
>>   drivers/staging/vme/devices/vme_user.h |  11 ++
>>   2 files changed, 213 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/staging/vme/devices/vme_user.c
>> b/drivers/staging/vme/devices/vme_user.c
>> index 8e61a3b..2434e5f 100644
>> --- a/drivers/staging/vme/devices/vme_user.c
>> +++ b/drivers/staging/vme/devices/vme_user.c
>> @@ -79,15 +79,18 @@ static unsigned int bus_num;
>>    * We shall support 4 masters and 4 slaves with this driver.
>>    */
>
>
> The comment just above here (cropped in the patch) describes the interface
> that this driver exposes and what is documented in
> Documentation/devices.txt.
I've come across a long time ago and at the time I realized that this
document is generally outdated and is not required to be updated.
First, "Last revised: 6th April 2009"
Second, the device path information is long obsolete in the light of udev.
Third, they want submissions on a separate list <device@xxxxxxxxxx>
Fourth, "20 block Hitachi CD-ROM (under development) 0 = /dev/hitcd"
-- this is not for real.
>
> I think this comment either needs updating to reflect the changes introduced
> in this patch, or deleted.
>
> (As an aside:
>
> The interface in Docmentation/devices.txt is an interesting oddity - it
> existed before any VME drivers were present in the kernel. Given that the
> driver at vmelinux.org hasn't been updated since some time in the 2.4 kernel
> series and the lack of mainlined drivers other than this one using that
> interface, should we update that file to reflect the additions?
>
> If we were to modify this driver sufficiently, so that chrdevs were
> dynamically allocated for example, should we delete that entry?
> )
Had same thoughts. I've concluded that it's not worth (see above).
>
>
>>   #define VME_MAJOR     221     /* VME Major Device Number */
>> -#define VME_DEVS       9       /* Number of dev entries */
>> +#define VME_DEVS       10      /* Number of dev entries */
>>     #define MASTER_MINOR        0
>>   #define MASTER_MAX    3
>>   #define SLAVE_MINOR   4
>>   #define SLAVE_MAX     7
>>   #define CONTROL_MINOR 8
>> +#define DMA_MINOR      9
>>   -#define PCI_BUF_SIZE  0x20000        /* Size of one slave image buffer
>> */
>> +#define PCI_BUF_SIZE   0x20000         /* Size of one slave image buffer
>> */
>> +
>> +#define VME_MAX_DMA_LEN        0x4000000       /* Maximal DMA transfer
>> length */
>>     /*
>>    * Structure to handle image related parameters.
>> @@ -112,7 +115,7 @@ static const int type[VME_DEVS] = { MASTER_MINOR,
>> MASTER_MINOR,
>>                                         MASTER_MINOR,   MASTER_MINOR,
>>                                         SLAVE_MINOR,    SLAVE_MINOR,
>>                                         SLAVE_MINOR,    SLAVE_MINOR,
>> -                                       CONTROL_MINOR
>> +                                       CONTROL_MINOR,  DMA_MINOR
>>                                 };
>>     struct vme_user_vma_priv {
>> @@ -281,6 +284,172 @@ static loff_t vme_user_llseek(struct file *file,
>> loff_t off, int whence)
>>         return -EINVAL;
>>   }
>>   +static int vme_user_sg_to_dma_list(const struct vme_dma_op *dma_op,
>> +                                  struct sg_table *sgt,
>> +                                  int sg_count, struct vme_dma_list
>> *dma_list)
>> +{
>> +       ssize_t pos = 0;
>> +       struct scatterlist *sg;
>> +       int i, ret;
>> +
>> +       if ((dma_op->dir != VME_DMA_MEM_TO_VME) &&
>> +           (dma_op->dir != VME_DMA_VME_TO_MEM))
>> +               return -EINVAL;
>> +
>
>
> Would this not be better implemented as a "default" case in the switch
> below?
I preferred to exit early to not any cleanup.
>
>
>> +       for_each_sg(sgt->sgl, sg, sg_count, i) {
>> +               struct vme_dma_attr *pci_attr, *vme_attr, *src, *dest;
>> +               dma_addr_t hw_address = sg_dma_address(sg);
>> +               unsigned int hw_len = sg_dma_len(sg);
>> +
>> +               vme_attr = vme_dma_vme_attribute(dma_op->vme_addr + pos,
>> +                                                dma_op->aspace,
>> +                                                dma_op->cycle,
>> +                                                dma_op->dwidth);
>> +               if (!vme_attr)
>> +                       return -ENOMEM;
>> +
>> +               pci_attr = vme_dma_pci_attribute(hw_address);
>> +               if (!pci_attr) {
>> +                       vme_dma_free_attribute(vme_attr);
>> +                       return -ENOMEM;
>> +               }
>> +
>> +               switch (dma_op->dir) {
>> +               case VME_DMA_MEM_TO_VME:
>> +                       src = pci_attr;
>> +                       dest = vme_attr;
>> +                       break;
>> +               case VME_DMA_VME_TO_MEM:
>> +                       src = vme_attr;
>> +                       dest = pci_attr;
>> +                       break;
>> +               }
>> +
>> +               ret = vme_dma_list_add(dma_list, src, dest, hw_len);
>> +
>> +               /*
>> +                * XXX VME API doesn't mention whether we should keep
>> +                * attributes around
>> +                */
>> +               vme_dma_free_attribute(vme_attr);
>> +               vme_dma_free_attribute(pci_attr);
>> +
>> +               if (ret)
>> +                       return ret;
>> +
>> +               pos += hw_len;
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +static enum dma_data_direction vme_dir_to_dma_dir(unsigned vme_dir)
>> +{
>> +       switch (vme_dir) {
>> +       case VME_DMA_VME_TO_MEM:
>> +               return DMA_FROM_DEVICE;
>> +       case VME_DMA_MEM_TO_VME:
>> +               return DMA_TO_DEVICE;
>> +       }
>> +
>> +       return DMA_NONE;
>> +}
>> +
>> +static ssize_t vme_user_dma_ioctl(unsigned int minor,
>> +                                 const struct vme_dma_op *dma_op)
>> +{
>> +       unsigned int offset = offset_in_page(dma_op->buf_vaddr);
>> +       unsigned long nr_pages;
>> +       enum dma_data_direction dir;
>> +       struct vme_dma_list *dma_list;
>> +       struct sg_table *sgt = NULL;
>> +       struct page **pages = NULL;
>> +       long got_pages;
>> +       ssize_t count;
>> +       int retval, sg_count;
>> +
>> +       /* Prevent WARN from dma_map_sg */
>> +       if (dma_op->count == 0)
>> +               return 0;
>> +
>> +       /*
>> +        * This is a voluntary limit to prevent huge allocation for pages
>> +        * array. VME_MAX_DMA_LEN is not a fundamental VME constraint.
>> +        */
>> +       count = min_t(size_t, dma_op->count, VME_MAX_DMA_LEN);
>> +       nr_pages = (offset + count + PAGE_SIZE - 1) >> PAGE_SHIFT;
>> +
>> +       dir = vme_dir_to_dma_dir(dma_op->dir);
>> +       if (dir == DMA_NONE)
>> +               return -EINVAL;
>> +
>> +       pages = kmalloc_array(nr_pages, sizeof(pages[0]), GFP_KERNEL);
>> +       if (!pages) {
>> +               retval = -ENOMEM;
>> +               goto free;
>> +       }
>> +
>> +       sgt = kzalloc(sizeof(*sgt), GFP_KERNEL);
>> +       if (!sgt) {
>> +               retval = -ENOMEM;
>> +               goto free;
>> +       }
>> +
>> +       dma_list = vme_new_dma_list(image[minor].resource);
>> +       if (!dma_list) {
>> +               retval = -ENOMEM;
>> +               goto free;
>> +       }
>> +
>> +       got_pages = get_user_pages_fast(dma_op->buf_vaddr, nr_pages,
>> +                                       dir == DMA_FROM_DEVICE, pages);
>> +       if (got_pages != nr_pages) {
>> +               pr_debug("Not all pages were pinned\n");
>> +               retval = (got_pages < 0) ? got_pages : -EFAULT;
>> +               goto release_pages;
>> +       }
>> +
>> +       retval = sg_alloc_table_from_pages(sgt, pages, nr_pages,
>> +                                          offset, count, GFP_KERNEL);
>> +       if (retval)
>> +               goto release_pages;
>> +
>> +       sg_count = dma_map_sg(vme_user_bridge->dev.parent,
>> +                             sgt->sgl, sgt->nents, dir);
>> +       if (!sg_count) {
>> +               pr_debug("DMA mapping error\n");
>> +               retval = -EFAULT;
>> +               goto free_sgt;
>> +       }
>> +
>> +       retval = vme_user_sg_to_dma_list(dma_op, sgt, sg_count, dma_list);
>> +       if (retval)
>> +               goto dma_unmap;
>> +
>> +       retval = vme_dma_list_exec(dma_list);
>> +
>> +dma_unmap:
>> +       dma_unmap_sg(vme_user_bridge->dev.parent, sgt->sgl, sgt->nents,
>> dir);
>> +
>> +free_sgt:
>> +       sg_free_table(sgt);
>> +
>> +release_pages:
>> +       if (got_pages > 0)
>> +               release_pages(pages, got_pages, 0);
>> +
>> +       vme_dma_list_free(dma_list);
>> +
>> +free:
>> +       kfree(sgt);
>> +       kfree(pages);
>> +
>> +       if (retval)
>> +               return retval;
>> +
>> +       return count;
>> +}
>> +
>>   /*
>>    * The ioctls provided by the old VME access method (the one at
>> vmelinux.org)
>>    * are most certainly wrong as the effectively push the registers layout
>> @@ -297,6 +466,7 @@ static int vme_user_ioctl(struct inode *inode, struct
>> file *file,
>>         struct vme_master master;
>>         struct vme_slave slave;
>>         struct vme_irq_id irq_req;
>> +       struct vme_dma_op dma_op;
>>         unsigned long copied;
>>         unsigned int minor = MINOR(inode->i_rdev);
>>         int retval;
>> @@ -406,6 +576,19 @@ static int vme_user_ioctl(struct inode *inode, struct
>> file *file,
>>                         break;
>>                 }
>>                 break;
>> +       case DMA_MINOR:
>> +               switch (cmd) {
>> +               case VME_DO_DMA:
>> +                       copied = copy_from_user(&dma_op, argp,
>> +                                               sizeof(dma_op));
>> +                       if (copied != 0) {
>> +                               pr_warn("Partial copy from userspace\n");
>> +                               return -EFAULT;
>> +                       }
>> +
>> +                       return vme_user_dma_ioctl(minor, &dma_op);
>> +               }
>> +               break;
>>         }
>>         return -EINVAL;
>> @@ -615,6 +798,15 @@ static int vme_user_probe(struct vme_dev *vdev)
>>                 }
>>         }
>>   +     image[DMA_MINOR].resource = vme_dma_request(vme_user_bridge,
>> +               VME_DMA_VME_TO_MEM | VME_DMA_MEM_TO_VME);
>> +       if (!image[DMA_MINOR].resource) {
>> +               dev_warn(&vdev->dev,
>> +                        "Unable to allocate dma resource\n");
>> +               err = -ENOMEM;
>> +               goto err_master;
>> +       }
>> +
>>         /* Create sysfs entries - on udev systems this creates the dev
>> files */
>>         vme_user_sysfs_class = class_create(THIS_MODULE, driver_name);
>>         if (IS_ERR(vme_user_sysfs_class)) {
>> @@ -637,6 +829,9 @@ static int vme_user_probe(struct vme_dev *vdev)
>>                 case SLAVE_MINOR:
>>                         name = "bus/vme/s%d";
>>                         break;
>> +               case DMA_MINOR:
>> +                       name = "bus/vme/dma0";
>> +                       break;
>>                 default:
>>                         err = -EINVAL;
>>                         goto err_sysfs;
>> @@ -661,6 +856,8 @@ err_sysfs:
>>         }
>>         class_destroy(vme_user_sysfs_class);
>>   +     vme_dma_free(image[DMA_MINOR].resource);
>> +
>>         /* Ensure counter set correcty to unalloc all master windows */
>>         i = MASTER_MAX + 1;
>>   err_master:
>> @@ -701,6 +898,8 @@ static int vme_user_remove(struct vme_dev *dev)
>>         }
>>         class_destroy(vme_user_sysfs_class);
>>   +     vme_dma_free(image[DMA_MINOR].resource);
>> +
>>         for (i = MASTER_MINOR; i < (MASTER_MAX + 1); i++) {
>>                 kfree(image[i].kern_buf);
>>                 vme_master_free(image[i].resource);
>> diff --git a/drivers/staging/vme/devices/vme_user.h
>> b/drivers/staging/vme/devices/vme_user.h
>> index b8cc7bc..252b1c9 100644
>> --- a/drivers/staging/vme/devices/vme_user.h
>> +++ b/drivers/staging/vme/devices/vme_user.h
>> @@ -48,11 +48,22 @@ struct vme_irq_id {
>>         __u8 statid;
>>   };
>>   +struct vme_dma_op {
>> +       __u64 vme_addr;         /* Starting Address on the VMEbus */
>> +       __u64 buf_vaddr;        /* Pointer to userspace memory */
>> +       __u32 count;            /* Count of bytes to copy */
>> +       __u32 aspace;           /* Address Space */
>> +       __u32 cycle;            /* Cycle properties */
>> +       __u32 dwidth;           /* Data transfer width */
>> +       __u32 dir;              /* Transfer Direction */
>> +};
>> +
>>   #define VME_GET_SLAVE _IOR(VME_IOC_MAGIC, 1, struct vme_slave)
>>   #define VME_SET_SLAVE _IOW(VME_IOC_MAGIC, 2, struct vme_slave)
>>   #define VME_GET_MASTER _IOR(VME_IOC_MAGIC, 3, struct vme_master)
>>   #define VME_SET_MASTER _IOW(VME_IOC_MAGIC, 4, struct vme_master)
>>   #define VME_IRQ_GEN _IOW(VME_IOC_MAGIC, 5, struct vme_irq_id)
>> +#define VME_DO_DMA _IOW(VME_IOC_MAGIC, 7, struct vme_dma_op)
>
>
> Why not 6?
I wanted to reserve 6 for "VME_IRQ_WAIT" kind of operation.
But, I guess, that is never coming around anyway.
>
>>     #endif /* _VME_USER_H_ */
>>
>
>
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel



[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux